feat: add durable delivery receipts and inbox cursors - #395
Conversation
Co-Authored-By: cmuxlayerCodex running gpt-5.6-sol <noreply@anthropic.com>
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_9d00e107-a1c3-4e8f-9eb9-bd4f243e48c0) |
📝 WalkthroughWalkthroughChangesMailbox and delivery lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant spawn_agent
participant AgentMonitor
participant AgentEngine
participant SpawnedAgent
spawn_agent->>AgentMonitor: initialize mailbox and cursor metadata
AgentMonitor->>SpawnedAgent: inject mailbox contract
spawn_agent->>AgentEngine: submit boot prompt
AgentEngine->>SpawnedAgent: guarded delivery
SpawnedAgent-->>AgentEngine: verified submission
AgentEngine-->>spawn_agent: delivery and readiness result
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: e9b1a0c345
ℹ️ 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".
| const agent = this.getAgentState(receipt.agent_id); | ||
| if (!agent || (agent.state !== "ready" && agent.state !== "idle")) { | ||
| continue; |
There was a problem hiding this comment.
When a working agent completes its current task, runSweepOnce() first transitions it to done in maybeMarkTaskDone(), and only afterward calls this drain, which accepts only ready or idle. Normal sweeps do not transition a working agent to idle, so a follow-up queued while it is busy commonly remains queued forever precisely when the TUI becomes available; eligibility should use fresh interactive-screen evidence or drain before the terminal transition.
AGENTS.md reference: AGENTS.md:L8-L16
Useful? React with 👍 / 👎.
| const data = { | ||
| accepted: true, | ||
| agent_id: agentId, | ||
| delivery_id: receipt.delivery_id, | ||
| delivery_state: receipt.delivery_state, | ||
| terminal: receipt.terminal, |
There was a problem hiding this comment.
For every send to a working agent this returns a nonterminal receipt, but repo-wide the only receipt lookup is the internal AgentEngine.getDeliveryReceipt() method, and no registered tool, resource, or notification exposes either that receipt or the delivery event log. An MCP caller therefore cannot learn whether its accepted delivery was later submitted or failed, so it cannot safely retry or depend on the message; expose a receipt-status/wait path through the tool API.
AGENTS.md reference: AGENTS.md:L11-L16
Useful? React with 👍 / 👎.
| const current = readInboxCursor(agentId, opts); | ||
| if (current) { | ||
| const currentIndex = messages.findIndex((message) => message.id === current); | ||
| if (currentIndex >= 0 && nextIndex < currentIndex) { |
There was a problem hiding this comment.
The helper accepts any later message ID, including when no cursor exists, rather than requiring that every preceding message has completed. If overlapping resumed processes handle messages out of order—or an agent simply finishes m3 before m2—writing m3 makes replayUndelivered() slice past m2, permanently suppressing an unhandled dispatch; preserve per-message acknowledgements or only advance through a contiguous handled prefix.
AGENTS.md reference: AGENTS.md:L27-L34
Useful? React with 👍 / 👎.
| try { | ||
| try { | ||
| mkdirSync(lockPath); | ||
| lockAcquired = true; |
There was a problem hiding this comment.
If the CLI process is killed or crashes after this mkdirSync() and before the finally, the lock directory remains indefinitely. Every resumed process then receives the same EEXIST retry error, so the cursor can never advance and already handled messages replay forever; store lock ownership and reclaim dead/stale locks, or use a locking primitive released by process exit.
AGENTS.md reference: AGENTS.md:L27-L34
Useful? React with 👍 / 👎.
| writeFileSync( | ||
| tempPath, | ||
| `${JSON.stringify([...this.deliveryReceipts.values()], null, 2)}\n`, | ||
| "utf8", |
There was a problem hiding this comment.
Every immediate or queued delivery remains in deliveryReceipts forever, including its full text, and each state change synchronously serializes and rewrites the entire map. On a long-running daemon this makes disk usage unbounded and turns ordinary sends into progressively larger O(total historical payload) writes; apply retention or compact terminal receipts before persisting.
Useful? React with 👍 / 👎.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| @@ -4463,6 +4512,207 @@ export class AgentEngine { | |||
|
|
|||
| async runSweep(): Promise<void> { | |||
There was a problem hiding this comment.
🟠 High src/agent-engine.ts:4513
If runSweepOnce() (or the lifecycle-mutation wrapper) rejects — e.g. due to a setStatus/setStatuses failure inside syncSidebar — drainDeliveryQueue() is never reached. Durable queued deliveries that were already accepted and persisted will remain stuck for as long as the sweep keeps failing, even when the target agent is ready/idle. Wrap the sweep in try/finally so the drain always runs.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agent-engine.ts around line 4513:
If `runSweepOnce()` (or the lifecycle-mutation wrapper) rejects — e.g. due to a `setStatus`/`setStatuses` failure inside `syncSidebar` — `drainDeliveryQueue()` is never reached. Durable queued deliveries that were already accepted and persisted will remain stuck for as long as the sweep keeps failing, even when the target agent is `ready`/`idle`. Wrap the sweep in `try/finally` so the drain always runs.
| let tempPath: string | null = null; | ||
| try { | ||
| try { | ||
| mkdirSync(lockPath); |
There was a problem hiding this comment.
🟡 Medium src/inbox.ts:385
writeInboxCursor permanently deadlocks if the process is killed between mkdirSync(lockPath) on line 385 and the finally cleanup on line 421. The orphaned lock directory survives the crash, and every subsequent call throws "Inbox cursor is locked for ${agentId}; retry" unconditionally — there is no stale-lock detection. This means handled messages can never be watermarked again, causing unbounded replay on every restart until an operator manually removes the .lock directory.
The EEXIST handler assumes the lock is always held by a live process, but a crash (or kill -9) invalidates that assumption. Consider writing the owning PID into a file inside the lock directory after acquisition, and on EEXIST, reading that PID and checking whether the process is still alive (e.g. process.kill(pid, 0)). If the owner is dead, remove the stale lock and re-acquire. Alternatively, fall back to the lock directory's mtime (via statSync) and break it after a reasonable timeout.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/inbox.ts around line 385:
`writeInboxCursor` permanently deadlocks if the process is killed between `mkdirSync(lockPath)` on line 385 and the `finally` cleanup on line 421. The orphaned lock directory survives the crash, and every subsequent call throws `"Inbox cursor is locked for ${agentId}; retry"` unconditionally — there is no stale-lock detection. This means handled messages can never be watermarked again, causing unbounded replay on every restart until an operator manually removes the `.lock` directory.
The `EEXIST` handler assumes the lock is always held by a live process, but a crash (or `kill -9`) invalidates that assumption. Consider writing the owning PID into a file inside the lock directory after acquisition, and on `EEXIST`, reading that PID and checking whether the process is still alive (e.g. `process.kill(pid, 0)`). If the owner is dead, remove the stale lock and re-acquire. Alternatively, fall back to the lock directory's `mtime` (via `statSync`) and break it after a reasonable timeout.
| } | ||
| } | ||
|
|
||
| private persistDeliveryReceipts(): void { |
There was a problem hiding this comment.
🟠 High src/agent-engine.ts:4565
Terminal delivery receipts (both resolved and drained) are never removed from deliveryReceipts. Every call to resolveDelivery, queueDelivery, or the drain loop appends to the map and then persistDeliveryReceipts rewrites the entire collection — including the full text payload of every historical receipt — as a single synchronous JSON file. On a long-running server this causes unbounded memory and disk growth, and each write gets progressively slower.
Consider pruning terminal receipts (e.g. receipts where terminal === true and resolved_at is older than a retention window) before persisting, or cap the number of retained receipts.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agent-engine.ts around line 4565:
Terminal delivery receipts (both resolved and drained) are never removed from `deliveryReceipts`. Every call to `resolveDelivery`, `queueDelivery`, or the drain loop appends to the map and then `persistDeliveryReceipts` rewrites the entire collection — including the full `text` payload of every historical receipt — as a single synchronous JSON file. On a long-running server this causes unbounded memory and disk growth, and each write gets progressively slower.
Consider pruning terminal receipts (e.g. receipts where `terminal === true` and `resolved_at` is older than a retention window) before persisting, or cap the number of retained receipts.
| this.deliveryReceipts.set(receipt.delivery_id, receipt); | ||
| try { | ||
| // Acceptance is not returned until the full replay payload is durable. | ||
| this.persistDeliveryReceipts(); |
There was a problem hiding this comment.
🟠 High src/agent-engine.ts:4601
persistDeliveryReceipts() serializes the entire in-memory deliveryReceipts map via atomic rename without first reading and merging the current file contents. When two AgentEngine instances share the same deliveryReceiptsPath (same state directory), the second writer's rename silently overwrites the first writer's receipt. The first caller already received an accepted receipt (the method returned successfully with delivery_state: "queued"), but its payload is permanently lost on disk and will never be drained after a restart.
The root cause is that persistDeliveryReceipts is a blind full-file replace with no read-merge step and no inter-process lock. Consider either (a) reloading the file and merging unknown entries before writing, ideally under an advisory file lock, or (b) using an append-only format (one JSON line per receipt) so concurrent writers cannot destroy each other's data.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agent-engine.ts around line 4601:
`persistDeliveryReceipts()` serializes the entire in-memory `deliveryReceipts` map via atomic rename without first reading and merging the current file contents. When two `AgentEngine` instances share the same `deliveryReceiptsPath` (same state directory), the second writer's rename silently overwrites the first writer's receipt. The first caller already received an accepted receipt (the method returned successfully with `delivery_state: "queued"`), but its payload is permanently lost on disk and will never be drained after a restart.
The root cause is that `persistDeliveryReceipts` is a blind full-file replace with no read-merge step and no inter-process lock. Consider either (a) reloading the file and merging unknown entries before writing, ideally under an advisory file lock, or (b) using an append-only format (one JSON line per receipt) so concurrent writers cannot destroy each other's data.
| const cursor = readFileSync(inboxCursorPath(agentId, opts), "utf8").trim(); | ||
| return cursor.length > 0 ? cursor : null; |
There was a problem hiding this comment.
🟡 Medium src/inbox.ts:362
readInboxCursor returns a .trim()-ed cursor value, but writeInboxCursor writes the raw messageId (plus a trailing newline). If a message ID contains leading or trailing whitespace (allowed by DispatchInput.id), readInboxCursor returns a different string than what was written. replayUndelivered then fails to find the trimmed cursor in the inbox and replays the entire message list, duplicating already-handled work. Strip only the trailing newline that writeInboxCursor appends instead of calling .trim().
| const cursor = readFileSync(inboxCursorPath(agentId, opts), "utf8").trim(); | |
| return cursor.length > 0 ? cursor : null; | |
| const raw = readFileSync(inboxCursorPath(agentId, opts), "utf8"); | |
| const cursor = raw.endsWith("\n") ? raw.slice(0, -1) : raw; | |
| return cursor.length > 0 ? cursor : null; |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/inbox.ts around lines 362-363:
`readInboxCursor` returns a `.trim()`-ed cursor value, but `writeInboxCursor` writes the raw `messageId` (plus a trailing newline). If a message ID contains leading or trailing whitespace (allowed by `DispatchInput.id`), `readInboxCursor` returns a different string than what was written. `replayUndelivered` then fails to find the trimmed cursor in the inbox and replays the entire message list, duplicating already-handled work. Strip only the trailing newline that `writeInboxCursor` appends instead of calling `.trim()`.
| let timeout: ReturnType<typeof setTimeout> | null = null; | ||
| const result = await Promise.race([ | ||
| this.deliverySubmitter(receipt), | ||
| new Promise<never>((_resolve, reject) => { |
There was a problem hiding this comment.
🟠 High src/agent-engine.ts:4649
drainDeliveryQueue checks this.deliverySubmitter only once at entry, then awaits multiple submissions in a loop using the mutable field directly. If setDeliverySubmitter(null) is called while a submission is in flight, the next loop iteration calls null(receipt), throws a TypeError, and permanently records that never-attempted delivery as failed. Capture the submitter reference before entering the loop, or re-check it before each invocation.
let timeout: ReturnType<typeof setTimeout> | null = null;
+ const submitter = this.deliverySubmitter;
+ if (!submitter) break;
const result = await Promise.race([
- this.deliverySubmitter(receipt),
+ submitter(receipt),🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agent-engine.ts around lines 4649-4652:
`drainDeliveryQueue` checks `this.deliverySubmitter` only once at entry, then awaits multiple submissions in a loop using the mutable field directly. If `setDeliverySubmitter(null)` is called while a submission is in flight, the next loop iteration calls `null(receipt)`, throws a `TypeError`, and permanently records that never-attempted delivery as `failed`. Capture the submitter reference before entering the loop, or re-check it before each invocation.
| try { | ||
| for (const receipt of this.deliveryReceipts.values()) { | ||
| if (receipt.delivery_state !== "queued") continue; | ||
| const agent = this.getAgentState(receipt.agent_id); |
There was a problem hiding this comment.
🟠 High src/agent-engine.ts:4640
drainDeliveryQueue looks up the agent via this.getAgentState(receipt.agent_id), which returns null after a restart if the agent was renamed (e.g. pending → final ID during session capture). In-memory registry aliases resolve the old ID within the same process, but reconstitute clears aliases on restart, so durable queued receipts referencing the old agent_id are silently skipped forever.
transferAgentRenameMemory rekeys every other in-memory map (readyPatternMatches, cliExitShellMatches, fleetScreenProgress, etc.) but does not update deliveryReceipts. Add rekeying of delivery receipts (updating each receipt's agent_id field) in transferAgentRenameMemory and call persistDeliveryReceipts() so the renamed ID survives a restart.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agent-engine.ts around line 4640:
`drainDeliveryQueue` looks up the agent via `this.getAgentState(receipt.agent_id)`, which returns `null` after a restart if the agent was renamed (e.g. pending → final ID during session capture). In-memory registry aliases resolve the old ID within the same process, but `reconstitute` clears aliases on restart, so durable queued receipts referencing the old `agent_id` are silently skipped forever.
`transferAgentRenameMemory` rekeys every other in-memory map (`readyPatternMatches`, `cliExitShellMatches`, `fleetScreenProgress`, etc.) but does not update `deliveryReceipts`. Add rekeying of delivery receipts (updating each receipt's `agent_id` field) in `transferAgentRenameMemory` and call `persistDeliveryReceipts()` so the renamed ID survives a restart.
| }; | ||
| throw error; | ||
| } | ||
| const receipt = engine.resolveDelivery({ |
There was a problem hiding this comment.
🟠 High src/server.ts:12273
If engine.resolveDelivery(...) throws after deliverAgentInput has already succeeded (e.g. disk-full during persistDeliveryReceipts()), the exception propagates to the outer catch, which returns an error response with an empty failedReceiptPayload. The caller sees ok: false with no delivery receipt, even though the text was already submitted to the terminal. A caller that retries the apparently-failed send will deliver the input a second time.
The failedReceiptPayload assignment on line 12287 happens after resolveDelivery, so a persistence failure means the success-path receipt identity is never recorded. Move the failedReceiptPayload assignment to before the resolveDelivery call and wrap resolveDelivery in a try-catch that continues on the success path, so a post-delivery persistence failure cannot mask a delivered terminal mutation as a retryable error.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/server.ts around line 12273:
If `engine.resolveDelivery(...)` throws after `deliverAgentInput` has already succeeded (e.g. disk-full during `persistDeliveryReceipts()`), the exception propagates to the outer `catch`, which returns an error response with an empty `failedReceiptPayload`. The caller sees `ok: false` with no delivery receipt, even though the text was already submitted to the terminal. A caller that retries the apparently-failed send will deliver the input a second time.
The `failedReceiptPayload` assignment on line 12287 happens *after* `resolveDelivery`, so a persistence failure means the success-path receipt identity is never recorded. Move the `failedReceiptPayload` assignment to *before* the `resolveDelivery` call and wrap `resolveDelivery` in a try-catch that continues on the success path, so a post-delivery persistence failure cannot mask a delivered terminal mutation as a retryable error.
| for (const receipt of this.deliveryReceipts.values()) { | ||
| if (receipt.delivery_state !== "queued") continue; | ||
| const agent = this.getAgentState(receipt.agent_id); | ||
| if (!agent || (agent.state !== "ready" && agent.state !== "idle")) { |
There was a problem hiding this comment.
🟠 High src/agent-engine.ts:4641
Queued delivery receipts for agents that are missing or in a terminal state (done/error) are silently skipped on every drain cycle but never resolved. If the target agent transitions from working directly to done/error (or is removed) without ever re-entering ready/idle, the receipt stays durably queued and nonterminal forever — callers waiting on resolution never receive either a success or failure.
The continue at line 4642 treats a missing/terminal agent the same as a transiently non-interactive one. The fix is to fail the receipt terminally when the agent is gone or terminal, and only skip when the agent is in a transient non-interactive state.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agent-engine.ts around line 4641:
Queued delivery receipts for agents that are missing or in a terminal state (`done`/`error`) are silently skipped on every drain cycle but never resolved. If the target agent transitions from `working` directly to `done`/`error` (or is removed) without ever re-entering `ready`/`idle`, the receipt stays durably `queued` and nonterminal forever — callers waiting on resolution never receive either a success or failure.
The `continue` at line 4642 treats a missing/terminal agent the same as a transiently non-interactive one. The fix is to fail the receipt terminally when the agent is gone or terminal, and only skip when the agent is in a transient non-interactive state.
Inject the mailbox cursor contract into every agent boot and retry known pre-mutation delivery rejections with capped backoff. Co-Authored-By: cmuxlayerCodex running gpt-5.6-sol <noreply@anthropic.com>
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_bd9f97c4-2f32-4e0f-ad07-456c54324c1e) |
|
Addressed both HIGH findings in 937c827.
Ready for re-review. — cmuxlayerCodex (gpt-5.6-sol) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 937c827797
ℹ️ 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".
| `cmuxlayer mailbox contract for ${agentId}: monitor with ${monitorBoot.monitor_command}; ` + | ||
| `after each handled message run CMUX_INBOX_MSG_ID=<handled-message-id> ${monitorBoot.cursor_update_command}`; |
There was a problem hiding this comment.
A freshly spawned recipient that follows this injected contract never proves its monitor is armed: ensureMonitorBoot() writes only a server_boot heartbeat, recommendedMonitorCommand() merely tails the file, and inbox-cursor does not call writeHeartbeat(). Since inboxMonitorState() deliberately considers only agent-authored heartbeats, every subsequent dispatch reports inbox_monitor_never_armed and falls back to a raw nudge even while the monitor is running; the arm/cursor path must publish and refresh an agent heartbeat.
AGENTS.md reference: AGENTS.md:L11-L16
Useful? React with 👍 / 👎.
| : "" | ||
| }cmuxlayer inbox-cursor ${shellQuote(agentId)}`; | ||
| try { | ||
| monitorCommand = recommendedMonitorCommand(agentId, inboxOpts); |
There was a problem hiding this comment.
For Codex or Cursor agents, this unconditionally supplies recommendedMonitorCommand() (tail -n0 -F) even though the repository's harness policy states those CLIs have no native asynchronous Monitor and must call replayUndelivered() on each turn. The injected prompt contains no replay/poll instruction, so a looping Codex/Cursor agent following it will not consume queued inbox messages unless the raw-terminal nudge happens to succeed; select the bootstrap contract using the agent's CLI and include the poll-on-turn path for non-Monitor harnesses.
AGENTS.md reference: AGENTS.md:L44-L47
Useful? React with 👍 / 👎.
| }; | ||
| } | ||
| }; | ||
| const mailboxBootContract = ( |
There was a problem hiding this comment.
The new bootstrap helper is only invoked by spawn_agent and spawn_in_workspace; the repo-wide new_worktree_split path at src/server.ts:10230-10343 still creates a formal worker without calling ensureMonitorBoot() or injecting the cursor contract. Agents created through that public spawn tool therefore remain permanently unarmed for the new inbox protocol, so this initialization should be centralized or added to that path as well.
AGENTS.md reference: AGENTS.md:L8-L9
Useful? React with 👍 / 👎.
| inboxBaseDir | ||
| ? `CMUXLAYER_INBOX_BASE_DIR=${shellQuote(inboxBaseDir)} ` | ||
| : "" | ||
| }cmuxlayer inbox-cursor ${shellQuote(agentId)}`; |
There was a problem hiding this comment.
When cmuxlayer is run from a source checkout via the documented npm run dev or npm start, or launched through a package runner rather than installed globally, the spawned agent's workspace need not have a cmuxlayer executable on PATH. In those installations the exact command injected here fails after every handled message, so the durable cursor never advances; construct the command from the currently running executable/entrypoint or expose an invocation that does not assume a global launcher.
AGENTS.md reference: AGENTS.md:L49-L49
Useful? React with 👍 / 👎.
| return receipt ? { ...receipt } : null; | ||
| } | ||
|
|
||
| async drainDeliveryQueue(): Promise<void> { |
There was a problem hiding this comment.
🟠 High src/agent-engine.ts:4646
drainDeliveryQueue processes queued receipts sequentially with a per-item timeout, so a drain with N hung submissions blocks runSweep for N * deliverySubmitTimeoutMs — e.g. 100 stuck receipts stall the lifecycle sweep for ~50 minutes at the default 30s timeout. The deliverySubmitTimeoutMs bound only limits each item, not the whole drain, so an unbounded queue of hung submissions can halt all periodic reconciliation and lifecycle work. Consider bounding the total drain time or processing receipts concurrently so one slow queue cannot block the sweep indefinitely.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agent-engine.ts around line 4646:
`drainDeliveryQueue` processes queued receipts sequentially with a per-item timeout, so a drain with N hung submissions blocks `runSweep` for `N * deliverySubmitTimeoutMs` — e.g. 100 stuck receipts stall the lifecycle sweep for ~50 minutes at the default 30s timeout. The `deliverySubmitTimeoutMs` bound only limits each item, not the whole drain, so an unbounded queue of hung submissions can halt all periodic reconciliation and lifecycle work. Consider bounding the total drain time or processing receipts concurrently so one slow queue cannot block the sweep indefinitely.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 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 4576-4589: Update delivery-receipt lifecycle handling around
persistDeliveryReceipts and drainDeliveryQueue: retain only bounded-age or
bounded-count receipts, remove terminal receipts when they exceed retention, and
clear or omit AgentDeliveryReceipt.text when a receipt becomes terminal. Batch
queue mutations so drainDeliveryQueue invokes persistence once per drain rather
than once per receipt, while preserving atomic file replacement.
- Around line 4523-4526: Update the stop cleanup path used by runSweep and
stopAgent so the route close operation receives stableSurfaceIdentity set to
route.surface_uuid, matching deliverAgentInput’s uuid-based lock key. Keep
drainDeliveryQueue() outside runLifecycleMutation(); do not change the delivery
queue ordering or lifecycle lock behavior.
In `@src/inbox.ts`:
- Around line 439-447: The unresolvable persisted cursor must be handled
consistently across replay and cursor advancement. In src/inbox.ts lines
439-447, update replayUndelivered to use ack-based filtering when cursorIndex is
-1 rather than returning all messages; in src/inbox.ts lines 398-418, update
writeInboxCursor to reject advances when currentIndex is -1 instead of bypassing
the backwards-move check.
- Around line 398-418: Update the cursor validation around readInboxCursor so an
existing current cursor whose ID is absent from messages is rejected or
preserved instead of allowing advancement. Keep the existing backwards-movement
check for located cursors, while ensuring unknown persisted cursors cannot move
to a new message.
- Around line 384-396: Update writeInboxCursor’s lock acquisition and cleanup to
write owner metadata, detect stale locks using a bounded age policy, and remove
a lock only when ownership can be established for the current writer. Preserve
the existing retry error for active locks, ensure stale locks can be recovered
safely, and add a regression test covering recovery from a stale lock.
In `@src/index.ts`:
- Around line 95-102: Update main’s inbox-cursor command flow around
writeInboxCursor to catch its failures instead of allowing the top-level handler
to print a stack; write exactly one line to stderr for each failure and exit
with the distinct retry status only when the error is specifically the
cursor-lock error, using its error code/type or exact lock-error check rather
than matching “locked” in the message.
In `@src/server.ts`:
- Around line 10709-10710: Update the ready-transition condition in the handler
around engine.getAgentState so it requires the recorded
bootPromptDelivery.submit_verified value to be true in addition to hasPrompt,
matching the verification gate used by spawn_agent. Keep the agent booting when
verification is null or false.
- Around line 5101-5105: Normalize prompt_text in the SubmitVerificationError
recovery return path to use the same hasInlinePrompt(rawPrompt) ? rawPrompt :
null logic as the success return path. Update the fallback return site near the
existing delivery object while preserving the current prompt_warning and other
delivery fields.
- Around line 12266-12296: Update the catch block around deliverAgentInput so
RetryableDeliveryError resolves the receipt as a queued, non-terminal delivery
while preserving its retry metadata and replay eligibility. Keep the existing
terminal failed-resolution behavior for non-retryable errors and preserve
SubmitVerificationError event handling.
- Around line 12245-12253: Update the queued send_to receipt response to use
okFormatted(...) instead of the hand-built content and structuredContent object.
Preserve the delivery receipt data while returning the standard ok and
retry_count fields, and remove the accepted-specific schema field.
In `@tests/agent-engine.test.ts`:
- Line 11167: Replace the direct private-field assignment in the test with a
dedicated AgentEngine constructed using the public deliverySubmitTimeoutMs
option. Ensure the test exercises AgentEngineOptions through normal construction
and remove the `(engine as any).deliverySubmitTimeoutMs` mutation.
In `@tests/inbox.test.ts`:
- Around line 225-235: Expand the writeInboxCursor tests around the existing
cursor-lock case to cover rejection of an unknown message id, rejection of a
backwards cursor move, and replay when the cursor id is absent from the inbox
file. Use the existing inbox setup and helpers, assert the documented errors for
the two rejection paths, and verify the absent-id replay returns the full
retained history.
In `@tests/pointer-discipline.test.ts`:
- Around line 120-123: Stop resetting submissionObservationPending in the
list-workspaces handling path, since topology collection may invoke it before
submission verification completes. Clear the flag only through an explicit test
signal or after a bounded read-screen count, while preserving the caller-screen
transition until verification has finished.
In `@tests/server-agent-tools.test.ts`:
- Around line 8234-8236: Update the “nothing was sent” assertion around mockExec
calls to also reject buffer-based delivery, checking for both set-buffer and
paste-buffer commands in addition to send. Preserve the existing zero-call
expectation for all pane-delivery paths.
- Around line 7057-7066: Extend the test around the existing failed delivery
assertions to verify the duplicate-event guard: using failed.delivery_id,
inspect the event log and assert exactly one matching terminal failure event
exists after the submission-verification failure. Keep the existing delivery
receipt and failure-state assertions unchanged.
In `@tests/spawn-workspace.test.ts`:
- Around line 146-152: Update the sendKey mock in the spawn workspace test to
mark a surface submitted only when a Return is delivered after the
mailbox-contract text has been delivered, rather than when the Return count
reaches two. Reuse the existing contract-delivery tracking pattern from
spawn-monitor-boot.test.ts and preserve the current returnCount tracking only
where needed for setup or assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6bca9ab3-80f6-4f48-aa82-b276a50d2b6d
📒 Files selected for processing (14)
docs/plans/2026-08-11-pr395-review-fixes.mdsrc/agent-engine.tssrc/agent-types.tssrc/inbox.tssrc/index.tssrc/server.tstests/agent-engine.test.tstests/inbox-nudge.test.tstests/inbox.test.tstests/pointer-discipline.test.tstests/server-agent-tools.test.tstests/server.test.tstests/spawn-monitor-boot.test.tstests/spawn-workspace.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Macroscope - Correctness Check
🧰 Additional context used
🧠 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/pointer-discipline.test.tstests/inbox-nudge.test.tstests/inbox.test.tstests/spawn-workspace.test.tstests/spawn-monitor-boot.test.tstests/agent-engine.test.tstests/server.test.tstests/server-agent-tools.test.ts
🔇 Additional comments (34)
docs/plans/2026-08-11-pr395-review-fixes.md (1)
1-13: LGTM!src/inbox.ts (1)
23-33: LGTM!Also applies to: 119-121, 356-367
src/index.ts (1)
22-22: LGTM!Also applies to: 41-43
tests/inbox.test.ts (1)
5-5: LGTM!Also applies to: 19-19, 30-32, 210-223
src/agent-engine.ts (3)
6-15: LGTM!Also applies to: 58-58, 165-197, 427-428, 952-972, 4622-4644, 4729-4754
4591-4620: LGTM!
4646-4667: 🩺 Stability & AvailabilityConfirm the delivery retry scheduler.
drainDeliveryQueue()runs only fromrunSweep(). A retryable failure setsnext_attempt_at, but no timer targets that deadline. With default 5-second active and 15-second idle sweeps, retry delays are rounded up to the next sweep. Schedule a timer if retries must follow the backoff deadline; otherwise document that sweep cadence controls retries.tests/agent-engine.test.ts (2)
21-21: LGTM!Also applies to: 11133-11155, 11187-11264, 11266-11307
11316-11319: 🎯 Functional CorrectnessNo change needed.
TEST_DIRis reset inbeforeEach, and this test queues one delivery before reading the receipt.persisted[0]is the receipt under test.> Likely an incorrect or invalid review comment.src/agent-types.ts (1)
191-195: LGTM!src/server.ts (8)
32-32: LGTM!Also applies to: 49-49, 115-115, 2237-2241, 2523-2523
4207-4207: LGTM!Also applies to: 4335-4343
9333-9333: LGTM!Also applies to: 9457-9461, 9508-9508
9540-9549: LGTM!
9844-9844: LGTM!Also applies to: 9910-9915, 9932-9947, 9982-9982, 10075-10075
10634-10634: LGTM!Also applies to: 10656-10680, 10749-10760, 10769-10769
12115-12115: LGTM!Also applies to: 12125-12133, 12297-12342
2902-2907: 🗄️ Data Integrity & IntegrationThe environment variable name is correct.
inbox-cursorreadsCMUXLAYER_INBOX_BASE_DIR, so the configured directory is preserved.> Likely an incorrect or invalid review comment.tests/server-agent-tools.test.ts (5)
103-135: LGTM!Also applies to: 148-148, 164-166, 235-235, 251-251
648-649: LGTM!Also applies to: 674-693
2639-2645: LGTM!Also applies to: 2740-2750, 2829-2840, 2861-2873, 2948-2949, 2961-2962, 3904-3910, 4015-4017, 4033-4033, 4095-4101, 4163-4165, 4181-4181
33-33: LGTM!Also applies to: 8198-8233, 8237-8271
1945-1945: LGTM!Also applies to: 2053-2053, 3694-3694, 4986-4986, 5802-5802, 8420-8465
tests/inbox-nudge.test.ts (1)
64-64: LGTM!Also applies to: 161-179
tests/pointer-discipline.test.ts (1)
53-53: LGTM!Also applies to: 74-92, 104-112
tests/server.test.ts (3)
1150-1152: LGTM!Also applies to: 1188-1218, 1242-1242, 1260-1283, 1291-1292
8110-8128: LGTM!Also applies to: 8197-8203, 8264-8269, 8414-8415, 8431-8432, 8470-8471, 8585-8586, 8597-8599, 8659-8664, 8771-8786, 8867-8867, 8960-8975, 9044-9049
9131-9131: LGTM!Also applies to: 9456-9456, 9686-9686
tests/spawn-monitor-boot.test.ts (3)
19-32: LGTM!Also applies to: 91-97
127-135: LGTM!
184-186: LGTM!Also applies to: 197-197, 207-223, 266-268
tests/spawn-workspace.test.ts (3)
79-81: LGTM!Also applies to: 94-123
138-145: LGTM!Also applies to: 153-166
242-258: LGTM!Also applies to: 275-281
| async runSweep(): Promise<void> { | ||
| await this.runLifecycleMutation(() => this.runSweepOnce()); | ||
| await this.drainDeliveryQueue(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find the delivery submitter registration and its mutation path.
rg -n -C15 'setDeliverySubmitter|deliverAgentInput' src --glob '*.ts'Repository: EtanHey/cmuxlayer
Length of output: 21881
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- lifecycle lock and delivery methods ---'
rg -n -C35 'runLifecycleMutation|drainDeliveryQueue|deliverySubmitter|deliveryDrainInFlight' src/agent-engine.ts
printf '%s\n' '--- server delivery implementation ---'
sed -n '9327,9549p' src/server.ts
printf '%s\n' '--- lifecycle mutation call sites ---'
rg -n -C8 'runLifecycleMutation\(' src --glob '*.ts'Repository: EtanHey/cmuxlayer
Length of output: 32543
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- delivery metadata refresh path ---'
rg -n -C20 'refreshManagedMetadataBestEffort|lifecycleRefreshManagedMetadata' src/server.ts src/agent-engine.ts
printf '%s\n' '--- lifecycle mutation implementations and terminal operations ---'
rg -n -C12 'stopAgent|recoverCrashedAgents|reconcileRolePlacements|closeSurface|bind.*Surface|withSurfaceWrite' src/agent-engine.ts src/server.ts
printf '%s\n' '--- write serialization implementation ---'
rg -n -C25 'function withSurfaceWrite|const withSurfaceWrite|withSurfaceWrite =' src --glob '*.ts'Repository: EtanHey/cmuxlayer
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- refresh lock and delivery write lock ---'
sed -n '2938,2964p' src/server.ts
sed -n '9137,9155p' src/server.ts
rg -n -C30 'withSurfaceWrite' src/server.ts src/agent-engine.ts
printf '%s\n' '--- direct lifecycle mutation wrappers near relevant tools ---'
rg -n -C10 'runLifecycleMutation' src/server.ts
printf '%s\n' '--- sweep startup and scheduling ---'
rg -n -C20 'startSweep|runSweep\(' src/agent-engine.ts src/server.tsRepository: EtanHey/cmuxlayer
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- surface-write lock implementation ---'
sed -n '3310,3575p' src/server.ts
printf '%s\n' '--- lifecycle surface mutation calls ---'
rg -n -C14 'this\.client\.(closeSurface|moveSurface|newSplit)|reconcileRolePlacements\(' src/agent-engine.ts
printf '%s\n' '--- stable identity propagation in recovery/reflow ---'
rg -n -C10 'stableSurfaceIdentity|surface_uuid' src/agent-engine.ts | head -240Repository: EtanHey/cmuxlayer
Length of output: 31403
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- stopAgent implementation and callers ---'
sed -n '6200,6355p' src/agent-engine.ts
rg -n -C20 '"stop_agent"|stopAgent\(' src/server.ts src/agent-engine.ts
printf '%s\n' '--- crash recovery close/write options ---'
sed -n '3144,3330p' src/agent-engine.ts
printf '%s\n' '--- lifecycle scheduler and direct sweep callers ---'
rg -n -C18 'startSweep|runSweep\(' src/agent-engine.ts src/server.tsRepository: EtanHey/cmuxlayer
Length of output: 32145
🏁 Script executed:
#!/bin/bash
set -eu
node - <<'JS'
"use strict";
// Model the lock semantics shown by withSurfaceWrite and runLifecycleMutation.
// acquireSurfaceWrite rejects only an identical key; different keys do not
// serialize. The lifecycle tail is a non-reentrant FIFO lock.
const active = new Map();
function acquire(key, owner) {
if (active.has(key)) throw new Error(`conflict:${key}`);
active.set(key, owner);
}
function release(key, owner) {
if (active.get(key) === owner) active.delete(key);
}
acquire("uuid:agent-1", "delivery");
let overlap = false;
try {
acquire("surface-ref-1", "stop-close");
overlap = true;
} finally {
release("surface-ref-1", "stop-close");
release("uuid:agent-1", "delivery");
}
let released = false;
let nestedCompleted = false;
const outer = Promise.resolve().then(async () => {
const nestedWait = new Promise((resolve) => {
if (released) resolve();
else setTimeout(() => resolve(), 0);
});
await Promise.race([
nestedWait.then(() => { nestedCompleted = true; }),
new Promise((resolve) => setTimeout(resolve, 10)),
]);
released = true;
});
outer.then(() => {});
setTimeout(() => {
if (!overlap) throw new Error("expected distinct surface keys to overlap");
if (nestedCompleted) throw new Error("nested lock unexpectedly completed");
console.log(JSON.stringify({
distinct_surface_keys_overlap: overlap,
nested_lifecycle_lock_waits: !nestedCompleted,
}));
}, 20);
JSRepository: EtanHey/cmuxlayer
Length of output: 481
🏁 Script executed:
#!/bin/bash
set -eu
node - <<'JS'
"use strict";
const active = new Map();
function acquire(key, owner) {
if (active.has(key)) throw new Error(`conflict:${key}`);
active.set(key, owner);
}
function release(key, owner) {
if (active.get(key) === owner) active.delete(key);
}
acquire("uuid:agent-1", "delivery");
let distinctKeysOverlap = false;
try {
acquire("surface-ref-1", "stop-close");
distinctKeysOverlap = true;
} finally {
release("surface-ref-1", "stop-close");
release("uuid:agent-1", "delivery");
}
let tail = Promise.resolve();
function runLifecycleMutation(operation) {
const previous = tail;
let release;
tail = new Promise((resolve) => { release = resolve; });
return previous.then(async () => {
try {
return await operation();
} finally {
release();
}
});
}
let nestedCompleted = false;
const outer = runLifecycleMutation(async () => {
await Promise.race([
runLifecycleMutation(async () => {
nestedCompleted = true;
}),
new Promise((resolve) => setTimeout(resolve, 20)),
]);
});
await outer;
console.log(JSON.stringify({
distinct_surface_keys_overlap: distinctKeysOverlap,
nested_lifecycle_lock_pending_during_outer_operation: !nestedCompleted,
}));
JSRepository: EtanHey/cmuxlayer
Length of output: 255
🏁 Script executed:
#!/bin/bash
set -eu
node - <<'JS'
"use strict";
let tail = Promise.resolve();
function runLifecycleMutation(operation) {
const previous = tail;
let release;
tail = new Promise((resolve) => { release = resolve; });
return previous.then(async () => {
try {
return await operation();
} finally {
release();
}
});
}
let nestedCompleted = false;
let nestedCompletedWhileOuterHeld = false;
let outerReleased = false;
let observedWhileOuterHeld = false;
const outer = runLifecycleMutation(async () => {
void runLifecycleMutation(async () => {
nestedCompleted = true;
if (!outerReleased) nestedCompletedWhileOuterHeld = true;
});
await new Promise((resolve) => setTimeout(resolve, 10));
observedWhileOuterHeld = !nestedCompleted;
});
await outer;
outerReleased = true;
await new Promise((resolve) => setTimeout(resolve, 0));
console.log(JSON.stringify({
nested_lock_pending_while_outer_held: observedWhileOuterHeld,
nested_completed_while_outer_held: nestedCompletedWhileOuterHeld,
nested_completed_after_outer_release: nestedCompleted,
}));
JSRepository: EtanHey/cmuxlayer
Length of output: 286
🏁 Script executed:
#!/bin/bash
set -eu
node - <<'JS'
"use strict";
let tail = Promise.resolve();
let outerReleased = false;
function runLifecycleMutation(id, operation) {
const previous = tail;
let release;
tail = new Promise((resolve) => { release = resolve; });
return previous.then(async () => {
try {
return await operation();
} finally {
if (id === "outer") outerReleased = true;
release();
}
});
}
let nestedCompleted = false;
let nestedCompletedWhileOuterHeld = false;
let observedWhileOuterHeld = false;
const outer = runLifecycleMutation("outer", async () => {
void runLifecycleMutation("nested", async () => {
nestedCompleted = true;
if (!outerReleased) nestedCompletedWhileOuterHeld = true;
});
await new Promise((resolve) => setTimeout(resolve, 10));
observedWhileOuterHeld = !nestedCompleted;
});
await outer;
await new Promise((resolve) => setTimeout(resolve, 0));
console.log(JSON.stringify({
nested_lock_pending_while_outer_held: observedWhileOuterHeld,
nested_completed_while_outer_held: nestedCompletedWhileOuterHeld,
nested_completed_after_outer_release: nestedCompleted && outerReleased,
}));
JSRepository: EtanHey/cmuxlayer
Length of output: 287
Use the same surface-write lock key for delivery and stop cleanup.
deliverAgentInput() locks uuid:${surface_uuid}. stopAgent() closes the route without stableSurfaceIdentity, so it locks the mutable surface reference instead. These keys can overlap on the same surface.
Do not move drainDeliveryQueue() inside runLifecycleMutation(). Its metadata refresh uses the non-reentrant lifecycle lock and can block until the delivery timeout. Pass stableSurfaceIdentity: route.surface_uuid to the stop close path.
🤖 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 4523 - 4526, Update the stop cleanup path
used by runSweep and stopAgent so the route close operation receives
stableSurfaceIdentity set to route.surface_uuid, matching deliverAgentInput’s
uuid-based lock key. Keep drainDeliveryQueue() outside runLifecycleMutation();
do not change the delivery queue ordering or lifecycle lock behavior.
| private persistDeliveryReceipts(): void { | ||
| mkdirSync(dirname(this.deliveryReceiptsPath), { recursive: true }); | ||
| const tempPath = `${this.deliveryReceiptsPath}.${process.pid}.${randomUUID()}.tmp`; | ||
| try { | ||
| writeFileSync( | ||
| tempPath, | ||
| `${JSON.stringify([...this.deliveryReceipts.values()], null, 2)}\n`, | ||
| "utf8", | ||
| ); | ||
| renameSync(tempPath, this.deliveryReceiptsPath); | ||
| } finally { | ||
| if (existsSync(tempPath)) unlinkSync(tempPath); | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Delivery receipts are never pruned, and the whole file is rewritten on every mutation.
Three consequences follow from the same root cause:
this.deliveryReceiptsonly ever grows. No code path deletes a terminal receipt. For a long-lived daemon,delivery-receipts.jsongrows without bound.persistDeliveryReceiptsserializes the entire map with 2-space indentation on every change.drainDeliveryQueuecalls it at least twice per queued receipt. Cost per drain is O(total receipts × queued receipts), and every sweep re-scans all historical receipts at Line 4650.AgentDeliveryReceipt.textstores the full delivery payload. Agent-to-agent message bodies are retained on disk in plaintext for the lifetime of the state directory.
Add retention: drop terminal receipts after a bounded age or count, and truncate or omit text once a receipt becomes terminal. Batch the persist calls in drainDeliveryQueue so one drain writes once.
🤖 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 4576 - 4589, Update delivery-receipt
lifecycle handling around persistDeliveryReceipts and drainDeliveryQueue: retain
only bounded-age or bounded-count receipts, remove terminal receipts when they
exceed retention, and clear or omit AgentDeliveryReceipt.text when a receipt
becomes terminal. Batch queue mutations so drainDeliveryQueue invokes
persistence once per drain rather than once per receipt, while preserving atomic
file replacement.
| try { | ||
| mkdirSync(lockPath); | ||
| lockAcquired = true; | ||
| } catch (error) { | ||
| if ( | ||
| error instanceof Error && | ||
| "code" in error && | ||
| error.code === "EEXIST" | ||
| ) { | ||
| throw new Error(`Inbox cursor is locked for ${agentId}; retry`); | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for any cleanup of inbox cursor lock directories.
rg -n -C4 'inbox\.cursor|cursor\.lock|\.lock' --glob '*.ts' -g '!**/node_modules/**'Repository: EtanHey/cmuxlayer
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- inbox.ts structure ---'
ast-grep outline src/inbox.ts
printf '%s\n' '--- lock-related code ---'
rg -n -C8 'lockPath|mkdirSync|writeInboxCursor|replayUndelivered|rmSync' src/inbox.ts
printf '%s\n' '--- relevant file references ---'
rg -n -C3 'writeInboxCursor|replayUndelivered|cursor.*lock|lockPath|Inbox cursor is locked' --glob '*.ts' --glob '!**/node_modules/**'Repository: EtanHey/cmuxlayer
Length of output: 9763
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- writeInboxCursor implementation ---'
sed -n '348,428p' src/inbox.ts
printf '%s\n' '--- all lock and cursor references ---'
rg -n -C5 'inboxCursorPath|writeInboxCursor|readInboxCursor|\.lock|Inbox cursor is locked' --glob '!**/node_modules/**' .
printf '%s\n' '--- tests and package scripts ---'
git ls-files | rg '(^|/)(test|tests|__tests__|spec)(/|\\.)|package.json$'
if [ -f package.json ]; then sed -n '1,180p' package.json; fiRepository: EtanHey/cmuxlayer
Length of output: 26002
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- inbox cursor tests ---'
sed -n '190,250p' tests/inbox.test.ts
printf '%s\n' '--- source-only lock lifecycle verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
source = Path("src/inbox.ts").read_text()
fn = source[source.index("export function writeInboxCursor"):source.index("/** Set of message ids", source.index("export function writeInboxCursor"))]
checks = {
"lock path is derived from cursor path": 'const lockPath = `${path}.lock`;' in fn,
"lock acquisition uses mkdirSync": "mkdirSync(lockPath);" in fn,
"existing lock always throws": 'error.code === "EEXIST"' in fn and
'throw new Error(`Inbox cursor is locked for ${agentId}; retry`);' in fn,
"cleanup is conditional on current process acquisition":
'if (lockAcquired) rmSync(lockPath, { recursive: true, force: true });' in fn,
"no owner or stale-lock metadata": not any(
token in fn for token in ("process.pid", "Date.now()", "statSync", "mtime", "owner")
),
}
for name, result in checks.items():
print(f"{name}: {result}")
raise SystemExit(0 if all(checks.values()) else 1)
PY
printf '%s\n' '--- inbox-specific cleanup references ---'
rg -n -C3 'inboxCursorPath|inbox\.cursor\.lock|Inbox cursor is locked|writeInboxCursor' src tests --glob '*.ts'Repository: EtanHey/cmuxlayer
Length of output: 2894
Add recovery for stale inbox cursor locks
writeInboxCursor removes ${path}.lock only when the owning process reaches finally. A terminated process can leave the directory behind and make all later writes fail with Inbox cursor is locked. Add owner metadata and a bounded stale-lock policy. Ensure a writer can remove only its own lock, and add a regression test for a stale lock.
🤖 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/inbox.ts` around lines 384 - 396, Update writeInboxCursor’s lock
acquisition and cleanup to write owner metadata, detect stale locks using a
bounded age policy, and remove a lock only when ownership can be established for
the current writer. Preserve the existing retry error for active locks, ensure
stale locks can be recovered safely, and add a regression test covering recovery
from a stale lock.
| // Read both the inbox and watermark only after acquiring the lock. This is | ||
| // the compare-and-set boundary across independently resumed agent processes. | ||
| const messages = readInbox(agentId, opts); | ||
| const nextIndex = messages.findIndex((message) => message.id === messageId); | ||
| if (nextIndex < 0) { | ||
| throw new Error(`Cannot advance inbox cursor to unknown message ${messageId}`); | ||
| } | ||
| const current = readInboxCursor(agentId, opts); | ||
| if (current) { | ||
| const currentIndex = messages.findIndex((message) => message.id === current); | ||
| if (currentIndex >= 0 && nextIndex < currentIndex) { | ||
| throw new Error( | ||
| `Cannot move inbox cursor backwards from ${current} to ${messageId}`, | ||
| ); | ||
| } | ||
| } | ||
| tempPath = `${path}.${process.pid}.${randomUUID()}.tmp`; | ||
| writeFileSync(tempPath, `${messageId}\n`, "utf8"); | ||
| renameSync(tempPath, path); | ||
| tempPath = null; | ||
| return messageId; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
The regression guard fails open for an out-of-range current cursor.
currentIndex is -1 when the persisted cursor id is absent from the inbox file. The condition currentIndex >= 0 && nextIndex < currentIndex then skips the check, and the cursor moves to any known message. This allows a silent backwards move after inbox truncation.
Reject the advance, or preserve the current cursor, when the current cursor id cannot be located.
🤖 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/inbox.ts` around lines 398 - 418, Update the cursor validation around
readInboxCursor so an existing current cursor whose ID is absent from messages
is rejected or preserved instead of allowing advancement. Keep the existing
backwards-movement check for located cursors, while ensuring unknown persisted
cursors cannot move to a new message.
| const messages = readInbox(agentId, opts); | ||
| const cursor = readInboxCursor(agentId, opts); | ||
| if (cursor) { | ||
| const cursorIndex = messages.findIndex((message) => message.id === cursor); | ||
| // An unknown/corrupt cursor cannot safely suppress anything: replay all. | ||
| return cursorIndex >= 0 ? messages.slice(cursorIndex + 1) : messages; | ||
| } | ||
| const acked = ackedIds(agentId, opts); | ||
| return readInbox(agentId, opts).filter((m) => !acked.has(m.id)); | ||
| return messages.filter((m) => !acked.has(m.id)); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Inconsistent handling of a cursor id that is absent from the inbox file. Both sites read the persisted cursor and locate it with findIndex. Neither site defines a coherent behavior for -1. replayUndelivered treats the unresolvable cursor as "suppress nothing" and returns the entire inbox. writeInboxCursor treats it as "no constraint" and permits any advance. One decision about the unresolvable-cursor case resolves both.
src/inbox.ts#L439-L447: fall back to ack-based filtering whencursorIndexis-1, instead of returning every message.src/inbox.ts#L398-L418: reject the advance whencurrentIndexis-1, instead of skipping the backwards-move check.
📍 Affects 1 file
src/inbox.ts#L439-L447(this comment)src/inbox.ts#L398-L418
🤖 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/inbox.ts` around lines 439 - 447, The unresolvable persisted cursor must
be handled consistently across replay and cursor advancement. In src/inbox.ts
lines 439-447, update replayUndelivered to use ack-based filtering when
cursorIndex is -1 rather than returning all messages; in src/inbox.ts lines
398-418, update writeInboxCursor to reject advances when currentIndex is -1
instead of bypassing the backwards-move check.
| it("serializes cursor advancement with a per-agent cross-process lock", () => { | ||
| dispatch("cursor-lock", { from: "orc", task: "t1", id: "m1" }, opts); | ||
| mkdirSync(`${inboxCursorPath("cursor-lock", opts)}.lock`, { | ||
| recursive: true, | ||
| }); | ||
|
|
||
| expect(() => writeInboxCursor("cursor-lock", "m1", opts)).toThrow( | ||
| /cursor.*locked/i, | ||
| ); | ||
| expect(readInboxCursor("cursor-lock", opts)).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add coverage for the remaining writeInboxCursor rejection paths.
The suite pins the lock path only. Three other documented behaviors have no test: rejection of an unknown message id, rejection of a backwards move, and replay behavior for a cursor id that is absent from the inbox file. The third case controls whether an agent receives its entire retained history, so it deserves an explicit assertion.
🤖 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/inbox.test.ts` around lines 225 - 235, Expand the writeInboxCursor
tests around the existing cursor-lock case to cover rejection of an unknown
message id, rejection of a backwards cursor move, and replay when the cursor id
is absent from the inbox file. Use the existing inbox setup and helpers, assert
the documented errors for the two rejection paths, and verify the absent-id
replay returns the full retained history.
| if (args.includes("list-workspaces")) { | ||
| // Spawn response shaping enumerates topology after boot submission has | ||
| // been verified. Return subsequent reads to the caller-controlled screen. | ||
| submissionObservationPending = false; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Reset the observation flag on an explicit signal, not on list-workspaces.
The mock clears submissionObservationPending when the server enumerates workspaces. That couples the fixture to an assumption that no workspace enumeration happens between the Return keypress and submission verification. Topology collection also calls list-workspaces, and the server does collect topology mid-delivery whenever the spawned surface exposes a stable UUID. This fixture avoids that only because list-pane-surfaces omits id.
If a future change adds a surface UUID to this fixture, the flag clears early, the caller screen returns before verification finishes, and the test flakes. Prefer clearing the flag after a bounded number of read-screen calls, or expose an explicit helper the test calls when it wants the caller-controlled screen back.
🤖 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/pointer-discipline.test.ts` around lines 120 - 123, Stop resetting
submissionObservationPending in the list-workspaces handling path, since
topology collection may invoke it before submission verification completes.
Clear the flag only through an explicit test signal or after a bounded
read-screen count, while preserving the caller-screen transition until
verification has finished.
| expect(result.isError).toBe(true); | ||
| expect(failed).toMatchObject({ | ||
| delivery_id: expect.any(String), | ||
| delivery_state: "failed", | ||
| terminal: true, | ||
| }); | ||
| expect(engine.getDeliveryReceipt(failed.delivery_id)).toMatchObject({ | ||
| delivery_state: "failed", | ||
| terminal: true, | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add an assertion for the duplicate-event guard.
send_to passes appendFailureEvent: !(error instanceof SubmitVerificationError) so a submission-verification failure does not append a second terminal event on top of the one deliverInputChunks already wrote. No test covers that guard. A regression there would double-count failures in delivery telemetry and stay green.
This test already holds the delivery id, so counting matching event-log entries is a small addition.
♻️ Proposed assertion
expect(engine.getDeliveryReceipt(failed.delivery_id)).toMatchObject({
delivery_state: "failed",
terminal: true,
});
+ expect(
+ engine.stateMgr
+ .getEventLog()
+ .readEntries()
+ .filter(
+ (entry: any) =>
+ entry.delivery_id === failed.delivery_id &&
+ entry.delivery_state === "failed",
+ ),
+ ).toHaveLength(1);
});📝 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(failed).toMatchObject({ | |
| delivery_id: expect.any(String), | |
| delivery_state: "failed", | |
| terminal: true, | |
| }); | |
| expect(engine.getDeliveryReceipt(failed.delivery_id)).toMatchObject({ | |
| delivery_state: "failed", | |
| terminal: true, | |
| }); | |
| expect(result.isError).toBe(true); | |
| expect(failed).toMatchObject({ | |
| delivery_id: expect.any(String), | |
| delivery_state: "failed", | |
| terminal: true, | |
| }); | |
| expect(engine.getDeliveryReceipt(failed.delivery_id)).toMatchObject({ | |
| delivery_state: "failed", | |
| terminal: true, | |
| }); | |
| expect( | |
| engine.stateMgr | |
| .getEventLog() | |
| .readEntries() | |
| .filter( | |
| (entry: any) => | |
| entry.delivery_id === failed.delivery_id && | |
| entry.delivery_state === "failed", | |
| ), | |
| ).toHaveLength(1); |
🤖 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 7057 - 7066, Extend the test
around the existing failed delivery assertions to verify the duplicate-event
guard: using failed.delivery_id, inspect the event log and assert exactly one
matching terminal failure event exists after the submission-verification
failure. Keep the existing delivery receipt and failure-state assertions
unchanged.
| expect( | ||
| mockExec.mock.calls.filter(([, args]) => args.includes("send")), | ||
| ).toHaveLength(0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Include buffer-based delivery in the "nothing was sent" assertion.
This asserts only that no send command ran. Boot and relay text now reaches panes through set-buffer plus paste-buffer, as the other assertions in this file were updated to reflect. A queued delivery that incorrectly wrote to the pane through the buffer path would keep this assertion green.
♻️ Proposed assertion
expect(
- mockExec.mock.calls.filter(([, args]) => args.includes("send")),
+ mockExec.mock.calls.filter(
+ ([, args]) =>
+ args.includes("send") ||
+ args.includes("set-buffer") ||
+ args.includes("paste-buffer"),
+ ),
).toHaveLength(0);📝 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( | |
| mockExec.mock.calls.filter(([, args]) => args.includes("send")), | |
| ).toHaveLength(0); | |
| expect( | |
| mockExec.mock.calls.filter( | |
| ([, args]) => | |
| args.includes("send") || | |
| args.includes("set-buffer") || | |
| args.includes("paste-buffer"), | |
| ), | |
| ).toHaveLength(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/server-agent-tools.test.ts` around lines 8234 - 8236, Update the
“nothing was sent” assertion around mockExec calls to also reject buffer-based
delivery, checking for both set-buffer and paste-buffer commands in addition to
send. Preserve the existing zero-call expectation for all pane-delivery paths.
| sendKey: vi.fn().mockImplementation(async (surface: string, key: string) => { | ||
| if (key === "return") { | ||
| const count = (returnCount.get(surface) ?? 0) + 1; | ||
| returnCount.set(surface, count); | ||
| if (count >= 2) submitted.add(surface); | ||
| } | ||
| }), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Key submission on the delivered contract, not on a Return count.
count >= 2 encodes the assumption that exactly one Return precedes the boot prompt and the second Return submits it. The delivery path can press additional Returns: verifySubmitAfterEnter performs a recovery-enter retry when input still looks pending. An extra Return would mark the surface submitted early and let a genuine verification failure pass.
tests/spawn-monitor-boot.test.ts already models this by tracking whether the mailbox-contract text was delivered and only then treating a Return as a submission. Use the same rule here so the fixture states its intent and does not depend on a keypress count.
♻️ Proposed fix
- const returnCount = new Map<string, number>();
+ const bootTextSent = new Set<string>(); send: vi.fn().mockImplementation(async (surface: string, text: string) => {
if (/Codex/.test(text)) activeCli.set(surface, "codex");
if (/Claude/.test(text)) activeCli.set(surface, "claude");
+ if (text.includes("cmuxlayer mailbox contract")) bootTextSent.add(surface);
}),
pasteText: vi.fn().mockImplementation(async (surface: string, text: string) => {
if (/Codex/.test(text)) activeCli.set(surface, "codex");
if (/Claude/.test(text)) activeCli.set(surface, "claude");
+ if (text.includes("cmuxlayer mailbox contract")) bootTextSent.add(surface);
}),
sendKey: vi.fn().mockImplementation(async (surface: string, key: string) => {
- if (key === "return") {
- const count = (returnCount.get(surface) ?? 0) + 1;
- returnCount.set(surface, count);
- if (count >= 2) submitted.add(surface);
- }
+ if (key === "return" && bootTextSent.has(surface)) {
+ submitted.add(surface);
+ }
}),🤖 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/spawn-workspace.test.ts` around lines 146 - 152, Update the sendKey
mock in the spawn workspace test to mark a surface submitted only when a Return
is delivered after the mailbox-contract text has been delivered, rather than
when the Return count reaches two. Reuse the existing contract-delivery tracking
pattern from spawn-monitor-boot.test.ts and preserve the current returnCount
tracking only where needed for setup or assertions.
Co-Authored-By: cmuxlayerCodex running gpt-5.6-sol <noreply@anthropic.com>
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_4f1c8afd-a45e-4e2c-9a17-abd2e4747911) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/server-agent-tools.test.ts`:
- Line 8260: Replace the fixed 275 ms sleep in the retry test with bounded
polling that reads the persisted next_attempt_at and proceeds once it is due, or
advance controlled timers using the same clock as AgentEngine. Keep the wait
bounded and preserve the test’s existing retry assertions without coupling them
to the current backoff duration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5fa3bc6d-6648-449b-a54c-4588e303e4c5
📒 Files selected for processing (1)
tests/server-agent-tools.test.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 0
File: :0-0
Timestamp: 2026-08-02T16:01:45.986Z
Learning: PR `#345` preserves created resource identities in `src/server.ts` spawn-related tool failure responses. The follow-up structural prevention work is tracked in GitHub issue `#348`.
📚 Learning: 2026-03-15T10:46:40.958Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 1
File: tests/sidebar-sync.test.ts:18-77
Timestamp: 2026-03-15T10:46:40.958Z
Learning: In the cmuxlayer project, each test file (e.g., tests/sidebar-sync.test.ts, tests/quality-tracking.test.ts, tests/agent-hierarchy.test.ts) is intentionally self-contained. All mock setup helpers (makeMockClient, makeSurface, makeRecord) are defined locally within each test file rather than in shared fixtures. This is a deliberate design choice so that when a test fails, all context is in one file. Shared fixtures are avoided to prevent coupling between test suites. Minor drift in mock fields across files (e.g., listStatus present in one file but not another) is acceptable — it only matters when a test explicitly calls that method. Do not flag duplicated test helpers or suggest extracting them into shared fixture modules.
Applied to files:
tests/server-agent-tools.test.ts
📚 Learning: 2026-07-18T01:46:21.272Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 338
File: tests/self-registration.test.ts:775-935
Timestamp: 2026-07-18T01:46:21.272Z
Learning: In the self-registration feature, `tests/self-registration.test.ts` intentionally contains fully mocked `AgentEngine` composition/boot-capture cases. These feature-level acceptance tests cover the contract between `src/self-registration.ts` and `src/agent-engine.ts`; do not require moving them to `tests/agent-engine.test.ts` solely to mirror source layout.
Applied to files:
tests/server-agent-tools.test.ts
📚 Learning: 2026-07-14T17:32:22.637Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 0
File: :0-0
Timestamp: 2026-07-14T17:32:22.637Z
Learning: In cmuxlayer's `src/agent-engine.ts`, `runCloseForensicsBestEffort` treats both `tab_close` and `workspace_teardown` close-forensics event origins as terminal operator intent for a matching managed surface, persisting that intent before absence reconciliation can treat it as a recoverable crash (as of commit cd1ac43). Previously only `tab_close` was treated this way. Genuine PTY-death recovery (respawn with attempt limits) is a separate code path and remains unaffected by this origin allowlist.
Applied to files:
tests/server-agent-tools.test.ts
📚 Learning: 2026-07-04T23:37:37.595Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 220
File: tests/agent-engine.test.ts:2977-2977
Timestamp: 2026-07-04T23:37:37.595Z
Learning: In tests/agent-engine.test.ts for the cmuxlayer project, the general guideline of using 1-second timeouts for `waitFor` in agent-engine tests does not apply to positive/ready-resolution test cases that depend on consecutive-match accumulation across multiple poll/sweep ticks (e.g., "codex-pending-ready", "gemini-identity-screen-ready"). In `AgentEngine.waitFor`, the elapsed time is checked against timeoutMs before the next evidence poll, so a 1s budget can cause a false timeout for these multi-poll cases. These specific tests intentionally use longer timeouts (e.g., 1500ms/2500ms) and this is verified/expected behavior, not a violation to flag.
Applied to files:
tests/server-agent-tools.test.ts
📚 Learning: 2026-07-14T17:22:29.038Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 0
File: :0-0
Timestamp: 2026-07-14T17:22:29.038Z
Learning: In cmuxlayer's `src/server.ts` `close_surface` tool handler, the UUID-less legacy-record fallback match (`record.surface_id === args.surface` for records without `surface_uuid`) must only be applied when `observedSurfaceUuid === undefined` (i.e., the live topology genuinely cannot resolve a UUID for the closed ref). This mirrors the invariant in `resolveAgentIoRoute` (`src/agent-engine.ts`), which only permits UUID-less ref-based terminal I/O when a complete fresh topology proves zero UUID coverage. Without this guard, a recycled mutable ref could be mistakenly attributed to a stale legacy record even when a live UUID is observed for that ref (belonging to a different, current owner).
Applied to files:
tests/server-agent-tools.test.ts
📚 Learning: 2026-04-01T20:31:10.910Z
Learnt from: CR
Repo: EtanHey/cmuxlayer PR: 0
File: site/CLAUDE.md:0-0
Timestamp: 2026-04-01T20:31:10.910Z
Learning: Applies to site/**/*agent*.test.{ts,tsx} : Agents must have comprehensive unit tests covering success and failure paths
Applied to files:
tests/server-agent-tools.test.ts
📚 Learning: 2026-06-05T17:19:12.114Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 0
File: :0-0
Timestamp: 2026-06-05T17:19:12.114Z
Learning: In the cmuxlayer project (src/server.ts / spawn lifecycle), readiness timeouts during agent launch are non-terminal for lifecycle state. A `BootPromptTimeoutError` should NOT transition the agent to `error` — the agent stays in `booting` with no `error` set. A timeout can mean the CLI chrome changed or the PTY is still healthy but not yet matched; transitioning to error ("poisoning the registry") would block `send_to_agent` and inbox wake. Only actual boot-prompt delivery failures (non-timeout) are terminal, because partial delivery can leave the receiver in an unreliable state.
Applied to files:
tests/server-agent-tools.test.ts
📚 Learning: 2026-03-16T22:37:27.455Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 0
File: :0-0
Timestamp: 2026-03-16T22:37:27.455Z
Learning: In the cmuxlayer project (src/agent-engine.ts / src/agent-types.ts), the inconsistency between `buildLaunchCommand` (throws on `/` in repo names for shell arg safety) and `generateAgentId` (sanitizes `/` to `-` for key safety) is intentional and tracked for follow-up. Do not flag this mismatch as a bug. Both approaches are valid for their respective contexts.
Applied to files:
tests/server-agent-tools.test.ts
📚 Learning: 2026-07-13T18:37:02.633Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 311
File: src/server.ts:93-93
Timestamp: 2026-07-13T18:37:02.633Z
Learning: In `src/server.ts`, `assertDeliveryTargetIsSafe` deliberately calls `isPickerOrMenuScreen(snapshot.text, cli)` without threading a resolved `cli` hint through the shared `deliverInputChunks` delivery-safety boundary. This is intentional: the delivery safety gate must fail closed across every recognized picker/menu shape regardless of CLI, because registry/launcher CLI metadata can be absent or stale (especially on recycled surfaces), and scoping the gate by CLI could let a live menu from a different CLI slip through and consume text. The optional `cli` parameter remains useful for targeted parser callers that need CLI-scoped detection (e.g., Codex-specific update-menu detection), but the delivery-safety path itself stays CLI-agnostic by design.
Applied to files:
tests/server-agent-tools.test.ts
📚 Learning: 2026-03-16T22:37:27.796Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 0
File: :0-0
Timestamp: 2026-03-16T22:37:27.796Z
Learning: In the cmuxlayer project (src/agent-registry.ts), orphan reparenting is NOT part of V1. When a parent agent crashes, children intentionally keep their parent_agent_id pointing to the dead parent (orphan survival). Reparenting children to root (setting parent_agent_id to null) is a V2 design feature that will be introduced in a dedicated future PR with its own tests. Do not flag missing reparenting logic in agent-registry.ts until the V2 reparenting PR lands.
Applied to files:
tests/server-agent-tools.test.ts
📚 Learning: 2026-07-11T13:51:37.614Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 285
File: src/control-health.ts:183-208
Timestamp: 2026-07-11T13:51:37.614Z
Learning: In the `cmuxlayer` repository, `src/monitor-registry.ts` internals (e.g., `readMonitorRegistry`, `queryMonitorRegistryForGates`) are considered out of scope for modification in PR `#285`; its public API only accepts registry file paths, not pre-parsed snapshots. A true single-snapshot read-validate-count fix for `collectSelfHealHealth` in `src/control-health.ts` (avoiding a TOCTOU risk from reopening the file twice) requires adding a new public snapshot parser/query API to `monitor-registry.ts`. This is tracked in issue `#286` (EtanHey/cmuxlayer) with concrete acceptance criteria; until then, the mitigation is size-bounded reads and fail-safe (unavailable) handling of malformed/invalid registries, relying on the registry writer's atomic-rename contract.
Applied to files:
tests/server-agent-tools.test.ts
📚 Learning: 2026-03-15T10:42:08.557Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 1
File: src/agent-engine.ts:174-178
Timestamp: 2026-03-15T10:42:08.557Z
Learning: In the cmuxlayer project (`src/agent-engine.ts`), the `CmuxClient` methods `send()` and `sendKey()` are backed by a cmux socket that processes commands in order. Awaiting them sequentially guarantees the prior command is fully delivered before the next is sent — no additional delay or confirmation is needed between consecutive `send()`/`sendKey()` calls.
Applied to files:
tests/server-agent-tools.test.ts
📚 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/server-agent-tools.test.ts
🔇 Additional comments (2)
tests/server-agent-tools.test.ts (2)
7057-7066: Verify the exact-one failure-event assertion.The previous review requested an event-log assertion for
failed.delivery_id. If Lines 7057-7066 still check only the terminal receipt, the duplicate-event guard remains untested. Assert that exactly one matching terminal failure event exists.#!/usr/bin/env bash set -euo pipefail sed -n '7050,7070p' tests/server-agent-tools.test.ts rg -n -C 5 'readEntries|delivery_id|delivery_state' tests/server-agent-tools.test.ts
33-33: LGTM!Also applies to: 103-135, 148-168, 235-251, 648-649, 674-694, 1945-1945, 2053-2053, 2639-2645, 2740-2752, 2829-2840, 2861-2873, 2948-2949, 2961-2962, 3694-3694, 3904-3910, 4015-4017, 4033-4033, 4095-4101, 4163-4165, 4181-4181, 4986-4986, 5802-5802, 7027-7056, 7067-7068, 8198-8259, 8261-8278, 8427-8473
|
|
||
| const ready = engine.stateMgr.updateRecord(agentId, { state: "idle" }); | ||
| registry.set(agentId, ready); | ||
| await new Promise((resolve) => setTimeout(resolve, 275)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target test context ---'
sed -n '8200,8295p' tests/server-agent-tools.test.ts
printf '%s\n' '--- retry/backoff and receipt references ---'
rg -n -C 4 'next_attempt_at|backoff|retry|delivery_id|getDeliveryReceipt|setTimeout' tests/server-agent-tools.test.ts src server packages 2>/dev/null | head -n 500
printf '%s\n' '--- candidate implementation files ---'
rg -l 'next_attempt_at|getDeliveryReceipt|first retry|retry.*250|250.*retry' . --glob '!node_modules' --glob '!dist' --glob '!build' | head -n 100Repository: EtanHey/cmuxlayer
Length of output: 31095
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- AgentEngine structure ---'
ast-grep outline src/agent-engine.ts | head -n 250
printf '%s\n' '--- queue implementation references ---'
rg -n -C 8 'drainDeliveryQueue|next_attempt_at|delivery_state|retry_count|delivery receipt|backoff' src/agent-engine.ts src/agent-types.ts tests/server-agent-tools.test.ts
printf '%s\n' '--- test setup and timer support ---'
sed -n '1,180p' tests/server-agent-tools.test.ts
rg -n -C 5 'useFakeTimers|advanceTimers|waitFor|poll|sleep|drainDeliveryQueue' tests/server-agent-tools.test.ts src/agent-engine.tsRepository: EtanHey/cmuxlayer
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact queue implementation ---'
sed -n '4530,4730p' src/agent-engine.ts
printf '%s\n' '--- delivery submitter wiring ---'
rg -n -C 12 'setDeliverySubmitter|RetryableDeliveryError|sendToAgent|send_to_agent|send_to' src/server.ts src/*.ts tests/server-agent-tools.test.ts | head -n 600
printf '%s\n' '--- queue test declaration and setup ---'
sed -n '8120,8279p' tests/server-agent-tools.test.ts
printf '%s\n' '--- deterministic source verifier ---'
python3 - <<'PY'
from pathlib import Path
p = Path("src/agent-engine.ts").read_text()
required = [
'Date.parse(receipt.next_attempt_at) > Date.now()',
'const backoffMs = Math.min(',
'250 * 2 ** Math.min(receipt.retry_count - 1, 16)',
'receipt.next_attempt_at = new Date(',
]
for needle in required:
print(f"{needle!r}: {needle in p}")
start = p.index("if (error instanceof RetryableDeliveryError)")
end = p.index("this.persistDeliveryReceipts();", start)
print("\n--- retry scheduling statements ---")
print(p[start:end])
PYRepository: EtanHey/cmuxlayer
Length of output: 47768
Replace the hard-coded retry delay.
Line 8260 waits 275 ms to exceed the current 250 ms backoff. This couples the test to the retry policy and adds fixed latency. Wait until the persisted next_attempt_at is due with a bounded polling helper, or advance controlled timers using the same clock as AgentEngine.
🤖 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` at line 8260, Replace the fixed 275 ms
sleep in the retry test with bounded polling that reads the persisted
next_attempt_at and proceeds once it is due, or advance controlled timers using
the same clock as AgentEngine. Keep the wait bounded and preserve the test’s
existing retry assertions without coupling them to the current backoff duration.
1 participant
Summary
submitted,queued, orfaileddelivery receipts fromsend_toinbox.cursorwith strict-after replay, cross-process locking, atomic advancement, and a shell-safe compiled CLI helperVerification
npm run typechecknpm run buildnpm test— 107 files passed; 2530 passed, 1 skippedrun_tests.sh— exit 0probe-msgsend_topresent, queued contract advertised— cmuxlayerCodex (worker) · codex/gpt-5.6-sol
Note
High Risk
Changes core agent input delivery and spawn-boot behavior for every role, including durable queue semantics and no-replay rules after restart/timeout. Mis-handling here can drop, duplicate, or mis-order agent keystrokes.
Overview
send_tonow returns durable keyed receipts (delivery_id,delivery_state,terminal) instead of rejecting busy agents. Withoutallow_busy, aworkingagent gets a nonterminal queued receipt; the lifecycle sweep drains it when the target is interactive.AgentEnginepersists a delivery queue indelivery-receipts.jsonwith atomic writes, submission timeouts, and exponential backoff (capped at 30s) forRetryableDeliveryError. Receipts go terminal only when the target is gone or the outcome is uncertain—including after restart whensubmission_started_atwas set, so in-flight input is never silently replayed.Every spawned role now gets inbox bootstrap plus an injected mailbox boot contract (monitor command +
inbox-cursorhelper). Caller task text stays the task summary;monitor_bootis returned for workers too, not just orchestrators.Agent-owned
inbox.cursoradvances via locked atomic rename;replayUndeliveredprefers strict-after-cursor replay when present. New CLI:CMUX_INBOX_MSG_ID=<id> cmuxlayer inbox-cursor <agent-id>.Reviewed by Cursor Bugbot for commit 9d2c7d7. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add durable delivery receipts and inbox cursor watermarks to agent message delivery
AgentDeliveryReceiptinsrc/agent-engine.tswith atomic persistence todelivery-receipts.json, exponential backoff (capped at 30s) for retryable failures, and a bounded per-submission timeout to prevent hung drains.send_tonow returns structured receipts withdelivery_id,delivery_state(queued/submitted/failed), andterminalflag; busy agents get a non-terminal queued receipt instead of an error.readInboxCursor/writeInboxCursorinsrc/inbox.tsfor atomic, cross-process-safe watermark advancement;replayUndeliverednow uses the cursor to suppress already-handled messages on restart.CMUX_INBOX_MSG_ID);monitor_bootnow always includescursor_path,cursor_update_command, andcursor_update_env.inbox-cursorCLI subcommand insrc/index.tsfor agents to advance their own cursor watermark.submission_started_atset at startup are markedfailed(no replay) to prevent double-delivery after process restart.Macroscope summarized 9d2c7d7.
Summary by CodeRabbit
New Features
inbox-cursorcommand for updating an agent’s inbox position.Bug Fixes