---
phase: 65.1-phase-65-code-review-remediation
plan: 10
subsystem: ffi
tags: [bridge-contract, borrow-contract, static-assert, runtime-probe, wr-06, error-buf, cpp-shim, ffi-signature]

# Dependency graph
requires:
  - phase: 65.1-phase-65-code-review-remediation
    provides: "Plan 02a's `(error_buf, error_buf_len)` ABI on sv_register_table_function + sv_register_scalar_function; Plan 02b's `decode_register_err_buf` helper + 17-site cascade in src/lib.rs::init_extension; Plan 08's converged single-shot TF exec callback shapes (no diff conflict with the registration helper call inside sv_register_parser_hooks)"
provides:
  - "File-scope `static_assert(sizeof(duckdb_connection) == sizeof(void*))` near the top of cpp/src/shim.cpp; catches DuckDB ABI drift that would change duckdb_connection's size"
  - "Load-time runtime BORROW-contract probe inside sv_register_parser_hooks (after wrapper unwrap, before any state mutation) — constructs Connection probe(db), bridge-casts, round-trips SELECT 1; failure refuses the LOAD entirely with `bridge contract broken — ...` written into error_buf"
  - "New `(char *error_buf, size_t error_buf_len)` trailing pair on sv_register_parser_hooks's C ABI (cpp/src/shim.hpp + cpp/src/shim.cpp + extern decl in src/lib.rs), matching the convention established by sv_parser_override_rust, the 17 read-side dispatchers, and the registration helpers"
  - "All previously stderr-only failure paths (sv_make_override_context null, helper-TF registration failure, std::exception catch) now route diagnostics through the new error_buf so ADBC/JDBC/Python callers see the underlying message instead of bare `Failed to register parser hooks`"
affects:
  - 65.1-plan-12  # Plan 12 (WR-09 parser-hook idempotence) inherits the new error_buf parameter when adding the idempotence check inside sv_register_parser_hooks

# Tech tracking
tech-stack:
  added: []
  patterns:
    - "Compile-time `static_assert` on pointer-sized FFI handle representation at file scope. The first occurrence in this repo; pattern is now established for any future FFI bridge that relies on layout equivalence."
    - "Load-time runtime probe pattern: stack-construct a Connection, bridge-cast to the C-API opaque handle, execute a trivial round-trip query (`SELECT 1`), check rc; on failure write diagnostic via snprintf into the caller's error_buf and refuse the LOAD. Catches representation drift the static_assert cannot (e.g. wrapping the pointer in a struct with the same layout but different semantics)."
    - "Probe-failure surfacing convention: probe failure (a layout-invariant violation, not a user error) uses the same `error_buf` channel as registration failures — D-13 explicitly rejects a separate error channel. The clear `bridge contract broken — duckdb_connection layout assumption failed at load time. DuckDB ABI may have changed; refusing to load semantic_views extension.` wording is the pinned diagnostic text."

key-files:
  created: []
  modified:
    - cpp/src/shim.cpp  # File-scope static_assert added; sv_register_parser_hooks signature updated to carry trailing `(error_buf, error_buf_len)` pair; runtime probe block inserted after wrapper unwrap; all four previously stderr-only failure paths routed through the new buffer
    - cpp/src/shim.hpp  # Extern declaration of sv_register_parser_hooks updated to match the new C ABI
    - src/lib.rs  # Extern declaration in `extension` module updated to match; the single call site in init_extension allocates a 1024-byte stack buffer, passes it to the C helper, and on failure decodes via `decode_register_err_buf` into the returned Err(...)

key-decisions:
  - "Static_assert placement: file scope near the top of cpp/src/shim.cpp (line 50), just after `using namespace duckdb;` and before the `extern \"C\"` block of Rust FFI declarations. The location is visible to every translation-unit user, surfaces the BORROW-contract contract via the adjacent docstring, and is uniformly evaluated regardless of which exec/bind callback the compiler is processing. An alternative location adjacent to any of the ~20 individual bridge-cast call sites was rejected — central placement DRY-encodes the invariant."
  - "Runtime probe placement: TOP of sv_register_parser_hooks, immediately after the wrapper unwrap that produces `auto &db = *wrapper->database->instance;`. Specifically BEFORE `sv_make_override_context()`, before the `ParserExtension::Register` call, and before the helper-TF registration. The point is to fail fast — if the bridge is broken, no semantic_views state (parser_override hook, helper TF, OverrideContext heap allocation) should survive the failed LOAD."
  - "Probe failure path writes via snprintf into `error_buf`, returns false, and does NOT call `duckdb_destroy_result` on the failure branch's stray (uninitialised by failure) `duckdb_result r` only when `rc != DuckDBSuccess`. Reviewing the DuckDB C API behaviour: `duckdb_query` populates `duckdb_result` even on error (to carry the error message). The safe contract is to always call `duckdb_destroy_result(&r)` after `duckdb_query` regardless of rc. The probe path now calls destroy on both branches for correctness; in the failure branch destroy precedes the snprintf so the failure surfacing cannot leak resources if snprintf were to throw."
  - "Routing previously stderr-only failures (sv_make_override_context null, helper-TF registration failure, std::exception catch) through the new error_buf was not strictly required by the plan but is the natural completion of WR-02's intent — the plan explicitly cites D-09 as the convention that ADBC/JDBC/Python may have redirected stderr. Leaving even one error path on stderr would re-introduce the silent-failure regression WR-02 was created to close. The diff is small and the safety win is real."
  - "Caller-side allocation in src/lib.rs uses the same per-site `let mut error_buf = [0u8; 1024];` shape as the 17 registration call sites Plan 02b established. The `c_char` and `CStr` imports are already in scope from Plan 02b; no new use statements needed."

patterns-established:
  - "FFI-side layout-invariant pair pattern: file-scope `static_assert` (catches size drift) + load-time runtime probe (catches representation drift). Future FFI bridges that rely on layout equivalence (none currently planned) should adopt this exact pair. The convention is documented inline at the static_assert location with a pointer to 65-REVIEW.md WR-06 and Phase 65.1 D-12."
  - "Probe-failure error wording template: `bridge contract broken — <subject> layout assumption failed at load time. DuckDB ABI may have changed; refusing to load semantic_views extension.` — pinned for any future probe of a similar layout invariant."

requirements-completed:
  - REM-WR-06

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

# Phase 65.1 Plan 10: BORROW-Contract Bridge Pin (WR-06) Summary

**Closed WR-06 by adding two complementary guards on the `reinterpret_cast<duckdb_connection>(Connection*)` BORROW-contract used at ~20 call sites in cpp/src/shim.cpp: (1) a file-scope `static_assert(sizeof(duckdb_connection) == sizeof(void*), ...)` that catches DuckDB ABI drift changing the handle's size, and (2) a load-time runtime probe inside `sv_register_parser_hooks` that bridge-casts a stack `Connection` and round-trips `SELECT 1`, refusing the LOAD with `bridge contract broken — ...` if the cast does not yield a working handle. The signature update added a trailing `(error_buf, error_buf_len)` pair to `sv_register_parser_hooks` (matching the convention established by Plan 02a/02b across the 17 read-side dispatchers); the Rust caller in `src/lib.rs::init_extension` consumes the buffer via the existing `decode_register_err_buf` helper. All previously stderr-only failure paths in `sv_register_parser_hooks` (sv_make_override_context null, helper-TF registration failure, std::exception catch) also route through the new buffer.**

## Performance

- **Duration:** ~25 min
- **Tasks:** 1
- **Files modified:** 3 (cpp/src/shim.cpp, cpp/src/shim.hpp, src/lib.rs)
- **LOC delta:** +130 / −24

## Accomplishments

- **Compile-time guard landed.** File-scope `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 / Phase 65.1 D-12");` at cpp/src/shim.cpp:50. The message references both the in-repo BORROW-contract docs and the upstream review document so a future contributor hit by an ABI change has the bread-crumb trail without external lookup. Adjacent docstring block (lines 36-49) explains the runtime-probe complement and points to the canonical bridge-cast call sites.

- **Runtime probe landed.** Inside `sv_register_parser_hooks`, immediately after the wrapper unwrap and BEFORE any user-visible state mutation:
  ```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) {
      duckdb_destroy_result(&r);
      if (error_buf != nullptr && error_buf_len > 0) {
          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);
  ```
  The probe runs once per LOAD; the `Connection` is stack-allocated and its dtor runs at scope exit. The bridged `handle` becomes invalid after the round-trip but is not touched again.

- **Signature update propagated end-to-end.** Three files updated in lock-step:
  - cpp/src/shim.cpp: `bool sv_register_parser_hooks(duckdb_database db_handle, char *error_buf, size_t error_buf_len)`.
  - cpp/src/shim.hpp: matching declaration with docstring citing Plan 10 / D-12 / D-13.
  - src/lib.rs: `fn sv_register_parser_hooks(db_handle: ffi::duckdb_database, error_buf: *mut c_char, error_buf_len: usize) -> bool;` extern declaration; call site allocates `let mut error_buf = [0u8; 1024];` and surfaces failure via `decode_register_err_buf(&error_buf)`.

- **Pre-existing stderr-only failure paths converted.** The function previously had three failure paths that printed to stderr:
  1. `sv_make_override_context()` returned null;
  2. Helper-TF (`__sv_compute_create_from_yaml`) registration failed (forwarded from the local `helper_err` buffer);
  3. `catch (const std::exception &e)` at the function bottom.

  All three now write into `error_buf` (with defensive `error_buf != nullptr && error_buf_len > 0` guard) — eliminating the last stderr-only failure path in the registration cascade, which is the natural completion of WR-02's intent (D-09: ADBC/JDBC/Python may have redirected stderr; the buffer is the only ABI-stable channel).

- **Quality gates.** `cargo test` (default `bundled` feature) reports 942/942 pass; `just build` exits 0; 11 sqllogictest files in the runner queue all PASS individually before hitting the pre-existing upstream `execute_restart` runner flake at `phase42_persistence.test` (logged in Plan 02b SUMMARY and Plan 08 SUMMARY — independent of Plan 10's changes; baseline-confirmed via stash + rebuild). Behavioural extension-load coverage is provided by integration tests: `test_adbc_transactions.py` 6/6, `test_multi_db_isolation.py` 3/3 (notably the "fifty DB open-close" test exercises 50 sequential LOADs, all hitting the new probe), `test_readonly_load.py` 12/12.

## Task Commits

| Task | Name | Commit | Files |
|------|------|--------|-------|
| 1 | Add file-scope static_assert + load-time runtime probe (sv_register_parser_hooks signature update) | `7140e41` | cpp/src/shim.cpp, cpp/src/shim.hpp, src/lib.rs |

_The plan-completion metadata commit lands after this SUMMARY is written._

## Files Created/Modified

### Modified

- **`cpp/src/shim.cpp`** — Inserted the BORROW-contract documentation block (lines 36-49) and the `static_assert(sizeof(duckdb_connection) == sizeof(void*), ...)` (line 50) at file scope just after `using namespace duckdb;`. Updated `sv_register_parser_hooks`'s signature to `(duckdb_database db_handle, char *error_buf, size_t error_buf_len)`. Inserted the runtime probe block immediately after the wrapper unwrap (lines ~2602-2632, before `sv_make_override_context`). Rewrote the three pre-existing stderr-only failure paths to write via `snprintf` into the new buffer; the docstring above the helper-TF registration block updated to remove the Plan 10 placeholder ("territory" → "now own[s]" plus a forwarding-comment justification).

- **`cpp/src/shim.hpp`** — Updated the `sv_register_parser_hooks` extern declaration to match the new C ABI; expanded the docstring with a Phase 65.1 Plan 10 (WR-06 D-12/D-13) reference paragraph that explains the trailing pair surfaces both BORROW-contract probe failure and helper-TF registration failure via the ABI-stable channel.

- **`src/lib.rs`** — Updated the `sv_register_parser_hooks` extern declaration (lines ~331-341) to carry the trailing `(error_buf, error_buf_len)` pair. Rewrote the call site (lines ~547-577) to allocate a 1024-byte stack buffer, pass it to the helper, and decode via `decode_register_err_buf` on failure for the returned Err message — same shape as the 17 other registration call sites Plan 02b established. The pre-existing docstring above the extern block (which previously said "Plan 06: signature slimmed…  Plan 10 territory") was rewritten to cite the now-landed Plan 10 update.

## Decisions Made

See `key-decisions` in frontmatter above.

## Deviations from Plan

### Auto-fixed Issues

**1. [Rule 2 — Critical functionality] Routed three pre-existing stderr-only failure paths through the new error_buf**

- **Found during:** Task 1 (drafting the signature update — naturally surfaced that the existing failure paths in `sv_register_parser_hooks` were inconsistent with the WR-02 convention).
- **Issue:** The plan's `<action>` section focused on the runtime probe's failure path writing into `error_buf`, but `sv_register_parser_hooks` already had three pre-existing stderr-only failure paths (sv_make_override_context null check, helper-TF registration failure, std::exception catch at bottom). Leaving them on stderr would have re-introduced exactly the silent-failure regression that WR-02 was created to close — ADBC/JDBC/Python callers see "Failed to register parser hooks via C++ helper" with no underlying diagnostic, the same surface WR-02 closed for the 17 read-side dispatchers + the registration helpers in Plan 02a/02b.
- **Fix:** Rewrote all three failure paths to write via `snprintf` into the new `error_buf` (with defensive `error_buf != nullptr && error_buf_len > 0` guard, matching the pattern at every other registration helper). The Rust caller's `decode_register_err_buf` then surfaces the diagnostic into the returned `Err(...)` so the user sees the underlying message. The stderr fprintf calls were removed in the same edit; the buffer is the single channel now.
- **Files modified:** cpp/src/shim.cpp
- **Committed in:** `7140e41` (Task 1 commit; bundled with the primary signature change)

### Acceptance-criterion deviations (documented, not auto-fixed)

**The literal plan-verification `just test-sql` exit-0 acceptance criterion is NOT met** because of a pre-existing upstream sqllogictest runner flake (`execute_restart` → `InvalidInputException: This setting is not adjustable by a user`) at `phase42_persistence.test`. This flake:

- Was documented in `65.1-02b-SUMMARY.md` "Issues Encountered" and `65.1-08-SUMMARY.md` quality-gates section as pre-existing.
- Was independently re-confirmed at the Plan 10 starting commit `501f5e8` by stash-and-baseline-build — identical failure at identical test file, identical line, identical error wording.
- Is structurally independent of Plan 10's changes: the failure happens inside the duckdb_sqllogictest Python runner's `execute_restart` method when it attempts to restore a non-adjustable DuckDB setting after a `restart` statement, with no involvement of any semantic_views code.

The 11 sqllogictest files that run BEFORE the flake all PASS with Plan 10's changes in place, confirming the extension still loads cleanly across every test scenario the runner is able to execute. Behavioural extension-load coverage is fully provided by `cargo test` (942/942) + `test_adbc_transactions.py` (6/6) + `test_multi_db_isolation.py` (3/3, 50 sequential LOADs) + `test_readonly_load.py` (12/12). Per the executor scope-boundary rule, the upstream runner flake is out of scope for Plan 10; the phase-level verifier (or a follow-up infrastructure task) will own it.

`just ci` was NOT run because (a) it transitively invokes `just test-all` which transitively invokes `just test-sql`, hitting the same pre-existing flake; (b) the linting/clippy/fmt portions of `just ci` are exercised through the pre-commit hook on the task commit. The commit passed pre-commit (no `--no-verify`), confirming `cargo fmt --check` + clippy pedantic + cargo-deny gates green at the touched-file level.

---

**Total deviations:** 1 auto-fixed (Rule 2 critical-functionality completion). 1 acceptance-criterion deviation documented (pre-existing upstream runner flake, scope-bounded out per the executor rules).

## Issues Encountered

- **Pre-existing upstream sqllogictest runner flake on `execute_restart`.** Identical to the issue logged in 65.1-02b SUMMARY and 65.1-08 SUMMARY. Baseline-confirmed independent of Plan 10's changes via stash + rebuild + re-run. Out of scope per executor rules.

## Verification Evidence

- **Plan acceptance-criterion grep checks all pass:**
  - `grep -nE "static_assert\(sizeof\(duckdb_connection\)" cpp/src/shim.cpp` returns a primary match at line 50 (file-scope guard) plus a comment match at line 2607 (the docstring inside `sv_register_parser_hooks` citing the static_assert) — the single primary `static_assert` is at file scope as the plan requires.
  - `grep -nE "bridge contract broken" cpp/src/shim.cpp` returns the runtime-probe diagnostic at line 51 (the docstring above the static_assert) and at line 2627 (the snprintf inside the probe failure branch). The single live snprintf is at line 2627 inside `sv_register_parser_hooks` as the plan requires.
  - `sv_register_parser_hooks` signature carries the `(char *error_buf, size_t error_buf_len)` pair — confirmed in both cpp/src/shim.cpp:2576-2578 and cpp/src/shim.hpp:38-44.
- **The runtime probe block** constructs `Connection probe(db)`, reinterpret_casts to `duckdb_connection`, calls `duckdb_query(handle, "SELECT 1", &r)`, checks the return code, and destroys the result on both branches.
- **`just build` exits 0** (debug + extension build chain green).
- **`cargo test` exits 0** — 942 tests pass across all integration/proptest/doctest targets.
- **`test/integration/test_adbc_transactions.py`:** 6/6 PASS — confirms the LOAD path through the new probe works under ADBC.
- **`test/integration/test_multi_db_isolation.py`:** 3/3 PASS — the "fifty DB open-close" test exercises 50 sequential LOADs, all hitting the new probe; RSS delta 0.0 MB over 50 iterations confirms no leak in the per-LOAD Connection allocation.
- **`test/integration/test_readonly_load.py`:** 12/12 PASS — read-only LOAD + repeated-load-close paths all green.
- **11 sqllogictest files pass individually** before the pre-existing upstream runner flake bites (the flake is independent of Plan 10's changes; baseline-confirmed).

## Threat Surface Scan

- **T-65.1-10-01 (Tampering / size drift): mitigated** by the file-scope `static_assert` — any DuckDB minor version that changes `sizeof(duckdb_connection)` fails the build outright, with the static_assert message pointing the maintainer at the BORROW-contract docs.
- **T-65.1-10-02 (Tampering / representation drift): mitigated** by the load-time runtime probe — if a future DuckDB version wraps the `duckdb_connection` pointer in a struct (same size, different representation), the probe's `duckdb_query` call fails and the LOAD is refused with a clear diagnostic. The bridge-cast pattern at the ~20 bind/exec callback sites is identical to the probe's; if the probe works, every callback works (and vice versa).
- **T-65.1-10-03 (Denial of service / spurious probe failure): accepted** — the probe is mechanically identical to the bind callbacks' bridge cast. If it spuriously fails, the entire extension's read path would be unusable on the next user query anyway; failing the LOAD with a clear message is the right disposition (D-13).
- No NEW threat surface introduced. The error_buf flow inherits the trust model established by Plan 02a/02b (caller-owned buffer, snprintf bounds enforced, defensive null guard). The runtime probe's `Connection` is stack-allocated and never escapes scope.

## User Setup Required

None — all changes are extension-internal (C++ shim + Rust extern declaration + caller site).

## Next Phase Readiness

- **Plan 11 (any plan that touches sv_register_parser_hooks)** inherits the new `(error_buf, error_buf_len)` parameter. The convention is now: any future failure path inside this function MUST write through `error_buf` (with the defensive `nullptr && len > 0` guard) rather than stderr.
- **Plan 12 (WR-09 parser-hook idempotence)** — the planned idempotence check inside `sv_register_parser_hooks` can use the new error_buf channel directly when surfacing any check-related diagnostic (e.g. "could not query existing parser_extensions list"). No additional plumbing needed.
- **Phase-level verifier** should treat the pre-existing upstream sqllogictest runner flake (`execute_restart` → "This setting is not adjustable by a user") as out-of-scope per the executor rules; it has been baseline-confirmed independent of Plan 10's changes and is logged in 65.1-02b and 65.1-08 SUMMARIES as a known follow-up.

## Self-Check: PASSED

- `cpp/src/shim.cpp` exists and contains the file-scope `static_assert(sizeof(duckdb_connection)` (line 50) — verified via grep.
- `cpp/src/shim.cpp` contains the snprintf'd `bridge contract broken` diagnostic inside `sv_register_parser_hooks` (line 2627) — verified via grep.
- `cpp/src/shim.cpp::sv_register_parser_hooks` signature carries `(char *error_buf, size_t error_buf_len)` — verified by inspection.
- `cpp/src/shim.hpp` declaration matches — verified by inspection.
- `src/lib.rs` extern declaration matches; the call site allocates `let mut error_buf = [0u8; 1024];` and decodes via `decode_register_err_buf` on failure — verified by inspection.
- Task 1 commit `7140e41` exists in `git log` — verified via `git log --oneline -3`.
- `just build` exits 0; `cargo test` exits 0; `test_adbc_transactions.py` 6/6; `test_multi_db_isolation.py` 3/3; `test_readonly_load.py` 12/12 — all verified.

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