From f7a818dc1df58b50305594ba908cb1a36914311d Mon Sep 17 00:00:00 2001 From: "jabber.developer2" Date: Fri, 10 Jul 2026 11:36:45 +0300 Subject: [PATCH] fix(ai): bound response parsing and harden AUTO fallback errors - bound _parse_responses scans to the output_text part's object and to the content array, so a later sibling item's "text" (e.g. a reasoning summary) can never be returned as the assistant reply - do not retry the other flavour on a curl timeout: the request likely reached the server and may still be generating, so a re-POST of the conversation could trigger a second billed generation - remember an unparseable 2xx from the first AUTO attempt and surface stashed first-attempt errors in both error paths, instead of showing only the final attempt's transport or HTTP error - recognize Ollama's model-not-found wording in _names_model so a model typo is not misread as a missing endpoint - drop the dead, racy provider-lookup fallback in the generic request thread: a missing provider ref is a caller bug and now fails loudly - fix ai_providers_lists_defaults to expect the header the command actually prints ("Configured providers:"); the test was broken since its introduction but CI never ran it - add functional test group 5 (AI command surface) to FUNC_TEST_GROUPS so the CI parallel target runs it; proftest.c port ranges already account for five groups --- Makefile.am | 2 +- src/ai/ai_client.c | 140 ++++++++++++++++++++++++------- tests/functionaltests/test_ai.c | 4 +- tests/unittests/test_ai_client.c | 42 ++++++++++ tests/unittests/test_ai_client.h | 3 + tests/unittests/unittests.c | 3 + 6 files changed, 159 insertions(+), 35 deletions(-) diff --git a/Makefile.am b/Makefile.am index c3e3503f..e44b38af 100644 --- a/Makefile.am +++ b/Makefile.am @@ -335,7 +335,7 @@ tests_functionaltests_functionaltests_LDADD = -lcmocka -lstabber @FORKPTY_LIB@ # Parallel functional tests target (~3x faster than sequential) # Usage: make check-functional-parallel # To add more groups: increase FUNC_TEST_GROUPS and add group in functionaltests.c -FUNC_TEST_GROUPS = 1 2 3 4 +FUNC_TEST_GROUPS = 1 2 3 4 5 check-functional-parallel: tests/functionaltests/functionaltests @echo "Running functional tests in parallel ($(words $(FUNC_TEST_GROUPS)) groups)..." diff --git a/src/ai/ai_client.c b/src/ai/ai_client.c index 029aa00d..969e5062 100644 --- a/src/ai/ai_client.c +++ b/src/ai/ai_client.c @@ -380,14 +380,12 @@ _json_unescape_substring(const gchar* start, const gchar* end) return g_realloc(out, w - out + 1); } -/* Locate "field":"...value..." in json and return the unescaped value. - * Skips JSON whitespace around the colon and treats any \X (X != \0) inside - * the string body as an opaque escape so an embedded \" cannot be misread as - * the closing quote. */ +/* Unescape the string value starting at @val (a pointer to its opening quote). + * Treats any \X (X != \0) inside the string body as an opaque escape so an + * embedded \" cannot be misread as the closing quote. */ static gchar* -_extract_json_string(const gchar* json, const gchar* field) +_extract_json_string_at(const gchar* val) { - const gchar* val = _find_json_field(json, field); if (!val || *val != '"') return NULL; @@ -404,6 +402,24 @@ _extract_json_string(const gchar* json, const gchar* field) return NULL; } +/* Locate "field":"...value..." in json and return the unescaped value. + * Skips JSON whitespace around the colon. @end (optional) bounds the search: + * a value starting at or past it is not extracted. */ +static gchar* +_extract_json_string_within(const gchar* json, const gchar* field, const gchar* end) +{ + const gchar* val = _find_json_field(json, field); + if (!val || (end && val >= end)) + return NULL; + return _extract_json_string_at(val); +} + +static gchar* +_extract_json_string(const gchar* json, const gchar* field) +{ + return _extract_json_string_within(json, field, NULL); +} + /* ======================================================================== * API type helpers * ======================================================================== */ @@ -456,11 +472,12 @@ _names_model(const gchar* s) { if (!s) return FALSE; - /* "does not exist" alone also appears in route-missing bodies; require it - * to talk about a model */ + /* "does not exist"/"not found" alone also appear in route-missing bodies; + * require them to talk about a model (covers Ollama's 'model "x" not + * found, try pulling it first') */ return strstr(s, "model_not_found") || strstr(s, "invalid_model") - || strstr(s, "unknown_model") || strstr(s, "model not found") - || (strstr(s, "model") && strstr(s, "does not exist")); + || strstr(s, "unknown_model") + || (strstr(s, "model") && (strstr(s, "does not exist") || strstr(s, "not found"))); } /* A missing endpoint answers 404/405/501. A wrong model can also 404/405, but @@ -1022,7 +1039,7 @@ typedef void (*ai_response_handler_t)(AIProvider* provider, const gchar* respons typedef struct { gchar* provider_name; - AIProvider* provider; /* owned ref taken on the main thread, or NULL */ + AIProvider* provider; /* owned ref taken on the main thread; required */ gpointer user_data; gchar* request_url; ai_response_handler_t handler; @@ -1086,20 +1103,16 @@ _ai_generic_request_thread(gpointer data) { ai_request_t* req = (ai_request_t*)data; - /* Keep the provider alive for the duration of this request. Prefer the ref - * taken on the main thread (guaranteed pre-removal); the lookup fallback - * still has a small window against /ai remove provider. */ + /* The ref taken on the main thread keeps the provider alive for the + * duration of this request; a thread-side lookup instead would race + * against /ai remove provider, so its absence is a caller bug */ AIProvider* provider = g_steal_pointer(&req->provider); if (!provider) { - provider = ai_get_provider(req->provider_name); - if (!provider) { - log_error("Provider '%s' not found for request", req->provider_name); - g_free(req->provider_name); - g_free(req->request_url); - g_free(req); - return NULL; - } - ai_provider_ref(provider); + log_error("BUG: request for '%s' spawned without a provider ref", req->provider_name); + g_free(req->provider_name); + g_free(req->request_url); + g_free(req); + return NULL; } CURL* curl = curl_easy_init(); @@ -1135,7 +1148,7 @@ _ai_generic_request_thread(gpointer data) curl_easy_cleanup(curl); g_free(response.data); - /* Release the reference taken after lookup — provider may now be freed. */ + /* Release the main-thread ref — provider may now be freed. */ ai_provider_unref(provider); g_free(req->provider_name); @@ -1833,6 +1846,33 @@ _same_json_object(const gchar* from, const gchar* to) return depth == 0; } +/* Pointer to the '}' closing the object that encloses @pos (which must sit + * outside a string literal, e.g. at a key's opening quote), or NULL if + * unterminated. Braces inside string literals are ignored. */ +static const gchar* +_json_object_end(const gchar* pos) +{ + gboolean in_string = FALSE; + gint depth = 0; + for (const gchar* p = pos; *p; p++) { + if (in_string) { + if (*p == '\\' && *(p + 1) != '\0') + p++; + else if (*p == '"') + in_string = FALSE; + } else if (*p == '"') { + in_string = TRUE; + } else if (*p == '{') { + depth++; + } else if (*p == '}') { + if (depth == 0) + return p; + depth--; + } + } + return NULL; +} + /* Pointer just past the ']' matching the '[' at @arr, or NULL if unterminated. * Brackets inside string literals are ignored. */ static const gchar* @@ -1870,7 +1910,11 @@ _parse_responses(const gchar* json) const gchar* part = strstr(output, "\"output_text\""); if (part) { - gchar* text = _extract_json_string(part, "text"); + /* Bound the forward scan to the part's own object so a "text" from a + * later sibling item (e.g. a following reasoning summary) can never + * be returned as the reply */ + const gchar* part_end = _json_object_end(part); + gchar* text = _extract_json_string_within(part, "text", part_end); if (text) return text; /* "text" may precede "type":"output_text" in the part: take the @@ -1883,7 +1927,7 @@ _parse_responses(const gchar* json) if (!key) break; if (_same_json_object(key, part)) { - text = _extract_json_string(key, "text"); + text = _extract_json_string_within(key, "text", part_end); if (text) return text; } @@ -1899,7 +1943,12 @@ _parse_responses(const gchar* json) const gchar* content_arr = strstr(output, "\"content\""); if (!content_arr) return NULL; - return _extract_json_string(content_arr, "text"); + /* Bound the scan to the content array itself: an empty or text-less array + * must fail the parse rather than pick up a "text" from a later item */ + const gchar* cval = _find_json_field(content_arr, "content"); + if (!cval || *cval != '[') + return NULL; + return _extract_json_string_within(cval, "text", _json_array_end(cval)); } /* Extract assistant content, trying the known request flavour's extractor @@ -2066,6 +2115,9 @@ _ai_request_thread(gpointer data) * actionable error; keep it in case the other flavour also fails */ auto_gchar gchar* first_reject_body = NULL; long first_reject_code = 0; + /* Same for a 2xx we could not parse: the user must learn their endpoint + * answered, even when the fallback attempt fails with its own error */ + auto_gchar gchar* first_unparsable_note = NULL; auto_gchar gchar* content = NULL; gboolean first_endpoint_missing = FALSE; for (gint i = 0; i < n_attempts; i++) { @@ -2109,9 +2161,12 @@ _ai_request_thread(gpointer data) continue; } - /* Connectivity failure (timeout/reset/DNS) on this flavour's endpoint: - * the other flavour may still be reachable, so try it before failing */ - if (res != CURLE_OK && more) { + /* Connectivity failure (reset/DNS/refused) on this flavour's endpoint: + * the other flavour may still be reachable, so try it before failing. + * A timeout is excluded: the request likely reached the server and may + * still be generating (and billing) — re-POSTing the conversation to + * the other endpoint could trigger a second generation */ + if (res != CURLE_OK && res != CURLE_OPERATION_TIMEDOUT && more) { log_info("[AI-THREAD] %s request for '%s' failed (%s), trying %s", ai_api_type_to_string(used_type), local_provider_name, curl_easy_strerror(res), ai_api_type_to_string(attempts[i + 1])); @@ -2126,9 +2181,12 @@ _ai_request_thread(gpointer data) content = ai_parse_response_typed(used_type, response.data); if (!content && more && configured == AI_API_TYPE_AUTO && http_code >= 200 && http_code < 300) { - log_info("[AI-THREAD] %s returned an unparseable 2xx for '%s', trying %s", + log_info("[AI-THREAD] %s returned an unparsable 2xx for '%s', trying %s", ai_api_type_to_string(used_type), local_provider_name, ai_api_type_to_string(attempts[i + 1])); + if (!first_unparsable_note) + first_unparsable_note = g_strdup_printf("%s endpoint answered HTTP %ld but the body could not be parsed", + ai_api_type_to_string(used_type), http_code); continue; } } @@ -2149,7 +2207,20 @@ _ai_request_thread(gpointer data) } if (res != CURLE_OK) { - auto_gchar gchar* error_msg = g_strdup(curl_easy_strerror(res)); + /* The stashed first-attempt failure is usually the actionable part: + * that endpoint did answer, this one did not */ + auto_gchar gchar* first_note = NULL; + if (first_reject_body) { + auto_gchar gchar* parsed = ai_parse_error_message(first_reject_body); + first_note = g_strdup_printf("%s endpoint rejected the request with HTTP %ld: %s", + ai_api_type_to_string(attempts[0]), first_reject_code, + parsed ? parsed : first_reject_body); + } else if (first_unparsable_note) { + first_note = g_strdup(first_unparsable_note); + } + auto_gchar gchar* error_msg = first_note + ? g_strdup_printf("%s (first attempt: %s)", curl_easy_strerror(res), first_note) + : g_strdup(curl_easy_strerror(res)); log_error("AI request failed for %s/%s: %s", local_provider_name, local_model, error_msg); _aiwin_display_error(user_data, error_msg); } else if (http_code >= 400) { @@ -2166,6 +2237,11 @@ _ai_request_thread(gpointer data) /* Try to extract the actual error message from the JSON response */ auto_gchar gchar* parsed_error = ai_parse_error_message(err_body); auto_gchar gchar* error_msg = parsed_error ? g_strdup_printf("HTTP %ld: %s", err_code, parsed_error) : g_strdup_printf("HTTP %ld: %s", err_code, err_body && strlen(err_body) > 0 ? err_body : "Unknown error"); + if (first_unparsable_note) { + gchar* with_note = g_strdup_printf("%s (first attempt: %s)", error_msg, first_unparsable_note); + g_free(error_msg); + error_msg = with_note; + } log_error("AI request failed for %s/%s: %s", local_provider_name, local_model, error_msg); _aiwin_display_error(user_data, error_msg); } else { diff --git a/tests/functionaltests/test_ai.c b/tests/functionaltests/test_ai.c index 9da93933..53742dae 100644 --- a/tests/functionaltests/test_ai.c +++ b/tests/functionaltests/test_ai.c @@ -55,11 +55,11 @@ ai_no_args_shows_help(void** state) void ai_providers_lists_defaults(void** state) { - /* `/ai providers` (no "list") shows the built-in list with URLs. */ + /* `/ai providers` (no "list") shows the configured list with URLs. */ prof_input("/ai providers"); prof_timeout(5); - assert_true(prof_output_exact("Available AI providers:")); + assert_true(prof_output_exact("Configured providers:")); /* At least one URL line is rendered — exact name agnostic. */ assert_true(prof_output_regex("https?://")); prof_timeout_reset(); diff --git a/tests/unittests/test_ai_client.c b/tests/unittests/test_ai_client.c index ccff7363..c8df36c3 100644 --- a/tests/unittests/test_ai_client.c +++ b/tests/unittests/test_ai_client.c @@ -1087,6 +1087,48 @@ test_ai_parse_response_responses_reasoning_only_returns_null(void** state) assert_null(out); } +void +test_ai_parse_response_responses_reasoning_after_message_skipped(void** state) +{ + /* A reasoning item ordered after the message, with the part's "text" + * preceding "type":"output_text": the forward scan must stay inside the + * part's object and the backscan must find the real answer. */ + const gchar* json = "{\"output\":[" + "{\"type\":\"message\",\"content\":[{\"text\":\"answer\",\"type\":\"output_text\"}]}," + "{\"type\":\"reasoning\",\"summary\":[{\"type\":\"summary_text\",\"text\":\"secret\"}]}" + "]}"; + auto_gchar gchar* out = ai_parse_response(json); + assert_non_null(out); + assert_string_equal("answer", out); +} + +void +test_ai_parse_response_responses_empty_content_not_leaked(void** state) +{ + /* An empty content array followed by a reasoning item must yield a parse + * failure; the scan must not escape the array and return the summary. */ + const gchar* json = "{\"output\":[" + "{\"type\":\"message\",\"content\":[]}," + "{\"type\":\"reasoning\",\"summary\":[{\"type\":\"summary_text\",\"text\":\"secret\"}]}" + "]}"; + gchar* out = ai_parse_response(json); + assert_null(out); +} + +void +test_ai_parse_response_responses_text_value_before_later_item(void** state) +{ + /* A reply that is exactly "text" plus a reasoning item after the message: + * the backscan's own extraction must also stay inside the part's object. */ + const gchar* json = "{\"output\":[" + "{\"type\":\"message\",\"content\":[{\"text\":\"text\",\"type\":\"output_text\"}]}," + "{\"type\":\"reasoning\",\"summary\":[{\"type\":\"summary_text\",\"text\":\"secret\"}]}" + "]}"; + auto_gchar gchar* out = ai_parse_response(json); + assert_non_null(out); + assert_string_equal("text", out); +} + void test_ai_parse_response_choices_in_string_value(void** state) { diff --git a/tests/unittests/test_ai_client.h b/tests/unittests/test_ai_client.h index 03a4ff91..b194c3b9 100644 --- a/tests/unittests/test_ai_client.h +++ b/tests/unittests/test_ai_client.h @@ -79,6 +79,9 @@ void test_ai_parse_response_responses_brace_in_text(void** state); void test_ai_parse_response_choices_logprobs_before_content(void** state); void test_ai_parse_response_reasoning_not_leaked_without_text(void** state); void test_ai_parse_response_responses_reasoning_only_returns_null(void** state); +void test_ai_parse_response_responses_reasoning_after_message_skipped(void** state); +void test_ai_parse_response_responses_empty_content_not_leaked(void** state); +void test_ai_parse_response_responses_text_value_before_later_item(void** state); void test_ai_parse_response_responses_text_value_text(void** state); void test_ai_parse_response_escaped_quote(void** state); void test_ai_parse_response_newline_escape(void** state); diff --git a/tests/unittests/unittests.c b/tests/unittests/unittests.c index 4907b8a0..1f2eaecc 100644 --- a/tests/unittests/unittests.c +++ b/tests/unittests/unittests.c @@ -789,6 +789,9 @@ main(int argc, char* argv[]) cmocka_unit_test(test_ai_parse_response_choices_logprobs_before_content), cmocka_unit_test(test_ai_parse_response_reasoning_not_leaked_without_text), cmocka_unit_test(test_ai_parse_response_responses_reasoning_only_returns_null), + cmocka_unit_test(test_ai_parse_response_responses_reasoning_after_message_skipped), + cmocka_unit_test(test_ai_parse_response_responses_empty_content_not_leaked), + cmocka_unit_test(test_ai_parse_response_responses_text_value_before_later_item), cmocka_unit_test(test_ai_parse_response_responses_text_value_text), cmocka_unit_test(test_ai_parse_response_escaped_quote), cmocka_unit_test(test_ai_parse_response_newline_escape),