fix: enforce spawn readiness and submit verification contracts - #345
Conversation
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_2461ecb2-633a-4afd-a633-c600b627bcb8) |
📝 WalkthroughWalkthroughThe change adds structured submit-verification reasons and retry-safety metadata. It also adds configurable boot-prompt timeouts across spawn and worktree flows, preserves launch identity, and expands reliability and timeout tests. ChangesAgent reliability
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SpawnTool
participant AgentEngine
participant ShellReadiness
participant AgentSurface
SpawnTool->>AgentEngine: spawn with boot_prompt_timeout_ms
AgentEngine->>ShellReadiness: wait for shell readiness
ShellReadiness-->>AgentEngine: readiness result
AgentEngine->>AgentSurface: dispatch launch command with timeout
AgentSurface-->>AgentEngine: launch result
AgentEngine-->>SpawnTool: AgentLaunchError with launch context
sequenceDiagram
participant SendTool
participant AgentComposer
participant ScreenVerification
SendTool->>AgentComposer: dispatch input
AgentComposer-->>ScreenVerification: updated screen state
ScreenVerification-->>SendTool: positive evidence or failure reason
SendTool-->>SendTool: record retry-safe status
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
@codex review @coderabbitai review @greptileai review |
|
✅ Action performedReview finished.
|
Independent Claude pair-review — head
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![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: 623ff3156b
ℹ️ 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/server.ts`:
- Around line 3682-3720: Extract the final submit-verification classification
from the surrounding verification function into a pure named helper such as
classifySubmitVerificationOutcome. Pass the five existing flags
(require_working_status, lastHasPendingInput, lastRetryEligiblePendingInput,
sawReadableScreen, and sawBlankScreen), preserve the current fail-closed
priority and return shape, and replace the nested ternaries with the helper
result so submit_verified and submit_verification_reason remain unchanged.
- Around line 8395-8396: Update the timeout handling in spawn_agent and
new_worktree_split so boot_prompt_timeout_ms continues to apply only to
deliverBootPrompt; keep waitForShellReady and waitForAgentLaunchReady on their
established independent defaults. Revise the option description near the timeout
schema to document the separate phase-specific behavior and avoid claiming one
override controls all readiness phases.
In `@tests/server-agent-tools.test.ts`:
- Around line 3433-3442: Update the readiness-timeout test around spawn.handler
to parse its arguments through spawn.inputSchema before invoking the handler,
ensuring boot_prompt_timeout_ms is validated by the registered schema. Also
include boot_prompt_timeout_ms in the relevant default-timeout test input so
this test group verifies schema acceptance rather than bypassing validation.
- Around line 3518-3526: Update the state lookup in the test assertion to match
the record by parsed.agent_id rather than surface_id, using the existing
stateMgr.listStates() result and preserving the subsequent error-state
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: ASSERTIVE
Plan: Pro Plus
Run ID: 7a054c23-0349-4246-b652-13d613e2eb14
📒 Files selected for processing (6)
src/agent-engine.tssrc/server.tstests/enter-reliability.test.tstests/false-green-empty-surface.test.tstests/server-agent-tools.test.tstests/server.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Macroscope - Correctness Check
- GitHub Check: test
🧰 Additional context used
📓 Path-based instructions (5)
tests/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.test.ts: Mirror source layout in tests (src/foo.ts->tests/foo.test.ts).
Do not add integration tests that require a running cmux instance; tests should be fully mocked.
Files:
tests/false-green-empty-surface.test.tstests/enter-reliability.test.tstests/server.test.tstests/server-agent-tools.test.ts
tests/server.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Server tests should mock the cmux client via
createServer({ exec, skipAgentLifecycle }).
Files:
tests/server.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.ts: Build the project with TypeScript (tsc) and keep source code compatible with Node 20+ and Zod-based typing.
Use theok(data)anderr(error)helpers for consistent MCP tool responses.
All MCP tool handlers must return{ content: TextContent[], structuredContent?, isError? }.
Files:
src/agent-engine.tssrc/server.ts
src/agent-engine.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Implement agent lifecycle behavior in
agent-engine.ts, including spawning, monitoring, and quality tracking.
Files:
src/agent-engine.ts
src/server.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Register all MCP tools in
server.ts, including the 33 tool handlers, and conditionally skip agent-lifecycle tools whenskipAgentLifecycle: true.
Files:
src/server.ts
🧠 Learnings (1)
📚 Learning: 2026-03-15T10:42:35.917Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 1
File: tests/quality-tracking.test.ts:171-200
Timestamp: 2026-03-15T10:42:35.917Z
Learning: In tests/quality-tracking.test.ts for the cmuxlayer project, ensure that at or above 80% context quality degradation, behavior depends on depth: depth-0 agents receive a /compact command; depth > 0 agents are killed and logged (kill + log). Respawn of non-root agents is out of scope for v1. Treat the design doc quality tracking section as the authoritative source for this behavior, and align test expectations accordingly.
Applied to files:
tests/false-green-empty-surface.test.tstests/enter-reliability.test.tstests/server.test.tstests/server-agent-tools.test.ts
🔇 Additional comments (26)
src/agent-engine.ts (5)
150-150: LGTM!Also applies to: 320-320
178-190: LGTM!
1861-1861: LGTM!Also applies to: 1897-1897
4752-4752: LGTM!
4771-4777: 🗄️ Data Integrity & IntegrationNo change needed
spawnAgentcallers handle launch failures through genericAgentLaunchErrorpaths, and placement binding failures are rethrown before launch error wrapping.> Likely an incorrect or invalid review comment.tests/server-agent-tools.test.ts (5)
82-82: LGTM!Also applies to: 211-211
3457-3485: LGTM!
3532-3594: LGTM!
3669-3718: LGTM!
3609-3611: 🎯 Functional CorrectnessNo change needed.
c-c,ctrl-c,ctrl+c, and^care normalized toctrl-c, so this guard matches the normalized interrupt key used by the relaunch path.src/server.ts (10)
42-42: LGTM!Also applies to: 473-495, 514-540, 591-591
761-793: LGTM!
2900-2901: LGTM!
3534-3565: LGTM!Also applies to: 3581-3597, 3615-3619
3778-3814: LGTM!Also applies to: 3842-3842
4311-4442: LGTM!Also applies to: 4624-4624, 4826-4827
7863-7873: LGTM!Also applies to: 8027-8068
8586-8625: LGTM!Also applies to: 8835-8837, 8899-8935
9814-9819: LGTM!Also applies to: 10605-10605
8778-8804: 🗄️ Data Integrity & IntegrationNo change needed for
SubmitVerificationErrorpropagation.Launch submit verification is decoded from launcher response/state, not returned as
AgentLaunchError.launch_cause;agent-engine.tsthrows unknown launch errors untyped intolaunch_cause, including non-Errorvalues in the current path.> Likely an incorrect or invalid review comment.tests/enter-reliability.test.ts (2)
353-390: Fake clients and fail-closed coverage matchverifySubmitAfterEnter's classification.
FakeTransientVerificationReadClientandFakeUnavailableVerificationScreenClientcorrectly simulate a single transient read failure versus a permanently unavailable/blank screen. The assertions onsubmit_verification_reason(surface_read_unavailablevssurface_screen_empty) andretry_safe: falseline up with the priority order implemented inverifySubmitAfterEnterinsrc/server.ts(pending input, then unreadable screen, then working-status, then absent evidence). Both fakes use fully mocked clients with no real cmux instance.As per path instructions for
tests/**/*.test.ts: "Do not add integration tests that require a running cmux instance; tests should be fully mocked."Also applies to: 635-699
502-503: LGTM!Also applies to: 575-575, 592-593, 717-718, 733-733, 749-750, 765-765, 782-783
tests/false-green-empty-surface.test.ts (1)
150-150: LGTM!Also applies to: 210-213
tests/server.test.ts (3)
2848-2899: LGTM!
2900-2979: LGTM!As per path instructions for
tests/server.test.ts: "Server tests should mock the cmux client viacreateServer({ exec, skipAgentLifecycle })." This test follows that pattern at Line 2940-2944.
9354-9356: LGTM!
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_98394e99-e7f0-479f-b72a-ed678ec1645e) |
|
Remote-review delta pushed at @codex review @coderabbitai review @greptileai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bac6419a1
ℹ️ 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".
9bac641 to
276ccb8
Compare
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_f7669041-8845-44f3-bbe6-874064a618dd) |
|
Rebased onto merged #346 at @codex review @coderabbitai review @greptileai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR metadata =='
gh pr view 345 --json number,headRefOid,baseRefOid,baseRefName,headRefName,title,url
echo '== Merge base and changed files =='
base="$(git merge-base HEAD origin/HEAD 2>/dev/null || true)"
if [ -z "$base" ]; then
base="$(git merge-base HEAD "$(git rev-parse HEAD^)" 2>/dev/null || true)"
fi
printf 'merge-base: %s\n' "$base"
git diff --stat "$base"...HEAD
git diff --name-status "$base"...HEAD
echo '== Source structure =='
ast-grep outline src/agent-engine.ts --items all --view expanded || true
ast-grep outline src/server.ts --items all --view expanded || true
echo '== Contract-related occurrences =='
rg -n -C 4 --glob 'src/**/*.ts' --glob 'tests/**/*.ts' \
'boot_prompt_timeout_ms|AgentLaunchError|SubmitVerificationFailureReason|submit_verification_reason|retry_safe|focus' \
src testsLength of output: 145719 🏁 Script executed: #!/bin/bash
set -euo pipefail
pr_json="$(gh pr view 345 --json baseRefOid,headRefOid,files)"
base="$(printf '%s' "$pr_json" | jq -r '.baseRefOid')"
head="$(printf '%s' "$pr_json" | jq -r '.headRefOid')"
echo "base=$base"
echo "head=$head"
echo '== changed files =='
printf '%s' "$pr_json" | jq -r '.files[] | "\(.path)\t+\(.additions)/-\(.deletions)"'
echo '== exact source diffs =='
git diff --unified=60 "$base" "$head" -- src/agent-engine.ts src/server.ts
echo '== agent engine contract declarations and spawn implementation =='
rg -n -C 12 \
'export class AgentLaunchError|export interface SpawnAgentParams|launchCommandSender|async spawn|spawn\(' \
src/agent-engine.ts
echo '== server contract helpers and propagation =='
rg -n -C 18 \
'type SubmitVerificationFailureReason|interface DeliveryRecord|function verify|const verify|submit_verification_reason|retry_safe|AgentLaunchError|boot_prompt_timeout_ms' \
src/server.ts
echo '== focus lifecycle helper and callers only =='
rg -n -C 16 \
'capture.*focus|restore.*focus|Focus.*Snapshot|focus.*restore|restore.*prior|with.*focus|focusSurface\(' \
src/server.ts
echo '== relevant test diff =='
git diff --unified=35 "$base" "$head" -- \
tests/server-agent-tools.test.ts \
tests/enter-reliability.test.ts \
tests/false-green-empty-surface.test.ts \
tests/server.test.tsLength of output: 246181 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== all submit-verification error handling =='
rg -n -C 10 'instanceof SubmitVerificationError|new SubmitVerificationError|submitVerificationFailurePayload' src/server.ts
echo '== delivery entry points and their error handling =='
rg -n -C 12 'const deliverAgentInput|server\.tool\(\s*"send_input"|server\.tool\(\s*"send_command"|server\.tool\(\s*"send_to"|startBackgroundDelivery|server\.tool\(\s*"broadcast"' src/server.ts
echo '== spawn boot-prompt failure and worktree failure ranges =='
sed -n '8750,8905p' src/server.ts
sed -n '9095,9245p' src/server.ts
echo '== focus lease implementation =='
sed -n '4760,4890p' src/server.ts
echo '== readiness call sites and timeout arguments =='
rg -n -C 7 'waitForLaunchShellReady\(|waitForAgentLaunchReady\(|waitForBootPromptReady\(|sendLauncherCommandToSurface\(' src/server.ts
echo '== test coverage for propagation and focus after merge =='
rg -n -C 6 'submit_verification_reason|retry_safe|focus restore|does not steal|boot_prompt_timeout_ms|AgentLaunchError' \
tests/enter-reliability.test.ts \
tests/false-green-empty-surface.test.ts \
tests/server.test.ts \
tests/server-agent-tools.test.tsLength of output: 83990 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== new_worktree_split failure context =='
sed -n '9150,9245p' src/server.ts
echo '== spawn_in_workspace creation and failure context =='
sed -n '9240,9535p' src/server.ts
echo '== all wrappers that convert boot-prompt errors to tool errors =='
rg -n -C 14 'BootPromptTimeoutError|BootPromptDeliveryError|BootPromptUpdateMenuBlockedError' src/server.ts
echo '== relevant existing failure identity assertions =='
rg -n -C 8 'agent_id.*surface_id|surface_id.*workspace_id|BootPromptTimeoutError|boot prompt.*timeout|readiness fails' \
tests/server-agent-tools.test.ts tests/server.test.tsLength of output: 45487
P1 — Preserve created identities for all spawn readiness failures.
Callers cannot recover, inspect, or clean up the created surface from these responses. Preserve the active result identity in these catch paths. For ✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Fresh CI on @codex review @coderabbitai review @greptileai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR refs =='
gh pr view 345 --json baseRefOid,headRefOid,headRefName,url
echo '== Recent commits =='
git log --oneline -8
echo '== Delta from previous reported head =='
git diff --stat 276ccb8..HEAD
git diff --name-status 276ccb8..HEAD
git diff --unified=80 276ccb8..HEAD
echo '== Modified test structure =='
fd -t f 'server-agent-tools\.test\.ts$' tests -x ast-grep outline {} --items all --view expanded
echo '== Timeout ceiling references =='
rg -n -C 10 '30_000|10_000|boot_prompt_timeout_ms|auto-update|virtual.*clock|advanceTimers' \
tests src
echo '== Prior finding: alternate spawn failure paths =='
rg -n -C 24 'new_worktree_split|spawn_in_workspace|AgentLaunchError|BootPromptTimeoutError|BootPromptDeliveryError' src/server.tsLength of output: 128668 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Checked-out and PR head commits =='
git rev-parse HEAD
gh pr view 345 --json headRefOid --jq '.headRefOid'
git show -s --format='%H%n%P%n%s' HEAD
git show -s --format='%H%n%P%n%s' 993641a
echo '== Exact delta: 276ccb8..993641a =='
git diff --stat 276ccb8..993641a
git diff --unified=100 276ccb8..993641a -- tests/server-agent-tools.test.ts
echo '== Test context around all 30-second ceiling references =='
rg -n -C 20 '30_000|10_000|auto-update|update.*(clock|timer)|clock.*update' tests/server-agent-tools.test.ts
echo '== new_worktree_split: complete catch =='
sed -n '9038,9240p' src/server.ts
echo '== spawn_in_workspace: declarations, spawn loop, and complete catch =='
sed -n '9275,9545p' src/server.ts
echo '== AgentLaunchError creation and engine spawn error boundaries =='
rg -n -C 16 'new AgentLaunchError|class AgentLaunchError|spawnAgent' src/agent-engine.tsLength of output: 30313
The delta only changes P1 — Preserve created identities in alternate spawn failure responses.
Return the created identities for these failure paths. For You are interacting with an AI system. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ff2e065a-c597-444d-afd4-2cdecce2c6a7) |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_6f936618-4db1-469d-b474-215d72c4077d) |
|
Resolved the latest CodeRabbit P1 on final head
Fresh local/pre-push evidence: typecheck and build pass; focus gate 16/16; CI-version Vitest 3.2.4 and @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR head =='
gh pr view 345 --json number,baseRefOid,headRefOid,url --jq '{number,baseRefOid,headRefOid,url}'
git rev-parse HEAD
echo '== commits and changed files since prior reviewed head =='
git log --oneline 276ccb8..HEAD
git diff --stat 276ccb8..HEAD
git diff --name-status 276ccb8..HEAD
echo '== source map =='
ast-grep outline src/server.ts --items all --view expanded | sed -n '/new_worktree_split/,+12p;/spawn_in_workspace/,+12p;/new_split/,+12p;/new_surface/,+12p'
echo '== final delta in source and tests =='
git diff --unified=70 276ccb8..HEAD -- \
src/server.ts \
tests/server-agent-tools.test.ts \
tests/server.test.ts
echo '== targeted failure handlers and identity payloads =='
rg -n -C 20 \
'new_worktree_split|spawn_in_workspace|new_split|new_surface|AgentLaunchError|BootPromptTimeoutError|BootPromptDeliveryError|PaneGone|surface_id|workspace_id|agent_id' \
src/server.ts
echo '== targeted regression tests =='
rg -n -C 14 \
'new_worktree_split|spawn_in_workspace|new_split|new_surface|AgentLaunchError|boot prompt|pane.gone|surface_id|workspace_id|agent_id' \
tests/server-agent-tools.test.ts tests/server.test.tsLength of output: 177863 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== final fix commit =='
git show -s --format='%H%n%P%n%s' 6cc6163
git diff --stat 993641a..6cc6163
git diff --unified=35 993641a..6cc6163 -- src/server.ts tests/server-agent-tools.test.ts tests/server.test.ts
echo '== handler locations =='
rg -n 'server\.tool\(\s*"(new_worktree_split|spawn_in_workspace|new_split|new_surface)"' src/server.ts
echo '== narrow source ranges =='
for range in 8850:9270 9270:9600 10270:10600 10600:10920; do
start="${range%:*}"
end="${range#*:}"
sed -n "${start},${end}p" src/server.ts
done
echo '== new regression test names and assertions only =='
rg -n -A38 -B4 \
'returns.*(agent|surface|workspace)|preserves.*(agent|surface|workspace)|post-creation|pane-gone|boot-prompt.*failure|launch.*failure' \
tests/server-agent-tools.test.ts tests/server.test.ts | head -n 900Length of output: 43819 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== spawn_in_workspace regression diff =='
git diff --unified=55 993641a..6cc6163 -- tests/spawn-workspace.test.ts
echo '== all final-commit test additions =='
git diff --unified=12 993641a..6cc6163 -- \
tests/server-agent-tools.test.ts \
tests/server.test.ts \
tests/spawn-workspace.test.ts \
| rg -n -C 8 '^\+.*(it\(|expect\(parsed\.(agent_id|surface_id|workspace_id|surface|workspace|agents)|submit_verification_reason|retry_safe)'
echo '== identity paths in final source =='
rg -n -C 8 \
'const createdIdentity|const activeSpawnIdentity|const createdAgentIdentities|const failureIdentityPayload|surfaceGonePayload\(e, createdIdentity\)|return err\(e, createdIdentity\)' \
src/server.ts
echo '== whitespace check for final fix =='
git diff --check 993641a..6cc6163Length of output: 19558
The prior P1 is resolved.
I found no blocking issues. ✏️ Learnings added
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cc6163285
ℹ️ 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".
| activeSpawnIdentity = { | ||
| agent_id: result.agent_id, | ||
| surface_id: result.surface_id, | ||
| workspace_id: result.workspace_id ?? workspace ?? null, |
There was a problem hiding this comment.
When cmux reports a placement mismatch, SpawnAgentResult.workspace_id remains the requested workspace while actual_workspace_id identifies where the pane really landed; spawnDeliveryWorkspace() already accounts for this distinction. Recording only workspace_id here means a later boot-prompt or post-spawn failure returns the wrong workspace for the current agent—and for prior agents in the batch—so cleanup or recovery using the returned route can target the wrong workspace. Prefer actual_workspace_id ?? workspace_id when capturing these identities.
AGENTS.md reference: AGENTS.md:L20-L25
Useful? React with 👍 / 👎.
| workspace_id: result.workspace_id ?? mutationWorkspace, | ||
| } | ||
| : {}; | ||
| if (e instanceof AgentLaunchError) { |
There was a problem hiding this comment.
In the checked new_worktree_split flow, an initial shell or launcher-readiness timeout is now wrapped as AgentLaunchError, so this branch returns before the later BootPromptTimeoutError handler can attach last_10_lines. Consequently the newly propagated boot_prompt_timeout_ms failures lose the screen evidence specifically collected for diagnosing readiness problems; inspect e.launch_cause for BootPromptTimeoutError and retain its payload alongside the created identity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/agent-engine.ts (1)
1898-1920: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPropagate
timeoutMsto the bare launch path.
this.launchCommandSenderreceivestimeout_ms, but the fallback atsrc/agent-engine.ts:1910-1919callsthis.client.sendandthis.client.sendKeywithout a timeout option. If an engine is constructed without the sender (for example during crash-recovery, which also callssendLaunchCommandwithout the timeout argument), a wedged write can block indefinitely. Propagate the timeout into that path or document/require the injected sender for the timeout contract.🤖 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/agent-engine.ts` around lines 1898 - 1920, Propagate the optional timeoutMs through the fallback path in sendLaunchCommand: include it in the options passed to both this.client.send and this.client.sendKey, alongside the existing stableSurfaceWriteOptions. Preserve the launchCommandSender path and current surface-binding assertions.tests/server-agent-tools.test.ts (1)
9132-9181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared focus-state handlers from the two fixture helpers.
makeFocusLifecycleExecduplicates three handlers frommakeFocusExec:
identify— Lines 9148-9164 repeat Lines 9050-9066.rpc+surface.focus— Lines 9165-9174 repeat Lines 9093-9102.select-workspace— Lines 9175-9181 repeat Lines 9086-9092.Both helpers now encode the same focus semantics. A future change to focus restoration must be applied twice, and the copies will drift. Extract a small shared focus-state object that owns
focusedWorkspace,focusedSurface, and these three handlers, then let both helpers delegate to it.🤖 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 `@tests/server-agent-tools.test.ts` around lines 9132 - 9181, Extract the duplicated focus state and handlers from makeFocusExec and makeFocusLifecycleExec into a shared focus-state helper owning focusedWorkspace, focusedSurface, and the identify, surface.focus RPC, and select-workspace behaviors. Update both fixture helpers to delegate to this shared object while preserving their existing options, failure behavior, and call tracking.
🤖 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/agent-engine.ts`:
- Around line 4786-4792: The launch-failure workspace contract is inconsistent
with the workspace persisted on the AgentRecord. Update the AgentLaunchError
construction in the launch flow to use the same workspace value persisted at
AgentRecord creation, then update the failure assertion in
tests/server-agent-tools.test.ts lines 3771-3775 to match the readiness-failure
expectation at lines 3711; both sites must use the single canonical workspace
value.
- Around line 185-196: Update the AgentLaunchError constructor to pass the
optional launch_cause through the native Error options as cause when calling
super, while preserving the existing message and identifier properties; remove
the redundant launch_cause field if no longer needed.
In `@src/server.ts`:
- Around line 6184-6190: Preserve the resolved workspace in failure identity
payloads: in src/server.ts lines 6184-6190 use result.workspace ||
targetWorkspace, and lines 6318-6324 use result.workspace || args.workspace for
createdIdentity. In the three AgentLaunchError branches at src/server.ts lines
8971-8997 and 9217-9243, fall back to spawnWorkspace and mutationWorkspace
respectively via the existing error workspace field. Add creation-output
variants omitting workspace in tests/server.test.ts lines 9564-9603 and reuse
that variant for the new_surface test at line 9665.
- Around line 10185-10190: Replace the hand-rolled SubmitVerificationError
response fields at the listed send_input, send_command, new_worktree_split,
spawn_in_workspace, wait_for, send_to_agent, and remaining send_to sites with
the centralized submit_verificationFailurePayload used by err(). Remove
redundant inline submit_verified/retry_count or reason fields while preserving
the existing error response behavior.
In `@tests/enter-reliability.test.ts`:
- Around line 682-687: Update the timer advances in the verification reliability
test around resultPromise to derive from SEND_INPUT_SUBMIT_VERIFY_TIMEOUT_MS
instead of hard-coded 4,900 and 1,000 millisecond values. Preserve the
assertions that the operation remains unsettled just before the timeout and then
completes after crossing the timeout boundary.
In `@tests/false-green-empty-surface.test.ts`:
- Around line 210-213: Add an assertion in the test around result.isError and
parsed fields that verifies the classified failure reason is
surface_screen_empty for the blank-screen fixture; if the fixture represents an
unreadable screen, assert surface_read_unavailable instead, ensuring
submit_evidence_absent is not accepted.
In `@tests/server-agent-tools.test.ts`:
- Around line 3542-3579: Adjust the test fixture around the read-screen handling
and boot_prompt_timeout_ms so the transient failure window ends before the 37 ms
timeout, allowing the readable response branch to execute and recovery to be
tested. Alternatively, remove the transient read failure branch if this test
should only validate the timeout value; preserve the existing timeout assertion.
In `@tests/server.test.ts`:
- Around line 9564-9603: Update the failure-path tests for new_split and
new_surface so their mocked new-split creation responses omit workspace, then
assert each response still reports the requested workspace alongside the created
surface and error details. Preserve the existing rename-failure setup and
assertions, covering the workspace fallback behavior implemented by the
corresponding server handlers.
---
Outside diff comments:
In `@src/agent-engine.ts`:
- Around line 1898-1920: Propagate the optional timeoutMs through the fallback
path in sendLaunchCommand: include it in the options passed to both
this.client.send and this.client.sendKey, alongside the existing
stableSurfaceWriteOptions. Preserve the launchCommandSender path and current
surface-binding assertions.
In `@tests/server-agent-tools.test.ts`:
- Around line 9132-9181: Extract the duplicated focus state and handlers from
makeFocusExec and makeFocusLifecycleExec into a shared focus-state helper owning
focusedWorkspace, focusedSurface, and the identify, surface.focus RPC, and
select-workspace behaviors. Update both fixture helpers to delegate to this
shared object while preserving their existing options, failure behavior, and
call tracking.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 06c0f4de-757c-40f6-8893-9f2bc4f84e6b
📒 Files selected for processing (7)
src/agent-engine.tssrc/server.tstests/enter-reliability.test.tstests/false-green-empty-surface.test.tstests/server-agent-tools.test.tstests/server.test.tstests/spawn-workspace.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
tests/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.test.ts: Mirror source layout in tests (src/foo.ts->tests/foo.test.ts).
Do not add integration tests that require a running cmux instance; tests should be fully mocked.
Files:
tests/spawn-workspace.test.tstests/false-green-empty-surface.test.tstests/server-agent-tools.test.tstests/server.test.tstests/enter-reliability.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.ts: Build the project with TypeScript (tsc) and keep source code compatible with Node 20+ and Zod-based typing.
Use theok(data)anderr(error)helpers for consistent MCP tool responses.
All MCP tool handlers must return{ content: TextContent[], structuredContent?, isError? }.
Files:
src/agent-engine.tssrc/server.ts
src/agent-engine.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Implement agent lifecycle behavior in
agent-engine.ts, including spawning, monitoring, and quality tracking.
Files:
src/agent-engine.ts
tests/server.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Server tests should mock the cmux client via
createServer({ exec, skipAgentLifecycle }).
Files:
tests/server.test.ts
src/server.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Register all MCP tools in
server.ts, including the 33 tool handlers, and conditionally skip agent-lifecycle tools whenskipAgentLifecycle: true.
Files:
src/server.ts
🧠 Learnings (1)
📚 Learning: 2026-03-15T10:42:35.917Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 1
File: tests/quality-tracking.test.ts:171-200
Timestamp: 2026-03-15T10:42:35.917Z
Learning: In tests/quality-tracking.test.ts for the cmuxlayer project, ensure that at or above 80% context quality degradation, behavior depends on depth: depth-0 agents receive a /compact command; depth > 0 agents are killed and logged (kill + log). Respawn of non-root agents is out of scope for v1. Treat the design doc quality tracking section as the authoritative source for this behavior, and align test expectations accordingly.
Applied to files:
tests/spawn-workspace.test.tstests/false-green-empty-surface.test.tstests/server-agent-tools.test.tstests/server.test.tstests/enter-reliability.test.ts
🔇 Additional comments (33)
tests/server-agent-tools.test.ts (7)
3494-3503: This handler call still bypassesspawn.inputSchema.parse, and it uses a very smallboot_prompt_timeout_ms: 20. If the registered schema constrains the minimum value, a real MCP client is rejected while this test passes. The same concern was raised and addressed for the tests at Lines 3433 and 3463.
80-84: LGTM!Also applies to: 207-217
3425-3453: LGTM!
3455-3483: LGTM!
3705-3711: LGTM!
9011-9024: LGTM!Also applies to: 9120-9130, 9259-9574
3607-3609: 🎯 Functional CorrectnessNo change needed.
The relaunch path sends
ctrl-cinsrc/server.ts, so the fixture matches the source behavior.src/agent-engine.ts (3)
145-169: LGTM!
322-329: LGTM!
4618-4625: LGTM!tests/spawn-workspace.test.ts (2)
222-247: LGTM!Also applies to: 256-274
248-255: 🎯 Functional CorrectnessNo change needed.
src/server.ts (12)
3682-3722: The nested classification block was already flagged in a previous review and withdrawn by the author as a non-functional readability refactor. No new concern is raised here.
514-540: LGTM!
761-767: LGTM!Also applies to: 793-793
2900-2901: LGTM!
3534-3597: LGTM!
3778-3779: LGTM!Also applies to: 3814-3814, 3842-3842
4311-4311: LGTM!Also applies to: 4325-4325, 4385-4385, 4442-4442
4599-4624: LGTM!Also applies to: 4642-4642
4931-4932: LGTM!
9326-9349: LGTM!Also applies to: 9396-9402, 9436-9438
9564-9587: LGTM!
8007-8017: 🩺 Stability & AvailabilityNo change needed.
boot_prompt_timeout_msis optional inspawnAgentand is forwarded tolaunchCommandSenderduring the spawn launch path; the resume crash-recovery path does not pass it.tests/enter-reliability.test.ts (3)
353-389: LGTM!
635-655: LGTM!
502-503: LGTM!Also applies to: 575-575, 592-593, 717-718, 733-733, 749-750, 765-765, 782-783
tests/server.test.ts (6)
2848-2848: LGTM!Also applies to: 2881-2898
2926-2979: LGTM!
7513-7517: LGTM!
9308-9312: LGTM!Also applies to: 9383-9385
9665-9703: LGTM!
2900-2924: 📐 Maintainability & Code QualityCleanup is already handled.
The test removes
stateDirafter thesend_commandassertions, so no change is needed.
| export class AgentLaunchError extends Error { | ||
| constructor( | ||
| message: string, | ||
| readonly agent_id: string, | ||
| readonly surface_id: string, | ||
| readonly workspace_id?: string, | ||
| readonly launch_cause?: unknown, | ||
| ) { | ||
| super(message); | ||
| this.name = "AgentLaunchError"; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the TS target and the AgentLaunchError consumption pattern.
set -euo pipefail
fd -H -t f 'tsconfig.*json' | while IFS= read -r f; do
echo "=== $f ==="
cat -n "$f"
done
echo "=== AgentLaunchError usages ==="
rg -nP --type=ts -C 6 '\bAgentLaunchError\b'Repository: EtanHey/cmuxlayer
Length of output: 1670
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/agent-engine.ts outline ==="
ast-grep outline src/agent-engine.ts --view expanded | sed -n '1,220p'
echo "=== src/agent-engine.ts lines 160-210 ==="
cat -n src/agent-engine.ts | sed -n '160,210p'
echo "=== AgentLaunchError text search ==="
rg -n -C 6 'AgentLaunchError|AgentLaunch' .
echo "=== Error cause probe ==="
node - <<'JS'
class AgentLaunchError extends Error {
constructor(message, launch_cause) {
super(message);
this.name = "AgentLaunchError";
Object.assign(this, { agent_id: "a", surface_id: "s", launch_cause });
}
}
const error = new AgentLaunchError("boom", new Error("cause"));
console.log("cause:", error.cause);
console.log("launch_cause:", error.launch_cause);
console.log("util.inspect:", ...require("util").inspect(error).split("\n").slice(0, 5));
JSRepository: EtanHey/cmuxlayer
Length of output: 20783
Use the native error cause for the underlying launch error.
AgentLaunchError.launch_cause duplicates the standard error.cause slot. If launch_cause is an error, pass { cause: launch_cause } to super so generic error-chain handling, util.inspect, and structured logging see the root cause.
♻️ Proposed change to chain the cause
export class AgentLaunchError extends Error {
constructor(
message: string,
readonly agent_id: string,
readonly surface_id: string,
readonly workspace_id?: string,
readonly launch_cause?: unknown,
) {
- super(message);
+ const superArgs = launch_cause instanceof Error
+ ? [message, { cause: launch_cause }]
+ : [message];
+ super(...superArgs);
this.name = "AgentLaunchError";
}
}🤖 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/agent-engine.ts` around lines 185 - 196, Update the AgentLaunchError
constructor to pass the optional launch_cause through the native Error options
as cause when calling super, while preserving the existing message and
identifier properties; remove the redundant launch_cause field if no longer
needed.
| throw new AgentLaunchError( | ||
| message, | ||
| failedAgentId, | ||
| surface.surface, | ||
| surface.actual_workspace ?? surface.workspace, | ||
| error, | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
The reported workspace_id on a launch failure has no single definition. The engine reports surface.actual_workspace ?? surface.workspace while the durable AgentRecord persists surface.workspace, and the value a caller observes also depends on how far the spawn progressed before failing. The two new new_worktree_split tests encode that inconsistency as expected behavior: an early shell-readiness failure reports the creation-time workspace, and a later boot-prompt failure reports the topology-resolved workspace.
src/agent-engine.ts#L4786-L4792: pick one workspace value for the failure contract, and makeAgentLaunchError.workspace_idagree with theworkspace_idpersisted on theAgentRecordat Line 4635.tests/server-agent-tools.test.ts#L3771-L3775: after the contract is fixed, assert the same workspace value here as the readiness-failure test asserts at Line 3711, instead of"workspace:1"versus"ws:1".
📍 Affects 2 files
src/agent-engine.ts#L4786-L4792(this comment)tests/server-agent-tools.test.ts#L3771-L3775
🤖 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/agent-engine.ts` around lines 4786 - 4792, The launch-failure workspace
contract is inconsistent with the workspace persisted on the AgentRecord. Update
the AgentLaunchError construction in the launch flow to use the same workspace
value persisted at AgentRecord creation, then update the failure assertion in
tests/server-agent-tools.test.ts lines 3771-3775 to match the readiness-failure
expectation at lines 3711; both sites must use the single canonical workspace
value.
| const createdIdentity = result | ||
| ? { | ||
| surface: result.surface, | ||
| workspace: result.workspace, | ||
| ...(result.surface_id ? { surface_id: result.surface_id } : {}), | ||
| } | ||
| : {}; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Failure-path identity payloads drop the resolved workspace. Each success path applies a workspace fallback, but the matching failure payload reads the raw value. When cmux omits the workspace, or when AgentLaunchError.workspace_id is undefined, the caller cannot locate the created surface. That defeats the identity-preservation contract this change adds.
src/server.ts#L6184-L6190: setworkspace: result.workspace || targetWorkspaceincreatedIdentity, matching Line 6091.src/server.ts#L6318-L6324: setworkspace: result.workspace || args.workspaceincreatedIdentity, matching Line 6270.src/server.ts#L8971-L8997: usee.workspace_id ?? spawnWorkspacein all threeAgentLaunchErrorbranches.src/server.ts#L9217-L9243: usee.workspace_id ?? mutationWorkspacein all threeAgentLaunchErrorbranches, matching thecreatedIdentityfallback at Line 9214.tests/server.test.ts#L9564-L9603: add a variant whose creation stdout omitsworkspace, and apply the same variant to thenew_surfacetest at Line 9665.
📍 Affects 2 files
src/server.ts#L6184-L6190(this comment)src/server.ts#L6318-L6324src/server.ts#L8971-L8997src/server.ts#L9217-L9243tests/server.test.ts#L9564-L9603
🤖 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/server.ts` around lines 6184 - 6190, Preserve the resolved workspace in
failure identity payloads: in src/server.ts lines 6184-6190 use result.workspace
|| targetWorkspace, and lines 6318-6324 use result.workspace || args.workspace
for createdIdentity. In the three AgentLaunchError branches at src/server.ts
lines 8971-8997 and 9217-9243, fall back to spawnWorkspace and mutationWorkspace
respectively via the existing error workspace field. Add creation-output
variants omitting workspace in tests/server.test.ts lines 9564-9603 and reuse
that variant for the new_surface test at line 9665.
| ...(e instanceof SubmitVerificationError | ||
| ? { | ||
| submit_verification_reason: e.reason, | ||
| retry_safe: e.retry_safe, | ||
| } | ||
| : {}), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Reuse submitVerificationFailurePayload at the remaining SubmitVerificationError sites.
send_to now uses the centralized payload, but send_input (Line 6576), send_command (Line 6736), new_worktree_split (Line 9253), spawn_in_workspace (Line 9617), wait_for (Line 9777), and send_to_agent (Line 11063) still hand-roll { submit_verified: false, retry_count }. err() already injects the full payload for this error type, so those inline extras are redundant and will drift when a new field is added.
Also applies to: 10976-10976
🤖 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/server.ts` around lines 10185 - 10190, Replace the hand-rolled
SubmitVerificationError response fields at the listed send_input, send_command,
new_worktree_split, spawn_in_workspace, wait_for, send_to_agent, and remaining
send_to sites with the centralized submit_verificationFailurePayload used by
err(). Remove redundant inline submit_verified/retry_count or reason fields
while preserving the existing error response behavior.
| await vi.advanceTimersByTimeAsync(4_900); | ||
| expect(settled).toBe(false); | ||
| expect(client.verificationReadAttempts).toBeGreaterThan(1); | ||
|
|
||
| await vi.advanceTimersByTimeAsync(1_000); | ||
| const result = await resultPromise; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Derive the clock advances from the verification timeout constant.
4_900 and 1_000 encode the 5s SEND_INPUT_SUBMIT_VERIFY_TIMEOUT_MS window as literals. If that constant changes, the "not settled" assertion stops testing the boundary and becomes either trivially true or flaky. Import the constant and compute the advances from it.
🤖 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 `@tests/enter-reliability.test.ts` around lines 682 - 687, Update the timer
advances in the verification reliability test around resultPromise to derive
from SEND_INPUT_SUBMIT_VERIFY_TIMEOUT_MS instead of hard-coded 4,900 and 1,000
millisecond values. Preserve the assertions that the operation remains unsettled
just before the timeout and then completes after crossing the timeout boundary.
| expect(result.isError).toBe(true); | ||
| expect(parsed.ok).toBe(false); | ||
| expect(parsed.submit_verified).toBe(false); | ||
| expect(parsed.retry_count).toBe(0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Assert the classified failure reason.
This suite targets the empty-surface false-green. The server now distinguishes surface_screen_empty from submit_evidence_absent and surface_read_unavailable. Without a reason assertion, a regression that reclassifies a blank screen still passes this test.
🧪 Proposed addition
expect(result.isError).toBe(true);
expect(parsed.ok).toBe(false);
expect(parsed.submit_verified).toBe(false);
+ expect(parsed.submit_verification_reason).toBe("surface_screen_empty");
+ expect(parsed.retry_safe).toBe(false);
expect(parsed.retry_count).toBe(0);Adjust the expected reason if the fixture screen is unreadable rather than blank.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(result.isError).toBe(true); | |
| expect(parsed.ok).toBe(false); | |
| expect(parsed.submit_verified).toBe(false); | |
| expect(parsed.retry_count).toBe(0); | |
| expect(result.isError).toBe(true); | |
| expect(parsed.ok).toBe(false); | |
| expect(parsed.submit_verified).toBe(false); | |
| expect(parsed.submit_verification_reason).toBe("surface_screen_empty"); | |
| expect(parsed.retry_safe).toBe(false); | |
| expect(parsed.retry_count).toBe(0); |
🤖 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 `@tests/false-green-empty-surface.test.ts` around lines 210 - 213, Add an
assertion in the test around result.isError and parsed fields that verifies the
classified failure reason is surface_screen_empty for the blank-screen fixture;
if the fixture represents an unreadable screen, assert surface_read_unavailable
instead, ensuring submit_evidence_absent is not accepted.
| if (args.includes("read-screen")) { | ||
| if ( | ||
| launcherSentAt !== null && | ||
| Date.now() - launcherSentAt < 200 | ||
| ) { | ||
| throw new Error("EAGAIN: launcher screen not readable yet"); | ||
| } | ||
| return { | ||
| stdout: JSON.stringify({ | ||
| surface: "surface:new", | ||
| text: | ||
| launcherSentAt === null | ||
| ? "$ " | ||
| : "agent launcher still starting", | ||
| lines: 20, | ||
| scrollback_used: false, | ||
| }), | ||
| stderr: "", | ||
| }; | ||
| } | ||
| return baseExec(cmd, args); | ||
| }); | ||
| const server = createLifecycleServer(exec); | ||
| const spawn = (server as any)._registeredTools["spawn_agent"]; | ||
|
|
||
| const resultPromise = spawn.handler( | ||
| { | ||
| repo: "brainlayer", | ||
| model: "codex", | ||
| cli: "codex", | ||
| prompt: "agent-launch timeout contract", | ||
| boot_prompt_timeout_ms: 37, | ||
| }, | ||
| {} as any, | ||
| ); | ||
| for (let elapsed = 0; elapsed < 1_000; elapsed += 50) { | ||
| await vi.advanceTimersByTimeAsync(50); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The transient screen-read failure window is unreachable within this timeout budget.
Lines 3543-3548 make read-screen throw for the first 200 ms after the launcher send. Line 3573 sets boot_prompt_timeout_ms: 37. The timeout expires long before the 200 ms window closes, so the readable branch at Lines 3549-3560 never runs and recovery from a transient read failure is never exercised.
The timeout assertion itself is still valid, because the 37 ms value appears in the error text. But the fixture advertises coverage that the test does not provide.
Either shorten the throw window below the timeout so the test proves recovery, or remove the throw branch and keep this test focused on the timeout value.
♻️ Proposed change to make the transient failure recoverable
if (args.includes("read-screen")) {
if (
launcherSentAt !== null &&
- Date.now() - launcherSentAt < 200
+ Date.now() - launcherSentAt < 10
) {
throw new Error("EAGAIN: launcher screen not readable yet");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (args.includes("read-screen")) { | |
| if ( | |
| launcherSentAt !== null && | |
| Date.now() - launcherSentAt < 200 | |
| ) { | |
| throw new Error("EAGAIN: launcher screen not readable yet"); | |
| } | |
| return { | |
| stdout: JSON.stringify({ | |
| surface: "surface:new", | |
| text: | |
| launcherSentAt === null | |
| ? "$ " | |
| : "agent launcher still starting", | |
| lines: 20, | |
| scrollback_used: false, | |
| }), | |
| stderr: "", | |
| }; | |
| } | |
| return baseExec(cmd, args); | |
| }); | |
| const server = createLifecycleServer(exec); | |
| const spawn = (server as any)._registeredTools["spawn_agent"]; | |
| const resultPromise = spawn.handler( | |
| { | |
| repo: "brainlayer", | |
| model: "codex", | |
| cli: "codex", | |
| prompt: "agent-launch timeout contract", | |
| boot_prompt_timeout_ms: 37, | |
| }, | |
| {} as any, | |
| ); | |
| for (let elapsed = 0; elapsed < 1_000; elapsed += 50) { | |
| await vi.advanceTimersByTimeAsync(50); | |
| } | |
| if (args.includes("read-screen")) { | |
| if ( | |
| launcherSentAt !== null && | |
| Date.now() - launcherSentAt < 10 | |
| ) { | |
| throw new Error("EAGAIN: launcher screen not readable yet"); | |
| } | |
| return { | |
| stdout: JSON.stringify({ | |
| surface: "surface:new", | |
| text: | |
| launcherSentAt === null | |
| ? "$ " | |
| : "agent launcher still starting", | |
| lines: 20, | |
| scrollback_used: false, | |
| }), | |
| stderr: "", | |
| }; | |
| } | |
| return baseExec(cmd, args); | |
| }); | |
| const server = createLifecycleServer(exec); | |
| const spawn = (server as any)._registeredTools["spawn_agent"]; | |
| const resultPromise = spawn.handler( | |
| { | |
| repo: "brainlayer", | |
| model: "codex", | |
| cli: "codex", | |
| prompt: "agent-launch timeout contract", | |
| boot_prompt_timeout_ms: 37, | |
| }, | |
| {} as any, | |
| ); | |
| for (let elapsed = 0; elapsed < 1_000; elapsed += 50) { | |
| await vi.advanceTimersByTimeAsync(50); | |
| } |
🤖 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 `@tests/server-agent-tools.test.ts` around lines 3542 - 3579, Adjust the test
fixture around the read-screen handling and boot_prompt_timeout_ms so the
transient failure window ends before the 37 ms timeout, allowing the readable
response branch to execute and recovery to be tested. Alternatively, remove the
transient read failure branch if this test should only validate the timeout
value; preserve the existing timeout assertion.
| it("new_split reports the created surface when rename fails", async () => { | ||
| mockExec = vi.fn().mockImplementation(async (_cmd, args: string[]) => { | ||
| if (args.includes("new-split")) { | ||
| return { | ||
| stdout: JSON.stringify({ | ||
| workspace: "workspace:1", | ||
| surface: "surface:2", | ||
| pane: "pane:1", | ||
| title: "New", | ||
| type: "terminal", | ||
| }), | ||
| stderr: "", | ||
| }; | ||
| } | ||
| if (args.includes("rename-tab")) { | ||
| throw new Error("rename failed after split creation"); | ||
| } | ||
| return { stdout: "{}", stderr: "" }; | ||
| }); | ||
|
|
||
| const server = createServer({ exec: mockExec, skipAgentLifecycle: true }); | ||
| const tool = (server as any)._registeredTools["new_split"]; | ||
|
|
||
| const result = await tool.handler( | ||
| { | ||
| direction: "right", | ||
| pane: "pane:1", | ||
| workspace: "workspace:1", | ||
| title: "Build Task", | ||
| }, | ||
| {} as any, | ||
| ); | ||
| const parsed = | ||
| result.structuredContent ?? JSON.parse(result.content[0].text); | ||
|
|
||
| expect(parsed.ok).toBe(false); | ||
| expect(parsed.error).toContain("rename failed after split creation"); | ||
| expect(parsed.surface).toBe("surface:2"); | ||
| expect(parsed.workspace).toBe("workspace:1"); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Cover the case where cmux omits the workspace in the creation result.
The mock returns workspace: "workspace:1", so this test passes whether or not src/server.ts applies a workspace fallback in the failure payload. Add a variant whose new-split stdout omits workspace, and assert the response still reports the requested workspace. Apply the same variant to the new_surface test at Line 9665. This pins the identity-preservation contract that Lines 6184-6190 and 6318-6324 of src/server.ts implement.
🤖 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 `@tests/server.test.ts` around lines 9564 - 9603, Update the failure-path tests
for new_split and new_surface so their mocked new-split creation responses omit
workspace, then assert each response still reports the requested workspace
alongside the created surface and error details. Preserve the existing
rename-failure setup and assertions, covering the workspace fallback behavior
implemented by the corresponding server handlers.
1 participant
Summary
boot_prompt_timeout_msacross initial shell, agent-launch, post-update relaunch, and boot-prompt readiness while preserving omitted phase defaults (10s / 15s / 60s)retry_safe: falseVerification
3e5a7af)git diff --check: passRebase conflict resolution
PR #346 overlapped this branch in the spawn lifecycle. The only manual content conflict was in
src/server.ts; it was resolved on behavior, keeping #346's focus lease lifecycle (focusTargetBeforeSplit,on_surface_created, and restore-after-readiness/error) while also threading this PR's explicit readiness timeout into the engine call. On failure, focus restoration remains best-effort and runs before the originalAgentLaunchErroror boot-delivery error is returned. The post-update relaunch reuses the captured surface and lease.The first fresh CI run exposed a test-driver-only ceiling in the auto-update integration test. Its fake-clock budget is now 30s to encompass two launcher submit-verification windows plus the independent 2s readiness phases; the production input remains
boot_prompt_timeout_ms: 2_000. This ceiling moved twice during the lane (5s → 10s → 30s). A third raise is a red flag, not a routine adjustment; follow-up work should assert the number of readiness phases directly instead of using elapsed virtual time as the implicit detector.Boundaries
Note
Medium Risk
Changes core spawn/launch and terminal submit-verification behavior in
server.tsandagent-engine.ts, which can flip previously ambiguous outcomes to explicit failures and alter timeout semantics for clients relying on old defaults.Overview
Tightens agent spawn readiness and Enter/submit verification so callers get predictable timeouts and honest failure metadata.
boot_prompt_timeout_msis threaded fromspawn_agent/new_worktree_splitthroughAgentEnginelaunch into shell readiness, agent-launch readiness, post-update relaunch, and boot-prompt delivery. The schema no longer defaults this field to 60s on spawn tools—when omitted, each phase keeps its existing default (10s shell, 15s launch, 60s boot prompt).Launch failures are wrapped in
AgentLaunchErrorwithagent_id,surface_id,workspace_id, and the underlying cause; spawn handlers map that (and nested safety/surface-gone errors) into structured tool error payloads so a surface that was created before readiness failed is still identifiable.Submit verification stops treating missing screen evidence as success or
nullfor most paths: the verifier polls through transient read failures and blank screens, then fails closed after the full window withsubmit_verification_reason(input_still_pending,surface_screen_empty,surface_read_unavailable,working_status_not_observed,submit_evidence_absent) andretry_safe: false. That metadata flows throughsend_to, broadcasts, delivery records, and boot-prompt delivery errors.Reviewed by Cursor Bugbot for commit 276ccb8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Note
Add spawn readiness timeouts and structured submit verification failure reasons
AgentLaunchErrorin agent-engine.ts so spawn failures carry agent/surface/workspace identity and the original cause, replacing bare error rethrows.SubmitVerificationFailureReasonunion type and areasonfield onSubmitVerificationError; the verification polling loop now returns specific reasons (input_still_pending,surface_screen_empty,surface_read_unavailable,working_status_not_observed,submit_evidence_absent) instead of returning null on transient failures.boot_prompt_timeout_msthrough spawn, relaunch, and launcher command paths so callers control shell/agent readiness wait windows; the schema default is removed in favor of downstream defaults.spawn_agent,spawn_in_workspace,send_to,send_command,new_surface, andnew_worktree_splitnow includesubmit_verification_reason,retry_safe, and created identity fields (agent_id,surface_id,workspace_id).boot_prompt_timeout_msno longer has a schema-level default, so callers omitting it will rely on internal defaults rather than a fixed value.Macroscope summarized 6cc6163.