fix: spawn_agent boot hardening — verify prompt delivery, report model mismatch, truthful ready/error state - #326
Merged
EtanHey merged 5 commits intoAug 4, 2026
Conversation
📝 WalkthroughWalkthroughBooting agents now inspect CLI interactivity, deliver and verify prompts, reconcile parsed models, persist settlement fields, and transition to ChangesBoot readiness verification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Sidebar
participant AgentEngine
participant Client
participant ScreenParser
Sidebar->>AgentEngine: process booting agent
AgentEngine->>Client: read CLI screen
Client-->>AgentEngine: screen contents
AgentEngine->>ScreenParser: parse status and model
ScreenParser-->>AgentEngine: interactive state and parsed model
AgentEngine->>Client: send prompt and enter
Client-->>AgentEngine: updated screen
AgentEngine->>AgentEngine: persist verification and ready/error state
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 3
🤖 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 401-437: Update maybeAdvanceBootingAgent so the
BOOT_READY_TIMEOUT_MS check occurs independently of the interactive branch,
before attempting delivery. When agent.updated_at is valid and the timeout has
expired, transition the agent to "error" regardless of whether parseScreen
reported it as interactive; preserve the existing handling for invalid or
non-expired timestamps and avoid leaving delivery failures in "booting"
indefinitely.
- Around line 439-494: Update the delivery and promotion flow in the
booting-agent method around submitVerified so a confirmed false verification
does not persist prompt_delivered as true or transition the record to "ready".
Preserve the booting state and return the current record (or otherwise use the
existing retry path) when screenShowsPendingInput identifies an unsubmitted
prompt, allowing the next sweep to resend it; keep successful and indeterminate
verification behavior unchanged.
In `@tests/sidebar-sync.test.ts`:
- Around line 315-540: The booting agent advancement suite is in the wrong test
file. Move the entire “Booting agent advancement (SDLC-87)” describe block,
including its setup, fixtures, and cases, from sidebar-sync.test.ts into
agent-engine.test.ts to mirror the source module containing AgentEngine;
preserve all test behavior and imports.
🪄 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
Run ID: 3572beb0-60f5-4b69-9f2f-0b19510cc3a6
📒 Files selected for processing (9)
src/agent-engine.tssrc/agent-facade.tssrc/agent-types.tssrc/screen-parser.tssrc/server.tssrc/state-manager.tstests/agent-facade.test.tstests/screen-parser.test.tstests/sidebar-sync.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
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/state-manager.tssrc/agent-facade.tssrc/screen-parser.tssrc/agent-engine.tssrc/agent-types.tssrc/server.ts
src/state-manager.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Keep sidebar state synchronization logic in
state-manager.ts.
Files:
src/state-manager.ts
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/agent-facade.test.tstests/screen-parser.test.tstests/sidebar-sync.test.ts
src/screen-parser.ts
📄 CodeRabbit inference engine (CLAUDE.md)
screen-parser.tsshould parse terminal output to detect agent type, model, token usage, context percentage, and supported agent output formats (Claude, Codex, Gemini, Cursor).
Files:
src/screen-parser.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 (2)
📚 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/agent-facade.test.tstests/screen-parser.test.tstests/sidebar-sync.test.ts
📚 Learning: 2026-03-15T10:42:36.027Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 1
File: tests/sidebar-sync.test.ts:79-279
Timestamp: 2026-03-15T10:42:36.027Z
Learning: In the cmuxlayer project, tests/sidebar-sync.test.ts should cover only the implemented channels: set-status, set-progress, and log. The rename-workspace and report_meta_block channels are intentionally deferred (per phase5-v2-cmux-sidebar-research.md) and must not be considered as missing test coverage. Do not flag or require tests for these two channels in this file.
Applied to files:
tests/sidebar-sync.test.ts
🔇 Additional comments (9)
tests/agent-facade.test.ts (1)
47-48: LGTM!tests/screen-parser.test.ts (1)
392-407: LGTM!src/agent-types.ts (1)
49-55: LGTM!Also applies to: 62-70
src/screen-parser.ts (2)
465-476: 🎯 Functional CorrectnessConfirm the
workingMarkersremoval is intentional.
"bypass permissions on"was dropped from the working-state markers. This changes status classification (working vs. idle) broadly for all CLI types, not just the booting-promotion path this PR targets, and isn't mentioned in the PR objectives. Please confirm this isn't an accidental scope-creep regression — if a Claude/other CLI screen legitimately contains this string while idle, removing it may be a deliberate false-positive fix; if not, this could newly misclassify a working agent as idle.
559-585: LGTM!src/server.ts (1)
37-42: LGTM!Also applies to: 299-303
src/agent-engine.ts (1)
36-40: LGTM!Also applies to: 71-75, 253-273, 655-656, 863-866
src/state-manager.ts (1)
221-227: LGTM!src/agent-facade.ts (1)
10-11: LGTM!
Comment on lines
+401
to
+437
| private async maybeAdvanceBootingAgent(agent: AgentRecord): Promise<AgentRecord> { | ||
| if (agent.state !== "booting" || agent.agent_id.startsWith("auto-")) { | ||
| return agent; | ||
| } | ||
|
|
||
| let screenText: string; | ||
| try { | ||
| const screen = await this.client.readScreen(agent.surface_id, { | ||
| lines: BOOT_READY_SCREEN_LINES, | ||
| }); | ||
| screenText = screen.text; | ||
| } catch { | ||
| return agent; | ||
| } | ||
|
|
||
| const parsed = parseScreen(screenText); | ||
| const interactive = parsed.agent_type !== "unknown" && parsed.status !== "frozen"; | ||
|
|
||
| if (!interactive) { | ||
| // updated_at, not created_at: crash-recovery respawns re-enter | ||
| // "booting" long after the original created_at, refreshing updated_at | ||
| // (mirrors isBootCaptureWindowOpen's use of updated_at above). | ||
| const since = Date.parse(agent.updated_at); | ||
| if (Number.isNaN(since) || Date.now() - since < BOOT_READY_TIMEOUT_MS) { | ||
| return agent; | ||
| } | ||
| try { | ||
| const failed = this.stateMgr.transition(agent.agent_id, "error", { | ||
| error: | ||
| "Stuck booting — CLI never became interactive within the boot timeout", | ||
| }); | ||
| this.registry.set(agent.agent_id, failed); | ||
| return failed; | ||
| } catch { | ||
| return agent; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Stuck-boot timeout only covers the "never interactive" case.
Once interactive is true, the boot timeout is never re-checked. If client.send/sendKey/readScreen in the delivery block (lines 454-478) keep throwing every sweep — e.g. a flaky surface — the outer catch (line 491) swallows it and the agent stays "booting" forever, since agent.updated_at is never refreshed on failure. This is the same "stuck indefinitely in booting" problem the PR is meant to fix, just reachable through a different path than the CLI-never-becomes-interactive case.
Consider tracking the timeout independent of the interactive branch (e.g. always compare agent.updated_at against BOOT_READY_TIMEOUT_MS before attempting delivery, and transition to "error" on expiry regardless of interactivity).
🤖 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 401 - 437, Update maybeAdvanceBootingAgent
so the BOOT_READY_TIMEOUT_MS check occurs independently of the interactive
branch, before attempting delivery. When agent.updated_at is valid and the
timeout has expired, transition the agent to "error" regardless of whether
parseScreen reported it as interactive; preserve the existing handling for
invalid or non-expired timestamps and avoid leaving delivery failures in
"booting" indefinitely.
Comment on lines
+439
to
+494
| try { | ||
| // SDLC-87 review fix: compute + persist prompt-delivery/model fields | ||
| // FIRST, while the record is still "booting", and only flip to "ready" | ||
| // as the final write. If send/sendKey/recheck throws anywhere above, | ||
| // the record never leaves "booting" and the next sweep retries safely | ||
| // (prompt_delivered stays false on disk until delivery actually | ||
| // completed). Flipping to "ready" before delivery would let a failure | ||
| // strand the record as ready-but-never-prompted, since the "state !== | ||
| // booting" guard at the top of this method would then skip every | ||
| // future retry. | ||
| const parsedModel = parsed.model; | ||
| const modelMismatch = computeModelMismatch(agent.model, parsedModel); | ||
| let promptDelivered = agent.prompt_delivered ?? false; | ||
| let submitVerified: boolean | null = agent.submit_verified ?? null; | ||
|
|
||
| if (!promptDelivered && agent.task_summary) { | ||
| await this.client.send( | ||
| agent.surface_id, | ||
| sanitizeTerminalInput(agent.task_summary), | ||
| ); | ||
| await this.client.sendKey(agent.surface_id, "return"); | ||
| promptDelivered = true; | ||
|
|
||
| await delay(PROMPT_DELIVERY_VERIFY_DELAY_MS); | ||
| try { | ||
| const recheck = await this.client.readScreen(agent.surface_id, { | ||
| lines: BOOT_READY_SCREEN_LINES, | ||
| }); | ||
| const reparsed = parseScreen(recheck.text); | ||
| if (isSubmitVerifiedStatus(reparsed.status)) { | ||
| submitVerified = true; | ||
| } else if (screenShowsPendingInput(recheck.text, agent.task_summary)) { | ||
| submitVerified = false; | ||
| } else { | ||
| submitVerified = null; | ||
| } | ||
| } catch { | ||
| submitVerified = null; | ||
| } | ||
| } | ||
|
|
||
| const patched = this.stateMgr.updateRecord(agent.agent_id, { | ||
| parsed_model: parsedModel, | ||
| model_mismatch: modelMismatch, | ||
| prompt_delivered: promptDelivered, | ||
| submit_verified: submitVerified, | ||
| }); | ||
| this.registry.set(agent.agent_id, patched); | ||
|
|
||
| const updated = this.stateMgr.transition(agent.agent_id, "ready", {}); | ||
| this.registry.set(agent.agent_id, updated); | ||
| return updated; | ||
| } catch { | ||
| return agent; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Promotion to "ready" ignores a confirmed failed submit verification, with no retry path.
When screenShowsPendingInput returns true, submitVerified is explicitly set to false (line 471) — meaning the prompt text is still visible/un-submitted. But promptDelivered was already forced to true right after sendKey (line 460), and the method unconditionally transitions the agent to "ready" (line 488) regardless of submitVerified. Since this method short-circuits once agent.state !== "booting", this agent can never be revisited/retried — it's a terminal ready state despite a verified delivery failure. This defeats the PR's stated goal of "allowing failed delivery attempts to remain retryable"; only the exception-thrown path actually retries.
🐛 Proposed fix: don't mark delivered/ready on a verified-false result
if (!promptDelivered && agent.task_summary) {
await this.client.send(
agent.surface_id,
sanitizeTerminalInput(agent.task_summary),
);
await this.client.sendKey(agent.surface_id, "return");
- promptDelivered = true;
await delay(PROMPT_DELIVERY_VERIFY_DELAY_MS);
try {
const recheck = await this.client.readScreen(agent.surface_id, {
lines: BOOT_READY_SCREEN_LINES,
});
const reparsed = parseScreen(recheck.text);
if (isSubmitVerifiedStatus(reparsed.status)) {
submitVerified = true;
+ promptDelivered = true;
} else if (screenShowsPendingInput(recheck.text, agent.task_summary)) {
submitVerified = false;
+ // leave promptDelivered=false so the next sweep retries delivery
} else {
submitVerified = null;
+ promptDelivered = true;
}
} catch {
submitVerified = null;
+ promptDelivered = true;
}
}
const patched = this.stateMgr.updateRecord(agent.agent_id, {
parsed_model: parsedModel,
model_mismatch: modelMismatch,
prompt_delivered: promptDelivered,
submit_verified: submitVerified,
});
this.registry.set(agent.agent_id, patched);
- const updated = this.stateMgr.transition(agent.agent_id, "ready", {});
+ const updated =
+ submitVerified === false
+ ? patched
+ : this.stateMgr.transition(agent.agent_id, "ready", {});
this.registry.set(agent.agent_id, updated);
return updated;Note this reintroduces resend-on-retry, which combined with the missing post-interactive timeout (see separate comment) could loop; pairing this fix with a bounded retry/timeout is recommended.
📝 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.
Suggested change
| try { | |
| // SDLC-87 review fix: compute + persist prompt-delivery/model fields | |
| // FIRST, while the record is still "booting", and only flip to "ready" | |
| // as the final write. If send/sendKey/recheck throws anywhere above, | |
| // the record never leaves "booting" and the next sweep retries safely | |
| // (prompt_delivered stays false on disk until delivery actually | |
| // completed). Flipping to "ready" before delivery would let a failure | |
| // strand the record as ready-but-never-prompted, since the "state !== | |
| // booting" guard at the top of this method would then skip every | |
| // future retry. | |
| const parsedModel = parsed.model; | |
| const modelMismatch = computeModelMismatch(agent.model, parsedModel); | |
| let promptDelivered = agent.prompt_delivered ?? false; | |
| let submitVerified: boolean | null = agent.submit_verified ?? null; | |
| if (!promptDelivered && agent.task_summary) { | |
| await this.client.send( | |
| agent.surface_id, | |
| sanitizeTerminalInput(agent.task_summary), | |
| ); | |
| await this.client.sendKey(agent.surface_id, "return"); | |
| promptDelivered = true; | |
| await delay(PROMPT_DELIVERY_VERIFY_DELAY_MS); | |
| try { | |
| const recheck = await this.client.readScreen(agent.surface_id, { | |
| lines: BOOT_READY_SCREEN_LINES, | |
| }); | |
| const reparsed = parseScreen(recheck.text); | |
| if (isSubmitVerifiedStatus(reparsed.status)) { | |
| submitVerified = true; | |
| } else if (screenShowsPendingInput(recheck.text, agent.task_summary)) { | |
| submitVerified = false; | |
| } else { | |
| submitVerified = null; | |
| } | |
| } catch { | |
| submitVerified = null; | |
| } | |
| } | |
| const patched = this.stateMgr.updateRecord(agent.agent_id, { | |
| parsed_model: parsedModel, | |
| model_mismatch: modelMismatch, | |
| prompt_delivered: promptDelivered, | |
| submit_verified: submitVerified, | |
| }); | |
| this.registry.set(agent.agent_id, patched); | |
| const updated = this.stateMgr.transition(agent.agent_id, "ready", {}); | |
| this.registry.set(agent.agent_id, updated); | |
| return updated; | |
| } catch { | |
| return agent; | |
| } | |
| } | |
| try { | |
| // SDLC-87 review fix: compute + persist prompt-delivery/model fields | |
| // FIRST, while the record is still "booting", and only flip to "ready" | |
| // as the final write. If send/sendKey/recheck throws anywhere above, | |
| // the record never leaves "booting" and the next sweep retries safely | |
| // (prompt_delivered stays false on disk until delivery actually | |
| // completed). Flipping to "ready" before delivery would let a failure | |
| // strand the record as ready-but-never-prompted, since the "state !== | |
| // booting" guard at the top of this method would then skip every | |
| // future retry. | |
| const parsedModel = parsed.model; | |
| const modelMismatch = computeModelMismatch(agent.model, parsedModel); | |
| let promptDelivered = agent.prompt_delivered ?? false; | |
| let submitVerified: boolean | null = agent.submit_verified ?? null; | |
| if (!promptDelivered && agent.task_summary) { | |
| await this.client.send( | |
| agent.surface_id, | |
| sanitizeTerminalInput(agent.task_summary), | |
| ); | |
| await this.client.sendKey(agent.surface_id, "return"); | |
| await delay(PROMPT_DELIVERY_VERIFY_DELAY_MS); | |
| try { | |
| const recheck = await this.client.readScreen(agent.surface_id, { | |
| lines: BOOT_READY_SCREEN_LINES, | |
| }); | |
| const reparsed = parseScreen(recheck.text); | |
| if (isSubmitVerifiedStatus(reparsed.status)) { | |
| submitVerified = true; | |
| promptDelivered = true; | |
| } else if (screenShowsPendingInput(recheck.text, agent.task_summary)) { | |
| submitVerified = false; | |
| // leave promptDelivered=false so the next sweep retries delivery | |
| } else { | |
| submitVerified = null; | |
| promptDelivered = true; | |
| } | |
| } catch { | |
| submitVerified = null; | |
| promptDelivered = true; | |
| } | |
| } | |
| const patched = this.stateMgr.updateRecord(agent.agent_id, { | |
| parsed_model: parsedModel, | |
| model_mismatch: modelMismatch, | |
| prompt_delivered: promptDelivered, | |
| submit_verified: submitVerified, | |
| }); | |
| this.registry.set(agent.agent_id, patched); | |
| const updated = | |
| submitVerified === false | |
| ? patched | |
| : this.stateMgr.transition(agent.agent_id, "ready", {}); | |
| this.registry.set(agent.agent_id, updated); | |
| return updated; | |
| } catch { | |
| return agent; | |
| } | |
| } |
🤖 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 439 - 494, Update the delivery and
promotion flow in the booting-agent method around submitVerified so a confirmed
false verification does not persist prompt_delivered as true or transition the
record to "ready". Preserve the booting state and return the current record (or
otherwise use the existing retry path) when screenShowsPendingInput identifies
an unsubmitted prompt, allowing the next sweep to resend it; keep successful and
indeterminate verification behavior unchanged.
Comment on lines
+315
to
+540
|
|
||
| describe("Booting agent advancement (SDLC-87)", () => { | ||
| let stateMgr: StateManager; | ||
| let mockClient: CmuxClient; | ||
| let engine: AgentEngine; | ||
| let liveSurfaces: CmuxSurface[]; | ||
|
|
||
| const IDLE_CLAUDE_SCREEN = { | ||
| surface: "surface:42", | ||
| text: ` | ||
| Say "go" when you're ready and I'll start your timer. | ||
|
|
||
| ────────────────────────────────────────────────────────────────────────────────────────── | ||
| ❯ | ||
| ────────────────────────────────────────────────────────────────────────────────────────── | ||
| ⎇ master | +1273,-196 | 🔧 11 418310 tokens | ||
| 🤖 Sonnet 4.6 | 💰 $0.10 current: 2.1.81 · latest… | ||
| ⏵⏵ bypass permissions on (shift+tab to cycle) | ||
| `, | ||
| lines: 40, | ||
| scrollback_used: false, | ||
| }; | ||
|
|
||
| const WORKING_CLAUDE_SCREEN = { | ||
| surface: "surface:42", | ||
| text: ` | ||
| Working on it… | ||
| ⏵⏵ bypass permissions on (shift+tab to cycle) | ||
| (esc to interrupt) | ||
| `, | ||
| lines: 40, | ||
| scrollback_used: false, | ||
| }; | ||
|
|
||
| const UNKNOWN_SCREEN = { | ||
| surface: "surface:42", | ||
| text: "$ ", | ||
| lines: 40, | ||
| scrollback_used: false, | ||
| }; | ||
|
|
||
| beforeEach(() => { | ||
| rmSync(TEST_DIR, { recursive: true, force: true }); | ||
| mkdirSync(TEST_DIR, { recursive: true }); | ||
| stateMgr = new StateManager(TEST_DIR); | ||
| mockClient = makeMockClient(); | ||
| liveSurfaces = []; | ||
| const surfaceProvider = async () => liveSurfaces; | ||
| const registry = new AgentRegistry(stateMgr, surfaceProvider); | ||
| engine = new AgentEngine(stateMgr, registry, mockClient, { | ||
| spawnPreflight: async () => {}, | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| engine.dispose(); | ||
| rmSync(TEST_DIR, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| it("promotes a booting agent to ready and delivers the stored prompt once the CLI is interactive", async () => { | ||
| let calls = 0; | ||
| (mockClient.readScreen as ReturnType<typeof vi.fn>).mockImplementation( | ||
| async () => { | ||
| calls += 1; | ||
| return calls === 1 ? IDLE_CLAUDE_SCREEN : WORKING_CLAUDE_SCREEN; | ||
| }, | ||
| ); | ||
| stateMgr.writeState( | ||
| makeRecord({ | ||
| agent_id: "a1", | ||
| state: "booting", | ||
| surface_id: "surface:42", | ||
| model: "sonnet", | ||
| task_summary: "design handoff", | ||
| }), | ||
| ); | ||
| liveSurfaces = [makeSurface("surface:42")]; | ||
| await engine.getRegistry().reconstitute(); | ||
|
|
||
| await engine.runSweep(); | ||
|
|
||
| const record = engine.getAgentState("a1"); | ||
| expect(record?.state).toBe("ready"); | ||
| expect(record?.parsed_model).toBe("Sonnet 4.6"); | ||
| expect(record?.model_mismatch).toBe(false); | ||
| expect(record?.prompt_delivered).toBe(true); | ||
| expect(record?.submit_verified).toBe(true); | ||
| expect(mockClient.send).toHaveBeenCalledWith( | ||
| "surface:42", | ||
| "design handoff", | ||
| ); | ||
| expect(mockClient.sendKey).toHaveBeenCalledWith("surface:42", "return"); | ||
| }); | ||
|
|
||
| it("reports a model mismatch when the parsed banner model disagrees with the requested model", async () => { | ||
| (mockClient.readScreen as ReturnType<typeof vi.fn>).mockResolvedValue( | ||
| IDLE_CLAUDE_SCREEN, | ||
| ); | ||
| stateMgr.writeState( | ||
| makeRecord({ | ||
| agent_id: "a1", | ||
| state: "booting", | ||
| surface_id: "surface:42", | ||
| model: "opus", | ||
| task_summary: "design handoff", | ||
| }), | ||
| ); | ||
| liveSurfaces = [makeSurface("surface:42")]; | ||
| await engine.getRegistry().reconstitute(); | ||
|
|
||
| await engine.runSweep(); | ||
|
|
||
| const record = engine.getAgentState("a1"); | ||
| expect(record?.state).toBe("ready"); | ||
| expect(record?.parsed_model).toBe("Sonnet 4.6"); | ||
| expect(record?.model_mismatch).toBe(true); | ||
| }); | ||
|
|
||
| it("reports a stuck-booting agent as errored once it exceeds the boot timeout", async () => { | ||
| (mockClient.readScreen as ReturnType<typeof vi.fn>).mockResolvedValue( | ||
| UNKNOWN_SCREEN, | ||
| ); | ||
| stateMgr.writeState( | ||
| makeRecord({ | ||
| agent_id: "a1", | ||
| state: "booting", | ||
| surface_id: "surface:42", | ||
| updated_at: "2020-01-01T00:00:00Z", | ||
| }), | ||
| ); | ||
| liveSurfaces = [makeSurface("surface:42")]; | ||
| await engine.getRegistry().reconstitute(); | ||
|
|
||
| await engine.runSweep(); | ||
|
|
||
| const record = engine.getAgentState("a1"); | ||
| expect(record?.state).toBe("error"); | ||
| expect(record?.error).toMatch(/stuck booting/i); | ||
| expect(mockClient.log).toHaveBeenCalledWith( | ||
| "errored: brainlayer", | ||
| expect.objectContaining({ level: "error", source: "cmux-mcp" }), | ||
| ); | ||
| }); | ||
|
|
||
| it("leaves a booting agent untouched while still within the boot window with no interactive signal", async () => { | ||
| (mockClient.readScreen as ReturnType<typeof vi.fn>).mockResolvedValue( | ||
| UNKNOWN_SCREEN, | ||
| ); | ||
| stateMgr.writeState( | ||
| makeRecord({ | ||
| agent_id: "a1", | ||
| state: "booting", | ||
| surface_id: "surface:42", | ||
| updated_at: new Date().toISOString(), | ||
| }), | ||
| ); | ||
| liveSurfaces = [makeSurface("surface:42")]; | ||
| await engine.getRegistry().reconstitute(); | ||
|
|
||
| await engine.runSweep(); | ||
|
|
||
| expect(engine.getAgentState("a1")?.state).toBe("booting"); | ||
| }); | ||
|
|
||
| it("does not advance auto-discovered agents", async () => { | ||
| (mockClient.readScreen as ReturnType<typeof vi.fn>).mockResolvedValue( | ||
| IDLE_CLAUDE_SCREEN, | ||
| ); | ||
| stateMgr.writeState( | ||
| makeRecord({ | ||
| agent_id: "auto-brainlayer-1", | ||
| state: "booting", | ||
| surface_id: "surface:42", | ||
| }), | ||
| ); | ||
| liveSurfaces = [makeSurface("surface:42")]; | ||
| await engine.getRegistry().reconstitute(); | ||
|
|
||
| await engine.runSweep(); | ||
|
|
||
| expect(engine.getAgentState("auto-brainlayer-1")?.state).toBe("booting"); | ||
| expect(mockClient.send).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("keeps a booting agent retry-safe (not stranded ready) when prompt delivery throws", async () => { | ||
| (mockClient.readScreen as ReturnType<typeof vi.fn>).mockResolvedValue( | ||
| IDLE_CLAUDE_SCREEN, | ||
| ); | ||
| (mockClient.send as ReturnType<typeof vi.fn>).mockRejectedValueOnce( | ||
| new Error("socket dropped mid-send"), | ||
| ); | ||
| stateMgr.writeState( | ||
| makeRecord({ | ||
| agent_id: "a1", | ||
| state: "booting", | ||
| surface_id: "surface:42", | ||
| model: "sonnet", | ||
| task_summary: "design handoff", | ||
| }), | ||
| ); | ||
| liveSurfaces = [makeSurface("surface:42")]; | ||
| await engine.getRegistry().reconstitute(); | ||
|
|
||
| await engine.runSweep(); | ||
|
|
||
| // A transient send failure must not persist "ready" ahead of delivery — | ||
| // otherwise the top-of-function "state !== booting" guard would skip | ||
| // this agent on every future sweep and the prompt would never be | ||
| // retried (SDLC-87 Codex review finding). | ||
| const afterFailure = engine.getAgentState("a1"); | ||
| expect(afterFailure?.state).toBe("booting"); | ||
| expect(afterFailure?.prompt_delivered).not.toBe(true); | ||
|
|
||
| // Next sweep, with the transient failure resolved, must retry and | ||
| // succeed rather than being permanently skipped. | ||
| (mockClient.readScreen as ReturnType<typeof vi.fn>).mockResolvedValue( | ||
| WORKING_CLAUDE_SCREEN, | ||
| ); | ||
| await engine.runSweep(); | ||
|
|
||
| const afterRetry = engine.getAgentState("a1"); | ||
| expect(afterRetry?.state).toBe("ready"); | ||
| expect(afterRetry?.prompt_delivered).toBe(true); | ||
| expect(mockClient.send).toHaveBeenCalledTimes(2); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move test suite to mirror source layout.
As per coding guidelines, test layout must mirror the source layout (src/foo.ts -> tests/foo.test.ts). Since the booting agent advancement logic is implemented in src/agent-engine.ts, this test suite should be placed in tests/agent-engine.test.ts rather than tests/sidebar-sync.test.ts.
🤖 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/sidebar-sync.test.ts` around lines 315 - 540, The booting agent
advancement suite is in the wrong test file. Move the entire “Booting agent
advancement (SDLC-87)” describe block, including its setup, fixtures, and cases,
from sidebar-sync.test.ts into agent-engine.test.ts to mirror the source module
containing AgentEngine; preserve all test behavior and imports.
Source: Coding guidelines
Owner
|
@jaredloman — status on this PR, and an apology for the silence: it has been open 19 days without the review it was promised. That is on us, not you. Where it stands (2026-08-03 fleet PR sweep): What would help: a rebase onto current If you would rather not carry it forward, say so and we will close it with your context recorded and re-implement the idea natively, crediting the PR. Either is fine; what is not fine is leaving it open indefinitely with no answer. — |
…l mismatch, truthful ready/error state
- screen-parser.ts: stop misclassifying idle Claude sessions as "working"
(the persistent "bypass permissions on" footer was wrongly used as a
workingMarker); extract isSubmitVerifiedStatus/screenShowsPendingInput
as shared exports for delivery verification.
- agent-engine.ts: new maybeAdvanceBootingAgent(), run from the sweep
alongside maybeCaptureBootSessionId(). Promotes booting -> ready once
the CLI screen is actually interactive (previously never happened for
engine-spawned agents); delivers the stored task_summary exactly once
on that transition and verifies submission landed; computes
parsed_model/model_mismatch against the requested model; times out
stuck boots into a truthful "error" state instead of hanging in
"booting" forever. Guards against touching auto-discovered ("auto-")
records, which are synced by a different path.
- agent-types.ts/agent-facade.ts/state-manager.ts: plumb the new
submit_verified/prompt_delivered/parsed_model/model_mismatch fields
through AgentRecord, PublicAgent projection, and ensureAutoRecord's
explicit "already settled" defaults for auto-discovered agents.
- Tests: regression test for the idle-vs-working misclassification,
and 5 new tests covering ready-promotion + prompt delivery,
model-mismatch detection, stuck-boot timeout, in-window no-op, and
the auto-discovered skip guard.
package-lock.json: incidental npm/bun sync — package.json already
declared cmuxlayer-app-server/transform-tty, lockfile was stale before
this session's install/build/test runs corrected it.
Codex review (SDLC-87) flagged that maybeAdvanceBootingAgent() persisted the "ready" state transition before attempting prompt delivery. Any throw during client.send/sendKey/recheck left the on-disk record "ready" while returning a stale in-memory "booting" snapshot to the caller — the next sweep's "state !== booting" guard would then skip the record forever, silently losing prompt delivery (undermining the whole point of this work item). Reordered so prompt_delivered/submit_verified/parsed_model/model_mismatch are computed and persisted via updateRecord first, while the record is still "booting"; the transition to "ready" is now the final write, only reached once delivery has actually been attempted and recorded. A failure anywhere in the delivery block now leaves the record retry-safely in "booting" instead of stranding it as ready-but-never-prompted. Adds a regression test that forces client.send to reject once and asserts the record stays booting/prompt_delivered-falsy after the failed sweep, then recovers to ready/prompt_delivered:true on the next sweep. Codex's other (low-severity) finding — stuck-boot timeout window widening via updated_at refreshes from maybeCaptureBootSessionId() — is deferred; see .agent/progress.md SDLC-87 session log for rationale. # Conflicts: # src/agent-engine.ts # tests/sidebar-sync.test.ts
# Conflicts: # package-lock.json
…est coverage Re-QA (Codex Desktop, independent evaluator) returned FAIL with two gaps: 1. AC4 incomplete: screen-parser.ts handled the idle-prompt case but not a context-limit / auto-compact banner — such a screen still parsed as "working". Added CONTEXT_LIMIT_BANNER_RE (matches several known Claude Code phrasings: "Context low", "Context window is almost full", "auto-compact(ing)", "compacting conversation" — there's no single stable banner string) and check it in inferStatus() ahead of every working/thinking signal, so it can't be shadowed by a co-occurring busy-looking marker like "esc to interrupt" during an in-progress compact. Maps to "idle" (no dedicated "blocked" status exists in ParsedScreenStatus). Two new screen-parser tests: a full banner screen, and a banner co-occurring with "esc to interrupt" to prove priority ordering. 2. AC1 test gap: existing tests only exercised submit_verified: true. Added a sidebar-sync test (STUCK_INPUT_SCREEN fixture: typed text still visible at the prompt post-Enter, so screenShowsPendingInput() is true and status isn't working/thinking/done) asserting submit_verified is surfaced as false — not swallowed to null or coerced to true — on both the persisted AgentRecord and its toPublicAgent() projection, which is what callers like get_agent_state/wait_for actually see. bun run typecheck / build / test all green: 27 files, 462/462 tests (459 prior + 3 new). package-lock.json untouched. # Conflicts: # src/screen-parser.ts # tests/screen-parser.test.ts # tests/sidebar-sync.test.ts
EtanHey
force-pushed
the
sdlc-87-verify-prompt-delivery-model-arg
branch
from
August 3, 2026 16:13
5bb9529 to
376e54f
Compare
Owner
|
Maintainer rebase completed for Jared — this was wanted contributor work, so I carried the drift instead of sending 20 days of repository churn back to him. The branch is now rebased onto current The diagnosis deserves explicit credit: on 2026-07-14 Jared named all four defect classes that burned us again 20 days later — missing task delivery, ignored model selection, indefinitely booting registry rows, and idle panes reported as working. Per-problem disposition
Tests and rebase adaptations
Security assessment — PASS WITH NOTES
I have not closed or merged the PR; the lead is routing final review. |
Owner
|
Cross-lane overlap check for the tool-description defect: current main already makes normal interactive send_to calls attempt submit verification; attempted-but-unconfirmed delivery becomes submit_verified:false and returns non-ok through SubmitVerificationError. A successful submit_verified:null means verification was intentionally not attempted. PR #326 remains complementary and necessary for spawn/boot settlement, but it does not need to own generic send_to semantics; #343/current main already does. This lane will not add a second semantic mechanism. |
This was referenced Aug 11, 2026
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.
Problem
spawn_agentcan returnokand boot the CLI while silently failing in ways only a human watching the pane would catch:modelarg is ignored — the session launches under a different model than requested.bootingwith a null pid indefinitely.workingfor a session that is actually sitting idle (or at a context-limit banner).Changes
agent-engine.ts— newmaybeAdvanceBootingAgent(): promotesbooting → readyonly after verifying prompt delivery, detects requested-vs-actual model mismatch, and applies a stuck-boot timeout.screen-parser.ts— fix idle-vs-workingmisclassification; extractisSubmitVerifiedStatus/screenShowsPendingInput.agent-types.ts/agent-facade.ts/state-manager.ts— plumbsubmit_verified/prompt_delivered/parsed_model/model_mismatchthrough the agent record and public shape.server.ts— surface the new fields.sidebar-sync,screen-parser,agent-facade, including a regression test for the ordering bug wherebooting → readywas persisted before prompt-delivery was confirmed (which reintroduced the silent-prompt-loss failure).Verification
bun run typecheck✓ ·bun run build✓ ·bun run test— 27 files / 459 tests pass.Prompted by real-world use driving Claude Code / Codex sessions through cmuxlayer, where a swallowed spawn failure surfaced only because an operator happened to look at the pane.
Note
Harden
spawn_agentboot to verify prompt delivery, detect model mismatch, and report accurate ready/error statesubmit_verified,prompt_delivered,parsed_model, andmodel_mismatchfields toAgentRecordandPublicAgentto track boot settlement throughout the agent lifecycle.spawn_agentnow only promotes an agent to ready when prompt submission is verified; timeout errors preserveboot_prompt_pendingfor later reconciliation by the sweep.maybeMarkBootReadysweep errors stale managed prompts that were never verified and transitions non-interactive boots to error after a new 45sBOOT_READY_TIMEOUT_MStimeout.computeModelMismatchto detect when the live CLI model text does not match the requested model.inferStatusin screen-parser.ts so Claude sessions parked at context-limit or auto-compact banners are reported as idle instead of working.auto-*) agents are no longer advanced by the boot sweep; stale stuck-booting errors are evicted usingcreated_atinstead ofupdated_at.Macroscope summarized 376e54f.
Summary by CodeRabbit
New Features
Bug Fixes