Repository navigation
fix(subagents): accept visible bare native ids in pickerOrder - #6346
Conversation
PUT /api/subagent-models validated pickerOrder against visible routed slugs only, so the documented complete-picker ordering (a bare native id such as gpt-5.6-sol) could not be saved through the API. Visible, enabled native catalog rows are now accepted; disabled and unknown native ids are still rejected. Closes #6338
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. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
Maintainer integration into |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b51052460
ℹ️ 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".
| // Evaluate visibility AFTER discovery: a concurrent visibility write may have completed. | ||
| const visible = new Set(filterCatalogVisibleModels(models, config).map(catalogModelSlug).filter(slug => slug.includes("/"))); | ||
| // A bare native id orders the complete picker (docs: guides/model-ordering.md). | ||
| for (const row of nativeModelRows(config)) if (!row.disabled) visible.add(row.slug); |
There was a problem hiding this comment.
Gate native IDs on native passthrough availability
When the configuration has an enabled non-OpenAI provider, or providers.openai uses key auth/noncanonical URL, shouldIncludeNativeOpenAi() returns false and catalog convergence omits every bare native row, but nativeModelRows(config) still returns its enabled static rows. This therefore accepts and persists an unavailable bare ID such as gpt-5.5; because any bare ID activates complete-picker ordering, it can also reorder the remaining routed catalog even though the requested target never appears. Apply the same native-inclusion predicate used by catalog convergence before adding these rows to visible, and cover a key-auth or non-OpenAI-only configuration.
Useful? React with 👍 / 👎.
Summary
PUT /api/subagent-modelsvalidated everypickerOrderentry against visible routed slugs only. It rejected the bare native id that the documented complete-picker ordering requires (docs-site/src/content/docs/guides/model-ordering.md), so that ordering could only be created by hand-editing config. Enabled native catalog rows fromnativeModelRows(config)are now accepted alongside visible routed models. Disabled and unknown native ids are still rejected.Closes #6338
Verification
tests/routing/subagent-roster-retention.test.ts: the new case (visible native id saves; disabled native id is rejected without mutation) fails before the fix (25 pass / 1 fail) and passes after (26 pass / 0 fail). The invalid-body table now uses an unknown native id in place ofgpt-5.5.bun x tsc --noEmitpasses;subagent-roster-migration, test-layout and file-size guards pass. Twocombo-management-api"response-path strategy accounting" cases fail locally with and without this change; they are pre-existing in this environment.devtip before release.Checklist
Summary by CodeRabbit