fix(nemotron): fail-open tool-call parsing (revives #1031, 2 pr_validate findings fixed)#1073
Merged
Conversation
Contributor
PR #1073 validation scorecardTitle: fix(nemotron): fail-open tool-call parsing (revives #1031, 2 pr_validate findings fixed) Verdict: MERGE-SAFE
|
NemotronToolParser keyed on the full <tool_call>...</tool_call> wrapper:
TOOL_CALL_PATTERN = re.compile(
r"<tool_call>\s*<function=([^>]+)>(.*?)</function>\s*</tool_call>",
re.DOTALL,
)
Nemotron models (notably at low quantization / after several tool rounds)
frequently emit near-miss variants of that shape. Because the wrapper was
mandatory, every one of these leaked the call through as plain assistant
`content` instead of a structured tool call:
(a) missing / truncated </tool_call>
<tool_call><function=f><parameter=x>1</parameter></function>
(b) bare <function=..>..</function> with no wrapper at all
<function=f><parameter=x>1</parameter></function>
(d) stray text between </function> and </tool_call>
<tool_call><function=f>..</function> ok </tool_call>
(e) prose between <tool_call> and <function=
<tool_call>Let me check.<function=f>..</function></tool_call>
Fix: the load-bearing signature of a call is <function=NAME>...</function>,
so key on that and treat the wrapper as optional/decorative.
- TOOL_CALL_PATTERN -> r"<function=([^>]+)>(.*?)</function>".
- RESIDUAL_WRAPPER_PATTERN strips leftover bare <tool_call>/</tool_call>
tags from content so they don't leak as text.
- Short-circuit returns not-called only when NEITHER <tool_call> NOR
<function= is present, so prose without a function signature still never
matches — no tool call is manufactured out of plain text.
- Streaming: the sentinel accepts <tool_call> or <function=, and fires on
</tool_call> OR </function> (a truncated variant may only ever emit
</function>). Re-parsed calls are de-duped against current_tool_id so a
call is emitted once even when </function> and </tool_call> arrive in
separate deltas.
- Diagnostic WARNING (truncated raw output) on the miss path, so any
remaining unhandled variant can be captured.
<parameter=..> handling and the JSON body path are unchanged.
Adds tests/test_nemotron_tool_parser_fail_open.py (15 cases: canonical
regression, variants a/b/d/e, surrounding-prose no-leak, zero false
positives, diagnostic-log miss path, and the streaming close-on-</function>
+ de-dup cases). Imports only the parser, so it runs without the MLX
runtime. Full tests/test_tool_parsers.py (156) still passes.
The parser is byte-identical in 0.9.8 and 0.10.1, so both are affected.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Finding 1 (privacy): the degraded-wire diagnostic logged model_output[:500]
at WARNING on every malformed marker — that raw slice can contain user
prompts, tool arguments, or credentials. Replace with a structural summary
(<tool_call> present + <function= tag count + 0 parseable); no payload.
Finding 2 (streaming): the streaming path fired only when a close tag
appeared within a single delta_text, so a close tag split across chunks
("</fun" + "ction>") never emitted the completed call. Trigger from the
completion state of current_text instead (re-parse + de-dupe against
current_tool_id). Add a regression test splitting </function> across two
chunks.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… format Refines the streaming fix so the completion check fires only when a NEW close tag finishes in the current delta (close-tag count in current_text > previous_text) instead of re-parsing whenever any </function> is present anywhere. This still handles a close tag split across chunks (the count only ticks up once both fragments land) while avoiding O(n^2) re-parsing and repeated fail-open WARNINGs on trailing deltas after a call has closed. Addresses the pr_validate codex_review finding on the reparse predicate. Add a regression test asserting no re-emit on trailing deltas; apply ruff format to the test module. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The streaming path returned None for every delta once a tool marker was in
current_text, so any assistant text after a call closed (notably a bare
<function=..></function> with no wrapper) was silently dropped. Now, once we
are past all tool-call markup, pass the delta through as a content event.
Safety: _in_clean_tail() only reports content-safe when the text after the
last complete close tag has no '<' at all, so no partial or complete tag
('<function=', '</function>', or a split fragment like '</fun'/'ction>') can
ever leak into user-visible content. Deltas that carry a close tag return
None; trailing prose flows in on the following clean deltas.
Applies uniformly to wrapped and bare calls (consistency). Adds regressions:
bare call + trailing prose, and a split close tag with no markup leak;
strengthens the trailing-delta test to assert the content events.
Addresses the pr_validate codex_review trailing-content finding.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When a single streaming delta carries both a completed close tag and trailing assistant text (e.g. the tokenizer emits "</function> done" as one chunk), the close-tag branch previously returned only tool_calls and dropped the trailing text. Emit it on the same delta via the combined content+tool_calls return the postprocessor already supports (see llama_tool_parser contract / postprocessor combined-delta handling). _clean_trailing_content() replaces _in_clean_tail(): it returns the content-safe tail after the last complete close tag (None while inside markup or once a new '<' starts), so a partial/complete tag can never leak. In the no-new-close branch we still emit only delta_text to avoid re-sending already-streamed trailing content. Add a regression test for the close-tag-plus-prose same-delta case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nly openers
Replace the 'any < suppresses' tail check with a proper content channel: diff
the cumulative safe-content of current vs previous text. Safe content =
everything with complete tool-call bodies / wrapper tags removed, holding back
only a trailing in-flight tool opener (<function= / <tool_call>, complete or a
partial prefix like '<fun'). An ordinary '<' in prose ('2 < 3') is NOT a tool
opener, so it now streams through instead of being dropped.
This also naturally covers prose that shares a delta with the opening marker
('Sure <function=...') and trailing prose after a call, with no re-emission
(content is a monotone suffix diff) and no markup leak. Tool_calls emission
(close-tag-count trigger + dedup) is unchanged; content rides out alongside it
via the combined content+tool_calls return the postprocessor supports.
Addresses the codex_review finding that '<'-bearing assistant prose after a
tool call was suppressed. Adds regressions for '2 < 3' trailing prose and
same-delta prefix prose.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the full-text _streamed_content recompute (and the leaky top guard)
with a single forward-scanning content channel that uses a persistent cursor,
so the whole response is scanned O(n) total instead of O(n^2) per token.
The scanner holds any trailing partial opener ('<fun', '<tool_', a lone '<')
until the next delta resolves it, so a split opener can no longer leak partial
tool markup as assistant content before the marker completes. It suppresses
<function=..></function> bodies, strips wrapper tags, and still streams a
genuine '<' in prose ('2 < 3'). Content state resets per request via reset().
Adds regressions for split <function= and split <tool_call> openers.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The incremental content cursor is per-request state. If a parser instance is reused for a new stream without reset(), a stale cursor could skip the new stream's leading content. Within one stream the cursor is always <= len(previous_text); if it is ahead (including a fresh stream where previous_text == ""), restart the scan. Add a reuse-without-reset regression. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rning - Track streaming tool-call de-dup with a parser-local _stream_calls_emitted counter advanced only by emitted calls (not the raw close-tag count), so a malformed earlier close tag cannot shift later call indices. Replaces the repurposing of base current_tool_id. - Only emit the fail-open WARNING when a genuine <tool_call> wrapper is present. A bare unmatched <function=> (no wrapper) is far more likely ordinary assistant prose than a real call, so it now fails open silently instead of logging noise. Add a no-warn regression. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…at EOS Streaming optimistically enters suppression on any <function=, so a bare <function=..> that never closes (ordinary prose, not a call) had its trailing text swallowed — inconsistent with the non-streaming fail-open. Implement the first-class flush_held_content hook (as llama/hermes/harmony do): at stream end, when no tool call fired, release the fail-open content that was not yet emitted (tracked via _content_emitted_len). Streaming + flush now reconstructs exactly the non-streaming content; empty when a real call parsed. Add regressions for the flush path and the no-flush-when-call-parsed case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reverts the incremental content scanner / flush_held_content / stream-emitted counter experiments (rounds addressing progressively deeper codex streaming edge cases) back to the minimal safe trailing-content passthrough (_clean_trailing_content: past-close tail with no '<'). Rationale: the deep machinery introduced a real double-emit regression through the postprocessor's no-'<' content fast-path (parser re-emitted content the postprocessor had already streamed). Also, the postprocessor drops ALL content once a tool call fires (postprocessor.py: 'if self.tool_calls_detected: return []'), so the trailing-content-after-a-call refinements codex kept requesting are moot end-to-end. This keeps the owner's two original findings (privacy log + split close-tag) plus the safe passthrough, with no double-emit and no XML leak. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
raullenchai
force-pushed
the
fix/nemotron-fail-open-1031
branch
from
0e688f9 to
1f04ab0
Compare
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
2 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.
Revives #1031 by @arthurhaliski (fail-open tool-call parsing for degraded Nemotron wire variants). His original commit is cherry-picked verbatim to preserve authorship. This PR addresses the two
pr_validatefindings that closed #1031, plus a minimal, safe streaming trailing-content passthrough.The two original findings (owner scope)
Privacy (
nemotron_tool_parser.py). The "marker present but nothing parsed" diagnostic loggedmodel_output[:500]at WARNING on every malformed marker — the normal degraded-wire path — which can contain user prompts, tool arguments, or credentials. Replaced with a structural summary (<tool_call>present +<function=tag count +0 parseable); no raw payload.Streaming split close-tag (
nemotron_tool_parser.py). The stream fired only when a close tag appeared inside a singledelta_text, so a close tag split across chunks ("</fun"then"ction>") never emitted the completed call. Now triggers from the completion state ofcurrent_text(close-tag count increase vsprevious_text) with de-dup, so the call is emitted exactly once however the close tag is chunked.Plus a minimal, bypass-safe trailing-content passthrough (
_clean_trailing_content: past-close tail with no<) that preserves same-delta trailing prose without leaking markup.Scope note
Deeper streaming content-channel refinements were explored and intentionally reverted: they risked (and in one case introduced) double-emit / dropped-content regressions, and are moot end-to-end because the postprocessor drops all content once a tool call fires. This PR keeps to the owner's stated scope + the safe passthrough.
Original PR / review: #1031
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com