fix(broker): deliver spawn brief only after real prompt readiness - #1470
Conversation
|
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe broker preserves queued PTY work until a verified prompt is available. It emits ChangesPTY readiness and spawn validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FleetE2E
participant PTYWorker
participant RuntimeEvents
participant StubAgent
participant ObservationFile
FleetE2E->>PTYWorker: Spawn agent with nonce-bearing brief
PTYWorker->>StubAgent: Start PTY and retain queued brief
PTYWorker->>RuntimeEvents: Emit harness_started with child PID
RuntimeEvents->>RuntimeEvents: Record PTY liveness
StubAgent-->>PTYWorker: Emit ->pty:ready after delayed startup
PTYWorker->>RuntimeEvents: Emit worker_ready
PTYWorker->>StubAgent: Deliver queued brief
StubAgent->>ObservationFile: Record nonce and node metadata
FleetE2E->>ObservationFile: Validate observation
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![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 `@tests/e2e/fleet/README.md`:
- Line 24: Update the E2E duration documentation in the matrix timing entry near
the “spawn completes E2E” scenario to reflect that the five serial spawns
require at least about 81 seconds before overhead, replacing the outdated
approximately 30-second estimate.
🪄 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: db2bab2b-a9a8-4346-8b93-599825e460d5
📒 Files selected for processing (8)
CHANGELOG.mdcrates/broker/src/pty_worker.rscrates/broker/src/worker.rstests/e2e/fleet/README.mdtests/e2e/fleet/fleet-e2e.test.tstests/e2e/fleet/nodes/node-a.tstests/e2e/fleet/nodes/node-b.tstests/e2e/fleet/nodes/stub-agent.cjs
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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 `@crates/broker/src/runtime/worker_events.rs`:
- Around line 37-43: Update protocol_pid to reject pid values of zero before
returning the converted PID, while continuing to accept valid nonzero u32
values. Add or update the protocol PID tests to cover zero and ensure malformed
zero-PID frames do not suppress the never-ready deadline.
🪄 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: 62e31292-954f-4e97-8878-777c4a72ca6d
📒 Files selected for processing (4)
crates/broker/src/pty_worker.rscrates/broker/src/runtime/worker_events.rscrates/broker/src/worker.rstests/e2e/fleet/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/fleet/README.md
- crates/broker/src/worker.rs
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
1 participant
Fixes #1469.
What changed
Root cause
The PTY wrapper emitted
worker_readyafter 25 seconds even when prompt detection had not succeeded. The broker then removed the sole queued initial task and wrote it to a still-booting TUI, where it could be consumed without action. The agent remained registered and heartbeating, so the previous E2E passed despite the lost brief.The corrected flow emits
harness_startedwith the child pid as soon as the PTY child is known, then emitsworker_readyonly after actual harness readiness. Liveness prevents a healthy slow or unrecognized prompt from being reaped, but onlyworker_readyreleases the initial task. A slow harness gets one warning while its queued work remains intact. Worker events carry a unique wrapper-generation UUID and are ignored unless that generation is still current, preventing delayed frames from an older same-name process from affecting the replacement.Real-harness validation also exposed Codex 0.144.5's directory-trust menu as a pre-prompt startup gate. The existing Claude and Gemini trust responders had no Codex equivalent. The final implementation detects the complete Codex menu in either accumulated raw output or the rendered terminal grid, accepts the pre-selected
Yes, continueonce, and still waits for the true input prompt before releasing the brief.Regression and negative control
The unchanged final regression test was run against unmodified
origin/main(7a42f3bd):1after 47.60swaitFor timed out (worker-a acted on nonce-bearing brief); last=nullAgainst this branch:
0, 5/5 acted, 89.07s0, 14/14 passed, 125.08sTimestamped isolated E2E observations from a five-spawn run (no follow-up delivery of any kind):
worker-anode-aspawn-brief-1-mslmhzxf2026-08-09T09:52:03.690Zworker-b1node-bspawn-brief-2-mslmil1m2026-08-09T09:52:04.282Zworker-a2node-aspawn-brief-3-mslmilov2026-08-09T09:52:31.899Zworker-b2node-bspawn-brief-4-mslmj6ta2026-08-09T09:52:32.483Zworker-a3node-aspawn-brief-5-mslmj75n2026-08-09T09:52:59.723ZThe approximately 27-second spacing across all three node-a observations independently crosses the historical 25-second race boundary.
Real product validation
A second real broker was built from this branch, started on its own port, state directory, and throwaway workspace, and read back live through
/healthbefore spawning:relay-1469-live-final1469-fixed-8e6ca134581492121776752601251842.1.153, Codex0.144.5Five consecutive real PTY spawns acted on their nonce-bearing initial briefs with zero follow-up input, DM, or message:
sha-codex-1finalsha-1469-codex-1-8e6ca1342026-08-09T11:59:18Zsha-claude-2finalsha-1469-claude-2-8e6ca1342026-08-09T12:00:46Zsha-codex-3finalsha-1469-codex-3-8e6ca1342026-08-09T12:01:27Zsha-claude-4finalsha-1469-claude-4-8e6ca1342026-08-09T12:02:45Zsha-codex-5finalsha-1469-codex-5-8e6ca1342026-08-09T12:03:24ZThe first real Codex attempt before the trust-menu fix was an explicit failed control: no nonce after 180 seconds (observer exit
1), and a read-only PTY snapshot showed it parked onDo you trust the contents of this directory?. A raw-stream-only responder also failed because Codex draws the menu with cursor motion; that observation led to the terminal-grid check in32229bed. The final five-spawn run started from zero after rebuilding and restarting only this isolated broker.Other validation
cargo test -p relay-pty: exit0(222 passed; doc test ignored)cargo test -p agent-relay-broker: exit0(864 passed, 4 ignored; integration tests also passed)cargo clippy -p agent-relay-broker -p relay-pty --lib -- -D warnings: exit0npm run typecheck: exit0git diff --check, and staged secret-pattern scan: exit0All validation used isolated E2E processes and ports. The resident broker at pid
99572was not changed, restarted, or reconfigured. The isolated real-harness agents were released and their broker stopped after evidence capture.Related
injectionModereportThis is separate from the readiness race. The current MCP
send_dmschema declaresmode: "wait" | "steer"and forwards that field correctly. A camelCaseinjectionModeinput is not declared, so schema parsing strips it and the request defaults towait; the returned record therefore appears to silently ignore the caller. No compatibility-alias change is included here so the spawn-readiness fix stays isolated.