fix(coding-agent): address PR #1388 review feedback

Refactor:
- session.ts: extract mapDebugpyMissingModule helper; replace the duplicated
  inline check in launch/attach catch blocks. Add jsdoc on DapStartRequestFailure.settled
  documenting per-call ownership and how throwPreferredDapStartError consumes it.
- path-utils.ts: replace the no-op keepOpaqueResourceUri branch with an
  OPAQUE_RESOURCE_SCHEMES Set so the structure carries the intent. Functionally
  equivalent; new opaque schemes become a one-line Set change.

Tests:
- dap-launch-failures: cover the debugpy stderr -> 'pip install debugpy'
  rewrite for launch and attach, plus a negative case (non-debugpy adapter
  with the substring in stderr is left untouched).
- dap-launch-failures: model the delayed-launch-failure case the new
  settled-race in throwPreferredDapStartError defends against. FakeDapClient
  gains optional launchErrorDelayMs/attachErrorDelayMs.
- dap-launch-failures (DebugTool): assert adapter:'debugpy' early-throw
  surfaces 'python not found in PATH' on both launch and attach when
  selectLaunchAdapter/selectAttachAdapter return null, and the unspecified-adapter
  path still falls back to the generic 'No debugger adapter' error.
- find.ts: export validateFindPathInputs and pin the new backslash-escape
  semantics (\, no longer trips the comma-joined heuristic) plus the
  existing brace-expansion and rejection paths.
- patch.ts: cover the post-write verification error message. The user-facing
  ToolError must contain the caller-supplied relative path and not the
  absolute resolvedPath (which still lives in the structured context for
  log correlation).
- split-internal-url-sel: reword two mcp:// test comments that described a
  'peeler refuses' guard that doesn't exist; rename the tests to reflect the
  actual opaque-scheme rule.

packages/coding-agent/src/dap/session.ts

@@ -108,6 +108,14 @@ function toErrorMessage(value: unknown): string {
 interface DapStartRequestFailure {
 	rejected: boolean;
 	error?: unknown;
+	/**
+	 * Resolves (never rejects) when the underlying launch/attach request
+	 * settles either way. Set by {@link trackDapStartRequest} on each call,
+	 * so a single failure object must not be reused across launch attempts.
+	 * Consumed by {@link throwPreferredDapStartError} to bound how long to
+	 * wait for a delayed adapter-side rejection before falling back to the
+	 * cascade error from configurationDone.
+	 */
 	settled?: Promise<void>;
 }
 
@@ -146,6 +154,21 @@ async function throwPreferredDapStartError(
 	}
 	throw configurationError;
 }
+
+const DEBUGPY_MISSING_MODULE_RE = /No module named ['"]?debugpy['"]?/;
+
+/**
+ * Map a generic adapter-side failure into the targeted `pip install debugpy`
+ * hint when the adapter is debugpy and stderr/the wrapping error mentions
+ * the missing module. Returns null when the heuristic does not apply, so the
+ * caller can rethrow the original error untouched.
+ */
+function mapDebugpyMissingModule(adapterName: string, error: unknown): Error | null {
+	if (adapterName !== "debugpy") return null;
+	if (!DEBUGPY_MISSING_MODULE_RE.test(toErrorMessage(error))) return null;
+	return new Error("adapter 'debugpy' is not available: install with 'pip install debugpy'");
+}
+
 function normalizePath(filePath: string): string {
 	return path.resolve(filePath);
 }
@@ -280,9 +303,8 @@ export class DapSessionManager {
 			return buildSummary(session);
 		} catch (error) {
 			await this.#disposeSession(session);
-			if (options.adapter.name === "debugpy" && /No module named ['"]?debugpy['"]?/.test(toErrorMessage(error))) {
-				throw new Error("adapter 'debugpy' is not available: install with 'pip install debugpy'");
-			}
+			const mapped = mapDebugpyMissingModule(options.adapter.name, error);
+			if (mapped) throw mapped;
 			throw error;
 		}
 	}
@@ -339,9 +361,8 @@ export class DapSessionManager {
 			return buildSummary(session);
 		} catch (error) {
 			await this.#disposeSession(session);
-			if (options.adapter.name === "debugpy" && /No module named ['"]?debugpy['"]?/.test(toErrorMessage(error))) {
-				throw new Error("adapter 'debugpy' is not available: install with 'pip install debugpy'");
-			}
+			const mapped = mapDebugpyMissingModule(options.adapter.name, error);
+			if (mapped) throw mapped;
 			throw error;
 		}
 	}

packages/coding-agent/src/tools/find.ts

@@ -59,7 +59,7 @@ const MAX_GLOB_TIMEOUT_MS = 60_000;
  * Commas inside brace expansion (`{a,b}`) are legitimate glob syntax and
  * must pass through.
  */
-function validateFindPathInputs(paths: readonly string[]): void {
+export function validateFindPathInputs(paths: readonly string[]): void {
 	for (const entry of paths) {
 		let braceDepth = 0;
 		for (let i = 0; i < entry.length; i++) {

packages/coding-agent/src/tools/path-utils.ts

@@ -29,6 +29,13 @@ const INTERNAL_SCHEMES_WITH_SELECTORS: Record<string, true> = {
 	rule: true,
 	skill: true,
 };
+// Schemes whose resource URIs are server-defined and may legitimately end
+// with selector-shaped tails (e.g. `:raw`, `:conflicts`, `:1-50`, `/:raw`).
+// `McpProtocolHandler` resolves by exact URI match (`r.uri === uri`), so
+// peeling syntactically can make valid resources unreachable. Keep these
+// schemes opaque; selector support for them needs a resolver-aware path that
+// tries the exact URI before interpreting any suffix as a read selector.
+const OPAQUE_RESOURCE_SCHEMES: ReadonlySet<string> = new Set(["mcp"]);
 const INTERNAL_URL_SCHEME_RE = /^([a-z][a-z0-9+.-]*):\/\//i;
 const NARROW_NO_BREAK_SPACE = "\u202F";
 const TOP_LEVEL_INTERNAL_URL_PREFIXES = [
@@ -173,27 +180,16 @@ export function splitPathAndSel(rawPath: string): { path: string; sel?: string }
  *
  * Falls back to the input unchanged when nothing matches.
  */
-/** MCP resource URIs are server-defined and may legitimately end with
- * selector-shaped tails like `:raw`, `:conflicts`, `:1-50`, or even `/:raw`.
- * `McpProtocolHandler` resolves by exact resource URI match (`r.uri === uri`),
- * so syntactically peeling a selector here can make valid resources
- * unreachable. Keep mcp:// opaque; selector support for MCP resources needs a
- * resolver-aware path that can try the exact URI before interpreting a suffix
- * as a read selector. */
-function keepOpaqueResourceUri(rawPath: string): { path: string } {
-	return { path: rawPath };
-}
 
 export function splitInternalUrlSel(rawPath: string): { path: string; sel?: string } {
 	const schemeMatch = rawPath.match(INTERNAL_URL_SCHEME_RE);
 	if (!schemeMatch) return { path: rawPath };
 	const scheme = schemeMatch[1].toLowerCase();
-	// `mcp://` resource URIs are server-defined and may legitimately contain
-	// selector-shaped suffixes. Keep them opaque so exact resource lookup wins.
-	if (!INTERNAL_SCHEMES_WITH_SELECTORS[scheme]) {
-		if (scheme === "mcp") return keepOpaqueResourceUri(rawPath);
-		return { path: rawPath };
-	}
+	// Opaque schemes (mcp://, etc.) carry server-defined resource URIs that may
+	// legitimately end in selector-shaped tails. Forward verbatim — see
+	// OPAQUE_RESOURCE_SCHEMES.
+	if (OPAQUE_RESOURCE_SCHEMES.has(scheme)) return { path: rawPath };
+	if (!INTERNAL_SCHEMES_WITH_SELECTORS[scheme]) return { path: rawPath };
 
 	const schemeEnd = schemeMatch[0].length;
 	let path = rawPath;

packages/coding-agent/test/debug/dap-launch-failures.test.ts

@@ -35,7 +35,9 @@ class FakeDapClient {
 		readonly cwd: string,
 		readonly options: {
 			launchError?: string;
+			launchErrorDelayMs?: number;
 			attachError?: string;
+			attachErrorDelayMs?: number;
 			configurationDoneError?: string;
 			rejectStopWaiters?: boolean;
 		},
@@ -62,9 +64,11 @@ class FakeDapClient {
 
 	async sendRequest(command: string): Promise<unknown> {
 		if (command === "launch" && this.options.launchError) {
+			if (this.options.launchErrorDelayMs) await Bun.sleep(this.options.launchErrorDelayMs);
 			throw new Error(this.options.launchError);
 		}
 		if (command === "attach" && this.options.attachError) {
+			if (this.options.attachErrorDelayMs) await Bun.sleep(this.options.attachErrorDelayMs);
 			throw new Error(this.options.attachError);
 		}
 		if (command === "configurationDone" && this.options.configurationDoneError) {
@@ -198,6 +202,90 @@ describe("DAP launch failure handling", () => {
 		expect(message).toContain("ENOENT");
 		expect(message).toContain(TEST_ADAPTER.name);
 	});
+
+	it("surfaces 'pip install debugpy' when launch stderr mentions missing module", async () => {
+		const manager = new DapSessionManager();
+		const debugpyAdapter: DapResolvedAdapter = { ...TEST_ADAPTER, name: "debugpy" };
+		const fake = new FakeDapClient(debugpyAdapter, process.cwd(), {
+			launchError: "ImportError: No module named 'debugpy'",
+		});
+		spyOn(DapClient, "spawn").mockResolvedValue(fake as unknown as DapClient);
+
+		let message = "";
+		try {
+			await manager.launch({ adapter: debugpyAdapter, program: "/bin/echo", cwd: process.cwd() });
+		} catch (error) {
+			expect(error).toBeInstanceOf(Error);
+			message = (error as Error).message;
+		}
+
+		expect(message).toContain("pip install debugpy");
+		expect(message).toContain("debugpy");
+	});
+
+	it("surfaces 'pip install debugpy' when attach stderr mentions missing module", async () => {
+		const manager = new DapSessionManager();
+		const debugpyAdapter: DapResolvedAdapter = { ...TEST_ADAPTER, name: "debugpy" };
+		const fake = new FakeDapClient(debugpyAdapter, process.cwd(), {
+			attachError: 'ModuleNotFoundError: No module named "debugpy"',
+		});
+		spyOn(DapClient, "spawn").mockResolvedValue(fake as unknown as DapClient);
+
+		let message = "";
+		try {
+			await manager.attach({ adapter: debugpyAdapter, cwd: process.cwd(), pid: 123 });
+		} catch (error) {
+			message = (error as Error).message;
+		}
+
+		expect(message).toContain("pip install debugpy");
+	});
+
+	it("does NOT rewrite to 'pip install debugpy' for non-debugpy adapters even when stderr mentions the module", async () => {
+		const manager = new DapSessionManager();
+		const fake = new FakeDapClient(TEST_ADAPTER, process.cwd(), {
+			launchError: "incidental log line: No module named debugpy was here but the adapter is lldb-dap",
+		});
+		spyOn(DapClient, "spawn").mockResolvedValue(fake as unknown as DapClient);
+
+		let message = "";
+		try {
+			await manager.launch({ adapter: TEST_ADAPTER, program: "/bin/echo", cwd: process.cwd() });
+		} catch (error) {
+			message = (error as Error).message;
+		}
+
+		expect(message).not.toContain("pip install debugpy");
+		expect(message).toContain("incidental log line");
+	});
+
+	it("prefers a delayed launch failure over the configurationDone cascade", async () => {
+		// Models real adapter I/O where the launch failure arrives via socket
+		// several ticks after configurationDone has already rejected. The old
+		// `await Promise.resolve()` (one microtask) would miss the late launch
+		// rejection and surface only the configurationDone cascade.
+		const manager = new DapSessionManager();
+		const fake = new FakeDapClient(TEST_ADAPTER, process.cwd(), {
+			launchError: "launch: 'C:\\repo\\program' is not a valid executable",
+			launchErrorDelayMs: 10,
+			configurationDoneError: "configurationDone: Expected process to be stopped.",
+		});
+		spyOn(DapClient, "spawn").mockResolvedValue(fake as unknown as DapClient);
+
+		let message = "";
+		try {
+			await manager.launch({ adapter: TEST_ADAPTER, program: "C:\\repo\\program", cwd: process.cwd() });
+		} catch (error) {
+			message = (error as Error).message;
+		}
+
+		// The combined error must include the launch failure as the preferred
+		// error — not just the configurationDone cascade. Both messages are
+		// present in the combined form (see combineDapStartErrors), but the
+		// regression-prone case is omitting the launch line entirely.
+		expect(message).toContain("launch: 'C:\\repo\\program' is not a valid executable");
+		expect(message).toContain("configurationDone: Expected process to be stopped.");
+	});
 });
 
 describe("DebugTool launch validation", () => {
@@ -221,4 +309,84 @@ describe("DebugTool launch validation", () => {
 			await fs.rm(cwd, { recursive: true, force: true });
 		}
 	});
+
+	it("throws targeted 'python not found in PATH' when adapter:'debugpy' is unresolvable for launch", async () => {
+		const dapModule = await import("../../src/dap");
+		const launchSpy = spyOn(dapModule, "selectLaunchAdapter").mockReturnValue(null);
+		try {
+			const cwd = await fs.mkdtemp(path.join(os.tmpdir(), "omp-debug-debugpy-"));
+			try {
+				await fs.writeFile(path.join(cwd, "main.py"), "print('hi')");
+				const session: ToolSession = {
+					cwd,
+					hasUI: false,
+					getSessionFile: () => null,
+					getSessionSpawns: () => "*",
+					settings: Settings.isolated({ "debug.enabled": true }),
+				};
+				const tool = new DebugTool(session);
+
+				await expect(
+					tool.execute("call", { action: "launch", program: "main.py", adapter: "debugpy" }),
+				).rejects.toThrow(/debugpy.*python not found in PATH/);
+			} finally {
+				await fs.rm(cwd, { recursive: true, force: true });
+			}
+		} finally {
+			launchSpy.mockRestore();
+		}
+	});
+
+	it("throws targeted 'python not found in PATH' when adapter:'debugpy' is unresolvable for attach", async () => {
+		const dapModule = await import("../../src/dap");
+		const attachSpy = spyOn(dapModule, "selectAttachAdapter").mockReturnValue(null);
+		try {
+			const cwd = await fs.mkdtemp(path.join(os.tmpdir(), "omp-debug-debugpy-attach-"));
+			try {
+				const session: ToolSession = {
+					cwd,
+					hasUI: false,
+					getSessionFile: () => null,
+					getSessionSpawns: () => "*",
+					settings: Settings.isolated({ "debug.enabled": true }),
+				};
+				const tool = new DebugTool(session);
+
+				await expect(tool.execute("call", { action: "attach", pid: 1234, adapter: "debugpy" })).rejects.toThrow(
+					/debugpy.*python not found in PATH/,
+				);
+			} finally {
+				await fs.rm(cwd, { recursive: true, force: true });
+			}
+		} finally {
+			attachSpy.mockRestore();
+		}
+	});
+
+	it("falls back to the generic 'No debugger adapter' error when adapter is unspecified", async () => {
+		const dapModule = await import("../../src/dap");
+		const launchSpy = spyOn(dapModule, "selectLaunchAdapter").mockReturnValue(null);
+		try {
+			const cwd = await fs.mkdtemp(path.join(os.tmpdir(), "omp-debug-noadapter-"));
+			try {
+				await fs.writeFile(path.join(cwd, "main.py"), "print('hi')");
+				const session: ToolSession = {
+					cwd,
+					hasUI: false,
+					getSessionFile: () => null,
+					getSessionSpawns: () => "*",
+					settings: Settings.isolated({ "debug.enabled": true }),
+				};
+				const tool = new DebugTool(session);
+
+				await expect(tool.execute("call", { action: "launch", program: "main.py" })).rejects.toThrow(
+					/No debugger adapter available/,
+				);
+			} finally {
+				await fs.rm(cwd, { recursive: true, force: true });
+			}
+		} finally {
+			launchSpy.mockRestore();
+		}
+	});
 });

packages/coding-agent/test/edit-patch-unchanged-error.test.ts

@@ -0,0 +1,112 @@
+import { afterEach, beforeEach, describe, expect, test } from "bun:test";
+import * as fs from "node:fs/promises";
+import * as os from "node:os";
+import * as path from "node:path";
+import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
+import { DEFAULT_FUZZY_THRESHOLD, executePatchSingle } from "@oh-my-pi/pi-coding-agent/edit";
+import type { FileDiagnosticsResult } from "@oh-my-pi/pi-coding-agent/lsp";
+import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools";
+
+function makeSession(cwd: string): ToolSession {
+	return {
+		cwd,
+		hasUI: false,
+		getSessionFile: () => null,
+		getSessionSpawns: () => "*",
+		enableLsp: false,
+		settings: Settings.isolated({ "edit.mode": "patch" }),
+		getArtifactsDir: () => null,
+		getSessionId: () => null,
+		getPlanModeState: () => undefined,
+	} as unknown as ToolSession;
+}
+
+const noopBeginDeferred = (_p: string) => ({
+	onDeferredDiagnostics: () => {},
+	signal: new AbortController().signal,
+	finalize: () => {},
+});
+
+/**
+ * Simulates an LSP host integration that claims success without persisting the
+ * write — the failure mode the post-write verification block in `patch.ts` is
+ * defending against. Unlike `writethroughNoop`, this really doesn't touch the
+ * filesystem.
+ */
+async function silentlySwallowingWritethrough(): Promise<FileDiagnosticsResult | undefined> {
+	return undefined;
+}
+
+let tempDir: string;
+
+beforeEach(async () => {
+	resetSettingsForTest();
+	tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-patch-unchanged-"));
+	await Settings.init({ inMemory: true, cwd: tempDir });
+});
+
+afterEach(async () => {
+	resetSettingsForTest();
+	await fs.rm(tempDir, { recursive: true, force: true });
+});
+
+describe("executePatchSingle — post-write verification error path", () => {
+	test("error message contains the caller-supplied relative path and not the absolute resolvedPath", async () => {
+		const relPath = "deep/nested/foo.txt";
+		await fs.mkdir(path.join(tempDir, "deep", "nested"), { recursive: true });
+		await fs.writeFile(path.join(tempDir, relPath), "a\n");
+
+		let caught: Error | undefined;
+		try {
+			await executePatchSingle({
+				session: makeSession(tempDir),
+				path: relPath,
+				params: { op: "update", diff: "@@\n-a\n+b" },
+				allowFuzzy: true,
+				fuzzyThreshold: DEFAULT_FUZZY_THRESHOLD,
+				writethrough: silentlySwallowingWritethrough,
+				beginDeferredDiagnosticsForPath: noopBeginDeferred,
+			});
+		} catch (err) {
+			caught = err as Error;
+		}
+
+		expect(caught).toBeInstanceOf(Error);
+		const message = caught?.message ?? "";
+
+		// The relative path supplied by the caller must appear in the
+		// user-facing error — it's what the outer composer in `executeSinglePathEntries`
+		// uses in its `Error editing ${path}: …` wrapper.
+		expect(message).toContain(relPath);
+
+		// The absolute resolved path must NOT appear in the user-facing
+		// message — leaking it embeds `$HOME`/`os.tmpdir()` in the TUI and
+		// double-embeds the path when the outer composer prepends its own.
+		// resolvedPath still lives in the structured `context` metadata.
+		expect(message).not.toContain(tempDir);
+	});
+
+	test("ToolError still carries the absolute resolvedPath in its structured context for log correlation", async () => {
+		const relPath = "foo.txt";
+		await fs.writeFile(path.join(tempDir, relPath), "a\n");
+
+		let caught: Error | undefined;
+		try {
+			await executePatchSingle({
+				session: makeSession(tempDir),
+				path: relPath,
+				params: { op: "update", diff: "@@\n-a\n+b" },
+				allowFuzzy: true,
+				fuzzyThreshold: DEFAULT_FUZZY_THRESHOLD,
+				writethrough: silentlySwallowingWritethrough,
+				beginDeferredDiagnosticsForPath: noopBeginDeferred,
+			});
+		} catch (err) {
+			caught = err as Error;
+		}
+
+		expect(caught).toBeInstanceOf(Error);
+		const context = (caught as Error & { context?: { path?: string } }).context;
+		expect(context?.path).toBe(path.join(tempDir, relPath));
+	});
+});

packages/coding-agent/test/tools/find-validate-paths.test.ts

@@ -0,0 +1,42 @@
+import { describe, expect, it } from "bun:test";
+import { validateFindPathInputs } from "../../src/tools/find";
+
+describe("validateFindPathInputs", () => {
+	it("accepts a normal array of glob entries", () => {
+		expect(() => validateFindPathInputs(["src/**/*.ts", "test/**/*.ts"])).not.toThrow();
+	});
+
+	it('rejects comma-joined entries (the `["a,b"]` shape)', () => {
+		expect(() => validateFindPathInputs(["a.py,b.py"])).toThrow(/paths is an array/);
+	});
+
+	it("allows commas inside brace expansion", () => {
+		expect(() => validateFindPathInputs(["src/{a,b}/*.ts"])).not.toThrow();
+		expect(() => validateFindPathInputs(["{foo,bar,baz}.md"])).not.toThrow();
+	});
+
+	it("allows backslash-escaped commas at top level (matches search.ts:containsTopLevelComma)", () => {
+		// Backslash-escapes a literal comma in a filename — must not trip the
+		// array-vs-string heuristic.
+		expect(() => validateFindPathInputs(["weird\\,name.txt"])).not.toThrow();
+		expect(() => validateFindPathInputs(["a\\,b\\,c"])).not.toThrow();
+	});
+
+	it("still rejects unescaped top-level commas mixed with escaped ones", () => {
+		// `a\,b,c` — the second comma is unescaped, so the heuristic should fire.
+		expect(() => validateFindPathInputs(["a\\,b,c"])).toThrow(/paths is an array/);
+	});
+
+	it("allows a trailing backslash without crashing", () => {
+		// `foo\\` is a backslash at end-of-string; the i+1<length guard must hold.
+		expect(() => validateFindPathInputs(["foo\\"])).not.toThrow();
+	});
+
+	it("treats `\\{a,b}` as an escaped brace, so the inner comma is still top-level", () => {
+		// Skip-next semantics: the backslash consumes the `{`, so braceDepth stays 0
+		// and the unescaped `,` between `a` and `b` rejects. This pins the literal
+		// behavior of the new escape-skip, which intentionally does NOT model glob
+		// brace semantics — it only mirrors search.ts's containsTopLevelComma.
+		expect(() => validateFindPathInputs(["\\{a,b}"])).toThrow(/paths is an array/);
+	});
+});

packages/coding-agent/test/tools/split-internal-url-sel.test.ts

@@ -95,16 +95,19 @@ describe("splitInternalUrlSel", () => {
 		});
 	});
 
-	it("does not peel when the only slash before the colon is part of the `://` separator", () => {
-		// Guards against degenerate inputs like `mcp://:1-50` — stripping the
-		// scheme's own slash would emit `mcp:/` as the path. The peeler refuses
-		// when the resulting path no longer carries a scheme separator.
+	it("keeps `mcp://:1-50`-shaped degenerate inputs opaque", () => {
+		// All mcp:// inputs are forwarded verbatim (see OPAQUE_RESOURCE_SCHEMES in
+		// path-utils.ts). This covers the edge case where the only `:` candidate
+		// for peeling sits next to the scheme's own `://`, so any peeler that ever
+		// replaces the opaque-scheme guard must still refuse these inputs.
 		expect(splitInternalUrlSel("mcp://:1-50")).toEqual({ path: "mcp://:1-50" });
 		expect(splitInternalUrlSel("mcp://:raw")).toEqual({ path: "mcp://:raw" });
 	});
 
-	it("rejects bare-integer suffixes even with the trailing-slash escape", () => {
-		// Could still be a port number after a path; require a richer selector form.
+	it("keeps mcp:// trailing-slash bare-integer suffixes opaque", () => {
+		// `:1234` could be a port number after a path. The opaque-scheme rule keeps
+		// it verbatim either way; if mcp ever moves to selector-aware peeling, a
+		// richer selector form should be required before treating `:N` as a tail.
 		expect(splitInternalUrlSel("mcp://server/resource/:1234")).toEqual({
 			path: "mcp://server/resource/:1234",
 		});