Merge PR #6665: fix(coding-agent): route js-debug commands to the stopped script child (@roboomp)

2026-07-26 15:45:09 +02:00
parent fb38343a89 80df85a84b
commit cc5f5a29a6
3 changed files with 198 additions and 14 deletions
@@ -49,6 +49,7 @@
 ### Fixed

 - Corrected Windows shell resolution errors to identify the active global, project, overlay, or runtime source for `shellPath`, including profile and custom configuration directories, instead of directing every user to the retired `settings.json` file ([#6579](https://github.com/can1357/oh-my-pi/issues/6579)).
+- Fixed `debug` (js-debug/`pwa-node`) stateful commands misrouting after launch: a lazily-attached `[worker N]` child session (or the threadless root launcher) would steal the active-session focus from the stopped script child, so `threads` listed only the worker thread, post-launch breakpoints read back as pending/unbound, and there was no way to step/continue/evaluate the script's thread. Focus now follows stops rather than registrations, and `threads` aggregates every live thread across the session tree ([#6663](https://github.com/can1357/oh-my-pi/issues/6663)).

 ## [17.1.3] - 2026-07-24

@@ -960,16 +960,47 @@ export class DapSessionManager {
 		signal?: AbortSignal,
 		timeoutMs: number = 30_000,
 	): Promise<{ snapshot: DapSessionSummary; threads: DapThread[] }> {
-		const session = this.#touchActiveSession();
-		const response = await this.#sendRequestWithConfig<DapThreadsResponse>(
-			session,
-			"threads",
-			undefined,
-			signal,
-			timeoutMs,
-		);
-		session.threads = response?.threads ?? [];
-		return { snapshot: buildSummary(session), threads: session.threads };
+		const anchor = this.#touchActiveSession();
+		// A js-debug launch is a session tree: the root may be a threadless
+		// launcher while each real thread lives in a child (main script,
+		// `[worker N]`, …), and other adapters keep every thread on the root.
+		// Querying only the active session would surface just one session's
+		// threads, so aggregate across the whole live tree. No topology guess:
+		// a threadless launcher simply returns no threads (or an error we skip).
+		const targets = this.#liveTreeSessions(anchor);
+		const merged: DapThread[] = [];
+		const seen = new Set<string>();
+		for (const target of targets) {
+			let threads: DapThread[];
+			try {
+				const response = await this.#sendRequestWithConfig<DapThreadsResponse>(
+					target,
+					"threads",
+					undefined,
+					signal,
+					timeoutMs,
+				);
+				threads = response?.threads ?? [];
+			} catch (error) {
+				logger.warn("Failed to list threads for debug session", {
+					sessionId: target.id,
+					error: toErrorMessage(error),
+				});
+				continue;
+			}
+			target.threads = threads;
+			// DAP thread IDs are scoped per client session, so identical IDs from
+			// different sessions (e.g. two identical worker scripts) are distinct
+			// live threads and MUST be preserved; only collapse an exact repeat
+			// within a single session's response.
+			for (const thread of threads) {
+				const key = `${target.id}\0${thread.id}`;
+				if (seen.has(key)) continue;
+				seen.add(key);
+				merged.push(thread);
+			}
+		}
+		return { snapshot: buildSummary(anchor), threads: merged };
 	}

 	async stackTrace(
@@ -1371,7 +1402,13 @@ export class DapSessionManager {
 		if (parentSessionId) {
 			this.#sessions.get(parentSessionId)?.childSessionIds.add(session.id);
 		}
-		this.#activeSessionId = session.id;
+		// Focus follows stops, not registrations: a lazily-attached child (e.g. a
+		// js-debug `[worker N]` session) must not steal focus from a sibling that
+		// is already stopped at a breakpoint / entry. Only claim focus when no
+		// live, stopped session currently holds it.
+		if (!this.#hasLiveStoppedActiveSession()) {
+			this.#activeSessionId = session.id;
+		}
 		const heartbeat = setInterval(() => {
 			if (!client.isAlive()) {
 				session.status = "terminated";
@@ -1696,6 +1733,12 @@ export class DapSessionManager {
 		return session;
 	}

+	/** True when the current active session is live and paused at a stop. */
+	#hasLiveStoppedActiveSession(): boolean {
+		const active = this.#getActiveSessionOrNull();
+		return active !== null && active.status === "stopped" && active.client.isAlive();
+	}
+
 	#getActiveSessionOrThrow(): DapSession {
 		const session = this.#getActiveSessionOrNull();
 		if (!session) {
@@ -1729,6 +1772,19 @@ export class DapSessionManager {
 		return sessions;
 	}

+	/**
+	 * Live (non-terminated, connected) sessions in `session`'s tree, or the
+	 * session itself when the tree has collapsed. Used to fan `threads` out
+	 * across the whole tree; a threadless session just reports no threads, so
+	 * this makes no assumption about which node owns them.
+	 */
+	#liveTreeSessions(session: DapSession): DapSession[] {
+		const live = this.#getTreeSessions(session).filter(
+			candidate => candidate.status !== "terminated" && candidate.client.isAlive(),
+		);
+		return live.length > 0 ? live : [session];
+	}
+
 	#touchSessionAndAncestors(session: DapSession): void {
 		const now = Date.now();
 		let current: DapSession | undefined = session;
@@ -6,6 +6,7 @@ import type {
 	DapClientState,
 	DapEventMessage,
 	DapResolvedAdapter,
+	DapThread,
 } from "@oh-my-pi/pi-coding-agent/dap/types";

 const TEST_ADAPTER: DapResolvedAdapter = {
@@ -25,6 +26,13 @@ const TEST_ADAPTER: DapResolvedAdapter = {
 type EventHandler = (body: unknown, event: DapEventMessage) => void | Promise<void>;
 type ReverseHandler = (args: unknown) => unknown | Promise<unknown>;

+interface FakeOptions {
+	/** Threads returned by this session's `threads` request. */
+	threads?: DapThread[];
+	/** Thread id reported by the synthetic `stopped` event (defaults to 7). */
+	stopThreadId?: number;
+}
+
 class FakeDapClient {
 	readonly proc: DapClientState["proc"];
 	readonly port = 8123;
@@ -39,6 +47,7 @@ class FakeDapClient {
 		readonly childConfiguration?: Record<string, unknown>,
 		readonly childRequest: "launch" | "attach" = "launch",
 		readonly stopOnStart = true,
+		readonly options: FakeOptions = {},
 	) {
 		this.proc = {
 			exited: this.#exited.promise,
@@ -71,10 +80,10 @@ class FakeDapClient {
 					});
 				});
 			} else if (this.stopOnStart) {
-				queueMicrotask(() => this.#emit("stopped", { reason: "entry", threadId: 7 }));
+				queueMicrotask(() => this.#emit("stopped", { reason: "entry", threadId: this.options.stopThreadId ?? 7 }));
 			}
 		}
-		if (command === "threads") return { threads: [{ id: 7, name: "target.js" }] };
+		if (command === "threads") return { threads: this.options.threads ?? [{ id: 7, name: "target.js" }] };
 		if (command === "stackTrace") {
 			return {
 				stackFrames: [{ id: 70, name: "main", line: 2, column: 1, source: { path: "/tmp/target.js" } }],
@@ -127,6 +136,11 @@ class FakeDapClient {
 		this.#emit(event, body);
 	}

+	/** Drive an adapter-initiated reverse request (e.g. a late `startDebugging`). */
+	async triggerReverse(command: string, args: unknown): Promise<void> {
+		await this.#emitReverse(command, args);
+	}
+
 	async #emitReverse(command: string, args: unknown): Promise<void> {
 		const handler = this.#reverseHandlers.get(command);
 		if (!handler) throw new Error(`Missing reverse handler for ${command}`);
@@ -196,6 +210,9 @@ describe("DAP multi-session debugging", () => {
 				__pendingTargetId: "attached-child",
 			},
 			"attach",
+			true,
+			// Threadless launcher: answers `threads` with an empty list.
+			{ threads: [] },
 		);
 		const child = new FakeDapClient(undefined, "launch", false);
 		spyOn(DapClient, "spawn").mockResolvedValue(root as unknown as DapClient);
@@ -209,7 +226,9 @@ describe("DAP multi-session debugging", () => {
 		expect(active?.parentSessionId).toBeDefined();
 		expect(threads.threads).toEqual([{ id: 7, name: "target.js" }]);
 		expect(child.requests.filter(request => request.command === "threads")).toHaveLength(1);
-		expect(root.requests.filter(request => request.command === "threads")).toHaveLength(0);
+		// The root is queried too (no topology guess), but being threadless it
+		// contributes nothing.
+		expect(root.requests.filter(request => request.command === "threads")).toHaveLength(1);

 		await manager.terminate(undefined, 100);
 	});
@@ -246,4 +265,112 @@ describe("DAP multi-session debugging", () => {

 		await manager.terminate(undefined, 100);
 	});
+
+	it("keeps focus on the stopped script child when a worker attaches later", async () => {
+		const root = new FakeDapClient(
+			{
+				name: "script.mts",
+				type: "pwa-node",
+				__pendingTargetId: "main",
+				program: "/tmp/script.mts",
+			},
+			"launch",
+			true,
+			// Threadless launcher: it answers `threads` with an empty list.
+			{ threads: [] },
+		);
+		// The script child stops on entry (thread 1), then a worker session
+		// attaches afterwards via a late reverse `startDebugging`.
+		const main = new FakeDapClient(undefined, "launch", true, {
+			threads: [{ id: 1, name: "script.mts" }],
+			stopThreadId: 1,
+		});
+		const worker = new FakeDapClient(undefined, "launch", false, {
+			threads: [{ id: 1, name: "[worker 1]" }],
+		});
+		const children = [main, worker];
+		spyOn(DapClient, "spawn").mockResolvedValue(root as unknown as DapClient);
+		spyOn(DapClient, "connect").mockImplementation(async () => {
+			const next = children.shift();
+			if (!next) throw new Error("Unexpected child DAP connection");
+			return next as unknown as DapClient;
+		});
+		const manager = new DapSessionManager();
+
+		const launched = await manager.launch(
+			{ adapter: TEST_ADAPTER, program: "/tmp/script.mts", cwd: "/tmp" },
+			undefined,
+			1_000,
+		);
+		expect(launched.status).toBe("stopped");
+		const scriptSessionId = launched.id;
+
+		// A worker_threads spawn triggers a late child attach on the launcher.
+		await root.triggerReverse("startDebugging", {
+			request: "launch",
+			configuration: { name: "[worker 1]", type: "pwa-node" },
+		});
+		expect(manager.listSessions()).toHaveLength(3);
+
+		// Focus must stay on the stopped script child, not jump to the worker.
+		const active = manager.getActiveSession();
+		expect(active?.id).toBe(scriptSessionId);
+		expect(active?.threadId).toBe(1);
+
+		// `threads` must surface every live thread across the tree, not just one.
+		const threads = await manager.threads(undefined, 1_000);
+		expect(threads.threads).toHaveLength(2);
+		expect(threads.threads).toEqual(
+			expect.arrayContaining([
+				{ id: 1, name: "script.mts" },
+				{ id: 1, name: "[worker 1]" },
+			]),
+		);
+		// The launcher is still queried, but being threadless it contributes none.
+		expect(root.requests.filter(request => request.command === "threads")).toHaveLength(1);
+
+		await manager.terminate(undefined, 1_000);
+	});
+
+	it("preserves per-session threads that share an id and name across children", async () => {
+		const root = new FakeDapClient(
+			{ name: "pool.mjs", type: "pwa-node", __pendingTargetId: "main", program: "/tmp/pool.mjs" },
+			"launch",
+			true,
+			{ threads: [] },
+		);
+		// Two identical worker scripts each expose the same session-local thread
+		// id and name; DAP scopes ids per session, so both are distinct threads.
+		const main = new FakeDapClient(undefined, "launch", true, {
+			threads: [{ id: 1, name: "worker.js" }],
+			stopThreadId: 1,
+		});
+		const worker = new FakeDapClient(undefined, "launch", false, {
+			threads: [{ id: 1, name: "worker.js" }],
+		});
+		const children = [main, worker];
+		spyOn(DapClient, "spawn").mockResolvedValue(root as unknown as DapClient);
+		spyOn(DapClient, "connect").mockImplementation(async () => {
+			const next = children.shift();
+			if (!next) throw new Error("Unexpected child DAP connection");
+			return next as unknown as DapClient;
+		});
+		const manager = new DapSessionManager();
+
+		await manager.launch({ adapter: TEST_ADAPTER, program: "/tmp/pool.mjs", cwd: "/tmp" }, undefined, 1_000);
+		await root.triggerReverse("startDebugging", {
+			request: "launch",
+			configuration: { name: "worker #2", type: "pwa-node" },
+		});
+		expect(manager.listSessions()).toHaveLength(3);
+
+		const threads = await manager.threads(undefined, 1_000);
+		// Both identical threads survive aggregation \u2014 not collapsed into one.
+		expect(threads.threads).toEqual([
+			{ id: 1, name: "worker.js" },
+			{ id: 1, name: "worker.js" },
+		]);
+
+		await manager.terminate(undefined, 1_000);
+	});
 });