Skip to content

feat(cli): add JSON/YAML output format to provider list command#1830

Open
jeffmaury wants to merge 5 commits into
NVIDIA:mainfrom
jeffmaury:feat/1745-add-output-format-provider-list/jeffmaury
Open

feat(cli): add JSON/YAML output format to provider list command#1830
jeffmaury wants to merge 5 commits into
NVIDIA:mainfrom
jeffmaury:feat/1745-add-output-format-provider-list/jeffmaury

Conversation

@jeffmaury

Copy link
Copy Markdown
Contributor

🏗️ build-from-issue-agent

Summary

Added -o/--output flag support (json, yaml, table) to openshell provider list by migrating it to use the existing generic output formatter introduced in #1750. This follows the established pattern already used by sandbox list, gateway list, and provider list-profiles commands.

Related Issue

Closes #1745

Changes

  • crates/openshell-cli/src/run.rs: Added provider_to_json() helper function that maps Provider proto fields to JSON, including only credential keys (never values) for security. Added output parameter to provider_list() function and inserted early-return call to crate::output::print_output_collection() for structured formats.
  • crates/openshell-cli/src/main.rs: Added output: OutputFormat field to ProviderCommands::List struct with conflicts_with constraints between names and output flags. Updated invocation to pass output.as_str() parameter.
  • crates/openshell-cli/tests/provider_commands_integration.rs: Updated provider_list() call to include "table" parameter.

Deviations from Plan

None — implemented as planned. The implementation leverages the generic output module (crates/openshell-cli/src/output.rs) that was created as part of #1750, avoiding code duplication.

Testing

  • mise run pre-commit passes (all 778 unit tests passed)
  • Unit tests added/updated
  • E2E tests added/updated (N/A — CLI command formatting does not require E2E coverage)

Tests added:

  • Unit: No new tests required — generic output helpers are already tested in output.rs, and the pattern is proven in production use
  • Integration: Updated existing integration test call signature to pass output parameter
  • E2E: N/A — CLI output formatting does not require E2E coverage

Security verification:
The provider_to_json() helper only exposes credential keys (the keys from the credentials map), never credential values. This prevents CWE-532 (insertion of sensitive information into output) and follows the same security pattern as the provider get command.

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)

Documentation updated:

  • None needed — this change affects CLI command output format only, not gateway configuration or published user-facing behavior documentation

@jeffmaury jeffmaury requested a review from a team as a code owner June 9, 2026 07:34
@copy-pr-bot

copy-pr-bot Bot commented Jun 9, 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.

@johntmyers johntmyers added the gator:in-review Gator is reviewing or awaiting PR review feedback label Jun 10, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

PR Review Status

Validation: This PR is project-valid because it implements linked issue #1745 as a small, focused CLI consistency improvement using the shared output formatter introduced for list commands.
Head SHA: 794ef69dc4e71761b27dea773bf3ffa821ce7e4f

Review findings:

  • crates/openshell-cli/src/run.rs:4740 serializes provider.config into JSON/YAML list output. Provider config is user-controlled provider data and may contain sensitive endpoint material, embedded tokens, headers, or provider-specific secrets depending on backend behavior. Please omit config from provider list structured output, or replace it with an audited allowlist of known-safe keys. As written, this is a CWE-200 information exposure risk.
  • crates/openshell-cli/tests/provider_commands_integration.rs:1000 only updates the existing table call site. Please add coverage for JSON/YAML provider list output that creates a provider with credentials and asserts the structured output includes credential keys but never credential values.
  • This is a direct CLI output change (openshell provider list -o json|yaml). Under the gator docs gate, please update the relevant Fern docs under docs/ and navigation if needed, or get a maintainer-authored comment stating docs are intentionally unnecessary.

Docs: Missing for direct CLI output change, unless waived by a maintainer.

Next state: gator:in-review

@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Author Follow-Up Nudge

This PR has been in gator:in-review for more than 48 business hours with unresolved review feedback.

@jeffmaury, please respond to the review comments or push an update. If this is no longer planned, please say so and a maintainer can close it out.

@jeffmaury

Copy link
Copy Markdown
Contributor Author

Security Enhancement

Updated provider_to_json() to expose only config keys (not values), matching the security model used for credentials.

Changes:

  • Field name changed from config to config_keys
  • Only the keys from provider.config are included in JSON/YAML output
  • This provides defense-in-depth even though config is marked as "non-secret" in the proto definition

Rationale:
Config values might contain sensitive information (URLs, paths, identifiers) that shouldn't be broadly exposed. Following the same pattern as credential_keys creates a consistent security posture.

All tests still pass (778 unit tests).

@jeffmaury

Copy link
Copy Markdown
Contributor Author

Test Coverage Added ✅

Added comprehensive test coverage for provider list JSON/YAML output formats.

Test Layers

1. Unit tests in run.rs (8 tests)

  • ✅ Core field validation (id, name, type)
  • Security: Credential keys exposed, values hidden
  • Security: Config keys exposed, values hidden
  • ✅ Metadata field inclusion/omission logic
  • ✅ Empty field omission behavior
  • ✅ Credential expiration times

2. CLI parsing tests in main.rs (4 tests)

  • ✅ JSON/YAML output format acceptance
  • ✅ Conflicts between --names and -o flags

3. Integration tests in provider_commands_integration.rs (3 tests)

  • ✅ JSON output execution with populated provider
  • ✅ YAML output execution with populated provider
  • ✅ Empty provider list edge case

Security Validation

Multiple tests explicitly verify that credential and config values are never exposed in JSON/YAML output (only keys). This prevents CWE-532 (insertion of sensitive information into output).

Test Results

✅ All 778 existing unit tests continue to pass
✅ 15 new tests added (total now: 793 unit tests)
✅ All pre-commit checks pass

Total added: ~307 lines of test code across 3 files

jeffmaury added a commit to jeffmaury/OpenShell that referenced this pull request Jun 15, 2026
…ist JSON/YAML

Update provider_to_json() to format created_at_ms as a human-readable
datetime string using format_epoch_ms(), matching the pattern used by
sandbox_to_json().

Changes:
- Field name: created_at_ms → created_at
- Value format: Raw milliseconds → "YYYY-MM-DD HH:MM:SS"
- Implementation: Use format_epoch_ms(meta.created_at_ms)

This change:
- Aligns provider list output with sandbox list output for consistency
- Makes JSON/YAML output more human-readable for interactive use
- Follows the established pattern across all *_to_json() functions

Breaking change: Field renamed and type changed (number → string).
However, this feature was just added in PR NVIDIA#1830 and hasn't been
released yet, so no users are affected.

Example output:
Before: {"created_at_ms": 1234567890000}
After:  {"created_at": "2009-02-13 23:31:30"}

Tests updated:
- provider_to_json_includes_metadata_fields_when_present: Assert formatted string
- provider_to_json_omits_zero_metadata_fields: Update field name
- provider_to_json_formats_created_at_as_human_readable: New test validating format
@jeffmaury

Copy link
Copy Markdown
Contributor Author

Human-Readable Timestamps ✅

Updated provider_to_json() to format created_at as a human-readable datetime string, matching the pattern used by sandbox_to_json().

Changes

Field name: created_at_mscreated_at
Value format: Raw milliseconds → "YYYY-MM-DD HH:MM:SS"
Implementation: Use format_epoch_ms(meta.created_at_ms)

Example Output

Before:

{
  "id": "prov-abc123",
  "name": "my-anthropic",
  "created_at_ms": 1234567890000
}

After:

{
  "id": "prov-abc123",
  "name": "my-anthropic",
  "created_at": "2009-02-13 23:31:30"
}

Breaking Change Note

This is a breaking change (field renamed, type changed from number to string), but since this feature was just added in this PR and hasn't been released yet, no users are affected.

Consistency

This change aligns provider list with sandbox list output:

  • sandbox list -o json uses "created_at": "2021-01-01 00:00:00"
  • provider list -o json now also uses "created_at": "2021-01-01 00:00:00"

Both now use the same field name and format for consistency across all list commands.

Tests

✅ All 778 existing unit tests pass
✅ New test added: provider_to_json_formats_created_at_as_human_readable
✅ Updated tests to expect formatted datetime string

@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Re-check After Author Update

I re-evaluated latest head 9156215add8f4b5dc826e7f664b5a922e44fe67f after @jeffmaury's June 15 comments and commits.

Disposition: partially resolved.

Remaining items:

  • Resolved: the prior config-value exposure finding is addressed. Structured output now emits config_keys rather than config values, matching the credential-key pattern.
  • Partially resolved: test coverage was added, but the public command path still needs a regression test proving openshell provider list --names remains valid with the new defaulted output option. The JSON/YAML integration tests should also assert captured output includes credential/config keys and excludes their values, rather than only asserting the calls succeed.
  • Still open: this adds a direct CLI output flag (openshell provider list -o json|yaml), but there are no docs/ updates and no maintainer-authored docs waiver yet.
  • Process blocker: GitHub currently reports this PR as having merge conflicts (mergeable_state=dirty).

Next action: @jeffmaury, please update/rebase the branch against main, then address the remaining test/docs items or get a maintainer docs waiver.

Next state: gator:blocked

@johntmyers johntmyers added gator:blocked Gator is blocked by process or repository gates and removed gator:in-review Gator is reviewing or awaiting PR review feedback labels Jun 15, 2026
Closes NVIDIA#1745

Add -o/--output flag support (json, yaml, table) to openshell provider list
by migrating it to use the existing generic output formatter from NVIDIA#1750.
This follows the established pattern used by sandbox list, gateway list,
and provider list-profiles commands.

Changes:
- crates/openshell-cli/src/run.rs: Add provider_to_json() helper that maps
  Provider proto fields to JSON, including only credential keys (not values)
  for security. Add output parameter to provider_list() and insert early-return
  for structured formats.
- crates/openshell-cli/src/main.rs: Add output field to ProviderCommands::List
  with conflicts_with constraints between names and output flags. Update
  invocation to pass output parameter.
- crates/openshell-cli/tests/provider_commands_integration.rs: Update
  provider_list() call to include output parameter.

Security: The provider_to_json() helper only exposes credential keys, never
credential values, preventing CWE-532 (insertion of sensitive information
into output).
Update provider_to_json() to only include config keys (not values) in
structured output, matching the security model used for credentials.
This prevents potential exposure of sensitive configuration values.

Changed field name from "config" to "config_keys" to clarify that only
keys are included, consistent with "credential_keys" field naming.
Add three layers of test coverage for provider list structured output:

1. Unit tests in run.rs for provider_to_json() (8 tests):
   - Core field validation (id, name, type)
   - Security: Credential keys exposed, values hidden
   - Security: Config keys exposed, values hidden
   - Metadata field inclusion/omission logic
   - Empty field omission behavior
   - Credential expiration times

2. CLI parsing tests in main.rs (4 tests):
   - JSON/YAML output format acceptance
   - Conflicts between --names and -o flags

3. Integration tests in provider_commands_integration.rs (3 tests):
   - JSON output execution with populated provider
   - YAML output execution with populated provider
   - Empty provider list edge case

Security focus: Multiple tests verify that credential and config VALUES
are never exposed in JSON/YAML output, only keys. This prevents CWE-532
(insertion of sensitive information into output).

All 778 existing unit tests continue to pass, plus 15 new tests added.
…ist JSON/YAML

Update provider_to_json() to format created_at_ms as a human-readable
datetime string using format_epoch_ms(), matching the pattern used by
sandbox_to_json().

Changes:
- Field name: created_at_ms → created_at
- Value format: Raw milliseconds → "YYYY-MM-DD HH:MM:SS"
- Implementation: Use format_epoch_ms(meta.created_at_ms)

This change:
- Aligns provider list output with sandbox list output for consistency
- Makes JSON/YAML output more human-readable for interactive use
- Follows the established pattern across all *_to_json() functions

Breaking change: Field renamed and type changed (number → string).
However, this feature was just added in PR NVIDIA#1830 and hasn't been
released yet, so no users are affected.

Example output:
Before: {"created_at_ms": 1234567890000}
After:  {"created_at": "2009-02-13 23:31:30"}

Tests updated:
- provider_to_json_includes_metadata_fields_when_present: Assert formatted string
- provider_to_json_omits_zero_metadata_fields: Update field name
- provider_to_json_formats_created_at_as_human_readable: New test validating format
@jeffmaury jeffmaury force-pushed the feat/1745-add-output-format-provider-list/jeffmaury branch from 9156215 to a5f59ce Compare June 15, 2026 16:02
@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test a5f59ce

@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Re-check After Author Update

I re-evaluated latest head a5f59ce97fa275810c10f664789bedf8378d788b after @jeffmaury's June 15 commits.

Disposition: partially resolved.

Resolved items:

  • The prior config-value exposure finding is addressed. Structured output now emits config_keys rather than config values, matching the credential-key pattern.
  • The prior merge-conflict blocker is resolved; GitHub REST now reports mergeable=true.
  • The copy-pr mirror gate was waiting for /ok to test; I posted /ok to test a5f59ce97fa275810c10f664789bedf8378d788b.

Remaining items:

  • The JSON/YAML integration tests still only assert that provider_list(..., "json"|"yaml") succeeds. Please add captured-output coverage proving credential/config keys are present and credential/config values such as test-key and us-west are absent.
  • Please add a parser regression test proving openshell provider list --names still parses with the new defaulted output option.
  • This is a direct CLI UX change. Please update docs/sandboxes/manage-providers.mdx with provider list -o json / -o yaml examples, or get a maintainer-authored docs waiver.

Credential-refresh context: this PR does not touch provider refresh, profile injection, or placeholder rewrite paths, so the prior GitHub EOF/stale revision-scoped credential-placeholder concern does not appear directly implicated by this changeset.

Next state: gator:in-review

@johntmyers johntmyers added gator:in-review Gator is reviewing or awaiting PR review feedback and removed gator:blocked Gator is blocked by process or repository gates labels Jun 15, 2026
Add documentation for the -o/--output flag on provider list command,
following the established pattern from sandbox list documentation.

Changes:
- Add examples showing -o json and -o yaml usage
- Explain output structure (metadata, credential keys, config keys)
- Emphasize security design (keys-only exposure, never values)
- Cross-reference existing Credential Injection section

The documentation matches the feature added in a26a634 and refined
in subsequent commits (509db49, a5f59ce).

Signed-off-by: Jeff MAURY <jmaury@redhat.com>
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Re-check After Author Update

I re-evaluated latest head 993e557e691a0c454bd19b3491af5cdb2fb21d61 after @jeffmaury's June 15 docs commit.

Disposition: partially resolved.

Resolved items:

  • The docs gate is now satisfied. docs/sandboxes/manage-providers.mdx documents openshell provider list -o json and -o yaml, including the keys-only credential/config exposure model.
  • The prior config-value exposure finding remains resolved. Structured output emits config_keys rather than config values.

Remaining items:

  • The JSON/YAML integration tests still only assert that provider_list(..., "json"|"yaml") succeeds. Please add captured-output coverage proving credential/config keys are present and credential/config values such as test-key and us-west are absent on the public list path.
  • Please add a parser regression test proving openshell provider list --names still parses with the new defaulted output option.

Credential-refresh context: this PR does not modify provider refresh, profile injection, placeholder rewrite, or credential/header request paths. It only reads provider credential/config map keys for CLI list serialization, so the stale revision-scoped credential-placeholder EOF concern does not appear directly implicated by this changeset.

Next state: gator:in-review

@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test 993e557

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

Labels

gator:in-review Gator is reviewing or awaiting PR review feedback

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: add JSON/YAML output format to provider list command

2 participants