fix(providers): honor saved baseUrl for alibaba-token-plan via override + choices#459
Conversation
…de + choices The alibaba-token-plan registry entry (Beijing) lacked allowBaseUrlOverride, so the router silently discarded any user-saved baseUrl and forced the Beijing endpoint. Configs migrated to this id with an international (ap-southeast-1) baseUrl were routed to cn-beijing and failed with 401. Give the entry allowBaseUrlOverride + baseUrlChoices (beijing default, intl/Singapore, custom), mirroring the existing qwen-cloud pattern. Default resolution is unchanged when no user baseUrl is set. Fixes lidge-jun#457
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64a8753ef5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| adapter: "openai-chat", | ||
| authKind: "key", | ||
| allowBaseUrlOverride: true, | ||
| baseUrlChoices: ALIBABA_TOKEN_PLAN_BASE_URL_CHOICES, |
There was a problem hiding this comment.
When the new intl choice is selected, this entry still projects the Beijing Personal Edition dashboardUrl, note, default/static model list, and liveModels: false. AddProviderModal.tsx uses that dashboard URL unconditionally for setup, while gatherRoutedModels enriches the saved provider from this entry and fetchProviderModels immediately returns its static models when live discovery is disabled. Consequently, a new Singapore/Team configuration is directed to the Beijing console and advertised only the Personal Edition catalog despite routing requests internationally; represent the editions separately or make the dashboard and catalog metadata endpoint-specific.
Useful? React with 👍 / 👎.
| baseUrl: ALIBABA_TOKEN_PLAN_BEIJING_BASE_URL, | ||
| adapter: "openai-chat", | ||
| authKind: "key", | ||
| allowBaseUrlOverride: true, |
There was a problem hiding this comment.
For a migrated configuration whose saved baseUrl ends in /—a representation the new matching test explicitly accepts—this opt-in makes routedProviderConfig preserve the URL verbatim. The openai-chat adapter then builds ${provider.baseUrl}/chat/completions, producing a path such as /compatible-mode/v1//chat/completions; upstreams that do not normalize double slashes will reject every request. Strip trailing slashes from the accepted override, or join the request path safely, before routing.
Useful? React with 👍 / 👎.
|
Thanks for chasing this down — the 401 is real, and the router analysis in #457 is spot on. While reading the same code I ran into two bits of history that are easy to miss, and I think they change what this PR should do. Sharing them here rather than in the issue so they're next to the diff.
|
| Commit | What |
|---|---|
9b412d8e |
cherry-pick of #189 — adds allowBaseUrlOverride: true to alibaba-token-plan |
a9b9048a |
Revert "fix: preserve configured Alibaba Token Plan base URL (#189)" |
7e63d2e0 |
feat: add alibaba-token-plan-intl provider for international token plan |
The revert carries no stated reason, but what replaced it is legible in the code: region became a separate registry id. alibaba-token-plan stays pinned to Beijing with no override, and alibaba-token-plan-intl is the entry that carries allowBaseUrlOverride — with ALIBABA_INTL_BASE_URL_CHOICES offering plan type within one region (token-plan / payg / custom), mirroring QWEN_CLOUD_BASE_URL_CHOICES rather than crossing regions.
This PR's beijing / intl / custom set puts both regions back under one id, which is the shape the revert removed. @lidge-jun is the only one who knows whether that revert was deliberate policy or a rollback for an unrelated reason, and the answer decides the whole PR — probably worth a word before investing more in it.
Also worth noting: your own confirmed workaround in #457 — renaming the id to alibaba-token-plan-intl — is exactly what the post-revert design expects.
On the migration: I couldn't find it, and I'd genuinely like to be wrong
The premise that a shipped migration merged user providers into alibaba-token-plan is what makes re-adding the override look necessary, so I tried to confirm it. Searching v2.7.36, v2.7.37, v2.7.38-preview.20260724, v2.7.39, v2.7.40 and dev:
- no hits for
bak-before,provider-rename,alibaba-token-plan-1, orrenameProviderId; alibabaappears only insrc/providers/registry.ts,src/providers/base-url-choices.ts,src/usage/expected-prices.ts,src/generated/jawcode-model-metadata.tsandgui/src/provider-icons.ts— no id-rewriting code;- the only config-backup names this codebase can produce are
<config>.pre-openai-tiers-v2.bak,<config>.pre-openai-tiers-v1-rollback.<ts>.bakand<config>.invalid-<iso>, none of which match.bak-before-<name>-<timestamp>.
You clearly saw that file, so the likeliest explanation is that I'm searching for the wrong thing. If you still have config.json.bak-before-alibaba-provider-rename-* on disk, its mtime and the ocx version that wrote it would settle this in a minute.
The part that seems unambiguous either way
Independent of how region ends up being modelled: src/router.ts:162 discards a saved, well-formed baseUrl with no diagnostic at all, so the only thing a user ever sees is a 401 from an endpoint they never configured. That's your suggested fix 2, which you deliberately kept out of this PR — and I think you were right to, because it's design-neutral: it holds for every pinned registry entry, whichever way the Alibaba question goes. There's a precedent for the shape at src/config.ts:714, where an invalid streamMode warns before falling back.
I'm happy to put that up as a small separate PR so it doesn't tangle with the policy question here — but it's your finding and your issue, so just say if you'd rather own it and I'll stay out of the way.
🔒 Under maintainer review — detailed feedback incoming@lidge-jun (maintainer) has this PR in an active review pass. Please do not merge, rebase, or This is a claim marker so two maintainers do not review or land the same PR at once. If you are a Baseline for this pass: No action needed from you until then. Thanks for the contribution and for your patience. Review tracker: |
|
Thanks for chasing down the Alibaba endpoint problem. The underlying complaint is legitimate, but the approach here conflicts with a design decision that has since landed on High — this restores a single-provider cross-region model that current The concrete failure: a user picks the Singapore URL through your If stale configuration is the real bug you're solving, the right shape is an explicit migration: detect a recognized Singapore URL saved under Medium — the tests don't exercise the behavior in the title. Exact conflicts against All three are semantic, not textual noise: Rebase instruction: git fetch origin
git rebase origin/devWhen resolving, please keep the current Also relevant: #464 adds a warning for exactly this situation — a pinned provider discarding a configured Test gap: no Reviewed as part of a maintainer review pass against |
|
Closing this without merging. The bug you reported is real — a saved Current The PR is also stale against On the underlying problem. If the real issue is that an unattended rename or merge left your config pointing Issue #457 stays open. This PR's body says If you can share what performed the rename — a version, a command, or the backup file's contents with credentials stripped — that would move #457 forward considerably. Thanks for reporting it; the silent-discard behavior was worth surfacing. Reviewed as part of a maintainer PABCD pass against |
통합(dev push 완료): #437 803807a, #460 74ddd96, #431 82a47db, #405 be16c1d, #468 후속 테스트 bcaf029. close: 통합 4건 + 통합 없이 1건(#459, 설계 충돌). #457은 OPEN 유지. 이슈 close 0건 — dev에서 고쳐졌는데 안 닫힌 이슈가 없었다. 무효화: WP4(#466)와 WP6(#467)은 동료가 직접 머지했고, 우리 분석이 지적한 결함을 동료 커밋이 같은 방향으로 해소했다. 보안 보류 5건에 신규 발견 게시: #429 재분류, #355 OAuth 토큰 목적지, #424 유료 브리지 기본 ON, #408 설치 락 해제, #403 4분할 요청. A-gate는 사이클마다 3~5라운드를 돌았다. 반복된 원인은 계획서에 테스트나 코드를 적으면서 실물 소스를 확인하지 않은 것이었다.
… at diff level Two issues reproduce against current dev and get implementation phases; the rest are recorded with why they are not code fixes this unit can close. The roadmap went through six adversarial audit rounds before any code, and two findings changed the shape of the work rather than its details: - #477's suggested fix (delete the early return in writeJournal) would snapshot an already-injected config as the user's original, making opencodex routing survive 'ocx stop'. The guard is load-bearing; ownership is now checked from the bytes being journaled instead of trusted from the caller. - #457's obvious fix (allowBaseUrlOverride on the Beijing entry) shipped once, was reverted twice, and was refused again when PR #459 closed on 2026-07-26. Beijing and International are deliberately separate providers, so the repair is a config migration, not an override flag. The migration is split: the provider-id reference rewriter is its own phase, because a missed reference silently orphans a setting or invalidates a combo target, which makes loadConfig discard the whole config. Also recorded: an integrity rule added under audit pressure would have thrown out of startServer and refused to boot the proxy after a legitimate config edit. It was removed in favour of copy-verify-link publication.
Closes #457. A provider rename left users with `alibaba-token-plan` holding an international (ap-southeast-1) endpoint. That entry is pinned to cn-beijing and takes no baseUrl override, so the router discards the saved URL and sends an international key to Beijing: 401 on every request. #464 made the discard visible; this makes it recoverable. The obvious fix is not available. Giving the Beijing entry `allowBaseUrlOverride` shipped once, was backed out twice, and was refused again when PR #459 closed on 2026-07-26: `alibaba-token-plan` and `alibaba-token-plan-intl` are deliberately separate products with different model lists, context windows, modality maps and dashboards. A URL override carries none of that, so the endpoint would work while the catalog described the wrong product — and the provider's key would become sendable to whatever destination happened to be saved. The maintainer named the right shape when closing that PR: a migration, not a flag. So the entry moves to the id whose registry entry actually serves its endpoint. The destination is built with `providerConfigSeed`, the same function `ocx provider add` uses, so the migrated row is indistinguishable from one created against the international provider directly; only credentials and genuinely user-owned fields are overlaid. A user-chosen `defaultModel` survives only if the destination can serve it. Every reference to the old id is re-pointed through `rewriteProviderReferences`. The migration refuses rather than guesses in two cases: when the destination provider row already exists, and when the rewrite reports an occupied destination key. Merging two credential sets or two context caps is a user decision. A pre-migration snapshot is taken first. Relying on the OpenAI tier backup was considered and rejected: it only writes when its own projection changes, which is not the common path, so a config already at tier v2 would migrate with no rollback point at all. The snapshot is copy-verify-link — exclusive creation alone would let a crash mid-copy leave a truncated file that the next run accepts as valid. An existing snapshot is reused as-is rather than validated against the current config: that check would refuse to boot the proxy after a legitimate edit following an aborted run, and a safety net must not become the thing that stops the product. Fail-closed on a backup write failure: no rollback point, no credential rewrite. 15 tests across three files.
Fixes #457
Problem
The
alibaba-token-planregistry entry (Beijing) lacksallowBaseUrlOverride, so the router silently discards any user-savedbaseUrland forces the Beijing endpoint (router.ts: registry URL wins unless the entry is a template or opts into override).After the July provider migration merged user configs into the id
alibaba-token-plan, configs holding an international (ap-southeast-1) baseUrl are still routed tocn-beijing→ all requests fail with 401, with no warning that the saved URL was ignored.Fix
Mirror the existing
qwen-cloudpattern:src/providers/base-url-choices.ts: addALIBABA_TOKEN_PLAN_BEIJING_BASE_URL,ALIBABA_TOKEN_PLAN_INTL_BASE_URL, andALIBABA_TOKEN_PLAN_BASE_URL_CHOICES(beijing/intl/custom).src/providers/registry.ts:alibaba-token-plangetsallowBaseUrlOverride: true+baseUrlChoices, and uses the Beijing constant (value unchanged).Default resolution is unchanged when no user
baseUrlis set — only configs with a saved URL now have it honored, and the GUI can offer the region choice.Tests
tests/alibaba-token-plan-endpoints.test.tsmirrors the qwen-cloud endpoint tests (registry shape, presets projection,matchBaseUrlChoicemapping).tests/provider-registry-parity.test.ts.tsc --noEmitclean, full suite: 3231 pass / 0 fail, privacy scan passed.Notes