feat(rook): implement shared domain services for accounts, pools, routes, and health#603
Merged
Merged
Conversation
…tes, and health Close #581
Deploying corvus with
|
| Latest commit: |
445a2d0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://22343558.corvus-42x.pages.dev |
| Branch Preview URL: | https://feat-rook-581-shared-service.corvus-42x.pages.dev |
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
![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: 5
🤖 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/src/services/account.rs`:
- Around line 62-68: The create method currently calls
self.store.lock()?.insert(...) which will silently overwrite existing accounts;
change create (in the create method) to first acquire the mutex
(self.store.lock()), then check the map for an existing key (e.g.,
map.contains_key(&id) or use map.entry(id)), and if the id already exists return
an Err with an appropriate RookError (either add an AlreadyExists variant to
RookError or return RookError::Registry with a clear "account already exists"
message); only call insert when the id is absent and then return Ok(id).
In `@clients/rook/src/services/health.rs`:
- Around line 136-146: is_available currently rejects any
HealthStatus::Unhealthy before checking cooldown_until, causing accounts to
remain permanently excluded; modify is_available (and its use of
get/HealthStatus::Unhealthy and cooldown_until) so that it first checks whether
a cooldown_until exists and has expired (if expired, consider the account
eligible regardless of Unhealthy), or alternatively check cooldown first and
only treat Unhealthy as ineligible while cooldown_until is in the future; ensure
mark_failure can still set status to Unhealthy and cooldown_until, but
is_available must return true when cooldown_until <= Utc::now().
- Around line 64-69: Change the signatures of mark_success and mark_failure to
return Result<(), RookError> and propagate lock errors instead of silently
ignoring them; in mark_failure, use i64::try_from(cooldown_seconds) to convert
the seconds and call DateTime::checked_add_signed(Utc::now(),
Duration::seconds(...)) (or DateTime::checked_add_signed with the Duration) to
avoid panics on overflow, returning Err(RookError::...) on any conversion,
checked-add, or lock failure; update any callers to handle the Result and
preserve existing behavior on Ok. Ensure references to mark_success,
mark_failure, RookError, AccountId, Duration, Utc, i64::try_from, and
DateTime::checked_add_signed are used so the fixes are applied in the correct
functions.
In `@clients/rook/src/services/pool.rs`:
- Around line 73-79: The create method currently uses HashMap::insert which
upserts and can overwrite existing pools; change create (in the ProviderPool ->
create implementation that returns Result<PoolId, RookError>) to first lock the
store, check whether the map already contains the pool.id (e.g.,
map.contains_key(&id) or use map.entry(id)), and if it exists return an
Err(RookError::Registry(...)) (or add/use an appropriate "already
exists"/conflict RookError variant), otherwise insert and return Ok(id); ensure
the store lock error handling remains unchanged.
In `@clients/rook/src/services/route.rs`:
- Around line 66-90: resolve currently returns the first matching ModelRoute but
create/update allow duplicate logical_model entries; ensure uniqueness by
updating create and update to scan the locked store (guard.values()) and return
a RookError::Registry if any existing route has the same logical_model (for
create: any match; for update: any match where r.id != route.id), and change
resolve to detect multiple matches by collecting matches from self.store.lock()
and only return Some(route) when exactly one match exists (otherwise return
None) to avoid nondeterministic resolution; use the symbols resolve, create,
update, ModelRoute, RouteId, and RookError when making these checks.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b82fd34-f754-48fa-a53d-3662ad586103
📒 Files selected for processing (6)
clients/rook/src/lib.rsclients/rook/src/services/account.rsclients/rook/src/services/health.rsclients/rook/src/services/mod.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/lib.rsclients/rook/src/services/mod.rsclients/rook/src/services/account.rsclients/rook/src/services/pool.rsclients/rook/src/services/route.rsclients/rook/src/services/health.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/lib.rsclients/rook/src/services/mod.rsclients/rook/src/services/account.rsclients/rook/src/services/pool.rsclients/rook/src/services/route.rsclients/rook/src/services/health.rs
🧠 Learnings (4)
📓 Common learnings
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/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests
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
📚 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/services/mod.rsclients/rook/src/services/account.rsclients/rook/src/services/pool.rsclients/rook/src/services/route.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/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests
Applied to files:
clients/rook/src/services/mod.rsclients/rook/src/services/health.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/{security,gateway,tools}/**/*.rs : Treat `src/security/`, `src/gateway/`, `src/tools/` as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Applied to files:
clients/rook/src/services/mod.rs
🔇 Additional comments (2)
clients/rook/src/lib.rs (1)
11-11: LGTM — service module is exported at the crate root.This correctly wires the new shared services layer for downstream consumers.
clients/rook/src/services/mod.rs (1)
1-9: LGTM — shared service module layout is clear.The module docs and public submodule declarations match the intended service-layer boundary.
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 4 file(s) based on 5 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 4 file(s) based on 5 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
clients/rook/src/services/health.rs (1)
141-155:⚠️ Potential issue | 🟠 MajorCooldown expiry still doesn't re-enable unhealthy accounts — previous fix is incomplete.
Walk through the state produced by
mark_failure:status = Unhealthyandcooldown_until = Some(t). Aftertpasses:
- Line 144-148:
Utc::now() < untilis false → falls through (good).- Line 151:
status == Unhealthy && cooldown_until.is_some()is still true (we never clearedcooldown_until), sois_availablereturnsfalseforever.Net result: an account that failed once stays permanently excluded from routing until an out-of-band
mark_success, which defeats the whole cooldown mechanism. This is the same defect flagged on the prior revision; the current guard just reshuffled the condition without fixing it.Proposed fix
fn is_available(&self, account_id: AccountId) -> bool { let health = self.get(account_id); - // Check cooldown first - if expired, account is available regardless of status - if let Some(until) = health.cooldown_until { - if Utc::now() < until { - return false; - } - } - // If no cooldown or cooldown has expired, only reject if explicitly Unhealthy - // and cooldown hasn't expired (which we already checked above) - if health.status == HealthStatus::Unhealthy && health.cooldown_until.is_some() { - return false; - } - true + match health.cooldown_until { + Some(until) if Utc::now() < until => false, + Some(_) => true, // cooldown expired → eligible to retry + None => health.status != HealthStatus::Unhealthy, + } }Consider adding a test that advances time (or uses a past
cooldown_until) to guard against regressions.As per coding guidelines,
**/*.rs: "Focus on Rust idioms, memory safety, and ownership/borrowing correctness."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/rook/src/services/health.rs` around lines 141 - 155, is_available incorrectly treats any Unhealthy record with a non-None cooldown_until as permanently unavailable because it doesn't check expiry when deciding to reject; update is_available to only reject when status == HealthStatus::Unhealthy AND cooldown_until is Some(until) AND Utc::now() < until (i.e., combine the checks so expired cooldowns no longer block availability), and add a unit test that constructs a health record with cooldown_until in the past (or advances time) to assert the account becomes available; reference the is_available method and mark_failure/mark_success behaviors to guide the fix.clients/rook/src/services/route.rs (1)
81-97:⚠️ Potential issue | 🟠 Major
createcan silently overwrite byRouteId.
createchecks for duplicatelogical_modelbut not for an existingRouteId. A caller passing aModelRoutewhoseidcollides with a stored route will have the old route replaced (and thelogical_modelcheck passes because the colliding record matches itself). Mirror the pattern used inaccount.rs/pool.rs: reject duplicate IDs up front.As per coding guidelines,
**/*: "Validate input boundaries, auth/authz implications, and secret management."Proposed fix
fn create(&self, route: ModelRoute) -> Result<RouteId, RookError> { let id = route.id; let mut guard = self.store .lock() .map_err(|e| RookError::Registry(e.to_string()))?; + if guard.contains_key(&id) { + return Err(RookError::Registry(format!("route {id} already exists"))); + } + // Check for duplicate logical_model if guard.values().any(|r| r.logical_model == route.logical_model) { return Err(RookError::Registry(format!( "route with logical_model '{}' already exists", route.logical_model ))); } guard.insert(id, route); Ok(id) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/rook/src/services/route.rs` around lines 81 - 97, The create method (fn create) can overwrite an existing route when the incoming ModelRoute.id collides with a stored RouteId because only logical_model is checked; add an up-front existence check on the id in the locked store (self.store.lock() guard) and return a RookError::Registry error if guard.contains_key(&id) is true (mirroring the duplicate-ID rejection in account.rs/pool.rs) before the logical_model duplicate check so IDs cannot be replaced silently.
🤖 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/src/services/health.rs`:
- Around line 231-233: The two calls to svc.mark_success(good1) and
svc.mark_success(good2) are dropping a #[must_use] Result and should be handled
like the existing svc.mark_failure(...). Change those calls to unwrap (e.g.,
call .unwrap() on the Result) or otherwise propagate/handle the Result so
failures (RookError) are not silently ignored; update the
svc.mark_success(good1) and svc.mark_success(good2) call sites to match the
pattern used with svc.mark_failure(bad, 9999).
---
Duplicate comments:
In `@clients/rook/src/services/health.rs`:
- Around line 141-155: is_available incorrectly treats any Unhealthy record with
a non-None cooldown_until as permanently unavailable because it doesn't check
expiry when deciding to reject; update is_available to only reject when status
== HealthStatus::Unhealthy AND cooldown_until is Some(until) AND Utc::now() <
until (i.e., combine the checks so expired cooldowns no longer block
availability), and add a unit test that constructs a health record with
cooldown_until in the past (or advances time) to assert the account becomes
available; reference the is_available method and mark_failure/mark_success
behaviors to guide the fix.
In `@clients/rook/src/services/route.rs`:
- Around line 81-97: The create method (fn create) can overwrite an existing
route when the incoming ModelRoute.id collides with a stored RouteId because
only logical_model is checked; add an up-front existence check on the id in the
locked store (self.store.lock() guard) and return a RookError::Registry error if
guard.contains_key(&id) is true (mirroring the duplicate-ID rejection in
account.rs/pool.rs) before the logical_model duplicate check so IDs cannot be
replaced silently.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 69f57de0-5970-458f-b00e-b11637db637c
📒 Files selected for processing (4)
clients/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). (3)
- GitHub Check: submit-gradle
- GitHub Check: report / Contributor Quality Report
- 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/account.rsclients/rook/src/services/pool.rsclients/rook/src/services/health.rsclients/rook/src/services/route.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/account.rsclients/rook/src/services/pool.rsclients/rook/src/services/health.rsclients/rook/src/services/route.rs
🧠 Learnings (5)
📓 Common learnings
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/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests
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
📚 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/services/account.rsclients/rook/src/services/pool.rsclients/rook/src/services/route.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/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests
Applied to files:
clients/rook/src/services/health.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
📚 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/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Applied to files:
clients/rook/src/services/health.rs
Comment on lines
+231
to
+233
| svc.mark_success(good1); | ||
| svc.mark_success(good2); | ||
| svc.mark_failure(bad, 9999).unwrap(); |
Contributor
There was a problem hiding this comment.
mark_success return values dropped — #[must_use] warning and hidden test failures.
mark_success now returns Result<(), RookError> (a #[must_use] type), but these two calls discard it. Under -D warnings (clippy clean is claimed in the PR), this should fail CI; more importantly, if the lock were poisoned the test would pass while silently skipping the setup. Unwrap them to match the other call sites.
Proposed fix
- svc.mark_success(good1);
- svc.mark_success(good2);
+ svc.mark_success(good1).unwrap();
+ svc.mark_success(good2).unwrap();
svc.mark_failure(bad, 9999).unwrap();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/rook/src/services/health.rs` around lines 231 - 233, The two calls to
svc.mark_success(good1) and svc.mark_success(good2) are dropping a #[must_use]
Result and should be handled like the existing svc.mark_failure(...). Change
those calls to unwrap (e.g., call .unwrap() on the Result) or otherwise
propagate/handle the Result so failures (RookError) are not silently ignored;
update the svc.mark_success(good1) and svc.mark_success(good2) call sites to
match the pattern used with svc.mark_failure(bad, 9999).
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
Implements issue #581 — establishes the shared domain service layer for Corvus Rook v1.
What's included
clients/rook/src/services/mod.rs— module root re-exporting 4 sub-modulesclients/rook/src/services/account.rs—AccountServicetrait +InMemoryAccountService+ 4 testsclients/rook/src/services/pool.rs—PoolServicetrait +InMemoryPoolService+ member add/remove + 7 testsclients/rook/src/services/route.rs—RouteServicetrait +InMemoryRouteService+resolve()by logical model + 6 testsclients/rook/src/services/health.rs—HealthServicetrait +InMemoryHealthService+ cooldown logic + 5 testsclients/rook/src/lib.rs— wiredpub mod servicesDesign decisions
Arc<Mutex<_>>— services are shared across HTTP handlers, dashboard, and TUI. Using sync traits avoids async lock overhead for in-memory impls; async adapters can wrap these later.InMemory*impls — used for tests and bootstrap. They satisfy the same trait contracts as future SQLite impls.HealthService— tracks per-account cooldown (cooldown_until) and consecutive failures.list_healthy()filters out accounts in cooldown, enabling the routing engine (Extract a reusable routing and account-pool engine from current runtime capabilities #586–588) to avoid failed accounts.Validation
cargo check --manifest-path clients/rook/Cargo.toml— ✅ 0 errorscargo clippy --manifest-path clients/rook/Cargo.toml -- -D warnings— ✅ 0 warningscargo test --manifest-path clients/rook/Cargo.toml— ✅ 36 tests pass (13 pre-existing domain + 23 new service tests)Close #581