feat: add monitors CLI command (API parity)#81
Conversation
Adds the monitors resource to the CLI with full CRUD support: - monitors list - monitors get <id> - monitors create (with --conditions, --name, --scope, etc.) - monitors update <id> (PATCH semantics) - monitors delete <id> - monitors test-evaluate <id> --run-id <id> - monitors events <id> [list] Verified working against API with create, list, get, update, delete, events, and test-evaluate operations.
WalkthroughThis PR adds a "monitors" resource across the CLI. It introduces Monitor/MonitorEvent data models with an auth-token redaction deserializer, a MonitorsClient with list/get/create/update/delete/test-evaluate/list-events API methods, a new commands::monitors module implementing MonitorCommands and EventCommands with a dispatcher, CLI wiring (Commands::Monitors variant, resource/operation mappings, run() routing), agent-discovery entries for "monitors" and "tags" resources, and corresponding test coverage. Changes
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
🟢 |
borgesius
left a comment
borgesius
left a comment
There was a problem hiding this comment.
Holding approval on one correctness issue: CreateArgs.evaluation_type has a clap default and is always inserted into the request, so --input-json with an evaluation_type gets silently overwritten with ON_RUN_COMPLETE unless the user also passes --evaluation-type. Please make it Option and only insert when the flag is provided, or otherwise avoid overwriting an input-json value.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/cli_tests.rs (1)
2175-2256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect tests, but no coverage for the default
evaluation_typefallback or other monitors subcommands.The two tests correctly validate
evaluation_typepreservation vs.--evaluation-typeoverride, matching theMonitorCommands::Createlogic insrc/commands/monitors.rs. However, there's no test for the default-value path (whenevaluation_typeis absent from both--input-jsonand the flag, it should fall back toDEFAULT_EVALUATION_TYPE), nor any coverage forlist,get,update,delete,test-evaluate, oreventssubcommands.Consider adding at least one test for the default fallback and a smoke test per remaining subcommand to match the coverage depth given to other resources in this file.
🤖 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 `@tests/cli_tests.rs` around lines 2175 - 2256, The current monitors tests cover create override behavior, but they miss the default evaluation_type fallback and the rest of the monitors CLI surface. Add a test around MonitorCommands::Create that verifies when neither --input-json nor --evaluation-type supplies a value, the request uses DEFAULT_EVALUATION_TYPE from src/commands/monitors.rs. Also add lightweight smoke coverage for the remaining monitors subcommands in tests/cli_tests.rs: list, get, update, delete, test-evaluate, and events, using their command names and expected request shapes.src/commands/monitors.rs (2)
141-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded default
page_sizeduplicatesListEventArgs'sdefault_value.The
Nonebranch of theEventsmatch constructsListEventArgs { outcome: None, page_size: 50 }with a literal50, duplicating thedefault_value = "50"declared onListEventArgs::page_size. If the default changes in one place, it's easy to miss the other.♻️ Proposed fix using Default
+impl Default for ListEventArgs { + fn default() -> Self { + Self { outcome: None, page_size: 50 } + } +} + ... - None => ListEventArgs { - outcome: None, - page_size: 50, - }, + None => ListEventArgs::default(),Also applies to: 316-343
🤖 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 `@src/commands/monitors.rs` around lines 141 - 146, The `Events` match is hardcoding `page_size: 50` when constructing `ListEventArgs`, duplicating the default already declared on `ListEventArgs::page_size`. Update the `None` branch to rely on a shared default source instead of a literal, ideally by implementing or using `Default` for `ListEventArgs` and reusing it where `ListEventArgs` is constructed. Apply the same cleanup anywhere else `ListEventArgs` is manually built so the default stays in one place.
70-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce duplicated CSV-splitting/field-building logic between Create and Update.
agent_ids,required_tags, andscheduled_run_idsare parsed identically via manualsplit(',')+trim()in bothCreateandUpdate(6 near-identical blocks total). Other resources in this codebase (e.g.run_templates.rs) avoid this by letting clap parse comma-separated values directly intoVec<String>viavalue_delimiter, removing the need for manual splitting and its duplication entirely.♻️ Proposed refactor using clap's value_delimiter
#[arg(long)] - agent_ids: Option<String>, + #[arg(long, value_delimiter = ',')] + agent_ids: Option<Vec<String>>, #[arg(long)] - required_tags: Option<String>, + #[arg(long, value_delimiter = ',')] + required_tags: Option<Vec<String>>, #[arg(long)] - scheduled_run_ids: Option<String>, + #[arg(long, value_delimiter = ',')] + scheduled_run_ids: Option<Vec<String>>,Apply the same field changes to
UpdateArgs, then in bothexecutebranches replace:- if let Some(ref ids) = args.agent_ids { - let v: Vec<String> = ids.split(',').map(|s| s.trim().to_string()).collect(); - input_json::insert(&mut input, "agent_ids", Some(v))?; - } - if let Some(ref ids) = args.required_tags { - let v: Vec<String> = ids.split(',').map(|s| s.trim().to_string()).collect(); - input_json::insert(&mut input, "required_tags", Some(v))?; - } - if let Some(ref ids) = args.scheduled_run_ids { - let v: Vec<String> = ids.split(',').map(|s| s.trim().to_string()).collect(); - input_json::insert(&mut input, "scheduled_run_ids", Some(v))?; - } + input_json::insert(&mut input, "agent_ids", args.agent_ids)?; + input_json::insert(&mut input, "required_tags", args.required_tags)?; + input_json::insert(&mut input, "scheduled_run_ids", args.scheduled_run_ids)?;Also applies to: 100-126, 200-211, 263-274
🤖 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 `@src/commands/monitors.rs` around lines 70 - 97, The CreateArgs and UpdateArgs paths in monitors.rs duplicate manual CSV parsing for agent_ids, required_tags, and scheduled_run_ids; switch these clap fields to parse directly into Vec<String> using value_delimiter, matching the approach used in run_templates.rs. Update both CreateArgs and UpdateArgs, then simplify the execute logic to use the parsed vectors directly instead of repeating split(',') and trim() field-building blocks.
🤖 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 `@src/cli.rs`:
- Around line 117-120: The Monitors CLI variant is using the wrong branch name
for the events subcommand path, which causes next_actions::list_result to emit
an unmapped monitor-events argv. Update the `Monitors` entry in `src/cli.rs` to
keep the events branch under `monitors`, and make sure the corresponding
`commands::monitors::MonitorCommands` handling in `src/commands/monitors.rs`
continues to resolve the existing `context|get` flow without introducing a new
`monitor-events` resource unless you also add a matching top-level command.
In `@src/commands/monitors.rs`:
- Around line 100-126: Expose the missing monitor update flag by adding
`evaluation_type` to `UpdateArgs` alongside the other update fields in
`monitors.rs`. Wire it through the same clap parsing pattern as the existing
options so `UpdateMonitorRequest` can receive it from `update_monitor` without
requiring `--input-json`. Use the existing `UpdateArgs` and request-building
code to locate where the new option should be forwarded.
---
Nitpick comments:
In `@src/commands/monitors.rs`:
- Around line 141-146: The `Events` match is hardcoding `page_size: 50` when
constructing `ListEventArgs`, duplicating the default already declared on
`ListEventArgs::page_size`. Update the `None` branch to rely on a shared default
source instead of a literal, ideally by implementing or using `Default` for
`ListEventArgs` and reusing it where `ListEventArgs` is constructed. Apply the
same cleanup anywhere else `ListEventArgs` is manually built so the default
stays in one place.
- Around line 70-97: The CreateArgs and UpdateArgs paths in monitors.rs
duplicate manual CSV parsing for agent_ids, required_tags, and
scheduled_run_ids; switch these clap fields to parse directly into Vec<String>
using value_delimiter, matching the approach used in run_templates.rs. Update
both CreateArgs and UpdateArgs, then simplify the execute logic to use the
parsed vectors directly instead of repeating split(',') and trim()
field-building blocks.
In `@tests/cli_tests.rs`:
- Around line 2175-2256: The current monitors tests cover create override
behavior, but they miss the default evaluation_type fallback and the rest of the
monitors CLI surface. Add a test around MonitorCommands::Create that verifies
when neither --input-json nor --evaluation-type supplies a value, the request
uses DEFAULT_EVALUATION_TYPE from src/commands/monitors.rs. Also add lightweight
smoke coverage for the remaining monitors subcommands in tests/cli_tests.rs:
list, get, update, delete, test-evaluate, and events, using their command names
and expected request shapes.
🪄 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: CHILL
Plan: Pro
Run ID: 0a8b50d5-5078-4cce-9064-bcc21a67e111
📒 Files selected for processing (8)
src/agent_discovery.rssrc/cli.rssrc/client/mod.rssrc/client/models/mod.rssrc/client/models/monitor.rssrc/commands/mod.rssrc/commands/monitors.rstests/cli_tests.rs
3 participants
Summary
Adds the
coval monitorscommand to the CLI, bringing it to parity with the/v1/monitorsAPI endpoints.What's New
coval monitors list [--scope <scope>]— list all monitorscoval monitors get <id>— get a monitor by IDcoval monitors create— create a monitor with --name, --conditions (JSON), --scope, etc.coval monitors update <id>— PATCH-update a monitorcoval monitors delete <id>— delete (soft-delete) a monitorcoval monitors test-evaluate <id> --run-id <id>— test-evaluate a monitor against a runcoval monitors events <id> [list]— list monitor trigger eventsFiles Changed
src/client/models/monitor.rs— Monitor, MonitorEvent, MonitorCondition, and request/response structs with Tabular implssrc/commands/monitors.rs— MonitorCommands enum with all CRUD + test-evaluate + events subcommandssrc/client/mod.rs— MonitorsClient with list, get, create, update, delete, test_evaluate, list_events methodssrc/client/models/mod.rs— registered new monitor modulesrc/commands/mod.rs— registered monitors modulesrc/cli.rs— wired Monitors command into Commands enum with context + execute handlersVerification
Tested all operations against the Coval API (
cal-demoorg):cargo build— passescargo clippy— no warningscargo test— 90/90 tests passGreptile Summary
Adds the
coval monitorscommand group with full CRUD parity against the/v1/monitorsAPI:list,get,create,update,delete,test-evaluate, andevents. This is a large but mechanical addition that follows the same structure as the existingreports,tags, and other resource modules.getnow correctly unwraps throughGetMonitorResponse,evaluation_typeisOption<String>with acontains_keyguard so--input-jsonvalues are never silently overwritten, and--name/--conditionsare validated beforefinishwith actionablebail!messages. Two new integration tests cover theevaluation_typepreservation logic.channelsanddispatched_channelsare redacted at deserialization time via a recursiveredact_auth_tokenshelper, with unit tests proving the behavior.Confidence Score: 5/5
Safe to merge — all previously identified runtime defects have been corrected and are now covered by tests.
The three bugs flagged in the prior review round (wrong deserialization path for
get, silent default overwrite ofevaluation_type, and missing pre-flight validation for required fields) are all fixed and verified with new integration tests. The newmonitorsmodule is structurally consistent with the rest of the codebase, andcargo testreports 90/90 passing.No files require special attention.
Reviews (5): Last reviewed commit: "merge origin/main into monitors CLI PR" | Re-trigger Greptile