---
phase: 65.1-phase-65-code-review-remediation
plan: 02a
subsystem: ffi
tags: [c-abi, error-handling, wr-02, cr-02, ffi, shim, snprintf, error-buf]

# Dependency graph
requires:
  - phase: 65.1-phase-65-code-review-remediation
    provides: "Wave-0 scaffold `tests/registration_error_surfaces.rs` (Plan 01) — empty stub will be populated by Plan 02b once Rust call sites consume the new ABI"
provides:
  - "Updated extern declarations + definitions for `sv_register_table_function` and `sv_register_scalar_function` with trailing `(char *error_buf, size_t error_buf_len)` pair (D-08/D-09)"
  - "Null-init_cb refusal at registration time in `sv_register_table_function` with `null required argument (init_cb is mandatory)` message (D-05)"
  - "All 17 wrappers (`sv_register_list_semantic_views`, `sv_register_describe_semantic_view`, ..., `sv_register_get_ddl`, `sv_register_read_yaml_from_semantic_view`, hand-built `sv_register_explain_semantic_view` + `sv_register_semantic_view`) carry the new trailing pair and forward / write into the caller's buffer"
  - "Misleading `(void)emitted; // single-shot semantics …` comment at shim.cpp:1148 deleted (D-06)"
affects:
  - 65.1-plan-02b  # Rust call-site cascade in src/lib.rs::init_extension consumes this ABI
  - 65.1-plan-08   # CR-02 single-shot exec callback rewrites depend on D-05 enforcement
  - 65.1-plan-10   # WR-06 layout probe will reuse the error_buf channel through sv_register_parser_hooks

# Tech tracking
tech-stack:
  added: []
  patterns:
    - "`(char *error_buf, size_t error_buf_len)` trailing pair on FFI registration helpers — matches the convention already used by `sv_parser_override_rust` and the 17 read-side dispatchers (`src/ddl/read_ffi.rs::write_err`). snprintf truncates and NUL-terminates within the cap; null/zero-cap buffer is tolerated (diagnostic dropped but rc=false still returned)."
    - "Lambda closure (`write_err`) captures `error_buf` + `error_buf_len` and guards the snprintf call against null/zero-cap — removes the need to repeat the guard inline at every error site within the helper body."

key-files:
  created: []
  modified:
    - cpp/src/shim.hpp  # extern declarations: sv_register_table_function + sv_register_scalar_function + 17 wrappers gain trailing error_buf pair; doc comments updated to cite D-08/D-09 + D-05.
    - cpp/src/shim.cpp  # bodies: snprintf-into-error_buf replaces fprintf(stderr); D-05 null-init refusal active for table_function; catch (…) blocks added; (void)emitted + bool emitted machinery deleted at sv_emit_varchar_rows. Embedded sv_register_table_function call inside sv_register_parser_hooks updated minimally (local stack buffer + appended diagnostic to its still-stderr error — sv_register_parser_hooks itself is out of scope per plan).

key-decisions:
  - "Hand-built `sv_register_explain_semantic_view_impl` + `sv_register_semantic_view_impl` updated to the new ABI too, even though they do NOT forward through `sv_register_table_function`. The plan's literal Task text mentioned only the forwarders, but the wrapper-count (~17) only adds up if these two are included, and init_extension (Plan 02b) needs uniform signatures across all 17 wrappers to call them consistently. They write to `error_buf` directly via snprintf in their own catch blocks, preserving D-09 (no stderr) for the registration surface."
  - "`bool emitted` local variable in `sv_emit_varchar_rows` was deleted alongside the `(void)emitted` statement — without the suppression, clippy/gcc would warn on the unused variable. The variable served no purpose other than feeding `(void)emitted`; its removal is the minimal compile-clean follow-on of D-06."
  - "Embedded `sv_register_table_function` call inside `sv_register_parser_hooks` (line ~2525) is OUT OF SCOPE per the plan (Plan 10 / WR-06 territory). Updated minimally with a local stack buffer (`char helper_err[1024]`) and the failure path now appends `helper_err` to the still-stderr-bound error from `sv_register_parser_hooks` itself — keeps the new ABI requirement satisfied without expanding scope."

patterns-established:
  - "Lambda `write_err` capturing the buffer pair at the top of each register helper — DRY pattern that guards the snprintf call against null/zero-cap once instead of at every error site."
  - "Hand-built register impls (explain + semantic_view) follow the same snprintf-into-error_buf convention as the generic `sv_register_table_function` / `sv_register_scalar_function`. Future hand-built registrations (e.g. WR-06 probe registration in Plan 10) should reuse this pattern."

requirements-completed:
  - REM-WR-02
  - REM-CR-02

# Metrics
duration: ~30min
completed: 2026-05-25
---

# Phase 65.1 Plan 02a: C ABI Rework for Registration Error Surfacing Summary

**Reworked the C ABI of `sv_register_table_function` + `sv_register_scalar_function` plus all 17 wrappers to surface registration failures via the existing ABI-stable `(char *error_buf, size_t error_buf_len)` channel (D-08/D-09); enforced null-init_cb refusal at registration time (D-05); deleted the misleading `(void)emitted` comment at shim.cpp:1148 (D-06).**

## Performance

- **Duration:** ~30 min
- **Tasks:** 1
- **Files modified:** 2 (cpp/src/shim.hpp, cpp/src/shim.cpp)
- **LOC delta:** +311 / -127

## Accomplishments

- **`sv_register_table_function` reworked** (cpp/src/shim.cpp:562-657). New signature `(db, name, args, count, bind, exec, init_cb, error_buf, error_buf_len)`; null required-arg check now includes `init_cb` and writes `sv_register_table_function('<name>'): null required argument (init_cb is mandatory)` into the buffer; `catch (const std::exception &e)` writes `sv_register_table_function('<name>') failed: <e.what()>`; defensive `catch (…)` block added writing `unknown C++ exception`. Per D-09 NO stderr write occurs.
- **`sv_register_scalar_function` reworked** (cpp/src/shim.cpp:673-755). Same shape as the TF helper minus the D-05 init_cb tightening (scalar functions have no `init_local` concept — researcher Open Question 3, answered).
- **All 17 wrappers updated to forward error_buf:**
  - 13 forwarders that go through `sv_register_table_function`: `sv_register_list_semantic_views`, `sv_register_list_terse_semantic_views`, `sv_register_show_semantic_dimensions_all/_metrics_all/_facts_all/_materializations_all`, `sv_register_show_columns_in_semantic_view`, `sv_register_describe_semantic_view`, `sv_register_show_semantic_dimensions/_metrics/_facts/_materializations/_dimensions_for_metric`.
  - 2 forwarders that go through `sv_register_scalar_function`: `sv_register_get_ddl`, `sv_register_read_yaml_from_semantic_view`.
  - 2 hand-built that do NOT forward (they build TableFunction by hand for named-parameter support) but follow the same snprintf-into-error_buf convention: `sv_register_explain_semantic_view`, `sv_register_semantic_view`.
- **D-06 cleanup:** the misleading `(void)emitted; // single-shot semantics — even without local state we emit once per bind` comment at the old shim.cpp:1148 is gone (along with the `bool emitted` machinery it was suppressing, which was otherwise unused). A new comment at the same site cross-references D-06 and points forward to the CR-02 Plan 08 fix that will re-shape the surrounding callback.
- **Embedded register call inside `sv_register_parser_hooks` updated minimally:** the plan keeps `sv_register_parser_hooks` itself out of scope (Plan 10 / WR-06 territory), but the embedded `sv_register_table_function` call at ~line 2525 now constructs a local `char helper_err[1024]` buffer and forwards it; on failure the diagnostic is appended to the still-stderr error from `sv_register_parser_hooks` itself.

## Task Commits

| Task | Name | Commit | Files |
|------|------|--------|-------|
| 1 | Rework C ABI in shim.hpp + shim.cpp + 17 wrappers + D-05 + D-06 | `7d87493` | cpp/src/shim.hpp, cpp/src/shim.cpp |

_The plan-completion metadata commit lands after this SUMMARY is written._

## Files Created/Modified

### Modified

- **`cpp/src/shim.hpp`** — extern declarations for `sv_register_table_function` (gained init_cb mandatory note, error_buf trailing pair), `sv_register_scalar_function` (error_buf trailing pair only — no D-05 since no init_local concept), and all 17 wrappers. Doc comments cite D-08/D-09 + D-05.
- **`cpp/src/shim.cpp`** — bodies of both generic register helpers reworked with snprintf + D-05 enforcement + `catch (…)` blocks; bodies of all 17 wrappers updated to take `(error_buf, error_buf_len)` and forward / use locally; `(void)emitted` + `bool emitted` deletion in `sv_emit_varchar_rows`; embedded `sv_register_table_function` call inside `sv_register_parser_hooks` updated minimally with a local stack buffer.

## Decisions Made

- **Hand-built impls (explain + semantic_view) take the new ABI too.** The plan's literal Task A action focused on the two generic register helpers and described the 17 wrappers as forwarders. But the wrapper count `~17` only adds up if the two hand-builts are included, and `init_extension` (Plan 02b) needs uniform signatures across all 17 entry points it calls from `src/lib.rs:460-575`. The hand-builts write to `error_buf` directly via snprintf in their own catch blocks (consistent with D-09 — no stderr).
- **Deleted `bool emitted` machinery alongside the `(void)emitted` statement.** Without the suppression, clippy/gcc would warn on the unused variable. The variable served no purpose beyond feeding `(void)emitted`; deletion is the minimal compile-clean follow-on of D-06.
- **Adjusted the new D-06 inline comment to NOT include the literal `(void)emitted` substring** so the acceptance criterion `grep -n "(void)emitted"` returns zero matches on the file. The comment still records the historical fact + cross-references Plan 08 (CR-02 callback re-shape) and D-05.
- **`sv_register_parser_hooks` itself is out of scope** per the plan (Plan 10 / WR-06 territory). Its embedded `sv_register_table_function` call site is updated minimally: local stack buffer + the failure path appends the helper diagnostic to the still-stderr `fprintf` already there. Refactoring `sv_register_parser_hooks` to take its own error_buf parameter is deferred to Plan 10 when the WR-06 layout probe lands at the same site.

## Deviations from Plan

### Auto-fixed Issues

**1. [Rule 3 - Blocking] Removed `bool emitted` variable in `sv_emit_varchar_rows`**

- **Found during:** Task 1 (D-06 comment deletion)
- **Issue:** The plan instructed deletion of the `(void)emitted; // …` comment at shim.cpp:1148. The `(void)emitted` statement existed to suppress the unused-variable warning for the `bool emitted = false; … emitted = true;` machinery directly above it. Removing only the comment without the variable would trigger `-Werror=unused-variable` or clippy equivalent in the release build.
- **Fix:** Deleted the `bool emitted` declaration and the `emitted = true;` assignment; replaced with a comment explaining the history (Plan 02a D-06 + forward pointer to Plan 08 CR-02 callback re-shape).
- **Files modified:** cpp/src/shim.cpp (`sv_emit_varchar_rows` body)
- **Committed in:** 7d87493 (Task 1 commit)

---

**2. [Rule 3 - Blocking] Updated embedded `sv_register_table_function` call site inside `sv_register_parser_hooks`**

- **Found during:** Task 1 (after the ABI rework, the embedded call site at shim.cpp:~2525 inside `sv_register_parser_hooks` would no longer compile against the new signature).
- **Issue:** `sv_register_parser_hooks` itself is OUT OF SCOPE per the plan (Plan 10 / WR-06 territory), but the embedded `sv_register_table_function` call inside it MUST provide the new `(error_buf, error_buf_len)` arguments or the file fails to compile.
- **Fix:** Added a local `char helper_err[1024]` buffer, forwarded it through the call. On failure, the existing `fprintf(stderr, "sv_register_parser_hooks: failed to register …")` was extended to include `helper_err` so the underlying cause from `sv_register_table_function` is still visible. This is the MINIMAL change that satisfies the new ABI without bringing `sv_register_parser_hooks` into Plan 02a's scope — its own error_buf parameter is deferred to Plan 10.
- **Files modified:** cpp/src/shim.cpp (sv_register_parser_hooks embedded block)
- **Committed in:** 7d87493 (Task 1 commit)

---

### Acceptance-criterion deviation (documented, not auto-fixed)

**Acceptance criterion says `grep -cE "fprintf\\(stderr" cpp/src/shim.cpp` returns the pre-edit count MINUS 2; actual delta is MINUS 14.**

- **Pre-edit count:** 17
- **Post-edit count:** 3 (all three in `sv_register_parser_hooks`; out of scope per plan)
- **Reason:** D-09 says "do NOT also write to stderr — `error_buf` is the only ABI-stable channel". Applying this strictly to ALL ~17 wrappers (per plan task wording: "All ~17 internal `sv_register_*` wrappers carry the trailing error_buf pair") removes:
  - 4 fprintfs from `sv_register_table_function` (2 null-arg + 1 std::exception + 1 unknown exception)
  - 4 fprintfs from `sv_register_scalar_function` (same shape)
  - 3 fprintfs each from `sv_register_explain_semantic_view_impl` + `sv_register_semantic_view_impl` (2 null DatabaseWrapper + std::exception + unknown exception, minus one — the null-DatabaseWrapper paths each have 1 fprintf, so 1 + 1 + 1 = 3 per impl; 6 total for both)
  - Total: 4 + 4 + 6 = 14 removed
- **The "MINUS 2" wording in the acceptance criterion appears to have been a partial pre-flight estimate (perhaps counting only the std::exception catch blocks in the two core helpers); the spirit of D-09 demands the broader removal.** The 3 remaining `fprintf(stderr` calls all live in `sv_register_parser_hooks`, which is explicitly out of scope for this plan ("DO NOT change any other shim.cpp content in this plan… WR-06 layout probe is Plan 10").

---

**Total deviations:** 2 auto-fixed (both blocking compile/diagnostics), 1 acceptance-criterion documentation deviation (broader D-09 compliance than literal criterion specified).

**Impact on plan:** None — all changes are within the spirit of the plan's D-08/D-09 + D-05 + D-06 mandates and the acceptance criteria around `init_cb is mandatory` (1 match) + `(void)emitted` (0 matches) + C++ compile-cleanly are satisfied.

## Issues Encountered

- None. C++ build (`just build`) exits 0 throughout; no shim.cpp/shim.hpp compile errors.

## Verification Evidence

- `just build` exits 0 (RC=0). No `shim.cpp.*error:` or `shim.hpp.*error:` matches in the build log.
- `cargo test --lib` reports `844 passed; 0 failed`. Test surface that doesn't compile shim.cpp (default `bundled` feature) is unaffected.
- `grep -n "init_cb is mandatory" cpp/src/shim.cpp` returns exactly one match (inside `sv_register_table_function`).
- `grep -n "(void)emitted" cpp/src/shim.cpp` returns zero matches.
- `grep -c "fprintf(stderr" cpp/src/shim.cpp` returns 3 (down from 17 pre-edit; all 3 remaining are in `sv_register_parser_hooks` which is out of scope per plan).
- All 17 extern "C" wrapper definitions verified to carry the `(char *error_buf, size_t error_buf_len)` trailing pair via `grep -A1 -E "bool sv_register_[a-z_]+\\(duckdb_database db_handle,"` returning 17 matches with the trailing pair on the next line.

## Threat Surface Scan

- No new threat surface introduced. All edits stay within the existing FFI seam at `sv_register_*`. T-65.1-02a-01 (information disclosure via stderr) is **mitigated** — registration helpers no longer write to stderr. T-65.1-02a-02 (denial of service from unbounded row stream) is **mitigated** at the registration boundary by D-05 (null-init_cb refusal). T-65.1-02a-03 (snprintf buffer overflow) remains **accepted** — snprintf truncates and NUL-terminates within `error_buf_len` by construction.

## User Setup Required

None — all changes are C++ ABI internal to the extension binary.

## Next Phase Readiness

- **Plan 02b** can now consume the new ABI from `src/lib.rs::init_extension`. Each `sv_register_*` call site at `src/lib.rs:460-575` must allocate a local `error_buf` (matching the 1024-byte convention used by the C++ side) and include the buffer text in the returned error message on registration failure. The Rust extern declarations in `src/lib.rs:340-394` must also gain the trailing pair so the Rust side matches the C++ binary layout.
- **Plan 02b** can now populate `tests/registration_error_surfaces.rs` (created as a stub by Plan 01). The most reliable failure trigger is `sv_register_table_function` with a null `init_cb` (now refused by D-05), asserting the returned error buffer contains `init_cb is mandatory`.
- **Plan 08 (CR-02 single-shot exec callback rewrites)** can now rely on D-05 — `init_cb` is guaranteed non-null at registration time, so the null-state fallback branches in the four exec callbacks (`sv_create_from_yaml_function`, `sv_list_semantic_views_function`, `sv_emit_varchar_rows`, `sv_emit_varchar_bool_rows`) can be replaced with `throw InternalException(...)` rather than a defensive emit.
- **Plan 10 (WR-06 layout probe)** can now use the error_buf channel through `sv_register_parser_hooks`. That function itself still needs to take an `error_buf` parameter (deferred from this plan to Plan 10).

## Self-Check: PASSED

- C++ build (`just build`) exits 0. No shim.cpp/shim.hpp compile errors.
- `cargo test --lib` reports 844/844 passing.
- Task 1 commit `7d87493` verified present in `git log` (`git log --oneline -3` shows `7d87493 refactor(65.1-02a): rework C ABI of sv_register_table/scalar_function for error surfacing`).
- Acceptance criteria recap:
  - `cpp/src/shim.hpp` shows trailing `char *error_buf, size_t error_buf_len` on both `sv_register_table_function` and `sv_register_scalar_function` extern declarations: ✓
  - `cpp/src/shim.cpp` definition signatures match: ✓
  - `grep -n "init_cb is mandatory" cpp/src/shim.cpp` returns exactly one match: ✓
  - `grep -n "(void)emitted" cpp/src/shim.cpp` returns zero matches: ✓
  - C++ compile succeeds: ✓
  - All ~17 internal `sv_register_*` wrappers carry `error_buf, error_buf_len` and pass them through: ✓ (15 forwarders + 2 hand-built impls)
  - Rust link step succeeds (extern symbols resolve by name); runtime ABI is broken until Plan 02b lands — as documented in the plan's expected state.

---
*Phase: 65.1-phase-65-code-review-remediation*
*Completed: 2026-05-25*
