Preserve Pi builtins in replace mode without hypa replacements

docs/guides/pi.md

@@ -29,19 +29,19 @@ hypa init --agent pi
 - Intercepts Pi `bash` tool calls and asks Hypa for a rewrite via `hypa rewrite --json`.
 - Mutates rewritten bash commands before execution.
 - Provides `/hypa` diagnostics.
-- Registers CLI-backed tools:
-  - `hypa_shell`
-  - `hypa_read`
-  - `hypa_grep`
-  - `hypa_find`
-  - `hypa_ls`
+- Registers CLI-backed tools (replace mode takes over the Pi builtin when both are active):
+  - `hypa_shell` ← `bash`
+  - `hypa_read` ← `read`
+  - `hypa_grep` ← `grep`
+  - `hypa_find` ← `find`
+  - `hypa_ls` ← `ls`
 
 ## Configuration
 
 | Variable | Default | Description |
 |---|---|---|
 | `HYPA_BIN` | bundled `@hypabolic/hypa`, then `hypa` | Hypa executable or absolute path. |
-| `HYPA_PI_MODE` | `additive` | `additive` keeps Pi builtins; `replace` disables Pi `bash/read/grep/find/ls` after registering `hypa_*` tools. |
+| `HYPA_PI_MODE` | `additive` | `additive` keeps Pi builtins; `replace` disables each of Pi `bash/read/grep/find/ls` only while its matching `hypa_*` tool is active (fail-open if the replacement is absent, e.g. subagent/`--tools` allowlists). |
 | `HYPA_PI_REWRITE_TIMEOUT_MS` | `5000` | Rewrite CLI timeout in milliseconds. |
 | `HYPA_PI_ASK_NON_INTERACTIVE` | `deny` | `Ask` fallback when `ctx.hasUI === false`: `deny` or `allow`. |
 

packages/pi-hypa/README.md

@@ -55,7 +55,7 @@ Hypa is invoked via the platform-native binary whenever it is installed as an op
 | Variable | Default | Description |
 |---|---|---|
 | `HYPA_BIN` | bundled `@hypabolic/hypa`, then `hypa` | Hypa executable or absolute path. |
-| `HYPA_PI_MODE` | `additive` | `additive` keeps Pi builtins; `replace` disables Pi `bash/read/grep/find/ls` after registering `hypa_*` tools. |
+| `HYPA_PI_MODE` | `additive` | `additive` keeps Pi builtins; `replace` disables each of Pi `bash/read/grep/find/ls` only while its matching `hypa_*` tool is active (fail-open if the replacement is absent, e.g. subagent/`--tools` allowlists). |
 | `HYPA_PI_REWRITE_TIMEOUT_MS` | `5000` | Rewrite CLI timeout in milliseconds. |
 | `HYPA_PI_ASK_NON_INTERACTIVE` | `deny` | `Ask` fallback when `ctx.hasUI === false`: `deny` or `allow`. |
 | `HYPA_PI_ENABLE_MCP_PROXY` | `0` | Enable `hypa_mcp_proxy`, a lazy discovery/invocation bridge for upstream MCP servers configured in Hypa. |
@@ -82,15 +82,15 @@ All JSON fields are optional.
 
 ## CLI-backed tools
 
-When registered, the extension exposes Hypa-backed equivalents of Pi's file and shell builtins. In `additive` mode they sit alongside Pi's own tools; in `replace` mode they take over.
+When registered, the extension exposes Hypa-backed equivalents of Pi's file and shell builtins. In `additive` mode they sit alongside Pi's own tools; in `replace` mode each `hypa_*` tool takes over its matching builtin only when both are active.
 
-| Tool | Purpose |
-|---|---|
-| `hypa_shell` | Run shell commands with rewrite rules, compression, and evidence recording. |
-| `hypa_read` | Read files with full, outline, signatures, pruned, or smart selection. |
-| `hypa_grep` | Search file contents with safe ripgrep options. |
-| `hypa_find` | Find files with an optional result limit. |
-| `hypa_ls` | List directory contents. |
+| Tool | Replaces | Purpose |
+|---|---|---|
+| `hypa_shell` | `bash` | Run shell commands with rewrite rules, compression, and evidence recording. |
+| `hypa_read` | `read` | Read files with full, outline, signatures, pruned, or smart selection. |
+| `hypa_grep` | `grep` | Search file contents with safe ripgrep options. |
+| `hypa_find` | `find` | Find files with an optional result limit. |
+| `hypa_ls` | `ls` | List directory contents. |
 
 `hypa_*` tool outputs are capped at 50KB / 2000 lines; truncated full output is saved to a temp file for recovery.
 

packages/pi-hypa/extensions/index.ts

@@ -5,10 +5,30 @@ import { registerHypaMcpProxyBridge } from "./mcp-proxy-bridge.js";
 import { registerHypaTools } from "./tools.js";
 import type { HypaDiagnostics, RewriteStatus } from "./types.js";
 
-export const REPLACE_MODE_DISABLED_BUILTINS = new Set(["bash", "read", "grep", "find", "ls"]);
+// Pi --tools allowlists both builtins and extension tools, so a session may have
+// bash/read without hypa_* (subagent/explore). Strip a builtin only when its pair is active.
+// Builtin names match @earendil-works/pi-coding-agent dist/core/tools/* (bash, read, grep, find, ls).
+export const REPLACE_MODE_BUILTIN_REPLACEMENTS = {
+  bash: "hypa_shell",
+  read: "hypa_read",
+  grep: "hypa_grep",
+  find: "hypa_find",
+  ls: "hypa_ls",
+} as const satisfies Readonly<Record<string, string>>;
+
+export type ReplaceableBuiltin = keyof typeof REPLACE_MODE_BUILTIN_REPLACEMENTS;
+
+export function isReplaceableBuiltin(name: string): name is ReplaceableBuiltin {
+  return Object.hasOwn(REPLACE_MODE_BUILTIN_REPLACEMENTS, name);
+}
 
 export function applyReplaceModeFilter(tools: string[], mode: string): string[] {
-  return mode === "replace" ? tools.filter((name) => !REPLACE_MODE_DISABLED_BUILTINS.has(name)) : tools;
+  if (mode !== "replace") return tools;
+  const active = new Set(tools);
+  return tools.filter((name) => {
+    if (!isReplaceableBuiltin(name)) return true;
+    return !active.has(REPLACE_MODE_BUILTIN_REPLACEMENTS[name]);
+  });
 }
 
 type HypaExtensionAPI = ExtensionAPI & {
@@ -38,8 +58,10 @@ export default function (pi: ExtensionAPI) {
 
   if (config.mode === "replace") {
     pi.on("before_agent_start", () => {
-      const active = applyReplaceModeFilter(hypaPi.getActiveTools(), config.mode);
-      hypaPi.setActiveTools(active);
+      const current = hypaPi.getActiveTools();
+      const active = applyReplaceModeFilter(current, config.mode);
+      // Filter only removes; skip the write when nothing changed (common fail-open path).
+      if (active.length !== current.length) hypaPi.setActiveTools(active);
     });
   }
 

packages/pi-hypa/test/replace-mode.test.ts

@@ -1,45 +1,208 @@
 import test from "node:test";
 import assert from "node:assert/strict";
-import { applyReplaceModeFilter } from "../extensions/index.js";
+import type { ExtensionAPI } from "@earendil-works/pi-coding-agent";
+import registerHypaExtension, {
+  applyReplaceModeFilter,
+  REPLACE_MODE_BUILTIN_REPLACEMENTS,
+} from "../extensions/index.js";
 
-test("replace mode filter removes all disabled builtins", () => {
-  const tools = ["bash", "read", "grep", "find", "ls", "hypa_shell", "hypa_read", "hypa_grep", "hypa_find", "hypa_ls"];
-  const filtered = applyReplaceModeFilter(tools, "replace");
-  assert.deepEqual(filtered, ["hypa_shell", "hypa_read", "hypa_grep", "hypa_find", "hypa_ls"]);
+const PARENT_FULL = [
+  "bash",
+  "read",
+  "grep",
+  "find",
+  "ls",
+  "hypa_shell",
+  "hypa_read",
+  "hypa_grep",
+  "hypa_find",
+  "hypa_ls",
+];
+
+const PARENT_HYPA_ONLY = ["hypa_shell", "hypa_read", "hypa_grep", "hypa_find", "hypa_ls"];
+
+const SUBAGENT_NO_HYPA = ["read", "bash", "edit", "write", "grep", "find", "ls"];
+
+const EXPLORE_STYLE = ["read", "bash", "grep", "find", "ls"];
+
+const PARTIAL_ALLOWLIST = ["bash", "read", "grep", "find", "ls", "edit", "write", "hypa_read"];
+
+test("replace mode strips all builtins when all five hypa replacements are present", () => {
+  const filtered = applyReplaceModeFilter(PARENT_FULL, "replace");
+  assert.deepEqual(filtered, PARENT_HYPA_ONLY);
+});
+
+test("replace mode keeps builtins when no hypa_* tools are present (subagent)", () => {
+  assert.deepEqual(applyReplaceModeFilter(SUBAGENT_NO_HYPA, "replace"), SUBAGENT_NO_HYPA);
+});
+
+test("replace mode keeps explore-style builtin-only tool lists unchanged", () => {
+  assert.deepEqual(applyReplaceModeFilter(EXPLORE_STYLE, "replace"), EXPLORE_STYLE);
+});
+
+test("replace mode strips only builtins whose hypa replacement is present", () => {
+  const filtered = applyReplaceModeFilter(PARTIAL_ALLOWLIST, "replace");
+  assert.deepEqual(filtered, ["bash", "grep", "find", "ls", "edit", "write", "hypa_read"]);
+});
+
+test("unpaired hypa tools do not authorize stripping builtins", () => {
+  const tools = ["bash", "read", "grep", "hypa_mcp_proxy"];
+  assert.deepEqual(applyReplaceModeFilter(tools, "replace"), tools);
+});
+
+test("replace mode strips builtins when hypa replacement appears before them", () => {
+  const tools = ["hypa_read", "read", "bash"];
+  assert.deepEqual(applyReplaceModeFilter(tools, "replace"), ["hypa_read", "bash"]);
+});
+
+test("replace mode filter returns empty array unchanged", () => {
+  assert.deepEqual(applyReplaceModeFilter([], "replace"), []);
 });
 
 test("replace mode filter is a no-op when builtins are absent", () => {
   const tools = ["hypa_shell", "hypa_read", "hypa_grep"];
   assert.deepEqual(applyReplaceModeFilter(tools, "replace"), tools);
 });
 
-test("replace mode filter is idempotent", () => {
-  const tools = ["bash", "hypa_shell", "hypa_read"];
-  const once = applyReplaceModeFilter(tools, "replace");
-  const twice = applyReplaceModeFilter(once, "replace");
-  assert.deepEqual(once, twice);
+test("additive mode does not apply replace filter", () => {
+  const tools = ["bash", "read", "grep", "find", "ls", "hypa_shell"];
+  assert.deepEqual(applyReplaceModeFilter(tools, "additive"), tools);
+});
+
+test("replace mode filter is idempotent on full parent, no-hypa subagent, and partial lists", () => {
+  for (const tools of [PARENT_FULL, SUBAGENT_NO_HYPA, PARTIAL_ALLOWLIST]) {
+    const once = applyReplaceModeFilter(tools, "replace");
+    const twice = applyReplaceModeFilter(once, "replace");
+    assert.deepEqual(once, twice);
+  }
 });
 
 test("replace mode filter re-runs on subsequent turns (handles Pi reloads)", () => {
   // The filter runs on every before_agent_start — idempotency means this is safe
   // and also correct if Pi re-registers built-ins during a reload.
-  const toolsWithBuiltins = ["bash", "read", "grep", "find", "ls", "hypa_shell", "hypa_read"];
-  let activeTools = [...toolsWithBuiltins];
+  let activeTools = [...PARENT_FULL];
 
   function simulateBeforeAgentStart() {
     activeTools = applyReplaceModeFilter(activeTools, "replace");
   }
 
   simulateBeforeAgentStart();
-  assert.deepEqual(activeTools, ["hypa_shell", "hypa_read"]);
+  assert.deepEqual(activeTools, PARENT_HYPA_ONLY);
 
   // Simulate Pi re-registering builtins (e.g. after /reload) — filter must re-apply correctly
-  activeTools = [...toolsWithBuiltins];
+  activeTools = [...PARENT_FULL];
   simulateBeforeAgentStart();
-  assert.deepEqual(activeTools, ["hypa_shell", "hypa_read"]);
+  assert.deepEqual(activeTools, PARENT_HYPA_ONLY);
 });
 
-test("additive mode does not apply replace filter", () => {
-  const tools = ["bash", "read", "grep", "find", "ls", "hypa_shell"];
-  assert.deepEqual(applyReplaceModeFilter(tools, "additive"), tools);
+test("replace mode filter does not mutate the input array", () => {
+  for (const mode of ["replace", "additive"]) {
+    const tools = [...PARENT_FULL];
+    const copy = [...tools];
+    applyReplaceModeFilter(tools, mode);
+    assert.deepEqual(tools, copy);
+  }
+});
+
+type Handler = (...args: unknown[]) => unknown;
+
+function createFakePi(initialTools: string[] = []) {
+  const handlers = new Map<string, Handler[]>();
+  const registeredTools: string[] = [];
+  let activeTools = [...initialTools];
+  let setActiveToolsCalls = 0;
+  const pi = {
+    on(event: string, handler: Handler) {
+      const list = handlers.get(event) ?? [];
+      list.push(handler);
+      handlers.set(event, list);
+    },
+    registerTool(definition: Record<string, unknown>) {
+      if (typeof definition.name === "string") registeredTools.push(definition.name);
+    },
+    registerCommand() {},
+    getActiveTools() {
+      return activeTools;
+    },
+    setActiveTools(names: string[]) {
+      setActiveToolsCalls += 1;
+      activeTools = [...names];
+    },
+  };
+  return {
+    pi: pi as unknown as ExtensionAPI,
+    handlers,
+    registeredTools,
+    get setActiveToolsCalls() {
+      return setActiveToolsCalls;
+    },
+    getActiveTools: () => activeTools,
+    setActiveTools: (names: string[]) => {
+      activeTools = [...names];
+    },
+  };
+}
+
+function withEnv(env: Record<string, string | undefined>, fn: () => void) {
+  const keys = Object.keys(env);
+  const previous = new Map(keys.map((key) => [key, process.env[key]]));
+  try {
+    for (const [key, value] of Object.entries(env)) {
+      if (value === undefined) delete process.env[key];
+      else process.env[key] = value;
+    }
+    fn();
+  } finally {
+    for (const [key, value] of previous) {
+      if (value === undefined) delete process.env[key];
+      else process.env[key] = value;
+    }
+  }
+}
+
+const HOOK_TEST_ENV = {
+  HYPA_PI_MODE: "replace",
+  HYPA_PI_CONFIG: "none",
+  HYPA_PI_ENABLE_MCP_PROXY: "0",
+  HYPA_BIN: "/tmp/hypa",
+} as const;
+
+test("replace mode registers before_agent_start and filters parent vs subagent lists", () => {
+  withEnv({ ...HOOK_TEST_ENV, HYPA_PI_MODE: "replace" }, () => {
+    const fake = createFakePi(PARENT_FULL);
+    registerHypaExtension(fake.pi);
+
+    assert.equal(fake.handlers.get("before_agent_start")?.length, 1);
+
+    // Pairing map must match tools actually registered by registerHypaTools
+    const registered = new Set(fake.registeredTools);
+    for (const hypaName of Object.values(REPLACE_MODE_BUILTIN_REPLACEMENTS)) {
+      assert.equal(registered.has(hypaName), true, `expected registered tool ${hypaName}`);
+    }
+
+    for (const handler of fake.handlers.get("before_agent_start") ?? []) {
+      handler();
+    }
+    assert.deepEqual(fake.getActiveTools(), PARENT_HYPA_ONLY);
+    assert.equal(fake.setActiveToolsCalls, 1);
+
+    // Subagent-style list with no hypa replacements stays intact and skips setActiveTools
+    fake.setActiveTools(SUBAGENT_NO_HYPA);
+    const callsBefore = fake.setActiveToolsCalls;
+    for (const handler of fake.handlers.get("before_agent_start") ?? []) {
+      handler();
+    }
+    assert.deepEqual(fake.getActiveTools(), SUBAGENT_NO_HYPA);
+    assert.equal(fake.setActiveToolsCalls, callsBefore);
+  });
+});
+
+test("additive mode does not register before_agent_start replace filter", () => {
+  withEnv({ ...HOOK_TEST_ENV, HYPA_PI_MODE: "additive" }, () => {
+    const fake = createFakePi(PARENT_FULL);
+    registerHypaExtension(fake.pi);
+
+    assert.equal(fake.handlers.has("before_agent_start"), false);
+    assert.deepEqual(fake.getActiveTools(), PARENT_FULL);
+  });
 });