From 8acd63718acbc30645ceffb272dc1816ba5e2dcf Mon Sep 17 00:00:00 2001 From: Callum Reid Date: Tue, 4 Aug 2026 10:57:48 -0700 Subject: [PATCH 1/8] Add reports merge and reports rows commands `coval reports merge` combines two or more reports into one grouped report, mirroring the app's "Merge reports" action: one group per source report, each simulation attributed to the first selected report that contains it. Also adds `coval reports rows` (the endpoint merge pages through, previously a recorded coverage gap) and `--view-mode` on `reports create`. Depends on the backend change that accepts custom dimensions on POST /v1/reports. --- README.md | 7 +- api-coverage.toml | 6 +- src/agent_discovery.rs | 52 +++++-- src/client/mod.rs | 27 ++++ src/client/models/report.rs | 75 ++++++++++ src/commands/reports.rs | 211 +++++++++++++++++++++++++++- tests/cli_tests.rs | 266 ++++++++++++++++++++++++++++++++++++ 7 files changed, 622 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index c1f7990..fc1cd66 100644 --- a/README.md +++ b/README.md @@ -63,7 +63,7 @@ coval simulations list --run-id | `coval dashboards` | Manage dashboards and widgets | | `coval review-annotations` | Manage human-review annotations | | `coval review-projects` | Manage human-review projects | -| `coval reports` | Save multi-run comparison reports | +| `coval reports` | Save, merge, and read multi-run comparison reports | | `coval monitors` | Manage production monitors and events | | `coval tags` | Manage resource tags | | `coval traces` | Search and inspect OpenTelemetry traces | @@ -145,6 +145,11 @@ coval reports create \ --run-ids run1,run2 \ --compare-by test_case +# Merge existing reports into one report with a group per source report +coval reports merge \ + --name "Q3 Scorecard" \ + --report-ids 01HAAAAAAAAAAAAAAAAAAAAAAA,01HBBBBBBBBBBBBBBBBBBBBBBB + # Upload a custom background sound coval personas background-sounds upload ./lobby-noise.mp3 \ --display-name "Lobby Noise" diff --git a/api-coverage.toml b/api-coverage.toml index f049de9..c38a886 100644 --- a/api-coverage.toml +++ b/api-coverage.toml @@ -5,7 +5,7 @@ catalog_url = "https://api.coval.dev/v1/openapi" reviewed_at = "2026-08-04" published_operations = 174 -cli_supported_operations = 117 +cli_supported_operations = 118 [[allowed_extra]] operation = "POST /test-cases/{test_case_id}/media:upload-url" @@ -184,10 +184,6 @@ reason = "Persona version rollback remains to be modeled under COVAL-2079." operation = "POST /personas/{persona_id}/duplicate" reason = "Persona duplication remains to be modeled under COVAL-2079." -[[known_gap]] -operation = "GET /reports/{report_id}/rows" -reason = "Report row expansion remains to be modeled under COVAL-2079." - [[known_gap]] operation = "POST /review-annotations:withMetricOutputs" reason = "Advanced review reads remain to be modeled under COVAL-2079." diff --git a/src/agent_discovery.rs b/src/agent_discovery.rs index b62e1ee..0990151 100644 --- a/src/agent_discovery.rs +++ b/src/agent_discovery.rs @@ -559,7 +559,16 @@ const RESOURCE_SPECS: &[ResourceSpec] = &[ }, ResourceSpec { name: "reports", - commands: &["context", "list", "get", "create", "update", "delete"], + commands: &[ + "context", + "list", + "get", + "rows", + "create", + "merge", + "update", + "delete", + ], description: "Reports save a comparison view across multiple runs, grouped by a chosen dimension.", id_name: "report_id", id_format: "26-character ULID", @@ -567,21 +576,36 @@ const RESOURCE_SPECS: &[ResourceSpec] = &[ optional: &[], produces: &["shareable report views"], related: &["runs", "agents", "personas", "mutations"], - workflows: &[WorkflowSpec { - name: "Compare runs by test case", - argv: &[ - "reports", - "create", - "--name", - "", - "--run-ids", - "", - "--compare-by", - "test_case", - ], - }], + workflows: &[ + WorkflowSpec { + name: "Compare runs by test case", + argv: &[ + "reports", + "create", + "--name", + "", + "--run-ids", + "", + "--compare-by", + "test_case", + ], + }, + WorkflowSpec { + name: "Merge reports into one grouped comparison", + argv: &[ + "reports", + "merge", + "--name", + "", + "--report-ids", + ",", + ], + }, + ], pitfalls: &[ "metadata_key is required when compare-by is metadata and rejected otherwise.", + "merge needs at least two distinct report IDs and reads every source report's rows.", + "A simulation in several merged reports lands in the first one's group only.", "PUBLIC reports also mark their runs public.", ], }, diff --git a/src/client/mod.rs b/src/client/mod.rs index 3642641..6ddfedb 100644 --- a/src/client/mod.rs +++ b/src/client/mod.rs @@ -1200,6 +1200,33 @@ impl ReportsClient<'_> { let url = self.0.url(&format!("/v1/reports/{id}")); self.0.delete(url).await } + + pub async fn rows( + &self, + id: &str, + cursor: Option<&str>, + limit: Option, + metric_ids: Option<&str>, + simulation_output_ids: Option<&str>, + ) -> Result { + let mut url = self.0.url(&format!("/v1/reports/{id}/rows")); + { + let mut pairs = url.query_pairs_mut(); + if let Some(cursor) = cursor { + pairs.append_pair("cursor", cursor); + } + if let Some(limit) = limit { + pairs.append_pair("limit", &limit.to_string()); + } + if let Some(metric_ids) = metric_ids { + pairs.append_pair("metric_ids", metric_ids); + } + if let Some(simulation_output_ids) = simulation_output_ids { + pairs.append_pair("simulation_output_ids", simulation_output_ids); + } + } + self.0.get(url).await + } } impl WidgetsClient<'_> { diff --git a/src/client/models/report.rs b/src/client/models/report.rs index 8e2cd25..6ea8b8e 100644 --- a/src/client/models/report.rs +++ b/src/client/models/report.rs @@ -40,6 +40,37 @@ pub enum CompareBy { #[serde(rename = "metadata")] #[value(name = "metadata")] Metadata, + #[serde(rename = "custom")] + #[value(name = "custom")] + Custom, +} + +#[derive(Debug, Clone, Copy, Serialize, Deserialize, ValueEnum)] +pub enum ReportViewMode { + #[serde(rename = "rows")] + #[value(name = "rows")] + Rows, + #[serde(rename = "grouped")] + #[value(name = "grouped")] + Grouped, +} + +/// One named bucket of simulations inside a report's custom dimension. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ReportCustomDimensionGroup { + pub id: String, + pub name: String, + #[serde(default)] + pub simulation_ids: Vec, +} + +/// A caller-defined grouping of a report's simulations. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ReportCustomDimension { + pub id: String, + pub name: String, + pub groups: Vec, + pub hide_unassigned: bool, } #[derive(Debug, Clone, Copy, Serialize, Deserialize, ValueEnum)] @@ -61,6 +92,12 @@ pub struct CreateReportRequest { #[serde(skip_serializing_if = "Option::is_none")] pub metadata_key: Option, #[serde(skip_serializing_if = "Option::is_none")] + pub custom_dimensions: Option>, + #[serde(skip_serializing_if = "Option::is_none")] + pub custom_dimension_id: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub view_mode: Option, + #[serde(skip_serializing_if = "Option::is_none")] pub permissions: Option, } @@ -99,6 +136,44 @@ pub struct UpdateReportResponse { pub report: Report, } +/// One simulation in a report, with its metric outputs kept in `extra` for JSON output. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ReportRow { + pub simulation_id: String, + pub run_id: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub persona_id: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub agent_id: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub status: Option, + #[serde(flatten)] + pub extra: serde_json::Map, +} + +#[derive(Debug, Serialize, Deserialize)] +pub struct ListReportRowsResponse { + #[serde(default)] + pub rows: Vec, + pub next_page_token: Option, +} + +impl Tabular for ReportRow { + fn headers() -> Vec<&'static str> { + vec!["SIMULATION ID", "RUN ID", "AGENT", "PERSONA", "STATUS"] + } + + fn row(&self) -> Vec { + vec![ + self.simulation_id.clone(), + self.run_id.clone(), + self.agent_id.clone().unwrap_or_default(), + self.persona_id.clone().unwrap_or_default(), + self.status.clone().unwrap_or_default(), + ] + } +} + impl Tabular for Report { fn headers() -> Vec<&'static str> { vec!["ID", "NAME", "COMPARE BY", "RUNS", "PERMISSIONS"] diff --git a/src/commands/reports.rs b/src/commands/reports.rs index 9aa0751..a979c53 100644 --- a/src/commands/reports.rs +++ b/src/commands/reports.rs @@ -1,22 +1,34 @@ +use std::collections::HashSet; + use anyhow::Result; use clap::{Args, Subcommand}; use crate::client::models::{ - CompareBy, CreateReportRequest, ReportPermission, UpdateReportRequest, + CompareBy, CreateReportRequest, ReportCustomDimension, ReportCustomDimensionGroup, + ReportPermission, ReportViewMode, UpdateReportRequest, }; use crate::client::CovalClient; use crate::input_json::{self, InputJsonArg}; use crate::next_actions; use crate::output::{ - emit_list_with_actions, emit_one_with_actions, emit_success_with_actions, OutputContext, + emit_list_with_actions, emit_one_with_actions, emit_success_with_actions, print_list, + OutputContext, OutputFormat, }; +/// A merged report carries exactly one dimension, so a fixed ID is unambiguous. +const MERGE_DIMENSION_ID: &str = "merged-reports"; +const MERGE_DIMENSION_NAME: &str = "Report"; +const MERGE_ROWS_PAGE_SIZE: u32 = 500; +const MERGE_MAX_PAGES_PER_REPORT: usize = 200; + #[derive(Subcommand)] pub enum ReportCommands { Context, List(ListArgs), Get(GetArgs), + Rows(RowsArgs), Create(CreateArgs), + Merge(MergeArgs), Update(UpdateArgs), Delete(DeleteArgs), } @@ -27,7 +39,9 @@ impl ReportCommands { Self::Context => "context", Self::List(_) => "list", Self::Get(_) => "get", + Self::Rows(_) => "rows", Self::Create(_) => "create", + Self::Merge(_) => "merge", Self::Update(_) => "update", Self::Delete(_) => "delete", } @@ -50,6 +64,24 @@ pub struct GetArgs { report_id: String, } +#[derive(Args)] +pub struct RowsArgs { + /// Report ID (26-character ULID) + report_id: String, + /// Opaque cursor from a previous response's next_page_token + #[arg(long)] + cursor: Option, + /// Rows per page (1-2000, default 2000) + #[arg(long)] + limit: Option, + /// Comma-separated metric IDs to include + #[arg(long, value_delimiter = ',')] + metric_ids: Option>, + /// Comma-separated simulation IDs to restrict the page to + #[arg(long, value_delimiter = ',')] + simulation_ids: Option>, +} + #[derive(Args)] pub struct CreateArgs { #[command(flatten)] @@ -66,11 +98,30 @@ pub struct CreateArgs { /// Metadata key to group by (required when --compare-by metadata, rejected otherwise) #[arg(long)] metadata_key: Option, + /// Report layout (default rows) + #[arg(long, value_enum)] + view_mode: Option, /// Report visibility (default PRIVATE) #[arg(long, value_enum)] permissions: Option, } +#[derive(Args)] +pub struct MergeArgs { + /// Comma-separated IDs of the reports to merge (min 2, must be distinct) + #[arg(long, required = true, value_delimiter = ',')] + report_ids: Vec, + /// Display name for the merged report (1-200 characters) + #[arg(long)] + name: String, + /// Label for the generated grouping dimension (default "Report") + #[arg(long, default_value = MERGE_DIMENSION_NAME)] + dimension_name: String, + /// Merged report visibility (default PRIVATE) + #[arg(long, value_enum)] + permissions: Option, +} + #[derive(Args)] pub struct UpdateArgs { /// Report ID (26-character ULID) @@ -130,14 +181,37 @@ pub async fn execute(cmd: ReportCommands, client: &CovalClient, ctx: &OutputCont next_actions::item_result("reports", &report.id), ); } + ReportCommands::Rows(args) => { + let response = client + .reports() + .rows( + &args.report_id, + args.cursor.as_deref(), + args.limit, + args.metric_ids.map(|ids| ids.join(",")).as_deref(), + args.simulation_ids.map(|ids| ids.join(",")).as_deref(), + ) + .await?; + let actions = next_actions::item_result("reports", &args.report_id); + if ctx.human() { + print_list(&response.rows, OutputFormat::Table); + if let Some(cursor) = &response.next_page_token { + println!("Next cursor: {cursor}"); + } + } else { + emit_one_with_actions(ctx, "reports", operation, &response, actions); + } + } ReportCommands::Create(args) => { let mut input = args.input_json.object()?; input_json::insert(&mut input, "name", args.name)?; input_json::insert(&mut input, "run_ids", args.run_ids)?; input_json::insert(&mut input, "compare_by", args.compare_by)?; input_json::insert(&mut input, "metadata_key", args.metadata_key)?; + input_json::insert(&mut input, "view_mode", args.view_mode)?; input_json::insert(&mut input, "permissions", args.permissions)?; validate_metadata_key(&input)?; + validate_custom_dimensions(&input)?; let req: CreateReportRequest = input_json::finish(input)?; if req.run_ids.is_empty() { anyhow::bail!("--run-ids requires at least one run ID"); @@ -151,6 +225,16 @@ pub async fn execute(cmd: ReportCommands, client: &CovalClient, ctx: &OutputCont next_actions::item_result("reports", &report.id), ); } + ReportCommands::Merge(args) => { + let report = merge_reports(args, client).await?; + emit_one_with_actions( + ctx, + "reports", + operation, + &report, + next_actions::item_result("reports", &report.id), + ); + } ReportCommands::Update(args) => { let mut input = args.input_json.object()?; input_json::insert(&mut input, "name", args.name)?; @@ -183,6 +267,129 @@ pub async fn execute(cmd: ReportCommands, client: &CovalClient, ctx: &OutputCont Ok(()) } +/// Build one report grouping the source reports' simulations, one group per source. +/// +/// Mirrors the app's "Merge reports" action: a simulation is attributed to the first +/// selected report that contains it, so overlapping reports do not double-count. +async fn merge_reports( + args: MergeArgs, + client: &CovalClient, +) -> Result { + let mut requested = HashSet::new(); + for report_id in &args.report_ids { + if !requested.insert(report_id.as_str()) { + anyhow::bail!("--report-ids contains {report_id} twice; ids must be distinct"); + } + } + if args.report_ids.len() < 2 { + anyhow::bail!("--report-ids requires at least two report IDs to merge"); + } + + let mut seen_simulation_ids = HashSet::new(); + let mut seen_run_ids = HashSet::new(); + let mut run_ids: Vec = Vec::new(); + let mut groups: Vec = Vec::new(); + + for report_id in &args.report_ids { + let source = client.reports().get(report_id).await?; + for run_id in &source.run_ids { + if seen_run_ids.insert(run_id.clone()) { + run_ids.push(run_id.clone()); + } + } + + let mut simulation_ids: Vec = Vec::new(); + let mut cursor: Option = None; + let mut drained = false; + for _ in 0..MERGE_MAX_PAGES_PER_REPORT { + let page = client + .reports() + .rows( + report_id, + cursor.as_deref(), + Some(MERGE_ROWS_PAGE_SIZE), + None, + None, + ) + .await?; + for row in page.rows { + if seen_simulation_ids.insert(row.simulation_id.clone()) { + simulation_ids.push(row.simulation_id); + } + } + match page.next_page_token { + Some(token) => cursor = Some(token), + None => { + drained = true; + break; + } + } + } + if !drained { + anyhow::bail!( + "report {report_id} has more than {} rows; merge cannot page past that", + MERGE_MAX_PAGES_PER_REPORT as u32 * MERGE_ROWS_PAGE_SIZE + ); + } + + let name = source.name.trim(); + groups.push(ReportCustomDimensionGroup { + id: source.id.clone(), + name: if name.is_empty() { + "Unnamed report".to_string() + } else { + name.to_string() + }, + simulation_ids, + }); + } + + if run_ids.is_empty() { + anyhow::bail!( + "the selected reports have no runs to merge; a merged report needs at least one run" + ); + } + + let request = CreateReportRequest { + name: args.name, + run_ids, + compare_by: Some(CompareBy::Custom), + metadata_key: None, + custom_dimensions: Some(vec![ReportCustomDimension { + id: MERGE_DIMENSION_ID.to_string(), + name: args.dimension_name, + groups, + hide_unassigned: false, + }]), + custom_dimension_id: Some(MERGE_DIMENSION_ID.to_string()), + view_mode: Some(ReportViewMode::Grouped), + permissions: args.permissions, + }; + + Ok(client.reports().create(request).await?) +} + +/// Validate the custom_dimensions / compare_by pairing before sending. +/// +/// The API requires `custom_dimensions` when `compare_by` is custom and rejects it +/// otherwise. Only `--input-json` can carry them on `reports create`; `reports merge` +/// assembles them itself. +fn validate_custom_dimensions(input: &serde_json::Map) -> Result<()> { + let is_custom = input.get("compare_by").and_then(serde_json::Value::as_str) == Some("custom"); + let has_custom_dimensions = input.contains_key("custom_dimensions"); + + if is_custom && !has_custom_dimensions { + anyhow::bail!( + "custom_dimensions is required when --compare-by is custom; use `coval reports merge` \ + to build them from existing reports" + ); + } + if !is_custom && has_custom_dimensions { + anyhow::bail!("custom_dimensions can only be set when --compare-by is custom"); + } + Ok(()) +} + /// Validate the metadata_key / compare_by pairing before sending. /// /// The API requires `metadata_key` when `compare_by` is metadata and rejects it diff --git a/tests/cli_tests.rs b/tests/cli_tests.rs index 6330661..5c86230 100644 --- a/tests/cli_tests.rs +++ b/tests/cli_tests.rs @@ -23,6 +23,17 @@ impl Match for BodyExcludes { } } +struct QueryParamAbsent(&'static str); + +impl Match for QueryParamAbsent { + fn matches(&self, request: &Request) -> bool { + !request + .url + .query_pairs() + .any(|(key, _)| key.as_ref() == self.0) + } +} + fn write_skill(root: &std::path::Path, id: &str, description: &str) { let skill_dir = root.join("skills").join(id); std::fs::create_dir_all(&skill_dir).unwrap(); @@ -3835,6 +3846,261 @@ fn test_reports_create_metadata_key_rejected_without_metadata_compare_by() { )); } +#[tokio::test] +async fn test_reports_rows_forwards_paging_and_filters() { + let mock_server = MockServer::start().await; + + Mock::given(method("GET")) + .and(path("/v1/reports/01HXXXXXXXXXXXXXXXXXXXXXXX/rows")) + .and(header("X-API-Key", "test_key")) + .and(query_param("cursor", "500")) + .and(query_param("limit", "50")) + .and(query_param("metric_ids", "metric1,metric2")) + .and(query_param("simulation_output_ids", "sim1")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "rows": [ + { + "simulation_id": "sim1", + "run_id": "run1", + "agent_id": "agent1", + "persona_id": "persona1", + "status": null, + "metrics": [] + } + ], + "next_page_token": null + }))) + .mount(&mock_server) + .await; + + coval() + .arg("--api-key") + .arg("test_key") + .arg("--api-url") + .arg(mock_server.uri()) + .arg("reports") + .arg("rows") + .arg("01HXXXXXXXXXXXXXXXXXXXXXXX") + .arg("--cursor") + .arg("500") + .arg("--limit") + .arg("50") + .arg("--metric-ids") + .arg("metric1,metric2") + .arg("--simulation-ids") + .arg("sim1") + .assert() + .success() + .stdout(predicate::str::contains("sim1")); +} + +async fn mount_merge_source( + mock_server: &MockServer, + report_id: &str, + name: &str, + run_ids: Value, + row_pages: Vec<(Option<&str>, Value, Option<&str>)>, +) { + Mock::given(method("GET")) + .and(path(format!("/v1/reports/{report_id}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "report": { + "id": report_id, + "name": name, + "run_ids": run_ids, + "compare_by": "none", + "permissions": "PRIVATE" + } + }))) + .mount(mock_server) + .await; + + for (cursor, rows, next_page_token) in row_pages { + let mock = Mock::given(method("GET")) + .and(path(format!("/v1/reports/{report_id}/rows"))) + .and(query_param("limit", "500")); + let mock = match cursor { + Some(cursor) => mock.and(query_param("cursor", cursor)), + None => mock.and(QueryParamAbsent("cursor")), + }; + mock.respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "rows": rows, + "next_page_token": next_page_token + }))) + .mount(mock_server) + .await; + } +} + +#[tokio::test] +async fn test_reports_merge_builds_one_group_per_source_report() { + let mock_server = MockServer::start().await; + + mount_merge_source( + &mock_server, + "01HAAAAAAAAAAAAAAAAAAAAAAA", + "Baseline", + json!(["run1", "run2"]), + vec![ + ( + None, + json!([{"simulation_id": "sim1", "run_id": "run1"}]), + Some("500"), + ), + ( + Some("500"), + json!([{"simulation_id": "sim2", "run_id": "run2"}]), + None, + ), + ], + ) + .await; + + // sim2 is in both reports; first-seen attribution keeps it in the Baseline group only. + mount_merge_source( + &mock_server, + "01HBBBBBBBBBBBBBBBBBBBBBBB", + "Candidate", + json!(["run2", "run3"]), + vec![( + None, + json!([ + {"simulation_id": "sim2", "run_id": "run2"}, + {"simulation_id": "sim3", "run_id": "run3"} + ]), + None, + )], + ) + .await; + + Mock::given(method("POST")) + .and(path("/v1/reports")) + .and(header("X-API-Key", "test_key")) + .and(body_partial_json(json!({ + "name": "Q3 Scorecard", + "run_ids": ["run1", "run2", "run3"], + "compare_by": "custom", + "view_mode": "grouped", + "custom_dimension_id": "merged-reports", + "custom_dimensions": [{ + "id": "merged-reports", + "name": "Report", + "hide_unassigned": false, + "groups": [ + { + "id": "01HAAAAAAAAAAAAAAAAAAAAAAA", + "name": "Baseline", + "simulation_ids": ["sim1", "sim2"] + }, + { + "id": "01HBBBBBBBBBBBBBBBBBBBBBBB", + "name": "Candidate", + "simulation_ids": ["sim3"] + } + ] + }] + }))) + .respond_with(ResponseTemplate::new(201).set_body_json(json!({ + "report": { + "id": "01HMERGEDMERGEDMERGEDMERGE", + "name": "Q3 Scorecard", + "run_ids": ["run1", "run2", "run3"], + "compare_by": "custom", + "custom_dimension_id": "merged-reports", + "permissions": "PRIVATE" + } + }))) + .mount(&mock_server) + .await; + + coval() + .arg("--api-key") + .arg("test_key") + .arg("--api-url") + .arg(mock_server.uri()) + .arg("reports") + .arg("merge") + .arg("--name") + .arg("Q3 Scorecard") + .arg("--report-ids") + .arg("01HAAAAAAAAAAAAAAAAAAAAAAA,01HBBBBBBBBBBBBBBBBBBBBBBB") + .assert() + .success() + .stdout(predicate::str::contains("01HMERGEDMERGEDMERGEDMERGE")); +} + +#[test] +fn test_reports_merge_requires_two_reports() { + coval() + .arg("--api-key") + .arg("test_key") + .arg("reports") + .arg("merge") + .arg("--name") + .arg("Q3 Scorecard") + .arg("--report-ids") + .arg("01HAAAAAAAAAAAAAAAAAAAAAAA") + .assert() + .failure() + .stderr(predicate::str::contains("requires at least two report IDs")); +} + +#[test] +fn test_reports_merge_rejects_duplicate_report_ids() { + coval() + .arg("--api-key") + .arg("test_key") + .arg("reports") + .arg("merge") + .arg("--name") + .arg("Q3 Scorecard") + .arg("--report-ids") + .arg("01HAAAAAAAAAAAAAAAAAAAAAAA,01HAAAAAAAAAAAAAAAAAAAAAAA") + .assert() + .failure() + .stderr(predicate::str::contains("ids must be distinct")); +} + +#[test] +fn test_reports_create_custom_requires_custom_dimensions() { + coval() + .arg("--api-key") + .arg("test_key") + .arg("reports") + .arg("create") + .arg("--name") + .arg("Bad Report") + .arg("--run-ids") + .arg("run1") + .arg("--compare-by") + .arg("custom") + .assert() + .failure() + .stderr(predicate::str::contains("custom_dimensions is required")); +} + +#[test] +fn test_reports_create_custom_dimensions_rejected_without_custom_compare_by() { + coval() + .arg("--api-key") + .arg("test_key") + .arg("reports") + .arg("create") + .arg("--name") + .arg("Bad Report") + .arg("--run-ids") + .arg("run1") + .arg("--compare-by") + .arg("agent") + .arg("--input-json") + .arg(r#"{"custom_dimensions": [{"id": "d1", "name": "Report", "groups": [], "hide_unassigned": false}]}"#) + .assert() + .failure() + .stderr(predicate::str::contains( + "custom_dimensions can only be set when --compare-by is custom", + )); +} + #[tokio::test] async fn test_traces_search_sends_structured_filters_and_preserves_cursor() { let mock_server = MockServer::start().await; From 3f2e5b4cebc78e2ed278cc7284892be4b93b3cc3 Mon Sep 17 00:00:00 2001 From: Callum Reid Date: Tue, 4 Aug 2026 11:54:09 -0700 Subject: [PATCH 2/8] Treat null custom_dimensions as absent when validating create input --- src/commands/reports.rs | 5 ++++- tests/cli_tests.rs | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/src/commands/reports.rs b/src/commands/reports.rs index a979c53..72c8ffc 100644 --- a/src/commands/reports.rs +++ b/src/commands/reports.rs @@ -376,7 +376,10 @@ async fn merge_reports( /// assembles them itself. fn validate_custom_dimensions(input: &serde_json::Map) -> Result<()> { let is_custom = input.get("compare_by").and_then(serde_json::Value::as_str) == Some("custom"); - let has_custom_dimensions = input.contains_key("custom_dimensions"); + // An explicit null deserializes to None, so it counts as absent rather than as a value. + let has_custom_dimensions = input + .get("custom_dimensions") + .is_some_and(|value| !value.is_null()); if is_custom && !has_custom_dimensions { anyhow::bail!( diff --git a/tests/cli_tests.rs b/tests/cli_tests.rs index 5c86230..fc032e2 100644 --- a/tests/cli_tests.rs +++ b/tests/cli_tests.rs @@ -4101,6 +4101,46 @@ fn test_reports_create_custom_dimensions_rejected_without_custom_compare_by() { )); } +#[test] +fn test_reports_create_custom_rejects_null_custom_dimensions() { + coval() + .arg("--api-key") + .arg("test_key") + .arg("reports") + .arg("create") + .arg("--name") + .arg("Bad Report") + .arg("--run-ids") + .arg("run1") + .arg("--compare-by") + .arg("custom") + .arg("--input-json") + .arg(r#"{"custom_dimensions": null}"#) + .assert() + .failure() + .stderr(predicate::str::contains("custom_dimensions is required")); +} + +#[test] +fn test_reports_create_allows_null_custom_dimensions_without_custom_compare_by() { + coval() + .arg("--api-key") + .arg("test_key") + .arg("reports") + .arg("create") + .arg("--name") + .arg("Report") + .arg("--run-ids") + .arg("run1") + .arg("--compare-by") + .arg("agent") + .arg("--input-json") + .arg(r#"{"custom_dimensions": null}"#) + .assert() + .failure() + .stderr(predicate::str::contains("custom_dimensions can only be set").not()); +} + #[tokio::test] async fn test_traces_search_sends_structured_filters_and_preserves_cursor() { let mock_server = MockServer::start().await; From ac52039975867ef4a732adbac5f0fd68386c2069 Mon Sep 17 00:00:00 2001 From: Callum Reid Date: Wed, 5 Aug 2026 09:54:50 -0700 Subject: [PATCH 3/8] Regenerate api-coverage-report.md for the new rows coverage --- api-coverage-report.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/api-coverage-report.md b/api-coverage-report.md index 232a556..31ee249 100644 --- a/api-coverage-report.md +++ b/api-coverage-report.md @@ -12,9 +12,9 @@ opens or updates a PR only when coverage actually changes. | --- | ---: | | Reconciliation status | PASS | | Published operations | 174 | -| First-class CLI operations | 117 | -| Reviewed gaps | 57 | -| Client operations | 125 | +| First-class CLI operations | 118 | +| Reviewed gaps | 56 | +| Client operations | 126 | Catalog: https://api.coval.dev/v1/openapi @@ -72,7 +72,6 @@ Catalog: https://api.coval.dev/v1/openapi - `GET /organization/monitoring-metrics` - `GET /personas/tags` - `GET /personas/{persona_id}/versions` -- `GET /reports/{report_id}/rows` - `GET /review-annotations/metric-health-stats` - `GET /review-projects/{project_id}/insights` - `GET /review-projects/{project_id}/metric-agreement` From 4d400a26f4b2f0aa0f00a36b862f038cdab8537f Mon Sep 17 00:00:00 2001 From: Callum Reid Date: Wed, 5 Aug 2026 14:47:32 -0700 Subject: [PATCH 4/8] Match the API's custom-dimension caps, defaults and id pairing --- src/client/models/report.rs | 2 + src/commands/reports.rs | 73 ++++++++-- tests/cli_tests.rs | 262 ++++++++++++++++++++++++++++++++++++ 3 files changed, 327 insertions(+), 10 deletions(-) diff --git a/src/client/models/report.rs b/src/client/models/report.rs index 6ea8b8e..90fe614 100644 --- a/src/client/models/report.rs +++ b/src/client/models/report.rs @@ -70,6 +70,8 @@ pub struct ReportCustomDimension { pub id: String, pub name: String, pub groups: Vec, + /// The API defaults this to false, so an input payload may omit it. + #[serde(default)] pub hide_unassigned: bool, } diff --git a/src/commands/reports.rs b/src/commands/reports.rs index 72c8ffc..33ad05a 100644 --- a/src/commands/reports.rs +++ b/src/commands/reports.rs @@ -20,6 +20,10 @@ const MERGE_DIMENSION_ID: &str = "merged-reports"; const MERGE_DIMENSION_NAME: &str = "Report"; const MERGE_ROWS_PAGE_SIZE: u32 = 500; const MERGE_MAX_PAGES_PER_REPORT: usize = 200; +/// Mirrors the API's per-group `simulation_ids` and per-dimension `groups` ceilings. +/// Checked client-side so an oversized merge fails before paging the whole source. +const MERGE_MAX_SIMULATIONS_PER_GROUP: usize = 10_000; +const MERGE_MAX_SOURCE_REPORTS: usize = 500; #[derive(Subcommand)] pub enum ReportCommands { @@ -284,6 +288,12 @@ async fn merge_reports( if args.report_ids.len() < 2 { anyhow::bail!("--report-ids requires at least two report IDs to merge"); } + if args.report_ids.len() > MERGE_MAX_SOURCE_REPORTS { + anyhow::bail!( + "--report-ids has {} reports; a merged report holds at most {MERGE_MAX_SOURCE_REPORTS} groups", + args.report_ids.len() + ); + } let mut seen_simulation_ids = HashSet::new(); let mut seen_run_ids = HashSet::new(); @@ -317,6 +327,14 @@ async fn merge_reports( simulation_ids.push(row.simulation_id); } } + // Checked per page so an oversized source stops here instead of paging to the + // ceiling and then having the create rejected. + if simulation_ids.len() > MERGE_MAX_SIMULATIONS_PER_GROUP { + anyhow::bail!( + "report {report_id} contributes more than {MERGE_MAX_SIMULATIONS_PER_GROUP} \ + simulations; a merged report's group cannot hold more than that" + ); + } match page.next_page_token { Some(token) => cursor = Some(token), None => { @@ -369,26 +387,61 @@ async fn merge_reports( Ok(client.reports().create(request).await?) } -/// Validate the custom_dimensions / compare_by pairing before sending. +/// Validate the custom_dimensions / custom_dimension_id / compare_by pairing before sending. /// -/// The API requires `custom_dimensions` when `compare_by` is custom and rejects it -/// otherwise. Only `--input-json` can carry them on `reports create`; `reports merge` -/// assembles them itself. +/// The API requires `custom_dimensions` when `compare_by` is custom and rejects both it and +/// `custom_dimension_id` otherwise. Only `--input-json` can carry them on `reports create`; +/// `reports merge` assembles them itself. fn validate_custom_dimensions(input: &serde_json::Map) -> Result<()> { let is_custom = input.get("compare_by").and_then(serde_json::Value::as_str) == Some("custom"); // An explicit null deserializes to None, so it counts as absent rather than as a value. - let has_custom_dimensions = input + let custom_dimensions = input .get("custom_dimensions") - .is_some_and(|value| !value.is_null()); + .filter(|value| !value.is_null()); + let custom_dimension_id = input + .get("custom_dimension_id") + .filter(|value| !value.is_null()); + + if !is_custom { + if custom_dimensions.is_some() { + anyhow::bail!("custom_dimensions can only be set when --compare-by is custom"); + } + if custom_dimension_id.is_some() { + anyhow::bail!("custom_dimension_id can only be set when --compare-by is custom"); + } + return Ok(()); + } - if is_custom && !has_custom_dimensions { + let Some(custom_dimensions) = custom_dimensions else { anyhow::bail!( "custom_dimensions is required when --compare-by is custom; use `coval reports merge` \ to build them from existing reports" ); - } - if !is_custom && has_custom_dimensions { - anyhow::bail!("custom_dimensions can only be set when --compare-by is custom"); + }; + validate_custom_dimension_id_target(custom_dimensions, custom_dimension_id) +} + +/// Check that a supplied custom_dimension_id names one of the supplied dimensions. +/// +/// The API defaults the grouping to the first dimension, so an absent ID is valid. Shapes +/// serde will reject anyway are passed through so the type error survives this check. +fn validate_custom_dimension_id_target( + custom_dimensions: &serde_json::Value, + custom_dimension_id: Option<&serde_json::Value>, +) -> Result<()> { + let (Some(dimension_id), Some(dimensions)) = ( + custom_dimension_id.and_then(serde_json::Value::as_str), + custom_dimensions.as_array(), + ) else { + return Ok(()); + }; + let names_a_dimension = dimensions.iter().any(|dimension| { + dimension.get("id").and_then(serde_json::Value::as_str) == Some(dimension_id) + }); + if !names_a_dimension { + anyhow::bail!( + "custom_dimension_id {dimension_id} does not match the id of any supplied custom dimension" + ); } Ok(()) } diff --git a/tests/cli_tests.rs b/tests/cli_tests.rs index fc032e2..47d7318 100644 --- a/tests/cli_tests.rs +++ b/tests/cli_tests.rs @@ -4141,6 +4141,268 @@ fn test_reports_create_allows_null_custom_dimensions_without_custom_compare_by() .stderr(predicate::str::contains("custom_dimensions can only be set").not()); } +/// Mount a merge source whose rows page out to `total` distinct simulations, 500 per page. +async fn mount_merge_source_with_simulations( + mock_server: &MockServer, + report_id: &str, + name: &str, + total: usize, +) { + Mock::given(method("GET")) + .and(path(format!("/v1/reports/{report_id}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "report": { + "id": report_id, + "name": name, + "run_ids": ["run1"], + "compare_by": "none", + "permissions": "PRIVATE" + } + }))) + .mount(mock_server) + .await; + + let mut emitted = 0usize; + while emitted < total { + let count = (total - emitted).min(500); + let rows: Vec = (emitted..emitted + count) + .map(|index| json!({"simulation_id": format!("{report_id}-sim{index}"), "run_id": "run1"})) + .collect(); + let mock = Mock::given(method("GET")) + .and(path(format!("/v1/reports/{report_id}/rows"))) + .and(query_param("limit", "500")); + let mock = if emitted == 0 { + mock.and(QueryParamAbsent("cursor")) + } else { + mock.and(query_param("cursor", emitted.to_string())) + }; + emitted += count; + let next_page_token = (emitted < total).then(|| emitted.to_string()); + mock.respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "rows": rows, + "next_page_token": next_page_token + }))) + .mount(mock_server) + .await; + } +} + +#[tokio::test] +async fn test_reports_merge_rejects_a_source_over_the_simulation_cap() { + let mock_server = MockServer::start().await; + + // 10,001 crosses the API's 10,000-per-group ceiling on the final page. No POST is + // mounted, so the asserted message is what distinguishes the guard from a stray 404. + mount_merge_source_with_simulations( + &mock_server, + "01HAAAAAAAAAAAAAAAAAAAAAAA", + "Baseline", + 10_001, + ) + .await; + mount_merge_source_with_simulations(&mock_server, "01HBBBBBBBBBBBBBBBBBBBBBBB", "Candidate", 1) + .await; + + coval() + .arg("--api-key") + .arg("test_key") + .arg("--api-url") + .arg(mock_server.uri()) + .arg("reports") + .arg("merge") + .arg("--name") + .arg("Too Big") + .arg("--report-ids") + .arg("01HAAAAAAAAAAAAAAAAAAAAAAA,01HBBBBBBBBBBBBBBBBBBBBBBB") + .assert() + .failure() + .stderr(predicate::str::contains( + "contributes more than 10000 simulations", + )); +} + +#[tokio::test] +async fn test_reports_merge_allows_a_source_at_the_simulation_cap() { + let mock_server = MockServer::start().await; + + mount_merge_source_with_simulations( + &mock_server, + "01HAAAAAAAAAAAAAAAAAAAAAAA", + "Baseline", + 10_000, + ) + .await; + mount_merge_source_with_simulations(&mock_server, "01HBBBBBBBBBBBBBBBBBBBBBBB", "Candidate", 1) + .await; + + Mock::given(method("POST")) + .and(path("/v1/reports")) + .respond_with(ResponseTemplate::new(201).set_body_json(json!({ + "report": { + "id": "01HMERGEDMERGEDMERGEDMERGE", + "name": "At The Cap", + "run_ids": ["run1"], + "compare_by": "custom", + "custom_dimension_id": "merged-reports", + "permissions": "PRIVATE" + } + }))) + .mount(&mock_server) + .await; + + coval() + .arg("--api-key") + .arg("test_key") + .arg("--api-url") + .arg(mock_server.uri()) + .arg("reports") + .arg("merge") + .arg("--name") + .arg("At The Cap") + .arg("--report-ids") + .arg("01HAAAAAAAAAAAAAAAAAAAAAAA,01HBBBBBBBBBBBBBBBBBBBBBBB") + .assert() + .success() + .stdout(predicate::str::contains("01HMERGEDMERGEDMERGEDMERGE")); +} + +#[test] +fn test_reports_merge_rejects_more_reports_than_the_group_cap() { + let report_ids: Vec = (0..501).map(|index| format!("01H{index:023}")).collect(); + + coval() + .arg("--api-key") + .arg("test_key") + .arg("reports") + .arg("merge") + .arg("--name") + .arg("Too Many") + .arg("--report-ids") + .arg(report_ids.join(",")) + .assert() + .failure() + .stderr(predicate::str::contains("at most 500 groups")); +} + +#[tokio::test] +async fn test_reports_create_custom_dimension_defaults_hide_unassigned() { + let mock_server = MockServer::start().await; + + // hide_unassigned is omitted from the input and custom_dimension_id names d1, so this + // covers both the serde default and the membership check's accepting path. + Mock::given(method("POST")) + .and(path("/v1/reports")) + .and(body_partial_json(json!({ + "compare_by": "custom", + "custom_dimension_id": "d1", + "custom_dimensions": [{ + "id": "d1", + "name": "Report", + "hide_unassigned": false, + "groups": [{"id": "g1", "name": "A", "simulation_ids": ["sim1"]}] + }] + }))) + .respond_with(ResponseTemplate::new(201).set_body_json(json!({ + "report": { + "id": "01HCUSTOMCUSTOMCUSTOMCUSTO", + "name": "Custom Report", + "run_ids": ["run1"], + "compare_by": "custom", + "custom_dimension_id": "d1", + "permissions": "PRIVATE" + } + }))) + .mount(&mock_server) + .await; + + coval() + .arg("--api-key") + .arg("test_key") + .arg("--api-url") + .arg(mock_server.uri()) + .arg("reports") + .arg("create") + .arg("--name") + .arg("Custom Report") + .arg("--run-ids") + .arg("run1") + .arg("--compare-by") + .arg("custom") + .arg("--input-json") + .arg( + r#"{"custom_dimension_id": "d1", "custom_dimensions": [{"id": "d1", "name": "Report", "groups": [{"id": "g1", "name": "A", "simulation_ids": ["sim1"]}]}]}"#, + ) + .assert() + .success() + .stdout(predicate::str::contains("01HCUSTOMCUSTOMCUSTOMCUSTO")); +} + +#[test] +fn test_reports_create_custom_dimension_id_rejected_without_custom_compare_by() { + coval() + .arg("--api-key") + .arg("test_key") + .arg("reports") + .arg("create") + .arg("--name") + .arg("Bad Report") + .arg("--run-ids") + .arg("run1") + .arg("--compare-by") + .arg("agent") + .arg("--input-json") + .arg(r#"{"custom_dimension_id": "d1"}"#) + .assert() + .failure() + .stderr(predicate::str::contains( + "custom_dimension_id can only be set when --compare-by is custom", + )); +} + +#[test] +fn test_reports_create_allows_null_custom_dimension_id_without_custom_compare_by() { + coval() + .arg("--api-key") + .arg("test_key") + .arg("reports") + .arg("create") + .arg("--name") + .arg("Report") + .arg("--run-ids") + .arg("run1") + .arg("--compare-by") + .arg("agent") + .arg("--input-json") + .arg(r#"{"custom_dimension_id": null}"#) + .assert() + .failure() + .stderr(predicate::str::contains("custom_dimension_id can only be set").not()); +} + +#[test] +fn test_reports_create_custom_dimension_id_must_name_a_supplied_dimension() { + coval() + .arg("--api-key") + .arg("test_key") + .arg("reports") + .arg("create") + .arg("--name") + .arg("Bad Report") + .arg("--run-ids") + .arg("run1") + .arg("--compare-by") + .arg("custom") + .arg("--input-json") + .arg( + r#"{"custom_dimension_id": "missing", "custom_dimensions": [{"id": "d1", "name": "Report", "groups": [], "hide_unassigned": false}]}"#, + ) + .assert() + .failure() + .stderr(predicate::str::contains( + "custom_dimension_id missing does not match the id of any supplied custom dimension", + )); +} + #[tokio::test] async fn test_traces_search_sends_structured_filters_and_preserves_cursor() { let mock_server = MockServer::start().await; From 5e13e01aeae971b89eadc26d2fe7ee6e5f9c85ca Mon Sep 17 00:00:00 2001 From: Callum Reid Date: Wed, 5 Aug 2026 14:49:18 -0700 Subject: [PATCH 5/8] Document the merge source-report ceiling in --report-ids help --- src/commands/reports.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commands/reports.rs b/src/commands/reports.rs index 33ad05a..b666d8c 100644 --- a/src/commands/reports.rs +++ b/src/commands/reports.rs @@ -112,7 +112,7 @@ pub struct CreateArgs { #[derive(Args)] pub struct MergeArgs { - /// Comma-separated IDs of the reports to merge (min 2, must be distinct) + /// Comma-separated IDs of the reports to merge (2-500, must be distinct) #[arg(long, required = true, value_delimiter = ',')] report_ids: Vec, /// Display name for the merged report (1-200 characters) From dfad8776f1f2a9098dc82cc5bd42007ca0f8105d Mon Sep 17 00:00:00 2001 From: Callum Reid Date: Wed, 5 Aug 2026 14:50:15 -0700 Subject: [PATCH 6/8] Note the merge caps and custom-dimension pairing in agent pitfalls --- src/agent_discovery.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/agent_discovery.rs b/src/agent_discovery.rs index 0990151..fc997ea 100644 --- a/src/agent_discovery.rs +++ b/src/agent_discovery.rs @@ -605,7 +605,9 @@ const RESOURCE_SPECS: &[ResourceSpec] = &[ pitfalls: &[ "metadata_key is required when compare-by is metadata and rejected otherwise.", "merge needs at least two distinct report IDs and reads every source report's rows.", + "merge takes at most 500 source reports, each contributing at most 10,000 simulations.", "A simulation in several merged reports lands in the first one's group only.", + "custom_dimensions is required when compare-by is custom and rejected otherwise; custom_dimension_id must name one of them.", "PUBLIC reports also mark their runs public.", ], }, From 2fa773968bcd05997156ae1eaef571bd04e63def Mon Sep 17 00:00:00 2001 From: Callum Reid Date: Wed, 5 Aug 2026 14:52:52 -0700 Subject: [PATCH 7/8] Bound the merged report's run count client-side --- src/agent_discovery.rs | 2 +- src/commands/reports.rs | 11 +++++++-- tests/cli_tests.rs | 49 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/src/agent_discovery.rs b/src/agent_discovery.rs index fc997ea..04e5ee1 100644 --- a/src/agent_discovery.rs +++ b/src/agent_discovery.rs @@ -605,7 +605,7 @@ const RESOURCE_SPECS: &[ResourceSpec] = &[ pitfalls: &[ "metadata_key is required when compare-by is metadata and rejected otherwise.", "merge needs at least two distinct report IDs and reads every source report's rows.", - "merge takes at most 500 source reports, each contributing at most 10,000 simulations.", + "merge takes at most 500 source reports, 2,000 runs in total, and 10,000 simulations per source.", "A simulation in several merged reports lands in the first one's group only.", "custom_dimensions is required when compare-by is custom and rejected otherwise; custom_dimension_id must name one of them.", "PUBLIC reports also mark their runs public.", diff --git a/src/commands/reports.rs b/src/commands/reports.rs index b666d8c..a0a1d80 100644 --- a/src/commands/reports.rs +++ b/src/commands/reports.rs @@ -20,10 +20,11 @@ const MERGE_DIMENSION_ID: &str = "merged-reports"; const MERGE_DIMENSION_NAME: &str = "Report"; const MERGE_ROWS_PAGE_SIZE: u32 = 500; const MERGE_MAX_PAGES_PER_REPORT: usize = 200; -/// Mirrors the API's per-group `simulation_ids` and per-dimension `groups` ceilings. -/// Checked client-side so an oversized merge fails before paging the whole source. +/// Mirror the create request's `simulation_ids`, `groups` and `run_ids` ceilings. +/// Checked client-side so an oversized merge fails before paging every source. const MERGE_MAX_SIMULATIONS_PER_GROUP: usize = 10_000; const MERGE_MAX_SOURCE_REPORTS: usize = 500; +const MERGE_MAX_RUNS: usize = 2_000; #[derive(Subcommand)] pub enum ReportCommands { @@ -307,6 +308,12 @@ async fn merge_reports( run_ids.push(run_id.clone()); } } + if run_ids.len() > MERGE_MAX_RUNS { + anyhow::bail!( + "the selected reports span more than {MERGE_MAX_RUNS} runs; a merged report \ + cannot hold more than that" + ); + } let mut simulation_ids: Vec = Vec::new(); let mut cursor: Option = None; diff --git a/tests/cli_tests.rs b/tests/cli_tests.rs index 47d7318..2898c5e 100644 --- a/tests/cli_tests.rs +++ b/tests/cli_tests.rs @@ -4266,6 +4266,55 @@ async fn test_reports_merge_allows_a_source_at_the_simulation_cap() { .stdout(predicate::str::contains("01HMERGEDMERGEDMERGEDMERGE")); } +#[tokio::test] +async fn test_reports_merge_rejects_sources_over_the_run_cap() { + let mock_server = MockServer::start().await; + + // 2,001 distinct runs across two sources crosses the create request's run_ids ceiling. + for (report_id, name, range) in [ + ("01HAAAAAAAAAAAAAAAAAAAAAAA", "Baseline", 0..2_000), + ("01HBBBBBBBBBBBBBBBBBBBBBBB", "Candidate", 2_000..2_001), + ] { + let run_ids: Vec = range.map(|index| json!(format!("run{index}"))).collect(); + Mock::given(method("GET")) + .and(path(format!("/v1/reports/{report_id}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "report": { + "id": report_id, + "name": name, + "run_ids": run_ids, + "compare_by": "none", + "permissions": "PRIVATE" + } + }))) + .mount(&mock_server) + .await; + Mock::given(method("GET")) + .and(path(format!("/v1/reports/{report_id}/rows"))) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "rows": [{"simulation_id": format!("{report_id}-sim"), "run_id": "run0"}], + "next_page_token": null + }))) + .mount(&mock_server) + .await; + } + + coval() + .arg("--api-key") + .arg("test_key") + .arg("--api-url") + .arg(mock_server.uri()) + .arg("reports") + .arg("merge") + .arg("--name") + .arg("Too Many Runs") + .arg("--report-ids") + .arg("01HAAAAAAAAAAAAAAAAAAAAAAA,01HBBBBBBBBBBBBBBBBBBBBBBB") + .assert() + .failure() + .stderr(predicate::str::contains("span more than 2000 runs")); +} + #[test] fn test_reports_merge_rejects_more_reports_than_the_group_cap() { let report_ids: Vec = (0..501).map(|index| format!("01H{index:023}")).collect(); From fbc7d9121a337bf0e11657f20a02a43931171f4c Mon Sep 17 00:00:00 2001 From: Callum Reid Date: Wed, 5 Aug 2026 14:54:38 -0700 Subject: [PATCH 8/8] Defer to serde when a custom dimension has no readable id --- src/commands/reports.rs | 13 +++++++++---- tests/cli_tests.rs | 27 +++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/src/commands/reports.rs b/src/commands/reports.rs index a0a1d80..29536a0 100644 --- a/src/commands/reports.rs +++ b/src/commands/reports.rs @@ -442,10 +442,15 @@ fn validate_custom_dimension_id_target( ) else { return Ok(()); }; - let names_a_dimension = dimensions.iter().any(|dimension| { - dimension.get("id").and_then(serde_json::Value::as_str) == Some(dimension_id) - }); - if !names_a_dimension { + let mut dimension_ids = Vec::with_capacity(dimensions.len()); + for dimension in dimensions { + // A dimension with no readable id is serde's error to report, not this one's. + let Some(id) = dimension.get("id").and_then(serde_json::Value::as_str) else { + return Ok(()); + }; + dimension_ids.push(id); + } + if !dimension_ids.contains(&dimension_id) { anyhow::bail!( "custom_dimension_id {dimension_id} does not match the id of any supplied custom dimension" ); diff --git a/tests/cli_tests.rs b/tests/cli_tests.rs index 2898c5e..3188923 100644 --- a/tests/cli_tests.rs +++ b/tests/cli_tests.rs @@ -4428,6 +4428,33 @@ fn test_reports_create_allows_null_custom_dimension_id_without_custom_compare_by .stderr(predicate::str::contains("custom_dimension_id can only be set").not()); } +#[test] +fn test_reports_create_reports_a_malformed_dimension_as_a_type_error() { + // The dimension has no id, so the membership check defers and serde names the real fault + // rather than blaming custom_dimension_id. + coval() + .arg("--api-key") + .arg("test_key") + .arg("reports") + .arg("create") + .arg("--name") + .arg("Bad Report") + .arg("--run-ids") + .arg("run1") + .arg("--compare-by") + .arg("custom") + .arg("--input-json") + .arg( + r#"{"custom_dimension_id": "d1", "custom_dimensions": [{"name": "Report", "groups": []}]}"#, + ) + .assert() + .failure() + .stderr( + predicate::str::contains("missing field `id`") + .and(predicate::str::contains("does not match the id").not()), + ); +} + #[test] fn test_reports_create_custom_dimension_id_must_name_a_supplied_dimension() { coval()