fix(approvals): stop empty fields tripping the safety rules

CHANGELOG.md

@@ -156,6 +156,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 run on a laptop can stay off the local network
 
 A managed computer listens on every network interface, which a container needs. Run straight on a

server/src/approvals/policy.ts

@@ -109,18 +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]) => `${key} ${flatten(entry, depth + 1)}`)
-      .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;
 }
 
 /**

server/tests/approvals-policy.test.ts

@@ -315,6 +315,52 @@ 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();
+  });
+
+  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<string, unknown>) =>
+      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<string, unknown>) =>
+      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();
   });
 });