Conversation
Remove `image_generation` and `shell` from harmony's `BUILTIN_TOOLS` array. Per the openai-harmony spec, gpt-oss was trained to emit only `web_search_preview`, `web_search`, `code_interpreter`/`container`, `file_search`, plus `mcp` / `function` / `custom` tools. Advertising tools outside that set in the system message leads to undefined model behavior — hallucinated malformed calls, ignored advertisements, or garbled output. Neither `image_generation` (added in T4 #1303) nor `shell` (added in T6 #1342) belongs in gpt-oss's builtin set. The hosted-tool support surface is properly a per-worker capability (what each model was trained for), not a router-level constant. This patch restores clean non-advertisement for the two tools that slipped in; a follow-up introduces per-worker hosted-tool capability flags and makes `BUILTIN_TOOLS` dynamic. Tools outside this set fall through to the existing custom / function-tool path. Multi-turn replay of `image_generation_call` / `shell_call` items continues to reject cleanly at the `responses_to_chat` + `parse_response_item_to_harmony_message` conversion layers. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
|
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)
📝 WalkthroughWalkthroughThe pull request modifies the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request updates the BUILTIN_TOOLS list to align with the tools gpt-oss was trained to emit, specifically removing image_generation and shell. While the update is intended to prevent undefined behavior, it exposes several critical issues: is_builtin implementations are now inconsistent with the updated list, tools removed from the built-in set are being incorrectly filtered out rather than falling through to the custom path, and a logic error in the Responses API incorrectly disables the commentary channel when only built-in tools are used, which prevents the delivery of user-facing content.
There was a problem hiding this comment.
Clean, well-scoped fix. Removing image_generation and shell from BUILTIN_TOOLS is correct — these tools fall outside the gpt-oss training set and advertising them as built-in leads to undefined model behavior. The doc comment clearly explains the rationale and the future direction (per-worker capability flags + MCP dispatch). No issues found.
Summary
Remove
image_generationandshellfromharmony/builder.rsBUILTIN_TOOLS. Per the openai-harmony spec, gpt-oss is trained to emit onlyweb_search_preview,web_search,code_interpreter/container,file_search, plusmcp/function/customtools. The two extras (landed via T4 #1303 and T6 #1342 respectively) advertise hosted tools that gpt-oss was not trained for, producing undefined model behavior.Why this matters
Today's path for a request like
\{model: gpt-oss-120b, tools: [\{type: image_generation\}]}:harmony/builder.rsprojectsResponseTool::ImageGeneration→ string"image_generation".BUILTIN_TOOLS.contains("image_generation")→true→has_custom_tools()returnsfalse→ model is advertised the tool as a builtin in the system message.image_generation_call, it's not dispatched anywhere.responses_to_chatandparse_response_item_to_harmony_messagebothreturn Err("Unsupported input item type"), but that only fires on replay, not first-turn emission.Same story for
shellon gpt-oss.What this changes
BUILTIN_TOOLSshrinks to[web_search_preview, web_search, code_interpreter, container, file_search]— the gpt-oss-native hosted-tool set.image_generationandshelltools fall through to the existing custom/function-tool path instead of being advertised as builtins.ErrforImageGenerationCall/ShellCallitems).Followup (tracked as R0 in the audit plan)
The proper fix is per-worker hosted-tool capability flags (
HostedToolCapabilitiesbitflags or extension ofModelType), populated per model (gpt-oss = WEB_SEARCH_PREVIEW + WEB_SEARCH + CODE_INTERPRETER + FILE_SEARCH + MCP + CUSTOM; OpenAI gpt-5 = all; SGLang Llama = none).BUILTIN_TOOLSbecomes per-requestworker.hosted_tools() ∩ request.tools. Separate PR.Test plan
cargo test -p smg --lib— 621 passed, 0 failed.cargo clippy -p smg --lib --tests -- -D warnings— clean.cargo fmt --all— clean.Summary by CodeRabbit