From 6bda01286a42e4689d1e547748a444db07f5a4db Mon Sep 17 00:00:00 2001 From: Saim Shafique Date: Sat, 31 Oct 2026 22:10:41 +0500 Subject: [PATCH 1/3] fix(approvals): ignore empty fields when matching safety rules A field the Bot left empty says nothing about the action. Writing only a note on a Shopify order was handed to the person as money-moving because the action's schema has a billing_address field that was never filled in; field names now only count when they hold something. Refs #890 --- CHANGELOG.md | 7 +++++++ server/src/approvals/policy.ts | 7 ++++++- server/tests/approvals-policy.test.ts | 16 ++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 54c57cadcc..1fb4fc4395 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -84,6 +84,13 @@ The built-in safety rules hand paying, purchasing or moving money to you, but a (such as `SHOPIFY_CREATE_REFUND`) passed straight through: "refund" was not one of the words the rule looked for. It is now, so issuing a refund is handed to you like any other movement of money. +### Empty fields no longer trip the safety rules + +The built-in safety rules matched on the names of fields a Bot left empty: writing only a note on a +Shopify order was handed to you as money-moving because the action has a `billing_address` field the +Bot never filled in. Field names now only count when the field actually holds something, so that +note goes straight through. + ### A computer closes browsers nobody is using when its memory runs high A computer runs one browser for each Bot that is browsing, and the only limits were a count and half diff --git a/server/src/approvals/policy.ts b/server/src/approvals/policy.ts index 3de1eb482e..797df7c1d7 100644 --- a/server/src/approvals/policy.ts +++ b/server/src/approvals/policy.ts @@ -118,7 +118,12 @@ function flatten(value: unknown, depth = 0): string { return value.map((entry) => flatten(entry, depth + 1)).join(" "); if (typeof value === "object") return Object.entries(value) - .map(([key, entry]) => `${key} ${flatten(entry, depth + 1)}`) + .map(([key, entry]) => { + // A field left empty says nothing about the action: match the arguments actually + // sent, not a name like "billing_address" with nothing in it. + const text = flatten(entry, depth + 1); + return text ? `${key} ${text}` : ""; + }) .join(" "); return ""; } diff --git a/server/tests/approvals-policy.test.ts b/server/tests/approvals-policy.test.ts index c3ccb59fa8..d51202661b 100644 --- a/server/tests/approvals-policy.test.ts +++ b/server/tests/approvals-policy.test.ts @@ -315,6 +315,22 @@ describe("built-in safety requirements", () => { expect( shopify("SHOPIFY_UPDATE_ORDER", { id: 123, note: "gift wrap" }), ).toBeUndefined(); + // A field left empty is not an argument sent: the billing_address the Bot did not + // fill in must not hand the note off as money-moving (#890). + expect( + shopify("SHOPIFY_UPDATE_ORDER", { + id: 123, + billing_address: null, + note: "gift wrap", + }), + ).toBeUndefined(); + expect( + shopify("SHOPIFY_UPDATE_ORDER", { + id: 123, + billing_address: {}, + note: "", + }), + ).toBeUndefined(); }); }); From febafe96cabe538f8a6b2aa4488e86113098e43c Mon Sep 17 00:00:00 2001 From: Saim Shafique Date: Sat, 31 Oct 2026 22:52:57 +0500 Subject: [PATCH 2/3] fix: restore PR changes dropped in bad merge The merge of main (20de77db) rebuilt the tree from upstream main plus the merged CHANGELOG.md, silently dropping this PR's server/src/approvals/policy.ts and server/tests/approvals-policy.test.ts changes. Restore both files from the pre-merge head. --- server/src/approvals/policy.ts | 7 ++++++- server/tests/approvals-policy.test.ts | 16 ++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/server/src/approvals/policy.ts b/server/src/approvals/policy.ts index 3de1eb482e..797df7c1d7 100644 --- a/server/src/approvals/policy.ts +++ b/server/src/approvals/policy.ts @@ -118,7 +118,12 @@ function flatten(value: unknown, depth = 0): string { return value.map((entry) => flatten(entry, depth + 1)).join(" "); if (typeof value === "object") return Object.entries(value) - .map(([key, entry]) => `${key} ${flatten(entry, depth + 1)}`) + .map(([key, entry]) => { + // A field left empty says nothing about the action: match the arguments actually + // sent, not a name like "billing_address" with nothing in it. + const text = flatten(entry, depth + 1); + return text ? `${key} ${text}` : ""; + }) .join(" "); return ""; } diff --git a/server/tests/approvals-policy.test.ts b/server/tests/approvals-policy.test.ts index c3ccb59fa8..d51202661b 100644 --- a/server/tests/approvals-policy.test.ts +++ b/server/tests/approvals-policy.test.ts @@ -315,6 +315,22 @@ describe("built-in safety requirements", () => { expect( shopify("SHOPIFY_UPDATE_ORDER", { id: 123, note: "gift wrap" }), ).toBeUndefined(); + // A field left empty is not an argument sent: the billing_address the Bot did not + // fill in must not hand the note off as money-moving (#890). + expect( + shopify("SHOPIFY_UPDATE_ORDER", { + id: 123, + billing_address: null, + note: "gift wrap", + }), + ).toBeUndefined(); + expect( + shopify("SHOPIFY_UPDATE_ORDER", { + id: 123, + billing_address: {}, + note: "", + }), + ).toBeUndefined(); }); }); From 5382ec2a77413d4da80929f0dd7fffc66aeb6cc5 Mon Sep 17 00:00:00 2001 From: David McKay Date: Mon, 2 Nov 2026 09:51:03 -0800 Subject: [PATCH 3/3] Keep a field's name when the depth cap, not emptiness, hid its value Dropping the names of empty fields also dropped the name of any field whose value sat past the six-level depth cap, because an unread value flattens to nothing just like an empty one. A payment_method or refund field nested deep enough then slipped past the money rule. Flattening now tracks whether a value holds anything separately from its text, so only a genuinely empty field loses its name and an unread one keeps it. --- server/src/approvals/policy.ts | 68 +++++++++++++++++++++------ server/tests/approvals-policy.test.ts | 30 ++++++++++++ 2 files changed, 83 insertions(+), 15 deletions(-) diff --git a/server/src/approvals/policy.ts b/server/src/approvals/policy.ts index 797df7c1d7..e0dcff7bc9 100644 --- a/server/src/approvals/policy.ts +++ b/server/src/approvals/policy.ts @@ -109,23 +109,61 @@ const SAFETY_REQUIREMENTS: readonly { }, ]; +/** How deep a value is read. Anything nested further is not, so matching stays cheap. */ +const MAX_DEPTH = 6; + function flatten(value: unknown, depth = 0): string { - if (depth > 6 || value === null || value === undefined) return ""; - if (typeof value === "string") return value; + return words(value, depth).text; +} + +/** + * The words of a value, and whether it holds anything at all. + * + * `filled` is separate from `text` because of the depth cap. A value past the cap is never read, so + * its text is empty whether or not it holds something, and an unread value is not an empty one. + */ +function words( + value: unknown, + depth: number, +): { text: string; filled: boolean } { + if (value === null || value === undefined) return { text: "", filled: false }; + // Not read, so not known to be empty. Only a value that is plainly empty on its face counts as + // nothing sent; anything else keeps its field's name in the words. + if (depth > MAX_DEPTH) return { text: "", filled: !plainlyEmpty(value) }; + if (typeof value === "string") return { text: value, filled: value !== "" }; if (typeof value === "number" || typeof value === "boolean") - return String(value); - if (Array.isArray(value)) - return value.map((entry) => flatten(entry, depth + 1)).join(" "); - if (typeof value === "object") - return Object.entries(value) - .map(([key, entry]) => { - // A field left empty says nothing about the action: match the arguments actually - // sent, not a name like "billing_address" with nothing in it. - const text = flatten(entry, depth + 1); - return text ? `${key} ${text}` : ""; - }) - .join(" "); - return ""; + return { text: String(value), filled: true }; + if (Array.isArray(value)) { + const entries = value.map((entry) => words(entry, depth + 1)); + return { + text: entries.map((entry) => entry.text).join(" "), + filled: entries.some((entry) => entry.filled), + }; + } + if (typeof value === "object") { + const fields = Object.entries(value).map(([key, entry]) => { + // A field left empty says nothing about the action: match the arguments actually + // sent, not a name like "billing_address" with nothing in it. + const read = words(entry, depth + 1); + return { + text: read.filled ? `${key} ${read.text}` : "", + filled: read.filled, + }; + }); + return { + text: fields.map((field) => field.text).join(" "), + filled: fields.some((field) => field.filled), + }; + } + return { text: "", filled: false }; +} + +function plainlyEmpty(value: unknown): boolean { + if (value === "") return true; + if (Array.isArray(value)) return value.length === 0; + if (typeof value === "object" && value !== null) + return Object.keys(value).length === 0; + return false; } /** diff --git a/server/tests/approvals-policy.test.ts b/server/tests/approvals-policy.test.ts index d51202661b..591f6c0514 100644 --- a/server/tests/approvals-policy.test.ts +++ b/server/tests/approvals-policy.test.ts @@ -332,6 +332,36 @@ describe("built-in safety requirements", () => { }), ).toBeUndefined(); }); + + test("a field nested past the depth cap still counts by its name", () => { + // Values deeper than the cap are not read, so a field there flattens to nothing whether or not + // it holds anything. Its name is all the rule gets to see of it, and an unread value is not an + // empty one: dropping the name too let a money field through just by nesting it deep enough. + const sent = (args: Record) => + safetyRequirement( + candidate({ + toolRef: "composio-shopify/SHOPIFY_GRAPHQL_MUTATION", + scope: "composio-shopify", + args, + target: { + serverId: "composio-shopify", + toolName: "SHOPIFY_GRAPHQL_MUTATION", + url: "https://mcp.composio.dev/shopify", + effect: "write", + }, + }), + )?.id; + const nested = (leaf: Record) => + sent({ a: { b: { c: { d: { e: { f: leaf } } } } } }); + expect(nested({ payment_method: { card: "tok_123" } })).toBe("payments"); + expect(nested({ billing: [{ line: 1 }] })).toBe("payments"); + // A list read at the cap whose entries are past it is not empty either. + expect( + sent({ a: { b: { c: { d: { e: { refund_lines: [{ id: 1 }] } } } } } }), + ).toBe("payments"); + // Past the cap as well, but genuinely empty: still nothing sent. + expect(nested({ billing_address: {}, refund: [] })).toBeUndefined(); + }); }); describe("the reviewer's safety classification", () => {