Playlist usability update#17
Conversation
…umbers Calculator: - New "exact" action: arbitrary-precision integer evaluator (calculator_bignum) over + - * / ^ ! ; falls back to the float path for non-integer expressions. - Factorial: fix tinyexpr fac() to accumulate in double (overflowed past 20!), rewrite postfix "!" to fac(); calculator is now SCHEDULABLE (usable in plans). TTS number reading: - number_to_words (common/, scales through trigintillion) speaks large results as words instead of comma groups; wired into tts_preprocessing (>=13 digits). - word_to_number extended to the same scale vocabulary (symmetry). - Factorial reads naturally: prompt nudge writes "N factorial", WebUI compacts it back to "52!" on screen; espeak says the word. LLM tool loop: - Exempt non-deterministic actions (calculator random) from the duplicate-call guard; reword the dup hint so it isn't flagged as injection; gate the thinking-as-response fallback to local models; honest "model requested" log. Security: bound parser recursion (depth guard + 1024-char input cap) to stop a stack-exhaustion crash from deeply nested expressions on reduced worker stacks. Tests: calculator 43, number_to_words 19, word_to_number 30, tts_preprocessing 44 — all pass (incl. deep-nesting + length-cap regressions). ASan-clean on the bignum and TTS two-pass paths; build + format clean.
…arch
Adds the discover→curate→enqueue→edit→control workflow the music tool
lacked (surfaced by a real playlist-building session):
- enqueue (append) + play/enqueue items:[...] explicit track lists
- batch search items:[...] returning paths for exact enqueue
- clear/remove queue editing; deterministic shuffle/repeat (no blind
toggle); per-action state footer so the LLM always sees queue state
- description rewrite: one-field search semantics + themed-playlist recipe
Infra:
- new TOOL_PARAM_TYPE_ARRAY (string array) param type; LLM emits a native
JSON array, delivered via the terminal ::items:: slot + tail extractor;
registration guard enforces ARRAY-must-be-last
- plan_executor: support {{var}}/{{var.field}} (matches the documented
syntax; $var still works)
- split webui_music_tool.c out of webui_music_handlers.c; shared
transport-free queue helpers used by both browser and LLM paths
Reviewed (arch/efficiency/security): batch DoS cap, over-cap reporting,
stack trim, path-valid defense-in-depth. 67/67 CI tests pass (+6 new),
zero warnings.
…plays Two live-test fixes on the playlist tooling: - Bare titles like "Africa" resolved to music_db_search's first row, which is alphabetical by artist (Bo Burnham before Toto). Add a pure, unit-tested music_db_pick_best_match() that ranks candidates by title closeness (exact > prefix > substring) with a dominant artist-match bonus and a shorter-title tiebreak; no match keeps the DB's first row. Both the WebUI and local resolvers now search once and rank instead of taking top-1. (conv 813: "add Africa" queued the Bo Burnham track.) - WebUI enqueue only adds now — it no longer auto-starts playback when the queue was idle. "Add X to the queue" should append, not restart playback; the browser has its own play control. Local play (voice) still autoplays. (conv 814: enqueue restarted playback unexpectedly.) Adds 5 test_music_db cases incl. the conv-813 regression. 67/67 CI, zero warnings, format clean.
Code Review by Qodo
Context used✅ Compliance rules (platform):
27 rules 1.
|
There was a problem hiding this comment.
Pull request overview
This PR improves LLM tool usability for building and iteratively editing playlists (primarily via the WebUI music backend), while also extending the broader tool framework to support array parameters and more robust plan interpolation. It additionally enhances numeric/factorial handling across the calculator and TTS pipeline to better support “large result” workflows.
Changes:
- Add WebUI music playlist affordances (append/enqueue, batch search, queue editing, deterministic shuffle/repeat, state footer) and refactor shared queue mutation helpers.
- Extend the tool framework with
TOOL_PARAM_TYPE_ARRAY(schema + packed-arg encoding/decoding contract) and support{{var}}/{{var.field}}interpolation in the plan executor. - Improve factorial + large-number handling end-to-end (tinyexpr factorial fix, calculator exact big-int evaluator, number-to-words for TTS, and UI display compaction of “N factorial” → “N!”).
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| www/js/ui/format.js | Display-only cleanup for rendered markdown (strip <command> tags; compact unquoted “N factorial” to “N!”). |
| tests/test_word_to_number.c | Add unit coverage for extended short-scale magnitudes. |
| tests/test_tts_preprocessing.c | Add tests for large-number preprocessing into spoken words. |
| tests/test_tool_registry.c | Add tests for array-param schema emission and terminal-slot extraction behavior. |
| tests/test_plan_executor.c | Add tests for {{var}} and dot-access interpolation parity with $var. |
| tests/test_number_to_words.c | New unit tests for number-to-words conversion (incl. factorial-scale coverage). |
| tests/test_music_db.c | Add tests for the new pure relevance ranking music_db_pick_best_match. |
| tests/test_calculator.c | Add tests for postfix factorial and exact-mode big-integer evaluation. |
| tests/CMakeLists.txt | Wire new calculator bignum + number_to_words sources into tests. |
| src/word_to_number.c | Extend magnitude vocabulary to match number_to_words short-scale table. |
| src/webui/webui_music_tool.c | New WebUI LLM-tool entry points (enqueue/batch items/search, queue editing, deterministic shuffle/repeat, state footer). |
| src/webui/webui_music_handlers.c | Extract shared, transport-free queue mutation helpers and reuse them in WS handlers. |
| src/webui/webui_message_dispatch.c | Clarify model logging (“requested” vs “set”) and log OpenRouter bare-model drop behavior. |
| src/tts/text_to_speech.cpp | Add logging of preprocessed TTS text (currently at INFO). |
| src/tools/tool_registry.c | Enforce array terminal-slot contract at registration; emit JSON schema type:array + items. |
| src/tools/tinyexpr.c | Fix factorial accumulation overflow by using double accumulator and clamp at 170!. |
| src/tools/plan_executor.c | Implement {{var}} / {{var.field}} interpolation using shared resolver logic. |
| src/tools/music_tool.c | Expand music tool actions, add items[] support, batch search output with paths, WebUI routing improvements. |
| src/tools/calculator.c | Add postfix factorial rewrite for tinyexpr + implement calculator_evaluate_exact_str() with exact→float fallback. |
| src/tools/calculator_tool.c | Add new exact action, mark tool schedulable, and adjust duplicate-call behavior for random. |
| src/tools/calculator_bignum.c | New arbitrary-precision integer evaluator/parser for calculator exact mode. |
| src/llm/llm_tools.c | Add array schema emission; enforce packed-arg size bounds; exempt repeatable actions from duplicate-call guard. |
| src/llm/llm_tool_loop.c | Reword duplicate-call hint to avoid “[System:]” phrasing. |
| src/llm/llm_streaming.c | Prevent cloud chain-of-thought leakage by gating “thinking-as-response” fallback to local LLMs only. |
| src/llm/llm_openai_chat_completions.c | Align duplicate-call hint phrasing with the tool loop change. |
| src/llm/llm_command_parser.c | Add always-on prose formatting rule for factorial wording; preserve it even when tools are disabled. |
| src/audio/music_db.c | Add music_db_pick_best_match() relevance ranking helper. |
| include/webui/webui_music_internal.h | Declare shared queue mutation helpers + outcome enum for WebUI music. |
| include/tools/tool_registry.h | Document array delivery contract and add tool_param_extract_custom_tail() helper. |
| include/tools/calculator.h | Document + expose calculator_evaluate_exact_str() and add expression length cap constant. |
| include/tools/calculator_bignum.h | New public header for exact-mode evaluator + digit cap constant. |
| include/audio/music_db.h | Declare music_db_pick_best_match() API. |
| common/src/tts/tts_preprocessing.cpp | Add large-number token detection and conversion via number_to_words for TTS. |
| common/src/tts/number_to_words.c | New number-to-words implementation with short-scale names through trigintillion. |
| common/include/tts/number_to_words.h | New public API for number-to-words conversion. |
| common/CMakeLists.txt | Add number_to_words to common TTS library build. |
| CMakeLists.txt | Add new sources (number_to_words, calculator_bignum, webui_music_tool) to main build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pthread_mutex_lock(&uq->queue_mutex); | ||
| int len = uq->queue_length; | ||
| bool shuffle = uq->shuffle; | ||
| music_repeat_mode_t rep = uq->repeat_mode; | ||
| int idx = state->queue_index; | ||
| if (len > 0 && idx >= 0 && idx < len) { | ||
| safe_strncpy(now_title, uq->queue[idx].title, sizeof(now_title)); | ||
| safe_strncpy(now_artist, uq->queue[idx].artist, sizeof(now_artist)); | ||
| } | ||
| pthread_mutex_unlock(&uq->queue_mutex); |
| /* Build queue list — strbuf so a long queue cannot silently truncate | ||
| * mid-row the way the prior fixed sizing did when artist/title strings | ||
| * exceeded the per-row estimate. */ | ||
| strbuf_t sb; | ||
| strbuf_init(&sb, 1024 + (size_t)uq->queue_length * 64); | ||
| strbuf_appendf(&sb, "Queue (%d tracks, currently #%d):\n", uq->queue_length, | ||
| state->queue_index + 1); | ||
| const char *rep_s = uq->repeat_mode == MUSIC_REPEAT_ALL | ||
| ? "all" | ||
| : (uq->repeat_mode == MUSIC_REPEAT_ONE ? "one" : "none"); | ||
| strbuf_appendf(&sb, "Shuffle: %s | Repeat: %s\n", uq->shuffle ? "on" : "off", rep_s); | ||
| for (int i = 0; i < uq->queue_length; i++) { | ||
| const char *marker = (i == state->queue_index) ? "\xE2\x96\xB6 " : " "; | ||
| if (strbuf_appendf(&sb, "%s%d. %s - %s\n", marker, i + 1, | ||
| uq->queue[i].artist[0] ? uq->queue[i].artist : "Unknown", | ||
| uq->queue[i].title[0] ? uq->queue[i].title : "Unknown") < 0) | ||
| break; | ||
| } |
| static int local_resolve_item(const char *item, music_search_result_t *out) { | ||
| if (!item || !item[0]) { | ||
| return 0; | ||
| } | ||
| music_search_result_t r[MUSIC_RESOLVE_CANDIDATES]; | ||
| int count = 0; | ||
|
|
| int len = 0; | ||
| int remaining = (int)buffer_size; | ||
|
|
||
| /* Prose output-format rules first — apply regardless of tool mode. */ | ||
| len += snprintf(buffer + len, remaining - len, "%s", OUTPUT_FORMATTING_RULES); | ||
|
|
| assert(text != nullptr && "Received a null pointer"); | ||
| std::string inputText = preprocess_text_for_tts(std::string(text)); | ||
| OLOG_INFO("TTS preprocessed: \"%.200s\"", inputText.c_str()); | ||
|
|
| // Preprocess text for better TTS output | ||
| std::string processedText = preprocess_text_for_tts(std::string(text)); | ||
| OLOG_INFO("TTS preprocessed: \"%.200s\"", processedText.c_str()); | ||
|
|
…, lock + status Follow-up from Copilot/Qodo review: - Resolvers now accept exact library paths from `search` results: WebUI gates on webui_music_is_path_valid (covers local AND plex: paths), local adds an absolute-path music_db_get_by_path fast path. Fixes "search → enqueue returned paths" for Plex-backed libraries and the local backend. - tool_param_extract_custom_tail/_custom reject out_len==0 (a SIZE_MAX underflow would otherwise drive a huge memcpy). - append_state_footer + list snapshot queue_index under state_mutex (nested in queue_mutex, correct order) instead of reading it racily. - webui_music_execute_tool returns SUCCESS/FAILURE instead of raw 0/1. +1 unit test (extractor out_len==0). 67/67 CI, zero warnings, format clean.
…logs From external review (Copilot) of PR #17 — pre-existing issues in unrelated subsystems, fixed separately off main: - build_system_instructions_to_buffer() did `len += snprintf(...)`, which returns the would-have-written length; on truncation `len` could exceed buffer_size and later `buffer+len` / `size-len` writes go out of bounds. Add a clamping instr_appendf() helper and a bounded final memcpy. - text_to_speech logged the fully preprocessed TTS text at INFO on every request (and every WebUI streaming chunk), surfacing user content in operator logs. Downgrade both to DEBUG.
…, lock + status Follow-up from Copilot/Qodo review: - Resolvers now accept exact library paths from `search` results: WebUI gates on webui_music_is_path_valid (covers local AND plex: paths), local adds an absolute-path music_db_get_by_path fast path. Fixes "search → enqueue returned paths" for Plex-backed libraries and the local backend. - tool_param_extract_custom_tail/_custom reject out_len==0 (a SIZE_MAX underflow would otherwise drive a huge memcpy). - append_state_footer + list snapshot queue_index under state_mutex (nested in queue_mutex, correct order) instead of reading it racily. - webui_music_execute_tool returns SUCCESS/FAILURE instead of raw 0/1. +1 unit test (extractor out_len==0). 67/67 CI, zero warnings, format clean.
PR Summary by Qodo
Music playlist tooling + array params, interpolation fix, and calculator exact mode
✨ Enhancement🐞 Bug fix🧪 Tests⚙️ Configuration changes🕐 40+ MinutesWalkthroughs
User Description
Make the music tool usable for building playlists
Why
A real session (conv 811) exposed that the music tool couldn't support the workflow an LLM naturally reaches for. Asked to "build a diverse 80s playlist, then add more artists," Friday hit wall after wall — and each failed attempt was a spec for a missing affordance:
What changed
New LLM-facing music actions (WebUI):
Framework + infra:
Parity: full feature set on WebUI; the local-speaker path gets the cheap wins (play items[], batch search) plus honest "available on the web interface" fallbacks for queue editing it has no model for.
Behavior fixes from live testing
Testing
Addendum — external review pass (Copilot + Qodo)
Two follow-up commits address findings from the automated reviews. All six were real and triaged on merit (none skipped as "pre-existing").
Music tooling (commit 3):
searchresults — WebUI gates onwebui_music_is_path_valid(covers local andplex:paths), local adds an absolute-pathmusic_db_get_by_pathfast path. Fixes the "search → enqueue returned paths" workflow for Plex-backed libraries and the local backend.tool_param_extract_custom_tail/_customrejectout_len == 0(aSIZE_MAXunderflow would otherwise drive a hugememcpy). +1 unit test.append_state_footer+listsnapshotqueue_indexunderstate_mutex(nested inqueue_mutex) instead of reading it racily.webui_music_execute_toolusesSUCCESS/FAILUREinstead of raw0/1.Adjacent safety fixes (commit 4) — unrelated subsystems, folded in rather than deferred:
build_system_instructions_to_buffer:len += snprintf(...)could passbuffer_sizeon truncation (snprintf returns would-have-written), making laterbuffer+len/size-lenwrites go out of bounds. Added a clamping append helper + bounded finalmemcpy.Verified: 67/67 CI, zero warnings, format clean. Commits kept separate (no squash) so the review-fix history stays legible.
Notes / follow-ups
Stats: 14 files, +2008 / −576 (the big deletion is the handler split). 2 commits.
AI Description
• Add WebUI music actions for playlist workflows: enqueue, batch search, queue edit, state footer. • Introduce TOOL_PARAM_TYPE_ARRAY and fix plan interpolation to support {{var}} and dot-access. • Add calculator exact big-integer mode and TTS large-number reading with new unit coverage.Diagram
graph TD LLM["LLM tool loop"] --> PLAN["Plan executor"] --> REG["Tool registry"] --> EXE["Tool exec & schema"] EXE --> MUSICWEB["WebUI music tool"] --> DB[("Music DB")] MUSICWEB --> QUEUE[("WebUI queue")] EXE --> MUSICLOCAL["Local music tool"] --> DBHigh-Level Assessment
The following are alternative approaches to this PR:
1. Switch tool callbacks to accept structured JSON args
2. Unify WebUI + local queue logic behind a shared queue core
Recommendation: Current approach is a pragmatic incremental step: it delivers LLM-usable playlist building immediately by extending the existing schema + arg-packing pipeline (ARRAY param as serialized JSON tail) and sharing transport-free WebUI queue mutation helpers. Keep this as-is for now; consider migrating to structured JSON callback args only if delimiter-based packing becomes a recurring limitation, and defer queue-core unification until local-speaker parity or a third consumer justifies it.
File Changes
Enhancement (18)
Bug fix (7)
Refactor (2)
Tests (8)
Other (2)