---
phase: 49-security-correctness-hardening
plan: 02
type: execute
wave: 2
depends_on: ["49-01"]
files_modified:
  - src/lib.rs
  - src/catalog.rs
  - src/ddl/define.rs
  - src/ddl/drop.rs
  - src/ddl/alter.rs
  - src/ddl/list.rs
  - src/ddl/get_ddl.rs
  - src/ddl/show_facts.rs
  - src/ddl/show_dims.rs
  - src/ddl/show_metrics.rs
  - src/ddl/show_columns.rs
  - src/ddl/show_dims_for_metric.rs
  - src/ddl/describe.rs
  - src/query/table_function.rs
  - src/query/explain.rs
  - src/expand/facts.rs
  - src/expand/sql_gen.rs
  - src/expand/types.rs
autonomous: true
requirements:
  - SEC-01
  - SEC-03

must_haves:
  truths:
    - "A Rust panic inside any VTab bind/init/func method is caught and converted to a DuckDB error instead of unwinding through C++ frames"
    - "A Rust panic inside the extension init function is caught and returns false instead of unwinding"
    - "A Rust panic inside FFI catalog functions is caught and returns -1 instead of unwinding"
    - "A cyclic derived metric dependency at query time produces a clear error message"
    - "A cyclic fact dependency at query time produces a clear error message instead of silently returning empty"
    - "Deeply nested derived metrics (beyond 64 levels) produce a depth-limit error"
  artifacts:
    - path: "src/query/table_function.rs"
      provides: "catch_unwind in VTab bind/init/func"
      contains: "catch_unwind"
    - path: "src/expand/types.rs"
      provides: "CycleDetected and MaxDepthExceeded error variants"
      contains: "CycleDetected"
    - path: "src/expand/facts.rs"
      provides: "Cycle detection in toposort_derived and depth limit in inline_derived_metrics"
      contains: "MAX_DERIVATION_DEPTH"
    - path: "src/lib.rs"
      provides: "catch_unwind wrapping extension init"
      contains: "catch_unwind"
    - path: "src/catalog.rs"
      provides: "catch_unwind wrapping FFI catalog functions"
      contains: "catch_unwind"
  key_links:
    - from: "src/expand/facts.rs"
      to: "src/expand/types.rs"
      via: "toposort_derived returns ExpandError::CycleDetected on cycle"
      pattern: "CycleDetected"
    - from: "src/expand/sql_gen.rs"
      to: "src/expand/facts.rs"
      via: "toposort_facts error propagated instead of .unwrap_or_default()"
      pattern: "toposort_facts.*\\?"
    - from: "VTab bind/init/func"
      to: "DuckDB C++ runtime"
      via: "catch_unwind converts panics to error strings"
      pattern: "catch_unwind"
---

<objective>
Wrap all FFI entry points in catch_unwind for panic safety, and add cycle detection + depth limits to derived metric/fact resolution at query time.

Purpose: Prevent Rust panics from unwinding through C++ stack frames (undefined behavior) and prevent infinite loops or stack overflows from cyclic/deeply nested derived metric definitions that bypass CREATE-time validation.

Output: Every FFI entry point catches panics. Cycle and depth limits produce clear errors at query expansion time.
</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/49-security-correctness-hardening/49-01-SUMMARY.md
@src/expand/facts.rs
@src/expand/sql_gen.rs
@src/expand/types.rs
@src/query/table_function.rs
@src/lib.rs
@src/catalog.rs
@src/parse.rs

<interfaces>
<!-- Existing catch_unwind pattern from parse.rs (lines 1262-1298) -->
```rust
// From src/parse.rs — the established pattern for catch_unwind at FFI boundaries:
let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
    // ... Rust logic here ...
}));
result.unwrap_or(fallback_value) // on panic: return safe fallback
```

<!-- VTab trait from duckdb crate — bind/init/func are called from unsafe extern "C" -->
```rust
pub trait VTab: Sized {
    type InitData: Sized + Send + Sync;
    type BindData: Sized + Send + Sync;
    fn bind(bind: &BindInfo) -> Result<Self::BindData, Box<dyn std::error::Error>>;
    fn init(init: &InitInfo) -> Result<Self::InitData, Box<dyn std::error::Error>>;
    fn func(func: &TableFunctionInfo<Self>, output: &mut DataChunkHandle) -> Result<(), Box<dyn std::error::Error>>;
}
```

<!-- The duckdb crate's extern "C" wrappers (vtab/mod.rs lines 85-137) do NOT have catch_unwind -->
<!-- We must wrap INSIDE our VTab method implementations -->

<!-- From src/expand/types.rs — ExpandError enum (our code) -->
```rust
pub enum ExpandError {
    EmptyRequest { view_name: String },
    UnknownDimension { ... },
    // ... ~12 variants ...
    WindowMetricRequiredDimension { ... },
}
```

<!-- From src/expand/facts.rs — toposort_derived (line 208) silently ignores cycles -->
<!-- toposort_facts (line 77) returns Err on cycle but caller uses .unwrap_or_default() -->
```rust
fn toposort_derived(derived: &[(usize, &Metric)], _resolved: &HashMap<String, String>) -> Vec<usize>;
pub(super) fn toposort_facts(facts: &[Fact]) -> Result<Vec<usize>, String>;
pub(super) fn inline_derived_metrics(metrics: &[Metric], facts: &[Fact], topo: &[usize]) -> HashMap<String, String>;
```
</interfaces>
</context>

<tasks>

<task type="auto" tdd="true">
  <name>Task 1: Add cycle detection and depth limits to derived metric/fact resolution</name>
  <files>src/expand/facts.rs, src/expand/sql_gen.rs, src/expand/types.rs</files>
  <read_first>src/expand/facts.rs, src/expand/sql_gen.rs, src/expand/types.rs, src/graph/derived_metrics.rs</read_first>
  <behavior>
    - Test 1: toposort_derived with a->b->a cycle returns Err (not silent truncation)
    - Test 2: toposort_facts cycle is propagated as ExpandError (not swallowed by .unwrap_or_default())
    - Test 3: inline_derived_metrics with >64 levels of nesting returns Err
    - Test 4: Normal derived metrics (no cycles, shallow nesting) continue to work correctly
    - Test 5: All existing cargo test tests pass unchanged
  </behavior>
  <action>
**Step 1: Add error variants to ExpandError** (src/expand/types.rs)

Add two new variants to the `ExpandError` enum:

```rust
/// A cycle was detected in derived metric or fact dependencies at query expansion time.
CycleDetected {
    view_name: String,
    cycle_description: String,
},
/// Derived metric nesting exceeds the maximum allowed depth.
MaxDepthExceeded {
    view_name: String,
    depth: usize,
    max_depth: usize,
},
```

Add Display implementations:

```rust
Self::CycleDetected { view_name, cycle_description } => {
    write!(
        f,
        "semantic view '{view_name}': cycle detected in metric/fact dependencies \
         during query expansion: {cycle_description}"
    )
}
Self::MaxDepthExceeded { view_name, depth, max_depth } => {
    write!(
        f,
        "semantic view '{view_name}': derived metric nesting depth {depth} exceeds \
         maximum allowed depth of {max_depth}"
    )
}
```

**Step 2: Fix toposort_derived to return Result** (src/expand/facts.rs)

Change `toposort_derived` signature from:
```rust
fn toposort_derived(
    derived: &[(usize, &crate::model::Metric)],
    _resolved_names: &HashMap<String, String>,
) -> Vec<usize>
```
to:
```rust
fn toposort_derived(
    derived: &[(usize, &crate::model::Metric)],
    _resolved_names: &HashMap<String, String>,
) -> Result<Vec<usize>, String>
```

Change the existing comment at lines 273-276 from silently returning partial results:
```rust
// If order.len() != n, there is a cycle. This should have been caught at
// define time by validate_derived_metrics, but as a defensive fallback we
// just return what we have.
order
```
to:
```rust
if order.len() != n {
    // Build cycle description from remaining nodes
    let remaining: Vec<&str> = derived
        .iter()
        .enumerate()
        .filter(|(i, _)| !order.contains(i))
        .map(|(_, (_, m))| m.name.as_str())
        .collect();
    return Err(format!("cycle in derived metrics: [{}]", remaining.join(", ")));
}
Ok(order)
```

**Step 3: Change inline_derived_metrics to return Result** (src/expand/facts.rs)

Change signature from:
```rust
pub(super) fn inline_derived_metrics(
    metrics: &[crate::model::Metric],
    facts: &[Fact],
    fact_topo_order: &[usize],
) -> HashMap<String, String>
```
to:
```rust
pub(super) fn inline_derived_metrics(
    metrics: &[crate::model::Metric],
    facts: &[Fact],
    fact_topo_order: &[usize],
) -> Result<HashMap<String, String>, String>
```

Update the body:
- Where it calls `toposort_derived`, propagate the error: `let derived_topo = toposort_derived(&derived, &resolved)?;`
- At the end, `return Ok(resolved);` instead of `return resolved;`
- For the early return when `derived.is_empty()`, `return Ok(resolved);`

Add a MAX_DERIVATION_DEPTH constant at the top of facts.rs:
```rust
/// Maximum allowed nesting depth for derived metric resolution.
/// Prevents stack overflow from deeply nested metric chains that pass
/// cycle detection (linear chains: a->b->c->d->... up to 64 levels).
const MAX_DERIVATION_DEPTH: usize = 64;
```

Add a depth check after the toposort, before inlining:
```rust
if derived_topo.len() > MAX_DERIVATION_DEPTH {
    return Err(format!(
        "derived metric nesting depth {} exceeds maximum of {}",
        derived_topo.len(),
        MAX_DERIVATION_DEPTH
    ));
}
```

**Step 4: Fix toposort_facts call site in sql_gen.rs** (src/expand/sql_gen.rs)

In `expand()` function (around line 296), change:
```rust
let topo_order = toposort_facts(&def.facts).unwrap_or_default();
```
to:
```rust
let topo_order = toposort_facts(&def.facts).map_err(|e| ExpandError::CycleDetected {
    view_name: view_name.to_string(),
    cycle_description: e,
})?;
```

Similarly in `expand_facts()` function (around line 93), change:
```rust
let topo_order = toposort_facts(&def.facts).unwrap_or_default();
```
to:
```rust
let topo_order = toposort_facts(&def.facts).map_err(|e| ExpandError::CycleDetected {
    view_name: view_name.to_string(),
    cycle_description: e,
})?;
```

**Step 5: Update inline_derived_metrics call sites**

In `expand()` (sql_gen.rs, around line 297), change:
```rust
let resolved_exprs = inline_derived_metrics(&def.metrics, &def.facts, &topo_order);
```
to:
```rust
let resolved_exprs = inline_derived_metrics(&def.metrics, &def.facts, &topo_order)
    .map_err(|e| ExpandError::CycleDetected {
        view_name: view_name.to_string(),
        cycle_description: e,
    })?;
```

Apply the same change to ALL call sites of `inline_derived_metrics` — search for it in:
- `src/expand/sql_gen.rs` (expand function)
- `src/expand/semi_additive.rs` (if it calls inline_derived_metrics)
- `src/expand/window.rs` (if it calls inline_derived_metrics)

Each call site must propagate the Result with `?` and map the String error to the appropriate ExpandError variant.

**Step 6: Add unit tests** in src/expand/facts.rs (in the existing #[cfg(test)] mod tests block, or create one)

```rust
#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn toposort_derived_detects_cycle() {
        use crate::model::Metric;
        let met_a = Metric {
            name: "a".to_string(),
            expr: "b + 1".to_string(),
            source_table: None,
            ..Metric::default_for_test()
        };
        let met_b = Metric {
            name: "b".to_string(),
            expr: "a + 1".to_string(),
            source_table: None,
            ..Metric::default_for_test()
        };
        let derived: Vec<(usize, &Metric)> = vec![(0, &met_a), (1, &met_b)];
        let resolved = std::collections::HashMap::new();
        let result = toposort_derived(&derived, &resolved);
        assert!(result.is_err(), "Expected cycle error");
        assert!(result.unwrap_err().contains("cycle"), "Error should mention cycle");
    }

    #[test]
    fn max_derivation_depth_enforced() {
        // This test verifies the constant exists and the check works
        assert!(MAX_DERIVATION_DEPTH == 64);
    }
}
```

Note: The `Metric::default_for_test()` method may not exist. If not, construct the Metric manually with all required fields. Check `src/model.rs` for the Metric struct definition and use appropriate default values for fields like `access`, `synonyms`, `comment`, `non_additive_by`, `window_spec`, `using_relationships`, `output_type`, `column_type_inferred`.
  </action>
  <verify>
    <automated>cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && cargo test 2>&1 | tail -5</automated>
  </verify>
  <acceptance_criteria>
    - grep 'CycleDetected' src/expand/types.rs returns at least 2 matches (variant + Display)
    - grep 'MaxDepthExceeded' src/expand/types.rs returns at least 2 matches (variant + Display)
    - grep 'MAX_DERIVATION_DEPTH' src/expand/facts.rs returns at least 2 matches (const + usage)
    - grep 'unwrap_or_default' src/expand/sql_gen.rs returns 0 matches (no more silent cycle swallowing)
    - grep 'toposort_derived' src/expand/facts.rs shows Result return type
    - grep 'inline_derived_metrics' src/expand/facts.rs shows Result return type
    - cargo test -- toposort_derived_detects_cycle passes
    - cargo test passes with 0 failures
  </acceptance_criteria>
  <done>Derived metric and fact resolution have cycle detection and depth limits. toposort_derived returns Err on cycles instead of silent truncation. toposort_facts errors are propagated instead of swallowed. inline_derived_metrics enforces MAX_DERIVATION_DEPTH of 64. All existing tests pass.</done>
</task>

<task type="auto">
  <name>Task 2: Wrap all FFI entry points in catch_unwind</name>
  <files>src/lib.rs, src/catalog.rs, src/ddl/define.rs, src/ddl/drop.rs, src/ddl/alter.rs, src/ddl/list.rs, src/ddl/get_ddl.rs, src/ddl/show_facts.rs, src/ddl/show_dims.rs, src/ddl/show_metrics.rs, src/ddl/show_columns.rs, src/ddl/show_dims_for_metric.rs, src/ddl/describe.rs, src/query/table_function.rs, src/query/explain.rs</files>
  <read_first>src/parse.rs (lines 1250-1300 for catch_unwind pattern), src/lib.rs (lines 640-672 for extension init), src/catalog.rs (lines 184-262 for FFI catalog functions), src/query/table_function.rs (lines 398-680 for VTab impl), src/ddl/define.rs (lines 155-220 for VTab bind pattern)</read_first>
  <action>
The duckdb crate's `unsafe extern "C"` wrappers for VTab bind/init/func do NOT include catch_unwind. A panic in any of our VTab method implementations will unwind through C++ stack frames, which is undefined behavior. We wrap the body of each method in catch_unwind.

**Approach:** For each VTab impl's bind/init/func methods, wrap the body in `std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { ... }))` and convert panics to error Results.

**Step 1: Create a helper function** in a new file or at the top of a shared location.

Add this helper to `src/util.rs` (or wherever the shared utilities live):

```rust
/// Wrap a closure in catch_unwind, converting panics to Box<dyn Error>.
/// Used at FFI boundaries to prevent Rust panics from unwinding through C++ frames.
pub fn catch_unwind_to_result<F, T>(f: F) -> Result<T, Box<dyn std::error::Error>>
where
    F: FnOnce() -> Result<T, Box<dyn std::error::Error>> + std::panic::UnwindSafe,
{
    match std::panic::catch_unwind(f) {
        Ok(result) => result,
        Err(payload) => {
            let msg = if let Some(s) = payload.downcast_ref::<&str>() {
                format!("internal error (panic): {s}")
            } else if let Some(s) = payload.downcast_ref::<String>() {
                format!("internal error (panic): {s}")
            } else {
                "internal error (panic): unknown cause".to_string()
            };
            Err(msg.into())
        }
    }
}
```

**Step 2: Wrap VTab bind() methods**

For each of the 18 VTab implementations, wrap the body of `bind()` in catch_unwind_to_result. The pattern:

```rust
fn bind(bind: &BindInfo) -> Result<Self::BindData, Box<dyn std::error::Error>> {
    crate::util::catch_unwind_to_result(std::panic::AssertUnwindSafe(|| {
        // ... existing bind body moved here ...
    }))
}
```

Apply to ALL VTab implementations:
1. `DefineFromJsonVTab` (src/ddl/define.rs) — bind
2. `DropSemanticViewVTab` (src/ddl/drop.rs) — bind
3. `AlterRenameVTab` (src/ddl/alter.rs) — bind
4. `AlterSetCommentVTab` (src/ddl/alter.rs) — bind
5. `AlterUnsetCommentVTab` (src/ddl/alter.rs) — bind
6. `ListSemanticViewsVTab` (src/ddl/list.rs) — bind
7. `ListTerseSemanticViewsVTab` (src/ddl/list.rs) — bind
8. `ShowColumnsInSemanticViewVTab` (src/ddl/show_columns.rs) — bind
9. `ShowSemanticDimensionsVTab` (src/ddl/show_dims.rs) — bind
10. `ShowSemanticDimensionsAllVTab` (src/ddl/show_dims.rs) — bind
11. `ShowDimensionsForMetricVTab` (src/ddl/show_dims_for_metric.rs) — bind
12. `ShowSemanticMetricsVTab` (src/ddl/show_metrics.rs) — bind
13. `ShowSemanticMetricsAllVTab` (src/ddl/show_metrics.rs) — bind
14. `ShowSemanticFactsVTab` (src/ddl/show_facts.rs) — bind
15. `ShowSemanticFactsAllVTab` (src/ddl/show_facts.rs) — bind
16. `DescribeSemanticViewVTab` (src/ddl/describe.rs) — bind
17. `SemanticViewVTab` (src/query/table_function.rs) — bind AND func
18. `ExplainSemanticViewVTab` (src/query/explain.rs) — bind

For `SemanticViewVTab`, also wrap `func()` since it acquires a Mutex and executes SQL (both panic-prone). All other VTab `func()` methods are trivial (swap AtomicBool, copy from pre-computed data) and don't need wrapping — but wrap them too for consistency if straightforward.

For VTab `init()` methods: Most are trivial (`Ok(InitData { done: AtomicBool::new(false) })`). These don't need wrapping. Only wrap `init()` if it does non-trivial work.

**Step 3: Wrap VScalar invoke()**

For `GetDdlScalar` (src/ddl/get_ddl.rs), wrap `invoke()`:

```rust
unsafe fn invoke(
    state: &Self::State,
    input: &mut DataChunkHandle,
    output: &mut dyn WritableVector,
) -> Result<(), Box<dyn std::error::Error>> {
    // Note: catch_unwind requires UnwindSafe. We use AssertUnwindSafe because
    // the FFI types (DataChunkHandle, WritableVector) are borrowed from DuckDB
    // and valid for the call duration.
    crate::util::catch_unwind_to_result(std::panic::AssertUnwindSafe(|| {
        // ... existing invoke body moved here ...
    }))
}
```

**Step 4: Wrap extension init** (src/lib.rs)

In `semantic_views_init_c_api` (line 650), add catch_unwind around the `semantic_views_init_c_api_internal` call:

```rust
pub unsafe extern "C" fn semantic_views_init_c_api(
    info: ffi::duckdb_extension_info,
    access: *const ffi::duckdb_extension_access,
) -> bool {
    let init_result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
        semantic_views_init_c_api_internal(info, access)
    }));

    match init_result {
        Ok(Ok(val)) => val,
        Ok(Err(x)) => {
            let error_c_string = std::ffi::CString::new(x.to_string());
            match error_c_string {
                Ok(e) => {
                    (*access).set_error.unwrap()(info, e.as_ptr());
                }
                Err(_e) => {
                    let error_alloc_failure = c"An error occurred but the extension failed to allocate memory for an error string";
                    (*access).set_error.unwrap()(info, error_alloc_failure.as_ptr());
                }
            }
            false
        }
        Err(_panic) => {
            let panic_msg = c"Extension init panicked unexpectedly. This is a bug in semantic_views.";
            (*access).set_error.unwrap()(info, panic_msg.as_ptr());
            false
        }
    }
}
```

**Step 5: Wrap FFI catalog functions** (src/catalog.rs)

In the `ffi_catalog` module (under `#[cfg(feature = "extension")]`), wrap each of the 4 extern "C" functions:

For `semantic_views_catalog_insert`:
```rust
pub unsafe extern "C" fn semantic_views_catalog_insert(
    catalog_ptr: *const CatalogState,
    name_ptr: *const c_char,
    json_ptr: *const c_char,
) -> i32 {
    let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
        let Some(state) = catalog_ptr.as_ref() else {
            return -1;
        };
        let (Some(name), Some(json)) = (str_from_ptr(name_ptr), str_from_ptr(json_ptr)) else {
            return -1;
        };
        catalog_insert(state, name, json).map(|_| 0).unwrap_or(-1)
    }));
    result.unwrap_or(-1)
}
```

Apply same pattern to `semantic_views_catalog_upsert`, `semantic_views_catalog_delete`, `semantic_views_catalog_delete_if_exists`.

**Important notes:**
- `AssertUnwindSafe` is necessary because VTab method parameters (`&BindInfo`, `&TableFunctionInfo`, etc.) and raw pointers are not `UnwindSafe` by default. This is safe because we are catching the panic at the FFI boundary and converting to an error — we will not observe partially-mutated state.
- The `catch_unwind_to_result` helper handles both `&str` and `String` panic payloads for better error messages.
- Only add `use std::panic::AssertUnwindSafe;` at the top of files that need it.
  </action>
  <verify>
    <automated>cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && cargo test 2>&1 | tail -5</automated>
  </verify>
  <acceptance_criteria>
    - grep -rn 'catch_unwind' src/ddl/ src/query/ src/lib.rs src/catalog.rs | wc -l returns at least 20 (18 VTab binds + VScalar invoke + init + catalog FFI)
    - grep 'catch_unwind_to_result' src/util.rs returns at least 1 match (helper function)
    - grep 'catch_unwind' src/lib.rs returns at least 1 match (extension init)
    - grep 'catch_unwind' src/catalog.rs returns at least 4 matches (FFI catalog functions)
    - grep 'catch_unwind' src/query/table_function.rs returns at least 2 matches (bind + func)
    - grep 'catch_unwind' src/ddl/define.rs returns at least 1 match
    - grep 'catch_unwind' src/ddl/get_ddl.rs returns at least 1 match
    - cargo test passes with 0 failures
  </acceptance_criteria>
  <done>All FFI entry points (18 VTab bind methods, VScalar invoke, extension init, 4 FFI catalog functions, VTab func for SemanticViewVTab) are wrapped in catch_unwind. Panic payloads are converted to descriptive error strings. All existing tests pass.</done>
</task>

</tasks>

<threat_model>
## Trust Boundaries

| Boundary | Description |
|----------|-------------|
| VTab bind/init/func -> DuckDB C++ | Rust panic crossing into C++ = undefined behavior |
| VScalar invoke -> DuckDB C++ | Same boundary as VTab |
| Extension init -> DuckDB C++ | Panic during LOAD = undefined behavior |
| FFI catalog functions -> C++ shim | Panic in catalog mutation = undefined behavior |
| Derived metric expansion | Cyclic definitions bypass CREATE-time validation = infinite loop or stack overflow |

## STRIDE Threat Register

| Threat ID | Category | Component | Disposition | Mitigation Plan |
|-----------|----------|-----------|-------------|-----------------|
| T-49-04 | D (Denial of Service) | VTab bind/init/func | mitigate | Wrap all 18 VTab bind methods + func in catch_unwind_to_result |
| T-49-05 | D (Denial of Service) | VScalar invoke | mitigate | Wrap GetDdlScalar invoke in catch_unwind_to_result |
| T-49-06 | D (Denial of Service) | extension init | mitigate | Wrap semantic_views_init_c_api_internal call in catch_unwind |
| T-49-07 | D (Denial of Service) | FFI catalog functions | mitigate | Wrap all 4 extern "C" catalog functions in catch_unwind |
| T-49-08 | D (Denial of Service) | expand/facts.rs toposort_derived | mitigate | Return Err on cycle detection instead of silent partial result |
| T-49-09 | D (Denial of Service) | expand/facts.rs inline_derived_metrics | mitigate | Enforce MAX_DERIVATION_DEPTH=64 to prevent stack overflow |
| T-49-10 | D (Denial of Service) | expand/sql_gen.rs toposort_facts | mitigate | Propagate toposort_facts Err as ExpandError::CycleDetected instead of .unwrap_or_default() |
</threat_model>

<verification>
```bash
# All tests pass
cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && cargo test

# Full test suite including sqllogictest
just test-all

# Count catch_unwind usage (should be >= 24)
grep -rn 'catch_unwind' src/ddl/ src/query/ src/lib.rs src/catalog.rs src/util.rs | wc -l

# No more silent cycle swallowing
grep 'unwrap_or_default' src/expand/sql_gen.rs
```
</verification>

<success_criteria>
- All 18 VTab bind() methods wrapped in catch_unwind
- VScalar invoke() wrapped in catch_unwind
- Extension init wrapped in catch_unwind
- 4 FFI catalog functions wrapped in catch_unwind
- toposort_derived returns Err on cycles (not silent truncation)
- toposort_facts errors propagated (not .unwrap_or_default())
- MAX_DERIVATION_DEPTH = 64 enforced
- All 487+ existing tests pass
- just test-all passes (including sqllogictest)
</success_criteria>

<output>
After completion, create `.planning/phases/49-security-correctness-hardening/49-02-SUMMARY.md`
</output>
