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") + } +}