perf(notecard): Collapse execute() prologue to non-template core

execute<RequestT>() is instantiated once per request type, so every branch in its body was code-generated and coverage-counted per type. Move the type-independent prologue — begin/end timing and the transport/alloc/backend path selection — into non-template begin_execute()/end_execute() members, so those branches (and the inlined debug_timing null-checks) are emitted once rather than per instantiation. Only the type-dependent execute_streaming/ execute_tree calls remain in the template.

Behaviour is unchanged (still one TransactionBegin + one TransactionEnd per call; identical error strings; request-id bump only on the buffered path). Host suite + full HIL sweep (serial + i2c on MPCB 1.9) pass; AVR unaffected (StaticNotecard is a separate, already-collapsed path).

Raises raw branch coverage 89.6% -> 92.1%; MIN_BRANCH_COV restored 89 -> 92. ci.sh documents why 93 is not stably reachable (the raw metric counts inlined-per-instantiation branch copies; see DESIGN notes).

Author: m-mcgowan

ci.sh

@@ -488,17 +488,31 @@ MIN_FUNC_COV=90
 # Line (96.3%) and function (97.7%) — the logic-level signals — stay well above
 # their floors.
 #
-# EXPLORATION (design session pending — see DESIGN-branch-coverage-floor.md):
-# reaching 93 again is NOT a small fix. The gap from ~89.6 to 93 is ~430 branch
-# records; the known collapse refactors (body_handler_factory type-erasure ~18,
-# struct_sink make_body_handler switch ~18) recover far less. The session should
-# decide between: (a) an instantiation-stable branch metric (de-inflate BRDA to
-# source branches, mirroring the function check's per-file FNF/FNH), (b) broader
-# template-collapse refactors, (c) a library-wide pass of targeted error-path
-# branch tests, or (d) accepting a lower floor as the honest reality for a
-# heavily-templated header lib and documenting why. Until then, 89 holds the
-# line at the measured value so the gate stays meaningful, not green-by-default.
-MIN_BRANCH_COV=89
+# RESOLVED (2026-06, see DESIGN-branch-coverage-floor.md): the investigation
+# concluded that the raw BRDA metric is fundamentally instantiation-noisy on this
+# header-template library — it counts (instantiation x branch) tuples, so the
+# number moves unpredictably as GCC regenerates instantiations. Proven three ways:
+# collapsing retry_transaction LOWERED it (-2.7, type-erasure adds uncovered
+# per-instantiation trampolines); adding generated per-op retry tests LOWERED it
+# (-0.6) EVEN with retries correctly enabled — retry_transaction is inlined into
+# each execute<T>, so extra test call sites multiply the inlined branch copies
+# (notecard.hpp L339) faster than the tests cover them (retry.hpp itself hit
+# 94%, but the inlined copies dominate); and adding the lock tests lowered it
+# earlier (-1.4). The de-inflated source-level metric (~90%) is the only stable
+# measure.
+#
+# What worked: collapsing Notecard::execute()'s type-independent prologue
+# (timing + transport/alloc/backend selection) into the non-template
+# begin_execute()/end_execute() — those branches are now emitted and measured
+# ONCE instead of per request-type, lifting raw branch 89.6% -> 92.1%.
+#
+# Floor set to 92 on the raw metric (the post-collapse value). Reproducible for
+# the current tree; the documented risk is that FUTURE test/code additions can
+# shift instantiations and dip it below 92 (the metric's noise, not a real
+# regression). If that becomes a recurring annoyance, switch to the
+# instantiation-stable de-inflated metric (gcov --json fold; prototype validated
+# at ~90%) and floor there — it is immune to the pathology.
+MIN_BRANCH_COV=92
 
 check_coverage_thresholds() {
     local lcov_file="$1"

src/note/notecard.hpp

@@ -319,45 +319,38 @@ class Notecard {
     ApiResult<typename RequestT::Response> execute(const RequestT& req) {
         return run_operation([&]() -> ApiResult<typename RequestT::Response> {
             using Rsp = typename RequestT::Response;
-
-            debug_timing(debug_, TimingEvent::TransactionBegin, RequestT::notecard_request);
-
-            if (!transport_) {
-                debug_timing(debug_, TimingEvent::TransactionEnd, RequestT::notecard_request);
-                return Unexpected(make_error(Error::NotReady, NOTE_ERR("no transport configured")));
-            }
-
-            // Streaming path: SAX-stream the response straight into the
-            // typed Rsp::Sink, no intermediate tree. Parallel to
-            // execute_tree below — the two are siblings, picked at this
-            // call site by (a) whether the response type has a sink and
-            // (b) whether an allocator is configured.
-            if constexpr (std::is_void_v<Rsp> || detail::has_sink<Rsp>::value) {
-                if (alloc_.has_value()) {
-                    auto result = execute_streaming(req);
-                    debug_timing(debug_, TimingEvent::TransactionEnd, RequestT::notecard_request);
-                    return result;
+            constexpr bool can_stream =
+                std::is_void_v<Rsp> || detail::has_sink<Rsp>::value;
+
+            // Type-independent prologue (timing + path selection) — see
+            // begin_execute(). Only the type-dependent calls below stay here.
+            ExecDecision d = begin_execute(can_stream, RequestT::notecard_request);
+
+            ApiResult<Rsp> result = [&]() -> ApiResult<Rsp> {
+                // Streaming path: SAX-stream the response straight into the
+                // typed Rsp::Sink, no intermediate tree.
+                if constexpr (can_stream) {
+                    if (d.path == ExecPath::Streaming) {
+                        return execute_streaming(req);
+                    }
                 }
-            }
-
-            // Buffered fallback: requires a JsonBackend + buffered transport.
+                // Buffered fallback: JsonBackend + buffered transport, retried.
 #if !NOTE_NO_JSON_TREE
-            if (backend_) {
-                const uint32_t req_id = request_ids_enabled_ ? next_request_id_++ : 0;
-                auto attempt = [&]() -> ApiResult<Rsp> {
-                    return execute_tree(req, req_id);
-                };
-                auto reset = [&]() { transport_->reset(); };
-
-                auto result = retry_transaction<ApiResult<Rsp>>(
-                    hal(), timing_, RequestT::safety, retry_policy_,
-                    attempt, reset);
-                debug_timing(debug_, TimingEvent::TransactionEnd, RequestT::notecard_request);
-                return result;
-            }
+                if (d.path == ExecPath::Buffered) {
+                    auto attempt = [&]() -> ApiResult<Rsp> {
+                        return execute_tree(req, d.req_id);
+                    };
+                    auto reset = [&]() { transport_->reset(); };
+                    return retry_transaction<ApiResult<Rsp>>(
+                        hal(), timing_, RequestT::safety, retry_policy_,
+                        attempt, reset);
+                }
 #endif
-            debug_timing(debug_, TimingEvent::TransactionEnd, RequestT::notecard_request);
-            return Unexpected(make_error(Error::NotReady, NOTE_ERR("no backend or streaming transport configured")));
+                return *d.error;
+            }();
+
+            end_execute(RequestT::notecard_request);
+            return result;
         });
     }
 
@@ -940,6 +933,50 @@ class Notecard {
         return request_ids_enabled_ ? next_request_id_++ : 0;
     }
 
+    // ── Type-independent execute() core ─────────────────────────────────────
+    // execute<RequestT>() is instantiated once per request type, so any branch
+    // in its body is counted (and code-generated) once per type. The config
+    // checks below (transport/streaming/buffered selection, the debug-timing
+    // null checks) are identical across every RequestT, so they live here in
+    // non-template members: emitted and measured once, not per instantiation.
+    // Only the genuinely type-dependent calls (execute_streaming/execute_tree)
+    // stay in the template. See DESIGN-branch-coverage-floor.md.
+    enum class ExecPath : uint8_t { Streaming, Buffered, Error };
+    struct ExecDecision {
+        ExecPath               path = ExecPath::Error;
+        uint32_t               req_id = 0;
+        std::optional<Unexpected> error;   // set iff path == Error
+    };
+
+    // Prologue: emit begin-timing, validate config, choose the dispatch path.
+    // `can_stream` is the compile-time streamability of the response type.
+    ExecDecision begin_execute(bool can_stream, string_view req_name) {
+        debug_timing(debug_, TimingEvent::TransactionBegin, req_name);
+        ExecDecision d;
+        if (!transport_) {
+            d.error = make_error(Error::NotReady, NOTE_ERR("no transport configured"));
+            return d;
+        }
+        if (can_stream && alloc_.has_value()) {
+            d.path = ExecPath::Streaming;
+            return d;
+        }
+#if !NOTE_NO_JSON_TREE
+        if (backend_) {
+            d.path = ExecPath::Buffered;
+            d.req_id = next_request_id_or_zero();
+            return d;
+        }
+#endif
+        d.error = make_error(Error::NotReady, NOTE_ERR("no backend or streaming transport configured"));
+        return d;
+    }
+
+    // Epilogue: emit end-timing. Called once per execute(), regardless of path.
+    void end_execute(string_view req_name) {
+        debug_timing(debug_, TimingEvent::TransactionEnd, req_name);
+    }
+
     /// Allocator-backed durable copy of a Notecard error message — used
     /// by the Api singleton thunk path to give `NcErrorCapture::view()`
     /// a pointer that outlives the caller-stack-allocated NcErrorCapture