# Phase 65.1: Phase 65 Code Review Remediation — Pattern Map

**Mapped:** 2026-05-25
**Files analyzed:** 28 (15 modified C++/Rust + 13 new tests/changelog)
**Analogs found:** 28 / 28

## File Classification

### Modified Files

| File | Role | Data Flow | Closest Analog | Match Quality |
|------|------|-----------|----------------|---------------|
| `cpp/src/shim.cpp` (CR-01 site, `sv_create_from_yaml_bind` ~line 732-816) | C++ bind callback | request-response (FFI) | `cpp/src/shim.cpp:927-1014` (`sv_list_semantic_views_bind`) — BORROW + error_buf + SvOwnedBuffer pattern | role-match |
| `cpp/src/shim.cpp` (CR-02 site, 4 exec callbacks at 825-856, 1023-1059, 1132-1157, varchar_bool sibling) | C++ exec callback | streaming (single-shot emit) | `cpp/src/shim.cpp:1132-1157` (`sv_emit_varchar_rows`) — the cleanest of the four; target shape is "no fallback branch" | exact (after refactor) |
| `cpp/src/shim.cpp` (WR-02 ABI, `sv_register_table_function` 562-626, `sv_register_scalar_function` 639-689) | C ABI registration helper | request-response | `cpp/src/shim.cpp:57-61` (`sv_parser_override_rust` extern declaration) — `(char *error_buf, size_t error_buf_len)` trailing pattern | exact (template applies) |
| `cpp/src/shim.cpp` (WR-06 layout guard, top of file + inside `sv_register_parser_hooks` 2465-2544) | C++ build-time + load-time guard | one-shot probe | `cpp/src/shim.cpp:2469-2471` (existing `wrapper->database->instance` access in `sv_register_parser_hooks`) | exact (same access path) |
| `cpp/src/shim.cpp` (WR-07 hard-error, `sv_resolve_output_logical_types` 2203-2255) | C++ binder | request-response (error surface) | `cpp/src/shim.cpp:761-763` (existing `throw BinderException("FROM YAML FILE failed: " + result->GetError())`) | exact (same throw shape) |
| `cpp/src/shim.cpp` (WR-09 idempotent register, `sv_register_parser_hooks` 2480-2502) | C++ parser-extension registration | one-shot init-time | No direct analog — pattern is new. Closest: `cbmgr.ParserExtensions()` iterator usage from amalgamation `cpp/include/duckdb.cpp:281157` (read-only template) | partial |
| `cpp/src/shim.cpp` (D-24 helper TF arg trim, `sv_create_from_yaml_bind` 726-797 + register block 2518-2537) | C++ bind callback + registration | request-response | own pre-change shape (3-arg sibling pattern at shim.cpp:2519-2524) | exact |
| `cpp/src/shim.hpp` (WR-02 ABI signature update at 68-75 + 95-101) | C ABI header | declaration-only | `cpp/src/shim.hpp:23-35` (existing extern declarations carrying `error_buf, error_buf_len`) | exact |
| `src/lib.rs::init_extension` (WR-02 callers, 17 sites at 450-575) | Rust FFI caller | request-response | `src/lib.rs:450-451` (`sv_register_parser_hooks` call shape) but needs error_buf allocation — see `src/parse.rs` ctx_ptr_call pattern | role-match |
| `src/ddl/read_ffi.rs` (WR-05 add `BorrowedConnection`) | Rust FFI helper / newtype | type-level guard | Existing `probe_catalog_table_present(conn)` at lines 57-74 (same module, same FFI shape) | exact (sibling addition) |
| `src/ddl/list.rs` (WR-05 dispatcher + IN-06 dedup, line 81-184 + 191-220) | Rust FFI dispatcher | request-response | itself — IN-06 dedup means consolidating to `crate::ddl::read_ffi::*` | exact |
| `src/ddl/describe.rs:18`, `src/ddl/show_columns.rs:21`, `src/ddl/show_dims.rs:25,148`, `src/ddl/show_dims_for_metric.rs:21`, `src/ddl/show_metrics.rs:16,139`, `src/ddl/show_facts.rs:16,139`, `src/ddl/show_materializations.rs:18,130`, `src/ddl/get_ddl.rs:37`, `src/ddl/read_yaml.rs:42`, `src/ddl/list.rs:265`, `src/query/explain.rs:98`, `src/query/table_function.rs:101` (16 remaining WR-05 sites) | Rust FFI dispatchers | request-response | `src/ddl/list.rs:81-184` (canonical Wave 0 spike) — wrap-on-entry pattern | exact (mechanical) |
| `src/query/table_function.rs::try_infer_schema` (WR-08, lines 704-729) | Rust schema-probe helper | transform | `src/ddl/read_ffi.rs::probe_catalog_table_present` (defensive probe shape, but here we PROMOTE Result) | role-match |
| `src/query/table_function.rs` callers (lines 294, 349) of `try_infer_schema` (WR-08) | Rust dispatcher error plumb | request-response | `src/query/table_function.rs:120-132` existing `write_err` calls | exact |
| `src/parse.rs::existence_guard_select` (WR-03, 2119-2126) + paired `rename_collision_guard_select` 2134-2141 | Rust SQL generator | one-shot | itself — extension is local; pattern derives from inner CASE | exact (additive) |
| `src/ddl/alter_helpers_ffi.rs::sv_compute_create_from_yaml_rust` (D-24 IN-04, 120-132) | Rust FFI handler | transform | `src/ddl/list.rs::sv_list_semantic_views_bind_rust` (panic-catch + error_buf pattern) | exact (param removal) |
| `src/type_cache.rs` + `src/lib.rs` mod decl (D-23 IN-02 deletion) | Module deletion | n/a | n/a (deletion-only) | n/a |
| `src/query/table_function.rs::type_id_to_display_name` (D-23 IN-03 deletion, 500-568) | Function deletion | n/a | n/a (deletion-only) | n/a |
| `src/parse.rs::rewrite_yaml_file_create` (D-24 — emit 3-arg TF call) | Rust SQL generator | request-response | the same function's existing pre-change call site | exact (mechanical) |
| `tests/no_long_lived_conn.rs` (D-11 optional extension to walk all `src/` + match `duckdb_disconnect`) | Rust AST guard | one-shot validation | itself (lines 60-99 Finder struct + `Visit` impl) | exact (additive) |
| `CHANGELOG.md` (D-27 — `## [Unreleased]` Security/Changed/Fixed/Removed entries) | Docs | n/a | Existing `## [Unreleased]` section in repo root | exact |

### New Files (Wave 0 gaps)

| File | Role | Data Flow | Closest Analog | Match Quality |
|------|------|-----------|----------------|---------------|
| `test/integration/test_concurrent_writes_per_call_conn.py` (D-19 WR-04) | Python integration test | threading | `test/integration/test_concurrent_reads_per_call_conn.py` (verbatim shape, swap read→write SQL) | exact |
| `test/integration/test_drop_on_fresh_readonly_clear_error.py` (D-18 WR-03) | Python integration test | request-response | `test/integration/test_concurrent_reads_per_call_conn.py` (PEP-723 + open_master fixture) | role-match |
| `test/integration/test_load_extension_twice_idempotent.py` (D-21 WR-09) | Python integration test | request-response | same — PEP-723 + `LOAD semantic_views` twice + assert single hook (via `SELECT * FROM duckdb_extensions()` shape) | role-match |
| `test/sql/phase651_yaml_filesystem_access_gating.test` (CR-01 — enable_external_access=false) | sqllogictest | request-response | `test/sql/65_metadata_via_sql.test` (similar phase-prefixed test using `statement error ----` block form) | role-match |
| `test/sql/phase651_null_name_inputs.test` (D-25 IN-05) | sqllogictest | request-response | `test/sql/65_metadata_via_sql.test` (existing scalar null-handling tests) | role-match |
| `test/sql/phase651_bind_probe_hard_errors.test` (D-16 — WR-07/WR-08 user-visible errors) | sqllogictest | request-response | `test/sql/65_pk_error.test` (existing `statement error` block-form pattern) | exact |
| `tests/registration_error_surfaces.rs` (Wave 0 — WR-02 force-fail registration) | Rust integration test | request-response | `tests/no_long_lived_conn.rs` (top-level `tests/*.rs` integration test layout) | role-match |
| Doc-test `compile_fail` block inside `BorrowedConnection` impl (Wave 0 — WR-05 negative) | Rust doc-test | compile-time | No exact analog in repo; pattern is `/// ```compile_fail` Rustdoc convention | partial |

## Pattern Assignments

---

### `cpp/src/shim.cpp` — CR-01 FileSystem-direct YAML read (`sv_create_from_yaml_bind`)

**Analog:** `cpp/src/shim.cpp:927-1014` (`sv_list_semantic_views_bind` — per-call Connection + error_buf + SvOwnedBuffer ownership pattern) + DuckDB amalgamation `cpp/include/duckdb.hpp:8205-8217`.

**Existing problematic block** (lines 731-775 — to be replaced):
```cpp
std::string path_escaped = bd->file_path;
{
    size_t pos = 0;
    while ((pos = path_escaped.find('\'', pos)) != std::string::npos) {
        path_escaped.replace(pos, 1, "''");
        pos += 2;
    }
}
Connection probe(*context.db);
std::string read_sql =
    "SELECT content FROM read_text('" + path_escaped + "')";
auto result = probe.Query(read_sql);
if (result->HasError()) { ... }
```

**Target shape (FileSystem API)** — pattern from amalgamation public header:
```cpp
auto &fs = FileSystem::GetFileSystem(context);
unique_ptr<FileHandle> handle;
try {
    handle = fs.OpenFile(bd->file_path, FileFlags::FILE_FLAGS_READ);
} catch (const std::exception &e) {
    throw BinderException("FROM YAML FILE failed: " + std::string(e.what()));
}
int64_t size = fs.GetFileSize(*handle);
std::string yaml_content(static_cast<size_t>(size), '\0');
int64_t got = fs.Read(*handle, yaml_content.data(), size);
if (got != size) {
    throw BinderException("FROM YAML FILE failed: short read ...");
}
// yaml_content now owns bytes; FileHandle drops at end of scope.
```

**Error-wording pattern** (lines 758-763 — keep verbatim prefix):
```cpp
throw BinderException("FROM YAML FILE failed: " + result->GetError());
```
The `FROM YAML FILE failed:` prefix is pinned by `test/sql/phase53_yaml_file.test`. New error paths MUST keep this prefix.

---

### `cpp/src/shim.cpp` — CR-02 single-shot emit (`sv_emit_varchar_rows` is the cleanest, the 4 callbacks converge on its shape)

**Analog:** `cpp/src/shim.cpp:1132-1157` (current — with the misleading `(void)emitted` to delete).

**Target shape** (delete null-state fallback, delete `(void)emitted`):
```cpp
static void sv_emit_varchar_rows(
    ClientContext & /*context*/,
    TableFunctionInput &data_p,
    DataChunk &output) {
    auto &bd = data_p.bind_data->Cast<SvVarcharBindData>();
    auto *state_p = data_p.local_state.get();
    if (state_p == nullptr) {
        // Registration guarantees init_local was wired (sv_register_table_function
        // refuses null init_cb at registration time per Phase 65.1 D-05). Reaching
        // this branch indicates a corrupted TableFunctionInput; fail loud rather
        // than risk an unbounded row stream.
        throw InternalException(
            "sv_emit_varchar_rows: local_state missing despite init_local registration");
    }
    auto &state = state_p->Cast<SvVarcharLocalState>();
    if (state.emitted) {
        output.SetCardinality(0);
        return;
    }
    idx_t n = bd.rows.size();
    for (idx_t i = 0; i < n; ++i) {
        const auto &row = bd.rows[i];
        for (size_t c = 0; c < row.size(); ++c) {
            output.SetValue(c, i, Value(row[c]));
        }
    }
    output.SetCardinality(n);
    state.emitted = true;
}
```

**Apply same converged shape to the 3 sibling callbacks:**
- `sv_create_from_yaml_function` (cpp/src/shim.cpp:825-856) — delete lines 837-847 fallback
- `sv_list_semantic_views_function` (cpp/src/shim.cpp:1023-1059) — delete lines 1029-1042 fallback
- `sv_emit_varchar_bool_rows` (sibling of varchar_rows, parallel structure) — same deletion

---

### `cpp/src/shim.cpp` — WR-02 error_buf ABI rework (`sv_register_table_function` + `sv_register_scalar_function`)

**Analog:** `cpp/src/shim.cpp:57-61` (`sv_parser_override_rust` extern declaration — canonical `(char *error_buf, size_t error_buf_len)` trailing-pair convention).

**Existing `sv_register_table_function` signature** (lines 562-570):
```cpp
bool sv_register_table_function(
    duckdb_database db_handle,
    const char *name,
    const LogicalType *arg_types,
    size_t arg_count,
    table_function_bind_t bind_cb,
    table_function_t exec_cb,
    table_function_init_local_t init_cb);
```

**Target signature** (add trailing error_buf pair):
```cpp
bool sv_register_table_function(
    duckdb_database db_handle,
    const char *name,
    const LogicalType *arg_types,
    size_t arg_count,
    table_function_bind_t bind_cb,
    table_function_t exec_cb,
    table_function_init_local_t init_cb,
    char *error_buf, size_t error_buf_len);  // ← new
```

**Existing catch block** (lines 615-625 — stderr-only):
```cpp
} catch (const std::exception &e) {
    fprintf(stderr,
        "sv_register_table_function('%s') failed: %s\n",
        name ? name : "(null)", e.what());
    return false;
}
```

**Target catch block** (write to buf + stderr both for debug; the buf is the only ABI-stable channel per D-09):
```cpp
} catch (const std::exception &e) {
    snprintf(error_buf, error_buf_len,
        "sv_register_table_function('%s') failed: %s",
        name ? name : "(null)", e.what());
    return false;
}
```

**D-05 enforcement** (null-init refusal — replaces lines 572-577):
```cpp
if (db_handle == nullptr || name == nullptr ||
    bind_cb == nullptr || exec_cb == nullptr || init_cb == nullptr) {
    snprintf(error_buf, error_buf_len,
        "sv_register_table_function('%s'): null required argument "
        "(init_cb is mandatory)",
        name ? name : "(null)");
    return false;
}
```

**Note on `sv_register_scalar_function`:** apply only the error_buf rework. Scalar functions have no `init_local` concept; the null-init refusal does NOT apply here (researcher Open Question 3 — answered).

---

### `cpp/src/shim.cpp` — WR-06 layout guard + load-time probe

**Analog (compile-time):** none in-repo — `static_assert` is new. Place near the top of `shim.cpp` extern block.

**Compile-time guard** (file-scope near line 50):
```cpp
static_assert(sizeof(duckdb_connection) == sizeof(void*),
    "duckdb_connection must be pointer-sized — bridge contract broken; "
    "see cpp/src/shim.cpp BORROW contract docs and 65-REVIEW.md WR-06");
```

**Analog (runtime probe):** `cpp/src/shim.cpp:2469-2471` (existing wrapper-access path used by `sv_register_parser_hooks`).

**Existing wrapper unwrap** (line 2469-2471):
```cpp
auto *wrapper = reinterpret_cast<duckdb::DatabaseWrapper *>(
    db_handle->internal_ptr);
auto &db = *wrapper->database->instance;
```

**Target runtime probe** (insert at TOP of `sv_register_parser_hooks` after wrapper unwrap, AFTER the WR-02 error_buf parameter is added to the function's signature):
```cpp
{
    Connection probe(db);
    auto handle = reinterpret_cast<duckdb_connection>(&probe);
    duckdb_result r;
    auto rc = duckdb_query(handle, "SELECT 1", &r);
    if (rc != DuckDBSuccess) {
        snprintf(error_buf, error_buf_len,
            "bridge contract broken — duckdb_connection layout assumption failed "
            "at load time. DuckDB ABI may have changed; refusing to load "
            "semantic_views extension.");
        // Note: probe dtor runs on scope exit; handle becomes invalid but is not
        // touched again.
        return false;
    }
    duckdb_destroy_result(&r);
}
```

---

### `cpp/src/shim.cpp` — WR-07 hard-error promotion (`sv_resolve_output_logical_types`)

**Analog:** `cpp/src/shim.cpp:761-763` (canonical `throw BinderException(prefix + ...)` shape used 4× elsewhere in shim.cpp).

**Existing fallback** (lines 2236-2242 — to delete):
```cpp
if (probe_result->HasError() || probe_result->types.size() != cols.size()) {
    // Fall back to type_id-only declaration if the probe fails.
    for (const auto &c : cols) {
        out.emplace_back(sv_logical_type_from_c_type_id(c.type_id));
    }
    return out;
}
```

**Target shape** (split into two distinct error messages):
```cpp
if (probe_result->HasError()) {
    throw BinderException(
        "semantic_view: failed to infer column types via LIMIT 0 probe: " +
        probe_result->GetError());
}
if (probe_result->types.size() != cols.size()) {
    throw BinderException(
        "semantic_view: LIMIT 0 probe returned " +
        std::to_string(probe_result->types.size()) +
        " columns, expected " + std::to_string(cols.size()));
}
```

---

### `cpp/src/shim.cpp` — WR-09 parser-hook idempotence (`sv_register_parser_hooks`)

**Analog:** no in-repo precedent. Pattern derives from amalgamation's read-only iterator at `cpp/include/duckdb.cpp:281157`.

**Existing registration** (lines 2480-2501):
```cpp
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);
auto &config = DBConfig::GetConfig(db);
ParserExtension::Register(config, ext);
```

**Target shape (idempotent register)** — wrap entire "build + Register" block to avoid leaking SemanticViewsParserInfo:
```cpp
auto &config = DBConfig::GetConfig(db);
auto &cbmgr = config.GetCallbackManager();

// Idempotence check: if our parser_override is already in the list, skip
// registration. ParserExtension::Register always appends — without this
// dedup, repeat LOAD semantic_views in the same process accumulates hooks
// in DBConfig::parser_extensions. Comparing function pointers is portable
// ISO C++ because sv_parser_override is a file-static C++ function (same
// TU on both sides of ==). See WR-09 Pitfall 3 for the documented
// theoretical race with concurrent LOAD calls.
bool already_registered = false;
for (auto &existing : cbmgr.ParserExtensions()) {
    if (existing.parser_override == sv_parser_override) {
        already_registered = true;
        break;
    }
}

if (!already_registered) {
    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"));
}
// Helper-TF registrations below use ALTER_ON_CONFLICT so they're naturally
// idempotent — fall through.
```

---

### `src/ddl/read_ffi.rs` — WR-05 `BorrowedConnection` newtype (new sibling to `probe_catalog_table_present`)

**Analog (same module):** `src/ddl/read_ffi.rs:57-74` (existing `probe_catalog_table_present` — same FFI shape, same `unsafe` doc-comment shape).

**Existing helper doc-comment style** (lines 47-56):
```rust
/// Probe whether `semantic_layer._definitions` exists on the given borrowed
/// connection. ...
///
/// # Safety
///
/// `conn` must be a valid `duckdb_connection`. The handle is borrowed and
/// must outlive this call — the typical caller is a bind dispatcher running
/// inside a C++ bind callback that owns a stack `Connection probe(*context.db)`.
pub unsafe fn probe_catalog_table_present(conn: ffi::duckdb_connection) -> bool {
```

**Target newtype** (add after existing helpers, with `compile_fail` doc-test for D-10 negative coverage):
```rust
/// Borrowed `duckdb_connection` handle. The bridge contract: this handle is
/// owned by a stack `Connection probe(*context.db)` constructed in a C++
/// bind/exec callback. The Rust side MUST NOT call `duckdb_disconnect` —
/// doing so would `delete` a stack object (UB).
///
/// This newtype enforces the contract at compile time: `ffi::duckdb_disconnect`
/// accepts `ffi::duckdb_connection`, not `BorrowedConnection`, so the call
/// simply does not type-check. Defence in depth alongside the AST-walk guard
/// at `tests/no_long_lived_conn.rs`.
///
/// # Negative compile coverage
///
/// ```compile_fail
/// use duckdb_semantic_views::ddl::read_ffi::BorrowedConnection;
/// use libduckdb_sys as ffi;
/// let raw: ffi::duckdb_connection = std::ptr::null_mut();
/// let borrowed = unsafe { BorrowedConnection::new(raw) };
/// unsafe { ffi::duckdb_disconnect(&mut borrowed) };  // ← must fail to compile
/// ```
#[repr(transparent)]
pub struct BorrowedConnection(ffi::duckdb_connection);

impl BorrowedConnection {
    /// Wrap a raw FFI handle as a borrowed connection.
    ///
    /// # Safety
    /// The caller must guarantee that `conn` outlives this `BorrowedConnection`
    /// and that no other code path will call `duckdb_disconnect(conn)` while
    /// this borrow is live.
    #[must_use]
    pub unsafe fn new(conn: ffi::duckdb_connection) -> Self {
        Self(conn)
    }

    /// Access the underlying raw handle for FFI calls (`duckdb_query`,
    /// `duckdb_prepare`, etc.). The caller MUST NOT pass the returned handle
    /// to `duckdb_disconnect`.
    #[must_use]
    pub fn as_raw(&self) -> ffi::duckdb_connection {
        self.0
    }

    #[must_use]
    pub fn is_null(&self) -> bool {
        self.0.is_null()
    }
}
// Deliberately NO Drop, NO Clone, NO trait impls that could lead to disconnect.
```

---

### Rust dispatcher migration (17 sites) — WR-05 mechanical wrap-on-entry

**Analog (canonical Wave 0 spike):** `src/ddl/list.rs:81-184` (`sv_list_semantic_views_bind_rust`).

**Current pattern** (list.rs:81-103 — to migrate):
```rust
pub unsafe extern "C" fn sv_list_semantic_views_bind_rust(
    conn: libduckdb_sys::duckdb_connection,
    out_ptr: *mut *mut u8,
    out_len: *mut usize,
    error_buf: *mut u8,
    error_buf_len: usize,
) -> u8 {
    use std::panic::AssertUnwindSafe;
    let result = std::panic::catch_unwind(AssertUnwindSafe(|| {
        if conn.is_null() {
            write_err(error_buf, error_buf_len, "duckdb_connection is null");
            return 1_u8;
        }
        let table_present = probe_catalog_table_present(conn);
        let reader = CatalogReader::new(conn, table_present);
```

**Target pattern** (FFI signature unchanged; wrap-on-entry):
```rust
pub unsafe extern "C" fn sv_list_semantic_views_bind_rust(
    conn: libduckdb_sys::duckdb_connection,  // FFI ABI unchanged
    out_ptr: *mut *mut u8,
    out_len: *mut usize,
    error_buf: *mut u8,
    error_buf_len: usize,
) -> u8 {
    use crate::ddl::read_ffi::BorrowedConnection;
    use std::panic::AssertUnwindSafe;
    let result = std::panic::catch_unwind(AssertUnwindSafe(|| {
        // Shadow the raw param with a typed BorrowedConnection. Anywhere
        // downstream that needs the raw handle goes through .as_raw(); the
        // borrow itself cannot be passed to ffi::duckdb_disconnect.
        let borrowed = BorrowedConnection::new(conn);
        if borrowed.is_null() {
            write_err(error_buf, error_buf_len, "duckdb_connection is null");
            return 1_u8;
        }
        let table_present = probe_catalog_table_present(&borrowed);
        let reader = CatalogReader::new(&borrowed, table_present);
```

**Pitfall reminder (researcher Pitfall 1):** the raw `conn` parameter is still in scope. Naming convention `conn` → `borrowed` (immediate shadow) is the discipline; the AST-walk guard at `tests/no_long_lived_conn.rs` (extended per D-11) catches any direct `ffi::duckdb_disconnect(conn)` regression.

**Helpers that need migration alongside dispatchers:**
- `probe_catalog_table_present` (signature: `&BorrowedConnection` — uses `.as_raw()` internally for `ffi::duckdb_query`)
- `CatalogReader::new` (same)
- `collect_explain_lines` (`src/query/explain.rs:340`)
- `try_infer_schema` (`src/query/table_function.rs:438, 705`) — combined with WR-08 fix

---

### `src/query/table_function.rs::try_infer_schema` — WR-08 Result promotion

**Analog (existing FFI error plumb):** `src/query/table_function.rs:120-132` (existing `write_err` call pattern after a Result fails).

**Current signature** (lines 704-708):
```rust
pub(crate) unsafe fn try_infer_schema(
    conn: ffi::duckdb_connection,
    sql: &str,
) -> Option<(Vec<String>, Vec<ffi::duckdb_type>)> {
    let mut result = execute_sql_raw(conn, sql).ok()?;
    ...
}
```

**Target signature** (return Result with error text):
```rust
pub(crate) unsafe fn try_infer_schema(
    borrowed: &BorrowedConnection,
    sql: &str,
) -> Result<(Vec<String>, Vec<ffi::duckdb_type>), String> {
    let mut result = execute_sql_raw(borrowed.as_raw(), sql)
        .map_err(|e| e.to_string())?;
    // ... existing body returns Ok((names, types)) ...
}
```

**Caller migration** (analog: existing `write_err`-then-`return 1_u8` shape at table_function.rs:120-132):
```rust
match try_infer_schema(&borrowed, &limit0_sql) {
    Ok((names, types)) => { /* existing happy path */ }
    Err(msg) => {
        write_err(error_buf, error_buf_len,
            &format!("semantic_view: type inference failed for query `{limit0_sql}`: {msg}"));
        return 1_u8;
    }
}
```

The downstream C++ `sv_semantic_view_bind` already re-raises `rc=1` as `BinderException(std::string("semantic_view: ") + error_buf)` (cpp/src/shim.cpp:2296-2298) — no new C++ plumbing needed.

---

### `src/parse.rs::existence_guard_select` — WR-03 outer-CASE wrap

**Analog:** the existing inner CASE block (lines 2120-2125 itself) — the fix adds a guarding outer CASE composing with the inner.

**Current shape** (lines 2119-2126):
```rust
fn existence_guard_select(name_escaped: &str) -> String {
    format!(
        "SELECT CASE WHEN NOT EXISTS \
                   (SELECT 1 FROM semantic_layer._definitions WHERE name = '{name_escaped}') \
                THEN error('semantic view ''{name_escaped}'' does not exist') \
                ELSE TRUE END"
    )
}
```

**Target shape** (outer guard for "table never bootstrapped" — short-circuits before the inner query touches `_definitions`):
```rust
fn existence_guard_select(name_escaped: &str) -> String {
    // CASE expressions short-circuit in evaluation order, so the first WHEN
    // protects the second from `Catalog Error: Table _definitions does not
    // exist` on a read-only DB that was never bootstrapped. Same "does not
    // exist" wording for both branches — user sees the canonical message
    // pinned by phase20/phase34_1/phase45/65_alter_* sqllogictests.
    format!(
        "SELECT CASE \
              WHEN NOT EXISTS (SELECT 1 FROM information_schema.tables \
                                WHERE table_schema = 'semantic_layer' \
                                  AND table_name = '_definitions') \
                THEN error('semantic view ''{name_escaped}'' does not exist') \
              WHEN NOT EXISTS (SELECT 1 FROM semantic_layer._definitions \
                                WHERE name = '{name_escaped}') \
                THEN error('semantic view ''{name_escaped}'' does not exist') \
              ELSE TRUE \
            END"
    )
}
```

**Apply same wrap to `rename_collision_guard_select`** (lines 2134-2141) for symmetry — researcher flagged it as paired; not strictly needed for reachability but improves hygiene.

---

### `tests/no_long_lived_conn.rs` — D-11 optional extension to walk `duckdb_disconnect`

**Analog (template):** `tests/no_long_lived_conn.rs:60-99` itself — `Finder` struct + `Visit` impl.

**Existing Finder shape** (lines 65-99 — match `duckdb_connect`):
```rust
struct Finder {
    in_init_extension: bool,
    found_call_sites: Vec<String>,
}

impl<'ast> Visit<'ast> for Finder {
    fn visit_item_fn(&mut self, f: &'ast ItemFn) {
        let was = self.in_init_extension;
        if f.sig.ident == "init_extension" {
            self.in_init_extension = true;
        }
        syn::visit::visit_item_fn(self, f);
        self.in_init_extension = was;
    }

    fn visit_expr_call(&mut self, c: &'ast ExprCall) {
        if self.in_init_extension {
            if let syn::Expr::Path(p) = &*c.func {
                if let Some(last) = p.path.segments.last() {
                    if last.ident == "duckdb_connect" {
                        ...
```

**Target extension** (add a sibling `DisconnectFinder` that walks ALL of `src/` — not scoped to `init_extension`, since `duckdb_disconnect` anywhere is the contract violation):
```rust
/// Walks all of src/ looking for any call expression whose last path
/// segment is the identifier `duckdb_disconnect`. The BorrowedConnection
/// newtype (Phase 65.1 D-10) makes such a call fail to type-check via
/// the type system; this AST guard catches the case where a contributor
/// would call duckdb_disconnect on a raw ffi::duckdb_connection (which
/// still type-checks). Defence in depth.
struct DisconnectFinder {
    found_call_sites: Vec<(String, String)>, // (file_path, full_path)
}

impl<'ast> Visit<'ast> for DisconnectFinder {
    fn visit_expr_call(&mut self, c: &'ast ExprCall) {
        if let syn::Expr::Path(p) = &*c.func {
            if let Some(last) = p.path.segments.last() {
                if last.ident == "duckdb_disconnect" {
                    let segs: Vec<String> = p.path.segments.iter()
                        .map(|s| s.ident.to_string()).collect();
                    self.found_call_sites.push(
                        (String::new(), segs.join("::"))
                    );
                }
            }
        }
        syn::visit::visit_expr_call(self, c);
    }
}

#[test]
fn no_duckdb_disconnect_anywhere_in_src() {
    let mut all_findings = Vec::new();
    for entry in walkdir::WalkDir::new("src/") {
        let entry = entry.expect("walkdir");
        if entry.path().extension().and_then(|s| s.to_str()) != Some("rs") {
            continue;
        }
        let src = std::fs::read_to_string(entry.path())
            .expect("read source file");
        let Ok(file) = syn::parse_str::<syn::File>(&src) else {
            continue; // Skip unparseable files (shouldn't happen in src/)
        };
        let mut f = DisconnectFinder { found_call_sites: Vec::new() };
        f.visit_file(&file);
        for (_, path) in f.found_call_sites {
            all_findings.push(format!("{}: {}", entry.path().display(), path));
        }
    }
    assert!(all_findings.is_empty(),
        "duckdb_disconnect call sites found in src/: {:?}. \
         BorrowedConnection newtype (D-10) prevents this via the type \
         system; raw ffi::duckdb_connection should never be disconnected.",
        all_findings);
}
```

Note: `walkdir` may need to be added to `[dev-dependencies]` if not already present.

---

### `test/integration/test_concurrent_writes_per_call_conn.py` — WR-04 (new file)

**Analog:** `test/integration/test_concurrent_reads_per_call_conn.py` (verbatim shape).

**PEP-723 header pattern** (lines 1-5):
```python
#!/usr/bin/env python3
# /// script
# dependencies = ["duckdb==1.5.2"]
# requires-python = ">=3.10"
# ///
```

**Fixture pattern** (lines 54-66):
```python
def open_master(db_path: str):
    import duckdb
    conn = duckdb.connect(
        db_path,
        config={
            "allow_unsigned_extensions": "true",
            "extension_directory": EXT_DIR,
        },
    )
    conn.execute(f"FORCE INSTALL '{EXT_PATH}'")
    conn.execute("LOAD semantic_views")
    return conn
```

**Worker + gate pattern** (lines 89-110 — start-gate for simultaneous start):
```python
def worker(thread_conn, result: WorkerResult, gate: threading.Event) -> None:
    gate.wait()
    try:
        for _ in range(CALLS_PER_THREAD):
            rows = thread_conn.execute(
                "SELECT name, table_name FROM show_semantic_dimensions('v1') ORDER BY name"
            ).fetchall()
            result.rows_seen.append(tuple(tuple(r) for r in rows))
        result.ok = True
    except Exception as exc:
        result.error = exc
```

**Adapt for write workload** (per D-19 + D-20):
- Replace read SQL with a mix of `CREATE SEMANTIC VIEW ... IF NOT EXISTS`, `DROP SEMANTIC VIEW ... IF EXISTS`, `ALTER SEMANTIC VIEW ... SET COMMENT '...'` targeting **overlapping** view names (e.g. each thread cycles through `v_{0..3}` to maximise contention).
- Per D-20: tolerate PK violation errors as a legitimate outcome category for `CREATE IF NOT EXISTS` (TECH-DEBT 23). Categorise each outcome as `success | already_exists | constraint_violation | unknown_error`; assert no `unknown_error`.
- Final-state check: `list_semantic_views()` reflects union of `success`-marked CREATEs minus `success`-marked DROPs.
- Wall budget 30s — same as reads test.

---

### `test/integration/test_drop_on_fresh_readonly_clear_error.py` — WR-03 (new file)

**Analog:** `test/integration/test_concurrent_reads_per_call_conn.py` (PEP-723 + `open_master` fixture).

**Test shape** (no concurrency — single thread):
```python
# 1. Create RW DB at a temp path; LOAD extension; close.
# 2. Reopen as RO: connect(path, read_only=True); LOAD extension.
# 3. Attempt DROP SEMANTIC VIEW nonexistent → expect error with substring
#    "semantic view 'nonexistent' does not exist" (NOT "Catalog Error: Table
#    _definitions does not exist").
# 4. Attempt ALTER SEMANTIC VIEW nonexistent SET COMMENT 'x' → same expected
#    error wording.
```

---

### `test/integration/test_load_extension_twice_idempotent.py` — WR-09 (new file)

**Analog:** `test/integration/test_concurrent_reads_per_call_conn.py` fixture.

**Test shape:**
```python
# 1. open_master() — LOAD semantic_views once.
# 2. conn.execute("LOAD semantic_views")  # second load
# 3. Sentinel: a SHOW or query against duckdb_extensions() that surfaces
#    hook count (research notes: there's no public API for parser-extension
#    list — the test must rely on indirect evidence such as: no exception
#    on second LOAD, no warning to stderr, and a successful DDL operation
#    after the second LOAD that goes through the parser hook).
# 4. Stretch goal: subprocess-fork two LOADs and assert single-hook
#    behavior — but this is best-effort given the API surface.
```

---

### `test/sql/phase651_*.test` files — sqllogictest patterns

**Analog:** `test/sql/65_pk_error.test` and `test/sql/65_metadata_via_sql.test` (existing phase-prefixed tests).

**Block-form `statement error` pattern** (per CLAUDE.md — inline form is unsupported):
```
statement error
DROP SEMANTIC VIEW nonexistent
----
semantic view 'nonexistent' does not exist
```

**Required TEST_LIST registration** — append paths to `test/sql/TEST_LIST`:
```
test/sql/phase651_yaml_filesystem_access_gating.test
test/sql/phase651_null_name_inputs.test
test/sql/phase651_bind_probe_hard_errors.test
```

---

### `tests/registration_error_surfaces.rs` — WR-02 (new file)

**Analog:** `tests/no_long_lived_conn.rs` (top-level Rust integration test under `tests/`, dev-deps available, runs via `cargo test`).

**Test shape** — force a registration failure (e.g. by registering two functions with the same name when ALTER_ON_CONFLICT is disabled, or by passing a deliberately bad signature) and assert the surfaced error contains the underlying message. Given that ALTER_ON_CONFLICT swallows name collisions, the most reliable failure trigger is a controlled FFI call into `sv_register_table_function` with null required args (after D-05) and assertion that the returned error buffer contains "null required argument".

---

### `CHANGELOG.md` — D-27

**Analog:** Existing `## [Unreleased]` section at the top of `CHANGELOG.md`.

**Subheading order** (Keep-a-Changelog 1.1.0 per CLAUDE.md): Added / Changed / Deprecated / Removed / Fixed / Security. Allowed deviations: `Known limitations` (final). Phase 65.1 will populate:
- `### Security` — CR-01 SQL-injection elimination
- `### Changed` — WR-07 + WR-08 silent-fallback → hard error
- `### Fixed` — WR-03 wording, WR-02 surfacing, WR-09 idempotence, CR-02 emit-fallback
- `### Removed` — IN-02 + IN-03 dead-code purge

**Wording template** (from researcher D-27 detail at RESEARCH.md lines 774-789):
```markdown
### Security
- Eliminated SQL-injection surface in the `CREATE SEMANTIC VIEW v FROM YAML FILE '<path>'` helper. The path argument is now read via DuckDB's `FileSystem` API directly, removing the `SELECT content FROM read_text('...')` indirection that relied on quote-doubling.

### Changed
- `semantic_view(...)` and the SHOW/DESCRIBE family now raise a clear `Binder Error` when DuckDB's column-type inference fails at bind time (previously fell back to `VARCHAR` or `DECIMAL(18,3)` silently). ...

### Fixed
- `DROP SEMANTIC VIEW` and `ALTER SEMANTIC VIEW` on a read-only database that was never bootstrapped now report `semantic view 'X' does not exist` ...
- Extension-load registration failures now surface the underlying DuckDB exception message ...
- `LOAD semantic_views` is now idempotent — repeat loads no longer accumulate parser-extension hooks in DBConfig.
- Removed defensive single-shot fallback branches in four table-function exec callbacks ...

### Removed
- Internal `src/type_cache.rs` module and `type_id_to_display_name` helper — both were unused after the read-side rebuild in v0.10.0 Phase 65.
```

---

## Shared Patterns

### Per-call `Connection` + bridge cast (BORROW contract — C++ side)

**Source:** `cpp/src/shim.cpp:953-970` (`sv_list_semantic_views_bind` — canonical bridge spike).

**Apply to:** Every bind callback that needs to bridge into Rust with a connection handle. Phase 65.1 does not add new bind callbacks but the WR-06 runtime probe uses the SAME pattern.

```cpp
Connection probe(*context.db);
duckdb_connection borrowed = reinterpret_cast<duckdb_connection>(&probe);
// ... use `borrowed` for FFI calls into Rust dispatcher ...
// Connection dtor at scope exit cleans up.
```

### Error-buffer convention — C++ + Rust pair

**C++ caller side** (cpp/src/shim.cpp:973-974, 783-784, and ~15 other sites):
```cpp
char error_buf[1024];
std::memset(error_buf, 0, sizeof(error_buf));
// Pass &error_buf[0], sizeof(error_buf) into the Rust call.
```

**C++ catch + write (WR-02 new shape):**
```cpp
} catch (const std::exception &e) {
    snprintf(error_buf, error_buf_len, "<context>: %s", e.what());
    return false;
}
```

**Rust writer** (`src/ddl/read_ffi.rs:84-95`):
```rust
pub unsafe fn write_err(buf: *mut u8, buf_len: usize, msg: &str) {
    if buf.is_null() || buf_len == 0 {
        return;
    }
    let max = buf_len.saturating_sub(1);
    let bytes = msg.as_bytes();
    let n = bytes.len().min(max);
    if n > 0 {
        std::ptr::copy_nonoverlapping(bytes.as_ptr(), buf, n);
    }
    *buf.add(n) = 0;
}
```

**Apply to:** WR-02 (registration helpers gain error_buf), WR-06 (probe failure writes to same buffer), WR-08 (Rust caller writes via existing `write_err`).

### Rust panic catch + error-buffer plumb

**Source:** `src/ddl/list.rs:88-184` (canonical `catch_unwind` + `write_err` shape).

**Apply to:** All 17 dispatcher migrations (the catch_unwind frame is preserved; only the wrap-on-entry changes).

```rust
let result = std::panic::catch_unwind(AssertUnwindSafe(|| {
    // ... happy path returns 0_u8; recoverable errors call write_err + return 1_u8 ...
}));
match result {
    Ok(rc) => rc,
    Err(_) => {
        write_err(error_buf, error_buf_len,
            "internal error: panic inside <fn_name>");
        2
    }
}
```

### Two-statement guard SQL (Phase 60 design)

**Source:** `src/parse.rs::rewrite_drop` (lines 2162-2167), `rewrite_alter_rename` (2198-2205), `rewrite_alter_comment` (2275-2282).

**Apply to:** WR-03 — the outer-CASE wrap goes INSIDE the existing two-statement string. Statement separator stays `;`; the SELECT (now with outer CASE) is the first statement; the DELETE/UPDATE follows unchanged.

```rust
let guard = existence_guard_select(name_escaped);  // returns the augmented SELECT
Ok(Some(format!(
    "{guard}; \
     DELETE FROM semantic_layer._definitions WHERE name = '{name_escaped}' \
     RETURNING name AS view_name"
)))
```

### `ALTER_ON_CONFLICT` for idempotent re-registration

**Source:** `cpp/src/shim.cpp:606-609, 671`:
```cpp
info.on_conflict = OnCreateConflict::ALTER_ON_CONFLICT;
```

**Apply to:** All table-function and scalar-function registrations. Already universally applied; relevant to WR-09 as the natural-idempotence path for catalog functions (only the parser-extension list needs explicit dedup).

### PEP-723 + open_master fixture for Python integration tests

**Source:** `test/integration/test_concurrent_reads_per_call_conn.py:1-66`.

**Apply to:** All 3 new Python integration tests in this phase. Verbatim copy of:
- PEP-723 header (lines 1-5)
- `sys.path.insert + from test_ducklake_helpers import get_ext_dir, get_extension_path` (lines 43-44)
- `open_master(db_path)` function (lines 54-66)
- `tempfile.TemporaryDirectory()` wrapper for the test body

## No Analog Found

| File | Role | Data Flow | Reason |
|------|------|-----------|--------|
| Doc-test `compile_fail` block inside `BorrowedConnection` impl | Rust doc-test | compile-time negative | No existing `compile_fail` doc-tests in repo. Rustdoc convention; well-known but new to this codebase. |
| WR-09 idempotent register pattern in C++ shim | C++ parser-extension dedup | one-shot | No precedent in codebase. Pattern derives directly from DuckDB amalgamation's `ParserExtensions()` iterator + function-pointer comparison. |

Both are documented as Rustdoc / DuckDB amalgamation patterns rather than in-repo analogs — the planner should treat the research excerpts as authoritative.

## Metadata

**Analog search scope:**
- `cpp/src/shim.cpp` (full 2,545 LOC scanned via offset-targeted Reads on referenced line ranges)
- `cpp/src/shim.hpp` (header signature block)
- `src/ddl/*.rs` (read_ffi, list, alter_helpers_ffi inspected directly; 12 dispatcher sites enumerated in research)
- `src/query/*.rs` (table_function, explain — confirmed pattern surface)
- `src/lib.rs:450-575` (init_extension registration sequence)
- `src/parse.rs:2100-2300` (existence_guard_select + DROP/ALTER rewrite SQL)
- `tests/no_long_lived_conn.rs` (full AST guard template)
- `test/integration/test_concurrent_reads_per_call_conn.py` (full Python integration template)
- `test/sql/TEST_LIST` + listing of `test/sql/65_*.test` (sqllogictest conventions)
- `CHANGELOG.md` Unreleased section structure

**Files scanned:** 11 source files + 3 reference test files = 14 distinct files read in this session.

**Pattern extraction date:** 2026-05-25

**Lockfile alignment:** All extracted patterns reference live code as of branch `milestone/v0.10.0` HEAD (commit `04c3ac6`). DuckDB amalgamation pinned at 1.5.2 via Cargo.lock — API signatures stable.
