fix(api): tighten reasoning_content heuristic to prevent false-positi…#1201
fix(api): tighten reasoning_content heuristic to prevent false-positi…#1201LifeJiggy wants to merge 2 commits into
Conversation
…ve provider configuration leaks (issue Gitlawb#859)
gnanam1990
left a comment
There was a problem hiding this comment.
Nice catch on this — #859 is open and real, and the false-positive you describe is a legitimate problem worth fixing. The diagnosis is right; the matcher just needs a tweak.
The issue: normalizeModelApiName (runtimeMetadata.ts:22) only lowercases/trims — it doesn't strip path prefixes — so switching to startsWith('deepseek') will now miss common prefixed IDs like accounts/fireworks/models/deepseek-v3 or openrouter/deepseek/deepseek-chat. That would drop preserveReasoningContent for exactly the aggregator/custom-endpoint users this fork serves, reintroducing #859 for them — trading one false-positive for a broader false-negative.
Could you use a segment/boundary-aware match (or lean on the explicit vendor descriptors as the primary path and keep the heuristic conservative), plus a regression test covering both the false-positive alias case and prefixed-path IDs? With that this should be a quick approve — thanks for the fix.
|
Addresses review feedback on a269b7a Thanks for the careful read — the startsWith() rewrite was too aggressive and would drop preserveReasoningContent for aggregator paths like openrouter/deepseek/deepseek-chat, exactly the users at risk of #859. Replaced it with a segment-boundary check: This catches "openrouter/deepseek/deepseek-chat" and "accounts/fireworks/models/deepseek-v3" (segment = "deepseek-v3" → startsWith('deepseek') ✅) while ignoring "my-deepseek-rag" (single segment doesn’t start with deepseek) ✅. Same guard applied to kimi/moonshot. No logic elsewhere was touched. Ready for re-review when you have a moment. |
jatmn
left a comment
There was a problem hiding this comment.
Findings
- [P2] Add regression coverage for the segment-boundary heuristic
src/integrations/runtimeMetadata.ts:145
The follow-up implementation now handles the prefixed model IDs from the earlier review, but the PR still changes a fragile fallback heuristic without adding the requested regression tests. Existing coverage exercises descriptor-backed DeepSeek/Kimi routes and a genericgpt-4onegative case, but it does not lock in the two cases this fix is specifically about: a custom alias such asmy-deepseek-ragmust not inferpreserveReasoningContent, while prefixed IDs such asopenrouter/deepseek/deepseek-chatoraccounts/fireworks/models/deepseek-v3still must. Please add focused coverage aroundresolveOpenAIShimRuntimeContextor the OpenAI shim request body so this does not regress back to either the original false-positive or the first follow-up's false-negative.
Summary
What changed — src/integrations/runtimeMetadata.ts lines 145 & 156–158:
Why — preserveReasoningContent: true is the switch that propagates reasoning_content into outbound OpenAI-format request bodies. The heuristic that set that flag used includes() on the lower-cased model name. Any custom endpoint whose apiName happened to contain the substring deepseek (e.g. a user's own my-deepseek-rag alias) silently got preserveReasoningContent: true, which in turn wrote reasoning_content into requests sent to providers that neither expect nor understand the field — causing the crashes described in #859.
Changing to startsWith() restricts the heuristic to official model identifiers (which always take the form deepseek-…, kimi-…, moonshot-…), eliminating the false-positive configuration bleed entirely. Providers still configured via explicit vendor descriptors (the primary control path) are unaffected.
Path tested: Gitlawb/openclaude — origin/fix/859-reasoning-content-fallback
Follow-up: Vendor descriptors for DeepSeek, Moonshot, and Kimi already set preserveReasoningContent: true explicitly; the heuristic only fires for dynamic/custom model paths, so tightening it here has zero impact on the already-correct cases.