Skip to content

Add output mode for human-readable and JSON results#2635

Open
fnando wants to merge 3 commits into
mainfrom
output-system-multiformat
Open

Add output mode for human-readable and JSON results#2635
fnando wants to merge 3 commits into
mainfrom
output-system-multiformat

Conversation

@fnando

@fnando fnando commented Jun 30, 2026

Copy link
Copy Markdown
Member

What

Introduces an Output abstraction that lets commands emit either human-readable or JSON output via --output text|json|json-formatted, and applies it to network info, network health, and network settings. When JSON output is requested, errors are rendered as a structured {"error": {...}} envelope on stdout (with the JSON-RPC code/message/data when 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 network subcommands (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. --output is 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 --output isn'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 --output defaults to JSON (currently network settings) will still print the human error line on a no-flag failure; pass --output json explicitly to get a parseable error. This goes away once --output becomes a global option.

Copilot AI review requested due to automatic review settings June 30, 2026 19:36
@github-project-automation github-project-automation Bot moved this to Backlog (Not Ready) in DevX Jun 30, 2026
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.rs with Format, Output, and error_json() for consistent JSON output and structured error envelopes.
  • Migrated network info, network health, and network settings to the new output routing (readable output suppressed in JSON mode; final JSON emitted via Output::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.

Comment thread cmd/soroban-cli/src/commands/network/health.rs Outdated
Comment thread cmd/soroban-cli/src/cli.rs
Comment thread cmd/soroban-cli/src/output.rs Outdated
@fnando

fnando commented Jun 30, 2026

Copy link
Copy Markdown
Member Author

Addressed feedback from a follow-up review:

Structured-error branch is now tested (was only the fallback before). Added json_error_envelope_includes_structured_rpc_code (rpc_provider.rs, d4f585a9): it mocks an RPC that returns a JSON-RPC error body and asserts error.code + error.message on stdout. This travels the real ClientError::Call(ErrorObjectOwned) downcast end-to-end, so it guards the jsonrpsee-types = "0.26" pin — if jsonrpsee drifted to two versions in the tree the downcast would silently fall back to { message } and the test would fail. The earlier connection-refused test only exercised Transport(..)/the fallback, as noted.

Top-level error hook applies to all --output json* commands. Intentional and desirable; documented in the PR description. Only the migrated commands are covered by tests for now.

network settings default mismatch. Minor: since the raw-arg sniffer can't see per-command defaults, a no-flag failure of settings (which defaults to JSON) still prints the human error line; pass --output json explicitly for a parseable error. Documented as a known limitation — resolved once --output becomes a global option.

@fnando fnando moved this from Backlog (Not Ready) to Needs Review in DevX Jun 30, 2026
@fnando fnando self-assigned this Jun 30, 2026
@fnando fnando force-pushed the output-system-multiformat branch from d4f585a to 52f3962 Compare June 30, 2026 20:55
@aristidesstaffieri

Copy link
Copy Markdown
Collaborator

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:

  1. Unconverted commands silently change failure behavior. ~15 commands define their own --output json (e.g. events, fee-stats, ledger fetch, tx decode/encode/fetch). Running any of them with --output json against a failing RPC now emits {"error":{...}} on stdout instead of the previous error: on stderr. Scripts that distinguish these errors via stderr — or that assumed no stdout on failure — break, even though these commands weren't touched here.
  2. events --output json splices an error object into the NDJSON stream. events streams one JSON object per line to stdout. A mid-stream failure now appends {"error":{...}} to that same stdout stream, after already-emitted event lines — breaking per-line NDJSON consumers.
  3. network settings fails to emit JSON in its own default mode. settings defaults --output to json, but because --output isn't literally present in argv, output_format_from_raw_args() returns None and the failure renders as a human error: line on stderr — an unparseable failure from a command whose default output is machine-readable, which is the opposite of what this PR intends.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

3 participants