Refactor per-provider model lookup and restore catalog docs#52
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe catalog, builder, and related hashing/packing layers were refactored to use provider-scoped models: ModelSourceRow, ProviderHash, and ModelHash were removed and replaced by ProviderModelSource and Hash64-based composite provider+model hashing. model_table was replaced by provider_model_table; build/insert APIs now accept separate providers and provider_models and return provider_idx values. Packed table entry types, hash utilities, error variants, tests, and benchmarks were renamed/adjusted. Lookup semantics now require both provider_key and model_key and accessors were renamed to provider_count/provider_model_count. Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/llm-coding-tools-core/src/models/catalog/internal/builder.rs (2)
654-671: Preallocate large test vectors with known sizes.Line 658 and Line 689 build vectors with fixed loop bounds (
5463,5462). UsingVec::with_capacity(...)would avoid repeated reallocations in these stress/boundary tests.Suggested test-only improvement
- let mut providers = Vec::new(); + let mut providers = Vec::with_capacity(5463); for i in 0..5463usize { providers.push(provider_row( &format!("provider_{}", i), provider( "https://example.com", &["VAR1", "VAR2", "VAR3"], ProviderType::Azure, ), )); }- let mut providers = Vec::new(); + let mut providers = Vec::with_capacity(5462); for i in 0..5462usize { providers.push(provider_row( &format!("provider_{}", i), provider( "https://example.com", &["VAR1", "VAR2", "VAR3"], ProviderType::Azure, ), )); }As per coding guidelines,
src/**/*.rs: Preallocate collections when size is known or estimable usingVec::with_capacity(count).Also applies to: 686-708
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/models/catalog/internal/builder.rs` around lines 654 - 671, In the test too_many_total_env_vars_returns_error, preallocate the vectors used in the fixed-size loops to avoid repeated reallocations: replace the plain Vec::new() for the providers vector with Vec::with_capacity(5463) (matching the loop bound) and preallocate provider_models with the known size (e.g., Vec::with_capacity(1) or the actual count used); do the same for the other fixed-size loop around lines 686–708. Update the test code that builds providers via provider_row/provider(...) and provider_models via provider_model_row(...) so the allocations use with_capacity while leaving all test logic and calls to build_from_source unchanged.
139-144: Exact duplicate keys should short-circuit instead of reseeding.For deterministic duplicates, Line 139 and Line 227 collisions cannot be fixed by changing seeds, so retrying until
HashCollisionExhausteddoes avoidable work and returns a less precise error. Consider detecting exact duplicate source keys before hash-table insertion and returning a dedicated duplicate-key error immediately.Also applies to: 227-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/models/catalog/internal/builder.rs` around lines 139 - 144, The collision branch currently treats any occupied bucket as a hash collision and retries with a new seed (returning ModelCatalogBuildError::HashCollision / eventually HashCollisionExhausted), but for deterministic exact-duplicate source keys we should short-circuit: before attempting to reseed/rehash in the insertion logic (the code paths handling TableEntry::Occupied for LookupTableKind::Provider and the analogous provider/other-table insertion at the later block), compare the incoming source key (the canonical ID/string used as the lookup key) against the existing entry in the occupied bucket and if they are byte-for-byte equal return a new explicit duplicate-key error (e.g. ModelCatalogBuildError::DuplicateKey or add one) immediately instead of retrying with state.seed; only treat non-identical entries as true hash collisions that merit reseeding/retry. Ensure you reference TableEntry::Occupied, state.seed, LookupTableKind::Provider and the later occupied-handling block so the duplicate-key check is applied in both places.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/llm-coding-tools-core/src/models/catalog/internal/builder.rs`:
- Around line 654-671: In the test too_many_total_env_vars_returns_error,
preallocate the vectors used in the fixed-size loops to avoid repeated
reallocations: replace the plain Vec::new() for the providers vector with
Vec::with_capacity(5463) (matching the loop bound) and preallocate
provider_models with the known size (e.g., Vec::with_capacity(1) or the actual
count used); do the same for the other fixed-size loop around lines 686–708.
Update the test code that builds providers via provider_row/provider(...) and
provider_models via provider_model_row(...) so the allocations use with_capacity
while leaving all test logic and calls to build_from_source unchanged.
- Around line 139-144: The collision branch currently treats any occupied bucket
as a hash collision and retries with a new seed (returning
ModelCatalogBuildError::HashCollision / eventually HashCollisionExhausted), but
for deterministic exact-duplicate source keys we should short-circuit: before
attempting to reseed/rehash in the insertion logic (the code paths handling
TableEntry::Occupied for LookupTableKind::Provider and the analogous
provider/other-table insertion at the later block), compare the incoming source
key (the canonical ID/string used as the lookup key) against the existing entry
in the occupied bucket and if they are byte-for-byte equal return a new explicit
duplicate-key error (e.g. ModelCatalogBuildError::DuplicateKey or add one)
immediately instead of retrying with state.seed; only treat non-identical
entries as true hash collisions that merit reseeding/retry. Ensure you reference
TableEntry::Occupied, state.seed, LookupTableKind::Provider and the later
occupied-handling block so the duplicate-key check is applied in both places.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9305326-9d25-4ec7-a6f0-0f89921980fb
📒 Files selected for processing (11)
src/llm-coding-tools-core/benches/model_catalog_builder.rssrc/llm-coding-tools-core/src/models/catalog/internal/builder.rssrc/llm-coding-tools-core/src/models/catalog/internal/hash_utils.rssrc/llm-coding-tools-core/src/models/catalog/internal/mod.rssrc/llm-coding-tools-core/src/models/catalog/internal/model_hash.rssrc/llm-coding-tools-core/src/models/catalog/internal/packed_provider_model_table_entry.rssrc/llm-coding-tools-core/src/models/catalog/internal/provider_hash.rssrc/llm-coding-tools-core/src/models/catalog/mod.rssrc/llm-coding-tools-core/src/models/catalog/public/builder_types.rssrc/llm-coding-tools-core/src/models/catalog/public/entry.rssrc/llm-coding-tools-core/src/models/mod.rs
💤 Files with no reviewable changes (2)
- src/llm-coding-tools-core/src/models/catalog/internal/provider_hash.rs
- src/llm-coding-tools-core/src/models/catalog/internal/model_hash.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build and Test (Async/Tokio) (windows-latest, x86_64-pc-windows-msvc, false)
- GitHub Check: Build and Test (Blocking) (windows-latest, x86_64-pc-windows-msvc, false)
- GitHub Check: Build and Test (Blocking) (macos-latest, aarch64-apple-darwin, false)
- GitHub Check: Build and Test (Async/Tokio) (ubuntu-latest, x86_64-unknown-linux-gnu, false)
- GitHub Check: Build and Test (Blocking) (ubuntu-latest, x86_64-unknown-linux-gnu, false)
- GitHub Check: Build and Test (Async/Tokio) (macos-latest, aarch64-apple-darwin, false)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by calling.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Usememchrcrate for fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Use [TypeName] rustdoc links instead of backticks in documentation
Files:
src/llm-coding-tools-core/src/models/mod.rssrc/llm-coding-tools-core/src/models/catalog/public/entry.rssrc/llm-coding-tools-core/src/models/catalog/mod.rssrc/llm-coding-tools-core/benches/model_catalog_builder.rssrc/llm-coding-tools-core/src/models/catalog/internal/packed_provider_model_table_entry.rssrc/llm-coding-tools-core/src/models/catalog/internal/mod.rssrc/llm-coding-tools-core/src/models/catalog/public/builder_types.rssrc/llm-coding-tools-core/src/models/catalog/internal/builder.rssrc/llm-coding-tools-core/src/models/catalog/internal/hash_utils.rs
🧠 Learnings (1)
📚 Learning: 2026-02-07T22:53:26.067Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-02-07T22:53:26.067Z
Learning: Applies to src/**/*.rs : Document public items with `///` and add examples in docs where helpful
Applied to files:
src/llm-coding-tools-core/src/models/catalog/public/entry.rs
🔇 Additional comments (23)
src/llm-coding-tools-core/src/models/catalog/public/entry.rs (1)
4-12: LGTM!Documentation references correctly updated to reflect the rename from
ModelSourceRowto [ProviderModelSourceRow]. The rustdoc link syntax is properly used.src/llm-coding-tools-core/src/models/mod.rs (1)
6-9: LGTM!Public API surface correctly updated to export [
ProviderModelSourceRow] in place of the removedModelSourceRow. This breaking change is documented in the PR objectives.src/llm-coding-tools-core/src/models/catalog/public/builder_types.rs (3)
79-127: LGTM!The [
ProviderModelSourceRow] struct is well-designed with clear documentation. Thenew()method properly usesimpl Into<String>for flexibility, and theFromimplementation provides a consistent tuple-based construction path.
130-146: LGTM!The
LookupTableKind::ProviderModelvariant rename and itsDisplayimplementation are consistent with the new provider-model terminology throughout the codebase.
175-182: LGTM!The
ProviderKeyNotFoundForModelerror variant provides helpful diagnostic context by including both theprovider_keyandmodel_key, making debugging easier when provider-model rows reference unknown providers.src/llm-coding-tools-core/src/models/catalog/internal/hash_utils.rs (2)
22-33: LGTM!The
hash_provider_model_keyfunction correctly implements composite key hashing. Using0xFFas a delimiter is a sound choice since it's an invalid UTF-8 byte, guaranteeing thathash("providerA", "model")will never collide withhash("provider", "Amodel")due to key boundary confusion.
17-20: LGTM!The return type change from
ProviderHashto [Hash64] consolidates the hash wrapper types as intended by the PR.src/llm-coding-tools-core/benches/model_catalog_builder.rs (3)
16-17: LGTM!Good defensive addition of
debug_assert!(provider_count > 0)to guard against division by zero in the modulo operation at line 37.
34-66: LGTM!Dataset construction correctly updated to build
provider_modelswith proper provider association viaprovider_idx. The temperature calculation1.0 + ((cfg % 5000) as f32 * 0.001)produces values in range [1.0, 5.999], which is within the valid sampling limit of 6.5535.
69-77: LGTM!The
ModelCatalog::buildcall correctly uses the new API signature with&dataset.providersand&dataset.provider_models.src/llm-coding-tools-core/src/models/catalog/internal/packed_provider_model_table_entry.rs (2)
1-58: LGTM!The rename from
PackedModelTableEntryto [PackedProviderModelTableEntry] is consistent throughout. The bit layout (48-bit hash + 16-bit index = 64 bits) is verified by the compile-time assertion at line 20, and the 8-byte size is confirmed by the test.
84-89: LGTM!Good addition of test coverage for the
model_config_idx_val()method, verifying the [ModelIdx] roundtrip throughfrom_parts_idx.src/llm-coding-tools-core/src/models/catalog/internal/mod.rs (2)
9-13: LGTM!Hash utility re-exports correctly updated to expose
hash_provider_model_keyandprovider_model_table_entry_hash, replacing the removed model-centric equivalents.
27-34: LGTM!Module structure and re-exports consistently updated to the provider-model naming convention.
src/llm-coding-tools-core/src/models/catalog/mod.rs (6)
1-68: LGTM!Excellent module-level documentation explaining the dual-table architecture, memory savings rationale, and the composite key hashing strategy using
0xFFas delimiter. The collision probability tables and reseeding analysis provide valuable insight into the design decisions.
241-255: LGTM!The
ModelCatalogstruct correctly updated withprovider_model_tablefield of typeHashTable<PackedProviderModelTableEntry>, replacing the previous model-table field.
384-392: LGTM!The
is_empty()implementation correctly checks bothprovider_tableandprovider_model_table, ensuring it returnstrueonly when the catalog is completely empty.
394-437: LGTM!The
lookup()method correctly implements per-provider model lookup by:
- First resolving the provider via
lookup_provider_hash- Then resolving the model using the composite hash from
hash_provider_model_key(provider_key, model_key)This ensures the same model name under different providers returns distinct entries.
439-461: LGTM!The private helper methods
lookup_provider_hashandlookup_provider_model_hashcleanly encapsulate the hash-based lookup logic, properly truncating to 48 bits and delegating to the appropriate*_from_indexmethods.
591-645: LGTM!The test
lookup_is_provider_model_specificcorrectly validates that the same model key ("m1") under different providers ("alpha"vs"beta") returns distinct entries with provider-specific metadata. Themissing_provider_model_edge_returns_nonetest properly verifies that cross-provider lookups returnNone.src/llm-coding-tools-core/src/models/catalog/internal/builder.rs (3)
47-60: Strong capacity planning and migration wiring.Good job preallocating state and threading
provider_modelsthroughbuild_from_source/populate_tables_once; this keeps the new provider-model build path allocation-conscious and coherent.Also applies to: 74-77, 96-117
168-173: Provider-model validation and insertion order looks correct.Line 168 validates provider existence before model/config packing and provider-model table insert, which gives clear early failure semantics for bad source rows.
Also applies to: 214-239
491-512: Edge-case test coverage is materially improved.The added duplicate provider-model and unknown-provider tests are valuable guardrails for the new provider-model mapping semantics.
Also applies to: 558-578
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #52 +/- ##
==========================================
- Coverage 75.20% 74.58% -0.63%
==========================================
Files 67 65 -2
Lines 2029 2050 +21
==========================================
+ Hits 1526 1529 +3
- Misses 503 521 +18
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Make `ModelCatalog` require an exact provider+model match so the same model name can exist under multiple providers without ambiguity, while still deduplicating shared model metadata and config. Replace `ModelSourceRow` with `ProviderModelSourceRow`, update lookup-table and collision handling for `ProviderModelTable`, use allocation-free composite hashing for provider/model pairs, and remove the old standalone provider/model lookup APIs in favor of exact lookup plus provider iteration helpers. Also bring the catalog rustdoc back closer to the earlier wording while keeping it accurate for the new behavior, including the module rationale, collision and reseeding notes, numeric limits, memory layout details, and expanded public API docs.
Renamed ProviderSourceRow and ProviderModelSourceRow to ProviderSource and ProviderModelSource. Removed "row" terminology from all public APIs and internal code to simplify the mental model. Changes: - Renamed ProviderSourceRow → ProviderSource - Renamed ProviderModelSourceRow → ProviderModelSource - Updated internal functions: insert_provider_row → insert_provider, insert_provider_model_row → insert_provider_model, analyze_provider_rows → analyze_provider_sources - Updated test helpers and benchmark code - Updated documentation to remove "row" terminology - Updated doc links in entry.rs Benefits: - Simpler, clearer API naming without implementation-detail "row" terminology - More concise type names - Documentation focuses on what the types represent rather than internal storage
Keep sampling values in `Fixed4` form through internal catalog lookup paths so lookup and model reconstruction do not convert through `f32` unnecessarily. Changes: - Added crate-private raw `Fixed4` accessors for packed model config and public model entries - Updated catalog lookup and model reconstruction to pass stored sampling values through directly - Adjusted internal tests to validate raw fixed-point accessors instead of the removed float roundtrip path Benefits: - Removes unnecessary `Fixed4 -> f32 -> Fixed4` conversions in hot lookup paths - Keeps the public sampling API unchanged while making internal storage handling clearer
Sewer56
force-pushed
the
feat/provider-model-hash-table
branch
from
d652cd3 to
16e4cbc
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/llm-coding-tools-core/src/models/catalog/internal/builder.rs (1)
69-84:⚠️ Potential issue | 🟠 MajorFail fast on exact duplicate keys instead of treating them as hash collisions.
Right now a repeated
provider_keyor repeated(provider_key, model_key)deterministically hitsOccupied, bubbles out asHashCollision, and makes the outer loop retry every seed before returningHashCollisionExhausted. That turns a simple validation error into up to 256 full rebuild attempts and hides the real cause from callers. Please reject duplicates before inserting into the hash tables and surface dedicated build errors.Possible direction
-use ahash::AHashMap; +use ahash::{AHashMap, AHashSet}; ... fn populate_tables_once( state: &mut BuildState, providers: &[ProviderSource], provider_models: &[ProviderModelSource], ) -> Result<(), ModelCatalogBuildError> { let mut env_start: u16 = 0; let mut provider_idx_by_key: AHashMap<&str, ProviderIdx> = AHashMap::with_capacity(providers.len()); + let mut seen_provider_models: AHashSet<(&str, &str)> = + AHashSet::with_capacity(provider_models.len()); for provider in providers { + if provider_idx_by_key.contains_key(provider.provider_key.as_str()) { + return Err(ModelCatalogBuildError::DuplicateProviderKey { + provider_key: provider.provider_key.clone(), + }); + } let provider_info = &provider.provider; let env_count = provider_info.env_vars.len() as u8; let provider_idx = insert_provider( state, &provider.provider_key, env_start, env_count, provider_info.api_type, )?; provider_idx_by_key.insert(provider.provider_key.as_str(), provider_idx); env_start += u16::from(env_count); } for provider_model in provider_models { + if !seen_provider_models.insert(( + provider_model.provider_key.as_str(), + provider_model.model_key.as_str(), + )) { + return Err(ModelCatalogBuildError::DuplicateProviderModelKey { + provider_key: provider_model.provider_key.clone(), + model_key: provider_model.model_key.clone(), + }); + } insert_provider_model(state, &provider_idx_by_key, provider_model)?; } Ok(()) }You would also need matching
ModelCatalogBuildErrorvariants insrc/llm-coding-tools-core/src/models/catalog/public/builder_types.rs.Also applies to: 99-117, 163-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/models/catalog/internal/builder.rs` around lines 69 - 84, The build currently treats repeated provider_key or (provider_key, model_key) insert attempts as hash collisions which triggers retries; update the insertion logic (in populate_tables_once and places that attempt to insert into provider and model hash tables) to pre-check for existing keys and return new explicit errors (e.g., DuplicateProviderKey { key: String } and DuplicateProviderModelKey { provider: String, model: String }) instead of returning ModelCatalogBuildError::HashCollision; add corresponding variants to ModelCatalogBuildError in builder_types.rs and ensure build_from_source continues to retry only on genuine HashCollision while propagating these new duplicate-key errors immediately (no seed advance or retry via advance_seed_and_clear).src/llm-coding-tools-core/src/models/catalog/mod.rs (1)
118-157:⚠️ Potential issue | 🟡 MinorThe documented reseed limit is out of sync with the builder.
This section now says reseeding stops after 16 attempts, but
src/llm-coding-tools-core/src/models/catalog/internal/builder.rsstill usesMAX_SEED = u8::MAX, so the current implementation tries 256 seeds (0 through 255). Please align the public contract, the numeric-limits table, and the actual retry cap before release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/models/catalog/mod.rs` around lines 118 - 157, The docs claim a max reseed attempts of 16 but the implementation constant MAX_SEED in internal/builder.rs is set to u8::MAX (256 attempts); change the implementation to match the public contract by replacing MAX_SEED (or its initializer) with 16 (or a constant like MAX_SEED: u8 = 16) and run/adjust any code paths that iterate over seed values (e.g., functions in builder.rs that loop 0..=MAX_SEED or similar) so they use the new cap, then verify the numeric-limits table remains accurate and update any tests or comments that assumed 256 seeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/llm-coding-tools-core/src/models/catalog/internal/builder.rs`:
- Around line 69-84: The build currently treats repeated provider_key or
(provider_key, model_key) insert attempts as hash collisions which triggers
retries; update the insertion logic (in populate_tables_once and places that
attempt to insert into provider and model hash tables) to pre-check for existing
keys and return new explicit errors (e.g., DuplicateProviderKey { key: String }
and DuplicateProviderModelKey { provider: String, model: String }) instead of
returning ModelCatalogBuildError::HashCollision; add corresponding variants to
ModelCatalogBuildError in builder_types.rs and ensure build_from_source
continues to retry only on genuine HashCollision while propagating these new
duplicate-key errors immediately (no seed advance or retry via
advance_seed_and_clear).
In `@src/llm-coding-tools-core/src/models/catalog/mod.rs`:
- Around line 118-157: The docs claim a max reseed attempts of 16 but the
implementation constant MAX_SEED in internal/builder.rs is set to u8::MAX (256
attempts); change the implementation to match the public contract by replacing
MAX_SEED (or its initializer) with 16 (or a constant like MAX_SEED: u8 = 16) and
run/adjust any code paths that iterate over seed values (e.g., functions in
builder.rs that loop 0..=MAX_SEED or similar) so they use the new cap, then
verify the numeric-limits table remains accurate and update any tests or
comments that assumed 256 seeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 810ee827-bc54-4c2e-8573-c35c4362f7b4
📒 Files selected for processing (13)
src/llm-coding-tools-core/benches/model_catalog_builder.rssrc/llm-coding-tools-core/src/models/catalog/internal/builder.rssrc/llm-coding-tools-core/src/models/catalog/internal/hash_utils.rssrc/llm-coding-tools-core/src/models/catalog/internal/mod.rssrc/llm-coding-tools-core/src/models/catalog/internal/model_config_entry.rssrc/llm-coding-tools-core/src/models/catalog/internal/model_hash.rssrc/llm-coding-tools-core/src/models/catalog/internal/packed_env_range.rssrc/llm-coding-tools-core/src/models/catalog/internal/packed_provider_model_table_entry.rssrc/llm-coding-tools-core/src/models/catalog/internal/provider_hash.rssrc/llm-coding-tools-core/src/models/catalog/mod.rssrc/llm-coding-tools-core/src/models/catalog/public/builder_types.rssrc/llm-coding-tools-core/src/models/catalog/public/entry.rssrc/llm-coding-tools-core/src/models/mod.rs
💤 Files with no reviewable changes (2)
- src/llm-coding-tools-core/src/models/catalog/internal/provider_hash.rs
- src/llm-coding-tools-core/src/models/catalog/internal/model_hash.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/llm-coding-tools-core/src/models/mod.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Test (Async/Tokio) (windows-latest, x86_64-pc-windows-msvc, false)
- GitHub Check: Build and Test (Async/Tokio) (macos-latest, aarch64-apple-darwin, false)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by calling.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Usememchrcrate for fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Use [TypeName] rustdoc links instead of backticks in documentation
Files:
src/llm-coding-tools-core/src/models/catalog/internal/packed_env_range.rssrc/llm-coding-tools-core/src/models/catalog/internal/model_config_entry.rssrc/llm-coding-tools-core/benches/model_catalog_builder.rssrc/llm-coding-tools-core/src/models/catalog/internal/packed_provider_model_table_entry.rssrc/llm-coding-tools-core/src/models/catalog/public/entry.rssrc/llm-coding-tools-core/src/models/catalog/internal/mod.rssrc/llm-coding-tools-core/src/models/catalog/public/builder_types.rssrc/llm-coding-tools-core/src/models/catalog/internal/builder.rssrc/llm-coding-tools-core/src/models/catalog/mod.rssrc/llm-coding-tools-core/src/models/catalog/internal/hash_utils.rs
🧠 Learnings (1)
📚 Learning: 2026-02-07T22:53:26.067Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-02-07T22:53:26.067Z
Learning: Organize code with `llm-coding-tools-serdesai/` containing `src/absolute/`, `src/allowed/`, `src/schema.rs`, and `src/convert.rs` for serdesAI framework implementations
Applied to files:
src/llm-coding-tools-core/src/models/catalog/public/builder_types.rs
🔇 Additional comments (5)
src/llm-coding-tools-core/src/models/catalog/internal/packed_env_range.rs (1)
29-31: LGTM!The safety comment correctly references the renamed
analyze_provider_sourcesfunction, aligning with the broader provider-model terminology refactor in this PR.src/llm-coding-tools-core/src/models/catalog/internal/model_config_entry.rs (2)
61-71: LGTM!Clean internal refactor: exposing raw
Fixed4values withpub(crate)visibility allows internal callers flexibility while the public API (via [Model] struct) continues to exposeOption<f32>through.value(). Theconst fnaddition and#[inline]are appropriate for these trivial accessors.
88-89: LGTM!Tests correctly updated to use the new
_fixed()accessor names and chain.value()to obtainOption<f32>for assertions. Coverage for None, Some, and partial value cases is preserved.Also applies to: 96-97, 104-105
src/llm-coding-tools-core/src/models/catalog/public/entry.rs (1)
67-77: Preserving the encoded sampling values here is a good call.These accessors keep the stored
Fixed4values intact whenCatalogEntryis assembled, so lookup does not need to round-trip throughf32and drift from the deduplicated config entry.src/llm-coding-tools-core/src/models/catalog/internal/hash_utils.rs (1)
18-32: Composite provider+model hashing looks solid.The explicit
0xFFseparator keeps the concatenation unambiguous for UTF-8 keys while letting identical model names stay provider-scoped without widening the packed table entry.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/llm-coding-tools-core/src/models/catalog/public/builder_types.rs (1)
215-222: Keep duplicate provider/model context structured in the public error type.
DuplicateKey.keyflattensprovider_keyandmodel_keyinto one delimitedString. That makes downstream handling brittle and loses the original structure for provider-model duplicates. I'd keep those fields separate here, then format them inDisplayas needed.Suggested API shape
- #[error("duplicate key in {table} table: {key}")] + #[error( + "duplicate key in {table} table: provider_key={provider_key:?}, model_key={model_key:?}" + )] DuplicateKey { /// Table where the duplicate was detected. table: LookupTableKind, - /// The duplicate key (provider_key or "provider_key/model_key"). - key: String, + /// Provider key that was duplicated. + provider_key: String, + /// Model key that was duplicated in the provider-model table, if any. + model_key: Option<String>, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/models/catalog/public/builder_types.rs` around lines 215 - 222, The public error variant DuplicateKey currently flattens provider/model into a single String (field key); change DuplicateKey to keep structured fields (e.g., provider_key: String and model_key: Option<String>) instead of key: String, update the error annotation/Display formatting to render either "provider_key" or "provider_key/model_key" as before, and adjust any code that constructs or matches DuplicateKey to pass provider_key and model_key separately (references: DuplicateKey variant name and LookupTableKind enum to locate the variant and places constructing/formatting the error).src/llm-coding-tools-core/src/models/catalog/internal/builder.rs (1)
543-585: Add a regression for the samemodel_keyunder different providers.This test is close, but it still uses
"m1"and"m2", so it doesn't pin the PR's main behavior change. Reusing the samemodel_keyfor both providers and asserting both lookups succeed would catch a regression back to global model-key lookup.Targeted test tweak
- fn model_entries_are_deduplicated_by_info_and_config() { + fn same_model_key_across_providers_still_deduplicates_model_entries() { let providers = vec![ provider_source( "alpha", @@ ), provider_model_source( "beta", - "m2", + "m1", ModelInfo { modalities: Modality::TEXT, max_input: 4096, @@ let catalog = build_from_source(&providers, &provider_models).expect("source build should succeed"); + assert!(catalog.lookup("alpha", "m1").is_some()); + assert!(catalog.lookup("beta", "m1").is_some()); assert_eq!(catalog.provider_model_count(), 2); assert_eq!(catalog.model_config_count(), 1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/models/catalog/internal/builder.rs` around lines 543 - 585, The test uses different model_key values ("m1"/"m2") so it doesn't exercise the regression; change both provider_model_source calls to use the same model_key (e.g., "m1") while keeping different provider ids ("alpha"/"beta"), then keep the build_from_source(...) call and assert that catalog.provider_model_count() == 2 and catalog.model_config_count() == 1 and that lookups for both provider+model_key combinations succeed; update the two provider_model_source invocations (and any related assertions) to reference the same model_key so the test pins down deduplication by ModelInfo+config rather than global model_key lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/llm-coding-tools-core/src/models/catalog/internal/builder.rs`:
- Around line 543-585: The test uses different model_key values ("m1"/"m2") so
it doesn't exercise the regression; change both provider_model_source calls to
use the same model_key (e.g., "m1") while keeping different provider ids
("alpha"/"beta"), then keep the build_from_source(...) call and assert that
catalog.provider_model_count() == 2 and catalog.model_config_count() == 1 and
that lookups for both provider+model_key combinations succeed; update the two
provider_model_source invocations (and any related assertions) to reference the
same model_key so the test pins down deduplication by ModelInfo+config rather
than global model_key lookup.
In `@src/llm-coding-tools-core/src/models/catalog/public/builder_types.rs`:
- Around line 215-222: The public error variant DuplicateKey currently flattens
provider/model into a single String (field key); change DuplicateKey to keep
structured fields (e.g., provider_key: String and model_key: Option<String>)
instead of key: String, update the error annotation/Display formatting to render
either "provider_key" or "provider_key/model_key" as before, and adjust any code
that constructs or matches DuplicateKey to pass provider_key and model_key
separately (references: DuplicateKey variant name and LookupTableKind enum to
locate the variant and places constructing/formatting the error).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4dcde32-7ec7-4771-a070-0cb4d6f60613
📒 Files selected for processing (2)
src/llm-coding-tools-core/src/models/catalog/internal/builder.rssrc/llm-coding-tools-core/src/models/catalog/public/builder_types.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and Test (Async/Tokio) (windows-latest, x86_64-pc-windows-msvc, false)
- GitHub Check: Build and Test (Async/Tokio) (ubuntu-latest, x86_64-unknown-linux-gnu, false)
- GitHub Check: Build and Test (Async/Tokio) (macos-latest, aarch64-apple-darwin, false)
- GitHub Check: Build and Test (Blocking) (macos-latest, aarch64-apple-darwin, false)
- GitHub Check: Build and Test (Blocking) (windows-latest, x86_64-pc-windows-msvc, false)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by calling.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Usememchrcrate for fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Use [TypeName] rustdoc links instead of backticks in documentation
Files:
src/llm-coding-tools-core/src/models/catalog/internal/builder.rssrc/llm-coding-tools-core/src/models/catalog/public/builder_types.rs
🧠 Learnings (1)
📚 Learning: 2026-02-07T22:53:26.067Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-02-07T22:53:26.067Z
Learning: Organize code with `llm-coding-tools-serdesai/` containing `src/absolute/`, `src/allowed/`, `src/schema.rs`, and `src/convert.rs` for serdesAI framework implementations
Applied to files:
src/llm-coding-tools-core/src/models/catalog/public/builder_types.rs
Refactor per-provider model lookup and restore catalog docs
1 participant
Summary
This PR refactors the model catalog to use per-provider model lookup, ensuring that model names are scoped to providers and preventing ambiguity when the same model name exists under multiple providers.
Key Changes
Core Refactoring
ModelSourceRow→ProviderModelSourceRow: Model rows now require bothprovider_keyandmodel_key, making the provider-model relationship explicitModelTable→ProviderModelTable: Models are now hashed using(provider_key + 0xFF + model_key)combinationModelHash/ProviderHashtypes: Consolidated to useHash64frominternal::hash64modulePackedModelTableEntry→PackedProviderModelTableEntry: Reflects the new per-provider model lookup semanticsAPI Updates
&[ProviderModelSourceRow]instead of&[ModelSourceRow]catalog.lookup(provider_key, model_key)- no API change for consumersProviderKeyNotFoundForModelerror variant when provider-model rows reference unknown providersDocumentation
Performance
Motivation
Breaking Changes
ModelSourceRowrenamed toProviderModelSourceRowand now requiresprovider_keyfieldbuild(&[ProviderSourceRow], &[ProviderModelSourceRow])Testing