fix(ai): unbreak CI and harden custom settings after chat-completions alignment #152

jabber.developer2 · 2026-07-04T11:01:09Z

jabber.developer2 commented

2026-07-04 11:01:09 +00:00

Summary

Master CI is red since 9913344bb (chat-completions alignment): all 4 build flavors fail on two unit tests that still assert the removed legacy Perplexity "text" extraction (CI task 4817, RESULT: FAILED (builds 1 2 3 4)). This PR fixes the tests and, while in the area, hardens the /ai set custom payload path introduced by 91631aa91, which could silently produce invalid JSON and race between threads.

What was broken

CI blocker — stale tests. test_ai_parse_response_perplexity_text and test_ai_parse_response_text_preferred_over_content assert the dropped /v1/agent "text" behavior; ai_parse_response() now extracts only chat-completions "content", so both fail and make check breaks on every flavor.
Invalid JSON from string-valued custom settings. _build_json_payload_from_list() emitted custom values JSON-escaped but unquoted: /ai set custom <p> reasoning_effort high → "reasoning_effort": high → the whole request fails with a parse error on the provider side. Quoting manually could not work either, since the escaper rewrites " as \". Only numeric/boolean values could ever pass. The built-in /help ai example (/ai set custom perplexity tools enabled) produced exactly this broken payload.
Cross-thread race on provider->settings. The main thread mutates the hash via /ai set custom (g_hash_table_insert/remove) while the AI request thread iterates it with GHashTableIter when building the payload; session->lock does not cover the mutator. A concurrent insert can rehash under the iterator → undefined behavior.
Reserved-key collisions. Custom keys model/messages/stream duplicated the fixed payload fields with parser-dependent (last-wins) precedence: a custom stream was silently ignored, a custom model silently overrode the standard one.
Stale docs. ai_parse_response() header comment still described the Perplexity-first extraction; the payload docstring still said "input"; help example promoted a non-existent string param.

Changes

1210daf35 — fix(ai): harden custom settings payload against invalid JSON and races (src/ai/ai_client.{c,h}, src/command/cmd_funcs.c, src/command/cmd_defs.c)

Emit RFC 8259 scalars (number / true / false / null, validated by regex) bare; everything else as a quoted, escaped JSON string — every value is now representable and the payload is always valid JSON.
Reject reserved payload keys (model, messages, stream) in ai_set_provider_setting() (now returns gboolean; /ai set custom shows an error) and skip them at payload-build time as a second line of defense for hand-edited prefs.
Add per-provider settings_lock (pthread mutex): held by the mutator, the reader (ai_get_provider_setting) and the request-thread iteration. Lock order is session->lock → settings_lock on the worker only; the main thread takes settings_lock alone — no inversion.
Refresh stale docs and switch the help example to a realistic scalar setting (/ai set custom perplexity temperature 0.7).

ff0ec78e7 — test(ai): align parse_response tests with chat completions API (tests/unittests/)

test_ai_parse_response_perplexity_text → test_ai_parse_response_legacy_text_format_unsupported: the legacy nested format now yields NULL by design.
test_ai_parse_response_text_preferred_over_content → test_ai_parse_response_content_preferred_over_text: "content" is authoritative now.
New test_ai_set_provider_setting_reserved_key: reserved keys rejected, ordinary keys still work.

Testing

Full build clean with -Werror (autotools, Full flavor configure flags).
Unit tests: 645/645 PASSED, including the renamed and the new ones.
Payload sanity by construction: scalars bare ("temperature": 0.7), strings quoted ("reasoning_effort": "high"), reserved keys never emitted.

Notes

Once merged, master CI should go green again — the four red flavors all failed on these two tests only. If AUR publishing is gated on CI (per the "AUR не выдаёт последнюю версию" report), this unblocks it.
Objects/arrays as custom values are intentionally emitted as JSON strings for now; if a real need for raw JSON objects appears, a validating pass-through can be added later.

## Summary Master CI is red since `9913344bb` (chat-completions alignment): all 4 build flavors fail on two unit tests that still assert the removed legacy Perplexity `"text"` extraction (CI task 4817, `RESULT: FAILED (builds 1 2 3 4)`). This PR fixes the tests and, while in the area, hardens the `/ai set custom` payload path introduced by `91631aa91`, which could silently produce invalid JSON and race between threads. ## What was broken 1. **CI blocker — stale tests.** `test_ai_parse_response_perplexity_text` and `test_ai_parse_response_text_preferred_over_content` assert the dropped `/v1/agent` "text" behavior; `ai_parse_response()` now extracts only chat-completions `"content"`, so both fail and `make check` breaks on every flavor. 2. **Invalid JSON from string-valued custom settings.** `_build_json_payload_from_list()` emitted custom values JSON-escaped but unquoted: `/ai set custom <p> reasoning_effort high` → `"reasoning_effort": high` → the whole request fails with a parse error on the provider side. Quoting manually could not work either, since the escaper rewrites `"` as `\"`. Only numeric/boolean values could ever pass. The built-in `/help ai` example (`/ai set custom perplexity tools enabled`) produced exactly this broken payload. 3. **Cross-thread race on `provider->settings`.** The main thread mutates the hash via `/ai set custom` (`g_hash_table_insert/remove`) while the AI request thread iterates it with `GHashTableIter` when building the payload; `session->lock` does not cover the mutator. A concurrent insert can rehash under the iterator → undefined behavior. 4. **Reserved-key collisions.** Custom keys `model`/`messages`/`stream` duplicated the fixed payload fields with parser-dependent (last-wins) precedence: a custom `stream` was silently ignored, a custom `model` silently overrode the standard one. 5. **Stale docs.** `ai_parse_response()` header comment still described the Perplexity-first extraction; the payload docstring still said `"input"`; help example promoted a non-existent string param. ## Changes **`1210daf35` — fix(ai): harden custom settings payload against invalid JSON and races** (`src/ai/ai_client.{c,h}`, `src/command/cmd_funcs.c`, `src/command/cmd_defs.c`) - Emit RFC 8259 scalars (number / `true` / `false` / `null`, validated by regex) bare; everything else as a quoted, escaped JSON string — every value is now representable and the payload is always valid JSON. - Reject reserved payload keys (`model`, `messages`, `stream`) in `ai_set_provider_setting()` (now returns `gboolean`; `/ai set custom` shows an error) and skip them at payload-build time as a second line of defense for hand-edited prefs. - Add per-provider `settings_lock` (pthread mutex): held by the mutator, the reader (`ai_get_provider_setting`) and the request-thread iteration. Lock order is `session->lock` → `settings_lock` on the worker only; the main thread takes `settings_lock` alone — no inversion. - Refresh stale docs and switch the help example to a realistic scalar setting (`/ai set custom perplexity temperature 0.7`). **`ff0ec78e7` — test(ai): align parse_response tests with chat completions API** (`tests/unittests/`) - `test_ai_parse_response_perplexity_text` → `test_ai_parse_response_legacy_text_format_unsupported`: the legacy nested format now yields `NULL` by design. - `test_ai_parse_response_text_preferred_over_content` → `test_ai_parse_response_content_preferred_over_text`: `"content"` is authoritative now. - New `test_ai_set_provider_setting_reserved_key`: reserved keys rejected, ordinary keys still work. ## Testing - Full build clean with `-Werror` (autotools, Full flavor configure flags). - Unit tests: **645/645 PASSED**, including the renamed and the new ones. - Payload sanity by construction: scalars bare (`"temperature": 0.7`), strings quoted (`"reasoning_effort": "high"`), reserved keys never emitted. ## Notes - Once merged, master CI should go green again — the four red flavors all failed on these two tests only. If AUR publishing is gated on CI (per the "AUR не выдаёт последнюю версию" report), this unblocks it. - Objects/arrays as custom values are intentionally emitted as JSON strings for now; if a real need for raw JSON objects appears, a validating pass-through can be added later.

jabber.developer2 added 2 commits 2026-07-04 11:01:09 +00:00

fix(ai): harden custom settings payload against invalid JSON and races 1210daf357

Custom setting values were JSON-escaped but emitted unquoted, so any
non-numeric value (e.g. "high") produced an invalid JSON payload and
failed the whole request; quoting the value manually could not work
either, since the escaper turns quotes into \". Emit RFC 8259 scalars
(number/true/false/null) bare and everything else as a quoted JSON
string.

Reserved payload keys (model, messages, stream) would duplicate the
fixed fields with parser-dependent precedence; reject them in
ai_set_provider_setting (surfaced as an error by /ai set custom) and
skip them at payload-build time for settings loaded from hand-edited
prefs.

provider->settings was mutated on the main thread while the request
thread iterates it when building the payload; guard both sides with a
new per-provider settings_lock.

Also refresh stale docs: ai_parse_response no longer mentions the
dropped Perplexity "text" path, the payload docstring says "messages"
instead of "input", and the /ai help example uses a realistic scalar
setting.

test(ai): align parse_response tests with chat completions API

CI Code / Check spelling (pull_request) Successful in 17s

Details

CI Code / Check coding style (pull_request) Failing after 31s

Details

CI Code / Linux (ubuntu) (pull_request) Successful in 7m40s

Details

CI Code / Linux (debian) (pull_request) Successful in 7m54s

Details

CI Code / Code Coverage (pull_request) Successful in 8m38s

Details

CI Code / Linux (arch) (pull_request) Successful in 11m0s

Details

ff0ec78e78

The chat-completions refactor removed the legacy Perplexity "text"
extraction but left two tests asserting the old behavior, breaking
make check on every CI flavor:

- test_ai_parse_response_perplexity_text expected the legacy /v1/agent
  nested format to parse; it now yields NULL — renamed to
  test_ai_parse_response_legacy_text_format_unsupported.
- test_ai_parse_response_text_preferred_over_content expected "text"
  to win; "content" is authoritative now — renamed to
  test_ai_parse_response_content_preferred_over_text.

Add test_ai_set_provider_setting_reserved_key covering the new
reserved-key rejection.

jabber.developer2 force-pushed fix/ai-chat-completions-followup from ff0ec78e78 to 5d7b7bb23d

2026-07-04 11:08:12 +00:00

Compare

jabber.developer2 manually merged commit 5d7b7bb23d into master

2026-07-04 11:21:29 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: devs/cproof#152