fix(qwen): allow explicit qwen3.6-plus on Coding Plan#72664
Conversation
openclaw-barnacle
Bot
added
agents
Greptile SummaryThis PR fixes explicit Confidence Score: 4/5Safe to merge; the core logic change is correct and well-tested, with only minor test maintainability concerns. No P0 or P1 issues found. Both P2 comments are about test maintainability (mock drift, undocumented SDK assumption) and do not affect production behavior. The suppression-reordering fix is correct, the new
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/pi-embedded-runner/model.test.ts
Line: 5-38
Comment:
**Test mock duplicates production suppression logic**
The `vi.mock("../model-suppression.js")` factory inlines its own copies of `isQwenCodingPlanBaseUrl` and `resolveConfiguredQwenBaseUrl`. Both functions are already defined in `extensions/qwen/models.ts` and `extensions/qwen/index.ts`, respectively. Because the mock is not imported from the real module, any future change to the production logic (e.g., adding a new Coding Plan hostname, changing the `modelstudio` alias check) will silently leave the mock out of sync and the tests will continue to pass while the production path regresses. Consider importing the real helpers or, at minimum, adding a comment pointing to the canonical implementations.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/qwen/index.test.ts
Line: 31-44
Comment:
**`registerProvider` may never be called on the SDK-wrapped plugin**
`registerQwenProviderForTest` feeds a mock API with a `registerProvider` spy into `qwenPlugin.register`. The user-level `register(api)` in `index.ts` only calls `api.registerMediaUnderstandingProvider` and `api.registerVideoGenerationProvider` — it never calls `api.registerProvider`. This works only if `defineSingleProviderPluginEntry` internally wraps `register` so that its output calls `registerProvider` before the user callback runs. If that SDK contract were to change (or the test helper were ever reused against a plain plugin), the function would silently throw `"Qwen provider was not registered"`. A comment explaining the SDK wrapping assumption, or a test assertion on `provider !== undefined` before the throw, would make the invariant explicit.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(qwen): allow explicit qwen3.6-plus o..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| vi.mock("../model-suppression.js", () => { | ||
| function isQwenCodingPlanBaseUrl(value: string | undefined): boolean { | ||
| if (!value?.trim()) { | ||
| return false; | ||
| } | ||
| try { | ||
| const hostname = new URL(value).hostname.toLowerCase(); | ||
| return ( | ||
| hostname === "coding.dashscope.aliyuncs.com" || | ||
| hostname === "coding-intl.dashscope.aliyuncs.com" | ||
| ); | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| function resolveConfiguredQwenBaseUrl(config: unknown): string | undefined { | ||
| const providers = (config as { models?: { providers?: Record<string, { baseUrl?: string }> } }) | ||
| ?.models?.providers; | ||
| if (!providers) { | ||
| return undefined; | ||
| } | ||
| return `Unknown model: ${provider}/gpt-5.3-codex-spark. gpt-5.3-codex-spark is no longer exposed by the OpenAI or Codex catalogs. Use openai/gpt-5.5.`; | ||
| }, | ||
| })); | ||
| for (const [provider, entry] of Object.entries(providers)) { | ||
| const normalizedProvider = provider.trim().toLowerCase(); | ||
| if (normalizedProvider !== "qwen" && normalizedProvider !== "modelstudio") { | ||
| continue; | ||
| } | ||
| const baseUrl = entry?.baseUrl?.trim(); | ||
| if (baseUrl) { | ||
| return baseUrl; | ||
| } | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
The vi.mock("../model-suppression.js") factory inlines its own copies of isQwenCodingPlanBaseUrl and resolveConfiguredQwenBaseUrl. Both functions are already defined in extensions/qwen/models.ts and extensions/qwen/index.ts, respectively. Because the mock is not imported from the real module, any future change to the production logic (e.g., adding a new Coding Plan hostname, changing the modelstudio alias check) will silently leave the mock out of sync and the tests will continue to pass while the production path regresses. Consider importing the real helpers or, at minimum, adding a comment pointing to the canonical implementations.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/model.test.ts
Line: 5-38
Comment:
**Test mock duplicates production suppression logic**
The `vi.mock("../model-suppression.js")` factory inlines its own copies of `isQwenCodingPlanBaseUrl` and `resolveConfiguredQwenBaseUrl`. Both functions are already defined in `extensions/qwen/models.ts` and `extensions/qwen/index.ts`, respectively. Because the mock is not imported from the real module, any future change to the production logic (e.g., adding a new Coding Plan hostname, changing the `modelstudio` alias check) will silently leave the mock out of sync and the tests will continue to pass while the production path regresses. Consider importing the real helpers or, at minimum, adding a comment pointing to the canonical implementations.
How can I resolve this? If you propose a fix, please make it concise.| }; | ||
|
|
||
| function registerQwenProviderForTest(): RegisteredQwenProvider { | ||
| let provider: RegisteredQwenProvider | undefined; | ||
| qwenPlugin.register?.({ | ||
| registerProvider: (next: RegisteredQwenProvider) => { | ||
| provider = next; | ||
| }, | ||
| registerMediaUnderstandingProvider: () => {}, | ||
| registerVideoGenerationProvider: () => {}, | ||
| } as never); | ||
| if (!provider) { | ||
| throw new Error("Qwen provider was not registered"); | ||
| } |
There was a problem hiding this comment.
registerProvider may never be called on the SDK-wrapped plugin
registerQwenProviderForTest feeds a mock API with a registerProvider spy into qwenPlugin.register. The user-level register(api) in index.ts only calls api.registerMediaUnderstandingProvider and api.registerVideoGenerationProvider — it never calls api.registerProvider. This works only if defineSingleProviderPluginEntry internally wraps register so that its output calls registerProvider before the user callback runs. If that SDK contract were to change (or the test helper were ever reused against a plain plugin), the function would silently throw "Qwen provider was not registered". A comment explaining the SDK wrapping assumption, or a test assertion on provider !== undefined before the throw, would make the invariant explicit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/qwen/index.test.ts
Line: 31-44
Comment:
**`registerProvider` may never be called on the SDK-wrapped plugin**
`registerQwenProviderForTest` feeds a mock API with a `registerProvider` spy into `qwenPlugin.register`. The user-level `register(api)` in `index.ts` only calls `api.registerMediaUnderstandingProvider` and `api.registerVideoGenerationProvider` — it never calls `api.registerProvider`. This works only if `defineSingleProviderPluginEntry` internally wraps `register` so that its output calls `registerProvider` before the user callback runs. If that SDK contract were to change (or the test helper were ever reused against a plain plugin), the function would silently throw `"Qwen provider was not registered"`. A comment explaining the SDK wrapping assumption, or a test assertion on `provider !== undefined` before the throw, would make the invariant explicit.
How can I resolve this? If you propose a fix, please make it concise.
4736571 to
566a393
Compare
566a393 to
40e1e3a
Compare
262775b to
8cf1b73
Compare
8cf1b73 to
41efb98
Compare
|
Aisle follow-up reviewed. The inline-model ordering is intentional here, not a suppression bypass: manifest The branch now also canonicalizes Coding Plan hosts for manifest suppression and the Qwen catalog path, including whitespace/trailing-dot URLs, so unconfigured built-in rows remain suppressed. Proof:
|
|
Thanks for landing the narrower fix and for preserving the explicit I have one follow-up question to better understand the intended policy going forward: why should From a user/configuration perspective, the distinction is a little subtle:
Is the catalog suppression mainly intended as a conservative product signal because Coding Plan availability may be tenant/plan/rollout dependent, while explicit config is treated as an operator opt-in? Or is there another security/runtime reason for keeping the built-in catalog boundary? Understanding that distinction would help downstream users decide whether they should rely on explicit config only, or whether broader catalog exposure may be considered later once Qwen availability is more uniform. |
2 participants
Summary
qwen/qwen3.6-plusresolve on Qwen Coding Plan endpoints.Credit
This carries forward the focused fix from @jepson-liu in source PR #63987.
Validation
pnpm check:changedpnpm -s vitest run extensions/qwen/index.test.ts src/agents/pi-embedded-runner/model.test.tsNotes
#66367 is routed to central security handling by this cluster and is not used as an autonomous mutation target.
ProjectClownfish replacement details:
Finished in 897ms on 4 files using 4 threads.
tool-display snapshot is up to date
OK apps/macos/Sources/OpenClaw/HostEnvSecurityPolicy.generated.swift
Found 0 warnings and 0 errors.
Found forbidden low-level body reads in auth-sensitive webhook handlers:
Use plugin-sdk webhook guards (
readJsonWebhookBodyOrReject/readWebhookBodyOrReject) with explicit pre-auth/post-auth profiles.ELIFECYCLE Command failed with exit code 1.
ELIFECYCLE Command failed with exit code 1.