Merge PR #8185: fix(session): resolve wire-name aliases in hasBuiltInTool (@roboomp)

2026-08-13 01:14:50 +02:00
parent 9543be240a ec411d60b6
commit 30f078c6fa
3 changed files with 118 additions and 2 deletions
@@ -117,6 +117,7 @@

 - Clarified that the production collab relay source and binaries are not currently published, and documented the source-available local protocol relay ([#8165](https://github.com/can1357/oh-my-pi/issues/8165)).
 - Fixed snapcompact compaction shipping its redundant frame archive out of `SessionMaintenance.compact()` on both the manual RPC response (which hard-failed protocol v1 with a transport error after the compaction had already persisted) and the `auto_compaction_end` event payload (which forced the shrink ladder on every unattended pass); the archive is now stripped from both exits while the persisted compaction entry keeps it ([#8168](https://github.com/can1357/oh-my-pi/issues/8168)).
+- Fixed the edit tool showing no diff preview in `apply_patch` mode: the built-in `edit` tool presents on the wire as `apply_patch`, but the renderer-provenance gate did not resolve that alias to its built-in owner, so the edit renderer was skipped ([#8184](https://github.com/can1357/oh-my-pi/issues/8184)).

 ## [17.2.12] - 2026-08-08

@@ -379,9 +379,23 @@ export class SessionTools {
 		return this.#toolRegistry.get(name);
 	}

-	/** Whether a registry entry came from a built-in factory. */
+	/**
+	 * Whether a registry entry came from a built-in factory.
+	 *
+	 * Resolves `customWireName` aliases too: a built-in tool may present on the
+	 * wire under a different name (e.g. `edit` exposes itself as `apply_patch` in
+	 * apply_patch mode), and tool cards render the call under that wire name. An
+	 * extension registering the literal alias name shadows it — the agent loop
+	 * routes exact-name matches ahead of wire aliases — so a registered non-built-in
+	 * tool with that name wins and the alias no longer counts as built-in.
+	 */
 	hasBuiltInTool(name: string): boolean {
-		return this.#builtInToolNames.has(name);
+		if (this.#builtInToolNames.has(name)) return true;
+		if (this.#toolRegistry.has(name)) return false;
+		for (const builtInName of this.#builtInToolNames) {
+			if (this.#toolRegistry.get(builtInName)?.customWireName === name) return true;
+		}
+		return false;
 	}

 	/** Updates source provenance when a live registry entry is replaced or restored. */
@@ -0,0 +1,101 @@
+import { describe, expect, it } from "bun:test";
+import * as path from "node:path";
+import { type } from "@oh-my-pi/omptype";
+import { Agent, type AgentTool } from "@oh-my-pi/pi-agent-core";
+import { createMockModel } from "@oh-my-pi/pi-ai/providers/mock";
+import { buildModel } from "@oh-my-pi/pi-catalog/build";
+import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry";
+import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
+import { AgentSession } from "@oh-my-pi/pi-coding-agent/session/agent-session";
+import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage";
+import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager";
+import { TempDir } from "@oh-my-pi/pi-utils";
+
+function makeTool(name: string, customWireName?: string): AgentTool {
+	return {
+		name,
+		label: name,
+		description: `${name} tool`,
+		parameters: type({}),
+		...(customWireName ? { customWireName } : {}),
+		async execute() {
+			return { content: [{ type: "text", text: name }] };
+		},
+	};
+}
+
+async function withSession(
+	tools: readonly AgentTool[],
+	builtInToolNames: readonly string[],
+	run: (session: AgentSession) => void,
+): Promise<void> {
+	const tempDir = TempDir.createSync("@apply-patch-preview-");
+	const authStorage = await AuthStorage.create(path.join(tempDir.path(), "auth.db"));
+	authStorage.setRuntimeApiKey("anthropic", "test-key");
+	const settings = Settings.isolated({ "compaction.enabled": false });
+	const model = buildModel({
+		id: "mock",
+		name: "mock",
+		api: "openai-responses",
+		provider: "openai",
+		baseUrl: "https://example.invalid",
+		reasoning: false,
+		input: ["text"],
+		cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 },
+		contextWindow: 8192,
+		maxTokens: 2048,
+	});
+	const agent = new Agent({
+		getApiKey: () => "test-key",
+		initialState: { model, systemPrompt: ["initial"], tools: [...tools] },
+		streamFn: createMockModel({ responses: [{ content: ["ok"] }] }).stream,
+	});
+	const session = new AgentSession({
+		agent,
+		sessionManager: SessionManager.inMemory(tempDir.path()),
+		settings,
+		modelRegistry: new ModelRegistry(authStorage, path.join(tempDir.path(), "models.yml")),
+		toolRegistry: new Map<string, AgentTool>(tools.map(tool => [tool.name, tool])),
+		builtInToolNames: [...builtInToolNames],
+		rebuildSystemPrompt: async toolNames => ({ systemPrompt: [toolNames.join(",")] }),
+	});
+	try {
+		run(session);
+	} finally {
+		await session.dispose();
+		authStorage.close();
+		tempDir.removeSync();
+	}
+}
+
+/**
+ * Regression #8184: the built-in `edit` tool presents on the wire as
+ * `apply_patch` in apply_patch mode (`customWireName`). Tool cards render the
+ * streamed call under that wire name, and the renderer registry is gated behind
+ * `hasBuiltInTool(name)`. If the alias does not resolve to its built-in owner,
+ * the edit renderer/preview is skipped for apply_patch-mode edits.
+ */
+describe("AgentSession.hasBuiltInTool wire-name aliases", () => {
+	it("resolves a built-in tool's customWireName alias to built-in provenance", async () => {
+		// Mirrors `edit` in apply_patch mode: internal name `edit`, wire name
+		// `apply_patch`.
+		const edit = makeTool("edit", "apply_patch");
+		await withSession([edit], ["edit"], session => {
+			expect(session.hasBuiltInTool("edit")).toBe(true);
+			// The wire alias must resolve to its built-in owner so the edit
+			// renderer/preview is used for apply_patch-mode cards.
+			expect(session.hasBuiltInTool("apply_patch")).toBe(true);
+		});
+	});
+
+	it("lets an extension registering the literal alias name shadow the built-in", async () => {
+		// The agent loop routes exact-name matches ahead of wire aliases, so a
+		// non-built-in tool registered under the literal `apply_patch` name wins;
+		// it must not reuse the built-in edit renderer.
+		const edit = makeTool("edit", "apply_patch");
+		const shadow = makeTool("apply_patch");
+		await withSession([edit, shadow], ["edit"], session => {
+			expect(session.hasBuiltInTool("apply_patch")).toBe(false);
+		});
+	});
+});