feat(rook): add SQLite persistence for ProviderAccount, ProviderPool, ModelRoute, and RoutingPolicy#605
Merged
Merged
Conversation
… ModelRoute, and RoutingPolicy Close #583
Deploying corvus with
|
| Latest commit: |
895527c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://13f1a378.corvus-42x.pages.dev |
| Branch Preview URL: | https://feat-rook-583-sqlite-models.corvus-42x.pages.dev |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a SQLite-backed persistence layer with migrations and full CRUD for ProviderAccount, ProviderPool, and ModelRoute; introduces a SqliteDb wrapper that runs migrations; and adjusts in-memory services (account, pool, route, health) with small behavioral API and logic changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant SqliteDb as SqliteDb (pool & API)
participant SQLite as SQLite (file or :memory:)
Client->>SqliteDb: SqliteDb::open(path) / open_in_memory()
SqliteDb->>SQLite: Open connection (foreign_keys=ON)
SqliteDb->>SQLite: SELECT applied migrations
alt 0001 not applied
SqliteDb->>SQLite: Execute migration SQL (0001_initial.sql)
SQLite-->>SqliteDb: OK
SqliteDb->>SQLite: INSERT schema_migrations record
end
Client->>SqliteDb: insert_account / insert_pool / insert_route
SqliteDb->>SQLite: INSERT/transactional statements (serialize JSON, timestamps)
SQLite-->>SqliteDb: Result rows/OK
SqliteDb-->>Client: Result / domain object(s)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Contributor
✅ Contributor ReportUser: @yacosta738
Contributor Report evaluates based on public GitHub activity. Analysis period: 2025-04-20 to 2026-04-20 |
3 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/rook/Cargo.toml`:
- Line 44: The sqlx dependency declaration includes unused features "macros" and
"migrate"; edit the sqlx entry (the dependency key "sqlx" with features
["runtime-tokio-rustls", "sqlite", "macros", "migrate"]) to remove "macros" and
"migrate" so it only enables the needed features (e.g. ["runtime-tokio-rustls",
"sqlite"]), then run a cargo build to verify no compile errors.
In `@clients/rook/migrations/0001_initial.sql`:
- Around line 35-44: The model_routes table currently leaves fallback_route_id
unconstrained and leaves target_pool_id FK without an explicit ON DELETE policy;
add a self-referential FOREIGN KEY on fallback_route_id referencing
model_routes(id) and add an explicit ON DELETE clause on the existing FOREIGN
KEY (target_pool_id) REFERENCES provider_pools(id) — choose and apply the
intended behavior (e.g., ON DELETE RESTRICT to prevent pool deletion while
routes exist or ON DELETE CASCADE if routes should be removed with the pool) and
update the constraint definition for model_routes accordingly; also add an index
on pool_members(account_id) to support reverse lookups for which pools an
account is in.
In `@clients/rook/src/db/account.rs`:
- Around line 61-63: The current casts "weight as u32" and "priority as u32"
silently truncate invalid i64 values; replace them with fallible conversions
using u32::try_from(weight).map_err(|_| RookError::Registry(format!(\"weight out
of range: {}\", weight)))? and similarly for priority
(u32::try_from(priority).map_err(|_| RookError::Registry(format!(\"priority out
of range: {}\", priority)))?), so the conversion in the place that constructs
the struct (the variables weight and priority) fails loudly on negative or
>u32::MAX values instead of returning bogus wrapped values.
- Around line 77-81: The current logic serializes account.vendor to JSON then
uses vendor_json.trim_matches('"') which corrupts escaped quotes/backslashes for
ProviderVendor::Other; replace this by explicitly matching on ProviderVendor in
the serialization path (the code that produces vendor_str) to produce a
canonical unescaped vendor identifier for named variants and to extract the raw
inner string for Other without dropping escape characters, and update the
deserialization path (row_to_account where it re-wraps with
format!("\"{vendor_str}\"")) to parse the stored form consistently (either store
full JSON and parse with serde_json::from_str or store the raw inner string and
construct ProviderVendor::Other explicitly); add a unit test for
ProviderVendor::Other("weird\"name".into()) to lock behavior and ensure
round-trip correctness.
In `@clients/rook/src/db/mod.rs`:
- Around line 72-78: The current run_migrations function executes MIGRATION_SQL
every startup with no version tracking, so future ALTER/DDL migrations will
re-run and fail; update run_migrations(&SqlitePool) to track applied migrations
either by switching to sqlx::migrate::Migrator (enable and call
Migrator::new(...).await?.run(pool).await and propagate errors) or implement a
minimal schema_migrations table (e.g., schema_migrations(version TEXT PRIMARY
KEY, applied_at TEXT) and change run_migrations to insert the migration id
before/after applying each migration and skip already-applied versions),
referencing the run_migrations function and MIGRATION_SQL to ensure migrations
are applied once and in order.
- Around line 34-67: The SQLite pool creation in open() and open_in_memory()
doesn't enable foreign keys, so ON DELETE CASCADE rules are not enforced; update
both functions to build SqliteConnectOptions with foreign_keys(true) and use
SqlitePool::connect_with (or PoolOptions::connect_with) with those options
instead of connect/connect("sqlite::memory:"), ensuring every connection in the
pool has PRAGMA foreign_keys=ON so cascade FKs in tables like pool_members and
model_routes are enforced; adjust the SqlitePool creation in open(), and the
PoolOptions::<sqlx::Sqlite>::new()...connect(...) call in open_in_memory() to
use the configured options.
In `@clients/rook/src/db/pool.rs`:
- Around line 67-69: The current serialization of pool.strategy uses
serde_json::to_string + trim_matches('"'), which is brittle and duplicates the
same fragile pattern used elsewhere for SelectionStrategy; replace this with a
centralized helper (e.g., enum_to_db_str and db_str_to_enum) that reliably
converts SelectionStrategy to and from the DB representation (or choose to store
the full JSON string without trimming) and update the code paths that reference
pool.strategy and SelectionStrategy to call those helpers so both account.rs and
pool.rs share the same robust serialization/deserialization logic.
- Around line 117-140: list_pools currently does an N+1 by calling
self.get_pool_members(&pool_id) for each row; instead, after reading rows in
list_pools collect all pool ids (from the "id" column / PoolId::new path), query
pool_members once with a WHERE pool_id IN (...) (bind each id via sqlx::query
and .bind), fetch_all, group the returned rows into a HashMap<PoolId,
Vec<AccountId>> and then call row_to_pool(row, members_for_row)? using the
grouped members (or an empty vec if none) rather than invoking get_pool_members
per-pool; keep existing error mapping (RookError::Registry) for the new query
and reuse PoolId/row parsing logic to match keys.
- Around line 65-93: The insert_pool operation must be executed inside a DB
transaction so the provider_pools insert and all per-member inserts are atomic:
start a transaction (begin on the sqlx pool), run the INSERT INTO provider_pools
using the transaction executor, then insert each member within the same
transaction (either by calling a variant of add_pool_member that accepts a
mutable Transaction or by performing the member INSERTs inline against that
transaction), and finally commit the transaction only if all member inserts
succeed; on any error roll back/return the error so no partial state remains.
Reference symbols: insert_pool, add_pool_member, provider_pools, self.pool(),
and the sqlx::query/execute calls — modify add_pool_member signature or add a
new helper that accepts &mut sqlx::Transaction to ensure all DB operations use
the same transaction.
- Around line 145-163: The SQLite pool is created without enabling
per-connection foreign key enforcement, so ON DELETE CASCADE in the migration
doesn't work and add_pool_member can create orphaned rows; update both
connection builders (open() and open_in_memory()) to use
SqliteConnectOptions::new().foreign_keys(true) (or set foreign_keys(true) on the
SqliteConnectOptions used) so FK enforcement is enabled for every connection,
and add a regression test that creates a provider_pools row with pool_members,
deletes the provider_pools row, then asserts pool_members rows for that pool_id
are gone to verify cascade-delete behavior; reference functions: open(),
open_in_memory(), add_pool_member and tables: provider_pools, pool_members.
In `@clients/rook/src/db/route.rs`:
- Around line 58-61: The code silently swallows JSON parse errors for policy by
using serde_json::from_str(...).unwrap_or_default(), which drops
capability_constraints; replace this with a parse that propagates an error (e.g.
serde_json::from_str(&policy_json).map_err(|e|
RookError::Registry(format!("invalid policy JSON: {e};
policy_json={policy_json}")))?), so the StoredPolicy deserialization failure
surfaces instead of producing a default/empty policy; update the code around
policy_json and StoredPolicy to return that RookError::Registry on parse
failure.
- Around line 76-106: The model_routes table allows orphaned fallback_route_id
references; update the schema or methods to enforce referential integrity: add a
self-referential foreign key on model_routes(fallback_route_id) ->
model_routes(id) in the migration with the chosen policy (ON DELETE CASCADE or
ON DELETE RESTRICT), or implement application-level checks in insert_route and
delete_route — in insert_route (function insert_route) validate that
fallback_route_id exists before inserting (return a RookError if not), and in
delete_route validate there are no rows referencing the deleted id (or
cascade-delete them) before removing; add a regression test that inserts routeA,
inserts routeB with fallback_route_id = routeA.id, then calls
delete_route(routeA) and asserts behavior matches the chosen FK policy (failure
for RESTRICT, routeB removed for CASCADE).
In `@clients/rook/src/services/account.rs`:
- Around line 62-69: The in-memory AccountService create method (fn create in
clients/rook/src/services/account.rs) currently silently overwrites an existing
ProviderAccount keyed by AccountId; change it to match the SQLite-backed
behavior by checking the store (after acquiring the lock) for an existing key
(e.g., contains_key(&id)) and returning an appropriate error instead of
inserting when a duplicate exists (or rename to upsert if you intend to allow
overwrites). Update the function to perform the existence check before insert
and return the same RookError variant used by the SQLite impl for
constraint/duplicate failures so semantics across create implementations are
consistent.
In `@clients/rook/src/services/health.rs`:
- Around line 111-134: mark_success and mark_failure currently ignore
poisoned-lock Err from self.lock(), making health updates silently no-op; change
both to log a tracing::warn! (including account_id and error) when self.lock()
returns Err so operators see poisoned-mutex incidents, and keep the existing
successful-path logic. Additionally, in mark_failure replace the unchecked cast
cooldown_seconds as i64 with a safe conversion/capping (e.g.,
i64::try_from(cooldown_seconds).unwrap_or(i64::MAX) or cap to a sane bound)
before calling chrono::Duration::seconds to avoid wraparound for very large
values; refer to the methods mark_success and mark_failure and the
cooldown_until assignment to locate the changes.
In `@clients/rook/src/services/pool.rs`:
- Around line 73-80: The in-memory create() currently upserts and silently
overwrites existing ProviderPool entries; instead check for an existing PoolId
in self.store (lock via self.store.lock() and call contains_key(&id)) and if
present return an Err describing the duplicate (e.g.
Err(RookError::Registry(format!("duplicate pool id {}", id)))) rather than
inserting, so behavior matches the SQLite primary-key constraint; alternatively,
if you intend an upsert rename the method to upsert and document the semantic
change (references: create, ProviderPool, PoolId, RookError, self.store).
In `@clients/rook/src/services/route.rs`:
- Around line 66-92: Enforce the schema's uniqueness and make resolution
deterministic: in resolve(&self, logical_model) iterate over self.store (via
lock) and collect/filter entries where r.logical_model == logical_model, then
return the single match (Some(route.clone())) only if exactly one found
(otherwise None or an error path), ensuring deterministic behavior; in
create(&self, route) acquire the store lock and first check that no existing
entry has the same logical_model (reject with RookError::Registry on conflict)
before inserting; in update(&self, route) lock the store, verify the route.id
exists, and also verify that no OTHER entry (id != route.id) has the same
logical_model (reject on conflict) before inserting the updated route.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7f55f25e-bbb1-485e-9631-e0b096367ff4
⛔ Files ignored due to path filters (1)
clients/rook/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
clients/rook/Cargo.tomlclients/rook/migrations/0001_initial.sqlclients/rook/src/db/account.rsclients/rook/src/db/mod.rsclients/rook/src/db/pool.rsclients/rook/src/db/route.rsclients/rook/src/lib.rsclients/rook/src/services/account.rsclients/rook/src/services/health.rsclients/rook/src/services/pool.rsclients/rook/src/services/route.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). (1)
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.
Files:
clients/rook/src/services/pool.rsclients/rook/src/services/account.rsclients/rook/src/lib.rsclients/rook/src/services/route.rsclients/rook/src/db/mod.rsclients/rook/src/db/route.rsclients/rook/src/db/account.rsclients/rook/src/services/health.rsclients/rook/src/db/pool.rs
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
clients/rook/src/services/pool.rsclients/rook/Cargo.tomlclients/rook/src/services/account.rsclients/rook/migrations/0001_initial.sqlclients/rook/src/lib.rsclients/rook/src/services/route.rsclients/rook/src/db/mod.rsclients/rook/src/db/route.rsclients/rook/src/db/account.rsclients/rook/src/services/health.rsclients/rook/src/db/pool.rs
🧠 Learnings (5)
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/Cargo.toml : Do not add heavy dependencies for minor convenience; justify new crate additions
Applied to files:
clients/rook/Cargo.toml
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/Cargo.toml : Preserve release-size profile assumptions in `Cargo.toml` and avoid adding heavy dependencies unless clearly justified
Applied to files:
clients/rook/Cargo.toml
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/*.rs : Run `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` for code validation, or document which checks were skipped and why
Applied to files:
clients/rook/Cargo.toml
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/providers/**/*.rs : Implement `Provider` trait in `src/providers/` and register in `src/providers/mod.rs` factory when adding a new provider
Applied to files:
clients/rook/src/lib.rsclients/rook/src/db/mod.rsclients/rook/src/db/account.rsclients/rook/src/db/pool.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Applied to files:
clients/rook/src/services/health.rs
🔇 Additional comments (1)
clients/rook/src/lib.rs (1)
7-13: LGTM — module exports align with the new persistence layer.
- Remove unused sqlx features (macros, migrate) - Add FK on fallback_route_id and index on pool_members(account_id) - Add migration tracking with schema_migrations table - Fix fallible weight/priority conversions - Fix ProviderVendor serialization - Fix SelectionStrategy serialization - Propagate JSON parse errors - Add duplicate checks in services - Log poisoned locks in health service
fix: rook 583 apply
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 11 file(s) based on 16 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 11 file(s) based on 16 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
This was referenced Apr 21, 2026
Merged
8 tasks
This was referenced Apr 25, 2026
Closed
8 tasks
This was referenced Apr 28, 2026
Merged
8 tasks
8 tasks
8 tasks
This was referenced May 5, 2026
Merged
8 tasks
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
dbmodule to the Rook crate with full SQLite persistence viasqlxforProviderAccount,ProviderPool(with members), andModelRoutemigrations/0001_initial.sql) at compile time; runs viasqlx::raw_sqlat runtime — no compile-time macros requiredsqlx::query()API (noDATABASE_URLenv var needed at build time)rusqlite(was only referenced in a doc comment) to eliminate the duplicatesqlite3native link conflict withsqlx-sqliteApproach
SqliteDbstruct wraps aSqlitePooland exposesopen(),open_in_memory(), andrun_migrations()SelectionStrategyandProviderVendorenum variants serialise as bare snake_case strings in the DB; deserialized by wrapping back in quotes beforeserde_json::from_strRoutingPolicy'scapability_constraintsare stored as a JSON column (StoredPolicy) onmodel_routesTesting
Close #583