mirror of
https://github.com/Gitlawb/openclaude.git
synced 2026-08-24 10:14:19 -05:00
fix(bg): match background-session command args on token boundaries (#1834)
* 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 * 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). * 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 * 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 <id> 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. --------- Co-authored-by: 0xghost42 <nikhilbajajj01@gmail.com>
This commit is contained in:
@@ -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', () => {
|
||||
@@ -803,3 +805,155 @@ 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('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('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
|
||||
// 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,
|
||||
getProcessCommand: () => 'node openclaude 1642',
|
||||
})
|
||||
expect(alive).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
+51
-7
@@ -505,22 +505,66 @@ 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
|
||||
let offset = 0
|
||||
for (const arg of args) {
|
||||
const index = commandLine.indexOf(arg, offset)
|
||||
if (index === -1) return false
|
||||
offset = index + arg.length
|
||||
// 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 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 = 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) {
|
||||
let matched = true
|
||||
for (let offset = 0; offset < argTokens.length; offset += 1) {
|
||||
if (tokens[start + offset] !== argTokens[offset]) {
|
||||
matched = false
|
||||
break
|
||||
}
|
||||
}
|
||||
if (matched) return true
|
||||
}
|
||||
return true
|
||||
return false
|
||||
}
|
||||
|
||||
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)
|
||||
|
||||
Reference in New Issue
Block a user