feat: add Qwen OAuth integration and provider support#650
Conversation
0b750ac to
985f3ae
Compare
- Add Qwen OAuth authentication support via /onboard-qwen command - Implement Qwen provider configuration in provider manager UI - Add Qwen-specific API client handling with proper headers and token refresh - Include Qwen in model configuration and provider detection logic - Add Qwen credentials management and storage at ~/.qwen/oauth_creds.json - Support Qwen model (qwen3-coder-plus) with portal.qwen.ai endpoint - Use qwen-coder@alibabacloud.com for Qwen co-author email so commits link to the @qwencoder GitHub profile Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Qwen-Coder <qwen-coder@alibabacloud.com>
a5d03e9 to
ade1d38
Compare
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Review: Qwen OAuth integration and provider support
Reviewed on head ade1d38. CI green ✅. 23 files, +1,046/-26.
This is a substantial feature PR adding a full Qwen OAuth provider with device-flow authentication, credentials management, provider UI integration, and OpenAI-shim transport. Overall the architecture follows the established Mistral/Gemini/GitHub patterns well, but there are several blockers and concerns that need addressing before merge.
🔴 Blocker 1: Unrelated .gitignore change
.gitignore adds .openclaude/settings.local.json — this has nothing to do with Qwen integration and no code in the PR writes to that path. Either remove it or explain why it's needed here.
🔴 Blocker 2: Unrelated QueryEngine.ts behavioral change
// Before (clean):
const mainLoopModel = modelFromUserInput ?? initialMainLoopModel
// After (behavior change mixed into Qwen PR):
const mainLoopModel = modelFromUserInput ?? (userSpecifiedModel
? parseUserSpecifiedModel(userSpecifiedModel)
: getMainLoopModel())This changes model resolution logic for all providers, not just Qwen. The old code used the initialMainLoopModel (computed once at startup). The new code re-parses userSpecifiedModel or re-calls getMainLoopModel(). This is a behavioral shift that could affect model selection on every provider and deserves its own PR with focused testing. Remove it from this PR.
🔴 Blocker 3: No test coverage
Zero test files in a +1,046-line PR that includes:
- PKCE + device flow logic
- Token refresh with expiry/leeway
- Credential persistence (file I/O with
0o600permissions) resolveQwenBaseUrlnormalizationbuildQwenProfileEnv/mergeUserSettingsEnvenv manipulation
The deviceFlow.ts functions are particularly testable — they accept fetchImpl for injection. The credentials module has a QWEN_OAUTH_CREDS_PATH override for path injection. At minimum, add tests for:
resolveQwenBaseUrl(you already have testable edge cases: bare host, full URL, v1/v2/v1beta, trailing slashes)resolveQwenCredentialrefresh logic (token valid → no refresh, expired → refresh, refresh fails →kind: 'none')fromTokenResponse/saveQwenCredentials/loadQwenCredentialsround-tripbuildQwenProfileEnvenv output- Device flow
requestQwenDeviceCode/pollQwenDeviceTokenwith mockfetchImpl
🔴 Blocker 4: Hardcoded QwenCode/0.14.3 user-agent string
const qwenUa = `QwenCode/0.14.3 (${process.platform}; ${process.arch})`This will go stale the moment qwen-code releases 0.14.4 or OpenClaude updates. Options:
- Read from
package.jsonversion (if matching qwen-code's API contract) - Make it a constant that's easy to update, with a comment noting the qwen-code version it aligns with
- Best: derive dynamically or make it clear this needs manual updates on qwen-code releases
🟡 Concern: Attribution changes overreach
The attribution.ts changes are more complex than needed:
// New code:
? (getAPIProvider() === 'qwen' ? publicModelDisplayName! : getPublicModelName(model))
: model.toString().toLowerCase().includes('qwen')
? 'Qwen-Coder' // Special handling
: 'Claude Opus 4.6'Issues:
- The
publicModelDisplayName!non-null assertion is unsafe —isKnownPublicModelguaranteesgetPublicModelDisplayName(model) !== null, butpublicModelDisplayNameis captured before the check in a separate variable. If the function has side effects or the model changes between calls, this could be null. - The fallback
model.toString().toLowerCase().includes('qwen')leaks internal model names as attribution. If someone names a model "my-qwen-finetune", it gets attributed as "Qwen-Coder" — incorrect. - The
qwen-coder@alibabacloud.comco-author email is hardcoded without verification that Alibaba owns/accepts that address. The existing pattern usesnoreply@domains from the project or the first-party provider. Considernoreply@qwen.aiornoreply@alibabacloud.cominstead, or match the existingopenclaude.devpattern for third-party providers.
Simpler approach: just add Qwen to the existing getPublicModelDisplayName switch and keep the co-author domain logic as apiProvider === 'firstParty' ? 'anthropic.com' : 'openclaude.dev' (unchanged). The Qwen display name belongs in getPublicModelDisplayName, not in attribution-specific branching.
🟡 Concern: resolveQwenCredential silent failure on refresh error
In qwenCredentials.ts, when token refresh fails:
catch (err) {
if (err instanceof QwenDeviceFlowError) {
return { kind: 'none' } // Silent failure
}
throw err
}But in openaiShim.ts, there's an explicit throw:
if (!finalKey) {
throw new Error('Qwen access token expired. Run /onboard-qwen to re-authenticate.')
}These two paths are inconsistent. The resolveQwenCredential path silently returns none, then the caller in withRetry.ts just doesn't update the env var — the user gets a generic 401 error instead of the helpful "Run /onboard-qwen" message. Consider having resolveQwenCredential throw or return an error kind so the retry path can surface the same helpful message.
🟡 Concern: process.env mutation in openaiShim.ts
process.env.OPENAI_API_KEY = resolved.accessTokenMutating process.env in the middle of a request is a side effect. If two concurrent requests hit this code and one triggers a refresh, the other might see a partially-written token. The Gemini and GitHub providers also do this, so it's an existing pattern — but worth noting it's not thread-safe. Since this matches the existing convention, not blocking, but flag for future cleanup.
🟡 Concern: Missing qwen3-coder-plus context window entry
qwen3-coder-plus is the default model but has no entry in openaiContextWindows.ts. It falls through to the 128k fallback (from PR #636). Per official docs, Qwen3-Coder has a 1M token context window. The 128k fallback is safe (won't cause infinite auto-compact) but will cause premature compaction — the model can handle 1M tokens but OpenClaude will compact at ~108k effective. Add:
'qwen3-coder-plus': 1_048_576,(and max output tokens if known).
🟢 Good: Security properties
- Credentials stored at
~/.qwen/oauth_creds.jsonwithmode: 0o600✅ CLAUDE_CODE_USE_QWENfollows the sameUSE_*flag pattern ✅buildQwenProfileEnvcorrectly omitsOPENAI_API_KEY— token resolved at runtime ✅buildLaunchEnvforqwen-oauthcorrectly clears all other provider env vars ✅clearProviderProfileEnvFromProcessEnvclearsCLAUDE_CODE_USE_QWEN✅- PKCE with S256 challenge (not plain) ✅
- Device flow timeout and
slow_downhandling ✅
🟢 Good: Architecture consistency
ProviderProfiletype extended correctly ✅getAPIProvider()returns'qwen'before other provider checks ✅providerConfig.tsfallback model isqwen3-coder-plus✅client.tsroutes Qwen through OpenAI shim ✅openaiShim.tsadds Qwen tomax_tokens/max_completion_tokensgate ✅- Qwen doesn't support
storeorstream_options— correctly deleted ✅ StartupScreen.tsdetects Qwen provider ✅modelStrings.tsmapsqwen→openaifor config lookups ✅ModelConfigtype correctly madePartialformistralandqwen✅
🟢 Good: Device flow implementation
- Uses same
client_idas qwen-code CLI — credentials are interchangeable ✅ code_verifierreturned alongsidedevice_codefor PKCE verification ✅- Browser open is best-effort with catch ✅
mergeUserSettingsEnvcleans other provider keys from user settings ✅hasExistingQwenLoginchecks forrefresh_tokenpresence (not justaccess_token) ✅shouldForceQwenReloginsupports--force/--relogin/--reauth✅
Verdict: Needs changes 🔧
4 blockers to address:
- Remove unrelated
.gitignorechange - Remove unrelated
QueryEngine.tsbehavioral change - Add test coverage (at minimum:
resolveQwenBaseUrl,resolveQwenCredential, credentials round-trip,buildQwenProfileEnv) - Fix hardcoded
0.14.3UA string (make updatable or derive dynamically)
Plus 4 non-blocking concerns to consider:
- Simplify attribution logic (move Qwen display name to
getPublicModelDisplayName, usenoreply@pattern for co-author email) - Make
resolveQwenCredentialrefresh failure propagate the helpful "Run /onboard-qwen" error - Add
qwen3-coder-pluscontext window entry (1M tokens) - Note
process.envmutation concurrency concern for future cleanup
Summary
/onboard-qwencommand~/.qwen/oauth_creds.jsonqwen3-coder-plus) with portal.qwen.ai endpointTest plan
/onboard-qwencommand works for OAuth flow🤖 Generated with OpenClaude