gguf: carry a GGUF input's embedded sidecars, and check the finished package against the one it was made from

app/gguf/main.cpp

@@ -6,6 +6,7 @@
 #include <cctype>
 #include <filesystem>
 #include <iostream>
+#include <iterator>
 #include <optional>
 #include <set>
 #include <sstream>
@@ -392,30 +393,60 @@ std::vector<std::string> validate_candidate(const PackageSpecCandidate & candida
     return errors;
 }
 
-// Where the conversion looks for 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.
-std::filesystem::path resolve_sidecar_root(const std::filesystem::path & requested,
-                                           const std::vector<engine::assets::TensorSourceInput> & inputs,
-                                           const std::vector<engine::assets::GgufEmbeddedFile> & 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<engine::assets::GgufEmbeddedFile> 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;
+};
+
+// 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<engine::assets::TensorSourceInput> & inputs,
+                          const std::vector<engine::assets::GgufEmbeddedFile> & 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()
+        // ⚠ 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<engine::assets::GgufEmbeddedFile> carried;
+        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::cerr << "note: reusing the " << carried.size() << " sidecars embedded in " << input.path.string()
                   << "; pass --root to override\n";
-        return materialized;
+        return {materialized, std::move(carried), input.path};
     }
-    return parent;
+    return {parent, {}, {}};
 }
 
 PackageSpecCandidate select_package_spec(const std::vector<engine::assets::TensorSourceInput> & inputs,
@@ -651,8 +682,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())
@@ -679,8 +713,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 = 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.
+        sidecars.insert(sidecars.end(), sidecar_plan.carried.begin(), sidecar_plan.carried.end());
         std::optional<engine::assets::GgufEmbeddedModelSpec> embedded_model_spec;
         if (!allow_missing_model_spec) {
             embedded_model_spec =
@@ -711,11 +749,52 @@ 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")
-                  << "\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";
+        // ⚠ 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<std::string> expected_set(expected.begin(), expected.end());
+            const std::set<std::string> written(written_sidecars.begin(), written_sidecars.end());
+            std::vector<std::string> 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_sidecars=" << (written_sidecars.empty() ? "false" : "true") << "\n";
+        std::cout << "embedded_sidecar_count=" << written_sidecars.size() << "\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) {

include/engine/framework/assets/tensor_source.h

@@ -196,6 +196,13 @@ void convert_tensor_sources_to_gguf(const std::vector<TensorSourceInput> & 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. 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<std::string> gguf_embedded_sidecar_names(const std::filesystem::path & path);
 [[nodiscard]] std::optional<GgufEmbeddedModelSpec> read_gguf_embedded_model_spec(const std::filesystem::path & path);
 [[nodiscard]] std::filesystem::path materialize_gguf_sidecars(const std::filesystem::path & path);
 struct PreparedModelDirectory {

src/framework/assets/tensor_source.cpp

@@ -1542,8 +1542,14 @@ std::shared_ptr<const TensorSource> 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<std::pair<std::string, std::string>> 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<std::pair<std::string, std::string>> read_gguf_embedded_sidecars(
                 }
                 std::string content;
                 const size_t length = static_cast<size_t>(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,7 +1632,15 @@ std::vector<std::pair<std::string, std::string>> 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<std::string> gguf_embedded_sidecar_names(const std::filesystem::path & path) {
+    const auto sidecars = read_gguf_embedded_sidecars(path, false);
+    std::vector<std::string> names;
+    names.reserve(sidecars.size());
+    for (const auto & sidecar : sidecars) names.push_back(sidecar.first);
+    return names;
 }
 
 std::optional<GgufEmbeddedModelSpec> read_gguf_embedded_model_spec(const std::filesystem::path & path) {
@@ -1674,7 +1690,7 @@ std::optional<GgufEmbeddedModelSpec> 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());
     }

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 <cstring>
@@ -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<float>{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;