fix(provider): normalize GPT-OSS tool template payloads#429
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This PR introduces model-specific chat-template normalization and Jinja sanitization for GPT-OSS/Harmony and Gemma 4 models; no security mitigations are weakened, but two new attack-surface concerns warrant attention before merge. Trust Boundaries Touched
Threat Assessment
New Attack Surface Not Covered by Existing Threats1. Operator-controlled
The operator has unrestricted write access to
This is a T-012 / T-007 variant not explicitly covered: an operator can manipulate inference behavior (template logic, EOS stops) without touching weight files, remaining invisible to the coordinator's weight-hash check. The weight hash covers Recommendation: Add a 2.
normalized.contains("gpt-oss") || normalized.contains("gpt_oss") || normalized.contains("gptoss")A consumer-supplied The practical impact is limited — the EOS tokens may not resolve in non-Harmony tokenizers ( Recommendation: Gate Open Findings AddressedNone of the tracked SEC-* findings are resolved by this PR. SEC-007 (weight-hash enforcement fail-open) remains open and is now a dependency of the 🔐 Threat model: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56661f01ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| messages: normalizeJinjaMessagesForTemplate(messages, modelId: request.model), | ||
| tools: normalizeJinjaToolsForTemplate(toolSpecs, modelId: request.model), |
There was a problem hiding this comment.
When a GPT-OSS model is served under a local alias that does not contain gpt-oss, the production render still enables Harmony-specific normalization because streamChatCompletion passes the loaded slot's modelType, but this fallback recount only passes request.model. In the missing-usage/cancelled-stream path, the same tool payload that rendered successfully can therefore be re-rendered as non-Harmony; with GPT-OSS schema cleanup skipped, applyChatTemplate can throw and the try? here returns a 0 prompt-token floor, underbilling those requests. Thread the captured slot modelType through promptTokenFloor so this path really mirrors production.
Useful? React with 👍 / 👎.
|
@codex review |
ethenotethan
left a comment
ethenotethan
left a comment
There was a problem hiding this comment.
Automated Code Review — Layr-Labs/d-inference#
Verdict: REQUEST_CHANGES
Security — 1 finding(s)
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/InboundChatNormalization.swift:82— Function name validation only checks for empty after trimming whitespace- Suggestion: Add validation for function name length limits and character restrictions to prevent potential template injection or parsing issues
Performance — 5 finding(s) (2 blocking)
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/InboundChatNormalization.swift:54-101— JSON parsing and serialization in request normalization path- Suggestion: Consider caching parsed JSON or moving normalization to background thread for large payloads
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/InboundChatNormalization.swift:67— Enumerated map creates new array even when no changes needed- Suggestion: Use lazy enumeration or early return when no function_call fields detected
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/GPTOSSHarmonyTemplateFixes.swift:95-173— Recursive schema normalization creates deep object copies- Suggestion: Use copy-on-write or in-place mutation to avoid unnecessary allocations
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/GPTOSSHarmonyTemplateFixes.swift:125-144— Multiple iterations over schema arrays for different union keys- Suggestion: Combine oneOf/anyOf/allOf processing into single loop
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/Inference/MultiModelBatchSchedulerEngine.swift:306-313— ChatTemplateFixContext created multiple times per request- Suggestion: Create context once and reuse across normalization calls
Type_diligence — 13 finding(s) (4 blocking)
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/InboundChatNormalization.swift:58— map[string]any used for JSON manipulation without type safety- Suggestion: Consider using a typed struct for message normalization or add explicit type assertions with ok checks
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/InboundChatNormalization.swift:62— map[string]any used for messages array without type safety- Suggestion: Consider using a typed struct for message normalization or add explicit type assertions with ok checks
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/InboundChatNormalization.swift:67— map[string]any used for individual message without type safety- Suggestion: Consider using a typed struct for message normalization or add explicit type assertions with ok checks
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/InboundChatNormalization.swift:81— map[string]any used for function call without type safety- Suggestion: Consider using a typed struct for function call normalization or add explicit type assertions with ok checks
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/ChatTemplateFixes.swift:23— map[string]any used for messages without type safety- Suggestion: Consider using a typed struct for message representation or add explicit type assertions
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/ChatTemplateFixes.swift:38— map[string]any used for tools without type safety- Suggestion: Consider using a typed struct for tool representation or add explicit type assertions
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/GPTOSSHarmonyTemplateFixes.swift:15— map[string]any used for messages without type safety- Suggestion: Consider using a typed struct for message representation or add explicit type assertions
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/GPTOSSHarmonyTemplateFixes.swift:23— map[string]any used for tools without type safety- Suggestion: Consider using a typed struct for tool representation or add explicit type assertions
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/GPTOSSHarmonyTemplateFixes.swift:78— map[string]any used for tool spec without type safety- Suggestion: Consider using a typed struct for tool specification or add explicit type assertions
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/GPTOSSHarmonyTemplateFixes.swift:82— map[string]any used for function without type safety- Suggestion: Consider using a typed struct for function representation or add explicit type assertions
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/GPTOSSHarmonyTemplateFixes.swift:104— map[string]any used for schema object without type safety- Suggestion: Consider using a typed struct for schema representation or add explicit type assertions
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/Gemma4TemplateFixes.swift:15— map[string]any used for messages without type safety- Suggestion: Consider using a typed struct for message representation or add explicit type assertions
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/Gemma4TemplateFixes.swift:21— map[string]any used for tools without type safety- Suggestion: Consider using a typed struct for tool representation or add explicit type assertions
Additive_complexity — 6 finding(s) (2 blocking)
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/InboundChatNormalization.swift:49-101— Complex legacy function_call normalization could be simplified with helper functions- Suggestion: Extract validation logic into smaller helper functions like validateFunctionCall() and buildToolCall() to reduce cognitive load
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/ChatTemplateFixes.swift:1-120— New abstraction layer with only two implementations may be premature- Suggestion: Consider inlining the GPT-OSS and Gemma4 logic directly into the calling sites until more model families require fixes
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/Inference/GPTOSSHarmonyTemplateFixes.swift:82-174— Schema normalization logic duplicates JSON traversal patterns- Suggestion: Extract a generic JSON schema normalizer that takes transformation rules as parameters to reduce the recursive traversal duplication
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/Gemma4TemplateFixes.swift:1-29— Gemma4TemplateFix is a complete no-op placeholder- Suggestion: Remove this file until actual Gemma4-specific fixes are needed, or add a comment explaining why the placeholder is necessary
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/BatchScheduler.swift:535-571— Model config parsing logic mixed into scheduler- Suggestion: Move modelType parsing to a dedicated config utility class to separate concerns
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/BatchSchedulerTypes.swift:90— Added modelType field increases struct complexity- Suggestion: Consider if modelType could be derived from existing fields or passed separately to avoid expanding the LoadSnapshot struct
25 finding(s) total, 8 blocking. Verdict: REQUEST_CHANGES.
🤖 Automated review by Centaur · DAR-186
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f684700f89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let fixContext = ChatTemplateFixContext(modelId: request.model) | ||
| guard let ids = try? tokenizer.inner.applyChatTemplate( | ||
| messages: sanitizeJinjaMessages(messages), | ||
| tools: sanitizeJinjaTools(toolSpecs), | ||
| messages: ChatTemplateFixes.normalizeMessages(messages, context: fixContext), | ||
| tools: ChatTemplateFixes.normalizeTools(toolSpecs, context: fixContext), | ||
| additionalContext: additionalContext |
There was a problem hiding this comment.
When a GPT-OSS/Harmony model is served under an alias that does not contain gpt-oss, the main render path uses the loaded slot's modelType to enable the Harmony fixes, but this recovery path builds the fix context from request.model only. If a stream is cancelled after output or otherwise misses the usage chunk, promptTokenFloor re-renders the same prompt without the GPT-OSS normalizations and the try? can fall back to 0 for the tool/reasoning payloads this patch is handling, so the completion settles with zero prompt tokens despite having performed prefill work. Thread the loaded model type through this helper so the billing floor matches the production render path.
Useful? React with 👍 / 👎.
| root["messages"] = try messages.enumerated().map { index, value in | ||
| guard var message = value as? [String: Any] else { return value } | ||
| guard (message["role"] as? String) == "assistant" else { return value } | ||
| guard message["tool_calls"] == nil else { return value } |
There was a problem hiding this comment.
When a client serializes optional fields and sends a legacy assistant function_call with tool_calls: null or tool_calls: [], this guard treats the placeholder as an existing modern tool call and skips the conversion. The strict decoder then drops the unknown function_call (or keeps no calls), leaving any following role:function result orphaned, so the Harmony validation/template path still rejects the legacy transcript this normalizer is meant to repair.
Useful? React with 👍 / 👎.
| let fixContext = ChatTemplateFixContext(modelId: request.model) | ||
| let tokens = try tokenizer.inner.applyChatTemplate( | ||
| messages: sanitizeJinjaMessages(messages), | ||
| tools: sanitizeJinjaTools(tools), | ||
| messages: ChatTemplateFixes.normalizeMessages(messages, context: fixContext), | ||
| tools: ChatTemplateFixes.normalizeTools(tools, context: fixContext), |
There was a problem hiding this comment.
For /apply-template calls against a resident GPT-OSS model exposed under an alias that does not include gpt-oss, this context never includes the registry entry's modelType, unlike the production streaming path. The endpoint can therefore render the same tool/reasoning transcript without the Harmony-specific normalizations and fail even though normal inference for that alias succeeds; carry the resolved model type alongside the tokenizer for this utility path before choosing template fixes.
Useful? React with 👍 / 👎.
2 participants
Summary
function_callpayloads into moderntool_callsbefore strict decode can drop them.<|return|>,<|endoftext|>,<|call|>) and bridgereasoning_contentinto the Harmonythinkingfield for template replay.Fixes DAR-344.
Verification
swift test --filter JinjaSanitizationTestsswift test --filter InboundDecodeTestsswift test --filter MultiModelBatchSchedulerEngineTestsswift buildDARKBLOOM_LIVE_MLX_TESTS=1 ... swift test --filter JinjaSanitizationLiveTests/nullBearingToolRequestInfersDARKBLOOM_LIVE_MLX_TESTS=1 DARKBLOOM_LIVE_MLX_GEMMA=1 ... swift test --filter GemmaToolCallLiveTests/realTemplateRendersSingleBraceDARKBLOOM_LIVE_MLX_TESTS=1 DARKBLOOM_LIVE_MLX_GEMMA=1 ... swift test --filter GemmaToolCallLiveTests/realModelEmitsCleanToolCallgit diff --checkNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.