Repository navigation
fix(clarify): drop maxLength from the choices schema so llama.cpp can parse the grammar - #131286
Conversation
|
Verified at f9032fe69a: test_schema_sanitizer.py passes 40/40. |
alt-glitch
added
type/bug
f9032fe to
33bd60a
Compare
|
I refreshed this branch on the latest main, and the CI workflows are waiting for a maintainer to approve the run since I am a first-time contributor. Could someone approve the workflow run so CI can validate the change? Thanks. |
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
The direct clarify schema change addresses #131278, and the focused suites pass. I found one cross-provider regression in the broader recovery change: expanding the shared strip helper removes supported length and item bounds from every xAI Responses request. I also confirmed that the reported llama.cpp error text does not yet enter this recovery path on current main; PR #130076 covers that classifier change separately. Details are inline.
| _STRIP_ON_RECOVERY_KEYS = frozenset({"pattern", "format"}) | ||
| # ``pattern``/``format`` use a regex subset llama.cpp lacks; ``maxLength``/``minLength``/``maxItems``/ | ||
| # ``minItems`` expand into bounded repetitions that its converter rejects at >= 2000 (#131278). | ||
| _STRIP_ON_RECOVERY_KEYS = frozenset({"pattern", "format", "maxLength", "minLength", "maxItems", "minItems"}) |
There was a problem hiding this comment.
[Bug] (blocking) This set also controls proactive xAI sanitization, not only llama.cpp recovery. Both _build_codex_kwargs() and _CodexCompletionsAdapter._build_responses_kwargs() call strip_pattern_and_format() on every xAI Responses request before any rejection. Expanding the shared set therefore removes valid minItems and maxItems constraints, including schemas that require exactly two coordinates, even though those call sites only document xAI rejecting pattern and format. A request that previously told Grok to emit exactly two coordinates is now sent without either bound, so malformed arrays can reach the handler. Please keep bounded-repetition stripping confined to the llama.cpp recovery path, or otherwise preserve the bounds that xAI supports.
There was a problem hiding this comment.
Thanks, you're right: the shared strip set is also used proactively on xAI Responses, so widening it would drop valid bounds there. I narrowed this PR to the clarify schema change that fixes #131278 and removed the recovery-set change (pushed as 13c84d3). The classifier gap you noted is covered separately by #130076.
| "b": {"type": "array", "items": {"type": "string", "maxLength": 2000}, "maxItems": 5000, "minItems": 1}, | ||
| }, | ||
| }}}] | ||
| result, stripped = strip_pattern_and_format(tools) |
There was a problem hiding this comment.
[Suggestion] (non-blocking) This direct helper test bypasses the classifier that gates production recovery. The reported 400 says Failed to initialize samplers: failed to parse grammar, but current main classifies that exact text as non-retryable format_error, so turn_recovery never calls this new stripping behavior. PR #130076 would add that match, but it is still open. An end-to-end regression from the reported error through classification and recovery would make this dependency explicit and prevent the helper-only test from going green while the real recovery path remains inactive.
… parse the grammar
33bd60a to
13c84d3
Compare
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
Approval conclusion (formal approval unavailable)
English verdict: APPROVE at exact head 13c84d3. The earlier cross-provider blocker is fixed, and I found no remaining blocking issue.
Motivation
Issue #131278 is a real regression. The base clarify schema sends choices.items.maxLength as 8000, while llama.cpp rejects bounded repetitions at 2000 or above before it generates any text. The issue isolates the 1999/2000 boundary, and a second Ollama user independently confirmed that removing this one keyword restores normal requests.
Approach
This revision fixes the constraint at its source. CLARIFY_SCHEMA no longer advertises maxLength for each choice, while _normalize_questions still rejects choice text longer than MAX_CHOICE_CHARS before any UI callback runs. That keeps the existing 8,000-character application limit without sending a schema keyword that breaks the provider.
The previous review at 33bd60a2 found that the broader recovery change also stripped valid bounds from xAI requests. That code is no longer in this diff. tools/schema_sanitizer.py is untouched, so the cross-provider regression is resolved.
Changes
The production change is one focused schema edit in tools/clarify_tool.py. The tests add two invariants: the clarify schema must stay below llama.cpp's repetition limit, and no registered built-in tool may introduce another bound at or above that limit. The second check matters because Hermes sends the selected registered tool schemas with model requests; one incompatible schema can reject an otherwise unrelated turn.
The positive path now sends the clarify tool without maxLength, so compatible local servers can build the grammar and process the request. The negative path remains owned by Hermes: if the model produces a choice longer than 8,000 characters, _normalize_questions returns the existing tool error instead of passing it to a UI surface.
Risk to main
I found no blocking issue. The exact head passed 68 repository tests across test_schema_sanitizer.py and test_clarify_tool.py. A separate review probe passed on this head and failed on the parent with maxLength=8000, which confirms that the test distinguishes the regression. The patch also merges cleanly into current upstream/main at 158fd63, and that composed tree passed the same 68 tests. git diff --check passed.
GitHub currently reports no hosted checks or commit statuses. I also did not run a real llama.cpp server locally, so the strongest end-to-end evidence remains the two independent reports on #131278.
One non-blocking housekeeping note: the PR description still lists the removed tools/schema_sanitizer.py change and the old validation totals. Please refresh it before merge so it matches this narrowed two-file diff.
Overall assessment
This is now a small, proportional fix at the correct boundary. It restores llama.cpp compatibility, preserves runtime validation, and avoids changing shared provider sanitization.
unsupportedpastels
left a comment
unsupportedpastels
left a comment
There was a problem hiding this comment.
Thanks @charan-rathore — clean, well-scoped fix, and the regression test that scans every registered tool schema is a nice guard against this coming back.
Verified locally against a llama.cpp server built before ggml-org/llama.cpp#26613:
clarify → choices.items |
Result |
|---|---|
no maxLength (this PR) |
✅ 200 |
maxLength: 8000 (main) |
❌ 400 failed to parse grammar |
maxLength: 2000 |
❌ 400 |
maxLength: 1999 |
✅ 200 |
The 8000-char cap is still enforced at runtime in clarify_tool.py, so behavior is unchanged for every other provider. CI is green. Merging — thanks again!
Infographic
Every request to a llama.cpp server fails with HTTP 400 "failed to parse grammar" because the clarify tool schema carries
maxLength: 8000, and llama.cpp rejects bounded repetition of 2000 or more.Closes #131278.
What happened
The clarify tool's
choicesitems schema setmaxLengthto 8000 (commit a4c31d5). llama.cpp's json-schema-to-grammar converter expandsmaxLength/minLength/maxItems/minItemsinto bounded repetitions and rejects anything at 2000 or more. The tool list is sent on every request, so every request to a llama.cpp server fails with "Failed to initialize samplers: failed to parse grammar", whatever the user asked. The reporter's bisect (see Credit) pinned the cutoff at 1999 working and 2000 failing.Change
This PR touches two files.
tools/clarify_tool.py: thechoicesitems schema no longer setsmaxLength. The runtime over-limit check on choices is untouched, so the limit is still enforced.tests/tools/test_schema_sanitizer.py: two new tests,test_clarify_schema_has_no_unbounded_repetition_keyword(the clarify schema carries no length bound of 2000 or more) andtest_registered_tool_schemas_stay_inside_llama_cpp_repetition_limit(no registered built-in tool ships a length or count bound of 2000 or more).tools/schema_sanitizer.pyis not changed. An earlier version of this PR also extended the grammar-failure recovery strip there; that was removed to keep the PR narrow.Credit
maxLength: 8000breaks every request on llama.cpp servers ("Failed to initialize samplers: failed to parse grammar") #131278.Verification
Ran on Python 3.12 at 13c84d3:
tests/tools/test_schema_sanitizer.pyandtests/tools/test_clarify_tool.py, 68 passed.Maintainer notes
tools/clarify_tool.py(reject clarify choices over the renderer cap), so one of the two PRs may need a small rebase.agent/error_classifier.py. This PR does not touch that; it removes the trigger.