[https://nvbugs/6456085][fix] Harmony: stop discarding malformed tool-call messages silently - #17464
Conversation
…-call messages silently When a harmony tool-call message reaches the non-streaming adapter with a malformed header (no <|message|> token), _strip_incomplete_messages drops everything from the last <|start|> onwards. The remaining analysis message then parses cleanly, so no fallback is triggered and nothing is logged. The response is a valid HTTP 200 with finish_reason "stop", a populated reasoning_content and an empty tool_calls list -- indistinguishable from the model choosing not to call a tool. Callers cannot detect the loss, and the generated tokens are still billed in completion_tokens. Stripping an incomplete trailing message is correct when generation was cut short (e.g. max_tokens). It is not benign when the discarded span carries a stop token or a tool recipient: that message was complete, only malformed. Warn in that case, and log the benign case at debug level so the warning stays meaningful. This makes the failure diagnosable; it does not change which tool calls are recovered. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe Harmony adapter reports discarded tokens from incomplete sequences. It warns for malformed tool calls and logs ordinary truncated messages at debug level. Tests cover valid, malformed, and truncated tool-call sequences. ChangesHarmony diagnostics
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tensorrt_llm/serve/harmony_adapter.py`:
- Around line 1081-1093: Update the discarded-token logging around
_safe_decode_utf8 to remove raw text from both warning and debug messages.
Extract the function recipient from the discarded content, include only the
token count and recipient in the warning, and preserve the debug message with
token count but no discarded text; ensure the warning identifies the dropped
function.
In `@tests/unittest/llmapi/apps/test_harmony_parsing.py`:
- Around line 1341-1346: Update the adapter fixture to stop converting arbitrary
HarmonyAdapter initialization failures into skipped tests. In adapter, either
remove the broad exception handling so construction errors fail normally, or
catch only the specific exception that indicates the test environment is
unavailable while preserving pytest.skip for that case.
- Around line 1409-1430: Update test_truncated_tail_does_not_warn to assert
mock_logger.debug receives a message containing “incomplete trailing harmony
message”, while retaining the no-warning assertion. Narrow the adapter fixture’s
exception handling to only the expected setup exceptions so unexpected
initialization errors fail tests rather than causing skips. Run pytest
tests/unittest/ before merge and verify the added harmony parsing tests remain
covered.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 914b9af4-6954-4f6f-b33e-4762d452bd7b
📒 Files selected for processing (2)
tensorrt_llm/serve/harmony_adapter.pytests/unittest/llmapi/apps/test_harmony_parsing.py
|
PR_Github #64970 [ run ] triggered by Bot. Commit: |
|
PR_Github #64970 [ run ] completed with state
|
…ssage text The warning logged the decoded discarded span, which holds the tool call's arguments -- routinely user data, occasionally secrets. This path fires at warning level in production, so it would persist that data in logs. Inspect the decoded text but never log it: extract the functions.* recipient and report only the token count and the dropped function name, which is what makes the drop actionable. Drop the text from the debug branch too. Also narrow the test fixture: the module-level pytestmark already skips when harmony is unavailable, so catching every exception from HarmonyAdapter() only hid real regressions as skips. Assert the benign path still logs at debug, and assert the warning does not leak tool arguments. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
Thanks @coderabbitai — all three addressed in 1. Do not log the full discarded message text (Major, Security/Privacy) — agreed, and this was the real bug in the patch. The discarded span is the tool call's arguments, so a warning-level path in production would have persisted user data, and potentially secrets, into logs. The decoded text is now inspected but never logged: the 2. Broad 3. Assert the debug path in Re-ran in |
|
Tip For best results, initiate chat on the files or code changes.
The fixes address all three findings. In particular, excluding decoded tool-call arguments from both warning and debug logs prevents sensitive data exposure. The targeted test result is also consistent with the intended behavior. 🐇 ✅ ✏️ Learnings added
You are interacting with an AI system. |
|
The failure on Re-running on /bot run |
2 participants
Description
Reported in GitHub #16377 / NVBug 6456085: with
gpt-oss-120b+ Eagle3 ontrtllm-serve, a tool call is returned as an emptytool_callsarray with emptycontent— HTTP 200,finish_reason: "stop", and nothing in the logs. A caller cannot tell this apart from the model declining to call the tool.This PR fixes the silence, not the upstream corruption (see Caveat below).
Mechanism
HarmonyAdapter._strip_incomplete_messagesremoves everything from the last<|start|>onwards when no<|message|>token follows it:If a tool-call message arrives with a malformed header, its entire span is deleted. The remaining analysis message then parses cleanly, so
_harmony_parsing_failedis never set and the raw-text fallback never runs. The result is a well-formed response that is simply missing the tool call — whilecompletion_tokensstill bills the discarded tokens.Stripping a truncated trailing message is correct when generation was cut short (e.g.
max_tokens). It is not benign when the discarded span carries a stop token or ato=functions.recipient: that message was complete, only malformed. This PR warns in that case and logs the benign case at debug level, so the warning stays meaningful.Test Coverage
tests/unittest/llmapi/apps/test_harmony_parsing.py::TestStripIncompleteMessagesReporting— 3 cases built from canonical harmony tokens (rendered byopenai_harmony, not hand-assembled):test_wellformed_tool_call_is_parsed_without_warningtest_malformed_tool_call_header_is_reportedtest_truncated_tail_does_not_warnVerified as a genuine regression test:
test_malformed_tool_call_header_is_reportedfails on the unmodified adapter and passes with this change. Full file: 3 passed / 49 deselected.Caveat — this does not fix the root cause
Why the harmony header is malformed in the first place is not addressed here, and I was unable to reproduce that half. Attempted repro with the reporter's exact container (
nvcr.io/nvidia/tensorrt-llm/release:1.3.0rc13),gpt-oss-120b+gpt-oss-120b-Eagle3, running the reporter's A/B config unchanged:The repair bot likewise could not reproduce on B200. The reporter is on GB10 / sm_121 (DGX Spark), which neither attempt covers, and uses a different draft checkpoint (
nvidia/gpt-oss-120b-Eagle3-long-context). Both remain untested variables.So the failing input shape here is inferred from the sibling reports (#8615, #10612), not observed directly. That is exactly why the logging matters: with this change, a user hitting it gets a warning naming the dropped function instead of silence, which is enough to confirm or refute the hypothesis from a normal server log.
Related, not fixed here
While investigating I found a second silent path: a message whose
<|channel|>token is missing but whose recipient isfunctions.*is routed tofinal_content, turning a tool call into assistant text — also with no warning. It does not match this bug's signature (contentwas empty, not populated), so I left it out to keep this PR to one concern. Worth a follow-up.PR Checklist
[JIRA/NVBUG/None][type] summaryDev Engineer Review
_strip_incomplete_messagesnow reports discarded trailing tokens._log_discarded_tokenswarns when discarded tokens contain a stop token or ato=functions.*recipient.QA Engineer Review
Added tests:
test_wellformed_tool_call_is_parsed_without_warningtest_malformed_tool_call_header_is_reportedtest_truncated_tail_does_not_warnThe tests are not listed in
tests/integration/test_lists/for CI (test-db/) or manual QA (qa/).Verdict: insufficient. Add the test coverage to the applicable test list, or document why unit-test discovery does not require test-list registration.