---
phase: 65.1-phase-65-code-review-remediation
plan: 08
type: execute
wave: 3
depends_on:
  - 65.1-02b
files_modified:
  - cpp/src/shim.cpp
autonomous: true
requirements:
  - REM-CR-02
  - REM-IN-01

must_haves:
  truths:
    - "Four TF exec callbacks (`sv_create_from_yaml_function`, `sv_list_semantic_views_function`, `sv_emit_varchar_rows`, `sv_emit_varchar_bool_rows`) have NO `if (state_p == nullptr) { ... emit-anyway }` fallback branch — the only `nullptr` case throws `InternalException`"
    - "Each of the four callbacks emits via the converged single-shot pattern: `if (state.emitted) return cardinality 0; ... emit ...; state.emitted = true;`"
    - "The misleading `(void)emitted; // single-shot semantics — even without local state we emit once per bind` comment is gone (D-06 deletion landed in Plan 02 Task 1; this plan confirms it stays gone after the callback rewrite)"
    - "The stale comment block at `cpp/src/shim.cpp:830-844` (IN-01) is rewritten to reflect that `init_local` IS wired in and registration enforces it (per Plan 02's D-05 enforcement)"
    - "No new build/test failures introduced; existing single-bind sqllogictests still pass"
  artifacts:
    - path: cpp/src/shim.cpp
      provides: "Four converged single-shot exec callbacks; null-state path throws InternalException; IN-01 comment refreshed"
      contains: "registration must supply init_local callback"
  key_links:
    - from: cpp/src/shim.cpp::sv_emit_varchar_rows (and 3 siblings)
      to: cpp/src/shim.cpp::sv_register_table_function (D-05 null-init refusal in Plan 02)
      via: "registration-time refusal makes the null-state branch unreachable; throw is defence in depth"
      pattern: "InternalException.*registration must supply init_local"
---

<objective>
Rewrite the four TF exec callbacks identified by CR-02 to converge on the same single-shot emit shape demonstrated by `sv_emit_varchar_rows` (after cleanup). Each callback:

1. Checks `data_p.local_state.get()` — if null, throws `InternalException("<fn>: local_state missing despite init_local registration")`. NEVER emits a fallback row stream (CR-02 hazard root cause).
2. Casts the local state to its concrete type.
3. Tests `state.emitted` — if true, sets `output.SetCardinality(0)` and returns.
4. Emits the rows.
5. Sets `state.emitted = true`.

Plan 02 already enforced D-05 at registration time (refuse null `init_cb`) and deleted the `(void)emitted` comment at `cpp/src/shim.cpp:1148` (D-06). This plan completes CR-02 by removing the unreachable fallback branches in the four call sites and aligning their shape.

Plus IN-01 (D-07) — update the stale comment block at `cpp/src/shim.cpp:830-844`. Per the original review: the comment recommends "flip the registration to use the init_local path" which is now stale (init_local IS wired and registration refuses null-init_cb). Refresh the comment to reflect current state and cite Phase 65.1 D-07.

Wave 2 because it shares `cpp/src/shim.cpp` with Plan 02 (WR-02 ABI rework lands first in Wave 1).

Implements D-04 (CR-02 fallback removal), D-07 (IN-01 comment refresh). D-05 + D-06 land in Plan 02.
</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/STATE.md
@.planning/phases/65.1-phase-65-code-review-remediation/65.1-CONTEXT.md
@.planning/phases/65.1-phase-65-code-review-remediation/65.1-RESEARCH.md
@.planning/phases/65.1-phase-65-code-review-remediation/65.1-PATTERNS.md
@cpp/src/shim.cpp
</context>

<interfaces>
Four sites (RESEARCH.md lines 313-319):

| Function | Line range | Current fallback to delete |
|----------|------------|---------------------------|
| sv_create_from_yaml_function | 825-856 | Lines 837-847 — emits single row unconditionally if state is null |
| sv_list_semantic_views_function | 1023-1059 | Lines 1029-1042 — emits all rows unconditionally if state is null |
| sv_emit_varchar_rows | 1132-1157 | Old `(void)emitted` pattern (comment deleted by Plan 02; restructure callback here) |
| sv_emit_varchar_bool_rows | adjacent to varchar_rows | Same shape as varchar_rows |

Target shape (PATTERNS.md lines 102-133):

```
static void sv_emit_varchar_rows(
    ClientContext & /*context*/,
    TableFunctionInput &data_p,
    DataChunk &output) {
    auto &bd = data_p.bind_data->Cast<SvVarcharBindData>();
    auto *state_p = data_p.local_state.get();
    if (state_p == nullptr) {
        // Registration refuses null init_cb at registration time per Phase 65.1
        // D-05 enforcement (cpp/src/shim.cpp sv_register_table_function). Reaching
        // this branch indicates corrupted TableFunctionInput; fail loud rather
        // than risk an unbounded row stream (Phase 65.1 D-04, CR-02).
        throw InternalException(
            "sv_emit_varchar_rows: local_state missing despite init_local registration");
    }
    auto &state = state_p->Cast<SvVarcharLocalState>();
    if (state.emitted) {
        output.SetCardinality(0);
        return;
    }
    idx_t n = bd.rows.size();
    for (idx_t i = 0; i < n; ++i) {
        const auto &row = bd.rows[i];
        for (size_t c = 0; c < row.size(); ++c) {
            output.SetValue(c, i, Value(row[c]));
        }
    }
    output.SetCardinality(n);
    state.emitted = true;
}
```

IN-01 site: `cpp/src/shim.cpp:830-844` — the stale comment block. Original text per the review recommended flipping registration to init_local path; current state is that init_local IS used. Refresh the comment.
</interfaces>

<tasks>

<task type="auto">
  <name>Task 1: Converge four exec callbacks on the single-shot shape</name>
  <read_first>
    - cpp/src/shim.cpp lines 820-870 (sv_create_from_yaml_function)
    - cpp/src/shim.cpp lines 1020-1070 (sv_list_semantic_views_function)
    - cpp/src/shim.cpp lines 1130-1170 (sv_emit_varchar_rows + its bool sibling sv_emit_varchar_bool_rows — find via grep)
    - .planning/phases/65.1-phase-65-code-review-remediation/65.1-PATTERNS.md lines 99-140 (target shape — verbatim template)
    - .planning/phases/65.1-phase-65-code-review-remediation/65.1-RESEARCH.md lines 311-332 (the four-site enumeration)
  </read_first>
  <files>cpp/src/shim.cpp</files>
  <action>
For each of the four callbacks (find precise line ranges by grep on the function names):

1. **Delete the `if (state_p == nullptr) { ... emit-anyway ... }` fallback branch entirely** — the branch that emits one row (or N rows) WITHOUT touching `state.emitted` because `state_p` is null. This is the CR-02 hazard.

2. **Replace with the target shape from `<interfaces>` above**:
   - `auto *state_p = data_p.local_state.get();`
   - `if (state_p == nullptr) { throw InternalException("<fn-name>: local_state missing despite init_local registration"); }`
   - Cast to concrete state type
   - `if (state.emitted) { output.SetCardinality(0); return; }`
   - Emit rows (preserve existing row-emission code verbatim)
   - `state.emitted = true;`

3. **For `sv_emit_varchar_bool_rows`** — same structural rewrite, but the row data has a trailing bool column; preserve that columnar emission as-is, just wrap in the same single-shot scaffold.

4. The function-name string inside the `throw InternalException(...)` must MATCH the function name (so stack traces are accurate). Use the actual symbol name verbatim.

5. DO NOT touch the bind callbacks for these TFs. Only the exec callbacks. Bind sets up `local_state`; exec consumes it. The fix is exec-only.

6. After rewriting all four, recompile and run the existing sqllogictests that exercise these TFs:
   - `sv_create_from_yaml_function` → exercised by `phase53_yaml_file.test` (post-Plan-07-Task-2 form, 3-arg call)
   - `sv_list_semantic_views_function` → exercised by `phase37_show_list.test` (or similar — find via grep on `list_semantic_views`)
   - `sv_emit_varchar_rows` / `sv_emit_varchar_bool_rows` → exercised by most SHOW commands (e.g. `phase34_1_1_show_filtering.test`)

   All must remain green — the converged shape is functionally identical to the existing happy path (the fallback was unreachable in any working setup; init_local was always wired).

7. Build incrementally per CLAUDE.md Rule 1 (redirect to file then tail).
  </action>
  <verify>
    <automated>just build &gt; /tmp/claude/p08_t1_build.log 2>&amp;1; RC=$?; tail -60 /tmp/claude/p08_t1_build.log; test $RC -eq 0 &amp;&amp; just test-sql &gt; /tmp/claude/p08_t1_sql.log 2>&amp;1; RC2=$?; tail -50 /tmp/claude/p08_t1_sql.log; test $RC2 -eq 0</automated>
  </verify>
  <acceptance_criteria>
    - `grep -nE "local_state missing despite init_local registration" cpp/src/shim.cpp | wc -l` returns at least 4 (one per converged callback; the function names in the throw text differ)
    - The four target functions each contain exactly ONE `state.emitted = true;` line
    - No callback has both an "emit if null state" branch AND a "emit if not emitted" branch — every callback has exactly one emission path gated on `state.emitted`
    - `grep -nE "single-shot semantics — even without local state we emit once per bind" cpp/src/shim.cpp` returns zero matches (the D-06 comment from Plan 02 stays gone)
    - `just build` exits 0
    - `just test-sql` exits 0 — all existing SHOW/CREATE FROM YAML FILE / list tests still pass
  </acceptance_criteria>
  <done>CR-02 closed: all four callbacks single-shot via converged shape; null-state throws loud; no row-stream hazard.</done>
</task>

<task type="auto">
  <name>Task 2: IN-01 refresh stale comment at cpp/src/shim.cpp:830-844</name>
  <read_first>
    - cpp/src/shim.cpp lines 820-870 (the current comment block around 830-844 above sv_create_from_yaml_function)
    - .planning/phases/65.1-phase-65-code-review-remediation/65.1-CONTEXT.md (D-07)
    - .planning/phases/65-overridecontext-connection-teardown/65-REVIEW.md (IN-01 finding for the original stale-recommendation text)
  </read_first>
  <files>cpp/src/shim.cpp</files>
  <action>
Locate the comment block at approximately `cpp/src/shim.cpp:830-844`. Per the original review (IN-01), this block recommends "flip the registration to use the init_local path" — a recommendation that is now stale because init_local IS wired and the registration helper (Plan 02 Task 1) refuses null `init_cb`.

Replace the comment block with a refreshed version that:

1. States current ground truth: `init_local` callback is wired for this TF (the `state.emitted` single-shot machinery used by the matching exec callback depends on it).
2. Cites the registration-time refusal of null `init_cb` (Phase 65.1 D-05, landed in Plan 02 — see `sv_register_table_function`).
3. Removes the obsolete recommendation entirely.
4. Cites Phase 65.1 D-07 for traceability.

The new comment should be a short paragraph (4-8 lines) and live in the same location (just above whatever declaration the original comment annotated). Do NOT move it.

If the original comment annotates `sv_create_from_yaml_function`, the refresh should explain: "This TF's single-shot emission is enforced via init_local (`<init_local_function_name>` in this file at line <N>). Registration via `sv_register_table_function` refuses null init_cb at registration time per Phase 65.1 D-05; reaching the InternalException branch in the exec callback below is a corrupted-state diagnostic, not a normal failure mode."
  </action>
  <verify>
    <automated>just build &gt; /tmp/claude/p08_t2_build.log 2>&amp;1; RC=$?; tail -30 /tmp/claude/p08_t2_build.log; test $RC -eq 0</automated>
  </verify>
  <acceptance_criteria>
    - The stale "flip the registration to use the init_local path" recommendation text is gone from `cpp/src/shim.cpp` (`grep -n "flip the registration" cpp/src/shim.cpp` returns zero matches)
    - A refreshed comment exists at the same location citing init_local being wired AND citing Phase 65.1 D-05 / D-07
    - `just build` exits 0
  </acceptance_criteria>
  <done>IN-01 stale comment replaced with current-state description; no behavioural change.</done>
</task>

</tasks>

<threat_model>
## Trust Boundaries

| Boundary | Description |
|----------|-------------|
| TF exec callback invocation | DuckDB invokes exec with TableFunctionInput; null local_state would be a corrupted-state signal |

## STRIDE Threat Register

| Threat ID | Category | Component | Disposition | Mitigation Plan |
|-----------|----------|-----------|-------------|-----------------|
| T-65.1-08-01 | Denial of service | Unbounded row stream from missing state.emitted check | mitigate | Remove fallback branch entirely; throw InternalException on null state (D-04) |
| T-65.1-08-02 | Repudiation | Silent emit-anyway hides registration bugs | mitigate | Loud throw + registration-time refusal (Plan 02 D-05) ensures any state corruption surfaces immediately |
</threat_model>

<verification>
  <automated>just test-all &gt; /tmp/claude/p08_all.log 2>&amp;1; RC=$?; tail -100 /tmp/claude/p08_all.log; test $RC -eq 0</automated>
</verification>

<success_criteria>
- Four TF exec callbacks converged on single-shot shape
- Null `state_p` path throws InternalException with function-name string (loud failure)
- IN-01 stale comment refreshed
- `just test-all` green
</success_criteria>

<output>
Create `.planning/phases/65.1-phase-65-code-review-remediation/65.1-08-SUMMARY.md` when done.
</output>
