Harden workforce persona spawn readiness#153
Conversation
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
📝 WalkthroughWalkthroughThis PR adds workforce persona harness readiness verification: after a persona is spawned and registered with the broker, the system marks it pending, waits for harness mount success from worker-stream output, sends an injected probe message to confirm broker delivery capability, and releases the persona if verification fails. Unverified personas are filtered from agent listings. Test mocks now support event history and listener subscriptions. The spawn handler wraps cache invalidation in try/finally. ChangesPersona Readiness Verification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 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 introduces a verification mechanism for Workforce persona harness and broker delivery readiness before they are exposed to the system. The review identified two critical issues: a potential state leak in pendingPersonaReadiness if broker registration verification fails and throws an error, and a race condition in waitForPersonaHarnessReadyOutput that can cause duplicate event processing and corrupt the readiness check output.
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.
| this.markPersonaReadinessPending(session, spawned.name) | ||
| try { | ||
| await this.verifyPersonaHarnessReady(session, spawned.name, trimmedPersonaId, { | ||
| workerStreamBaselineSeq: spawned.workerStreamBaselineSeq, | ||
| workerStreamBaselineCount: spawned.workerStreamBaselineCount | ||
| }) | ||
| } catch (err) { | ||
| await this.releaseUnreadyPersona(session, spawned.name, 'persona harness readiness verification failed') | ||
| throw err | ||
| } finally { | ||
| this.clearPersonaReadinessPending(session, spawned.name) | ||
| } | ||
| return { | ||
| name: spawned.name, | ||
| runtime: spawned.runtime, | ||
| ...(spawned.cli ? { cli: spawned.cli } : {}) | ||
| } | ||
| } | ||
|
|
||
| await session.client.release(spawned.name, 'persona broker registration verification failed').catch((err) => { | ||
| if (!isMissingAgentError(err)) { | ||
| console.warn(`[broker] Failed to release unverified persona agent ${spawned.name}:`, err) | ||
| } | ||
| }) | ||
| await this.releaseUnreadyPersona(session, spawned.name, 'persona broker registration verification failed') | ||
| throw new Error( | ||
| `Workforce persona ${trimmedPersonaId} launched but did not stay registered with the broker` | ||
| ) |
There was a problem hiding this comment.
If verifyPersonaBrokerRegistration (called at line 2546) throws an error (e.g., due to a network or broker connection failure during listAgents), the error will propagate immediately. Because spawnPersonaWithMode has already marked the agent as pending, and the error prevents entering the try...finally block or reaching the releaseUnreadyPersona call at the end of the method, the agent's name will be permanently leaked in pendingPersonaReadiness. This will cause the agent to be permanently hidden from listAgents calls. Consider wrapping the registration verification in a try...catch block to ensure releaseUnreadyPersona is always called on any failure.
| const observe = (event: BrokerEvent): void => { | ||
| if (!isWorkerStreamForAgent(event, name)) return | ||
| output += brokerEventChunk(event) | ||
| const failure = personaHarnessFailedFromOutput(output) | ||
| if (failure) { | ||
| finish(new Error(`Workforce persona ${personaId} failed during harness startup: ${failure}`)) | ||
| return | ||
| } | ||
| if (personaHarnessReadyFromOutput(output)) { | ||
| finish() | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a potential race condition and double-processing of events in waitForPersonaHarnessReadyOutput. Because the live event listener is registered via session.client.onEvent before querying the historical events via this.workerStreamEvents, any event emitted during or shortly after subscription can be processed both by the live listener and by the history loop. This will cause chunks to be double-appended to output, corrupting the accumulated string and potentially failing the readiness check or regex matching. We should deduplicate events by tracking processed sequence numbers (seq) or event IDs.
const processedSeqs = new Set<number>()
const processedIds = new Set<string>()
const observe = (event: BrokerEvent): void => {
if (!isWorkerStreamForAgent(event, name)) return
const seq = brokerEventNumber(event, 'seq')
const id = brokerEventString(event, 'event_id') || brokerEventString(event, 'id')
if (seq !== undefined) {
if (processedSeqs.has(seq)) return
processedSeqs.add(seq)
} else if (id !== undefined) {
if (processedIds.has(id)) return
processedIds.add(id)
}
output += brokerEventChunk(event)
const failure = personaHarnessFailedFromOutput(output)
if (failure) {
finish(new Error(`Workforce persona ${personaId} failed during harness startup: ${failure}`))
return
}
if (personaHarnessReadyFromOutput(output)) {
finish()
}
}
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Implemented the PR fixes I found:
Verification run:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.ts`:
- Around line 2748-2769: The current workerStreamEvents uses a duck-typed type
assertion and runtime typeof check for queryEvents; instead, update the
BrokerSession/client typing so session.client is typed to include queryEvents
(the same surface used elsewhere like readAgentOutput and mocked in
broker.test.ts) and call session.client.queryEvents(...) directly without the
typeof check; adjust workerStreamEvents and workerStreamBaseline to rely on the
stronger compile-time type (or narrow via a proper interface) and remove the
fallback to [] so the code uses the client API consistently.
🪄 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: 23eb40fa-8222-4370-84a0-e24c25819d8f
📒 Files selected for processing (3)
src/main/broker.test.tssrc/main/broker.tssrc/main/ipc-handlers.ts
| private workerStreamEvents(session: BrokerSession, name: string, limit: number): BrokerEvent[] { | ||
| const queryEvents = (session.client as unknown as { | ||
| queryEvents?: (filter: { kind: string; name: string; limit: number }) => BrokerEvent[] | ||
| }).queryEvents | ||
| return typeof queryEvents === 'function' | ||
| ? queryEvents.call(session.client, { kind: 'worker_stream', name, limit }) | ||
| : [] | ||
| } | ||
|
|
||
| private workerStreamBaseline(session: BrokerSession, name: string): { | ||
| workerStreamBaselineSeq?: number | ||
| workerStreamBaselineCount: number | ||
| } { | ||
| const events = this.workerStreamEvents(session, name, 1000) | ||
| const seqs = events | ||
| .map((event) => brokerEventNumber(event, 'seq')) | ||
| .filter((seq): seq is number => seq !== undefined) | ||
| return { | ||
| workerStreamBaselineSeq: seqs.length > 0 ? Math.max(...seqs) : undefined, | ||
| workerStreamBaselineCount: events.length | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if queryEvents is documented or used elsewhere in the codebase
rg -n "queryEvents" --type ts -C2Repository: AgentWorkforce/pear
Length of output: 1804
Remove the “undocumented/internal API” concern for queryEvents and narrow the remaining issue
queryEvents is used directly in src/main/broker.ts (e.g., readAgentOutput calls session.client.queryEvents(...)) and is also mocked in src/main/broker.test.ts, so it’s part of the expected client surface in this codebase. The only remaining concern is that workerStreamEvents re-introduces duck-typing/type-assertion (typeof queryEvents === 'function' ? ... : []) rather than relying on the client’s typings (if they already include queryEvents).
🤖 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 2748 - 2769, The current workerStreamEvents
uses a duck-typed type assertion and runtime typeof check for queryEvents;
instead, update the BrokerSession/client typing so session.client is typed to
include queryEvents (the same surface used elsewhere like readAgentOutput and
mocked in broker.test.ts) and call session.client.queryEvents(...) directly
without the typeof check; adjust workerStreamEvents and workerStreamBaseline to
rely on the stronger compile-time type (or narrow via a proper interface) and
remove the fallback to [] so the code uses the client API consistently.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ipc-handlers.test.ts`:
- Around line 213-223: Add a failing-path test for the 'broker:spawn-persona'
handler: simulate mock.brokerManager.spawnPersona rejecting (throwing) when
invoked, call the handler (e.g., handler?.({}, 'project-1', 'autonomous-actor'))
and assert that mock.integrationEventBridge.invalidateProjectAgentCache was
still called with 'project-1' even on rejection; optionally add a second
call/replay scenario to ensure duplicate/replay behavior still triggers
invalidateProjectAgentCache. Ensure you reference the same handler key
('broker:spawn-persona'), mock.brokerManager.spawnPersona, and
mock.integrationEventBridge.invalidateProjectAgentCache when adding the
assertions.
🪄 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: b2a50c4d-b836-41c3-9e38-b3e3f13dd2be
📒 Files selected for processing (4)
src/main/broker.test.tssrc/main/broker.tssrc/main/ipc-handlers.test.tssrc/main/ipc-handlers.ts
💤 Files with no reviewable changes (1)
- src/main/ipc-handlers.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/broker.ts
- src/main/broker.test.ts
| it('invalidates the integration agent cache after persona spawn settles', async () => { | ||
| const handler = mock.handlers.get('broker:spawn-persona') | ||
| expect(handler).toBeTypeOf('function') | ||
|
|
||
| const result = await handler?.({}, 'project-1', 'autonomous-actor') | ||
|
|
||
| expect(result).toEqual({ name: 'persona', runtime: 'pty', cli: 'claude' }) | ||
| expect(mock.brokerManager.spawnPersona).toHaveBeenCalledWith('project-1', 'autonomous-actor') | ||
| expect(mock.integrationEventBridge.invalidateProjectAgentCache).toHaveBeenCalledTimes(1) | ||
| expect(mock.integrationEventBridge.invalidateProjectAgentCache).toHaveBeenCalledWith('project-1') | ||
| }) |
There was a problem hiding this comment.
Add the failure-path regression for broker:spawn-persona cache invalidation.
This suite only verifies the success path. The contract in ipc-handlers.ts is “invalidate cache after spawn settles” (success or throw), so you should also assert invalidation when spawnPersona rejects (and optionally on repeated calls to cover replay/duplicate behavior).
Suggested test addition
+ it('invalidates the integration agent cache when persona spawn fails', async () => {
+ const handler = mock.handlers.get('broker:spawn-persona')
+ expect(handler).toBeTypeOf('function')
+
+ mock.brokerManager.spawnPersona.mockRejectedValueOnce(new Error('spawn failed'))
+
+ await expect(handler?.({}, 'project-1', 'autonomous-actor')).rejects.toThrow('spawn failed')
+ expect(mock.integrationEventBridge.invalidateProjectAgentCache).toHaveBeenCalledWith('project-1')
+ })As per coding guidelines, tests touching spawned personas/integration notifications must include regression coverage beyond happy path (including duplicate/replay-style cases).
🤖 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/ipc-handlers.test.ts` around lines 213 - 223, Add a failing-path
test for the 'broker:spawn-persona' handler: simulate
mock.brokerManager.spawnPersona rejecting (throwing) when invoked, call the
handler (e.g., handler?.({}, 'project-1', 'autonomous-actor')) and assert that
mock.integrationEventBridge.invalidateProjectAgentCache was still called with
'project-1' even on rejection; optionally add a second call/replay scenario to
ensure duplicate/replay behavior still triggers invalidateProjectAgentCache.
Ensure you reference the same handler key ('broker:spawn-persona'),
mock.brokerManager.spawnPersona, and
mock.integrationEventBridge.invalidateProjectAgentCache when adding the
assertions.
Sources: Coding guidelines, Learnings
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
2 participants
Summary
Tests