fix(volcengine): scope assistant placeholder by endpoint - #1574
Conversation
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe adapter now applies the structured empty assistant-content placeholder only to recognized Volcengine Ark pay-as-you-go endpoints using ChangesVolcengine Ark assistant-content handling
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to The change narrows the Volcengine assistant placeholder behavior to the intended endpoint while preserving the existing contract elsewhere. No actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Exact-head CI is fully green at |
Wibias
left a comment
There was a problem hiding this comment.
The endpoint split is the right direction, but this needs one compatibility fix before merge.
Latest dev now accepts openai-chat baseUrl values that already include a terminal /chat/completions and normalizes that suffix away before sending. This PR classifies Ark by inspecting the raw configured pathname and requiring it to equal exactly /api/v3.
That means a now-valid pay-as-you-go configuration such as:
https://ark.cn-beijing.volces.com/api/v3/chat/completions
is misclassified as non-PAYG and falls back to content: "", even though it is the same PAYG endpoint as /api/v3 after the adapter's URL normalization.
Please normalize a terminal /chat/completions before deciding the Ark endpoint family, and add regression coverage for both:
/api/v3/chat/completions-> same structured placeholder behavior as/api/v3/api/coding/v3/chat/completions-> same empty-string behavior as/api/coding/v3
The branch also predates the current dev, so please rebase after that fix and rerun the relevant CI.
…ngine-coding-content
Summary
Fixes #1571 by separating the two Volcengine Ark Chat endpoint contracts.
The #796 workaround previously matched only the Ark hostname and therefore sent its inferred structured empty assistant placeholder to both:
/api/v3/api/coding/v3Live evidence in #1571 shows that Coding Plan accepts
content: ""and rejects[{"type":"text","text":""}]for a tool-call continuation. This patch keeps the structured placeholder only on recognized Ark hosts at the exact normalized/api/v3base path. Coding Plan and unrelated paths retain the ordinary empty string.Why this shape
Model IDs cannot identify the wire contract because the same model family can appear behind different Ark products. Removing the workaround globally would reopen the still-distinct #796 contract. The base endpoint family is the narrow boundary supported by the available evidence.
The architecture note records this decision and its tradeoffs.
Validation
Exact head
52b1bef7e05aef23711e82cbbc672702943d3c2a:bun test tests/volcengine-ark-assistant-content.test.ts— 8 pass, 0 failbun run typecheck— passbun run privacy:scan— passgit diff --check— passA CPU-limited full-suite run reached this change's focused coverage but later encountered unrelated environment/timeout failures: the ambient
OPENCODEX_API_AUTH_TOKENoverrode an isolated shim fixture, and several existing long-running history/credential-sidecar tests timed out under the two-core limit. The isolated shim test passes when that ambient variable is removed. Exact-head repository CI remains required before merge.Summary by CodeRabbit
Bug Fixes
Documentation