fix(config-validate): emit structured output for -o json/yaml on success#56
Closed
shreemaan-abhishek wants to merge 1 commit into
Closed
fix(config-validate): emit structured output for -o json/yaml on success#56shreemaan-abhishek wants to merge 1 commit into
shreemaan-abhishek wants to merge 1 commit into
Conversation
`a7 config validate -f <file> -o json` previously printed the human-readable
string `Config is valid` on stdout and exited 0, ignoring the requested
format. Pipelines like `... -o json | jq '.valid'` failed with a parse
error.
Read the inherited --output flag in RunE; on the success path, emit a
`validateResult{Valid: true, Message: "Config is valid"}` via the shared
cmdutil exporter for json/yaml. Table / unset output is unchanged, so
interactive users keep seeing the friendly message.
Adds three unit tests covering json, yaml, and the default path.
📝 WalkthroughWalkthroughThe PR adds structured output support to the ChangesAdd structured output format support to validate command
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cmd/config/validate/validate.go`:
- Around line 89-95: The switch on opts.Output in the validate flow currently
only handles "json" and "yaml" and silently falls back for unknown non-empty
values; update the logic around opts.Output (the switch and its default path) to
detect unsupported non-empty outputs and return a non-nil error (e.g., using
fmt.Errorf) instead of proceeding to plaintext success, so callers relying on
structured output get a clear failure; keep using
cmdutil.NewExporter(opts.Output, opts.IO.Out).Write(validateResult{...}) for the
supported "json"/"yaml" branches and ensure validateResult remains used for the
successful export.
- Line 48: Change the code that reads the --output flag to handle and return the
error from c.Flags().GetString("output") instead of discarding it (assign to
opts.Output and check err, returning err up the call chain). After reading
opts.Output validate its value: if opts.Output is non-empty, either
construct/validate via cmdutil.NewExporter(opts.Output) (and return its error)
or explicitly reject values other than "json" or "yaml" by returning an error
like "unsupported output format"; ensure the function (the validate command
handler where opts.Output is used) returns that error rather than silently
falling back to the human message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fdbcc3ec-bae5-4a0c-b9f1-e98d9468c2e4
📒 Files selected for processing (2)
pkg/cmd/config/validate/validate.gopkg/cmd/config/validate/validate_test.go
| if opts.File == "" { | ||
| return &cmdutil.FlagError{Err: fmt.Errorf("required flag \"file\" not set")} | ||
| } | ||
| opts.Output, _ = c.Flags().GetString("output") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/cmd/config/validate/validate.go"
echo "== file =="
wc -l "$file"
echo
echo "== lines 1-140 (for context) =="
sed -n '1,140p' "$file" | nl -ba
echo
echo "== lines around 35-60 =="
sed -n '35,70p' "$file" | nl -ba
echo
echo "== lines around 80-110 =="
sed -n '80,120p' "$file" | nl -ba
echo
echo "== find where --output/-o is defined (c.Flags().GetString(\"output\")) =="
rg -n "GetString\\(\"output\"\\)" pkg/cmd/config/validate/validate.go pkg/cmd/config/validate -S || true
rg -n "persistent.*--output|persistent.*output|--output\"|StringVarP\\(|String\\(\"output\"" pkg/cmd/config -S || true
echo
echo "== find validate command wiring =="
rg -n "NewCmd\\(|validate" pkg/cmd/config/validate/validate.go -S || trueRepository: api7/a7
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/cmd/config/validate/validate.go"
echo "== file =="
wc -l "$file"
echo
echo "== lines 1-140 =="
sed -n '1,140p' "$file" | cat -n
echo
echo "== lines 35-70 =="
sed -n '35,70p' "$file" | cat -n
echo
echo "== lines 80-120 =="
sed -n '80,120p' "$file" | cat -n
echo
echo "== find GetString(\"output\") in this file/folder =="
rg -n "GetString\\(\"output\"\\)" pkg/cmd/config/validate/validate.go pkg/cmd/config/validate -S || true
echo
echo "== find where --output/-o is defined elsewhere =="
rg -n "persistent.*--output|persistent.*output|--output\"|StringVarP\\(|String\\(\"output\"" pkg/cmd/config -S || true
echo
echo "== validate command wiring =="
rg -n "NewCmd\\(|validate" pkg/cmd/config/validate/validate.go -S || trueRepository: api7/a7
Length of output: 9674
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== rg where output flag is defined across pkg/cmd =="
rg -n "StringVarP\\(&[^\\n]*opts\\.Output[^\\n]*,\\s*\"output\"\\s*,\\s*\"o\"" pkg/cmd -S || true
rg -n "StringVarP\\(&[^\\n]*,\\s*\"output\"\\s*,\\s*\"o\"" pkg/cmd -S || true
rg -n "Persistent.*StringVarP\\(.*\"output\".*\"o\"" pkg/cmd -S || true
rg -n "Persistent.*--output|--output\\b|\"output\"\\s*,\\s*\"o\"" pkg/cmd -S || true
echo
echo "== rg for GetString(\"output\") across pkg/cmd =="
rg -n "GetString\\(\"output\"\\)" pkg/cmd -S || trueRepository: api7/a7
Length of output: 8978
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate cmdutil.NewExporter implementation =="
fd -a "exporter" pkg/cmdutil pkg -t f || true
rg -n "func NewExporter" -S pkg/cmdutil pkg || true
rg -n "NewExporter\\(" -S pkg/cmdutil pkg/cmd || trueRepository: api7/a7
Length of output: 9085
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== root persistent --output definition =="
sed -n '1,120p' pkg/cmd/root/root.go | cat -n
echo
echo "== cmdutil/exporter.go =="
sed -n '1,220p' pkg/cmdutil/exporter.go | cat -n
echo
echo "== validate tests =="
ls -la pkg/cmd/config/validate || true
sed -n '1,200p' pkg/cmd/config/validate/validate_test.go | cat -nRepository: api7/a7
Length of output: 13353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "newCmdWithOutputFlag\\(" pkg/cmd/config/validate/validate_test.go
rg -n "NewCmdValidate\\(" pkg/cmd/config/validate/validate_test.go
echo
echo "== show remaining part of validate_test.go around newCmdWithOutputFlag usage =="
sed -n '1,120p' pkg/cmd/config/validate/validate_test.go | cat -n
sed -n '120,260p' pkg/cmd/config/validate/validate_test.go | cat -n
sed -n '260,520p' pkg/cmd/config/validate/validate_test.go | cat -nRepository: api7/a7
Length of output: 15736
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== validate.go output handling =="
rg -n "switch opts\\.Output|case \"json\"|case \"yaml\"|fmt\\.Fprintln\\(opts\\.IO\\.Out" pkg/cmd/config/validate/validate.go
sed -n '80,110p' pkg/cmd/config/validate/validate.go | cat -n
echo
echo "== config/dump.go output handling =="
sed -n '1,140p' pkg/cmd/config/dump/dump.go | cat -n
rg -n "opts\\.Output|NewExporter\\(" pkg/cmd/config/dump/dump.go
echo
echo "== config/diff.go output handling =="
sed -n '1,140p' pkg/cmd/config/diff/diff.go | cat -n
rg -n "opts\\.Output|switch opts\\.Output|NewExporter\\(" pkg/cmd/config/diff/diff.go
echo
echo "== other commands: how they handle invalid opts.Output =="
rg -n "switch opts\\.Output" pkg/cmd -S --glob='*.go'
rg -n "case \"json\", \"yaml\"|case \"json\".*case \"yaml\"" pkg/cmd -S --glob='*.go'Repository: api7/a7
Length of output: 6821
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "output.*(invalid|unsupported|toml|table|xml|yaml|json)" pkg/cmd/config/validate/validate_test.go -S || true
rg -n "\"-o\"|\"--output\"" pkg/cmd/config/validate/validate_test.go -S || true
rg -n "SuccessOutput|Output" pkg/cmd/config/validate/validate_test.go -S || trueRepository: api7/a7
Length of output: 1446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
pkg/cmd/context/list/list.go \
pkg/cmd/stream-route/list/list.go \
pkg/cmd/service/list/list.go \
pkg/cmd/route/list/list.go
do
echo "== $f =="
rg -n "GetString\\(\"output\"\\)" "$f" -n || true
# show nearby output handling
# print switch/if on opts.Output if present
rg -n "switch opts\\.Output|if opts\\.Output|case \"json\"|case \"yaml\"" "$f" -S || true
sed -n '1,140p' "$f" | cat -n
echo
doneRepository: api7/a7
Length of output: 15128
Handle --output flag error and reject unsupported output values in config validate
- Line 48:
opts.Output, _ = c.Flags().GetString("output")discards the returned error; return it instead to follow the “never suppress errors” rule. - Lines 89-97: if
opts.Outputis non-empty but notjson/yaml, the command silently falls back to printing the human message ("Config is valid"), instead of erroring (in contrast to other commands that route throughcmdutil.NewExporter, which returnsunsupported output format).
Proposed fix
- opts.Output, _ = c.Flags().GetString("output")
+ output, err := c.Flags().GetString("output")
+ if err != nil {
+ return fmt.Errorf("failed to read output flag: %w", err)
+ }
+ opts.Output = output🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/cmd/config/validate/validate.go` at line 48, Change the code that reads
the --output flag to handle and return the error from
c.Flags().GetString("output") instead of discarding it (assign to opts.Output
and check err, returning err up the call chain). After reading opts.Output
validate its value: if opts.Output is non-empty, either construct/validate via
cmdutil.NewExporter(opts.Output) (and return its error) or explicitly reject
values other than "json" or "yaml" by returning an error like "unsupported
output format"; ensure the function (the validate command handler where
opts.Output is used) returns that error rather than silently falling back to the
human message.
Comment on lines
+89
to
+95
| switch opts.Output { | ||
| case "json", "yaml": | ||
| return cmdutil.NewExporter(opts.Output, opts.IO.Out).Write(validateResult{ | ||
| Valid: true, | ||
| Message: "Config is valid", | ||
| }) | ||
| } |
There was a problem hiding this comment.
Return an error for unsupported non-empty output values (Lines 89-95).
Right now unknown values (e.g. -o jsn) silently fall back to plaintext success, which can break automation expecting structured output.
Proposed fix
switch opts.Output {
case "json", "yaml":
return cmdutil.NewExporter(opts.Output, opts.IO.Out).Write(validateResult{
Valid: true,
Message: "Config is valid",
})
+ case "", "table":
+ fmt.Fprintln(opts.IO.Out, "Config is valid")
+ return nil
+ default:
+ return fmt.Errorf("unsupported output format: %s", opts.Output)
}
- fmt.Fprintln(opts.IO.Out, "Config is valid")
- return nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/cmd/config/validate/validate.go` around lines 89 - 95, The switch on
opts.Output in the validate flow currently only handles "json" and "yaml" and
silently falls back for unknown non-empty values; update the logic around
opts.Output (the switch and its default path) to detect unsupported non-empty
outputs and return a non-nil error (e.g., using fmt.Errorf) instead of
proceeding to plaintext success, so callers relying on structured output get a
clear failure; keep using cmdutil.NewExporter(opts.Output,
opts.IO.Out).Write(validateResult{...}) for the supported "json"/"yaml" branches
and ensure validateResult remains used for the successful export.
5 tasks
Contributor
Author
|
I think it's alright if -o json or -o yaml does not work here, maybe we can do this sometime in future. |
shreemaan-abhishek
added a commit
that referenced
this pull request
May 27, 2026
…gaps The first CI run on this branch surfaced two problems: 1. The suite ran inside the regular `make test-e2e` target. That target is the existing CI gate; bringing 290+ permutation cases into it both inflates the run time and tied PR merge to the suite reporting clean — which it does not yet, because three real CLI inconsistencies are still open (fixed in PR #56 and PR #57). The permutation matrix is meant to be a manual triage tool, not a CI gate. Fix: tag the Describe with Label("permutation") and update test-e2e / test-e2e-full to skip it via --label-filter='!permutation'. The dedicated test-e2e-permutation target inverts the filter to include only the permutation specs. Net effect: regular CI no longer sees the suite; `make test-e2e-permutation` is unchanged. 2. The CI EE rejected `stream-route create` with `Error: API error (status 400): can not create a Stream Route to the HTTP Service`. Stream routes need an L4 service on that deployment; my local EE accepted them attached to an HTTP service. The walker then cascaded 7 failures (get / list / export / delete all error with "resource not found"). Fix: detect known capability-gap stderr patterns after the create step and downgrade the rest of the walker to a single informational "skipped" record. Patterns cover stream-route on HTTP-only EEs plus the existing secret-provider / vault gaps already handled by local_stability. The walker now also stops short on any create failure (capability gap or otherwise) rather than producing cascade noise from "resource not found" on every follow-up step.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
a7 config validate -f <file> -o jsonpreviously printed the human-readable stringConfig is validand exited 0 regardless of the requested format, so pipelines like... -o json | jq '.valid'failed with a parse error.--outputflag inRunE; on the success path emitvalidateResult{Valid: true, Message: "Config is valid"}via the sharedcmdutilexporter forjson/yaml.How the bug was found
Surfaced by a new combinatorial/permutation test suite (separate follow-up PR). One of the 290+ cases asserted that
-o jsonproduces parseable JSON on success; the case failed with `stdout is not valid JSON: invalid character 'C'` (`'C'` is the first byte of "Config is valid").Test plan
Output shape
```json
{
"valid": true,
"message": "Config is valid"
}
```
```yaml
valid: true
message: Config is valid
```
The error path (validation failed) is unchanged — it still returns a non-zero exit code with the existing `config validation failed:` message on stderr. Structured error output is a separate concern not in scope here.
Summary by CodeRabbit
New Features
-o/--outputflag. Users can specify--output jsonor--output yamlto receive validation results in machine-readable formats with status and message fields.Tests