feat(jans-cedarling): implement sending health checks to the lock server#14023
Merged
Conversation
Signed-off-by: dagregi <dagmawi.m@proton.me>
Signed-off-by: dagregi <dagmawi.m@proton.me>
Member
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a health subsystem: HealthRegistry for callbacks, HealthTicker background task, LockServerHealthEntry and AuditKind::Health, REST/gRPC transport handling (per-item JSON parsing and proto conversion), LockService wiring to spawn/stop the ticker, logger integration, minor API/formatting tweaks, and example import cleanups. ChangesPeriodic Health Checks System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
mo-auto
added
comp-jans-cedarling
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
jans-cedarling/cedarling/src/jwt/key_service.rs (1)
487-531: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueNew tests use synchronous
.create()instead of.create_async()— consider making async consistently.Both new
#[tokio::test]tests (lines 487 and 534) call.create()on the mockito mock builder while the rest of the test suite (e.g., lines 284–288, 311–315) uses.create_async(). In mockito 1.x the synchronous form is safe in async contexts, but mixing the two styles without reason is inconsistent. Aligning to.create_async()avoids any future surprise if the mockito backend changes.♻️ Suggested fix (same for the second new test at line 534)
- .create(); + .create_async() + .await;🤖 Prompt for 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. In `@jans-cedarling/cedarling/src/jwt/key_service.rs` around lines 487 - 531, In the async test refresh_returns_max_age_from_cache_control_header (and the other new #[tokio::test] added nearby), replace the synchronous mockito call chain ending in .create() with the async variant .create_async() and await it (i.e., call .create_async().await) on the mock builder returned from server.mock(...); this matches the rest of the suite’s async mocks and ensures the mock is created inside the async context.jans-cedarling/cedarling/src/lock/transport/rest.rs (1)
79-107: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReuse
transport::deserialize_entriesfor the Health branch.The Log/Telemetry branches use
transport::deserialize_entriesto deserialize-and-warn-and-error-when-all-malformed; the Health branch reimplements that logic inline (same pattern is duplicated ingrpc.rsHealth branch). SinceLockServerHealthEntrydeserializes directly (noCedarlingX → LockServerXmapping), the helper still works with identityTryFrom<T> for T(provided by the std blanketimpl<T> From<T> for T).♻️ Proposed refactor
AuditKind::Health(url) => { - let entries: Vec<LockServerHealthEntry> = entries - .iter() - .enumerate() - .filter_map(|(idx, v)| match serde_json::from_str(v) { - Ok(e) => Some(e), - Err(e) => { - self.logger.log_any(LockLogEntry::warn(format!( - "failed to parse health entry[{idx}]: {e}" - ))); - None - }, - }) - .collect(); - - if entries.is_empty() { - return Err(TransportError::Serialization( - "all health entries were malformed, nothing to send".to_string(), - )); - } + let entries = transport::deserialize_entries::< + LockServerHealthEntry, + LockServerHealthEntry, + >(entries, "health", warn)?; self.client .raw_client .post(url.as_str()) .json(&entries) .send() .await? .error_for_status()?; },The same refactor applies to the equivalent
AuditKind::Health(_)branch ingrpc.rs(lines 127–149).🤖 Prompt for 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. In `@jans-cedarling/cedarling/src/lock/transport/rest.rs` around lines 79 - 107, Replace the inline deserialization in the AuditKind::Health branch with a call to transport::deserialize_entries so the same deserialize-and-warn-and-error-when-all-malformed behavior is reused: call transport::deserialize_entries::<LockServerHealthEntry>(&entries_iter_or_vec) (leveraging the identity From/TryFrom for LockServerHealthEntry), propagate the TransportError on empty/malformed result, then post the returned Vec<LockServerHealthEntry> via self.client.raw_client.post(url.as_str()).json(&deserialized_entries).send().await?.error_for_status()?; update the Health branch in rest.rs (and mirror the same change in grpc.rs) to remove the duplicated parsing logic and use the helper.
🤖 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 `@jans-cedarling/cedarling/src/lock/health_registry.rs`:
- Around line 37-75: Add unit tests exercising HealthRegistry: create tests that
instantiate HealthRegistry, use register(name, check) and verify collect()
returns the expected HashMap entries (positive case), a test that new
HealthRegistry.collect() returns an empty map (empty-case), a test that multiple
distinct register() calls produce per-name results, and a test for
duplicate-name behavior (register same name twice -> assert expected behavior,
e.g., both entries present or later overrides depending on intended semantics).
Place tests in a #[cfg(test)] mod tests block (or a tests file) and reference
HealthRegistry, register, collect, and RegisteredCheck when locating code; use
simple closures returning distinct strings like "success"/"failure" for
assertions.
- Around line 65-74: The collect() method currently holds the RwLock read guard
while invoking each callback; change it to first snapshot the registry entries
into a temporary Vec of (String, Arc<.../cloneable callback>) by cloning each
check handle (e.g., clone c.name and c.check or wrap callbacks in Arc) while
holding the read lock, then drop the read guard and iterate that snapshot to
call each callback and build the HashMap; reference the collect method, the
checks read lock and the register method/HealthRegistry to avoid blocking
writers or potential deadlocks.
In `@jans-cedarling/cedarling/src/lock/health_ticker.rs`:
- Around line 33-54: The spawn function on HealthTicker takes many parameters
which makes call-sites noisy; change it to accept the existing
HealthTickerParams struct (or create a similar builder) instead of seven
separate args: update the function signature of HealthTicker::spawn to take a
HealthTickerParams (or &HealthTickerParams) and extract transport, health_url,
pdp_id, app_name, interval, logger, and registry from that struct when
constructing Self; update all call sites to pass the HealthTickerParams instance
(or construct one) so the API matches the HealthTickerParams usage introduced in
lock/mod.rs and improves clarity.
In `@jans-cedarling/cedarling/src/lock/mod.rs`:
- Around line 1024-1034: The test currently discards the LockService returned by
LockService::new by using let _ = ..., which allows it to be dropped
immediately; instead bind it to a named local (for example let _lock =
LockService::new(...).await.expect(...)) so the LockService instance (created by
LockService::new with pdp_id, &config, metrics, HttpClientConfig::default(),
etc.) remains in scope for the duration of the assertion and maintains ownership
so its spawned background tasks are not silently canceled by a future Drop.
- Around line 460-518: The create_health_ticker function currently uses expect()
on fallible calls (HeaderValue::from_str, init_http_client, GrpcTransport::new)
and should instead return a Result so callers can propagate InitLockServiceError
consistently; change create_health_ticker signature to return
Result<(CancellationToken, crate::http::JoinHandle<()>), InitLockServiceError>,
map HeaderValue::from_str errors to InitLockServiceError::InvalidAccessToken,
map init_http_client errors to InitializeHttpClientError (or wrap into
InitLockServiceError as used by create_worker), and propagate GrpcTransport::new
errors similarly, then return the HealthTicker::spawn tuple on Ok. Ensure
callers (e.g., where health ticker may be started without create_worker) use ?
to propagate the error.
In `@jans-cedarling/cedarling/src/lock/transport/mapping.rs`:
- Around line 70-79: Change the visibility of LockServerHealthEntry to be as
restrictive as possible: replace the current pub(crate) on the struct with
pub(in crate::lock) so that only modules inside crate::lock (e.g.,
health_ticker.rs, transport/rest.rs, transport/grpc.rs) can access it; update
any imports/usages if the visibility change exposes compile errors and run cargo
check to verify no external code relies on broader visibility.
---
Outside diff comments:
In `@jans-cedarling/cedarling/src/jwt/key_service.rs`:
- Around line 487-531: In the async test
refresh_returns_max_age_from_cache_control_header (and the other new
#[tokio::test] added nearby), replace the synchronous mockito call chain ending
in .create() with the async variant .create_async() and await it (i.e., call
.create_async().await) on the mock builder returned from server.mock(...); this
matches the rest of the suite’s async mocks and ensures the mock is created
inside the async context.
In `@jans-cedarling/cedarling/src/lock/transport/rest.rs`:
- Around line 79-107: Replace the inline deserialization in the
AuditKind::Health branch with a call to transport::deserialize_entries so the
same deserialize-and-warn-and-error-when-all-malformed behavior is reused: call
transport::deserialize_entries::<LockServerHealthEntry>(&entries_iter_or_vec)
(leveraging the identity From/TryFrom for LockServerHealthEntry), propagate the
TransportError on empty/malformed result, then post the returned
Vec<LockServerHealthEntry> via
self.client.raw_client.post(url.as_str()).json(&deserialized_entries).send().await?.error_for_status()?;
update the Health branch in rest.rs (and mirror the same change in grpc.rs) to
remove the duplicated parsing logic and use the helper.
🪄 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: 0f1af361-8dc5-4894-b97a-7dfdcba4a9d0
📒 Files selected for processing (19)
jans-cedarling/cedarling/examples/authorize_unsigned.rsjans-cedarling/cedarling/examples/bulk_authorization_benchmark.rsjans-cedarling/cedarling/examples/lock_integration.rsjans-cedarling/cedarling/examples/log_init.rsjans-cedarling/cedarling/examples/profiling_multi_issuer.rsjans-cedarling/cedarling/examples/profiling_unsigned.rsjans-cedarling/cedarling/src/http/mod.rsjans-cedarling/cedarling/src/jwt/key_service.rsjans-cedarling/cedarling/src/lib.rsjans-cedarling/cedarling/src/lock/health_registry.rsjans-cedarling/cedarling/src/lock/health_ticker.rsjans-cedarling/cedarling/src/lock/mod.rsjans-cedarling/cedarling/src/lock/transport/grpc.rsjans-cedarling/cedarling/src/lock/transport/mapping.rsjans-cedarling/cedarling/src/lock/transport/mod.rsjans-cedarling/cedarling/src/lock/transport/rest.rsjans-cedarling/cedarling/src/log/log_strategy.rsjans-cedarling/cedarling/src/log/mod.rsjans-cedarling/http_utils/src/lib.rs
tareknaser
requested changes
May 7, 2026
Signed-off-by: dagregi <dagmawi.m@proton.me>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
jans-cedarling/cedarling/src/lock/transport/mapping.rs (1)
70-80: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
LockServerHealthEntryvisibility could be tightened.The struct and its fields are only consumed within
crate::lock(byhealth_ticker.rs,transport/rest.rs, andtransport/grpc.rs).pub(in crate::lock)on both the struct and its fields would be the most restrictive valid level.🤖 Prompt for 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. In `@jans-cedarling/cedarling/src/lock/transport/mapping.rs` around lines 70 - 80, Tighten the visibility of the LockServerHealthEntry struct and its fields by changing their visibility from pub(crate) / pub to pub(in crate::lock) so they are only accessible within the crate::lock module; update the struct declaration LockServerHealthEntry and each field (creation_date, event_time, service, node_name, status, engine_status) to use pub(in crate::lock) visibility to restrict access to the intended modules (health_ticker, transport::rest, transport::grpc) while preserving serde derive and the engine_status HashMap behavior.jans-cedarling/cedarling/src/lock/health_registry.rs (1)
89-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
collect()holds the read lock while invoking all callbacks — potential deadlock.If any callback tries to acquire the write lock (e.g., by capturing the registry and calling
register()), it will deadlock on most platforms sincestd::sync::RwLockis not reentrant. Snapshot theArchandles first, drop the guard, then invoke.🤖 Prompt for 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. In `@jans-cedarling/cedarling/src/lock/health_registry.rs` around lines 89 - 98, collect() currently holds the RwLock read guard while invoking each callback which can deadlock if a callback calls back into the registry (e.g., register()); fix by first snapshotting the minimal data (clone the Arc/handles stored in self.checks into a local Vec of (name, check) or clone each check entry), then drop the read guard before invoking any (c.check)() functions so the lock is not held during callback execution; update the collect() implementation to read and clone the entries from self.checks, release the guard, then iterate the cloned list to call each check and build the HashMap.
🤖 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 `@jans-cedarling/cedarling/src/lock/health_registry.rs`:
- Around line 72-84: The register() method currently appends duplicate-named
RegisteredCheck entries into the checks Vec causing collect() to invoke stale
callbacks; change register(&self, name, check) to deduplicate by searching the
write-locked checks Vec for an existing RegisteredCheck with the same name and
replace its check Arc (or update its name+check) instead of pushing a new entry,
so only the latest callback is stored and invoked; update the test
test_register_same_name_twice comment to reflect that registration replaces the
original rather than saying "after dedup".
In `@jans-cedarling/cedarling/src/lock/health_ticker.rs`:
- Around line 102-123: Add unit tests for build_health_entry on HealthTicker to
directly exercise the status aggregation logic: create minimal HealthTicker
instances with a controlled HealthRegistry (use HealthRegistry::new() and
register() to set entries returning HealthStatus::Success or
HealthStatus::Failure) and call build_health_entry() to assert entry.status
equals "unknown" for empty registry, "running" when all registered functions
return Success, and "degraded" when any entry returns Failure (include a mixed
Success+Failure case). Use a small helper like ticker_with_registry(registry) to
construct a HealthTicker with a mock transport or default fields so tests can
focus on build_health_entry and reference the build_health_entry, HealthTicker,
HealthRegistry, and HealthStatus symbols.
- Around line 60-74: The function run currently has pub(crate) visibility but is
only used internally (called from spawn() in the same impl), so reduce its
visibility to private by removing the pub(crate) qualifier on fn run to follow
the "most restrictive visibility" guideline; update the signature for run and
ensure spawn() still calls run(self, cancel_tkn) without changes.
In `@jans-cedarling/cedarling/src/lock/mod.rs`:
- Around line 958-965: The mock_health_endpoint currently uses
mockito::Matcher::Any so it only checks that a request arrived; update
mock_health_endpoint to validate the JSON payload structure similar to
mock_log_endpoint by replacing .match_body(mockito::Matcher::Any) with a
PartialJson matcher that asserts the required fields (e.g., "node_name",
"service", "status" and, if emitted, "engine_status" or other ticker-populated
keys) and their expected types/values; keep the same mock creation chain
(server.mock(...).with_status(200).expect_at_least(2).create()) but supply
.match_body(mockito::Matcher::PartialJson(...)) constructed to validate those
specific fields so payload regressions are caught.
---
Duplicate comments:
In `@jans-cedarling/cedarling/src/lock/health_registry.rs`:
- Around line 89-98: collect() currently holds the RwLock read guard while
invoking each callback which can deadlock if a callback calls back into the
registry (e.g., register()); fix by first snapshotting the minimal data (clone
the Arc/handles stored in self.checks into a local Vec of (name, check) or clone
each check entry), then drop the read guard before invoking any (c.check)()
functions so the lock is not held during callback execution; update the
collect() implementation to read and clone the entries from self.checks, release
the guard, then iterate the cloned list to call each check and build the
HashMap.
In `@jans-cedarling/cedarling/src/lock/transport/mapping.rs`:
- Around line 70-80: Tighten the visibility of the LockServerHealthEntry struct
and its fields by changing their visibility from pub(crate) / pub to pub(in
crate::lock) so they are only accessible within the crate::lock module; update
the struct declaration LockServerHealthEntry and each field (creation_date,
event_time, service, node_name, status, engine_status) to use pub(in
crate::lock) visibility to restrict access to the intended modules
(health_ticker, transport::rest, transport::grpc) while preserving serde derive
and the engine_status HashMap behavior.
🪄 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: 046cbe6c-bdec-4530-8e7c-f47a982ad077
📒 Files selected for processing (6)
jans-cedarling/cedarling/src/lib.rsjans-cedarling/cedarling/src/lock/health_registry.rsjans-cedarling/cedarling/src/lock/health_ticker.rsjans-cedarling/cedarling/src/lock/mod.rsjans-cedarling/cedarling/src/lock/transport/grpc.rsjans-cedarling/cedarling/src/lock/transport/mapping.rs
Signed-off-by: dagregi <dagmawi.m@proton.me>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@jans-cedarling/cedarling/src/lock/health_ticker.rs`:
- Line 187: The assertions in the tests test_build_health_entry_all_success and
test_build_health_entry_mixed lack descriptive messages; update every assert_eq!
and assert!(...) that checks entry.engine_status.len() and
engine_status.get(...) to include a clear failure message (e.g., "expected 2
engine statuses" for assert_eq!(entry.engine_status.len(), 2) and messages
describing which engine key/value is missing or mismatched for
engine_status.get(...)). Locate the checks in the functions
test_build_health_entry_all_success and test_build_health_entry_mixed and add
concise descriptive strings to each assertion so failures explain what was being
validated.
- Line 228: The test test_build_health_entry_mixed asserts the wrong expected
length for entry.engine_status (currently assert_eq!(entry.engine_status.len(),
4)); change the assertion to assert_eq!(entry.engine_status.len(), 3) to match
the three registered engines ("core", "policy_store", "trusted_issuers") so the
test reflects the actual number of entries created by the test setup.
🪄 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: f1c9b90c-3740-4bd8-be2d-2a53c32b621d
📒 Files selected for processing (1)
jans-cedarling/cedarling/src/lock/health_ticker.rs
Signed-off-by: dagregi <dagmawi.m@proton.me>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
jans-cedarling/cedarling/src/lock/health_ticker.rs (1)
187-187:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing descriptive messages on
assert_eq!calls.Lines 187, 229, 230–233, and 234–237 are still missing the required assertion messages.
📝 Proposed fix
- assert_eq!(entry.engine_status.len(), 2); + assert_eq!(entry.engine_status.len(), 2, "two registered components should produce two engine_status entries");- assert_eq!(entry.engine_status.len(), 4); + assert_eq!(entry.engine_status.len(), 4, "four registered components should produce four engine_status entries"); - assert_eq!( - entry.engine_status.get("core"), - Some(&HealthStatus::Success) - ); - assert_eq!( - entry.engine_status.get("trusted_issuers"), - Some(&HealthStatus::Failure) - ); + assert_eq!( + entry.engine_status.get("core"), + Some(&HealthStatus::Success), + "core component should report Success" + ); + assert_eq!( + entry.engine_status.get("trusted_issuers"), + Some(&HealthStatus::Failure), + "trusted_issuers component should report Failure" + );As per coding guidelines: "All assertions must include a descriptive message explaining what is being tested."
Also applies to: 229-237
🤖 Prompt for 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. In `@jans-cedarling/cedarling/src/lock/health_ticker.rs` at line 187, Several assert_eq! macros lack descriptive messages; update each assert_eq! (including the one asserting entry.engine_status.len() == 2 and the subsequent asserts around entry fields such as entry.last_updated, entry.node_id, and related status checks) to include a clear message explaining what is being validated (e.g., "expected 2 engine_status entries", "expected last_updated to equal ...", "expected node_id to be ..."). Keep messages short but specific and apply the change to all assert_eq! occurrences noted in the diff.
🤖 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 `@jans-cedarling/cedarling/src/lock/health_ticker.rs`:
- Around line 31-42: The HealthTicker struct and its spawn method currently use
pub(crate) but are only needed by the parent lock module; change the visibility
to pub(super) on the struct declaration (pub(super) struct HealthTicker<T:
AuditTransport>) and on the associated function signature (pub(super) fn
spawn...) so both the type and constructor are restricted to the parent module;
ensure you update both the struct line and the impl's spawn signature for
HealthTicker<T: AuditTransport + 'static>.
---
Duplicate comments:
In `@jans-cedarling/cedarling/src/lock/health_ticker.rs`:
- Line 187: Several assert_eq! macros lack descriptive messages; update each
assert_eq! (including the one asserting entry.engine_status.len() == 2 and the
subsequent asserts around entry fields such as entry.last_updated,
entry.node_id, and related status checks) to include a clear message explaining
what is being validated (e.g., "expected 2 engine_status entries", "expected
last_updated to equal ...", "expected node_id to be ..."). Keep messages short
but specific and apply the change to all assert_eq! occurrences noted in the
diff.
🪄 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: d609e28b-3748-47f4-862d-f3c14ad2287e
📒 Files selected for processing (1)
jans-cedarling/cedarling/src/lock/health_ticker.rs
Signed-off-by: dagregi <dagmawi.m@proton.me>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
jans-cedarling/cedarling/src/lock/health_ticker.rs (1)
187-187:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
assert_eq!calls still missing required descriptive messages.Lines 187 and 229–237 have
assert_eq!calls without the mandatory descriptive third argument required by the coding guidelines.📝 Proposed fix
- assert_eq!(entry.engine_status.len(), 2); + assert_eq!(entry.engine_status.len(), 2, "two registered components should produce two engine_status entries");- assert_eq!(entry.engine_status.len(), 4); + assert_eq!(entry.engine_status.len(), 4, "four registered components should produce four engine_status entries"); - assert_eq!( - entry.engine_status.get("core"), - Some(&HealthStatus::Success) - ); - assert_eq!( - entry.engine_status.get("trusted_issuers"), - Some(&HealthStatus::Failure) - ); + assert_eq!( + entry.engine_status.get("core"), + Some(&HealthStatus::Success), + "core component should report Success" + ); + assert_eq!( + entry.engine_status.get("trusted_issuers"), + Some(&HealthStatus::Failure), + "trusted_issuers component should report Failure" + );As per coding guidelines: "All assertions must include a descriptive message explaining what is being tested."
Also applies to: 229-237
🤖 Prompt for 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. In `@jans-cedarling/cedarling/src/lock/health_ticker.rs` at line 187, The assertions like assert_eq!(entry.engine_status.len(), 2) (and the assert_eq! calls around lines 229–237) lack the required descriptive third-argument message; update each assert_eq! to include a clear message describing what is being tested (for example: assert_eq!(entry.engine_status.len(), 2, "expected two engine_status entries for Entry {{id/description}}") or similar for each case), referencing the specific value under test (e.g., entry.engine_status.len(), entry.queue_status, etc.) so the failure output explains the expectation and the variable being asserted.
🤖 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.
Duplicate comments:
In `@jans-cedarling/cedarling/src/lock/health_ticker.rs`:
- Line 187: The assertions like assert_eq!(entry.engine_status.len(), 2) (and
the assert_eq! calls around lines 229–237) lack the required descriptive
third-argument message; update each assert_eq! to include a clear message
describing what is being tested (for example:
assert_eq!(entry.engine_status.len(), 2, "expected two engine_status entries for
Entry {{id/description}}") or similar for each case), referencing the specific
value under test (e.g., entry.engine_status.len(), entry.queue_status, etc.) so
the failure output explains the expectation and the variable being asserted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e4ad9b8-0f64-48b2-89c0-df6c3dd0bc07
📒 Files selected for processing (1)
jans-cedarling/cedarling/src/lock/health_ticker.rs
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
olehbozhok
previously approved these changes
May 8, 2026
Signed-off-by: dagregi <dagmawi.m@proton.me>
tareknaser
approved these changes
May 9, 2026
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
5 participants
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.
Prepare
Description
Target issue
closes #10850
Implementation Details
Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit
New Features
Refactor