HITL: "Always allow this tool" via set_tool_approval gateway meta-tool

shared/glean/mcp/src/skill-writer.ts

@@ -34,6 +34,33 @@ function parseFrontmatter(content: string): Record<string, string> {
   return result;
 }
 
+/**
+ * Keep approval requirements out of the local skill cache. The remote
+ * get_tool_approval lookup is the only source of truth, so a stale or hand-edited
+ * skill file must not retain a second approval setting for the plugin or the model
+ * to read. Other tool metadata, especially inputSchema, remains cached for argument
+ * shaping and prompt construction.
+ */
+function sanitizeSkillFile(filePath: string, text: string): string {
+  if (!/^tools[\\/]\S+\.json$/.test(filePath)) return text;
+
+  try {
+    const parsed = JSON.parse(text) as unknown;
+    if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) {
+      return text;
+    }
+    const { requires_approval: _ignored, ...metadata } = parsed as Record<
+      string,
+      unknown
+    >;
+    return JSON.stringify(metadata);
+  } catch {
+    // Leave malformed/non-object tool files alone; run_tool will not use them as
+    // approval state, and preserving the original content keeps diagnostics intact.
+    return text;
+  }
+}
+
 type LogFn = (label: string, detail?: Record<string, unknown>) => void;
 
 /**
@@ -99,8 +126,10 @@ export async function writeSkillsToDisk(
         continue;
       }
       await fs.mkdir(path.dirname(fullPath), { recursive: true });
-      const text =
-        typeof content === "string" ? content : JSON.stringify(content);
+      const text = sanitizeSkillFile(
+        filePath,
+        typeof content === "string" ? content : JSON.stringify(content),
+      );
       await fs.writeFile(fullPath, text, "utf-8");
       writtenFiles.push(fullPath);
     }

shared/glean/mcp/src/tools/run-tool.ts

@@ -160,7 +160,6 @@ export async function resolveFileArgs(
 }
 
 interface ToolMetadata {
-  requires_approval?: boolean;
   name?: string;
   description?: string;
   server_id?: string;
@@ -229,6 +228,54 @@ async function buildApprovalMessage(
   return message.join("\n");
 }
 
+// This follow-up only controls approval for future calls.
+const alwaysAllowFollowUpTimeoutMs = 5_000;
+
+interface AlwaysAllowFollowUpResult {
+  accepted: boolean;
+  timedOut: boolean;
+}
+
+async function requestAlwaysAllowFollowUp(
+  mcpServer: Server,
+  toolName: string,
+): Promise<AlwaysAllowFollowUpResult> {
+  const startedAt = Date.now();
+  try {
+    const result = await mcpServer.elicitInput(
+      {
+        message:
+          `Always allow ${toolName} for future calls?\n\n` +
+          `(Auto-declines in 5 seconds)`,
+        // Empty form preserves the host-native Yes/No actions.
+        requestedSchema: { type: "object", properties: {} } as any,
+      },
+      { timeout: alwaysAllowFollowUpTimeoutMs },
+    );
+    const elapsedMs = Date.now() - startedAt;
+    return {
+      accepted: result.action === "accept",
+      timedOut:
+        result.action !== "accept" &&
+        elapsedMs >= alwaysAllowFollowUpTimeoutMs * 0.9,
+    };
+  } catch {
+    return {
+      accepted: false,
+      timedOut:
+        Date.now() - startedAt >= alwaysAllowFollowUpTimeoutMs * 0.9,
+    };
+  }
+}
+
+function alwaysAllowFollowUpTimeoutMessage(toolName: string): string {
+  return (
+    `The Always Allow prompt for ${toolName} timed out after 5 seconds ` +
+    `(auto-declined). The current action was approved, but it was not saved ` +
+    `for future calls; they will ask for approval again.`
+  );
+}
+
 // A WeakSet so a short-lived server in tests doesn't leak,
 // and so the burn happens exactly once per server instance.
 const elicitationIdPrimed = new WeakSet<object>();
@@ -312,6 +359,91 @@ export interface RunToolPolicy {
   fileArgs: boolean;
 }
 
+class ToolApprovalError extends Error {
+  constructor(message: string) {
+    super(message);
+    this.name = "ToolApprovalError";
+  }
+}
+
+function isRecord(value: unknown): value is Record<string, unknown> {
+  return typeof value === "object" && value !== null && !Array.isArray(value);
+}
+
+function approvalResponsePayload(result: CallToolResult): unknown {
+  const structured = (result as CallToolResult & {
+    structuredContent?: unknown;
+  }).structuredContent;
+  if (structured !== undefined) return structured;
+
+  const text = result.content.find((item) => item.type === "text");
+  if (!text || text.type !== "text") return undefined;
+  try {
+    return JSON.parse(text.text);
+  } catch {
+    return undefined;
+  }
+}
+
+/**
+ * Ask the remote control plane whether this downstream tool requires approval.
+ *
+ * This is deliberately a per-call lookup. The answer is not read from skill files,
+ * stored in this process, or persisted locally. A missing, malformed, or failed
+ * response fails closed so the downstream `run_tool` call cannot proceed without a
+ * current remote decision.
+ */
+export async function getToolApproval(
+  remoteClient: Client,
+  serverId: string,
+  toolName: string,
+): Promise<boolean> {
+  let result: CallToolResult;
+  try {
+    result = await callRemoteTool(remoteClient, "get_tool_approval", {
+      server_id: serverId,
+      tool_name: toolName,
+    });
+  } catch (err) {
+    const detail = err instanceof Error ? err.message : String(err);
+    throw new ToolApprovalError(`remote lookup failed: ${detail}`);
+  }
+
+  if (result.isError) {
+    const text = result.content.find((item) => item.type === "text");
+    const detail = text?.type === "text" ? text.text : "remote lookup returned an error";
+    throw new ToolApprovalError(detail);
+  }
+
+  const payload = approvalResponsePayload(result);
+  if (!isRecord(payload) || typeof payload.requires_approval !== "boolean") {
+    throw new ToolApprovalError(
+      "remote response did not contain boolean requires_approval",
+    );
+  }
+  return payload.requires_approval;
+}
+
+function approvalLookupFailure(
+  toolName: string,
+  error: unknown,
+): CallToolResult {
+  const detail = error instanceof Error ? error.message : String(error);
+  console.error(`[get_tool_approval] ${toolName}: ${detail}`);
+  return {
+    content: [
+      {
+        type: "text",
+        text:
+          `Could not determine whether ${toolName} requires approval from the ` +
+          `remote settings. The action was NOT executed. Retry when the approval ` +
+          `settings are available.`,
+      },
+    ],
+    isError: true,
+  };
+}
+
 export async function handleRunTool(
   remoteClient: Client,
   mcpServer: Server,
@@ -331,9 +463,9 @@ export async function handleRunTool(
     };
   }
 
-  // Load the downstream tool's metadata once, up front: its inputSchema drives
-  // file_args JSON-parsing (object/array params) and its requires_approval
-  // drives the HITL gate. Both paths must see it regardless of ENABLE_HITL.
+  // Load the downstream tool's metadata only for inputSchema. Approval is not
+  // taken from this file; it is fetched from the remote control plane below for
+  // every attempted downstream call.
   const toolMeta = await findToolJson(skillsBaseDir, toolName);
 
   // Refuse before reading any model-supplied path. Disabled file_args must be
@@ -369,18 +501,14 @@ export async function handleRunTool(
     throw err;
   }
 
+  let requiresApproval: boolean;
+  try {
+    requiresApproval = await getToolApproval(remoteClient, serverId, toolName);
+  } catch (err) {
+    return approvalLookupFailure(toolName, err);
+  }
+
   const hitlEnabled = process.env.ENABLE_HITL === "true";
-  // Fail CLOSED when the tool's approval requirement is unknown. The gate used
-  // to key on `toolMeta?.requires_approval`; a missing or unparseable tool JSON
-  // (evicted by evictStaleSkills after a week, called from memory without a
-  // fresh find_skills_and_tools, or corrupt) made that falsy, so the gate
-  // was skipped and — with the native prompt already suppressed via
-  // readOnlyHint — the tool executed with ZERO approval. Only skip the gate
-  // when we can positively confirm the tool is read-only.
-  const requiresApproval =
-    typeof toolMeta?.requires_approval === "boolean"
-      ? toolMeta.requires_approval
-      : true;
   // Cursor is deliberately not excepted: current Cursor builds can use the same
   // local elicitation gate as other capable hosts. Older builds that drop the
   // prompt fail closed, and the timeout response explains the upgrade path.
@@ -424,6 +552,43 @@ export async function handleRunTool(
             ],
           };
         }
+
+        const alwaysAllow = await requestAlwaysAllowFollowUp(
+          mcpServer,
+          toolName,
+        );
+        if (alwaysAllow.accepted) {
+          try {
+            await callRemoteTool(remoteClient, "set_tool_approval", {
+              server_id: serverId,
+              tool_name: toolName,
+              value: "ALWAYS_ALLOWED",
+            });
+          } catch (err) {
+            const detail = err instanceof Error ? err.message : String(err);
+            console.error(
+              `[set_tool_approval] failed to persist "${toolName}" to Glean: ${detail}`,
+            );
+          }
+        }
+
+        if (alwaysAllow.timedOut) {
+          const downstreamResult = await callRemoteTool(
+            remoteClient,
+            "run_tool",
+            buildRemoteArgs(serverId, toolName, resolvedArgs),
+          );
+          return {
+            ...downstreamResult,
+            content: [
+              {
+                type: "text",
+                text: alwaysAllowFollowUpTimeoutMessage(toolName),
+              },
+              ...downstreamResult.content,
+            ],
+          };
+        }
       } catch (err) {
         // Fail CLOSED. An approval gate that executes the action when the
         // prompt times out or errors defeats its own purpose — and the SDK