Fix Agent Relay startup and broker issue UI#185
Conversation
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Warning Review limit reached
More reviews will be available in 46 minutes and 19 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 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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
📝 WalkthroughWalkthroughThis PR bumps Agent Relay dependencies to version 8.3.1, adds an environment variable override for broker binary selection, refactors RelayBurn ingest to run in a child process with diagnostic filtering, removes the graph terminal view mode, and deduplicates broker errors in the store and UI. ChangesAgent Relay Release & UI/Operation Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the agent graph visualization feature (including the @xyflow/react dependency and associated components) and updates various @agent-relay packages to version 8.3.1. It also introduces a mechanism to run RelayBurn ingest in a spawned child process with a diagnostic filter to suppress specific tool-result warnings, adds support for overriding the broker binary path via the AGENT_RELAY_BIN environment variable, and improves broker error handling. The review feedback suggests using process.exit(1) instead of process.exitCode = 1 in the ingest child process to prevent it from hanging if active handles remain in the event loop. Additionally, it notes that the deduplication logic added to BrokerDetailsPage.tsx is redundant since the store's prependBrokerError function already handles deduplication.
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd6ef5fce8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/broker.ts (1)
287-302:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProbe the unpacked broker path before the executability check.
canExecute()is still being run against theapp.asarpath here. In packaged builds the broker binary is unpacked underapp.asar.unpacked, so this branch can miss the packaged broker entirely and fall through to the local-debug or SDK fallback instead of using the intended bundled binary.Suggested fix
const optionalPackageBinary = join( __dirname, '..', '..', 'node_modules', '`@agent-relay`', `broker-${suffix}`, 'bin', process.platform === 'win32' ? 'agent-relay-broker.exe' : 'agent-relay-broker' ) - if (canExecute(optionalPackageBinary)) return unpackIfPackaged(optionalPackageBinary) + const bundledOptionalPackageBinary = unpackIfPackaged(optionalPackageBinary) + if (canExecute(bundledOptionalPackageBinary)) return bundledOptionalPackageBinary🤖 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 287 - 302, canExecute is being called against the app.asar path so packaged builds miss the unpacked binary; modify the check around optionalPackageBinary (used in resolveBrokerBinaryOverride flow) to first compute unpackedBinary via the unpackIfPackaged helper (which uses app.isPackaged) and probe that path before or instead of the original optionalPackageBinary, i.e., call canExecute(unpackedBinary) and return unpackedBinary when true (fall back to the existing optionalPackageBinary check only if needed).
🧹 Nitpick comments (2)
src/main/__tests__/burn.test.ts (1)
15-47: 💤 Low valueConsider adding a duplicate-warning regression test.
Per the coding guidelines, tests touching spawned processes should include duplicate/replay cases. The filter logic handles multiple warnings, but there's no test verifying this. Adding a test with two consecutive warnings would confirm
suppressedCount()increments correctly for each.🧪 Suggested test case
test('burn ingest diagnostic filter suppresses multiple warnings in sequence', () => { const filter = createBurnIngestDiagnosticFilter() const twoWarnings = TOOL_RESULT_WARNING + TOOL_RESULT_WARNING const output = filter.filter(twoWarnings) + filter.flush() assert.equal(output, 'before\nafter\nbefore\nafter\n') assert.equal(filter.suppressedCount(), 2) })🤖 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/__tests__/burn.test.ts` around lines 15 - 47, Add a regression test that sends two consecutive TOOL_RESULT_WARNING messages through createBurnIngestDiagnosticFilter to ensure duplicate warnings are suppressed and counted separately: create a test (e.g., "burn ingest diagnostic filter suppresses multiple warnings in sequence") that constructs twoWarnings = TOOL_RESULT_WARNING + TOOL_RESULT_WARNING, passes it through filter.filter(...) + filter.flush(), asserts the combined output equals 'before\nafter\nbefore\nafter\n' and that filter.suppressedCount() === 2, using the existing createBurnIngestDiagnosticFilter, TOOL_RESULT_WARNING, filter.filter, filter.flush, and suppressedCount symbols.Source: Coding guidelines
src/renderer/src/components/broker/BrokerDetailsPage.tsx (1)
201-201: 💤 Low valueRemove redundant
break-wordsclass.The class list includes both
break-words(which setsoverflow-wrap: break-word) and[overflow-wrap:anywhere](which setsoverflow-wrap: anywhere). The arbitrary value[overflow-wrap:anywhere]overridesbreak-words, making it redundant.♻️ Simplify class list
-<p className="mt-1 whitespace-pre-wrap break-words text-sm leading-relaxed text-[var(--pear-text)] [overflow-wrap:anywhere]"> +<p className="mt-1 whitespace-pre-wrap text-sm leading-relaxed text-[var(--pear-text)] [overflow-wrap:anywhere]">🤖 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/renderer/src/components/broker/BrokerDetailsPage.tsx` at line 201, The class list on the paragraph in BrokerDetailsPage (the <p> element with className containing "mt-1 whitespace-pre-wrap break-words text-sm leading-relaxed text-[var(--pear-text)] [overflow-wrap:anywhere]") contains a redundant "break-words" since "[overflow-wrap:anywhere]" overrides it; remove "break-words" from the className so the element keeps "whitespace-pre-wrap", "[overflow-wrap:anywhere]", and the other styles but not the redundant class.
🤖 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 709-719: Current test only asserts the default packaged-binary
selection; add regression cases to cover the new broker-start branches by
extending broker.test.ts: create tests that (1) set AGENT_RELAY_BIN to a valid
override path and assert mock.HarnessDriverClient.spawn was called with that
binaryPath, (2) set AGENT_RELAY_BIN to an invalid path and assert the
manager.start falls back to the packaged binary (or adjacent build) as
appropriate, and (3) simulate "packaged-mode" resolution explicitly and assert
packaged binaryPath selection; reuse BrokerManager and manager.start along with
the existing mock.HarnessDriverClient.spawn calls to inspect
spawnOptions.binaryPath in each test and clean up env changes between tests and
after manager.shutdown.
In `@src/main/broker.ts`:
- Around line 304-307: The fallback path for the local relay binary hardcodes
"agent-relay-broker" which fails on Windows; update the construction of
localBinary in the function that computes the path (the variable named
localBinary) to append ".exe" when process.platform === 'win32' (e.g., choose
filename = process.platform === 'win32' ? 'agent-relay-broker.exe' :
'agent-relay-broker' and pass that to join), then keep the existing
canExecute(localBinary) check and warning/return behavior unchanged.
In `@src/renderer/src/components/broker/BrokerDetailsPage.tsx`:
- Around line 90-102: The duplicate composite-key logic (getBrokerErrorKey in
BrokerDetailsPage.tsx and brokerErrorKey in agent-store.ts) should be extracted
into a shared utility module (e.g., src/shared/lib/broker-errors.ts) that
exports a single getBrokerErrorKey function accepting { message, projectId } and
returning `${projectId || 'global'}\0${message}`; replace the local
getBrokerErrorKey implementation in BrokerDetailsPage.tsx and the brokerErrorKey
usage in agent-store.ts with imports from that shared module and update
dedupeBrokerErrorEntries to call the shared getBrokerErrorKey to keep key format
consistent across the codebase.
In `@src/renderer/src/stores/agent-store.ts`:
- Around line 87-96: Add unit tests for brokerErrorKey and prependBrokerError to
cover deduplication by the (projectId, message) composite key,
newest-first/prepend ordering, enforcement of MAX_BROKER_ERRORS cap, and mapping
an undefined/empty projectId to 'global'. Specifically, create Vitest cases that
(1) call brokerErrorKey with empty/undefined projectId and verify it emits
'global\0message', (2) call prependBrokerError with an existing entry and a
duplicate (same projectId and message) to assert the duplicate is removed and
the new entry is placed first, (3) insert more than MAX_BROKER_ERRORS items via
repeated prependBrokerError calls and assert the returned array length equals
MAX_BROKER_ERRORS and older entries are dropped, and (4) ensure two entries with
same message but different projectId are treated as distinct; reference
brokerErrorKey, prependBrokerError, and MAX_BROKER_ERRORS to locate the code
under test.
---
Outside diff comments:
In `@src/main/broker.ts`:
- Around line 287-302: canExecute is being called against the app.asar path so
packaged builds miss the unpacked binary; modify the check around
optionalPackageBinary (used in resolveBrokerBinaryOverride flow) to first
compute unpackedBinary via the unpackIfPackaged helper (which uses
app.isPackaged) and probe that path before or instead of the original
optionalPackageBinary, i.e., call canExecute(unpackedBinary) and return
unpackedBinary when true (fall back to the existing optionalPackageBinary check
only if needed).
---
Nitpick comments:
In `@src/main/__tests__/burn.test.ts`:
- Around line 15-47: Add a regression test that sends two consecutive
TOOL_RESULT_WARNING messages through createBurnIngestDiagnosticFilter to ensure
duplicate warnings are suppressed and counted separately: create a test (e.g.,
"burn ingest diagnostic filter suppresses multiple warnings in sequence") that
constructs twoWarnings = TOOL_RESULT_WARNING + TOOL_RESULT_WARNING, passes it
through filter.filter(...) + filter.flush(), asserts the combined output equals
'before\nafter\nbefore\nafter\n' and that filter.suppressedCount() === 2, using
the existing createBurnIngestDiagnosticFilter, TOOL_RESULT_WARNING,
filter.filter, filter.flush, and suppressedCount symbols.
In `@src/renderer/src/components/broker/BrokerDetailsPage.tsx`:
- Line 201: The class list on the paragraph in BrokerDetailsPage (the <p>
element with className containing "mt-1 whitespace-pre-wrap break-words text-sm
leading-relaxed text-[var(--pear-text)] [overflow-wrap:anywhere]") contains a
redundant "break-words" since "[overflow-wrap:anywhere]" overrides it; remove
"break-words" from the className so the element keeps "whitespace-pre-wrap",
"[overflow-wrap:anywhere]", and the other styles but not the redundant class.
🪄 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: 081ac831-30d2-43a3-83aa-025d77077ef5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
electron-builder.mcp-resources.ymlpackage.jsonsrc/main/__tests__/burn.test.tssrc/main/broker.test.tssrc/main/broker.tssrc/main/burn.tssrc/preload/index.tssrc/renderer/src/components/broker/BrokerDetailsPage.tsxsrc/renderer/src/components/graph/AgentNode.tsxsrc/renderer/src/components/graph/GraphView.tsxsrc/renderer/src/components/graph/HierarchyEdge.tsxsrc/renderer/src/components/graph/MessageEdge.tsxsrc/renderer/src/components/terminal/TerminalPane.tsxsrc/renderer/src/stores/agent-store.tssrc/renderer/src/stores/ui-store.tssrc/shared/types/ipc.ts
💤 Files with no reviewable changes (6)
- src/renderer/src/components/graph/GraphView.tsx
- src/renderer/src/components/graph/HierarchyEdge.tsx
- src/renderer/src/components/graph/MessageEdge.tsx
- src/preload/index.ts
- src/renderer/src/components/graph/AgentNode.tsx
- src/shared/types/ipc.ts
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
willwashburn
force-pushed
the
codex/agent-relay-8-3-broker-fixes
branch
from
25f40c0 to
87e966f
Compare
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
willwashburn
force-pushed
the
codex/agent-relay-8-3-broker-fixes
branch
from
87e966f to
d7191cd
Compare
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Updated this branch from main (rebased onto c67ca6a) and pushed d7191cd. Handled the review feedback:
Validation passed:
The CodeAnt/CodeRabbit limit comments are billing/rate-limit notices, so there is no code action to take for those. |
willwashburn
force-pushed
the
codex/agent-relay-8-3-broker-fixes
branch
from
d7191cd to
427983b
Compare
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Rebased onto latest main (4e1e885) and pushed 427983b. Handled the remaining PR feedback and conflicts:
Validation passed locally:
PR is conflict-free now; CI is still running. |
1 participant
Summary
Verification