feat: add combo rename and public aliases#238
Conversation
|
This revision was reviewed with GPT-5.6 Sol at high reasoning effort. GPT-5.6 Sol High adversarial validation: passed. The final review covered the complete 28-file merge diff at commit
Both findings were fixed and regression-tested. The same GPT-5.6 Sol High reviewer then rechecked the complete diff on the latest base and reported No findings. Final validation:
The final changes in this PR were made from the GPT-5.6 Sol High findings. |
aa86686 to
4d25b37
Compare
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
This is a substantial and potentially useful feature, and no backdoor/code-execution issue was found. It is still a draft with no CI, and the bare-alias policy needs explicit resolution: aliases are resolved before provider routing and can shadow non-OpenAI provider model IDs in routing and the public catalog. Please document and test the intended collision policy (or require namespaced aliases), then run the full matrix before marking ready.
Other repairs:
|
|
Thanks for the update. I rechecked the PR timeline: the current head is still 4d25b37, committed before my review, so there is no new code addressing the requested change. The added screenshot and external-model review summary do not resolve the blocking policy issue. Please explicitly address bare alias collisions with provider model IDs. Either:
After that, push the changes, run the full CI matrix, and mark the PR ready for review. The PR remains CHANGES_REQUESTED until those items are present. |
4d25b37 to
0cbc947
Compare
0cbc947 to
3535632
Compare
|
Addressed in
The requested regression matrix now uses an exact
Validation completed locally:
I am marking the PR ready for review now so the full CI matrix can run on the updated head. Update: GitHub created the Cross-platform CI and React Doctor runs for |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 353563247b
ℹ️ 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".
|
|
||
| export function tryPickComboModel(config: OcxConfig, modelId: string): ComboPick | null { | ||
| const comboId = parseComboModelId(modelId); | ||
| const comboId = resolveComboId(config, modelId); |
There was a problem hiding this comment.
When a combo target's concrete selector is also a combo alias (for example combo A targets a/m1 and combo B uses alias a/m1), resolving aliases here makes routeModel() treat the child request from handleComboResponses as combo B even though options.comboAttempt only skipped the outer failover loop. The attempt recorded for A can be sent to B's provider/model, or fail because B has no available targets, so combo failover/round-robin no longer honors its configured targets; combo child routing needs a no-alias path.
Useful? React with 👍 / 👎.
| // Combo management stores the public alias, while canonical `combo/<id>` references | ||
| // remain valid for backward compatibility through slugEquals below. | ||
| if (m.alias !== undefined && stored === catalogModelSlug(m)) return false; | ||
| if (slugEquals(stored, m.provider, m.id)) return false; |
There was a problem hiding this comment.
When disabledModels contains an aliased combo, this removes the combo from the catalog input while the provider model with the same public slug remains eligible, so buildCatalogEntries can advertise the provider row under that slug. Requests for that picker row still resolve through resolveComboId() first because routing ignores disabledModels, so users who disable a shadowing combo can be shown a restored provider selector that actually hits the hidden combo; either keep the provider row hidden or make disabled combo aliases yield to the provider.
Useful? React with 👍 / 👎.
| if (config.claudeCode) { | ||
| const claudeCode = { ...config.claudeCode }; | ||
| for (const field of ["model", "smallFastModel"] as const) { | ||
| if (claudeCode[field]) claudeCode[field] = migrateAgentReference(claudeCode[field]); | ||
| } |
There was a problem hiding this comment.
When a combo alias/id changes while a web-search or vision sidecar is configured to use the old public model (webSearchSidecar.model, visionSidecar.model, or the Claude-specific overrides), this migration leaves those model references unchanged even though it rewrites disabled/subagent/injection/shadow/Claude main fields. The next sidecar request will try the retired alias/canonical id and either fall back to the default provider or fail, so renames are not atomic for sidecar users; include the sidecar model fields in this migration.
Useful? React with 👍 / 👎.
| .filter(m => typeof m.slug === "string" | ||
| && !(m.slug as string).includes("/") | ||
| && m.owned_by !== COMBO_NAMESPACE |
There was a problem hiding this comment.
When routed model discovery returns empty, the sync code has a fallback that preserves existing routed rows, but bare combo aliases are now marked owned_by: "combo" and contain no /. This line removes them from the native preservation set, while the routed fallback below only keeps slash-containing slugs, so a transient discovery/cache failure deletes still-configured bare alias rows from the Codex catalog until a later successful sync restores them; treat combo-owned bare aliases as routed rows in the preservation path.
Useful? React with 👍 / 👎.
| const visibleRouted = [...new Set(models | ||
| .filter(m => ![...disabled].some(stored => | ||
| stored === catalogModelSlug(m) || slugEquals(stored, m.provider, m.id) | ||
| )) | ||
| .map(catalogModelSlug))]; |
There was a problem hiding this comment.
When this picker returns an aliased combo via catalogModelSlug(m), saving it stores the public alias in config.subagentModels. The Claude agent sync later looks up context windows using combo/<id>/Claude aliases derived from provider+id, not CatalogModel.alias, so an aliased combo with a large context window will generate ocx-* agents without the needed [1m] marker and Claude Code can under-account context/compact too late; either register alias keys in the context-window map or keep the canonical combo id for that downstream path.
Useful? React with 👍 / 👎.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Re-reviewed commit 3535632 after the requested alias-collision contract and regression matrix were added. The fix consistently applies combo precedence across routing, /v1/models, catalog output, rename, and deletion restoration. Full local suite: 3,454 tests passed; typecheck, GUI lint/build, privacy scan, and diff check passed. GitHub Ubuntu/macOS/Windows and React Doctor checks passed. Full 30-file security review found no malware, backdoor, credential exfiltration, auth bypass, or unsafe command/file/network execution. Approved for dev.
2 participants
Summary
Adds atomic combo renaming and configurable public model names, superseding the closed #214 implementation on the current
devbranch.Motivation
Combo IDs are internal configuration keys, but they currently leak into every public surface as
combo/<id>. Users need to rename a combo safely and choose the exact client-facing model name during creation or editing, without breaking existing configuration references.Features Added
aka, a custom prefix such asvendor/aka, or leave it blank for the defaultcombo/<id>.combo/<id>slug or the configured public alias./v1/models, the management API, and the GUI.Changes
/v1/modelsselectors while keeping exact combo-alias collisions deterministic./v1/models, Codex catalog output, alias rename, and combo deletion using an exactdeepseek/deepseek-chatcollision.disabledModels,subagentModels,injectionModel,shadowCallIntercept.model, and Claude Code model fields during rename or alias changes, then immediately refreshed generated Claude agent definitions.Benefits
combo/prefix.Testing
/v1/models, Codex catalog output, alias rename restoration, and combo deletion restoration fordeepseek/deepseek-chat.tests/server-auth.test.tsfailures reproduce on pure upstreamdev(fdacd146); the newer base commit is documentation-only.No findings.after its actionable findings were fixed and regression-tested; the later collision-contract update is supported by the explicit tests above.Example Configuration
{ "combos": { "coding": { "model": "openai/gpt-5.2-codex", "alias": "my-coding-model" } } }Both
combo/codingandmy-coding-modelroute to the same combo, while public catalog surfaces exposemy-coding-model.Related: #133
Supersedes: #214