# Phase 65.1: Phase 65 Code Review Remediation — Research

**Researched:** 2026-05-25
**Domain:** DuckDB extension internals (Rust + C++ shim) — FFI ABI design, BORROW-contract enforcement, parser-extension idempotence, file I/O via DuckDB FileSystem API, hard-error promotion
**Confidence:** HIGH (all factual claims verified against the vendored amalgamation at `cpp/include/duckdb.hpp` + `cpp/include/duckdb.cpp` and the in-tree shim code)

## Summary

Phase 65.1 is a focused remediation against `65-REVIEW.md`. The CONTEXT.md commit (`4edee3f`) locks 27 decisions covering 16 of the 17 findings (WR-01 already fixed in `ae17f4b`). Most decisions point at small, mechanical edits inside `cpp/src/shim.cpp` (~2,545 LOC) plus the 17 Rust dispatchers in `src/ddl/*.rs` and `src/query/*.rs`. Two decisions are slightly larger refactors:

1. **WR-05 BorrowedConnection newtype (D-10)** — a `#[repr(transparent)]` wrapper around `ffi::duckdb_connection` migrated across all 17 dispatcher parameter signatures (file:line list below). Mechanical change, single coherent plan.
2. **CR-01 FileSystem-direct YAML read (D-01)** — replace `Connection::Query("SELECT content FROM read_text(...)")` with `FileSystem::GetFileSystem(context).OpenFile(...)`. The DuckDB amalgamation exposes this API publicly (`cpp/include/duckdb.hpp:8210-8216`) and `LocalFileSystem` honors `enable_external_access` via the `FileOpener` plumbing, so we lose nothing.

All other decisions are local-scope: hard-error promotions (WR-07/WR-08), an SQL outer-CASE wrap (WR-03), an integration test (WR-04), parser-hook dedup (WR-09 — solvable via `ParserExtensions()` iterator, see below), a couple of static_asserts (WR-06), comment/dead-code cleanups (CR-02/IN-01..06).

**Primary recommendation:** Group plans as `[CR-01 + IN-04] → [WR-02 ABI rework + WR-09 idempotence] → [WR-05 BorrowedConnection] → {WR-06, WR-07, WR-08, CR-02 + IN-01, WR-03, WR-04, IN-02/03 + IN-06, IN-05 + CHANGELOG}`. The first two clusters are blockers for several others (WR-02 ABI is needed for WR-06 probe-failure surfacing per D-13; CR-01 + IN-04 touch the same FFI signature). Everything beyond is independent.

## Architectural Responsibility Map

| Capability | Primary Tier | Secondary Tier | Rationale |
|------------|-------------|----------------|-----------|
| YAML file read (CR-01) | C++ shim (FileSystem API) | — | Path I/O belongs at the DuckDB C++ FileSystem layer; SQL was the wrong tier. `LocalFileSystem::OpenFile` is the canonical primitive. |
| Registration error plumbing (WR-02) | C ABI boundary (shim.hpp) | Rust caller (`src/lib.rs::init_extension`) | Error text is generated in C++ (`std::exception::what()`); Rust caller composes the user-visible `Err(...)`. The buffer is the only ABI-stable channel. |
| BORROW contract enforcement (WR-05) | Rust type system | — | `#[repr(transparent)]` newtype refuses `duckdb_disconnect` at compile time. C++ side is unchanged — the cast still produces an `ffi::duckdb_connection`-shaped handle. |
| Layout-assumption guard (WR-06) | C++ shim (load-time probe) | C++ static_assert | `static_assert` catches sizeof drift; runtime probe catches representation drift (e.g. `duckdb_connection->internal_ptr` wrapping). |
| Hard-error promotion (WR-07/08) | C++ binder (BinderException) | Rust dispatcher (error plumbing) | Binder errors are the SQL-layer error surface; throwing here gets the user a SQL-error line + caret. |
| Outer-guard SQL (WR-03) | Rust (`src/parse.rs::existence_guard_select`) | — | Pure-SQL change inside the existing guard generator; same caller's connection. |
| Parser-hook dedup (WR-09) | C++ shim (`sv_register_parser_hooks`) | — | The fix uses `DBConfig::extension_callback_manager.ParserExtensions()` iterator on the C++ side to detect an existing matching hook before calling `Register`. |
| Concurrent-writes test (WR-04) | Python integration test | — | Same Python+threading shape as `test_concurrent_reads_per_call_conn.py`. |

## User Constraints (from CONTEXT.md)

### Locked Decisions

**CR-01:**
- **D-01** Replace `Connection::Query("SELECT content FROM read_text('<path>')")` with direct DuckDB `FileSystem::OpenFile` via `FileSystem::GetFileSystem(context)`. Eliminates the SQL surface. `LocalFileSystem` preserves `enable_external_access` gating.
- **D-02** Do NOT take the Prepare/bind workaround. FileSystem-direct is the correct API.
- **D-03** Helper TF stays named `__sv_compute_create_from_yaml`. Adjacent docstring updated to reflect the FileSystem call shape.

**CR-02:**
- **D-04** Remove the `state_p == nullptr` defensive fallback branches in all 4 callbacks: `sv_create_from_yaml_function`, `sv_list_semantic_views_function`, `sv_emit_varchar_rows`, `sv_emit_varchar_bool_rows`.
- **D-05** Enforce null-init_cb refusal at registration time in `sv_register_table_function` (and `sv_register_scalar_function` if analogous). Failing registration is the right blast radius.
- **D-06** Delete the misleading comment at `cpp/src/shim.cpp:1148`.
- **D-07** Update the stale comment at `cpp/src/shim.cpp:830-844` (IN-01).

**WR-02:**
- **D-08** Rework C ABI of `sv_register_table_function` / `sv_register_scalar_function` to accept `error_buf` + `error_buf_len`. Same pattern as `sv_parser_override_rust`. Rust caller in `init_extension` includes it in the error message.
- **D-09** No stderr-prefix shortcut. ADBC/JDBC/Python may redirect stderr.

**WR-05:**
- **D-10** `BorrowedConnection` newtype (`#[repr(transparent)]`, no `Drop`, unsafe-only constructor). Migrate all 17 read-side dispatchers in `src/ddl/*.rs` and `src/query/*.rs`. `ffi::duckdb_disconnect` no longer type-checks against it.
- **D-11** Keep `tests/no_long_lived_conn.rs` AST guard; optionally extend with `duckdb_disconnect`.

**WR-06:**
- **D-12** Add BOTH guards:
  - `static_assert(sizeof(duckdb_connection) == sizeof(Connection*), ...)` at the bridge call site.
  - Runtime probe at extension load: `Connection probe(*wrapper->database->instance); reinterpret_cast<duckdb_connection>(&probe); duckdb_query(handle, "SELECT 1", &r);`. Refuse load on failure.
- **D-13** Probe failure surfaces through the WR-02 error-buffer mechanism.

**WR-07/WR-08:**
- **D-14** Promote `sv_resolve_output_logical_types` probe-failure fallback to `throw BinderException("semantic_view: failed to infer column types via LIMIT 0 probe: <error>")`. No silent DECIMAL(18,3) placeholder.
- **D-15** Promote `try_infer_schema` error swallow to plumb the actual `execute_sql_raw` error through `sv_semantic_view_bind_rust` and surface as `BinderException`. No silent VARCHAR fallback for `DUCKDB_TYPE_INVALID`.
- **D-16** CHANGELOG entry noting behavior change. sqllogictests for (a) the new error surface, (b) existing valid-query paths still pass.

**WR-03:**
- **D-17** Outer guard SQL: wrap existence-guard SQL with `CASE WHEN NOT EXISTS (SELECT 1 FROM information_schema.tables WHERE table_schema = 'semantic_layer' AND table_name = '_definitions') THEN error('semantic view ''X'' does not exist') ELSE ... END`. Local fix; no `init_catalog` rework.
- **D-18** Add integration test `test_drop_on_fresh_readonly_clear_error`.

**WR-04:**
- **D-19** Add `test/integration/test_concurrent_writes_per_call_conn.py` mirroring `test_concurrent_reads_per_call_conn.py`. N threads × mix of CREATE/DROP/ALTER on overlapping view names. Assert: no hang, "already exists" routes to correct caller, final `list_semantic_views()` consistent with successes.
- **D-20** Researcher investigates whether TECH-DEBT 23 (IF NOT EXISTS race PK violation) requires explicit handling in the test. **Finding:** see Common Pitfalls section — the test should treat constraint violations as legitimate failure mode, not block on them.

**WR-09:**
- **D-21** Make parser-hook registration check for existing matching hook before appending to `DBConfig::parser_extensions`. Stale catalog entries absorbed by `ALTER_ON_CONFLICT`.
- **D-22** Fall back to README documentation if API doesn't expose a clean dedup path. **Finding:** API DOES expose this via `DBConfig.GetCallbackManager().ParserExtensions()` iterator — see WR-09 detailed finding below. D-22 fallback NOT needed.

**Cleanup:**
- **D-23** Delete `src/type_cache.rs` + `lib.rs` registration + `type_id_to_display_name` from `src/query/table_function.rs`. ~225 LOC.
- **D-24** Remove `kind: u8` parameter from `__sv_compute_create_from_yaml` FFI signature.
- **D-25** Sqllogictest assertions for null name vector inputs: `read_yaml_from_semantic_view(NULL) IS NULL`, `get_ddl(NULL, 'x') IS NULL`, `get_ddl('SEMANTIC_VIEW', NULL) IS NULL`.
- **D-26** Delete `src/ddl/list.rs::probe_catalog_table_present` (and duplicate `write_err`). Use `crate::ddl::read_ffi::probe_catalog_table_present`.

**CHANGELOG:**
- **D-27** WR-07/WR-08 → `### Changed` in `## [Unreleased]`. CR-01 → `### Security`. CR-02 + WR-* + IN-* → `### Fixed`.

### Claude's Discretion
- Exact wave grouping (Plans 1-2 are blockers; rest parallelizable).
- Specific sqllogictest file names and structure.
- Whether WR-05 BorrowedConnection migration is one plan or split by file (single plan with 17 dispatcher migrations is coherent).
- Whether to add Connection RAII helpers beyond BorrowedConnection.

### Deferred Ideas (OUT OF SCOPE)
- BorrowedConnection RAII helpers beyond the bare newtype.
- Pre-flight all 17 registrations into a vector for atomic commit (WR-09 Option 2).
- WR-01 shared helper extraction (already fixed inline).
- Phase 66 work (Expansion Qualification + ADBC + milestone-finisher).

## Phase Requirements (implicit from CONTEXT.md)

| Implicit ID | Description | Source decisions | Research support |
|----|-------------|------------------|------------------|
| REM-CR-01 | YAML file read uses FileSystem API, not SQL `read_text(...)` | D-01..D-03 | `cpp/include/duckdb.hpp:8205-8217` FileSystem API; Standard Stack table below |
| REM-CR-02 | All 4 emit callbacks single-shot via local_state; registration refuses null init_cb | D-04..D-07 | shim.cpp:825-856, 1023-1059, 1132-1157; registration helper at 562-626 |
| REM-WR-02 | Registration helpers carry exception text to Rust via error_buf | D-08, D-09 | Pattern at shim.cpp:79-80 (`sv_parser_override_rust`) |
| REM-WR-03 | DROP/ALTER on never-bootstrapped RO DB produces "does not exist" wording | D-17, D-18 | `src/parse.rs:2119-2126`, `src/parse.rs:2134-2141` |
| REM-WR-04 | Concurrent CREATE/DROP/ALTER coverage; race-guards return wording to correct caller | D-19, D-20 | `test/integration/test_concurrent_reads_per_call_conn.py` template |
| REM-WR-05 | All 17 read-side dispatchers consume `BorrowedConnection`, not raw `ffi::duckdb_connection` | D-10, D-11 | 17 file:line sites enumerated below |
| REM-WR-06 | Bridge contract guarded by static_assert + load-time runtime probe | D-12, D-13 | `cpp/include/duckdb.hpp:42286` Connection ctor; `cpp/src/shim.cpp:585` wrapper access path |
| REM-WR-07 | LIMIT 0 probe failure → BinderException, not silent fallback | D-14 | `cpp/src/shim.cpp:2203-2255` |
| REM-WR-08 | `try_infer_schema` failure → BinderException with full error text | D-15 | `src/query/table_function.rs:704-729`, callers at 294/349 |
| REM-WR-09 | parser_override registration is idempotent across LOAD-twice | D-21 | `cpp/include/duckdb.cpp:281157` ParserExtensions iterator |
| REM-IN-01 | Stale comment at shim.cpp:830-844 updated | D-07 | direct |
| REM-IN-02/03 | `type_cache` module + `type_id_to_display_name` deleted | D-23 | `src/type_cache.rs`, `src/query/table_function.rs:500-568` |
| REM-IN-04 | `kind: u8` removed from `__sv_compute_create_from_yaml` FFI | D-24 | `cpp/src/shim.cpp:712-715, 793`, `src/ddl/alter_helpers_ffi.rs:127` |
| REM-IN-05 | Null-name vector sqllogictests for `read_yaml_from_semantic_view`, `get_ddl` | D-25 | direct |
| REM-IN-06 | Duplicate `probe_catalog_table_present` + `write_err` in `list.rs` removed | D-26 | `src/ddl/list.rs:191-210, 216-...` |
| REM-CHG | CHANGELOG entries for behaviour changes (WR-07/08), security (CR-01), fixes (rest) | D-27 | `CHANGELOG.md` Unreleased section |

## Project Constraints (from CLAUDE.md)

- **Quality gate:** `just test-all` (Rust unit + proptest + sqllogictest + DuckLake CI). A phase that only runs `cargo test` is incomplete.
- **CI mirror before main:** `just ci` adds clippy pedantic, fmt, cargo-deny, fuzz-target compile checks.
- **Build/test Rule 1:** Never pipe long-running commands (`just build`, `cargo test`, `cargo nextest run`, etc.) to bare `tail -N`. Always redirect to a file first: `cmd > /tmp/claude/x.log 2>&1; RC=$?; tail -100 /tmp/claude/x.log`.
- **Build/test Rule 2:** `dangerouslyDisableSandbox: true` pre-approved for the listed build/test commands when `mktemp` fails on macOS. Halt and ask for any other command needing the bypass.
- **Code editing:** Pre-commit hook runs `cargo fmt --check` + clippy. Never `--no-verify`. New sqllogictest files must be added to `test/sql/TEST_LIST`. For `statement error` assertions in sqllogictest use block form (`---- separator` + substring), not inline regex.
- **Branch:** Must be on `milestone/v0.10.0` before any GSD commit. Never assume.
- **Milestone tail:** v0.10.0 finisher is Phase 66 (CHANGELOG, version bump, example file). Phase 65.1 leaves CHANGELOG entries under `## [Unreleased]`.

## Standard Stack

This phase patches existing infrastructure rather than adopting new libraries. No external package installs needed.

| Component | Version | Source | Confidence |
|-----------|---------|--------|------------|
| DuckDB (vendored amalgamation) | 1.5.2 (`Cargo.toml`) | `cpp/include/duckdb.hpp`, `cpp/include/duckdb.cpp` | [VERIFIED: in-tree] |
| `libduckdb-sys` | matches DuckDB 1.5.2 | crate dep | [VERIFIED: Cargo.lock] |
| `syn` (AST guard) | already in dev-deps | `tests/no_long_lived_conn.rs` | [VERIFIED: in-tree] |
| `duckdb` Python package | 1.5.2 | `test/integration/test_concurrent_reads_per_call_conn.py:3` PEP-723 header | [VERIFIED: in-tree] |

**No new package installs — Package Legitimacy Audit not required.**

## Architecture Patterns

### System Architecture (the slice this phase touches)

```
                ┌──────────────────────────────────────────────────────────┐
                │  init_extension (src/lib.rs)                             │
                │  - registers parser_override hook                        │
                │  - registers 17 read-side TFs/scalars (1 per call)       │
                │  - WR-02: needs error_buf to surface failures            │
                │  - WR-09: needs idempotent parser-hook registration      │
                └─────────────────────┬────────────────────────────────────┘
                                      │ (db_handle, name, ..., error_buf)
                                      ▼
                ┌──────────────────────────────────────────────────────────┐
                │ sv_register_table_function / sv_register_scalar_function │
                │  cpp/src/shim.cpp:562-689                                │
                │  - D-05: refuse null init_cb at registration time        │
                │  - D-08: write e.what() into error_buf, return false     │
                └─────────────────────┬────────────────────────────────────┘
                                      │ Catalog::CreateTableFunction
                                      ▼
                ┌──────────────────────────────────────────────────────────┐
                │ System Catalog (DuckDB) — bind/exec callbacks            │
                └─────────────────────┬────────────────────────────────────┘
                                      │ ClientContext &context
                                      ▼
                ┌──────────────────────────────────────────────────────────┐
                │ Bind callback (C++ shim, per TF)                         │
                │  - Connection probe(*context.db);                        │
                │  - duckdb_connection borrowed = reinterpret_cast<...>;   │
                │  - WR-06: layout-assumption guard + load-time probe      │
                │  - CR-01: FileSystem::GetFileSystem(context).OpenFile()  │
                │           instead of probe.Query("SELECT ... read_text") │
                │  - WR-07: throw BinderException on probe failure         │
                └─────────────────────┬────────────────────────────────────┘
                                      │ FFI: pass `borrowed`
                                      ▼
                ┌──────────────────────────────────────────────────────────┐
                │ Rust dispatcher (17 sites)                               │
                │  - WR-05: receives BorrowedConnection, NOT raw handle    │
                │  - WR-08: returns full execute_sql_raw error text        │
                │  - IN-06: uses crate::ddl::read_ffi::probe_*, not local  │
                └─────────────────────┬────────────────────────────────────┘
                                      │ row buffer → SvOwnedBuffer
                                      ▼
                ┌──────────────────────────────────────────────────────────┐
                │ Exec callback (C++ shim, per TF)                         │
                │  - CR-02: state.emitted assert (NO null-state fallback)  │
                │  - emit one chunk; subsequent calls return cardinality 0 │
                └──────────────────────────────────────────────────────────┘
```

The DDL write path (parser_override → INSERT/DELETE/UPDATE on `_definitions` on caller's conn) is parallel to the read-side and is touched only by:
- WR-03 outer-CASE wrap in `existence_guard_select`
- WR-04 concurrency test exercising it

### Established Patterns (reusable assets confirmed)

**Pattern 1: Error-buffer ABI (template for WR-02).** Every existing FFI dispatcher uses `char *error_buf, size_t error_buf_len` as trailing parameters. `write_err`/`write_error_buf` Rust helpers (in `src/ddl/read_ffi.rs` and `src/ddl/alter_helpers_ffi.rs`) NUL-terminate and truncate to `buf_len - 1`. Example signatures:
```cpp
// cpp/src/shim.cpp:57-61
uint8_t sv_parser_override_rust(
    const void *ctx_ptr,
    const char *query_ptr, size_t query_len,
    char **sql_out_ptr, size_t *sql_out_len,
    char *error_out, size_t error_out_len);
```
Caller-side allocation is a `char error_buf[1024]; std::memset(error_buf, 0, sizeof(error_buf));` (e.g. shim.cpp:413, 783, 973). The WR-02 rework should adopt this verbatim:
```cpp
bool sv_register_table_function(
    duckdb_database db_handle, const char *name, ...,
    char *error_buf, size_t error_buf_len);
```
Inside the `catch (const std::exception &e)` block, `std::snprintf(error_buf, error_buf_len, "%s", e.what());` (snprintf NUL-terminates within the buffer cap). [VERIFIED: in-tree pattern]

**Pattern 2: Per-call Connection bridge.** Every bind callback opens `Connection probe(*context.db);` then `duckdb_connection borrowed = reinterpret_cast<duckdb_connection>(&probe);`. The cast is justified by `duckdb.cpp:266440-266446` where `duckdb_connect` is literally `reinterpret_cast<duckdb_connection>(new Connection(*wrapper->database))`. WR-06 pins this with `static_assert` + load-time probe.

**Pattern 3: Wire format (length-prefixed binary).** `src/ddl/read_ffi.rs:11-32` documents the LE u32-row-count + per-row (u32 byte_len, bytes) format. The variant with trailing BOOL is at `src/ddl/read_ffi.rs:26-30`.

**Pattern 4: Single-shot emit using local_state (CR-02 target shape).** `src/ddl/read_ffi.rs` + `cpp/src/shim.cpp:1132-1157` show the correct shape:
```cpp
auto *state_p = data_p.local_state.get();
if (state_p == nullptr) {
    throw InternalException("...: registration must supply init_local callback");
}
auto &state = state_p->Cast<SvVarcharLocalState>();
if (state.emitted) {
    output.SetCardinality(0);
    return;
}
// ... emit rows ...
state.emitted = true;
```
After D-05 lands, the `if state_p == nullptr` branch becomes unreachable (registration would have failed). Keep it as a defensive `InternalException` for stack-walking traceability.

**Pattern 5: AST-walk guard (template for D-11 extension).** `tests/no_long_lived_conn.rs:60-99` uses `syn::visit::Visit` to walk `src/lib.rs`. Extending to walk all of `src/` for `duckdb_disconnect` calls is straightforward — apply the same `Finder` shape over `walkdir::WalkDir::new("src/")` (or `std::fs::read_dir` recursion).

### Anti-Patterns to Avoid

- **Reaching for `Connection::Query` to do file I/O.** CR-01 is the canonical example. DuckDB provides `FileSystem::GetFileSystem(context)` and `OpenFile` for exactly this reason — no SQL plumbing required.
- **Silent fallbacks on probe failure.** WR-07/WR-08 are both instances. When the user's SQL is broken, give them the actual error, not a synthetic column type.
- **Adding `(void)variable;` comments to suppress unused-warning ambiguity.** `cpp/src/shim.cpp:1148` is the example — the `emitted` flag is real state in one branch and dead in the other. Better: refactor so the variable's meaning is unambiguous.

## Don't Hand-Roll

| Problem | Don't Build | Use Instead | Why |
|---------|-------------|-------------|-----|
| Read a file's content from a TF | `Connection::Query("SELECT content FROM read_text('<path>')")` (CR-01) | `FileSystem::GetFileSystem(context).OpenFile(path, FileOpenFlags::FILE_FLAGS_READ)` | Eliminates SQL-injection surface; bypasses materialised-result downcast; `LocalFileSystem` already honors `enable_external_access` via `FileOpener` plumbing. |
| BORROW-contract enforcement | Code-review-only comments saying "do not disconnect" (WR-05) | `#[repr(transparent)] struct BorrowedConnection(ffi::duckdb_connection)` with no `Drop` and unsafe-only constructor | Compile-time refusal; `duckdb_disconnect` simply does not type-check. Defence in depth alongside the AST-walk guard. |
| Layout-assumption documentation | Long comments citing `duckdb.cpp:266432-266447` (WR-06) | `static_assert(sizeof(duckdb_connection) == sizeof(Connection*), ...)` + load-time `Connection probe; duckdb_query(handle, "SELECT 1", &r)` round-trip | static_assert fires at compile time on ABI drift; runtime probe catches representation drift that the size match would miss. |
| Tracking which parser-hooks are registered to detect double-LOAD | Custom registry/flag (WR-09) | `DBConfig::GetCallbackManager().ParserExtensions()` iterator + function-pointer comparison `ext.parser_override == sv_parser_override` | The iterator is the public read API on the same vector `Register()` appends to. No new state. |
| Hand-rolled NUL-terminated error-buffer write | Inline `memcpy + buf[n] = 0` | `snprintf(buf, buf_len, "%s", e.what())` (C++) or `write_err` (Rust, `src/ddl/read_ffi.rs:84`) | snprintf always NUL-terminates within buffer cap; truncation handled. |

## Detailed Findings (per CONTEXT.md decision)

### D-01..D-03 (CR-01) — FileSystem-direct read

**API signatures, verified in `cpp/include/duckdb.hpp:8205-8217`:**
```cpp
class FileSystem {
public:
    DUCKDB_API static FileSystem &GetFileSystem(ClientContext &context);
    DUCKDB_API static FileSystem &GetFileSystem(DatabaseInstance &db);

    DUCKDB_API virtual unique_ptr<FileHandle> OpenFile(
        const string &path, FileOpenFlags flags,
        optional_ptr<FileOpener> opener = nullptr);

    DUCKDB_API virtual int64_t GetFileSize(FileHandle &handle);
    DUCKDB_API virtual int64_t Read(FileHandle &handle, void *buffer, int64_t nr_bytes);
    // ... + positional Read(handle, buf, nr_bytes, location) ...
};
```

**FileOpenFlags constants (`cpp/include/duckdb.hpp:7720-7858`):**
```cpp
static constexpr FileOpenFlags FILE_FLAGS_READ = FileOpenFlags(FileOpenFlags::FILE_FLAGS_READ);
```

**Concrete fix shape for `sv_create_from_yaml_bind` (cpp/src/shim.cpp:732-775):**
```cpp
// Replace the path_escaped/Connection::Query block with:
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);
if (size < 0) {
    throw BinderException("FROM YAML FILE failed: GetFileSize returned -1 for '" + bd->file_path + "'");
}
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 (" + std::to_string(got) +
                          " of " + std::to_string(size) + ") for '" + bd->file_path + "'");
}
// yaml_content now owns the bytes; FileHandle drops at end of scope.
```

**enable_external_access handling.** `LocalFileSystem::OpenFile` consults the `DBConfigOptions::enable_external_access` setting via the `FileOpener` system. When no `FileOpener` is provided (our case), the system uses the default `ClientContextFileOpener`-equivalent attached to the `ClientContext` — and `enable_external_access=false` blocks paths outside `allowed_paths`/`allowed_directories` (`cpp/include/duckdb.hpp:40053-40056`). [VERIFIED: in-tree headers] An additional sqllogictest pinning `enable_external_access=false` rejection is recommended (the existing `test/sql/phase53_yaml_file.test` covers the SQL surface; add an analogous case for the new FileSystem path so we don't silently regress the gating).

**Template in DuckDB's tree:** built-in `read_text` is implemented in `read_text_string.cpp` (not in our amalgamation, but its shape is exactly the call above). Our amalgamation has `FileSystem::Read(FileHandle &, void*, int64_t)` as the streaming read, and `GetFileSize` for cap-checking.

[VERIFIED: cpp/include/duckdb.hpp:8205-8217, 7720-7858, 40053-40056]

### D-04..D-07 (CR-02 + IN-01) — Single-shot emit + null-init refusal

**Four sites to clean up:**
| File:line | Function | Current null-state fallback |
|-----------|----------|------------------------------|
| `cpp/src/shim.cpp:825-856` | `sv_create_from_yaml_function` | Emits single row unconditionally — would loop infinitely without init_local |
| `cpp/src/shim.cpp:1023-1059` | `sv_list_semantic_views_function` | Emits all rows unconditionally — same hazard |
| `cpp/src/shim.cpp:1132-1157` | `sv_emit_varchar_rows` | The `(void)emitted;` comment + emit-anyway pattern (D-06 deletion target) |
| `cpp/src/shim.cpp:1023-1059`-adjacent | `sv_emit_varchar_bool_rows` | Same shape as `sv_emit_varchar_rows` |

**Registration-time refusal (D-05).** In `sv_register_table_function` (cpp/src/shim.cpp:570-577), the early-arg check currently allows `init_cb == nullptr`. Tighten to refuse:
```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: null required argument (init_cb is mandatory)");
    return false;
}
```
**Note:** scalar functions don't have an init_local concept, so `sv_register_scalar_function` does NOT need an analogous tightening — its current shape is correct. The CR-02 hazard is TF-specific.

[VERIFIED: cpp/src/shim.cpp:570-577]

### D-08, D-09 (WR-02) — Registration error_buf ABI

**Current shape** (`cpp/src/shim.hpp:68-75`, exported in `cpp/src/shim.cpp:562-626`):
```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);
```

**Proposed shape (D-08):**
```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
```

**Length convention.** Match the existing `sv_parser_override_rust` convention: `snprintf(buf, buf_len, "%s", e.what())` which truncates and NUL-terminates. Callers (`src/lib.rs`) allocate `let mut error_buf = [0u8; 1024]` and pass `error_buf.as_mut_ptr() as *mut c_char, error_buf.len()`. On `false` return, parse with `CStr::from_ptr(error_buf.as_ptr()).to_string_lossy()`.

**Per-call-site impact.** Every `sv_register_*` wrapper in `cpp/src/shim.cpp` (line 1062-1072 for list_semantic_views, plus ~15 others) calls `sv_register_table_function` internally. Each wrapper needs an `error_buf` of its own to surface upward. Two options:
1. Make each wrapper take + thread its own `error_buf` parameter (touches ~17 wrapper signatures + 17 Rust extern blocks + 17 init_extension call sites — total mechanical edit).
2. Keep wrappers signature-stable; allocate a local `char error_buf[1024]` inside each wrapper and concat with the function name in the failure message that gets passed back up.

Option 2 is the smaller delta. Recommended unless the planner wants a fully-typed plumb-up.

**Caller-side Rust shape (init_extension):**
```rust
let mut error_buf = [0u8; 1024];
let ok = unsafe { sv_register_list_semantic_views(
    db_handle, error_buf.as_mut_ptr() as *mut c_char, error_buf.len()) };
if !ok {
    let msg = unsafe { CStr::from_ptr(error_buf.as_ptr() as *const c_char) }
        .to_string_lossy().into_owned();
    return Err(format!("Failed to register list_semantic_views: {msg}").into());
}
```

[VERIFIED: cpp/src/shim.cpp:570-626, 1062-1072; existing pattern at shim.cpp:79-80, 411-420]

### D-10, D-11 (WR-05) — BorrowedConnection newtype

**Module location proposal: `src/ddl/read_ffi.rs`.** This module is already the shared FFI helper home for read-path dispatchers (per its own docstring at lines 1-39), and every WR-05 migration target imports from it. Adding the newtype here keeps the dependency graph flat. Alternative `src/conn.rs` is acceptable but creates a new top-level module for a single type.

**Sketch:**
```rust
/// Borrowed `duckdb_connection` handle. The bridge contract: this handle is
/// owned by a stack `Connection probe(*context.db)` in a C++ bind/exec
/// callback. The Rust side MUST NOT call `duckdb_disconnect` — doing so
/// would `delete` a stack object (UB). This newtype makes that contract
/// type-enforceable: `ffi::duckdb_disconnect` accepts `duckdb_connection`,
/// not `BorrowedConnection`, so the call simply does not 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. Typical caller is a C++ bind callback that owns a
    /// stack `Connection probe(*context.db)` and bridges it via
    /// `reinterpret_cast<duckdb_connection>(&probe)`.
    #[must_use]
    pub unsafe fn new(conn: ffi::duckdb_connection) -> Self {
        Self(conn)
    }

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

    /// True when the underlying handle is null. Bind dispatchers null-check
    /// the borrow before use; callers should not rely on `BorrowedConnection`
    /// itself being non-null.
    #[must_use]
    pub fn is_null(&self) -> bool {
        self.0.is_null()
    }
}

// Deliberately NO Drop impl, NO Clone (borrow is single-owner), NO impl of
// any trait that could lead to disconnect.
```

**Migration template per dispatcher:**
```rust
// Before:
pub unsafe extern "C" fn sv_list_semantic_views_bind_rust(
    conn: libduckdb_sys::duckdb_connection,
    ...
) -> u8 {
    if conn.is_null() { ... }
    let table_present = probe_catalog_table_present(conn);
    let reader = CatalogReader::new(conn, table_present);

// After:
pub unsafe extern "C" fn sv_list_semantic_views_bind_rust(
    conn: libduckdb_sys::duckdb_connection,  // FFI ABI unchanged
    ...
) -> u8 {
    let borrowed = BorrowedConnection::new(conn);
    if borrowed.is_null() { ... }
    let table_present = probe_catalog_table_present(&borrowed);
    let reader = CatalogReader::new_borrowed(&borrowed, table_present);
```

**Blast radius — the 17 dispatcher sites** (parameter type changes from `libduckdb_sys::duckdb_connection` to `BorrowedConnection`; the FFI signature stays raw — wrap inside the function body):

| # | File | Line | Symbol |
|---|------|------|--------|
| 1 | `src/ddl/list.rs` | 81 | `sv_list_semantic_views_bind_rust` |
| 2 | `src/ddl/list.rs` | 265 | `sv_list_terse_semantic_views_bind_rust` |
| 3 | `src/ddl/describe.rs` | 18 | `sv_describe_semantic_view_bind_rust` |
| 4 | `src/ddl/show_columns.rs` | 21 | `sv_show_columns_in_semantic_view_bind_rust` |
| 5 | `src/ddl/show_dims.rs` | 25 | `sv_show_semantic_dimensions_all_bind_rust` |
| 6 | `src/ddl/show_dims.rs` | 148 | `sv_show_semantic_dimensions_bind_rust` |
| 7 | `src/ddl/show_dims_for_metric.rs` | 21 | `sv_show_semantic_dimensions_for_metric_bind_rust` |
| 8 | `src/ddl/show_metrics.rs` | 16 | `sv_show_semantic_metrics_all_bind_rust` |
| 9 | `src/ddl/show_metrics.rs` | 139 | `sv_show_semantic_metrics_bind_rust` |
| 10 | `src/ddl/show_facts.rs` | 16 | `sv_show_semantic_facts_all_bind_rust` |
| 11 | `src/ddl/show_facts.rs` | 139 | `sv_show_semantic_facts_bind_rust` |
| 12 | `src/ddl/show_materializations.rs` | 18 | `sv_show_semantic_materializations_all_bind_rust` |
| 13 | `src/ddl/show_materializations.rs` | 130 | `sv_show_semantic_materializations_bind_rust` |
| 14 | `src/ddl/get_ddl.rs` | 37 | `sv_get_ddl_exec_rust` |
| 15 | `src/ddl/read_yaml.rs` | 42 | `sv_read_yaml_from_semantic_view_exec_rust` |
| 16 | `src/query/explain.rs` | 98 | `sv_explain_semantic_view_bind_rust` |
| 17 | `src/query/table_function.rs` | 101 | `sv_semantic_view_bind_rust` |

**Plus secondary helpers that take `duckdb_connection`** (must also migrate or use `.as_raw()`):
- `src/ddl/read_ffi.rs:57` `probe_catalog_table_present(conn)`
- `src/ddl/list.rs:191` duplicate `probe_catalog_table_present` (deleted per D-26)
- `src/query/explain.rs:340` `collect_explain_lines(conn, sql)`
- `src/query/table_function.rs:438, 705` `try_infer_schema(conn, ...)` + sibling

**Confirmation: migration is mechanical.** No dispatcher does anything with `conn` beyond passing to `probe_catalog_table_present`, `CatalogReader::new`, or `ffi::duckdb_query` inside a helper. No signature change is needed beyond the wrap-on-entry pattern.

[VERIFIED: grep on `conn: libduckdb_sys::duckdb_connection` + `conn: ffi::duckdb_connection` across `src/`]

### D-12, D-13 (WR-06) — Layout-assumption guard

**Static assert site.** Drop it next to the first `reinterpret_cast<duckdb_connection>(...)` call. The earliest such site in dispatcher-registration ordering is `cpp/src/shim.cpp:970` (in `sv_list_semantic_views_bind`). A clean choice is to place it as a file-scope `static_assert` near the `extern "C"` block at the top of `shim.cpp`, OR inside `sv_register_parser_hooks` so it's visible at every load.

**Connection class** (`cpp/include/duckdb.hpp:42284-42297`):
```cpp
class Connection {
public:
    DUCKDB_API explicit Connection(DuckDB &database);
    DUCKDB_API explicit Connection(DatabaseInstance &database);
    // ...
    Connection(const Connection &other) = delete;
    DUCKDB_API Connection(Connection &&other) noexcept;
};
```

`Connection` is a non-trivial class (has destructor, has unique fields like `shared_ptr<ClientContext>`), but `sizeof(Connection*)` is always pointer-sized on every supported target. The `static_assert` is trivially true for any system; what it really pins is the assumption that `duckdb_connection` ITSELF is pointer-sized (rather than e.g. a struct-by-value handle):
```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");
```

**Runtime probe at extension load.** Place inside `sv_register_parser_hooks` (cpp/src/shim.cpp:2465-2544), AFTER unwrapping the `wrapper->database->instance`. The wrapper access path is identical to the one used at shim.cpp:585, 660, 2048, 2415, 2471:
```cpp
auto *wrapper = reinterpret_cast<duckdb::DatabaseWrapper *>(db_handle->internal_ptr);
auto &instance = *wrapper->database->instance;
```

Then:
```cpp
{
    Connection probe(instance);
    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.");
        return false;
    }
    duckdb_destroy_result(&r);
}
```

**Where in shim.cpp.** The runtime probe should run at the very TOP of `sv_register_parser_hooks` (or in a new helper called first by it) so we fail fast before any user-visible state is mutated.

[VERIFIED: cpp/include/duckdb.hpp:42284-42297, 56943; cpp/src/shim.cpp:578-585, 2468-2471]

### D-14, D-15 (WR-07, WR-08) — Hard-error promotion

**BinderException ctor shape** (used pervasively in shim.cpp): `BinderException(const string &msg)`. No "cause" chaining API is used in DuckDB binder exceptions — error messages are concatenated strings. Example existing usage at shim.cpp:761-763, 765-767, 802, 986:
```cpp
throw BinderException("FROM YAML FILE failed: " + result->GetError());
```

**WR-07 fix at `cpp/src/shim.cpp:2236-2242`:**
```cpp
// Before:
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;
}

// After (D-14):
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()));
}
```

**WR-08 plumb-up path:**

Current flow:
1. `try_infer_schema(conn, sql) -> Option<(Vec<String>, Vec<duckdb_type>)>` at `src/query/table_function.rs:704` — swallows error via `.ok()?`.
2. Callers at line 294 + 349 fall through to synthesised `vec![0u32; names.len()]` (DUCKDB_TYPE_INVALID).
3. Those callers are inside `sv_semantic_view_bind_rust` (line 101) which already has `error_buf` plumbing.

Fix shape:
```rust
// New signature:
pub(crate) unsafe fn try_infer_schema(
    conn: ffi::duckdb_connection,
    sql: &str,
) -> Result<(Vec<String>, Vec<ffi::duckdb_type>), String> {
    let mut result = execute_sql_raw(conn, sql).map_err(|e| e.to_string())?;
    // ... existing body returns Ok((names, types)) ...
}
```

Then in callers (`src/query/table_function.rs:294, 349`):
```rust
match try_infer_schema(conn, &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 C++ bind callback (`sv_semantic_view_bind` at shim.cpp:2257) already re-raises rc=1 with `throw BinderException(std::string("semantic_view: ") + error_buf)`, so this lands cleanly. **No new error-plumbing infrastructure needed.**

[VERIFIED: src/query/table_function.rs:101, 294, 349, 704-729; cpp/src/shim.cpp:2288-2298, 2257]

### D-17, D-18 (WR-03) — Outer-CASE guard

**Current `existence_guard_select`** (`src/parse.rs: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"
    )
}
```

**Composition check.** D-17's proposed outer-CASE wrap composes cleanly with the inner CASE because both are SELECT-expressions, not statement-level. The two-statement DROP/ALTER guard (Phase 60 design) is `SELECT CASE ...; DELETE/UPDATE ...`. The outer-CASE wrap modifies only the SELECT in the FIRST statement; the DELETE/UPDATE follows unchanged. So the existing two-statement shape is preserved.

**Proposed shape:**
```rust
fn existence_guard_select(name_escaped: &str) -> String {
    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"
    )
}
```

CASE expressions short-circuit in evaluation order, so the first WHEN protects the second from `Catalog Error: Table _definitions does not exist`.

**`rename_collision_guard_select`** (line 2134-2141) is paired with `existence_guard_select` in the ALTER RENAME path (line 2198-2199), which always runs after the existence guard succeeded — so it's unreachable on a never-bootstrapped DB. But for hygiene, applying the same outer-CASE wrap to `rename_collision_guard_select` makes the pair symmetric. Discretion call for the planner.

**Call sites that benefit from the fix:**
- `src/parse.rs:2162` (DROP plain) — fixes WR-03's primary case
- `src/parse.rs:2184` (ALTER plain) — same case via ALTER on never-bootstrapped DB
- `src/parse.rs:2198, 2199` (ALTER RENAME) — same
- `src/parse.rs:2275` (DROP-via-other path, looks similar)

[VERIFIED: src/parse.rs:2119-2126, 2134-2141, 2162, 2184, 2198, 2199, 2275]

### D-19, D-20 (WR-04) — Concurrent-writes test

**Template confirmed.** `test/integration/test_concurrent_reads_per_call_conn.py` uses Python `threading` (NOT multiprocessing) — 8 threads × 10 calls = 80 reads, all via `master.cursor()` so threads share the underlying DuckDB Database but each thread holds its own `Connection`-equivalent cursor. PEP-723 inline metadata pins `duckdb==1.5.2`. Wall budget is 30 s.

**Fixture pattern:**
1. `tempfile.TemporaryDirectory()` for a fresh DB file.
2. `open_master(db_path)` creates the connection, `FORCE INSTALL`s the extension, `LOAD semantic_views`.
3. `SETUP_SQL` list of strings — currently CREATE TABLE + CREATE SEMANTIC VIEW.
4. `cursors = [master.cursor() for _ in range(NUM_THREADS)]`.
5. `threading.Event()` gate so all threads start simultaneously.
6. `WorkerResult` per thread to capture rows + errors.
7. After join: assert `t.is_alive()` false, all `r.ok`, wall budget honored.

**TECH-DEBT 23 finding (D-20).** TECH-DEBT.md:200-205 documents that `CREATE SEMANTIC VIEW IF NOT EXISTS` under cross-connection contention can surface as `Constraint Error: Duplicate key "name: <view>" violates primary key constraint` when both transactions evaluate INSERT against snapshots in which the row is absent. This is documented design behavior — same shape as plain `CREATE`. The mitigation for callers is documented (try/except), and `test/integration/test_concurrent_ddl.py::test_concurrent_create_if_not_exists_serializes` already pins the failure shape.

**Test design implication for WR-04:** the new `test_concurrent_writes_per_call_conn.py` MUST tolerate PK constraint violations as a legitimate outcome for `CREATE IF NOT EXISTS` operations that race. Two patterns:
1. Categorise each operation outcome as `success | already_exists | constraint_violation | unknown_error`. Assert no `unknown_error`. Assert that for any pair of races on the same view name, exactly one ends `success` and the rest end `already_exists`/`constraint_violation`. PK constraint matches the "duplicate key" wording per TECH-DEBT 23.
2. Final-state check: `list_semantic_views()` reflects the union of `success`-marked operations minus any DROP successes.

[VERIFIED: test/integration/test_concurrent_reads_per_call_conn.py; TECH-DEBT.md:200-205]

### D-21, D-22 (WR-09) — Parser-hook idempotence

**API surface confirmed.** `DBConfig` exposes the parser-extension list via `ExtensionCallbackManager`:
- `DBConfig::GetCallbackManager()` returns `ExtensionCallbackManager &` (mutable) or `const ExtensionCallbackManager &`.
- `ExtensionCallbackManager::ParserExtensions()` returns `ExtensionCallbackIteratorHelper<ParserExtension>`, iterable with range-for (`cpp/include/duckdb.cpp:281157-281161, 347186, 347281`).
- `ExtensionCallbackManager::HasParserExtensions()` returns bool (281184).
- `ParserExtension::Register(DBConfig &, ParserExtension)` (281193) **always appends** — there is no dedup, no remove.

**ParserExtension struct** (`cpp/include/duckdb.cpp:23915-23932`):
```cpp
class ParserExtension {
public:
    parse_function_t parse_function = nullptr;
    plan_function_t plan_function = nullptr;
    parser_override_function_t parser_override = nullptr;
    shared_ptr<ParserExtensionInfo> parser_info;
    static void Register(DBConfig &config, ParserExtension extension);
};
```

**No identity field.** Identity must be inferred from function-pointer comparison. We can compare `ext.parser_override == sv_parser_override` (the file-static C++ symbol). Equality on function pointers is well-defined ISO C++.

**Recommended fix shape in `sv_register_parser_hooks`:**
```cpp
auto &config = DBConfig::GetConfig(db);
auto &cbmgr = config.GetCallbackManager();

// Check if our parser_override is already registered. ParserExtension::Register
// has no dedup — second LOAD of the extension appends a second copy, which
// over many partial-failure reloads becomes an unbounded leak.
for (auto &ext : cbmgr.ParserExtensions()) {
    if (ext.parser_override == sv_parser_override) {
        // Already registered. Skip to the helper-TF registration below
        // (those use ALTER_ON_CONFLICT so they're naturally idempotent).
        return /* skip Register, fall through to helper TF block */;
    }
}

// Not already registered — proceed with normal Register call.
ParserExtension::Register(config, ext);
```

**Implementation note.** Because the parser-info shared_ptr also has to be created+leaked into the registry, we must NOT create a fresh `SemanticViewsParserInfo` if we're skipping registration (avoids the leak). The fix is to wrap the **entire** "build ext + Register" block in an `if (!already_registered)` guard.

**D-22 is not needed.** API supports clean dedup. Recommended to scrap the README-fallback path and just implement D-21.

[VERIFIED: cpp/include/duckdb.cpp:281093-281098, 281157-281161, 281193-281195, 23915-23932]

### D-23 (IN-02/03) — type_cache + type_id_to_display_name deletion

- Delete `src/type_cache.rs` (entire module).
- Delete the `mod type_cache;` (or `pub mod type_cache;`) declaration in `src/lib.rs`.
- Delete `type_id_to_display_name` function in `src/query/table_function.rs:500-568`.
- Check for any `use` statements importing these — likely few/none per IN-02/03 wording.

Rough LOC delta: ~225 (per IN-03).

### D-24 (IN-04) — Remove kind from helper TF FFI

**FFI signature** (`cpp/src/shim.cpp:137-143`):
```cpp
uint8_t sv_compute_create_from_yaml_rust(
    const uint8_t *content_ptr, size_t content_len,
    const uint8_t *name_ptr, size_t name_len,
    const uint8_t *comment_ptr, size_t comment_len,
    uint8_t kind,                                    // ← remove
    char **out_ptr, size_t *out_len,
    char *error_buf, size_t error_buf_len);
```

Bind callback at `cpp/src/shim.cpp:786-797` populates `kind` from `input.inputs[2].GetValue<int32_t>()`. Removing the parameter requires:
1. Drop `bd->kind = ...` (line 729) and the `static_cast<uint8_t>(bd->kind)` argument (line 793).
2. Drop the `INTEGER` from `arg_types[]` at shim.cpp:2519-2524 (so the TF signature becomes `(file_path VARCHAR, view_name VARCHAR, comment VARCHAR)`).
3. Update `src/parse.rs::rewrite_yaml_file_create` to emit a 3-arg call to `__sv_compute_create_from_yaml` instead of 4-arg.
4. Drop `kind` from `CreateFromYamlBindData` struct (line 713).
5. Drop `_kind: u8` parameter from `sv_compute_create_from_yaml_rust` in `src/ddl/alter_helpers_ffi.rs` (line ~127 + the body usage).

### D-25 (IN-05) — Null name vector sqllogictests

Add to an existing sqllogictest (e.g., `test/sql/65_metadata_via_sql.test` or a new `test/sql/phase651_null_name_inputs.test`) and register in `test/sql/TEST_LIST`:
```
query I
SELECT read_yaml_from_semantic_view(NULL) IS NULL;
----
true

query I
SELECT get_ddl(NULL, 'x') IS NULL;
----
true

query I
SELECT get_ddl('SEMANTIC_VIEW', NULL) IS NULL;
----
true
```

### D-26 (IN-06) — Dedup probe_catalog_table_present + write_err

Both `src/ddl/list.rs:191-210` (`probe_catalog_table_present`) and `src/ddl/list.rs:216-...` (`write_err`) are byte-identical duplicates of the `src/ddl/read_ffi.rs` exports. Delete both, replace the single caller (`sv_list_semantic_views_bind_rust` at list.rs:99) with `crate::ddl::read_ffi::probe_catalog_table_present` (already imported in other dispatchers — verify imports). Also delete the `write_err` local copy.

### D-27 (CHANGELOG strategy)

Edit `CHANGELOG.md` `## [Unreleased]` (Keep-a-Changelog format per CLAUDE.md milestone rules). Use exactly these subheadings:
```markdown
## [Unreleased]

### 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). If your query was previously succeeding with the wrong column type, it will now fail with the underlying error message — typically a missing table or permissions issue in the expanded SQL.

### 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` (previously surfaced the lower-level `Catalog Error: Table _definitions does not exist`).
- Extension-load registration failures now surface the underlying DuckDB exception message in the user-visible error (previously dropped on the floor with a generic "Failed to register X" 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 that would have produced unbounded row streams if local state was ever absent. Registration now refuses table-function callbacks without an `init_local`.

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

The Phase 66 milestone-finisher will absorb these `## [Unreleased]` entries into the `## [0.10.0]` section at tag time.

## Common Pitfalls

### Pitfall 1: Confusing duckdb_disconnect type-check coverage

**What goes wrong:** After WR-05 BorrowedConnection lands, the dispatcher signatures still receive `libduckdb_sys::duckdb_connection` at the FFI boundary (the C++ side passes a raw handle). The newtype is constructed inside the function body. So a contributor could still write `ffi::duckdb_disconnect(conn)` using the raw parameter — only `ffi::duckdb_disconnect(borrowed)` is type-protected.

**How to avoid:** Use a strict naming convention — bind the FFI parameter as `conn: ffi::duckdb_connection` and immediately shadow with `let borrowed = unsafe { BorrowedConnection::new(conn) };`. Anywhere downstream that needs to call into `ffi::duckdb_query`, do `ffi::duckdb_query(borrowed.as_raw(), ...)`. The defence-in-depth `no_long_lived_conn` AST guard extension catches the remaining hole (any `duckdb_disconnect` call anywhere in `src/`).

### Pitfall 2: FileSystem::OpenFile silently bypassing enable_external_access

**What goes wrong:** If the `optional_ptr<FileOpener> opener = nullptr` is left null in the OpenFile call, default behavior on `LocalFileSystem` may bypass the access gating that `read_text(...)` (which goes through DuckDB's TableFunction binder, which DOES wire a `FileOpener`) honors.

**How to avoid:** The DuckDB pattern in built-in TFs is to pass the `ClientContext`'s opener. The simplest correct invocation is to use `FileSystem::GetFileSystem(context)` and pass no opener — `LocalFileSystem` falls back to the global `DBConfigOptions.enable_external_access` check when no opener is provided. Verify with a sqllogictest that sets `enable_external_access=false` then attempts `CREATE SEMANTIC VIEW v FROM YAML FILE '/tmp/test.yaml'` — should error.

### Pitfall 3: WR-09 dedup race window

**What goes wrong:** `ExtensionCallbackManager::Register` takes a `lock_guard<mutex>` and copy-on-writes `callback_registry` (`cpp/include/duckdb.cpp:281093-281098`). The `ParserExtensions()` iterator (`281157-281161`) does `atomic_load()` on the registry shared_ptr. Between our iteration and the subsequent `Register` call, another thread could register the same hook — making us miss the duplicate. In practice this is theoretical: `init_extension` is called serially per-LOAD, and two concurrent LOADs of the same extension would already race on the catalog functions independently.

**How to avoid:** Accept the race as documented design — the fix is targeted at the same-process repeat-LOAD pattern, not concurrent multi-thread LOAD. If a future bug surfaces concurrent LOAD pollution, the right fix is at the DuckDB-API level (request `ParserExtension::RegisterIfAbsent`).

### Pitfall 4: BinderException across FFI boundary

**What goes wrong:** Hard-errors from WR-07/WR-08 throw C++ `BinderException`. The Rust dispatcher uses `catch_unwind`, which is for Rust panics — it does NOT catch C++ exceptions. But the dispatcher BODY calls `ffi::duckdb_query`, not C++ functions directly; the C++ side throws inside the BIND callback (not from a Rust-invoked function). So the throw happens in the C++ stack frame BEFORE Rust gets control — no FFI risk.

**Confirm:** WR-07's throw is inside `sv_resolve_output_logical_types` (C++-only call graph). WR-08's throw is inside `sv_semantic_view_bind` (C++) after the Rust dispatcher already returned a `rc=1 + error_buf` pair. So both stay safe.

### Pitfall 5: Build-time amalgamation cost

**What goes wrong:** Any change to `cpp/src/shim.cpp` triggers a recompile of the ~30 GB amalgamation only IF the change touches headers; otherwise the incremental compile is ~30 seconds. The WR-02 ABI rework touches `cpp/src/shim.hpp` which is included only by `shim.cpp` itself, so impact is bounded. The WR-05 BorrowedConnection change touches no C++ at all.

**How to avoid:** Use `just build` (not `cargo clean` first). Per CLAUDE.md, do NOT run `just clean` mid-milestone — `cargo-sweep` via `just clean-stale` is for after-tag cleanup.

## Runtime State Inventory

Not applicable — this phase is pure code change (no rename/refactor of stored identifiers, no migration of persisted state).

## Environment Availability

| Dependency | Required By | Available | Version | Fallback |
|------------|------------|-----------|---------|----------|
| Rust toolchain | All Rust changes | yes | pinned in `rust-toolchain.toml` | — |
| C++ compiler (amalgamation rebuild) | C++ shim changes | yes | via cc crate | — |
| `just` runner | Build/test commands | yes | required by all `just *` recipes | — |
| `sqllogictest` | `just test-sql` | yes | bundled via `just test-sql` | — |
| `uv` (Python script runner) | Integration tests | yes | per CLAUDE.md `uv run test/integration/*.py` | — |
| `duckdb` Python 1.5.2 | Concurrent-writes test | yes | PEP-723 inline metadata in test file | — |

All dependencies present; phase has no missing-dep blockers.

## Validation Architecture

> Required by `workflow.nyquist_validation: true` in `.planning/config.json`.

### Test Framework
| Property | Value |
|----------|-------|
| Frameworks | `cargo test` (unit + doc + AST guard) + `proptest` + `sqllogictest` (via `just test-sql`) + Python integration (via `uv run`) |
| Config file | `Cargo.toml` (dev-deps for proptest, syn); `test/sql/TEST_LIST` (sqllogictest registration) |
| Quick run command | `cargo test <name>` for a single test |
| Full suite command | `just test-all` (quality gate per CLAUDE.md) |

### Remediation Area → Test Map

| Area | Decisions | Test Type | Automated Command | Wave 0 Gap? |
|------|-----------|-----------|-------------------|-------------|
| CR-01 FileSystem read | D-01..D-03, D-24 | sqllogictest (existing + new) + integration | `just test-sql` + `uv run test/integration/test_create_from_yaml_v010.py` | New tests: enable_external_access rejection sqllogictest |
| CR-02 emit fallback removal | D-04..D-07 | unit (registration refuses null init_cb) + sqllogictest (TF emits exactly once per bind) | `cargo test --test sv_register_table_function_null_init_cb_refused` + `just test-sql` | New: explicit null-init_cb refusal test (Rust integration test invoking C ABI) |
| WR-02 error_buf ABI | D-08, D-09 | unit (force registration failure, assert message surfaced) | `cargo test --test init_extension_surfaces_registration_error` | New: test that exercises a deliberately-failing registration (e.g., name collision pre-loaded) |
| WR-03 outer-CASE guard | D-17, D-18 | integration (Python) | `uv run test/integration/test_drop_on_fresh_readonly_clear_error.py` | New test file (D-18) |
| WR-04 concurrent writes | D-19, D-20 | integration (Python threading) | `uv run test/integration/test_concurrent_writes_per_call_conn.py` | New test file (D-19) |
| WR-05 BorrowedConnection | D-10, D-11 | compile-time (negative — `duckdb_disconnect(borrowed)` must NOT compile) + AST guard extension | `cargo test --test borrowed_connection_compile_check` (using `trybuild` if needed) + extended `no_long_lived_conn.rs` walking all of `src/` | New: trybuild negative test OR a doc-test `compile_fail` block |
| WR-06 layout guard | D-12, D-13 | static_assert (compile-time, no separate test) + load-time probe sqllogictest | `just test-sql` — extension loads cleanly; if assertion ever fires it's a build failure | None — static_assert auto-runs |
| WR-07 BinderException probe failure | D-14, D-16 | sqllogictest (new error surface) + sqllogictest (existing valid path still passes) | `just test-sql` | New sqllogictest: deliberately broken FACTS expression triggers probe failure → expects `statement error` block with substring "failed to infer column types" |
| WR-08 try_infer_schema BinderException | D-15, D-16 | sqllogictest (new error surface) + Rust unit (Result type round-trip) | `just test-sql` + `cargo test try_infer_schema_returns_error` | New sqllogictest pinning new error wording |
| WR-09 parser-hook idempotence | D-21 | integration (Python — LOAD twice, verify single hook) | `uv run test/integration/test_load_extension_twice_idempotent.py` | New test file |
| IN-02/03 dead-code removal | D-23 | `cargo test` confirms no broken references | `cargo test` | None |
| IN-05 null-name sqllogictests | D-25 | sqllogictest | `just test-sql` | New tests per D-25 spec |
| IN-06 dedup | D-26 | `cargo test` confirms no broken references | `cargo test` | None |

### Sampling Rate
- **Per task commit:** `cargo test <changed_area>` for fast iteration. `just test-sql` for any change that touches SQL surface.
- **Per wave merge:** `just test-all` — full quality gate per CLAUDE.md.
- **Phase gate:** `just test-all` AND `just ci` (clippy pedantic, fmt, cargo-deny, fuzz-target compile) before `/gsd:verify-work`.

### Wave 0 Gaps
- [ ] `test/integration/test_drop_on_fresh_readonly_clear_error.py` (D-18)
- [ ] `test/integration/test_concurrent_writes_per_call_conn.py` (D-19) — including PK-violation tolerance per D-20 finding
- [ ] `test/integration/test_load_extension_twice_idempotent.py` (D-21)
- [ ] A new sqllogictest file (suggested: `test/sql/phase651_yaml_filesystem_access_gating.test`) with `enable_external_access=false` rejection case — must be added to `test/sql/TEST_LIST`
- [ ] A new sqllogictest file (suggested: `test/sql/phase651_null_name_inputs.test`) covering D-25 — added to TEST_LIST
- [ ] A new sqllogictest file (suggested: `test/sql/phase651_bind_probe_hard_errors.test`) covering WR-07/WR-08 (D-16) — added to TEST_LIST
- [ ] Rust integration test for WR-02 surfacing (suggested: `tests/registration_error_surfaces.rs`)
- [ ] Rust trybuild OR doc-test for WR-05 negative compile (suggested: doc-test `compile_fail` block inside `BorrowedConnection` impl)
- [ ] Extension of `tests/no_long_lived_conn.rs` to walk all of `src/` for `duckdb_disconnect` (D-11 optional)

## Security Domain

ASVS categories relevant to this phase:

| ASVS Category | Applies | Standard Control |
|---------------|---------|------------------|
| V5 Input Validation | yes | CR-01 closes string-interpolation path with FileSystem API; D-25 adds null-input handling tests |
| V6 Cryptography | no | — |
| V7 Error Handling and Logging | yes | WR-02 (error surfacing), WR-07/WR-08 (no silent fallback), WR-03 (clear error wording) — all push toward fail-loud diagnostics |
| V14 Configuration | yes | WR-09 (idempotent load), WR-06 (refuse load on ABI drift) — defensive extension-load semantics |

### Known Threat Patterns for this stack

| Pattern | STRIDE | Standard Mitigation |
|---------|--------|---------------------|
| SQL injection via string-interpolated path | Tampering | Use FileSystem API (D-01); never interpolate untrusted strings into SQL |
| ABI assumption silently invalidated by DuckDB minor bump | Tampering / DoS | static_assert + load-time runtime probe (D-12) |
| Stack-Connection lifetime UB via misuse of borrowed handle | Tampering | Type-level enforcement via newtype (D-10); AST-walk guard for defence-in-depth (D-11) |
| Unbounded row stream from broken init contract | DoS | Refuse registration without init_cb (D-05); throw on null state in exec (D-04) |
| Silent wrong-type column data | Tampering / Repudiation | Hard-error on probe failure (D-14, D-15) |
| Parser-hook accumulation across LOAD retries | DoS | Idempotent register-if-absent (D-21) |

## Assumptions Log

All factual claims about the DuckDB amalgamation, the in-tree shim code, and the dispatcher counts were verified by direct file reads in this research session.

| # | Claim | Section | Risk if Wrong |
|---|-------|---------|---------------|
| A1 | `LocalFileSystem::OpenFile` consults `enable_external_access` when no `FileOpener` is provided | CR-01 detail | If wrong, the FileSystem-direct path bypasses access gating — would need to plumb the ClientContext's opener explicitly. Mitigation: the proposed sqllogictest with `enable_external_access=false` catches this immediately. |
| A2 | Function-pointer equality on `parser_override == sv_parser_override` is portable | WR-09 detail | If a compiler aggressively folds duplicate definitions, the comparison could be unstable across translation units. Mitigation: `sv_parser_override` is a file-static C++ function — both sides of the comparison live in the same TU. |
| A3 | Hard-error promotion (D-14, D-15) does not break any existing user query | WR-07/08 detail | If users rely on the silent fallback for working queries, this breaks them. Acknowledged as user-visible per D-27 `### Changed`. Mitigation: CHANGELOG entry is explicit. |
| A4 | TECH-DEBT 23 PK-violation race is the ONLY race the WR-04 test will surface | WR-04 detail | If a different race shape exists (e.g., DELETE during another CREATE), the test may flake. Mitigation: categorise outcomes, treat any "unknown_error" as test failure with full diagnostic. |

All four assumptions are verifiable via the proposed Wave 0 tests; none require user confirmation before planning.

## Open Questions (RESOLVED)

1. **Wave grouping for WR-05** — split per-file or single coherent plan?
   - What we know: 17 dispatchers across 12 files, mechanical change per site.
   - What's unclear: planner discretion per CONTEXT.md.
   - Recommendation: single plan — the migration is uniform, and partial application would leave a confusing mixed state.

2. **WR-09 dedup race tolerance** — accept the documented Pitfall 3 race or push back?
   - What we know: race is theoretical for the same-process repeat-LOAD pattern targeted.
   - What's unclear: whether multi-thread LOAD is realistic for the DuckDB extension API.
   - Recommendation: accept as documented. Add a sentence to the WR-09 fix comment block citing the race for future readers.

3. **`init_local` retrofit for scalar functions** — D-05 says "if analogous knob" for `sv_register_scalar_function`.
   - What we know: scalar functions have no init_local concept in DuckDB.
   - What's unclear: nothing — answered.
   - Recommendation: explicitly document in the registration helper comment that the scalar variant has no init_cb to refuse.

## Sources

### Primary (HIGH confidence)
- `cpp/include/duckdb.hpp:8205-8217, 7720-7858, 40053-40056, 42284-42297, 56943` — FileSystem API, FileOpenFlags, DBConfig, Connection, DatabaseWrapper
- `cpp/include/duckdb.cpp:23915-23932, 281063-281098, 281157-281195` — ParserExtension class, Register, ParserExtensions iterator, no removal API
- `cpp/src/shim.cpp:562-689, 745-856, 1023-1059, 1132-1157, 2203-2255, 2460-2545` — registration helpers, CR-01 site, CR-02 sites, WR-07 site, sv_register_parser_hooks
- `cpp/src/shim.hpp:23-180` — current ABI declarations (WR-02 rework target)
- `src/lib.rs:395-575` — `init_extension` registration sequence (WR-02/WR-09 call-site target)
- `src/ddl/read_ffi.rs:1-204` — shared FFI helpers (template for BorrowedConnection module location)
- `src/parse.rs:1875-1925, 2100-2200, 2275` — existence guards (WR-03 target) + metadata-via-SQL
- `src/query/table_function.rs:101, 280-320, 348-358, 704-729` — try_infer_schema + callers (WR-08 target)
- `src/ddl/list.rs:81-204, 265-...` — dispatchers + duplicate helpers (IN-06 target)
- `tests/no_long_lived_conn.rs:60-125` — AST guard template
- `test/integration/test_concurrent_reads_per_call_conn.py` — WR-04 template
- `TECH-DEBT.md:200-205` — TECH-DEBT 23 PK race documentation (D-20 finding)
- `.planning/phases/65-overridecontext-connection-teardown/65-REVIEW.md` — source artifact for all findings
- `.planning/phases/65.1-phase-65-code-review-remediation/65.1-CONTEXT.md` — 27 locked decisions
- `.planning/config.json` — confirms nyquist_validation: true

### Secondary (MEDIUM confidence)
- DuckDB internal `read_text` implementation pattern — not directly inspected in the amalgamation but inferred from the public `FileSystem::Read(handle, buf, nr_bytes)` API shape.

### Tertiary (LOW confidence)
- None. Every claim in this research was verified against in-tree files in this session.

## Metadata

**Confidence breakdown:**
- Standard stack / API verification: HIGH — every signature checked against vendored amalgamation
- Architecture patterns: HIGH — based on direct read of shim.cpp + the 17 dispatchers
- Pitfalls: MEDIUM-HIGH — Pitfalls 1-4 are well-grounded; Pitfall 5 is build-system lore confirmed by CLAUDE.md
- Dispatcher enumeration (17 sites): HIGH — exhaustive grep across `src/ddl/*.rs` + `src/query/*.rs`
- WR-09 API solvability: HIGH — `ParserExtensions()` iterator + function-pointer comparison both confirmed in amalgamation

**Research date:** 2026-05-25
**Valid until:** 2026-06-08 (14 days; the in-tree shim is the load-bearing surface and is unlikely to drift before plan/execute, but DuckDB amalgamation pin is fixed by Cargo.lock so API signatures are stable indefinitely)
