Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions electron-builder.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ asarUnpack:
- resources/**
- bin/**
- node_modules/@agent-relay/sdk/bin/**
- node_modules/@agent-relay/broker-*/bin/**
- node_modules/agent-relay/node_modules/@agent-relay/broker-*/bin/**

mac:
category: public.app-category.developer-tools
Expand Down
70 changes: 69 additions & 1 deletion src/main/broker.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ vi.mock('./burn', () => ({
getPearBurnAgentKey: vi.fn((projectId: string, name: string) => `${projectId}:${name}`)
}))

import { BrokerManager, resolveAgentRelayMcpCommand } from './broker'
import { BrokerManager, parseBrokerInitCliFlags, resolveAgentRelayMcpCommand } from './broker'

const PROJECT_ID = 'project-1'
const originalMcpCommand = process.env.AGENT_RELAY_MCP_COMMAND
Expand Down Expand Up @@ -407,6 +407,52 @@ describe('resolveAgentRelayMcpCommand', () => {
})
})

describe('parseBrokerInitCliFlags', () => {
it('detects current broker init flags', () => {
const flags = parseBrokerInitCliFlags(`
Usage: agent-relay-broker init [OPTIONS]

Options:
--name <NAME> Legacy broker instance name flag. Prefer --instance-name [default: ]
--instance-name <INSTANCE_NAME> Stable broker instance name within the Relay workspace
--workspace-key <WORKSPACE_KEY> Join an existing Relay workspace instead of creating a fresh one
--channels <CHANNELS> [default: general]
`)

expect(flags).toEqual({
supportsInstanceName: true,
supportsName: true,
supportsWorkspaceKey: true
})
})

it('detects legacy broker init flags', () => {
const flags = parseBrokerInitCliFlags(`
Usage: agent-relay-broker init [OPTIONS]

Options:
--name <NAME> [default: ]
--channels <CHANNELS> [default: general]
--persist Enable persistence
`)

expect(flags).toEqual({
supportsInstanceName: false,
supportsName: true,
supportsWorkspaceKey: false
})
})
})
Comment on lines +410 to +445

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add startup regression coverage for the compat-shim execution path.

These tests validate flag parsing, but they don’t verify BrokerManager.start actually switches to shim binary and forwards PEAR_AGENT_RELAY_BROKER_* env for legacy brokers (including replay/duplicate start behavior).

Based on learnings and coding guidelines: add regression tests when touching broker start, and include duplicate/replay cases, not just happy path.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/broker.test.ts` around lines 410 - 445, Add regression tests that
exercise the compat-shim execution path by invoking BrokerManager.start (or the
test harness that triggers it) and asserting that when a legacy broker is
detected the process launch switches to the compat-shim binary and forwards
environment variables with the PEAR_AGENT_RELAY_BROKER_* prefix; include tests
for duplicate/replay starts (calling start twice or replaying the same legacy
detection) to ensure it does not spawn extra processes and that the forwarded
env is identical on replay. Locate usages of parseBrokerInitCliFlags,
BrokerManager.start, and the compat-shim launch code in the broker startup tests
and add mocks/stubs for the child process spawn/exec to capture binary path and
env, then assert the expected behavior for legacy vs. modern flags and for
duplicate invocation.

Sources: Coding guidelines, Learnings


describe('electron-builder broker packaging', () => {
it('unpacks bundled broker binaries outside app.asar', async () => {
const config = await readFile('electron-builder.yml', 'utf8')

expect(config).toContain('node_modules/@agent-relay/broker-*/bin/**')
expect(config).toContain('node_modules/agent-relay/node_modules/@agent-relay/broker-*/bin/**')
})
})

describe('BrokerManager local + cloud coexistence', () => {
let personaTempDir: string | null = null
const inheritedWorkspaceKey = process.env.AGENT_RELAY_WORKSPACE_KEY
Expand Down Expand Up @@ -834,6 +880,28 @@ describe('BrokerManager local + cloud coexistence', () => {
await manager.shutdown()
})

it('lists workforce personas from a project root before the relay workspace starts', async () => {
personaTempDir = await mkdtemp(join(tmpdir(), 'pear-persona-list-'))
await writeAgentWorkforceFixture(personaTempDir)

const manager = new BrokerManager()

await expect(manager.listPersonas(PROJECT_ID, personaTempDir)).resolves.toEqual([
{ id: 'autonomous-actor', harness: 'claude' }
])
expect(mock.HarnessDriverClient.spawn).not.toHaveBeenCalled()

await manager.shutdown()
})

it('returns an empty persona list instead of throwing when no session or root is available', async () => {
const manager = new BrokerManager()

await expect(manager.listPersonas(PROJECT_ID)).resolves.toEqual([])

await manager.shutdown()
})

it('coalesces concurrent duplicate workforce persona spawn requests', async () => {
personaTempDir = await mkdtemp(join(tmpdir(), 'pear-persona-spawn-'))
await writeAgentWorkforceFixture(personaTempDir)
Expand Down
176 changes: 171 additions & 5 deletions src/main/broker.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { existsSync, readFileSync } from 'fs'
import { chmodSync, existsSync, mkdirSync, readFileSync, writeFileSync } from 'fs'
import { rm } from 'fs/promises'
import { randomUUID } from 'node:crypto'
import { homedir } from 'node:os'
import { delimiter, basename, isAbsolute, join } from 'path'
import { execFileSync } from 'child_process'
import { execFile, execFileSync } from 'child_process'
import { app, BrowserWindow } from 'electron'
import {
HarnessDriverClient as AgentRelayClient,
Expand Down Expand Up @@ -306,6 +306,154 @@ function resolveBundledBrokerBinary(): string {
return unpackIfPackaged(brokerBinary)
}

interface BrokerInitCliFlags {
supportsInstanceName: boolean
supportsName: boolean
supportsWorkspaceKey: boolean
}

export function parseBrokerInitCliFlags(helpText: string): BrokerInitCliFlags {
return {
supportsInstanceName: /(?:^|\s)--instance-name(?:\s|[<=[,]|$)/.test(helpText),
supportsName: /(?:^|\s)--name(?:\s|[<=[,]|$)/.test(helpText),
supportsWorkspaceKey: /(?:^|\s)--workspace-key(?:\s|[<=[,]|$)/.test(helpText)
}
}

function inspectBrokerInitCliFlags(binaryPath: string): Promise<BrokerInitCliFlags> {
return new Promise((resolve) => {
execFile(binaryPath, ['init', '--help'], {
encoding: 'utf8',
timeout: 2_000,
windowsHide: true
}, (err, stdout) => {
if (err) {
console.warn(`[broker] Failed to inspect broker init CLI for ${binaryPath}:`, err)
resolve({
supportsInstanceName: true,
supportsName: true,
supportsWorkspaceKey: true
})
return
}
resolve(parseBrokerInitCliFlags(stdout))
})
})
}

function brokerBinaryCompatShimSource(): string {
return `#!/usr/bin/env node
const { spawn } = require('node:child_process')

const realBinary = process.env.PEAR_AGENT_RELAY_BROKER_BINARY
if (!realBinary) {
console.error('[pear-broker-compat] PEAR_AGENT_RELAY_BROKER_BINARY is required')
process.exit(127)
}

const supportsInstanceName = process.env.PEAR_AGENT_RELAY_BROKER_SUPPORTS_INSTANCE_NAME === '1'
const supportsWorkspaceKey = process.env.PEAR_AGENT_RELAY_BROKER_SUPPORTS_WORKSPACE_KEY === '1'
const inputArgs = process.argv.slice(2)
const outputArgs = []

for (let index = 0; index < inputArgs.length; index += 1) {
const arg = inputArgs[index]
if (!supportsInstanceName && arg === '--instance-name') {
outputArgs.push('--name')
if (index + 1 < inputArgs.length) outputArgs.push(inputArgs[++index])
continue
}
if (!supportsInstanceName && arg.startsWith('--instance-name=')) {
outputArgs.push('--name=' + arg.slice('--instance-name='.length))
continue
}
if (!supportsWorkspaceKey && arg === '--workspace-key') {
index += 1
continue
}
if (!supportsWorkspaceKey && arg.startsWith('--workspace-key=')) {
continue
}
outputArgs.push(arg)
}

const child = spawn(realBinary, outputArgs, {
cwd: process.cwd(),
env: process.env,
stdio: 'inherit'
})

child.on('error', (error) => {
console.error('[pear-broker-compat] failed to launch broker:', error instanceof Error ? error.message : String(error))
process.exit(127)
})

child.on('exit', (code, signal) => {
if (signal) {
try {
process.kill(process.pid, signal)
} catch {
process.exit(1)
}
return
}
process.exit(code ?? 0)
})
Comment on lines +391 to +401

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

On Windows, calling process.kill with certain POSIX signals can throw an EINVAL error, causing the compatibility shim to crash with an unhandled exception. Wrapping the signal propagation in a try/catch block ensures the shim exits gracefully on all platforms.

child.on('exit', (code, signal) => {
  if (signal) {
    try {
      process.kill(process.pid, signal)
    } catch {
      process.exit(1)
    }
    return
  }
  process.exit(code ?? 0)
})

`
}

function ensureBrokerBinaryCompatShim(): string {
const shimDir = join(app.getPath('userData'), 'broker-compat')
const shimPath = join(shimDir, process.platform === 'win32' ? 'agent-relay-broker-compat.cmd' : 'agent-relay-broker-compat')
const source = process.platform === 'win32'
? `@echo off\r\nnode "%~dp0agent-relay-broker-compat.js" %*\r\n`
: brokerBinaryCompatShimSource()
Comment on lines +341 to +403

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Compat shim requires a system node binary, which can fail in packaged installs.

Both POSIX (#!/usr/bin/env node) and Windows (node ...) shim launch paths depend on external Node availability. On end-user machines without Node, broker startup can fail before the real binary runs.

Also applies to: 410-417

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/broker.ts` around lines 341 - 403, The current compat shim
(brokerBinaryCompatShimSource / ensureBrokerBinaryCompatShim) assumes a system
"node" binary; fix by bundling a JS launcher plus platform wrappers that invoke
the app's Node (process.execPath) instead of "node" or a shebang. Concretely:
split the shim into a JS file (agent-relay-broker-compat.js) containing the
existing launcher logic but without a shebang, then change
ensureBrokerBinaryCompatShim to write (1) a POSIX shell wrapper
"agent-relay-broker-compat" that execs the quoted process.execPath with the JS
file (e.g. exec "<process.execPath>" "$0.js" "$@"), and (2) a Windows CMD
wrapper "agent-relay-broker-compat.cmd" that calls the quoted process.execPath
with the JS file and %*; ensure brokerBinaryCompatShimSource is used for the JS
payload (no shebang) and that all wrappers pass arguments through and preserve
env/cwd/stdio behavior.


mkdirSync(shimDir, { recursive: true })
if (!existsSync(shimPath) || readFileSync(shimPath, 'utf8') !== source) {
writeFileSync(shimPath, source)
chmodSync(shimPath, 0o755)
}
if (process.platform === 'win32') {
const jsPath = join(shimDir, 'agent-relay-broker-compat.js')
const jsSource = brokerBinaryCompatShimSource()
if (!existsSync(jsPath) || readFileSync(jsPath, 'utf8') !== jsSource) {
writeFileSync(jsPath, jsSource)
}
}
return shimPath
}

async function resolveHarnessBrokerBinary(workspaceKey?: string): Promise<{ binaryPath: string; env: NodeJS.ProcessEnv }> {
const binaryPath = resolveBundledBrokerBinary()
const flags = await inspectBrokerInitCliFlags(binaryPath)

if (workspaceKey && !flags.supportsWorkspaceKey) {
throw new Error(
`Broker binary does not support --workspace-key: ${binaryPath}. Rebuild or update agent-relay-broker, or unset AGENT_RELAY_WORKSPACE_KEY.`
)
}

if (flags.supportsInstanceName) {
return { binaryPath, env: {} }
}

if (!flags.supportsName) {
throw new Error(`Broker binary supports neither --instance-name nor --name: ${binaryPath}`)
}

const shimPath = ensureBrokerBinaryCompatShim()
console.warn(`[broker] Broker binary uses legacy --name flag; launching through compatibility shim: ${binaryPath}`)
return {
binaryPath: shimPath,
env: {
PEAR_AGENT_RELAY_BROKER_BINARY: binaryPath,
PEAR_AGENT_RELAY_BROKER_SUPPORTS_INSTANCE_NAME: flags.supportsInstanceName ? '1' : '0',
PEAR_AGENT_RELAY_BROKER_SUPPORTS_WORKSPACE_KEY: flags.supportsWorkspaceKey ? '1' : '0'
}
}
}

type TerminalAttachMode = 'view' | 'drive' | 'passthrough'

export interface CloudAgentSandboxHandle {
Expand Down Expand Up @@ -1396,15 +1544,17 @@ export class BrokerManager {

// Phase 1 of #125: the local broker stays the workspace creator, so the
// key is only threaded when explicitly pinned via env.
const brokerBinary = await resolveHarnessBrokerBinary(explicitWorkspaceKey)
const opts: AgentRelaySpawnOptions = {
cwd,
brokerName: name,
channels: nextChannels,
binaryArgs: { persist: true },
binaryPath: resolveBundledBrokerBinary(),
binaryPath: brokerBinary.binaryPath,
...(explicitWorkspaceKey ? { workspaceKey: explicitWorkspaceKey } : {}),
env: {
PATH: augmentedPath(),
...brokerBinary.env,
...(agentRelayMcpCommand ? { AGENT_RELAY_MCP_COMMAND: agentRelayMcpCommand } : {})
},
onStderr: (line: string) => {
Expand Down Expand Up @@ -2595,8 +2745,24 @@ export class BrokerManager {
throw new Error(`Unable to allocate an agent name for ${relayAwareInput.name}`)
}

async listPersonas(projectId: string): Promise<WorkforcePersona[]> {
const session = this.getSessionForProject(projectId)
async listPersonas(projectId: string, cwd?: string): Promise<WorkforcePersona[]> {
const personaCwd = cwd?.trim()
if (personaCwd) {
try {
return listWorkforcePersonas(personaCwd)
} catch (err) {
console.warn(`[broker] Failed to list workforce personas for project ${projectId} at ${personaCwd}:`, err)
return []
}
}

let session: BrokerSession
try {
;[session] = await this.getOrAwaitSessionsForProject(projectId)
} catch (err) {
if (isWorkspaceNotStartedError(err)) return []
throw err
}

try {
return listWorkforcePersonas(session.cwd)
Expand Down
38 changes: 38 additions & 0 deletions src/main/ipc-handlers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ vi.mock('./cli', () => ({
}))

import { registerIpcHandlers } from './ipc-handlers'
import { findProjectForPath } from './cli'
import { isDirectory } from './path-utils'

describe('registerIpcHandlers broker:start', () => {
beforeEach(() => {
Expand Down Expand Up @@ -199,6 +201,42 @@ describe('registerIpcHandlers broker:spawn-agent', () => {
})
})

describe('registerIpcHandlers broker:list-personas', () => {
beforeEach(() => {
mock.handlers.clear()
mock.ipcMain.handle.mockClear()
mock.ipcMain.on.mockClear()
mock.brokerManager.listPersonas.mockReset()
vi.mocked(findProjectForPath).mockReset()
vi.mocked(isDirectory).mockReset()
vi.mocked(isDirectory).mockReturnValue(true)
registerIpcHandlers()
})

it('lists personas for a cwd inside the requested project root', async () => {
const handler = mock.handlers.get('broker:list-personas')
expect(handler).toBeTypeOf('function')
vi.mocked(findProjectForPath).mockReturnValue({ id: 'project-1' } as never)
mock.brokerManager.listPersonas.mockResolvedValueOnce([{ id: 'autonomous-actor' }])

const result = await handler?.({}, 'project-1', '/tmp/project-1')

expect(result).toEqual([{ id: 'autonomous-actor' }])
expect(mock.brokerManager.listPersonas).toHaveBeenCalledWith('project-1', '/tmp/project-1')
})

it('rejects persona cwd outside the requested project root', async () => {
const handler = mock.handlers.get('broker:list-personas')
expect(handler).toBeTypeOf('function')
vi.mocked(findProjectForPath).mockReturnValue({ id: 'project-2' } as never)

await expect(handler?.({}, 'project-1', '/tmp/project-2')).rejects.toThrow(
/Path is outside project roots for project project-1/
)
expect(mock.brokerManager.listPersonas).not.toHaveBeenCalled()
})
})

describe('registerIpcHandlers broker:spawn-persona', () => {
beforeEach(() => {
mock.handlers.clear()
Expand Down
14 changes: 13 additions & 1 deletion src/main/ipc-handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,19 @@ export function registerIpcHandlers(): void {
return toBrokerSpawnAgentResult(result)
})

ipcMain.handle('broker:list-personas', async (_, projectId: string) => {
ipcMain.handle('broker:list-personas', async (_, projectId: string, cwd?: string) => {
const personaCwd = cwd?.trim()
if (personaCwd) {
const resolved = resolve(personaCwd)
const pathProjectId = getProjectIdForPath(resolved)
if (!pathProjectId || pathProjectId !== projectId.trim()) {
throw new Error(`Path is outside project roots for project ${projectId}: ${resolved}`)
}
if (!isDirectory(resolved)) {
return []
}
return brokerManager.listPersonas(projectId, resolved)
}
return brokerManager.listPersonas(projectId)
})
Comment on lines +268 to 282

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The broker:list-personas IPC handler accepts an arbitrary cwd path from the renderer process without validating that it lies within the allowed project roots. Since listPersonas eventually executes binaries from the provided directory (via resolveAgentWorkforceCommand), a compromised renderer could exploit this to execute arbitrary local binaries. Please add assertPathWithinProjects(personaCwd) to enforce path validation, similar to other file system and Git IPC handlers.

  ipcMain.handle('broker:list-personas', async (_, projectId: string, cwd?: string) => {
    const personaCwd = cwd?.trim()
    if (personaCwd) {
      assertPathWithinProjects(personaCwd)
      if (!isDirectory(personaCwd)) {
        return []
      }
    }
    return brokerManager.listPersonas(projectId, personaCwd)
  })


Expand Down
4 changes: 2 additions & 2 deletions src/preload/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,8 @@ const api = {
connectCloud: () => invoke<string>('broker:connect-cloud'),
spawnAgent: (projectId: string, input: BrokerSpawnAgentInput) =>
invoke<BrokerSpawnAgentResult>('broker:spawn-agent', projectId, input),
listPersonas: (projectId: string) =>
invoke<WorkforcePersona[]>('broker:list-personas', projectId),
listPersonas: (projectId: string, cwd?: string) =>
invoke<WorkforcePersona[]>('broker:list-personas', projectId, cwd),
spawnPersona: (projectId: string, personaId: string) =>
invoke<BrokerSpawnAgentResult>('broker:spawn-persona', projectId, personaId),
attachTerminal: (input: BrokerAttachTerminalInput) =>
Expand Down
Loading
Loading