fix(mcp): a fields number sent through MCP keeps its literal, so an integer above 2^53 is no longer rounded by mcp-go's argument decode (BUG-3217) (#1565)

* fix(mcp): a fields number sent through MCP keeps its literal, so an integer above 2^53 is no longer rounded by mcp-go's argument decode (BUG-3217)

* docs(mcp): bump the tool surface to 0.54 for the fields-number conflict refusal (BUG-3217)

internal/mcp/bug3217_fields_number_literals_test.go

@@ -0,0 +1,159 @@
+package mcp
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"strings"
+	"testing"
+
+	"github.com/PerpetualSoftware/pad/internal/models"
+	"github.com/PerpetualSoftware/pad/internal/server"
+	"github.com/PerpetualSoftware/pad/internal/store/storetest"
+	mcpserver "github.com/mark3labs/mcp-go/server"
+)
+
+// BUG-3217. mcp-go decodes a tool call's arguments with a plain
+// json.Unmarshal, so a `fields` number reached the dispatcher as a float64:
+// 9007199254740993 was stored as 9007199254740992, and 1.10 as 1.1. These
+// tests send RAW JSON-RPC through a real mcp-go server, because a test that
+// builds the arguments map itself hands the handler a value mcp-go never
+// would and cannot see the decode at all.
+
+func toolsCall(id int, args string) []byte {
+	return []byte(fmt.Sprintf(`{"jsonrpc":"2.0","id":%d,"method":"tools/call","params":{"name":"pad_item","arguments":%s}}`, id, args))
+}
+
+func respText(t *testing.T, resp any) string {
+	t.Helper()
+	b, err := json.Marshal(resp)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return string(b)
+}
+
+// TestMCPFieldsKeepNumberLiterals_Remote drives the remote door: catalog →
+// HTTPHandlerDispatcher → the real handler chain → the store, and reads the
+// stored bytes back. The 12345 row is the control: it round-tripped before
+// the fix too, so a failure there is the harness, not the defect.
+func TestMCPFieldsKeepNumberLiterals_Remote(t *testing.T) {
+	s := storetest.NewSQLite(t)
+	srv := server.New(s)
+	t.Cleanup(srv.Stop)
+	owner, err := s.CreateUser(models.UserCreate{Email: "o@example.com", Name: "O", Password: "correct-horse-battery-staple"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "W", Slug: "w3217", OwnerID: owner.ID})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := s.AddWorkspaceMember(ws.ID, owner.ID, "owner"); err != nil {
+		t.Fatal(err)
+	}
+	coll, err := s.CreateCollection(ws.ID, models.CollectionCreate{Name: "Tasks", Slug: "tasks", Prefix: "TASK",
+		Schema: `{"fields":[{"key":"n","type":"number"},{"key":"j","type":"json"}]}`})
+	if err != nil {
+		t.Fatal(err)
+	}
+	item, err := s.CreateItem(ws.ID, coll.ID, models.ItemCreate{Title: "x"})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	m := mcpserver.NewMCPServer("t", "1", mcpserver.WithToolCapabilities(true))
+	d := &HTTPHandlerDispatcher{Handler: srv, UserResolver: func(context.Context) *models.User { return owner }}
+	if _, err := RegisterCatalog(m, CatalogOptions{Doc: fieldsAliasDoc(t), Workspace: NewWorkspaceState(ws.Slug), Dispatcher: d, PadVersion: "test"}); err != nil {
+		t.Fatal(err)
+	}
+
+	for i, tc := range []struct{ fields, want string }{
+		{`{"n":12345}`, `"n":12345`},
+		{`{"n":9007199254740993}`, `"n":9007199254740993`},
+		{`{"n":-9007199254740993}`, `"n":-9007199254740993`},
+		{`{"n":1.10}`, `"n":1.10`},
+		{`{"n":0.1000000000000000055511151231257827}`, `"n":0.1000000000000000055511151231257827`},
+		// A number NESTED in a json field value, which the float64 decode
+		// rounded just the same.
+		{`{"j":{"id":9007199254740993,"xs":[18014398509481985]}}`, `"j":{"id":9007199254740993,"xs":[18014398509481985]}`},
+	} {
+		args := fmt.Sprintf(`{"action":"update","workspace":%q,"ref":%q,"fields":%s}`, ws.Slug, item.Slug, tc.fields)
+		resp := respText(t, m.HandleMessage(context.Background(), toolsCall(i, args)))
+		if strings.Contains(resp, `"isError":true`) {
+			t.Fatalf("%s: tool call failed: %s", tc.fields, resp)
+		}
+		got, err := s.GetItem(item.ID)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if !strings.Contains(got.Fields, tc.want) {
+			t.Fatalf("%s: stored fields = %s, want it to contain %s", tc.fields, got.Fields, tc.want)
+		}
+	}
+}
+
+// TestMCPFieldsKeepNumberLiterals_Stdio covers the local door, which the
+// filing believed unaffected: a `fields` OBJECT number is stringified into a
+// --field entry, and the float64 it was stringified from had already been
+// rounded. A `field: ["n=…"]` string entry was always safe; the object was not.
+func TestMCPFieldsKeepNumberLiterals_Stdio(t *testing.T) {
+	fd := &fakeDispatcher{}
+	m := mcpserver.NewMCPServer("t", "1", mcpserver.WithToolCapabilities(true))
+	if _, err := RegisterCatalog(m, CatalogOptions{Doc: fieldsAliasDoc(t), Workspace: NewWorkspaceState("ws"), Dispatcher: fd, PadVersion: "test"}); err != nil {
+		t.Fatal(err)
+	}
+	for i, tc := range []struct{ fields, want string }{
+		{`{"n":12345}`, "n=12345"},
+		{`{"n":9007199254740993}`, "n=9007199254740993"},
+		{`{"n":1.10}`, "n=1.10"},
+	} {
+		fd.gotArgs = nil
+		resp := respText(t, m.HandleMessage(context.Background(), toolsCall(i, `{"action":"update","ref":"TASK-1","fields":`+tc.fields+`}`)))
+		if strings.Contains(resp, `"isError":true`) {
+			t.Fatalf("%s: tool call failed: %s", tc.fields, resp)
+		}
+		if !containsString(fd.gotArgs, tc.want) {
+			t.Fatalf("%s: dispatched args %q, want an entry %q", tc.fields, fd.gotArgs, tc.want)
+		}
+	}
+}
+
+func containsString(xs []string, want string) bool {
+	for _, x := range xs {
+		if x == want {
+			return true
+		}
+	}
+	return false
+}
+
+// TestMCPFieldsNumberConflictCompare pins the conflict pass. A `fields`
+// number and a `field` entry naming the same key are ONE write when they are
+// the same number, however spelled — the answer they got while both were
+// float64 — and a conflict when they are different numbers, including two
+// that used to round to the same float64 and so compared equal.
+func TestMCPFieldsNumberConflictCompare(t *testing.T) {
+	fd := &fakeDispatcher{}
+	m := mcpserver.NewMCPServer("t", "1", mcpserver.WithToolCapabilities(true))
+	if _, err := RegisterCatalog(m, CatalogOptions{Doc: fieldsAliasDoc(t), Workspace: NewWorkspaceState("ws"), Dispatcher: fd, PadVersion: "test"}); err != nil {
+		t.Fatal(err)
+	}
+	for i, tc := range []struct {
+		name, fields, entry string
+		refused             bool
+	}{
+		{"same integer", `{"n":3}`, "n=3", false},
+		{"3.0 is 3", `{"n":3.0}`, "n=3", false},
+		{"1.10 is 1.1", `{"n":1.10}`, "n=1.1", false},
+		{"different numbers", `{"n":4}`, "n=3", true},
+		{"distinct above 2^53", `{"n":9007199254740993}`, "n=9007199254740992", true},
+		{"not a number on the string side", `{"n":3}`, "n=three", true},
+	} {
+		args := fmt.Sprintf(`{"action":"update","ref":"TASK-1","fields":%s,"field":[%q]}`, tc.fields, tc.entry)
+		resp := respText(t, m.HandleMessage(context.Background(), toolsCall(i, args)))
+		if got := strings.Contains(resp, `"isError":true`); got != tc.refused {
+			t.Errorf("%s: refused=%v, want %v: %s", tc.name, got, tc.refused, resp)
+		}
+	}
+}

internal/mcp/catalog.go

@@ -2,6 +2,7 @@ package mcp
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"sort"
 	"strings"
@@ -10,6 +11,7 @@ import (
 	"github.com/mark3labs/mcp-go/server"
 
 	"github.com/PerpetualSoftware/pad/internal/cmdhelp"
+	"github.com/PerpetualSoftware/pad/internal/models"
 )
 
 // ─────────────────────────────────────────────────────────────────────
@@ -412,7 +414,7 @@ func paramDefToToolOption(p ParamDef) mcp.ToolOption {
 func makeFanOutHandler(def ToolDef, env ActionEnv) server.ToolHandlerFunc {
 	declared := declaredInputKeys(def)
 	return func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) {
-		input := req.GetArguments()
+		input := withFieldNumberLiterals(req.GetArguments(), req.Params.RawArguments)
 		if input == nil {
 			input = map[string]any{}
 		}
@@ -442,6 +444,50 @@ func makeFanOutHandler(def ToolDef, env ActionEnv) server.ToolHandlerFunc {
 	}
 }
 
+// withFieldNumberLiterals re-reads the `fields` argument from the raw request
+// bytes with every number kept as its literal (json.Number), so a field write
+// through this door keeps the number the caller sent. mcp-go fills Arguments
+// with a plain json.Unmarshal of those same bytes, which turns every number
+// into a float64: 9007199254740993 arrived as 9007199254740992, and 1.10 as
+// 1.1, before any of our code ran (BUG-3217, the MCP half of BUG-3202, whose
+// rule is that a field write keeps its number's literal).
+//
+// ONLY `fields` is re-read. It is the one argument whose numbers are stored
+// as the caller's value; every other numeric param (limit, expected_seq, …)
+// is a control input that its consumers read as float64, and changing its
+// type would widen this fix to every one of them for no stored value.
+//
+// On anything unexpected — no raw bytes, no `fields` in them, bytes that do
+// not decode — the Arguments map is returned untouched, which is the behaviour
+// before this function existed. The map is cloned rather than edited, as the
+// fan-out handler already does, so req.Params.Arguments is never mutated.
+func withFieldNumberLiterals(input map[string]any, raw json.RawMessage) map[string]any {
+	if len(raw) == 0 || input == nil {
+		return input
+	}
+	if _, ok := input["fields"]; !ok {
+		return input
+	}
+	var top map[string]json.RawMessage
+	if err := json.Unmarshal(raw, &top); err != nil {
+		return input
+	}
+	rawFields, ok := top["fields"]
+	if !ok {
+		return input
+	}
+	var fields any
+	if err := models.DecodeJSONKeepingNumbers(rawFields, &fields); err != nil {
+		return input
+	}
+	out := make(map[string]any, len(input))
+	for k, v := range input {
+		out[k] = v
+	}
+	out["fields"] = fields
+	return out
+}
+
 // applyResultMode keeps only the result channel the configured client exposes
 // to its model. Errors and one-channel results stay untouched so compact mode
 // can never turn a useful result into an empty one.

internal/mcp/catalog_item_fields.go

@@ -2,8 +2,10 @@ package mcp
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
+	"math/big"
 	"sort"
 	"strconv"
 	"strings"
@@ -482,7 +484,12 @@ func detectFieldConflicts(prefix string, input map[string]any) *mcp.CallToolResu
 				return errStructured(prefix, fmt.Errorf(
 					"%s conflicts with %s — one key cannot be both a structured value and a string", a.source, b.source))
 			}
-			if a.value != b.value {
+			// scalarEqual rather than a.value != b.value: each side's value
+			// IS stringifyFieldValue(raw), so for strings and float64 this
+			// is the same comparison, and a `fields` json.Number (BUG-3217)
+			// compares by numeric value — 3.0 next to "n=3" stays one write,
+			// as it was while both were float64.
+			if !scalarEqual(a.raw, b.raw) {
 				return errStructured(prefix, fmt.Errorf(
 					"%s conflicts with %s (%s vs %s) — pass one of them, or the same value in both",
 					a.source, b.source, a.value, b.value))
@@ -999,6 +1006,8 @@ func stringifyFieldValue(v any) (string, error) {
 		return strconv.FormatBool(t), nil
 	case float64: // JSON numbers decode to float64
 		return strconv.FormatFloat(t, 'f', -1, 64), nil
+	case json.Number: // a `fields` number, kept as its literal (BUG-3217)
+		return t.String(), nil
 	case int:
 		return strconv.Itoa(t), nil
 	case nil:
@@ -1013,10 +1022,25 @@ func stringifyFieldValue(v any) (string, error) {
 // form (so 3 == 3.0 and "done" == "done"); anything non-scalar (e.g.
 // two tags arrays) compares by fmt.Sprint — good enough to distinguish
 // "same call twice" from a genuine conflict.
+//
+// A json.Number (a `fields` number kept as its literal, BUG-3217) compares
+// by numeric VALUE against the other side's string form, so 3.0 still equals
+// 3 and "1.1" still equals 1.10, as they did while both were float64 — and
+// 9007199254740993 no longer equals 9007199254740992, which it did only
+// because both had been rounded to the same float64.
 func scalarEqual(a, b any) bool {
 	as, aerr := stringifyFieldValue(a)
 	bs, berr := stringifyFieldValue(b)
 	if aerr == nil && berr == nil {
+		_, aNum := a.(json.Number)
+		_, bNum := b.(json.Number)
+		if aNum || bNum {
+			ar, aok := new(big.Rat).SetString(as)
+			br, bok := new(big.Rat).SetString(bs)
+			if aok && bok {
+				return ar.Cmp(br) == 0
+			}
+		}
 		return as == bs
 	}
 	return fmt.Sprint(a) == fmt.Sprint(b)

internal/mcp/instructions.md

@@ -6,7 +6,7 @@ Pad is a project tracker for developers and AI agents — issues (TASK, BUG), pl
 
 If the user is asking general code questions with no project-management thread, you don't need this server.
 
-## Tool surface (v0.53)
+## Tool surface (v0.54)
 
 Ten resource × action tools, plus `pad_set_workspace` (which takes a `workspace` slug only — no action enum). Eleven tools total.
 

internal/mcp/version.go

@@ -1017,6 +1017,25 @@ const CmdhelpVersion = "0.1"
 //     browser tab and BUG-3000 carries the open half — so no surface
 //     here states a duration.
 //
+//     0.54 — BUG-3217. A `fields` number is decoded from the raw request
+//     bytes with its literal kept, on BOTH transports: mcp-go fills the
+//     tool arguments with a plain json.Unmarshal, so 9007199254740993 used
+//     to arrive as 9007199254740992 and 1.10 as 1.1. Remote /mcp stored the
+//     rounded value; local stdio stringified the rounded float64 into its
+//     `--field` entry, so it was affected too (the filing believed it was
+//     not). Only `fields` is re-read; every other numeric param is unchanged.
+//     The BUMP is owed by one NEW REFUSAL (lead-ruled on BUG-3217's trail):
+//     on `pad_item.action=create` / `update`, a `fields` number beside a
+//     `field: ["n=…"]` entry for the same
+//     key is compared by exact numeric value, so two numbers that differ
+//     only above 2^53 — which rounded to one float64 and were silently
+//     collapsed into ONE rounded write — are now refused with
+//     `validation_failed`, message "pad_item.<action>: fields.n conflicts
+//     with the field array entry "n=…" (… vs …) — pass one of them, or the
+//     same value in both", hint "Check the input shape against the tool's
+//     schema." A matching pair still succeeds as one write however it is
+//     spelled: 3.0 beside n=3, and 1.10 beside n=1.1, collapse as they did.
+//
 //     0.53 — BUG-2819. ADDITIVE: every attachment row returned by
 //     `pad_attachment.action=list` gains `filename_source` (caller |
 //     normalised | substituted | derived | unknown), on BOTH transports —
@@ -1598,7 +1617,7 @@ const CmdhelpVersion = "0.1"
 //     this surface can receive it; the entry exists so a future action does
 //     not collapse it to permission_denied. When an action that can reach
 //     it is added, that addition is the contract change and owns the bump.
-const ToolSurfaceVersion = "0.53"
+const ToolSurfaceVersion = "0.54"
 
 // MetaVersionURI is the canonical URI of the queryable version document.
 // Lives outside the pad://workspace/{ws}/... namespace because it's a