Skip to content

fix(server): resolve a namespaced tool call to the offered tool (replaces #1248) - #1254

Merged
justinchuby merged 2 commits into
mainfrom
serverstack-1248
Aug 18, 2026
Merged

justinchuby merged 2 commits into
mainfrom
serverstack-1248

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

Replacement for #1248, rebased onto current main after #1197/#1224/#1236 landed as squashes (the stacked base diverged). The original functional commit plus two added tests, re-validated on top of the merged stack.

Symptom

A model driven with flat tool names (bash, edit, glob, grep, read, write) emits a namespaced call such as glob.glob, which the client rejects: it can only dispatch a name it offered.

Cause (as claimed by the PR, and what I could and could not confirm)

The caller offers flat tool definitions over the API. The instruction text the model reads is rendered by the package's own chat template, which the PR says assumes namespaced names, takes name.split('.')[0], and renders # Valid recipients: "self", "glob.*", ..., "user". — so the model follows a grammar that is wrong for a flat offer and writes glob.glob.

I could not inspect that template with my own eyes. It ships inside the model package (Muse Glimmer / harmony-style), which is not in this repository or in my local model set; searching the repo and local packages for the split('.') / "Valid recipients" grammar found nothing. What I can confirm from the server side corroborates the addressing scheme: the ATEM parser recognises to=functions.<name> channels and <atem:invoke name="..."> blocks (extract_atem_tool_calls, ATEM_USER_CHANNEL = "to=user<|message|>"), so a namespaced recipient like functions.glob producing a call named glob.glob is consistent with the diagnosis.

Template fix vs dispatch fix — my conclusion: resolving at dispatch is the right layer, and this is a considered choice, not merely the first available. The template belongs to the model author and is rendered in the jinja sandbox; the server cannot rewrite an arbitrary third-party package's template at runtime, and the namespaced grammar is not universally wrong (it is correct for a caller that offers namespaced tools — only a flat offer breaks it). The server is the only component that sees both the caller's offer and the model's emitted call, so it is the only place that can reconcile them without editing model files. A template correction for flat offers would still be worthwhile in the package, but it cannot be the server's runtime responsibility.

Fix

When a parsed call names a tool that is not on offer but whose final .-segment exactly equals an offered tool's whole name, the call is resolved to that offered tool; anything else is left exactly as the model spelled it. Applied at the three call sites that already have the request in scope (buffered, streaming, and the logprob path), via ParsedAssistantOutput::aligned_to / align_tool_calls.

Ambiguity is impossible by construction, not merely avoided: the resolution target is always the single final segment string, and it resolves only if that exact string is an offered (unique) name — there is never a choice between two candidates. When two offered tools merely share a final segment (fs.read, net.read) and the model writes a third namespace (svc.read), the bare segment read is offered by neither, so the call is left alone for the caller to reject.

Verification (mine, on this branch atop the merged stack)

  • 216 lib + 31 http + 11 bundle tests pass; cargo clippy -p onnx-genai-server --all-targets -- -D warnings clean.
  • Falsified the positive claim (revert → RED → restore → GREEN): neutering align_tool_calls to a no-op made a_namespaced_call_resolves_to_the_offered_tool and the new full-parse-path test return glob.glob instead of glob.
  • Confirmed the negative tests have teeth, and characterised their limits honestly: a no-op revert leaves the negatives green (they guard against over-resolution, not absence). Dropping the exact-offer guard makes an_offered_name_is_left_alone go RED (fs.read → read), which is exactly what that test protects.
  • Added tests: a_shared_final_segment_is_not_resolved_arbitrarily (the ambiguity case above) and a_namespaced_atem_call_dispatches_through_the_full_parse_path, which drives a raw <atem:invoke name="glob.glob"> turn through the real parse_assistant_output → align_tool_calls path and asserts the dispatched name is glob.
  • End-to-end note: I could not reproduce the OpenCode/Muse path — no local model emits ATEM namespaced calls — so the end-to-end evidence is the full-parse-path unit test above (the server's actual code path), clearly labelled as such rather than an HTTP round-trip.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

justinchuby and others added 2 commits August 18, 2026 07:54
This package's tool instructions carry a namespaced example —
`example_tool_name.example_function_name` — so a model offered a bare
name sometimes answers with a qualified one. OpenCode rejects the call
outright: "Model tried to call unavailable tool 'glob.glob'. Available
tools: bash, edit, glob, grep, read, write."

The namespace is the model's spelling, not the caller's, and a client
can only dispatch a name it offered. When the qualified name is not on
offer but its final segment is, the call names that tool and is now
resolved to it.

Anything else is left exactly as the model spelled it, so a caller keeps
reporting the unavailable tool it already knows how to report rather
than being handed a target it never offered.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e
…ix ambiguity

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@justinchuby
justinchuby merged commit 39771f3 into main Aug 18, 2026
6 of 14 checks passed
@justinchuby
justinchuby deleted the serverstack-1248 branch August 18, 2026 15:05
justinchuby added a commit that referenced this pull request Aug 18, 2026
CI's format lane had fallen behind — `cargo fmt --all -- --check` was
failing on `main` at `7d669c76`.

The drift came from several changes landing in the same window: the
session-state unification (#1255), the server stack
(#1197/#1249/#1250/#1254), and the native CUDA vision kernels (#1251).

**No logic changes.** This is `cargo fmt --all` and nothing else — 7
source files, plus two `.squad/decisions/inbox` notes that were already
sitting uncommitted in the tree.

Verified locally: `cargo fmt --all -- --check` exits clean on this
branch.

Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com>
@codecov

codecov Bot commented Aug 18, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.92%. Comparing base (5417d04) to head (7acf2a7).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1254      +/-   ##
==========================================
- Coverage   79.95%   79.92%   -0.03%     
==========================================
  Files         360      360              
  Lines      158518   158605      +87     
  Branches   158518   158605      +87     
==========================================
+ Hits       126741   126771      +30     
- Misses      27160    27213      +53     
- Partials     4617     4621       +4     
Flag Coverage Δ
mlas 85.09% <ø> (ø)
offline 79.82% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 12 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

Copy link
Copy Markdown

🔴 Benchmark Regression Detected

Comparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).

ℹ️ Absolute times are informational only — they vary with runner load. The % change column is the reliable signal because both sides ran under identical conditions.

Status Scenario Base PR Change
🔴 matmul/large_generic_f32_threads=8/32x1024x1024 5.61 ms 9.20 ms +64.1%
🔴 sampling_latency/min_p_per_token 214.10 µs 313.45 µs +46.4%
🔴 kv_cache/alloc_dealloc_pages 38.28 µs 53.90 µs +40.8%
⚠️ qwen3_sampling_processors/top_k_top_p_fast 654.04 µs 842.25 µs +28.8%
⚠️ matmul/medium_generic_bf16_threads=8/32x512x512 529.16 µs 676.93 µs +27.9%
⚠️ sampling_latency/top_p_per_token 385.73 µs 477.80 µs +23.9%
⚠️ logit_processing/seven_processor_chain_per_step 326.91 µs 399.48 µs +22.2%
⚠️ block_quantized_matmul_cached_dense/mxfp4_uncached_dequant_each_call/1x1024x1024 705.27 µs 860.23 µs +22.0%
⚠️ qwen3_sampling_processors/top_k_partial_selection 139.16 µs 161.93 µs +16.4%
✅ matmul/large_generic_f32_threads=1/32x1024x1024 11.56 ms 13.26 ms +14.7%
✅ qwen3_sampling_processors/top_k_full_sort_baseline 2.13 ms 2.43 ms +14.3%
✅ qwen3_sampling_processors/top_p_full_sort_after_top_k_baseline 3.51 ms 4.00 ms +13.8%
✅ tokenization/decode_tokens_per_second 6.13 ms 6.95 ms +13.4%
✅ grammar_masking/llguidance_compute_mask/32 93.92 µs 106.48 µs +13.4%
✅ gather/small_bf16_threads=1-internal/4096 537.7 ns 609.4 ns +13.3%
✅ qwen3_sampling_processors/top_k_top_p_full_sort_baseline 5.69 ms 6.34 ms +11.4%
✅ gather/large_f32_threads=1-internal/131072 37.92 µs 42.11 µs +11.0%
✅ tokenization/encode_tokens_per_second 375.66 µs 412.47 µs +9.8%
✅ block_quantized_moe_cached_dense/mxfp4_cached_dense_expert_repeated_call/rows=1,H=256,I=256,E=4,top_k=1 69.05 µs 75.52 µs +9.4%
✅ matmul/large_generic_bf16_threads=8/32x1024x1024 1.57 ms 1.68 ms +7.3%
✅ matmul/large_generic_f16_threads=1/32x1024x1024 105.75 µs 112.87 µs +6.7%
✅ gather/large_bf16_threads=1-internal/131072 14.76 µs 15.41 µs +4.4%
✅ sampling_latency/greedy_per_token 3.45 µs 3.59 µs +4.0%
✅ gather/small_f16_threads=1-internal/4096 513.7 ns 533.3 ns +3.8%
✅ matmul/small_generic_bf16_threads=1/1x256x256 37.54 µs 38.62 µs +2.9%
✅ gather/medium_f32_threads=1-internal/32768 4.64 µs 4.76 µs +2.7%
✅ qwen3_sampling_processors/top_p_fast_after_top_k 567.59 µs 576.69 µs +1.6%
✅ add/medium_f32_threads=1-internal/262144 26.70 µs 26.57 µs -0.5%
✅ matmul/small_generic_bf16_threads=8/1x256x256 39.21 µs 38.70 µs -1.3%
✅ gather/medium_f16_threads=1-internal/32768 2.69 µs 2.65 µs -1.4%
✅ sampling_latency/top_k_per_token 65.39 µs 64.05 µs -2.0%
✅ reduce_mean/large_f32_threads=1-internal/262144 1.09 ms 1.06 ms -2.3%
✅ matmul/small_generic_f32_threads=1/1x256x256 47.50 µs 46.17 µs -2.8%
✅ reduce_mean/medium_f32_threads=1-internal/65536 275.03 µs 267.14 µs -2.9%
✅ add/medium_f16_threads=1-internal/262144 117.61 µs 113.82 µs -3.2%
✅ add/small_f16_threads=1-internal/1024 526.8 ns 501.7 ns -4.8%
✅ add/small_bf16_threads=1-internal/1024 519.7 ns 485.1 ns -6.6%
✅ gather/small_f32_threads=1-internal/4096 816.9 ns 754.7 ns -7.6%
✅ reduce_mean/small_f32_threads=1-internal/4096 17.35 µs 16.00 µs -7.8%
✅ gather/large_f16_threads=1-internal/131072 17.50 µs 16.09 µs -8.0%
✅ gather/medium_bf16_threads=1-internal/32768 3.17 µs 2.86 µs -9.7%
✅ add/medium_bf16_threads=1-internal/262144 120.74 µs 108.77 µs -9.9%
✅ block_quantized_moe_cached_dense/mxfp4_uncached_expert_dequant_each_call/rows=1,H=256,I=256,E=4,top_k=1 461.38 µs 412.57 µs -10.6%
✅ add/small_f32_threads=1-internal/1024 259.5 ns 229.4 ns -11.6%
✅ matmul/medium_generic_bf16_threads=1/32x512x512 622.82 µs 548.83 µs -11.9%
✅ matmul/large_generic_f16_threads=8/32x1024x1024 129.40 µs 112.72 µs -12.9%
✅ add/large_f32_threads=1-internal/4194304 864.55 µs 735.06 µs -15.0%
🟢 matmul/small_generic_f16_threads=1/1x256x256 43.46 µs 36.91 µs -15.1%
🟢 matmul/medium_generic_f16_threads=1/32x512x512 44.23 µs 36.88 µs -16.6%
🟢 matmul/small_generic_f32_threads=8/1x256x256 51.43 µs 42.88 µs -16.6%
🟢 matmul/small_generic_f16_threads=8/1x256x256 46.75 µs 38.08 µs -18.5%
🟢 matmul/medium_generic_f16_threads=8/32x512x512 39.63 µs 31.32 µs -21.0%
🟢 matmul/large_generic_bf16_threads=1/32x1024x1024 2.92 ms 2.31 ms -21.1%
🟢 add/large_f16_threads=1-internal/4194304 2.37 ms 1.82 ms -23.1%
🟢 matmul/medium_generic_f32_threads=1/32x512x512 3.25 ms 2.46 ms -24.4%
🟢 matmul/medium_generic_f32_threads=8/32x512x512 1.88 ms 1.37 ms -26.9%
🟢 add/large_bf16_threads=1-internal/4194304 2.48 ms 1.79 ms -27.7%
🟢 block_quantized_matmul_cached_dense/mxfp4_cached_dense_repeated_call/1x1024x1024 98.79 µs 68.12 µs -31.0%
🟢 block_quantized_matmul_cached_dense/mxfp4_preexpanded_dense_oncelock_like_proxy/1x1024x1024 92.80 µs 55.86 µs -39.8%

Visual flags: ⚠️ ≥ 15% slower, 🔴 ≥ 30% slower — calibrated against measured runner noise (~27% worst-case on multi-threaded matmul)

Host info
CPU: Apple M1 (Virtual)
Cores: 3
OS: Darwin 25.5.0 arm64
Rust: rustc 1.97.1 (8bab26f4f 2026-07-14)
Load avg: { 11.60 5.45 6.37 }
What this cannot catch
  • Regressions in code paths not covered by these benchmarks (e.g., end-to-end decode with a real model)
  • Sub-threshold regressions that compound over multiple PRs
  • Performance changes that only manifest under GPU execution
  • Latency changes in the ORT integration path (these benchmarks exercise the native Rust kernels)

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.

2 participants