# Phase 65.1: Phase 65 Code Review Remediation - Context

**Gathered:** 2026-05-25
**Status:** Ready for planning
**Source:** `/gsd-discuss-phase 65.1 --assumptions` (decisions confirmed inline)

<domain>
## Phase Boundary

Address the 10 remaining findings from `.planning/phases/65-overridecontext-connection-teardown/65-REVIEW.md`:
- 2 Critical: CR-01 (SQL injection in YAML-file helper TF), CR-02 (double-emit / unbounded-loop hazard in 4 exec callbacks)
- 8 Warnings: WR-02 through WR-09 (WR-01 already fixed inline in commit `ae17f4b` — out of scope)
- 6 Info: IN-01 through IN-06 (folded into this phase per user decision)

Out of scope: WR-01 (closed in `ae17f4b`). Phase 66 (Expansion Qualification + ADBC tests + milestone-finisher) is a separate phase.

</domain>

<decisions>
## Implementation Decisions

### CR-01: SQL injection in `__sv_compute_create_from_yaml`
- **D-01:** Take the **root-cause structural fix** — replace `Connection::Query("SELECT content FROM read_text('<path>')")` with direct DuckDB `FileSystem::OpenFile` via `FileSystem::GetFileSystem(context)`. Eliminates the SQL surface entirely. Preserves `enable_external_access` gating (LocalFileSystem honors the same settings).
- **D-02:** Do NOT take the Prepare/bind workaround. The author's note that Prepare returns a non-materialized result triggering `InternalException` was a symptom of mis-using SQL for what is fundamentally a file-read operation. FileSystem-direct is the correct API.
- **D-03:** Helper TF stays named `__sv_compute_create_from_yaml` (already private-prefixed). Adjacent docstring/comment block updated to reflect the FileSystem call shape.

### CR-02: Double-emit / unbounded-loop hazard
- **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 it has an analogous knob). Failing registration is the right blast-radius — catching it per-call duplicates the check across N exec callbacks.
- **D-06:** Delete the misleading comment `(void)emitted; // single-shot semantics — even without local state we emit once per bind` at cpp/src/shim.cpp:1148.
- **D-07:** Update the stale comment at cpp/src/shim.cpp:830-844 (IN-01) to reflect that `init_local` is wired in. The "flip the registration to use the init_local path" recommendation is removed.

### WR-02: Registration error swallowing
- **D-08:** Rework the C ABI of `sv_register_table_function` and `sv_register_scalar_function` to accept an `error_buf` + `error_buf_len` parameter, the same pattern as `sv_parser_override_rust` and the 17 read-side dispatchers. Write `e.what()` into the buffer on failure. Rust caller in `init_extension` includes it in the error message.
- **D-09:** Do NOT take the stderr-prefix shortcut. ADBC/JDBC/Python callers may have redirected stderr; surfacing through the buffer is the only reliable path.

### WR-05: BORROW contract enforcement
- **D-10:** Take **Option 2 — type-level enforcement**. Introduce `BorrowedConnection` newtype (`#[repr(transparent)] struct BorrowedConnection(ffi::duckdb_connection)`, no `Drop` impl, unsafe-only constructor). Migrate all 17 read-side dispatchers in `src/ddl/*.rs` and `src/query/*.rs` to take `BorrowedConnection` rather than raw `duckdb_connection`. `ffi::duckdb_disconnect` no longer type-checks against it.
- **D-11:** Keep `tests/no_long_lived_conn.rs` AST guard for `duckdb_connect` (defence in depth). Optionally extend with `duckdb_disconnect` walk, but the newtype is the primary mechanism.

### WR-06: Layout-assumption guard
- **D-12:** Add BOTH guards:
  - `static_assert(sizeof(duckdb_connection) == sizeof(Connection*), "...")` at the bridge call site in `cpp/src/shim.cpp`.
  - Runtime probe at extension load: `Connection probe(*wrapper->database->instance); reinterpret_cast<duckdb_connection>(&probe); duckdb_query(handle, "SELECT 1", &r);` — refuse extension load on failure.
- **D-13:** Probe failure surfaces through the WR-02 error-buffer mechanism so users see a clear "bridge contract broken — DuckDB ABI may have changed" message.

### WR-07 / WR-08: Silent type fallbacks → hard errors
- **D-14:** Promote `sv_resolve_output_logical_types` probe-failure fallback (WR-07) 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 (WR-08) to plumb the actual `execute_sql_raw` error through `sv_semantic_view_bind_rust` and surface as `BinderException` with full diagnostic text. No silent VARCHAR fallback for `DUCKDB_TYPE_INVALID`.
- **D-16:** Add CHANGELOG entry noting this behavior change. Add sqllogictests covering: (a) the new error surface, (b) confirming existing valid-query paths still pass.

### WR-03: DROP/ALTER on fresh RO DB error wording
- **D-17:** Take the **outer guard SQL approach** (Option b): wrap the 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` covering DROP and ALTER on a never-bootstrapped RO DB.

### WR-04: Concurrent write coverage
- **D-19:** Add `test/integration/test_concurrent_writes_per_call_conn.py` mirroring the shape of `test_concurrent_reads_per_call_conn.py`. N threads each running mix of CREATE / DROP / ALTER targeting overlapping view names. Assert: no hang, "already exists" errors return to correct caller, final `list_semantic_views()` consistent with success-returning operations.
- **D-20:** Researcher should investigate whether known TECH-DEBT 23 (IF NOT EXISTS race PK violation under contention) requires explicit handling in the test or remains documented behavior.

### WR-09: init_extension partial-failure idempotence
- **D-21:** Make parser-hook registration **check for existing matching hook before appending** to `DBConfig::parser_extensions`. Stale catalog entries (functions registered before a failing 5th) are already absorbed by `ALTER_ON_CONFLICT`.
- **D-22:** If DuckDB's `ParserExtension::Register` API does not expose a way to see the existing list cleanly, fall back to documenting the failure mode in README (per WR-09 Option 3). Researcher should investigate the API surface before planning commits to a specific mechanism.

### IN-02 / IN-03: Delete unused `type_cache` module + `type_id_to_display_name`
- **D-23:** Delete `src/type_cache.rs` and its `lib.rs` registration. Delete `type_id_to_display_name` from `src/query/table_function.rs`. ~225 LOC purge. Do not keep as forward-compat scaffolding — speculative infrastructure is a contributor trap.

### Remaining IN findings
- **D-24:** IN-04 — remove `kind: u8` parameter from `__sv_compute_create_from_yaml` FFI signature. The kind is redundantly encoded in the outer parser_override INSERT shape (`OR IGNORE`/`OR REPLACE`/plain + CASE guard). Slimmer FFI surface.
- **D-25:** IN-05 — add sqllogictest assertions for null name vector inputs: `SELECT read_yaml_from_semantic_view(NULL) IS NULL`, `SELECT get_ddl(NULL, 'x') IS NULL`, `SELECT get_ddl('SEMANTIC_VIEW', NULL) IS NULL`.
- **D-26:** IN-06 — delete `src/ddl/list.rs::probe_catalog_table_present` (module-private duplicate). Use `crate::ddl::read_ffi::probe_catalog_table_present` in `sv_list_semantic_views_bind_rust`. Same for duplicate `write_err` in list.rs.

### CHANGELOG strategy
- **D-27:** WR-07 and WR-08 (silent-fallback → hard error) are user-visible behavior changes. Document as `Changed` entries in `## [Unreleased]` section of CHANGELOG.md. CR-01 noted under `Security`. CR-02 + WR-* + IN-* under `Fixed`.

### Claude's Discretion
- Exact wave grouping (Plans 1-2 are blockers; rest parallelizable — planner decides specifics)
- 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 large but coherent)
- Whether to add Connection RAII helpers beyond BorrowedConnection (not requested but may emerge from research)

</decisions>

<canonical_refs>
## Canonical References

**Downstream agents MUST read these before planning or implementing.**

### Source artifact (the review being remediated)
- `.planning/phases/65-overridecontext-connection-teardown/65-REVIEW.md` — full text of 17 findings (committed in `6c1dbd6`). WR-01 already addressed in `ae17f4b`.

### Phase 65 deliverables (the architecture being patched)
- `.planning/phases/65-overridecontext-connection-teardown/65-VERIFICATION.md` — Phase 65 closeout
- `cpp/src/shim.cpp` — `sv_register_table_function`, `sv_register_scalar_function`, all 17 exec/bind callbacks, BORROW-contract reinterpret_casts
- `cpp/src/shim.hpp` — registration helper signatures (ABI rework target for WR-02)
- `src/lib.rs:450-575` — `init_extension` registration sequence (WR-09 target)
- `src/parse.rs:2119-2126, 1892-1901` — existence guard SQL (WR-03 target)
- `src/query/explain.rs:54-87` — already-fixed `parse_string_list` (WR-01 reference)
- `src/query/table_function.rs:704-729` — `try_infer_schema` (WR-08 target)
- `cpp/src/shim.cpp:2203-2255` — `sv_resolve_output_logical_types` (WR-07 target)
- `cpp/src/shim.cpp:745-756, 825-856, 1023-1059` — CR-01 + CR-02 sites
- `tests/no_long_lived_conn.rs` — structural guard pattern (template for any AST-walk additions)
- `test/integration/test_concurrent_reads_per_call_conn.py` — template for WR-04 concurrent-writes test
- `src/type_cache.rs` — to be deleted (D-23)
- `src/query/table_function.rs:500-568` — `type_id_to_display_name` to be deleted (D-23)
- `src/ddl/list.rs:191-210` — duplicate `probe_catalog_table_present` to be deleted (D-26)
- `src/ddl/read_ffi.rs:57-74` — canonical `probe_catalog_table_present` (kept)

### Project-wide rules
- `CLAUDE.md` — build/test command rules (Rule 1: no bare `tail -N` on long-running commands; Rule 2: sandbox bypass for build/test list). Quality gate is `just test-all`.
- `TECH-DEBT.md` — TECH-DEBT 23 (IF NOT EXISTS race PK violation) is relevant to WR-04 test design.

### DuckDB API references (for researcher)
- DuckDB `FileSystem::GetFileSystem(ClientContext&)` and `FileSystem::OpenFile` (for D-01 CR-01 fix)
- DuckDB `ParserExtension::Register` and `DBConfig::parser_extensions` shape (for D-21 WR-09 fix)
- DuckDB `Connection` lifecycle, `duckdb_connection` C ABI representation (for D-12 WR-06 static_assert)

</canonical_refs>

<code_context>
## Existing Code Insights

### Reusable Assets
- **Error-buffer ABI pattern**: `sv_parser_override_rust` and the 17 read-side dispatchers already use the `(char *error_buf, size_t error_buf_len)` shape. WR-02 ABI rework should follow this exact pattern.
- **AST-walk infrastructure**: `tests/no_long_lived_conn.rs` uses `syn::visit::Visit` to walk `src/`. Extension point for any additional structural guards.
- **Test template for concurrency**: `test/integration/test_concurrent_reads_per_call_conn.py` (8 threads × 10 calls) is the direct template for WR-04 writes test.
- **CHANGELOG `## [Unreleased]`**: section is already present per CLAUDE.md milestone-completion rules; WR-07/WR-08 behavior changes go in `### Changed`.

### Established Patterns
- **BORROW contract**: `reinterpret_cast<duckdb_connection>(Connection*)` is repeated across ~20 call sites in `cpp/src/shim.cpp` (lines 970, 1241, 1267, 1294, 1317, 1840, 1875, 2007, 2283, etc.). All become candidates for the BorrowedConnection newtype on the Rust receiving side.
- **Single-shot TF emit pattern**: Where `init_local` is properly wired, the pattern is `if (state.emitted) { output.SetCardinality(0); return; } ... state.emitted = true;`. CR-02 fix should converge all 4 callbacks on this shape.
- **`json_merge_patch` metadata-via-SQL**: parser_override emits embedded JSON-merge into INSERTs (per Phase 65 D-12 design). The WR-04 concurrent-writes test must account for this — writes are serialized on `_definitions` PK.

### Integration Points
- `init_extension` (`src/lib.rs`) is the single seam where registration helpers are called. WR-02 ABI rework affects every call site here; pattern is mechanical.
- `cpp/src/shim.cpp` registration helpers (`sv_register_table_function`, `sv_register_scalar_function`) are the boundary for D-05 null-init_cb refusal.
- `BorrowedConnection` newtype defined in (likely) `src/conn.rs` or as a sibling to existing FFI helpers — researcher should propose the module location.

</code_context>

<deferred>
## Deferred Ideas

- **BorrowedConnection RAII helpers beyond the bare newtype** — not requested. If research surfaces a natural Connection-borrowing abstraction (e.g., per-call `CatalogReader::new(borrowed: BorrowedConnection)`), include in plan; otherwise the bare newtype is sufficient.
- **Pre-flight all 17 registrations into a vector for atomic commit (WR-09 Option 2)** — heavy refactor, explicitly deferred. The check-before-append fix (D-21) addresses the parser-hook accumulation; partial catalog-function state on registration failure is absorbed by `ALTER_ON_CONFLICT`.
- **WR-01 shared helper extraction** — WR-01's note suggested extracting `parse_string_list` into `src/ddl/read_ffi.rs`. Out of scope (WR-01 already fixed in `ae17f4b`); the deduplication can be a future cleanup or rolled into the IN-06 dedup work if it falls naturally.
- **Phase 66 work** (Expansion Qualification across all paths + ADBC test scaffolding + milestone-finisher: CHANGELOG, version bump, example file) — separate phase per ROADMAP.md.

</deferred>

---

*Phase: 65.1-phase-65-code-review-remediation*
*Context gathered: 2026-05-25 via `/gsd-discuss-phase --assumptions`*
