feat(providers): add Alibaba Token Plan Responses compatibility - #3255
feat(providers): add Alibaba Token Plan Responses compatibility#3255MoonOld wants to merge 3 commits into
Conversation
|
@Astro-Han Could you review the compatibility architecture when convenient? Current main owns parsing/serialization through @ai-sdk/open-responses; Maka adds typed request policies and bounded durable summary identity for Alibaba while keeping DeepSeek content-only. The automated contract and negative-path coverage is in place. A protected Token Plan rerun with retained evidence and the requested real TUI screenshot are still pending. |
|
Review follow-up complete on final head 41a1047. Four independently identified boundaries are now covered: header-only request customization cannot bypass store:false/tool-choice policy; legacy and foreign-profile reasoning degrades without blocking; multiple and empty reasoning items retain identity and summary-part boundaries; and streamed reasoning must equal the provider's final summary before durable metadata is attached. A mismatch flushes visible partial text without makaResponses and a later Turn remains usable. DeepSeek is explicitly unchanged from #2972: content-only replay, no new item ID or durable state. Three independent subagent reviews now report no remaining P0-P2. Local evidence: Core 557 passed, focused Runtime 263 passed, provider matrix/thinking 168 passed, root build passed; full Runtime recorded 2,962 passed / 12 skipped with one unrelated PTY 10-second timeout reproduced in isolation. The PR and #3162 descriptions have been updated to match the final behavior. |
|
@Astro-Han The final review follow-ups are now on head 41a1047. DeepSeek has been restored exactly to main's content-only behavior, while Alibaba's request policy, profile-isolated durable replay, multi/empty item handling, and failure-safe summary verification now have focused regression coverage. Could you review this final architecture when convenient? GitHub still requires maintainer authorization before the external-fork Actions can start. |
|
Reviewing! Thanks for asking~ |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the final architecture update. I re-reviewed this exact head and found one P2 in the reasoning-item failure path, added inline below.
The provider routing, final store: false override, profile isolation, durable summary validation, and AI provenance otherwise look sound. Approval also needs one author-provided screenshot from the actual user-facing surface: please attach a real terminal or Desktop capture showing qwen3.8-max reasoning followed by a Maka-owned tool continuation. Text-only live verification is not enough for this UI/UX gate.
The remaining exact-head CI gates are still pending, so this is not merge-ready yet.
AI-assisted review disclosure: OpenAI Codex performed the exact-head architecture, failure-recovery, provenance, UI-evidence, and CI analysis; I verified the reproduction, severity, smallest fix, and live GitHub state before posting.
中文说明
当前有一个 P2：第二个 reasoning item 的非法 ID 会让它的无 ID delta 追加到已完成的第一个 item，最终把持久化 summary 状态写坏，导致下一轮在发请求前失败。另请作者提供一张真实 terminal 或 Desktop 截图，展示 qwen3.8-max reasoning 后继续执行 Maka-owned tool；文字说明不能替代 UI/UX 截图。CI 也仍需全绿。
| } | ||
| part.text += event.text; | ||
| } else if (stepResponsesThinkingParts.length > 0) { | ||
| stepResponsesThinkingParts.at(-1)!.text += event.text; |
There was a problem hiding this comment.
[P2] Keep an invalid next reasoning item from corrupting the finalized item
If item A has already finalized with makaResponses.summaryPartLengths and item B then supplies an ID rejected by safePlaintextResponsesReasoningItemId (for example, over 512 characters or containing C0), B's deltas arrive without an item ID and this branch appends them to A. B's reasoning-end fails, but the partial flush can persist A's old lengths with A+B text; the next turn then fails in reconstructSummaryParts() before provider dispatch. Please start a new undurable part when an idless delta follows a part with finalized provider metadata (or clear that durable metadata before the partial-error flush), and add a valid-A + invalid-B + next-turn recovery regression.
中文说明
已完成的 A 带有持久化长度元数据；非法 ID 的 B 会走无 ID 分支并把文本追加到 A。随后错误路径会保存“A 的旧长度 + A/B 合并文本”，下一轮重建时长度不一致，整个会话无法继续。最小修复是在已 finalized part 后遇到无 ID delta 时新建不带 durable metadata 的 part，并补恢复回归。
There was a problem hiding this comment.
Fixed in 34bed03. When an idless delta follows a finalized plaintext-summary item, Runtime now starts a separate undurable part instead of appending to the finalized item. The exact regression covers valid A → unsafe-ID B → provider error/partial flush → next-Turn recovery: A retains its original text and makaResponses boundary state, B persists without provider metadata, and the recovery prompt replays only A. Focused Runtime/Responses coverage is 264/264 green; Biome and diff checks pass.
|
Addressed Astro-Han’s P2 inline finding on head 34bed03. The finalized item can no longer be corrupted by deltas from a following unsafe-ID item, and the new restart/recovery regression proves the failed partial Turn does not brick the next request. The remaining reviewer request is the author-provided real Terminal/Desktop screenshot; code-side focused verification is green. |
|
Correction to this correction: the credential owner has clarified that the supplied key is a Token Plan key; the brief Coding Plan classification was incorrect. The earlier temporary run still retained no raw HTTP artifact or request ID, so it remains withdrawn as merge evidence until the reproducible Token Plan probe and requested TUI screenshot complete. |
|
Protected credential/routing probe completed on 2026-08-20 through the branch model factory. Coding Plan Chat returned HTTP 401 ( |
|
Real Maka TUI verification on head 34bed03 using Alibaba Token Plan China with qwen3.8-max: streamed reasoning, a Maka-owned Read tool continuation, and the final answer completed successfully. |
|
@Astro-Han The P2 reasoning-item isolation finding is fixed on head |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at 34bed03f. This does not work as written, and the reason is a single unverified assumption about the wire contract.
provider-registry.ts declares reasoningReplay: 'plaintext-summary' for the Alibaba Token Plan provider, which commits the adapter to receiving reasoning as a summary-channel event. The vendored @ai-sdk/open-responses@2.0.28 in this tree only handles response.reasoning_text.delta — the content channel — and emits nothing on the summary channel at all. Because ai-sdk-backend.ts:2394 checks the carrier with strict equality and throws on mismatch, every turn that produces reasoning ends in an error rather than a degraded rendering. That is a P0: the feature's primary path is the path that fails. The fix is either to declare the carrier the vendored SDK actually produces, or to verify against a live endpoint that the summary channel is emitted and pin the SDK version that does so.
Architecturally, the PR builds three things that already exist one seam over: a second provider-options key resolver alongside openAiCompatibleProviderName, a second body-rewriting fetch alongside request-customization-fetch.ts, and a STATE_VERSION that does not match the one sessions were written with. The first two are duplication that will drift; the third silently strands existing sessions. None of them is hard to fold back into the existing seam, and doing so now is much cheaper than after this ships.
Reviewed with Claude Opus as an analysis assistant. The carrier finding is confirmed by reading both this head and the vendored SDK source; it is not reproduced against a live Alibaba endpoint, and that verification is exactly what I am asking for. Reproduction status is stated per finding.
| name: 'provider', | ||
| responses: { | ||
| adapter: 'open-responses', | ||
| reasoningReplay: 'plaintext-summary', |
There was a problem hiding this comment.
[P0] Verify the reasoning carrier before declaring it, or declare the one the SDK actually emits. reasoningReplay: 'plaintext-summary' commits the adapter to the Responses summary channel, but the vendored @ai-sdk/open-responses@2.0.28 in this tree handles only response.reasoning_text.delta — the content channel — and never emits a summary event. ai-sdk-backend.ts:2394 compares the carrier with strict equality and throws on mismatch, so every Alibaba Token Plan turn that produces reasoning terminates in an error instead of rendering. Confirmed by reading this head and the vendored SDK source; not reproduced against a live endpoint, which is the verification this line needs. Either set the carrier to what the pinned SDK produces, or capture a live transcript showing the summary channel and pin the SDK version that parses it. Regression test: a recorded Alibaba reasoning stream replayed through the adapter, asserting a rendered turn rather than a throw.
| safePlaintextResponsesReasoningItemId(streamItemIdValue); | ||
| const summaryParts = plaintextSummaryParts(provider); | ||
| if (!itemId || !summaryParts) { | ||
| throw new Error('Plaintext Responses reasoning item is missing final summary metadata'); |
There was a problem hiding this comment.
[P1] Do not throw on a reasoning-end that carries no providerMetadata. The SDK's flush() path emits reasoning-end with no metadata attached whenever the stream terminates early — including when the provider itself failed mid-turn — so this hard throw fires during the cleanup of an already-failing request and replaces the provider's real error with a generic plaintext-reasoning message. The user sees a misleading cause and the actual failure is lost. Confirmed by reading code at this head and the SDK's flush path; not executed. Treat missing metadata on reasoning-end as a no-op and let the underlying error surface. Regression test: abort a reasoning stream mid-flight, assert the surfaced error is the transport error and not this message.
| }; | ||
| } | ||
|
|
||
| function responsesProviderOptionsKey( |
There was a problem hiding this comment.
[P2] Reuse openAiCompatibleProviderName instead of adding a second provider-options key resolver. responsesProviderOptionsKey reimplements the same provider-to-key mapping that openAiCompatibleProviderName already owns, so a provider added to one will silently take the wrong options path in the other — and the failure is a silently ignored provider option, not an error. Confirmed by reading code at this head. Derive the key from the existing resolver. Regression test: add a provider to the existing mapping only, assert the Responses path picks up the same key.
| return body; | ||
| } | ||
|
|
||
| async function parseJsonBody(request: Request): Promise<Record<string, unknown>> { |
There was a problem hiding this comment.
[P2] Route this body rewrite through request-customization-fetch.ts rather than a second fetch wrapper. parseJsonBody plus the surrounding wrapper reimplements request-body interception that the existing customization fetch already performs, which means two layers can now rewrite the same request with no defined ordering between them, and a future change to one leaves the other stale. Confirmed by reading code at this head. Express this compatibility shim as a customization within the existing seam. Regression test: a request that both layers would touch, assert a single well-defined resulting body.
| ) { | ||
| return undefined; | ||
| } | ||
| throw new Error('Malformed durable plaintext Responses reasoning state'); |
There was a problem hiding this comment.
[P2] Reconcile STATE_VERSION with the version existing sessions were written at, or migrate them. The durable plaintext-reasoning state is read back under a version that does not match what previously-persisted sessions carry, so this throw makes those sessions permanently unresumable — the user's only recovery is to abandon the conversation, and nothing in the error says so. Confirmed by reading code at this head; not executed. Either keep the version stable and tolerate the older shape, or add an explicit migration that upgrades stored state on read. Regression test: persist a session at the pre-PR version, resume it at this head, assert it resumes.
| case 'force-store-false': | ||
| body = { ...body, store: false }; | ||
| break; | ||
| case 'reject-forced-tool-choice': { |
There was a problem hiding this comment.
[P3] Remove the reject-forced-tool-choice branch or wire it up. Nothing in this PR ever produces that compatibility mode, so the case is unreachable as merged; leaving it in place suggests a behaviour the build does not have. Confirmed by reading code at this head. Delete it, and reintroduce it with its call site when it is actually needed.
|
@Astro-Han Thanks for the exact-head review. I rechecked The P0 premise does not match the pinned adapter's two-stage stream mapping:
In this PR, The real Maka TUI run on this exact head completed streamed reasoning, a Maka-owned Read continuation, and the final answer. That path cannot complete if the final summary metadata is absent or differs from the streamed text. Both On the three architecture points:
Given the green exact-head CI, approval, pinned adapter, and live strict-summary evidence, I do not see a code change required for these findings. If you have a concrete Review assistance disclosure: OpenAI Codex rechecked the exact head, pinned npm tarball, and PR commit history; I reviewed and approved this response. |
|
Architecture follow-up before the next head update: the new Alibaba Token Plan wire selection, plaintext-summary replay contract, and request compatibility modules ( |
|
Follow-up completed locally for the next head update: (1) the conformance matrix now resolves reasoning replay through the effective Runtime adapter and binds both |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
COMMENT. My earlier P1 and P3 are properly fixed; the P0 has moved rather than been answered, and the way it moved introduces a new architecture problem that I think is the most important thing in this round.
Fixed, and well:
- P1, the throw on a metadata-less
reasoning-end.isUnfinalizedPlaintextSummaryReasoningEnd(model-adapter.ts:869-879) now detects exactly the SDK'sflush()trailer and:364-372defers the decision to the terminal outcome, so an existing provider failure wins while a clean stream still fails closed rather than silently losing replay state. That is a more precise fix than the one I asked for, and the comment explains why. - P3, the unreachable
reject-forced-tool-choicebranch. It is now reachable —ALIBABA_TOKEN_PLAN_RESPONSES.compatibilitylists it.
Still open from the previous head, and unchanged by this one: the second fetch wrapper in open-responses-compatibility.ts (parseJsonBody plus a wrapping fetch) rather than routing the body rewrite through request-customization-fetch.ts, and the duplicate provider-to-key resolver in model-runtime.ts alongside openAiCompatibleProviderName. Both are P2 and both are now more clearly worth doing, because the new provider-runtime-policy.ts makes three separate places where this provider's special-casing lives. On the STATE_VERSION P2 I will hold rather than restate it — the durable state moved into a new responses-reasoning-state.ts at version 1 and I could not re-derive at this head whether any previously written session reads back under a different version. Please say either way in the PR body.
Two P1s inline. To be explicit about what would move me: for the second one, a captured qwen3.8-max Responses body showing the reasoning on the reasoningSummary channel would close it outright and is probably ten minutes of work. For the first one, putting the contract back in the registry — verified — closes it too, so the two are really one decision.
AI disclosure: this review was assisted by Claude (Opus) for code search and cross-checking. Everything above I re-derived myself against the source at b625a699c.
| | Exclude<ProviderRuntimeAdapter, { kind: 'openai-compatible' }> | ||
| | OpenAiCompatibleRuntimeAdapter; | ||
|
|
||
| const ALIBABA_TOKEN_PLAN_RESPONSES = { |
There was a problem hiding this comment.
[P1] This is a second declaration authority for provider capabilities, shadowing the registry. The previous head declared reasoningReplay: 'plaintext-summary', the two compatibility modules, and the qwen3.8-max Responses routing in packages/core — provider-registry.ts and model-metadata.ts, the seam the whole codebase reads. This head deletes all three from core and re-declares them here: ProviderResponsesCompatibilityModule verbatim, plaintext-summary re-added to a runtime-local RuntimeProviderResponsesContract, and openAiAdapterApiProtocol's Alibaba branch reborn as defaultOpenAiApiProtocol wrapping the core function. resolveRuntimeProviderAdapter then overrides whatever the registry declared for these two provider types at model-construction time (model-runtime.ts:140). Two consequences. Anything reading PROVIDER_DEFAULTS[...].runtimeAdapter.responses — settings, capability display, any future consumer — now sees a contract the runtime does not actually use, with nothing at the boundary saying so. And the core test that pinned the routing rule (model-metadata.test.ts's "routes only Qwen3.8 Max through Alibaba Token Plan Responses") was deleted rather than moved, so the rule lost its coverage at the level where the rule lives. AGENTS.md asks for the closest existing seam rather than a parallel path, and the registry is that seam. If the concern was keeping an unverified carrier out of the published contract, the answer is to verify it, not to relocate it. Confirmed by reading the source at this head. Regression test needed: whatever replaces the deleted model-metadata.test.ts case, asserting the protocol routing at the level that owns it.
|
|
||
| const ALIBABA_TOKEN_PLAN_RESPONSES = { | ||
| adapter: 'open-responses', | ||
| reasoningReplay: 'plaintext-summary', |
There was a problem hiding this comment.
[P1] The substance of my earlier P0 is still open — this declares the Responses summary channel and nothing in the PR establishes that the vendored @ai-sdk/open-responses emits it for this provider. Moving the declaration out of packages/core did not answer the question; it only changed which file makes the claim. Everything downstream is built on it: plaintextSummaryParts (model-adapter.ts:881-893) accepts only reasoningSummary entries shaped { type: 'summary_text', text } and returns undefined for anything else, and model-adapter.ts:837 then throws Plaintext Responses reasoning item is missing final summary metadata for every finalized item. So if the SDK actually delivers this provider's reasoning on the content channel rather than the summary channel, the failure mode is not degraded replay — it is a thrown error on the normal path. The tests I can see all construct the summary shape themselves, so they confirm the handler, not the premise. A captured response body from a real qwen3.8-max Responses call showing the reasoningSummary array would settle it; short of that, plaintext-content is the safe declaration. Confirmed by reading the source at this head. Regression test needed: a wire-level fixture recorded from the provider, not a hand-built chunk.
|
Final review/fix loop update on pushed head |
|
@Astro-Han I updated the PR body with direct provider evidence for the two remaining architecture/carrier questions. Alibaba official Responses documentation defines reasoning items with required |
|
Occam/razor cleanup pushed on |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at b0d4362fe. I started this round on fc20601c0, the head moved while I was verifying, and I re-derived every finding against the new head rather than carrying anything forward. I dispatched four scoped sub-reviews — security, correctness and resource bounds, integration, simplification and test quality — and used them as leads only; everything published below I confirmed myself by reading this head.
Four things are fixed, two of them the way I would have asked for.
The second fetch wrapper is gone. open-responses-compatibility.ts is now a fourteen-line body finalizer plugged into the existing request-customization-fetch.ts seam instead of a parallel wrapper with its own request reconstruction — that is exactly the right shape, and it removes the behavioural divergence I was about to file (the old copy hard-threw on GET/HEAD/null-body/non-JSON requests where the established wrapper passes them through). The carrier dimension is also gone: plaintext-summary no longer carries a carrier discriminant, which deletes a union arm no code path could produce. The reasoning-end-without-metadata throw is replaced by a deferral, and the yield in the finally does deliver the deferred error because the generator body completes normally there. reject-forced-tool-choice is now genuinely declared.
I have to correct my own P1 about the declaration split. Two of the three things I offered as evidence do not hold, and I would rather retract them than have you refute them one at a time:
- I said the deleted core routing test was not moved. It was, and it is stronger than what it replaced:
__tests__/responses-wire-contract.test.tsasserts both Token Plan providers routeqwen3.8-maxto Responses, thatqwen3.7-maxstays on chat, and that an account-declaredapiProtocoloverride wins. The rule is covered at the layer it now lives in. - I said settings and capability display would see a contract the runtime does not use. There is no such consumer. Outside
provider-registry.tsitself andpackages/runtime/, nothing in the repo readsruntimeAdapter— not desktop, not the CLI. The divergence is latent, not observable today.
What survives is one thing, and I still think it matters: one fact has two declaration sites, and the core side states the opposite of the truth rather than a stale version of it. packages/core/src/provider-registry.ts gives both Alibaba Token Plan providers a runtimeAdapter with no responses at all, and ProviderResponsesContract has no plaintext-summary member, so core's declared position is "this provider does not support Responses" while the runtime routes it there. Relatedly, the docstring at packages/core/src/model-metadata.ts:94-98 now asserts two things that are false at this head — it calls itself the single declared source of the protocol split, and names the runtime model factory and the conformance matrix as its consumers, both of which have moved to defaultOpenAiApiProtocol. Whichever way the layering decision goes, that paragraph has to change.
The architectural question I would like a maintainer to settle, because it outlives this PR: are we adopting "core declares catalog facts, runtime declares execution policy, and runtime policy may add capabilities core says are absent" as a layering? Today it is neither documented nor annotated at runtimeAdapter. A smaller instance of the same question: model-protocol.ts adds reasoningItemId and reasoningSummaryText to the generic thinking event, filled only by the plaintext-summary path. Extending our own protocol is legitimate, but it sets the precedent that every new provider carrier adds fields to the shared event, and that is worth accepting or declining explicitly rather than by default.
For the record on the Runtime Host boundary: this PR does not touch packages/runtime-host at all, and settleModelStepOutcome decides a model step's terminal state, a kind already produced on main. The P2 below is intent bundling, not an authority violation.
One open thread I re-confirmed rather than re-filed, with a sharper mechanism than I first gave: the STATE_VERSION trap is worse than described. The profile is parsed before the version check, so a same-profile version mismatch decodes as malformed with a matching profile, misses the graceful return undefined in materializeRuntimeReplayPlan, and falls through to the throw. A future version bump therefore bricks every session written at version 1. It should degrade the way a profile mismatch already does — that is a two-line change and it is the difference between a migration and a data loss.
Carrying a P1 → COMMENT, not approval.
AI disclosure: reviewed with Claude Code, including four scoped sub-reviews used as leads. Every finding published here I re-derived at b0d4362fe myself — I read the pinned @ai-sdk/open-responses@2.0.28 stream mapping, the decode and replay paths, and the finish-classification diff, and I ran the repo-wide enumeration that retracts my two earlier evidence claims. Sub-review findings I could not confirm at this head are not included.
| (candidate.providerOptions?.openai as { itemId?: unknown } | undefined) | ||
| ?.itemId === itemId, | ||
| ); | ||
| let part = stepResponsesThinkingPartsByItemId.get(itemId); |
There was a problem hiding this comment.
[P1] Guard this id-keyed branch against a delta that arrives after its item was finalized, the way the id-less branch below already is. When a reasoning-delta carries an itemId this map already resolves to a part whose providerOptions hold a finalized makaResponses state, the branch appends event.text to part.text and leaves summaryPartLengths untouched; deltas carry no reasoningSummaryText, so the mismatch check immediately below never runs. flushStep then persists text of length |T|+|X| against lengths summing to |T|, and on the next turn reconstructSummaryParts in responses-reasoning-state.ts throws Durable plaintext Responses reasoning summary boundaries do not match text from inside materializeRuntimeReplayPlan; nothing between there and provider dispatch catches it and the persisted RuntimeEvent is immutable, so every later turn in that session fails before reaching the provider. The id-less branch a few lines down handles exactly this hazard correctly — it decodes the last part and starts a fresh one when the state is valid — so the fix is to apply that same decode here and start a new part, or drop the durable metadata, instead of reusing the finalized one. Evidence: confirmed by reading code at this head; reachability depends on the provider emitting a stray or duplicated response.reasoning_text.delta after response.output_item.done, which the pinned SDK forwards unconditionally (@ai-sdk/open-responses@2.0.28, dist/index.js:803-808 tracks no finalization), so nothing in the stack defends it. Regression test: reasoning-start A → delta → reasoning-end A(metadata) → delta A → finish, assert the persisted part is either split or stripped of makaResponses, then assert the following turn replays instead of throwing.
| if (finishReason === 'content-filter' || finishReason === 'error') { | ||
| const terminalFailure = | ||
| finishReason === 'error' | ||
| ? providerFinishFailure(rawFinishReason) |
There was a problem hiding this comment.
[P2] Split this finish-reason reclassification out of the provider-compatibility PR, or at minimum cover it. On main a step with finishReason === 'error' is unconditionally terminal-failure carrying a non-retryable provider_unavailable; here it runs through providerFinishFailure → normalizeProviderFailure, and the outcome kind becomes terminalFailure.retryable ? 'retryable-failure' : 'terminal-failure'. That is a retry-behaviour change for every ai-sdk provider in the tree, not only Alibaba: any provider whose raw finish reason normalizes to a retryable code is now retried where it previously failed terminally. The PR body says the retry policy is unchanged — true of the policy, false of the classification fed into it. Under the one-revertable-intent rule that is the problem: reverting the Alibaba work would also roll back a cross-provider failure reclassification that has nothing to do with it. The content-filter arm is unaffected, since modelStepFailure always sets retryable: false; only the error arm moves. Evidence: confirmed by reading code at this head, not executed. Regression test: a non-Alibaba adapter finishing with finishReason: 'error' and a raw reason that maps to a retryable code, asserting both the outcome kind and the retry decision — the new retryable-failure arm has no test today, and the one test on this path asserts terminal-failure.
|
|
||
| const ALIBABA_TOKEN_PLAN_RESPONSES = { | ||
| adapter: 'open-responses', | ||
| reasoningReplay: 'plaintext-summary', |
There was a problem hiding this comment.
[P3] Record which wire event this declaration actually depends on, and cover it with a raw-SSE fixture. This corrects my own earlier P1 on this line: I claimed nothing established that the summary channel carries reasoning here, and having now read the pinned SDK myself I no longer think that premise is in doubt — @ai-sdk/open-responses@2.0.28 builds reasoning-end's reasoningSummary from item.summary unconditionally (dist/index.js:883-895), so any provider populating summary on response.output_item.done satisfies plaintextSummaryParts. What survives is narrower and worth stating in a comment: the streamed text and the durable lengths arrive on two different channels. Deltas reach you through response.reasoning_text.delta (dist/index.js:803) while the lengths come from item.summary, and the SDK has zero handling for response.reasoning_summary_text.delta — 0 occurrences in dist/index.js, typed but unhandled in its source. So this contract holds only because Alibaba emits summary content on the content-channel delta event. If it ever moves to the standard summary delta, the streamed text is empty while the final summary is not, and the mismatch check in ai-sdk-backend.ts throws on every reasoning turn — a hard turn failure, not degraded replay. Evidence: confirmed by reading the pinned SDK at this head; the provider-side behaviour is inference from its published Responses shape, not executed here. Regression test: a wire-level fixture driving the real SDK from raw SSE frames rather than hand-built post-SDK chunks, asserting the streamed text equals the concatenated summary.
| runtime?: ResolvedModelRuntime, | ||
| ): ModelStreamEvent[] { | ||
| switch (chunk.type) { | ||
| case 'reasoning-start': { |
There was a problem hiding this comment.
[P3] Delete the now-dead second case 'reasoning-start' further down in this switch. That label is still grouped with start-step/tool-result/tool-error and their providerExecuted/toolCallId guard, but this new case precedes it, so the old arm is unreachable. Behaviour is unchanged today only because both returned [] for this chunk type — which is precisely why it will not be noticed later: TypeScript does not diagnose duplicate case labels, and biome.jsonc runs preset: none with noDuplicateCase off, so neither gate reports it. Evidence: confirmed by reading code at this head. No regression test needed; this is a deletion.
|
@Astro-Han @jackwener Exact-head follow-up is pushed at The latest review findings are addressed:
The earlier invalid-ID isolation, metadata-less trailer settlement, shared request-customization seam, single Runtime provider-name authority, forced-tool-choice declaration, and real TUI evidence remain intact. Core still has zero diff. Runtime now explicitly documents that its provider policy is layered over Core's catalog-level adapter default and that account Current-main integration also exposed three stale legacy Verification on the pushed tree: root |
|
Hi — this PR conflicts with current I tested a rebase onto current
These are real source conflicts, so they need your judgement rather than a mechanical rebase — please rebase onto current Thanks for the contribution — happy to help if any conflict is unclear. AI-assisted maintenance note, not a review. It does not count as the required human review under |
d84bfc9 to
41427b5
Compare
|
@Astro-Han @jackwener Rebase requested in the maintenance note is complete on head The three conflicts were resolved semantically:
Core and Runtime Host both have zero diff. Verification on the rebased tree: root The new exact-head Actions run again needs external-fork authorization: https://github.com/apache/maka/actions/runs/32645781601. Once it is green, this head is ready for the promised review pickup. |
41427b5 to
20ef5f6
Compare
|
@Astro-Han @jackwener The requested repair is complete on exact head
Local verification on the rebased tree: ASF header check, lint, format, root build, and root typecheck passed; Runtime full suite passed 3,016 with 13 skipped and 0 failed. The remaining workspace suites passed. Eval's 75 Node tests passed; its Python suite requires Python 3.10+ (macOS system Python 3.9 cannot parse The new exact-head CI run is |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
逐条核对 current head 20ef5f6 上的 7 条存活 finding（isOutdated=false）：
- provider-runtime-policy.ts:53 P1 未修：第二 capability 权威遮蔽 registry，resolveRuntimeProviderAdapter 覆盖 registry 的 responses 契约
- provider-runtime-policy.ts:58 P1 未修：Responses summary 独立 policy 未回 registry
- ai-sdk-backend.ts:2314 P1 未修：id-keyed finalize 后 delta 仍可追加
- model-adapter.ts:807 P1 已修：reasoning-end 无 metadata 改为 defer
- ai-sdk-backend.ts:3801 P2 悬置：STATE_VERSION 迁至 responses-reasoning-state.ts v1，旧会话兼容性待作者说明
- model-adapter.ts:578 P2 部分修：fetch wrapper 已并入 request-customization-fetch，duplication 仍存
- model-adapter.ts:907 P3 部分修
门禁：test success, mergeable true，卡点为上述未闭合 finding，非 CI。
Generated-by: OpenAI Codex
20ef5f6 to
850ae74
Compare
|
@Astro-Han @jackwener I agreed with the surviving declaration-authority finding and reworked the architecture on exact head Proposed boundary for maintainer review:
I also rechecked the other surviving threads against this head:
Exact-head local gates: clean build, full typecheck, ASF headers, lint, format, full Could you review whether this protocol-reference / execution-profile split closes the authority concern, and recheck the late-delta thread against the current guard and regression? |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
#3255 850ae74 — review (bind exact head)
Gate: CI test success (run 32683983955). Sampling basis noted.
Scope: Alibaba Token Plan Responses compat — provider-runtime-policy, model-adapter, reasoning state, contract matrices.
Sampled checks (not full coverage):
- Request/response field mapping sampled: Responses providerOptions and reasoning state wiring appear scoped to Alibaba token plan, not global.
- Error code classification sampled: no broad retry-policy override observed in sample.
- Abstraction reuse: new
provider-runtime-policy.ts+responses-reasoning-state.ts— appears dedicated to this provider surface, not obvious duplication of existing Responses abstraction but full duplication check not completed in sample.
Limit: 26 files / 2503 lines — this review is sampled, not exhaustive; recommend second pass on contract matrices.
Verdict: COMMENT — no P0-P2 in sample, content tentatively GO within sampled scope, gate green.
中文
抽样检查未见阻断，26文件量大建议二次全量。
There was a problem hiding this comment.
Full-pass review at exact head 850ae74a91077d83a6cc37998de15f5e373cb908 (25 files, +2503/-116, MERGEABLE). I read every production file in full; the three open questions get full-coverage answers below, plus one finding.
The three open questions
1. Duplication vs the existing Responses abstractions (provider-runtime-policy.ts, responses-reasoning-state.ts) — no duplicate authority.
The split is a deliberate seam, not a parallel system: provider-registry.ts gains ProviderRuntimeProfileId where Core owns only that a runtime profile exists, and OpenAiCompatibleRuntimeAdapter makes responses and runtimeProfile mutually exclusive at the type level. Runtime's provider-runtime-policy.ts is the only resolver of profile → concrete contract, and responses-wire-contract pins that Core references and Runtime implementations match exactly (Core Runtime-profile references exactly match Runtime implementations). responses-reasoning-state.ts is the first durable plaintext-summary state carrier — the pre-existing plaintext-content mode stays untouched in place, and the new plaintext-item kind joins the existing ModelAdapterRuntimeEventReplaySupport union rather than forking it. The reasoning-item reader (responsesReasoningItemId) even keeps reading the legacy openai.itemId shape. This is extension along the existing seam.
2. Request/response field mapping — internally consistent and wire-pinned; provider-truth caveat below.
The wire-contract tests drive real doGenerate through the SDK against a mock fetch and assert the exact body: store: false survives a hostile requestBodyOverlay: { store: true } (the finalizer runs after caller overlays — correctly, since provider policy must win), reasoning.effort is preserved, and the replayed reasoning item is {type:'reasoning', id, summary} with no content field. tool_choice: 'required'/object shapes throw before dispatch. What I could not verify from here: that this matches Alibaba Token Plan's live API — I have no credentials for that endpoint, and the tests pin the SDK's own translation, not the provider's answers. The raw-SSE contract tests reference observed provider frames; I'm taking the pinned frames as the author measured them.
3. Error classification — no retry-policy widening. The one touched path (settleModelStepOutcome + providerFinishFailure) preserves the raw finish reason's classification for diagnostics but forces retryable: false, and the outcome kind follows that — so nothing retryable and non-retryable got mixed, and no provider error became newly retryable. The comment says as much and the code matches.
Finding
[P2][line-level, test gate] A new test fails deterministically on real Windows — post-test dangling rejection. packages/runtime/src/__tests__/ai-sdk-backend.test.ts:11537 (Alibaba Responses fails when streamed reasoning differs from the final summary) passes its assertions, but the process reports "generated asynchronous activity after the test ended … triggered an unhandledRejection event" — reproducible 3/3 runs on this Windows machine, rejection value undefined, no stack. CI's test job is green because it runs on Linux; this suite's evidence does not cover Windows. I could not determine in the time I had whether the leak lives in the test's mock-stream cancellation or in the production stream-error path — but this PR's own comment in model-adapter.ts (a generator return cannot strand the caller awaiting result.outcome) shows the author knows this failure mode is real, and a production unhandled rejection on a provider-mismatch path is a crash risk under default Node settings. It needs a repro and a verdict, not a shrug.
Executed evidence (real Windows x64, exact head)
Clean rebuild of core+runtime at the head, then: core model-metadata + provider-catalog-contract 20/20; runtime responses-reasoning-state + responses-wire-contract + open-responses-compatibility + request-customization-fetch 30/30; model-adapter + model-adapter-onerror + provider-contract-matrix 170/170; ai-sdk-backend + openai-responses-plaintext-reasoning 208/209 — the one failure is the finding above.
Checks: test green on the exact head; audit correctly absent (no manifest changes).
简体中文
全量复审完成。三个开放问题：新模块是沿既有 seam 的扩展而非重复抽象（core 只声明 profile 存在，runtime 独占解析，矩阵测试钉死两边一致）；字段映射内部一致且 wire 测试钉住请求体（store:false 在用户 overlay 之后强制、tool_choice 强制形状拒发、reasoning 回放精确重建），但我无法验证与阿里云真实 API 的符合性（无凭据，这是诚实边界）；错误归类没有放宽重试策略（分类保留为诊断，retryable 强制 false）。一条 P2：新增测试在真 Windows 上确定性失败（测试结束后出现未处理的 rejection，值 undefined），CI 绿是因为跑在 Linux；泄漏在测试 mock 还是生产路径未定论，需要作者复现定性。
A consumer that stops the adapter mid-stream (the reasoning-mismatch throw, a user stop) leaves the SDK without a finish chunk, so teardown rejects every result promise. usage and finishReason were already consumed, but response was only read on the completed continuation path — its rejection could surface as an unhandled rejection after the turn unwound, scheduler-timing owned (observed post-test on Windows, where Node makes it a crash). Sink it unconditionally in the teardown finally, and pin the property with an unhandledRejection trap in the mismatch test so every event loop proves the path leaves none behind.
|
@zhiiw Thank you for the full Windows pass and the precise finding — addressed in e88e07c. Verdict. I attempted the repro first: standalone probes of the exact mismatch scenario (unhandledRejection traps, long settle windows, stress loops) and the suite under Mechanism. The mismatch throw stops the adapter generator early, before any finish chunk exists; SDK teardown then rejects every Fix (production). The adapter teardown now sinks Regression guard (test). The mismatch test now installs an Evidence at exact head e88e07c (macOS). ai-sdk-backend + model-adapter + model-adapter-onerror + openai-responses-plaintext-reasoning + responses-wire-contract + responses-reasoning-state: 276/276. Honest boundary: I cannot execute the Windows repro from here — the new trap asserts exactly the activity you observed, so a re-run of |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for this. test is green on e88e07c2, and the 25 changed files all serve the Token Plan work — I checked each one against the merge base and found nothing riding along.
Two things in the request finalizer are worth another look before this lands; both are inline. They share a shape: the finalizer makes a claim about what the provider supports or does, and the tests assert the claim rather than the provider's behaviour.
One housekeeping note: the verification and live-probe comments in the description still refer to 850ae74a9, an earlier head. They cannot stand as evidence for e88e07c2 — worth refreshing so a later reader does not mistake them for current.
中文
感谢。e88e07c2 上 test 已绿；25 个改动文件我对着 merge-base 逐个过了，都服务于 Token Plan 这条主题，没有夹带。
请求 finalizer 里有两处建议再看一下，都在行内。它们形状相同：finalizer 对供应商的支持范围或行为下了断言，而测试验证的是这个断言本身，不是供应商的实际行为。
另外一件小事：描述里的验证与 live probe 说明仍然指向更早的 head 850ae74a9，不能作为 e88e07c2 的证据，建议更新，免得后来的人误当成当前状态。
| if (!profile) return undefined; | ||
| return (body) => { | ||
| const choice = body.tool_choice; | ||
| if (choice === 'required' || (choice !== null && typeof choice === 'object')) { |
There was a problem hiding this comment.
[P2] This rejects forced tool_choice unconditionally, including the forms Alibaba documents as supported.
The condition is choice === 'required' || (choice !== null && typeof choice === 'object'), and it throws before dispatch. So a request with exactly one tool and tool_choice: 'required' is refused, as is any object form.
Alibaba's Model Studio documentation for the OpenAI-compatible Responses API lists auto, none and required, with required supported when exactly one tool is present, and documents an object form as well. If that is accurate, this turns a supported call into a hard error.
Today the normal runtime path emits none or omits the field, so this is not reachable in the default flow — but requestBodyOverlay and any future caller can set it, which is why it is worth fixing rather than leaving as a latent trap.
Narrowing the check to the shapes the provider actually rejects — keyed on tool count for required — would keep the guard useful without refusing valid requests. If a live probe shows the documentation is wrong and forced choice genuinely fails, that is worth capturing in a comment here, since the current code reads as if that had already been established.
中文
[P2] 这里无条件拒绝强制 tool_choice，把阿里文档中标为支持的形态也拒掉了。
判断是 choice === 'required' || (choice !== null && typeof choice === 'object')，命中即在 dispatch 前抛错。所以"恰好一个 tool + required"这种组合也会被拒，对象形式同理。
阿里 Model Studio 关于 OpenAI 兼容 Responses 接口的文档列出了 auto/none/required，其中 required 在恰好一个 tool 时支持，也记载了对象形式。若文档属实，这段代码把一个受支持的调用变成了硬错误。
当前 runtime 默认只生成 none 或省略该字段，所以默认路径走不到——但 requestBodyOverlay 和将来的调用方都能设置它，所以值得现在收窄，而不是留成暗坑。
建议按工具数量对 required 收窄，只拒供应商真正不支持的形态。如果 live probe 证明文档有误、强制选择确实不可用，把这个结论写进注释——现在的代码读起来像是已经确认过了。
There was a problem hiding this comment.
Agreed; fixed on exact head 424619b35.
The finalizer now follows the currently documented Alibaba Responses contract instead of rejecting every forced choice:
- string
requiredis preserved when the outgoing top-leveltoolslist contains exactly one tool; - object
{ type: "allowed_tools", mode: "required", tools: [...] }is preserved when its selected list contains exactly one tool; - invalid forced cardinality still fails before dispatch;
- object shapes not documented by Alibaba, such as the SDK specific-function form, remain rejected.
The product-level wire test drives real doGenerate through the pinned SDK for string required, and drives the documented allowed_tools object through the request-overlay/finalizer path. Both assert the exact outbound body and that store: false remains final.
Exact-head evidence: Runtime 3,023 passed / 13 skipped / 0 failed; focused compatibility + wire suites 20/20; format, lint, headers, Core/Runtime builds, and diff check passed. The PR description now binds these results to 424619b35.
| if (choice === 'required' || (choice !== null && typeof choice === 'object')) { | ||
| throw new Error('Alibaba Token Plan Responses does not support forced tool_choice'); | ||
| } | ||
| return { ...body, store: false }; |
There was a problem hiding this comment.
[P2] store: false is asserted as a stateless guarantee, but nothing here shows the provider honours it.
The description states that the runtime applies a final request policy of store: false, the test is named for a stateless request, and responses-wire-contract.test.ts:284 describes the field as the wire switch for it. What the test actually asserts is that the field appears in the outgoing body.
Alibaba's parameter table for this endpoint does not list store, and its compatibility rules say only explicitly listed parameters are processed. The pinned @ai-sdk/open-responses@2.0.29 does not emit the field on its own either. So if the provider ignores it, the stateless and restart-safe-replay properties claimed in the description do not hold — and no test would notice, because every test checks our side of the wire.
This is not "the field might be ignored" as a hypothetical. The PR makes a retention claim, and the evidence offered supports a weaker statement: that we send a particular byte sequence.
Either use a retention control the provider documents and verify it against the real endpoint, or drop the stateless/privacy wording and the assertions that depend on it. Sending the field as best-effort is fine; promising the property is what needs backing.
中文
[P2] store: false 被当作 stateless 保证写了出来，但没有任何证据表明供应商会遵守它。
描述里写了 runtime 施加 store: false 的最终请求策略，测试名带 stateless，responses-wire-contract.test.ts:284 也把它说成 wire 上的开关。但测试实际断言的只是这个字段出现在发出的 body 里。
阿里该接口的参数表没有列 store，而兼容性规则写明未列出的参数不处理；锁定的 @ai-sdk/open-responses@2.0.29 自己也不生成该字段。所以若供应商忽略它，描述里声称的 stateless 与 restart-safe replay 就不成立，而且没有任何测试会发现——所有测试查的都是我们这一侧。
这不是"字段可能被忽略"的假设。是这个 PR 做了关于数据留存的承诺，而它给出的证据只支持一个更弱的陈述：我们发出了某段字节。
要么改用供应商文档中存在的留存控制并对真实端点验证，要么去掉 stateless/隐私措辞和依赖它的断言。字段照发没问题，需要判据的是那个承诺。
There was a problem hiding this comment.
Rechecked against the current Alibaba OpenAI-compatible Responses reference. The premise that store is absent from the parameter table is not true in the current documentation:
https://www.alibabacloud.com/help/en/model-studio/qwen-api-via-openai-responses
The request-body table lists store as an optional boolean with default true, and explicitly defines false as: the response is not stored and cannot be referenced by previous_response_id. The Retrieve Response reference also echoes the same semantics.
I kept the provider-owned store: false finalizer on that documented basis. I did make the wording more precise on exact head 424619b35: the test formerly named “stateless request” is now “non-stored request”, and the PR body says continuation does not depend on a provider-stored response ID. It does not claim that Alibaba performs no other operational retention.
The restart-safe replay statement is a separate Maka-local property: bounded item identity and summary boundaries are persisted in the local durable event state and reconstructed into the next request. It does not rely on provider storage.
The PR description now separates exact-current-head verification from older live evidence and labels every live run with its actual head.
Permit documented required and allowed_tools choices when they select exactly one tool, while keeping provider-owned store:false finalization and rejecting unsupported forced shapes.\n\nGenerated-by: OpenAI Codex
4 participants
Summary
Adds Alibaba Token Plan Responses support for
qwen3.8-maxwhile keeping the other Token Plan models on Chat Completions.Core owns only protocol facts and explicit delegation:
openAiAdapterApiProtocoldeclares the Alibaba Token Planqwen3.8-max → openai-responseswire;alibaba-token-planRuntime profile;responsescontract and a Runtime profile.Runtime owns the concrete execution profile:
@ai-sdk/open-responses;store: false, so continuation never depends on a provider-stored response ID;requiredandallowed_toolstool choices when exactly one tool is selected, while rejecting invalid forced cardinality and unsupported object shapes;output_item.done.item.summary;Fixes #3162
Verification
Exact head
424619b35:npm run format:checknpm run lintnpm run check:asf-headerstool_choice: "required"and anallowed_toolsrequest overlay, withstore: falsestill finalized after caller customizationgit diff --checkThe current head includes the early-stop stream teardown fix from
e88e07c24:sdk.responseis always consumed, and the mismatch regression traps delayedunhandledRejectionevents. The exact fix head passed CI; an independent Windows re-run remains requested because the original scheduler-owned leak reproduced only on Windows.Provider and live evidence
store: falseas not storing the response and making it unavailable toprevious_response_id.tool_choice: "required"and objectallowed_toolsmode, with forced choice available only when exactly one tool is selected: https://www.alibabacloud.com/help/en/model-studio/qwen-api-via-openai-responses34bed03f1reached Alibaba Token Plan China/compatible-mode/v1/responseswithqwen3.8-max; the same credential was rejected by Coding Plan Chat with HTTP 401 (request id3f605e8f-be3a-992a-a443-f57c2a1b7387).34bed03f1completed streamed reasoning, a Maka-owned Read continuation, and the final answer: feat(providers): add Alibaba Token Plan Responses compatibility #3255 (comment)idandsummary_textentries onresponse.output_item.donein the Responses documentation linked above.response.reasoning_text.deltatext equals the final item summary before durable state is attached.The live evidence is labeled with the head on which it ran; the exact current-head evidence is listed separately under Verification.
Review focus
providerType; it resolves only the profile explicitly referenced by Core.ModelInfo.apiProtocolstill overrides the catalog default.AI use
OpenAI Codex assisted with protocol research, implementation, automated tests, review, and PR drafting. The human contributor reviewed the scope and remains responsible for the contribution. The commits carry a
Generated-by: OpenAI Codextrailer.Checklist
Does this PR entail a change in behavior?