fix(cli): normalize explicit boolean flag values safely
This commit is contained in:
1 parent
1fb966dbff
commit
d083de5f84
17 files changed
+470
-36
No files matched your search
@@ -0,0 +1,80 @@
|
||||
// Copyright 2026 Alibaba Group
|
||||
// Licensed under the Apache License, Version 2.0 (the "License");
|
||||
// you may not use this file except in compliance with the License.
|
||||
// You may obtain a copy of the License at
|
||||
//
|
||||
// http://www.apache.org/licenses/LICENSE-2.0
|
||||
//
|
||||
// Unless required by applicable law or agreed to in writing, software
|
||||
// distributed under the License is distributed on an "AS IS" BASIS,
|
||||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
// See the License for the specific language governing permissions and
|
||||
// limitations under the License.
|
||||
|
||||
package handlers
|
||||
|
||||
import (
|
||||
"strings"
|
||||
|
||||
"github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/pipeline"
|
||||
"github.com/DingTalk-Real-AI/dingtalk-workspace-cli/pkg/cmdutil"
|
||||
)
|
||||
|
||||
// BoolValueHandler makes an explicit, whitespace-separated boolean value
|
||||
// unambiguous before pflag parsing. pflag interprets `--yes false` as a bare
|
||||
// `--yes` (true) plus a positional `false`; rewriting the pair to
|
||||
// `--yes=false` preserves the value the caller actually supplied.
|
||||
//
|
||||
// This handler runs after all flag-name handlers so camelCase, semantic
|
||||
// aliases, and conservative fuzzy corrections have already reached their
|
||||
// canonical names. It consumes only exact boolean literals and stops at `--`.
|
||||
type BoolValueHandler struct{}
|
||||
|
||||
func (BoolValueHandler) Name() string { return "boolvalue" }
|
||||
func (BoolValueHandler) Phase() pipeline.Phase { return pipeline.PreParse }
|
||||
|
||||
func (BoolValueHandler) Handle(ctx *pipeline.Context) error {
|
||||
if len(ctx.Args) < 2 || len(ctx.FlagSpecs) == 0 {
|
||||
return nil
|
||||
}
|
||||
|
||||
tokens := make(map[string]pipeline.FlagInfo)
|
||||
for _, spec := range ctx.FlagSpecs {
|
||||
if spec.Name == "" || (spec.Type != "bool" && spec.Type != "boolean") {
|
||||
continue
|
||||
}
|
||||
tokens["--"+spec.Name] = spec
|
||||
if spec.Shorthand != "" {
|
||||
tokens["-"+spec.Shorthand] = spec
|
||||
}
|
||||
}
|
||||
|
||||
result := make([]string, 0, len(ctx.Args))
|
||||
for index := 0; index < len(ctx.Args); index++ {
|
||||
argument := ctx.Args[index]
|
||||
if argument == "--" {
|
||||
result = append(result, ctx.Args[index:]...)
|
||||
break
|
||||
}
|
||||
|
||||
spec, matched := tokens[argument]
|
||||
if !matched || ctx.IsFlagProtected(spec.Name) || index+1 >= len(ctx.Args) {
|
||||
result = append(result, argument)
|
||||
continue
|
||||
}
|
||||
normalized, ok := cmdutil.NormalizeBoolLiteral(ctx.Args[index+1])
|
||||
if !ok {
|
||||
result = append(result, argument)
|
||||
continue
|
||||
}
|
||||
|
||||
corrected := "--" + spec.Name + "=" + normalized
|
||||
original := strings.Join(ctx.Args[index:index+2], " ")
|
||||
ctx.AddCorrection("boolvalue", pipeline.PreParse, "--"+spec.Name, original, corrected, "explicit-bool")
|
||||
result = append(result, corrected)
|
||||
index++
|
||||
}
|
||||
|
||||
ctx.Args = result
|
||||
return nil
|
||||
}
|
||||
@@ -0,0 +1,86 @@
|
||||
// Copyright 2026 Alibaba Group
|
||||
// Licensed under the Apache License, Version 2.0 (the "License");
|
||||
// you may not use this file except in compliance with the License.
|
||||
// You may obtain a copy of the License at
|
||||
//
|
||||
// http://www.apache.org/licenses/LICENSE-2.0
|
||||
//
|
||||
// Unless required by applicable law or agreed to in writing, software
|
||||
// distributed under the License is distributed on an "AS IS" BASIS,
|
||||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
// See the License for the specific language governing permissions and
|
||||
// limitations under the License.
|
||||
|
||||
package handlers
|
||||
|
||||
import (
|
||||
"reflect"
|
||||
"testing"
|
||||
|
||||
"github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/pipeline"
|
||||
)
|
||||
|
||||
func TestBoolValueHandler(t *testing.T) {
|
||||
boolFlags := []pipeline.FlagInfo{{Name: "yes", Shorthand: "y", Type: "bool"}}
|
||||
tests := []struct {
|
||||
name string
|
||||
args []string
|
||||
flags []pipeline.FlagInfo
|
||||
protected []string
|
||||
want []string
|
||||
correction bool
|
||||
}{
|
||||
{name: "long false", args: []string{"--yes", "false"}, flags: boolFlags, want: []string{"--yes=false"}, correction: true},
|
||||
{name: "long true synonym", args: []string{"--yes", "yes"}, flags: boolFlags, want: []string{"--yes=true"}, correction: true},
|
||||
{name: "shorthand zero", args: []string{"-y", "0"}, flags: boolFlags, want: []string{"--yes=false"}, correction: true},
|
||||
{name: "preserves following flags", args: []string{"--yes", "off", "--format", "json"}, flags: boolFlags, want: []string{"--yes=false", "--format", "json"}, correction: true},
|
||||
{name: "explicit equals stays native", args: []string{"--yes=false"}, flags: boolFlags, want: []string{"--yes=false"}},
|
||||
{name: "bare bool stays native", args: []string{"--yes"}, flags: boolFlags, want: []string{"--yes"}},
|
||||
{name: "invalid literal is positional", args: []string{"--yes", "maybe"}, flags: boolFlags, want: []string{"--yes", "maybe"}},
|
||||
{name: "non bool flag is unchanged", args: []string{"--name", "false"}, flags: []pipeline.FlagInfo{{Name: "name", Type: "string"}}, want: []string{"--name", "false"}},
|
||||
{name: "unknown flag is unchanged", args: []string{"--confirm", "false"}, flags: boolFlags, want: []string{"--confirm", "false"}},
|
||||
{name: "shorthand cluster is unchanged", args: []string{"-vy", "false"}, flags: boolFlags, want: []string{"-vy", "false"}},
|
||||
{name: "protected bool is unchanged", args: []string{"--yes", "false"}, flags: boolFlags, protected: []string{"yes"}, want: []string{"--yes", "false"}},
|
||||
{name: "stops at double dash", args: []string{"--", "--yes", "false"}, flags: boolFlags, want: []string{"--", "--yes", "false"}},
|
||||
{name: "no specs", args: []string{"--yes", "false"}, want: []string{"--yes", "false"}},
|
||||
}
|
||||
|
||||
for _, test := range tests {
|
||||
t.Run(test.name, func(t *testing.T) {
|
||||
ctx := &pipeline.Context{
|
||||
Args: append([]string(nil), test.args...),
|
||||
FlagSpecs: test.flags,
|
||||
}
|
||||
for _, protected := range test.protected {
|
||||
ctx.ProtectFlag(protected, pipeline.FlagProtectionBlocked)
|
||||
}
|
||||
handler := BoolValueHandler{}
|
||||
if err := handler.Handle(ctx); err != nil {
|
||||
t.Fatalf("Handle() error = %v", err)
|
||||
}
|
||||
if !reflect.DeepEqual(ctx.Args, test.want) {
|
||||
t.Fatalf("Args = %#v, want %#v", ctx.Args, test.want)
|
||||
}
|
||||
wantCorrections := 0
|
||||
if test.correction {
|
||||
wantCorrections = 1
|
||||
}
|
||||
if len(ctx.Corrections) != wantCorrections {
|
||||
t.Fatalf("Corrections = %#v, want %d", ctx.Corrections, wantCorrections)
|
||||
}
|
||||
if test.correction {
|
||||
correction := ctx.Corrections[0]
|
||||
if correction.Handler != "boolvalue" || correction.Kind != "explicit-bool" || correction.Field != "--yes" {
|
||||
t.Fatalf("correction metadata = %#v", correction)
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestBoolValueHandlerMeta(t *testing.T) {
|
||||
handler := BoolValueHandler{}
|
||||
if handler.Name() != "boolvalue" || handler.Phase() != pipeline.PreParse {
|
||||
t.Fatalf("handler metadata = %q/%v", handler.Name(), handler.Phase())
|
||||
}
|
||||
}
|
||||
@@ -21,7 +21,7 @@ import (
|
||||
)
|
||||
|
||||
// TestFullPreParsePipeline exercises the complete PreParse handler
|
||||
// chain: AliasHandler → StickyHandler → ParamNameHandler. It
|
||||
// chain: AliasHandler → StickyHandler → ParamNameHandler → BoolValueHandler. It
|
||||
// simulates a model-generated CLI invocation with multiple errors
|
||||
// and verifies the pipeline corrects all of them in one pass.
|
||||
func TestFullPreParsePipeline(t *testing.T) {
|
||||
@@ -30,6 +30,7 @@ func TestFullPreParsePipeline(t *testing.T) {
|
||||
AliasHandler{},
|
||||
StickyHandler{},
|
||||
ParamNameHandler{},
|
||||
BoolValueHandler{},
|
||||
)
|
||||
|
||||
// Numeric / boolean flag typing matters for the sticky guard. The
|
||||
@@ -106,6 +107,20 @@ func TestFullPreParsePipeline(t *testing.T) {
|
||||
want: "--limit-value 100",
|
||||
corrections: 1, // sticky handles both kebab-normalisation and split
|
||||
},
|
||||
{
|
||||
name: "camelCase bool with detached value",
|
||||
args: []string{"--dryRun", "false"},
|
||||
flags: []pipeline.FlagInfo{{Name: "dry-run", Type: "bool"}},
|
||||
want: "--dry-run=false",
|
||||
corrections: 2, // alias(dryRun) + boolvalue(false)
|
||||
},
|
||||
{
|
||||
name: "fuzzy bool with detached value",
|
||||
args: []string{"--yess", "no"},
|
||||
flags: []pipeline.FlagInfo{{Name: "yes", Type: "bool"}},
|
||||
want: "--yes=false",
|
||||
corrections: 2, // paramname(yess) + boolvalue(no)
|
||||
},
|
||||
|
||||
// Hardening: a mistyped flag whose name happens to start with
|
||||
// a real flag must NOT be split. The pipeline should leave the
|
||||
@@ -177,6 +192,7 @@ func TestFullPreParsePipelineStopsAtDoubleDash(t *testing.T) {
|
||||
SemanticAliasHandler{Lookup: fakeLookup(map[string]string{"keyword": "query"}, nil, nil)},
|
||||
StickyHandler{},
|
||||
ParamNameHandler{},
|
||||
BoolValueHandler{},
|
||||
)
|
||||
ctx := &pipeline.Context{
|
||||
Command: "dws demo cmd",
|
||||
@@ -276,7 +292,7 @@ func TestFullPipelineEndToEnd(t *testing.T) {
|
||||
t.Fatalf("PreParse error: %v", err)
|
||||
}
|
||||
|
||||
want := "--user-id u001 --page-size 50 --verbose true"
|
||||
want := "--user-id u001 --page-size 50 --verbose=true"
|
||||
got := strings.Join(ctx.Args, " ")
|
||||
if got != want {
|
||||
t.Errorf("after PreParse: Args = %q, want %q", got, want)
|
||||
@@ -333,6 +349,7 @@ func TestFullFivePhasePipeline(t *testing.T) {
|
||||
AliasHandler{},
|
||||
StickyHandler{},
|
||||
ParamNameHandler{},
|
||||
BoolValueHandler{},
|
||||
ParamValueHandler{},
|
||||
PreRequestHandler{},
|
||||
PostResponseHandler{},
|
||||
@@ -375,7 +392,7 @@ func TestFullFivePhasePipeline(t *testing.T) {
|
||||
t.Fatalf("PreParse error: %v", err)
|
||||
}
|
||||
|
||||
want := "--user-id u001 --page-size 50 --verbose true"
|
||||
want := "--user-id u001 --page-size 50 --verbose=true"
|
||||
got := strings.Join(ctx.Args, " ")
|
||||
if got != want {
|
||||
t.Errorf("after PreParse: Args = %q, want %q", got, want)
|
||||
@@ -475,6 +492,7 @@ func TestFivePhasePipelineCorrectHandlerCounts(t *testing.T) {
|
||||
AliasHandler{},
|
||||
StickyHandler{},
|
||||
ParamNameHandler{},
|
||||
BoolValueHandler{},
|
||||
ParamValueHandler{},
|
||||
PreRequestHandler{},
|
||||
PostResponseHandler{},
|
||||
@@ -485,7 +503,7 @@ func TestFivePhasePipelineCorrectHandlerCounts(t *testing.T) {
|
||||
want int
|
||||
}{
|
||||
{pipeline.Register, 1},
|
||||
{pipeline.PreParse, 3},
|
||||
{pipeline.PreParse, 4},
|
||||
{pipeline.PostParse, 1},
|
||||
{pipeline.PreRequest, 1},
|
||||
{pipeline.PostResponse, 1},
|
||||
@@ -495,8 +513,8 @@ func TestFivePhasePipelineCorrectHandlerCounts(t *testing.T) {
|
||||
t.Errorf("Handlers(%v) = %d, want %d", tt.phase, got, tt.want)
|
||||
}
|
||||
}
|
||||
if got := engine.HandlerCount(); got != 7 {
|
||||
t.Errorf("HandlerCount = %d, want 7", got)
|
||||
if got := engine.HandlerCount(); got != 8 {
|
||||
t.Errorf("HandlerCount = %d, want 8", got)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -522,6 +540,7 @@ func TestPreParseDoesNotBreakValidArgs(t *testing.T) {
|
||||
AliasHandler{},
|
||||
StickyHandler{},
|
||||
ParamNameHandler{},
|
||||
BoolValueHandler{},
|
||||
)
|
||||
|
||||
original := []string{
|
||||
|
||||
@@ -18,9 +18,9 @@ import (
|
||||
"github.com/DingTalk-Real-AI/dingtalk-workspace-cli/pkg/cmdutil"
|
||||
)
|
||||
|
||||
// StickyHandler detects glued flag-value pairs in raw argv and splits
|
||||
// them into separate tokens. For example, "--limit100" becomes
|
||||
// "--limit", "100" when "limit" is a known flag name.
|
||||
// StickyHandler detects glued flag-value pairs in raw argv and normalizes
|
||||
// them. For example, "--limit100" becomes "--limit", "100" while a boolean
|
||||
// such as "--verbosefalse" becomes the single safe token "--verbose=false".
|
||||
//
|
||||
// The handler only operates on tokens that start with "--" and do not
|
||||
// contain "=". It tries to match the longest known flag name prefix
|
||||
@@ -54,8 +54,14 @@ func (StickyHandler) Handle(ctx *pipeline.Context) error {
|
||||
}
|
||||
split, ok := trySplitSticky(arg, specByName)
|
||||
if ok {
|
||||
ctx.AddCorrection("sticky", pipeline.PreParse, split.flag, arg, split.flag+" "+split.value, "sticky")
|
||||
result = append(result, split.flag, split.value)
|
||||
if split.inline {
|
||||
corrected := split.flag + "=" + split.value
|
||||
ctx.AddCorrection("sticky", pipeline.PreParse, split.flag, arg, corrected, "sticky")
|
||||
result = append(result, corrected)
|
||||
} else {
|
||||
ctx.AddCorrection("sticky", pipeline.PreParse, split.flag, arg, split.flag+" "+split.value, "sticky")
|
||||
result = append(result, split.flag, split.value)
|
||||
}
|
||||
} else {
|
||||
result = append(result, arg)
|
||||
}
|
||||
@@ -66,8 +72,9 @@ func (StickyHandler) Handle(ctx *pipeline.Context) error {
|
||||
}
|
||||
|
||||
type stickyPair struct {
|
||||
flag string
|
||||
value string
|
||||
flag string
|
||||
value string
|
||||
inline bool
|
||||
}
|
||||
|
||||
// trySplitSticky checks if arg looks like a glued flag-value (e.g.
|
||||
@@ -86,7 +93,7 @@ type stickyPair struct {
|
||||
// split to "--page-size", "50".
|
||||
func trySplitSticky(arg string, specByName map[string]pipeline.FlagInfo) (stickyPair, bool) {
|
||||
pair, ok := pipeline.SplitStickyFlag(arg, specByName)
|
||||
return stickyPair{flag: pair.Flag, value: pair.Value}, ok
|
||||
return stickyPair{flag: pair.Flag, value: pair.Value, inline: pair.Inline}, ok
|
||||
}
|
||||
|
||||
// buildFlagSpecIndex creates an index of known flag names (without "--"
|
||||
|
||||
@@ -157,10 +157,24 @@ func TestStickyHandler(t *testing.T) {
|
||||
corrections: 0,
|
||||
},
|
||||
{
|
||||
name: "boolean-like value splits when type is bool",
|
||||
name: "boolean-like value normalizes inline when type is bool",
|
||||
args: []string{"--verbosetrue"},
|
||||
flags: specs(flagSpec{name: "verbose", typ: "bool"}),
|
||||
want: "--verbose true",
|
||||
want: "--verbose=true",
|
||||
corrections: 1,
|
||||
},
|
||||
{
|
||||
name: "confirmation false normalizes to an inline false value",
|
||||
args: []string{"--yesfalse"},
|
||||
flags: specs(flagSpec{name: "yes", typ: "bool"}),
|
||||
want: "--yes=false",
|
||||
corrections: 1,
|
||||
},
|
||||
{
|
||||
name: "model-friendly boolean no normalizes to inline false",
|
||||
args: []string{"--yesno"},
|
||||
flags: specs(flagSpec{name: "yes", typ: "bool"}),
|
||||
want: "--yes=false",
|
||||
corrections: 1,
|
||||
},
|
||||
{
|
||||
|
||||
Reference in new issue
Block a user