fix(tooling): spawn-peer OK only when the peer's agent actually took a turn - #139
fix(tooling): spawn-peer OK only when the peer's agent actually took a turn#139heavygee wants to merge 1 commit into
Conversation
![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: 2c923ecdf6
ℹ️ 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".
| sleep 1 | ||
| COUNT=$(message_count) | ||
| ACTIVE=$(session_active) | ||
| if [[ "${COUNT:-0}" -ge 1 && ( "$ACTIVE" == "true" || "${COUNT:-0}" -ge 2 ) ]]; then |
There was a problem hiding this comment.
For the default Cursor ACP path, active does not prove that the underlying agent initialized: ApiSessionClient emits session-alive immediately when its hub socket connects (cli/src/api/apiSession.ts:328-341), while Cursor emits the distinct session-ready event only after establishing the ACP session (cli/src/cursor/cursorAcpRemoteLauncher.ts:367-388). A slow invalid-model or authentication failure can therefore accept the ping during this pre-ready window and satisfy this predicate on its first poll; if the failure finishes sooner, runCursor also persists a Cursor Agent failed event as a second message (cli/src/cursor/runCursor.ts:196-203), allowing the COUNT >= 2 branch to report success after archival. Gate on actual readiness or classify the message contents rather than accepting either transient activity or any second stored row.
Useful? React with 👍 / 👎.
| if [[ "${COUNT:-0}" -ge 1 && ( "$ACTIVE" == "true" || "${COUNT:-0}" -ge 2 ) ]]; then | ||
| break | ||
| fi | ||
| if (( SECONDS >= DEADLINE )); then |
There was a problem hiding this comment.
When the hub accepts connections but stalls these GETs, the deadline is checked only after both requests complete. Each request permits a ten-second transfer (curl --help all defines --max-time as “Maximum time allowed for transfer”), so one iteration can consume roughly 21 seconds before reaching this check; HAPI_SPAWN_PEER_VERIFY_TIMEOUT_S=3 can therefore block for about 21 seconds, and the documented 45-second default can reach about 63 seconds. Bound each request by the remaining verification budget or check the deadline between calls so automation can rely on the advertised timeout.
Useful? React with 👍 / 👎.
| err "VERIFY FAILED: session $PEER_ID took the remit but never became active" | ||
| err " and produced no agent output within ${VERIFY_TIMEOUT_S}s." | ||
| err " Most likely the agent died at startup - check --agent/--model against" | ||
| err " the machine's catalog (agent --list-models), not a CLI --help example." |
There was a problem hiding this comment.
This recovery instruction is unsafe on the common same-machine Cursor setup: the repository explicitly documents that starting agent --list-models while a Cursor ACP process is running terminates that process with SIGTERM (cli/src/agent/backends/acp/agentCliGuard.ts:17-26), and the built-in catalog implementation deliberately refuses this probe while the ACP lock is held (cli/src/modules/common/cursorModels.ts:135-143). A parent Cursor session that sees its spawned peer fail can therefore follow this advice and crash itself or another peer; for --agent values other than Cursor, the command also queries the wrong agent's models. Direct users to the hub's guarded, agent-specific machine catalog instead of the raw Cursor command.
Useful? React with 👍 / 👎.
…a turn The final check was messages>=1, but the wrapper's own remit ping is that message. A peer whose agent died at startup — a rejected --model spawns a child that never survives the agent handshake — archived with our ping as its only turn and the wrapper still printed OK. That is exactly the empty shell this wrapper exists to catch, one turn later. `active` alone would not have fixed it: the hub marks a session active when the CLI socket connects, and sessionFactory connects that socket before launching the agent, so every spawn reads active the moment we have its id. What separates a live peer from a corpse is what happens next — a dead child sends session-end or stops heartbeating, and the hub drops it inactive within ~30s. So: poll for an agent turn (a message beyond our ping, or thinking) and leave early on it; otherwise wait that window out and require the session to be still active at the end. Undelivered remit still exits 4; delivered-onto-a-corpse now exits 5 with a diagnosis pointing at the machine's model catalog rather than a CLI --help example. The OK line carries which proof it got. A non-numeric HAPI_SPAWN_PEER_VERIFY_TIMEOUT_S is now a usage error up front instead of a set -e crash after the remit has already gone out. Adds hapi-spawn-peer.test.sh, which drives the wrapper against a stub hub modelled on the real one (active from t=0 in every scenario) across live, dead, quiet, silent and bad-timeout. Red-green verified: on the pre-fix wrapper the dead-agent case exits 0. Context: upstream tiann#1752, tiann#1753 via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run>
2c923ec to
a4382a2
Compare
heavygee
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4382a26ac
ℹ️ 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".
| VERIFY_TIMEOUT_S="${HAPI_SPAWN_PEER_VERIFY_TIMEOUT_S:-60}" | ||
| [[ "$VERIFY_TIMEOUT_S" =~ ^[0-9]+$ ]] \ | ||
| || die "HAPI_SPAWN_PEER_VERIFY_TIMEOUT_S must be a whole number of seconds, got: $VERIFY_TIMEOUT_S" |
There was a problem hiding this comment.
When HAPI_SPAWN_PEER_VERIFY_TIMEOUT_S is nonnumeric, this check runs only after the script has spawned and renamed the peer and delivered its handoff. The script then exits 2 despite those side effects, so automation that retries after the apparent usage failure can create duplicate peers and duplicate work; parse and validate the timeout before the first API mutation.
Useful? React with 👍 / 👎.
1 participant
Estate-side half of upstream tiann#1752 (upstream fix: tiann#1753).
Problem
hapi-spawn-peer's final gate wasmessages>=1— but the wrapper's own remit ping is that message. A peer whose agent died at startup (a rejected--modelspawns a child that never survives the agent handshake) archived with our ping as its only turn, and the wrapper still printed:That is the empty shell the wrapper exists to catch, one turn later. Two peers were handed remits this way (
b853fbd9…,52e454f0…) with an OK on both.Why
activealone is not the fixThe first cut of this gated on
session.active, and cold review killed it: the hub marks a session active when the CLI socket connects, andsessionFactoryconnects that socket before launching the agent. So by the timePOST /spawnhas returned us a sessionId,activeis already true — for healthy and doomed spawns alike.What separates them is what happens next. A child that dies at agent handshake either sends
session-endor stops heartbeating, and the hub drops it inactive within ~30s (sessionCache.expireInactive).Fix
Poll for an agent turn — a message beyond our ping, or
thinking— and leave early on it. Otherwise wait that window out and require the session to be still active at the end.agent --list-models) rather than a CLI--helpexampleproof=agent-turnorproof=still-active-after-NsHAPI_SPAWN_PEER_VERIFY_TIMEOUT_Sis rejected up front, instead of crashing underset -eafter the remit has already gone outDefault wait is 60s; a healthy peer leaves early on its first turn.
Tests
scripts/tooling/hapi-spawn-peer.test.shdrives the real wrapper against a stub hub modelled on the real one — active from t=0 in every scenario — across live / dead / quiet / silent / bad-timeout:Red-green verified: with
origin/main's wrapper restored, the dead-agent case exits 0 instead of 5.🤖 Generated with Claude Code