chat: refactor handling supports_string_content / supports_typed_content (#27130)

* better supports_string_content cap detect

* test: add "skip"

* messages_inp_normalizer

common/chat.cpp

@@ -470,36 +470,80 @@ std::vector<common_chat_msg> common_chat_msgs_parse_oaicompat(const json & messa
     return msgs;
 }
 
+struct messages_inp_normalizer {
+    const jinja::caps & caps;
+
+    messages_inp_normalizer(const jinja::caps & c) : caps(c) {}
+
+    // handle supports_string_content / supports_typed_content
+    // if string=true and array=false, convert array to string
+    // if string=false and array=true, convert string to array
+    // if both are true, do nothing
+    json normalize(const json & messages) {
+        bool only_string = caps.supports_string_content && !caps.supports_typed_content;
+        bool only_typed  = !caps.supports_string_content && caps.supports_typed_content;
+        if ((!only_string && !only_typed) || !messages.is_array()) {
+            return messages;
+        }
+        json normalized = json::array();
+        for (const auto & msg : messages) {
+            json copy = msg;
+            auto it = copy.find("content");
+            if (it != copy.end()) {
+                if (only_typed && it->is_string()) {
+                    *it = json::array({
+                        json{
+                            {"type", "text"},
+                            {"text", it->get<std::string>()},
+                        }
+                    });
+                } else if (only_string && it->is_array()) {
+                    *it = concat_content_parts(*it);
+                }
+            }
+            normalized.push_back(std::move(copy));
+        }
+        return normalized;
+    }
+
+    // join parts with newline, do not add newline before or after media markers
+    static std::string concat_content_parts(const json & parts) {
+        std::string text;
+        bool last_was_media_marker = false;
+        for (const auto & part : parts) {
+            std::string type = part.value("type", "");
+            bool add_new_line = true;
+            if (type == "text") {
+                add_new_line = !last_was_media_marker && !text.empty();
+                last_was_media_marker = false;
+            } else if (type == "media_marker") {
+                add_new_line = false;
+                last_was_media_marker = true;
+            } else {
+                LOG_WRN("Ignoring content part type: %s\n", type.c_str());
+                continue;
+            }
+
+            if (add_new_line) {
+                text += '\n';
+            }
+
+            text += part.value("text", "");
+        }
+        return text;
+    }
+};
+
 static json render_message_to_json(const std::vector<common_chat_msg> & msgs, const jinja::caps & c) {
     if (!c.supports_string_content && !c.supports_typed_content) {
         LOG_WRN("%s: Neither string content nor typed content is supported by the template. This is unexpected and may lead to issues.\n", __func__);
     }
 
-    bool only_string_accepted =  c.supports_string_content && !c.supports_typed_content;
-    bool only_typed_accepted  = !c.supports_string_content &&  c.supports_typed_content;
-
     json messages = json::array();
     for (const auto & msg : msgs) {
-        if (only_string_accepted) {
+        messages.push_back(msg.to_json_oaicompat(/* concat_typed_text= */ false));
-            json jmsg = msg.to_json_oaicompat(/* concat_typed_text= */ true);
-            messages.push_back(jmsg);
-        } else if (only_typed_accepted) {
-            json jmsg = msg.to_json_oaicompat(/* concat_typed_text= */ false);
-            if (jmsg.at("content").is_string()) {
-                jmsg["content"] = json::array({
-                    json{
-                        {"type", "text"},
-                        {"text", jmsg.at("content").get<std::string>()},
-                    }
-                });
-            }
-            messages.push_back(jmsg);
-        } else {
-            json jmsg = msg.to_json_oaicompat(/* concat_typed_text= */ false);
-            messages.push_back(jmsg);
-        }
     }
-    return messages;
+    return messages_inp_normalizer(c).normalize(messages);
 }
 
 // DEPRECATED: only used in tests
@@ -892,8 +936,11 @@ static std::string common_chat_template_direct_apply_impl(
     const std::optional<json> & additional_context = std::nullopt) {
     jinja::context ctx(tmpl.source());
 
+    // messages_override is already built for this template, do not touch its content parts
     nlohmann::ordered_json inp = nlohmann::ordered_json{
-        {"messages", messages_override.has_value() ? *messages_override : inputs.messages},
+        {"messages", messages_override.has_value()
+            ? *messages_override
+            : messages_inp_normalizer(tmpl.original_caps()).normalize(inputs.messages)},
         {"bos_token", tmpl.bos_token()},
         {"eos_token", tmpl.eos_token()},
         {"enable_thinking", inputs.enable_thinking},
@@ -957,14 +1004,12 @@ static std::string common_chat_template_generation_prompt_impl(
     const std::optional<json> & tools_override = std::nullopt,
     const std::optional<json> & additional_context = std::nullopt) {
 
-    auto adjusted_messages = messages_override ? *messages_override : inputs.messages;
-
     autoparser::generation_params params = inputs;
     params.add_generation_prompt = false;
     params.continue_final_message = COMMON_CHAT_CONTINUATION_NONE;
-    std::string no_gen_prompt    = common_chat_template_direct_apply_impl(tmpl, params, adjusted_messages, tools_override, additional_context);
+    std::string no_gen_prompt    = common_chat_template_direct_apply_impl(tmpl, params, messages_override, tools_override, additional_context);
     params.add_generation_prompt = true;
-    std::string gen_prompt       = common_chat_template_direct_apply_impl(tmpl, params, adjusted_messages, tools_override, additional_context);
+    std::string gen_prompt       = common_chat_template_direct_apply_impl(tmpl, params, messages_override, tools_override, additional_context);
 
     size_t prefix_len = 0;
     size_t min_size = std::min(no_gen_prompt.size(), gen_prompt.size());

common/jinja/caps.cpp

@@ -23,7 +23,7 @@ void caps_apply_preserve_reasoning(jinja::context & ctx, bool enabled) {
     ctx.set_val("preserve_thinking",         mk_val<value_bool>(enabled));
     ctx.set_val("clear_thinking",            mk_val<value_bool>(!enabled));
     ctx.set_val("truncate_history_thinking", mk_val<value_bool>(!enabled));
-    ctx.set_val("drop_thinking",            mk_val<value_bool>(!enabled));
+    ctx.set_val("drop_thinking",             mk_val<value_bool>(!enabled));
 }
 
 void caps_apply_reasoning_effort(jinja::context & ctx, const std::string & effort) {
@@ -117,6 +117,8 @@ caps caps_get(jinja::program & prog) {
 
     JJ_DEBUG("%s\n", ">>> Running capability check: typed content");
 
+    static const std::string content_marker = "STRING_MARKER";
+
     // case: typed content support
     caps_try_execute(
         prog,
@@ -125,22 +127,26 @@ caps caps_get(jinja::program & prog) {
             return json::array({
                 {
                     {"role", "user"},
-                    {"content", "content"}
+                    {"content", content_marker}
                 }
             });
         },
         nullptr, // ctx_fn
         nullptr, // tools_fn
-        [&](context &, bool success, value & messages, value &, const std::string &) {
+        [&](context &, bool success, value & messages, value &, const std::string & rendered) {
             auto & content = messages->at(0)->at("content");
             caps_print_stats(content, "messages[0].content");
-            if (has_op(content, "selectattr") || has_op(content, "array_access")) {
+            bool used_as_array = has_op(content, "selectattr") || has_op(content, "array_access");
+            if (used_as_array) {
                 // accessed as an array
                 result.supports_typed_content = true;
             }
             if (!success) {
                 // failed to execute with content as string
                 result.supports_string_content = false;
+            } else if (used_as_array && rendered.find(content_marker) == std::string::npos) {
+                // edge case: string may be accessed for checking, but does not appear in the output
+                result.supports_string_content = false;
             }
         }
     );

tests/test-jinja.cpp

@@ -10,6 +10,7 @@
 #include "jinja/parser.h"
 #include "jinja/lexer.h"
 #include "jinja/utils.h"
+#include "jinja/caps.h"
 
 #include "testing.h"
 
@@ -33,6 +34,7 @@ static void test_array_methods(testing & t);
 static void test_object_methods(testing & t);
 static void test_hasher(testing & t);
 static void test_stats(testing & t);
+static void test_caps(testing & t);
 static void test_string_parts(testing & t);
 static void test_fuzzing(testing & t);
 
@@ -73,6 +75,7 @@ int main(int argc, char *argv[]) {
     if (!g_python_mode) {
         t.test("hasher", test_hasher);
         t.test("stats", test_stats);
+        t.test("caps", test_caps);
         t.test("string parts", test_string_parts);
         t.test("fuzzing", test_fuzzing);
     }
@@ -2059,6 +2062,51 @@ static void test_stats(testing & t) {
     });
 }
 
+static void test_caps(testing & t) {
+    static auto get_caps = [](const std::string & tmpl) -> jinja::caps {
+        jinja::lexer lexer;
+        auto lexer_res = lexer.tokenize(tmpl);
+
+        jinja::program prog = jinja::parse_from_tokens(lexer_res);
+
+        return jinja::caps_get(prog);
+    };
+
+    t.test("string content", [](testing & t) {
+        auto caps = get_caps(
+            "{% for message in messages %}"
+            "{{ message['role'] + ': ' + message['content'] }}"
+            "{% endfor %}"
+        );
+        t.assert_true("supports string content", caps.supports_string_content);
+        t.assert_true("does not support typed content", !caps.supports_typed_content);
+    });
+
+    t.test("typed content, raises on string", [](testing & t) {
+        // 'selectattr' is not a String filter, so it throws
+        auto caps = get_caps(
+            "{% for message in messages %}"
+            "{% for content in message['content'] | selectattr('type', 'equalto', 'text') %}"
+            "{{ content['text'] }}"
+            "{% endfor %}"
+            "{% endfor %}"
+        );
+        t.assert_true("does not support string content", !caps.supports_string_content);
+        t.assert_true("supports typed content", caps.supports_typed_content);
+    });
+
+    t.test("typed content, silently drops string", [](testing & t) {
+        // no throw here, but content[0]['text'] is undefined for a string (MiniMax-M1 case)
+        auto caps = get_caps(
+            "{% for message in messages %}"
+            "{{ message['content'][0]['text'] }}"
+            "{% endfor %}"
+        );
+        t.assert_true("does not support string content", !caps.supports_string_content);
+        t.assert_true("supports typed content", caps.supports_typed_content);
+    });
+}
+
 static void test_string_parts(testing & t) {
     static auto render = [](const std::string & tmpl, const json & vars) -> jinja::string {
         jinja::lexer lexer;
@@ -2116,8 +2164,7 @@ static void test_template_cpp(testing & t, const std::string & name, const std::
                 t.log("Actual  : " + json(rendered).dump());
             }
         } catch (const jinja::not_implemented_exception & e) {
-            // TODO @ngxson : remove this when the test framework supports skipping tests
+            t.skip(e.what());
-            t.log("Skipped: " + std::string(e.what()));
         }
     });
 }

tests/testing.h

@@ -21,6 +21,11 @@ struct testing {
     int failures = 0;
     int unnamed = 0;
     int exceptions = 0;
+    int skipped = 0;
+
+    // set by skip(), read by the innermost test()
+    bool skip_current = false;
+    std::string skip_reason;
 
     static constexpr std::size_t status_column = 80;
 
@@ -78,7 +83,12 @@ struct testing {
         }
     }
 
-    void print_result(const std::string &label, int new_failures, int new_assertions, const std::string &extra = "") const {
+    void skip(const std::string &reason = "") {
+        skip_current = true;
+        skip_reason  = reason;
+    }
+
+    void print_result(const std::string &label, int new_failures, int new_assertions, const std::string &extra = "", bool was_skipped = false) const {
         std::string line = indent() + label;
 
         std::string details;
@@ -101,7 +111,7 @@ struct testing {
             line += " (" + details + ")";
         }
 
-        std::string status = (new_failures == 0) ? "[PASS]" : "[FAIL]";
+        std::string status = new_failures != 0 ? "[FAIL]" : (was_skipped ? "[SKIP]" : "[PASS]");
 
         if (line.size() + 1 < status_column) {
             line.append(status_column - line.size(), ' ');
@@ -126,12 +136,26 @@ struct testing {
         int before_failures   = failures;
         int before_assertions = assertions;
 
+        // do not let a skipped subtest also mark its parent as skipped
+        bool        outer_skip        = skip_current;
+        std::string outer_skip_reason = skip_reason;
+        skip_current = false;
+        skip_reason.clear();
+
         run_with_exceptions([&] { f(*this); }, "test");
 
         int new_failures   = failures   - before_failures;
         int new_assertions = assertions - before_assertions;
 
-        print_result(name, new_failures, new_assertions);
+        bool was_skipped = skip_current && new_failures == 0;
+        if (was_skipped) {
+            ++skipped;
+        }
+
+        print_result(name, new_failures, new_assertions, was_skipped ? skip_reason : "", was_skipped);
+
+        skip_current = outer_skip;
+        skip_reason  = outer_skip_reason;
 
         stack.pop_back();
     }
@@ -238,6 +262,7 @@ struct testing {
         out << "assertions : " << assertions << "\n";
         out << "failures   : " << failures << "\n";
         out << "exceptions : " << exceptions << "\n";
+        out << "skipped    : " << skipped << "\n";
         return failures == 0 ? 0 : 1;
     }
 };