Repository navigation
feat(registry): derive selector decode hints from a classified registry - #5177
Conversation
An encoded selector decodes against the native model ids a provider is known to accept. That set was a hand-written list of eight registry maps, so an id declared only in an unlisted map could not be decoded, and every new model-keyed field had to be remembered by whoever added it. src/providers/registry/model-ids.ts now classifies every field of ProviderRegistryEntry by what its KEYS mean, and knownModelIdsForProvider asks the registry instead of listing maps itself. Fifteen direct maps contribute keys, keyAuthServiceTier contributes its nested modelSupportsServiceTier keys, and the remaining seventy-five fields are classified as carrying no identity. Classifying the non-model fields is the point: the satisfies clause makes a new registry field a type error until somebody decides what its keys mean, rather than letting it default to invisible. Two checks, deliberately different. The satisfies clause proves the classification is exhaustive over the interface when this compiles; a parity case proves the fields the shipped entries actually carry are all classified. A new interface field fails the first, a shipped entry carrying something unclassified fails the second. Hints stay identity evidence. They publish no catalog row, grant no availability or entitlement, and confer no transport authority: registry transport identity is still checked before hints are read, an ambiguous selector is still rejected rather than guessed, and virtualModels contributes its selectable keys only, never the wire target a client may not name. Coverage: each formerly omitted direct map and the nested map independently seed a decode, with negatives for unknown pass-through, ambiguous rejection, transport mismatch and no catalog row from a hint alone; the parity suite adds per-map collection, non-model records contributing nothing, stable first-seen-wins ordering, and the shipped meta-muse entry recovering its ids from maps alone. The classifier is imported directly rather than through the registry facade, which is at its file-size cap. No layout inventory changes, since no test file was added. routedProviderConfig and the static policy paths are untouched. Local checks: NOT RUN.
… level The route-level table proved the seven maps the old hand-written list omitted. The other eight rested on helper parity alone, which is a weaker claim: after this change every one of them is reached through the same classification, so they belong in the same public table. It now covers all fifteen direct maps plus the nested key-auth map. Also asserts the helper's documented contract rather than assuming it: the returned array is frozen and rejects a push, and the registry entry is byte-identical afterwards. That matters because the registry is process-wide shared state, and a decode path that sorted an entry's maps in place would be editing it for every later caller. A shipped-entry route case was written and then dropped rather than kept. The Muse ids contain no slash, so encoding them is the identity function and the route would pass them through whether or not any hint existed. It proved nothing about decoding, and the fifteen synthetic slash-bearing rows already prove that at the public level. The shipped-entry evidence stays where it is honest: the parity suite asserts the helper recovers those ids from the classified maps, which is meaningful because the helper never reads the models list even though this entry also declares them there. No production change, no new fixture mechanism, no cap or layout movement. Local checks: NOT RUN.
|
✅ Deterministic PR hygiene checks passed. |
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change classifies all ChangesRegistry selector hints
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Selector as Selector decoder
participant Router as knownModelIdsForProvider
participant Registry as registryModelIdKeys
participant Catalog as Catalog
Selector->>Router: Request known model IDs
Router->>Registry: Extract classified registry keys
Registry-->>Router: Return deduplicated native model IDs
Router-->>Selector: Return decode hints
Selector->>Catalog: Decode without adding a catalog row
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
The decode-hint loop asserted the entry to Record<string, unknown> to index it by field name. ProviderRegistryEntry is an interface, so it carries no implicit index signature and tsc rejected the conversion as insufficiently overlapping (TS2352 at model-ids.ts:152), which failed Typecheck and the test that spawns a real tsc over the production graph. Iterating the classification's own keys as keyof ProviderRegistryEntry keeps the read typed: the record-keys branch indexes the entry by that key and guards the value as a non-null object, and the nested role reads keyAuthServiceTier by name because that field is its only owner. Same fields, same order, same keys; no assertion through unknown and no behavior change. local checks: NOT RUN
|
Maintainer integration into dev under MAINTAINERS.md by the authenticated project owner, not a self-approval. Reviewed head Current dev Native-stack membership is empty. Actor, dev base and exact head were revalidated. No local suites, typecheck, build or runtime execution were performed. Issue #5115 closes only after actual dev landing and ancestry verification. |
1 participant
Summary
An encoded provider/model selector is decoded against the native model ids a provider is known to accept. That set was a hand-written list of eight registry maps in
knownModelIdsForProvider, so an id declared only in an unlisted identity-bearing map could not be decoded, and every new model-keyed registry field had to be remembered by whoever added it.src/providers/registry/model-ids.tsnow classifies every field ofProviderRegistryEntryby what its keys mean, and the router asks the registry instead of listing maps itself. Fifteen direct maps contribute keys,keyAuthServiceTiercontributes its nestedmodelSupportsServiceTierkeys, and the remaining seventy-five fields are classified as carrying no identity.Classifying the non-model fields is the point. The
satisfiesclause makes a new registry field a type error until somebody decides what its keys mean, rather than letting it default to invisible. That is the difference between this and a longer hand-written list.Two exhaustiveness checks, deliberately different
satisfies Record<keyof ProviderRegistryEntry, RegistryFieldModelIdRole>clause proves the classification covers the interface, at compile time.A new interface field fails the first. A shipped entry carrying something unclassified fails the second. Neither substitutes for the other.
What a hint is not
Decode hints are identity evidence only. They publish no catalog row, grant no availability or entitlement, and confer no transport authority: registry transport identity is still checked before hints are read, and an ambiguous selector is still rejected rather than guessed.
virtualModelscontributes its selectable keys only, never thewireModelIdtransport target, which is not something a client may name.Closes #5115.
Verification
ocxcommand. Exact-head hosted CI on this PR is the executable evidence.tests/codex-integration/slug-codec.test.ts: all fifteen classified direct maps and the nested key-auth map each independently seed a decode, using distinct synthetic slash-bearing ids so the encoded selector round-trip is meaningful. Negatives cover unknown pass-through, ambiguous-selector rejection, transport mismatch inheriting nothing, and no catalog row from a hint alone. The helper contract is asserted too: the returned array is frozen and rejects a push, and the registry entry is byte-identical afterwards, which matters because the registry is process-wide shared state.tests/providers/provider-registry-parity.test.ts: runtime classification coverage over the shipped registry, per-map independent collection, non-model records (staticHeaders,reasoningEffortMap, the repair object) contributing nothing, stable first-seen-wins ordering in registry-field order, and a shipped entry whose ids are recovered from its classified maps even though the helper never reads itsmodelslist.Checklist
Summary by CodeRabbit
Enhancements
Documentation