From b2bdba52215cc1a1e20d5e455424a2ff3ab60533 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=8E=89=E6=BE=9C?= Date: Wed, 10 Jun 2026 09:49:35 +0800 Subject: [PATCH] fix(cli): skip reserved flag names in canonical MCP command generator MCP tools whose inputSchema contains properties named "params", "json", "format", "timeout", "yes", or any other name that collides with built-in or inherited persistent flags cause a pflag panic ("flag redefined") when the canonical command tree is constructed. The compat/overlay path already filters "json" and "params", but the canonical path in applyFlagSpecs did not. This adds a dynamic collision check using cmd.Flags().Lookup so any future persistent flag additions are automatically covered. Colliding alias names are also skipped. Users can still pass these values via --json '{"params":"..."}'. Co-Authored-By: Claude Opus 4.6 (1M context) --- internal/cli/canonical.go | 10 +++++ internal/cli/canonical_test.go | 81 ++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+) diff --git a/internal/cli/canonical.go b/internal/cli/canonical.go index 1ddbf4f0..22ad5248 100644 --- a/internal/cli/canonical.go +++ b/internal/cli/canonical.go @@ -544,10 +544,20 @@ func applyFlagSpecs(cmd *cobra.Command, specs []FlagSpec) { if primary == "" { continue } + // Skip schema properties whose derived flag name collides with an + // already-registered flag (built-in --json/--params or inherited + // persistent flags like --format/--output/--timeout/--yes). Users + // can still pass these values via --json '{"params":"..."}'. + if cmd.Flags().Lookup(primary) != nil || cmd.InheritedFlags().Lookup(primary) != nil { + continue + } alias := strings.TrimSpace(spec.Alias) if alias == primary { alias = "" } + if alias != "" && (cmd.Flags().Lookup(alias) != nil || cmd.InheritedFlags().Lookup(alias) != nil) { + alias = "" + } switch spec.Kind { case flagString, flagJSON: diff --git a/internal/cli/canonical_test.go b/internal/cli/canonical_test.go index 2bd5e0b6..5a85d32a 100644 --- a/internal/cli/canonical_test.go +++ b/internal/cli/canonical_test.go @@ -72,6 +72,87 @@ func TestBuildFlagSpecsGeneratesOnlySupportedTopLevelFlags(t *testing.T) { } } +func TestApplyFlagSpecsSkipsReservedAndInheritedFlags(t *testing.T) { + t.Parallel() + + // Simulate the root command with persistent flags (format, yes, timeout) + // and a tool command with built-in --json/--params. + root := &cobra.Command{Use: "dws"} + root.PersistentFlags().StringP("format", "f", "json", "output format") + root.PersistentFlags().BoolP("yes", "y", false, "skip confirmation") + root.PersistentFlags().Int("timeout", 30, "HTTP timeout") + + cmd := &cobra.Command{Use: "chat-permission-grant"} + root.AddCommand(cmd) + cmd.Flags().String("json", "", "JSON payload") + cmd.Flags().String("params", "", "additional JSON") + + // MCP schema that includes fields colliding with reserved names. + specs := BuildFlagSpecs(map[string]any{ + "type": "object", + "properties": map[string]any{ + "params": map[string]any{"type": "string", "description": "命令授权参数"}, + "json": map[string]any{"type": "string", "description": "some json field"}, + "format": map[string]any{"type": "string", "description": "output format"}, + "yes": map[string]any{"type": "boolean", "description": "confirm"}, + "timeout": map[string]any{"type": "integer", "description": "timeout sec"}, + "agentCode": map[string]any{"type": "string", "description": "agent code"}, + "scope": map[string]any{"type": "string", "description": "auth scope"}, + }, + }, nil) + + // applyFlagSpecs must not panic — previously this panicked with + // "flag redefined: params" because reserved names were not skipped. + applyFlagSpecs(cmd, specs) + + // Reserved/inherited flags should NOT be re-registered; only + // non-conflicting business flags should appear. + if cmd.Flags().Lookup("agentCode") == nil { + t.Error("expected --agentCode flag to be registered") + } + if cmd.Flags().Lookup("scope") == nil { + t.Error("expected --scope flag to be registered") + } + + // Verify the reserved names were skipped (still point to the originals). + jsonFlag := cmd.Flags().Lookup("json") + if jsonFlag == nil || jsonFlag.Usage != "JSON payload" { + t.Error("--json should be the original built-in flag, not re-registered from schema") + } + paramsFlag := cmd.Flags().Lookup("params") + if paramsFlag == nil || paramsFlag.Usage != "additional JSON" { + t.Error("--params should be the original built-in flag, not re-registered from schema") + } +} + +func TestApplyFlagSpecsSkipsCollidingAlias(t *testing.T) { + t.Parallel() + + root := &cobra.Command{Use: "dws"} + root.PersistentFlags().StringP("format", "f", "json", "output format") + + cmd := &cobra.Command{Use: "test-tool"} + root.AddCommand(cmd) + cmd.Flags().String("json", "", "JSON payload") + cmd.Flags().String("params", "", "additional JSON") + + specs := []FlagSpec{ + {PropertyName: "myField", FlagName: "my-field", Alias: "format", Kind: flagString, Description: "alias collides with inherited --format"}, + } + + applyFlagSpecs(cmd, specs) + + if cmd.Flags().Lookup("my-field") == nil { + t.Error("expected --my-field flag to be registered") + } + // The alias "format" collides with the inherited persistent flag, + // so it should NOT be registered as a hidden alias. + formatFlag := cmd.InheritedFlags().Lookup("format") + if formatFlag == nil || formatFlag.Usage != "output format" { + t.Error("--format should remain the inherited persistent flag") + } +} + func TestFixtureLoaderLoadsCatalog(t *testing.T) { t.Parallel()