Skip to content

refactor: migrate poe to openai responses api#13502

Open
kovsu wants to merge 3 commits intoCherryHQ:mainfrom
kovsu:poe-openai-responses-api
Open

refactor: migrate poe to openai responses api#13502
kovsu wants to merge 3 commits intoCherryHQ:mainfrom
kovsu:poe-openai-responses-api

Conversation

@kovsu
Copy link
Collaborator

@kovsu kovsu commented Mar 16, 2026

Ref: #13434 (comment)

  • align Poe request construction with OpenAI Responses API semantics
  • add coverage for Poe reasoning and transport behavior

Note

this change has been validated at request-building/test level, not against live Poe API responses

GPT with Poe
{
    "model": "gpt-5.4",
    "input": [
        {
            "role": "system",
            "content": "你是一位经验丰富的网页开发者,精通 HTML/JS/CSS/TailwindCSS,请使用这些技术来创建我需要的页面。\n\n请以下面的格式提供代码,所有代码都需要放在一个 HTML 文件中:\n\n```html\n这里是 HTML 代码\n```"
        },
        {
            "role": "user",
            "content": [
                {
                    "type": "input_text",
                    "text": "hi"
                }
            ]
        },
    ],
    "text": {
        "verbosity": "medium"
    },
    "store": false,
    "include": [
        "web_search_call.action.sources",
        "reasoning.encrypted_content"
    ],
    "reasoning": {
        "effort": "xhigh",
        "summary": "auto"
    },
    "tools": [
        {
            "type": "web_search",  <----
            "search_context_size": "low"
        }
    ],
    "tool_choice": "auto",
    "stream": true
}
Claude with Poe
{
    "model": "claude-sonnet-4.6",
    "input": [
        {
            "role": "system",
            "content": "你是一位经验丰富的网页开发者,精通 HTML/JS/CSS/TailwindCSS,请使用这些技术来创建我需要的页面。\n\n请以下面的格式提供代码,所有代码都需要放在一个 HTML 文件中:\n\n```html\n这里是 HTML 代码\n```"
        },
        {
            "role": "user",
            "content": [
                {
                    "type": "input_text",
                    "text": "hi"
                }
            ]
        },
    ],
    "store": false,
    "include": [
        "web_search_call.action.sources",
        "reasoning.encrypted_content"
    ],
    "reasoning": {
        "effort": "xhigh",
        "summary": "auto"
    },
    "tools": [
        {
            "type": "web_search",     <----
            "search_context_size": "low"
        }
    ],
    "tool_choice": "auto",
    "stream": true
}

@EurFelux
Copy link
Collaborator

EurFelux commented Mar 16, 2026

Note

This issue/comment/review was translated by Claude.

Involves architecture changes, needs careful review


Original Content

涉及架构改动,需要慎重 review

kangfenmao

This comment was marked as outdated.

Copy link
Collaborator

@kangfenmao kangfenmao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Good refactoring PR that cleanly separates transport-layer routing into a dedicated transport.ts module and migrates Poe from the old extra_body-based OpenAI Chat Completions API to the OpenAI Responses API transport.

Significant Issues (2)

  1. Dead code in getReasoningEffort() — The old Poe-specific extra_body reasoning branch (lines 162-224) is now unreachable since Poe routes through buildOpenAIProviderOptionsgetOpenAIReasoningParams instead. Should be cleaned up in this PR.

  2. Web search migration gap — The old extra_body: { web_search: true } is removed from getWebSearchParams, relying on buildProviderBuiltinWebSearchConfig('openai', ...) to provide the OpenAI Responses web_search tool. This should work but lacks test coverage for the full flow.

Minor Issues (3)

  1. createAiSdkProvider behavior change — No longer mutates config.providerId; verify no callers depend on the mutation.
  2. Redundant forceReasoning — Set both in buildOpenAIProviderOptions and in the Poe-specific patch; clarify that the patch exists for systemMessageMode.
  3. Homogeneous test cases — 4 Poe transport tests all assert the same result; consider consolidating.

Positives

  • Clean separation of concerns with resolveAiSdkTransport / resolveAiSdkRuntimeProviderIdByMode
  • Good inline documentation with reference links
  • Comprehensive test coverage for the new transport routing
  • Correct handling of forceReasoning + systemMessageMode: 'system' for non-OpenAI models on Poe

@@ -25,15 +25,6 @@ export function getWebSearchParams(model: Model): Record<string, any> {
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Significant: The old Poe web search logic (extra_body: { web_search: true }) is removed here, but there's no explicit replacement in this PR for the getWebSearchParams path.

After this PR, Poe routes through openai transport with responses mode, so web search for Poe would rely on buildProviderBuiltinWebSearchConfig('openai', ...) in parameterBuilder.ts — which returns the OpenAI Responses web_search tool config. This should work since the PR description shows Poe Responses API accepts tools: [{ type: "web_search" }].

However, this behavioral change should be explicitly documented or tested. Currently the web search test only asserts getWebSearchParams returns {} for Poe, but doesn't verify that the web_search tool is correctly added through the buildProviderBuiltinWebSearchConfig path. Consider adding an integration-level test that covers the full buildStreamTextParams flow for Poe + web search.

Comment on lines 107 to 117
export async function createAiSdkProvider(config: AiSdkConfig): Promise<AiSdkProvider | null> {
let localProvider: Awaited<AiSdkProvider> | null = null
let runtimeProviderId = config.providerId
try {
if (config.providerId === 'openai' && config.options?.mode === 'chat') {
config.providerId = `${config.providerId}-chat`
} else if (config.providerId === 'azure' && config.options?.mode === 'responses') {
config.providerId = `${config.providerId}-responses`
} else if (config.providerId === 'cherryin' && config.options?.mode === 'chat') {
config.providerId = 'cherryin-chat'
}
localProvider = await createProviderCore(config.providerId, config.options)
runtimeProviderId = resolveAiSdkRuntimeProviderIdByMode(config.providerId, config.options?.mode)
localProvider = await createProviderCore(runtimeProviderId, config.options)

logger.debug('Local provider created successfully', {
providerId: config.providerId,
providerId: runtimeProviderId,
hasOptions: !!config.options,
localProvider: localProvider,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The refactoring of createAiSdkProvider introduced a subtle behavior change: the old code mutated config.providerId in-place, so callers that inspected config.providerId after this function returned would see the runtime-resolved ID. The new code uses a local runtimeProviderId variable and leaves config.providerId untouched.

This is likely the correct behavior (avoiding side effects on input), but please verify no callers depend on the mutated config.providerId after calling createAiSdkProvider. If any do, they would now get the original ID instead of the runtime-resolved one.

Copy link
Collaborator

@EurFelux EurFelux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Well-structured refactoring that cleanly separates transport-layer routing into transport.ts and migrates Poe from extra_body-based Chat Completions to OpenAI Responses API. The architecture is sound — resolveAiSdkTransport gives a clear single point of truth for transport decisions.

What works well

  • Clean separation of concerns: actualProviderId vs transportProviderId distinction in buildProviderOptions makes the code much easier to reason about
  • Good inline documentation: Reference links to Poe and AI SDK docs where needed
  • Comprehensive test coverage: Both transport-level and config-level tests for Poe routing
  • Correct forceReasoning + systemMessageMode: 'system' handling for non-OpenAI models on Poe

Action items

  1. [Significant] Dead code in getReasoningEffort() lines 165-224 — old Poe extra_body branches are now unreachable. Should be cleaned up in this PR.
  2. [Nit] forceReasoning is set redundantly (upstream in buildOpenAIProviderOptions and again in the Poe patch). Clarify or deduplicate.
  3. [Question] Error handling for Poe models that might not support /responses — what does the user see?

Agreement with prior reviews

+1 on kangfenmao's points about dead code cleanup and web search migration gap. The web search behavioral change (from extra_body.web_search to OpenAI Responses web_search tool) should work based on the PR description's JSON examples, but an integration-level test covering buildProviderBuiltinWebSearchConfig for Poe would increase confidence.

Comment on lines +18 to +25
export function resolveAiSdkTransport(actualProvider: Provider, model: Model): AiSdkTransport {
// Poe 官方提供统一的 /responses 入口,这里统一复用 OpenAI Responses transport。
// TODO: 如果后续确认某些 Poe 模型不支持 /responses,再基于真实错误补回退逻辑。
if (actualProvider.id === SystemProviderIds.poe) {
return {
providerId: 'openai',
mode: 'responses'
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: The early return for Poe means getAiSdkProviderId() is never called for Poe — so any future changes to getAiSdkProviderId (e.g., adding a static mapping for 'poe') would be silently ignored. This is probably fine and intentional (Poe always routes to OpenAI Responses regardless of provider resolution), but worth noting in the comment.

Also: the TODO mentions "基于真实错误补回退逻辑" — if Poe's /responses endpoint returns an error for certain models, will the current code surface a clear error to the user, or will it silently fail? Consider documenting what the expected error shape would be so future implementers know what to catch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

现在 transport.ts 的执行顺序是:

  1. 先执行
    const providerId = getAiSdkProviderId(actualProvider)
  2. 然后才判断
    if (actualProvider.id === SystemProviderIds.poe)

也就是说:

  • getAiSdkProviderId() 对 Poe 仍然会被调用
  • Poe 分支并没有绕过 provider resolution
  • Poe 分支真正“强行指定”的只有 mode: 'responses'

如果某些 Poe 模型实际上不支持 /responses,失败时会把底层请求错误往上抛,注释里没写更具体的 error shape,是因为目前我们还没有一个已确认的 Poe /responses 错误样本(因为我没有 apikey 去测试)。所以我只确认了请求的时候的参数是正确的

@kovsu kovsu force-pushed the poe-openai-responses-api branch from c481a0e to 2ebd48e Compare March 17, 2026 01:46
@kovsu kovsu requested a review from EurFelux March 17, 2026 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants