feat: add model alias domain model and SQLite repository (Part 1/2) - #109
Merged
Merged
Conversation
- Add ModelAlias struct and ModelAliasRepositoryPort trait - Create alias-sqlite crate with SqliteModelAliasRepository - Add V5 migration for model_aliases table - Include 26 built-in aliases (OpenAI, Anthropic, Google, Mistral, Groq) - Add 11 unit tests for repository operations - Implement cycle prevention and idempotent seeding Part of #47
Contributor
|
Warning Review limit reached
More reviews will be available in 7 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (23)
📝 WalkthroughWalkthroughThis PR introduces end-to-end model alias resolution: friendly model aliases map to canonical models before routing decisions. Domain types and a SQLite repository provide persistent storage; configuration enables optional startup seeding; the application layer optionally resolves aliases before restrictions; HTTP handlers expose alias CRUD operations; comprehensive tests validate all layers. ChangesModel Alias Resolution Feature
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
- Add [model_aliases] config section with enabled and auto_seed flags - Wire SqliteModelAliasRepository into DI container with startup seeding - Implement alias resolution in RouteRequest before restrictions check - Add GET/POST/DELETE endpoints for alias management at /api/models/aliases - Add cycle prevention validation in create endpoint - Add 7 E2E tests for alias resolution and seeding - Add 10 HTTP API integration tests - Add 5 config tests for model aliases section Part of #47
…y (2/2) (#110) * feat(cache): content-based cache keys with SHA-256 signatures Implement content-based caching for issue #50: - Add signature field to CacheKey (SHA-256 hex string) - Update CompletionRequest.cache_key() to hash (model + messages + params) - Add CacheStats struct with hit_rate() and utilization() methods - Add LRU eviction to InMemoryCache with max_entries support - Add AtomicU64 counters (hits, misses, evictions) - Implement stats() method in InMemoryCache - Add comprehensive unit tests for hashing, LRU, and stats Breaking change: CacheKey now requires signature field (documented in proposal) * fix(cache): address code review findings - Add documentation for approximate LRU under concurrency - Fix evict_if_needed to only trigger on new keys (not overwrites) - Add test for no-eviction-on-overwrite behavior - Clarify spec: None=unlimited, Some(0)=rejected (usize cannot be negative) - Update tasks to specify max_entries validation rules * feat(cache): HTTP management API, config validation, and observability (2/2) Completes issue #50 implementation: **Phase 3: Ports** - Add stats() method to CachePort trait - Add delete_by_signature() for HTTP endpoint support - Implement both methods in InMemoryCache **Phase 4: Configuration** - Add max_entries field to CacheConfig - Implement validate() rejecting ttl > 24h and max_entries = Some(0) - Wire validation at config load (fail-fast) - Pass max_entries to cache constructor in DI **Phase 5: Application** - Add cache() accessor to RouteRequest (already tracking stats) **Phase 6: Transport** - Create cache.rs handler module - Implement GET /api/cache/stats (200 with CacheStats JSON) - Implement DELETE /api/cache (204 clear all) - Implement DELETE /api/cache/:signature (204/404) - Wire cache routes (management API, requires auth) - Extend /health with cache_entries, cache_hit_rate, cache_utilization **Phase 7: Observability** - Add rook_cache_evictions counter description - Wire eviction metric in InMemoryCache **Tests** - 5 config validation tests - 6 cache HTTP endpoint integration tests - All 450+ tests passing * fix: apply remaining code review findings from PR #110 - cache-memory: only increment deleted/evictions when store.remove returns Some - routes.rs: use Axum 0.8 path syntax {signature} - cache_routes.rs: add test_cache_routes_require_management_auth - config_tests.rs: fix assertion message to match actual validation error - tasks.md: fix dependencies (9.* → 9.1-9.4), update delete behavior description
- Add ModelAlias struct and ModelAliasRepositoryPort trait - Create alias-sqlite crate with SqliteModelAliasRepository - Add V5 migration for model_aliases table - Include 26 built-in aliases (OpenAI, Anthropic, Google, Mistral, Groq) - Add 11 unit tests for repository operations - Implement cycle prevention and idempotent seeding Part of #47
- Add [model_aliases] config section with enabled and auto_seed flags - Wire SqliteModelAliasRepository into DI container with startup seeding - Implement alias resolution in RouteRequest before restrictions check - Add GET/POST/DELETE endpoints for alias management at /api/models/aliases - Add cycle prevention validation in create endpoint - Add 7 E2E tests for alias resolution and seeding - Add 10 HTTP API integration tests - Add 5 config tests for model aliases section Part of #47
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/application/rook-usecases/src/route_request.rs`:
- Around line 99-137: execute_stream_with_format currently skips the alias
resolution performed in execute_with_format, causing streaming requests to use
unresolved req.model; update execute_stream_with_format to perform the same
alias lookup when self.alias_config.enabled by calling
self.alias_repository.find_by_alias(&req.model, None).await and, on
Ok(Some(alias_entry)), set req.model = alias_entry.canonical (log with
tracing::debug), handle Ok(None) silently and Err(e) with tracing::warn like
execute_with_format does so restrictions and routing see the canonical model for
streaming requests as well.
In `@crates/domain/rook-core/src/model.rs`:
- Line 560: ModelAlias.created_at is currently a String but should use
DateTime<Utc> like other domain models (e.g., User.created_at,
Session.created_at, Combo.created_at); change the field type in the ModelAlias
struct to chrono::DateTime<chrono::Utc>, add the appropriate chrono imports, and
then update all places that construct, serialize/deserialize, and persist
ModelAlias instances (repository methods and any conversion/mapper functions) to
pass and store DateTime<Utc> values instead of strings, ensuring any
database/ORM conversion code is adapted to accept DateTime<Utc>.
In `@crates/infrastructure/alias-sqlite/src/builtin.rs`:
- Around line 3-5: The comment above DEFAULT_ALIASES is stale: it claims
"Provider ID is None for global aliases" but every tuple in DEFAULT_ALIASES
currently uses Some(provider_id); either implement global aliases with None
entries or update the comment to match the current data. Fix by editing the doc
comment for DEFAULT_ALIASES to accurately describe that each alias is
provider-scoped (provider_id is always Some) or, if global aliases are required,
add the intended tuples with Option::None provider IDs and corresponding tests;
reference DEFAULT_ALIASES and the tuple shape (&str, &str, Option<&str>) when
making the change.
In `@crates/infrastructure/alias-sqlite/src/repository.rs`:
- Around line 221-236: The builtin_aliases function currently captures a single
timestamp (let now = Utc::now().to_rfc3339()) and clones it for every
ModelAlias, causing all built-in aliases to share the same created_at; to make
timestamps unique, move the Utc::now().to_rfc3339() call inside the map closure
(e.g., compute created_at per item) so each ModelAlias constructed in the
iterator gets its own created_at instead of cloning a shared now; update
references to remove the outer now variable and stop cloning it.
- Around line 133-147: The comment "prevent cycles" is misleading because the
code around canonical_is_alias (query_row checking alias.canonical) only
prevents direct (depth-1) canonical aliases; update the comment to explicitly
state it only prevents direct aliasing (e.g., "prevent direct canonical-as-alias
(depth-1)"), and optionally rename/comment the canonical_is_alias variable or
add a FIXME if you want full transitive cycle detection later; keep the existing
logic (the query on alias.canonical and the
ModelAliasRepositoryError::InvalidAlias error) unchanged unless you intend to
implement full graph traversal cycle detection.
In `@crates/infrastructure/transport-axum/src/handlers/aliases.rs`:
- Around line 136-141: Replace the brittle string match on
e.to_string().contains("already exists") with a direct match on the repository
error enum (ModelAliasRepositoryError) in the alias creation handler in
handlers/aliases.rs: match the specific variant (e.g.,
ModelAliasRepositoryError::AlreadyExists or the exact variant name used by your
repo) and map it to the HttpError with StatusCode::BAD_REQUEST, code
"ALIAS_ALREADY_EXISTS" and the same message; leave the final generic Err(e) arm
to handle other errors. Ensure you reference the repository error type in the
match arm (bring it into scope if needed) so the handler discriminates
duplicates by type rather than by display text.
- Around line 93-96: The cycle pre-check currently calls
repo.find_by_alias(&canonical_model_id, None) ignoring the request's provider
scope; update the aliases handler to pass the request's provider_id (i.e. use
req.provider_id.map(ProviderId::new)) into find_by_alias so the lookup is
provider-scoped (change the call in the handler where
ModelId::new(req.canonical.clone()) is used). Also update the SQLite
repository's create() cycle query (in alias-sqlite::repository::create) to
include provider_id in the WHERE clause (make the SELECT 1 FROM model_aliases
... LIMIT 1 query filter on provider_id) so the database-level cycle check
matches find_by_alias semantics.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: cd85acf4-9c69-4b8d-9005-ff4e21bc6455
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
Cargo.tomlapps/rook/Cargo.tomlapps/rook/src/config.rsapps/rook/src/di.rsapps/rook/tests/alias_routing_e2e.rsapps/rook/tests/config_tests.rscrates/application/rook-usecases/src/route_request.rscrates/application/rook-usecases/tests/route_request_restrictions.rscrates/domain/rook-core/src/model.rscrates/domain/rook-core/src/ports.rscrates/infrastructure/alias-sqlite/Cargo.tomlcrates/infrastructure/alias-sqlite/src/builtin.rscrates/infrastructure/alias-sqlite/src/lib.rscrates/infrastructure/alias-sqlite/src/repository.rscrates/infrastructure/db-migration/src/migrations/V5__model_aliases.sqlcrates/infrastructure/transport-axum/src/alias_routes.rscrates/infrastructure/transport-axum/src/bootstrap_helpers.rscrates/infrastructure/transport-axum/src/handlers/aliases.rscrates/infrastructure/transport-axum/src/handlers/mod.rscrates/infrastructure/transport-axum/src/lib.rscrates/infrastructure/transport-axum/src/routes.rscrates/infrastructure/transport-axum/tests/alias_api.rscrates/infrastructure/transport-axum/tests/format_translation_integration.rs
…tion - Merged cache validation tests with model alias tests in config_tests.rs - Exposed both cache() and alias_repository() in RouteRequest for management APIs
- Add alias resolution to execute_stream_with_format for streaming requests - Change ModelAlias.created_at from String to DateTime<Utc> for consistency - Generate unique timestamps per alias in builtin_aliases() - Update builtin.rs comment to reflect provider-scoped aliases - Clarify cycle detection as depth-1 only in repository - Replace string matching with enum matching for AlreadyExists error - Make cycle check provider-scoped in handler and repository query - Add db_migration import to fix test initialization All changes verified with full CI passing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Part 1 of 2 for Model Aliasing & Normalization (#47). This PR establishes the foundation layer: domain model, repository port, SQLite implementation, and migration.
Changes
Domain Layer
ModelAliasstruct with alias, canonical model ID, optional provider ID, and timestampModelAliasRepositoryPorttrait with CRUD + seeding methodsInfrastructure Layer
alias-sqlitecrate following existing SQLite patterns (combo-sqlite, provider-sqlite)SqliteModelAliasRepositorywith parameterized queries and cycle preventionmodel_aliasestable with indexed columnsINSERT OR IGNORETests
Out of Scope (Part 2)
The following will be implemented in a follow-up PR:
[model_aliases])/api/models/aliases)Verification
Related