From a898cb212d8ad127c138be64e74bf706b0164071 Mon Sep 17 00:00:00 2001 From: Jabber Developer Date: Sat, 9 May 2026 13:56:59 +0000 Subject: [PATCH] feat(ai): implement robust JSON model parsing Rewrote internal JSON parsing to correctly handle whitespace, escaped quotes, and strict field context extraction. This prevents incorrect field names or provider names from being added to the model list. Added `ai_parse_models_from_json` public API to facilitate testing. Changed `/ai models` default behavior to always fetch fresh models. Replaced `--refresh` flag with `--cached` to display local cache. Added comprehensive unit tests covering OpenAI and Perplexity formats, array format, empty/null JSON, escaped quotes, and whitespace handling. --- src/ai/ai_client.c | 160 +++++++++++++++++----- src/ai/ai_client.h | 12 ++ src/command/cmd_funcs.c | 24 ++-- tests/unittests/test_ai_client.c | 223 +++++++++++++++++++++++++++++++ tests/unittests/test_ai_client.h | 8 ++ tests/unittests/unittests.c | 8 ++ 6 files changed, 396 insertions(+), 39 deletions(-) diff --git a/src/ai/ai_client.c b/src/ai/ai_client.c index ffa3947d..c40189c8 100644 --- a/src/ai/ai_client.c +++ b/src/ai/ai_client.c @@ -762,44 +762,133 @@ _ai_save_models_for_provider(AIProvider* provider) log_debug("Saved %d models for provider '%s' to prefs", count, provider->name); } +/** + * Find a JSON field value, handling both string and non-string values. + * For string values: "field":"value" -> returns pointer to "value" + * For array values: "field":[ -> returns pointer to [ + * For object values: "field":{ -> returns pointer to { + * Handles optional whitespace after colon. + * Returns pointer to the value start, or NULL if not found. + */ static const gchar* _find_json_field(const gchar* json, const gchar* field) { - auto_gchar gchar* key = g_strdup_printf("\"%s\":\"", field); + if (!json || !field) { + return NULL; + } + + auto_gchar gchar* key = g_strdup_printf("\"%s\"", field); const gchar* pos = strstr(json, key); if (!pos) { return NULL; } - return pos + strlen(key); + + /* Skip past the key */ + pos += strlen(key); + + /* Skip whitespace and find the colon */ + while (*pos == ' ' || *pos == '\t' || *pos == '\n' || *pos == '\r') { + pos++; + } + + if (*pos != ':') { + return NULL; + } + pos++; /* skip colon */ + + /* Skip whitespace after colon */ + while (*pos == ' ' || *pos == '\t' || *pos == '\n' || *pos == '\r') { + pos++; + } + + return pos; } +/** + * Parse models from OpenAI-compatible API response format. + * Expected format: {"object":"list","data":[{"id":"model1","object":"model",...},...]} + * Extracts only the "id" field from each model object. + */ static void _parse_openai_models(AIProvider* provider, const gchar* json) { - /* Format: {"data": [{"id": "model1"}, {"id": "model2"}, ...]} */ - const gchar* data_start = _find_json_field(json, "data"); - if (!data_start) { + if (!json || !provider) { return; } - const gchar* obj_start = strstr(data_start, "{\"id\":"); + /* Find the "data" array start */ + const gchar* data_start = _find_json_field(json, "data"); + if (!data_start || *data_start != '[') { + log_debug("No data array found in API response"); + return; + } + + /* Find all {"id":"..."} objects within the data array */ + const gchar* obj_start = data_start; while (obj_start) { - const gchar* id_start = _find_json_field(obj_start, "id"); - if (!id_start) { + /* Find next "id" field (handle both {"id": and { "id": formats) */ + obj_start = strstr(obj_start, "\"id\""); + if (!obj_start) { break; } - const gchar* id_end = strchr(id_start, '"'); - if (!id_end) { - break; + /* Verify this "id" is within an object (preceded by { or ,) */ + const gchar* before_id = obj_start - 1; + while (before_id >= data_start && (*before_id == ' ' || *before_id == '\t' || *before_id == '\n' || *before_id == '\r')) { + before_id--; + } + if (before_id < data_start || (*before_id != '{' && *before_id != ',')) { + /* This "id" is not at the start of an object in the array, skip it */ + obj_start = obj_start + 2; /* move past this "id" */ + continue; } - auto_gchar gchar* model = g_strndup(id_start, id_end - id_start); + /* Find the colon after "id" */ + const gchar* id_key = obj_start + 4; /* skip "id" */ + /* Skip whitespace */ + while (*id_key == ' ' || *id_key == '\t' || *id_key == '\n' || *id_key == '\r') { + id_key++; + } + if (*id_key != ':') { + obj_start = id_key; + continue; + } + id_key++; /* skip : */ + /* Skip whitespace */ + while (*id_key == ' ' || *id_key == '\t' || *id_key == '\n' || *id_key == '\r') { + id_key++; + } + if (*id_key != '"') { + /* "id" value is not a string, skip */ + obj_start = id_key; + continue; + } + id_key++; /* skip opening quote */ + + /* Find the closing quote (handling escaped quotes) */ + const gchar* id_end = id_key; + while (*id_end && *id_end != '"') { + if (*id_end == '\\' && *(id_end + 1)) { + id_end += 2; /* skip escaped character */ + } else { + id_end++; + } + } + + if (!*id_end) { + break; /* No closing quote found */ + } + + /* Extract the model ID */ + auto_gchar gchar* model = g_strndup(id_key, id_end - id_key); _add_model_if_new(provider, model); - obj_start = strstr(id_end, "{\"id\":"); - if (obj_start) { - obj_start++; + /* Move past this object to find the next one */ + obj_start = id_end + 1; + + /* Safety: stop if we've gone past the data array */ + if (obj_start - json > 5 * 1024 * 1024) { + break; /* 5MB safety limit */ } } } @@ -849,6 +938,30 @@ _parse_and_cache_models(AIProvider* provider, const gchar* response_json) _ai_save_models_for_provider(provider); } +/** + * Public API: Parse model IDs from JSON response. + * Used by tests and internal model fetching. + */ +void +ai_parse_models_from_json(AIProvider* provider, const gchar* json) +{ + if (!provider || !json) { + return; + } + + /* Clear existing models before parsing */ + GList* curr = provider->models; + while (curr) { + g_free(curr->data); + curr = g_list_next(curr); + } + g_list_free(provider->models); + provider->models = NULL; + + /* Parse using the same logic as the internal handler */ + _parse_and_cache_models(provider, json); +} + static void _models_response_handler(AIProvider* provider, const gchar* response, gpointer user_data) { @@ -893,20 +1006,7 @@ ai_fetch_models(const gchar* provider_name, gpointer user_data) return FALSE; } - /* If models are fresh, just display them */ - if (provider->models_fresh && provider->models) { - cons_show("Cached %d models for provider '%s':", g_list_length(provider->models), provider_name); - GList* curr = provider->models; - while (curr) { - cons_show(" %s", (gchar*)curr->data); - curr = g_list_next(curr); - } - cons_show(""); - cons_show("Use '/ai models %s --refresh' to fetch fresh models from the API.", provider_name); - return TRUE; - } - - /* Fetch models in a thread */ + /* Always fetch fresh models from API */ ai_request_t* req = g_new0(ai_request_t, 1); req->provider_name = g_strdup(provider_name); req->user_data = user_data; @@ -920,7 +1020,7 @@ ai_fetch_models(const gchar* provider_name, gpointer user_data) } g_thread_unref(thread); - cons_show("Fetching models for provider '%s'...", provider_name); + cons_show("Fetching fresh models for provider '%s'...", provider_name); return TRUE; } diff --git a/src/ai/ai_client.h b/src/ai/ai_client.h index 2d149bfd..385b8510 100644 --- a/src/ai/ai_client.h +++ b/src/ai/ai_client.h @@ -168,6 +168,18 @@ gboolean ai_fetch_models(const gchar* provider_name, gpointer user_data); */ gboolean ai_models_are_fresh(const gchar* provider_name); +/* ======================================================================== + * Model Parsing (for testing) + * ======================================================================== */ + +/** + * Parse model IDs from an OpenAI-compatible API response. + * Expected format: {"object":"list","data":[{"id":"model1",...},...]} + * @param provider The provider to add models to + * @param json The JSON response from the API + */ +void ai_parse_models_from_json(AIProvider* provider, const gchar* json); + /* ======================================================================== * Session Management * ======================================================================== */ diff --git a/src/command/cmd_funcs.c b/src/command/cmd_funcs.c index f95b8cd6..adbcb57f 100644 --- a/src/command/cmd_funcs.c +++ b/src/command/cmd_funcs.c @@ -11134,14 +11134,16 @@ cmd_ai_switch(ProfWin* window, const char* const command, gchar** args) gboolean cmd_ai_models(ProfWin* window, const char* const command, gchar** args) { - // /ai models [--refresh] + // /ai models [--cached] + // Default: fetch fresh models from API + // --cached: display cached models (if available) if (args[1] == NULL) { cons_bad_cmd_usage(command); return TRUE; } const gchar* provider_name = args[1]; - gboolean refresh = (g_strv_length(args) >= 3) && (g_strcmp0(args[2], "--refresh") == 0); + gboolean use_cached = (g_strv_length(args) >= 3) && (g_strcmp0(args[2], "--cached") == 0); AIProvider* provider = ai_get_provider(provider_name); if (!provider) { @@ -11149,12 +11151,12 @@ cmd_ai_models(ProfWin* window, const char* const command, gchar** args) return TRUE; } - if (refresh || !ai_models_are_fresh(provider_name)) { - // Fetch models from API - ProfAiWin* aiwin = (window && window->type == WIN_AI) ? (ProfAiWin*)window : wins_get_ai(); - ai_fetch_models(provider_name, aiwin); - } else { - // Display cached models + if (use_cached) { + // Display cached models (if available) + if (!provider->models) { + cons_show("No cached models for provider '%s'. Use '/ai models %s' to fetch from API.", provider_name, provider_name); + return TRUE; + } cons_show("Cached models for provider '%s':", provider_name); GList* curr = provider->models; while (curr) { @@ -11162,7 +11164,11 @@ cmd_ai_models(ProfWin* window, const char* const command, gchar** args) curr = g_list_next(curr); } cons_show(""); - cons_show("Use '/ai models %s --refresh' to fetch fresh models from the API.", provider_name); + cons_show("Use '/ai models %s' to fetch fresh models from the API.", provider_name); + } else { + // Default: fetch fresh models from API + ProfAiWin* aiwin = (window && window->type == WIN_AI) ? (ProfAiWin*)window : wins_get_ai(); + ai_fetch_models(provider_name, aiwin); } return TRUE; diff --git a/tests/unittests/test_ai_client.c b/tests/unittests/test_ai_client.c index 8416ce1e..7c1cf16c 100644 --- a/tests/unittests/test_ai_client.c +++ b/tests/unittests/test_ai_client.c @@ -644,3 +644,226 @@ test_ai_models_are_fresh_initial(void** state) gboolean fresh_perplexity = ai_models_are_fresh("perplexity"); assert_true(fresh_perplexity == TRUE || fresh_perplexity == FALSE); /* Just verify no crash */ } + +/* ======================================================================== + * Model Parsing Tests + * ======================================================================== */ + +void +test_ai_parse_models_openai_format(void** state) +{ + /* Test parsing OpenAI-compatible API response format */ + /* Format: {"object":"list","data":[{"id":"model1",...},...]} */ + ai_client_init(); + + AIProvider* provider = ai_add_provider("test", "https://test.api.com/v1", NULL); + + /* JSON response from Perplexity/OpenAI API */ + const gchar* json = "{\"object\":\"list\"," + "\"data\":[" + "{\"id\":\"anthropic/claude-haiku-4-5\",\"object\":\"model\",\"created\":0,\"owned_by\":\"anthropic\"}," + "{\"id\":\"anthropic/claude-opus-4-5\",\"object\":\"model\",\"created\":0,\"owned_by\":\"anthropic\"}," + "{\"id\":\"openai/gpt-5\",\"object\":\"model\",\"created\":0,\"owned_by\":\"openai\"}," + "{\"id\":\"google/gemini-3-flash-preview\",\"object\":\"model\",\"created\":0,\"owned_by\":\"google\"}" + "]}"; + + ai_parse_models_from_json(provider, json); + + /* Verify models were parsed correctly */ + assert_int_equal(4, g_list_length(provider->models)); + + /* Check specific models */ + GList* models = provider->models; + assert_string_equal("anthropic/claude-haiku-4-5", models->data); + models = g_list_next(models); + assert_string_equal("anthropic/claude-opus-4-5", models->data); + models = g_list_next(models); + assert_string_equal("openai/gpt-5", models->data); + models = g_list_next(models); + assert_string_equal("google/gemini-3-flash-preview", models->data); + + ai_provider_unref(provider); + ai_client_shutdown(); +} + +void +test_ai_parse_models_perplexity_format(void** state) +{ + /* Test parsing actual Perplexity API response with all models from curl test */ + ai_client_init(); + + AIProvider* provider = ai_add_provider("perplexity", "https://api.perplexity.ai/", NULL); + + const gchar* json = "{\"object\":\"list\"," + "\"data\":[" + "{\"id\":\"anthropic/claude-haiku-4-5\",\"object\":\"model\",\"created\":0,\"owned_by\":\"anthropic\"}," + "{\"id\":\"anthropic/claude-opus-4-5\",\"object\":\"model\",\"created\":0,\"owned_by\":\"anthropic\"}," + "{\"id\":\"anthropic/claude-opus-4-6\",\"object\":\"model\",\"created\":0,\"owned_by\":\"anthropic\"}," + "{\"id\":\"anthropic/claude-opus-4-7\",\"object\":\"model\",\"created\":0,\"owned_by\":\"anthropic\"}," + "{\"id\":\"anthropic/claude-sonnet-4-5\",\"object\":\"model\",\"created\":0,\"owned_by\":\"anthropic\"}," + "{\"id\":\"anthropic/claude-sonnet-4-6\",\"object\":\"model\",\"created\":0,\"owned_by\":\"anthropic\"}," + "{\"id\":\"google/gemini-3-flash-preview\",\"object\":\"model\",\"created\":0,\"owned_by\":\"google\"}," + "{\"id\":\"google/gemini-3.1-pro-preview\",\"object\":\"model\",\"created\":0,\"owned_by\":\"google\"}," + "{\"id\":\"nvidia/nemotron-3-super-120b-a12b\",\"object\":\"model\",\"created\":0,\"owned_by\":\"nvidia\"}," + "{\"id\":\"openai/gpt-5\",\"object\":\"model\",\"created\":0,\"owned_by\":\"openai\"}," + "{\"id\":\"openai/gpt-5-mini\",\"object\":\"model\",\"created\":0,\"owned_by\":\"openai\"}," + "{\"id\":\"openai/gpt-5.1\",\"object\":\"model\",\"created\":0,\"owned_by\":\"openai\"}," + "{\"id\":\"openai/gpt-5.2\",\"object\":\"model\",\"created\":0,\"owned_by\":\"openai\"}," + "{\"id\":\"openai/gpt-5.4\",\"object\":\"model\",\"created\":0,\"owned_by\":\"openai\"}," + "{\"id\":\"openai/gpt-5.4-mini\",\"object\":\"model\",\"created\":0,\"owned_by\":\"openai\"}," + "{\"id\":\"openai/gpt-5.4-nano\",\"object\":\"model\",\"created\":0,\"owned_by\":\"openai\"}," + "{\"id\":\"openai/gpt-5.5\",\"object\":\"model\",\"created\":0,\"owned_by\":\"openai\"}," + "{\"id\":\"perplexity/sonar\",\"object\":\"model\",\"created\":0,\"owned_by\":\"perplexity\"}," + "{\"id\":\"xai/grok-4-1-fast-non-reasoning\",\"object\":\"model\",\"created\":0,\"owned_by\":\"xai\"}," + "{\"id\":\"xai/grok-4.20-multi-agent\",\"object\":\"model\",\"created\":0,\"owned_by\":\"xai\"}," + "{\"id\":\"xai/grok-4.20-non-reasoning\",\"object\":\"model\",\"created\":0,\"owned_by\":\"xai\"}," + "{\"id\":\"xai/grok-4.20-reasoning\",\"object\":\"model\",\"created\":0,\"owned_by\":\"xai\"}," + "{\"id\":\"xai/grok-4.3\",\"object\":\"model\",\"created\":0,\"owned_by\":\"xai\"}" + "]}"; + + ai_parse_models_from_json(provider, json); + + /* Verify all 23 models were parsed correctly */ + assert_int_equal(23, g_list_length(provider->models)); + + /* Verify NO field names or owned_by values are in the model list */ + /* The bug was that "id", "object", "model", "created", "owned_by" and provider names + * like "anthropic", "google", "openai", "nvidia", "perplexity", "xai" were extracted */ + GList* models = provider->models; + while (models) { + const gchar* model = (const gchar*)models->data; + /* These should never be model IDs */ + assert_false(g_strcmp0(model, "id") == 0); + assert_false(g_strcmp0(model, "object") == 0); + assert_false(g_strcmp0(model, "model") == 0); + assert_false(g_strcmp0(model, "created") == 0); + assert_false(g_strcmp0(model, "owned_by") == 0); + assert_false(g_strcmp0(model, "list") == 0); + /* These are owned_by values, not model IDs */ + assert_false(g_strcmp0(model, "anthropic") == 0); + assert_false(g_strcmp0(model, "google") == 0); + assert_false(g_strcmp0(model, "openai") == 0); + assert_false(g_strcmp0(model, "nvidia") == 0); + assert_false(g_strcmp0(model, "perplexity") == 0); + assert_false(g_strcmp0(model, "xai") == 0); + models = g_list_next(models); + } + + /* Verify first and last model IDs */ + models = provider->models; + assert_string_equal("anthropic/claude-haiku-4-5", models->data); + models = g_list_last(models); + assert_string_equal("xai/grok-4.3", models->data); + + ai_provider_unref(provider); + ai_client_shutdown(); +} + +void +test_ai_parse_models_array_format(void** state) +{ + /* Test parsing simple array format: ["model1", "model2", ...] */ + ai_client_init(); + + AIProvider* provider = ai_add_provider("test", "https://test.api.com/v1", NULL); + + const gchar* json = "[\"model1\", \"model2\", \"model3\"]"; + + ai_parse_models_from_json(provider, json); + + assert_int_equal(3, g_list_length(provider->models)); + + GList* models = provider->models; + assert_string_equal("model1", models->data); + models = g_list_next(models); + assert_string_equal("model2", models->data); + models = g_list_next(models); + assert_string_equal("model3", models->data); + + ai_provider_unref(provider); + ai_client_shutdown(); +} + +void +test_ai_parse_models_empty_json(void** state) +{ + /* Test parsing empty/invalid JSON */ + ai_client_init(); + + AIProvider* provider = ai_add_provider("test", "https://test.api.com/v1", NULL); + + ai_parse_models_from_json(provider, ""); + assert_null(provider->models); + assert_int_equal(0, g_list_length(provider->models)); + + ai_parse_models_from_json(provider, "{\"invalid\": true}"); + assert_null(provider->models); + assert_int_equal(0, g_list_length(provider->models)); + + ai_provider_unref(provider); + ai_client_shutdown(); +} + +void +test_ai_parse_models_null_handling(void** state) +{ + /* Test NULL handling */ + ai_client_init(); + + AIProvider* provider = ai_add_provider("test", "https://test.api.com/v1", NULL); + + /* NULL json should not crash */ + ai_parse_models_from_json(provider, NULL); + assert_null(provider->models); + + ai_provider_unref(provider); + ai_client_shutdown(); +} + +void +test_ai_parse_models_escaped_quotes(void** state) +{ + /* Test parsing model IDs with escaped quotes */ + ai_client_init(); + + AIProvider* provider = ai_add_provider("test", "https://test.api.com/v1", NULL); + + const gchar* json = "{\"object\":\"list\"," + "\"data\":[" + "{\"id\":\"test/model\\\"with\\\"quotes\",\"object\":\"model\",\"created\":0,\"owned_by\":\"test\"}" + "]}"; + + ai_parse_models_from_json(provider, json); + + assert_int_equal(1, g_list_length(provider->models)); + /* Model ID should have the escaped quotes handled */ + GList* models = provider->models; + assert_non_null(models); + + ai_provider_unref(provider); + ai_client_shutdown(); +} + +void +test_ai_parse_models_with_whitespace(void** state) +{ + /* Test parsing JSON with whitespace after colons */ + ai_client_init(); + + AIProvider* provider = ai_add_provider("test", "https://test.api.com/v1", NULL); + + /* JSON with spaces after colons (common in formatted JSON) */ + const gchar* json = "{ \"object\": \"list\", " + "\"data\": [ " + "{ \"id\": \"model-with-spaces\", \"object\": \"model\", \"created\": 0, \"owned_by\": \"test\" }" + "]}"; + + ai_parse_models_from_json(provider, json); + + assert_int_equal(1, g_list_length(provider->models)); + GList* models = provider->models; + assert_string_equal("model-with-spaces", models->data); + + ai_provider_unref(provider); + ai_client_shutdown(); +} diff --git a/tests/unittests/test_ai_client.h b/tests/unittests/test_ai_client.h index 90d1d0b6..4e6db394 100644 --- a/tests/unittests/test_ai_client.h +++ b/tests/unittests/test_ai_client.h @@ -47,6 +47,14 @@ void test_ai_set_provider_setting(void** state); void test_ai_get_provider_setting(void** state); /* Model caching tests */ void test_ai_models_are_fresh_initial(void** state); +/* Model parsing tests */ +void test_ai_parse_models_openai_format(void** state); +void test_ai_parse_models_perplexity_format(void** state); +void test_ai_parse_models_array_format(void** state); +void test_ai_parse_models_empty_json(void** state); +void test_ai_parse_models_null_handling(void** state); +void test_ai_parse_models_escaped_quotes(void** state); +void test_ai_parse_models_with_whitespace(void** state); /* AI autocomplete integration tests */ void test_ai_start_provider_autocomplete_only_on_exact(void** state); void test_ai_models_find_null_session(void** state); diff --git a/tests/unittests/unittests.c b/tests/unittests/unittests.c index 9dceef57..af459a3a 100644 --- a/tests/unittests/unittests.c +++ b/tests/unittests/unittests.c @@ -697,6 +697,14 @@ main(int argc, char* argv[]) cmocka_unit_test_setup_teardown(test_ai_get_provider_setting, ai_client_setup, ai_client_teardown), /* Model caching tests */ cmocka_unit_test_setup_teardown(test_ai_models_are_fresh_initial, ai_client_setup, ai_client_teardown), + /* Model parsing tests */ + cmocka_unit_test(test_ai_parse_models_openai_format), + cmocka_unit_test(test_ai_parse_models_perplexity_format), + cmocka_unit_test(test_ai_parse_models_array_format), + cmocka_unit_test(test_ai_parse_models_empty_json), + cmocka_unit_test(test_ai_parse_models_null_handling), + cmocka_unit_test(test_ai_parse_models_escaped_quotes), + cmocka_unit_test(test_ai_parse_models_with_whitespace), /* AI autocomplete integration tests */ cmocka_unit_test_setup_teardown(test_ai_start_provider_autocomplete_only_on_exact, ai_client_setup, ai_client_teardown), cmocka_unit_test_setup_teardown(test_ai_models_find_null_session, ai_client_setup, ai_client_teardown),