From 18a514b579eef6fab925945cd988b3c5f37fc5c5 Mon Sep 17 00:00:00 2001 From: luvs01 Date: Sun, 30 Aug 2026 14:12:13 +0900 Subject: [PATCH 1/2] fix(slug): resolve self-namespaced native ids before the provider-qualified reading --- src/providers/slug-codec.ts | 16 ++++++++++++---- tests/slug-codec.test.ts | 35 ++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/src/providers/slug-codec.ts b/src/providers/slug-codec.ts index 47573b878c..7f529c6ee0 100644 --- a/src/providers/slug-codec.ts +++ b/src/providers/slug-codec.ts @@ -140,13 +140,21 @@ export function resolveSlugSelection( // selection as provider-qualified made `a/b` resolve against provider "a", so the same // collision reported ambiguous through the dash spelling and unambiguous through the slash // spelling — the exact asymmetry this resolver exists to remove. - const qualified = selection.startsWith(`${provider}/`) + const matched: string[] = []; + let exact: string | undefined; + const ids = [...knownIds]; + // A selection that starts with `/` is genuinely ambiguous: it reads as the + // provider-qualified form of `b`, but it is ALSO the native spelling of a self-namespaced + // id `provider/b`. Stripping the prefix unconditionally erased that second reading, so a + // published `acme/turbo` became unreachable while a sibling `turbo` silently absorbed the + // selection. The native id wins when the roster actually publishes it, because only then is + // the literal spelling known to name a real row. + const namesNativeId = ids.some(id => id === selection); + const qualified = !namesNativeId && selection.startsWith(`${provider}/`) ? selection : routedSlug(provider, selection); const selectionKey = slugEquivalenceKey(qualified); - const matched: string[] = []; - let exact: string | undefined; - for (const id of knownIds) { + for (const id of ids) { if (slugEquivalenceKey(routedSlug(provider, id)) !== selectionKey) continue; matched.push(id); if (id === selection || `${provider}/${id}` === selection) exact = id; diff --git a/tests/slug-codec.test.ts b/tests/slug-codec.test.ts index 93c780a74e..5482fcca17 100644 --- a/tests/slug-codec.test.ts +++ b/tests/slug-codec.test.ts @@ -369,5 +369,38 @@ describe("#2491 one selection resolver reports what it actually matched", () => // Encoded-only: the operator did not type the native spelling. expect(match.exact).toBeUndefined(); }); -}); + /** + * A native id may be self-namespaced: provider "acme" publishing `acme/turbo`. Its literal + * spelling is indistinguishable from the provider-qualified form of a sibling `turbo`, so + * treating every `/…` selection as qualified made the published row unreachable + * and, worse, silently redirected the selection onto the sibling. `ocx models remove` reads + * its match from this resolver, so the redirect targets a destructive command. + */ + test("a self-namespaced native id wins over the provider-qualified reading", () => { + const match = resolveSlugSelection("acme", "acme/turbo", ["acme/turbo", "turbo"]); + expect(match.matched).toEqual(["acme/turbo"]); + expect(match.exact).toBe("acme/turbo"); + expect(match.ambiguous).toBe(false); + }); + + test("a self-namespaced native id resolves even when it is the only known id", () => { + const match = resolveSlugSelection("acme", "acme/turbo", ["acme/turbo"]); + expect(match.matched).toEqual(["acme/turbo"]); + expect(match.exact).toBe("acme/turbo"); + }); + + test("the sibling is still reachable through its own bare spelling", () => { + const match = resolveSlugSelection("acme", "turbo", ["acme/turbo", "turbo"]); + expect(match.matched).toEqual(["turbo"]); + expect(match.exact).toBe("turbo"); + }); + + test("the provider-qualified reading still applies when no native id matches literally", () => { + // Nothing is spelled `acme/turbo` natively here, so the selection keeps its qualified + // meaning and resolves against the encoded roster as before. + const match = resolveSlugSelection("acme", "acme/turbo", ["turbo"]); + expect(match.matched).toEqual(["turbo"]); + expect(match.exact).toBe("turbo"); + }); +}); From cd0be49d3e20244fe3bf12f16e86c088f514db39 Mon Sep 17 00:00:00 2001 From: luvs01 Date: Sun, 30 Aug 2026 14:22:19 +0900 Subject: [PATCH 2/2] fix(cli): resolve models remove against the whole provider roster --- src/cli/models.ts | 18 +++++++++++++-- tests/cli-models.test.ts | 48 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/src/cli/models.ts b/src/cli/models.ts index c9920e3c1d..a1472c99e2 100644 --- a/src/cli/models.ts +++ b/src/cli/models.ts @@ -285,11 +285,25 @@ async function handleCustomRemove(args: string[]): Promise { // the right default for a destructive command. const separator = target.indexOf("/"); const selectedProvider = separator >= 0 ? target.slice(0, separator) : undefined; + // Resolve ONCE against the provider's whole roster, then map the decision back onto rows. + // Calling the resolver per row with a singleton roster hid every cross-row fact it needs: + // a self-namespaced `acme/turbo` and a sibling `turbo` each matched their own singleton, + // so the command saw two matches and aborted as ambiguous even though the selector names + // one row exactly. + const rosterMatched = selectedProvider === undefined + ? undefined + : resolveSlugSelection( + selectedProvider, + target, + existing.filter(model => model.provider === selectedProvider).map(model => model.modelId), + ); + // Deliberately admit the whole matched set rather than narrowing to `exact`: an encoded + // selector that spans a real collision must still abort below. Removal stays exact-or-refuse. + const admitted = new Set(rosterMatched?.matched ?? []); const matchingIndexes = existing.flatMap((model, index) => { if (selectedProvider === undefined) return model.id === target ? [index] : []; if (model.provider !== selectedProvider) return []; - const resolved = resolveSlugSelection(selectedProvider, target, [model.modelId]); - return resolved.matched.length > 0 ? [index] : []; + return admitted.has(model.modelId) ? [index] : []; }); if (matchingIndexes.length === 0) fail(`custom model "${target}" not found`); if (matchingIndexes.length > 1) { diff --git a/tests/cli-models.test.ts b/tests/cli-models.test.ts index c9de6f6b40..c311ab241d 100644 --- a/tests/cli-models.test.ts +++ b/tests/cli-models.test.ts @@ -516,4 +516,52 @@ describe("#2491 the removal selector uses the shared equivalence relation", () = rmSync(dir, { recursive: true, force: true }); } }); + + /** + * A provider may publish a native id that is itself namespaced under its own name, so + * `acme` owning `acme/turbo` makes the selector `acme/turbo` name that row exactly while + * ALSO reading as the provider-qualified form of a sibling `turbo`. The resolver was called + * once per row with a singleton roster, so each row matched its own reading, the command saw + * two matches and aborted — the exact native spelling could never remove its own row. + */ + test("a self-namespaced selector removes the row it names exactly", () => { + const { dir } = freshConfig({ + customModels: [ + { id: "11111111-1111-4111-8111-111111111111", provider: "acme", modelId: "acme/turbo" }, + { id: "22222222-2222-4222-8222-222222222222", provider: "acme", modelId: "turbo" }, + ], + }); + try { + const result = runCli(["models", "remove", "acme/turbo", "--yes"], { OPENCODEX_HOME: dir }); + expect(result.status).toBe(0); + const config = JSON.parse(readFileSync(join(dir, "config.json"), "utf8")); + // The sibling survives: the selector named the native row, not the qualified reading. + expect(config.customModels).toEqual([ + expect.objectContaining({ provider: "acme", modelId: "turbo" }), + ]); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + /** + * Guards the narrow path: with no sibling there is no collision, so this already worked and + * must keep working. It pins the case the resolver-level fix covers, so a future change that + * narrows the roster lookup cannot silently make a sole self-namespaced row unreachable. + */ + test("a self-namespaced row is removable when it is the provider's only row", () => { + const { dir } = freshConfig({ + customModels: [ + { id: "11111111-1111-4111-8111-111111111111", provider: "acme", modelId: "acme/turbo" }, + ], + }); + try { + const result = runCli(["models", "remove", "acme/turbo", "--yes"], { OPENCODEX_HOME: dir }); + expect(result.status).toBe(0); + const config = JSON.parse(readFileSync(join(dir, "config.json"), "utf8")); + expect(config.customModels).toBeUndefined(); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); });