From 1819737f4e9d7e23207dd197e624f5d3a9a0196e Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 27 Jul 2026 01:55:31 +0000 Subject: [PATCH] backend(audio-cpp): correct the vevo2 clause, and assert the absences Review found a false clause in the AudioToAudioStream refusal. It said s2s is "offline voice conversion ... which converts one clip into another speaker's voice", which is true of miocodec and false of vevo2: vevo2's s2s route is `editing` and only `editing` (default_route_for_task and route_matches_task in src/models/vevo2/session.cpp), documented as "Edit source speech into new target text while using the target voice" and requiring --target-text, so it rewrites what was said. vevo2's voice conversion is its separate vc task. It now reads "offline clip-to-clip processing against a target voice, declared only by miocodec (voice conversion) and vevo2 (speech editing)", and a test asserts the miscast cannot come back. The conclusion is unchanged: neither family converses. That defect was undetectable on the wire, since vevo2 does not load here, which is the argument for upstream_absence_ctest.cpp. It links engine_runtime purely to interrogate make_default_registry() and asserts the five premises the refusal reasons rest on: no codec task kind, no family advertising spk, no streaming for sep/vc/svc/s2s, miocodec advertising exactly vc and s2s, and s2s advertised by exactly miocodec and vevo2. The last two are exact sets, so an addition fails here rather than leaving a message stale. A positive control proves the registry is populated and the query works before any absence is believed, and every assertion has a reproduced negative control. This turns an AUDIO_CPP_VERSION bump from "remember to re-read five prose paragraphs" into a test failure. unsupported_surface now switches over UnsupportedRpc with no default label, so -Wswitch reports a sixth enumerator added without a row at build time; the runtime bounds guard it replaces is deleted. The AudioTransformStream reason had a true premise and an overreaching conclusion: an offline sep family could be buffered into a stream, as other LocalAI backends do. It now says this backend declines to offer a buffered offline call in disguise, rather than implying impossibility. Assisted-by: Claude:claude-opus-5 [Claude Code] Signed-off-by: Ettore Di Giacinto --- backend/cpp/audio-cpp/CMakeLists.txt | 21 ++ backend/cpp/audio-cpp/capability_routing.cpp | 67 +++-- .../cpp/audio-cpp/capability_routing_test.cpp | 50 ++-- .../cpp/audio-cpp/upstream_absence_ctest.cpp | 283 ++++++++++++++++++ 4 files changed, 372 insertions(+), 49 deletions(-) create mode 100644 backend/cpp/audio-cpp/upstream_absence_ctest.cpp diff --git a/backend/cpp/audio-cpp/CMakeLists.txt b/backend/cpp/audio-cpp/CMakeLists.txt index 455d22ddf..75161a475 100644 --- a/backend/cpp/audio-cpp/CMakeLists.txt +++ b/backend/cpp/audio-cpp/CMakeLists.txt @@ -278,4 +278,25 @@ if(AUDIO_CPP_GRPC_BUILD_TESTS) # a test binary is harmless, since package.sh ships only grpc-server, and # forcing "$ORIGIN" here means ctest cannot start the binary at all. add_test(NAME streaming_driver COMMAND streaming_driver_ctest) + + # Asserts that the upstream ABSENCES capability_routing.cpp's refusal + # messages rest on are still absences, by querying make_default_registry() + # rather than by re-reading upstream. This is what makes an AUDIO_CPP_VERSION + # bump that adds a codec task kind, an spk family or a streaming converter + # fail the build instead of leaving a false statement on the wire. + # + # It links engine_runtime purely to run that query, which is why it lives + # here rather than with the standalone *_test.cpp files, and it needs no + # "$ORIGIN" rpath override for the same reason streaming_driver_ctest does + # not: see the note above. + add_executable(upstream_absence_ctest upstream_absence_ctest.cpp) + target_include_directories(upstream_absence_ctest PRIVATE + "${AUDIO_CPP_DIR}/include" + "${CMAKE_CURRENT_SOURCE_DIR}") + target_link_libraries(upstream_absence_ctest PRIVATE + engine_runtime + ggml + Threads::Threads) + target_compile_options(upstream_absence_ctest PRIVATE -Wall -Wextra -Wpedantic) + add_test(NAME upstream_absence COMMAND upstream_absence_ctest) endif() diff --git a/backend/cpp/audio-cpp/capability_routing.cpp b/backend/cpp/audio-cpp/capability_routing.cpp index 803eeff00..198a5ff4d 100644 --- a/backend/cpp/audio-cpp/capability_routing.cpp +++ b/backend/cpp/audio-cpp/capability_routing.cpp @@ -132,17 +132,34 @@ const char *const kCodecReason = // planned to say and is false: silero_vad advertises vad with RunMode::Streaming // (src/models/silero_vad/session.cpp). The claim that actually holds is the // narrower one below, about the four tasks AudioTransform routes to. +// The trailing clause is not padding. The premise is an absence, and an absence +// does not on its own make the RPC impossible: an offline sep family could be +// buffered and emitted as a stream, which is what several LocalAI backends do. +// Stopping at "nothing advertises streaming" would imply an impossibility the +// evidence does not support. What is true, and what the caller needs, is that +// this backend declines to dress an offline call up as a streaming one. const char *const kTransformStreamReason = "no audio.cpp family advertises streaming for any task AudioTransform routes " "to (sep, vc, svc, s2s); upstream advertises RunMode::Streaming for tts, asr " "and vad only, and no conversion or separation family even implements its " - "IStreamingVoiceTaskSession interface"; + "IStreamingVoiceTaskSession interface, so a streaming transform here would be " + "a buffered offline call in disguise, which this backend does not pretend to " + "offer"; +// "clip-to-clip processing against a target voice", NOT "voice conversion". The +// latter is true of miocodec and FALSE of vevo2, whose s2s route is `editing` +// and only `editing`: src/models/vevo2/session.cpp's default_route_for_task maps +// SpeechToSpeech to Editing and route_matches_task accepts nothing else, and +// docs/models/vevo2.md defines that route as "Edit source speech into new target +// text while using the target voice", requiring --target-text. It rewrites what +// was said. vevo2's actual voice conversion is its separate vc task, which is +// why upstream's README tags the family "TTS, Music, VC, Edit". The conclusion +// is unaffected: neither family converses. const char *const kAudioToAudioReason = "LocalAI's contract here is OpenAI-Realtime shaped, an audio conversation " "emitting audio, transcript and tool-call deltas from a system prompt and a " - "tool list; audio.cpp's s2s is offline voice conversion, declared only by " - "miocodec and vevo2, which converts one clip into another speaker's voice " - "and is a different thing"; + "tool list; audio.cpp's s2s is offline clip-to-clip processing against a " + "target voice, declared only by miocodec (voice conversion) and vevo2 " + "(speech editing), with no conversation, system prompt or tool loop"; const char *const kVoiceEmbedReason = "no audio.cpp family advertises the spk (SpeakerRecognition) task, so " "nothing in the engine can produce a speaker embedding; the task kind " @@ -239,8 +256,9 @@ std::string describe_capabilities(const Capabilities &caps) { } const std::vector &unsupported_surfaces() { - // Ordered as UnsupportedRpc declares them, which unsupported_surface() - // relies on to index straight in. + // Ordered as UnsupportedRpc declares them. unsupported_surface() names each + // index in a switch rather than casting the enum, so the order is checked at + // compile time rather than trusted. static const std::vector kSurfaces = { {"AudioEncode", kCodecReason}, {"AudioDecode", kCodecReason}, @@ -251,24 +269,31 @@ const std::vector &unsupported_surfaces() { return kSurfaces; } +// A switch with NO default label, deliberately. -Wswitch is on under -Wall, so a +// sixth UnsupportedRpc added without a case here is a BUILD diagnostic, which is +// the only place this class of mistake can be caught for free: a positional +// static_cast(rpc) would compile fine and read past the end of the table +// at run time, on the one code path whose entire job is to be diagnosable. The +// table stays a table because the tests iterate it. +// +// The trailing return is unreachable through the enum and exists only for a +// caller that hands over a value outside it, which is already undefined +// behaviour by the time it arrives. const UnsupportedSurface &unsupported_surface(UnsupportedRpc rpc) { const std::vector &surfaces = unsupported_surfaces(); - const size_t index = static_cast(rpc); - // The binding between the enum and the table is positional, so a sixth - // enumerator added without a row indexes past the end. operator[] would make - // that undefined behaviour, i.e. a crash or a garbage string on the wire, in - // the one code path whose entire job is to be diagnosable. This turns it - // into a message that says what happened. It is not reachable today, and the - // test that would notice it going stale is in capability_routing_test.cpp. - if (index >= surfaces.size()) { - static const UnsupportedSurface kMissingRow{ - "unknown", - "this backend declared an UnsupportedRpc with no matching row in " - "unsupported_surfaces(), which is a bug in capability_routing.cpp " - "and not a statement about audio.cpp"}; - return kMissingRow; + switch (rpc) { + case UnsupportedRpc::AudioEncode: + return surfaces[0]; + case UnsupportedRpc::AudioDecode: + return surfaces[1]; + case UnsupportedRpc::AudioTransformStream: + return surfaces[2]; + case UnsupportedRpc::AudioToAudioStream: + return surfaces[3]; + case UnsupportedRpc::VoiceEmbed: + return surfaces[4]; } - return surfaces[index]; + return surfaces[0]; } std::string unsupported_surface_message(const Capabilities &caps, const char *rpc, diff --git a/backend/cpp/audio-cpp/capability_routing_test.cpp b/backend/cpp/audio-cpp/capability_routing_test.cpp index 8569d0536..303d70dd8 100644 --- a/backend/cpp/audio-cpp/capability_routing_test.cpp +++ b/backend/cpp/audio-cpp/capability_routing_test.cpp @@ -342,27 +342,9 @@ static void test_unsupported_surface_table_matches_the_enum() { } } -// An enumerator with no table row must not read past the end. Casting an -// out-of-range value into a scoped enum whose underlying type is int is -// well-defined, so this test itself is legal; what it exercises is the guard, -// which is the only thing standing between a future sixth enumerator and -// undefined behaviour on the one path whose job is to be diagnosable. -static void test_unsupported_surface_out_of_range_is_diagnosable() { - const auto beyond = - static_cast(unsupported_surfaces().size()); - const auto &surface = unsupported_surface(beyond); - check(std::string(surface.rpc) == "unknown", - "an enumerator with no table row does not read past the end"); - check(std::string(surface.reason).find("bug in capability_routing.cpp") != - std::string::npos, - "the missing-row reason blames this backend, not audio.cpp"); - // And it still formats, rather than dereferencing something that is not - // there. - const std::string message = - unsupported_surface_message(nemotron(), surface.rpc, surface.reason); - check(message.find("nemotron_asr") != std::string::npos, - "the missing-row surface still produces a whole message"); -} +// There is no out-of-range test for unsupported_surface(). It switches over the +// enumerators with no default label, so a sixth UnsupportedRpc without a case is +// a -Wswitch diagnostic at build time and cannot reach a run-time check at all. // Every entry, not just the one somebody remembered to cover. A refusal that // drops the family, the RPC or the reason is a refusal the caller cannot act @@ -434,16 +416,29 @@ static void test_unsupported_reasons_name_the_upstream_limitation() { check(std::string(transform.reason).find("tts and asr only") == std::string::npos, "the AudioTransformStream reason does not repeat the refuted claim"); + // An absence is not an impossibility. A sep family could be buffered and + // emitted as a stream, so the reason has to say this backend declines to + // rather than cannot, or it overreaches on a true premise. + check(std::string(transform.reason).find("buffered offline call in disguise") != + std::string::npos, + "the AudioTransformStream reason does not overclaim impossibility"); const auto &s2s = unsupported_surface(UnsupportedRpc::AudioToAudioStream); check(std::string(s2s.reason).find("Realtime") != std::string::npos && - std::string(s2s.reason).find("voice conversion") != std::string::npos, + std::string(s2s.reason).find("clip-to-clip") != std::string::npos, "the AudioToAudioStream reason contrasts the two contracts"); - // Naming both s2s families makes the claim checkable: a reader can open - // those two loaders and see offline voice conversion rather than a - // conversation. - check(std::string(s2s.reason).find("miocodec and vevo2") != std::string::npos, - "the AudioToAudioStream reason names the only two s2s families"); + // Naming both s2s families, and what each of them actually does, makes the + // claim checkable. It must NOT say s2s is voice conversion full stop: that + // is true of miocodec and false of vevo2, whose s2s route is `editing` and + // rewrites the spoken content against a target voice. + check(std::string(s2s.reason).find("miocodec (voice conversion)") != + std::string::npos && + std::string(s2s.reason).find("vevo2 (speech editing)") != + std::string::npos, + "the AudioToAudioStream reason names each s2s family's actual task"); + check(std::string(s2s.reason).find("s2s is offline voice conversion") == + std::string::npos, + "the AudioToAudioStream reason does not miscast vevo2 as conversion"); const auto &embed = unsupported_surface(UnsupportedRpc::VoiceEmbed); check(std::string(embed.reason).find("spk") != std::string::npos, @@ -495,7 +490,6 @@ int main() { test_describe_capabilities(); test_empty_capabilities(); test_unsupported_surface_table_matches_the_enum(); - test_unsupported_surface_out_of_range_is_diagnosable(); test_every_unsupported_surface_message_is_diagnosable(); test_unsupported_reasons_name_the_upstream_limitation(); test_unsupported_surface_message_with_empty_capabilities(); diff --git a/backend/cpp/audio-cpp/upstream_absence_ctest.cpp b/backend/cpp/audio-cpp/upstream_absence_ctest.cpp new file mode 100644 index 000000000..51ddb3b06 --- /dev/null +++ b/backend/cpp/audio-cpp/upstream_absence_ctest.cpp @@ -0,0 +1,283 @@ +// Asserts the ABSENCES that capability_routing.cpp's five refusal reasons rest +// on, against the engine itself rather than against somebody's reading of it. +// +// Those reasons are prose making checkable claims about a pinned third-party +// checkout: "VoiceTaskKind has no codec entry", "no family advertises spk", +// "miocodec advertises only vc and s2s". Prose rots silently across an +// AUDIO_CPP_VERSION bump, and it rots in the worst possible place, since a +// refusal that states a false fact is worse than a bare UNIMPLEMENTED: it will +// be believed. One of the four claims this backend was planned against was +// already false when it was written ("streaming exists for tts and asr only" is +// contradicted by silero_vad). This test is what turns the next such change +// from a silent lie on the wire into a build failure. +// +// Pinned at audio.cpp e800d435d130dc776baf6f3e6129bb62b1495c89. What follows +// was true of that commit; a bump is exactly when it needs to be re-run. +// +// It links engine_runtime and queries make_default_registry(), touching only +// include/engine/framework/**, like every other unit in this backend. It loads +// no model and reads no file: advertise_loaders() is the path-free catalog +// upstream publishes for --list-loaders. +// +// FOUR CAVEATS, so nobody reads more into a green run than it earns: +// +// 1. It cannot assert the AudioToAudioStream reason. That one contrasts +// LocalAI's OpenAI-Realtime contract (conversation, system prompt, tool +// loop) with what audio.cpp's s2s families actually do, and semantics are +// not a queryable property. What IS asserted is the enumerable half: that +// s2s is advertised by exactly miocodec and vevo2, which is the clause the +// message names by hand. +// 2. It queries the LOADER catalog, while grpc-server.cpp reads the LOADED +// model's own capabilities(). The two agree today, cross-checked on the +// wire: this test asserts miocodec advertises {vc/offline, s2s/offline}, +// and a live LoadModel of miocodec-q8_0.gguf reports exactly +// "vc/offline, s2s/offline". A family whose loaded capabilities diverged +// from its advertisement would slip past, but nothing loads without a +// model file and a ctest cannot depend on one. +// 3. Capabilities need not come from a loader at all. +// src/framework/model_spec/metadata.cpp's advertised_capabilities() builds +// a CapabilitySet from a spec's "capabilities"/"tasks"/"modes" keys, which +// is a second route by which a bump could falsify claim 3. Today no +// shipped model_specs/*.json carries a top-level "tasks" key and no loader +// calls that function, so the route is dead. It is covered anyway to the +// extent that a loader adopting it would surface through advertise_loaders +// like any other capability, which is why every assertion below queries +// advertised capabilities rather than loader source. +// 4. An absence test passes trivially when the query is broken, so +// test_catalog_is_populated below is a POSITIVE control and is not +// optional. It proves the registry is non-empty and that the query does +// find a capability it should, before any absence is believed. + +#include "engine/framework/runtime/registry.h" +#include "engine/framework/runtime/session.h" + +#include +#include +#include +#include +#include +#include + +namespace { + +int failures = 0; + +void check(bool ok, const std::string &name) { + if (!ok) { + failures++; + fprintf(stderr, "FAIL: %s\n", name.c_str()); + } else { + fprintf(stderr, "ok: %s\n", name.c_str()); + } +} + +using engine::runtime::LoaderAdvertisement; +using engine::runtime::RunMode; +using engine::runtime::VoiceTaskKind; + +// Built once. make_default_registry constructs every loader, which is the +// expensive part, and none of the assertions mutates it. +const std::vector &catalog() { + static const std::vector kCatalog = + engine::runtime::make_default_registry().advertise_loaders(); + return kCatalog; +} + +bool advertises(const LoaderAdvertisement &loader, VoiceTaskKind task, + RunMode mode) { + for (const auto &capability : loader.capabilities.supported_tasks) { + if (capability.task != task) { + continue; + } + return std::find(capability.modes.begin(), capability.modes.end(), mode) != + capability.modes.end(); + } + return false; +} + +bool advertises_any_mode(const LoaderAdvertisement &loader, VoiceTaskKind task) { + return advertises(loader, task, RunMode::Offline) || + advertises(loader, task, RunMode::Streaming); +} + +std::string describe(const LoaderAdvertisement &loader) { + std::string out; + for (const auto &capability : loader.capabilities.supported_tasks) { + for (const RunMode mode : capability.modes) { + if (!out.empty()) { + out += ", "; + } + out += engine::runtime::to_string(capability.task); + out += "/"; + out += engine::runtime::to_string(mode); + } + } + return out.empty() ? "nothing" : out; +} + +// THE POSITIVE CONTROL. Every other test here asserts an absence, and an +// absence is what a broken query returns for everything. If make_default_registry +// ever returns an empty registry, or advertise_loaders stops populating modes, +// this is the test that fails instead of the suite going quietly green while +// asserting nothing. +void test_catalog_is_populated() { + check(catalog().size() >= 20, + "the default registry advertises a plausible number of families (" + + std::to_string(catalog().size()) + ")"); + + bool found_streaming_asr = false; + for (const auto &loader : catalog()) { + if (advertises(loader, VoiceTaskKind::Asr, RunMode::Streaming)) { + found_streaming_asr = true; + break; + } + } + check(found_streaming_asr, + "the query finds a capability that IS advertised (asr/streaming)"); +} + +// The AudioEncode and AudioDecode premise. Not "no family does codec" but the +// stronger "the task kind does not exist", which is what makes those two RPCs +// unroutable rather than merely unserved. +// +// Note what is NOT asserted: model_spec/schema.cpp's task whitelist DOES accept +// the string "codec", so a spec declaring it validates and then fails here. That +// is a hole in upstream's own validation, and it is deliberately kept off the +// wire; the refusal rests on this parser, which is the thing routing would have +// to go through. +void test_no_codec_task_kind() { + bool threw = false; + try { + (void)engine::runtime::parse_voice_task_kind("codec"); + } catch (const std::exception &) { + threw = true; + } + check(threw, "no codec task kind: parse_voice_task_kind(\"codec\") throws"); + + // The control for the line above: a real name must NOT throw, or the test + // would pass against a parser that rejected everything. + bool tts_threw = false; + try { + (void)engine::runtime::parse_voice_task_kind("tts"); + } catch (const std::exception &) { + tts_threw = true; + } + check(!tts_threw, "parse_voice_task_kind accepts a real name (\"tts\")"); +} + +// The VoiceEmbed premise. SpeakerRecognition IS in the enum, and TitaNet and +// ECAPA-TDNN exist as internal conditioning encoders; what is missing is any +// registered family advertising the task, which is what the message says. +void test_no_family_advertises_speaker_recognition() { + std::string offenders; + for (const auto &loader : catalog()) { + if (advertises_any_mode(loader, VoiceTaskKind::SpeakerRecognition)) { + if (!offenders.empty()) { + offenders += ", "; + } + offenders += loader.family; + } + } + check(offenders.empty(), + "no family advertises spk" + + (offenders.empty() ? std::string() : " (found: " + offenders + ")")); +} + +// The AudioTransformStream premise, and the one that has to be scoped exactly. +// The broad claim "upstream streams tts and asr only" is FALSE: silero_vad +// advertises vad/streaming. The claim that holds is that none of the four tasks +// AudioTransform routes to is advertised streaming by anybody. +// +// NEGATIVE CONTROL: add VoiceTaskKind::Tts to kTransformTasks and this test must +// fail, naming the tts families. A run where that edit stays green means the +// query is broken and every absence above is worthless. +void test_no_streaming_for_the_transform_tasks() { + const VoiceTaskKind kTransformTasks[] = { + VoiceTaskKind::SourceSeparation, + VoiceTaskKind::VoiceConversion, + VoiceTaskKind::Svc, + VoiceTaskKind::SpeechToSpeech, + }; + + std::string offenders; + for (const VoiceTaskKind task : kTransformTasks) { + for (const auto &loader : catalog()) { + if (advertises(loader, task, RunMode::Streaming)) { + if (!offenders.empty()) { + offenders += ", "; + } + offenders += loader.family; + offenders += "/"; + offenders += engine::runtime::to_string(task); + } + } + } + check(offenders.empty(), + "no streaming for sep/vc/svc/s2s" + + (offenders.empty() ? std::string() : " (found: " + offenders + ")")); +} + +// The clause the AudioEncode message names by hand: miocodec carries a Codec tag +// in upstream's README, and its loader advertises only vc and s2s. Asserted +// exactly, not as a subset, so a bump that ADDS a codec capability to miocodec +// fails here rather than leaving the message stale. +void test_miocodec_advertises_exactly_vc_and_s2s() { + const LoaderAdvertisement *miocodec = nullptr; + for (const auto &loader : catalog()) { + if (loader.family == "miocodec") { + miocodec = &loader; + break; + } + } + if (miocodec == nullptr) { + check(false, "miocodec is a registered family"); + return; + } + check(describe(*miocodec) == "vc/offline, s2s/offline", + "miocodec advertises exactly vc/offline, s2s/offline (got: " + + describe(*miocodec) + ")"); +} + +// The "declared only by" clause in the AudioToAudioStream message. Exact set, +// for the same reason as above: a third s2s family would make the message stale +// without making it obviously wrong. +void test_speech_to_speech_is_exactly_miocodec_and_vevo2() { + std::set families; + for (const auto &loader : catalog()) { + if (advertises_any_mode(loader, VoiceTaskKind::SpeechToSpeech)) { + families.insert(loader.family); + } + } + const std::set expected = {"miocodec", "vevo2"}; + std::string found; + for (const auto &family : families) { + if (!found.empty()) { + found += ", "; + } + found += family; + } + check(families == expected, + "s2s is advertised by exactly miocodec and vevo2 (got: " + + (found.empty() ? "nothing" : found) + ")"); +} + +} // namespace + +int main() { + test_catalog_is_populated(); + test_no_codec_task_kind(); + test_no_family_advertises_speaker_recognition(); + test_no_streaming_for_the_transform_tasks(); + test_miocodec_advertises_exactly_vc_and_s2s(); + test_speech_to_speech_is_exactly_miocodec_and_vevo2(); + if (failures) { + fprintf(stderr, + "%d upstream absence check(s) failed. A refusal message in " + "capability_routing.cpp now states something that is not true " + "of the pinned audio.cpp; fix the message, not this test.\n", + failures); + return 1; + } + fprintf(stderr, "all upstream absence checks passed\n"); + return 0; +}