From 7dd0fc2308b4776ebb850c272090b6d4c8ea828a Mon Sep 17 00:00:00 2001 From: Phan Thong IT Date: Tue, 28 Jul 2026 11:19:04 +0700 Subject: [PATCH] fix(mcp): stop stripping all-caps business identifiers from tool arguments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit isAllCapsPlaceholder() classified ANY string of >=3 chars made only of A-Z and underscore as an LLM placeholder, and stripEmptyOptionalArgs() then removed it from the outbound CallTool arguments. Real business identifiers match that shape: bank codes BKASH, NAGAD, ROCKET, VCB currency codes BDT, VND, USD, PKR, MMK enum values SUCCESS, PENDING, ALL Impact is silent and severe: dropping an optional filter turns a filtered query into an unfiltered one. The MCP server still answers 200 with the full result set, so nothing errors — the model reports totals for the entire dataset as if they were the filtered result, with full confidence. Observed in production: asking "how many BKASH proxies are active" returned 6,610 (all banks) instead of 1,263. Three different bank codes returned byte-identical results, which also drove the model into a retry loop until the loop guard fired. Diagnosis was slow because the removal is invisible: the outbound log records args_len BEFORE stripping, and stripEmptyOptionalArgs logs nothing at all — while normalizeArgsForSchema right below it does emit mcp.tool.args.coerced. Confirmed the argument never reaches the server by echoing the raw JSON-RPC arguments from RequestContext[CallToolRequestParams] on the server side. Changes: - isAllCapsPlaceholder: require multi-word (contains "_") or membership in a known single-word placeholder list. Keeps the original intent — the doc comment's own examples (SHOULD_NOT_BE_HERE, DO_NOT_SEND, NOT_APPLICABLE) are all multi-word — while letting business codes through. - stripEmptyOptionalArgs: emit slog.Warn("mcp.tool.args.stripped") whenever an argument is dropped, so this class of bug is one grep away instead of a multi-hour investigation. - Tests: guard both layers against regression. Verified: go build ./... clean; go test ./internal/mcp/ passes. --- internal/mcp/bridge_tool.go | 33 ++++++++++++++++- internal/mcp/bridge_tool_test.go | 63 ++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 2 deletions(-) diff --git a/internal/mcp/bridge_tool.go b/internal/mcp/bridge_tool.go index 51c4de78..93674a47 100644 --- a/internal/mcp/bridge_tool.go +++ b/internal/mcp/bridge_tool.go @@ -370,12 +370,18 @@ func (t *BridgeTool) stripEmptyOptionalArgs(args map[string]any) map[string]any } if s, ok := v.(string); ok { // Strip known placeholder values (e.g. "optional", "null", "http://example.com"). + // Logged: dropping an argument silently changes query semantics (a filtered + // call becomes unfiltered) and is otherwise invisible in traces. if isPlaceholderValue(s) { + slog.Warn("mcp.tool.args.stripped", + "tool", t.registeredName, "arg", k, "value", s, "reason", "placeholder") continue } // Type-aware empty string: keep for string-typed params (user may want empty), // strip for non-string params (empty string is never valid for number/boolean/UUID). if s == "" && t.propertyType(k) != "string" { + slog.Warn("mcp.tool.args.stripped", + "tool", t.registeredName, "arg", k, "reason", "empty_string_non_string_param") continue } } @@ -441,17 +447,40 @@ func detectLogicalErrorPayload(text string) (string, bool) { // isAllCapsPlaceholder detects LLM-generated all-caps placeholder strings // like "SHOULD_NOT_BE_HERE", "DO_NOT_SEND", "NOT_APPLICABLE", "PLACEHOLDER". +// +// NOTE: being all-caps is NOT sufficient. Real business identifiers are commonly +// all-caps: bank codes (BKASH, NAGAD, ROCKET), currency codes (BDT, VND, USD), +// country codes, ticker symbols, enum values (SUCCESS, PENDING). Silently dropping +// those turns a filtered query into an unfiltered one — the MCP server still returns +// 200 with the full result set, so neither the model nor the operator notices, and +// the model then reports a wrong number with full confidence. +// +// Only treat a value as a placeholder when it is multi-word (contains "_") or matches +// a known single-word placeholder. func isAllCapsPlaceholder(s string) bool { trimmed := strings.TrimSpace(s) if len(trimmed) < 3 { return false } + hasUnderscore := false for _, r := range trimmed { - if r != '_' && (r < 'A' || r > 'Z') { + if r == '_' { + hasUnderscore = true + continue + } + if r < 'A' || r > 'Z' { return false } } - return true + if hasUnderscore { + return true + } + switch trimmed { + case "PLACEHOLDER", "TODO", "TBD", "XXX", "NONE", "NULL", "UNKNOWN", + "VALUE", "STRING", "EXAMPLE", "CHANGEME", "REPLACE", "OMIT", "SKIP": + return true + } + return false } // wrapMCPContent wraps MCP tool results as external/untrusted content. diff --git a/internal/mcp/bridge_tool_test.go b/internal/mcp/bridge_tool_test.go index 55de8302..ea92b7d6 100644 --- a/internal/mcp/bridge_tool_test.go +++ b/internal/mcp/bridge_tool_test.go @@ -483,3 +483,66 @@ func TestEnsureMCPPrefix(t *testing.T) { }) } } + +// TestIsAllCapsPlaceholder_KeepsBusinessCodes guards a production incident: +// all-caps business identifiers (bank codes, currency codes, enum values) were +// classified as LLM placeholders and silently dropped from tool arguments, so a +// filtered query silently became an unfiltered one and the model reported totals +// for the whole dataset as if they were the filtered result. +func TestIsAllCapsPlaceholder_KeepsBusinessCodes(t *testing.T) { + keep := []string{ + "BKASH", "NAGAD", "ROCKET", "ROCKETC", // bank codes + "BDT", "VND", "USD", "PKR", "MMK", // currency codes + "SUCCESS", "PENDING", "ALL", // enum values + "VCB", "SPACEPROXY", + } + for _, s := range keep { + if isAllCapsPlaceholder(s) { + t.Errorf("business identifier %q must NOT be treated as a placeholder", s) + } + } + + drop := []string{ + "SHOULD_NOT_BE_HERE", "DO_NOT_SEND", "NOT_APPLICABLE", // multi-word + "PLACEHOLDER", "TODO", "TBD", "XXX", "CHANGEME", // known single-word + } + for _, s := range drop { + if !isAllCapsPlaceholder(s) { + t.Errorf("placeholder %q must be detected", s) + } + } +} + +// TestStripEmptyOptionalArgs_KeepsAllCapsBusinessCode is the end-to-end guard for +// the same incident, at the layer that actually mutates the outbound arguments. +func TestStripEmptyOptionalArgs_KeepsAllCapsBusinessCode(t *testing.T) { + bt := &BridgeTool{ + requiredSet: map[string]bool{"merchant_id": true}, + inputSchema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "merchant_id": map[string]any{"type": "string"}, + "bank_code": map[string]any{"type": "string"}, + "currency": map[string]any{"type": "string"}, + "note": map[string]any{"type": "string"}, + }, + }, + } + + cleaned := bt.stripEmptyOptionalArgs(map[string]any{ + "merchant_id": "CPS_C1", + "bank_code": "BKASH", // optional all-caps business code → keep + "currency": "BDT", // optional all-caps business code → keep + "note": "SHOULD_NOT_BE_HERE", // real placeholder → strip + }) + + if cleaned["bank_code"] != "BKASH" { + t.Errorf("bank_code must survive, got %v", cleaned["bank_code"]) + } + if cleaned["currency"] != "BDT" { + t.Errorf("currency must survive, got %v", cleaned["currency"]) + } + if _, ok := cleaned["note"]; ok { + t.Error("multi-word all-caps placeholder should still be stripped") + } +}