Software factory change - #1843
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if ((targetNode || useSandbox) && options.confirm !== false && confirmTimeoutMs < 90_000) { | ||
| throw new Error('--confirm-timeout must be at least 90000ms for verified targeted spawns.'); |
There was a problem hiding this comment.
🟡 Minimum confirmation timeout expires early
A --confirm-timeout of 90000 passes validation but expires before the broker's 90-second readiness window can finish. The broker starts its deadline after launch setup, while SDK confirmation starts after dispatch acceptance. Late readiness becomes spawn_unconfirmed despite arriving within the broker window.
Learn more
The CLI's confirmation budget covers the whole period after dispatch acceptance. The broker's VERIFIED_SPAWN_READY_TIMEOUT starts later, after spawn_worker_from_request completes its launch and registration work in handle_fleet_action_spawn. Therefore equal 90-second budgets do not nest: every millisecond spent launching reduces the client time available for the broker's full readiness window.
Example: A caller passes --confirm-timeout 90000. Launch setup takes two seconds, then the harness proves readiness 89 seconds into the broker window. The broker accepts it after 91 seconds overall, but the SDK stops polling at 90 seconds and reports spawn_unconfirmed.
Recommended fix: Require a confirmation timeout strictly larger than the broker window with explicit transport and launch margin, or reduce the broker window so the documented 90000ms minimum safely contains it. Keep the CLI help, changelog, and validation aligned with the resulting minimum.
Was this helpful? React with 👍 or 👎 to provide feedback.
…as asked Review of #1843 found the new readiness contract was requested and judged by three predicates that disagreed. This resolves it once, per request. - `placement.spawn` derived `verify_ready` from `confirm !== false`, so the documented default (`confirm` omitted) asked the broker to hold the action open and release the worker at 90s while the caller returned immediately — killing a worker that would otherwise have survived. Gate it on `confirm === true`, matching the two predicates that consume it. - The confirmation poll demanded `ready:true` for every `spawn:*` invocation regardless of the mode requested, so `verifyReady: false` failed a healthy launch with the issue's own error string. Thread the resolved mode through and judge the requested contract on both the ack and poll paths. Persona stays pinned to proven readiness: its engine-owned handler never reads `verify_ready` but does report readiness itself. - `state: 'ready'` is now reserved for a confirmation that actually verified readiness; a launch-only confirmation reports `confirmed` + `accepted`. - The "did not honour verify_ready" message no longer fires on the path that never sent `verify_ready`; there it names the missing `ready` boolean. - `--confirm-timeout` floor raised to 95000ms. The broker's 90s readiness window starts after launch, registration and token minting, so equal budgets do not nest and 90000 reported a released worker as `spawn_unconfirmed`. The floor check now runs after the numeric guard so `-5` reports what is actually wrong with it. - The readiness-timeout failure result bypasses `send_fleet_action_result`; log its correlation fields so the most-investigated outcome is traceable. - Drop the unreachable `verifyReady` parameter on `spawnLifecycleState`, use the suite's binary-resolution helpers in the broker integration test, remove the SDK placement arms that CI never runs, and drop the committed `summary.md` workflow artifact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Already addressed — this comment was posted at 18:53Z against 69a060c, and 7c2546f (19:13Z) raised the floor before it could be actioned.
A
On keeping help, changelog and validation aligned: the help string interpolates the same constant ( |
1 participant
Targeted fleet spawns now request the broker's verified-readiness contract, so a healthy worker can complete confirmation instead of being rejected for missing
ready:true. Unverified broker success explicitly returnsready:false;--no-confirmaccepts that launch evidence while continuing to reject obsolete handlers that omit readiness entirely. Persona defaults and MCP readiness requirements remain unchanged.The CLI rejects confirmed targeted-spawn timeouts below the broker's 90-second readiness window. Readiness timeout errors explain that the worker was released. Invocation-correlated spawn timing and dropped-result diagnostics help investigate the separate
spawn_unconfirmedreport. The changelog is raised to[Unreleased - Minor]as specified by the reviewed plan.Validation:
env -u RELAY_BASE_URL npm --ignore-scripts test, after building packages). Two ambient-URL failures disappeared with that environment override removed. The final additional completed-ack CLI regression passed in the 74-test fleet command suite.env -u GIT_CONFIG_COUNT cargo test -p agent-relay-broker). The environment's forcedcore.hooksPath=/dev/nullcaused four hook-test failures before its removal.npm run typecheckpassed.npm run lintpassed with 108 existing warnings.node --test dist/fleet-spawn-readiness.test.jspassed in approximately 1.1 seconds. A loopback engine forwards the SDK invocation to the real broker, which registers exactly one worker, launches a Claude stub in a real PTY, and returns{spawned:true, ready:true}through node control and requester polling.payload.verify_ready = truefrom the built SDK made that same real-broker test fail withspawn_failed; restoring it passed. Removingready:falsefrom the Rust unverified result madespawn_success_always_declares_readinessfail; restoring it passed. The requester regression also failed against the original source before implementation.npm run test:integration:brokerrun encountered continuity/event failures and stalled; it was stopped. The isolated readiness integration above passes. This is not a claim that the full broker integration suite passed.Remaining validation and follow-up:
Note
Medium Risk
Changes fleet spawn placement, broker worker lifecycle (90s verified readiness release), and confirmation semantics; broad test coverage but affects production dispatch paths.
Overview
Targeted fleet spawns now use a consistent launch vs. harness-readiness contract end to end, fixing false
spawn_failed/spawn_unconfirmedoutcomes when workers were healthy.The SDK only sends
verify_readywhen the caller waits for confirmation (confirm: true), so the default dispatch no longer arms the broker’s ~90s readiness release while the client walks away. Confirmation checks the contract that was actually requested: verified spawns requirespawned:trueandready:true;verifyReady: falseacceptsready:false. Placementstate: 'ready'is reserved for proven readiness; launch-only confirmation staysaccepted.spawn:personakeeps engine-owned behavior (noverify_readyon the wire, readiness still required when confirming).The broker always includes an explicit
readyboolean on spawn success (falsefor unverified launch,truewhen readiness is proven), improves readiness-timeout messaging, adds invocation-correlated spawn logs, and warns when fleetaction.resultframes are dropped on disconnect or send failure (replay deferred).CLI
fleet spawnenforces a 95000ms floor on--confirm-timeoutfor verified targeted/sandbox spawns so the client budget nests inside the broker window. Changelog bumped to Unreleased – Minor.Reviewed by Cursor Bugbot for commit 7c2546f. Bugbot is set up for automated code reviews on this repo. Configure here.