Skip to content

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

Merged
fnando merged 5 commits into
mainfrom
output-system-multiformat
Jul 8, 2026
Merged

Add output mode for human-readable and JSON results#2635
fnando merged 5 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 Outdated
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.

@fnando

fnando commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

Thanks for the thorough breakdown — all three consequences were spot-on, and I've reworked the approach in 32652e7.

The root cause you identified (resolving the error format from raw argv instead of the parsed command) is fixed. The top-level handler now reads the format from the parsed command and only applies the JSON error envelope to commands actually migrated to the Output system (network health/info/settings). Concretely:

  1. Unconverted commands (events, fee-stats, ledger fetch, tx decode/encode/fetch, …) fall through to None and keep their previous behavior — human error: line on stderr, no stdout envelope. No silent contract change.
  2. events NDJSON is untouched — it's not in the migrated set, so nothing gets spliced into its stdout stream.
  3. network settings now reads its own parsed --output value, so a no-flag failure correctly honors its json default and emits a parseable envelope. The argv sniffer couldn't see per-command defaults; reading the parsed command does.

Added integration coverage for both directions: a non-migrated command (ledger latest --output json) keeps the human error line, and network settings with no flag renders the JSON envelope.

On the versioning question: I don't think we need a major bump or feature branch for this. The definitive error signal is the non-zero exit code, which is unchanged for every command. Parsing human-readable error text is brittle by definition — it's not a contract we intend to support or that consumers should be relying on. So the only thing evolving here is the representation of an already-signaled failure, and now that it's scoped to migrated commands, existing consumers of untouched commands see no change at all.

@fnando fnando enabled auto-merge (squash) July 8, 2026 16:02
@fnando fnando merged commit 7952188 into main Jul 8, 2026
226 of 227 checks passed
@fnando fnando deleted the output-system-multiformat branch July 8, 2026 16:15
@github-project-automation github-project-automation Bot moved this from Needs Review to Done in DevX Jul 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants