Add output mode for human-readable and JSON results#2635
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a shared Output abstraction in soroban-cli to standardize human-readable vs JSON output across commands, and updates network info, network health, and network settings to use it. It also adds top-level JSON error rendering (to stdout) when --output json|json-formatted is requested, so machine consumers can reliably parse failures.
Changes:
- Added
cmd/soroban-cli/src/output.rswithFormat,Output, anderror_json()for consistent JSON output and structured error envelopes. - Migrated
network info,network health, andnetwork settingsto the new output routing (readable output suppressed in JSON mode; final JSON emitted viaOutput::json_value). - Updated CLI top-level error handling to emit JSON error envelopes on stdout when
--output json*is detected from raw args.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/soroban-cli/src/output.rs | New shared output abstraction (Format/Output) plus structured JSON error envelope helper and unit tests. |
| cmd/soroban-cli/src/lib.rs | Exposes the new output module. |
| cmd/soroban-cli/src/commands/network/settings.rs | Routes warnings via Output::print() and JSON output via Output::json_value while keeping XDR output special-cased. |
| cmd/soroban-cli/src/commands/network/info.rs | Replaces per-command JSON printing with Output gating + unified JSON serialization. |
| cmd/soroban-cli/src/commands/network/health.rs | Reworks text/JSON output through Output, including JSON error envelope emission on failure. |
| cmd/soroban-cli/src/cli.rs | Adds raw-arg sniffing for --output json* so top-level errors can be printed as structured JSON on stdout. |
| cmd/soroban-cli/Cargo.toml | Adds jsonrpsee-types dependency to enable downcasting to ErrorObjectOwned. |
| Cargo.lock | Locks in jsonrpsee-types dependency. |
|
Addressed feedback from a follow-up review: Structured-error branch is now tested (was only the fallback before). Added Top-level error hook applies to all
|
d4f585a to
52f3962
Compare
|
The output_format_from_raw_args() helper in cli.rs scans raw argv for --output json / --output=json / json-formatted on every error, independent of which subcommand actually ran. As the inline comment notes, --output is per-command, so the parsed value isn't available this far up — but sniffing argv means the new JSON error rendering fires for all commands, while only network health/info/settings were converted to the Output system. Three consequences:
The underlying issue is that error-format resolution is driven by raw argv rather than by the parsed command that actually knows its own --output value and default. Resolving the effective format from the command (and only applying JSON error rendering to commands migrated to Output) would fix all three at once. Several of these commands already produce JSON success output with a bare error: line on stderr for failures. Changing the failure representation (stdout JSON envelope, non-zero exit) for commands that weren't part of this migration is a breaking change to the API. Should API breaking changes be targeted for a major version bump or feature branch? It seems like the current approach will incrementally change individual API responses over time which could make it hard for consumers to migrate from the old API to the new one cleanly. |
What
Introduces an
Outputabstraction that lets commands emit either human-readable or JSON output via--output text|json|json-formatted, and applies it tonetwork info,network health, andnetwork settings. When JSON output is requested, errors are rendered as a structured{"error": {...}}envelope on stdout (with the JSON-RPCcode/message/datawhen available) instead of the human❌ error:line.Why
We're working toward every command supporting both JSON and human-readable output. Until now each command hand-rolled its own format handling, and progress messages were only gated by
--quiet. This adds a shared mechanism: human-readable progress is suppressed in JSON mode while the final result streams to stdout without buffering, so slow operations don't hold the terminal silently. It also ensures machine consumers get parseable errors rather than a human-formatted line.Known limitations
The remaining
networksubcommands (ls,add,rm,use,unset,root-account) are not yet migrated and are planned as a follow-up — they don't have JSON support today anyway, so this is no regression.--outputis also still defined per-command; the end goal is to promote it to a global option.The top-level JSON error envelope is emitted for any command invoked with
--output json/--output json-formatted(the handler sniffs the raw args, since--outputisn't global yet), not just the three migrated commands. This is intentional and desirable, though only the migrated commands are exercised by tests so far.Because that raw-arg sniffer can't see per-command defaults, a command whose
--outputdefaults to JSON (currentlynetwork settings) will still print the human error line on a no-flag failure; pass--output jsonexplicitly to get a parseable error. This goes away once--outputbecomes a global option.