feat(codec): add yaml-cpp codec wrapper - #123
Conversation
Wraps yaml-cpp (0.9.0) as a proper codec module following the TOML pattern: - Serializer builds YAML::Node trees via streaming interface - Deserializer reads from YAML::Node with node_slot to avoid yaml-cpp's destructive operator= semantics on shared nodes - Convenience API: parse/to_string/from_yaml/to_yaml - CMake integration with KOTA_CODEC_ENABLE_YAML option - 12 roundtrip tests covering structs, vectors, scalars, optionals, errors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…pport Adds snapshot testing inspired by Rust's insta crate: - EXPECT_SNAPSHOT/ASSERT_SNAPSHOT for any pretty_dump-able value - EXPECT_JSON_SNAPSHOT/ASSERT_JSON_SNAPSHOT via codec::json serialization - EXPECT_YAML_SNAPSHOT/ASSERT_YAML_SNAPSHOT via codec::content -> YAML - EXPECT_INLINE_SNAPSHOT/ASSERT_INLINE_SNAPSHOT for inline comparisons - SNAPSHOT_GLOB for iterating files matching a pattern - Snapshot files stored in __snapshots__/ with ZEST_UPDATE_SNAPSHOTS=1 to update - TestContext tracks suite/test/file/counter per test for path computation - 18 tests covering all snapshot modes and utilities Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR introduces a new YAML codec for kota with Serializer/Deserializer implementations conforming to streaming serialization protocols, alongside a comprehensive snapshot testing system for kota::zest that tracks per-test context, manages snapshot files, and provides comparison macros with JSON/YAML formatting support. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Serializer
participant Frame as Frame Stack
participant Node as YAML::Node
participant YamlCpp as yaml-cpp
User->>Serializer: to_yaml(value)
Serializer->>Serializer: create Serializer<Config>
Serializer->>Serializer: serialize(value)
Serializer->>Frame: begin_object()
Frame->>Frame: push new frame
loop for each field
Serializer->>Frame: field(name)
Serializer->>Serializer: serialize_field(value)
Serializer->>Node: write scalar/nested
end
Serializer->>Frame: end_object()
Frame->>Frame: pop frame, merge to parent
Serializer->>Node: retrieve root node
Node-->>User: return YAML::Node
sequenceDiagram
participant Test as Test Execution
participant SnapAPI as snap::check()
participant Registry as TestContext
participant FS as Filesystem
participant Report as Output
Test->>Registry: populate current_test_context()
Test->>SnapAPI: check(content, expr, loc)
SnapAPI->>Registry: read snap_counter
SnapAPI->>FS: construct snapshot path
alt snapshot file exists
SnapAPI->>FS: read stored snapshot
alt content matches
SnapAPI-->>Test: return matched
else content differs
SnapAPI->>Report: log location + colored diff
SnapAPI->>FS: optionally update if ZEST_UPDATE_SNAPSHOTS
SnapAPI-->>Test: return updated/mismatch
end
else snapshot file missing
SnapAPI->>FS: create new snapshot file
SnapAPI-->>Test: return created
end
Registry->>Registry: increment snap_counter
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…exception builds - Deserializer now uses YAML::convert<T>::decode() which returns bool instead of node.as<T>() which throws on failure - parse_node() guarded with #if KOTA_ENABLE_EXCEPTIONS for YAML::Load() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
include/kota/codec/yaml/serializer.h (2)
31-41: Error type mismatch with the deserializer.
Serializer::error_typeiserror_kindwhileDeserializer::error_typeininclude/kota/codec/yaml/deserializer.h(line 64) isyaml::error. The free functionto_yaml(line 200) is declared to returnstd::expected<YAML::Node, error>, so there's an implicit conversion happening at line 204. This works as long aserroris constructible fromerror_kind, but the asymmetry will surprise anyone composing the two backends or writing generic code overyaml::Serializer/yaml::Deserializer. Prefer aligning both toyaml::error.🛠️ Suggested alignment
- using error_type = error_kind; + using error_type = yaml::error;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/kota/codec/yaml/serializer.h` around lines 31 - 41, Serializer's error_type is currently error_kind while Deserializer uses yaml::error, causing implicit conversions in to_yaml and surprising asymmetry; change Serializer::error_type (and any dependent typedefs like result_t and status_t) from error_kind to yaml::error so both backends align, update any uses of error_kind within serializer.h to use yaml::error (or adapt constructors if needed), and ensure the free function to_yaml's declared return std::expected<YAML::Node, error> (and any alias named error) matches the new yaml::error type so no implicit conversion is required.
98-120: Defensive:field/end_object/end_arrayassume non-empty stack.
ser_stack.back()is UB on an empty vector. The codec streaming protocol should always pair begin/end correctly, so today this is a pure invariant, but a strayserialize_field(...)at the top level (e.g., due to a bug in a customserialize_traits) would quietly corrupt memory rather than surface a diagnosable error. An assertion or anerror_kind::invalid_statereturn would make the failure mode debuggable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/kota/codec/yaml/serializer.h` around lines 98 - 120, Add defensive checks for an empty ser_stack at the start of field, end_object (and similarly end_array) to avoid UB from ser_stack.back() on an empty vector: if ser_stack.empty() return an error_t status indicating invalid_state (or assert) before touching ser_stack; only call ser_stack.back(), pop_back(), or access parent after that check. Update the functions named field(std::string_view), end_object(), and end_array() to perform this guard and return error_kind::invalid_state via the status_t on failure so callers get a diagnosable error instead of silent UB.tests/unit/zest/snap_test.cpp (1)
159-175: Minor: missing coverage for combinedcounter > 0+GlobContext.The three
snapshot_path_*cases exercise counter-only or glob-only, but not both together. The implementation insrc/zest/snap.cpporders the filename assuite__test__stem@N.snap, which is worth pinning with a test so an accidental ordering swap (e.g.,suite__test@N__stem.snap) doesn't regress silently.🧪 Suggested extra case
TEST_CASE(snapshot_path_glob) { snap::GlobContext glob_ctx{"input1"}; auto path = snap::snapshot_path("/home/user/tests/test.cpp", "suite", "case", 0, glob_ctx); EXPECT_EQ(path.filename().string(), "suite__case__input1.snap"); } + +TEST_CASE(snapshot_path_glob_multi) { + snap::GlobContext glob_ctx{"input1"}; + auto path = + snap::snapshot_path("/home/user/tests/test.cpp", "suite", "case", 2, glob_ctx); + EXPECT_EQ(path.filename().string(), "suite__case__input1@3.snap"); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/zest/snap_test.cpp` around lines 159 - 175, Add a unit test in tests/unit/zest/snap_test.cpp that calls snap::snapshot_path with both a positive counter (e.g., 1) and a non-empty snap::GlobContext (e.g., {"input1"}) to assert the filename ordering is "suite__case__stem@N.snap" (i.e., glob stem before `@N`); implement this as a new TEST_CASE (name it something like snapshot_path_counter_and_glob) that verifies path.filename().string() equals "suite__case__input1@2.snap" for the inputs used and reuse the existing pattern of constructing the path and checking filename().string().src/zest/snap.cpp (2)
399-406: Staleglob_ctxleaks to subsequent tests on exception.If
callback(path)throws, theglob_ctx.reset()at line 405 is skipped and the thread-localGlobContextremains set to the last iteration's stem. Subsequent snapshot calls outside the glob block will then be routed under that stale stem. Wrap the iteration in RAII so the reset runs unconditionally.🛠️ Suggested RAII fix
auto& glob_ctx = current_glob_context(); + struct glob_guard { + std::optional<GlobContext>& ref; + ~glob_guard() { ref.reset(); } + } guard{glob_ctx}; for(const auto& path : matched) { glob_ctx = GlobContext{path.stem().string()}; reset_counter(); callback(path); } - glob_ctx.reset(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/zest/snap.cpp` around lines 399 - 406, The loop leaks a stale thread-local GlobContext if callback(path) throws because glob_ctx.reset() is skipped; wrap per-iteration context management in an RAII guard so the GlobContext is set at start and always reset on scope exit (even on exceptions). Replace the manual assignment/reset of current_glob_context()/glob_ctx and the trailing glob_ctx.reset() with a small scoped helper (e.g., ScopedGlobContext or similar) that constructs from path.stem().string() and clears the thread-local in its destructor, and keep calls to reset_counter() and callback(path) unchanged inside the loop so reset runs unconditionally.
186-189: Dead conditional — remove or replace with intended logic.The
if(!child_compound && i < arr.size() - 1) { }block has an empty body with only an explanatory comment. Either the guard is unused and should be dropped, or the intended separator logic was never implemented.🧹 Cleanup
- bool child_compound = arr[i].is_object() || arr[i].is_array(); emit_yaml_impl(out, arr[i], depth + 1, true); - if(!child_compound && i < arr.size() - 1) { - // scalar items are already complete - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/zest/snap.cpp` around lines 186 - 189, The conditional if(!child_compound && i < arr.size() - 1) { } is a dead/empty guard and should be fixed: either remove this entire if statement (leaving no-op behavior) or implement the missing separator/continuation logic for scalar array items—e.g., invoke the serializer/emitter method used elsewhere to append a separator (comma/newline) between items; locate usages around arr, i, and child_compound in snap.cpp (the array serialization loop) and make the change consistently with how compound items are handled.include/kota/zest/snap.h (1)
93-128: Undeclared dependencies onjson.h,content/serializer.h, andzest.h.The JSON/YAML snapshot macros reference
::kota::codec::json::to_json,::kota::codec::content::Serializer,::kota::codec::serialize,::kota::zest::pretty_dump,::kota::zest::print_trace, and::kota::zest::failure, none of which are declared by this header. Today the test files include these explicitly, but the requirement is invisible to a user who writes only#include "kota/zest/snap.h". Add a comment block near the macros listing the required headers (or include the minimally necessary ones here).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/kota/zest/snap.h` around lines 93 - 128, The header uses symbols like ::kota::codec::json::to_json, ::kota::codec::content::Serializer<>, ::kota::codec::serialize, ::kota::zest::prettify_json, ::kota::zest::value_to_yaml, ::kota::zest::snap::check, ::kota::zest::snap::Result, ::kota::zest::print_trace and ::kota::zest::failure but does not declare or include their headers; add a short comment block above the snapshot macros listing the required includes (or add the minimal `#include` directives) such as the headers that declare kota/codec/json.h, kota/codec/content/serializer.h, and kota/zest.h so any user including kota/zest/snap.h gets the needed declarations for EXPECT_JSON_SNAPSHOT, ASSERT_JSON_SNAPSHOT, EXPECT_YAML_SNAPSHOT and ASSERT_YAML_SNAPSHOT to compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/kota/codec/yaml/deserializer.h`:
- Around line 290-310: The next_field() function ignores the boolean result of
YAML::convert<std::string>::decode, so a failed key decode will leave
pending_key stale and return an empty field name; update next_field() to check
the decode return value (call to
YAML::convert<std::string>::decode(frame.iter->first, frame.pending_key)) and if
it returns false propagate an error via mark_invalid (e.g.
mark_invalid(error_kind::decode_error) or an appropriate error_kind), ensuring
you do not set frame.pending_valid or current_value before returning the error;
keep the rest of the iterator advancement logic intact so pending_valid is only
set on a successful decode.
- Around line 162-189: deserialize_uint currently decodes the scalar into
std::int64_t which loses values > INT64_MAX; change the reader lambda inside
deserialize_uint/read_scalar to decode into std::uint64_t (use
YAML::convert<std::uint64_t>::decode) and then reject negative values by
checking the node or decoded value as needed before narrowing; keep the
subsequent cast to uint64_t/narrow_uint<T> and error mappings
(mark_invalid(error_kind::number_out_of_range) or type_mismatch) intact so large
unsigned values in (INT64_MAX, UINT64_MAX] are accepted and still validated by
codec::detail::narrow_uint, and ensure any type-mismatch for negative scalars is
preserved.
In `@include/kota/codec/yaml/yaml.h`:
- Around line 17-27: The no-exceptions branch of parse_node currently calls
YAML::Load(std::string(text)) unguarded so if yaml-cpp was built with exceptions
enabled a ParserException will still be thrown (leading to terminate); either
document that yaml-cpp must be built with exceptions disabled (e.g.,
YAML_CPP_NO_EXCEPTIONS) when KOTA_ENABLE_EXCEPTIONS==0, or replace the
no-exceptions path with explicit error-handling using yaml-cpp's low-level
Parser and a custom EventHandler to detect parse failures and return
std::unexpected(error_kind::parse_error); update parse_node, mention YAML::Load
and error_kind::parse_error, and gate behavior on KOTA_ENABLE_EXCEPTIONS so
behavior is well-defined.
- Around line 48-58: The to_string<T> template currently returns the Emitter
buffer even when YAML::Emitter failed; update the function (the to_string
template that calls to_yaml and uses YAML::Emitter) to call emitter.good() after
emitting and before returning, and if !emitter.good() return std::unexpected
with an error constructed using an appropriate kind (e.g.,
error_kind::invalid_state) and the emitter.GetLastError() or emitter.c_str() as
the message; keep the existing to_yaml call and error path for node creation
unchanged.
In `@include/kota/zest/snap.h`:
- Around line 111-125: The macro ZEST_YAML_SNAPSHOT_CHECK wraps __VA_ARGS__ in
parentheses, causing a comma-expression and silently dropping earlier arguments
(e.g., codec::serialize(_zest_yaml_ser_, (__VA_ARGS__)) makes (a, b) evaluate to
b); remove the parentheses so the macro forwards __VA_ARGS__ directly like
ZEST_SNAPSHOT_CHECK and ZEST_JSON_SNAPSHOT_CHECK (i.e., use
codec::serialize(_zest_yaml_ser_, __VA_ARGS__)) so multi-argument calls fail to
compile rather than silently collapse — alternatively implement an
__VA_OPT__/helper check to explicitly reject multiple arguments if you prefer an
explicit diagnostic.
In `@src/codec/CMakeLists.txt`:
- Around line 127-163: When KOTA_CODEC_ENABLE_YAML is enabled but
KOTA_ENABLE_EXCEPTIONS is OFF, yaml-cpp will break builds; update the CMake
logic so the kota_codec_yaml target (kota::codec::yaml) and its
target_link_libraries/target_include_directories are only created when
KOTA_ENABLE_EXCEPTIONS is ON, otherwise emit a clear CMake error/message that
the YAML backend requires exceptions; additionally, adjust
include/kota/codec/yaml/yaml.h to avoid calling YAML::Load or including yaml-cpp
headers in the no-exceptions branch (remove or guard the YAML::Load invocation)
so no translation unit pulls in yaml-cpp when exceptions are disabled.
In `@src/zest/snap.cpp`:
- Around line 128-143: The float-emission branch (case ValueKind::floating)
currently uses val.as_double() into d and std::to_string(d) — this emits
"inf"/"-inf"/"nan" and loses precision; update this block to first test
non-finite values with std::isfinite(d) and emit YAML-compliant strings ".inf",
"-.inf", or ".nan" for +inf/-inf/NaN respectively (use val/as_double() variable
d), and for finite values replace std::to_string(d) with a round-trip-safe
formatter (std::to_chars or std::format/ostringstream with sufficient precision)
so you can still perform the trailing-zero trimming logic on the resulting
string (refer to variables s, dot, last_nonzero used in the existing code).
---
Nitpick comments:
In `@include/kota/codec/yaml/serializer.h`:
- Around line 31-41: Serializer's error_type is currently error_kind while
Deserializer uses yaml::error, causing implicit conversions in to_yaml and
surprising asymmetry; change Serializer::error_type (and any dependent typedefs
like result_t and status_t) from error_kind to yaml::error so both backends
align, update any uses of error_kind within serializer.h to use yaml::error (or
adapt constructors if needed), and ensure the free function to_yaml's declared
return std::expected<YAML::Node, error> (and any alias named error) matches the
new yaml::error type so no implicit conversion is required.
- Around line 98-120: Add defensive checks for an empty ser_stack at the start
of field, end_object (and similarly end_array) to avoid UB from ser_stack.back()
on an empty vector: if ser_stack.empty() return an error_t status indicating
invalid_state (or assert) before touching ser_stack; only call ser_stack.back(),
pop_back(), or access parent after that check. Update the functions named
field(std::string_view), end_object(), and end_array() to perform this guard and
return error_kind::invalid_state via the status_t on failure so callers get a
diagnosable error instead of silent UB.
In `@include/kota/zest/snap.h`:
- Around line 93-128: The header uses symbols like ::kota::codec::json::to_json,
::kota::codec::content::Serializer<>, ::kota::codec::serialize,
::kota::zest::prettify_json, ::kota::zest::value_to_yaml,
::kota::zest::snap::check, ::kota::zest::snap::Result, ::kota::zest::print_trace
and ::kota::zest::failure but does not declare or include their headers; add a
short comment block above the snapshot macros listing the required includes (or
add the minimal `#include` directives) such as the headers that declare
kota/codec/json.h, kota/codec/content/serializer.h, and kota/zest.h so any user
including kota/zest/snap.h gets the needed declarations for
EXPECT_JSON_SNAPSHOT, ASSERT_JSON_SNAPSHOT, EXPECT_YAML_SNAPSHOT and
ASSERT_YAML_SNAPSHOT to compile.
In `@src/zest/snap.cpp`:
- Around line 399-406: The loop leaks a stale thread-local GlobContext if
callback(path) throws because glob_ctx.reset() is skipped; wrap per-iteration
context management in an RAII guard so the GlobContext is set at start and
always reset on scope exit (even on exceptions). Replace the manual
assignment/reset of current_glob_context()/glob_ctx and the trailing
glob_ctx.reset() with a small scoped helper (e.g., ScopedGlobContext or similar)
that constructs from path.stem().string() and clears the thread-local in its
destructor, and keep calls to reset_counter() and callback(path) unchanged
inside the loop so reset runs unconditionally.
- Around line 186-189: The conditional if(!child_compound && i < arr.size() - 1)
{ } is a dead/empty guard and should be fixed: either remove this entire if
statement (leaving no-op behavior) or implement the missing
separator/continuation logic for scalar array items—e.g., invoke the
serializer/emitter method used elsewhere to append a separator (comma/newline)
between items; locate usages around arr, i, and child_compound in snap.cpp (the
array serialization loop) and make the change consistently with how compound
items are handled.
In `@tests/unit/zest/snap_test.cpp`:
- Around line 159-175: Add a unit test in tests/unit/zest/snap_test.cpp that
calls snap::snapshot_path with both a positive counter (e.g., 1) and a non-empty
snap::GlobContext (e.g., {"input1"}) to assert the filename ordering is
"suite__case__stem@N.snap" (i.e., glob stem before `@N`); implement this as a new
TEST_CASE (name it something like snapshot_path_counter_and_glob) that verifies
path.filename().string() equals "suite__case__input1@2.snap" for the inputs used
and reuse the existing pattern of constructing the path and checking
filename().string().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfa7ec7d-5337-40f7-9d5a-8071794cac08
⛔ Files ignored due to path filters (8)
tests/unit/zest/__snapshots__/zest_snap__snapshot_json_struct.snapis excluded by!**/*.snaptests/unit/zest/__snapshots__/zest_snap__snapshot_json_vector.snapis excluded by!**/*.snaptests/unit/zest/__snapshots__/zest_snap__snapshot_multiple_per_test.snapis excluded by!**/*.snaptests/unit/zest/__snapshots__/zest_snap__snapshot_multiple_per_test@2.snapis excluded by!**/*.snaptests/unit/zest/__snapshots__/zest_snap__snapshot_multiple_per_test@3.snapis excluded by!**/*.snaptests/unit/zest/__snapshots__/zest_snap__snapshot_scalar.snapis excluded by!**/*.snaptests/unit/zest/__snapshots__/zest_snap__snapshot_scalar@2.snapis excluded by!**/*.snaptests/unit/zest/__snapshots__/zest_snap__snapshot_yaml_struct.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
CMakeLists.txtinclude/kota/codec/yaml/deserializer.hinclude/kota/codec/yaml/error.hinclude/kota/codec/yaml/serializer.hinclude/kota/codec/yaml/yaml.hinclude/kota/zest/detail/registry.hinclude/kota/zest/detail/suite.hinclude/kota/zest/snap.hsrc/codec/CMakeLists.txtsrc/zest/CMakeLists.txtsrc/zest/snap.cpptests/CMakeLists.txttests/unit/codec/yaml/yaml_tests.cpptests/unit/zest/snap_test.cpp
| template <codec::uint_like T> | ||
| status_t deserialize_uint(T& value) { | ||
| std::int64_t parsed = 0; | ||
| auto status = read_scalar(parsed, [](const YAML::Node& node) -> result_t<std::int64_t> { | ||
| std::int64_t v{}; | ||
| if(!YAML::convert<std::int64_t>::decode(node, v)) { | ||
| return std::unexpected(error_kind::type_mismatch); | ||
| } | ||
| return v; | ||
| }); | ||
| if(!status) { | ||
| return std::unexpected(status.error()); | ||
| } | ||
|
|
||
| if(parsed < 0) { | ||
| return mark_invalid(error_kind::number_out_of_range); | ||
| } | ||
|
|
||
| const auto unsigned_value = static_cast<std::uint64_t>(parsed); | ||
| auto narrowed = | ||
| codec::detail::narrow_uint<T>(unsigned_value, error_kind::number_out_of_range); | ||
| if(!narrowed) { | ||
| return mark_invalid(narrowed.error()); | ||
| } | ||
|
|
||
| value = *narrowed; | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does yaml-cpp YAML::convert<std::uint64_t>::decode reject negative scalar strings?
💡 Result:
Yes, yaml-cpp's YAML::convertstd::uint64_t::decode rejects negative scalar strings by returning false, which causes node.asstd::uint64_t to throw YAML::TypedBadConversion.
Citations:
- 1: Negative integer can be decoded as unsigned jbeder/yaml-cpp#859
- 2: https://github.com/jbeder/yaml-cpp/blob/master/include/yaml-cpp/node/convert.h
deserialize_uint loses values above INT64_MAX.
Decoding through std::int64_t first causes the method to reject unsigned integers in the range (INT64_MAX, UINT64_MAX]. For example, deserializing 18446744073709551615 (max uint64_t) into a std::uint64_t field fails with error_kind::type_mismatch instead of returning the correct value. Decode directly into std::uint64_t, which safely rejects negative scalars during the conversion.
🛠️ Suggested fix
template <codec::uint_like T>
status_t deserialize_uint(T& value) {
- std::int64_t parsed = 0;
- auto status = read_scalar(parsed, [](const YAML::Node& node) -> result_t<std::int64_t> {
- std::int64_t v{};
- if(!YAML::convert<std::int64_t>::decode(node, v)) {
+ std::uint64_t parsed = 0;
+ auto status = read_scalar(parsed, [](const YAML::Node& node) -> result_t<std::uint64_t> {
+ std::uint64_t v{};
+ if(!YAML::convert<std::uint64_t>::decode(node, v)) {
return std::unexpected(error_kind::type_mismatch);
}
return v;
});
if(!status) {
return std::unexpected(status.error());
}
- if(parsed < 0) {
- return mark_invalid(error_kind::number_out_of_range);
- }
-
- const auto unsigned_value = static_cast<std::uint64_t>(parsed);
auto narrowed =
- codec::detail::narrow_uint<T>(unsigned_value, error_kind::number_out_of_range);
+ codec::detail::narrow_uint<T>(parsed, error_kind::number_out_of_range);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/kota/codec/yaml/deserializer.h` around lines 162 - 189,
deserialize_uint currently decodes the scalar into std::int64_t which loses
values > INT64_MAX; change the reader lambda inside deserialize_uint/read_scalar
to decode into std::uint64_t (use YAML::convert<std::uint64_t>::decode) and then
reject negative values by checking the node or decoded value as needed before
narrowing; keep the subsequent cast to uint64_t/narrow_uint<T> and error
mappings (mark_invalid(error_kind::number_out_of_range) or type_mismatch) intact
so large unsigned values in (INT64_MAX, UINT64_MAX] are accepted and still
validated by codec::detail::narrow_uint, and ensure any type-mismatch for
negative scalars is preserved.
| result_t<std::optional<std::string_view>> next_field() { | ||
| if(!is_valid || deser_stack.empty()) { | ||
| return mark_invalid(error_kind::invalid_state); | ||
| } | ||
| auto& frame = deser_stack.back(); | ||
|
|
||
| if(frame.pending_valid) { | ||
| ++frame.iter; | ||
| frame.pending_valid = false; | ||
| } | ||
|
|
||
| if(frame.iter == frame.end_iter) { | ||
| current_value.clear(); | ||
| return std::optional<std::string_view>(std::nullopt); | ||
| } | ||
|
|
||
| YAML::convert<std::string>::decode(frame.iter->first, frame.pending_key); | ||
| frame.pending_valid = true; | ||
| current_value.set(frame.iter->second); | ||
| return std::optional<std::string_view>(frame.pending_key); | ||
| } |
There was a problem hiding this comment.
Unchecked YAML::convert<std::string>::decode in next_field.
The return value is ignored, so if key decoding ever fails (non-scalar key, unusual node state), pending_key is left in its prior/empty state and the caller receives an empty string_view as the field name without an error being reported. Check the return and propagate a real error.
🛠️ Suggested fix
- YAML::convert<std::string>::decode(frame.iter->first, frame.pending_key);
+ if(!YAML::convert<std::string>::decode(frame.iter->first, frame.pending_key)) {
+ return mark_invalid(error_kind::type_mismatch);
+ }
frame.pending_valid = true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| result_t<std::optional<std::string_view>> next_field() { | |
| if(!is_valid || deser_stack.empty()) { | |
| return mark_invalid(error_kind::invalid_state); | |
| } | |
| auto& frame = deser_stack.back(); | |
| if(frame.pending_valid) { | |
| ++frame.iter; | |
| frame.pending_valid = false; | |
| } | |
| if(frame.iter == frame.end_iter) { | |
| current_value.clear(); | |
| return std::optional<std::string_view>(std::nullopt); | |
| } | |
| YAML::convert<std::string>::decode(frame.iter->first, frame.pending_key); | |
| frame.pending_valid = true; | |
| current_value.set(frame.iter->second); | |
| return std::optional<std::string_view>(frame.pending_key); | |
| } | |
| result_t<std::optional<std::string_view>> next_field() { | |
| if(!is_valid || deser_stack.empty()) { | |
| return mark_invalid(error_kind::invalid_state); | |
| } | |
| auto& frame = deser_stack.back(); | |
| if(frame.pending_valid) { | |
| +frame.iter; | |
| frame.pending_valid = false; | |
| } | |
| if(frame.iter == frame.end_iter) { | |
| current_value.clear(); | |
| return std::optional<std::string_view>(std::nullopt); | |
| } | |
| if(!YAML::convert<std::string>::decode(frame.iter->first, frame.pending_key)) { | |
| return mark_invalid(error_kind::type_mismatch); | |
| } | |
| frame.pending_valid = true; | |
| current_value.set(frame.iter->second); | |
| return std::optional<std::string_view>(frame.pending_key); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/kota/codec/yaml/deserializer.h` around lines 290 - 310, The
next_field() function ignores the boolean result of
YAML::convert<std::string>::decode, so a failed key decode will leave
pending_key stale and return an empty field name; update next_field() to check
the decode return value (call to
YAML::convert<std::string>::decode(frame.iter->first, frame.pending_key)) and if
it returns false propagate an error via mark_invalid (e.g.
mark_invalid(error_kind::decode_error) or an appropriate error_kind), ensuring
you do not set frame.pending_valid or current_value before returning the error;
keep the rest of the iterator advancement logic intact so pending_valid is only
set on a successful decode.
| inline auto parse_node(std::string_view text) -> std::expected<YAML::Node, error> { | ||
| #if KOTA_ENABLE_EXCEPTIONS | ||
| try { | ||
| return YAML::Load(std::string(text)); | ||
| } catch(const YAML::ParserException&) { | ||
| return std::unexpected(error_kind::parse_error); | ||
| } | ||
| #else | ||
| return YAML::Load(std::string(text)); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does yaml-cpp 0.9.0 provide a YAML::Load variant that reports parse errors without throwing exceptions?
💡 Result:
No, yaml-cpp 0.9.0 does not provide a YAML::Load variant that reports parse errors without throwing exceptions. All YAML::Load and YAML::LoadFile functions explicitly throw ParserException on malformed input, as documented in the header file. The low-level YAML::Parser class's HandleNextDocument requires an EventHandler and does not directly produce Nodes without potential exceptions internally. Open GitHub issues confirm no support for exceptionless parsing, even when exceptions are compiler-disabled; throws are hardcoded. The 0.9.0 release notes list bugfixes but no API changes for non-throwing parse error reporting.
Citations:
- 1: https://github.com/jbeder/yaml-cpp/blob/master/include/yaml-cpp/node/parse.h
- 2: https://github.com/jbeder/yaml-cpp/releases
- 3: https://github.com/jbeder/yaml-cpp/releases/tag/yaml-cpp-0.9.0
- 4: https://github.com/jbeder/yaml-cpp
- 5: Please make throw optional (ie support build without exceptions) jbeder/yaml-cpp#196
- 6: Is it possiable to disable exception? jbeder/yaml-cpp#930
No-exceptions branch will abort if yaml-cpp is built with exceptions enabled.
When KOTA_ENABLE_EXCEPTIONS is 0, the code calls YAML::Load(...) unguarded. yaml-cpp 0.9.0 has no variant that reports parse errors without throwing exceptions—all YAML::Load and YAML::LoadFile functions explicitly throw YAML::ParserException on malformed input. Therefore, unless yaml-cpp itself has been built without exceptions (e.g., with YAML_CPP_NO_EXCEPTIONS), malformed input will trigger std::terminate instead of returning error_kind::parse_error.
Document the requirement that yaml-cpp must also be built without exceptions, or implement custom error handling using yaml-cpp's low-level Parser class with a custom EventHandler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/kota/codec/yaml/yaml.h` around lines 17 - 27, The no-exceptions
branch of parse_node currently calls YAML::Load(std::string(text)) unguarded so
if yaml-cpp was built with exceptions enabled a ParserException will still be
thrown (leading to terminate); either document that yaml-cpp must be built with
exceptions disabled (e.g., YAML_CPP_NO_EXCEPTIONS) when
KOTA_ENABLE_EXCEPTIONS==0, or replace the no-exceptions path with explicit
error-handling using yaml-cpp's low-level Parser and a custom EventHandler to
detect parse failures and return std::unexpected(error_kind::parse_error);
update parse_node, mention YAML::Load and error_kind::parse_error, and gate
behavior on KOTA_ENABLE_EXCEPTIONS so behavior is well-defined.
| template <typename T> | ||
| auto to_string(const T& value) -> std::expected<std::string, error> { | ||
| auto node = to_yaml(value); | ||
| if(!node) { | ||
| return std::unexpected(node.error()); | ||
| } | ||
|
|
||
| YAML::Emitter emitter; | ||
| emitter << *node; | ||
| return std::string(emitter.c_str()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does YAML::Emitter::c_str() return nullptr on error in yaml-cpp 0.9.0?
💡 Result:
No, YAML::Emitter::c_str does not return nullptr on error in yaml-cpp 0.9.0. It always returns the const char* to the internal stringstream's content via m_stream.str, even when !good. Check good or GetLastError to detect errors.
Citations:
- 1: https://github.com/jbeder/yaml-cpp/blob/master/src/emitter.cpp
- 2: https://github.com/jbeder/yaml-cpp/blob/master/include/yaml-cpp/emitter.h
- 3: https://github.com/jbeder/yaml-cpp
- 4: https://github.com/jbeder/yaml-cpp/releases/tag/yaml-cpp-0.9.0
🏁 Script executed:
# Search for yaml-cpp in the repository to find version info and understand the code context
fd -t f "yaml" | head -20
git ls-files | grep -i yaml | head -20Repository: clice-io/kotatsu
Length of output: 438
🏁 Script executed:
# Check the yaml.h file to see the full context and check for any error handling patterns
cat -n include/kota/codec/yaml/yaml.h | head -100Repository: clice-io/kotatsu
Length of output: 2635
🏁 Script executed:
# Check the error.h file to understand available error kinds
cat include/kota/codec/yaml/error.hRepository: clice-io/kotatsu
Length of output: 1012
Check emitter.good() to prevent silently returning corrupted YAML output.
YAML::Emitter::c_str() does not return nullptr on error; instead, it returns a pointer to the internal buffer even when the emitter has failed. This means a serialization error (invalid node structure, etc.) would silently produce partial or corrupted YAML rather than propagating the error to the caller. Call emitter.good() before returning to surface the failure properly.
Note: error_kind::serialize_error does not exist; use an appropriate error kind like error_kind::invalid_state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/kota/codec/yaml/yaml.h` around lines 48 - 58, The to_string<T>
template currently returns the Emitter buffer even when YAML::Emitter failed;
update the function (the to_string template that calls to_yaml and uses
YAML::Emitter) to call emitter.good() after emitting and before returning, and
if !emitter.good() return std::unexpected with an error constructed using an
appropriate kind (e.g., error_kind::invalid_state) and the
emitter.GetLastError() or emitter.c_str() as the message; keep the existing
to_yaml call and error path for node creation unchanged.
| #define ZEST_YAML_SNAPSHOT_CHECK(return_action, ...) \ | ||
| do { \ | ||
| ::kota::codec::content::Serializer<> _zest_yaml_ser_; \ | ||
| auto _zest_yaml_val_ = ::kota::codec::serialize(_zest_yaml_ser_, (__VA_ARGS__)); \ | ||
| auto _zest_snap_str_ = _zest_yaml_val_ \ | ||
| ? ::kota::zest::snap::value_to_yaml(*_zest_yaml_val_) \ | ||
| : std::string("<yaml serialization failed>"); \ | ||
| auto _zest_snap_result_ = ::kota::zest::snap::check( \ | ||
| _zest_snap_str_, #__VA_ARGS__, std::source_location::current()); \ | ||
| if(_zest_snap_result_ == ::kota::zest::snap::Result::mismatch) { \ | ||
| ::kota::zest::print_trace(std::source_location::current()); \ | ||
| ::kota::zest::failure(); \ | ||
| return_action; \ | ||
| } \ | ||
| } while(0) |
There was a problem hiding this comment.
Parenthesizing (__VA_ARGS__) creates a comma-expression for multi-arg calls.
Unlike ZEST_SNAPSHOT_CHECK and ZEST_JSON_SNAPSHOT_CHECK which forward __VA_ARGS__ directly, this macro wraps it: (__VA_ARGS__). If a caller writes EXPECT_YAML_SNAPSHOT(a, b), the expansion becomes codec::serialize(_zest_yaml_ser_, (a, b)), and the parenthesized (a, b) collapses to the second operand via the comma operator — silently dropping a. Either strip the parentheses (to get the same "single-value only" semantics as the other macros via a compile error on multi-arg calls) or switch to __VA_OPT__/helper to reject multi-arg usage explicitly.
🛠️ Suggested fix
- auto _zest_yaml_val_ = ::kota::codec::serialize(_zest_yaml_ser_, (__VA_ARGS__)); \
+ auto _zest_yaml_val_ = ::kota::codec::serialize(_zest_yaml_ser_, __VA_ARGS__); \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/kota/zest/snap.h` around lines 111 - 125, The macro
ZEST_YAML_SNAPSHOT_CHECK wraps __VA_ARGS__ in parentheses, causing a
comma-expression and silently dropping earlier arguments (e.g.,
codec::serialize(_zest_yaml_ser_, (__VA_ARGS__)) makes (a, b) evaluate to b);
remove the parentheses so the macro forwards __VA_ARGS__ directly like
ZEST_SNAPSHOT_CHECK and ZEST_JSON_SNAPSHOT_CHECK (i.e., use
codec::serialize(_zest_yaml_ser_, __VA_ARGS__)) so multi-argument calls fail to
compile rather than silently collapse — alternatively implement an
__VA_OPT__/helper check to explicitly reject multiple arguments if you prefer an
explicit diagnostic.
| if(KOTA_CODEC_ENABLE_YAML) | ||
| set(YAML_CPP_BUILD_TESTS OFF CACHE BOOL "" FORCE) | ||
| set(YAML_CPP_BUILD_TOOLS OFF CACHE BOOL "" FORCE) | ||
| set(YAML_CPP_BUILD_CONTRIB OFF CACHE BOOL "" FORCE) | ||
| set(YAML_CPP_INSTALL OFF CACHE BOOL "" FORCE) | ||
| set(YAML_CPP_FORMAT_SOURCE OFF CACHE BOOL "" FORCE) | ||
|
|
||
| kota_add_git_dependency( | ||
| yaml-cpp | ||
| GIT_REPOSITORY https://github.com/jbeder/yaml-cpp.git | ||
| GIT_TAG yaml-cpp-0.9.0 | ||
| OPTIONS | ||
| YAML_CPP_BUILD_TESTS OFF | ||
| YAML_CPP_BUILD_TOOLS OFF | ||
| YAML_CPP_BUILD_CONTRIB OFF | ||
| YAML_CPP_INSTALL OFF | ||
| YAML_CPP_FORMAT_SOURCE OFF | ||
| ) | ||
|
|
||
| add_library(kota_codec_yaml INTERFACE) | ||
| add_library(kota::codec::yaml ALIAS kota_codec_yaml) | ||
|
|
||
| target_include_directories(kota_codec_yaml INTERFACE | ||
| "${PROJECT_SOURCE_DIR}/include" | ||
| ) | ||
|
|
||
| target_link_libraries(kota_codec_yaml INTERFACE | ||
| kota::meta | ||
| yaml-cpp::yaml-cpp | ||
| ) | ||
|
|
||
| kota_apply_project_options(kota_codec_yaml) | ||
|
|
||
| target_link_libraries(kota_codec INTERFACE | ||
| kota::codec::yaml | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether yaml-cpp headers use try/catch that would be incompatible with -fno-exceptions in consuming TUs.
rg -n --type=cpp -g '!**/test/**' -C1 '\b(try|catch)\s*[({]' 2>/dev/null | head -50 || true
# Check how the codec header uses YAML and whether exception-bearing APIs are reachable when KOTA_ENABLE_EXCEPTIONS=0.
fd -t f 'yaml\.h$' include/kota/codec/yaml | xargs -I{} sh -c 'echo "=== {} ==="; cat {}'
fd -t f 'deserializer\.h$' include/kota/codec/yaml | xargs -I{} sh -c 'echo "=== {} ==="; cat {}'Repository: clice-io/kotatsu
Length of output: 20839
🌐 Web query:
Does yaml-cpp 0.9.0 support being built or consumed with -fno-exceptions (equivalent of TOML_EXCEPTIONS=0 / SIMDJSON_EXCEPTIONS=0)?
💡 Result:
No, yaml-cpp 0.9.0 does not support being built or consumed with -fno-exceptions. The library uses throw statements directly in its source code without conditional compilation to disable exceptions, causing compilation failures when exceptions are disabled. Multiple open GitHub issues (#799, #930, #196) request this support, and an unmerged PR (#1146) proposes a solution using a YAML_throw macro that calls a user-provided handler when exceptions are disabled. The 0.9.0 release notes list bugfixes and CMake updates but no mention of exceptions support. Stack Overflow confirms exceptions are hardcoded for version 0.8.0, and no evidence indicates changes in 0.9.0. Users must compile with exceptions enabled (-fexceptions).
Citations:
- 1: https://stackoverflow.com/questions/77294164/how-to-use-yaml-cpp-without-exceptions-support
- 2: Please make throw optional (ie support build without exceptions) jbeder/yaml-cpp#196
- 3: Question - Compiling without exception support jbeder/yaml-cpp#799
- 4: Support for building with exceptions disabled jbeder/yaml-cpp#1146
- 5: Is it possiable to disable exception? jbeder/yaml-cpp#930
- 6: https://github.com/jbeder/yaml-cpp/blob/master/include/yaml-cpp/exceptions.h
- 7: https://github.com/jbeder/yaml-cpp/releases/tag/yaml-cpp-0.9.0
- 8: https://github.com/jbeder/yaml-cpp/blob/master/CMakeLists.txt
- 9: https://github.com/jbeder/yaml-cpp/releases
yaml-cpp has no support for building or consuming with exceptions disabled; restrict header exposure or document platform incompatibility.
The yaml-cpp 0.9.0 library unconditionally uses throw statements without conditional compilation to disable exceptions. Unlike the TOML and simdjson backends (which forward TOML_EXCEPTIONS and SIMDJSON_EXCEPTIONS based on KOTA_ENABLE_EXCEPTIONS), yaml-cpp provides no equivalent mechanism.
When KOTA_ENABLE_EXCEPTIONS=OFF and the downstream project compiles with -fno-exceptions, including yaml-cpp/yaml.h (directly in kota::codec::yaml::Deserializer header) will fail to compile because the library's headers contain try-catch blocks that are incompatible with -fno-exceptions.
Additionally, the current code at include/kota/codec/yaml/yaml.h lines 23–24 still invokes YAML::Load() in the no-exceptions branch, which relies on yaml-cpp's exception-based error reporting without a mechanism to catch or handle those exceptions.
Either document that the YAML codec backend is incompatible with KOTA_ENABLE_EXCEPTIONS=OFF and prevent the target from being built in that configuration, or refactor to delay yaml-cpp header inclusion until exception handling is confirmed (e.g., via a runtime check or separate TU compiled with -fexceptions).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/codec/CMakeLists.txt` around lines 127 - 163, When KOTA_CODEC_ENABLE_YAML
is enabled but KOTA_ENABLE_EXCEPTIONS is OFF, yaml-cpp will break builds; update
the CMake logic so the kota_codec_yaml target (kota::codec::yaml) and its
target_link_libraries/target_include_directories are only created when
KOTA_ENABLE_EXCEPTIONS is ON, otherwise emit a clear CMake error/message that
the YAML backend requires exceptions; additionally, adjust
include/kota/codec/yaml/yaml.h to avoid calling YAML::Load or including yaml-cpp
headers in the no-exceptions branch (remove or guard the YAML::Load invocation)
so no translation unit pulls in yaml-cpp when exceptions are disabled.
| case ValueKind::floating: { | ||
| auto d = val.as_double(); | ||
| auto s = std::to_string(d); | ||
| // trim trailing zeros but keep at least one decimal | ||
| auto dot = s.find('.'); | ||
| if(dot != std::string::npos) { | ||
| auto last_nonzero = s.find_last_not_of('0'); | ||
| if(last_nonzero != std::string::npos && last_nonzero > dot) { | ||
| s.erase(last_nonzero + 1); | ||
| } else { | ||
| s.erase(dot + 2); | ||
| } | ||
| } | ||
| out += s; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Float emission mishandles inf/-inf/nan.
std::to_string(±inf) returns "inf"/"-inf" and NaN returns "nan" — none of them contain . so they bypass the trailing-zero trim and get emitted verbatim. YAML spec expects .inf / -.inf / .nan for these special values, so a snapshot containing a non-finite double will be non-standard YAML. Also worth noting: std::to_string(double) uses %f with 6-digit precision, which loses precision for very small/large values compared to std::format("{}", d) or std::to_chars round-trip formatting.
🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 137-137: Syntax Error
(internalAstError)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/zest/snap.cpp` around lines 128 - 143, The float-emission branch (case
ValueKind::floating) currently uses val.as_double() into d and std::to_string(d)
— this emits "inf"/"-inf"/"nan" and loses precision; update this block to first
test non-finite values with std::isfinite(d) and emit YAML-compliant strings
".inf", "-.inf", or ".nan" for +inf/-inf/NaN respectively (use val/as_double()
variable d), and for finite values replace std::to_string(d) with a
round-trip-safe formatter (std::to_chars or std::format/ostringstream with
sufficient precision) so you can still perform the trailing-zero trimming logic
on the resulting string (refer to variables s, dot, last_nonzero used in the
existing code).
1 participant
Summary
YAML::Nodetrees via the streaming codec interfaceYAML::Node, usingnode_slotwrapper to avoid yaml-cpp's destructiveoperator=semantics on shared nodesparse()/to_string()/from_yaml()/to_yaml()KOTA_CODEC_ENABLE_YAMLoption (auto-enabled byKOTA_BUILD_ALL_TESTS)Test plan
node_slotfix)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes