refactor: share tool mutation classification

2026-02-19 18:39:20 -05:00 · 2026-02-14 16:57:55 -05:00
parent 0e721e6665
commit 385dcbd8a9
4 changed files with 285 additions and 184 deletions
--- a/src/agents/pi-embedded-runner/run/payloads.ts
+++ b/src/agents/pi-embedded-runner/run/payloads.ts
@@ -18,38 +18,10 @@ import {
  extractAssistantThinking,
  formatReasoningMessage,
 } from "../../pi-embedded-utils.js";
+import { isLikelyMutatingToolName } from "../../tool-mutation.js";

 type ToolMetaEntry = { toolName: string; meta?: string };

-const MUTATING_TOOL_NAMES = new Set([
-  "write",
-  "edit",
-  "apply_patch",
-  "exec",
-  "bash",
-  "process",
-  "message",
-  "sessions_send",
-  "cron",
-  "gateway",
-  "canvas",
-  "nodes",
-  "session_status",
-]);
-
-function isLikelyMutatingTool(toolName: string): boolean {
-  const normalized = toolName.trim().toLowerCase();
-  if (!normalized) {
-    return false;
-  }
-  return (
-    MUTATING_TOOL_NAMES.has(normalized) ||
-    normalized.endsWith("_actions") ||
-    normalized.startsWith("message_") ||
-    normalized.includes("send")
-  );
-}
-
 export function buildEmbeddedRunPayloads(params: {
  assistantTexts: string[];
  toolMetas: ToolMetaEntry[];
@@ -259,7 +231,8 @@ export function buildEmbeddedRunPayloads(params: {
      errorLower.includes("needs") ||
      errorLower.includes("requires");
    const isMutatingToolError =
-      params.lastToolError.mutatingAction ?? isLikelyMutatingTool(params.lastToolError.toolName);
+      params.lastToolError.mutatingAction ??
+      isLikelyMutatingToolName(params.lastToolError.toolName);
    const shouldShowToolError = isMutatingToolError || (!hasUserFacingReply && !isRecoverableError);

    // Always surface mutating tool failures so we do not silently confirm actions that did not happen.
--- a/src/agents/pi-embedded-subscribe.handlers.tools.ts
+++ b/src/agents/pi-embedded-subscribe.handlers.tools.ts
@@ -16,170 +16,21 @@ import {
  sanitizeToolResult,
 } from "./pi-embedded-subscribe.tools.js";
 import { inferToolMetaFromArgs } from "./pi-embedded-utils.js";
+import { buildToolMutationState, isSameToolMutationAction } from "./tool-mutation.js";
 import { normalizeToolName } from "./tool-policy.js";

 /** Track tool execution start times and args for after_tool_call hook */
 const toolStartData = new Map<string, { startTime: number; args: unknown }>();

-const READ_ONLY_ACTIONS = new Set([
-  "get",
-  "list",
-  "read",
-  "status",
-  "show",
-  "fetch",
-  "search",
-  "query",
-  "view",
-  "poll",
-  "log",
-  "inspect",
-  "check",
-  "probe",
-]);
-const PROCESS_MUTATING_ACTIONS = new Set(["write", "send_keys", "submit", "paste", "kill"]);
-const MESSAGE_MUTATING_ACTIONS = new Set([
-  "send",
-  "reply",
-  "thread_reply",
-  "threadreply",
-  "edit",
-  "delete",
-  "react",
-  "pin",
-  "unpin",
-]);
-
-function asRecord(value: unknown): Record<string, unknown> | undefined {
-  return value && typeof value === "object" ? (value as Record<string, unknown>) : undefined;
-}
-
-function normalizeActionName(value: unknown): string | undefined {
-  if (typeof value !== "string") {
-    return undefined;
-  }
-  const normalized = value
-    .trim()
-    .toLowerCase()
-    .replace(/[\s-]+/g, "_");
-  return normalized || undefined;
-}
-
-function normalizeFingerprintValue(value: unknown): string | undefined {
-  if (typeof value !== "string") {
-    return undefined;
-  }
-  const normalized = value.trim();
-  return normalized ? normalized.toLowerCase() : undefined;
-}
-
-function isMutatingToolCall(toolName: string, args: unknown): boolean {
-  const normalized = toolName.trim().toLowerCase();
-  const record = asRecord(args);
-  const action = normalizeActionName(record?.action);
-
-  switch (normalized) {
-    case "write":
-    case "edit":
-    case "apply_patch":
-    case "exec":
-    case "bash":
-    case "sessions_send":
-      return true;
-    case "process":
-      return action != null && PROCESS_MUTATING_ACTIONS.has(action);
-    case "message":
-      return (
-        (action != null && MESSAGE_MUTATING_ACTIONS.has(action)) ||
-        typeof record?.content === "string" ||
-        typeof record?.message === "string"
-      );
-    case "session_status":
-      return typeof record?.model === "string" && record.model.trim().length > 0;
-    default: {
-      if (normalized === "cron" || normalized === "gateway" || normalized === "canvas") {
-        return action == null || !READ_ONLY_ACTIONS.has(action);
-      }
-      if (normalized === "nodes") {
-        return action == null || action !== "list";
-      }
-      if (normalized.endsWith("_actions")) {
-        return action == null || !READ_ONLY_ACTIONS.has(action);
-      }
-      if (normalized.startsWith("message_") || normalized.includes("send")) {
-        return true;
-      }
-      return false;
-    }
-  }
-}
-
-function buildActionFingerprint(
-  toolName: string,
-  args: unknown,
-  meta?: string,
-): string | undefined {
-  if (!isMutatingToolCall(toolName, args)) {
-    return undefined;
-  }
-  const normalizedTool = toolName.trim().toLowerCase();
-  const record = asRecord(args);
-  const action = normalizeActionName(record?.action);
-  const parts = [`tool=${normalizedTool}`];
-  if (action) {
-    parts.push(`action=${action}`);
-  }
-  for (const key of [
-    "path",
-    "filePath",
-    "oldPath",
-    "newPath",
-    "to",
-    "target",
-    "messageId",
-    "sessionKey",
-    "jobId",
-    "id",
-    "model",
-  ]) {
-    const value = normalizeFingerprintValue(record?.[key]);
-    if (value) {
-      parts.push(`${key.toLowerCase()}=${value}`);
-    }
-  }
-  const normalizedMeta = meta?.trim().replace(/\s+/g, " ").toLowerCase();
-  if (normalizedMeta) {
-    parts.push(`meta=${normalizedMeta}`);
-  }
-  return parts.join("|");
-}
-
 function buildToolCallSummary(toolName: string, args: unknown, meta?: string): ToolCallSummary {
-  const actionFingerprint = buildActionFingerprint(toolName, args, meta);
+  const mutation = buildToolMutationState(toolName, args, meta);
  return {
    meta,
-    mutatingAction: actionFingerprint != null,
-    actionFingerprint,
+    mutatingAction: mutation.mutatingAction,
+    actionFingerprint: mutation.actionFingerprint,
  };
 }

-function sameToolAction(
-  existing: { toolName: string; meta?: string; actionFingerprint?: string },
-  toolName: string,
-  meta?: string,
-  actionFingerprint?: string,
-): boolean {
-  if (existing.actionFingerprint != null || actionFingerprint != null) {
-    // For mutating flows, fail closed: only clear when both fingerprints exist and match.
-    return (
-      existing.actionFingerprint != null &&
-      actionFingerprint != null &&
-      existing.actionFingerprint === actionFingerprint
-    );
-  }
-  return existing.toolName === toolName && (existing.meta ?? "") === (meta ?? "");
-}
-
 function extendExecMeta(toolName: string, args: unknown, meta?: string): string | undefined {
  const normalized = toolName.trim().toLowerCase();
  if (normalized !== "exec" && normalized !== "bash") {
@@ -347,7 +198,13 @@ export async function handleToolExecutionEnd(
  } else if (ctx.state.lastToolError) {
    // Keep unresolved mutating failures until the same action succeeds.
    if (ctx.state.lastToolError.mutatingAction) {
-      if (sameToolAction(ctx.state.lastToolError, toolName, meta, callSummary?.actionFingerprint)) {
+      if (
+        isSameToolMutationAction(ctx.state.lastToolError, {
+          toolName,
+          meta,
+          actionFingerprint: callSummary?.actionFingerprint,
+        })
+      ) {
        ctx.state.lastToolError = undefined;
      }
    } else {
--- a/src/agents/tool-mutation.test.ts
+++ b/src/agents/tool-mutation.test.ts
@@ -0,0 +1,70 @@
+import { describe, expect, it } from "vitest";
+import {
+  buildToolActionFingerprint,
+  buildToolMutationState,
+  isLikelyMutatingToolName,
+  isMutatingToolCall,
+  isSameToolMutationAction,
+} from "./tool-mutation.js";
+
+describe("tool mutation helpers", () => {
+  it("treats session_status as mutating only when model override is provided", () => {
+    expect(isMutatingToolCall("session_status", { sessionKey: "agent:main:main" })).toBe(false);
+    expect(
+      isMutatingToolCall("session_status", {
+        sessionKey: "agent:main:main",
+        model: "openai/gpt-4o",
+      }),
+    ).toBe(true);
+  });
+
+  it("builds stable fingerprints for mutating calls and omits read-only calls", () => {
+    const writeFingerprint = buildToolActionFingerprint(
+      "write",
+      { path: "/tmp/demo.txt", id: 42 },
+      "write /tmp/demo.txt",
+    );
+    expect(writeFingerprint).toContain("tool=write");
+    expect(writeFingerprint).toContain("path=/tmp/demo.txt");
+    expect(writeFingerprint).toContain("id=42");
+    expect(writeFingerprint).toContain("meta=write /tmp/demo.txt");
+
+    const readFingerprint = buildToolActionFingerprint("read", { path: "/tmp/demo.txt" });
+    expect(readFingerprint).toBeUndefined();
+  });
+
+  it("exposes mutation state for downstream payload rendering", () => {
+    expect(
+      buildToolMutationState("message", { action: "send", to: "telegram:1" }).mutatingAction,
+    ).toBe(true);
+    expect(buildToolMutationState("browser", { action: "list" }).mutatingAction).toBe(false);
+  });
+
+  it("matches tool actions by fingerprint and fails closed on asymmetric data", () => {
+    expect(
+      isSameToolMutationAction(
+        { toolName: "write", actionFingerprint: "tool=write|path=/tmp/a" },
+        { toolName: "write", actionFingerprint: "tool=write|path=/tmp/a" },
+      ),
+    ).toBe(true);
+    expect(
+      isSameToolMutationAction(
+        { toolName: "write", actionFingerprint: "tool=write|path=/tmp/a" },
+        { toolName: "write", actionFingerprint: "tool=write|path=/tmp/b" },
+      ),
+    ).toBe(false);
+    expect(
+      isSameToolMutationAction(
+        { toolName: "write", actionFingerprint: "tool=write|path=/tmp/a" },
+        { toolName: "write" },
+      ),
+    ).toBe(false);
+  });
+
+  it("keeps legacy name-only mutating heuristics for payload fallback", () => {
+    expect(isLikelyMutatingToolName("sessions_send")).toBe(true);
+    expect(isLikelyMutatingToolName("browser_actions")).toBe(true);
+    expect(isLikelyMutatingToolName("message_slack")).toBe(true);
+    expect(isLikelyMutatingToolName("browser")).toBe(false);
+  });
+});
--- a/src/agents/tool-mutation.ts
+++ b/src/agents/tool-mutation.ts
@@ -0,0 +1,201 @@
+const MUTATING_TOOL_NAMES = new Set([
+  "write",
+  "edit",
+  "apply_patch",
+  "exec",
+  "bash",
+  "process",
+  "message",
+  "sessions_send",
+  "cron",
+  "gateway",
+  "canvas",
+  "nodes",
+  "session_status",
+]);
+
+const READ_ONLY_ACTIONS = new Set([
+  "get",
+  "list",
+  "read",
+  "status",
+  "show",
+  "fetch",
+  "search",
+  "query",
+  "view",
+  "poll",
+  "log",
+  "inspect",
+  "check",
+  "probe",
+]);
+
+const PROCESS_MUTATING_ACTIONS = new Set(["write", "send_keys", "submit", "paste", "kill"]);
+
+const MESSAGE_MUTATING_ACTIONS = new Set([
+  "send",
+  "reply",
+  "thread_reply",
+  "threadreply",
+  "edit",
+  "delete",
+  "react",
+  "pin",
+  "unpin",
+]);
+
+export type ToolMutationState = {
+  mutatingAction: boolean;
+  actionFingerprint?: string;
+};
+
+export type ToolActionRef = {
+  toolName: string;
+  meta?: string;
+  actionFingerprint?: string;
+};
+
+function asRecord(value: unknown): Record<string, unknown> | undefined {
+  return value && typeof value === "object" ? (value as Record<string, unknown>) : undefined;
+}
+
+function normalizeActionName(value: unknown): string | undefined {
+  if (typeof value !== "string") {
+    return undefined;
+  }
+  const normalized = value
+    .trim()
+    .toLowerCase()
+    .replace(/[\s-]+/g, "_");
+  return normalized || undefined;
+}
+
+function normalizeFingerprintValue(value: unknown): string | undefined {
+  if (typeof value === "string") {
+    const normalized = value.trim();
+    return normalized ? normalized.toLowerCase() : undefined;
+  }
+  if (typeof value === "number" || typeof value === "bigint" || typeof value === "boolean") {
+    return String(value).toLowerCase();
+  }
+  return undefined;
+}
+
+export function isLikelyMutatingToolName(toolName: string): boolean {
+  const normalized = toolName.trim().toLowerCase();
+  if (!normalized) {
+    return false;
+  }
+  return (
+    MUTATING_TOOL_NAMES.has(normalized) ||
+    normalized.endsWith("_actions") ||
+    normalized.startsWith("message_") ||
+    normalized.includes("send")
+  );
+}
+
+export function isMutatingToolCall(toolName: string, args: unknown): boolean {
+  const normalized = toolName.trim().toLowerCase();
+  const record = asRecord(args);
+  const action = normalizeActionName(record?.action);
+
+  switch (normalized) {
+    case "write":
+    case "edit":
+    case "apply_patch":
+    case "exec":
+    case "bash":
+    case "sessions_send":
+      return true;
+    case "process":
+      return action != null && PROCESS_MUTATING_ACTIONS.has(action);
+    case "message":
+      return (
+        (action != null && MESSAGE_MUTATING_ACTIONS.has(action)) ||
+        typeof record?.content === "string" ||
+        typeof record?.message === "string"
+      );
+    case "session_status":
+      return typeof record?.model === "string" && record.model.trim().length > 0;
+    default: {
+      if (normalized === "cron" || normalized === "gateway" || normalized === "canvas") {
+        return action == null || !READ_ONLY_ACTIONS.has(action);
+      }
+      if (normalized === "nodes") {
+        return action == null || action !== "list";
+      }
+      if (normalized.endsWith("_actions")) {
+        return action == null || !READ_ONLY_ACTIONS.has(action);
+      }
+      if (normalized.startsWith("message_") || normalized.includes("send")) {
+        return true;
+      }
+      return false;
+    }
+  }
+}
+
+export function buildToolActionFingerprint(
+  toolName: string,
+  args: unknown,
+  meta?: string,
+): string | undefined {
+  if (!isMutatingToolCall(toolName, args)) {
+    return undefined;
+  }
+  const normalizedTool = toolName.trim().toLowerCase();
+  const record = asRecord(args);
+  const action = normalizeActionName(record?.action);
+  const parts = [`tool=${normalizedTool}`];
+  if (action) {
+    parts.push(`action=${action}`);
+  }
+  for (const key of [
+    "path",
+    "filePath",
+    "oldPath",
+    "newPath",
+    "to",
+    "target",
+    "messageId",
+    "sessionKey",
+    "jobId",
+    "id",
+    "model",
+  ]) {
+    const value = normalizeFingerprintValue(record?.[key]);
+    if (value) {
+      parts.push(`${key.toLowerCase()}=${value}`);
+    }
+  }
+  const normalizedMeta = meta?.trim().replace(/\s+/g, " ").toLowerCase();
+  if (normalizedMeta) {
+    parts.push(`meta=${normalizedMeta}`);
+  }
+  return parts.join("|");
+}
+
+export function buildToolMutationState(
+  toolName: string,
+  args: unknown,
+  meta?: string,
+): ToolMutationState {
+  const actionFingerprint = buildToolActionFingerprint(toolName, args, meta);
+  return {
+    mutatingAction: actionFingerprint != null,
+    actionFingerprint,
+  };
+}
+
+export function isSameToolMutationAction(existing: ToolActionRef, next: ToolActionRef): boolean {
+  if (existing.actionFingerprint != null || next.actionFingerprint != null) {
+    // For mutating flows, fail closed: only clear when both fingerprints exist and match.
+    return (
+      existing.actionFingerprint != null &&
+      next.actionFingerprint != null &&
+      existing.actionFingerprint === next.actionFingerprint
+    );
+  }
+  return existing.toolName === next.toolName && (existing.meta ?? "") === (next.meta ?? "");
+}