Conversation
…forwarded role (lidge-jun#5334 follow-up) lidge-jun#5334 made the developer wire role tri-state: an undeclared destination folds it to system. Two suites asserting role:"developer" on the Chat wire were missed because they are about tool-result repair ordering and document parts, not role selection — declare the destination, per the convention the change established. Verified: both files fail on dev@600075d2 with system-for-developer wire roles and pass with the declaration. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: luvs01/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The migration's provider guard checked only the adapter, so a saved row repointed at a custom gateway but still carrying the generic openai-chat adapter had its context window overwritten with the registry's corrected value. Require the configured baseUrl to match the registry endpoint or a declared baseUrlChoices entry, the same ownership rule model-rename-migration applies. Co-Authored-By: Epinephrine <luvs01@hanmail.net>
|
Fixed in f9ef2af: |
|
Fixed in f9ef2af: the provider guard now takes the full row and requires the configured |
| const known = [entry.baseUrl, ...choices] | ||
| .filter((url): url is string => typeof url === "string") | ||
| .map(url => url.replace(/\/+$/, "")); | ||
| return known.includes(prov.baseUrl.replace(/\/+$/, "")); |
There was a problem hiding this comment.
🟡 Equivalent registry URLs can prevent the context-window repair
An official endpoint with whitespace or URL-equivalent casing fails this string comparison because normalization removes only trailing slashes. Valid saved rows then retain stale context windows.
Learn more
Provider baseUrl validation parses baseUrl.trim(), so surrounding whitespace and URL casing remain valid configuration. URL schemes and hostnames are case-insensitive, but this comparison treats them as ordinary case-sensitive text after removing only trailing slashes. The repository already has URL-aware endpoint normalization in normalizedProviderEndpoint, although it is currently private.
Example: A saved Alibaba row using HTTPS://TOKEN-PLAN.CN-BEIJING.MAAS.ALIYUNCS.COM/compatible-mode/v1/ still targets the registry host. This check rejects it, so qwen3.8-max remains at 983,616 instead of becoming 1,000,000.
Recommended fix: Reuse or extract the URL-aware normalization used by normalizedProviderEndpoint, and apply it to both registry choices and the configured URL. Add focused coverage for surrounding whitespace and scheme/host casing without lowercasing case-sensitive path components.
Was this helpful? React with 👍 or 👎 to provide feedback.
This branch has not been deployed
1 participant
Motivation
qwen3.8-maxas 1,000,000 tokens, but existing persisted Alibaba Token Plan provider rows could still contain the old 983,616 value and remain unchanged by enrichment, causing underreported context windows for upgraded installs.Description
src/providers/stale-context-window-migration.tsthat replaceqwen3.8-max === 983,616with1_000_000for bothalibaba-token-planandalibaba-token-plan-intlwhile preserving operator-chosen values and only touching rows that still match the registry adapter.tests/providers/context-window-seed-repair.test.tswith focused coverage that asserts both Alibaba variants are repaired by the migration and that operator overrides remain untouched.structure/providers-and-adapters.mdto document that the startup pass runs an exact-value repair for known-bad registry seeds (including the Alibabaqwen3.8-maxformer value) and to explain the migration contract.Testing
bun test tests/providers/context-window-seed-repair.test.ts, which passed (all cases green).bun run structure:checkandbun run typecheck, both of which succeeded.bun run test) to exercise review-ready gates; the focused changes remained green but the full run revealed unrelated failures in WebSocket relay, API-key attribution, and Cursor bearer-admission tests, so the PR contains only the minimal focused migration and its tests rather than attempting to address those unrelated failures.Codex Task