---
phase: 65.1-phase-65-code-review-remediation
plan: 03a
subsystem: infra
tags: [rust, ffi, type-safety, newtype, borrow-contract, duckdb, repr-transparent, compile-fail-doctest]

# Dependency graph
requires:
  - phase: 65-overridecontext-connection-teardown
    provides: "17 read-side FFI dispatchers built on the BORROW contract (Connection probe(*context.db) bridged via reinterpret_cast<duckdb_connection>(&probe))"
  - phase: 65.1-phase-65-code-review-remediation
    provides: "65.1-01 Wave 0 test scaffolds (BorrowedConnection compile_fail doctest gap pre-noted in 65.1-VALIDATION.md)"
provides:
  - "BorrowedConnection #[repr(transparent)] newtype with W-02-fixed compile_fail doctest proving duckdb_disconnect(&mut borrowed) does not type-check (type mismatch, NOT mutability)"
  - "15 ddl/ dispatcher sites migrated to wrap-on-entry: let borrowed = BorrowedConnection::new(conn) at the top of each body"
  - "Helper signatures probe_catalog_table_present + CatalogReader::new on &BorrowedConnection — pulls the type-level guard through to every catalog read"
  - "IN-06 / D-26 dedup absorbed: src/ddl/list.rs duplicate probe_catalog_table_present + write_err DELETED; single canonical home in src/ddl/read_ffi.rs"
affects: [65.1-03b, 65.1-09]

# Tech tracking
tech-stack:
  added: []
  patterns:
    - "#[repr(transparent)] newtype wrapping a raw FFI handle to deny disconnect at the type level (BORROW contract enforcement)"
    - "compile_fail doctest with `let mut` binding to prove the failure is type-mismatch, not mutability (W-02 plan-checker fix)"
    - "Wrap-on-entry pattern: extern \"C\" signature unchanged (FFI ABI stable), body wraps raw conn into newtype as the first executable statement"
    - "Canonical home in src/ddl/read_ffi.rs with import-based call sites — duplicates deleted to prevent contributor drift"

key-files:
  created: []
  modified:
    - "src/ddl/read_ffi.rs — BorrowedConnection newtype + as_raw/is_null/new accessors + W-02-fixed compile_fail doctest; probe_catalog_table_present migrated to take &BorrowedConnection"
    - "src/catalog.rs — CatalogReader::new accepts &BorrowedConnection (+ 3 in-tree short-circuit test callers updated to construct BorrowedConnection inline)"
    - "src/ddl/list.rs — both dispatchers (sv_list_semantic_views_bind_rust + sv_list_terse_semantic_views_bind_rust) migrated; duplicate probe_catalog_table_present + write_err DELETED (IN-06 absorbed)"
    - "src/ddl/describe.rs — sv_describe_semantic_view_bind_rust migrated"
    - "src/ddl/show_columns.rs — sv_show_columns_in_semantic_view_bind_rust migrated"
    - "src/ddl/show_dims.rs — sv_show_semantic_dimensions_all_bind_rust + sv_show_semantic_dimensions_bind_rust migrated"
    - "src/ddl/show_dims_for_metric.rs — sv_show_semantic_dimensions_for_metric_bind_rust migrated"
    - "src/ddl/show_metrics.rs — sv_show_semantic_metrics_all_bind_rust + sv_show_semantic_metrics_bind_rust migrated"
    - "src/ddl/show_facts.rs — sv_show_semantic_facts_all_bind_rust + sv_show_semantic_facts_bind_rust migrated"
    - "src/ddl/show_materializations.rs — sv_show_semantic_materializations_all_bind_rust + sv_show_semantic_materializations_bind_rust migrated"
    - "src/ddl/get_ddl.rs — sv_get_ddl_exec_rust migrated"
    - "src/ddl/read_yaml.rs — sv_read_yaml_from_semantic_view_exec_rust migrated"
    - "src/query/explain.rs — minimal BorrowedConnection wrap at CatalogReader::new call site (Plan 03b owns the full wrap-on-entry migration)"
    - "src/query/table_function.rs — minimal BorrowedConnection wrap at CatalogReader::new call site (Plan 03b owns the full migration)"

key-decisions:
  - "BorrowedConnection has NO Drop / Clone / Copy / trait impls — minimal call surface keeps the type-level guard tight. Discretionary methods like query()/prepare() were explicitly NOT added (would defeat the guard by encouraging passing BorrowedConnection around without unwrapping intent)."
  - "FFI signature stays `conn: libduckdb_sys::duckdb_connection` (the C++ side passes a raw handle); the newtype lives entirely inside the Rust body. Wrap-on-entry on line 1 of the catch_unwind closure, immediately after BorrowedConnection import."
  - "W-02 fix: doctest uses `let mut b: BorrowedConnection = unsafe { std::mem::zeroed() };` so the &mut b in duckdb_disconnect(&mut b) cannot fail for the WRONG reason. Verified manually — rustc error is `expected '*mut *mut _duckdb_connection', found '&mut BorrowedConnection'`."
  - "B-04 absorption: IN-06 dedup landed alongside the BorrowedConnection migration in the same plan (not deferred to Plan 09) — avoids brittle byte-diff coupling. The byte-identical duplicates were trivially mergeable post-migration."
  - "Minimal-touch boundary for query/explain.rs + query/table_function.rs: only changed the single CatalogReader::new call line per file (not full wrap-on-entry). Plan 03b owns the full migration. The minimal touch is required because the helper signature change breaks the type system for those callers and the plan must leave src/ compiling."
  - "Test-side migration: 3 short-circuit tests in src/catalog.rs::tests now construct `let borrowed = unsafe { BorrowedConnection::new(std::ptr::null_mut()) };` to feed CatalogReader::new. The null short-circuit path never dereferences the pointer."

patterns-established:
  - "Type-level FFI contract enforcement: wrap raw handles in #[repr(transparent)] newtypes denying the dangerous operation by signature mismatch (here: duckdb_disconnect cannot accept &mut BorrowedConnection)"
  - "compile_fail doctest with mutable binding + crate-path import + explanatory comment as the canonical negative-compile coverage pattern in this repo"
  - "Wrap-on-entry shadowing: `let borrowed = BorrowedConnection::new(conn)` on the first body line shadows the raw param so downstream accidental use of `conn` is now caught by the compiler"

requirements-completed:
  - REM-WR-05
  - REM-IN-06

# Metrics
duration: 25min
completed: 2026-05-25
---

# Phase 65.1 Plan 03a: BorrowedConnection (D-10 ddl/ half) + IN-06 dedup Summary

**Type-level BORROW-contract enforcement via #[repr(transparent)] BorrowedConnection newtype; 15 ddl/ dispatchers + helpers migrated to wrap-on-entry; duplicate probe_catalog_table_present + write_err in src/ddl/list.rs deleted.**

## Performance

- **Duration:** ~25 min
- **Started:** 2026-05-25T17:30:00Z (approx)
- **Completed:** 2026-05-25T17:55:00Z (approx)
- **Tasks:** 2
- **Files modified:** 14

## Accomplishments

- BorrowedConnection newtype lands with the W-02-fixed compile_fail doctest. The doctest constructs `let mut b: BorrowedConnection = unsafe { std::mem::zeroed() };` and attempts `unsafe { libduckdb_sys::duckdb_disconnect(&mut b) };`. Manual verification confirmed rustc reports a type mismatch (`expected '*mut *mut _duckdb_connection', found '&mut BorrowedConnection'`), not an immutable-binding error — exactly the regression this guard prevents.
- All 15 read-side ddl/ dispatcher sites consume `&BorrowedConnection` via wrap-on-entry. Helpers (`probe_catalog_table_present`, `CatalogReader::new`) migrated to take `&BorrowedConnection`; the type-level guard now reaches every catalog read on the read-side bind/exec callbacks.
- IN-06 (D-26) absorbed in this plan (not deferred to Plan 09 per plan-checker B-04). The byte-identical `probe_catalog_table_present` and `write_err` duplicates in `src/ddl/list.rs` are DELETED; `sv_list_semantic_views_bind_rust` now imports the canonical versions from `src/ddl/read_ffi.rs` exactly like `sv_list_terse_semantic_views_bind_rust` already did.
- Quality gates: `cargo build --features extension` exit 0; `cargo test --lib` 844/844; `cargo test --lib --features extension` 859/859; `cargo test --doc --features extension BorrowedConnection` 1/1 (compile_fail proof).

## Task Commits

Each task was committed atomically:

1. **Task 1: BorrowedConnection newtype + W-02-fixed compile_fail doctest** — `b400cd8` (feat)
2. **Task 2: Migrate 15 ddl/ dispatchers + helpers; absorb IN-06 dedup** — `702556a` (refactor)

## Files Created/Modified

See `key-files.modified` above. Summary by category:

- **Newtype + canonical helpers (1 file):** `src/ddl/read_ffi.rs`
- **CatalogReader migration + test caller updates (1 file):** `src/catalog.rs`
- **ddl/ dispatcher migration (10 files, 15 sites):** `src/ddl/{list,describe,show_columns,show_dims,show_dims_for_metric,show_metrics,show_facts,show_materializations,get_ddl,read_yaml}.rs`
- **query/ minimum-touch compile fix (2 files, 2 sites):** `src/query/{explain,table_function}.rs` — Plan 03b owns the full wrap-on-entry migration

## Decisions Made

All decisions inherited from the plan's `<interfaces>` + CONTEXT D-10 — implementation followed the spec as written. The Decision-section bullets in the frontmatter document the *implementation-time* refinements (e.g., scope of `query/*` touches, test-caller migration shape, IN-06 dedup landing site). No architectural deviations.

## Deviations from Plan

### Auto-fixed Issues

**1. [Rule 3 - Blocking] query/explain.rs + query/table_function.rs CatalogReader::new call sites needed BorrowedConnection wrap to keep the build green**

- **Found during:** Task 2 verification (`cargo build --features extension`)
- **Issue:** The plan scoped explicitly to `src/ddl/` and noted "src/query/table_function.rs::try_infer_schema and src/query/explain.rs::collect_explain_lines are migrated in Plan 03b". But the helper-signature change for `CatalogReader::new` (Part B of Task 2) and `probe_catalog_table_present` (Task 1's revert + Part B re-application) breaks the compile of those query/ callers — they pass raw `conn` to the now-typed helpers.
- **Fix:** Minimal one-line wrap at each `CatalogReader::new` callsite in `src/query/explain.rs:183` and `src/query/table_function.rs:181`: `let borrowed = crate::ddl::read_ffi::BorrowedConnection::new(conn); let reader = CatalogReader::new(&borrowed, probe_catalog_table_present(&borrowed));`. Explicit comment marker that the full wrap-on-entry migration is Plan 03b's job. No semantic change — the wrap is constructed inline and discarded after the helper call.
- **Files modified:** `src/query/explain.rs`, `src/query/table_function.rs`
- **Verification:** `cargo build --features extension` exit 0; `cargo test --lib --features extension` 859/859.
- **Committed in:** `702556a` (Task 2 commit)

---

**Total deviations:** 1 auto-fixed (Rule 3 — blocking compile)
**Impact on plan:** Necessary to ship Task 2 without a half-broken type system. Surface area is tiny (4 lines added across 2 files); Plan 03b's full wrap-on-entry migration can collapse these minimum-touch sites cleanly.

## Issues Encountered

- **Task 1 initial overreach:** First draft of read_ffi.rs migrated `probe_catalog_table_present` to take `&BorrowedConnection` together with the newtype addition. Build under `--features extension` then failed at all 17 read-side callsites — the helper change was Task 2's scope, not Task 1's. Reverted the signature change in Task 1, kept only the newtype + doctest. Task 2 then re-applied the signature change with all callers updated atomically.

## User Setup Required

None.

## Next Phase Readiness

**Plan 03b ready:** consumes the canonical BorrowedConnection newtype + migrated helper signatures. Plan 03b's surface = the 2 query/ dispatcher sites (`sv_semantic_view_bind_rust` and `sv_explain_semantic_view_bind_rust`) + the AST guard extension in `tests/no_long_lived_conn.rs` for `duckdb_disconnect` walks.

**Wave 1 blocker (cross-plan):** `just test-all` quality gate is not green until Plan 02b lands the Rust call-site cascade for the C ABI rework from Plan 02a. This plan is independent of 02b — `cargo build --features extension` and `cargo test --lib` both green here. The full `just test-all` verification is correctly deferred to the Wave 2 verifier dispatch.

**Threat model (T-65.1-03a-01 — Tampering: Stack-Connection lifetime UB via duckdb_disconnect on borrowed handle):** Mitigated by the BorrowedConnection type-level guard. Defence in depth via `tests/no_long_lived_conn.rs` AST walk extension lives in Plan 03b.

**Threat model (T-65.1-03a-02 — contributor reintroduces duplicate probe_catalog_table_present after dedup):** Accepted. Single canonical home in `src/ddl/read_ffi.rs` + import-based call sites make any divergence visible at code review. No structural guard added (LOW likelihood, code-review caught).

## Self-Check: PASSED

Verified:
- `src/ddl/read_ffi.rs` contains `#[repr(transparent)] pub struct BorrowedConnection(ffi::duckdb_connection);` ✓
- `impl BorrowedConnection` has exactly 3 methods: `unsafe fn new`, `fn as_raw`, `fn is_null` ✓
- Zero `impl X for BorrowedConnection` trait impls (`grep -E "impl\s+\w+\s+for\s+BorrowedConnection" src/ddl/read_ffi.rs` → 0) ✓
- compile_fail doctest with `let mut b: BorrowedConnection = unsafe { std::mem::zeroed() };` + `unsafe { libduckdb_sys::duckdb_disconnect(&mut b) }` present ✓
- Doctest passes (`cargo test --doc --features extension BorrowedConnection` → 1/1) ✓
- Manual rustc error verified: `expected '*mut *mut _duckdb_connection', found '&mut BorrowedConnection'` (type mismatch, not mutability) ✓
- 15 `BorrowedConnection::new(conn)` call sites in `src/ddl/` (one per dispatcher) ✓
- Zero `conn.is_null()` inside dispatcher bodies — all replaced with `borrowed.is_null()` ✓
- `probe_catalog_table_present(borrowed: &BorrowedConnection)` signature ✓
- `CatalogReader::new(borrowed: &BorrowedConnection, ...)` signature ✓
- Zero `fn probe_catalog_table_present` or `fn write_err` definitions in `src/ddl/list.rs` ✓
- Both `sv_list_semantic_views_bind_rust` (line 95) + `sv_list_terse_semantic_views_bind_rust` (line 247) wrap `conn` into `BorrowedConnection` and import canonical helpers ✓
- `cargo build --features extension` exit 0 ✓
- `cargo test --lib --features extension` exit 0 (859/859) ✓
- Both task commits exist (`b400cd8` Task 1, `702556a` Task 2) ✓

## Threat Flags

None — this plan introduces no new network endpoints, auth paths, file access, or schema changes at trust boundaries. The newtype tightens an existing trust boundary (Rust ↔ borrowed C++ handle) without adding new surface.

---
*Phase: 65.1-phase-65-code-review-remediation*
*Completed: 2026-05-25*
