---
phase: 65.1-phase-65-code-review-remediation
verified: 2026-05-26T00:00:00Z
status: passed
score: 17/17 must-haves verified
overrides_applied: 0
---

# Phase 65.1: Phase 65 Code Review Remediation — Verification Report

**Phase Goal:** Address the 10 remaining Critical + Warning findings from Phase 65's REVIEW.md (1 of 11 — WR-01 — already fixed inline in commit `ae17f4b`). Findings span: SQL-injection surface in YAML-file helper TF (CR-01), double-emit hazards in 4 exec callbacks (CR-02), and 8 warnings covering registration-failure surfacing (WR-02), parser_override write-path concurrency (WR-04), read-only DROP error wording (WR-03), BORROW-contract enforcement (WR-05/WR-06), silent-fallback hard errors (WR-07/WR-08), and idempotent registration (WR-09). Plus IN-01..06 internal cleanups and a CHANGELOG entry per D-27.

**Verified:** 2026-05-26
**Status:** PASSED — phase goal achieved
**Re-verification:** No — initial verification

## Goal Achievement

### Observable Truths

| #  | Truth (Requirement ID) | Status     | Evidence |
| -- | ---------------------- | ---------- | -------- |
| 1  | REM-CR-01: SQL-injection surface in YAML-file helper TF eliminated via FileSystem-direct read | ✓ VERIFIED | `cpp/src/shim.cpp:832` `auto &fs = FileSystem::GetFileSystem(context);` replaces `read_text(...)` SQL. No `path_escaped` quote-doubling remains. Access-gating sqllogictest `test/sql/phase651_yaml_filesystem_access_gating.test` registered in TEST_LIST and passing. |
| 2  | REM-CR-02: Four TF exec callbacks converged on single-shot; null-state path throws InternalException (no emit-anyway fallback) | ✓ VERIFIED | `cpp/src/shim.cpp:919, 1109, 1215, 1286` — all four (`sv_create_from_yaml_function`, `sv_list_semantic_views_function`, `sv_emit_varchar_rows`, `sv_emit_varchar_bool_rows`) throw `InternalException` on null state. D-05 null-init refusal enforced at registration time (`cpp/src/shim.cpp:615` `init_cb is mandatory`). |
| 3  | REM-WR-02: Registration helpers carry `(error_buf, error_buf_len)` ABI; failures surface `e.what()` to caller | ✓ VERIFIED | `cpp/src/shim.hpp:45,87,147,187,209,215,221,...` extern declarations carry the pair. `src/lib.rs:339-340,358-359,367-368,377-378` Rust cascade propagates buffer into returned `Err(...)`. Integration test `tests/registration_error_surfaces.rs::init_extension_surfaces_registration_error_buf` PASS. |
| 4  | REM-WR-03: DROP/ALTER on never-bootstrapped RO DB returns canonical wording `semantic view 'X' does not exist` | ✓ VERIFIED | `src/parse.rs:2171 definitions_table_guard_select` + outer-CASE on `information_schema.tables`. Integration test `test/integration/test_drop_on_fresh_readonly_clear_error.py:112,123,159` asserts wording. Test PASSES in `just test-all` run. |
| 5  | REM-WR-04: Concurrent CREATE/DROP/ALTER coverage with `unknown_error` budget = 0 | ✓ VERIFIED | `test/integration/test_concurrent_writes_per_call_conn.py` (340 lines) populated with `success | already_exists | constraint_violation | unknown_error` categorisation, threading.Event start gate, list_semantic_views() reconciliation. Passes in `just test-all`. |
| 6  | REM-WR-05: BorrowedConnection newtype enforces BORROW contract at the type level across all 17 read-side dispatchers | ✓ VERIFIED | `src/ddl/read_ffi.rs:79-80` `#[repr(transparent)] pub struct BorrowedConnection(ffi::duckdb_connection);` with W-02-fixed compile_fail doctest. All ddl/ + 2 query/ dispatchers wrap on entry. `tests/no_long_lived_conn.rs::DisconnectFinder` (lines 171-199) walks `src/**/*.rs` for `duckdb_disconnect` and PASSES. |
| 7  | REM-WR-06: BORROW-contract bridge pinned via static_assert + load-time runtime probe | ✓ VERIFIED | `cpp/src/shim.cpp:50-51` file-scope `static_assert(sizeof(duckdb_connection) == sizeof(void*), ...)`. Runtime probe at `cpp/src/shim.cpp:2697` writes `"bridge contract broken — duckdb_connection layout..."` into error_buf on failure. `sv_register_parser_hooks` signature carries error_buf pair. |
| 8  | REM-WR-07: `sv_resolve_output_logical_types` probe failure throws BinderException (no silent DECIMAL(18,3) fallback) | ✓ VERIFIED | `cpp/src/shim.cpp:2393` throws `BinderException("semantic_view: failed to infer column types via LIMIT 0 probe: " + ...)`. sqllogictest `phase651_bind_probe_hard_errors.test` pins the surface. |
| 9  | REM-WR-08: `try_infer_schema` returns `Result<(Vec<String>, Vec<ffi::duckdb_type>), String>`; callers surface via error_buf | ✓ VERIFIED | `src/query/table_function.rs:658` `-> Result<(Vec<String>, Vec<ffi::duckdb_type>), String>`. Accepts `&BorrowedConnection`. |
| 10 | REM-WR-09: `LOAD semantic_views` is idempotent; parser-extension list does not grow on repeat LOAD; no OverrideContext leak on skip path | ✓ VERIFIED | `cpp/src/shim.cpp:2745-2752` iterates `cbmgr.ParserExtensions()` and short-circuits on match. Critical fix at `cpp/src/shim.cpp:2767`: `sv_make_override_context()` moved INSIDE `if (!already_registered)` block (CR-01 fix commit `7a9fc24`). Behavioural test `test_load_extension_twice_idempotent.py` + structural test `tests/parser_hook_idempotent.rs::parser_hook_register_is_idempotent` both PASS. |
| 11 | REM-IN-01: Stale comment `(void)emitted; // single-shot semantics` removed; `init_local IS wired in` comment block updated | ✓ VERIFIED | No matches for `(void)emitted` in `cpp/src/shim.cpp`. Refreshed comments reference `init_cb is mandatory` (line 907). |
| 12 | REM-IN-02: `src/type_cache.rs` deleted; `mod type_cache;` removed from `src/lib.rs` | ✓ VERIFIED | `ls src/type_cache.rs` → "No such file or directory". `grep type_cache src/lib.rs` → 0 matches. |
| 13 | REM-IN-03: `type_id_to_display_name` function in `src/query/table_function.rs` deleted | ✓ VERIFIED | `grep type_id_to_display_name src/query/table_function.rs` → 0 matches. |
| 14 | REM-IN-04: `__sv_compute_create_from_yaml` helper TF kind parameter removed; signature is 3-arg `(content, name, comment)` | ✓ VERIFIED | `cpp/src/shim.cpp:156-161` shows `sv_compute_create_from_yaml_rust(content_ptr/len, name_ptr/len, comment_ptr/len, out_ptr/len, error_buf/len)`. No `kind:` parameter in `src/ddl/alter_helpers_ffi.rs`. |
| 15 | REM-IN-05: Null-name sqllogictest assertions in `phase651_null_name_inputs.test` | ✓ VERIFIED | `test/sql/phase651_null_name_inputs.test` contains `SELECT read_yaml_from_semantic_view(NULL) IS NULL`, `SELECT get_ddl(NULL, 'x') IS NULL`, `SELECT get_ddl('SEMANTIC_VIEW', NULL) IS NULL`. Registered in TEST_LIST. |
| 16 | REM-IN-06: Duplicate `probe_catalog_table_present` and `write_err` in `src/ddl/list.rs` deleted; single caller imports from `read_ffi` | ✓ VERIFIED | `src/ddl/list.rs:88` `use crate::ddl::read_ffi::{probe_catalog_table_present, write_err, BorrowedConnection};`. Line 193 comment confirms "DELETED". |
| 17 | REM-CHG: CHANGELOG.md `[Unreleased]` populated per Keep-a-Changelog 1.1.0 with Security/Changed/Fixed/Removed; no phase-internal subheadings | ✓ VERIFIED | CHANGELOG.md `[Unreleased]` contains exactly the 4 standard categories. Security: CR-01 SQL-injection. Changed: WR-07/WR-08 hard-error promotion. Removed: type_cache+type_id_to_display_name. Fixed: WR-03, WR-02, WR-09, CR-02. No `Phase 65 …` headings. |

**Score:** 17/17 truths verified

### Required Artifacts

| Artifact | Expected | Status | Details |
| -------- | -------- | ------ | ------- |
| `cpp/src/shim.cpp` | FileSystem-direct YAML; static_assert + probe; idempotent parser hooks; D-05 enforcement; four converged exec callbacks; column-mismatch throw; reuse-Connection in sv_resolve_output_logical_types | ✓ VERIFIED | All patterns present (lines 50, 615, 832, 919/1109/1215/1286, 2393, 2551, 2697, 2745-2767, 2867+) |
| `cpp/src/shim.hpp` | extern signatures with (error_buf, error_buf_len) | ✓ VERIFIED | 14+ extern declarations updated |
| `src/lib.rs` | error_buf cascade in init_extension; type_cache mod removed | ✓ VERIFIED | 4 extern declarations carry buf; no type_cache mod |
| `src/parse.rs` | definitions_table_guard_select + outer information_schema CASE; char_indices UTF-8 safe; write_error_to_buffer on rc=2 err | ✓ VERIFIED | All 3 fixes present (lines 354/1104 char_indices, 2171 guard, 2679/2762/2810 write_error) |
| `src/ddl/read_ffi.rs` | BorrowedConnection newtype + W-02 compile_fail doctest; canonical probe_catalog_table_present | ✓ VERIFIED | Lines 79-131 |
| `src/ddl/list.rs` | Duplicate helpers deleted; imports from read_ffi | ✓ VERIFIED | Line 88 import, line 193 dedup comment |
| `src/ddl/{describe,get_ddl,read_yaml,show_*}.rs` | BorrowedConnection wrap-on-entry | ✓ VERIFIED | grep shows all 11 ddl/ files contain `BorrowedConnection::new` |
| `src/query/{explain,table_function}.rs` | BorrowedConnection + Result<_, String> parse_string_list | ✓ VERIFIED | Lines 142/152 query/explain; 134/142/658 query/table_function |
| `src/type_cache.rs` | DELETED | ✓ VERIFIED | File does not exist |
| `tests/no_long_lived_conn.rs` | DisconnectFinder AST walker; allow-list for `RawDb::Drop` | ✓ VERIFIED | Lines 171-199; test passes |
| `tests/registration_error_surfaces.rs` | Force-fail null-init test asserts `init_cb is mandatory` in surfaced error | ✓ VERIFIED | Line 131; test passes |
| `tests/parser_hook_idempotent.rs` | Structural pre/post test using sv_count_parser_extensions FFI | ✓ VERIFIED | Test passes |
| `test/sql/TEST_LIST` | Registration of 3 new phase651_*.test files | ✓ VERIFIED | All three registered |
| `test/sql/phase651_yaml_filesystem_access_gating.test` | enable_external_access positive/negative coverage | ✓ VERIFIED | Populated |
| `test/sql/phase651_null_name_inputs.test` | 3 null-name assertions | ✓ VERIFIED | Populated |
| `test/sql/phase651_bind_probe_hard_errors.test` | WR-07/WR-08 binder error coverage | ✓ VERIFIED | Populated |
| `test/integration/test_drop_on_fresh_readonly_clear_error.py` | DROP + ALTER on never-bootstrapped RO DB pins canonical wording | ✓ VERIFIED | Populated, passing |
| `test/integration/test_load_extension_twice_idempotent.py` | LOAD-twice behavioural regression | ✓ VERIFIED | Populated, passing |
| `test/integration/test_concurrent_writes_per_call_conn.py` | N-thread CREATE/DROP/ALTER race | ✓ VERIFIED | Populated, passing |
| `CHANGELOG.md` | [Unreleased] populated with 4 K-a-C categories | ✓ VERIFIED | Security/Changed/Fixed/Removed all present |
| `Cargo.toml` | walkdir in dev-dependencies | ✓ VERIFIED | Line found |

### Key Link Verification

| From | To | Via | Status | Details |
| ---- | -- | --- | ------ | ------- |
| `init_extension` (src/lib.rs) | C registration helpers (shim.cpp) | extern declarations with error_buf pair | WIRED | All extern decls carry pair; init_extension surfaces buffer into Err |
| ddl/list.rs dispatcher | read_ffi::probe_catalog_table_present | `use crate::ddl::read_ffi::` | WIRED | Line 88 import; canonical helper called via &BorrowedConnection |
| 4 TF exec callbacks | sv_register_table_function D-05 refusal | registration-time null-init refusal makes null-state branch unreachable | WIRED | All 4 callbacks throw InternalException; registration refuses null init_cb |
| `sv_create_from_yaml_bind` | FileSystem::OpenFile | direct FS call (no SQL indirection) | WIRED | cpp/src/shim.cpp:832 |
| `sv_register_parser_hooks` | DBConfig::ParserExtensions() | dedup-check before Register call | WIRED | Lines 2745-2752; OverrideContext allocation moved inside guard (CR-01 fix) |
| `try_infer_schema` | C++ binder | error_buf plumb: Rust write_err → rc=1 → throw BinderException | WIRED | Result<_, String> at table_function.rs:658; C++ at shim.cpp:2393 |
| `existence_guard_select` | information_schema.tables | outer CASE WHEN short-circuits | WIRED | parse.rs:2174 |
| `DisconnectFinder` | src/**/*.rs | walkdir + syn AST visit | WIRED | tests/no_long_lived_conn.rs lines 171-199; PASS |

### Behavioral Spot-Checks (Step 7b)

| Behavior | Command | Result | Status |
| -------- | ------- | ------ | ------ |
| Full quality gate (Rust unit + proptest + sqllogictest + DuckLake CI) | `just test-all` | exit 0; 950 nextest tests passed; sqllogictest passed; all integration suites passed | ✓ PASS |
| BORROW-contract structural guard | `cargo test no_long_lived_conn` (run as part of test-all) | PASS for both `init_extension_has_no_duckdb_connect_call` and `no_duckdb_disconnect_anywhere_in_src` | ✓ PASS |
| Registration error_buf surfacing | `cargo test registration_error_surfaces` (run as part of test-all) | PASS — `init_extension_surfaces_registration_error_buf` | ✓ PASS |
| Parser-hook idempotence (structural) | `cargo test parser_hook_idempotent` (run as part of test-all) | PASS — `parser_hook_register_is_idempotent` | ✓ PASS |

### Probe Execution (Step 7c)

No project-conventional probe scripts (`scripts/*/tests/probe-*.sh`) exist in this repo. Phase 65.1 PLANs/SUMMARY do not declare probe scripts; the project's quality gate per CLAUDE.md is `just test-all`, which was executed above and exited 0.

### Requirements Coverage

| Requirement | Source Plans | Description | Status | Evidence |
| ----------- | ------------ | ----------- | ------ | -------- |
| REM-CR-01   | 01, 07       | SQL-injection elimination via FileSystem-direct YAML read | ✓ SATISFIED | Plan 07 implementation + Plan 01 sqllogictest stub populated |
| REM-CR-02   | 02a, 08      | Double-emit/unbounded-loop hazard eliminated in 4 exec callbacks | ✓ SATISFIED | Throw on null state at lines 919/1109/1215/1286 |
| REM-WR-02   | 01, 02a, 02b | Registration error_buf ABI cascaded through C and Rust | ✓ SATISFIED | shim.hpp/cpp + src/lib.rs + tests/registration_error_surfaces.rs |
| REM-WR-03   | 01, 04       | Read-only DROP/ALTER clear error wording | ✓ SATISFIED | definitions_table_guard_select + integration test |
| REM-WR-04   | 05          | Concurrent writes coverage | ✓ SATISFIED | test_concurrent_writes_per_call_conn.py |
| REM-WR-05   | 03a, 03b    | BorrowedConnection newtype + 17 dispatcher migration + DisconnectFinder | ✓ SATISFIED | BorrowedConnection in read_ffi.rs + DisconnectFinder AST guard |
| REM-WR-06   | 10          | Static_assert + load-time runtime probe | ✓ SATISFIED | shim.cpp:50 + 2697 |
| REM-WR-07   | 01, 11      | sv_resolve_output_logical_types probe failure → BinderException | ✓ SATISFIED | shim.cpp:2393 |
| REM-WR-08   | 01, 11      | try_infer_schema Result + error_buf surfacing | ✓ SATISFIED | table_function.rs:658 |
| REM-WR-09   | 01, 12      | Idempotent parser-hook registration + structural test | ✓ SATISFIED | shim.cpp:2745-2767; CR-01 leak fix in commit 7a9fc24 |
| REM-IN-01   | 08          | Stale `(void)emitted` comment removed; init_local comment refreshed | ✓ SATISFIED | No `(void)emitted` matches in shim.cpp |
| REM-IN-02   | 09          | type_cache module deleted | ✓ SATISFIED | File does not exist |
| REM-IN-03   | 09          | type_id_to_display_name function deleted | ✓ SATISFIED | grep returns 0 matches |
| REM-IN-04   | 07          | __sv_compute_create_from_yaml kind param removed | ✓ SATISFIED | 3-arg signature in shim.cpp:156 |
| REM-IN-05   | 01, 09      | Null-name sqllogictest assertions | ✓ SATISFIED | phase651_null_name_inputs.test populated |
| REM-IN-06   | 03a         | Duplicate probe_catalog_table_present + write_err in list.rs deleted | ✓ SATISFIED | list.rs:88 imports from read_ffi |
| REM-CHG     | 06          | CHANGELOG [Unreleased] populated per K-a-C 1.1.0 | ✓ SATISFIED | 4 standard subheadings, no phase-internal headings |

**Coverage:** 17/17 requirement IDs satisfied, 0 orphaned. All ROADMAP-declared REM-* IDs are claimed by at least one plan and verified in the codebase. Note: REQUIREMENTS.md still lists v0.9.1 milestone IDs (LIFE-*, EXPAND-CTX-*, REL-*) which were inherited from the prior milestone reframing — REM-* IDs live in the ROADMAP phase header per the documented "derived during /gsd-plan-phase from REVIEW.md findings" convention.

### Anti-Patterns Found

| File | Line | Pattern | Severity | Impact |
| ---- | ---- | ------- | -------- | ------ |
| (none — code-review report iteration found and fixed CR-01 leak + 7 warnings; 4 IN-* findings explicitly deferred per scope directive) | — | — | — | — |

The post-implementation code review (`65.1-REVIEW.md`) flagged 1 critical (CR-01 OverrideContext leak — regression in Plan 12) + 7 warnings + 4 info. The critical and all 7 warnings were closed in iteration 1 of gsd-code-fixer (`65.1-REVIEW-FIX.md`, commits 7a9fc24 → 4055196). The 4 Info findings (IN-01..04 in the REVIEW.md namespace — distinct from the phase's REM-IN-01..06) are non-blocking maintenance items deferred per the scope directive.

### Human Verification Required

No items requiring human verification — all observable truths are programmatically verifiable via:
- File existence + grep patterns for code-level artifacts
- `just test-all` exit code + per-test PASS lines for behavioural confirmation
- AST-walker structural tests for BORROW contract + parser-hook idempotence + registration error surfacing

### Gaps Summary

No gaps. All 17 must-haves verified; quality gate (`just test-all`) exits 0 with 950 nextest tests passing; the 12 LIFE-* watchdog tests for the original v0.9.1 milestone goal still pass after the phase's significant refactoring (BorrowedConnection migration, FileSystem-direct YAML, idempotent parser hooks, error_buf cascade). The post-implementation code review fixes (review-fix iteration 1) successfully closed the regression that escaped the structural test (CR-01) and tightened all 7 warnings. The 4 Info findings deferred per scope directive are non-blocking maintenance items.

---

_Verified: 2026-05-26_
_Verifier: Claude (gsd-verifier)_
