refactor(browser): tighten the session-reset skip guard and its tests

Drop the platform and absolute-path guards: ownsSocketDirectory is already
false on Windows and for inherited directories, and an Orca-derived directory
is always absolute. Fold the empty-name and traversal checks into agent-browser's
own session-name rule.

Stop lstat state leaking between lifecycle tests, and pin the probed socket path
so the skip test cannot pass on an unwired mock.
This commit is contained in:
Neil
2026-09-06 14:26:56 -07:00
parent 8a8138b7e8
commit 03a663c0bb
4 changed files with 37 additions and 33 deletions
@@ -176,7 +176,6 @@ export abstract class AgentBrowserBridgeExecution extends AgentBrowserBridgeTabs
protected closeStaleAgentBrowserSession(sessionName: string): Promise<void> {
if (
canSkipAgentBrowserSessionReset({
platform: process.platform,
ownsSocketDirectory: this.ownsAgentBrowserSocketDirectory,
socketDirectory: this.agentBrowserEnv.AGENT_BROWSER_SOCKET_DIR,
sessionName
@@ -81,6 +81,14 @@ function closeCallCount(): number {
describe('AgentBrowserBridge', () => {
let bridge: AgentBrowserBridge
// The mocked fs has no mkdirSync, so the constructor never claims a socket directory itself.
function ownSocketDirectory(): void {
Object.assign(bridge, {
ownsAgentBrowserSocketDirectory: true,
agentBrowserEnv: { AGENT_BROWSER_SOCKET_DIR: '/tmp/orca-ab-test' }
})
}
beforeEach(() => {
resetAgentBrowserBridgeMocks({
webContentsFromIdMock,
@@ -89,35 +97,28 @@ describe('AgentBrowserBridge', () => {
stdinWrites,
cdpWsProxyInstances: CdpWsProxyMock.instances
})
// Default to a socket that exists so an unprepared test still takes the reset path.
lstatSyncMock.mockReset()
lstatSyncMock.mockReturnValue({})
bridge = new AgentBrowserBridge(mockBrowserManager())
bridge.setActiveTab(100)
})
it('snapshots a fresh owned session without launching a helper just to close it', async () => {
vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin')
Object.assign(bridge, {
ownsAgentBrowserSocketDirectory: true,
agentBrowserEnv: { AGENT_BROWSER_SOCKET_DIR: '/tmp/orca-ab-test' }
})
ownSocketDirectory()
lstatSyncMock.mockImplementation(() => {
throw Object.assign(new Error('No socket'), { code: 'ENOENT' })
})
webContentsFromIdMock.mockReturnValue(mockWebContents(100))
succeedWith({ snapshot: 'ready' })
try {
expect(await bridge.snapshot()).toMatchObject({ snapshot: 'ready' })
expect(closeCallCount()).toBe(0)
} finally {
vi.restoreAllMocks()
}
expect(await bridge.snapshot()).toMatchObject({ snapshot: 'ready' })
expect(closeCallCount()).toBe(0)
expect(lstatSyncMock).toHaveBeenCalledWith('/tmp/orca-ab-test/orca-tab-tab-1.sock')
})
it('fails closed when stale agent-browser session ownership cannot be reset', async () => {
Object.assign(bridge, {
ownsAgentBrowserSocketDirectory: true,
agentBrowserEnv: { AGENT_BROWSER_SOCKET_DIR: '/tmp/orca-ab-test' }
})
ownSocketDirectory()
lstatSyncMock.mockReturnValue({})
vi.useFakeTimers()
try {
@@ -6,27 +6,28 @@ vi.mock('node:fs', () => ({ lstatSync }))
import { canSkipAgentBrowserSessionReset } from './agent-browser-session-reset'
const owned = {
platform: 'darwin' as const,
ownsSocketDirectory: true,
socketDirectory: '/tmp/orca-ab-profile',
sessionName: 'orca-tab-page'
}
const socketPath = join(owned.socketDirectory, 'orca-tab-page.sock')
beforeEach(() => {
lstatSync.mockReset()
})
it.each(['darwin', 'linux'] as const)('skips an absent owned socket on %s', (platform) => {
it('skips an absent owned socket', () => {
lstatSync.mockImplementation(() => {
throw Object.assign(new Error('No socket'), { code: 'ENOENT' })
})
expect(canSkipAgentBrowserSessionReset({ ...owned, platform })).toBe(true)
expect(lstatSync).toHaveBeenCalledWith(join(owned.socketDirectory, 'orca-tab-page.sock'))
expect(canSkipAgentBrowserSessionReset(owned)).toBe(true)
expect(lstatSync).toHaveBeenCalledWith(socketPath)
})
it('requires reset when a socket or symlink exists', () => {
lstatSync.mockReturnValue({})
expect(canSkipAgentBrowserSessionReset(owned)).toBe(false)
expect(lstatSync).toHaveBeenCalledWith(socketPath)
})
it.each(['EACCES', 'EIO', 'ENOTDIR'])('requires reset for %s', (code) => {
@@ -34,14 +35,15 @@ it.each(['EACCES', 'EIO', 'ENOTDIR'])('requires reset for %s', (code) => {
throw Object.assign(new Error('Socket inspection failed'), { code })
})
expect(canSkipAgentBrowserSessionReset(owned)).toBe(false)
expect(lstatSync).toHaveBeenCalledWith(socketPath)
})
// Windows and inherited socket directories both arrive as ownsSocketDirectory: false.
it.each([
{ platform: 'win32' as const },
{ ownsSocketDirectory: false },
{ socketDirectory: undefined },
{ socketDirectory: 'relative' },
{ sessionName: '../other' },
{ sessionName: 'has space' },
{ sessionName: '' }
])('requires reset without an owned Unix socket address: %j', (override) => {
expect(canSkipAgentBrowserSessionReset({ ...owned, ...override })).toBe(false)
+13 -11
View File
@@ -1,28 +1,30 @@
import { lstatSync } from 'node:fs'
import { basename, isAbsolute, join } from 'node:path'
import { join } from 'node:path'
// agent-browser's own session-name rule; doubles as a traversal fence for the `join` below.
const SAFE_SESSION_NAME = /^[A-Za-z0-9_-]+$/
/**
* True when no daemon can be holding `sessionName`, so closing it would only start one.
*
* Only an Orca-derived socket directory proves that (`ownsSocketDirectory`): it is a
* private per-profile `/tmp` directory, never an inherited one shared with a second
* profile, and never Windows, which uses named pipes and leaves no socket to inspect.
*/
export function canSkipAgentBrowserSessionReset(options: {
platform: NodeJS.Platform
ownsSocketDirectory: boolean
socketDirectory: string | undefined
sessionName: string
}): boolean {
const { socketDirectory, sessionName } = options
if (
options.platform === 'win32' ||
!options.ownsSocketDirectory ||
!socketDirectory ||
!isAbsolute(socketDirectory) ||
!sessionName ||
basename(sessionName) !== sessionName
) {
if (!options.ownsSocketDirectory || !socketDirectory || !SAFE_SESSION_NAME.test(sessionName)) {
return false
}
try {
lstatSync(join(socketDirectory, `${sessionName}.sock`))
return false
} catch (error) {
// An absent owned socket cannot be reused; permission and other failures prove nothing.
// Only a proven-absent socket is safe to skip; permission and other failures prove nothing.
return (error as NodeJS.ErrnoException).code === 'ENOENT'
}
}