feat: verify AgentWorkforce persona spawns - #1464
Open
khaliqgant wants to merge 7 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThe MCP ChangesPersona spawning and readiness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant Messaging
participant Broker
participant Worker
MCPClient->>Messaging: invoke spawn:persona
Messaging->>Broker: create verified spawn
Broker->>Worker: start persona worker
Worker->>Broker: report registration and worker_ready
Broker-->>Messaging: return completed invocation
Messaging-->>MCPClient: return readiness result
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
khaliqgant
marked this pull request as ready for review
August 8, 2026 11:12
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![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: 8696810e9f
ℹ️ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/cli/src/cli/agent-relay-mcp.ts (1)
771-807: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the spawn execution branches.
The async handler has complexity 22. The configured limit is 15. Extract the raw CLI path and persona path into dedicated helpers. Keep validation and action-input construction in small helpers.
🤖 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 `@packages/cli/src/cli/agent-relay-mcp.ts` around lines 771 - 807, Reduce the complexity of the spawn handler by extracting the non-persona CLI invocation and persona invocation into dedicated helper functions. Keep the existing validation checks in the handler, and move actionInput construction into a small helper so the handler only validates inputs, builds the request, and dispatches through the appropriate helper while preserving current behavior.Source: Linters/SAST tools
🤖 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 `@crates/broker/src/runtime/fleet.rs`:
- Around line 533-542: Update crates/broker/src/runtime/fleet.rs lines 533-542
so the deregistration result from deregister_fleet_agent is handled: only prune
local Fleet state after AgentDeregister is successfully queued, while
channel-full or closed errors retain the FleetDeliveryBook entry and schedule
retryable cleanup. Apply the same flow in
crates/broker/src/runtime/maintenance.rs lines 120-129 before removing timed-out
worker Fleet state; both sites must preserve state until deregistration
succeeds.
In `@packages/cli/src/cli/agent-relay-mcp.ts`:
- Around line 83-107: Update waitForPersonaSpawn to catch failures from
actions.getInvocation and retry transient network or authorization read errors
until deadline, preserving the existing polling interval. Fail immediately for
confirmed terminal invocation failures and non-retryable errors such as
invalid-agent-token responses, while retaining the current timeout and success
handling.
---
Nitpick comments:
In `@packages/cli/src/cli/agent-relay-mcp.ts`:
- Around line 771-807: Reduce the complexity of the spawn handler by extracting
the non-persona CLI invocation and persona invocation into dedicated helper
functions. Keep the existing validation checks in the handler, and move
actionInput construction into a small helper so the handler only validates
inputs, builds the request, and dispatches through the appropriate helper while
preserving current behavior.
🪄 Autofix
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: 3ba52309-4515-4d5c-89aa-c11c1931f570
📒 Files selected for processing (16)
CHANGELOG.mdcrates/broker/src/protocol.rscrates/broker/src/runtime/event_loop.rscrates/broker/src/runtime/fleet.rscrates/broker/src/runtime/init.rscrates/broker/src/runtime/maintenance.rscrates/broker/src/runtime/relaycast_events.rscrates/broker/src/runtime/worker_events.rspackages/cli/src/cli/agent-relay-mcp.startup.test.tspackages/cli/src/cli/agent-relay-mcp.tspackages/fleet/README.mdpackages/fleet/src/index.tspackages/fleet/src/serve-node.test.tspackages/fleet/src/serve-node.tspackages/harness-driver/src/agent-handle.tspackages/harness-driver/src/agent-result.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 16 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
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 `@packages/harness-driver/src/agent-handle.ts`:
- Line 75: Scope SpawnedAgentHandle event lookups to the current spawn
generation instead of relying only on the reused agent name or the Date.now()
readyNotBefore boundary. Update the handle’s event-tracking state and the logic
around waitForReady/exit (including the queryEvents usage at lines 146-148) to
establish a reliable per-spawn boundary, such as the relevant worker_ready event
or broker-provided handle ID, so in-flight events are found and prior workers’
worker_ready/agent_exited events cannot satisfy the current handle.
🪄 Autofix
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: 024722d2-17b5-4dd0-9601-ec602c035898
📒 Files selected for processing (7)
crates/broker/src/runtime/fleet.rscrates/broker/src/runtime/maintenance.rscrates/broker/src/runtime/relaycast_events.rscrates/broker/src/runtime/worker_events.rspackages/cli/src/cli/agent-relay-mcp.startup.test.tspackages/cli/src/cli/agent-relay-mcp.tspackages/harness-driver/src/agent-handle.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/cli/src/cli/agent-relay-mcp.startup.test.ts
- crates/broker/src/runtime/maintenance.rs
- packages/cli/src/cli/agent-relay-mcp.ts
- crates/broker/src/runtime/worker_events.rs
- crates/broker/src/runtime/relaycast_events.rs
- crates/broker/src/runtime/fleet.rs
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/broker/src/runtime/fleet.rs (1)
626-632: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the pending spawn only after a successful release.
If
release_worker_locally()returnsReleaseOutcome::Failed, this code removes the verified spawn and sendsspawn_released_before_ready. A later validworker_readycannot complete the original spawn action. Move this block into theReleaseOutcome::Releasedpath.Proposed fix
- if let Some(pending) = self.pending_verified_spawns.remove(&name) { - self.send_fleet_action_result(verified_spawn_failed_result( - pending.invocation_id, - "spawn_released_before_ready", - )) - .await; + if outcome == super::relaycast_events::ReleaseOutcome::Released { + if let Some(pending) = self.pending_verified_spawns.remove(&name) { + self.send_fleet_action_result(verified_spawn_failed_result( + pending.invocation_id, + "spawn_released_before_ready", + )) + .await; + } }🤖 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 `@crates/broker/src/runtime/fleet.rs` around lines 626 - 632, Move the pending_verified_spawns removal and verified_spawn_failed_result notification out of the unconditional release handling and into the ReleaseOutcome::Released branch of release_worker_locally(). Preserve the pending spawn and avoid sending spawn_released_before_ready when the release outcome is ReleaseOutcome::Failed, so a later worker_ready can complete the original action.
🤖 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 `@crates/broker/src/runtime/worker_events.rs`:
- Around line 751-759: Bind lifecycle handling to an immutable broker-assigned
worker generation: in crates/broker/src/runtime/worker_events.rs:751-759, store
the expected generation in PendingVerifiedSpawn and complete only matching
worker_ready events; in packages/harness-driver/src/client.ts:581-588 and
:624-631, obtain that generation atomically for PTY, CLI, and headless spawns
and pass it through; in packages/harness-driver/src/agent-handle.ts:276-286,
filter history and live lifecycle events by generation rather than name and
temporal cutoff. Add a regression test covering an old worker event arriving
between release and same-name respawn completion.
---
Outside diff comments:
In `@crates/broker/src/runtime/fleet.rs`:
- Around line 626-632: Move the pending_verified_spawns removal and
verified_spawn_failed_result notification out of the unconditional release
handling and into the ReleaseOutcome::Released branch of
release_worker_locally(). Preserve the pending spawn and avoid sending
spawn_released_before_ready when the release outcome is ReleaseOutcome::Failed,
so a later worker_ready can complete the original action.
🪄 Autofix
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: 9eb5f52b-d809-41a2-90bd-a3d3e6959015
📒 Files selected for processing (9)
crates/broker/src/runtime/fleet.rscrates/broker/src/runtime/maintenance.rscrates/broker/src/runtime/relaycast_events.rscrates/broker/src/runtime/worker_events.rspackages/cli/src/cli/agent-relay-mcp.startup.test.tspackages/cli/src/cli/agent-relay-mcp.tspackages/harness-driver/src/agent-handle.tspackages/harness-driver/src/agent-result.test.tspackages/harness-driver/src/client.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/cli/src/cli/agent-relay-mcp.startup.test.ts
- crates/broker/src/runtime/relaycast_events.rs
- crates/broker/src/runtime/maintenance.rs
- packages/harness-driver/src/agent-result.test.ts
- packages/cli/src/cli/agent-relay-mcp.ts
Comment on lines
+751
to
+759
| if let Some(pending) = pending_verified_spawns.remove(&name) { | ||
| let _ = fleet_control_tx | ||
| .send(FleetControlCommand::Send( | ||
| crate::fleet_wire::BrokerToRelaycast::ActionResult( | ||
| verified_spawn_ready_result(pending.invocation_id, &name), | ||
| ), | ||
| )) | ||
| .await; | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Bind lifecycle events to an immutable spawn generation.
A cursor captured before /api/spawn excludes only events that already exist. It accepts an old same-name event emitted after the cursor read and before the new spawn is established. The broker also completes a verified spawn from any worker_ready with the same name. A release followed by a fast respawn can therefore mark the new action ready from an old worker event, or resolve the new handle from an old exit.
crates/broker/src/runtime/worker_events.rs#L751-L759: Store the expected worker generation inPendingVerifiedSpawn. Complete it only when the readiness event has that generation.packages/harness-driver/src/client.ts#L581-L588: Obtain a broker-assigned generation identifier atomically with the PTY spawn result.packages/harness-driver/src/client.ts#L624-L631: Obtain and pass the same identifier for CLI and headless spawns.packages/harness-driver/src/agent-handle.ts#L276-L286: Filter lifecycle history and live events by that identifier instead of a name plus temporal cutoff.
Add a regression test that queues an old worker's lifecycle event after release and before a same-name respawn completes.
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 \
'enum WorkerEvent|struct WorkerEvent|WorkerEvent::Message|spawn_worker_from_request|release_worker_locally|pending_verified_spawns|worker_ready' \
crates/broker/src packages/harness-driver/src📍 Affects 3 files
crates/broker/src/runtime/worker_events.rs#L751-L759(this comment)packages/harness-driver/src/client.ts#L581-L588packages/harness-driver/src/client.ts#L624-L631packages/harness-driver/src/agent-handle.ts#L276-L286
🤖 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 `@crates/broker/src/runtime/worker_events.rs` around lines 751 - 759, Bind
lifecycle handling to an immutable broker-assigned worker generation: in
crates/broker/src/runtime/worker_events.rs:751-759, store the expected
generation in PendingVerifiedSpawn and complete only matching worker_ready
events; in packages/harness-driver/src/client.ts:581-588 and :624-631, obtain
that generation atomically for PTY, CLI, and headless spawns and pass it
through; in packages/harness-driver/src/agent-handle.ts:276-286, filter history
and live lifecycle events by generation rather than name and temporal cutoff.
Add a regression test covering an old worker event arriving between release and
same-name respawn completion.
Contributor
There was a problem hiding this comment.
1 issue found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/broker/src/runtime/fleet.rs">
<violation number="1" location="crates/broker/src/runtime/fleet.rs:615">
P1: Fleet control receives `released: true` when `agent.deregister` could not be queued, so it has no reason to retry and the retained authoritative identity can remain registered indefinitely. Preserve the identity as here, but return an action error when deregistration enqueue fails.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| ) | ||
| .await; | ||
| } | ||
| Err(error) => { |
Contributor
There was a problem hiding this comment.
P1: Fleet control receives released: true when agent.deregister could not be queued, so it has no reason to retry and the retained authoritative identity can remain registered indefinitely. Preserve the identity as here, but return an action error when deregistration enqueue fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/broker/src/runtime/fleet.rs, line 615:
<comment>Fleet control receives `released: true` when `agent.deregister` could not be queued, so it has no reason to retry and the retained authoritative identity can remain registered indefinitely. Preserve the identity as here, but return an action error when deregistration enqueue fails.</comment>
<file context>
@@ -595,13 +595,34 @@ impl BrokerRuntime {
+ )
+ .await;
+ }
+ Err(error) => {
+ tracing::warn!(worker = %name, %error, "retaining fleet identity after release cleanup");
+ prune_fleet_inventory_entry(
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
spawnMCP tool accept an AgentWorkforcepersonaid or JSON path instead of a raw CLI and wait for the terminal capability resultspawn:personaactions on their dynamically resolved CLI rather than rewriting them as nativepersonaharness shadows, with a runtime feature marker for dependent SDKsworker_ready, with duplicate-name rejection, a 90-second deadline, and release/deregistration on failureSpawnedAgentHandle.waitForReady()and document the AgentWorkforce fleet-node pathThis is the Relay part of AgentWorkforce/workforce#306. It uses the in-process Fleet/SDK path and does not invoke the
agentworkforcebinary.Verification
cargo test -p agent-relay-broker --lib— 860 passed, 4 ignoredcargo fmt --allandcargo check -p agent-relay-broker— exit 0Stack
Publish this as Agent Relay 11.5+ before the dependent Workforce local-surface PR is released.