Repository navigation
test(gpt-oss): post-tool response integrity for harmony fix - #1548
CatherineSue wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds GPT-OSS fixtures (system prompt, nested tool schema, and regex detectors) and a new E2E test that performs a post-tool round trip and asserts the final assistant output is non-empty and contains no malformed residue. ChangesGPT-OSS post-tool validation
Sequence DiagramsequenceDiagram
participant TestClient
participant GPT_OSS_Model
participant ToolRunner
TestClient->>GPT_OSS_Model: send system + user prompt (request action via tools)
GPT_OSS_Model->>TestClient: assistant confirmation or tool_call
alt assistant did not emit tool_call
TestClient->>GPT_OSS_Model: confirmation user turn (ask to proceed)
GPT_OSS_Model->>TestClient: emits tool_call
end
GPT_OSS_Model->>ToolRunner: tool_call(payload)
ToolRunner->>GPT_OSS_Model: tool_result {"ok": true}
GPT_OSS_Model->>TestClient: final assistant message (validated against regexes)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 a new end-to-end test, test_tool_call_arguments_are_compact_json, to verify that tool-call arguments returned to the client are formatted as compact JSON. The review feedback suggests making the test assertion more robust by allowing both Python's default compact JSON format (with spaces after separators) and Rust's ultra-compact format (without spaces after separators) to prevent false positives from minor whitespace differences.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@e2e_test/chat_completions/test_openai_server.py`:
- Around line 529-531: The test builds a canonical JSON string using
json.dumps(parsed) which emits spaces and can mismatch compact args; change the
construction of canonical to use compact JSON by calling json.dumps(parsed,
separators=(',', ':')) (keep any existing sort_keys usage if required) so the
assertion comparing args to canonical uses the same compact formatting; update
the line that defines canonical (currently canonical = json.dumps(parsed)) to
use the separators parameter.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38671555-3ae7-49e6-89f2-3f82a46aa401
📒 Files selected for processing (1)
e2e_test/chat_completions/test_openai_server.py
1b29a51 to
fc8f497
Compare
CatherineSue
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
e2e_test/chat_completions/test_openai_server.py (1)
638-779:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing assertion for canonical compact tool-call arguments (core repro contract).
Line 638 onward adds a residue-focused flow, but it does not assert the compact JSON invariant for
tool_calls[].function.arguments, which is the PR’s stated repro target. This can let the pretty-JSON argument regression slip through undetected.Proposed minimal assertion helper and checks
@@ def test_post_tool_response_has_no_malformed_residue(self, model, api_client): @@ + def _assert_compact_tool_args(tool_calls): + for tc in tool_calls or []: + args = tc.function.arguments + parsed = json.loads(args) # also validates JSON + canonical = json.dumps(parsed, separators=(",", ":")) + assert args == canonical, ( + "tool call arguments must be canonical compact JSON; " + f"got {args!r}" + ) + @@ msg1 = response1.choices[0].message + _assert_compact_tool_args(msg1.tool_calls) @@ msg2 = response2.choices[0].message + _assert_compact_tool_args(msg2.tool_calls)🤖 Prompt for 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. In `@e2e_test/chat_completions/test_openai_server.py` around lines 638 - 779, Add an assertion that the tool call arguments are canonical compact JSON (no pretty-print newlines/extra spaces) for the core repro: for each tool call in tool_call_msg.tool_calls (referencing tool_call_msg and tc.function.arguments), parse tc.function.arguments as JSON to ensure it's valid and assert the original string contains no newline characters or repeated indentation (e.g., no '\n' or two consecutive spaces) so pretty-printed argument regressions fail the test; place this check after the loop that appends the minimal tool result messages and before creating response3.
🤖 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 `@e2e_test/chat_completions/test_openai_server.py`:
- Around line 180-183: The _ENGLISH_SELF_ANALYSIS regular expression is too
broad and will false-positive on benign apologies like "Sorry, we apologize" or
"Sorry the delay" — tighten it by narrowing or removing the generic apology
branch; for example update the pattern in _ENGLISH_SELF_ANALYSIS to remove or
constrain the "Sorry,? (?:we|the)" fragment (e.g., only match "Sorry, we
(must|should|need to|have to|are going to)" or drop the "the" alternative) so it
only flags explicit self-analysis phrases while keeping the rest of the rule/
guideline and assistant anchors intact.
---
Duplicate comments:
In `@e2e_test/chat_completions/test_openai_server.py`:
- Around line 638-779: Add an assertion that the tool call arguments are
canonical compact JSON (no pretty-print newlines/extra spaces) for the core
repro: for each tool call in tool_call_msg.tool_calls (referencing tool_call_msg
and tc.function.arguments), parse tc.function.arguments as JSON to ensure it's
valid and assert the original string contains no newline characters or repeated
indentation (e.g., no '\n' or two consecutive spaces) so pretty-printed argument
regressions fail the test; place this check after the loop that appends the
minimal tool result messages and before creating response3.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9548c180-9824-41f7-8cc6-319b8951a9a4
📒 Files selected for processing (1)
e2e_test/chat_completions/test_openai_server.py
Adds an integration test in TestChatCompletionGptOss (inherited by TestChatCompletionGptOss120B) that drives a multi-turn tool-call flow through the gateway and asserts the post-tool choices[].message.content is non-empty and contains no: - Runs of invisible characters - Long whitespace runs - Ellipsis loops - Leaked English self-analysis markers These are the integrity properties the gpt-oss Harmony round-trip fix in PR #1547 is meant to guarantee. On failure the assertion reports the matched marker for triage. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
fc8f497 to
d7fce2f
Compare
CatherineSue
changed the title
| # runs of invisible characters, ellipsis loops, long whitespace runs, and | ||
| # leaked English self-analysis. Tuned to flag runs (3+) of invisibles so | ||
| # isolated occurrences in normal text do not produce false positives. | ||
| _INVISIBLE_RUN = re.compile("[ ]{3,}") |
There was a problem hiding this comment.
🟡 Nit: This character class contains 8 literal invisible Unicode codepoints (U+200B, U+200C, U+200D, U+2060, U+FEFF, U+00A0, U+2007, U+202F). They render identically in editors, diffs, and code review UIs — you can't tell what's in the class or whether someone accidentally added/removed one.
Using \u escapes makes the pattern auditable and diff-friendly:
| _INVISIBLE_RUN = re.compile("[ ]{3,}") | |
| _INVISIBLE_RUN = re.compile("[ ]{3,}") |
| final_content = response3.choices[0].message.content or "" | ||
|
|
||
| assert final_content.strip(), ( |
There was a problem hiding this comment.
🟡 Nit: If the model responds to the tool results with another tool call instead of text, content will be None/"" and this assertion fires with "must not be empty" — which doesn't explain the actual failure mode. Adding a guard for unexpected tool calls would make CI failures much easier to triage:
| final_content = response3.choices[0].message.content or "" | |
| assert final_content.strip(), ( | |
| final_content = response3.choices[0].message.content or "" | |
| assert not response3.choices[0].message.tool_calls, ( | |
| "expected a text response after tool results, but the model " | |
| "issued another tool call" | |
| ) | |
| assert final_content.strip(), ( |
1 participant
Description
Problem
After a tool round trip on gpt-oss models served through the gateway, the post-tool assistant message in
choices[].message.contentshould be a clean user-facing paragraph, not garbled or self-analysis text. PR #1547 fixes the Harmony round-trip path that can otherwise let malformed residue surface on a successful HTTP 200.There is no integration test today that asserts this end-to-end. The Rust-level unit tests cover the parser/builder pieces individually but do not exercise the property as it would appear to an API client.
Solution
Adds
test_post_tool_response_has_no_malformed_residuetoTestChatCompletionGptOssine2e_test/chat_completions/test_openai_server.py.TestChatCompletionGptOss120Binherits it, so the case runs on both the 20B and 120B targets.The test drives a multi-turn tool-call flow with a style-constrained system prompt, a multi-tool nested-args schema, a confirmation turn, a minimal
{"ok": true}tool result, andreasoning_effort="high". It then asserts that the final-turnchoices[].message.content:\s{6,})The assistant,We need to,We should,We must,According to (rules|guidelines|the spec),Sorry, we|the)These are the integrity properties #1547 is meant to guarantee. On failure the assertion message reports the matched marker plus the offending
final_contentso triage is direct.Changes
test_post_tool_response_has_no_malformed_residuetoTestChatCompletionGptOssine2e_test/chat_completions/test_openai_server.py.Test Plan
pytest -q -k "test_post_tool_response_has_no_malformed_residue" e2e_test/chat_completions/test_openai_server.pyExpected:
main: case may fail on the gpu(2)/gpu(4) targets with anAssertionErrorreporting the matched marker and the failingfinal_content.Once CI on this PR shows the failure mode on
main, the same test will be cherry-picked onto #1547 to confirm it passes there.Checklist
cargo +nightly fmtpasses (no Rust changes)cargo clippy --all-targets --all-features -- -D warningspasses (no Rust changes)Summary by CodeRabbit