From 585b195593ea5066b55ad2f4f8ec5648c4b1a902 Mon Sep 17 00:00:00 2001 From: Chris Thompson Date: Wed, 16 Sep 2026 07:59:40 -0600 Subject: [PATCH 1/5] gguf: carry a GGUF input's embedded sidecars by name, not by walking for them Re-encoding a finished package dropped every sidecar the exclusion heuristic happens to match, exited 0, and reported embedded_sidecars=true: audiocpp_gguf --input kokoro-82m-q8_0.gguf --output out.gguf \ --type orig --model-spec model_specs/kokoro_tts.json note: reusing the sidecars embedded in kokoro-82m-q8_0.gguf embedded_sidecars=true 54 voices in -> 0 voices out, 28 MB smaller, and every load fails on voices/zm_yunxia.bin resolve_sidecar_root already materialises the input's sidecars and points the conversion at them, which is correct. What follows is not: the writer rediscovers them by walking that directory, and the walk applies the same filter used for a source tree of weights -- .safetensors, .gguf, .bin, .pt, .pth, anything over 64 MiB. Kokoro's voices are voices/*.bin, so they were read out of the package and then thrown away. The heuristic is right where it is used for a directory someone points at, and wrong for a directory this tool just wrote out of a finished package: those files are not candidates, they are the sidecar set. So carry them explicitly. Explicit sidecars already bypass both copies of the filter and are de-duplicated against the walk, so nothing is embedded twice and the safetensors path is untouched. The note now says how many, because "reusing the sidecars" was true of the old code too. Verified against the shipped Kokoro package, without --root: note: reusing the 59 sidecars embedded in kokoro-82m-q8_0.gguf extracted tree, before: 4 entries, no voices/ directory extracted tree, after: 59 files including voices/ with 54 -- byte-for-byte identical to the input's own output: 189,548,384 bytes vs 189,611,360 in (the delta is the replaced spec) The round trip it was blocking now works end to end: re-emitting that package with a spec declaring a new request option produces a package that offers the option AND still has its voices. --no-sidecars still refuses, --root still wins, and an explicit --sidecar set still suppresses the reuse as before. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EkxqpYvUbjCpRDnFiNiVfx --- app/gguf/main.cpp | 52 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/app/gguf/main.cpp b/app/gguf/main.cpp index 5296f0d67..803cc85ee 100644 --- a/app/gguf/main.cpp +++ b/app/gguf/main.cpp @@ -398,24 +398,50 @@ std::vector validate_candidate(const PackageSpecCandidate & candida // they are exactly the files that package ships, where the directory the file // happens to sit in is only a guess. Everything else keeps using that // directory, which is what every safetensors conversion does. -std::filesystem::path resolve_sidecar_root(const std::filesystem::path & requested, - const std::vector & inputs, - const std::vector & explicit_sidecars, - bool embed_sidecars) { +struct SidecarPlan { + std::filesystem::path root; + // Carried across by name rather than rediscovered by walking `root`. A + // directory walk applies excluded_sidecar(), which drops .bin, .pt, .pth and + // anything over 64 MiB — right for a source tree full of weights, wrong for + // files that were already chosen as a finished package's sidecars. Kokoro's + // voices are voices/*.bin, so a re-encode silently produced a package with + // none of its 54 voices while reporting embedded_sidecars=true. + std::vector carried; +}; + +SidecarPlan resolve_sidecar_root(const std::filesystem::path & requested, + const std::vector & inputs, + const std::vector & explicit_sidecars, + bool embed_sidecars) { if (!requested.empty()) - return std::filesystem::weakly_canonical(requested); + return {std::filesystem::weakly_canonical(requested), {}}; const auto parent = std::filesystem::weakly_canonical(inputs.front().path.parent_path()); if (!embed_sidecars || !explicit_sidecars.empty()) - return parent; + return {parent, {}}; for (const auto & input : inputs) { if (!is_gguf_path(input.path) || !engine::assets::gguf_has_embedded_sidecars(input.path)) continue; const auto materialized = engine::assets::materialize_gguf_sidecars(input.path); - std::cerr << "note: reusing the sidecars embedded in " << input.path.string() + std::vector carried; + std::error_code walk_error; + for (std::filesystem::recursive_directory_iterator it(materialized, walk_error), end; + !walk_error && it != end; it.increment(walk_error)) { + if (!it->is_regular_file()) + continue; + carried.push_back({it->path(), + std::filesystem::relative(it->path(), materialized).lexically_normal()}); + } + if (walk_error) { + throw std::runtime_error("failed to enumerate sidecars embedded in " + input.path.string() + ": " + + walk_error.message()); + } + std::sort(carried.begin(), carried.end(), + [](const auto & lhs, const auto & rhs) { return lhs.destination < rhs.destination; }); + std::cerr << "note: reusing the " << carried.size() << " sidecars embedded in " << input.path.string() << "; pass --root to override\n"; - return materialized; + return {materialized, std::move(carried)}; } - return parent; + return {parent, {}}; } PackageSpecCandidate select_package_spec(const std::vector & inputs, @@ -679,8 +705,12 @@ int main(int argc, char ** argv) { } } const auto storage_type = engine::assets::parse_tensor_storage_type(type); - const auto resolved_sidecar_root = - resolve_sidecar_root(sidecar_root, inputs, sidecars, embed_sidecars); + const auto sidecar_plan = resolve_sidecar_root(sidecar_root, inputs, sidecars, embed_sidecars); + const auto & resolved_sidecar_root = sidecar_plan.root; + // Empty unless the input was a GGUF whose sidecars are being reused, which + // resolve_sidecar_root only does when the caller passed no --sidecar of its + // own -- so these cannot collide with an explicit destination. + sidecars.insert(sidecars.end(), sidecar_plan.carried.begin(), sidecar_plan.carried.end()); std::optional embedded_model_spec; if (!allow_missing_model_spec) { embedded_model_spec = From eb6263b60e4348681e42eb64657a76139098574e Mon Sep 17 00:00:00 2001 From: Chris Thompson Date: Wed, 16 Sep 2026 08:03:31 -0600 Subject: [PATCH 2/5] review: read the sidecar list from the GGUF, and let --inspect show the count Four things from reviewing my own change. The carried set came from walking the materialisation directory. That directory is a cache under temp, keyed by path+size+mtime and shared between runs, and the carried set now bypasses every filter -- so anything sitting in it would have been embedded. The GGUF is the authority on what the package contains, so read the names from it; gguf_embedded_sidecar_names is the small accessor that was missing, alongside gguf_has_embedded_sidecars which already reads the same map and throws the contents away. Each name is then required to exist in the materialised tree rather than assumed, so a partial extraction fails with the name rather than silently shipping fewer files -- which is the shape of the bug being fixed. resolve_sidecar_root no longer only resolves a root, so it is plan_sidecars, and the comment above it describes both halves. --inspect reported embedded_sidecars=true for a package with 59 sidecars and for the same package with 5, which is part of why this went unnoticed. It now prints embedded_sidecar_count as well, and so does the conversion summary. The existing boolean line is left alone so anything parsing it still works. kokoro-82m-q8_0.gguf embedded_sidecars=true embedded_sidecar_count=59 kokoro-respec.gguf embedded_sidecars=true embedded_sidecar_count=5 Re-verified after the rewrite: 59 carried, extracted tree byte-for-byte identical to the input's, the phoneme round trip passes with no spec override, --no-sidecars still refuses, --root still suppresses the reuse. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EkxqpYvUbjCpRDnFiNiVfx --- app/gguf/main.cpp | 45 ++++++++++--------- .../engine/framework/assets/tensor_source.h | 4 ++ src/framework/assets/tensor_source.cpp | 9 ++++ 3 files changed, 38 insertions(+), 20 deletions(-) diff --git a/app/gguf/main.cpp b/app/gguf/main.cpp index 803cc85ee..26b265528 100644 --- a/app/gguf/main.cpp +++ b/app/gguf/main.cpp @@ -392,7 +392,8 @@ std::vector validate_candidate(const PackageSpecCandidate & candida return errors; } -// Where the conversion looks for the small files (configs, tokenizers) that +// What the conversion embeds, and where it looks for anything else: the small +// files (configs, tokenizers) that // belong in the output. `--root` wins, and so does an explicit `--sidecar` // set. Otherwise a GGUF input's own embedded copies are used — for a re-encode // they are exactly the files that package ships, where the directory the file @@ -409,7 +410,7 @@ struct SidecarPlan { std::vector carried; }; -SidecarPlan resolve_sidecar_root(const std::filesystem::path & requested, +SidecarPlan plan_sidecars(const std::filesystem::path & requested, const std::vector & inputs, const std::vector & explicit_sidecars, bool embed_sidecars) { @@ -422,21 +423,20 @@ SidecarPlan resolve_sidecar_root(const std::filesystem::path & requested, if (!is_gguf_path(input.path) || !engine::assets::gguf_has_embedded_sidecars(input.path)) continue; const auto materialized = engine::assets::materialize_gguf_sidecars(input.path); + // ⚠ The names come from the GGUF, not from a walk of `materialized`. That + // directory is a cache keyed by path+size+mtime and shared between runs, so + // walking it would carry across anything that happened to be sitting in it. + // The file is the authority on what the package contains. std::vector carried; - std::error_code walk_error; - for (std::filesystem::recursive_directory_iterator it(materialized, walk_error), end; - !walk_error && it != end; it.increment(walk_error)) { - if (!it->is_regular_file()) - continue; - carried.push_back({it->path(), - std::filesystem::relative(it->path(), materialized).lexically_normal()}); - } - if (walk_error) { - throw std::runtime_error("failed to enumerate sidecars embedded in " + input.path.string() + ": " + - walk_error.message()); + for (const auto & name : engine::assets::gguf_embedded_sidecar_names(input.path)) { + const auto destination = std::filesystem::path(name).lexically_normal(); + const auto source = materialized / destination; + if (!engine::io::is_existing_file(source)) { + throw std::runtime_error("sidecar '" + name + "' embedded in " + input.path.string() + + " did not materialise at " + source.string()); + } + carried.push_back({source, destination}); } - std::sort(carried.begin(), carried.end(), - [](const auto & lhs, const auto & rhs) { return lhs.destination < rhs.destination; }); std::cerr << "note: reusing the " << carried.size() << " sidecars embedded in " << input.path.string() << "; pass --root to override\n"; return {materialized, std::move(carried)}; @@ -677,8 +677,11 @@ int main(int argc, char ** argv) { std::cout << "gguf=" << std::filesystem::weakly_canonical(inspect_path).string() << "\n"; std::cout << "tensors=" << tensors.size() << "\n"; std::cout << "rank0_scalars=" << scalar_count << "\n"; - std::cout << "embedded_sidecars=" - << (engine::assets::gguf_has_embedded_sidecars(inspect_path) ? "true" : "false") << "\n"; + // The count, not just the boolean: a package that lost 54 of its 59 + // sidecars still reports true, which is how that went unnoticed. + const auto inspect_sidecars = engine::assets::gguf_embedded_sidecar_names(inspect_path); + std::cout << "embedded_sidecars=" << (inspect_sidecars.empty() ? "false" : "true") << "\n"; + std::cout << "embedded_sidecar_count=" << inspect_sidecars.size() << "\n"; const auto model_spec = engine::assets::read_gguf_embedded_model_spec(inspect_path); std::cout << "embedded_model_spec=" << (model_spec.has_value() ? "true" : "false") << "\n"; if (model_spec.has_value()) @@ -705,10 +708,10 @@ int main(int argc, char ** argv) { } } const auto storage_type = engine::assets::parse_tensor_storage_type(type); - const auto sidecar_plan = resolve_sidecar_root(sidecar_root, inputs, sidecars, embed_sidecars); + const auto sidecar_plan = plan_sidecars(sidecar_root, inputs, sidecars, embed_sidecars); const auto & resolved_sidecar_root = sidecar_plan.root; // Empty unless the input was a GGUF whose sidecars are being reused, which - // resolve_sidecar_root only does when the caller passed no --sidecar of its + // plan_sidecars only does when the caller passed no --sidecar of its // own -- so these cannot collide with an explicit destination. sidecars.insert(sidecars.end(), sidecar_plan.carried.begin(), sidecar_plan.carried.end()); std::optional embedded_model_spec; @@ -741,7 +744,9 @@ int main(int argc, char ** argv) { std::cout << "excluded_prefix=" << prefix << "\n"; std::cout << "type_overrides=" << conversion_options.type_overrides.size() << "\n"; std::cout << "folded_weight_norm_patterns=" << conversion_options.folded_weight_norm_patterns.size() << "\n"; - std::cout << "embedded_sidecars=" << (engine::assets::gguf_has_embedded_sidecars(output) ? "true" : "false") + const auto written_sidecars = engine::assets::gguf_embedded_sidecar_names(output); + std::cout << "embedded_sidecar_count=" << written_sidecars.size() << "\n"; + std::cout << "embedded_sidecars=" << (written_sidecars.empty() ? "false" : "true") << "\n"; std::cout << "embedded_model_spec=" << (embedded_model_spec.has_value() ? "true" : "false") << "\n"; if (embedded_model_spec.has_value()) { diff --git a/include/engine/framework/assets/tensor_source.h b/include/engine/framework/assets/tensor_source.h index 03f6090f3..fda0585e2 100644 --- a/include/engine/framework/assets/tensor_source.h +++ b/include/engine/framework/assets/tensor_source.h @@ -196,6 +196,10 @@ void convert_tensor_sources_to_gguf(const std::vector & input void convert_tensor_source_to_gguf(const std::filesystem::path & input_path, const std::filesystem::path & output_path, TensorStorageType weight_type, bool overwrite = false, bool embed_sidecars = true); [[nodiscard]] bool gguf_has_embedded_sidecars(const std::filesystem::path & path); +// The destinations of a GGUF's embedded sidecars, in the order it stores them. +// Read from the file itself, so a caller re-emitting a package does not have to +// infer the set from whatever is sitting in the materialisation cache. +[[nodiscard]] std::vector gguf_embedded_sidecar_names(const std::filesystem::path & path); [[nodiscard]] std::optional read_gguf_embedded_model_spec(const std::filesystem::path & path); [[nodiscard]] std::filesystem::path materialize_gguf_sidecars(const std::filesystem::path & path); struct PreparedModelDirectory { diff --git a/src/framework/assets/tensor_source.cpp b/src/framework/assets/tensor_source.cpp index d5e8348da..2d092557e 100644 --- a/src/framework/assets/tensor_source.cpp +++ b/src/framework/assets/tensor_source.cpp @@ -1627,6 +1627,15 @@ bool gguf_has_embedded_sidecars(const std::filesystem::path & path) { return !read_gguf_embedded_sidecars(path).empty(); } +std::vector gguf_embedded_sidecar_names(const std::filesystem::path & path) { + std::vector names; + for (const auto & [name, content] : read_gguf_embedded_sidecars(path)) { + (void) content; + names.push_back(name); + } + return names; +} + std::optional read_gguf_embedded_model_spec(const std::filesystem::path & path) { ggml_context * tensor_context = nullptr; gguf_context * gguf = gguf_init_from_file(path.string().c_str(), gguf_init_params{true, &tensor_context}); From 6766e843d17e5e4b47dcd4e8832b5efb3ec7aba3 Mon Sep 17 00:00:00 2001 From: Chris Thompson Date: Wed, 16 Sep 2026 08:07:45 -0600 Subject: [PATCH 3/5] gguf: check the finished package against the one it was made from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tool reported on the output without checking it. embedded_sidecars was read from the file but was only a boolean, so a package holding 5 of its 59 sidecars still said true; embedded_model_spec was read from the in-memory intent and never from the file at all, so a spec that failed to embed would have reported success too. A conversion that reuses a GGUF's sidecars now reads the finished file back and compares it against the package it came from, naming what is missing: error: re-encoding kokoro-82m-q8_0.gguf kept 5 of its 59 sidecars; missing: voices/af_alloy.bin voices/af_aoede.bin ... and 46 more and exits 1. The model spec line is read back from the output as well. ⚠ The first version of this check did not work, and only a deliberate regression showed it. It compared the output against planned_sidecar_destinations -- which is built by the same filtered walk that dropped the voices in the first place, so it agreed with a run that kept 5 of 59 and exited 0. A check derived from the thing it is checking cannot fail. The expected set is now read from the INPUT package, independently of the plan that produced the output, so a bug in the carrying is caught by the check rather than shared with it. Verified both ways: with the carry deliberately truncated to 5 the run fails with the list above; with it intact, 59 sidecars, spec present, exit 0, and the re-emitted package drives the phoneme path with no spec override. --root pointing at a directory without the sidecars fails exactly as it did before. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EkxqpYvUbjCpRDnFiNiVfx --- app/gguf/main.cpp | 59 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 52 insertions(+), 7 deletions(-) diff --git a/app/gguf/main.cpp b/app/gguf/main.cpp index 26b265528..5e8012d8b 100644 --- a/app/gguf/main.cpp +++ b/app/gguf/main.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -408,6 +409,10 @@ struct SidecarPlan { // voices are voices/*.bin, so a re-encode silently produced a package with // none of its 54 voices while reporting embedded_sidecars=true. std::vector carried; + // The package being re-emitted, if any. The end-of-run check reads its sidecar + // list again from the file rather than trusting `carried`, so a bug in the + // carrying is caught by the check instead of hidden by it. + std::filesystem::path reused_from; }; SidecarPlan plan_sidecars(const std::filesystem::path & requested, @@ -415,10 +420,10 @@ SidecarPlan plan_sidecars(const std::filesystem::path & requested, const std::vector & explicit_sidecars, bool embed_sidecars) { if (!requested.empty()) - return {std::filesystem::weakly_canonical(requested), {}}; + return {std::filesystem::weakly_canonical(requested), {}, {}}; const auto parent = std::filesystem::weakly_canonical(inputs.front().path.parent_path()); if (!embed_sidecars || !explicit_sidecars.empty()) - return {parent, {}}; + return {parent, {}, {}}; for (const auto & input : inputs) { if (!is_gguf_path(input.path) || !engine::assets::gguf_has_embedded_sidecars(input.path)) continue; @@ -439,9 +444,9 @@ SidecarPlan plan_sidecars(const std::filesystem::path & requested, } std::cerr << "note: reusing the " << carried.size() << " sidecars embedded in " << input.path.string() << "; pass --root to override\n"; - return {materialized, std::move(carried)}; + return {materialized, std::move(carried), input.path}; } - return {parent, {}}; + return {parent, {}, {}}; } PackageSpecCandidate select_package_spec(const std::vector & inputs, @@ -744,13 +749,53 @@ int main(int argc, char ** argv) { std::cout << "excluded_prefix=" << prefix << "\n"; std::cout << "type_overrides=" << conversion_options.type_overrides.size() << "\n"; std::cout << "folded_weight_norm_patterns=" << conversion_options.folded_weight_norm_patterns.size() << "\n"; + // ⚠ READ THE FINISHED FILE BACK AND CHECK IT AGAINST WHAT WAS ASKED FOR. + // This tool used to write a package missing 54 of its 59 sidecars, exit 0, + // and report embedded_sidecars=true -- because "true" was read from the + // output but was too coarse to notice, and the model-spec line was read + // from the in-memory intent rather than from the file at all. A claim + // about the output that is not read from the output is not a check. const auto written_sidecars = engine::assets::gguf_embedded_sidecar_names(output); + // ⚠ The expected set is read from the INPUT package, not from the plan that + // produced the output. An earlier version of this check compared against + // planned_sidecar_destinations, which is built by the same filtered walk that + // caused the bug -- so it agreed with a run that dropped 54 voices. A check + // derived from the thing it is checking cannot fail. + if (embed_sidecars && !sidecar_plan.reused_from.empty()) { + const auto expected = engine::assets::gguf_embedded_sidecar_names(sidecar_plan.reused_from); + const std::set expected_set(expected.begin(), expected.end()); + const std::set written(written_sidecars.begin(), written_sidecars.end()); + std::vector missing; + std::set_difference(expected_set.begin(), expected_set.end(), written.begin(), written.end(), + std::back_inserter(missing)); + if (!missing.empty()) { + std::ostringstream report; + report << "re-encoding " << sidecar_plan.reused_from.string() << " kept " << written.size() + << " of its " << expected_set.size() << " sidecars; missing:"; + for (size_t i = 0; i < missing.size() && i < 8; ++i) + report << "\n " << missing[i]; + if (missing.size() > 8) + report << "\n ... and " << (missing.size() - 8) << " more"; + throw std::runtime_error(report.str()); + } + } + // Read back, not reported from intent: a spec that failed to embed would + // otherwise still print true. + const auto written_spec = engine::assets::read_gguf_embedded_model_spec(output); + if (embedded_model_spec.has_value() && !written_spec.has_value()) { + throw std::runtime_error("model spec for family '" + embedded_model_spec->family + + "' was not embedded in " + output.string()); + } + if (embedded_model_spec.has_value() && written_spec->family != embedded_model_spec->family) { + throw std::runtime_error("embedded model spec family is '" + written_spec->family + "', expected '" + + embedded_model_spec->family + "'"); + } std::cout << "embedded_sidecar_count=" << written_sidecars.size() << "\n"; std::cout << "embedded_sidecars=" << (written_sidecars.empty() ? "false" : "true") << "\n"; - std::cout << "embedded_model_spec=" << (embedded_model_spec.has_value() ? "true" : "false") << "\n"; - if (embedded_model_spec.has_value()) { - std::cout << "model_spec_family=" << embedded_model_spec->family << "\n"; + std::cout << "embedded_model_spec=" << (written_spec.has_value() ? "true" : "false") << "\n"; + if (written_spec.has_value()) { + std::cout << "model_spec_family=" << written_spec->family << "\n"; } return 0; } catch (const std::exception & error) { From 46db94aa2c8b955e08dc12344280ccc6d1f3830f Mon Sep 17 00:00:00 2001 From: Chris Thompson Date: Wed, 16 Sep 2026 08:10:03 -0600 Subject: [PATCH 4/5] review: read only the names, and tidy what the edits left behind gguf_embedded_sidecar_names went through read_gguf_embedded_sidecars, which copies every sidecar's CONTENT into a string in order to return the keys -- 38 MB for the Kokoro package, and it is now called three times per run. The names are their own KV array, so read that. The unsafe-name rejection is carried over rather than dropped: a name that escapes the package root must not reach a caller that is about to write files from it. Cosmetics the earlier edits left: plan_sidecars' continuation lines were still aligned to the longer name it used to have, the comment describing it had been spliced mid-sentence and then separated from it by the new struct, and the conversion summary printed the count before the boolean where --inspect prints it after. Re-verified: 59 sidecars carried, extracted tree byte-for-byte identical to the source, --inspect and the summary agree, and the re-emitted package drives the phoneme path with no spec override. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EkxqpYvUbjCpRDnFiNiVfx --- app/gguf/main.cpp | 27 +++++++++--------- src/framework/assets/tensor_source.cpp | 39 ++++++++++++++++++++++++-- 2 files changed, 49 insertions(+), 17 deletions(-) diff --git a/app/gguf/main.cpp b/app/gguf/main.cpp index 5e8012d8b..55c285bfc 100644 --- a/app/gguf/main.cpp +++ b/app/gguf/main.cpp @@ -393,13 +393,6 @@ std::vector validate_candidate(const PackageSpecCandidate & candida return errors; } -// What the conversion embeds, and where it looks for anything else: the small -// files (configs, tokenizers) that -// belong in the output. `--root` wins, and so does an explicit `--sidecar` -// set. Otherwise a GGUF input's own embedded copies are used — for a re-encode -// they are exactly the files that package ships, where the directory the file -// happens to sit in is only a guess. Everything else keeps using that -// directory, which is what every safetensors conversion does. struct SidecarPlan { std::filesystem::path root; // Carried across by name rather than rediscovered by walking `root`. A @@ -415,10 +408,17 @@ struct SidecarPlan { std::filesystem::path reused_from; }; +// What the conversion embeds, and where it looks for anything else. The small +// files (configs, tokenizers) that belong in the output: `--root` wins, and so +// does an explicit `--sidecar` set. Otherwise a GGUF input's own embedded copies +// are used — for a re-encode they are exactly the files that package ships, +// where the directory the file happens to sit in is only a guess. Everything +// else keeps using that directory, which is what every safetensors conversion +// does. SidecarPlan plan_sidecars(const std::filesystem::path & requested, - const std::vector & inputs, - const std::vector & explicit_sidecars, - bool embed_sidecars) { + const std::vector & inputs, + const std::vector & explicit_sidecars, + bool embed_sidecars) { if (!requested.empty()) return {std::filesystem::weakly_canonical(requested), {}, {}}; const auto parent = std::filesystem::weakly_canonical(inputs.front().path.parent_path()); @@ -716,8 +716,8 @@ int main(int argc, char ** argv) { const auto sidecar_plan = plan_sidecars(sidecar_root, inputs, sidecars, embed_sidecars); const auto & resolved_sidecar_root = sidecar_plan.root; // Empty unless the input was a GGUF whose sidecars are being reused, which - // plan_sidecars only does when the caller passed no --sidecar of its - // own -- so these cannot collide with an explicit destination. + // plan_sidecars only does when the caller passed no --sidecar of its own, + // so these cannot collide with an explicit destination. sidecars.insert(sidecars.end(), sidecar_plan.carried.begin(), sidecar_plan.carried.end()); std::optional embedded_model_spec; if (!allow_missing_model_spec) { @@ -790,9 +790,8 @@ int main(int argc, char ** argv) { throw std::runtime_error("embedded model spec family is '" + written_spec->family + "', expected '" + embedded_model_spec->family + "'"); } + std::cout << "embedded_sidecars=" << (written_sidecars.empty() ? "false" : "true") << "\n"; std::cout << "embedded_sidecar_count=" << written_sidecars.size() << "\n"; - std::cout << "embedded_sidecars=" << (written_sidecars.empty() ? "false" : "true") - << "\n"; std::cout << "embedded_model_spec=" << (written_spec.has_value() ? "true" : "false") << "\n"; if (written_spec.has_value()) { std::cout << "model_spec_family=" << written_spec->family << "\n"; diff --git a/src/framework/assets/tensor_source.cpp b/src/framework/assets/tensor_source.cpp index 2d092557e..6c1120c23 100644 --- a/src/framework/assets/tensor_source.cpp +++ b/src/framework/assets/tensor_source.cpp @@ -1628,11 +1628,44 @@ bool gguf_has_embedded_sidecars(const std::filesystem::path & path) { } std::vector gguf_embedded_sidecar_names(const std::filesystem::path & path) { + // Reads only "audiocpp.embedded_files.names". Going through + // read_gguf_embedded_sidecars would copy every sidecar's CONTENT into a string + // to return the keys -- 38 MB for the Kokoro package and far more for a large + // one, per call. + ggml_context * tensor_context = nullptr; + gguf_context * gguf = + gguf_init_from_file(path.string().c_str(), gguf_init_params{true, &tensor_context}); + if (gguf == nullptr) { + if (tensor_context != nullptr) ggml_free(tensor_context); + throw std::runtime_error("failed to read GGUF metadata: " + path.string()); + } std::vector names; - for (const auto & [name, content] : read_gguf_embedded_sidecars(path)) { - (void) content; - names.push_back(name); + try { + const int64_t names_key = gguf_find_key(gguf, "audiocpp.embedded_files.names"); + if (names_key >= 0 && gguf_get_kv_type(gguf, names_key) == GGUF_TYPE_ARRAY && + gguf_get_arr_type(gguf, names_key) == GGUF_TYPE_STRING) { + const size_t count = gguf_get_arr_n(gguf, names_key); + names.reserve(count); + for (size_t i = 0; i < count; ++i) { + // Same rejection as the full reader: a name that escapes the package + // root must not reach a caller that is about to write files from it. + const std::string name = gguf_get_arr_str(gguf, names_key, i); + const std::filesystem::path relative(name); + const auto normalized = relative.lexically_normal(); + if (name.empty() || relative.is_absolute() || normalized.empty() || + *normalized.begin() == "..") { + throw std::runtime_error("GGUF contains an unsafe embedded sidecar name: " + name); + } + names.push_back(normalized.generic_string()); + } + } + } catch (...) { + gguf_free(gguf); + if (tensor_context != nullptr) ggml_free(tensor_context); + throw; } + gguf_free(gguf); + if (tensor_context != nullptr) ggml_free(tensor_context); return names; } From e867b8505d50abddb58c06d146214f3ae245c05b Mon Sep 17 00:00:00 2001 From: Chris Thompson Date: Wed, 16 Sep 2026 11:23:33 -0600 Subject: [PATCH 5/5] gguf: make the name reader validate what the content reader validates The name-only accessor added for the sidecar carry read `audiocpp.embedded_files.names` directly and checked only the names, so a package whose byte ranges do not fit its blob was reported as holding sidecars instead of being rejected: `--inspect` printed a count and exited 0 where it used to fail, since the boolean it replaced went through the full reader. The shortcut is now a flag on that reader rather than a second implementation of it. `with_contents == false` still walks the whole metadata and every byte range, and skips only the copy out of the blob -- which is what the accessor was for, 38 MB per call on the Kokoro package. `gguf_has_embedded_sidecars` takes the same path and stops copying too. The test builds a package with one sidecar, overwrites its offsets with a range past the end of the blob, and requires both accessors to throw. It fails on the previous reader with "name reader accepted an out-of-range sidecar byte range". Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014X4jSwJu1ScZVgUspcA9HC --- .../engine/framework/assets/tensor_source.h | 5 +- src/framework/assets/tensor_source.cpp | 58 +++++-------------- tests/unittests/test_gguf_tensor_source.cpp | 50 ++++++++++++++++ 3 files changed, 70 insertions(+), 43 deletions(-) diff --git a/include/engine/framework/assets/tensor_source.h b/include/engine/framework/assets/tensor_source.h index fda0585e2..4da534418 100644 --- a/include/engine/framework/assets/tensor_source.h +++ b/include/engine/framework/assets/tensor_source.h @@ -198,7 +198,10 @@ void convert_tensor_source_to_gguf(const std::filesystem::path & input_path, con [[nodiscard]] bool gguf_has_embedded_sidecars(const std::filesystem::path & path); // The destinations of a GGUF's embedded sidecars, in the order it stores them. // Read from the file itself, so a caller re-emitting a package does not have to -// infer the set from whatever is sitting in the materialisation cache. +// infer the set from whatever is sitting in the materialisation cache. Throws on +// the same malformed metadata that materialising the sidecars would reject: this +// answers what the package holds, and a package whose byte ranges do not fit its +// blob holds nothing anyone can read. [[nodiscard]] std::vector gguf_embedded_sidecar_names(const std::filesystem::path & path); [[nodiscard]] std::optional read_gguf_embedded_model_spec(const std::filesystem::path & path); [[nodiscard]] std::filesystem::path materialize_gguf_sidecars(const std::filesystem::path & path); diff --git a/src/framework/assets/tensor_source.cpp b/src/framework/assets/tensor_source.cpp index 6c1120c23..f94ba1f66 100644 --- a/src/framework/assets/tensor_source.cpp +++ b/src/framework/assets/tensor_source.cpp @@ -1542,8 +1542,14 @@ std::shared_ptr make_prefixed_tensor_source( namespace { +// `with_contents == false` validates the same metadata but skips copying each +// sidecar's bytes out of the blob -- 38 MB for the Kokoro package, per call, when +// the caller only wants the names. Validation is deliberately not skipped with it: +// a caller that reads the names of a malformed package must fail the way a caller +// that reads its contents does. std::vector> read_gguf_embedded_sidecars( - const std::filesystem::path & path) { + const std::filesystem::path & path, + bool with_contents) { ggml_context * tensor_context = nullptr; gguf_context * gguf = gguf_init_from_file( path.string().c_str(), gguf_init_params{true, &tensor_context}); @@ -1605,10 +1611,12 @@ std::vector> read_gguf_embedded_sidecars( } std::string content; const size_t length = static_cast(offsets[i + 1] - offsets[i]); - if (length > 0) content.assign(data + offsets[i], length); + if (with_contents && length > 0) content.assign(data + offsets[i], length); result.emplace_back(normalized.generic_string(), std::move(content)); } else { - result.emplace_back(normalized.generic_string(), gguf_get_arr_str(gguf, contents_key, i)); + result.emplace_back( + normalized.generic_string(), + with_contents ? std::string(gguf_get_arr_str(gguf, contents_key, i)) : std::string()); } } } catch (...) { @@ -1624,48 +1632,14 @@ std::vector> read_gguf_embedded_sidecars( } // namespace bool gguf_has_embedded_sidecars(const std::filesystem::path & path) { - return !read_gguf_embedded_sidecars(path).empty(); + return !read_gguf_embedded_sidecars(path, false).empty(); } std::vector gguf_embedded_sidecar_names(const std::filesystem::path & path) { - // Reads only "audiocpp.embedded_files.names". Going through - // read_gguf_embedded_sidecars would copy every sidecar's CONTENT into a string - // to return the keys -- 38 MB for the Kokoro package and far more for a large - // one, per call. - ggml_context * tensor_context = nullptr; - gguf_context * gguf = - gguf_init_from_file(path.string().c_str(), gguf_init_params{true, &tensor_context}); - if (gguf == nullptr) { - if (tensor_context != nullptr) ggml_free(tensor_context); - throw std::runtime_error("failed to read GGUF metadata: " + path.string()); - } + const auto sidecars = read_gguf_embedded_sidecars(path, false); std::vector names; - try { - const int64_t names_key = gguf_find_key(gguf, "audiocpp.embedded_files.names"); - if (names_key >= 0 && gguf_get_kv_type(gguf, names_key) == GGUF_TYPE_ARRAY && - gguf_get_arr_type(gguf, names_key) == GGUF_TYPE_STRING) { - const size_t count = gguf_get_arr_n(gguf, names_key); - names.reserve(count); - for (size_t i = 0; i < count; ++i) { - // Same rejection as the full reader: a name that escapes the package - // root must not reach a caller that is about to write files from it. - const std::string name = gguf_get_arr_str(gguf, names_key, i); - const std::filesystem::path relative(name); - const auto normalized = relative.lexically_normal(); - if (name.empty() || relative.is_absolute() || normalized.empty() || - *normalized.begin() == "..") { - throw std::runtime_error("GGUF contains an unsafe embedded sidecar name: " + name); - } - names.push_back(normalized.generic_string()); - } - } - } catch (...) { - gguf_free(gguf); - if (tensor_context != nullptr) ggml_free(tensor_context); - throw; - } - gguf_free(gguf); - if (tensor_context != nullptr) ggml_free(tensor_context); + names.reserve(sidecars.size()); + for (const auto & sidecar : sidecars) names.push_back(sidecar.first); return names; } @@ -1716,7 +1690,7 @@ std::optional read_gguf_embedded_model_spec(const std::fi std::filesystem::path materialize_gguf_sidecars(const std::filesystem::path & path) { const auto canonical = std::filesystem::weakly_canonical(path); - const auto sidecars = read_gguf_embedded_sidecars(canonical); + const auto sidecars = read_gguf_embedded_sidecars(canonical, true); if (sidecars.empty()) { throw std::runtime_error("GGUF does not contain embedded model sidecars: " + canonical.string()); } diff --git a/tests/unittests/test_gguf_tensor_source.cpp b/tests/unittests/test_gguf_tensor_source.cpp index 7ff0ab297..87e2ad4df 100644 --- a/tests/unittests/test_gguf_tensor_source.cpp +++ b/tests/unittests/test_gguf_tensor_source.cpp @@ -2,6 +2,7 @@ #include "engine/framework/assets/tensor_source.h" #include "engine/framework/io/filesystem.h" #include "engine/framework/io/safetensors.h" +#include "gguf.h" #include "test_assert.h" #include @@ -375,6 +376,54 @@ void test_package_spec_errors_name_selected_spec() { std::filesystem::remove_all(root); } +// The name reader is a shortcut past the content copy, not past the validation: a +// package whose byte ranges do not fit its blob is malformed whether the caller +// wants the names or the bytes. --inspect reads the names, and used to report a +// count for a package no loader could ever read (PR #579 review). +void test_malformed_embedded_sidecars_are_rejected_by_the_name_reader() { + const auto root = std::filesystem::temp_directory_path() / "audiocpp_malformed_sidecar_test"; + std::filesystem::remove_all(root); + std::filesystem::create_directories(root); + const auto safetensors = root / "model.safetensors"; + const auto gguf = root / "model.gguf"; + engine::io::write_safetensors_file(safetensors, { + {"weight", "F32", {1}, bytes_for(std::vector{1.0F})}, + }); + { + std::ofstream output(root / "config.json", std::ios::binary); + output << "{}"; + } + engine::assets::convert_tensor_sources_to_gguf({{safetensors, ""}}, gguf, engine::assets::TensorStorageType::F16, + false, true, root, {{root / "config.json", "config.json"}}); + engine::test::require_eq(engine::assets::gguf_embedded_sidecar_names(gguf).size(), size_t{1}, + "intact package sidecar names"); + + auto * ctx = gguf_init_from_file(gguf.string().c_str(), {true, nullptr}); + engine::test::require(ctx != nullptr, "open fixture GGUF"); + const uint64_t offsets[] = {0, 999}; + gguf_set_arr_data(ctx, "audiocpp.embedded_files.offsets", GGUF_TYPE_UINT64, offsets, 2); + engine::test::require(gguf_write_to_file(ctx, gguf.string().c_str(), true), "write malformed fixture"); + gguf_free(ctx); + + bool names_rejected = false; + try { + (void)engine::assets::gguf_embedded_sidecar_names(gguf); + } catch (const std::runtime_error &) { + names_rejected = true; + } + engine::test::require(names_rejected, "name reader accepted an out-of-range sidecar byte range"); + + bool presence_rejected = false; + try { + (void)engine::assets::gguf_has_embedded_sidecars(gguf); + } catch (const std::runtime_error &) { + presence_rejected = true; + } + engine::test::require(presence_rejected, "presence check accepted an out-of-range sidecar byte range"); + + std::filesystem::remove_all(root); +} + } // namespace int main() { @@ -385,6 +434,7 @@ int main() { test_embedded_model_spec_roundtrip_and_precedence(); test_release_named_gguf_directory_selects_the_gguf_source(); test_package_spec_errors_name_selected_spec(); + test_malformed_embedded_sidecars_are_rejected_by_the_name_reader(); } catch (const std::exception & error) { std::cerr << error.what() << '\n'; return 1;