Skip to content

Harden broker startup and persona discovery#164

Merged
khaliqgant merged 2 commits into
mainfrom
codex/harden-broker-persona-packaging
Jun 8, 2026
Merged

Harden broker startup and persona discovery#164
khaliqgant merged 2 commits into
mainfrom
codex/harden-broker-persona-packaging

Conversation

@khaliqgant

Copy link
Copy Markdown
Member

Summary

  • list Workforce personas from the selected project root without requiring a live broker session
  • harden broker startup against local broker CLI skew by inspecting supported init flags and adapting legacy --name binaries when safe
  • explicitly unpack bundled broker binaries in the macOS app package so shipped brokers execute outside app.asar

Tests

  • npx vitest run src/main/broker.test.ts
  • npx tsc --noEmit -p tsconfig.json
  • npm run build

@codeant-ai

codeant-ai Bot commented Jun 8, 2026

Copy link
Copy Markdown

Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI.

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@khaliqgant, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 51 minutes and 16 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9770891a-6cd1-4585-b6b4-911ec947a357

📥 Commits

Reviewing files that changed from the base of the PR and between 31f774f and 99417c1.

📒 Files selected for processing (3)
  • src/main/broker.ts
  • src/main/ipc-handlers.test.ts
  • src/main/ipc-handlers.ts
📝 Walkthrough

Walkthrough

This PR introduces broker compatibility detection and extends personas listing to support working directory filtering. The broker compatibility layer automatically detects broker CLI capabilities and creates a shim for legacy flag remapping. The personas API is broadened throughout the IPC stack to accept an optional working directory, enabling filtered listings. Build packaging is updated to unpack broker binaries at runtime.

Changes

Broker Compatibility and Personas API

Layer / File(s) Summary
Broker compatibility shim and local session integration
src/main/broker.ts, src/main/broker.test.ts
Detects broker CLI flags by parsing init --help output. Generates a compatibility shim that remaps legacy --instance-name/--workspace-key arguments via environment variables when the broker lacks --name. Local broker spawn uses the shim binary and merges shim environment. Tests validate flag detection and electron-builder unpacking rules.
Personas listing API surface with cwd support
src/shared/types/ipc.ts, src/preload/index.ts, src/main/ipc-handlers.ts, src/main/broker.ts, src/renderer/src/lib/spawn-agent.ts, src/main/broker.test.ts
IPC contract, preload wrapper, and handler accept an optional working directory parameter. IPC handler validates cwd as an existing directory and returns empty list on invalid paths. BrokerManager lists personas from the directory when provided, or resolves sessions when not. Renderer passes root path to personas listing. Tests cover directory-specific and directory-absent cases.
Build packaging configuration for broker binaries
electron-builder.yml
Extends asarUnpack glob patterns to unpack broker binary directories from node_modules instead of keeping them archived inside the app ASAR.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🐰 A shim to bridge the old ways,
And personas listed by their paths,
Binaries unpacked at last,
Compatibility's future cast!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main changes: hardening broker startup and persona discovery capabilities through runtime flag detection and directory-based persona listing.
Description check ✅ Passed The description is directly related to the changeset, clearly outlining the three main objectives: persona discovery without live sessions, broker CLI compatibility handling, and binary unpacking configuration.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/harden-broker-persona-packaging

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a compatibility shim to support legacy broker binaries using older CLI flags (like --name instead of --instance-name), and updates the persona listing mechanism to support querying a project root before the workspace starts. The review feedback highlights a critical security vulnerability where the broker:list-personas IPC handler accepts an unvalidated cwd path, which could lead to remote code execution; it is recommended to enforce path validation. Additionally, the reviewer suggests refactoring the synchronous CLI flag inspection to be asynchronous to avoid blocking the Electron main thread, and wrapping signal propagation in a try/catch block to prevent potential crashes on Windows.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/main/ipc-handlers.ts
Comment on lines +268 to 274
ipcMain.handle('broker:list-personas', async (_, projectId: string, cwd?: string) => {
const personaCwd = cwd?.trim()
if (personaCwd && !isDirectory(personaCwd)) {
return []
}
return brokerManager.listPersonas(projectId, personaCwd)
})

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)
  })

Comment thread src/main/broker.ts Outdated
Comment on lines +323 to +339
function inspectBrokerInitCliFlags(binaryPath: string): BrokerInitCliFlags {
try {
const help = execFileSync(binaryPath, ['init', '--help'], {
encoding: 'utf8',
timeout: 2_000,
stdio: ['ignore', 'pipe', 'pipe']
})
return parseBrokerInitCliFlags(help)
} catch (err) {
console.warn(`[broker] Failed to inspect broker init CLI for ${binaryPath}:`, err)
return {
supportsInstanceName: true,
supportsName: true,
supportsWorkspaceKey: true
}
}
}

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

Executing external processes synchronously using execFileSync on the Electron main thread blocks the entire application UI and responsiveness for up to 2 seconds (the configured timeout). This should be refactored to use asynchronous execFile wrapped in a Promise.

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

Comment thread src/main/broker.ts Outdated
Comment on lines +420 to +448
function resolveHarnessBrokerBinary(workspaceKey?: string): { binaryPath: string; env: NodeJS.ProcessEnv } {
const binaryPath = resolveBundledBrokerBinary()
const flags = 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'
}
}
}

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

Since inspectBrokerInitCliFlags should be asynchronous to avoid blocking the main thread, resolveHarnessBrokerBinary should also be updated to be an async function and await the flags inspection.

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'
    }
  }
}

Comment thread src/main/broker.ts Outdated

// Phase 1 of #125: the local broker stays the workspace creator, so the
// key is only threaded when explicitly pinned via env.
const brokerBinary = resolveHarnessBrokerBinary(explicitWorkspaceKey)

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

Update this call to await the asynchronous resolveHarnessBrokerBinary function.

      const brokerBinary = await resolveHarnessBrokerBinary(explicitWorkspaceKey)

Comment thread src/main/broker.ts
Comment on lines +388 to +394
child.on('exit', (code, signal) => {
if (signal) {
process.kill(process.pid, signal)
return
}
process.exit(code ?? 0)
})

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)
})

@codeant-ai

codeant-ai Bot commented Jun 8, 2026

Copy link
Copy Markdown

Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI.

@khaliqgant

Copy link
Copy Markdown
Member Author

Addressed the review feedback in 99417c1:

  • validates broker:list-personas cwd against the requested project root before invoking persona discovery
  • switches broker init flag inspection from execFileSync to async execFile
  • awaits the broker binary resolution in startup
  • wraps compatibility shim signal propagation in try/catch
  • adds IPC regression coverage for rejecting persona cwd outside the requested project

Verified:

  • npx vitest run src/main/broker.test.ts src/main/ipc-handlers.test.ts
  • npx tsc --noEmit -p tsconfig.json
  • npm run build

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🤖 Prompt for all review comments with 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.

Inline comments:
In `@src/main/broker.test.ts`:
- Around line 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.

In `@src/main/broker.ts`:
- Around line 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.
- Around line 323-337: The catch block in the inspectBrokerInitCliFlags function
returns optimistic defaults with all flags set to true when the execFileSync
call fails, which bypasses compatibility checks and can cause issues with legacy
binaries. Change the error handling to return conservative defaults where all
flags (supportsInstanceName, supportsName, supportsWorkspaceKey) are set to
false instead of true, ensuring compatibility paths are properly exercised when
the CLI inspection fails.

In `@src/main/ipc-handlers.ts`:
- Around line 269-273: Normalize whitespace-only cwd to undefined before
validation and forwarding: after computing personaCwd from cwd (symbol:
personaCwd derived from cwd), convert empty-string results to undefined (e.g.,
personaCwd = cwd?.trim() || undefined) so the truthy check and
isDirectory(personaCwd) validation behave correctly; then call
brokerManager.listPersonas(projectId, personaCwd) with that normalized value.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c32b4e70-2d8b-4fc1-b665-22773d770458

📥 Commits

Reviewing files that changed from the base of the PR and between 7b8e1b5 and 31f774f.

📒 Files selected for processing (7)
  • electron-builder.yml
  • src/main/broker.test.ts
  • src/main/broker.ts
  • src/main/ipc-handlers.ts
  • src/preload/index.ts
  • src/renderer/src/lib/spawn-agent.ts
  • src/shared/types/ipc.ts

Comment thread src/main/broker.test.ts
Comment on lines +410 to +445
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
})
})
})

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

Comment thread src/main/broker.ts Outdated
Comment on lines +323 to +337
function inspectBrokerInitCliFlags(binaryPath: string): BrokerInitCliFlags {
try {
const help = execFileSync(binaryPath, ['init', '--help'], {
encoding: 'utf8',
timeout: 2_000,
stdio: ['ignore', 'pipe', 'pipe']
})
return parseBrokerInitCliFlags(help)
} catch (err) {
console.warn(`[broker] Failed to inspect broker init CLI for ${binaryPath}:`, err)
return {
supportsInstanceName: true,
supportsName: true,
supportsWorkspaceKey: true
}

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 | ⚡ Quick win

Avoid optimistic defaults when broker CLI inspection fails.

If init --help probing fails, returning all flags as supported skips the compatibility path and can break startup on legacy binaries with unclear downstream errors.

Suggested hardening
 function inspectBrokerInitCliFlags(binaryPath: string): BrokerInitCliFlags {
   try {
@@
     return parseBrokerInitCliFlags(help)
   } catch (err) {
     console.warn(`[broker] Failed to inspect broker init CLI for ${binaryPath}:`, err)
-    return {
-      supportsInstanceName: true,
-      supportsName: true,
-      supportsWorkspaceKey: true
-    }
+    // Conservative fallback: prefer compatibility behavior over false capability claims.
+    return {
+      supportsInstanceName: false,
+      supportsName: true,
+      supportsWorkspaceKey: false
+    }
   }
 }
🤖 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 323 - 337, The catch block in the
inspectBrokerInitCliFlags function returns optimistic defaults with all flags
set to true when the execFileSync call fails, which bypasses compatibility
checks and can cause issues with legacy binaries. Change the error handling to
return conservative defaults where all flags (supportsInstanceName,
supportsName, supportsWorkspaceKey) are set to false instead of true, ensuring
compatibility paths are properly exercised when the CLI inspection fails.

Comment thread src/main/broker.ts
Comment on lines +341 to +403
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) {
process.kill(process.pid, signal)
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()

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.

Comment thread src/main/ipc-handlers.ts Outdated
@agent-relay-code

Copy link
Copy Markdown
Contributor

Implemented the current, validated review fixes in the checkout:

  • broker:list-personas now validates renderer-provided cwd against known project roots before invoking persona discovery.
  • Broker init flag inspection now uses async execFile, and startup awaits broker binary resolution.
  • Compatibility shim signal forwarding is wrapped in try/catch.
  • Added IPC regression coverage for rejecting untrusted persona paths.

Validation run:

  • npm exec -- vitest run src/main/broker.test.ts src/main/ipc-handlers.test.ts
  • npm exec -- tsc --noEmit -p tsconfig.json
  • Generated shim sample executed against a throwaway fake broker
  • npm run verify:mcp-resources-drift
  • npm test
  • npm run build

Addressed comments

  • codeant-ai[bot] issue comment 4648834537: quota notice only; no actionable code review.
  • coderabbitai[bot] issue comment 4648835753: review-in-progress/status comment only; no actionable finding.
  • gemini-code-assist[bot] discussion_r3373083995: unvalidated broker:list-personas cwd; fixed in src/main/ipc-handlers.ts:271, regression in src/main/ipc-handlers.test.ts:229.
  • gemini-code-assist[bot] discussion_r3373084001: synchronous broker flag inspection blocks main thread; fixed in src/main/broker.ts:323.
  • gemini-code-assist[bot] discussion_r3373084005: resolveHarnessBrokerBinary should become async; fixed in src/main/broker.ts:426.
  • gemini-code-assist[bot] discussion_r3373084010: startup should await async broker binary resolution; fixed in src/main/broker.ts:1546.
  • gemini-code-assist[bot] discussion_r3373084013: shim signal propagation can throw on Windows; fixed in src/main/broker.ts:392.
  • gemini-code-assist[bot] review 4449345299: summary of the same five findings above; fixed at the same referenced lines.
  • codeant-ai[bot] issue comment 4648873649: quota notice only; no actionable code review.
  • khaliqgant issue comment 4648874471: stated these findings were addressed in a later commit, but the current checkout still lacked them; fixed in the current checkout at the lines listed above.

I did not verify remote GitHub CI status, macOS dist:mac packaging, or mergeability from this Linux sandbox, so I’m not marking the PR ready.

@khaliqgant khaliqgant merged commit 72fbe0b into main Jun 8, 2026
4 checks passed
@khaliqgant khaliqgant deleted the codex/harden-broker-persona-packaging branch June 8, 2026 12:22
@agent-relay-code

Copy link
Copy Markdown
Contributor

⚠️ pr-reviewer did not push — the PR branch advanced during the review, so fixes were withheld to avoid overwriting newer commits. Re-trigger the review once the branch settles. The notes below are advisory and were not pushed.

The checked-out PR artifacts identify this as PR #166 (claude/chat-virtualization), not #164. I reviewed and fixed the current checkout’s PR changes.

I changed ChatView.tsx to share a stable room key between the virtualized list and its React key, forcing a clean virtualizer remount when switching channels/DMs. I also added flow-root to the measured virtual row container at ChatView.tsx so date-divider margins are included in row measurement.

Validation run:

  • npm ci
  • npm exec -- vitest run src/renderer/src/components/chat/ChatView.dom.test.ts
  • npm exec -- tsc --noEmit -p tsconfig.json
  • npm run build
  • Checked for conflict markers: none found

GitHub API showed the head SHA’s checks/statuses were successful, but the PR API reports PR #166 is already merged/closed, so I am not printing READY.

Addressed comments

  • gemini-code-assist[bot] review summary: raised stale virtualizer state on room switches and date-divider margin measurement; fixed in src/renderer/src/components/chat/ChatView.tsx:74, src/renderer/src/components/chat/ChatView.tsx:183, and src/renderer/src/components/chat/ChatView.tsx:692.
  • gemini-code-assist[bot] discussion 3373123684: requested a unique key for VirtualizedMessageList per room; fixed in src/renderer/src/components/chat/ChatView.tsx:692.
  • gemini-code-assist[bot] discussion 3373123693: requested flow-root on measured virtual rows; fixed in src/renderer/src/components/chat/ChatView.tsx:183.
  • cubic-dev-ai[bot] review summary: identified the date-divider margin measurement issue; fixed in src/renderer/src/components/chat/ChatView.tsx:183.
  • cubic-dev-ai[bot] discussion 3373167159: same DateDivider margin measurement issue; fixed in src/renderer/src/components/chat/ChatView.tsx:183.
  • codeant-ai[bot] issue comment 4648886678: quota notice only; no actionable code review.
  • coderabbitai[bot] issue comment 4648887171: rate-limit/status comment only; no actionable finding.

@agent-relay-code

Copy link
Copy Markdown
Contributor

pr-reviewer applied fixes — committed and pushed a2422c3 to this PR. The notes below describe what changed.

Implemented fixes for the valid PR findings.

Changed src/main/broker.ts so failed broker CLI inspection no longer assumes modern flag support, and changed the compat shim at src/main/broker.ts to write a JS launcher plus platform wrapper using process.execPath instead of requiring a system node.

Added broker-start regression coverage in src/main/broker.test.ts, including modern direct launch, legacy compat-shim launch, duplicate reused start behavior, failed inspection fallback, and an executed POSIX shim fixture.

Verification run:
node node_modules/vitest/vitest.mjs run src/main/broker.test.ts src/main/ipc-handlers.test.ts --project node passed: 2 files, 67 tests.
Direct build also passed via npm run generate:mcp-resources && node scripts/install-relayfile-mount.mjs --optional && node node_modules/electron-vite/dist/cli.js build.

Addressed comments

  • codeant-ai[bot]: Raised only “free trial PR review limit reached”; no code issue to fix.
  • coderabbitai[bot]: Compat shim requires system node; fixed in src/main/broker.ts:344 and src/main/broker.ts:412.
  • coderabbitai[bot]: Broker CLI inspection fallback was optimistic; fixed in src/main/broker.ts:332 with legacy-compatible fallback flags. I did not use all-false because this resolver requires supportsName to enter the compat shim path.
  • coderabbitai[bot]: Add compat-shim broker-start regression tests, including duplicate/replay start behavior; fixed in src/main/broker.test.ts:615, src/main/broker.test.ts:631, and src/main/broker.test.ts:687.
  • coderabbitai[bot]: Normalize whitespace-only cwd; stale, already handled in current checkout because src/main/ipc-handlers.ts:269 trims and whitespace-only input falls through to brokerManager.listPersonas(projectId) at src/main/ipc-handlers.ts:281 without forwarding ''.
  • coderabbitai[bot]: Top-level docstring coverage warning; not changed because it is a generic external pre-merge warning with no specific failing file or demonstrated checkout breakage.

@agent-relay-code

Copy link
Copy Markdown
Contributor

⚠️ pr-reviewer did not push — the PR branch advanced during the review, so fixes were withheld to avoid overwriting newer commits. Re-trigger the review once the branch settles. The notes below are advisory and were not pushed.

No additional code edits were needed in this checkout. I validated the PR changes and the existing fixes against the current files.

Validation run:

  • npm exec -- vitest run src/main/broker.test.ts src/main/ipc-handlers.test.ts passed: 67 tests
  • npm exec -- tsc --noEmit -p tsconfig.json passed
  • npm run verify:mcp-resources-drift passed
  • npm test passed: 104 tests
  • npm run build passed
  • The generated compat shim sample is exercised by src/main/broker.test.ts

GitHub currently reports PR #164 as merged. I’m not printing READY because there is no longer an open PR awaiting human action.

Addressed comments

  • codeant-ai[bot] issue comment 4648834537: quota notice only; no actionable code review.
  • coderabbitai[bot] issue comment 4648835753: status/walkthrough plus rate-limit notice; no actionable code change required.
  • codeant-ai[bot] issue comment 4648873649: quota notice only; no actionable code review.
  • khaliqgant issue comment 4648874471: stated the earlier review feedback was addressed; validated in current checkout at src/main/ipc-handlers.ts, src/main/broker.ts, src/main/broker.ts, src/main/broker.ts, and src/main/broker.ts.
  • agent-relay-code[bot] issue comment 4648910805: prior harness summary; validated current checkout still contains those fixes at the same referenced files/lines.
  • gemini-code-assist[bot] discussion_r3373083995: unvalidated broker:list-personas cwd; fixed in src/main/ipc-handlers.ts, regression in src/main/ipc-handlers.test.ts.
  • gemini-code-assist[bot] discussion_r3373084001: synchronous broker flag inspection; fixed with async execFile in src/main/broker.ts.
  • gemini-code-assist[bot] discussion_r3373084005: resolveHarnessBrokerBinary should be async; fixed in src/main/broker.ts.
  • gemini-code-assist[bot] discussion_r3373084010: startup should await broker binary resolution; fixed in src/main/broker.ts.
  • gemini-code-assist[bot] discussion_r3373084013: shim signal propagation can throw; fixed in src/main/broker.ts.
  • gemini-code-assist[bot] review 4449345299: summary of the same Gemini findings above; fixed at the same referenced lines.
  • coderabbitai[bot] discussion_r3373123520: missing startup regression coverage for compat shim; fixed in src/main/broker.test.ts, including duplicate start coalescing and generated shim execution at src/main/broker.test.ts.
  • coderabbitai[bot] discussion_r3373123523: optimistic broker CLI inspection fallback; fixed with conservative legacy fallback in src/main/broker.ts.
  • coderabbitai[bot] discussion_r3373123529: shim depended on system node; fixed with app process.execPath wrappers in src/main/broker.ts.
  • coderabbitai[bot] discussion_r3373123532: whitespace-only cwd; stale against current checkout because blank cwd skips the branch and calls brokerManager.listPersonas(projectId) at src/main/ipc-handlers.ts.
  • coderabbitai[bot] review 4449389056: summary of the four CodeRabbit findings above; fixed or stale at the referenced lines.

@agent-relay-code

Copy link
Copy Markdown
Contributor

⚠️ pr-reviewer push failed (exit 1) — fixes were not applied to the PR. The notes below are advisory and were not pushed.

Implemented the valid fixes I could reproduce in the current checkout.

Changed src/main/broker.ts so failed broker CLI inspection now falls back to the legacy-compatible shim path instead of assuming modern flags, and changed the compat shim wrapper to invoke the app’s process.execPath instead of requiring node on PATH. Added broker-start regression tests in src/main/broker.test.ts, including modern direct launch, legacy shim launch, failed inspection fallback, duplicate start coalescing, and a real executed POSIX shim fixture.

Validation passed:

  • npm ci
  • npm exec -- vitest run src/main/broker.test.ts src/main/ipc-handlers.test.ts --project node
  • npm exec -- tsc --noEmit -p tsconfig.json
  • npm run verify:mcp-resources-drift
  • npm test
  • npm run build

GitHub reports PR #164 is already closed and merged, so I’m not ending with READY.

Addressed comments

  • codeant-ai[bot] issue comment 4648834537: quota notice only; no actionable code review.
  • coderabbitai[bot] issue comment 4648835753: status/walkthrough plus rate-limit notice; no actionable code change required.
  • coderabbitai[bot]: compat shim required system node; fixed in src/main/broker.ts:410 and covered in src/main/broker.test.ts:549.
  • coderabbitai[bot]: broker CLI inspection fallback was optimistic; fixed in src/main/broker.ts:332 and covered in src/main/broker.test.ts:576.
  • coderabbitai[bot]: missing broker-start compat-shim regression coverage, including duplicate/replay behavior; fixed in src/main/broker.test.ts:527, src/main/broker.test.ts:549, src/main/broker.test.ts:593, and src/main/broker.test.ts:618.
  • coderabbitai[bot]: whitespace-only cwd; stale, already handled in current checkout by src/main/ipc-handlers.ts:269 falling through to src/main/ipc-handlers.ts:281.
  • coderabbitai[bot]: docstring coverage warning; invalid for this checkout because it is a generic external warning with no demonstrated failing file.
  • gemini-code-assist[bot]: unvalidated broker:list-personas cwd; already handled in current checkout at src/main/ipc-handlers.ts:271, with regression at src/main/ipc-handlers.test.ts:228.
  • gemini-code-assist[bot]: synchronous broker flag inspection; already handled in current checkout at src/main/broker.ts:323.
  • gemini-code-assist[bot]: resolveHarnessBrokerBinary should be async and startup should await it; already handled in current checkout at src/main/broker.ts:424 and src/main/broker.ts:1544.
  • gemini-code-assist[bot]: shim signal propagation can throw; already handled in current checkout at src/main/broker.ts:392.
  • agent-relay-code[bot] issue comment 4649039803: prior harness summary raised the same valid broker fallback, shim launcher, and regression-test items; fixed/covered at src/main/broker.ts:332, src/main/broker.ts:410, src/main/broker.test.ts:576, and src/main/broker.test.ts:618.
  • agent-relay-code[bot] issue comment 4649050912: claimed no additional edits were needed, but current checkout still reproduced the fallback/shim issues; fixed at src/main/broker.ts:332 and src/main/broker.ts:410.
  • khaliqgant issue comment 4648874471: stated earlier feedback was addressed; validated current checkout and fixed the remaining reproducible broker issues at src/main/broker.ts:332 and src/main/broker.ts:410.

@agent-relay-code

Copy link
Copy Markdown
Contributor

⚠️ pr-reviewer push failed (exit 1) — fixes were not applied to the PR. The notes below are advisory and were not pushed.

Implemented the remaining validated PR fixes and kept the changes scoped to broker compatibility and its tests.

Changed src/main/broker.ts so failed broker CLI probing falls back to legacy-compatible flags, and the compatibility shim now writes a JS launcher plus POSIX/Windows wrappers that invoke process.execPath instead of requiring system node. Added regression coverage in src/main/broker.test.ts for modern launch, legacy shim launch, failed-inspection fallback, duplicate start coalescing, and executing the generated POSIX shim against a throwaway broker fixture.

Validation passed:

  • npm ci
  • npm exec -- vitest run src/main/broker.test.ts src/main/ipc-handlers.test.ts --project node
  • npm exec -- tsc --noEmit -p tsconfig.json
  • npm run verify:mcp-resources-drift
  • npm test
  • npm run build
  • conflict marker scan with rg -n "^(<<<<<<<|=======|>>>>>>>)" -S .

Review comments checked against the public PR page: #164

Addressed comments

  • codeant-ai[bot]: quota notice only; no actionable code review.
  • coderabbitai[bot]: initial status/walkthrough and rate-limit notice; no actionable code change required.
  • coderabbitai[bot]: docstring coverage warning; no specific failing file or demonstrated checkout breakage, so no code change.
  • gemini-code-assist[bot]: unvalidated broker:list-personas cwd; already fixed in current checkout at src/main/ipc-handlers.ts:271, regression at src/main/ipc-handlers.test.ts:228.
  • gemini-code-assist[bot]: synchronous broker flag inspection; already fixed with async execFile at src/main/broker.ts:331.
  • gemini-code-assist[bot]: resolveHarnessBrokerBinary should be async; already fixed at src/main/broker.ts:432.
  • gemini-code-assist[bot]: startup should await async broker binary resolution; already fixed at src/main/broker.ts:1552.
  • gemini-code-assist[bot]: shim signal propagation can throw; already fixed at src/main/broker.ts:398.
  • gemini-code-assist[bot] review summary: same Gemini findings above; fixed at the same referenced lines.
  • coderabbitai[bot]: add compat-shim broker-start regression tests, including duplicate/replay behavior; fixed in src/main/broker.test.ts:617, src/main/broker.test.ts:635, src/main/broker.test.ts:656, and src/main/broker.test.ts:701.
  • coderabbitai[bot]: optimistic broker CLI inspection fallback; fixed in src/main/broker.ts:341. I kept supportsName: true because this resolver needs that legacy capability to enter the compat-shim path.
  • coderabbitai[bot]: compat shim depends on system node; fixed with JS payload plus process.execPath wrappers in src/main/broker.ts:352 and src/main/broker.ts:412.
  • coderabbitai[bot]: whitespace-only cwd; stale against current checkout because whitespace trims falsy and calls brokerManager.listPersonas(projectId) without forwarding '' at src/main/ipc-handlers.ts:269 and src/main/ipc-handlers.ts:281.
  • coderabbitai[bot] review summary: same four CodeRabbit findings above; fixed or stale at the referenced lines.
  • khaliqgant: noted earlier review feedback was addressed; validated current checkout and completed the remaining valid CodeRabbit fixes at the lines above.
  • agent-relay-code[bot]: prior harness summaries; validated current checkout and implemented the still-valid shim/fallback/test coverage fixes at the lines above.

PR #164 appears merged on GitHub, so there is no open PR awaiting human action; I am not printing READY.

@agent-relay-code

Copy link
Copy Markdown
Contributor

Implemented the validated fixes in the current checkout.

Changed src/main/broker.ts so failed broker CLI flag inspection falls back to the legacy-compatible shim path instead of assuming modern flags. Updated the generated broker compat shim at src/main/broker.ts to write a JS payload plus POSIX/Windows wrappers that invoke process.execPath, avoiding reliance on a system node.

Added focused regressions in src/main/broker.test.ts for modern direct launch, legacy shim launch, failed-inspection fallback, duplicate start coalescing, and executing the generated POSIX shim against a throwaway broker fixture.

Validation passed:

  • npm ci
  • npm exec -- vitest run src/main/broker.test.ts src/main/ipc-handlers.test.ts --project node
  • npm exec -- tsc --noEmit -p tsconfig.json
  • npm test
  • npm run verify:mcp-resources-drift
  • npm run build
  • Conflict-marker scan found no matches

GitHub API reports PR #164 is already closed and merged, so I am not ending with READY.

Addressed comments

  • codeant-ai[bot] issue comment 4648834537: quota notice only; no actionable code review.
  • coderabbitai[bot] issue comment 4648835753: initial walkthrough/rate-limit notice and generic docstring warning; no specific checkout breakage to fix.
  • coderabbitai[bot]: optimistic broker CLI inspection fallback; fixed in src/main/broker.ts:332.
  • coderabbitai[bot]: compat shim depended on system node; fixed in src/main/broker.ts:417.
  • coderabbitai[bot]: missing compat-shim broker-start regression coverage; fixed in src/main/broker.test.ts:583, src/main/broker.test.ts:603, src/main/broker.test.ts:622, and src/main/broker.test.ts:648.
  • coderabbitai[bot]: whitespace-only cwd; stale, already handled by src/main/ipc-handlers.ts:269 falling through to src/main/ipc-handlers.ts:281.
  • gemini-code-assist[bot]: unvalidated broker:list-personas cwd; already handled in current checkout at src/main/ipc-handlers.ts:271, with regression at src/main/ipc-handlers.test.ts:228.
  • gemini-code-assist[bot]: synchronous broker flag inspection; already handled in current checkout with async execFile at src/main/broker.ts:323.
  • gemini-code-assist[bot]: resolveHarnessBrokerBinary should be async and startup should await it; already handled at src/main/broker.ts:432 and src/main/broker.ts:1547.
  • gemini-code-assist[bot]: shim signal propagation can throw; already handled at src/main/broker.ts:390.
  • agent-relay-code[bot] issue comment 4649178861: advisory note said fixes were not applied; validated and fixed fallback/shim issues at src/main/broker.ts:332 and src/main/broker.ts:417, with tests at src/main/broker.test.ts:603 and src/main/broker.test.ts:648.
  • agent-relay-code[bot] issue comment 4649206884: same advisory fix set; applied in current checkout at the same lines above.
  • khaliqgant: noted earlier feedback was addressed; validated current checkout and fixed the remaining reproducible broker fallback/shim gaps at src/main/broker.ts:332 and src/main/broker.ts:417.

@agent-relay-code

Copy link
Copy Markdown
Contributor

⚠️ pr-reviewer push failed (exit 1) — fixes were not applied to the PR. The notes below are advisory and were not pushed.

Implemented the validated fixes in the current checkout.

Changed src/main/broker.ts so failed broker CLI flag inspection falls back to the legacy-compatible shim path instead of assuming modern flags. Updated the generated broker compat shim at src/main/broker.ts to write a JS payload plus POSIX/Windows wrappers that invoke process.execPath, avoiding reliance on a system node.

Added focused regressions in src/main/broker.test.ts for modern direct launch, legacy shim launch, failed-inspection fallback, duplicate start coalescing, and executing the generated POSIX shim against a throwaway broker fixture.

Validation passed:

  • npm ci
  • npm exec -- vitest run src/main/broker.test.ts src/main/ipc-handlers.test.ts --project node
  • npm exec -- tsc --noEmit -p tsconfig.json
  • npm test
  • npm run verify:mcp-resources-drift
  • npm run build
  • Conflict-marker scan found no matches

GitHub API reports PR #164 is already closed and merged, so I am not ending with READY.

Addressed comments

  • codeant-ai[bot] issue comment 4648834537: quota notice only; no actionable code review.
  • coderabbitai[bot] issue comment 4648835753: initial walkthrough/rate-limit notice and generic docstring warning; no specific checkout breakage to fix.
  • coderabbitai[bot]: optimistic broker CLI inspection fallback; fixed in src/main/broker.ts:332.
  • coderabbitai[bot]: compat shim depended on system node; fixed in src/main/broker.ts:417.
  • coderabbitai[bot]: missing compat-shim broker-start regression coverage; fixed in src/main/broker.test.ts:583, src/main/broker.test.ts:603, src/main/broker.test.ts:622, and src/main/broker.test.ts:648.
  • coderabbitai[bot]: whitespace-only cwd; stale, already handled by src/main/ipc-handlers.ts:269 falling through to src/main/ipc-handlers.ts:281.
  • gemini-code-assist[bot]: unvalidated broker:list-personas cwd; already handled in current checkout at src/main/ipc-handlers.ts:271, with regression at src/main/ipc-handlers.test.ts:228.
  • gemini-code-assist[bot]: synchronous broker flag inspection; already handled in current checkout with async execFile at src/main/broker.ts:323.
  • gemini-code-assist[bot]: resolveHarnessBrokerBinary should be async and startup should await it; already handled at src/main/broker.ts:432 and src/main/broker.ts:1547.
  • gemini-code-assist[bot]: shim signal propagation can throw; already handled at src/main/broker.ts:390.
  • agent-relay-code[bot] issue comment 4649178861: advisory note said fixes were not applied; validated and fixed fallback/shim issues at src/main/broker.ts:332 and src/main/broker.ts:417, with tests at src/main/broker.test.ts:603 and src/main/broker.test.ts:648.
  • agent-relay-code[bot] issue comment 4649206884: same advisory fix set; applied in current checkout at the same lines above.
  • khaliqgant: noted earlier feedback was addressed; validated current checkout and fixed the remaining reproducible broker fallback/shim gaps at src/main/broker.ts:332 and src/main/broker.ts:417.

@agent-relay-code

Copy link
Copy Markdown
Contributor

⚠️ pr-reviewer push failed (exit 1) — fixes were not applied to the PR. The notes below are advisory and were not pushed.

Implemented the remaining validated fixes in the current checkout.

Changed src/main/broker.ts so failed broker CLI probing falls back to legacy-compatible shim flags instead of assuming modern support. Changed the broker compat shim to write a JS payload plus POSIX/Windows wrappers that invoke process.execPath, avoiding a system node dependency at src/main/broker.ts. Added broker-start regression coverage, including modern direct launch, legacy shim launch, failed-inspection fallback, duplicate start coalescing, and executing the generated POSIX shim against a throwaway broker fixture in src/main/broker.test.ts.

Validated:

  • npm ci
  • npm exec -- vitest run src/main/broker.test.ts src/main/ipc-handlers.test.ts --project node
  • npm exec -- tsc --noEmit -p tsconfig.json
  • npm run verify:mcp-resources-drift
  • npm test
  • npm run build
  • rg -n "^(<<<<<<<|=======|>>>>>>>)" -S . found no conflict markers

GitHub’s PR page shows #164 is already merged, so I’m not printing READY. Source checked: #164

Addressed comments

  • codeant-ai[bot] issue comment 4648834537: quota notice only; no actionable code review.
  • codeant-ai[bot] issue comment 4648873649: quota notice only; no actionable code review.
  • coderabbitai[bot] issue comment 4648835753: status/walkthrough plus rate-limit notice; no actionable code change required.
  • coderabbitai[bot] pre-merge docstring coverage warning: no specific failing file or reproducible checkout breakage, so no code change.
  • gemini-code-assist[bot] discussion r3373083995: unvalidated broker:list-personas cwd; already handled in current checkout at src/main/ipc-handlers.ts:271, regression at src/main/ipc-handlers.test.ts:228.
  • gemini-code-assist[bot] discussion r3373084001: synchronous broker flag inspection; already async in current checkout at src/main/broker.ts:323.
  • gemini-code-assist[bot] discussion r3373084005: resolveHarnessBrokerBinary should be async; already handled at src/main/broker.ts:434.
  • gemini-code-assist[bot] discussion r3373084010: startup should await broker binary resolution; already handled at src/main/broker.ts:1554.
  • gemini-code-assist[bot] discussion r3373084013: shim signal propagation can throw; already handled at src/main/broker.ts:390.
  • gemini-code-assist[bot] review 4449345299: summary of the same Gemini findings above; fixed or already handled at the referenced lines.
  • coderabbitai[bot] discussion r3373123520: missing broker-start compat-shim coverage; fixed in src/main/broker.test.ts:617, src/main/broker.test.ts:641, src/main/broker.test.ts:657, and src/main/broker.test.ts:671.
  • coderabbitai[bot] discussion r3373123523: optimistic broker CLI inspection fallback; fixed in src/main/broker.ts:332.
  • coderabbitai[bot] discussion r3373123529: compat shim depended on system node; fixed in src/main/broker.ts:344 and src/main/broker.ts:412.
  • coderabbitai[bot] discussion r3373123532: whitespace-only cwd; stale because current code trims and falls through without forwarding blank cwd at src/main/ipc-handlers.ts:269 and src/main/ipc-handlers.ts:281.
  • coderabbitai[bot] review 4449389056: summary of the four CodeRabbit findings above; fixed or stale at the referenced lines.
  • khaliqgant issue comment 4648874471: stated earlier feedback was addressed; validated current checkout and fixed remaining reproducible broker issues at src/main/broker.ts:332 and src/main/broker.ts:412.
  • agent-relay-code[bot] issue comment 4648910805: prior harness summary; validated current checkout and completed the still-reproducible fallback/shim/test coverage fixes at src/main/broker.ts:332, src/main/broker.ts:412, and src/main/broker.test.ts:617.
  • agent-relay-code[bot] issue comment 4649039803: prior harness summary raised the same broker fallback, shim launcher, and regression-test items; fixed at src/main/broker.ts:332, src/main/broker.ts:412, and src/main/broker.test.ts:641.
  • agent-relay-code[bot] issue comment 4649050912: claimed no additional edits were needed, but current checkout still reproduced fallback/shim gaps; fixed at src/main/broker.ts:332 and src/main/broker.ts:412.
  • agent-relay-code[bot] issue comment 4649178861: advisory because its push failed; applied and verified the same valid fixes at src/main/broker.ts:332, src/main/broker.ts:412, and src/main/broker.test.ts:671.
  • agent-relay-code[bot] issue comment 4649206884: advisory because its push failed; applied and verified the same valid fixes at src/main/broker.ts:332, src/main/broker.ts:412, and src/main/broker.test.ts:671.
  • agent-relay-code[bot] misidentified feat(chat): virtualize message list with @tanstack/react-virtual #166 advisory comment: invalid for this checkout because it refers to ChatView.tsx and PR feat(chat): virtualize message list with @tanstack/react-virtual #166, not PR Harden broker startup and persona discovery #164.

@agent-relay-code

Copy link
Copy Markdown
Contributor

Implemented the validated fixes in the checkout.

Changed src/main/broker.ts so broker CLI help inspection reads stdout and stderr, and failed probing falls back to the legacy-compatible shim path instead of assuming modern flags. Changed shim generation at src/main/broker.ts to use process.execPath wrappers instead of relying on node from PATH.

Added regression coverage in src/main/broker.test.ts, src/main/broker.test.ts, src/main/broker.test.ts, src/main/broker.test.ts, and src/main/broker.test.ts, including executing the generated POSIX shim against a throwaway broker fixture.

Validation passed:

  • npm ci
  • npx vitest run src/main/broker.test.ts
  • npx vitest run src/main/ipc-handlers.test.ts
  • npm exec -- vitest run src/main/broker.test.ts src/main/ipc-handlers.test.ts --project node
  • npm exec -- tsc --noEmit -p tsconfig.json
  • npm test
  • npm run verify:mcp-resources-drift
  • npm run build
  • conflict-marker scan found no matches

GitHub reports PR #164 is already closed and merged, so I am not ending with READY.

Addressed comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant