Studio: Gemma tool-call streaming follow-ups + nested-XML escape fix (#6476) - #6611
Conversation
Address review findings on the tool-strip and streaming paths: - strip_tool_call_markup stripped Gemma-native spans with a plain regex that stops at the first <tool_call|>, so a literal close marker inside a <|"|>-quoted argument truncated the span and leaked its suffix into visible text. A brace/quote-aware _strip_gemma_native_spans now removes complete spans (keeping an incomplete one unless final), matching the parser's own balance logic. - The Gemma close pattern this PR added (<\|tool_call>.*?<tool_call\|>) had no \Z fallback, so a run of unclosed markers backtracked from every open position (quadratic, and the streaming stripper re-scans per token). It is now anchored to (?:<tool_call|>|\Z) like routes/inference.py's _TOOL_XML_RE, linear with identical output on well-formed input. - _SameTaskStreamingResponse added unstarted_cleanup for the OpenAI passthrough, but the local GGUF/safetensors streams that enter _TrackedCancel before returning only unregister in the generator finally, which never runs if the client disconnects before the body iterator starts, leaking cancel-registry entries. Each such stream now passes unstarted_cleanup to exit its tracker. - __call__ reads _unstarted_cleanup via getattr so a response built through __new__ (the cancel-timing test) without __init__ does not raise AttributeError; the test also sets the attribute explicitly. - Document that the verbatim /v1/chat/completions passthrough delegates <think>/<|tool_call> splitting to llama-server (--jinja, --reasoning-format auto) and is intentionally not re-parsed locally, noting the llama.cpp dependency. Adds a regression test for the close-marker-inside-quoted-argument strip.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces brace- and quote-balanced stripping of Gemma-native tool call spans to prevent premature truncation, optimizes the associated regexes to avoid backtracking, and fixes a resource leak where cancel-registry entries could be leaked if a client disconnected before a streaming response started. Feedback on these changes highlights a performance bottleneck in the new _strip_gemma_native_spans function, where repeated string slicing on a growing buffer during streaming results in quadratic time complexity; a code suggestion is provided to perform regex matching directly on the original string using index offsets.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| tail = text[brace_end + 1 :] | ||
| leading_ws = len(tail) - len(tail.lstrip()) | ||
| close = _TC_GEMMA_END_TAG_RE.match(tail, leading_ws) | ||
| if close is None: | ||
| if final: | ||
| out.append(text[cursor:start]) | ||
| cursor = len(text) | ||
| continue | ||
| out.append(text[cursor:start]) | ||
| cursor = brace_end + 1 + close.end() |
There was a problem hiding this comment.
Performance Bottleneck: Quadratic String Slicing in Streaming Path
In _strip_gemma_native_spans, slicing the remainder of the string with tail = text[brace_end + 1 :] creates a new string copy on every match. Since this function is called repeatedly on a growing cumulative buffer during streaming (per token), this slicing introduces a quadratic
We can completely avoid string slicing by matching _TC_GEMMA_END_TAG_RE directly on text using the pos argument of re.match.
| tail = text[brace_end + 1 :] | |
| leading_ws = len(tail) - len(tail.lstrip()) | |
| close = _TC_GEMMA_END_TAG_RE.match(tail, leading_ws) | |
| if close is None: | |
| if final: | |
| out.append(text[cursor:start]) | |
| cursor = len(text) | |
| continue | |
| out.append(text[cursor:start]) | |
| cursor = brace_end + 1 + close.end() | |
| close_idx = brace_end + 1 | |
| while close_idx < len(text) and text[close_idx].isspace(): | |
| close_idx += 1 | |
| close = _TC_GEMMA_END_TAG_RE.match(text, close_idx) | |
| if close is None: | |
| if final: | |
| out.append(text[cursor:start]) | |
| cursor = len(text) | |
| continue | |
| out.append(text[cursor:start]) | |
| cursor = close.end() |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d53b00d1d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if pat is _TC_GEMMA_CLOSED_PAT: | ||
| continue |
There was a problem hiding this comment.
When a Gemma block is closed but does not match _TC_GEMMA_START_RE exactly (for example, malformed call: syntax or <|tool_call>{"name":"x"}<tool_call|>), _strip_gemma_native_spans() leaves it untouched and this branch skips the only generic Gemma regex. The remaining orphan-close pattern then only removes <tool_call|>, so the opening marker and payload leak into visible content in both normal and final stripping; the previous generic pattern removed these closed spans even when they were not parseable, which is important for auto-heal/display cleanup of malformed model output.
Useful? React with 👍 / 👎.
| start = match.start() | ||
| if start < cursor: | ||
| continue | ||
| brace_end = _balanced_brace_end(text, match.end() - 1, gemma_quotes = True) |
There was a problem hiding this comment.
For outputs containing repeated unbalanced but syntactically valid Gemma starts (for example a model loops on <|tool_call>call:x{), this loop calls _balanced_brace_end() from every start and each call walks to EOF before returning -1. That makes strip_tool_call_markup() quadratic on exactly the kind of unclosed marker run this change is trying to make safe; after the first unmatched span the remainder should be kept/dropped according to final without scanning later starts.
Useful? React with 👍 / 👎.
| # spans via the brace/quote-aware _strip_gemma_native_spans, so a literal close | ||
| # marker inside a <|"|>-quoted argument cannot truncate the span and leak its | ||
| # suffix; the regex below is the streaming-stripper fallback. | ||
| _TC_GEMMA_CLOSED_PAT = re.compile(r"<\|tool_call>.*?(?:<tool_call\|>|\Z)", re.DOTALL) |
There was a problem hiding this comment.
This regex is still exported through _TOOL_ALL_PATS, and the GGUF/safetensors streaming strippers apply those patterns directly instead of calling _strip_gemma_native_spans(). A well-formed streamed call whose <|"|>-quoted argument contains the literal close marker still matches only up to that inner <tool_call|>, so the suffix (for example ")<|"|>} after) leaks into incremental display even though final strip_tool_call_markup() removes it correctly.
Useful? React with 👍 / 👎.
Compress the verbose comment blocks added with the Gemma tool-call / streaming work to crisp one or two liners, drop restatements of obvious code, and shorten docstrings, keeping the load-bearing rationale (ReDoS anchor, quote-aware strip, unstarted-cleanup, llama.cpp passthrough dependency). Code is unchanged (verified comment-only via AST/ast signature, docstrings stripped).
…eaming
- Security: the XML fallback in parse_tool_calls_from_text scanned the whole
content for <function=...> markers and only skipped those inside an open XML
parameter, not those inside a collected JSON/Gemma candidate span. A balanced
but unparsable Gemma call whose argument data contained XML tool markup
(<|tool_call>call:outer{code:<function=terminal>...}<tool_call|>) therefore
fell through to the fallback and returned an executable terminal call. The
fallback now also excludes <function=> markers inside any candidate span,
including ones that failed to parse.
- strip_tool_call_markup no longer skips the generic Gemma regex after running
the quote-aware _strip_gemma_native_spans, so a closed Gemma span the helper
cannot match (malformed, e.g. <|tool_call>{"name":"x"}<tool_call|>) is still
stripped instead of leaking its opener and payload into visible text.
- _strip_gemma_native_spans stops at the first unbalanced start instead of
re-scanning every later start to EOF, keeping it linear on a run of unclosed
markers rather than quadratic.
- The GGUF and safetensors streaming strippers run _strip_gemma_native_spans
before the regex patterns, so a well-formed streamed call whose quoted
argument contains a literal close marker no longer leaks its suffix into
incremental display.
Adds regression tests for the nested-XML escape and the malformed-span strip.
danielhanchen
changed the title
|
Hardened the sandboxed-Python static check ( The
Benign same-named usage stays allowed: Tests: 19 cases added to Scope: this strengthens the static tripwire for the |
|
Went through the four inline review notes on Applied (e552de5):
Already addressed in the current head (these reference the earlier commit state):
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e552de5c50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # (like _TOOL_XML_RE) so an unclosed run strips linearly, not quadratically; | ||
| # strip_tool_call_markup uses the quote-aware _strip_gemma_native_spans instead, | ||
| # leaving this regex as the streaming fallback. | ||
| _TC_GEMMA_CLOSED_PAT = re.compile(r"<\|tool_call>.*?(?:<tool_call\|>|\Z)", re.DOTALL) |
There was a problem hiding this comment.
Because this pattern is included in _TOOL_CLOSED_PATS, strip_tool_call_markup(..., final=False) now removes an unclosed Gemma marker through EOF: for example strip_tool_call_markup('before <|tool_call>call:x{a:1} after') returns 'before '. That contradicts the helper's non-final contract (and the existing unclosed-block behavior used while text is still streaming), so a partial or literal Gemma marker can hide all following visible content before the final cleanup pass. Keep the EOF alternative only in the final/streaming pattern set, and require <tool_call|> for the closed pattern.
Useful? React with 👍 / 👎.
| if not call_node.args: | ||
| return False |
There was a problem hiding this comment.
When the model calls __import__ or importlib.import_module with a keyword-only module name, this returns False before checking name=, but both functions accept that form. As a result, sandboxed code such as import importlib; importlib.import_module(name='os').system('id') bypasses the new dynamic-import block and then the outer .system call is not recognized by the existing os alias checks because it is invoked on a Call expression.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This targets the dynamic-import gate from an earlier revision that was reverted. tools.py is unchanged from main in this PR (empty diff), so there is nothing to gate on the current head.
| if isinstance(func, ast.Attribute) and isinstance(root, ast.Name): | ||
| if root.id in self.ctypes_aliases: | ||
| dyn = f"ctypes.{func.attr}" # CDLL / cdll.LoadLibrary / util.find_library |
There was a problem hiding this comment.
For from ctypes import cdll (or windll/oledll), the alias is recorded in dyn_exec_aliases, but this branch only treats names in ctypes_aliases as unsafe when the call is an attribute call. That lets cdll.LoadLibrary('libc.so.6').system(...) bypass the new ctypes loader protection, because dyn_exec_aliases is only consulted when the callee itself is an ast.Name.
Useful? React with 👍 / 👎.
| elif isinstance(func, ast.Name): | ||
| if func.id in _CODE_EXEC_BUILTINS: | ||
| dyn = func.id |
There was a problem hiding this comment.
This only flags bare eval/exec/compile names, so accessing the same builtins through the builtins module or a from-import alias bypasses the new dynamic-exec check. For example, import builtins; builtins.exec("import os; os.system('id')") contains no statically visible os.system call outside the string, so it passes the sandbox analysis and runs the blocked command.
Useful? React with 👍 / 👎.
| elif func.id in ("getattr", "setattr") and node.args: | ||
| tgt = node.args[0] | ||
| if isinstance(tgt, ast.Name) and ( | ||
| tgt.id in self.os_aliases or tgt.id in self.subprocess_aliases | ||
| ): |
There was a problem hiding this comment.
The new getattr handling only covers os and subprocess, but the same indirection bypasses the dynamic-import and ctypes checks added here because the outer call's callee becomes an ast.Call. For example, import importlib; getattr(importlib, 'import_module')('os').system('id') and import ctypes; getattr(ctypes, 'CDLL')('libc.so.6') are not added to dynamic_exec even though the direct forms are blocked.
Useful? React with 👍 / 👎.
Match the Gemma close marker with re pos directly on the buffer instead of slicing tail = text[brace_end + 1:] on every span. The streaming strippers re-scan a growing cumulative buffer per token, so the per-span remainder copy was quadratic. Behavior is unchanged.
e552de5 to
a06bbd5
Compare
|
Dropped the sandbox-Python indirection-blocking commit from this branch. Allowing The branch keeps only the on-topic change from this round: avoid the per-span remainder copy in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a06bbd564c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not _inside_open_parameter(content, fm.start()) | ||
| and not any(s <= fm.start() <= e for s, e in spans) |
There was a problem hiding this comment.
This guard only filters XML starts that fall inside spans, but spans is populated only after _balanced_brace_end succeeds. If the model emits an unbalanced Gemma call containing XML, e.g. <|tool_call>call:outer{code:<function=terminal><parameter=command>id</parameter></function>, no span is recorded and the fallback returns a terminal call, re-opening the nested-XML escape this change is trying to close in both allow_incomplete modes. Treat unclosed Gemma candidates as exclusion spans through EOF before scanning <function=>.
Useful? React with 👍 / 👎.
…followups # Conflicts: # studio/backend/routes/inference.py # tests/studio/test_stream_cancel_registration_timing.py
The nested-XML guard only skipped <function=> markers inside recorded
candidate spans, but a span is recorded only when the braces balance. An
unbalanced call such as <|tool_call>call:outer{code:<function=terminal>...
recorded no span, so the fallback still promoted the inner <function=> to
an executable terminal call. Treat unclosed JSON/Gemma starts as exclusion
spans through EOF before scanning. Standalone <function=> calls with no
preceding unclosed start still parse. Regression tests added.
|
Addressed the P1 and merged main (conflicts resolved). P1 (unclosed Gemma spans before the XML fallback): the exclusion guard only skipped Merge: brought in
Parser/strip/loop suites (164), the cancel-timing suite (25), and main's GGUF non-streaming suite (5) all pass. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75ce5bb569
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # (like _TOOL_XML_RE) so an unclosed run strips linearly, not quadratically; | ||
| # strip_tool_call_markup uses the quote-aware _strip_gemma_native_spans instead, | ||
| # leaving this regex as the streaming fallback. | ||
| _TC_GEMMA_CLOSED_PAT = re.compile(r"<\|tool_call>.*?(?:<tool_call\|>|\Z)", re.DOTALL) |
There was a problem hiding this comment.
When strip_tool_call_markup() is called with its default final=False, _TOOL_CLOSED_PATS is supposed to remove only fully closed blocks, but this pattern now also matches through \Z. As a result, a non-final cleanup of text like before <|tool_call>call:terminal{partial returns before instead of preserving the still-incomplete fragment, unlike the JSON-format path and the function’s documented semantics; callers that use the non-final stripper while streaming visible text can drop the rest of a literal/incomplete Gemma marker before the response is final. Consider keeping the EOF alternative only in the final=True/_TOOL_ALL_PATS pattern.
Useful? React with 👍 / 👎.
The lazy closed-pair strip patterns (<tool_call>.*?</tool_call>, <function=...>.*?</function>) rescan to EOF from every opener when their close token is absent, which is O(n^2) and re-runs per streamed token. Add strip_tool_patterns, which skips a pass whose close token is not present in the text; output is identical to the per-pattern loop (verified by fuzz), and a degenerate run drops from ~minutes to milliseconds. Used by strip_tool_call_markup and the GGUF/safetensors streaming strippers.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 939614efbd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # span (its argument data, even if it failed to parse) and inside any | ||
| # unclosed start through EOF; otherwise nested XML in a malformed call | ||
| # escapes into an executable tool call. | ||
| exclusion_spans = spans + [(s, len(content)) for s in unclosed_starts] |
There was a problem hiding this comment.
This exclusion only covers the balanced JSON/Gemma braces because spans ends at brace_end, not at the surrounding tool close marker. If a balanced but unparsable outer call has trailing XML before its close marker, e.g. <|tool_call>call:outer{broken:{x}}<function=terminal>...<tool_call|>, the <function=terminal> starts outside exclusion_spans and the fallback returns it as an executable tool call. Extend failed-candidate exclusions through their matching close tag or EOF before scanning XML.
Useful? React with 👍 / 👎.
| if end >= 0: | ||
| candidates.append((m.start(), end, "gemma", m)) | ||
| else: | ||
| unclosed_starts.append(m.start()) |
There was a problem hiding this comment.
This records an unclosed Gemma start only for the later XML fallback, but the JSON/Gemma candidate loop still accepts balanced starts that occur after it. For example, <|tool_call>call:outer{code:<|tool_call>call:terminal{command:id}<tool_call|> returns an executable terminal call in both allow_incomplete modes, even though the inner marker is inside the unclosed outer call's argument data; skip candidates covered by an earlier unclosed start before executing them. Fresh evidence is that the new unclosed_starts list is not consulted during candidate acceptance, only during XML fallback filtering.
Useful? React with 👍 / 👎.
| if close is None: | ||
| if final: | ||
| out.append(text[cursor:start]) | ||
| cursor = len(text) |
There was a problem hiding this comment.
When a Gemma span has a valid call:name{...} prefix but any non-whitespace before its close marker, this branch treats the close as missing and final=True drops through EOF. For example, final/streaming cleanup of pre <|tool_call>call:t{a:1} note <tool_call|> post returns only pre, losing the visible post; let the fallback strip the malformed closed span through <tool_call|> or search for that close before truncating to EOF.
Useful? React with 👍 / 👎.
Key the parser and stripper off the full <|tool_call>...<tool_call|> /
<tool_call>...</tool_call> envelope (start to close marker, searched after
the braces; EOF if unclosed) instead of just the braces:
- XML between the closing brace and the close marker
(call:outer{broken:{x}}<function=terminal>...<tool_call|>) is now inside
the envelope, so the fallback no longer promotes it to a tool call.
- A balanced inner call inside an unclosed outer
(call:outer{code:<|tool_call>call:terminal{...}<tool_call|>) is skipped
via the envelope nested check, not just the XML fallback.
- strip_tool_call_markup searches for the close marker after the braces, so
junk before <tool_call|> is stripped through the close and text after it is
preserved instead of truncated to EOF; a no-close run stops early (linear).
Regression tests added; standalone XML and well-formed calls unaffected.
|
Addressed all three findings. Root cause was shared: the parser and stripper keyed off the braces, not the full
Regression tests added for all three. Standalone |
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bd13b9a95
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # strip_tool_call_markup uses the quote-aware _strip_gemma_native_spans instead, | ||
| # leaving this regex as the streaming fallback. | ||
| _TC_JSON_CLOSED_PAT = re.compile(r"<tool_call>.*?</tool_call>", re.DOTALL) | ||
| _TC_GEMMA_CLOSED_PAT = re.compile(r"<\|tool_call>.*?(?:<tool_call\|>|\Z)", re.DOTALL) |
There was a problem hiding this comment.
Because _TC_GEMMA_CLOSED_PAT remains in _TOOL_CLOSED_PATS, adding the |\Z alternative makes the non-final path strip incomplete Gemma tool calls even though strip_tool_call_markup(..., final=False) documents that incomplete blocks are preserved. For example, an in-progress buffer like before <|tool_call>call:t{ now returns before instead of keeping the partial block, unlike the JSON/function formats and the existing non-final contract.
Useful? React with 👍 / 👎.
| env_end = len(content) | ||
| else: | ||
| cm = close_re.search(content, brace_end + 1) | ||
| env_end = cm.end() if cm else len(content) |
There was a problem hiding this comment.
When a marker's braces balance but its closing tag is missing, setting its envelope to EOF makes the nesting check below treat every later JSON/Gemma marker as part of that first call. In a response such as <|tool_call>call:a{x:1} ... <|tool_call>call:b{y:2}<tool_call|>, the valid closed b call is skipped (and with allow_incomplete=False, no call is returned at all), regressing the parser's ability to recover subsequent valid tool calls after one omitted close marker.
Useful? React with 👍 / 👎.
Split the nested-skip from the XML fallback exclusion: nesting is decided by each marker's brace region, so a balanced call after one with a missing close marker is recovered instead of being swallowed to EOF. Only the XML fallback keeps the search-to-close envelope, so trailing nested markup still cannot escape as an executable call. Use a closed-only Gemma pattern in the non-final strip list so an incomplete block is preserved (matching the JSON and function paths); the final list keeps the close-or-EOF Gemma pattern in its original position, so streaming display output is byte-for-byte unchanged. Add regression tests for both cases.
|
Addressed both findings in 9cca79b. 1. Non-final strip dropped incomplete Gemma blocks ( 2. Missing-close envelope swallowed a later valid call. Using one search-to-close (or EOF) envelope for both the nested-skip and the XML fallback meant a balanced call with no close marker got an envelope running to EOF, so the nested-skip dropped every later call. Regression tests added for both. The five earlier escape variants still block the nested |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4bc02c716
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| well-formed Gemma spans (quote-aware); then the regex sweeps mop up malformed | ||
| spans and drop any unclosed remainder to EOF. Surrounding whitespace is kept. | ||
| """ | ||
| text = strip_tool_patterns(text, _TOOL_CLOSED_BLOCK_PATS) |
There was a problem hiding this comment.
With mixed Gemma + XML tool output, this pre-pass runs the broad <function=...>.*?</function> regex before the Gemma span has been removed quote-aware. If a Gemma argument contains a literal <function=x> string and a later XML tool call closes with </function>, the pre-pass deletes across the Gemma boundary; then _strip_gemma_native_spans(..., final=True) sees the remaining Gemma opener as unclosed and truncates the rest of the visible response. For example, before <|tool_call>call:python{code:<|"|>print("<function=x>")<|"|>}<tool_call|> <function=terminal>...</function> after currently strips to just before instead of preserving the text after the tool calls.
Useful? React with 👍 / 👎.
The final display strip ran the closed JSON/function regex pre-pass before removing Gemma-native spans, so a literal <function=...> quoted inside a Gemma argument plus any later </function> (a real call's close or even prose) was deleted across the Gemma boundary. That mangled the Gemma close marker, the quote-aware helper then saw an unclosed opener, and the whole visible tail after the call was truncated. The pre-pass now skips matches that start inside a complete Gemma span (that text is the span's argument data) and resumes scanning at the end of the covering span, so a real function-XML call after the Gemma call is still stripped. The original ordering rationale is preserved: a Gemma opener inside a JSON or function argument still cannot truncate that block, covered by regression tests for both directions.
for more information, see https://pre-commit.ci
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
# Conflicts: # studio/backend/core/inference/llama_cpp.py # studio/backend/core/inference/safetensors_agentic.py # studio/backend/core/inference/tool_call_parser.py # studio/backend/core/tool_healing.py # studio/backend/tests/test_gemma_tool_parse_edge_cases.py # studio/backend/tests/test_tool_call_parser_strict.py
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
1 participant
Follow-up to #6476 (merged), landing review findings that arrived after the merge. The merge introduced one failing test into main, which this fixes, along with a security finding and several tool-strip/streaming gaps in the Gemma-native tool-call path.
Security
The XML fallback in
parse_tool_calls_from_textscanned the whole content for<function=...>markers and only skipped those inside an open XML parameter, not those inside a collected JSON/Gemma candidate span. A balanced but unparsable Gemma call whose argument data contained XML tool markup (<|tool_call>call:outer{code:<function=terminal>...}<tool_call|>) fell through to the fallback and was returned as an executableterminalcall, reachable from the safetensors tool loop. The fallback now also excludes<function=>markers inside any candidate span, including ones that failed to parse. Regression test added.Why (the test regression)
#6476 added
_SameTaskStreamingResponse.__init__with a_unstarted_cleanupattribute, but the fix for the test that depends on it landed in a later commit that was not part of the squash, sotests/studio/test_stream_cancel_registration_timing.py::test_same_task_response_closes_body_iterator_on_send_disconnectraisesAttributeErroronmaintoday.Changes
Fix the failing cancel-timing test.
__call__reads_unstarted_cleanupviagetattrso a response built through__new__without__init__does not raise; the test also sets the attribute.Quote-aware Gemma stripping.
strip_tool_call_markupstripped Gemma spans with a regex that stops at the first<tool_call|>, so a literal close marker inside a<|"|>-quoted argument truncated the span and leaked its suffix. A brace/quote-aware_strip_gemma_native_spansnow removes complete spans; the regex patterns still mop up any malformed Gemma span the helper cannot match.Anchor the Gemma close pattern.
<\|tool_call>.*?<tool_call\|>had no\Zfallback, so an unclosed run backtracked from every open position. It is anchored to(?:<tool_call|>|\Z)like_TOOL_XML_RE, and_strip_gemma_native_spansstops at the first unbalanced start instead of re-walking to EOF, keeping both linear.Quote-aware streaming. The GGUF and safetensors streaming strippers run
_strip_gemma_native_spansbefore the regex patterns, so a streamed call whose quoted argument contains a literal close marker no longer leaks its suffix into incremental display.Symmetric unstarted cleanup. The local GGUF/safetensors streams that enter
_TrackedCancelbefore returning now passunstarted_cleanupto exit the tracker on a pre-start disconnect, matching the OpenAI passthrough.Tighten the comments added with this work (verified comment-only via the
comment_tools.py checkAST signature).Document that the verbatim
/v1/chat/completionspassthrough delegates<think>/<|tool_call>splitting to llama-server.Tests
tests/studio/test_stream_cancel_registration_timing.pygreen again.