fix: classify invalid CLI input as validation errors - #2267
Open
evandance wants to merge 1 commit into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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@e829dd6ad2d9f4dd7374dd56efd733c247f44a9e🧩 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.43% 76.45% +0.02%
==========================================
Files 1046 1047 +1
Lines 115162 115251 +89
==========================================
+ Hits 88022 88114 +92
+ Misses 20377 20374 -3
Partials 6763 6763 ☔ 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
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
evandance
force-pushed
the
refactor/error-classification
branch
5 times, most recently
from
August 11, 2026 11:40
a7ec4d7 to
52390c3
Compare
evandance
changed the title
evandance
force-pushed
the
refactor/error-classification
branch
3 times, most recently
from
August 17, 2026 07:23
7be353d to
9506ad9
Compare
Cobra surfaced several command-line validation failures as plain errors. Classifying them by message text could report correctable input as internal/unknown, misleading agents and returning the wrong exit code. Classify errors at the boundary that produces them. Args and residual Cobra validation become validation/invalid_argument, while raw execution hooks and plugin failures become internal/unknown. Preserve typed errors, causes, bare exits, and partial failures. Make final-tree instrumentation stateless, type pre-callback framework failures at their source, and keep rendering and Shutdown lifecycle observations consistent. A Shutdown handler receives the error the command returned with its wrapping intact, so errors.Is still reaches the producer's sentinels. Add regression coverage for repeated execution, late help, lazy completion, writer failures, shortcut diagnostics, credential-provider classification, and lifecycle isolation.
evandance
force-pushed
the
refactor/error-classification
branch
from
August 17, 2026 11:50
9506ad9 to
e829dd6
Compare
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
Invalid CLI input surfaced by Cobra could be emitted as
internal/unknown(exit 5), telling agents the CLI failed when they only needed to fix the command. Classification depended on error wording, so new validators and Cobra wording changes could silently be misclassified. This PR classifies errors at their source and keeps that result consistent across rendering and lifecycle hooks.Changes
Args, command discovery, required/group flags, and lazy completion argumentsvalidation/invalid_argumentPersistentPreRunE,PreRunE,RunE,PostRunE,PersistentPostRunE, and plugin wrappersinternal/unknown--versionwritesinternal/unknownBareError, andPartialFailureErrorExecutecalls and separate command trees cannot contaminate one another.paramstays unset, because a shortcut declares no positional parameter for a stray word to belong to.Shutdown, and give each lifecycle handler its own copy of the error where the SDK owns the value; a wrapped chain is shared whole soerrors.Isanderrors.Asstill reach the producer's sentinels.Test Plan
Argsand all five error-returning execution hooks classify by origin and preserve their causes.__complete, plugin short-circuits,--versionwrite failures, and silent short writes.BareError,PartialFailureError, wrapped chains, and extension-defined typed errors.make unit-test,make vet,make fmt-check,go mod tidy, golangci-lint, repository contract lint, lint module tests,make build, andmake quality-gate.Related Issues