fix(dynamic): send booleans the caller set to false instead of dropping them (#19)

Dynamic commands only forwarded a boolean when it was true:

    for propName, val := range boolFlags {
        if val != nil && *val { toolArgs[propName] = true }
    }

Cobra parses --flag=false and records it as Changed, but the value was
then discarded, making an explicit false byte-identical to omitting the
flag. False could not be transmitted at all, so any tool parameter whose
server-side default is true was stuck on. 15 such tool/flag pairs exist
across the Meta and Google caches, and they failed in both directions:

  - delete_catalog_products --dry-run=false stayed in dry run, so the
    tool could never execute a deletion through a dynamic command.
  - publish_facebook_page_post --published=false, create_collection_ad
    and create_instant_experience --is-published=false silently produced
    a LIVE published object for a caller who asked for a draft.
  - create_google_ads_campaign --target-youtube=false could not opt out
    of a network.

Key off cmd.Flags().Changed() and forward the parsed value, so an unset
boolean is still omitted and the server default applies. Both the flag
registration and the change lookup now go through propNameToFlagName, so
the underscore-to-hyphen mapping cannot drift between them.

Argument building is extracted into buildToolArgs and flag registration
into registerToolFlags so the behaviour is unit-testable without network
I/O; the new tests fail against the previous implementation.

Author: nictuku

cmd/dynamic.go

@@ -140,6 +140,13 @@ func (p PropertySchema) resolveType() string {
 	return "string"
 }
 
+// propNameToFlagName maps a JSON Schema property name to its CLI flag name.
+// Both flag registration and argument building go through this, so the two can
+// never disagree about what a property's flag is called.
+func propNameToFlagName(propName string) string {
+	return strings.ReplaceAll(propName, "_", "-")
+}
+
 func buildToolCommand(server ServerConfig, tool client.ToolDefinition) *cobra.Command {
 	cmdName := toolNameToCommandName(tool.Name, server.ToolPrefix)
 
@@ -148,6 +155,24 @@ func buildToolCommand(server ServerConfig, tool client.ToolDefinition) *cobra.Co
 		_ = json.Unmarshal(tool.InputSchema, &schema)
 	}
 
+	cmd := &cobra.Command{
+		Use:   cmdName,
+		Short: truncate(tool.Description, 80),
+		Long:  tool.Description,
+	}
+
+	stringFlags, boolFlags := registerToolFlags(cmd, schema)
+
+	cmd.RunE = func(cmd *cobra.Command, args []string) error {
+		return runDynamicTool(cmd, server, tool.Name, schema, stringFlags, boolFlags)
+	}
+
+	return cmd
+}
+
+// registerToolFlags declares one flag per JSON Schema property and returns the
+// storage those flags parse into, keyed by property name.
+func registerToolFlags(cmd *cobra.Command, schema InputSchema) (map[string]*string, map[string]*bool) {
 	requiredSet := make(map[string]bool)
 	for _, r := range schema.Required {
 		requiredSet[r] = true
@@ -157,17 +182,8 @@ func buildToolCommand(server ServerConfig, tool client.ToolDefinition) *cobra.Co
 	stringFlags := make(map[string]*string)
 	boolFlags := make(map[string]*bool)
 
-	cmd := &cobra.Command{
-		Use:   cmdName,
-		Short: truncate(tool.Description, 80),
-		Long:  tool.Description,
-		RunE: func(cmd *cobra.Command, args []string) error {
-			return runDynamicTool(server, tool.Name, schema, stringFlags, boolFlags)
-		},
-	}
-
 	for propName, prop := range schema.Properties {
-		flagName := strings.ReplaceAll(propName, "_", "-")
+		flagName := propNameToFlagName(propName)
 		desc := prop.Description
 		if len(prop.Enum) > 0 {
 			desc += " (options: " + strings.Join(prop.Enum, ", ") + ")"
@@ -186,10 +202,10 @@ func buildToolCommand(server ServerConfig, tool client.ToolDefinition) *cobra.Co
 		}
 	}
 
-	return cmd
+	return stringFlags, boolFlags
 }
 
-func runDynamicTool(server ServerConfig, toolName string, schema InputSchema, stringFlags map[string]*string, boolFlags map[string]*bool) error {
+func runDynamicTool(cmd *cobra.Command, server ServerConfig, toolName string, schema InputSchema, stringFlags map[string]*string, boolFlags map[string]*bool) error {
 	token, err := getToken()
 	if err != nil {
 		return err
@@ -202,7 +218,20 @@ func runDynamicTool(server ServerConfig, toolName string, schema InputSchema, st
 		return fmt.Errorf("handshake failed: %w", err)
 	}
 
-	// Build arguments from flag values, coercing types based on schema
+	toolArgs := buildToolArgs(cmd, schema, stringFlags, boolFlags)
+
+	fmt.Fprintf(os.Stderr, "Calling %s...\n", toolName)
+	result, err := c.CallTool(server.Path, toolName, toolArgs)
+	if err != nil {
+		return fmt.Errorf("tool call failed: %w", err)
+	}
+
+	return printToolResult(result)
+}
+
+// buildToolArgs turns parsed flag values into the tools/call arguments object,
+// coercing each one to the type its JSON Schema declares.
+func buildToolArgs(cmd *cobra.Command, schema InputSchema, stringFlags map[string]*string, boolFlags map[string]*bool) map[string]interface{} {
 	toolArgs := make(map[string]interface{})
 
 	for propName, val := range stringFlags {
@@ -235,19 +264,24 @@ func runDynamicTool(server ServerConfig, toolName string, schema InputSchema, st
 		}
 	}
 
+	// Send a boolean whenever the caller actually typed the flag, including
+	// `--flag=false`. Forwarding only `true` made an explicit false
+	// indistinguishable from omitting the flag, so any tool param whose
+	// server-side default is true could never be turned off — e.g.
+	// `delete_catalog_products --dry-run=false` stayed in dry run and could
+	// never execute, and `publish_facebook_page_post --published=false`
+	// silently published a post the caller asked to keep as a draft.
 	for propName, val := range boolFlags {
-		if val != nil && *val {
-			toolArgs[propName] = true
+		if val == nil {
+			continue
 		}
+		if cmd != nil && !cmd.Flags().Changed(propNameToFlagName(propName)) {
+			continue
+		}
+		toolArgs[propName] = *val
 	}
 
-	fmt.Fprintf(os.Stderr, "Calling %s...\n", toolName)
-	result, err := c.CallTool(server.Path, toolName, toolArgs)
-	if err != nil {
-		return fmt.Errorf("tool call failed: %w", err)
-	}
-
-	return printToolResult(result)
+	return toolArgs
 }
 
 // printToolResult extracts text content from the MCP tools/call response and prints it.

cmd/dynamic_test.go

@@ -5,6 +5,7 @@ import (
 	"testing"
 
 	"github.com/pipeboard-co/pipeboard-cli/internal/client"
+	"github.com/spf13/cobra"
 )
 
 func TestToolNameToCommandName(t *testing.T) {
@@ -170,3 +171,140 @@ func containsSubstr(s, sub string) bool {
 	}
 	return false
 }
+
+// parseToolArgs registers the schema's flags on a throwaway command, parses
+// argv through the same cobra machinery the real command uses, and returns the
+// arguments object that would be sent to tools/call.
+func parseToolArgs(t *testing.T, rawSchema string, argv ...string) map[string]interface{} {
+	t.Helper()
+
+	var schema InputSchema
+	if err := json.Unmarshal([]byte(rawSchema), &schema); err != nil {
+		t.Fatalf("unmarshal schema: %v", err)
+	}
+
+	cmd := &cobra.Command{Use: "test"}
+	stringFlags, boolFlags := registerToolFlags(cmd, schema)
+
+	if err := cmd.Flags().Parse(argv); err != nil {
+		t.Fatalf("parse %v: %v", argv, err)
+	}
+
+	return buildToolArgs(cmd, schema, stringFlags, boolFlags)
+}
+
+// A boolean the caller explicitly set to false must reach the server as false.
+// Sending only `true` made `--flag=false` indistinguishable from omitting the
+// flag, so any tool param whose server-side default is true could never be
+// turned off — delete_catalog_products could never leave dry run, and
+// publish_facebook_page_post --published=false published a post regardless.
+func TestBuildToolArgsBooleans(t *testing.T) {
+	schema := `{
+		"type": "object",
+		"properties": {
+			"catalog_id": {"type": "string"},
+			"dry_run": {"type": "boolean"},
+			"readback": {"type": "boolean"}
+		}
+	}`
+
+	tests := []struct {
+		name string
+		argv []string
+		want map[string]interface{}
+	}{
+		{
+			name: "explicit false is sent, not dropped",
+			argv: []string{"--catalog-id", "123", "--dry-run=false"},
+			want: map[string]interface{}{"catalog_id": "123", "dry_run": false},
+		},
+		{
+			name: "explicit true is sent",
+			argv: []string{"--dry-run=true"},
+			want: map[string]interface{}{"dry_run": true},
+		},
+		{
+			name: "bare flag means true",
+			argv: []string{"--dry-run"},
+			want: map[string]interface{}{"dry_run": true},
+		},
+		{
+			name: "unset booleans are omitted so the server default applies",
+			argv: []string{"--catalog-id", "123"},
+			want: map[string]interface{}{"catalog_id": "123"},
+		},
+		{
+			name: "each boolean is tracked independently",
+			argv: []string{"--dry-run=false"},
+			want: map[string]interface{}{"dry_run": false},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := parseToolArgs(t, schema, tc.argv...)
+			if len(got) != len(tc.want) {
+				t.Fatalf("got %v, want %v", got, tc.want)
+			}
+			for k, want := range tc.want {
+				if got[k] != want {
+					t.Errorf("arg %q = %v (%T), want %v (%T)", k, got[k], got[k], want, want)
+				}
+			}
+		})
+	}
+}
+
+// Underscored properties are exposed as hyphenated flags; the change-tracking
+// lookup must use the same mapping or every boolean silently drops out again.
+func TestBuildToolArgsHyphenatedBooleanFlag(t *testing.T) {
+	schema := `{
+		"type": "object",
+		"properties": {"is_published": {"type": "boolean"}}
+	}`
+
+	got := parseToolArgs(t, schema, "--is-published=false")
+	if v, ok := got["is_published"]; !ok || v != false {
+		t.Errorf("is_published = %v (present=%v), want false", v, ok)
+	}
+}
+
+// Non-boolean flags keep their schema-declared types.
+func TestBuildToolArgsTypeCoercion(t *testing.T) {
+	schema := `{
+		"type": "object",
+		"properties": {
+			"name": {"type": "string"},
+			"limit": {"type": "integer"},
+			"ratio": {"type": "number"},
+			"product_ids": {"type": "array"},
+			"filter": {"type": "object"}
+		}
+	}`
+
+	got := parseToolArgs(t, schema,
+		"--name", "hello",
+		"--limit", "42",
+		"--ratio", "1.5",
+		"--product-ids", `["a","b"]`,
+		"--filter", `{"k":"v"}`,
+	)
+
+	if got["name"] != "hello" {
+		t.Errorf("name = %v, want hello", got["name"])
+	}
+	if got["limit"] != 42 {
+		t.Errorf("limit = %v (%T), want 42 (int)", got["limit"], got["limit"])
+	}
+	if got["ratio"] != 1.5 {
+		t.Errorf("ratio = %v (%T), want 1.5 (float64)", got["ratio"], got["ratio"])
+	}
+	ids, ok := got["product_ids"].([]interface{})
+	if !ok || len(ids) != 2 || ids[0] != "a" {
+		t.Errorf("product_ids = %v (%T), want [a b]", got["product_ids"], got["product_ids"])
+	}
+	filter, ok := got["filter"].(map[string]interface{})
+	if !ok || filter["k"] != "v" {
+		t.Errorf("filter = %v (%T), want map[k:v]", got["filter"], got["filter"])
+	}
+}