Add reports merge and reports rows commands - #102
Conversation
|
Warning Review limit reached
Next review available in: 52 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe 🚥 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 SummaryThe PR adds first-class report-row retrieval and report merging, extends report creation with custom dimensions and view modes, and updates command discovery and API coverage.
Confidence Score: 4/5The null custom-dimensions case should be rejected locally before merge because it currently sends a request that violates the command’s custom-comparison contract. The new presence-only validator accepts Files Needing Attention: src/commands/reports.rs Reviews (1): Last reviewed commit: "Add reports merge and reports rows comma..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/commands/reports.rs (2)
274-345: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDuplicate detection uses ids before the count check; both are correct here.
Paging control is sound. The loop bails when the final allowed page still returns a token, so a truncated merge cannot silently produce partial groups.
One note: the source reports are fetched and paged strictly in sequence. For many source reports this makes merge slow. This is acceptable for a CLI, so no change is required now.
🤖 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/reports.rs` around lines 274 - 345, No change is required: merge_reports correctly validates distinct IDs, enforces the minimum count, and prevents truncated pagination; sequential report fetching is acceptable for this CLI.
21-22: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider a larger merge page size.
RowsArgsdocuments the API limit as 1-2000.MERGE_ROWS_PAGE_SIZEis 500, so merge issues four times more requests than needed and reaches the page cap at 100,000 rows. A page size of 2000 reduces request count and raises the effective ceiling with the same cap.The merge tests assert
query_param("limit", "500"), so updatemount_merge_sourceintests/cli_tests.rsif you change this value.🤖 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/reports.rs` around lines 21 - 22, Increase MERGE_ROWS_PAGE_SIZE from 500 to the documented API maximum of 2000 so merge requests fetch larger batches. Update mount_merge_source in tests/cli_tests.rs and its merge test expectations from limit=500 to limit=2000, preserving the existing page-cap behavior.src/client/models/report.rs (1)
69-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a serde default for
hide_unassigned.
hide_unassignedis a requiredboolon deserialize. Users can supplycustom_dimensionsthrough--input-jsononreports create, and that JSON is deserialized intoCreateReportRequest. If the user omitshide_unassigned, deserialization fails instead of using the intended default.groupshas the same shape concern.♻️ Proposed change
pub struct ReportCustomDimension { pub id: String, pub name: String, + #[serde(default)] pub groups: Vec<ReportCustomDimensionGroup>, + #[serde(default)] pub hide_unassigned: bool, }🤖 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/client/models/report.rs` around lines 69 - 74, Update ReportCustomDimension deserialization so omitted hide_unassigned and groups fields receive their intended defaults via serde, while preserving explicitly provided values. Use the existing default conventions in the surrounding report models rather than introducing unrelated changes.tests/cli_tests.rs (1)
3897-4030: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGood coverage of first-seen attribution and paging.
Two behaviors from
merge_reportsremain untested: the page-cap bail when a report never drains, and the bail when the source reports contribute no runs. Both produce user-facing errors, so a test for each would lock in the messages.🤖 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 3897 - 4030, Add tests alongside test_reports_merge_builds_one_group_per_source_report covering merge_reports aborting when a source report exceeds the page limit without draining and when all source reports contribute no runs. Mock the required report/row requests, assert both commands fail, and verify each user-facing error contains the expected page-cap or no-runs message.
🤖 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.
Nitpick comments:
In `@src/client/models/report.rs`:
- Around line 69-74: Update ReportCustomDimension deserialization so omitted
hide_unassigned and groups fields receive their intended defaults via serde,
while preserving explicitly provided values. Use the existing default
conventions in the surrounding report models rather than introducing unrelated
changes.
In `@src/commands/reports.rs`:
- Around line 274-345: No change is required: merge_reports correctly validates
distinct IDs, enforces the minimum count, and prevents truncated pagination;
sequential report fetching is acceptable for this CLI.
- Around line 21-22: Increase MERGE_ROWS_PAGE_SIZE from 500 to the documented
API maximum of 2000 so merge requests fetch larger batches. Update
mount_merge_source in tests/cli_tests.rs and its merge test expectations from
limit=500 to limit=2000, preserving the existing page-cap behavior.
In `@tests/cli_tests.rs`:
- Around line 3897-4030: Add tests alongside
test_reports_merge_builds_one_group_per_source_report covering merge_reports
aborting when a source report exceeds the page limit without draining and when
all source reports contribute no runs. Mock the required report/row requests,
assert both commands fail, and verify each user-facing error contains the
expected page-cap or no-runs message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f9a0dda-6586-4969-8bdf-b5d0cf40ab39
📒 Files selected for processing (7)
README.mdapi-coverage.tomlsrc/agent_discovery.rssrc/client/mod.rssrc/client/models/report.rssrc/commands/reports.rstests/cli_tests.rs
`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.
f55c51e to
ac52039
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/cli_tests.rs (1)
4124-4142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo negative validation tests depend on an outbound request failing. Each test supplies a payload that validation accepts, then asserts
.failure()with a.not()predicate. Neither command sets--api-url, so the CLI reaches the default base URL and the exit status depends on the test environment rather than on CLI logic.
tests/cli_tests.rs#L4124-L4142: point the command at aMockServerthat answersPOST /v1/reports, then assert the absence of "custom_dimensions can only be set" against that deterministic run.tests/cli_tests.rs#L4362-L4380: apply the sameMockServerchange, then assert the absence of "custom_dimension_id can only be set".🤖 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 4124 - 4142, The negative validation tests at tests/cli_tests.rs:4124-4142 and tests/cli_tests.rs:4362-4380 currently depend on the default network endpoint. Update both CLI test commands to use a MockServer configured to answer POST /v1/reports, then retain the failure assertions while checking absence of “custom_dimensions can only be set” at 4124-4142 and “custom_dimension_id can only be set” at 4362-4380.
🤖 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.
Nitpick comments:
In `@tests/cli_tests.rs`:
- Around line 4124-4142: The negative validation tests at
tests/cli_tests.rs:4124-4142 and tests/cli_tests.rs:4362-4380 currently depend
on the default network endpoint. Update both CLI test commands to use a
MockServer configured to answer POST /v1/reports, then retain the failure
assertions while checking absence of “custom_dimensions can only be set” at
4124-4142 and “custom_dimension_id can only be set” at 4362-4380.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 29036c91-4bad-4447-9c45-c17e1dc3dc2f
📒 Files selected for processing (8)
README.mdapi-coverage-report.mdapi-coverage.tomlsrc/agent_discovery.rssrc/client/mod.rssrc/client/models/report.rssrc/commands/reports.rstests/cli_tests.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- README.md
- src/client/mod.rs
- api-coverage.toml
- src/agent_discovery.rs
- src/client/models/report.rs
5 participants
Problem
The app can merge several reports into one grouped comparison (
frontend/src/modules/reports/routes/useMergeReports.ts) — one group per source report, in a single shareable report. There was no way to do it from the CLI or the public API.Change
coval reports mergeSame algorithm as the app's action:
GET /v1/reports/{id}per source, for its display name and run IDs.GET /v1/reports/{id}/rowsper source, collecting simulation IDs.POST /v1/reportswith the union of run IDs (order-preserving),compare_by: custom,view_mode: grouped, and one dimension holding a group per source report.Guards: at least two report IDs, IDs must be distinct, and the merge fails loudly rather than silently truncating if a source exceeds the page ceiling (500 rows × 200 pages). If every source report is run-less the command explains that instead of letting the API reject an empty
run_ids.coval reports rowsGET /reports/{report_id}/rowsis the endpointmergepages through, and it was a recorded coverage gap. Exposing it makesmergea composition of first-class commands rather than a command reaching past the published surface. Human output printsNext cursor: …after the table;--format jsonand--agentemit the whole response includingnext_page_token, matchingtraces search.reports create --view-moderows|grouped, matching the field the backend PR adds.--input-jsoncan also carrycustom_dimensions/custom_dimension_idnow that the request model has them, with the same compare-by pairing guard the existingmetadata_keycheck uses.Coverage manifest
api-coverage.tomldrops theGET /reports/{report_id}/rowsknown-gap entry (it now reaches a command) and bumpscli_supported_operations124 → 125.Two things I deliberately did not touch:
api-coverage-report.mdis unchanged. It is generated from the live catalog, and regenerating it here would pull unrelated drift into this PR — see below.main. I ranscripts/audit_api_coverage.pyagainst a cleanorigin/mainworktree:FAIL: 117/174, because the published catalog has renamedmonitors-v1→alerts-v1while the CLI still shipscoval monitors. That surfaces as 7new_gaps(/alerts/*) and 7unexpected_cli_operations(/monitors/*). With this PR the same run reports118/174with those same 14 entries and nostale_gaps— i.e. this PR's only effect on the audit is the intended +1. Themonitors→alertsrename is pre-existing drift for the weeklychore/weekly-api-parityPR, not for this one. Note that the audit step iscontinue-on-erroron ordinary PR runs.Tests
Six new integration tests in
tests/cli_tests.rs, all wiremock-backed:mergebuilds one group per source, pages a multi-page source vianext_page_token, unions run IDs in order, and drops a simulation shared between two reports from the second group (first-seen attribution).mergerejects a single report ID and rejects duplicate IDs.rowsforwards cursor, limit, metric IDs and simulation IDs as query parameters.createrejects--compare-by customwithout dimensions, and dimensions without--compare-by custom.Added a
QueryParamAbsentmatcher so the first page can assert nocursoris sent.Observed locally (full CI command set):
cargo test --all-targets --all-features— 114 passed, 0 failed (3 + 114 across both targets)cargo clippy --all-targets --all-features -- -D warnings— cleancargo fmt --check— cleanpython -m unittest discover --start-directory scripts --pattern 'test_*.py'— 31 passedruff check scripts/ruff format --check scripts— clean (with the pinnedruff==0.15.9fromscripts/requirements-audit.txt; a newer ruff reports pre-existing findings on untouched files)python scripts/release_version.py—v0.6.0Out of scope
reports updatestill exposes only the legacy top-level--compare-by/--metadata-keyflags; the backend's typedview_configpatch (which also gained the custom-dimension fields, and needsIf-Match) has no CLI surface yet. Regrouping an existing report by a custom dimension therefore still needs a raw API call. Worth a follow-up.