From d4a947b525fb8aa6f3e96355ab52d494398888d9 Mon Sep 17 00:00:00 2001 From: Matthew Gribben Date: Thu, 13 Aug 2026 10:34:16 +1000 Subject: [PATCH] Preserve Pi builtins in replace mode without hypa replacements Strip a Pi builtin only when its paired hypa_* tool is active in the same session. Subagent allowlists that omit hypa_* keep bash/read/grep/find/ls instead of being left with only edit/write. Closes #93 --- docs/guides/pi.md | 14 +- packages/pi-hypa/README.md | 18 +- packages/pi-hypa/extensions/index.ts | 30 +++- packages/pi-hypa/test/replace-mode.test.ts | 199 +++++++++++++++++++-- 4 files changed, 223 insertions(+), 38 deletions(-) diff --git a/docs/guides/pi.md b/docs/guides/pi.md index 3929ad9..86d7606 100644 --- a/docs/guides/pi.md +++ b/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`. | diff --git a/packages/pi-hypa/README.md b/packages/pi-hypa/README.md index b4c44ea..8c5747d 100644 --- a/packages/pi-hypa/README.md +++ b/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. diff --git a/packages/pi-hypa/extensions/index.ts b/packages/pi-hypa/extensions/index.ts index cc9c6c3..9357217 100644 --- a/packages/pi-hypa/extensions/index.ts +++ b/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>; + +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); }); } diff --git a/packages/pi-hypa/test/replace-mode.test.ts b/packages/pi-hypa/test/replace-mode.test.ts index 2295ec4..dcea3f1 100644 --- a/packages/pi-hypa/test/replace-mode.test.ts +++ b/packages/pi-hypa/test/replace-mode.test.ts @@ -1,11 +1,62 @@ 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", () => { @@ -13,33 +64,145 @@ test("replace mode filter is a no-op when builtins are absent", () => { 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(); + 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) { + 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, 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); + }); });