Skip to content

probing: add measurements command and -o csv output#14

Merged
matthieugouel merged 3 commits into
mainfrom
feat/probing-measurements-and-csv
Jun 19, 2026
Merged

probing: add measurements command and -o csv output#14
matthieugouel merged 3 commits into
mainfrom
feat/probing-measurements-and-csv

Conversation

@matthieugouel

Copy link
Copy Markdown
Member

Implements the two Probing ergonomics items from the v2 CLI roadmap (nxthdr/roadmap#38).

probing measurements [--limit N]

Lists your recent measurements as a table (id · started · agents (done/total) · probes (sent/expected) · status), so the measurement ID returned by send isn't the only handle. --limit defaults to 20.

Depends on nxthdr/saimiris-gateway#42 (GET /api/user/measurements). Until that gateway is deployed, this command returns a normal 404 API error.

-o csv

Adds csv as a third value to the global -o/--output flag (was text|json):

  • table() → header row + data rows; kv() → keys header + one data row; both RFC-4180 escaped (quote/comma/newline handled).
  • Replaced the old is_json() / literal-[] empty-result pattern with a format-aware output::empty(headers) helper + is_text() gating, so every command (not just results) stays valid under text/json/csv. Migrated all call sites in peering.rs/probing.rs.

Testing

  • cargo build, cargo clippy --all-targets clean; cargo test passes (added CSV-escaping unit tests).
  • Verified the CSV kv path offline via status -o csv (ClickHouse/gateway aren't reachable from the dev sandbox, so results/measurements weren't exercised end-to-end):
    auth,token
    logged in,valid 11h 27m
    
  • Updated CLAUDE.md to reflect the three formats and the output::empty() convention.

🤖 Generated with Claude Code

Implements the two 'Probing ergonomics' items from the v2 CLI roadmap (nxthdr/roadmap#38).

- `probing measurements [--limit N]`: lists recent measurements via the new GET /api/user/measurements endpoint, so the ID from `send` isn't the only handle.
- `-o csv`: third global output format alongside text/json. table() renders header+rows and kv() renders a single-row CSV, both RFC-4180 escaped. Empty-result and text-only decoration handling routed through a new output::empty()/is_text() so every command stays valid in all three formats.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@matthieugouel

matthieugouel commented Jun 18, 2026

Copy link
Copy Markdown
Member Author

Claude encountered an error after 2m 2s —— View job


Code Review

  • Gather context
  • Read changed files
  • Analyze diff
  • Post findings

Overall the PR is clean and well-structured. Two issues worth addressing before merge:


1. --limit not validated despite claiming a 1-100 range

main.rs:76 documents "Maximum number of measurements to list (1-100)" but no validation is performed. Any u32 (including 0 or 4294967295) is passed straight to the API:

// probing.rs:283
.get(&format!("/api/user/measurements?limit={limit}"))

--limit 0 will return an empty result or a confusing API error instead of a clear message. Per CLAUDE.md conventions, anyhow::ensure! is the right fix:

pub async fn measurements(limit: u32) -> anyhow::Result<()> {
    anyhow::ensure!((1..=100).contains(&limit), "limit must be between 1 and 100");
    // …

Fix this →


2. measurement-status -o csv produces two incompatible CSV blocks on stdout

probing::measurement_status() (not modified in this PR, but CSV support is new and global) calls both output::kv() and output::table() in sequence. Under -o csv, each call independently emits its own header row:

id,status,agents,probes
abc123,complete,2/2,1000/1000
agent,probes sent/expected,complete      ← second header row: invalid for CSV parsers
vltcdg01,1000/1000,yes

This is a newly reachable broken state introduced by adding the global csv format. Most CSV tools (pandas.read_csv, cut, mlr) expect exactly one header. Consider either:

  • Flatten: merge the kv pairs into the table as extra columns (same output schema as the measurements list table), or
  • Section-prefix: emit ## section comments above each block (not strict RFC 4180, but acceptable for human-pipeline usage), or
  • Guard: document that measurement-status -o csv is unsupported and bail early when !is_text() with a clear error.

The simplest fix consistent with the existing pattern is to guard measurement_status against CSV with anyhow::ensure!(output::is_text() || output::fmt() == OutputFormat::Json, ...), but the flattened-table approach would give the most useful machine output.

Fix this →


Everything else looks good: the output::empty() migration is correct at all call sites, the RFC 4180 escaping is correct (commas, quotes, CRLF all handled), tests cover the edge cases, and the is_text() rename correctly gates decoration in both JSON and CSV modes.

- validate `--limit` is within the documented 1-100 range (anyhow::ensure), failing fast instead of relying on the gateway to clamp.
- gate the `measurement-status` summary kv to text mode so json/csv emit only the per-agent table — previously `-o csv`/`-o json` printed a summary block followed by a table (two CSV headers / two JSON values), which is invalid for parsers. Mirrors the existing `peering lookup` pattern.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Tracks the gateway rename (saimiris-gateway: /api/user/measurements -> /api/measurements) so the measurements list and measurement-status routes are consistent.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@matthieugouel matthieugouel merged commit 44a839f into main Jun 19, 2026
10 checks passed
@matthieugouel matthieugouel deleted the feat/probing-measurements-and-csv branch June 19, 2026 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant