feat: support external model and pricing configuration from TOML#140
feat: support external model and pricing configuration from TOML#140mike1858 merged 6 commits intoPiebald-AI:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 19 minutes and 10 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughConfig accepts user-provided Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as main()
participant Config as Config Loader
participant Init as models::init_external_models()
participant Registry as Global Registry (OnceLock + RwLock)
CLI->>Config: load configuration
Config-->>CLI: return Config { models, aliases, ... }
CLI->>Init: init_external_models(models, aliases)
Init->>Registry: acquire write lock
Registry->>Registry: merge models & aliases (guarded by AtomicBool)
Registry-->>Init: release lock
Init-->>CLI: initialization complete
CLI->>CLI: continue startup and command dispatch
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/models.rs (1)
91-108: Consider documenting or enforcing single initialization.The function comment says "should be called once at startup," but nothing prevents multiple calls. Each call will:
- Initialize defaults (only first time via
get_or_init)- Merge/overwrite entries (every time)
This is technically correct for the current use case, but if
init_external_modelsis accidentally called multiple times (e.g., in tests or future refactoring), entries accumulate rather than being replaced entirely.If this is intentional (allowing incremental additions), the doc comment should clarify. If single-init is expected, consider using a flag to warn or no-op on subsequent calls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models.rs` around lines 91 - 108, The init_external_models function currently merges external_models and external_aliases into REGISTRY on every call (using REGISTRY.get_or_init and then writing into registry.index/registry.aliases), which contradicts the "should be called once" doc; either document that incremental merges are intentional or enforce single initialization by adding a guard: detect if REGISTRY was already initialized or if registry.index/registry.aliases are non-empty and then either return/no-op or log/warn and refuse to overwrite; update the comment to state behavior (merge vs single-init) and modify init_external_models to check REGISTRY.get_or_init / Registry state before inserting (using REGISTRY, init_external_models, Registry, index, aliases, and get_or_init as anchors).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config.rs`:
- Around line 298-325: The test test_model_registry_merging mutates the global
REGISTRY via init_external_models which can leak state across parallel tests;
make the test deterministic by either marking it serial (add #[serial] from
serial_test above fn test_model_registry_merging), or ensure cleanup by calling
a test-only reset function that clears external entries from REGISTRY (add a
#[cfg(test)] fn clear_external_models() that removes entries added by
init_external_models and call it in a teardown), or move the test into an
integration test file so it runs in its own process; reference
init_external_models, REGISTRY, get_model_info, and test_model_registry_merging
when applying the chosen fix.
---
Nitpick comments:
In `@src/models.rs`:
- Around line 91-108: The init_external_models function currently merges
external_models and external_aliases into REGISTRY on every call (using
REGISTRY.get_or_init and then writing into registry.index/registry.aliases),
which contradicts the "should be called once" doc; either document that
incremental merges are intentional or enforce single initialization by adding a
guard: detect if REGISTRY was already initialized or if
registry.index/registry.aliases are non-empty and then either return/no-op or
log/warn and refuse to overwrite; update the comment to state behavior (merge vs
single-init) and modify init_external_models to check REGISTRY.get_or_init /
Registry state before inserting (using REGISTRY, init_external_models, Registry,
index, aliases, and get_or_init as anchors).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0cf09d5e-f935-448a-85c4-7fe15398214c
📒 Files selected for processing (3)
src/config.rssrc/main.rssrc/models.rs
jimyag
force-pushed
the
feature/external-models-config
branch
from
6cbc2a7 to
eeddb84
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config.rs`:
- Around line 285-295: The test references PricingStructure::Flat but
PricingStructure isn't imported into the test module; add an import for the enum
(e.g. use crate::models::PricingStructure;) at the top of the test module so the
match on custom_model.pricing compiles, ensuring the test can match
PricingStructure::Flat and access input_per_1m/output_per_1m.
In `@src/models.rs`:
- Around line 1563-1566: The match is trying to move a non-Copy enum out of a
borrowed ModelInfo (`info`); change the match to pattern-match on a reference to
`pricing` (e.g., `match &info.pricing`) so you destructure
`PricingStructure::Flat { input_per_1m, .. }` by reference and avoid moving;
keep the same assertion on `input_per_1m` after adjusting the pattern to bind
the referenced field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fad62d8f-d06f-494b-9739-f7be2ff8e288
📒 Files selected for processing (3)
src/config.rssrc/main.rssrc/models.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/models.rs (1)
115-131: Consider documenting the single-threaded startup requirement.The initialization pattern has a brief window between the atomic flag swap and the merge completion where concurrent readers could see incomplete data. While this is mitigated by calling
init_external_modelsat the start ofmain()before spawning any tasks (as seen in the context), the function's doc comment could be more explicit about this requirement.📝 Suggested documentation improvement
/// Initialize the model registry with external configuration. -/// This should be called once at startup. +/// This should be called once at startup, before any concurrent access to +/// model lookups. Calling from `main()` before spawning tasks is recommended. pub fn init_external_models(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models.rs` around lines 115 - 131, Update the doc comment for init_external_models to explicitly state it must be called once during single-threaded startup (e.g., in main() before spawning threads/tasks) because EXTERNAL_MODELS_INITIALIZED is set before merging into REGISTRY and there is a brief window where concurrent readers could observe incomplete state; reference init_external_models, EXTERNAL_MODELS_INITIALIZED, REGISTRY, and Registry::merge in the comment so callers know to call it prior to concurrent access.
🤖 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/models.rs`:
- Around line 115-131: Update the doc comment for init_external_models to
explicitly state it must be called once during single-threaded startup (e.g., in
main() before spawning threads/tasks) because EXTERNAL_MODELS_INITIALIZED is set
before merging into REGISTRY and there is a brief window where concurrent
readers could observe incomplete state; reference init_external_models,
EXTERNAL_MODELS_INITIALIZED, REGISTRY, and Registry::merge in the comment so
callers know to call it prior to concurrent access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 589c8fe9-1678-4254-8597-c3fc99049638
📒 Files selected for processing (2)
src/config.rssrc/models.rs
|
@copilot resolve the merge conflicts in this pull request |
1 similar comment
|
@copilot resolve the merge conflicts in this pull request |
Bring the Copilot CLI analyzer, docs, and test updates into this branch while preserving the external model configuration work and resolving the src/models.rs conflicts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: jimyag <git@jimyag.com>
|
@copilot Resolve the new conflicts. I accidentally merged my own PR which adds some new models, directly conflicting with this PR. |
Bring the GPT-4.5 and GPT-5.4-nano pricing updates into this branch while preserving the external model registry and prior merge resolution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: jimyag <git@jimyag.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/models.rs (1)
1326-1335: Hot-path clones on every cost calculation.
lookup_modelnow clonesModelInfoon every call, andcalculate_total_costtriggers three lookups (input + output + cache) per usage record. For tiered models this allocates/dropsVec<PricingTier>andVec<CachingTier>each time, which is a regression vs. the previous staticphfmaps. On large session aggregations this can be noticeable.Consider storing
Arc<ModelInfo>in the registry (and returningArc<ModelInfo>fromget_model_info) so lookups are cheap ref-count bumps without heap allocation, or havecalculate_total_costperform a single lookup and pass the&ModelInfointo input/output/cache helpers.♻️ Sketch
struct Registry { - index: HashMap<String, ModelInfo>, + index: HashMap<String, Arc<ModelInfo>>, aliases: HashMap<String, String>, } ... -fn lookup_model(name: &str) -> Option<ModelInfo> { +fn lookup_model(name: &str) -> Option<Arc<ModelInfo>> { let registry = get_registry_lock().read(); if let Some(model_info) = registry.index.get(name) { - return Some(model_info.clone()); + return Some(Arc::clone(model_info)); } if let Some(canonical_name) = registry.aliases.get(name) { - return registry.index.get(canonical_name).cloned(); + return registry.index.get(canonical_name).map(Arc::clone); } None }And/or refactor
calculate_total_costto do one lookup and dispatch:pub fn calculate_total_cost(model_name: &str, input: u64, output: u64, cc: u64, cr: u64) -> f64 { let Some(info) = get_model_info(model_name) else { /* warn + return 0.0 */ }; input_cost(&info, input) + output_cost(&info, output) + cache_cost(&info, cc, cr) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models.rs` around lines 1326 - 1335, lookup_model currently clones ModelInfo on every call (via registry.index.get(name).cloned()), causing hot-path allocations when calculate_total_cost calls it multiple times; change the registry to store Arc<ModelInfo> (update the registry.index and registry.aliases types) and make lookup_model return Arc<ModelInfo> (or change get_model_info to return Arc<ModelInfo>) so lookups are just ref-count bumps, or alternatively refactor calculate_total_cost to perform a single lookup (call lookup_model/get_model_info once) and pass a &ModelInfo into input_cost/output_cost/cache_cost helpers to avoid repeated clones of Vec<PricingTier>/Vec<CachingTier>; update any call sites and signatures (lookup_model, get_model_info, calculate_total_cost, input_cost, output_cost, cache_cost) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/models.rs`:
- Around line 117-131: The flag EXTERNAL_MODELS_INITIALIZED is being set at the
start of init_external_models, so any panic or partial init before calling
registry.merge will leave the flag true and silently skip subsequent calls;
change the flow to perform the registry write and call Registry::merge first
(using REGISTRY and its RwLock), and only after a successful merge set
EXTERNAL_MODELS_INITIALIZED (or remove the flag entirely and rely on
OnceLock/one-call contract); also update warn_once to include more context
(function name or note that later [models]/[aliases] were ignored) so repeated
calls don’t silently drop configuration — key symbols: init_external_models,
EXTERNAL_MODELS_INITIALIZED, REGISTRY, RwLock, Registry::merge, warn_once.
---
Nitpick comments:
In `@src/models.rs`:
- Around line 1326-1335: lookup_model currently clones ModelInfo on every call
(via registry.index.get(name).cloned()), causing hot-path allocations when
calculate_total_cost calls it multiple times; change the registry to store
Arc<ModelInfo> (update the registry.index and registry.aliases types) and make
lookup_model return Arc<ModelInfo> (or change get_model_info to return
Arc<ModelInfo>) so lookups are just ref-count bumps, or alternatively refactor
calculate_total_cost to perform a single lookup (call
lookup_model/get_model_info once) and pass a &ModelInfo into
input_cost/output_cost/cache_cost helpers to avoid repeated clones of
Vec<PricingTier>/Vec<CachingTier>; update any call sites and signatures
(lookup_model, get_model_info, calculate_total_cost, input_cost, output_cost,
cache_cost) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b36357a-c9dc-4e55-b2ab-f3f2dc585a8f
📒 Files selected for processing (2)
src/main.rssrc/models.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main.rs
Use an Arc-backed model registry so repeated external config merges are applied without hot-path clones, and stabilize the upload test helpers and test-only HTTP client lifecycle uncovered during verification. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: jimyag <git@jimyag.com>
Resolve the new review findings by validating external tier configs, following alias chains transitively, restoring the missing doubao canonical model, and tightening upload TLS and header parsing behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: jimyag <git@jimyag.com>
|
@coderabbitai review again |
|
✅ Actions performedFull review triggered. |
2 participants
This PR adds support for loading custom models and pricing mappings from the
~/.splitrail.tomlconfiguration file.Changes:
src/models.rsto support dynamic model registration usingparking_lot::RwLockandOnceLock.Vecinstead of static slices for serialization support.modelsandaliasesfields to theConfigstruct insrc/config.rs.maininsrc/main.rs.Configuration Example:
Users can now add custom models to their
.splitrail.toml:Summary by CodeRabbit
New Features
Tests
UX