diff --git a/src/main/daemon/pty-subprocess/foreground-process-tracker.ts b/src/main/daemon/pty-subprocess/foreground-process-tracker.ts index 5970ec93cae..83ed57874b2 100644 --- a/src/main/daemon/pty-subprocess/foreground-process-tracker.ts +++ b/src/main/daemon/pty-subprocess/foreground-process-tracker.ts @@ -3,6 +3,7 @@ import { win32 as pathWin32 } from 'node:path' import { getAgentForegroundContextPaths } from '../../providers/agent-foreground-context-paths' import { resolveAgentForegroundProcessWithAvailability } from '../../providers/agent-foreground-process' import { readWindowsConptyProcessIds } from '../../providers/windows-conpty-process-membership' +import { readWindowsConsoleAttachedProcessIds } from '../../providers/windows-console-attached-processes' import { isAgentForegroundWrapperProcess, recognizeAgentProcess, @@ -247,7 +248,8 @@ export function createPtyForegroundProcessTracker(args: { ...(process.platform === 'win32' ? { forceProcessScan: true, - readWindowsConptyProcessIds: () => readWindowsConptyProcessIds(proc) + readWindowsConsoleAttachedProcessIds: () => + readWindowsConsoleAttachedProcessIds(proc.pid) } : {}) } diff --git a/src/main/providers/agent-foreground-process-pi.test.ts b/src/main/providers/agent-foreground-process-pi.test.ts index 025c3144dc1..b2e4dae0ed2 100644 --- a/src/main/providers/agent-foreground-process-pi.test.ts +++ b/src/main/providers/agent-foreground-process-pi.test.ts @@ -52,14 +52,14 @@ describe('Pi Windows foreground recognition', () => { getAllProcessesMock.mockImplementation((cb: (snapshot: unknown) => void) => { cb(withSelf(rows)) }) - const readWindowsConptyProcessIds = vi.fn(() => new Set([100, 101])) + const readWindowsConsoleAttachedProcessIds = vi.fn(async () => new Set([100, 101])) await expect( resolveAgentForegroundProcessWithAvailability(100, 'node.exe', { fresh: true, - readWindowsConptyProcessIds + readWindowsConsoleAttachedProcessIds }) ).resolves.toEqual({ available: true, processName: 'pi' }) - expect(readWindowsConptyProcessIds).toHaveBeenCalledTimes(1) + expect(readWindowsConsoleAttachedProcessIds).toHaveBeenCalledTimes(1) }) }) diff --git a/src/main/providers/agent-foreground-process.test.ts b/src/main/providers/agent-foreground-process.test.ts index 518762ce81a..47115fa6031 100644 --- a/src/main/providers/agent-foreground-process.test.ts +++ b/src/main/providers/agent-foreground-process.test.ts @@ -508,7 +508,7 @@ describe('resolveAgentForegroundProcess', () => { await expect( resolveAgentForegroundProcessWithAvailability(100, 'powershell.exe', { fresh: true, - readWindowsConptyProcessIds: () => new Set([100, 101]) + readWindowsConsoleAttachedProcessIds: async () => new Set([100, 101]) }) ).resolves.toEqual({ available: true, processName: 'droid' }) }) @@ -655,15 +655,15 @@ describe('resolveAgentForegroundProcess', () => { commandLine: 'droid' } ]) - const readWindowsConptyProcessIds = vi.fn(() => new Set([100, 101, 999])) + const readWindowsConsoleAttachedProcessIds = vi.fn(async () => new Set([100, 101, 999])) await expect( resolveAgentForegroundProcessWithAvailability(100, 'powershell.exe', { fresh: true, - readWindowsConptyProcessIds + readWindowsConsoleAttachedProcessIds }) ).resolves.toEqual({ available: true, processName: 'droid' }) - expect(readWindowsConptyProcessIds).toHaveBeenCalledTimes(1) + expect(readWindowsConsoleAttachedProcessIds).toHaveBeenCalledTimes(1) }) it('excludes a detached Windows Droid descendant from byte authority', async () => { @@ -686,7 +686,7 @@ describe('resolveAgentForegroundProcess', () => { await expect( resolveAgentForegroundProcessWithAvailability(100, 'powershell.exe', { fresh: true, - readWindowsConptyProcessIds: () => new Set([100, 999]) + readWindowsConsoleAttachedProcessIds: async () => new Set([100, 999]) }) ).resolves.toEqual({ available: true, processName: 'powershell.exe' }) }) @@ -701,14 +701,14 @@ describe('resolveAgentForegroundProcess', () => { commandLine: 'powershell.exe' } ]) - const readWindowsConptyProcessIds = vi.fn(() => new Set([100, 999])) + const readWindowsConsoleAttachedProcessIds = vi.fn(async () => new Set([100, 999])) await expect( resolveAgentForegroundProcessWithAvailability(100, 'powershell.exe', { fresh: true, - readWindowsConptyProcessIds + readWindowsConsoleAttachedProcessIds }) ).resolves.toEqual({ available: true, processName: 'powershell.exe' }) - expect(readWindowsConptyProcessIds).not.toHaveBeenCalled() + expect(readWindowsConsoleAttachedProcessIds).not.toHaveBeenCalled() }) }) diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index 88ecb976398..d6c48c0306b 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -65,6 +65,7 @@ import { getAgentForegroundContextPaths } from './agent-foreground-context-paths import { recognizeAgentProcessFromCommandLine } from '../../shared/agent-process-recognition' import { killWithDescendantSweep } from '../pty-descendant-termination' import { readWindowsConptyProcessIds } from './windows-conpty-process-membership' +import { readWindowsConsoleAttachedProcessIds } from './windows-console-attached-processes' import { terminatePtyJob } from '../windows/windows-pty-job' import { canConfirmAgentFromConsolePresence } from './windows-console-foreground' import { forceKillPosixPtyProcessGroups } from '../pty/posix-pty-process-groups' @@ -1480,7 +1481,8 @@ export class LocalPtyProvider implements IPtyProvider { ...(process.platform === 'win32' ? { forceProcessScan: true, - readWindowsConptyProcessIds: () => readWindowsConptyProcessIds(proc) + readWindowsConsoleAttachedProcessIds: () => + readWindowsConsoleAttachedProcessIds(proc.pid) } : {}) } diff --git a/src/main/providers/windows-agent-foreground-process.ts b/src/main/providers/windows-agent-foreground-process.ts index ab25a03b1b8..3474a928202 100644 --- a/src/main/providers/windows-agent-foreground-process.ts +++ b/src/main/providers/windows-agent-foreground-process.ts @@ -23,7 +23,7 @@ export type AgentForegroundResolutionOptions = { /** Force confirmation scans even when node-pty reports a recognized name. */ forceProcessScan?: boolean /** Lazily proves which global descendants still belong to this ConPTY. */ - readWindowsConptyProcessIds?: () => ReadonlySet | null + readWindowsConsoleAttachedProcessIds?: () => Promise | null> } export type WindowsAgentForegroundResolution = { @@ -71,15 +71,17 @@ export async function resolveWindowsAgentForegroundProcessWithAvailability( options.contextPaths ) let filteredCandidates = candidates - if (hasRecognizedCandidate && options.readWindowsConptyProcessIds) { - const conptyProcessIds = options.readWindowsConptyProcessIds() - // Why not bail on null: membership only ever NARROWED the candidates. When - // it cannot answer, the unfiltered list is still a usable answer, and - // failing the whole resolution instead reported "unavailable" exactly while - // an agent was recognized -- disabling the callers that gate on it. - if (conptyProcessIds) { - filteredCandidates = candidates.filter((candidate) => conptyProcessIds.has(candidate.pid)) + if (hasRecognizedCandidate && options.readWindowsConsoleAttachedProcessIds) { + // Why console attachment and not the job: this filter exists to DROP a + // descendant that detached from the console, and the job still contains + // those by design. Answering it from the job would re-admit precisely what + // the filter is for -- granting byte authority to a detached `Start-Process + // droid`, or making an attached agent look ambiguous. + const consoleProcessIds = await options.readWindowsConsoleAttachedProcessIds() + if (!consoleProcessIds) { + return { available: false, processName: null } } + filteredCandidates = candidates.filter((candidate) => consoleProcessIds.has(candidate.pid)) } return { available: true, diff --git a/src/main/providers/windows-conpty-process-membership.test.ts b/src/main/providers/windows-conpty-process-membership.test.ts index 3475253b80d..f6111d047dd 100644 --- a/src/main/providers/windows-conpty-process-membership.test.ts +++ b/src/main/providers/windows-conpty-process-membership.test.ts @@ -67,3 +67,24 @@ describe('readWindowsConptyProcessIds', () => { expect(membership).toEqual(new Set([100, 200])) }) }) + +describe('why the filter does NOT use this', () => { + it('documents that job membership keeps console-detached descendants', () => { + // The candidate filter in windows-agent-foreground-process.ts exists to DROP + // a descendant that left the console (`Start-Process droid`, a GUI child). + // The job still contains those by design, so answering that filter from the + // job would re-admit exactly what it is for -- granting byte authority to a + // pane no agent owns, or making an attached agent look ambiguous. + // docs/windows-wsl-root-cause-plan.html calls this out as "Use B". + // + // Measured on Windows 11 against a real WSL pane: job [40980,104068,4888,69908] + // vs console [69908,40980] -- the job is a superset. Harmless for the + // `size > 1` callers, wrong for the filter. + const detachedChild = 104068 + const membership = readWindowsConptyProcessIds(pty(40980), { + listJobProcessIds: () => [40980, detachedChild] + }) + + expect(membership?.has(detachedChild)).toBe(true) + }) +}) diff --git a/src/main/providers/windows-console-attached-processes.ts b/src/main/providers/windows-console-attached-processes.ts new file mode 100644 index 00000000000..0ab462c4a6f --- /dev/null +++ b/src/main/providers/windows-console-attached-processes.ts @@ -0,0 +1,97 @@ +import { fork, type ChildProcess } from 'node:child_process' + +const CONPTY_PROCESS_LIST_TIMEOUT_MS = 3_000 + +type ProcessListMessage = { consoleProcessList?: unknown } + +type WindowsConptyMembershipDeps = { + forkProcess?: typeof fork + resolveAgentPath?: () => string + timeoutMs?: number +} + +function resolveNodePtyConsoleListAgent(): string { + return require.resolve('node-pty/lib/conpty_console_list_agent.js') +} + +/** + * Processes ATTACHED TO THIS PANE'S CONSOLE, or null when unavailable. + * + * Distinct from job membership on purpose. `GetConsoleProcessList` must be + * called from a process attached to that console, and a process can hold only + * one console at a time -- which is why node-pty answers it from a separate + * process, and why this still forks. + * + * Only the candidate FILTER may use this. That filter exists to drop a + * descendant which detached from the console (`Start-Process`, a GUI child), and + * the job object deliberately still contains those, so the job cannot answer it + * -- see docs/windows-wsl-root-cause-plan.html, "Use B". + * + * This is not the fork storm in #10857: it runs only when a recognized agent + * candidate already exists, not on every foreground poll. Bounding it to one + * pooled, supervised helper is the remaining half of that fix. + */ +export function readWindowsConsoleAttachedProcessIds( + rootPid: number, + deps: WindowsConptyMembershipDeps = {} +): Promise | null> { + if (!Number.isSafeInteger(rootPid) || rootPid <= 0) { + return Promise.resolve(null) + } + let child: ChildProcess + try { + child = (deps.forkProcess ?? fork)( + (deps.resolveAgentPath ?? resolveNodePtyConsoleListAgent)(), + [String(rootPid)], + { silent: true } + ) + } catch { + return Promise.resolve(null) + } + + return new Promise((resolve) => { + let settled = false + const finish = (value: ReadonlySet | null): void => { + if (settled) { + return + } + settled = true + clearTimeout(timeout) + child.removeListener('message', onMessage) + // Why: kill failures can emit asynchronously after timeout settlement; + // teardown listeners stay until exit so they cannot crash the daemon. + resolve(value) + } + const onFailure = (): void => finish(null) + const onExit = (): void => { + child.removeListener('error', onFailure) + finish(null) + } + const onMessage = (message: ProcessListMessage): void => { + const value = message?.consoleProcessList + const helperPid = child.pid + if ( + !Array.isArray(value) || + helperPid === undefined || + !value.includes(rootPid) || + !value.includes(helperPid) || + value.some((pid) => !Number.isSafeInteger(pid) || pid <= 0) + ) { + finish(null) + return + } + // Why: GetConsoleProcessList includes this helper; removing it makes a + // root-only set authoritative shell-only evidence instead of a false child. + const consoleProcessIds = new Set(value) + consoleProcessIds.delete(helperPid) + finish(consoleProcessIds) + } + const timeout = setTimeout(() => { + child.kill() + finish(null) + }, deps.timeoutMs ?? CONPTY_PROCESS_LIST_TIMEOUT_MS) + child.once('message', onMessage) + child.once('error', onFailure) + child.once('exit', onExit) + }) +}