Repository navigation
fix(ai-proxy): align the token limiter usage test with the conversion abstraction - #8083
Conversation
… abstraction `ConnectProtocolAdapter` disappeared when apache#7910 replaced it with the directional converter registry, but `AiTokenLimiterResponsesUsageTest` still imported the removed class, so test compilation failed for every build after that commit. This keeps the whole `ci` and `CodeQL` jobs red for the last two days and hides real regressions in the token limiter. The test now drives the real conversion path through `ConnectResponsesToChatConverter`, which is the current entry point for turning a Chat Completions response (or stream) back into the Responses format the client asked for. The exchanged bytes stay identical, so the usage accounting assertions keep guarding issue apache#7911. Also drop the stale `ConnectProtocolAdapter` reference from the AI proxy README, where it survived the refactor. Co-authored-by: cloudbase-ai <22498243+cloudbase-ai@users.noreply.github.com>
Aias00
left a comment
There was a problem hiding this comment.
Review (Aias00)
Verdict: APPROVE. This is a targeted build repair, and the change is on the right side of the "fix the caller, not delete the test" line.
Root cause confirmation
Traced the deletion to 819b92a63 (#7910, "introduce protocol conversion abstraction"), which removed ConnectProtocolAdapter and ConnectProtocolAdapterTest but left AiTokenLimiterResponsesUsageTest importing the deleted class. Every push after that commit has a failing testCompile for shenyu-plugin-ai-token-limiter. I downloaded the Analyze (java) job log for run 40050109833 on master and it is the identical compile error:
cannot find symbol: class ConnectProtocolAdapter
location: package org.apache.shenyu.plugin.ai.proxy.enhanced.protocol
So the ci failure and the CodeQL failure are one defect, not two. codeql-analysis.yml builds with build-mode: manual on push events only (none for pull_request), which is exactly why this only ever showed up red on master and looked like an unrelated CodeQL problem.
The migration is faithful to the old assertions
ConnectResponsesToChatConverter().convert(request, stream)is the current owner of the request-scopedConversionContext;conversion.responseAdapter().adapt(upstream)is the current reverse conversion. That is the same "real adapter, not a hand-written fixture" property the test's comment on issue #7911 demands.- Request construction via
new OpenAiResponses().decodeRequest(rawBody, payload)reuses the production decoder rather than hand-building aShenyuAiRequest, sogetProtocol(),getModel()and thestreamflag are set exactly as the engine sets them. Good call — building the record by hand would have silently weakened the coverage. - The exchanged bytes are untouched: both
recordsTheSameUsage...andforwardsTheConvertedBytesUnchangedstill compare on the converted byte arrays, so the usage-accounting guarantee for #7911 is preserved rather than reduced.
Local reproduction (matches CI)
mvn -pl shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-token-limiter -am test: target test 3/3, module 30/30, upstreamshenyu-plugin-ai-proxy288/288, BUILD SUCCESS.mvn -pl ... -am checkstyle:check: clean.
Note for future builds in this repo: running -pl <module> without -am resolves shenyu-plugin-ai-proxy from the local ~/.m2 snapshot jar and fails with NoClassDefFoundError; ci.yml always uses -am, so the -am result is the one that reflects CI.
Nits (non-blocking, no changes required)
AiTokenLimiterResponsesUsageTestdeclaresthrows Exceptionon all four methods. Given the only checked failure isObjectMapper.readTree, the existing style in this module could tighten it tothrows JsonProcessingException/Exceptiononly where needed — butExceptionis consistent with sibling tests in the AI modules, so leaving it is fine.- The README edit is correct but the surrounding paragraph still says "Connect consumes JSON chunks" without naming the converter. Optional follow-up: rename that mention to
ConnectResponsesToChatConvertertoo, for consistency with step 3. - Nothing in the change asserts the new converter rejects a non-Responses source protocol from this call site. That behavior is already covered by
ConnectResponsesToChatConverterTest#testRejectsWrongSourceProtocol, so this is fine — just noting the boundary is covered elsewhere.
Follow-up worth considering (separate PR)
The deeper issue is that #7910 renamed the public conversion entry point and deleted the old class, and only the test caught it — because the compiler caught it two days later on master. It would be worth a short note in docs or the AI proxy README that the directional converter registry is the stable entry point and ConnectProtocolAdapter no longer exists, so contributors migrating local branches have a pointer.
CI: all checks green — ci pass, CodeQL / Analyze (java) pass, build (shenyu-integrated-test-http) pass, build (shenyu-integrated-test-k8s-gateway-api-http) pass, pr_build, registry_dependencies, check-license-header, rat_check all pass.
Summary
ciandCodeQLhave been failing onmastersince #7910 landed. Theroot cause is one stale import, not a product defect:
ConnectProtocolAdapterwith the directional converterregistry (
AiProtocolConverterFactory,AiProtocolConverter,AiResponseAdapter), and deleted the adapter plus its test.AiTokenLimiterResponsesUsageTeststill imported the removed class, sotestCompilefails forshenyu-plugin-ai-token-limiter:buildjob ofciand, becausecodeql-analysis.ymlbuilds the same sources inmanualmode on pushruns, the
Analyze (java)job ofCodeQLtoo. Both have been red forevery push since
819b92a63, so the gate can no longer catch realregressions in the AI plugins.
Change
The test now drives the current conversion path instead of the deleted
adapter:
ConnectResponsesToChatConverter().convert(request, stream)produces the request-scoped
AiProtocolConversion, andconversion.responseAdapter().adapt(upstream)performs the reverseconversion of a Chat Completions response (and stream) back into the
Responses format. The exchanged bytes are unchanged, so the usage
accounting assertions that guard issue #7911 keep their meaning.
The AI proxy README still described
ConnectProtocolAdapterin itsexchange lifecycle; that reference is updated as well.
Verification
mvn -pl shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-token-limiter -am test:AiTokenLimiterResponsesUsageTest3/3, module 30/30, upstreamshenyu-plugin-ai-proxy288/288, BUILD SUCCESS.mvn -pl shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-token-limiter -am checkstyle:check: clean.master(86e932cdf) first: samecannot find symbolon the untouched file.This is a build/test repair only: no production code path changes.
Fixes #7911