From 2c7b0f3fae416e86ad81c94143b2f8e5dd98cb63 Mon Sep 17 00:00:00 2001 From: Pablosinyores Date: Wed, 1 Jul 2026 16:44:04 +0530 Subject: [PATCH 1/4] fix(bg): match background-session command args on token boundaries commandLineContainsArgs matched each stored launch arg as a raw substring, so a selector like "1642" was satisfied by an unrelated live token "16420" in the same position. A reused PID whose command line merely contained the stored digits could therefore keep a dead session classified as running, and the kill path could target the wrong process. Match each stored arg against a whole whitespace-delimited token, in order, instead of a substring. Add regression coverage through isBackgroundSessionProcessAlive for the collision, the exact-token match, the session-id match, and the dead-process case. Refs #1770 --- src/cli/bgRegistry.test.ts | 55 ++++++++++++++++++++++++++++++++++++++ src/cli/bgRegistry.ts | 21 ++++++++++++--- 2 files changed, 72 insertions(+), 4 deletions(-) diff --git a/src/cli/bgRegistry.test.ts b/src/cli/bgRegistry.test.ts index 0fd726ae55..ba1b65a8f1 100644 --- a/src/cli/bgRegistry.test.ts +++ b/src/cli/bgRegistry.test.ts @@ -6,11 +6,13 @@ import { join } from 'node:path' import { _setBackgroundSessionsRootForTesting, createBackgroundSession, + isBackgroundSessionProcessAlive, isTerminalBackgroundSession, listBackgroundSessions, markBackgroundSessionKilled, refreshBackgroundSessionStatuses, resolveBackgroundSession, + type BackgroundSession, } from './bgRegistry.js' describe('background session registry', () => { @@ -675,3 +677,56 @@ describe('background session registry', () => { expect(await listBackgroundSessions()).toEqual([]) }) }) + +describe('isBackgroundSessionProcessAlive process identity', () => { + const session: BackgroundSession = { + id: 'bg-identity', + pid: 4242, + cwd: '/repo', + status: 'running', + startedAt: '2026-07-01T08:00:00.000Z', + updatedAt: '2026-07-01T08:00:00.000Z', + // sessionId deliberately absent from the command lines below so the stored + // launch invocation (command) is what has to match. + sessionId: 'conversation-identity', + command: ['node', 'openclaude', '1642'], + stdoutLogPath: '/tmp/stdout.log', + stderrLogPath: '/tmp/stderr.log', + } + + it('does not treat a reused PID whose command merely contains the arg as alive (#1770)', () => { + // The live process at this PID is unrelated: its final token "16420" only + // contains the stored selector "1642" as a substring. Ordered substring + // matching wrongly reported this session as alive, so `kill` could target + // the wrong process. + const alive = isBackgroundSessionProcessAlive(session, { + isProcessAlive: () => true, + getProcessCommand: () => 'node openclaude 16420 --serve', + }) + expect(alive).toBe(false) + }) + + it('still recognizes the real process by exact command tokens', () => { + const alive = isBackgroundSessionProcessAlive(session, { + isProcessAlive: () => true, + getProcessCommand: () => 'node openclaude 1642 --serve', + }) + expect(alive).toBe(true) + }) + + it('matches on the session id when it is present on the command line', () => { + const alive = isBackgroundSessionProcessAlive(session, { + isProcessAlive: () => true, + getProcessCommand: () => 'node openclaude conversation-identity', + }) + expect(alive).toBe(true) + }) + + it('reports a dead process regardless of command line', () => { + const alive = isBackgroundSessionProcessAlive(session, { + isProcessAlive: () => false, + getProcessCommand: () => 'node openclaude 1642', + }) + expect(alive).toBe(false) + }) +}) diff --git a/src/cli/bgRegistry.ts b/src/cli/bgRegistry.ts index 1fa355e8ea..58866e5050 100644 --- a/src/cli/bgRegistry.ts +++ b/src/cli/bgRegistry.ts @@ -506,11 +506,24 @@ type BackgroundSessionProcessState = 'alive' | 'dead' | 'unknown' function commandLineContainsArgs(commandLine: string, args: string[]): boolean { if (args.length === 0) return false - let offset = 0 + // Match each stored arg against a whole whitespace-delimited token, in order, + // rather than as a raw substring. Substring matching let a stored selector + // like "1642" satisfy a lookup against an unrelated live token "16420" (e.g. a + // reused PID whose command line merely contains those digits), so a dead + // session stayed classified as running. See #1770. + const tokens = commandLine.split(/\s+/).filter(token => token.length > 0) + let tokenIndex = 0 for (const arg of args) { - const index = commandLine.indexOf(arg, offset) - if (index === -1) return false - offset = index + arg.length + let matched = false + while (tokenIndex < tokens.length) { + const token = tokens[tokenIndex] + tokenIndex += 1 + if (token === arg) { + matched = true + break + } + } + if (!matched) return false } return true } From 34fa3a53b26cce722c01c6e5f6db824573a68608 Mon Sep 17 00:00:00 2001 From: Pablosinyores Date: Thu, 2 Jul 2026 13:56:11 +0530 Subject: [PATCH 2/4] fix(bg): match the session id on token boundaries and span multi-word args Address review on #1770: - commandLineMatchesBackgroundSession matched session.sessionId with a raw includes(), so a short id like 'sess-1' matched an unrelated live command containing 'sess-100' and kept a reused-PID session classified as running. Route it through the same whole-token matcher. - commandLineContainsArgs required each stored arg to equal a single token, so a prompt stored as one argv entry (e.g. 'refactor auth') never matched the words ps renders it as, breaking --from-pr/resume launches that rely on the stored command. Expand each arg into its own tokens and match the flattened sequence as an ordered subsequence. Add regression tests: session-id substring collision stays dead, exact id token is alive, and a multi-word prompt arg matches across command tokens (all proven fail-on-old). --- src/cli/bgRegistry.test.ts | 47 ++++++++++++++++++++++++++++++++++++++ src/cli/bgRegistry.ts | 20 ++++++++++++---- 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/src/cli/bgRegistry.test.ts b/src/cli/bgRegistry.test.ts index ba1b65a8f1..159faa3138 100644 --- a/src/cli/bgRegistry.test.ts +++ b/src/cli/bgRegistry.test.ts @@ -722,6 +722,53 @@ describe('isBackgroundSessionProcessAlive process identity', () => { expect(alive).toBe(true) }) + it('does not match the session id as a substring of a larger token (#1770)', () => { + // A short id must not match an unrelated live command that merely contains + // it inside a longer token — the same reused-PID collision class as the + // command-arg path. Command args are absent from the live line so only the + // session-id branch can produce a match here. + const shortIdSession: BackgroundSession = { + ...session, + sessionId: 'sess-1', + command: ['node', 'openclaude', 'unused-token'], + } + const alive = isBackgroundSessionProcessAlive(shortIdSession, { + isProcessAlive: () => true, + getProcessCommand: () => 'node openclaude sess-100 --serve', + }) + expect(alive).toBe(false) + }) + + it('matches the session id only as a whole token', () => { + const shortIdSession: BackgroundSession = { + ...session, + sessionId: 'sess-1', + command: ['node', 'openclaude', 'unused-token'], + } + const alive = isBackgroundSessionProcessAlive(shortIdSession, { + isProcessAlive: () => true, + getProcessCommand: () => 'node openclaude sess-1 --serve', + }) + expect(alive).toBe(true) + }) + + it('matches a stored multi-word prompt arg across command tokens (#1770)', () => { + // A prompt like "refactor auth" is stored as a single argv entry but `ps` + // renders it as separate words; the matcher must span both. The session id + // is absent from the live line so the command args are what must match. + const promptSession: BackgroundSession = { + ...session, + sessionId: 'conversation-absent', + command: ['node', 'openclaude', '--print', 'refactor auth'], + } + const alive = isBackgroundSessionProcessAlive(promptSession, { + isProcessAlive: () => true, + getProcessCommand: () => + 'node openclaude --print refactor auth --serve', + }) + expect(alive).toBe(true) + }) + it('reports a dead process regardless of command line', () => { const alive = isBackgroundSessionProcessAlive(session, { isProcessAlive: () => false, diff --git a/src/cli/bgRegistry.ts b/src/cli/bgRegistry.ts index 58866e5050..285d9a3d3e 100644 --- a/src/cli/bgRegistry.ts +++ b/src/cli/bgRegistry.ts @@ -506,19 +506,28 @@ type BackgroundSessionProcessState = 'alive' | 'dead' | 'unknown' function commandLineContainsArgs(commandLine: string, args: string[]): boolean { if (args.length === 0) return false - // Match each stored arg against a whole whitespace-delimited token, in order, + // Match the stored args against whole whitespace-delimited tokens, in order, // rather than as a raw substring. Substring matching let a stored selector // like "1642" satisfy a lookup against an unrelated live token "16420" (e.g. a // reused PID whose command line merely contains those digits), so a dead // session stayed classified as running. See #1770. + // + // A stored arg can itself contain whitespace — a prompt like "refactor auth" + // is a single argv entry but `ps` renders it as separate words — so expand + // each arg into its own tokens and match the flattened sequence as an ordered + // subsequence of whole command tokens. const tokens = commandLine.split(/\s+/).filter(token => token.length > 0) + const argTokens = args.flatMap(arg => + arg.split(/\s+/).filter(token => token.length > 0), + ) + if (argTokens.length === 0) return false let tokenIndex = 0 - for (const arg of args) { + for (const argToken of argTokens) { let matched = false while (tokenIndex < tokens.length) { const token = tokens[tokenIndex] tokenIndex += 1 - if (token === arg) { + if (token === argToken) { matched = true break } @@ -532,7 +541,10 @@ function commandLineMatchesBackgroundSession( commandLine: string, session: BackgroundSession, ): boolean { - if (commandLine.includes(session.sessionId)) return true + // Match the session id as a whole token, not a raw substring: an id like + // "sess-1" must not match an unrelated live command that merely contains + // "sess-100" (the same reused-PID collision this guard fixes for #1770). + if (commandLineContainsArgs(commandLine, [session.sessionId])) return true // PR resume launches write to the resumed transcript id without carrying // that id on argv, so use the stored launch invocation as the PID guard. return commandLineContainsArgs(commandLine, session.command) From 529de158785923b68215b3f3be407b47f61bd07a Mon Sep 17 00:00:00 2001 From: Pablosinyores Date: Mon, 6 Jul 2026 17:11:08 +0530 Subject: [PATCH 3/4] fix(bg): require background-session args to match as a contiguous token run The token-boundary matcher for background-session process identity treated the stored argv tokens as an ordered subsequence of the live command line, so unrelated tokens between matches were skipped. A reused PID whose command interleaves the stored tokens (e.g. stored [node, openclaude, 1642] against "node attacker openclaude extra 1642 --serve") was still classified alive, keeping the wrong-process kill risk open for token-insertion collisions. Match the flattened stored argv as one contiguous whole-token run instead. Leading interpreter path and trailing flags still match; interspersed tokens no longer do. Add a regression test asserting an interspersed-token command line is rejected (fails on the prior subsequence matcher, passes now). Refs #1770 --- src/cli/bgRegistry.test.ts | 14 ++++++++++++++ src/cli/bgRegistry.ts | 29 +++++++++++++++++------------ 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/src/cli/bgRegistry.test.ts b/src/cli/bgRegistry.test.ts index 159faa3138..91dfcb1dd2 100644 --- a/src/cli/bgRegistry.test.ts +++ b/src/cli/bgRegistry.test.ts @@ -769,6 +769,20 @@ describe('isBackgroundSessionProcessAlive process identity', () => { expect(alive).toBe(true) }) + it('does not treat interspersed stored tokens as alive (#1770)', () => { + // The stored tokens all appear on the live command line but only as an + // ordered subsequence with unrelated tokens ("attacker", "extra") wedged + // between them, i.e. a different process at a reused PID. Requiring a + // contiguous whole-token run rejects this token-insertion collision; a + // subsequence match would wrongly report it alive and risk killing the + // wrong process. + const alive = isBackgroundSessionProcessAlive(session, { + isProcessAlive: () => true, + getProcessCommand: () => 'node attacker openclaude extra 1642 --serve', + }) + expect(alive).toBe(false) + }) + it('reports a dead process regardless of command line', () => { const alive = isBackgroundSessionProcessAlive(session, { isProcessAlive: () => false, diff --git a/src/cli/bgRegistry.ts b/src/cli/bgRegistry.ts index 285d9a3d3e..b188910d85 100644 --- a/src/cli/bgRegistry.ts +++ b/src/cli/bgRegistry.ts @@ -514,27 +514,32 @@ function commandLineContainsArgs(commandLine: string, args: string[]): boolean { // // A stored arg can itself contain whitespace — a prompt like "refactor auth" // is a single argv entry but `ps` renders it as separate words — so expand - // each arg into its own tokens and match the flattened sequence as an ordered - // subsequence of whole command tokens. + // each arg into its own tokens and require the flattened sequence to appear as + // one CONTIGUOUS run of whole command tokens. An ordered-subsequence match + // (skipping unrelated tokens between matches) would let a reused PID whose + // command line merely interleaves the stored tokens pass — e.g. stored + // ["node", "openclaude", "1642"] satisfied by "node attacker openclaude extra + // 1642 --serve" — reopening the same wrong-process `kill` risk for token + // insertion collisions. The real launch invocation appears as an unbroken run + // (only the interpreter path or trailing flags differ), so leading/trailing + // tokens are fine but interspersed ones are not. const tokens = commandLine.split(/\s+/).filter(token => token.length > 0) const argTokens = args.flatMap(arg => arg.split(/\s+/).filter(token => token.length > 0), ) if (argTokens.length === 0) return false - let tokenIndex = 0 - for (const argToken of argTokens) { - let matched = false - while (tokenIndex < tokens.length) { - const token = tokens[tokenIndex] - tokenIndex += 1 - if (token === argToken) { - matched = true + if (argTokens.length > tokens.length) return false + for (let start = 0; start <= tokens.length - argTokens.length; start += 1) { + let matched = true + for (let offset = 0; offset < argTokens.length; offset += 1) { + if (tokens[start + offset] !== argTokens[offset]) { + matched = false break } } - if (!matched) return false + if (matched) return true } - return true + return false } function commandLineMatchesBackgroundSession( From 8810e222ff4db92efa25f4248971827c3e8da4ca Mon Sep 17 00:00:00 2001 From: 0xghost42 Date: Tue, 7 Jul 2026 14:34:19 +0530 Subject: [PATCH 4/4] fix(bg): trim quotes so Windows quoted command lines match stored argv Get-CimInstance CommandLine returns the raw, quoted Windows command line, so a whitespace split fuses quotes onto the edge tokens ("C:\Program, node.exe", "refactor, auth") while session.command stores those values unquoted. The contiguous whole-token run therefore never matched, and a live non-forked --from-pr resume (whose only identity path is the stored command) was marked stale, letting kill report success without signalling the process. Trim a single surrounding quote from each token before comparing; POSIX ps output is unquoted so this is a no-op there and preserves the #1770 token-boundary guard. --- src/cli/bgRegistry.test.ts | 38 ++++++++++++++++++++++++++++++++++++++ src/cli/bgRegistry.ts | 22 ++++++++++++++++++---- 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/src/cli/bgRegistry.test.ts b/src/cli/bgRegistry.test.ts index 91dfcb1dd2..8fcc10eac8 100644 --- a/src/cli/bgRegistry.test.ts +++ b/src/cli/bgRegistry.test.ts @@ -769,6 +769,44 @@ describe('isBackgroundSessionProcessAlive process identity', () => { expect(alive).toBe(true) }) + it('matches a quoted Windows command line with a spaced exe path and prompt (#1770)', () => { + // Windows `Get-CimInstance ... CommandLine` returns the raw command line + // with quoted paths/prompts, so a whitespace split fuses quotes onto the + // edge tokens (`"C:\Program`, `node.exe"`, `"refactor`, `auth"`). The stored + // argv holds those values unquoted, so without quote trimming the contiguous + // run never matched and a live `--from-pr` resume (whose only identity path + // is the stored command) was wrongly marked stale. + const windowsSession: BackgroundSession = { + ...session, + sessionId: 'conversation-absent', + command: [ + 'C:\\Program Files\\nodejs\\node.exe', + 'C:\\repo\\dist\\cli.mjs', + '--from-pr', + '1642', + '--print', + 'refactor auth', + ], + } + const alive = isBackgroundSessionProcessAlive(windowsSession, { + isProcessAlive: () => true, + getProcessCommand: () => + '"C:\\Program Files\\nodejs\\node.exe" C:\\repo\\dist\\cli.mjs --from-pr 1642 --print "refactor auth"', + }) + expect(alive).toBe(true) + }) + + it('quote trimming does not reopen the substring collision (#1770)', () => { + // Trimming surrounding quotes must not degrade to substring matching: a + // quoted live token "16420" still only contains the stored selector "1642", + // so it must not satisfy the lookup. + const alive = isBackgroundSessionProcessAlive(session, { + isProcessAlive: () => true, + getProcessCommand: () => '"node" openclaude "16420" --serve', + }) + expect(alive).toBe(false) + }) + it('does not treat interspersed stored tokens as alive (#1770)', () => { // The stored tokens all appear on the live command line but only as an // ordered subsequence with unrelated tokens ("attacker", "extra") wedged diff --git a/src/cli/bgRegistry.ts b/src/cli/bgRegistry.ts index b188910d85..22e046d3af 100644 --- a/src/cli/bgRegistry.ts +++ b/src/cli/bgRegistry.ts @@ -504,6 +504,22 @@ export async function refreshBackgroundSessionStatuses(options?: { type BackgroundSessionProcessState = 'alive' | 'dead' | 'unknown' +// A spaced path or prompt is a single argv entry, but the raw command line +// quotes it, so a whitespace split fuses a quote onto the edge tokens. Windows +// `Get-CimInstance ... CommandLine` returns exactly this form — e.g. +// "C:\Program Files\nodejs\node.exe" ...\cli.mjs --from-pr 1642 --print "refactor auth" +// splits to `"C:\Program`, `Files\nodejs\node.exe"`, ..., `"refactor`, `auth"`. +// The stored argv holds those same values unquoted, so trim a single leading +// and/or trailing quote from each token before comparing. POSIX `ps` output is +// unquoted, making this a no-op there, and it never widens the token-boundary +// match below (a stripped token still has to equal the stored one). See #1770. +function tokenizeCommandLine(value: string): string[] { + return value + .split(/\s+/) + .map(token => token.replace(/^["']|["']$/g, '')) + .filter(token => token.length > 0) +} + function commandLineContainsArgs(commandLine: string, args: string[]): boolean { if (args.length === 0) return false // Match the stored args against whole whitespace-delimited tokens, in order, @@ -523,10 +539,8 @@ function commandLineContainsArgs(commandLine: string, args: string[]): boolean { // insertion collisions. The real launch invocation appears as an unbroken run // (only the interpreter path or trailing flags differ), so leading/trailing // tokens are fine but interspersed ones are not. - const tokens = commandLine.split(/\s+/).filter(token => token.length > 0) - const argTokens = args.flatMap(arg => - arg.split(/\s+/).filter(token => token.length > 0), - ) + const tokens = tokenizeCommandLine(commandLine) + const argTokens = args.flatMap(tokenizeCommandLine) if (argTokens.length === 0) return false if (argTokens.length > tokens.length) return false for (let start = 0; start <= tokens.length - argTokens.length; start += 1) {