feat(cli): add JSON/YAML output format to provider list command#1830
feat(cli): add JSON/YAML output format to provider list command#1830jeffmaury wants to merge 5 commits into
Conversation
PR Review StatusValidation: 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. Review findings:
Docs: Missing for direct CLI output change, unless waived by a maintainer. Next state: |
Author Follow-Up NudgeThis PR has been in @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. |
Security EnhancementUpdated Changes:
Rationale: All tests still pass (778 unit tests). |
Test Coverage Added ✅Added comprehensive test coverage for Test Layers1. Unit tests in
2. CLI parsing tests in
3. Integration tests in
Security ValidationMultiple 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 Total added: ~307 lines of test code across 3 files |
…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
Human-Readable Timestamps ✅Updated ChangesField name: Example OutputBefore: {
"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 NoteThis 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. ConsistencyThis change aligns
Both now use the same field name and format for consistency across all list commands. Tests✅ All 778 existing unit tests pass |
Re-check After Author UpdateI re-evaluated latest head Disposition: partially resolved. Remaining items:
Next action: @jeffmaury, please update/rebase the branch against Next state: |
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
9156215 to
a5f59ce
Compare
|
/ok to test a5f59ce |
Re-check After Author UpdateI re-evaluated latest head Disposition: partially resolved. Resolved items:
Remaining items:
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: |
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>
Re-check After Author UpdateI re-evaluated latest head Disposition: partially resolved. Resolved items:
Remaining items:
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: |
|
/ok to test 993e557 |
Summary
Added
-o/--outputflag support (json, yaml, table) toopenshell provider listby migrating it to use the existing generic output formatter introduced in #1750. This follows the established pattern already used bysandbox list,gateway list, andprovider list-profilescommands.Related Issue
Closes #1745
Changes
crates/openshell-cli/src/run.rs: Addedprovider_to_json()helper function that maps Provider proto fields to JSON, including only credential keys (never values) for security. Addedoutputparameter toprovider_list()function and inserted early-return call tocrate::output::print_output_collection()for structured formats.crates/openshell-cli/src/main.rs: Addedoutput: OutputFormatfield toProviderCommands::Liststruct withconflicts_withconstraints betweennamesandoutputflags. Updated invocation to passoutput.as_str()parameter.crates/openshell-cli/tests/provider_commands_integration.rs: Updatedprovider_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-commitpasses (all 778 unit tests passed)Tests added:
output.rs, and the pattern is proven in production useSecurity 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 theprovider getcommand.Checklist
Documentation updated: