fix: report command-line mistakes as user errors, not internal faults - #2267
Open
evandance wants to merge 1 commit into
Open
fix: report command-line mistakes as user errors, not internal faults#2267evandance wants to merge 1 commit into
evandance wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe CLI now tracks whether failures occur during user-input validation or command execution. It normalizes untyped errors, passes isolated typed snapshots to shutdown hooks, preserves exit-code-only signals, and adds typed positional-argument and credential-provider errors. ChangesDispatch Error Classification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CobraDispatch
participant ErrorStageState
participant RootErrorHandler
participant ShutdownHook
participant ErrorRenderer
CobraDispatch->>ErrorStageState: reset and mark validation or command-body stage
CobraDispatch->>RootErrorHandler: return dispatch error
RootErrorHandler->>RootErrorHandler: normalize error by stage
RootErrorHandler->>ShutdownHook: provide cloned typed error snapshot
RootErrorHandler->>ErrorRenderer: render classified error
ErrorRenderer-->>RootErrorHandler: report envelope serialization failure
RootErrorHandler->>ErrorRenderer: render rebuilt typed error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
github-actions
Bot
added
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/error_stage_test.go (1)
316-318: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact exit code.
rebuildTypedErrorwithstageCommandBodyproduces an internal error, so the exit code is deterministic. Compare againstoutput.ExitInternalinstead of only excluding 1.♻️ Proposed stronger assertion
- if exit == 1 { - t.Errorf("exit = 1; the typed category's exit code must survive the rewrite") - } + if exit != int(output.ExitInternal) { + t.Errorf("exit = %d, want %d; the typed category's exit code must survive the rewrite", + exit, int(output.ExitInternal)) + }🤖 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 `@cmd/error_stage_test.go` around lines 316 - 318, Update the assertion in the rebuildTypedError test to require exit to equal output.ExitInternal, replacing the current check that only rejects 1. Keep the existing error message context while asserting the deterministic internal-error exit code produced with stageCommandBody.
🤖 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.go`:
- Around line 1238-1241: Update the validation error construction in the
positional-argument handling path to pass args[0] to the %q formatter instead of
the full args slice. Keep WithParam(args[0]) and the existing hint unchanged so
the message and parameter consistently report the first stray argument.
---
Nitpick comments:
In `@cmd/error_stage_test.go`:
- Around line 316-318: Update the assertion in the rebuildTypedError test to
require exit to equal output.ExitInternal, replacing the current check that only
rejects 1. Keep the existing error message context while asserting the
deterministic internal-error exit code produced with stageCommandBody.
🪄 Autofix
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 Plus
Run ID: f48227e1-359c-4a58-937e-01c00f04c684
📒 Files selected for processing (11)
cmd/build.gocmd/error_stage.gocmd/error_stage_test.gocmd/plugin_integration_test.gocmd/root.gocmd/root_integration_test.gocmd/root_test.goextension/platform/README.mdextension/platform/lifecycle.gointernal/cmdutil/factory.goshortcuts/common/runner.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@682bfbaf8b5be0183afd250313f108ac5401a452🧩 Skill updatenpx skills add larksuite/cli#refactor/error-classification -y -g |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2267 +/- ##
==========================================
+ Coverage 76.36% 76.39% +0.02%
==========================================
Files 1011 1013 +2
Lines 111269 111529 +260
==========================================
+ Hits 84970 85201 +231
- Misses 19815 19835 +20
- Partials 6484 6493 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
evandance
force-pushed
the
refactor/error-classification
branch
from
August 10, 2026 10:44
57d80e1 to
b929eaa
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_args_test.go`:
- Around line 46-60: Extend the validation-error assertions in the runner
argument test using errs.ProblemOf(err) to verify Category is
errs.CategoryValidation and Subtype is errs.SubtypeInvalidArgument. Retain
errors.As into *errs.ValidationError and the existing Param assertion for the
first stray word, since ProblemOf does not expose Param.
🪄 Autofix
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 Plus
Run ID: c5f45baf-4411-4997-83b4-92b1250bbc00
📒 Files selected for processing (1)
shortcuts/common/runner_args_test.go
evandance
force-pushed
the
refactor/error-classification
branch
from
August 10, 2026 10:54
b929eaa to
ccb620a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/error_stage_test.go (1)
241-249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProbe more than one rejected argument shape.
This walk only detects validators that reject ten arguments. A lower-bound validator, or a custom validator that rejects an empty or single-argument input, can remain unwrapped without failing this test. Probe empty, single, and oversized argument lists, then require typed errors for every rejected probe.
🤖 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 `@cmd/error_stage_test.go` around lines 241 - 249, Expand the argument-validation probes in the walk around c.Args to test empty, single-argument, and oversized argument lists instead of only ten arguments. For every rejected probe, increment the check and require errs.ProblemOf(err) to succeed, recording c.CommandPath() as unguarded when it does not; preserve the existing traversal behavior for accepted inputs.
🤖 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/error_stage_test.go`:
- Around line 359-369: Update the test around normalizeRootError to store the
raw errors passed for stageUserInput and stageCommandBody, then assert errors.Is
for each normalized result against its corresponding raw error. Keep the
existing ProblemOf classification checks and ensure both normalization paths
verify cause preservation.
- Around line 107-120: Update the test around the tamper plugin’s Shutdown
handler to maintain an invocation counter, increment it inside the handler, and
assert after executeWithCapturedOS returns that the counter equals one. Preserve
the existing error-rewrite behavior and exit-code assertions.
- Around line 319-321: Update the assertion in the error-stage test to compare
exit against output.ExitCodeOf(broken), ensuring the rewritten error preserves
the typed category’s exact exit code rather than merely checking that it is not
1.
---
Nitpick comments:
In `@cmd/error_stage_test.go`:
- Around line 241-249: Expand the argument-validation probes in the walk around
c.Args to test empty, single-argument, and oversized argument lists instead of
only ten arguments. For every rejected probe, increment the check and require
errs.ProblemOf(err) to succeed, recording c.CommandPath() as unguarded when it
does not; preserve the existing traversal behavior for accepted inputs.
🪄 Autofix
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 Plus
Run ID: 70984ba7-459e-4504-bfcc-ac6047d8a3f1
📒 Files selected for processing (1)
cmd/error_stage_test.go
Getting a flag wrong on a shortcut told the user the tool had broken. Leaving out one of a pair of mutually required flags, passing both when only one is allowed, or typing a stray word where a flag belongs all came back as an internal fault with the exit code reserved for "this should never happen" — so the most common way to mistype a command counted against the CLI's own health instead of telling the user what to fix. The cause was that classification guessed from the text of the error. Cobra reports these failures as plain sentences, and the dispatcher matched them against a list of known phrasings; anything unrecognized was assumed to be our fault. Any check cobra gained, any wording it changed, and any validator added here landed outside that list silently. Classification now follows where the failure happened instead of what it says. Cobra finishes validating the command line before a command body runs, so a failure before that point is about what was typed, and an unclassified failure after it is a gap on our side. Positional-argument rejections are typed where they are produced and now name the offending word and point at --help. A plugin's shutdown handler also sees the same classification the user gets, and receives a copy of it, so watching failures cannot alter them.
evandance
force-pushed
the
refactor/error-classification
branch
from
August 10, 2026 11:20
ccb620a to
682bfba
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 `@cmd/error_stage_test.go`:
- Around line 244-257: Update the rejecting-validator assertions in the
forEachCommand test to require errs.CategoryValidation and
errs.SubtypeInvalidArgument, rather than accepting any errs.Problem. When err
can be unwrapped as *errs.ValidationError, use errors.As to assert its Param
field as required; preserve the existing tracking of unclassified errors and
command paths.
🪄 Autofix
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 Plus
Run ID: 7b013e74-09f6-49a7-a03a-120400ba3ccf
📒 Files selected for processing (2)
cmd/error_stage_test.goshortcuts/common/runner_args_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/common/runner_args_test.go
Comment on lines
+244
to
+257
| forEachCommand(root, func(c *cobra.Command) { | ||
| if c.Args == nil { | ||
| return | ||
| } | ||
| // Feed enough positional words that any bounded validator rejects them. | ||
| err := c.Args(c, []string{"stray1", "stray2", "stray3", "stray4", "stray5", | ||
| "stray6", "stray7", "stray8", "stray9", "stray10"}) | ||
| if err == nil { | ||
| return | ||
| } | ||
| checked++ | ||
| if _, ok := errs.ProblemOf(err); !ok { | ||
| unguarded = append(unguarded, c.CommandPath()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the validation classification for every rejecting Args validator.
Lines 255-257 accept any typed errs.Problem. An errs.CategoryInternal error would pass this test and hide the dispatch regression.
Assert errs.CategoryValidation and errs.SubtypeInvalidArgument for each rejecting validator. Also assert Param through errors.As to *errs.ValidationError when the validator returns that type.
As per coding guidelines, error-path tests must assert typed metadata and cause preservation. Based on learnings, errs.ProblemOf does not expose Param; use errors.As to inspect *errs.ValidationError.Param.
🤖 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 `@cmd/error_stage_test.go` around lines 244 - 257, Update the
rejecting-validator assertions in the forEachCommand test to require
errs.CategoryValidation and errs.SubtypeInvalidArgument, rather than accepting
any errs.Problem. When err can be unwrapped as *errs.ValidationError, use
errors.As to assert its Param field as required; preserve the existing tracking
of unclassified errors and command paths.
Sources: Coding guidelines, Learnings
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
Getting a command line wrong could report an internal fault. Omitting one of a pair of mutually required flags, passing both when only one is allowed, or typing a stray word where a flag belongs all came back as
internal/unknownwith exit 5 — the code reserved for "this should never happen" — so the most common way to mistype a command counted against the CLI's own health instead of telling the user what to fix.sheets +csv-putand every+verbshortcut were affected.The cause was that classification guessed from the error's text: cobra reports these failures as plain sentences, the dispatcher matched them against a list of known phrasings, and anything unrecognized was assumed to be our own fault. Any check cobra gained, any wording it changed, and any validator added here fell outside that list silently.
Classification now follows where the failure happened rather than what it says, and the text list is gone. User-visible output is unchanged everywhere except the four cases being fixed.
Changes
--help, which it previously did not do at all.Argsvalidator, so a validator added later is covered without its author having to register anything.Shutdownhandler receives the same classification the user gets, as a copy — reading a failure can no longer alter the envelope or the exit code.Shutdown, and show how to readErrsafely.Test Plan
Argsvalidator shape, both flag-group modes, shortcut positional args, required flags, unknown command/subcommand, flag-parse errors, bootstrap and completion bypasses, help, exit-code-only signals): 27 byte-identical toorigin/main, and the only 4 that changed are the ones being fixedArgsvalidator returns an unclassified errorRelated Issues
Summary by CodeRabbit
Bug Fixes
Documentation