fix(rook): implement operational doctor diagnostics#709
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors effective-config assembly and validation into a reusable seam, adds startup-readiness diagnostics for DB/registry/server/assets/auth, updates doctor to emit structured check results (summary/guidance/details) and advisory checks, and adds tests and docs for deterministic local-first operational diagnostics. Changes
Sequence DiagramsequenceDiagram
actor User
participant Doctor as "rook doctor"
participant Config as "RookConfig / assemble_effective_config"
participant Server as "diagnose_startup_readiness"
participant Registry as "RookRegistry"
participant DB as "SqliteDb"
participant Dashboard as "DashboardAssets"
User->>Doctor: invoke
Doctor->>Config: assemble_effective_config(input)
Config->>Config: apply defaults/file/env/cli (no validation)
Config->>Config: validate_non_auth()
Config->>Config: inbound_auth.validate()
Config-->>Doctor: effective RookConfig
Doctor->>Server: diagnose_startup_readiness(&config)
Server->>Config: effective_bind_target()
Server->>Registry: check_startup_readiness(db_path)
Registry->>DB: check_startup_readiness(path)
DB->>DB: open/create + migrations
DB-->>Registry: DbStartupReadiness {opened,guidance}
Registry-->>Server: DbStartupReadiness
Server->>Dashboard: assets_ready()
Dashboard-->>Server: bool
Server-->>Doctor: StartupReadinessSnapshot
Doctor->>Doctor: build DoctorReport (summary/guidance/details + advisory_checks)
Doctor-->>User: render_report() + exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 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 |
Deploying corvus with
|
| Latest commit: |
78f3476
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8af1c729.corvus-42x.pages.dev |
| Branch Preview URL: | https://rook.corvus-42x.pages.dev |
✅ Contributor ReportUser: @yacosta738
Contributor Report evaluates based on public GitHub activity. Analysis period: 2025-04-28 to 2026-04-28 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/config/mod.rs`:
- Around line 178-180: The effective_bind_target method constructs "host:port"
naively causing IPv6 ambiguity; update effective_bind_target to detect if
self.host contains ':' (or is a literal IPv6) and, when so, wrap the host in
square brackets before appending ":{port}" so IPv6 like "::1" becomes
"[::1]:4141"; modify the function that returns String (effective_bind_target) to
perform this check using self.host and self.port and return the bracketed form
for IPv6 and unchanged host:port for IPv4/hostname.
In `@clients/rook/src/dashboard/mod.rs`:
- Around line 33-37: The current code calls .lock().expect("assets override lock
should work") on the Mutex inside ASSETS_READY_OVERRIDE which will panic if the
lock is poisoned; instead recover the inner guard on poison to avoid crashing
diagnostics: replace the .lock().expect(...) usage (and the analogous
occurrences around the 49-51 range) with handling that calls
.lock().unwrap_or_else(|poison| poison.into_inner()) (or the equivalent
map_or_else on the result) so you still get the locked guard even if the mutex
was poisoned, then continue using .as_ref() as before.
- Around line 47-52: The current set_assets_ready_override mutates
process-global ASSETS_READY_OVERRIDE and requires manual reset, which leaks
state; replace this with a scoped guard pattern: introduce an
AssetsReadyOverrideGuard (or similar) whose constructor acquires
ASSETS_READY_OVERRIDE, sets the override value, and whose Drop implementation
resets the override to None, then change callers to use
AssetsReadyOverrideGuard::new(value) (or provide a convenience
with_scoped_assets_ready_override) instead of calling set_assets_ready_override
directly; update or add tests to use the guard so overrides are automatically
cleared even if panics occur and remove or deprecate the standalone
set_assets_ready_override to avoid accidental global leaks.
In `@clients/rook/src/doctor.rs`:
- Around line 257-266: The advisory block in render logic (the loop over
report.advisory_checks) only emits name, status and summary but omits actionable
fields; update the loop that builds lines for advisory checks (iterating over
report.advisory_checks in doctor.rs) to also append the advisory check's details
and guidance when present—e.g., after the existing formatted summary line add
conditional lines for check.details and check.guidance (or
render_details/render_guidance helpers if available) so warn/fail advisories
surface actionable text to operators.
In `@clients/rook/tests/doctor_operational_diagnostics.rs`:
- Around line 38-57: The test
doctor_enabled_inbound_auth_without_token_reports_inbound_auth_failure relies on
default DB behavior causing nondeterministic failures; create and initialize a
temporary DB (as used by the happy-path test) and inject its path into the
environment (e.g., set ROOK_DB_PATH or the same config key the suite uses)
before calling run_with_config_path(None, &env) so the test isolates
inbound-auth errors; ensure you reference the temp DB lifecycle in the test and
remove any reliance on the default DB path so the inbound_auth check is the only
failure source.
- Around line 85-93: The test sets global state with
rook::dashboard::set_assets_ready_override(Some(false)) but restores it only
after operations, which won't run if expect_err panics; replace the manual
restore with a panic-safe drop guard: capture the previous override
(rook::dashboard::get_assets_ready_override() or call set_assets_ready_override
to read), then create a small RAII guard struct (e.g., AssetsReadyOverrideGuard)
that sets the override to Some(false) on creation and restores the previous
value in Drop, and use that guard around the run_with_config_path/ensure_success
calls so the original state is restored even on panic; update the test to remove
the final set_assets_ready_override(None) and rely on the guard's Drop.
In
`@openspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/proposal.md`:
- Line 5: The sentence in the proposal contains nonstandard phrasing "needs
answered"; update that phrase to a standard form such as "needs to be answered"
(or "must be answered") so the line reads e.g. "`rook doctor` already exists,
but today it only covers a narrow subset of the operational questions an
operator needs to be answered before putting Rook into service." Locate and edit
the phrase "needs answered" in the proposal text and replace with the chosen
standard phrasing, preserving the rest of the sentence.
In
`@openspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/state.yaml`:
- Around line 4-19: Update the top-level status key in the state.yaml so it
matches the completed phases: change the top-level "status" value from "planned"
to "completed" (or "archived" if you want to indicate archival) so the top-level
"status" aligns with the "phases" mapping where explore, propose, spec, design,
tasks, apply, and verify are all "completed".
In
`@openspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/verify-report.md`:
- Line 1: The markdown's first heading uses level 2 ("## Verification Report")
which violates markdownlint MD041; change that first heading to a top-level
heading by replacing "## Verification Report" with "# Verification Report" so
the document begins with an H1.
🪄 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: 6b8a7e26-045c-4955-b65a-7f3bc0179168
📒 Files selected for processing (15)
clients/rook/src/config/mod.rsclients/rook/src/dashboard/mod.rsclients/rook/src/db/mod.rsclients/rook/src/doctor.rsclients/rook/src/main.rsclients/rook/src/registry/mod.rsclients/rook/src/server/mod.rsclients/rook/tests/doctor_operational_diagnostics.rsopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/design.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/proposal.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/specs/gateway/spec.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/state.yamlopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/tasks.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/verify-report.mdopenspec/specs/gateway/spec.md
📜 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). (5)
- GitHub Check: report / Contributor Quality Report
- GitHub Check: sonar
- GitHub Check: pr-checks
- GitHub Check: submit-gradle
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (3)
**/*
⚙️ 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:
openspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/state.yamlopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/verify-report.mdclients/rook/src/registry/mod.rsopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/proposal.mdclients/rook/src/dashboard/mod.rsopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/design.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/tasks.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/specs/gateway/spec.mdclients/rook/src/main.rsclients/rook/src/server/mod.rsclients/rook/tests/doctor_operational_diagnostics.rsclients/rook/src/db/mod.rsclients/rook/src/config/mod.rsopenspec/specs/gateway/spec.mdclients/rook/src/doctor.rs
**/*.{md,mdx}
⚙️ CodeRabbit configuration file
**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.
For user-facing docs, check EN/ES parity or explicitly note pending translation gaps.
Files:
openspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/verify-report.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/proposal.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/design.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/tasks.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/specs/gateway/spec.mdopenspec/specs/gateway/spec.md
**/*.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/registry/mod.rsclients/rook/src/dashboard/mod.rsclients/rook/src/main.rsclients/rook/src/server/mod.rsclients/rook/tests/doctor_operational_diagnostics.rsclients/rook/src/db/mod.rsclients/rook/src/config/mod.rsclients/rook/src/doctor.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: Include threat/risk notes and rollback strategy for security, runtime, and gateway changes; add or update tests for boundary checks and failure modes
Applied to files:
openspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/proposal.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/specs/gateway/spec.md
📚 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:
openspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/specs/gateway/spec.mdclients/rook/src/main.rsopenspec/specs/gateway/spec.md
📚 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/main.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/**/*.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/src/main.rsclients/rook/tests/doctor_operational_diagnostics.rsclients/rook/src/doctor.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/main.rs
🪛 LanguageTool
openspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/proposal.md
[style] ~5-~5: The double modal “needs answered” is nonstandard (only accepted in certain dialects). Consider “to be answered”.
Context: ...operational questions an operator needs answered before putting Rook into service. It va...
(NEEDS_FIXED)
openspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/specs/gateway/spec.md
[style] ~66-~66: Consider a more concise word here.
Context: ...tion, or other network-dependent checks in order to determine overall success. Doctor cove...
(IN_ORDER_TO_PREMIUM)
🪛 markdownlint-cli2 (0.22.1)
openspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/verify-report.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
openspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/design.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (4)
clients/rook/src/main.rs (1)
733-736: Stronger doctor contract assertions look good.These updates improve validation of structured detail output and failure wording while preserving secret-safety checks.
Also applies to: 777-810
clients/rook/src/registry/mod.rs (1)
61-63: Startup-readiness seam delegation is clean.Keeping readiness evaluation in
SqliteDbavoids duplication and keeps registry behavior cohesive.clients/rook/src/server/mod.rs (1)
82-104: Startup readiness snapshot extraction is well-factored.This keeps doctor/startup checks aligned without coupling diagnostics to socket bind flow.
clients/rook/src/db/mod.rs (1)
121-151: DB startup-readiness diagnostics and failure-mode tests are solid.The readiness probe and guidance helper are backed by practical failing-scenario coverage, which improves operator actionability.
Also applies to: 293-372
| pub fn set_assets_ready_override(value: Option<bool>) { | ||
| *ASSETS_READY_OVERRIDE | ||
| .get_or_init(|| Mutex::new(None)) | ||
| .lock() | ||
| .expect("assets override lock should work") = value; | ||
| } |
There was a problem hiding this comment.
Global override API is leak-prone and can cause order-dependent test failures.
set_assets_ready_override relies on manual reset, so a panic/early-return can leave stale process-global state that affects other tests.
Suggested refactor (scoped override guard)
+#[doc(hidden)]
+pub struct AssetsReadyOverrideGuard {
+ previous: Option<bool>,
+}
+
+impl Drop for AssetsReadyOverrideGuard {
+ fn drop(&mut self) {
+ if let Ok(mut slot) = ASSETS_READY_OVERRIDE
+ .get_or_init(|| Mutex::new(None))
+ .lock()
+ {
+ *slot = self.previous;
+ }
+ }
+}
+
+#[doc(hidden)]
+pub fn scoped_assets_ready_override(value: Option<bool>) -> AssetsReadyOverrideGuard {
+ let mut slot = ASSETS_READY_OVERRIDE
+ .get_or_init(|| Mutex::new(None))
+ .lock()
+ .unwrap_or_else(|poisoned| poisoned.into_inner());
+ let previous = *slot;
+ *slot = value;
+ AssetsReadyOverrideGuard { previous }
+}As per coding guidelines "Security first, performance second... Look for behavioral regressions, missing tests, and contract breaks across modules."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/rook/src/dashboard/mod.rs` around lines 47 - 52, The current
set_assets_ready_override mutates process-global ASSETS_READY_OVERRIDE and
requires manual reset, which leaks state; replace this with a scoped guard
pattern: introduce an AssetsReadyOverrideGuard (or similar) whose constructor
acquires ASSETS_READY_OVERRIDE, sets the override value, and whose Drop
implementation resets the override to None, then change callers to use
AssetsReadyOverrideGuard::new(value) (or provide a convenience
with_scoped_assets_ready_override) instead of calling set_assets_ready_override
directly; update or add tests to use the guard so overrides are automatically
cleared even if panics occur and remove or deprecate the standalone
set_assets_ready_override to avoid accidental global leaks.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/doctor_operational_diagnostics.rs`:
- Around line 4-16: The temp DB created in initialized_db_env via
RookRegistry::open is never removed; change initialized_db_env to create the DB
inside a tempfile-managed resource (e.g., tempfile::NamedTempFile or
tempfile::TempDir), use its path with RookRegistry::open, and keep the
tempfile/TempDir guard alive for the duration of the test (for example return
the guard alongside the HashMap or wrap both in a small struct) so the OS file
is removed on drop; ensure the ROOK_DB_PATH value still points to the
guard.path().to_string_lossy() result and use the initialized_db_env and
RookRegistry::open symbols to locate the change.
🪄 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: f556d42b-dec3-4a3b-bc05-40a506ee5078
📒 Files selected for processing (7)
clients/rook/src/config/mod.rsclients/rook/src/dashboard/mod.rsclients/rook/src/doctor.rsclients/rook/tests/doctor_operational_diagnostics.rsopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/proposal.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/state.yamlopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/verify-report.md
📜 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: pr-checks
- GitHub Check: sonar
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (3)
**/*
⚙️ 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:
openspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/state.yamlclients/rook/src/dashboard/mod.rsopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/verify-report.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/proposal.mdclients/rook/src/config/mod.rsclients/rook/tests/doctor_operational_diagnostics.rsclients/rook/src/doctor.rs
**/*.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/dashboard/mod.rsclients/rook/src/config/mod.rsclients/rook/tests/doctor_operational_diagnostics.rsclients/rook/src/doctor.rs
**/*.{md,mdx}
⚙️ CodeRabbit configuration file
**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.
For user-facing docs, check EN/ES parity or explicitly note pending translation gaps.
Files:
openspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/verify-report.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/proposal.md
🧠 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/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/dashboard/mod.rsclients/rook/tests/doctor_operational_diagnostics.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/dashboard/mod.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/tools/**/*.rs : Implement `Tool` trait in `src/tools/` with strict parameter schema, validate and sanitize all inputs, and return structured `ToolResult` without panics in runtime path
Applied to files:
clients/rook/src/dashboard/mod.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: Include threat/risk notes and rollback strategy for security, runtime, and gateway changes; add or update tests for boundary checks and failure modes
Applied to files:
openspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/proposal.md
📚 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/tests/doctor_operational_diagnostics.rsclients/rook/src/doctor.rs
🔇 Additional comments (15)
clients/rook/src/doctor.rs (5)
257-275: Advisory checks now render full details.Previous review concern addressed: advisory checks now render
detailsandguidancefields, not just name/status/summary.
49-93: Solid early-return on config failure.Config validation failure returns immediately with actionable guidance rather than proceeding with subsequent checks that would fail or produce confusing output. The effective bind target is still reported even on failure for debugging context.
95-121: Startup-equivalent DB readiness check.Using
server::diagnose_startup_readiness()ensures doctor validates the same DB initialization path thatserveuses, preventing false confidence from a weaker read-only check.
171-211: Secret-safe inbound auth reporting.The
inbound_auth_check_resultfunction usesauth_state.summary()which returns only static strings like"enabled with token configured"— the actual token value is never included in summary, guidance, or details fields.
123-144: Assets check provides actionable guidance.Clear failure messaging with specific remediation steps ("rebuild the production binary with embedded dashboard assets") helps operators understand what action to take.
openspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/verify-report.md (1)
1-81: LGTM!The verification report is well-structured with clear completeness metrics, command outcomes, and spec compliance evidence. The previous MD041 heading issue has been addressed.
openspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/proposal.md (1)
1-121: LGTM!The proposal is comprehensive with clear scope boundaries, risk assessment, and rollback strategy. The previous grammar issue ("needs answered") has been corrected.
openspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/state.yaml (1)
1-29: LGTM!The previous status inconsistency has been resolved. Top-level status now correctly shows
completedto match all completed phases.clients/rook/src/dashboard/mod.rs (1)
30-61: LGTM! Past issues addressed.The poison-safe lock handling and RAII guard pattern resolve previous review concerns. The guard sets override on construction and clears to
Noneon drop, ensuring cleanup even on panic.Note: Nested guards would interfere (inner drop clears outer's value), but since this is
#[doc(hidden)]and test-only, that's acceptable for current usage.clients/rook/tests/doctor_operational_diagnostics.rs (2)
37-58: LGTM!Previous review concerns addressed: test now uses
initialized_db_env()to isolate inbound-auth failure from unrelated DB conditions.
83-105: LGTM!Previous panic-safety concern resolved. The test now uses
AssetsReadyOverrideGuardfor RAII-based cleanup instead of manualset_assets_ready_override(None).clients/rook/src/config/mod.rs (4)
178-184: LGTM!IPv6 bind-target formatting issue from previous review is resolved. The implementation correctly wraps IPv6 addresses in brackets.
235-258: Clean validation split.Separating
validate_non_auth()fromvalidate()allows doctor to run diagnostics on partially-valid configs and still report useful auth-specific failures separately. Good design for progressive diagnostics.
872-880: Secret-safe operator state reporting.
summary()returns only static strings ("disabled","enabled with token configured", etc.) — token value never exposed. This aligns with the spec requirement to "confirm that required credentials/config are present without leaking secret values."
587-601: Shared config assembly seam.
assemble_effective_configprovides the unvalidated config assembly that bothload_effective_configand doctor can use. This ensures doctor evaluates the same effective configuration asserve, preventing drift.
| async fn initialized_db_env() -> HashMap<String, String> { | ||
| let db_path = std::env::temp_dir().join(format!( | ||
| "rook-doctor-operational-{}.db", | ||
| uuid::Uuid::new_v4() | ||
| )); | ||
| rook::registry::RookRegistry::open(&db_path.to_string_lossy()) | ||
| .await | ||
| .expect("test database should initialize"); | ||
| HashMap::from([( | ||
| "ROOK_DB_PATH".to_string(), | ||
| db_path.to_string_lossy().to_string(), | ||
| )]) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Temp DB files persist after tests.
The RookRegistry::open() call creates the DB but there's no cleanup. These temp files accumulate over test runs.
Consider wrapping with a guard or using tempfile::NamedTempFile for automatic cleanup, though this is low priority for test code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/rook/tests/doctor_operational_diagnostics.rs` around lines 4 - 16,
The temp DB created in initialized_db_env via RookRegistry::open is never
removed; change initialized_db_env to create the DB inside a tempfile-managed
resource (e.g., tempfile::NamedTempFile or tempfile::TempDir), use its path with
RookRegistry::open, and keep the tempfile/TempDir guard alive for the duration
of the test (for example return the guard alongside the HashMap or wrap both in
a small struct) so the OS file is removed on drop; ensure the ROOK_DB_PATH value
still points to the guard.path().to_string_lossy() result and use the
initialized_db_env and RookRegistry::open symbols to locate the change.
|
1 participant
Related Issues
fixes #679
Summary
rook doctordiagnostics with structured pass/warn/fail output and actionable operator guidanceTested Information
cargo fmt --all -- --checkcargo clippy --all-targets -- -D warningscargo testDocumentation Impact
openspec/specs/gateway/spec.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/verify-report.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/proposal.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/design.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/tasks.mdopenspec/changes/archive/2026-04-28-rook-doctor-operational-diagnostics-679/specs/gateway/spec.mdBreaking Changes
None.
Checklist