---
phase: 65.1-phase-65-code-review-remediation
plan: 03b
subsystem: infra
tags: [rust, ffi, type-safety, newtype, borrow-contract, duckdb, ast-walker, dev-test, defence-in-depth]

# Dependency graph
requires:
  - phase: 65.1-phase-65-code-review-remediation
    provides: "65.1-03a — BorrowedConnection newtype + 15 ddl/ dispatchers migrated to wrap-on-entry; query/ side left with placeholder one-line wraps at CatalogReader::new callsites"
  - phase: 65.1-phase-65-code-review-remediation
    provides: "65.1-02b — Rust call-site cascade for the C ABI rework; test-all is runnable post-Wave-2"
provides:
  - "WR-05 17-site BorrowedConnection migration COMPLETE: the 2 remaining query/ dispatchers (sv_explain_semantic_view_bind_rust, sv_semantic_view_bind_rust) wrap conn into BorrowedConnection on the first body line"
  - "collect_explain_lines + try_infer_schema helper signatures accept &BorrowedConnection (try_infer_schema return type stays Option<...> — Plan 11 will stack the Result change on top)"
  - "tests/no_long_lived_conn.rs::no_duckdb_disconnect_anywhere_in_src — defence-in-depth AST walker (D-11) walking every src/*.rs file for duckdb_disconnect calls, with explicit allow-list for owned-handle Drop sites (RawDb test fixture)"
  - "walkdir = \"2\" added to [dev-dependencies] in Cargo.toml"
affects: []

# Tech tracking
tech-stack:
  added: ["walkdir = \"2\" (dev-dependencies)"]
  patterns:
    - "Wrap-on-entry mirroring src/ddl/list.rs:81-103 — extern \"C\" FFI signature stays raw duckdb_connection, body wraps into BorrowedConnection as the first executable statement"
    - "AST-walk defence-in-depth: walkdir + syn::visit::Visit walking all of src/ for duckdb_disconnect call expressions; allow-list constant for documented OWN-and-release sites (RawDb::Drop)"

key-files:
  created: []
  modified:
    - "src/query/explain.rs — sv_explain_semantic_view_bind_rust wraps conn into BorrowedConnection on entry; the placeholder Plan 03a one-line wrap removed; collect_explain_lines signature migrated to (&BorrowedConnection, &str); the dispatcher's CatalogReader::new + collect_explain_lines callsites consume &borrowed"
    - "src/query/table_function.rs — sv_semantic_view_bind_rust wraps conn into BorrowedConnection on entry; the placeholder Plan 03a one-line wrap removed; try_infer_schema signature migrated to (&BorrowedConnection, &str) (return type unchanged per plan — Plan 11 stacks Result); the dispatcher's CatalogReader::new + both try_infer_schema callsites consume &borrowed"
    - "tests/no_long_lived_conn.rs — added DisconnectFinder struct + Visit impl + no_duckdb_disconnect_anywhere_in_src test + ALLOWED_DISCONNECT_SITES constant (single deliberate exception: src/lib.rs RawDb::Drop OWN-and-release); existing Finder + init_extension_has_no_duckdb_connect_call test unchanged"
    - "Cargo.toml — walkdir = \"2\" added to [dev-dependencies] with explanatory comment about its role in the AST walker"

key-decisions:
  - "ALLOWED_DISCONNECT_SITES allow-list mechanism for documented OWN-and-release sites — Plan didn't anticipate that RawDb (test fixture in src/lib.rs::test_helpers) legitimately calls duckdb_disconnect on a handle it owns via paired duckdb_open + duckdb_connect. The allow-list captures the exception explicitly with a clear comment instead of weakening the walker scope. Each entry must be justified at code review."
  - "try_infer_schema return type left as Option<...> — Plan 11 (WR-08 / D-15) explicitly stacks the Result<_, String> change on top of this plan's signature change. Avoids brittle byte-diff coupling with Plan 11's diagnostic-plumbing work."
  - "DisconnectFinder walks the entire src/ tree (not just src/ddl/ + src/query/) to maximise blast radius — defence in depth is meaningless if narrowed to the same scope the type-level guard already covers."
  - "Walker SKIPS unparseable files rather than panicking — preserves the real signal (any duckdb_disconnect call) over the noise of a hypothetical syntactic regression in a sibling file."

patterns-established:
  - "Allow-list constant for documented exceptions to AST-walk guards — keeps the guard tight without hard-coding scope exclusions; review burden naturally surfaces at the allow-list edit"
  - "Two-layer FFI contract enforcement: type-level newtype (D-10) + AST walker (D-11) — neither alone is sufficient (newtype can be bypassed by holding a raw duckdb_connection; walker can be evaded by `use … as` aliasing) but the combination raises the bar substantially"

requirements-completed:
  - REM-WR-05

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

# Phase 65.1 Plan 03b: BorrowedConnection (D-10 query/ half) + AST guard extension (D-11) Summary

**Completes the WR-05 17-site BorrowedConnection migration started in Plan 03a; adds defence-in-depth AST walker for `duckdb_disconnect` calls anywhere in `src/` with explicit allow-list for owned-handle Drop sites.**

## Performance

- **Duration:** ~18 min
- **Started:** 2026-05-25T18:00:00Z (approx)
- **Completed:** 2026-05-25T18:18:00Z (approx)
- **Tasks:** 2
- **Files modified:** 4

## Accomplishments

- 2 query/ dispatcher sites (`sv_explain_semantic_view_bind_rust` at `src/query/explain.rs:98` and `sv_semantic_view_bind_rust` at `src/query/table_function.rs:101`) now wrap the raw FFI `conn` into a `BorrowedConnection` on the first body line of the `catch_unwind` closure. The placeholder Plan-03a one-line wraps at the `CatalogReader::new` callsites were removed; both dispatchers now flow `&borrowed` through every helper call. The WR-05 17-site migration is COMPLETE across `src/ddl/` (15 sites, Plan 03a) + `src/query/` (2 sites, this plan).
- Helper functions migrated:
  - `src/query/explain.rs::collect_explain_lines(&BorrowedConnection, &str) -> Vec<String>` — uses `borrowed.as_raw()` for `execute_sql_raw`.
  - `src/query/table_function.rs::try_infer_schema(&BorrowedConnection, &str) -> Option<(Vec<String>, Vec<ffi::duckdb_type>)>` — signature only; the return-type promotion to `Result<_, String>` per WR-08 D-15 is explicitly Plan 11's job and stacks cleanly on this change.
- `tests/no_long_lived_conn.rs` extended with `DisconnectFinder` + `no_duckdb_disconnect_anywhere_in_src` test that walks every `.rs` file under `src/` looking for any call expression whose terminal path segment is `duckdb_disconnect`. The walker uses the same `syn::visit::Visit` pattern as the existing `Finder`; results are filtered through a small `ALLOWED_DISCONNECT_SITES` allow-list. Existing `Finder` + `init_extension_has_no_duckdb_connect_call` test are unchanged in code and in behaviour.
- `Cargo.toml` `[dev-dependencies]` grew `walkdir = "2"` with a comment explaining its role.
- Quality gates: `cargo build` (default features) exit 0; `cargo build --no-default-features --features extension` exit 0; `cargo test --lib` 844/844; `cargo test --test no_long_lived_conn` 2/2 (both Finder and DisconnectFinder pass); `just build` exit 0; `cargo nextest run` 943/943.

## Task Commits

Each task was committed atomically:

1. **Task 1: Migrate 2 query/ dispatchers + helper functions** — `399ead2` (refactor)
2. **Task 2: Extend tests/no_long_lived_conn.rs with DisconnectFinder** — `2b5ac60` (test)

## Files Created/Modified

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

- **query/ dispatcher full wrap-on-entry (2 files, 2 dispatcher sites + 2 helper signatures):** `src/query/{explain,table_function}.rs`
- **AST guard extension (1 test file, 1 build manifest):** `tests/no_long_lived_conn.rs`, `Cargo.toml`

## Decisions Made

All architectural decisions inherited from the plan's `<interfaces>`, PATTERNS.md (lines 408-461 for wrap-on-entry, lines 551-633 for the DisconnectFinder shape), and CONTEXT D-10 / D-11. The frontmatter `key-decisions` block documents implementation-time refinements:

- ALLOWED_DISCONNECT_SITES emerged from running the first draft and discovering RawDb's legitimate self-owned-handle Drop. Plan didn't pre-anticipate this; the allow-list captures it explicitly without weakening scope.
- Helper signature changes to `&BorrowedConnection` only (not Result return type) — defers Plan 11's diagnostic-plumbing work cleanly.

## Deviations from Plan

### Auto-fixed Issues

**1. [Rule 3 — Blocking] RawDb test fixture's Drop calls `ffi::duckdb_disconnect` legitimately**

- **Found during:** Task 2 first verification (`cargo test --test no_long_lived_conn`).
- **Issue:** The plan's success criterion is `no_duckdb_disconnect_anywhere_in_src` passing — i.e., zero `duckdb_disconnect` calls anywhere under `src/`. But `src/lib.rs::test_helpers::RawDb` is a legitimate test-only OWN-and-release pattern: it allocates a `duckdb_database` + `duckdb_connection` via paired `duckdb_open` + `duckdb_connect` in `open_in_memory()`, then releases the pair in `Drop` via `duckdb_disconnect` + `duckdb_close`. This is NOT a BORROW-contract violation (the borrowed handle is owned by a C++-side `Connection probe(*context.db)` — RawDb's handle is owned by RawDb itself).
- **Fix:** Added `ALLOWED_DISCONNECT_SITES` constant — an array of `(file_path, full_call_path)` tuples filtered out of the walker's findings before the assertion. Single entry: `("src/lib.rs", "ffi::duckdb_disconnect")` documenting the RawDb::Drop site with a clear comment explaining the OWN-and-release pattern. Each future entry would require justification at code review, so the allow-list is bounded.
- **Files modified:** `tests/no_long_lived_conn.rs`.
- **Verification:** `cargo test --test no_long_lived_conn` passes both tests (2/2). The walker still fails on any new (unallowed) `duckdb_disconnect` call site in `src/`.
- **Committed in:** `2b5ac60` (Task 2 commit).

**Total deviations:** 1 auto-fixed (Rule 3 — blocking AST guard test).

**Impact on plan:** Net positive — the allow-list is a small, well-documented mechanism that makes the guard simultaneously tight (still fails on the regression case) and honest (acknowledges legitimate OWN-and-release fixtures). The alternative (scoping the walker to `src/ddl/` + `src/query/` only) would have eliminated this 1 case but also reduced blast radius; the current shape is strictly better.

## Issues Encountered

- **Pre-existing sqllogictest runner flake on `execute_restart`** — `just test-all` exits non-zero with multiple FAILED entries (e.g. `phase651_bind_probe_hard_errors.test`, `phase651_null_name_inputs.test`, `phase651_yaml_filesystem_access_gating.test`), each with stack trace:
  ```
  File "duckdb_sqllogictest/result.py", line 947, in execute_restart
      con.execute(query)
  _duckdb.InvalidInputException: Invalid Input Error: This setting is not adjustable by a user
  ```
  The failing tests have NO explicit `restart` statements (grep returns zero matches); the runner is attempting an internal between-test reset. Each individual test in the run-cascade reports `SUCCESS` immediately before the failure. The sibling executor for Plan 02b flagged the same flake (see sibling-context handoff). Confirmed unrelated to BorrowedConnection migration: `cargo build`, `cargo test --lib`, `just build`, and `cargo nextest run` all pass cleanly (943/943 Rust tests). Surfacing for milestone-finisher follow-up — likely a duckdb-sqllogictest version-mismatch issue or DuckDB-1.10.502 upstream tightening of the `restart`-internal setting toggle. Tracked here for the Wave 2 verifier; out of scope for this plan.
- **`just test-all` non-zero exit** — direct consequence of the above runner flake. `cargo nextest run` portion (943/943) and `just build` portion (exit 0) both pass; only the sqllogictest portion fails. Recommend the verifier examine the runner-level failure rather than treating this plan's diff as a regression source.

## User Setup Required

None.

## Next Phase Readiness

**WR-05 migration complete:** 17/17 read-side dispatcher sites consume `BorrowedConnection` via wrap-on-entry. The type-level guard (D-10) is in force everywhere; the AST-level guard (D-11) is the second layer.

**Plan 11 (WR-08) ready:** `try_infer_schema` already accepts `&BorrowedConnection`. Plan 11 stacks the return-type change `Option<_> -> Result<_, String>` and plumbs `e.to_string()` through `write_err` per `<interfaces>` lines 470-501 of PATTERNS.md. No coupling with this plan beyond the signature precondition.

**Threat model:**
- **T-65.1-03b-01 (Tampering: contributor introduces ffi::duckdb_disconnect on raw conn parameter):** Mitigated. The DisconnectFinder catches the regression even if the BorrowedConnection newtype is somehow bypassed (e.g., contributor reuses the raw `conn` FFI param directly instead of going through `borrowed.as_raw()`). The allow-list is bounded and audited.
- **T-65.1-03b-02 (Tampering: walker mis-identifies use … as alias):** Accept (per plan threat register). Documented limitation aligned with the existing `Finder`'s caveat at the top of `tests/no_long_lived_conn.rs`.

## Self-Check: PASSED

Verified:
- `grep -rE "BorrowedConnection::new\\(conn\\)" src/query/` returns 2 matches (one per dispatcher) ✓
- `grep -rE "\\bconn\\.is_null\\(\\)" src/query/` returns zero matches ✓
- `collect_explain_lines(borrowed: &crate::ddl::read_ffi::BorrowedConnection, sql: &str)` signature present in src/query/explain.rs ✓
- `try_infer_schema(borrowed: &crate::ddl::read_ffi::BorrowedConnection, sql: &str)` signature present in src/query/table_function.rs ✓
- `cargo build` exit 0 ✓
- `cargo build --no-default-features --features extension` exit 0 ✓
- `cargo test --lib` 844/844 ✓
- `cargo test --test no_long_lived_conn` 2/2 (init_extension_has_no_duckdb_connect_call + no_duckdb_disconnect_anywhere_in_src) ✓
- `walkdir = "2"` present in Cargo.toml [dev-dependencies] ✓
- `struct DisconnectFinder` defined in tests/no_long_lived_conn.rs ✓
- `fn no_duckdb_disconnect_anywhere_in_src()` defined ✓
- Test body uses `walkdir::WalkDir::new("src/")` and `syn::parse_str::<syn::File>(...)` ✓
- Failure message string references `BorrowedConnection` ✓
- Existing `Finder` + `init_extension_has_no_duckdb_connect_call` unchanged in behaviour (test still PASS) ✓
- `cargo nextest run` 943/943 ✓
- `just build` exit 0 ✓
- Both task commits exist: `399ead2` Task 1, `2b5ac60` Task 2 ✓

## Threat Flags

None — this plan introduces no new network endpoints, auth paths, file access, or schema changes at trust boundaries. The two layers of guards (newtype + AST walker) tighten existing trust boundaries (Rust ↔ borrowed C++ handle) without adding new surface.

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