feat: unify output surface contract for flags, formats and errors#1882
Open
sang-neo03 wants to merge 1 commit into
Open
feat: unify output surface contract for flags, formats and errors#1882sang-neo03 wants to merge 1 commit into
sang-neo03 wants to merge 1 commit into
Conversation
github-actions
Bot
added
domain/calendar
📝 WalkthroughWalkthroughThe CLI now centralizes output-format resolution, supports explicit pretty JSON rendering, validates unsupported formats, and converts Cobra parsing and argument failures into structured validation errors. Service-specific flag suggestion hooks are removed, and pure navigation groups are excluded from runnable command manifests. ChangesOutput format resolution and rendering
Structured Cobra validation
Command manifest classification
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Cobra
participant ValidationGuards
participant Command
User->>Cobra: Provide flags and arguments
Cobra->>ValidationGuards: Parse and validate input
ValidationGuards-->>Cobra: Structured validation error or valid input
Cobra->>Command: Execute validated command
Command-->>User: Output or typed error envelope
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1882 +/- ##
==========================================
- Coverage 74.66% 74.66% -0.01%
==========================================
Files 877 877
Lines 91731 91737 +6
==========================================
+ Hits 68494 68497 +3
+ Misses 17926 17911 -15
- Partials 5311 5329 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@041bf48e0efca620ff055315b7fe39c7bc22ea04🧩 Skill updatenpx skills add larksuite/cli#feat/output-surface-contract -y -g |
sang-neo03
force-pushed
the
feat/output-surface-contract
branch
from
108f371 to
f560b1a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@cmd/auth/auth_test.go`:
- Around line 358-378: Update TestAuthScopesCmd_JSONShorthand and the adjacent
test case around the same command to call t.Setenv("LARKSUITE_CLI_CONFIG_DIR",
t.TempDir()) before invoking cmdutil.TestFactory, ensuring each test uses an
isolated temporary CLI configuration directory.
In `@cmd/root.go`:
- Around line 724-753: The typed flag error tests in cmd/flag_suggest_test.go
cover neither the InvalidSyntaxError nor InvalidValueError branches of
typedFlagParseError. Add focused tests for both paths, verifying the returned
validation error includes the expected InvalidParam name and reason while
preserving the existing error behavior.
In `@internal/output/capabilities_test.go`:
- Around line 32-38: Update the wantErr assertion in the Resolve() test to
validate the ValidationError subtype as well as Param. After errors.As extracts
validationErr, assert both typed metadata fields match the expected
format-validation subtype and "--format" parameter, preserving the existing
failure message and early return behavior.
In `@shortcuts/common/runner.go`:
- Around line 1216-1223: Update shortcutFormatSupportsJSON to treat a format
flag with no enum values as JSON-supported when its default format is "json",
matching shortcutFormatCapabilities; preserve the existing enum-membership check
when choices are defined and the current true result when no format flag exists.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cfea247e-676e-4ca7-8848-26753b784af8
📒 Files selected for processing (32)
cmd/api/api.gocmd/api/api_test.gocmd/auth/auth_test.gocmd/auth/scopes.gocmd/auth/scopes_test.gocmd/build.gocmd/build_test.gocmd/flag_suggest_test.gocmd/root.gocmd/root_test.gocmd/service/service.gocmd/service/service_test.gocmd/unknown_subcommand_test.gogo.modinternal/output/capabilities.gointernal/output/capabilities_test.gointernal/output/format.gointernal/output/format_test.gointernal/output/format_type.gointernal/output/format_type_test.gointernal/qualitygate/cmd/manifest-export/collect.gointernal/qualitygate/cmd/manifest-export/main_test.goshortcuts/calendar/calendar_test.goshortcuts/common/runner.goshortcuts/common/runner_format_universal_test.goshortcuts/mail/flag_suggest.goshortcuts/mail/flag_suggest_test.goshortcuts/mail/mail_json_shorthand_test.goshortcuts/register.goshortcuts/register_test.goshortcuts/sheets/flag_ergonomics.goshortcuts/sheets/flag_ergonomics_test.go
💤 Files with no reviewable changes (5)
- shortcuts/mail/flag_suggest.go
- shortcuts/register.go
- shortcuts/mail/flag_suggest_test.go
- shortcuts/register_test.go
- shortcuts/sheets/flag_ergonomics_test.go
Consolidate the CLI output/error contract so agents can predict behavior from a command's declaration: - --json / --format resolved through a single FormatCapabilities set; explicit --format wins over --json across api, service, shortcut and auth scopes - --format pretty renders as indented JSON via its own Format case, no longer aliased to table - a single root FlagErrorFunc classifies pflag typed errors; the mail and sheets local FlagErrorFunc hooks are removed - cobra argument, required-flag and flag-group errors are typed at their validation stage; the usage-error text markers are removed - root and group unknown commands return structured suggestions - manifest-export marks pure group nodes as non-runnable
sang-neo03
force-pushed
the
feat/output-surface-contract
branch
from
f560b1a to
041bf48
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@shortcuts/common/runner_format_universal_test.go`:
- Around line 117-123: Update the error assertions in the test around errors.As
to call errs.ProblemOf(err) and assert the returned category, subtype, and param
metadata, including the existing --format expectation; retain the type assertion
only if needed for the test’s intended coverage.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c94897fa-85a3-489d-96d0-9c27d073d7c3
📒 Files selected for processing (32)
cmd/api/api.gocmd/api/api_test.gocmd/auth/auth_test.gocmd/auth/scopes.gocmd/auth/scopes_test.gocmd/build.gocmd/build_test.gocmd/flag_suggest_test.gocmd/root.gocmd/root_test.gocmd/service/service.gocmd/service/service_test.gocmd/unknown_subcommand_test.gogo.modinternal/output/capabilities.gointernal/output/capabilities_test.gointernal/output/format.gointernal/output/format_test.gointernal/output/format_type.gointernal/output/format_type_test.gointernal/qualitygate/cmd/manifest-export/collect.gointernal/qualitygate/cmd/manifest-export/main_test.goshortcuts/calendar/calendar_test.goshortcuts/common/runner.goshortcuts/common/runner_format_universal_test.goshortcuts/mail/flag_suggest.goshortcuts/mail/flag_suggest_test.goshortcuts/mail/mail_json_shorthand_test.goshortcuts/register.goshortcuts/register_test.goshortcuts/sheets/flag_ergonomics.goshortcuts/sheets/flag_ergonomics_test.go
💤 Files with no reviewable changes (5)
- shortcuts/mail/flag_suggest.go
- shortcuts/register.go
- shortcuts/mail/flag_suggest_test.go
- shortcuts/sheets/flag_ergonomics_test.go
- shortcuts/register_test.go
🚧 Files skipped from review as they are similar to previous changes (22)
- cmd/build.go
- shortcuts/calendar/calendar_test.go
- internal/output/format_type.go
- internal/output/capabilities_test.go
- internal/output/format_type_test.go
- internal/qualitygate/cmd/manifest-export/main_test.go
- cmd/auth/scopes.go
- cmd/api/api_test.go
- cmd/flag_suggest_test.go
- shortcuts/sheets/flag_ergonomics.go
- cmd/service/service.go
- cmd/root_test.go
- cmd/unknown_subcommand_test.go
- internal/qualitygate/cmd/manifest-export/collect.go
- cmd/auth/scopes_test.go
- cmd/auth/auth_test.go
- internal/output/capabilities.go
- cmd/api/api.go
- cmd/build_test.go
- cmd/service/service_test.go
- cmd/root.go
- shortcuts/common/runner.go
Comment on lines
+117
to
+123
| var validationErr *errs.ValidationError | ||
| if !errors.As(err, &validationErr) { | ||
| t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err) | ||
| } | ||
| if validationErr.Param != "--format" { | ||
| t.Fatalf("param = %q, want --format", validationErr.Param) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert Category and Subtype via errs.ProblemOf.
As per coding guidelines, error-path tests must assert typed metadata via errs.ProblemOf (category / subtype / param). This test currently checks the Param field using errors.As, but it omits the required errs.ProblemOf assertions for Category and Subtype.
💡 Proposed fix to include full metadata assertions
var validationErr *errs.ValidationError
if !errors.As(err, &validationErr) {
t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err)
}
if validationErr.Param != "--format" {
t.Fatalf("param = %q, want --format", validationErr.Param)
}
+
+ problem, ok := errs.ProblemOf(err)
+ if !ok {
+ t.Fatalf("error = %T, want typed errs problem carrier", err)
+ }
+ if problem.Category != errs.CategoryValidation {
+ t.Fatalf("category = %q, want %q", problem.Category, errs.CategoryValidation)
+ }
+ if problem.Subtype != errs.SubtypeInvalidArgument {
+ t.Fatalf("subtype = %q, want %q", problem.Subtype, errs.SubtypeInvalidArgument)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var validationErr *errs.ValidationError | |
| if !errors.As(err, &validationErr) { | |
| t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err) | |
| } | |
| if validationErr.Param != "--format" { | |
| t.Fatalf("param = %q, want --format", validationErr.Param) | |
| } | |
| var validationErr *errs.ValidationError | |
| if !errors.As(err, &validationErr) { | |
| t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err) | |
| } | |
| if validationErr.Param != "--format" { | |
| t.Fatalf("param = %q, want --format", validationErr.Param) | |
| } | |
| problem, ok := errs.ProblemOf(err) | |
| if !ok { | |
| t.Fatalf("error = %T, want typed errs problem carrier", err) | |
| } | |
| if problem.Category != errs.CategoryValidation { | |
| t.Fatalf("category = %q, want %q", problem.Category, errs.CategoryValidation) | |
| } | |
| if problem.Subtype != errs.SubtypeInvalidArgument { | |
| t.Fatalf("subtype = %q, want %q", problem.Subtype, errs.SubtypeInvalidArgument) | |
| } |
🤖 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 `@shortcuts/common/runner_format_universal_test.go` around lines 117 - 123,
Update the error assertions in the test around errors.As to call
errs.ProblemOf(err) and assert the returned category, subtype, and param
metadata, including the existing --format expectation; retain the type assertion
only if needed for the test’s intended coverage.
Source: Coding guidelines
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
The CLI output and error surface was inconsistent, so an agent could not predict a command's behavior from its declaration:
--jsonwas a dead flag on api/service yet worked (with differing priority) on shortcuts andauth scopes;--format prettywas advertised and handled differently across families; top-level unknown commands returned prose without structured suggestions; and flag/argument errors were classified by matching English text fragments, which misclassified flag-group errors as internal faults (exit 5). This PR unifies the whole surface into one predictable contract.Changes
internal/output/capabilities.go(FormatCapabilities) as the single source for--formatusage text, shell completion,--jsonshorthand resolution and format validation;cmd/api/api.go,cmd/service/service.go,cmd/auth/scopes.goandshortcuts/common/runner.gonow resolve through it, with explicit--formatwinning over--json--format prettyas indented JSON via its ownFormatPrettycase ininternal/output/format.goandformat_type.go, instead of aliasing it to tableFlagErrorFuncincmd/root.gothat classifies pflag typed errors; delete the mail and sheets local hooks (shortcuts/mail/flag_suggest.go,shortcuts/sheets/flag_ergonomics.go,shortcuts/register.go)installCobraValidationGuardsincmd/root.go, and remove thecobraUsageErrorMarkers/isCobraUsageErrortext matchingcmd/root.go,cmd/build.go)internal/qualitygate/cmd/manifest-export/collect.goKnown limitation
Warning
--format pretty/tableis only partially delivered for shortcuts.api, service and
auth scopeshonor the selected format, but a subset ofshortcuts that emit their result through the framework default
ctx.Outstill return the JSON envelope even though their help/completion advertise
pretty/table. Their format capability is injected uniformly by theframework instead of declared per command, so an advertised
--format tableis accepted but not actually rendered.
This is a pre-existing gap, not a regression — those shortcuts behaved the
same before this PR, which additionally fixes api/service and turns silent
fallback into an explicit error. Fully closing it means declaring each
shortcut's real format capability across ~260
ctx.Outcall sites in a dozendomains (base, sheets, drive, mail, ...); that shares the output-emitter area
tracked separately and is deferred to that follow-up.
Test Plan
make unit-testpassedgo build ./...andgo vet ./...are clean and manual verification below covers each behaviorapi GET <path> --dry-run --format pretty --json—--jsonno longer overrides an explicit formatauth scopes --format pretty --json— prints human text (explicit--formatwins)api GET <path> --format bogus— typed validation error, exit 2 (was warning + json)imm— structuredparams[].suggestions: ["im"], exit 2sheets +csv-put --url <u> --sheet-id <s> --csv a,b— flag-group error is validation/exit 2 withparams, was internal/exit 5mail +send --tos <addr>— still suggests--toRelated Issues
N/A
Summary by CodeRabbit
--jsonshorthand and data-driven--formathelp/completion.prettyformat for readable, indented JSON, including paginated results.