Skip to content

feat(cli): add --output json/yaml to sandbox get, status, and sandbox create#1989

Open
rhuss wants to merge 1 commit into
NVIDIA:mainfrom
rhuss:feat/cli-structured-output
Open

feat(cli): add --output json/yaml to sandbox get, status, and sandbox create#1989
rhuss wants to merge 1 commit into
NVIDIA:mainfrom
rhuss:feat/cli-structured-output

Conversation

@rhuss

@rhuss rhuss commented Jun 24, 2026

Copy link
Copy Markdown

Summary

Add --output json/yaml to sandbox get, status, and sandbox create so automation and MCP tool wrappers can consume structured data instead of parsing ANSI-colored human output.

Related Issue

Closes #1964

Changes

  • sandbox get --output json/yaml: New sandbox_detail_to_json converter enriches the existing sandbox_to_json with policy source, revision, and active policy content from GetSandboxConfigResponse. Uses sandbox_policy_to_json_value for direct proto-to-JSON conversion.
  • status --output json/yaml: New status_to_json converter emits gateway, server, status, and conditional fields (auth, version, error, http_status). Refactored gateway_status to build data before output routing.
  • sandbox create --output json/yaml: Emits sandbox metadata after Ready phase. Human chrome (header, spinner) suppressed from stdout; progress message on stderr. Uploads and port forwarding still execute before output. Clap rejects --output combined with --editor or trailing commands.
  • No-gateway path: status --output json with no configured gateway emits {"status": "not_configured"} instead of human text.
  • All three commands reuse the existing OutputFormat enum and print_output_single helper.
  • Default output (no --output flag) is byte-identical to current behavior (verified by diffing raw ANSI output).
  • Docs updated for sandbox get, sandbox create, and status in manage-sandboxes.mdx and manage-gateways.mdx.

Testing

  • cargo clippy clean, cargo fmt clean
  • 188 unit tests pass (cargo test --lib)
  • 5 new unit tests for sandbox_detail_to_json and status_to_json converters
  • Integration test call sites updated for new function signatures
  • Smoke-tested against live local gateway: sandbox get --output json, status --output json/yaml, default output byte-identical regression check
  • Deep review (5 agents + CodeRabbit + Copilot): all Critical/Important findings fixed

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

@rhuss rhuss requested review from a team, derekwaynecarr, maxamillion and mrunalp as code owners June 24, 2026 16:32
@copy-pr-bot

copy-pr-bot Bot commented Jun 24, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown

All contributors have signed the DCO ✍️ ✅
Posted by the DCO Assistant Lite bot.

@rhuss

rhuss commented Jun 24, 2026

Copy link
Copy Markdown
Author

I have read the DCO document and I hereby sign the DCO.

@rhuss

rhuss commented Jun 24, 2026

Copy link
Copy Markdown
Author

recheck

@johntmyers johntmyers self-assigned this Jun 24, 2026
Comment thread crates/openshell-cli/src/run.rs Outdated
println!(" {} {}", "HTTP:".dimmed(), hs);
}
if let Some(ref e) = error {
println!(" {} {}", "gRPC error:".dimmed(), e);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This isn't byte-for-byte identical I don't think. If http_health_check() returns None then we used to show Error: ... but now it should hit this arm and will show gRPC error:

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good catch. When http_health_check() returns None (no HTTP reachability at all), the original code used "Error:" as the label, not "gRPC error:". My refactoring incorrectly used "gRPC error:" for all cases in the "error" arm.

Fixed: the "error" arm now checks whether HTTP status is present. If yes, it shows both "HTTP:" and "gRPC error:" (matching the original dual-reachability case). If no HTTP status, it shows "Error:" (matching the original no-reachability case).

Applied in 9aa0ec4.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

(I'm still on it, not sure if this is the best solution)

Comment thread crates/openshell-cli/src/run.rs Outdated
Comment on lines +3505 to +3508
let policy_json = config.policy.as_ref().map_or_else(
|| serde_json::Value::Null,
|p| openshell_policy::sandbox_policy_to_json_value(p).unwrap_or(serde_json::Value::Null),
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there a reason this does not propagate the conversion error? Other parts do, do we really want to return a null policy if conversion fails?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You're right. Changed sandbox_detail_to_json to return Result<serde_json::Value> and propagate the conversion error via wrap_err. This matches how sandbox_settings_get (line 7213) handles sandbox_policy_to_json_value errors.

Applied in 9aa0ec4.

@rhuss rhuss force-pushed the feat/cli-structured-output branch from 1816278 to 9aa0ec4 Compare June 25, 2026 04:29
@rhuss

rhuss commented Jun 25, 2026

Copy link
Copy Markdown
Author

@johntmyers thanks for the review! As I'm on my way to dive into the codebase I will try to understand the codebase a bit better (not a Rust coder yet). But I think your comment ironically also hint to a problmen that this PR tries to partially solve: We need to be careful currently around the wording of log/error messages as they are parsed and used for result evaluation (i know at least one tool that wraps that parses the output of sandbox instead of using the SDK directly).

Therefor having the option for a structured output make much sense for these use cases. I wonder though, whether one should keep stderr free for informal messages or whether not (if -o json) is given also the error messages should be structured (I tend to the later). What's your take on this ?

@rhuss

rhuss commented Jun 25, 2026

Copy link
Copy Markdown
Author

I'll create a follow-up issue for emitting structured JSON errors when --output json/yaml is active, so automation consumers get parseable error responses instead of miette-formatted text. That's a broader change touching the error handling in main() and should cover all commands, not just the three in this PR.

@rhuss rhuss force-pushed the feat/cli-structured-output branch from 9aa0ec4 to b88a214 Compare June 25, 2026 05:38
… create

Add structured output support to three commands that previously only
supported human-readable table output:

- `sandbox get --output json/yaml`: emits sandbox detail including
  policy source, revision, and active policy content via a new
  `sandbox_detail_to_json` converter.

- `status --output json/yaml`: emits gateway connection state via a new
  `status_to_json` converter with conditional fields (auth, version,
  error, http_status) matching the human output.

- `sandbox create --output json/yaml`: emits sandbox metadata after the
  sandbox reaches Ready phase, suppressing spinners and ANSI chrome
  from stdout. Uploads and port forwarding still execute before output.
  Clap rejects `--output` combined with `--editor` or trailing commands.

All three commands reuse the existing `OutputFormat` enum and
`print_output_single` helper. Default output (no --output flag) is
byte-identical to current behavior. Unit tests cover both converters.

Closes NVIDIA#1964

Assisted-By: 🤖 Claude Code
Signed-off-by: Roland Huß <rhuss@redhat.com>
@rhuss rhuss force-pushed the feat/cli-structured-output branch from b88a214 to f067c05 Compare June 25, 2026 05:54
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.

Extend --output json to sandbox list, sandbox create, and status commands

2 participants