Type erased C++ API and related code

[hhenson]

This is hopefully the last attempt to create the new C++ API to make the code work in a more C++ appropriate way. This leverages off of the prior experiments and hopefully brings a more readable and debuggable code implementation.

NOTE: THIS IS STILL A WORK IN PROGRESS

[claude]

Code Review — PR #251: Type-erased C++ API

Note: This PR is marked as WIP, so this review is intended as early feedback rather than a gate.

---

Overview

This is a substantial architectural rework of the C++ runtime layer. The core idea is a shift from virtual dispatch + deep builder hierarchies to a struct-of-function-pointers (ops-table) type-erasure pattern, combined with compile-time reflection via StaticNodeSignature<T>. The net result is −193 deleted C++ header files replaced by ~41 denser, more capable ones, and a −119k line net reduction when including the removed state_trace.json test artifact.

The design direction is sound. The rest of this review covers specific concerns worth addressing before the PR is marked ready.

---

Architecture & Design

Strengths

• The ops-table pattern ({void* impl, const Ops* ops}) in evaluation_engine.h, node.h, and evaluation_clock.h is the right call for ABI stability and for eliminating per-instance vtable overhead.
• StaticNodeSignature<T> in static_signature.h is an elegant single source of truth — input/output schemas, activity policies, and the Python WiringNodeSignature all derive automatically from a plain C++ struct. This eliminates the per-node builder boilerplate that the old architecture required.
• Removing state_trace.json (144k lines) and state_trace_analysis.md from the test tree is good hygiene.

Concerns

1. tagged_ptr.h at 599 lines is doing a lot. Discriminated pointer types are useful, but this is a large, complex utility with no dedicated unit test file in the PR. Given how widely it's used through the value and TS layers, a standalone test for the pointer tag semantics (type identity, null handling, copy/move) would reduce risk.

2. ts_view.h (+1176) and time_series_state.h (+832) are very large single headers. These encode the complete view/state surface for all collection types (TSB, TSD, TSL, TSS, TSW) in a unified hierarchy. Consider whether these can be split by TS type at file level without sacrificing the unified interface — large headers have a compile-time cost and make diffs harder to reason about.

3. active_trie.h evict_to_pending / resolve_pending — the TSD structural-change path is subtle. The current test (test_active_trie.cpp) exercises the basic trie; it's unclear whether structural eviction under active inputs is covered. A dedicated test case for the "evict then resolve" path would be valuable.

---

Specific Issues

1. Committed build artifacts — blocker

cpp/tests/asan-build/ contains committed CMake-generated files. These must not be in version control. Please add the directory to .gitignore and remove the files before merge.

2. ConstNode::eval Python round-trip (acknowledged TODO)

// basic_nodes.h — ConstNode
void eval(Out<TS<nb::object>> out, PythonScalarArg<"value"> value) {
    out.from_python(value.object()); // TODO: direct value round-trip
}

The nb::object round-trip is fine for correctness but bypasses the typed value layer. This is a known gap, but worth flagging because it means ConstNode currently can't benefit from the zero-copy path for C++ scalar types that the value layer is designed to provide.

3. EngineEvaluationClockOps alarm scheduling — self-described code smell

// evaluation_clock.h
struct EngineEvaluationClockOps {
    // CODE SMELL: alarm scheduling shouldn't live on the clock
    void (*set_alarm)(void*, engine_time_t) = nullptr;
    void (*cancel_alarm)(void*) = nullptr;
};

Agreed with the self-assessment. If this is a known interim design, adding a tracking issue reference in the comment would help future maintainers understand the intent and avoid building on top of the interim placement.

4. upgrade_python_wiring_node() — __class__ mutation risk

# python_export.h -> CppStaticWiringNodeClass
nb::setattr(wiring_node, "__class__", cpp_static_wiring_node_cls)

CPython __class__ assignment requires the memory layout of the old and new types to be compatible (same __basicsize__, no __slots__ mismatch). The current implementation checks for PreResolvedWiringNodeWrapper and overload list rank, but there is no explicit guard against layout incompatibility. Consider adding an assertion or a PyObject_IsInstance check against an expected base before mutating. The new test test_cpp_v2_node_overrides.py covers the happy path but not the mismatched-layout case.

5. _use_cpp_runtime.py — silent NotImplementedError on mixed graphs

# _use_cpp_runtime.py
if not isinstance(graph_builder, _hgraph.GraphBuilder):
    raise NotImplementedError("Mixed builder graphs not supported")

This is better than the old silent wrapping, but the error message gives no guidance on how to fix it (e.g., "ensure use_cpp: true is set in hgraph_features.yaml"). A slightly more descriptive message would help users who hit this.

---

Test Coverage

Coverage has improved significantly with the new wiring/static-node tests, but there are gaps:

Area	Status
tagged_ptr.h discriminated pointer	No dedicated tests
BoundaryBindingPlan bind/unbind/rebind paths	No tests
ActiveTrie evict-to-pending path	Unclear from PR
TSInput/TSOutput lifecycle in isolation	No tests (only graph-level)
test_tracked_set.py (deleted)	Not replaced
test_memory_size.py (deleted)	Not replaced

The new conftest.py auto-skip for HGRAPH_USE_CPP=0 is a good addition — it prevents false failures in pure-Python environments.

---

Performance

• The ops-table pattern correctly avoids vtable-per-instance overhead.
• arg_provider<T> specializations with std::make_index_sequence expansion in invoke<Fn>() should compile down to a single inline call site — good.
• ActiveTrie uses std::unordered_map<size_t, std::unique_ptr<ActiveTrieNode>> for children. For small fan-outs (typical in TSB), a small flat array or absl::flat_hash_map might reduce allocation pressure. Not urgent, but worth noting for later.
• The state_trace.json removal is a significant improvement to clone/checkout times.

---

Minor / Nits

• Several design RFCs in docs/design/ use future tense ("will be implemented"). Once the work they describe lands, these should be updated or flagged as historical. A naming convention like docs/design/rfc/ vs docs/design/decisions/ might help.
• static_schema.h and static_signature.h are each over 1000 lines. Both are compile-time-only headers with dense template machinery. Consider factoring out the fixed_string utilities into a separate cpp/include/hgraph/util/fixed_string.h to keep the schema headers focused.

---

Summary

The architecture is moving in the right direction and the scale of cleanup is impressive. The main blockers before marking ready are:

1. Remove cpp/tests/asan-build/ from version control.
2. Fill the tagged_ptr, BoundaryBinding, and deleted-test coverage gaps (or explicitly track them as follow-up issues).
3. Resolve or track the ConstNode Python round-trip TODO.

The __class__ mutation and alarm-scheduling smell are medium-priority and can reasonably land as tracked issues rather than blocking the PR.

[claude]

Code Review — PR #251: Type-Erased C++ API

Note: This is an AI-generated review (Claude Sonnet 4.6). The PR is marked WORK IN PROGRESS — feedback is offered as early input rather than a blocking gate.

---

Overview

This is a substantial architectural rewrite of the hgraph C++ backend, introducing a complete type-erased value and time-series type system. The core ideas are sound:

• TypeMeta / TypeRegistry — schema metadata (what a type is), separate from behavior
• TSMeta / TSTypeRegistry — time-series schema built on top of TypeMeta
• Value / View — owning/non-owning type-erased value handles with schema-resolved dispatch
• ViewDispatch — virtual dispatch interface separating storage from behavior
• AtomicView, BundleView, ListView, etc. — typed view wrappers built over View
• TSValue, TSInput, TSOutput — time-series storage on top of the new value layer
• tagged_ptr / discriminated_ptr — pointer+tag packing utilities

The separation of "what a type is" (TypeMeta) from "how it behaves" (ViewDispatch) is a well-structured design for a dynamic schema system.

---

Code Quality & Strengths

• Consistent [[nodiscard]] usage throughout
• #pragma once used uniformly (good)
• Small value optimization pattern in Value (inline storage vs heap)
• MutationTracking enum cleanly separates plain vs delta-aware storage
• tagged_ptr / discriminated_ptr are well thought-out with good documentation
• Doxygen comments are thorough on the new infrastructure headers
• Optional sanitizer support (ASAN/UBSAN/TSAN) in CMakeLists.txt is excellent

---

Issues & Suggestions

1. Potential TOCTOU Race in scalar_type_meta<T>() (type_registry.h:360)

template<typename T>
const TypeMeta* scalar_type_meta() {
    static const TypeMeta* cached = [] {
        auto& registry = TypeRegistry::instance();
        auto* meta = registry.get_scalar<T>();
        return meta ? meta : registry.register_type<T>();  // ← race
    }();
    return cached;
}

The static local initialization is thread-safe per C++11, but two different instantiations (e.g. scalar_type_meta<int>() and scalar_type_meta<double>()) can race with each other on their respective get_scalar/register_type paths if called from different threads simultaneously. The documentation acknowledges this, but the get-then-register pattern inside a lambda that otherwise looks thread-safe is a subtle trap. Either add a mutex around registration or document that all types must be pre-registered before any concurrent access.

2. const_cast in TypeRegistry::register_type(name) (type_registry.h:416)

TypeMeta* meta_ptr = const_cast<TypeMeta*>(register_type<T>());

register_type<T>() returns const TypeMeta* but the register_type(name) overload immediately casts away const to mutate the name field. Since _scalar_types owns the objects, returning non-const internally would avoid the cast. Consider a private mutable_register<T>() helper or restructuring so the public interface never requires const_cast.

3. TypeMeta is a Public Mutable POD (type_meta.h:123)

All fields of TypeMeta are public and directly settable. This means callers can accidentally create malformed schemas by writing meta->kind = TypeKind::Bundle while leaving fields null. Consider making the struct fields const (or marking construction paths internal-only) so only the registry can build valid instances.

4. BundleBuilder::add_field takes const char* (type_registry.h:482)

BundleBuilder& add_field(const char* name, const TypeMeta* type);

Raw const char* requires the caller to guarantee string lifetime across the build() call. A string literal survives this, but a std::string::c_str() from a temporary does not. Prefer std::string_view or std::string to make the API harder to misuse.

5. TSMeta Union Destruction Is Fragile (ts_meta.h:258)

void destroy_active_member() {
    if (kind == TSKind::TSB) {
        data.tsb.~TSBData();
    }
}

Only TSB gets special destruction because TSBData holds an nb::object. This is correct today, but if a future KindData member ever acquires a non-trivial destructor, it is easy to forget to update destroy_active_member. A static_assert or comment calling out this invariant explicitly would help future maintainers.

6. nb::object in TSMeta::python_type() Static Sentinel (ts_meta.h:233)

[[nodiscard]] const nb::object& python_type() const noexcept {
    static const nb::object none_obj{};
    return kind == TSKind::TSB ? data.tsb.python_type : none_obj;
}

none_obj is a function-local static nb::object. Python extension objects in static storage can cause issues at interpreter shutdown (destruction order). This should either return by value (for the null case) or be guarded with Py_IsInitialized() on destruction.

7. Range<T> and KeyValueRange<K, V> lack size() / empty() (view.h)

These lazy filtered ranges only expose begin() / end(). In practice callers often need to know whether a range is empty before iterating. Without size() / empty(), they must dereference begin() != end(), which triggers an iteration step. Adding empty() based on whether the first include-match exists would be ergonomically useful.

8. atomicompare on non-comparable types falls through silently (atomic.h:24)

} else {
    static_assert(requires { lhs == rhs; },
                  "Atomic values require either operator== or an atomic_compare overload");
}

The static_assert fires if the type has neither <=> nor ==. However, a type with only operator< (no operator==) would satisfy the lhs < rhs branch but return an incorrect equivalent for all non-less cases. The middle branch should require both == and < before using the manual simulation.

9. ViewDispatch destructor comment is slightly misleading (view.h:249)

"trivial protected destructor avoids the vtable slot and per-instance overhead of a virtual destructor"

The vtable is per-class (not per-instance), and a virtual destructor would add one vtable slot per class. The actual benefit is preventing polymorphic deletion through a base pointer. The comment could be more precise to avoid confusing future readers.

10. namespace nb = nanobind; is redeclared in every header

Multiple headers independently declare namespace nb = nanobind; at file scope. Centralise this in hgraph_base.h (which already exists) and remove the repetition.

---

Test Coverage

The new hgraph_unit_tests/ts_tests/ directory shows good behavioral coverage across all time-series types (TS, TSS, TSD, TSL, TSW, TSB, REF). Tests follow the project's pytest function-based style correctly.

• _state_tracer.py + analyze_state_flows.py is sophisticated debugging infrastructure — consider gating it behind a separate pytest plugin so it doesn't add import overhead to normal runs.
• Tests target the Python API via eval_node, which is the right level for behavioral tests. Lower-level C++ unit tests for Value/View/TypeRegistry would help catch issues earlier (before Python binding is involved).
• The HGRAPH_TRACE_STATES env var gating in conftest.py is a clean pattern.

---

Build System

• C++23 requirement is fine; CMAKE_CXX_EXTENSIONS OFF is correct for portability.
• NB_USE_STABLE_ABI ON is important for binary distribution.
• The release flag -g -O3 keeps debug symbols in release builds, which is deliberate but adds binary size — worth a comment explaining the trade-off.

---

Summary

Area	Assessment
Architecture	Sound — clean separation of schema vs behavior
API Safety	Some rough edges (const_cast, raw char*, public TypeMeta fields)
Thread Safety	Documented but TOCTOU race in scalar_type_meta<T>()
Documentation	Strong — good Doxygen coverage
Test Coverage	Good behavioral tests; C++ unit tests for core value layer would add depth
Build	Well-structured with good sanitizer support

Main asks before merging:

1. Resolve the scalar_type_meta<T>() thread safety issue
2. Fix const_cast in register_type(name)
3. Address the nb::object static sentinel lifetime risk
4. Mark or enforce TypeMeta immutability after registry registration

Happy to dig deeper into any specific area.

[claude]

Code Review: Type-Erased C++ API (PR #251)

This is a substantial PR introducing the new type-erased C++ API. Given the stated WIP status, this review focuses on issues that should be addressed before or around merge rather than minor style nits. Overall the architecture is well-conceived — the dispatch-caching approach, RAII mutation scopes, and compile-time reflection system are all solid design choices.

---

Critical / Must Fix

1. Use-after-free in list_to_numpy() (py_value.cpp ~line 215)

PyArray_SimpleNewFromData() wraps ListView's backing buffer with a raw pointer. If the ListView is modified (push_back, resize) or destroyed while the NumPy array is alive, the array's data pointer becomes dangling. The cyclic_buffer_to_numpy() function in the same file already does this correctly via copy — list_to_numpy should follow the same pattern, or pin the ListView via a Python base object capsule.

2. Unsafe void reinterpret_cast without runtime validation (atomic.h ~line 115)*

[[nodiscard]] static AtomicState<T> *state(void *data) noexcept {
    return std::launder(reinterpret_cast<AtomicState<T> *>(data));
}

There is no check that data actually points to an AtomicState<T>. A dispatch/schema mismatch would silently corrupt memory. The dispatch caching is supposed to prevent this, but a single wrong registration or copy of a dispatch pointer would bypass that guarantee with no diagnostic.

---

High Severity

3. Thread-safety holes in TSViewContext::resolved() (ts_view.h ~lines 283-299)

The resolved() method is const and follows a LinkedTSContext chain without any synchronization. If these are read from multiple threads (or if a native thread touches them during Python graph evaluation), torn reads are possible. Either use std::atomic<> on the link pointers or document explicitly that this entire subsystem requires single-threaded access during traversal.

4. PythonTypeVarContext mutation not isolated (static_signature.h ~lines 615-631)

The Python type-var context is mutated during signature export. If any future code path exports signatures concurrently, this is a data race. Worth a comment at minimum; worth a thread-local or mutex if export is ever done outside the GIL.

5. GIL deadlock in intrusive_init callbacks (_hgraph_module.cpp ~lines 23-31)

[](PyObject *o) noexcept {
    nb::gil_scoped_acquire guard;  // deadlocks if GIL already held
    Py_INCREF(o);
}

nb::gil_scoped_acquire is not reentrant on all platforms. If these callbacks are ever invoked from a frame that already holds the GIL (e.g., during Python-initiated ref-counting), this will deadlock. Consider nb::gil_scoped_acquire only in the destructor-side callback and make the incref path a no-op acquire-release check, or use PyGILState_Ensure directly.

---

Medium Severity

6. Hash collision risk in TSWKey hash (ts_type_registry.h ~lines 158-162)

h ^= std::hash<int64_t>{}(k.range) << 2;
h ^= std::hash<int64_t>{}(k.min_range) << 3;

XOR with small constant shifts means (range=A, min_range=B) and (range=B, min_range=A) will produce the same hash. Use a proper combining function (Boost-style hash_combine or FNV mixing):

auto hash_combine = [](size_t h, size_t v) { return h ^ (v + 0x9e3779b9 + (h << 6) + (h >> 2)); };

7. Incomplete exception translator (_hgraph_module.cpp ~lines 34-56)

Only hgraph::NodeException is registered. std::runtime_error and std::out_of_range thrown throughout py_value.cpp will arrive in Python as generic RuntimeError with no context. Register translators for at least std::invalid_argument and std::out_of_range, or a catch-all for std::exception.

8. Python module re-import on every call (static_signature.h ~line 612)

hgraph_module() re-imports the module each call. Cache the result as a function-local static:

static nb::module_ mod = nb::module_::import_("hgraph");

9. Python type generation not cached (static_schema.h ~lines 723-738)

schema_descriptor<> recursively regenerates Python type objects for each schema reference. For complex nested schemas referenced in multiple nodes this creates redundant work and potentially multiple Python type objects for the same logical type. Add a static std::unordered_map<const TypeMeta*, nb::object> cache.

---

Build System

10. Header files passed as source files to nanobind_add_module (src/cpp/CMakeLists.txt ~line 52)

${HGRAPH_INCLUDES} appears to contain .h files passed as compilation units to nanobind_add_module. CMake silently ignores them at compile time but this breaks IDE project views. Remove headers from the source list — they're already visible via target_include_directories.

11. Deprecated include_directories() / add_definitions() (CMakeLists.txt ~lines 50, 305)

These set global scope and leak into downstream targets. Replace with:

target_include_directories(${PROJECT_NAME} PRIVATE ...)
target_compile_definitions(${PROJECT_NAME} PRIVATE hgraph_EXPORTS)

12. macOS deployment target defaults to 15.0 (CMakeLists.txt ~line 72)

This will silently break CI on any macOS runner < 15. Defaulting to 13.0 or 12.0 is safer and still allows std::jthread and other C++23 runtime features.

---

Test Coverage Gaps

The new ts_tests/ suite validates high-level TS/TSB behavior well, which is great. However the C++ binding layer is not directly tested:

• No tests for TypeRegistry builders (ts(), tsb(), tsd(), etc.)
• No tests for View navigation (navigate(), deep_visit())
• No tests for numpy interop lifecycle (ListView modification after list_to_numpy())
• No tests for Arrow C Data Interface export (__arrow_c_array__)
• No tests that exercise the exception translation path

Suggested location per CLAUDE.md conventions: hgraph_unit_tests/_types/_value/test_py_bindings.py

---

Minor

• atomic.h line ~267: try_as() returns a raw pointer into view storage. The lifetime dependency should be documented (or consider returning std::optional<std::reference_wrapper<T>>).
• view.h line ~305: View::invalid_for(schema) and View::empty() have inconsistent behaviour — some operations throw on empty views while others silently succeed. Document preconditions or make behaviour consistent.
• associative.h ~line 283: ~SetMutationView() calls end_mutation_scope() which can throw. Destructor should wrap this in try/catch.
• ts_view.h ~line 1049: std::getenv("HGRAPH_DEBUG_TSD_MOD_ITEMS") inside a hot predicate is a runtime branch that pollutes every iteration. Move to a compile-time flag or pre-compute at construction.
• Several constructors in static_schema.h that only do trivial member initialisation are not marked noexcept.

---

Summary

Category	Count
Critical (memory safety)	2
High (thread safety / correctness)	3
Medium (hash, exceptions, perf)	4
Build system	3
Test coverage	5 gaps
Minor	5

Given the WIP notice, most of these are expected to be addressed before merge. The architecture underneath is strong — the type-erasure dispatch model, the RAII mutation scopes, and the compile-time reflection system are all good design choices. The main things I'd want resolved before merge are the list_to_numpy lifetime bug (#1), the void* cast safety (#2), and the build system cleanup (#10–12).

[claude]

Code Review: Type-erased C++ API (PR #251)

This is a substantial architectural PR introducing a type-erased C++ API for the hgraph core runtime. Given the WIP label and the scale of the change (~67k additions, ~186k deletions, 399 files), this review focuses on the most important design and quality concerns.

---

Overview

The PR introduces several key components:

• tagged_ptr / discriminated_ptr (util/tagged_ptr.h) — pointer-tagging utilities that pack type discriminants into alignment-guaranteed low bits
• TSValue (types/time_series/ts_value.h + .cpp) — schema-bound owning storage for time-series values using a split value+TS-state memory layout
• ActiveTrie / ActiveTrieNode (types/time_series/active_trie.h) — sparse trie tracking which input paths are active, replacing a simpler structure
• StableSlotStorage (util/stable_slot_storage.h) — double-indexed block allocator providing non-moving slot payloads for delta-tracked collections
• TSTypeRegistry (types/time_series/ts_type_registry.h) — singleton registry caching TSMeta* pointers for stable type identity comparisons

The architectural direction is sound: separating schema/dispatch from data, using pointer-tagging for zero-overhead discrimination, and the trie for sparse active-state tracking are all good choices for a performance-sensitive reactive engine.

---

Strengths

• tagged_ptr.h is excellent. The implementation is clean, well-documented, uses static_asserts defensively, and has good test coverage in test_discriminated_ptr.cpp. The overloaded_fn visitor pattern mirrors std::variant idioms correctly.
• StableSlotStorage cleanly solves the non-moving slot requirement for delta-tracking collections. The block-based growth strategy is correct and simple.
• TSValue copy/move semantics are correctly implemented with proper exception safety — the copy assignment allocates, constructs, then swaps, which is strong exception safety.
• ActiveTrie tests cover the core cases well including the TSD pending eviction/reinstatement round trip.

---

Issues and Suggestions

1. TSValue — dangerous const_cast in value() const overload (ts_value.cpp:92)

View TSValue::value() const noexcept
{
    return m_builder != nullptr
        ? View{..., const_cast<void *>(value_memory()), ...}
        : View::invalid_for(nullptr);
}

The const_cast on internally-owned memory to construct a View suggests View does not propagate const. If View exposes mutation APIs, this could allow mutation through a const TSValue &. Consider whether View should have a ConstView counterpart, or whether View should be restricted to read-only operations when constructed from const context.

2. TSTypeRegistry hash functions are weak and collision-prone (ts_type_registry.h:122–187)

All the composite hash functions use the same << 1 / << 2 / << 3 shift pattern:

size_t h = std::hash<const void*>{}(k.key_type);
h ^= std::hash<const void*>{}(k.value_ts) << 1;

XOR-with-fixed-shift produces poor avalanche behaviour and high collision rates for pointer-aligned values (where many low bits are zero). Consider using a proper hash combining function such as boost::hash_combine's formula or FNV-based mixing:

h ^= hash_val + 0x9e3779b9 + (h << 6) + (h >> 2);

For TSBKeyHash, field order matters but is not captured distinctly — hashing the same field names in different orders would collide.

3. TSTypeRegistry singleton is not thread-safe for concurrent writes (header comment acknowledges this, but…)

The comment says: "Writes are NOT thread-safe but are expected to occur during wiring (single-threaded phase under Python's GIL)."

This is a reasonable assumption for CPython, but it should be enforced with a std::mutex or at minimum a HGRAPH_ASSERT(under_gil()) guard in debug builds to catch future misuse early. The assumption could be silently violated if types are constructed from C++ threads that don't hold the GIL.

4. ActiveTrieNode uses std::unordered_map with unique_ptr values for children — allocation pressure

std::unordered_map<size_t, std::unique_ptr<ActiveTrieNode>> children; allocates a unique_ptr<ActiveTrieNode> (heap) per active child, and unordered_map adds a second heap allocation per bucket. For the common case of TSB bundles with small fixed field counts, a std::vector<std::pair<size_t, std::unique_ptr<ActiveTrieNode>>> (with linear scan) or a small flat map would reduce allocations for the expected shallow trie depth. The has_any_active() linear scan through children is O(n) — for wide bundle nodes this could matter.

5. Missing TODO completion marker — active_trie.h:80–87

// TODO: Implement when integrating with TSD structural observers and
//       TargetLinkState unbind cleanup.

The bulk subscription management section is unimplemented. This is a known gap but it should be tracked in an issue rather than left as a TODO in a header that defines a public API, since consumers of ActiveTrieNode have no way to know this gap exists without reading the header.

6. TSValue::operator= — copy-assignment leaks on same-builder path (ts_value.cpp:54–69)

void *replacement = builder().allocate();
try {
    builder().copy_construct(replacement, ...);
} catch (...) {
    builder().deallocate(replacement);
    throw;
}

clear_storage();    // <-- existing storage destroyed here
m_builder = other.m_builder;
m_storage.set(...);

This is correct — the replacement is constructed before clear_storage() is called, which is strong exception safety. Just confirming this is intentional and correct.

7. TSBKeyHash does not account for field order stability

The TSBKeyHash hashes field names and TSMeta* pointers but the XOR-based accumulation is commutative — different field orderings produce the same hash. While the operator== comparison does use positional comparison (fields[i]), two structurally different bundles with the same fields in different orders hash identically, causing unnecessary equality checks. Consider hashing the position index alongside the field content.

8. discriminated_ptr — alignment requirement pushes type constraint to call sites

discriminated_ptr<Ts...> derives alignment = min_alignment_v<Ts...>, which means all alternatives must have sufficient alignment for the number of types. With 6+ alternatives (3 tag bits needed), all types must be alignas(8) minimum. This constraint is checked at compile time, but error messages from violated static_asserts deep in the template could be hard to diagnose. A named concept or a clearer error message would help:

static_assert(detail::min_alignment_v<Ts...> >= (size_t{1} << tag_bits_for_count_v<sizeof...(Ts)>),
    "discriminated_ptr: minimum alignment of alternatives is insufficient for the "
    "required tag bits. Ensure all types are alignas() to a sufficient power of two.");

---

Test Coverage

• test_discriminated_ptr.cpp — good coverage including edge cases (null, visit, 1-type case)
• test_active_trie.cpp — solid coverage of the trie lifecycle and TSD pending logic
• test_time_series_ts_value.cpp exists but wasn't reviewed in detail — ensure copy/move semantics and the builder mismatch throw path are tested
• The TODO bulk-subscription section of ActiveTrie has no test placeholder — worth adding an XFAIL or // NOT YET TESTED comment to make the gap explicit

---

Summary

Given the WIP status, the architectural foundation looks solid. The main action items before merge:

1. Investigate the const_cast in TSValue::value() const — confirm or add a ConstView
2. Fix the hash combining functions in TSTypeRegistry (correctness for production use)
3. Add a GIL assertion or mutex guard for TSTypeRegistry writes
4. Track the ActiveTrie bulk-subscription TODO as a GitHub issue
5. Improve discriminated_ptr alignment violation error messages

The tagged_ptr/discriminated_ptr utilities, StableSlotStorage, and the overall schema/dispatch separation are well-executed and represent a genuine improvement over the prior approach.

[claude]

Code Review: Type-Erased C++ API (PR #251)

Note: This PR is self-described as a work in progress. This review aims to give early architectural and code-quality feedback to inform the final implementation.

---

Overview

This is a significant architectural refactor that replaces the previous C++ API with a type-erased design using void* impl + const XxxOps* ops vtable-struct pairs throughout the core types (EvaluationEngineApi, EvaluationClock, Node, Graph, etc.). The approach trades virtual dispatch for explicit function-pointer tables — a well-established technique for ABI stability and cross-library boundary performance, and a reasonable choice for this project.

Scale: +67,226 / −186,416 lines across 300+ files. A large portion of the deletions appear to be the removal of old per-type builder headers (all of cpp/include/hgraph/builders/) and the old Python API wrappers (cpp/include/hgraph/api/python/).

---

Architecture — What Works Well

• Consistent type-erasure pattern across EvaluationClock, EvaluationEngineApi, and EvaluationEngine makes the boundary between implementation and interface clean and predictable.
• NodeBuilder fluent API is ergonomic and the implementation<T>() template cleanly encapsulates all the static-assert contracts for node types.
• runtime_ops_for<T> / arg_provider<T> dispatch in node_impl.h elegantly handles parameter injection at compile time with zero runtime overhead per argument type.
• Sanitizer support in CMakeLists.txt (ASAN, UBSAN, TSAN with mutual-exclusion guard) is a good addition for catching bugs during development.
• EvaluationEngineBuilder fluent builder mirrors the Python API well and makes engine configuration readable.
• ChildGraphTemplateRegistry using std::deque for stable pointer storage is a sensible ownership choice.

---

Issues and Suggestions

1. Missing file referenced in CMakeLists.txt

cpp/include/hgraph/types/base_time_series.h is listed in HGRAPH_INCLUDES in CMakeLists.txt but does not exist in the repository. This will cause a build failure once anything attempts to install or enumerate headers.

# CMakeLists.txt lists:
include/hgraph/types/base_time_series.h  # ← file not on disk

Action: Either create the file (even as a stub with #pragma once) or remove it from HGRAPH_INCLUDES until it is ready.

---

2. TODO in EngineEvaluationClockOps signals a design violation

// evaluation_clock.h
//TODO: This is not valid methods at this level, the evaluation engine clock does not
//      support setting or cancelling alarms, scheduling is a node level feature...
bool (*set_alarm)(void *impl, engine_time_t, std::string, std::function<void(engine_time_t)>);
void (*cancel_alarm)(void *impl, std::string_view name);

This is a self-identified design problem that should be resolved before the PR is marked ready. If alarm management belongs at the node level (via NodeScheduler), these function pointers should be removed from EngineEvaluationClockOps and the alarm interface exposed only via NodeScheduler. Leaving it here risks callers depending on a misplaced API.

---

3. BuiltNodeSpec mixes C++ and Python object lifetimes

struct HGRAPH_EXPORT BuiltNodeSpec
{
    ~BuiltNodeSpec() noexcept { /* GIL-aware Python object teardown */ }
    // ...
    nb::object python_signature;
    nb::object python_scalars;
};

BuiltNodeSpec is a low-level C++ struct intended to describe node layout, yet it holds nanobind::object members with GIL-dependent destructors. This has several risks:

• The destructor comment says "GIL-aware" — but noexcept and GIL teardown don't compose safely if Python is finalizing.
• It creates a hidden ordering constraint: BuiltNodeSpec instances must be destroyed before the Python interpreter shuts down.
• It prevents BuiltNodeSpec from being used in truly C++-only contexts without nanobind.

Suggestion: Consider splitting the Python-specific fields (python_signature, python_scalars) into a separate PythonNodeSpec struct or storing them in the node's runtime data rather than the builder spec.

---

4. hgraph_forward_declarations.h is almost empty after deletions

The old file had ~186 lines of declarations; the new one has essentially just Traits and 3 type aliases. If other forward declarations were moved to their respective headers, that is fine — but it is worth verifying that all translation units that included the old file have been updated, as missing includes can cause subtle ODR or incomplete-type errors that only surface in specific build configurations.

---

5. NodeScheduler holds a raw non-owning pointer to Node

struct HGRAPH_EXPORT NodeScheduler
{
    explicit NodeScheduler(Node *node) noexcept : m_node(node) {}
private:
    Node *m_node{nullptr};
    // ...
};

Since NodeScheduler is stored inside Node (via BuiltNodeSpec::scheduler_offset), the pointer will not dangle as long as both live in the same storage block — but this assumes the offset-based layout is always respected. If NodeScheduler is ever moved or the storage is reallocated, m_node becomes invalid. A [[nodiscard]] note in the constructor or an assertion in schedule() that m_node != nullptr would make the invariant explicit.

---

6. Release build includes debug symbols by default

set(CMAKE_CXX_FLAGS_RELEASE "-g -O3")

Including -g in RELEASE is non-standard and increases binary size significantly (can be 10x+ for header-heavy C++23 with templates). Debug symbols in release builds are useful for profiling but should be opt-in, not the default. Consider:

set(CMAKE_CXX_FLAGS_RELEASE "-O3 -DNDEBUG")
set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "-g -O3 -DNDEBUG")  # already correct

The RELWITHDEBINFO config already provides symbols-with-optimization for profiling use cases.

---

7. EvaluationClock returned by value from EvaluationEngineApi

[[nodiscard]] EvaluationClock evaluation_clock() const { return ops().evaluation_clock(m_impl); }

EvaluationClock is two pointers (16 bytes), so returning by value is fine for cost. However, the caller receives a copy of a non-owning view — if the underlying impl is destroyed or replaced, the returned EvaluationClock becomes invalid. This is consistent with how the type-erasure pattern works here, but it should be documented at the type level (e.g., a comment on EvaluationClock that it is a non-owning view with the same lifetime constraints as the engine).

---

8. Graph move constructor and m_storage ownership

struct HGRAPH_EXPORT Graph
{
    // ...
    bool  m_owns_storage{false};
    void *m_storage{nullptr};
    // ...
    Graph(Graph &&other);
};

When m_owns_storage is true, the destructor must free m_storage. The move constructor must zero other.m_owns_storage or other.m_storage to prevent a double-free. This is a well-known pitfall — please verify the implementation does this correctly, and consider adding a comment or a std::exchange pattern to make the intent explicit.

---

9. Compiler warning level is minimal

set(CMAKE_CXX_FLAGS "-Wall")

For a C++23 codebase using templates and type-erasure, consider also enabling -Wextra -Wpedantic and, on Clang, -Weverything with explicit suppressions. In particular, -Wshadow, -Wnon-virtual-dtor, and -Woverloaded-virtual catch common bugs in this pattern. The existing sanitizer support shows the project is invested in correctness — stricter warnings are a low-cost complement.

---

Test Coverage

No new tests appear in the 100 files sampled from this PR. Given the scale of the rewrite, it would be good to confirm that the existing test suite (hgraph_unit_tests/) passes against this new implementation before merge, particularly:

• hgraph_unit_tests/_operators/test_const.py (exercises ConstNode which was rewritten in basic_nodes.h)
• Any tests covering the evaluation engine lifecycle and observer callbacks

---

Summary

Category	Assessment
Architecture	Sound type-erasure design; consistent pattern across types
Correctness	Missing file in CMakeLists, TODO alarm API misplacement need resolution
Safety	BuiltNodeSpec GIL/Python entanglement is a latent risk
Build	Release build debug symbols should be opt-in
Tests	Needs verification that existing suite passes

This is a solid foundation for the new C++ API. The main blockers before marking it ready are: (1) resolving the base_time_series.h missing file, (2) addressing the set_alarm/cancel_alarm misplacement, and (3) confirming the test suite passes. The Python object mixing in BuiltNodeSpec is worth a design discussion but may be acceptable as an intentional bridge.

[claude]

Code Review: Type Erased C++ API and Related Code

Note: This review is based on reading the files present in the working tree, which correspond to the PR branch. The PR is explicitly marked WIP, so feedback focuses on design-level concerns, specific bugs, and patterns worth addressing before merge.

---

Overview

This is a large architectural rewrite (~67k additions, ~186k deletions) introducing a type-erased C++ API for the hgraph engine. The core idea — separating schema identity (TypeMeta), storage (plain byte blocks), and behavior dispatch (ViewDispatch vtable singletons) — is sound and well-executed. The View/Value split closely mirrors patterns like std::any + type-indexed dispatch, but with better performance characteristics (no typeid, no dynamic casts, schema pointer equality as the type check).

---

Strengths

• Clear separation of concerns. View (non-owning, erased read surface), Value (owning, schema-bound storage), and ViewDispatch (behavior singleton) are well-scoped with no overlap.
• Good use of C++20 concepts. The InlineValueEligible, HasAdlToString, Hashable, etc. concepts in atomic.h/view.h catch misuse at compile time.
• tagged_ptr / discriminated_ptr utilities. These are clean, well-documented, and properly constrained. Using alignof(T) to derive available tag bits is safer than requiring callers to specify alignment manually.
• ActiveTrie design. The sparse trie for active-state tracking is a solid replacement for per-collection active flags. Lazy root creation avoids allocation when nothing is active.
• Test coverage exists. Dedicated Catch2 tests for active_trie, atomic/associative/record/sequence values, and TSValue are a good foundation.

---

Issues and Suggestions

1. Range<T> and KeyValueRange<K,V> are near-identical — extract a base

cpp/include/hgraph/types/time_series/value/view.h lines 34–175

Both templates share the same iterator structure, begin()/end() logic, m_context/m_limit/m_predicate/m_projector members, and includes()/project() helpers. Only the return type of project() differs. This duplication will become a maintenance burden. Consider a shared FilteredRange<Projector> base or a single parameterized template:

template <typename Projector>
struct FilteredRange { /* shared logic */ };

template <typename T>
using Range = FilteredRange<T(*)(const void*, size_t)>;

2. ActiveTrieNode exposes all data as public

cpp/include/hgraph/types/time_series/active_trie.h lines 28–89

locally_active, children, pending, and slot_key are all public. Callers can mutate the trie in ways that bypass the invariants maintained by ensure_child, try_prune_child, evict_to_pending, and resolve_pending. The PendingMap key type (Value) and the slot_key (unique_ptr<Value>) are particularly sensitive: incorrect external manipulation could violate the eviction/re-insertion contract for TSD. Consider making these fields private and exposing mutation only through the existing methods.

3. PendingMap uses Value as a by-value key — equality can throw

cpp/include/hgraph/types/time_series/active_trie.h line 42

using PendingMap = std::unordered_map<Value, std::unique_ptr<ActiveTrieNode>>;

std::hash<hgraph::Value> returns 0 for empty values (harmless but degrades to O(n) for empty-key collisions). More importantly, std::equal_to<hgraph::Value> calls equals() → dispatch()->compare(), which can throw (e.g. Python dispatch catches and swallows, but C++ dispatch can throw on type mismatch). std::unordered_map lookup is not exception-safe if the comparator throws. Consider using a more defensive key type or wrapping comparison in noexcept.

4. Unimplemented TODO blocks bulk subscription management

cpp/include/hgraph/types/time_series/active_trie.h lines 76–88

// TODO: Implement when integrating with TSD structural observers and
//       TargetLinkState unbind cleanup.

The comment notes that make_active/make_passive during TSD unbind/rebind requires walking the trie to re-subscribe active nodes against the new target state. Without this, rebinding an active TSD subscription will silently lose activation state. This is a significant gap — worth tracking as a blocking issue before merge.

5. iterator in Range<T> is missing standard iterator traits

cpp/include/hgraph/types/time_series/value/view.h lines 39–63

The iterator has no iterator_category, value_type, difference_type, pointer, or reference type aliases. This means Range<T> does not satisfy std::ranges::range and standard algorithms (e.g. std::count_if, std::for_each) cannot be used with it. At minimum, using difference_type = ptrdiff_t; and using value_type = T; should be added, or the iterator should inherit from std::iterator (deprecated) / use std::contiguous_iterator_tag.

6. ViewDispatch destructor comment is slightly misleading

cpp/include/hgraph/types/time_series/value/view.h lines 248–264

"A trivial protected destructor avoids the vtable slot..."

The vtable already exists due to the virtual methods. The comment should say "avoids a virtual destructor slot" — the intent is preventing polymorphic deletion through base pointer, not avoiding the vtable itself. Minor, but worth clarifying for future readers.

7. static_cast<void>(schema) used for silencing unused warnings

cpp/include/hgraph/types/time_series/value/atomic.h lines 78, 83

static_cast<void>(schema);

The idiomatic C++ form is (void)schema;. Both are correct; static_cast<void> is more verbose and less standard. Small style nit.

8. Value::operator= schema rebinding is undocumented in header

cpp/include/hgraph/types/time_series/value/value.h lines 104–109

The comment says "Assignment does not permit schema rebinding. Matching builder identity is the contract..." but there is no assertion or runtime check in the header. If the .cpp implementation enforces this, a note linking to that enforcement would help reviewers. If it doesn't, an assertion should be added.

9. ts_root_state() uses std::visit for a simple base-pointer extraction

cpp/include/hgraph/types/time_series/ts_value.h lines 218–226

return std::visit([](auto &state_value) -> BaseState * { return &state_value; }, state_variant());

If TimeSeriesStateV is a std::variant, this generates a jump table over all alternatives on every call to ts_root_state(). If BaseState is a common base class of all variant alternatives, accessing the base via std::visit every time is heavier than it needs to be. Consider storing a pre-computed BaseState* pointer in the storage block, or at minimum commenting why std::visit is necessary here.

10. view_context() null check is partially duplicated

cpp/include/hgraph/types/time_series/ts_value.h lines 165–174

if (m_builder == nullptr || storage_memory() == nullptr) { return TSViewContext::none(); }

Both m_builder and storage_memory() need to be non-null for a valid context. Consider a single is_bound() helper that encapsulates both checks, to reduce the chance of forgetting one of them in a copy-paste.

---

Test Coverage Observations

The C++ unit tests cover the happy path for ActiveTrie, atomic/sequence/record values, and discriminated pointers well. Gaps to address before merge:

• No tests for Range<T> iteration — particularly the predicate filtering path.
• No tests for View::try_copy_from across mismatched schemas — the numeric widening path in AtomicDispatch<T> is only tested implicitly.
• No tests for TSValue copy/move — given the tagged-pointer ownership semantics, these are high-risk operations.
• No Python integration tests verifying the new C++ API is correctly exposed via the Python bindings.
• The ensure_trie_path / lookup_trie_path functions have no dedicated tests.

---

Summary

The architecture is clean and the core type-erasure pattern is well-implemented. The main concerns before merge are:

1. The unimplemented TSD unbind/rebind subscription management (item 4) — this appears to be a correctness gap, not just tech debt.
2. The Range/KeyValueRange duplication (item 1) — easy to fix now, harder later.
3. ActiveTrieNode encapsulation (item 2) — protects future maintainers from subtle invariant violations.
4. Missing standard iterator traits on Range<T> (item 5) — limits composability with the standard library.

The WIP label is appropriate; the design foundation is solid and worth building on.

[sonarqubecloud]

Quality Gate passed

Issues
11 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.6% Duplication on New Code

See analysis details on SonarQube Cloud

[claude]

Code Review: Type-erased C++ API and related code

Note: Reviewed from the ts_value_2603 branch. The PR is explicitly marked WIP; this review focuses on design concerns and issues to address before merge.

---

Overview

This is a substantial and well-motivated refactoring (~67K additions, ~186K deletions). The core idea — replacing the old Python-wrapper-centric C++ layer with a proper type-erased C++ API using ops-table dispatch — is the right architectural direction. The key new types are:

• NodeRuntimeOps / Node / BuiltNodeSpec — type-erased node with function-pointer vtable
• EvaluationClockOps / EvaluationClock / EngineEvaluationClock — type-erased clock hierarchy
• EvaluationEngineOps / EvaluationEngine / EvaluationEngineApi / GraphEvaluationEngine — type-erased engine surface
• GraphBuilder / NodeBuilder / Graph — fluent construction layer
• BoundaryBindingPlan / BoundaryBindingRuntime — nested graph wiring
• ChildGraphTemplate / ChildGraphInstance — nested operator lifecycle
• arg_provider<T> injection system in node_impl.h

---

Positive aspects

1. Clean type erasure design. The *Ops struct of function pointers pattern (e.g., NodeRuntimeOps, EvaluationEngineApiOps) is idiomatic and avoids vtable costs while preserving polymorphism across the Python/C++ boundary.

2. Compile-time injection in node_impl.h. The arg_provider<T> specializations combined with invoke<Fn>() and the Has* concepts give a genuinely ergonomic and type-safe way to wire node hooks. Static assertions in implementation<T>() catch mis-configurations at compile time.

3. Immutable BuiltNodeSpec. Treating the per-node spec as write-once after construction avoids aliasing bugs.

4. Good use of [[nodiscard]]. Consistently applied throughout.

5. Useful documentation. The doxygen @code examples in evaluation_engine.h and evaluation_clock.h are genuinely helpful context.

---

Issues to address before merge

Design / Correctness

1. Alarm methods in EngineEvaluationClockOps — the TODO needs resolution
evaluation_clock.h:32-35 has an acknowledged code smell: set_alarm / cancel_alarm are on the clock ops but are documented as not belonging there. Shipping this as-is will mean consumers write code against the wrong abstraction level. Either relocate these or document the interim state explicitly and file a follow-up issue.

2. BuiltNodeSpec destructor leaks Python objects on shutdown
node.h:77-87: when Py_IsInitialized() is false, the code calls python_signature.release() / python_scalars.release() and discards the returned PyObject*. That means the ref-counted Python objects are never freed — a leak. The safe pattern at interpreter shutdown is to use PyObject_CallNoArgs / Py_DECREF guarded by Py_IsInitialized(), or simply skip cleanup and accept the OS-level reclaim. Explicitly discarding the return value of release() is an intentional decision that deserves a comment.

3. Unchecked static_cast in runtime_data<T> with no type tag
node_impl.h:377-388: runtime_data<T>(node) blindly casts node.data() to T*. If a caller passes the wrong T this is silent UB. Since the code already distinguishes StaticNodeRuntimeData from NestedNodeRuntimeData, consider adding a discriminator tag (even a uint32_t magic number or an enum) to the header of the payload block so misuse can be caught with an assert in debug builds.

4. ChildGraphTemplate::create returns a raw owning pointer
child_graph.h:51: the factory returns const ChildGraphTemplate* with the documented constraint that it must outlive all graphs built from it. This is an invitation for dangling pointers. Since ChildGraphTemplateRegistry already owns the storage, the raw pointer should be replaced by a handle type or at minimum the lifetime contract should be enforced via assert at graph construction time.

5. ChildGraphTemplateRegistry singleton + reset() interaction
child_graph.h:63-80: The process-global registry with reset() for tests creates ordering sensitivity. If a test forgets to call reset(), stale templates leak into later tests. Consider a RAII scope guard or registering templates as test fixtures instead.

Performance

6. NodeScheduler uses std::set<std::pair<engine_time_t, std::string>>
node.h:153-155: For the scheduling hot path, std::set with heap-allocated std::string keys is expensive. A flat sorted std::vector (fast for the typical small cardinality of scheduled events per node) or std::priority_queue would be faster. The tag map (std::unordered_map<std::string, engine_time_t>) similarly benefits from small-string-optimised or interned keys.

7. std::function in EngineEvaluationClockOps::set_alarm
evaluation_clock.h:34: std::function carries heap-allocation overhead on every alarm registration. For what is essentially a callback pointer, std::move_only_function (C++23) or a custom type-erased callback would be cheaper and more expressive about the ownership semantics.

8. DebugPrintNode imports Python print on every eval
basic_nodes.h:105-106: nb::module_::import_("builtins").attr("print") performs a dictionary lookup on every evaluation tick. Cache the attribute at node start or as a static local.

9. push_message_arg_provider if-constexpr chain
node_impl.h:303-340: nine sequential if constexpr branches is workable now, but will become a maintenance burden as new value types are added. A trait-based dispatch table would be more extensible.

Minor / Style

10. static_cast<void>(node) pattern in no-op hooks
node_impl.h:447-450: Using static_cast<void>(x) to suppress unused-variable warnings is idiomatic in C, but [[maybe_unused]] on the parameter is the more C++-idiomatic form and signals intent at the declaration site.

11. NodeBuilder::slot_for_input_name is O(n)
node_builder.h:270-278: Linear field scan is fine for build-time use, but worth a note in case this path is ever called at runtime.

12. MapNodeRuntimeData and NestedNodeRuntimeData have duplicate TSInput/TSOutput fields
nested_node_builder.h:57-66, 69-83: The input, output, error_output, recordable_state pointer quartet appears in both structs verbatim. A shared NodeIOPointers base or helper struct would reduce drift.

---

Test coverage

The existing test_types_v2_graph.cpp and test_types_v2_nested.cpp provide good coverage of the Graph / Node assembly path. The following new abstractions appear to lack dedicated unit tests:

• BoundaryBindingRuntime (bind / unbind / rebind paths)
• ChildGraphInstance lifecycle (initialise → start → evaluate → stop → dispose)
• EvaluationEngineBuilder::build() + EvaluationEngine::run() in simulation mode
• NodeScheduler tag management (schedule, un_schedule, pop_tag)

Given the WIP status this is expected — flagging for pre-merge completeness.

---

Summary

The type-erased ops-table design is sound and a clear improvement over the previous Python-wrapper layer. The arg_provider<T> injection system is the highlight of the PR — it is clean, composable, and will make static node implementations easy to write correctly.

The main items to resolve before merge are: the alarm method placement (issue 1), the potential Python object leak on shutdown (issue 2), and the raw owning pointer from ChildGraphTemplate::create (issue 4). The performance items (scheduler data structures, std::function for alarms) are worth addressing but can follow in a subsequent PR if needed.

[claude]

Code Review: Type Erased C++ API

Note: This PR is marked as WIP, so comments are design/direction feedback rather than blocking issues.

---

Overview

This PR is a significant architectural rework of the C++ runtime. It replaces the previous inheritance-heavy, pybind11-coupled API with a clean type-erasure via explicit vtables approach. The core design:

• Node is a tiny, type-erased struct holding a pointer to its concrete payload (data()) and a NodeRuntimeOps vtable.
• Static node implementations are stateless struct types with static start/stop/eval hooks.
• Argument injection is resolved at compile time via arg_provider<T> template specializations — no virtual dispatch on the hot path.
• StaticNodeSignature<T> provides compile-time reflection to drive both runtime construction and Python wiring export.

The ~25-file builders/ hierarchy and ~20-file nodes/ hierarchy are replaced by a much leaner set of headers under types/. This is a clear improvement in readability and debuggability.

---

Code Quality & Style

Strengths:

• static_assert(std::is_empty_v<TImplementation>) enforcing stateless node structs is an excellent safety constraint.
• constexpr NodeRuntimeOps vtables avoid any heap allocation for the dispatch table.
• The arg_provider<T> injection pattern is clean and extensible — new injectables require only a new specialization.
• BuiltNodeSpec::~BuiltNodeSpec correctly handles the GIL and the !Py_IsInitialized() edge case.
• The fluent NodeBuilder interface is well-structured.
• Comprehensive static_assert guards on StaticNodeSignature<T> catch mismatches early.

Style observations:

• ConstNode::start takes explicit Graph& and Node& as parameters (basic_nodes.h:21). This is valid but slightly inconsistent with the "inject only what you need" philosophy of the system — other nodes use more minimal signatures.
• NodeScheduler::schedule takes std::optional<std::string> by value (node.h:140). Consider std::optional<std::string_view> + caller manages lifetime, or a separate tagless overload, to avoid an allocation on the common hot path.

---

Potential Bugs & Issues

1. runtime_data<T> unchecked cast in release builds (node_impl.h:377-388):
   The assert(node.data() != nullptr) only fires in debug builds. In release builds, if data() is null, static_cast<T*>(node.data()) is UB. The calling helpers (default_has_input, etc.) guard correctly, but the raw template itself has no protection in release mode.

2. python_scalar_or_throw TOCTOU pattern (node_impl.h:137-141):
   PyMapping_HasKey followed by py_getitem is a redundant check-then-act. Since the dict is captured at wiring time and not modified, this is safe in practice, but a single PyObject_GetItem with null check would be more idiomatic and avoids the double-key-hash.

3. Graph move and raw Node* in NodeScheduler (node.h:152):
   NodeScheduler stores a raw Node*. Since NodeScheduler is embedded in the same slab chunk as its owning Node, this pointer stays valid through Graph moves — but this invariant is implicit. If a NodeScheduler were ever copied or moved independently, the pointer would dangle. A comment stating the lifetime contract would help future maintainers.

4. EvaluationEngine move semantics:
   EvaluationEngine(EvaluationEngine&&) is declared but reset() must null out m_impl and m_ops in the moved-from object to prevent double-free in the destructor. Worth verifying the implementation enforces this.

---

Architecture / Code Smell

Acknowledged alarm placement (evaluation_clock.h:33-35):
The TODO comment is right — set_alarm and cancel_alarm in EngineEvaluationClockOps are a misplaced abstraction. Alarm scheduling is a node-level concern (already handled in NodeScheduler) and does not belong on the engine clock. This makes the EngineEvaluationClock interface carry knowledge it shouldn't. Recommend tracking this as a follow-up issue.

NodeEntry::force_eval overhead (graph.h:25):
The TODO is noted. Since NodeEntry occupies N * sizeof(NodeEntry) contiguous bytes for a graph of N nodes, and force_eval is only used at start time, encoding this as a sentinel value in the scheduled field (e.g., a dedicated constant) would save 8 bytes per node in the hot array.

Heavy header chain from node_builder.h:
node_builder.h includes node_impl.h and static_signature.h (the latter is ~1000 lines of template machinery). Any TU that wants a NodeBuilder pulls in the full reflection machinery. This may lengthen compile times noticeably as the project grows. Consider forward-declaring the template in node_builder.h and including static_signature.h only in files that call .implementation<T>().

---

Performance

• DebugPrintNode::eval imports builtins on every call (basic_nodes.h:106): nb::module_::import_("builtins").attr("print") does a dict lookup every tick. Cache the print callable (e.g., as a static or in node state) or use std::printf.
• ConstNode::eval Python scalar lookup (basic_nodes.h:42): PythonScalarArg<"value"> requires a Python dict lookup each eval. The already_emitted guard means this only runs once per node lifetime, so it's fine for const. The pattern would be a bottleneck for high-frequency nodes.

---

Test Coverage

Existing tests in test_cpp_static_node_export.py cover wiring-signature reflection well. However, the new infrastructure is missing direct unit tests for:

• BoundaryBindingRuntime — the bind, bind_keyed, unbind, and rebind paths are complex and untested directly.
• NodeScheduler — tag management, wall-clock alarms, and the advance()/reset() contract.
• Push-source apply_message dispatch — verify that the invoke_push_message path is exercised and that returning false correctly suppresses scheduling.
• ConstNode, NullSinkNode, DebugPrintNode with the new C++ builder — the existing test_const.py may cover ConstNode via the Python path, but not necessarily through the v2 static builder.

---

Security

No concerns. GIL acquisition in BuiltNodeSpec::~BuiltNodeSpec is correct. Python scalar access is guarded. No user-controlled strings are passed to unsafe APIs.

---

Summary

This is a well-considered design. The type-erasure + stateless-hook pattern achieves the stated goals of readability and debuggability. The main items to address before merging:

1. Resolve or track the EngineEvaluationClockOps alarm placement code smell.
2. Add tests for BoundaryBindingRuntime, NodeScheduler, and push-source dispatch.
3. Fix the python_scalar_or_throw double-lookup.
4. Cache the builtins.print reference in DebugPrintNode.
5. Document the NodeScheduler raw-pointer lifetime invariant.

Looking forward to this landing — it's a meaningful step forward for the C++ runtime.