Skip to content

fix(frontend): keep the last value on a duplicate tool-call parameter - #299

Open
adubkov wants to merge 1 commit into
Neroued:masterfrom
adubkov:fix/tool-call-duplicate-parameter
Open

adubkov wants to merge 1 commit into
Neroued:masterfrom
adubkov:fix/tool-call-duplicate-parameter

Conversation

@adubkov

@adubkov adubkov commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Problem and scope

Related Issue: #244

Scope note — broader than #244. #244 asks only about byte-identical
duplicates and states "Conflicting duplicates keep the current rejection
either way". This branch coalesces conflicting values too.

Captured agent sessions argue for that. Across the <tool_call> blocks in
those logs every observed repeat was conflicting, none identical
(6 conflicting / 0 identical), so an identical-only rule would have
recovered none of them. The shape was consistent: the model emitted a
parameter, continued with other parameters, then restated that same
parameter with a shortened rewording of the same intent, e.g.

<parameter=description>   (initial wording)
<parameter=subagent_type> ...
<parameter=description>   (shorter restatement of the same request)

Taking the last value adopts the model's own correction rather than an
arbitrary tie-break. Generations are from a private session and are
summarised structurally rather than quoted.

The lossy-replay tradeoff #244 raises still applies and is not addressed
here: returning markup verbatim preserves byte-exact history
reconstruction, whereas one structured argument replays as one
<parameter=…> block where the model emitted two. That is a cache
checkpoint against a dead agent turn, and the maintainer's call.

When a model repeats a <parameter=name> inside one <function=...> block,
parse_parameter returns DuplicateParameter. The decoder then restores the
entire tool-call region verbatim as assistant content.

To an OpenAI-compatible client this arrives as ordinary prose with
finish_reason: "stop" and no tool_calls, which is indistinguishable from
the model deciding to answer in text. The tool is never dispatched and nothing
surfaces the failure, so it presents as a hung session.

Reproduced against an Ornith-1.5-35B-A3B artifact (qwen3_6 chat template),
non-streaming and streaming alike:

finish_reason : stop
tool_calls    : None
content       : "<tool_call>\n<function=read_file>\n<parameter=path>\n..."

Frequency on one local deployment, from the operational log:

requests with a parsed tool call : 220
fell back to text                :  11
    5  malformed structure
    3  undeclared tool
    3  duplicate parameter

Change

  • parse_parameter keeps the last occurrence of a repeated parameter
    rather than discarding the call, matching how the same repetition reads in
    JSON object syntax. A call that is otherwise structurally sound survives.
  • ToolCallParseDiagnostics::duplicate_parameters_repaired counts the repair
    so it is observable rather than silent, and is emitted in the request log.
  • The fallback warning now includes a truncated, newline-flattened snippet of
    the offending markup. The reason alone names the verdict but not the bytes
    that earned it, which leaves malformed structure undiagnosable once the
    request is gone.

parse, parse_function and parse_parameter lose const because they now
update the repair counter. consume and find_parameter_close stay const.

Tests

tests/test_tool_call_parser.cpp:

  • Added test_duplicate_parameter_keeps_last_value, covering the structured
    result, the last-wins value, fallback_reason == None, and the repair count.
  • Removed the now-contradictory duplicate-rejection assertion from
    test_strict_structure_and_active_tool_set; that test is otherwise
    unchanged. The remaining 18 test functions are untouched.

Built and run in a container against this branch:

g++ -std=c++23 -O0 -Iinclude -Isrc -Ithird_party \
    src/models/qwen3_5/frontend/tool_call_parser.cpp \
    tests/test_tool_call_parser.cpp -o parser_test && ./parser_test
ok

Confirmed red before the change (5 assertions failing) and green after.

The full CUDA build succeeds and compiles the two touched serve files:

[348/433] .../qwen3_5/frontend/tool_call_parser.cpp.o
[398/433] .../ninfer_serve.dir/operational_log.cpp.o
[412/433] .../ninfer_serve.dir/request_log.cpp.o

End-to-end against a running server on this branch, a request whose history
contains two <parameter=path> entries with different values:

finish_reason : tool_calls
tool_calls    : [{"function": {"arguments": "{\"path\":\"/etc/hostname\"}", ...}}]
content       : None

Not run: the project's full test suite per tests/README.md, and any
benchmark. This change is not on a numerical or performance path.

Not addressed

Output-limit / truncation fallback is tracked separately in #169 and is out of
scope here.

malformed structure (the largest bucket above) is untouched. It is produced
by unclosed markup — output that ends inside a <parameter=...>, <function=
or <tool_call> block, i.e. the model stopping mid-call. Auto-closing those
would hand the caller a structurally valid call carrying a truncated
parameter value, which for an argument like a command or a subagent prompt is
worse than not dispatching at all. A duplicate parameter is different: both
values are complete, so taking the last one loses nothing.

A repeated <parameter=name> made parse_parameter return DuplicateParameter,
so the decoder restored the whole tool call verbatim as assistant content.
An OpenAI-compatible client sees prose with finish_reason "stop" and cannot
tell it apart from the model choosing not to call a tool, so the call is
silently dropped.

Keep the last occurrence instead, as JSON object syntax would, and count the
repair in ToolCallParseDiagnostics so it stays observable. Also log the
offending markup with the fallback reason: "malformed structure" names the
verdict but not the bytes that earned it, which makes the remaining fallbacks
undiagnosable after the request is gone.

Measured on a local deployment serving a Qwen3.5 artifact: 220 requests with
a parsed tool call, 11 fallbacks to text, 3 of them duplicate parameter.
@adubkov
adubkov marked this pull request as ready for review September 21, 2026 11:18
@adubkov adubkov changed the title Keep last value on duplicate tool-call parameter fix(frontend): keep the last value on a duplicate tool-call parameter Sep 21, 2026
@adubkov
adubkov marked this pull request as draft September 21, 2026 11:21
@adubkov
adubkov marked this pull request as ready for review September 21, 2026 12:52
Wallawalla47 pushed a commit to Wallawalla47/ninfer-custom that referenced this pull request Sep 21, 2026
Wallawalla47 pushed a commit to Wallawalla47/ninfer-custom that referenced this pull request Sep 21, 2026
…ameter repair)

Record the merged PR in the Upstream pull requests list (keep-the-last-value semantics plus the duplicate_parameters_repaired diagnostic and the markup snippet on fallback) and add adubkov to the Thanks section.
Wallawalla47 pushed a commit to Wallawalla47/ninfer-custom that referenced this pull request Sep 22, 2026
… forms

The Qwen tool-call parser accepted only the original <tool_call> /
<function=NAME> / <parameter=NAME> markup. Claude Code and other agent
harnesses emit the same call in XML forms the parser did not recognise, so
those responses fell back to plain text and the harness never saw a tool call:

  <tool_call><function name="X"><parameter name="d">v</parameter></function></tool_call>
  <function_calls><invoke name="X"><parameter name="d">v</parameter></invoke></function_calls>
  <invoke name="X"><param name="d">v</param></invoke>

Names are now read from the tag header, accepting either the original =NAME
form or a name= attribute, with attribute-token boundaries so an unrelated
filename= attribute cannot satisfy the name lookup, and with quote stripping.
Opening and closing forms must match, so a <function ...> closed by </invoke>
is a structural failure rather than a silent misparse. A <function_calls>
container accepts several children, and ordinary assistant text before the
call is preserved as content.

The streaming decoder tracked only a <tool_call> prefix; it now buffers any
partial marker from the full marker set, so every variant is detected
incrementally instead of leaking into visible content. Parameter value nesting
is counted against the matching close tag, so a nested <param ...> inside a
string value no longer truncates the value.

Taken from upstream PR Neroued#300 (tool-call parsing), by pkochubey, resolving
upstream issue Neroued#276. The PR rejects a repeated parameter with conflicting
values; this fork keeps the last value instead, per the local rule merged with
PR Neroued#299, so the duplicate branch writes the last value and counts the repair in
duplicate_parameters_repaired as before.

Verification: ninfer_tool_call_parser_test passes, including the new
function-name-attribute, invoke, function_calls container, standalone-invoke,
attribute-boundary, mismatched-closing-tag and Plan+TaskCreate reproduction
cases.
Wallawalla47 pushed a commit to Wallawalla47/ninfer-custom that referenced this pull request Sep 22, 2026
…calls

Ported from the gzenz/ninfer fork by David Oelfke (gzenz), September 2026: commit b2267e0 adds the flag and recovers complete calls with malformed wrapper or suffix output, commit 0ce6e3f recovers a single truncated final call when closing tags are cut off at region end, commit 0f3c9f5 recovers the function name when the closing bracket is omitted before a parameter tag, commit 3870983 keeps a value cut by the output budget and requires at least one complete parameter for a truncated final call (with the operational Info record guarded on kept calls), and commit 44f2c9c keeps complete calls with undeclared tool names.

gzenz's implementation sits on a divergent canonical parser, so the recovery logic is ported onto this fork's multi-marker parser, preserving PR Neroued#300 marker recognition and PR Neroued#299 last-value-wins duplicate handling. Tolerant mode keeps a good call when a trailing suffix or a malformed second call follows it (truncated_tail diagnostic, logged at Info), keeps a single final call cut at the region end with its partial value, recovers a missing closing bracket after the function name by an identifier-run scan, and keeps undeclared tool names structured. A truncated tail that keeps no call is returned as text with the reason recorded. The strict parser is unchanged and remains the default. Tests cover the tolerant paths; README and docs/serving.md credit the source.
Wallawalla47 pushed a commit to Wallawalla47/ninfer-custom that referenced this pull request Sep 24, 2026
Merges upstream Neroued#309 by Fedor Suchkov. The output session closed the reasoning
channel at the first literal </think>, so a model reasoning about chat templates had the rest
of its thinking published as content, which clients such as Qwen Code reject as leaked
thinking tags. A close now needs a boundary after the marker, and the tool-call parser tries
a later <tool_call> wrapper when an earlier, quoted one does not parse.

Adapted to this fork:
- only a line break confirms a close (the model serializes it as "\n</think>\n\n"); the PR
  also accepted a space, and a live check still leaked on "the </think> tag";
- the first candidate may be any marker form this fork recognizes, but retries walk only
  <tool_call> wrappers, so a truncated call is never re-read through its own nested
  <function=...> markup; tolerant truncated-tail recovery and PR Neroued#299 duplicate repair are
  kept.

Co-Authored-By: Fedor Suchkov <f.suchkov@gmail.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant