Fix GBNF grammar-generation bug breaking tool-call parsing (PagerDuty schema regression) - #117
Closed
dgabehar wants to merge 3 commits into
Closed
Fix GBNF grammar-generation bug breaking tool-call parsing (PagerDuty schema regression)#117dgabehar wants to merge 3 commits into
dgabehar wants to merge 3 commits into
Conversation
…al GBNF
common/json-schema-to-grammar.cpp's _visit_pattern() previously copied
PCRE-style shorthand character classes (\d, \D, \w, \W, \s, \S) straight
through into the generated GBNF grammar, both inside [...] bracket
expressions and as bare escapes elsewhere in the pattern. GBNF has no such
escapes -- src/llama-grammar.cpp's parse_char() only understands \x/\u/\U
(hex), \t/\r/\n, \\, \", \[, \] -- so it threw "unknown escape" at grammar
*parse* time, with no indication of which JSON schema or pattern produced
the bad grammar.
Because a tool-calling grammar is one combined blob for the whole toolset,
a single JSON Schema `pattern` using one of these shorthands (e.g. a
PagerDuty create_schedule MCP tool's leap-year-validated ISO-8601 pattern
containing sequences like \d\d[2468][048]) disabled grammar-constrained
decoding for the entire request -- confirmed via ~53 occurrences of
"parse: error parsing grammar: unknown escape at \d\d..." in production
logs, after which the server silently proceeded unconstrained.
Fix, entirely within _visit_pattern():
- Standalone \d/\D/\w/\W/\s/\S (not inside [...]) get their own GBNF
character-class rule ([0-9], [^0-9], [A-Za-z0-9_], [^A-Za-z0-9_],
[ \t\n\r], [^ \t\n\r]) via a new get_shorthand_class() helper, mirroring
the existing get_dot() handling for '.' so quantifiers compose correctly.
- \d/\w/\s inside a [...] bracket expression are always safe to inline as
extra members of that (possibly mixed) class, so they're translated
in place (e.g. [\dA-F] -> [0-9A-F]).
- \D/\W/\S inside a [...] bracket expression have no single-range GBNF
equivalent when mixed with other class members, so conversion now fails
loudly at schema-conversion time with an error naming the offending
pattern and escape, rather than silently emitting grammar text that
fails later, without context, inside the GBNF parser.
- The pre-existing "literal text" branch had the identical bug for any
shorthand escape following literal characters within the same token
(e.g. the \d in "^\d{4}-\d{2}-\d{2}$" right after the '-'); it now
breaks out and defers to the same top-level shorthand-class handling.
Adds regression coverage in tests/test-json-schema-to-grammar.cpp: mixed
and standalone shorthand classes, a pattern shaped like the actual
PagerDuty leap-year ISO-8601 pattern, the \D-in-brackets failure case
(asserting the error names the pattern/escape), and -- for these plus the
two pre-existing C++-only regexp tests, which weren't previously checked
this way -- confirms the generated grammar round-trips through
llama_grammar_parser::parse() without throwing.
Isolated to json-schema-to-grammar.cpp and its test file; no overlap with
the blocked upstream-merge grammar work or the Q2_0/DSpark CUDA territory.
Co-Authored-By: Claude <noreply@anthropic.com>
* chat: harden peg-native tool call parsing accept an optional leading type: function field in build_json_tools_flat_keys so openai style tool calls parse on templates whose serialization opens on the name field. return a clean error and log the unparsed fragment on a final peg parse failure instead of throwing the raw parser position and input. keep the raw arguments string in func_args_not_string when it is not valid json instead of aborting the prompt render. * chat: surface peg-native parse failures a final peg parse failure threw the raw parser position and input. log the unparsed fragment and raise a clearer error instead, so a model output that does not match the expected format no longer fails silently with an empty assistant turn. minimal change, no behavior change on successful parses. * chat: handle openai style tool calls in peg-native * nits * common: scope OpenAI wrapper grammar trigger via autoparser flag * chat: gate type:function parsing leniency on the analysis flag Thread accept_openai_wrapper from the generator to build_json_tools_flat_keys so the leading "type": "function" field is accepted only when openai_wrapper_trigger is set.
…g#24154) Throw on grammar parse failure so the server returns HTTP 400 instead of silently dropping the constraint. Add a regression test for the invalid-grammar response. Fixes ggml-org#24144
Author
|
Closing — not going through PR review for this, using the fork commit directly instead. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Root-caused and fixed a live bug where tool-calling requests carrying the
cai-mcptoolkit's full schema set (specifically PagerDuty'screate_scheduletool, whosepatternfield uses PCRE-style\descapes in an ISO-8601/leap-year regex) silently disabled grammar-constrained decoding for the entire request. llama.cpp's JSON-Schema-to-GBNF converter was copying PCRE shorthand character-class escapes (\d,\w,\s,\D,\W,\S) straight into generated GBNF text without translation; GBNF's own grammar parser (src/llama-grammar.cpp) doesn't understand those escapes and throwsunknown escape at .... Because the combined tool-calling grammar is one blob for the whole toolset, one bad schema took out constrained decoding for every tool call in the request — not just calls to the offending tool. Confirmed live in production logs (53 occurrences).Commits
common/json-schema-to-grammar.cpp: translate\d/\w/\sregex shorthand to legal GBNF — the actual root-cause fix. Standalone and in-bracket shorthand classes get proper GBNF translations (\d→[0-9], etc.);\D/\W/\Smixed inside a[...]class (no clean single-range GBNF equivalent) now fails loudly at schema-conversion time with a message naming the pattern and escape, instead of producing invalid GBNF that fails later with no context. New regression tests round-trip generated grammar through the real GBNF parser, including a test shaped exactly like the PagerDuty pattern that broke production.ggml-org/llama.cpp:581e8eca8— "chat: harden peg-native tool call parsing" (chat: harden peg-native tool call parsing ggml-org/llama.cpp#24329) — hardens the post-generation tool-call parser to tolerate more malformed shapes and fail with a clear logged error instead of silently producing an empty assistant turn.10786217e— "server: return HTTP 400 on invalid grammar" (Misc. bug: server: invalid GBNF grammar is silently ignored instead of returning HTTP 400 (regression from #17937) ggml-org/llama.cpp#24144/server : return HTTP 400 on invalid grammar (#24144) ggml-org/llama.cpp#24154) — defense-in-depth: if grammar compilation still fails for some other schema, return a clear error instead of silently proceeding with unconstrained decoding.All three build clean (CUDA + DSpark-Markov config matching production) and pass the full grammar/chat/peg test suite. Already running in production-adjacent use on the
bonsai-27b-midworkstation deployment with no grammar-parse errors observed post-deploy.🤖 Generated with Claude Code