fix: address 8 PR review comments

1. approval.ts: mkdir ~/.hyperagent/ before writing (first-run case)
2. config.ts: hash includes allowTools, denyTools, env key names
3. approval.ts: isMCPApproved checks tool list matches approved tools
4. plugin-adapter.ts: quote property names that aren't valid JS identifiers
5. plugins/mcp/index.ts: fix comment to match implementation (sentinel module)
6. index.ts: module_info author fixed to 'system' (was 'mcp')
7. validator.rs: arrow handler param check distinguishes () => from (event) =>
8. validator.rs: strip_js_comments guards against regex literal // sequences

Signed-off-by: Simon Davies <simongdavies@users.noreply.github.com>

Author: simongdavies

plugins/mcp/index.ts

@@ -56,12 +56,12 @@ Note: MCP servers are OS processes (not micro-VM sandboxed).
 // ── Host functions ───────────────────────────────────────────────────
 
 /**
- * The MCP gateway plugin provides no host functions.
- * Its hostModules array is empty — it exists solely to gate the
- * MCP subsystem via the plugin approval flow.
+ * The MCP gateway plugin provides a single sentinel host module,
+ * `mcp-gateway`, used to signal that the MCP subsystem is enabled
+ * via the plugin approval flow.
  *
  * Individual MCP servers register their own host modules dynamically
- * when enabled via /mcp enable.
+ * (`host:mcp-<name>`) when enabled via /mcp enable.
  */
 export function createHostFunctions(
   _config?: MCPGatewayConfig,

src/agent/index.ts

@@ -3902,7 +3902,7 @@ const moduleInfoTool = defineTool("module_info", {
           return {
             name: `mcp-${serverName}`,
             description: `MCP server "${serverName}" — ${conn.tools.length} tool(s)`,
-            author: "mcp",
+            author: "system",
             importAs: `import { ... } from "host:mcp-${serverName}"`,
             exports: conn.tools.map((t) => ({
               name: t.name,

src/agent/mcp/approval.ts

@@ -4,8 +4,8 @@
 // before a server can be connected. Approval is invalidated if the
 // server config (command + args) changes.
 
-import { readFileSync, writeFileSync, existsSync } from "node:fs";
-import { join } from "node:path";
+import { readFileSync, writeFileSync, existsSync, mkdirSync } from "node:fs";
+import { join, dirname } from "node:path";
 import { homedir } from "node:os";
 
 import type {
@@ -36,6 +36,11 @@ export function loadMCPApprovalStore(): MCPApprovalStore {
  */
 function saveMCPApprovalStore(store: MCPApprovalStore): void {
   try {
+    // Ensure directory exists (first-run case)
+    const dir = dirname(APPROVAL_FILE);
+    if (!existsSync(dir)) {
+      mkdirSync(dir, { recursive: true, mode: 0o700 });
+    }
     writeFileSync(APPROVAL_FILE, JSON.stringify(store, null, 2), {
       mode: 0o600,
     });
@@ -53,12 +58,27 @@ export function isMCPApproved(
   name: string,
   config: MCPServerConfig,
   store: MCPApprovalStore,
+  currentTools?: string[],
 ): boolean {
   const record = store[name];
   if (!record) return false;
 
   const currentHash = computeMCPConfigHash(name, config);
-  return record.configHash === currentHash;
+  if (record.configHash !== currentHash) return false;
+
+  // If tool list is provided, verify it matches approval-time tools
+  if (currentTools && Array.isArray(record.approvedTools)) {
+    const approved = [...record.approvedTools].sort();
+    const current = [...currentTools].sort();
+    if (
+      approved.length !== current.length ||
+      !approved.every((t, i) => t === current[i])
+    ) {
+      return false; // Tool set changed — re-approval required
+    }
+  }
+
+  return true;
 }
 
 /**

src/agent/mcp/config.ts

@@ -223,16 +223,22 @@ function substituteEnvVars(value: string): string {
 
 /**
  * Compute a config hash for approval validation.
- * Hash = SHA-256(name + command + JSON.stringify(args)).
- * Config changes invalidate the approval.
+ * Includes name, command, args, tool filtering, and env key names.
+ * Any execution-affecting config change invalidates the approval.
  */
 export function computeMCPConfigHash(
   name: string,
   config: MCPServerConfig,
 ): string {
-  return createHash("sha256")
-    .update(name, "utf8")
-    .update(config.command, "utf8")
-    .update(JSON.stringify(config.args ?? []), "utf8")
-    .digest("hex");
+  return (
+    createHash("sha256")
+      .update(name, "utf8")
+      .update(config.command, "utf8")
+      .update(JSON.stringify(config.args ?? []), "utf8")
+      .update(JSON.stringify(config.allowTools ?? []), "utf8")
+      .update(JSON.stringify(config.denyTools ?? []), "utf8")
+      // Hash env key names (not values — secrets stay out of the hash)
+      .update(JSON.stringify(Object.keys(config.env ?? {}).sort()), "utf8")
+      .digest("hex")
+  );
 }

src/agent/mcp/plugin-adapter.ts

@@ -138,7 +138,11 @@ function generateInputInterface(tool: MCPToolSchema): string | null {
     const desc = propSchema.description
       ? ` /** ${propSchema.description} */\n`
       : "";
-    lines.push(`${desc}  ${propName}${optional}: ${tsType};`);
+    // Quote property names that aren't valid JS identifiers
+    const safeName = /^[a-zA-Z_$][a-zA-Z0-9_$]*$/.test(propName)
+      ? propName
+      : `"${propName}"`;
+    lines.push(`${desc}  ${safeName}${optional}: ${tsType};`);
   }
 
   lines.push("}");

src/code-validator/guest/runtime/src/validator.rs

@@ -923,10 +923,18 @@ fn strip_js_comments(source: &str) -> String {
                 }
             }
             // Single-line comment — replace with spaces (preserve line structure)
+            // Guard: if preceded by a backslash, this might be inside a regex
+            // literal (e.g. /https?:\/\//). Skip the comment treatment.
             b'/' if i + 1 < len && bytes[i + 1] == b'/' => {
-                while i < len && bytes[i] != b'\n' {
-                    out.push(' ');
+                if i > 0 && bytes[i - 1] == b'\\' {
+                    // Likely inside a regex literal — keep as-is
+                    out.push('/');
                     i += 1;
+                } else {
+                    while i < len && bytes[i] != b'\n' {
+                        out.push(' ');
+                        i += 1;
+                    }
                 }
             }
             // Block comment — replace with spaces, preserve newlines
@@ -1085,10 +1093,29 @@ fn check_handler_has_param(source: &str) -> bool {
             }
         }
         // Arrow: const handler = (event) => or handler = event =>
+        // Must verify it actually has a parameter, not just () =>
         if (trimmed.contains("const handler") || trimmed.contains("let handler"))
             && trimmed.contains("=>")
         {
-            return true;
+            // Check for non-empty parens: handler = (something) =>
+            if let Some(eq_pos) = trimmed.find('=') {
+                let after_eq = trimmed[eq_pos + 1..].trim_start();
+                // Skip any second '=' (==)
+                if after_eq.starts_with('=') {
+                    // Not an assignment — skip
+                } else if after_eq.starts_with('(') {
+                    // Parenthesised params: check for non-empty (x) vs ()
+                    if let Some(close) = after_eq.find(')') {
+                        let params = after_eq[1..close].trim();
+                        if !params.is_empty() {
+                            return true;
+                        }
+                    }
+                } else if !after_eq.starts_with("=>") {
+                    // Bare param without parens: handler = event => ...
+                    return true;
+                }
+            }
         }
     }
     false

tests/mcp.test.ts

@@ -149,6 +149,41 @@ describe("computeMCPConfigHash", () => {
     const hash2 = computeMCPConfigHash("beta", config);
     expect(hash1).not.toBe(hash2);
   });
+
+  it("changes when allowTools change", () => {
+    const base = { command: "node" };
+    const hash1 = computeMCPConfigHash("test", {
+      ...base,
+      allowTools: ["a"],
+    });
+    const hash2 = computeMCPConfigHash("test", {
+      ...base,
+      allowTools: ["a", "b"],
+    });
+    expect(hash1).not.toBe(hash2);
+  });
+
+  it("changes when denyTools change", () => {
+    const base = { command: "node" };
+    const hash1 = computeMCPConfigHash("test", {
+      ...base,
+      denyTools: ["x"],
+    });
+    const hash2 = computeMCPConfigHash("test", { ...base, denyTools: [] });
+    expect(hash1).not.toBe(hash2);
+  });
+
+  it("changes when env keys change (not values)", () => {
+    const hash1 = computeMCPConfigHash("test", {
+      command: "node",
+      env: { TOKEN: "secret1" },
+    });
+    const hash2 = computeMCPConfigHash("test", {
+      command: "node",
+      env: { API_KEY: "secret2" },
+    });
+    expect(hash1).not.toBe(hash2);
+  });
 });
 
 // ── Sanitisation ─────────────────────────────────────────────────────