Repository navigation
feat(slug): one selection resolver that reports what it matched (#2491 partial) - #2594
Conversation
#2491 names four slug-equivalence relations. Two of them - catalog visibility and persisted sync - already share slugEquivalenceKey, so they agree with each other; what neither can do is tell a caller that a selection was AMBIGUOUS rather than exact. The lossiness is externally forced: Codex's models-manager tolerates exactly one slash, so a/b and a-b must share an encoded form. This does not try to remove that. It adds resolveSlugSelection, which keeps the tolerant matching and additionally reports the exact id and whether the selection admitted more than one, so a caller holding a complete known-id set can prefer the exact row instead of granting the whole collision class. Writing its test surfaced a real asymmetry in the first version: a slash in a selection is ambiguous on its own, since p/a-b is provider-qualified while a/b is a bare native id containing a slash. Treating every slash-bearing selection as qualified made the same collision report ambiguous through one spelling and unambiguous through the other.
|
✅ Deterministic PR hygiene checks passed. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesSlug selection resolution
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
![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: ef36b3d637
ℹ️ 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".
| const qualified = selection.startsWith(`${provider}/`) | ||
| ? selection | ||
| : routedSlug(provider, selection); |
There was a problem hiding this comment.
When a bare native ID begins with the provider name, this prefix test necessarily treats it as a fully routed slug. For example, resolveSlugSelection("openai", "openai/gpt-5.5", ["openai/gpt-5.5"]) returns no match, and if gpt-5.5 is also known it incorrectly selects that different model and marks it exact. This is a realistic form for provider-scoped selectedModels, so determine both interpretations from knownIds and report their ambiguity, or require the caller to specify whether the selection is qualified rather than inferring it from the prefix.
Useful? React with 👍 / 👎.
…e-jun#2594) lidge-jun#2491 names four slug-equivalence relations. Two of them - catalog visibility and persisted sync - already share slugEquivalenceKey, so they agree with each other; what neither can do is tell a caller that a selection was AMBIGUOUS rather than exact. The lossiness is externally forced: Codex's models-manager tolerates exactly one slash, so a/b and a-b must share an encoded form. This does not try to remove that. It adds resolveSlugSelection, which keeps the tolerant matching and additionally reports the exact id and whether the selection admitted more than one, so a caller holding a complete known-id set can prefer the exact row instead of granting the whole collision class. Writing its test surfaced a real asymmetry in the first version: a slash in a selection is ambiguous on its own, since p/a-b is provider-qualified while a/b is a bare native id containing a slash. Treating every slash-bearing selection as qualified made the same collision report ambiguous through one spelling and unambiguous through the other.
…e-jun#2594) lidge-jun#2491 names four slug-equivalence relations. Two of them - catalog visibility and persisted sync - already share slugEquivalenceKey, so they agree with each other; what neither can do is tell a caller that a selection was AMBIGUOUS rather than exact. The lossiness is externally forced: Codex's models-manager tolerates exactly one slash, so a/b and a-b must share an encoded form. This does not try to remove that. It adds resolveSlugSelection, which keeps the tolerant matching and additionally reports the exact id and whether the selection admitted more than one, so a caller holding a complete known-id set can prefer the exact row instead of granting the whole collision class. Writing its test surfaced a real asymmetry in the first version: a slash in a selection is ambiguous on its own, since p/a-b is provider-qualified while a/b is a bare native id containing a slash. Treating every slash-bearing selection as qualified made the same collision report ambiguous through one spelling and unambiguous through the other.
1 participant
Summary
Partial work on #2491.
The issue names four relations. Checking them against the tree, two of them — catalog
visibility (
provider-fetch.ts) and persisted sync (sync.ts) — already shareslugEquivalenceKey, so they agree with each other. The real gap is different from theissue's framing: nothing lets a caller learn that a selection was ambiguous rather than
exact, so an over-grant is indistinguishable from an intended match.
The lossiness itself is externally forced and not something to remove here. Codex's
models-manager resolves per-model metadata with an exact one-slash rule, so
a/banda-bmust collapse to one Codex-facing form.This adds
resolveSlugSelection: same tolerant matching, but it also reports the exact idwhen the operator typed one and whether the selection admitted more than one row. A caller
holding a complete known-id set can then prefer the exact row instead of granting the whole
collision class; a caller working from an incomplete live roster keeps today's behaviour.
Returning the ambiguity rather than resolving it is deliberate — only the caller knows
whether its id set is complete, and silently narrowing would hide a published id whenever
discovery omitted it.
A real bug found while writing the test
The first version treated every slash-bearing selection as provider-qualified. That is
wrong:
p/a-bis qualified, buta/bis a bare native id that happens to contain a slash.So the same collision reported ambiguous through the dash spelling and unambiguous
through the slash spelling — precisely the asymmetry this resolver exists to remove. Fixed,
and both directions are now pinned.
Not done
The
ocx models removerelation (slugEquals) and the routing collision check(
decodeRoutedModelIdOrThrow) are not migrated in this PR. They answer different questions— exact-selector matching and encode-collision refusal — and folding them into one relation
needs a decision about whether
models removeshould become tolerant, which changesdestructive-command behaviour. #2491 stays open.
Verification
The existing test that pins the known lossy over-grant is unchanged, so this adds a capability
without altering current visibility behaviour.
Checklist
devdevheadSummary by CodeRabbit