---
phase: 42-refactor-tidy-ups-and-test-reorganisation
plan: 01
type: execute
wave: 1
depends_on: []
files_modified:
  - src/catalog.rs
  - src/ddl/persist.rs
  - src/ddl/mod.rs
  - src/ddl/define.rs
  - src/ddl/drop.rs
  - src/ddl/alter.rs
  - src/body_parser.rs
autonomous: true
requirements: [TIDY-01, TIDY-02, TIDY-04]

must_haves:
  truths:
    - "catalog_insert and catalog_delete use a single write lock for check-and-mutate (no TOCTOU)"
    - "All persistence SQL uses duckdb_prepare + duckdb_bind_varchar instead of string interpolation"
    - "Body parser unwrap() calls have safety invariant comments explaining why they cannot panic"
  artifacts:
    - path: "src/catalog.rs"
      provides: "TOCTOU-free catalog_insert and catalog_delete"
      contains: "let mut guard = state.write().unwrap()"
    - path: "src/ddl/persist.rs"
      provides: "Shared execute_parameterized helper for prepared statements"
      exports: ["execute_parameterized"]
    - path: "src/ddl/define.rs"
      provides: "persist_define using parameterized queries"
      contains: "execute_parameterized"
    - path: "src/ddl/drop.rs"
      provides: "persist_drop using parameterized queries"
      contains: "execute_parameterized"
    - path: "src/ddl/alter.rs"
      provides: "persist_rename using parameterized queries"
      contains: "execute_parameterized"
    - path: "src/body_parser.rs"
      provides: "Safety invariant comments on unwrap() calls"
      contains: "SAFETY: extract_paren_content"
  key_links:
    - from: "src/ddl/define.rs"
      to: "src/ddl/persist.rs"
      via: "use super::persist::execute_parameterized"
      pattern: "execute_parameterized"
    - from: "src/ddl/drop.rs"
      to: "src/ddl/persist.rs"
      via: "use super::persist::execute_parameterized"
      pattern: "execute_parameterized"
    - from: "src/ddl/alter.rs"
      to: "src/ddl/persist.rs"
      via: "use super::persist::execute_parameterized"
      pattern: "execute_parameterized"
---

<objective>
Harden persistence correctness: fix the TOCTOU race in catalog lock acquisition, replace all string-interpolated persistence SQL with parameterized prepared statements via the DuckDB C API, and add safety invariant comments to body parser unwrap() calls.

Purpose: Defense-in-depth for the persistence layer (eliminate SQL injection surface entirely) and improve code maintainability by documenting parser invariants.
Output: Fixed catalog.rs, new ddl/persist.rs helper module, migrated ddl/define.rs + ddl/drop.rs + ddl/alter.rs, annotated body_parser.rs.
</objective>

<execution_context>
@$HOME/.claude/get-shit-done/workflows/execute-plan.md
@$HOME/.claude/get-shit-done/templates/summary.md
</execution_context>

<context>
@.planning/PROJECT.md
@.planning/ROADMAP.md
@.planning/STATE.md
@.planning/phases/42-refactor-tidy-ups-and-test-reorganisation/42-RESEARCH.md
@src/catalog.rs
@src/ddl/mod.rs
@src/ddl/define.rs
@src/ddl/drop.rs
@src/ddl/alter.rs
@src/body_parser.rs

<interfaces>
<!-- Key types and contracts the executor needs. -->

From src/catalog.rs (the correct pattern to follow -- already used in catalog_rename):
```rust
pub fn catalog_rename(
    state: &CatalogState,
    old_name: &str,
    new_name: &str,
) -> std::result::Result<(), Box<dyn std::error::Error>> {
    let mut guard = state.write().unwrap();
    // ... check-and-mutate inside single lock
}
```

From src/ddl/define.rs (current pattern to replace):
```rust
fn persist_define(conn: ffi::duckdb_connection, name: &str, json: &str) -> Result<(), String> {
    let safe_name = name.replace('\'', "''");
    let safe_json = json.replace('\'', "''");
    let sql = format!("INSERT OR REPLACE INTO ...");
    // uses ffi::duckdb_query
}
```

From src/ddl/drop.rs (current pattern to replace):
```rust
fn persist_drop(conn: ffi::duckdb_connection, name: &str) {
    let safe_name = name.replace('\'', "''");
    // uses ffi::duckdb_query
}
```

From src/ddl/alter.rs (current pattern to replace):
```rust
fn persist_rename(conn: ffi::duckdb_connection, old_name: &str, new_name: &str, json: &str) {
    // Two string-interpolated queries: DELETE old + INSERT new
    // uses ffi::duckdb_query
}
```

DuckDB C API functions available via libduckdb_sys (all loadable-extension compatible):
- `ffi::duckdb_prepare(conn, c_sql, &mut stmt) -> duckdb_state`
- `ffi::duckdb_bind_varchar(stmt, idx, c_value) -> duckdb_state` (1-based idx)
- `ffi::duckdb_execute_prepared(stmt, &mut result) -> duckdb_state`
- `ffi::duckdb_destroy_prepare(&mut stmt)`
- `ffi::duckdb_destroy_result(&mut result)`
- `ffi::duckdb_prepare_error(stmt) -> *const c_char`
- `ffi::DuckDBSuccess` constant
- `ffi::duckdb_prepared_statement` type (pointer, init to null_mut)
- `ffi::idx_t` type (for bind index cast)
</interfaces>
</context>

<tasks>

<task type="auto">
  <name>Task 1: Fix catalog TOCTOU + add body parser safety comments</name>
  <files>src/catalog.rs, src/body_parser.rs</files>
  <read_first>
    - src/catalog.rs (see current catalog_insert at line 95, catalog_delete at line 124, and the correct catalog_rename pattern at line 169)
    - src/body_parser.rs (see unwrap() calls at lines 448, 500, 796)
  </read_first>
  <action>
**catalog.rs -- Fix TOCTOU in catalog_insert (lines 95-119):**

Replace the read-then-write pattern with a single write lock, matching the `catalog_rename` pattern at line 169:

```rust
pub fn catalog_insert(
    state: &CatalogState,
    name: &str,
    json: &str,
) -> std::result::Result<(), Box<dyn std::error::Error>> {
    // Validate JSON before writing -- fail fast, nothing written on invalid input
    SemanticViewDefinition::from_json(name, json).map_err(Box::<dyn std::error::Error>::from)?;

    let mut guard = state.write().unwrap();
    if guard.contains_key(name) {
        return Err(format!(
            "semantic view '{name}' already exists; use CREATE OR REPLACE SEMANTIC VIEW to overwrite"
        )
        .into());
    }
    guard.insert(name.to_string(), json.to_string());
    Ok(())
}
```

Key change: Remove the separate `state.read().unwrap()` block (lines 103-112) and the separate `state.write().unwrap()` call (lines 114-117). Replace with a single `let mut guard = state.write().unwrap()` that holds the lock for both the existence check and the insert.

**catalog.rs -- Fix TOCTOU in catalog_delete (lines 124-137):**

Same pattern -- replace read-then-write with single write lock:

```rust
pub fn catalog_delete(
    state: &CatalogState,
    name: &str,
) -> std::result::Result<(), Box<dyn std::error::Error>> {
    let mut guard = state.write().unwrap();
    if !guard.contains_key(name) {
        return Err(format!("semantic view '{name}' does not exist").into());
    }
    guard.remove(name);
    Ok(())
}
```

Key change: Remove the separate `state.read().unwrap()` block (lines 128-133) and `state.write().unwrap()` call (line 135). Single write lock for check-and-remove.

**body_parser.rs -- Add safety invariant comments:**

At line 448, before `let close = after_pk.find(')').unwrap();`:
```rust
// SAFETY: extract_paren_content succeeded above (returned Some), confirming
// balanced parens exist. The closing ')' must be present in after_pk.
let close = after_pk.find(')').unwrap();
```

At line 500, before `let close = after_unique_kw.find(')').unwrap();`:
```rust
// SAFETY: extract_paren_content succeeded above (returned Some), confirming
// balanced parens exist. The closing ')' must be present in after_unique_kw.
let close = after_unique_kw.find(')').unwrap();
```

At line 796, before `let close = after_to.find(')').unwrap();`:
```rust
// SAFETY: extract_paren_content succeeded above (returned Some), confirming
// balanced parens exist. The closing ')' must be present in after_to.
let close = after_to.find(')').unwrap();
```
  </action>
  <verify>
    <automated>cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && cargo test catalog 2>&1 | tail -5</automated>
  </verify>
  <acceptance_criteria>
    - src/catalog.rs catalog_insert function body contains `let mut guard = state.write().unwrap()` followed by `guard.contains_key` and `guard.insert` with NO separate `state.read().unwrap()` block
    - src/catalog.rs catalog_delete function body contains `let mut guard = state.write().unwrap()` followed by `guard.contains_key` and `guard.remove` with NO separate `state.read().unwrap()` block
    - src/catalog.rs catalog_insert does NOT contain the pattern `{ let guard = state.read().unwrap()`
    - src/catalog.rs catalog_delete does NOT contain the pattern `{ let guard = state.read().unwrap()`
    - src/body_parser.rs contains `// SAFETY: extract_paren_content` at three locations (before each `.find(')').unwrap()`)
    - `cargo test catalog` exits 0 with all existing catalog tests passing
  </acceptance_criteria>
  <done>catalog_insert and catalog_delete use single write lock for check-and-mutate. All three body parser unwrap() calls have safety invariant comments.</done>
</task>

<task type="auto">
  <name>Task 2: Create parameterized query helper and migrate persistence functions</name>
  <files>src/ddl/persist.rs, src/ddl/mod.rs, src/ddl/define.rs, src/ddl/drop.rs, src/ddl/alter.rs</files>
  <read_first>
    - src/ddl/define.rs (see persist_define at lines 48-70, uses ffi::duckdb_query with string interpolation)
    - src/ddl/drop.rs (see persist_drop at lines 31-44, uses ffi::duckdb_query with string interpolation)
    - src/ddl/alter.rs (see persist_rename at lines 30-56, two ffi::duckdb_query calls with string interpolation)
    - src/ddl/mod.rs (module declarations -- need to add persist)
  </read_first>
  <action>
**Step 1: Create src/ddl/persist.rs with execute_parameterized helper:**

```rust
//! Shared persistence utilities for DDL operations.
//!
//! Provides parameterized query execution via the DuckDB C API prepared
//! statement interface, replacing string interpolation in persistence functions.

use libduckdb_sys as ffi;
use std::ffi::CString;

/// Execute a parameterized query with VARCHAR bindings via the DuckDB C API.
///
/// Uses `duckdb_prepare` + `duckdb_bind_varchar` + `duckdb_execute_prepared`
/// to avoid SQL injection risks from string interpolation.
///
/// Parameters use positional `$1`, `$2`, ... placeholders in the SQL string.
/// The `params` slice is bound in order (1-based indexing).
///
/// # Safety
///
/// `conn` must be a valid, open `duckdb_connection`.
pub(crate) unsafe fn execute_parameterized(
    conn: ffi::duckdb_connection,
    sql: &str,
    params: &[&str],
) -> Result<(), String> {
    let c_sql = CString::new(sql).map_err(|_| "SQL contains null byte".to_string())?;
    let mut stmt: ffi::duckdb_prepared_statement = std::ptr::null_mut();

    let rc = ffi::duckdb_prepare(conn, c_sql.as_ptr(), &mut stmt);
    if rc != ffi::DuckDBSuccess {
        let err = ffi::duckdb_prepare_error(stmt);
        let msg = if err.is_null() {
            "unknown prepare error".to_string()
        } else {
            std::ffi::CStr::from_ptr(err).to_string_lossy().into_owned()
        };
        ffi::duckdb_destroy_prepare(&mut stmt);
        return Err(msg);
    }

    for (i, param) in params.iter().enumerate() {
        let c_param = CString::new(*param)
            .map_err(|_| format!("parameter {} contains null byte", i + 1))?;
        let rc = ffi::duckdb_bind_varchar(stmt, (i + 1) as ffi::idx_t, c_param.as_ptr());
        if rc != ffi::DuckDBSuccess {
            ffi::duckdb_destroy_prepare(&mut stmt);
            return Err(format!("failed to bind parameter {}", i + 1));
        }
    }

    let mut result: ffi::duckdb_result = std::mem::zeroed();
    let rc = ffi::duckdb_execute_prepared(stmt, &mut result);
    let success = rc == ffi::DuckDBSuccess;
    ffi::duckdb_destroy_result(&mut result);
    ffi::duckdb_destroy_prepare(&mut stmt);

    if success {
        Ok(())
    } else {
        Err("prepared statement execution failed".to_string())
    }
}
```

**Step 2: Add `pub mod persist;` to src/ddl/mod.rs:**

Add as the first line (before `pub mod alter;`):
```rust
pub mod persist;
```

The full mod.rs should be:
```rust
pub mod persist;
pub mod alter;
pub mod define;
pub mod describe;
pub mod drop;
pub mod list;
pub mod show_dims;
pub mod show_dims_for_metric;
pub mod show_facts;
pub mod show_metrics;
```

**Step 3: Migrate persist_define in src/ddl/define.rs (lines 48-70):**

Replace the entire function body with:
```rust
fn persist_define(conn: ffi::duckdb_connection, name: &str, json: &str) -> Result<(), String> {
    unsafe {
        super::persist::execute_parameterized(
            conn,
            "INSERT OR REPLACE INTO semantic_layer._definitions (name, definition) VALUES ($1, $2)",
            &[name, json],
        )
        .map_err(|e| format!("failed to persist semantic view '{name}' to catalog table: {e}"))
    }
}
```

Remove the `safe_name` / `safe_json` variables and the `format!` SQL string. Remove the unused `CString` import if `persist_define` was the only user (check -- `CString` may still be used by other functions in define.rs; the `CStr` import at line 8 is used elsewhere).

**Step 4: Migrate persist_drop in src/ddl/drop.rs (lines 31-44):**

Replace the entire function body with:
```rust
fn persist_drop(conn: ffi::duckdb_connection, name: &str) {
    unsafe {
        let _ = super::persist::execute_parameterized(
            conn,
            "DELETE FROM semantic_layer._definitions WHERE name = $1",
            &[name],
        );
    }
}
```

The function intentionally ignores errors (the row may not exist in the table for in-memory sessions). Remove the `safe_name` variable and `format!` string. Check if `CString` import can be removed from drop.rs (it may be used elsewhere in the file).

**Step 5: Migrate persist_rename in src/ddl/alter.rs (lines 30-56):**

Replace the entire function body with:
```rust
fn persist_rename(conn: ffi::duckdb_connection, old_name: &str, new_name: &str, json: &str) {
    unsafe {
        // Delete old row
        let _ = super::persist::execute_parameterized(
            conn,
            "DELETE FROM semantic_layer._definitions WHERE name = $1",
            &[old_name],
        );
        // Insert new row
        let _ = super::persist::execute_parameterized(
            conn,
            "INSERT INTO semantic_layer._definitions (name, definition) VALUES ($1, $2)",
            &[new_name, json],
        );
    }
}
```

Remove the `safe_old` / `safe_new` / `safe_json` variables, both `format!` SQL strings, and both `if let Ok(c_sql)` blocks. Check if `CString` import can be removed from alter.rs.

**Step 6: Clean up unused imports in all three files.**

After migration, each of define.rs, drop.rs, alter.rs no longer needs to import `CString` for persistence (check that no other function in the file uses `CString` before removing). The `ffi` import in each file may still be needed for the VTab callback functions -- check before removing. Do NOT remove `use libduckdb_sys as ffi` if other functions in the file still reference `ffi::`.
  </action>
  <verify>
    <automated>cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && cargo check 2>&1 | tail -10</automated>
  </verify>
  <acceptance_criteria>
    - src/ddl/persist.rs exists and contains `pub(crate) unsafe fn execute_parameterized(`
    - src/ddl/persist.rs contains `ffi::duckdb_prepare`, `ffi::duckdb_bind_varchar`, `ffi::duckdb_execute_prepared`, `ffi::duckdb_destroy_prepare`
    - src/ddl/mod.rs contains `pub mod persist;`
    - src/ddl/define.rs persist_define function contains `execute_parameterized` and does NOT contain `replace('\'', "''")`
    - src/ddl/drop.rs persist_drop function contains `execute_parameterized` and does NOT contain `replace('\'', "''")`
    - src/ddl/alter.rs persist_rename function contains `execute_parameterized` and does NOT contain `replace('\'', "''")`
    - No file in src/ddl/ contains `replace('\'', "''")` (all string escaping eliminated)
    - `cargo check` exits 0 (compiles under the default bundled feature)
    - Note: Full integration verification via `just test-sql` will happen at wave-level
  </acceptance_criteria>
  <done>All persistence SQL uses parameterized prepared statements. The shared execute_parameterized helper lives in ddl/persist.rs. String interpolation with quote escaping is completely eliminated from the persistence layer.</done>
</task>

</tasks>

<verification>
After both tasks complete:
1. `cargo test` passes (all Rust unit tests including catalog tests)
2. `just build && just test-sql` passes (integration tests exercise the full persistence path through CREATE/DROP/ALTER DDL)
3. `just test-ducklake-ci` passes
4. grep confirms no `replace('\'', "''")` remains in src/ddl/
5. grep confirms no `state.read().unwrap()` in catalog_insert or catalog_delete
</verification>

<success_criteria>
- catalog_insert and catalog_delete hold a single write lock for the entire check-and-mutate operation (per TIDY-01)
- All persistence functions use duckdb_prepare/duckdb_bind_varchar parameterized queries (per TIDY-02)
- All three body parser unwrap() calls have SAFETY comments (per TIDY-04)
- `just test-all` passes with zero behavior changes
</success_criteria>

<output>
After completion, create `.planning/phases/42-refactor-tidy-ups-and-test-reorganisation/42-01-SUMMARY.md`
</output>
