Skip to content

[ISSUE #7871] refactor(ai-proxy): introduce protocol conversion abstraction - #7910

Merged
hengyuss merged 1 commit into
apache:masterfrom
hengyuss:refactor/ai-protocol-converter-routing
Oct 22, 2026
Merged

hengyuss merged 1 commit into
apache:masterfrom
hengyuss:refactor/ai-protocol-converter-routing

Conversation

@hengyuss

Copy link
Copy Markdown
Contributor

Purpose

Part of #7871. This is the second incremental AI Proxy refactoring after #7876.

Changes

  • introduce AiProtocolConverter, AiProtocolConverterFactory, and request-scoped AiProtocolConversion
  • route client protocols to Provider-native protocols, preferring native identity routes
  • bind forward request conversion and reverse response conversion to the same conversion context
  • replace ConnectProtocolAdapter with a Responses-to-Chat converter
  • separate Provider Chat-dialect adaptation from protocol conversion
  • compose Provider adaptation, reverse protocol conversion, and client response encoding before execution
  • keep AiUpstreamRequest limited to upstream HTTP request data
  • register the converter factory and default converter in Spring configuration

OpenAI Responses native passthrough and its /v1/responses endpoint remain follow-up work under #7871; this PR keeps the current Engine on the Responses-to-Chat route.

Tests

  • AI Proxy focused suite: 280 tests (269 sandbox-safe tests plus 11 local HTTP tests), all passed
  • AiProxyPluginConfigurationTest: 12 tests passed
  • Checkstyle passed with 0 violations
  • git diff --check passed

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review — APPROVE

I read the full 24-file diff at a1941f56, checked the new wiring against the Spring context built in AiProxyPluginConfigurationTest, and confirmed pr_build really ran this module (MODULE_ARGS="-pl shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-proxy,shenyu-spring-boot-starter/.../shenyu-spring-boot-starter-plugin-ai-proxy -am", ConnectResponsesToChatConverterTest 10/10, AiProtocolConverterFactoryTest 9/9, ConnectProviderDialectAdapterTest 3/3, AiProxyEngineOrchestrationTest 47/47, AiProxyPluginConfigurationTest 12/12, BUILD SUCCESS).

The separation itself is sound and I could not reproduce a functional regression. Details of what I verified, then non-blocking notes.

Verified

1. The adaptResponse hook was genuinely dead. AiProxyProvider.adaptResponse had no override in main (only OpenAi/OpenAiCompatible exist, and neither overrode it — OpenAi.prepareRequest returned the response adapter instead). Removing it and collapsing attempt() to prepared.responseAdapter().adapt(response) preserves the old order. AiProxyEngineOrchestrationTest now asserts exactly ["provider", "protocol"], which matches one provider pass + one client-protocol encode.

2. Dropping providerProfile from the reverse conversion is safe today. ConnectResponsesToChatConverter.createContext (lines 89–94) only sets toolLossPolicy and upstreamModel; the old ConnectProtocolAdapter also put profile into options.providerProfile for the same ConversionContext used by convertResponse. So the reverse pass lost the dialect. This is currently harmless: AiProxyProviderHttpTest.deepSeekChatStreamConvertsReasoningAndUsageForResponsesClients runs DeepSeek end-to-end and still asserts received.contains("trace") (from reasoning_content) plus "total_tokens":8 with no profile in the reverse context, i.e. Connect maps those fields generically. Flagging it only because the PR description says forward and reverse "share the same conversion context" and that is no longer literally true.

3. Capability gating still fails closed. chat() dropping OpenAiResponses.NAME / Operation.RESPONSE means resolveConverter always produces a Chat request for the bundled providers, and validate(providerRequest, stream) now checks the converted operation — which is what testCustomChatProviderValidatesConvertedOperation and the inverted unsupportedOperationsFailBeforeTransportWithoutEngineChanges pin down. provider.supports(providerRequest.getProtocol()) at engine line ~244 is unreachable (the factory only returns routes inside capabilities().protocols()), but it is a harmless belt-and-braces check.

4. applyModelOverride is needed and does not corrupt the payload. Without it the conversion context would have no model for the common config.model-only case, and OpenAi.prepareRequest would set the model after conversion. deepSeekChatStreamConvertsReasoningAndUsageForResponsesClients sends {"input":"hi"} with config.setModel("deepseek-flash") and would fail without it. decodeRequest deep-copies, so rawPayload is not aliased.

Non-blocking

  1. Two different "upstream model" values now reach the two contexts. createContext uses request.getModel() (client model, optionally overridden by config.model), while ConnectProviderDialectAdapter receives mappedModel after options.modelMappings(). Before this PR both used mappedModel. If a modelMappings alias remaps, the Responses→Chat pass sees the alias instead of the routed model. If Connect ever starts applying model-family rules during conversion, that is a silent wrong-field risk. Cheap fix: either resolve mappedModel in the engine and pass it through, or document that the conversion context is deliberately model-agnostic.

  2. The default route cannot be replaced without breaking startup. AiProtocolConverterFactory's constructor (diff line ~164) throws IllegalArgumentException("Duplicate AI protocol converter: ...") on a duplicate source -> target. connectResponsesToChatConverter() is only backed off by @ConditionalOnMissingBean on the concrete type (AiProxyPluginConfiguration:134–137), so an extension that registers its own AiProtocolConverter for openai-responses -> openai-chat gets the default plus its own converter and the context fails to refresh. For a PR whose stated goal is an extension point, some opt-out is needed — e.g. a shenyu.plugins.ai.proxy.conversion.default-responses-to-chat=false property, or putIfAbsent + a warning instead of a hard throw.

  3. List<AiProtocolConverter> collection injection is order-sensitive. aiProtocolConverterFactory(...) is declared after connectResponsesToChatConverter() in the same class, so the default is picked up; but a converter contributed by a @Configuration processed later silently misses the registry (no error, just an Unsupported AI protocol conversion at request time). Worth a line in the javadoc, or inject ObjectProvider<AiProtocolConverter> and resolve lazily.

  4. Test coverage dropped with the deleted ConnectProtocolAdapterTest (−258). Three behaviours lost their only tests: the cumulative MAX_CONVERSION_BYTES stream bound (the branch at ConnectResponsesToChatConverter:157–170 is now untested — only the complete-response limit is), the 199 status passthrough (< 200 || >= 300), and "stream=true with Content-Type: application/json must error". The custom-tool cross-request isolation case also went away. Please port those four rather than dropping them.

  5. AiProxyEngine's public constructors changed signature (the 4-arg forms lost protocol/provider). Anyone embedding the engine outside this starter gets a source break. Fine for an internal class, but it belongs in the PR description / release note.

  6. Double validation. prepare() calls provider.capabilities().validate(providerRequest, stream) (line 247) and OpenAi.prepareRequest validates the same request again (line 112). Pre-existing, but the new code makes the redundancy obvious.

  7. Minor: the "Unsupported AI upstream protocol/provider combination: X -> provider" branch (engine 243–246) is now unreachable — the factory is the only place that can reject a route, and its message (Unsupported AI protocol conversion from X to provider protocols [Y]) is the one users will actually see. That message drops the provider name, which is the piece an operator needs; passing it into resolveConverter, or raising the error in the engine, would be more actionable.

Unrelated, still open on master

build (shenyu-integrated-test-http) is red for everyone: #7903 changed CryptorResponsePlugin.fieldErrorParse to throw ResponsiveException(-116) while CryptorResponsePluginTest.testWhenDoesntExistFiled still asserts 10001. Not caused by this PR.

@wy471x wy471x left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two additional points (@Aias00 has covered model mapping, duplicate factory routes, and test coverage):

  1. shenyu-plugin-ai-proxy/README.md still references the removed/renamed ConnectProtocolAdapter and AiProxyProvider.adaptResponse (around lines 46/206). It should be updated to the AiProtocolConverter/prepareRequest flow; otherwise the migration guidance for custom providers is misleading.
  2. Removing AiProxyProvider.adaptResponse is a silent behavior change for third-party providers: their implementations still compile, but the hook is no longer called. Consider keeping a deprecated default hook for one release, or explicitly documenting the migration in the release notes.

Otherwise I agree with @Aias00's conclusions; non-2xx passthrough matches the previous behavior.

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