Skip to content

chat: only treat [tool_name( as tool-call start in LFM2.5 parser#24071

Closed
Dev-iL wants to merge 2 commits into
ggml-org:masterfrom
SummitSG-LLC:fix/lfm2-bracket-content-500
Closed

chat: only treat [tool_name( as tool-call start in LFM2.5 parser#24071
Dev-iL wants to merge 2 commits into
ggml-org:masterfrom
SummitSG-LLC:fix/lfm2-bracket-content-500

Conversation

@Dev-iL

@Dev-iL Dev-iL commented Jun 3, 2026

Copy link
Copy Markdown

Overview

LFM2.5 has no tool-call wrapper token, so its PEG parser stopped content at any bare [ and forced what followed into the tool-call branch. Ordinary content containing a [ (a list, an index, items = [...]) failed the parse, and the server surfaced the throw as an HTTP 500.

Per review feedback, the fix is limited to the LFM2.5 parser: content now stops only at [{tool_name}( for the defined tools (plus <|tool_call_start|>), matching the grammar triggers. A bare [ that doesn't start a defined tool call is consumed as content and the parse succeeds. This also fixes streaming, where the failed branch retracted already-streamed content.

Tests: two regression cases for bracketed content with tools defined, plus one partial-parse case asserting streamed content stays monotonic across a bare [.

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: YES: AI was used to pinpoint the problem and write the new tests' description.

@Dev-iL Dev-iL requested review from a team and pwilkin as code owners June 3, 2026 11:29
@github-actions github-actions Bot added the testing Everything test related label Jun 3, 2026
@ggml-gh-bot

ggml-gh-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown

Hi @Dev-iL, thanks for your contribution!

Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:

  • Multiple open PRs from a new contributor: We limit new contributors (those without a previously merged PR) to 1 open PR at a time. You currently have 2 open PRs.

Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below.

@aldehir

aldehir commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

The issue is here:
https://github.com/ggml-org/llama.cpp/blob/master/common/chat.cpp#L1765

If the model can produce tool calls without special token, then it should consume content until [{tool_name} for any tool instead of a single [. This will align it with the grammar triggers and constrain it to produce parseable output.

I don't know of any other parser that behaves this way, so the fix should be limited to the LFM2.5 parser.

@Dev-iL Dev-iL force-pushed the fix/lfm2-bracket-content-500 branch from 0229167 to 0953a1b Compare June 3, 2026 14:13
@Dev-iL Dev-iL changed the title chat: degrade bare-bracket parse failures to content instead of HTTP 500 chat: only treat [tool_name( as tool-call start in LFM2.5 parser Jun 3, 2026
@Dev-iL Dev-iL changed the title chat: only treat [tool_name( as tool-call start in LFM2.5 parser chat: only treat [tool_name( as tool-call start in LFM2.5 parser Jun 3, 2026
@Dev-iL

Dev-iL commented Jun 3, 2026

Copy link
Copy Markdown
Author

Thanks for the pointer!

  • The modifications to the shared failure path were reverted in favor of tightening the LPM-specific logic.
  • Another test case was added for streaming (partial-parse).

@tdakhran

tdakhran commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Discussed this internally with the team at Liquid AI, any valid tool call shall be wrapped by <|tool_call_start|> / <|tool_call_end|> tokens, existing logic in upstream is incorrect.

#24178 addresses the issue. There is a markdown test there that is treated as a tool call in upstream.

@Dev-iL

Dev-iL commented Jun 5, 2026

Copy link
Copy Markdown
Author

Discussed this internally with the team at Liquid AI, any valid tool call shall be wrapped by <|tool_call_start|> / <|tool_call_end|> tokens, existing logic in upstream is incorrect.

#24178 addresses the issue. There is a markdown test there that is treated as a tool call in upstream.

Is there anything in this PR worth keeping?

@tdakhran

tdakhran commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Discussed this internally with the team at Liquid AI, any valid tool call shall be wrapped by <|tool_call_start|> / <|tool_call_end|> tokens, existing logic in upstream is incorrect.
#24178 addresses the issue. There is a markdown test there that is treated as a tool call in upstream.

Is there anything in this PR worth keeping?

I suggest superseding it with #24178, it fixes more cases. Your fix for [ was on point and passes markdown test! Thanks for your contribution, @Dev-iL!

Screenshot 2026-06-05 164621

@Dev-iL

Dev-iL commented Jun 5, 2026

Copy link
Copy Markdown
Author

Is there anything in this PR worth keeping?

I suggest superseding it with #24178, it fixes more cases.

What about the new unit tests? Any point to keep them? If yes - perhaps include them in your PR?

@tdakhran

tdakhran commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Is there anything in this PR worth keeping?

I suggest superseding it with #24178, it fixes more cases.

What about the new unit tests? Any point to keep them? If yes - perhaps include them in your PR?

My PR adds new unit tests, including this one for markdown links.

@Dev-iL Dev-iL closed this Jun 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Everything test related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants