Skip to content

fix(cli): route models remove through the shared slug relation - #2596

Merged
lidge-jun merged 1 commit into
devfrom
codex/slug-unify-2491
Aug 25, 2026
Merged

lidge-jun merged 1 commit into
devfrom
codex/slug-unify-2491

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Aug 25, 2026 •

Copy link
Copy Markdown
Owner

Summary

Completes #2491, together with #2594.

slugEquals compares the raw and encoded spellings of one id, so a removal selector
written in the native slash form matched only the slash row while the encoded form matched
both. Catalog filtering and persisted sync had already agreed on the collision class through
slugEquivalenceKey; ocx models remove disagreed with both on the same config. That is
the inconsistency the issue reports.

Demonstrated directly against a config publishing both spellings:

p/a-b | slugEquals -> ["a/b","a-b"] | resolver -> ["a/b","a-b"]  ambiguous=true
p/a/b | slugEquals -> ["a/b"]       | resolver -> ["a/b","a-b"]  ambiguous=true

Removal now goes through resolveSlugSelection (added in #2594), so all three surfaces see
the same collision.

Ambiguous selectors still refuse, deliberately. That behaviour is unchanged — what
changes is that the command now refuses in a case where it previously deleted a row
silently. For a destructive command, refusing on an ambiguous selector is the right default,
so the relation was widened without making removal tolerant.

Not changed

decodeRoutedModelIdOrThrow keeps its own encode-collision check. It answers a different
question — may this request be routed at all — and it already fails closed on ambiguity, so
folding it into the selection resolver would trade a hard refusal for a softer match on the
request path. Recorded rather than silently left out.

Verification

Driven red: replacing the shared resolver with the narrow exact-only relation fails 3 of the
21 cases.

# narrow exact-only relation
$ bun test tests/cli-models.test.ts
 18 pass, 3 fail

# with the shared resolver
$ bun test tests/cli-models.test.ts
 21 pass, 0 fail

$ bun test tests/cli-models.test.ts tests/slug-codec.test.ts tests/selected-models.test.ts tests/router.test.ts
 89 pass, 0 fail, 234 expect() calls

$ bun x tsc --noEmit
(clean)

New cases pin both directions: a native-slash selector now sees the same collision the
encoded one does, and an unambiguous slash selector still removes its row — widening the
relation must not make ordinary removal ambiguous.

Checklist

  • Targets dev
  • Based on the current dev head
  • Destructive-command behaviour reviewed explicitly, not incidentally
  • Driven red first, then green
  • Typecheck clean
  • No secrets or account identifiers in the diff

Summary by CodeRabbit

  • Bug Fixes

    • Improved custom model removal using slash-form selectors.
    • Ambiguous selectors are now rejected without deleting any models.
    • Unambiguous slash-form selectors continue to remove the intended model.
    • UUID-based selectors retain exact matching behavior.
  • Tests

    • Added coverage for ambiguous and unambiguous custom model removal scenarios.

slugEquals compares the raw and encoded spellings of ONE id, so a removal
selector written in the native slash form matched only the slash row while
the encoded form matched both. Catalog filtering and persisted sync had
already agreed on the collision class through slugEquivalenceKey; this
command disagreed with both on the same config, which is the inconsistency
#2491 reports.

Removal now resolves through resolveSlugSelection, so all three surfaces
see the same collision. Behaviour on an ambiguous selector is unchanged and
deliberately so: it still refuses, which is the right default for a
destructive command - the widening makes it refuse in a case where it
previously deleted a row silently.

Driven red: with the narrow exact-only relation, 3 of the 21 cases fail.
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner August 25, 2026 17:43
@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@lidge-jun
lidge-jun merged commit 3a8baea into dev Aug 25, 2026
21 of 23 checks passed
@lidge-jun
lidge-jun deleted the codex/slug-unify-2491 branch August 25, 2026 17:43
@coderabbitai

coderabbitai Bot commented Aug 25, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4500cc2f-36a1-49bd-a9da-b4983ee88944

📥 Commits

Reviewing files that changed from the base of the PR and between b6d5509 and 51fd817.

📒 Files selected for processing (2)
  • src/cli/models.ts
  • tests/cli-models.test.ts

📝 Walkthrough

Walkthrough

Custom-model removal now uses shared slug resolution for slash-form selectors. CLI tests cover collisions between native and encoded selectors, plus successful removal for unambiguous selectors.

Changes

Custom model removal

Layer / File(s) Summary
Resolve removal selectors
src/cli/models.ts
At lines 14 and 280–290, slash-form selectors use resolveSlugSelection with each model’s provider and model ID. Non-slash selectors retain exact custom-model ID matching. Existing not-found and ambiguity checks remain in place.
Validate selector behavior
tests/cli-models.test.ts
At lines 437–462, tests verify that native and encoded slash selectors produce an ambiguity error without deleting either model. At lines 464–481, tests verify that an unambiguous native-slash selector removes only the matching model.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: ingwannu, ntdatt812

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/slug-unify-2491

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 51fd817861

ℹ️ 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".

Comment thread src/cli/models.ts
Comment on lines +287 to +289
if (!target.includes("/")) return model.id === target ? [index] : [];
const resolved = resolveSlugSelection(model.provider, target, [model.modelId]);
return resolved.matched.length > 0 ? [index] : [];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restrict slug resolution to the selected provider

When ocx models remove wanted/x --yes is used and wanted has no matching custom row, a custom model under another provider whose ID is wanted/x or wanted-x is matched here: resolveSlugSelection treats a selector not prefixed by the current model's provider as a bare native ID. The command can therefore delete a model belonging to the wrong provider instead of reporting "not found". Parse the provider prefix once and skip rows from other providers before applying the shared equivalence relation.

Useful? React with 👍 / 👎.

tarunravi pushed a commit to tarunravi/opencodex that referenced this pull request Sep 14, 2026
…-jun#2596)

slugEquals compares the raw and encoded spellings of ONE id, so a removal
selector written in the native slash form matched only the slash row while
the encoded form matched both. Catalog filtering and persisted sync had
already agreed on the collision class through slugEquivalenceKey; this
command disagreed with both on the same config, which is the inconsistency
lidge-jun#2491 reports.

Removal now resolves through resolveSlugSelection, so all three surfaces
see the same collision. Behaviour on an ambiguous selector is unchanged and
deliberately so: it still refuses, which is the right default for a
destructive command - the widening makes it refuse in a case where it
previously deleted a row silently.

Driven red: with the narrow exact-only relation, 3 of the 21 cases fail.
agentHits pushed a commit to agentHits/opencodex that referenced this pull request Sep 17, 2026
…-jun#2596)

slugEquals compares the raw and encoded spellings of ONE id, so a removal
selector written in the native slash form matched only the slash row while
the encoded form matched both. Catalog filtering and persisted sync had
already agreed on the collision class through slugEquivalenceKey; this
command disagreed with both on the same config, which is the inconsistency
lidge-jun#2491 reports.

Removal now resolves through resolveSlugSelection, so all three surfaces
see the same collision. Behaviour on an ambiguous selector is unchanged and
deliberately so: it still refuses, which is the right default for a
destructive command - the widening makes it refuse in a case where it
previously deleted a row silently.

Driven red: with the narrow exact-only relation, 3 of the 21 cases fail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant