From 0ed34ea9f7e95dc030833e40f4328e558061ce62 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sat, 24 Oct 2026 08:47:40 -0400 Subject: [PATCH] fix(mcp): integral numbers keep decimal notation on both transports (BUG-4056) (#2300) * fix(mcp): preserve decimal numeric arguments (BUG-4056) Share numeric formatting across CLI dispatch and HTTP queries, and validate remote chat sequences directly with int64 bounds. Preserve the existing tool surface and wire goldens. Claude-Session: https://claude.ai/code/session_01KYZ6vrKNro953q9kfdSGFd * fix(mcp): numeric array elements, playbook tokens and buildQuery's default share formatNumber (BUG-4056, r1) Lead ruling on r1 (tier A from here): toStringSlice spelled each JSON number element of an array with fmt.Sprint, so a repeatable flag or variadic positional given 1000000 got '1e+06'; playbook raw_args tokens used %v, so a string- or ref-typed argument would bind '1e+06'; buildQuery's default branch kept fmt.Sprint for float32. All three now use formatNumber. Claude-Session: https://claude.ai/code/session_01KYZ6vrKNro953q9kfdSGFd --- cmd/pad/bug4056_mcp_numbers_test.go | 36 +++++++++++ docs/mcp.md | 2 + internal/mcp/bug4056_numbers_test.go | 93 ++++++++++++++++++++++++++++ internal/mcp/catalog_playbook.go | 6 ++ internal/mcp/dispatch.go | 16 ++++- internal/mcp/dispatch_http_chat.go | 13 +++- internal/mcp/dispatch_http_routes.go | 18 +++--- internal/mcp/number.go | 42 +++++++++++++ internal/mcp/number_test.go | 80 ++++++++++++++++++++++++ 9 files changed, 292 insertions(+), 14 deletions(-) create mode 100644 cmd/pad/bug4056_mcp_numbers_test.go create mode 100644 internal/mcp/bug4056_numbers_test.go create mode 100644 internal/mcp/number.go create mode 100644 internal/mcp/number_test.go diff --git a/cmd/pad/bug4056_mcp_numbers_test.go b/cmd/pad/bug4056_mcp_numbers_test.go new file mode 100644 index 000000000..801936d43 --- /dev/null +++ b/cmd/pad/bug4056_mcp_numbers_test.go @@ -0,0 +1,36 @@ +package main + +import ( + "reflect" + "testing" + + "github.com/PerpetualSoftware/pad/internal/cmdhelp" + mcpserver "github.com/PerpetualSoftware/pad/internal/mcp" +) + +func TestBUG4056BuildCLIArgs(t *testing.T) { + root := newRootCmd() + doc := cmdhelp.Build(root, root, cmdhelp.Options{Binary: "pad", MaxDepth: -1}) + for _, tc := range []struct { + name, command string + input map[string]any + want []string + }{ + {"flag", "chat read", map[string]any{"target": "C12", "to": float64(1000000)}, []string{"--to", "1000000", "--format", "json", "--", "C12"}}, + {"positional", "chat delete", map[string]any{"target": "C12", "seq": float64(1000000)}, []string{"--format", "json", "--", "C12", "1000000"}}, + // BUG-4056 r1: a repeatable positional given a JSON array of numbers + // goes through toStringSlice, element by element. + {"repeatable positional", "item bulk-update", map[string]any{"ref": []any{float64(1000000), float64(2.5)}}, []string{"--format", "json", "--", "1000000", "2.5"}}, + } { + t.Run(tc.name, func(t *testing.T) { + cmd, ok := doc.Commands[tc.command] + if !ok { + t.Fatalf("catalog command %q missing from cmdhelp", tc.command) + } + got, err := mcpserver.BuildCLIArgs(cmd, tc.input, "", nil) + if err != nil || !reflect.DeepEqual(got, tc.want) { + t.Fatalf("BuildCLIArgs = %q, %v; want %q", got, err, tc.want) + } + }) + } +} diff --git a/docs/mcp.md b/docs/mcp.md index b09ea65ce..9149ad6c6 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -30,6 +30,8 @@ Surface: Post-0.30 without a bump (BUG-2995): a successful content write through the designated applier used to answer with the item's PREVIOUS content — on that path the markdown goes to a live tab's Y.Doc and the row write runs with `Content` nil, so the response described the item as it stood before the request. It now carries the content as SENT plus an additive `omitempty` `warnings.content_outcome: "applied_pending_flush"`. No bump, on the BUG-2304 rather than the v0.9 grounds: v0.9 bumped because list rows LOST fields consumers read, whereas here nothing is removed or retyped — what changed is the VALUE of one field on one path, which a caller does observe. The claim is that no DOCUMENTED OR SUPPORTED reliance breaks, not that none can exist: a consumer could have detected the applier path by noticing the response echoed something other than what it sent, and that stops working — but it was never documented, a genuinely lost write produced the same mismatch, and `warnings.content_outcome` answers that question properly in the same change. Reads are unchanged: during the window a `get` (or `list` with `full: true`) still answers from the row, so the warning means re-read later, not re-send. +Post-0.102 without a bump (BUG-4056): calls with integral numbers >= 1e6 that were wrongly refused now work. Numeric CLI flags and positionals retain decimal notation, and remote `pad_chat` validates sequences directly from the number. No schema, name, or shape change. + Both are also returned by `pad://_meta/version` and `pad_meta.action: version`. **The wire contract is pinned as golden bytes** (TASK-2306): `cmd/pad/mcp_wire_golden_test.go` compares the `initialize` and `tools/list` results, on BOTH transports, against `cmd/pad/testdata/mcp_wire/*.json`. It uses the production bindings: the real `pad mcp serve` subcommand, and `registerRemoteMCP` behind `NewRemoteTransport`. Any catalog, annotation, schema or `instructions.md` edit moves those bytes. Regenerate with `PAD_UPDATE_MCP_GOLDEN=1 go test ./cmd/pad/ -run TestMCPWireGolden`, and review the testdata diff as the contract diff it is: that diff is where you decide whether the change owes a `ToolSurfaceVersion` bump. A refactor or SDK bump that is meant to be behavior-neutral must leave the files untouched. diff --git a/internal/mcp/bug4056_numbers_test.go b/internal/mcp/bug4056_numbers_test.go new file mode 100644 index 000000000..ae5ea064f --- /dev/null +++ b/internal/mcp/bug4056_numbers_test.go @@ -0,0 +1,93 @@ +package mcp + +import ( + "context" + "encoding/json" + "fmt" + "math" + "net/http" + "net/url" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +func TestBUG4056BuildQueryNumbersUnchanged(t *testing.T) { + for _, tc := range []struct { + name string + in any + want string + }{ + {"integral", float64(1000000), "1000000"}, + {"large integral", float64(1e19), "10000000000000000000"}, + {"fractional", 2.5, "2.5"}, + {"negative zero", math.Copysign(0, -1), "0"}, + {"json number", json.Number("1e6"), "1e6"}, + } { + t.Run(tc.name, func(t *testing.T) { + query := buildQuery(map[string]any{"n": tc.in}, map[string]string{"n": "n"}) + values, err := url.ParseQuery(query) + if err != nil || values.Get("n") != tc.want { + t.Fatalf("buildQuery = %q, %v; want n=%q", query, err, tc.want) + } + }) + } +} + +func TestBUG4056ChatRemoteNumbers(t *testing.T) { + for _, tc := range []struct { + name, verb, field string + in any + invalid bool + }{ + {"million", "read", "to", float64(1000000), false}, + {"string", "read", "to", "1000000", false}, + {"int", "read", "to", int(1000000), false}, + {"int64", "read", "to", int64(1000000), false}, + {"out of range", "delete", "seq", float64(1e19), true}, + {"int64 upper bound", "delete", "seq", float64(math.MaxInt64), true}, + {"fractional", "delete", "seq", 2.5, true}, + {"nan", "delete", "seq", math.NaN(), true}, + {"infinity", "delete", "seq", math.Inf(1), true}, + } { + t.Run(tc.name, func(t *testing.T) { + calls := 0 + d := &HTTPHandlerDispatcher{ + UserResolver: func(context.Context) *models.User { return &models.User{ID: "u"} }, + Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + if tc.invalid { + t.Error("invalid sequence reached handler") + } + if r.Method != http.MethodPost || r.URL.Path != "/api/v1/me/conversations/12/read" { + t.Errorf("unexpected request: %s %s", r.Method, r.URL) + } + var body map[string]json.RawMessage + if err := json.NewDecoder(r.Body).Decode(&body); err != nil || string(body["seq"]) != "1000000" { + t.Errorf("request body = %s, %v; want seq=1000000", body, err) + } + fmt.Fprint(w, `{"read_seq":1000000}`) + }), + } + // @name forces target resolution to make a request if validation + // ever moves after it; invalid input must reach no handler at all. + target := "C12" + if tc.invalid { + target = "@rook" + } + input := map[string]any{"target": target, tc.field: tc.in} + res, err := d.Dispatch(WithDispatchInput(context.Background(), input), []string{"chat", tc.verb}, nil) + if err != nil || res == nil { + t.Fatalf("Dispatch = %v, %v", res, err) + } + if tc.invalid { + if !res.IsError || !strings.Contains(textOf(res), "validation_failed") || !strings.Contains(textOf(res), "seq must be an integer of at least 1") || calls != 0 { + t.Fatalf("want validation error without requests; calls=%d result=%s", calls, textOf(res)) + } + } else if res.IsError || calls != 1 { + t.Fatalf("want successful read request; calls=%d result=%s", calls, textOf(res)) + } + }) + } +} diff --git a/internal/mcp/catalog_playbook.go b/internal/mcp/catalog_playbook.go index 5a036d380..1b3e41fb1 100644 --- a/internal/mcp/catalog_playbook.go +++ b/internal/mcp/catalog_playbook.go @@ -169,6 +169,12 @@ func actionPlaybookRun(ctx context.Context, input map[string]any, env ActionEnv) // Skip — caller signaled "leave unbound"; let the // server's bindPlaybookArgs surface it as unbound. default: + // BUG-4056 r1: a number-typed argument parses '1e+06' fine, + // but a string- or ref-typed one would bind that text. + if number, ok := formatNumber(tv); ok { + tokens = append(tokens, k+"="+number) + continue + } tokens = append(tokens, fmt.Sprintf("%s=%v", k, tv)) } } diff --git a/internal/mcp/dispatch.go b/internal/mcp/dispatch.go index dc500d7f9..743ff4f33 100644 --- a/internal/mcp/dispatch.go +++ b/internal/mcp/dispatch.go @@ -502,6 +502,8 @@ func BuildCLIArgs( return nil, fmt.Errorf("argument %q: %w", arg.Name, err) } positionals = append(positionals, items...) + } else if number, ok := formatNumber(v); ok { + positionals = append(positionals, number) } else { positionals = append(positionals, fmt.Sprint(v)) } @@ -653,6 +655,11 @@ func toStringSlice(v any) ([]string, error) { case []any: out := make([]string, 0, len(t)) for _, e := range t { + // BUG-4056 r1: a JSON number element is a float64 too. + if number, ok := formatNumber(e); ok { + out = append(out, number) + continue + } out = append(out, fmt.Sprint(e)) } return out, nil @@ -663,8 +670,8 @@ func toStringSlice(v any) ([]string, error) { } // encodeFlagValue stringifies an MCP input value for inclusion as a CLI -// flag argument. Strings and primitives pass through fmt.Sprint as before; -// structured values (maps, slices) are JSON-encoded so that flags accepting +// flag argument. Numbers use decimal notation; strings and booleans pass through. +// Structured values (maps, slices) are JSON-encoded so that flags accepting // JSON literals — like `pad collection create --schema ''` — receive // a valid JSON string rather than Go's map-print syntax. // @@ -672,12 +679,15 @@ func toStringSlice(v any) ([]string, error) { // involved; the JSON string travels intact to the subprocess and is parsed // by the CLI flag's own decoder. func encodeFlagValue(v any) (string, error) { + if number, ok := formatNumber(v); ok { + return number, nil + } switch t := v.(type) { case nil: return "", nil case string: return t, nil - case bool, int, int32, int64, uint, uint32, uint64, float32, float64, json.Number: + case bool: return fmt.Sprint(v), nil default: // map[string]any, []any, or any other structured type: JSON-encode. diff --git a/internal/mcp/dispatch_http_chat.go b/internal/mcp/dispatch_http_chat.go index e95984af7..75dabd5d5 100644 --- a/internal/mcp/dispatch_http_chat.go +++ b/internal/mcp/dispatch_http_chat.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "math" "net/http" "net/url" "strconv" @@ -100,7 +101,17 @@ func chatSequence(input map[string]any, name string, min int64) (int64, error) { if !ok { return 0, fmt.Errorf("%s is required", name) } - n, err := strconv.ParseInt(fmt.Sprint(v), 10, 64) + var n int64 + var err error + if x, ok := v.(float64); ok { + // The upper bound is exclusive: float64(MaxInt64) is 2^63. + if math.Trunc(x) != x || x < math.MinInt64 || x >= 0x1p63 { + return 0, fmt.Errorf("%s must be an integer of at least %d", name, min) + } + n = int64(x) + } else { + n, err = strconv.ParseInt(fmt.Sprint(v), 10, 64) + } if err != nil || n < min { return 0, fmt.Errorf("%s must be an integer of at least %d", name, min) } diff --git a/internal/mcp/dispatch_http_routes.go b/internal/mcp/dispatch_http_routes.go index c0ebd86c9..949a4fa32 100644 --- a/internal/mcp/dispatch_http_routes.go +++ b/internal/mcp/dispatch_http_routes.go @@ -153,19 +153,17 @@ func buildQuery(input map[string]any, mapping map[string]string) string { q.Set(dst, "true") } case float64: - // Cheap int detection — JSON parser gives every number as - // float64, but CLI flags in cmdhelp can be "int" type so - // most callers pass whole numbers. Emit without the - // trailing ".0" so the wire format matches the CLI. - if x == float64(int64(x)) { - q.Set(dst, strconv.FormatInt(int64(x), 10)) - } else { - q.Set(dst, strconv.FormatFloat(x, 'f', -1, 64)) - } + number, _ := formatNumber(x) + q.Set(dst, number) case json.Number: q.Set(dst, x.String()) default: - q.Set(dst, fmt.Sprint(v)) + // BUG-4056 r1: float32 and the integer types share formatNumber. + if number, ok := formatNumber(v); ok { + q.Set(dst, number) + } else { + q.Set(dst, fmt.Sprint(v)) + } } } if len(q) == 0 { diff --git a/internal/mcp/number.go b/internal/mcp/number.go new file mode 100644 index 000000000..bc3b77ea0 --- /dev/null +++ b/internal/mcp/number.go @@ -0,0 +1,42 @@ +package mcp + +import ( + "encoding/json" + "math" + "strconv" +) + +// formatNumber formats native numbers without exponents and preserves +// json.Number literals. NaN and infinities keep strconv's spellings so callers +// can apply their own validation, as they did with fmt.Sprint. +func formatNumber(v any) (string, bool) { + var x float64 + switch n := v.(type) { + case float64: + x = n + case float32: + x = float64(n) + case int: + return strconv.Itoa(n), true + case int32: + return strconv.FormatInt(int64(n), 10), true + case int64: + return strconv.FormatInt(n, 10), true + case uint: + return strconv.FormatUint(uint64(n), 10), true + case uint32: + return strconv.FormatUint(uint64(n), 10), true + case uint64: + return strconv.FormatUint(n, 10), true + case json.Number: + return n.String(), true + default: + return "", false + } + // MaxInt64 rounds up to 2^63 as a float64, so the upper bound is + // exclusive. Converting only within range also normalizes -0 to 0. + if math.Trunc(x) == x && x >= math.MinInt64 && x < 0x1p63 { + return strconv.FormatInt(int64(x), 10), true + } + return strconv.FormatFloat(x, 'f', -1, 64), true +} diff --git a/internal/mcp/number_test.go b/internal/mcp/number_test.go new file mode 100644 index 000000000..93a848115 --- /dev/null +++ b/internal/mcp/number_test.go @@ -0,0 +1,80 @@ +package mcp + +import ( + "context" + "encoding/json" + "math" + "strings" + "testing" +) + +func TestFormatNumber(t *testing.T) { + for _, tc := range []struct { + name string + in any + want string + ok bool + }{ + {"million", float64(1000000), "1000000", true}, + {"one and a half million", float64(1500000), "1500000", true}, + {"quadrillion", float64(1e15), "1000000000000000", true}, + {"outside int64", float64(1e19), "10000000000000000000", true}, + {"fractional", 2.5, "2.5", true}, + {"negative", float64(-3), "-3", true}, + {"negative zero", math.Copysign(0, -1), "0", true}, + {"last float below int64 upper bound", math.Nextafter(0x1p63, 0), "9223372036854774784", true}, + {"int64 upper bound", float64(0x1p63), "9223372036854776000", true}, + {"int64 minimum", float64(math.MinInt64), "-9223372036854775808", true}, + {"below int64 minimum", math.Nextafter(-0x1p63, math.Inf(-1)), "-9223372036854778000", true}, + {"float32 integral", float32(1000000), "1000000", true}, + {"float32 fractional", float32(0.1), "0.10000000149011612", true}, + {"float32 outside int64", float32(0x1p64), "18446744073709552000", true}, + {"float32 negative zero", float32(math.Copysign(0, -1)), "0", true}, + {"int", int(7), "7", true}, + {"int32", int32(7), "7", true}, + {"int64", int64(7), "7", true}, + {"uint", uint(7), "7", true}, + {"uint32", uint32(7), "7", true}, + {"uint64", uint64(math.MaxUint64), "18446744073709551615", true}, + {"json number", json.Number("7"), "7", true}, + {"json exponent preserved", json.Number("1e6"), "1e6", true}, + {"nan", math.NaN(), "NaN", true}, + {"positive infinity", math.Inf(1), "+Inf", true}, + {"negative infinity", math.Inf(-1), "-Inf", true}, + {"string", "7", "", false}, + {"bool", true, "", false}, + } { + t.Run(tc.name, func(t *testing.T) { + got, ok := formatNumber(tc.in) + if got != tc.want || ok != tc.ok { + t.Fatalf("formatNumber(%v) = %q, %v; want %q, %v", tc.in, got, ok, tc.want, tc.ok) + } + }) + } +} + +// BUG-4056 r1: array elements and playbook argument tokens share the helper. +func TestBUG4056SliceAndPlaybookNumbers(t *testing.T) { + got, err := toStringSlice([]any{float64(1000000), float64(2.5), "x", true}) + if err != nil || strings.Join(got, ",") != "1000000,2.5,x,true" { + t.Fatalf("toStringSlice = %q, %v", got, err) + } + + disp := &fakeDispatcher{} + env := ActionEnv{Doc: liveCmdhelpDoc(t), Workspace: NewWorkspaceState("docapp"), Dispatcher: disp} + if _, err := padPlaybookTool.Actions["run"](context.Background(), map[string]any{ + "ref": "release", + "args": map[string]any{"n": float64(1000000), "version": float64(1500000)}, + }, env); err != nil { + t.Fatalf("dispatch error: %v", err) + } + joined := strings.Join(disp.gotArgs, " ") + for _, want := range []string{"n=1000000", "version=1500000"} { + if !strings.Contains(joined, want) { + t.Fatalf("playbook tokens %q lack %q", joined, want) + } + } + if strings.Contains(joined, "e+06") { + t.Fatalf("playbook tokens carry exponent text: %q", joined) + } +}