fix: harden agent spawn failures - #397
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_6f86ed6e-8ab9-417f-a546-4af371ccf636) |
📝 WalkthroughWalkthroughThe PR adds shared shell-prompt detection, created-identity propagation, launcher diagnostics, failed-launch cleanup, worktree rollback, and launcher-contract validation across agent spawning and surface-creation paths. ChangesSpawn robustness
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| return prompt?.input.trim() === ""; | ||
| } | ||
|
|
||
| export function launcherFailureFromShell(text: string): string | null { |
There was a problem hiding this comment.
🟡 Medium src/shell-prompt.ts:40
launcherFailureFromShell returns null for a normal Python traceback because it only checks the single line adjacent to the shell prompt. A typical traceback puts Traceback (most recent call last) several lines above the final exception line, so the pattern never matches and the caller waits until the generic timeout instead of getting the intended immediate failure diagnostic. Consider scanning all non-blank lines (not just the adjacent one) for the traceback marker.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/shell-prompt.ts around line 40:
`launcherFailureFromShell` returns `null` for a normal Python traceback because it only checks the single line adjacent to the shell prompt. A typical traceback puts `Traceback (most recent call last)` several lines above the final exception line, so the pattern never matches and the caller waits until the generic timeout instead of getting the intended immediate failure diagnostic. Consider scanning all non-blank lines (not just the adjacent one) for the traceback marker.
| this.identity[key] = current; | ||
| } | ||
|
|
||
| attach(error: unknown): Error { |
There was a problem hiding this comment.
🟡 Medium src/created-identity.ts:48
CreatedIdentityScope.attach calls Object.defineProperty directly on the received Error. If that error object is frozen or non-extensible, the defineProperty call throws a TypeError that masks the original failure, so the tool's catch block never returns the created resource IDs — the caller sees the TypeError instead of the actual creation error with identity metadata. Consider wrapping the defineProperty call in a try/catch so a non-extensible error object falls back to a shallow copy or an external sidecar map instead of throwing.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/created-identity.ts around line 48:
`CreatedIdentityScope.attach` calls `Object.defineProperty` directly on the received `Error`. If that error object is frozen or non-extensible, the `defineProperty` call throws a `TypeError` that masks the original failure, so the tool's catch block never returns the created resource IDs — the caller sees the `TypeError` instead of the actual creation error with identity metadata. Consider wrapping the `defineProperty` call in a `try`/`catch` so a non-extensible error object falls back to a shallow copy or an external sidecar map instead of throwing.
| @@ -0,0 +1,54 @@ | |||
| const SHELL_PROMPT_TERMINATOR = "[$%#>❯›»]"; | |||
There was a problem hiding this comment.
🟠 High src/shell-prompt.ts:1
SHELL_PROMPT_TERMINATOR includes a bare >, so matchesShellPrompt returns true for a line containing only >. A bare > is the standard shell continuation prompt emitted by bash while a command is still incomplete (for example, after an unmatched quote or trailing backslash). Treating a continuation prompt as a ready prompt causes the caller to send the launcher command into the still-open command instead of starting a new one, producing malformed or unintended shell input. If > is meant to match some non-continuation prompt style, consider restricting it (for example, only accept >>> or other non-ambiguous forms) or document why bare > is intentionally accepted.
| const SHELL_PROMPT_TERMINATOR = "[$%#>❯›»]"; | |
| +const SHELL_PROMPT_TERMINATOR = "[$%#❯›»]"; |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/shell-prompt.ts around line 1:
`SHELL_PROMPT_TERMINATOR` includes a bare `>`, so `matchesShellPrompt` returns `true` for a line containing only `>`. A bare `>` is the standard shell continuation prompt emitted by bash while a command is still incomplete (for example, after an unmatched quote or trailing backslash). Treating a continuation prompt as a ready prompt causes the caller to send the launcher command into the still-open command instead of starting a new one, producing malformed or unintended shell input. If `>` is meant to match some non-continuation prompt style, consider restricting it (for example, only accept `>>>` or other non-ambiguous forms) or document why bare `>` is intentionally accepted.
![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: 60bb6cfadf
ℹ️ 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".
| if ( | ||
| screenShowsPendingShellInput( | ||
| pendingScreen.text, | ||
| sanitizedCommand, | ||
| ) | ||
| ) { | ||
| throw new LauncherReadinessError( | ||
| `launcher command remained pending after Return on ${opts.surface}`, | ||
| tailLines(pendingScreen.text, 10), | ||
| ); |
There was a problem hiding this comment.
When a launcher accepts Return but takes more than 150 ms to print output, the submitted command can remain the last visible prompt line even though the process is starting normally. screenShowsPendingShellInput cannot distinguish that shell-history echo from editable input until another row appears, so this branch reports LauncherReadinessError, closes the surface, and may delete its worktree for a valid slow launch. Keep polling through the launch-readiness budget, or require stronger evidence that the command is still editable before performing destructive cleanup.
Useful? React with 👍 / 👎.
| await restoreFocusAfterRender( | ||
| focusRestoreLease, | ||
| result.surface_id, | ||
| launcherSurfaceClosed ? undefined : result.surface_id, | ||
| spawnDeliveryWorkspace(result, spawnWorkspace), |
There was a problem hiding this comment.
For the default focus:false spawn path, a launcher readiness failure now closes the created surface before this call. Closing the focused surface necessarily moves focus to another tab, while restoreFocusAfterRender restores only if the current target still equals lease.expected; passing undefined here does not change that check. Consequently the restore becomes a no-op and failed spawns leave the user on an arbitrary neighboring tab instead of the exact origin.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@docs/plans/2026-08-12-spawn-robustness.md`:
- Around line 72-75: Replace the absolute paths in the **Files** list with
repository-relative paths that accurately identify tracked targets, or
explicitly state that the referenced targets are outside version control; remove
the developer-specific `/Users/...` prefix and avoid presenting `docs.local/` as
a tracked repository directory.
- Line 13: Promote all five task headings in the document from level-three
headings to level-two headings, including the headings beginning with “Task 1”
through “Task 5,” so the outline follows the top-level `#` heading without
skipping `##`.
In `@src/server.ts`:
- Around line 7469-7500: Extract a shared helper for the four creation catch
blocks that maps the caught error to its specialized payload fields for
SurfaceGoneError, BootPromptTimeoutError, BootPromptUpdateMenuBlockedError, and
BootPromptDeliveryError, while preserving the generic fallback. Remove each
local createdIdentity construction and have new_split, this catch block,
new_worktree_split, and spawn_in_workspace call the helper so err() continues
merging scope identity.
- Around line 5051-5101: Update verifyPendingCommandSubmitted and the related
pending probe around client.readScreen so transient screen-read failures are
treated as inconclusive: catch read errors within the existing bounded polling
windows, retain the last known screen state, and continue polling like
waitForLaunchShellReady. Ensure failures do not escape before authoritative
readiness checks such as waitForAgentLaunchReady can run, while preserving
existing handling for confirmed surface loss and readiness timeout errors.
- Around line 10203-10252: Update cleanupFailedLauncherArtifacts so a
client.closeSurface failure records its diagnostic but does not return early.
Always execute the stateMgr.transition error handling and
rollbackPreparedWorktree cleanup, and preserve any cleanup failures in
error.message. Adjust the terminal-state error text to distinguish whether the
launcher surface was successfully closed or the close operation failed.
- Around line 4571-4577: Update the readiness failure path around
launcherFailureFromShell so it raises LauncherReadinessError only when the
matched failure evidence is attributable to the current launch, rather than
merely any failure before a shell prompt. Track or validate launch-scoped output
explicitly; do not rely on scrollback: false, and preserve the existing error
message and tailLines context once current-launch evidence is confirmed.
In `@src/shell-prompt.ts`:
- Around line 49-53: Update the failure detection logic around the adjacentLine
check to inspect the contiguous final command block bounded by the preceding
prompt, rather than testing only the tail line. Add recognition for standard
multi-line Python tracebacks where the traceback header appears earlier and the
final exception line precedes the prompt, while preserving the existing
launcher-error patterns.
- Around line 21-27: Update the prefixed-prompt parsing in the shell prompt
function to capture the prompt terminator, then apply the existing
allowRootInput policy before returning input: return null for prefixed prompts
using # when root input is not allowed, while preserving normal prefixed prompt
handling. Add a test covering a prefixed root prompt such as root@host / # with
allowRootInput unset.
In `@tests/created-identity.test.ts`:
- Around line 9-65: Add tests in the CreatedIdentityScope suite covering both
missing behaviors: verify error-supplied identity metadata, including agent_id,
takes precedence over recorded identity, and verify attaching a non-Error value
produces a new Error with the original value as cause and preserves identity on
the returned error. Use attach and createdIdentityFromError to assert the
documented outcomes.
In `@tests/fixtures/golem-dispatch-contract.zsh`:
- Line 1: Extend the header comment in golem-dispatch-contract.zsh to document
the mirrored launcher’s source path, the fixture capture date, and the procedure
for refreshing it from the real launcher. Keep the fixture contents unchanged
and make the provenance and refresh guidance self-contained in the header.
In `@tests/model-policy-drift.test.ts`:
- Around line 113-124: Update parseCodexEffortValues and parseCursorLauncher to
accept a source label and use it in parse-failure messages, so errors identify
the actual input file. Thread the same source argument through
parseClaudeDefault and parseCursorLauncher as needed, passing
contractFixturePath in the hermetic suite and dispatchPath in the parity suite;
preserve the existing parsing behavior.
In `@tests/server-agent-tools.test.ts`:
- Around line 4658-4663: Update the launcherReturns assertion in the pending
launcher command test to enforce the maximum allowed Return submissions,
preventing retries after an unverified acknowledgement. Prefer asserting the
exact expected count for this fixture; if multiple Returns are legitimately
sent, assert that precise count rather than using toBeGreaterThanOrEqual.
🪄 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: e5d9d140-f5ca-4187-b523-fe72ab5e9950
📒 Files selected for processing (11)
docs/plans/2026-08-12-spawn-robustness.mdsrc/agent-engine.tssrc/app-server-runtime.tssrc/created-identity.tssrc/server.tssrc/shell-prompt.tstests/created-identity.test.tstests/fixtures/golem-dispatch-contract.zshtests/model-policy-drift.test.tstests/server-agent-tools.test.tstests/shell-prompt.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Macroscope - Correctness Check
🧰 Additional context used
🧠 Learnings (25)
📓 Common learnings
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.
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.
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`.
Learnt from: CR
Repo: EtanHey/cmuxlayer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-08-09T13:21:30.476Z
Learning: Applies to <AGENTS.md> : If spawning fails after cmuxlayer creates a worktree, remove both that worktree and its newly created branch; never roll back a reused worktree.
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.
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.
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 0
File: :0-0
Timestamp: 2026-06-05T17:26:08.862Z
Learning: In the cmuxlayer project, `wait_for(done)` must NOT trust registry state alone. It must require terminal output evidence — either a persisted `task_done_detected_at` timestamp or a current parser-confirmed completion (`parseScreen` reporting `status === "done"`) — because registry state can be stale, manually mutated, or poisoned by prior launch/readiness failures (e.g. `BootPromptTimeoutError`). Completion signals must be accepted only from the current screen tail / chrome-adjacent area so echoed prompt instructions like `R2_WORKER_DONE` do not mark active work as done.
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.
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 345
File: src/server.ts:8413-8414
Timestamp: 2026-08-02T15:25:29.749Z
Learning: In `src/server.ts`, `boot_prompt_timeout_ms` is an intentional cross-phase override for `spawn_agent` and `new_worktree_split`. When supplied, it controls initial shell readiness, agent launch readiness, post-update relaunch readiness, and boot-prompt readiness. When omitted, the phases retain independent defaults: 10 seconds for shell readiness, 15 seconds for agent launch readiness, and 60 seconds for boot-prompt readiness.
📚 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/shell-prompt.test.tstests/created-identity.test.tstests/model-policy-drift.test.tstests/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/shell-prompt.test.tstests/server-agent-tools.test.ts
📚 Learning: 2026-08-02T15:25:29.749Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 345
File: src/server.ts:8413-8414
Timestamp: 2026-08-02T15:25:29.749Z
Learning: In `src/server.ts`, `boot_prompt_timeout_ms` is an intentional cross-phase override for `spawn_agent` and `new_worktree_split`. When supplied, it controls initial shell readiness, agent launch readiness, post-update relaunch readiness, and boot-prompt readiness. When omitted, the phases retain independent defaults: 10 seconds for shell readiness, 15 seconds for agent launch readiness, and 60 seconds for boot-prompt readiness.
Applied to files:
tests/shell-prompt.test.tsdocs/plans/2026-08-12-spawn-robustness.mdsrc/app-server-runtime.tstests/server-agent-tools.test.tssrc/server.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/shell-prompt.test.tstests/created-identity.test.tstests/server-agent-tools.test.ts
📚 Learning: 2026-06-05T18:18:12.145Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 0
File: :0-0
Timestamp: 2026-06-05T18:18:12.145Z
Learning: In the cmuxlayer project (src/screen-parser.ts `isEchoedPromptContextLine`), the explicit `TASK_DONE` alternation alongside the generic `[A-Z][A-Z0-9_]*_DONE` branch is intentionally redundant — kept for readability around the canonical signal name. Do not flag it as dead code.
Applied to files:
tests/shell-prompt.test.ts
📚 Learning: 2026-06-05T17:26:08.862Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 0
File: :0-0
Timestamp: 2026-06-05T17:26:08.862Z
Learning: In the cmuxlayer project, `wait_for(done)` must NOT trust registry state alone. It must require terminal output evidence — either a persisted `task_done_detected_at` timestamp or a current parser-confirmed completion (`parseScreen` reporting `status === "done"`) — because registry state can be stale, manually mutated, or poisoned by prior launch/readiness failures (e.g. `BootPromptTimeoutError`). Completion signals must be accepted only from the current screen tail / chrome-adjacent area so echoed prompt instructions like `R2_WORKER_DONE` do not mark active work as done.
Applied to files:
tests/shell-prompt.test.tsdocs/plans/2026-08-12-spawn-robustness.mdtests/server-agent-tools.test.tssrc/server.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/shell-prompt.test.tstests/created-identity.test.tstests/model-policy-drift.test.tstests/server-agent-tools.test.ts
📚 Learning: 2026-08-03T12:44:07.210Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 351
File: src/model-policy.ts:253-273
Timestamp: 2026-08-03T12:44:07.210Z
Learning: In `src/model-policy.ts`, repoGolem requires `REPOGOLEM_ALLOW_MODEL=1` for the Claude `opus` and `haiku` model aliases. The Claude `sonnet` alias is available without this environment variable through repoGolem’s `-S`/`--sonnet` launcher path. Therefore, the ungated accepted Claude model set is `claude-opus-5[1m], sonnet`.
Applied to files:
tests/fixtures/golem-dispatch-contract.zshtests/model-policy-drift.test.ts
📚 Learning: 2026-08-02T16:01:45.986Z
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`.
Applied to files:
tests/created-identity.test.tssrc/created-identity.tsdocs/plans/2026-08-12-spawn-robustness.mdtests/server-agent-tools.test.tssrc/server.ts
📚 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/created-identity.test.tstests/model-policy-drift.test.tstests/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:
src/agent-engine.tsdocs/plans/2026-08-12-spawn-robustness.mdtests/server-agent-tools.test.tssrc/server.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:
src/agent-engine.tstests/model-policy-drift.test.tsdocs/plans/2026-08-12-spawn-robustness.mdtests/server-agent-tools.test.tssrc/server.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:
src/agent-engine.tssrc/server.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*.{ts,tsx} : Use the Agent interface/base class for creating new agents
Applied to files:
src/agent-engine.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:
src/agent-engine.tsdocs/plans/2026-08-12-spawn-robustness.mdtests/server-agent-tools.test.tssrc/server.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*.{ts,tsx} : Document agent purpose and usage in agent implementation files
Applied to files:
src/agent-engine.ts
📚 Learning: 2026-04-01T16:08:15.301Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 0
File: :0-0
Timestamp: 2026-04-01T16:08:15.301Z
Learning: In the cmuxlayer project (src/agent-engine.ts), the switch statement in `buildLaunchCommand` has no `default` case by design. `CliType` is a compile-time exhaustive union (`'claude' | 'codex' | 'gemini' | 'kiro' | 'cursor'`); TypeScript enforces that all cases are covered, so adding a new CLI to the union without a corresponding case is a compile error. Do not flag the missing default case.
Applied to files:
tests/model-policy-drift.test.tssrc/server.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/model-policy-drift.test.tsdocs/plans/2026-08-12-spawn-robustness.md
📚 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/model-policy-drift.test.tssrc/server.ts
📚 Learning: 2026-08-09T13:21:30.476Z
Learnt from: CR
Repo: EtanHey/cmuxlayer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-08-09T13:21:30.476Z
Learning: Applies to <AGENTS.md> : If spawning fails after cmuxlayer creates a worktree, remove both that worktree and its newly created branch; never roll back a reused worktree.
Applied to files:
docs/plans/2026-08-12-spawn-robustness.mdtests/server-agent-tools.test.tssrc/server.ts
📚 Learning: 2026-06-05T18:04:24.095Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 0
File: :0-0
Timestamp: 2026-06-05T18:04:24.095Z
Learning: In the cmuxlayer project (src/layout-policy.ts), for `chooseAgentSpawnPlacement`, orchestrator/lead agents should tab into the leftmost lead column (leftPane) unless that pane qualifies as a worker dock via `isWorkerDockPane()` (requires: orchestratorCount=0, icCount=0, workerCount>0, workerCount>nonRoleCount — i.e. workers are strict majority with no leads/ICs). A stale registry role such as ic or worker on a Claude-LEAD tab, or a non-agent shell tab in the left lead pane, is contamination and must NOT force a new left split. Worker placement (docking) remains owned by the rightmost worker pane logic using the same `isWorkerDockPane` predicate.
Applied to files:
docs/plans/2026-08-12-spawn-robustness.md
📚 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:
docs/plans/2026-08-12-spawn-robustness.mdtests/server-agent-tools.test.ts
📚 Learning: 2026-04-01T16:08:15.301Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 0
File: :0-0
Timestamp: 2026-04-01T16:08:15.301Z
Learning: In the cmuxlayer project (src/agent-engine.ts), `buildLaunchCommand` explicitly rejects `.` and `..` as repo names (throws "Invalid repo name") to prevent path traversal where `cd ~/Gits/..` would escape to the parent directory. This guard was added in commit fe41149.
Applied to files:
tests/server-agent-tools.test.tssrc/server.ts
📚 Learning: 2026-08-09T13:21:30.476Z
Learnt from: CR
Repo: EtanHey/cmuxlayer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-08-09T13:21:30.476Z
Learning: Applies to <AGENTS.md> : For managed worktree spawns, treat `repo` as selecting the repoGolem registration and naming the worker; do not infer a repository path such as `~/Gits/<repo>`.
Applied to files:
tests/server-agent-tools.test.ts
🪛 ast-grep (0.45.1)
src/shell-prompt.ts
[warning] 8-8: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*(${SHELL_PROMPT_TERMINATOR})(?:\\s+(.*))?$, "u")
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 21-24: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^\\s*(?:(?:\\S+@\\S+)(?:\\s+(?:~|\\/)\\S*)?|(?:.*\\s)?(?:~|\\/)\\S*)(?:\\s+\\[[^\\]]+\\])?\\s*${SHELL_PROMPT_TERMINATOR}(?:\\s+(.*))?$,
"u",
)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🪛 markdownlint-cli2 (0.23.2)
docs/plans/2026-08-12-spawn-robustness.md
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🔇 Additional comments (8)
src/app-server-runtime.ts (1)
26-26: LGTM!src/shell-prompt.ts (1)
30-38: LGTM!src/agent-engine.ts (1)
238-238: LGTM!Also applies to: 265-267, 5394-5394, 5414-5414
src/created-identity.ts (1)
1-64: LGTM!src/server.ts (2)
790-799: LGTM!Also applies to: 1035-1050
1016-1027: 🗄️ Data Integrity & IntegrationDo not flag this identity-order change for fresh spawns.
generateAgentId(cli, repo)returns a random stable ID, not a-pending-ID.finalizeCapturedSessiononly renames IDs containing-pending-, so the described stale identity overwrite does not occur in these paths.> Likely an incorrect or invalid review comment.tests/server-agent-tools.test.ts (1)
53-53: LGTM!Also applies to: 62-66, 102-102, 141-141, 1227-1227, 3748-3839, 4910-4930, 4932-4960, 5099-5101, 5229-5229
tests/model-policy-drift.test.ts (1)
4-4: LGTM!Also applies to: 102-111, 127-129
|
|
||
| --- | ||
|
|
||
| ### Task 1: Shared shell-prompt contract |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the heading increment.
The document opens with # at Line 1 and then jumps to ### for each task. markdownlint reports MD001 because the level skips ##. Promote the five task headings to ##.
🔧 Proposed fix
-### Task 1: Shared shell-prompt contract
+## Task 1: Shared shell-prompt contractApply the same change to Task 2 (Line 27), Task 3 (Line 43), Task 4 (Line 57), and Task 5 (Line 70).
📝 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.
| ### Task 1: Shared shell-prompt contract | |
| ## Task 1: Shared shell-prompt contract |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 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 `@docs/plans/2026-08-12-spawn-robustness.md` at line 13, Promote all five task
headings in the document from level-three headings to level-two headings,
including the headings beginning with “Task 1” through “Task 5,” so the outline
follows the top-level `#` heading without skipping `##`.
Source: Linters/SAST tools
| **Files:** | ||
| - Modify: `/Users/etanheyman/Gits/cmuxlayer/docs.local/plan/stability-v2/phase-7/findings.md` | ||
| - Modify: `/Users/etanheyman/Gits/cmuxlayer/docs.local/plan/stability-v2/collab.md` | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the absolute developer-local paths with repository-relative paths.
Lines 73 and 74 name /Users/etanheyman/Gits/cmuxlayer/docs.local/.... A committed plan document cannot resolve those paths on any other machine, and the entry exposes one contributor's home directory layout. The docs.local/ segment also indicates a directory that is not tracked in this repository.
Use paths relative to the repository root, or state that the targets live outside version control.
🔧 Proposed fix
**Files:**
-- Modify: `/Users/etanheyman/Gits/cmuxlayer/docs.local/plan/stability-v2/phase-7/findings.md`
-- Modify: `/Users/etanheyman/Gits/cmuxlayer/docs.local/plan/stability-v2/collab.md`
+- Modify (untracked, local only): `docs.local/plan/stability-v2/phase-7/findings.md`
+- Modify (untracked, local only): `docs.local/plan/stability-v2/collab.md`📝 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.
| **Files:** | |
| - Modify: `/Users/etanheyman/Gits/cmuxlayer/docs.local/plan/stability-v2/phase-7/findings.md` | |
| - Modify: `/Users/etanheyman/Gits/cmuxlayer/docs.local/plan/stability-v2/collab.md` | |
| **Files:** | |
| - Modify (untracked, local only): `docs.local/plan/stability-v2/phase-7/findings.md` | |
| - Modify (untracked, local only): `docs.local/plan/stability-v2/collab.md` |
🤖 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 `@docs/plans/2026-08-12-spawn-robustness.md` around lines 72 - 75, Replace the
absolute paths in the **Files** list with repository-relative paths that
accurately identify tracked targets, or explicitly state that the referenced
targets are outside version control; remove the developer-specific `/Users/...`
prefix and avoid presenting `docs.local/` as a tracked repository directory.
| const caught = creation.attach(e); | ||
| const createdIdentity = result | ||
| ? { | ||
| surface: result.surface, | ||
| workspace: result.workspace, | ||
| ...(result.surface_id ? { surface_id: result.surface_id } : {}), | ||
| } | ||
| : {}; | ||
| if (e instanceof SurfaceGoneError) { | ||
| return err(e, surfaceGonePayload(e, createdIdentity)); | ||
| if (caught instanceof SurfaceGoneError) { | ||
| return err(caught, surfaceGonePayload(caught, createdIdentity)); | ||
| } | ||
| if (e instanceof BootPromptTimeoutError) { | ||
| return err(e, { | ||
| if (caught instanceof BootPromptTimeoutError) { | ||
| return err(caught, { | ||
| ...createdIdentity, | ||
| last_10_lines: e.last_10_lines, | ||
| last_10_lines: caught.last_10_lines, | ||
| }); | ||
| } | ||
| if (e instanceof BootPromptUpdateMenuBlockedError) { | ||
| return err(e, { | ||
| if (caught instanceof BootPromptUpdateMenuBlockedError) { | ||
| return err(caught, { | ||
| ...createdIdentity, | ||
| error_code: e.error_code, | ||
| last_10_lines: e.last_10_lines, | ||
| recovery: e.recovery, | ||
| error_code: caught.error_code, | ||
| last_10_lines: caught.last_10_lines, | ||
| recovery: caught.recovery, | ||
| }); | ||
| } | ||
| if (e instanceof BootPromptDeliveryError) { | ||
| return err(e, { | ||
| if (caught instanceof BootPromptDeliveryError) { | ||
| return err(caught, { | ||
| ...createdIdentity, | ||
| delivered_chars: e.delivered_chars, | ||
| delivered_chars: caught.delivered_chars, | ||
| }); | ||
| } | ||
| return err(e, createdIdentity); | ||
| return err(caught, createdIdentity); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consolidate the repeated created-identity error ladders.
This catch block is now byte-identical in shape to new_split (Lines 7312-7351), and the same four-branch ladder appears again in new_worktree_split (Lines 10932-10970) and spawn_in_workspace (Lines 11356-11394). Each copy must stay in sync as new error types are added.
The local createdIdentity object is also redundant here: creation.record at Line 7414 already stores surface, workspace, and surface_id, and err() merges scope identity into every payload.
Extract one helper that maps a caught error to its specialized payload fields, then call it from all four sites.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server.ts` around lines 7469 - 7500, Extract a shared helper for the four
creation catch blocks that maps the caught error to its specialized payload
fields for SurfaceGoneError, BootPromptTimeoutError,
BootPromptUpdateMenuBlockedError, and BootPromptDeliveryError, while preserving
the generic fallback. Remove each local createdIdentity construction and have
new_split, this catch block, new_worktree_split, and spawn_in_workspace call the
helper so err() continues merging scope identity.
| return /(?:command not found|no such file(?: or directory)?|permission denied|traceback \(most recent call last\)|invalid (?:option|argument|model|effort))/i.test( | ||
| adjacentLine, | ||
| ) | ||
| ? adjacentLine | ||
| : null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Detect standard multi-line tracebacks at the screen tail.
A normal Python traceback ends with an exception line immediately before the prompt. Traceback (most recent call last) appears earlier in the block. This check therefore returns null for the traceback format that it declares as a launcher failure. Inspect the contiguous final command block, bounded by the preceding prompt, and add a multi-line traceback test.
🤖 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/shell-prompt.ts` around lines 49 - 53, Update the failure detection logic
around the adjacentLine check to inspect the contiguous final command block
bounded by the preceding prompt, rather than testing only the tail line. Add
recognition for standard multi-line Python tracebacks where the traceback header
appears earlier and the final exception line precedes the prompt, while
preserving the existing launcher-error patterns.
| describe("CreatedIdentityScope", () => { | ||
| it("attaches recorded identity without changing the error type or cause", () => { | ||
| const cause = new Error("socket stderr"); | ||
| const error = new UnclassifiedPostCreationError("later failure", { | ||
| cause, | ||
| }); | ||
| const scope = new CreatedIdentityScope(); | ||
| scope.record({ surface: "surface:7", workspace: "workspace:2" }); | ||
|
|
||
| const attached = scope.attach(error); | ||
|
|
||
| expect(attached).toBe(error); | ||
| expect(attached).toBeInstanceOf(UnclassifiedPostCreationError); | ||
| expect(attached.cause).toBe(cause); | ||
| expect(createdIdentityFromError(attached)).toEqual({ | ||
| surface: "surface:7", | ||
| workspace: "workspace:2", | ||
| }); | ||
| }); | ||
|
|
||
| it("does not invent identity before creation", () => { | ||
| const scope = new CreatedIdentityScope(); | ||
| expect(createdIdentityFromError(scope.attach(new Error("preflight")))).toEqual( | ||
| {}, | ||
| ); | ||
| }); | ||
|
|
||
| it("accumulates prior batch identities and updates the failing member", () => { | ||
| const scope = new CreatedIdentityScope(); | ||
| const sameSurface = ( | ||
| left: Record<string, unknown>, | ||
| right: Record<string, unknown>, | ||
| ) => left.surface_id === right.surface_id; | ||
| scope.append( | ||
| "agents", | ||
| { agent_id: "pending-a", surface_id: "surface:a" }, | ||
| sameSurface, | ||
| ); | ||
| scope.append( | ||
| "agents", | ||
| { agent_id: "agent-a", surface_id: "surface:a" }, | ||
| sameSurface, | ||
| ); | ||
| scope.append( | ||
| "agents", | ||
| { agent_id: "agent-b", surface_id: "surface:b" }, | ||
| sameSurface, | ||
| ); | ||
|
|
||
| expect(createdIdentityFromError(scope.attach(new Error("batch")))).toEqual({ | ||
| agents: [ | ||
| { agent_id: "agent-a", surface_id: "surface:a" }, | ||
| { agent_id: "agent-b", surface_id: "surface:b" }, | ||
| ], | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add the two uncovered CreatedIdentityScope behaviors.
The plan for this task lists four unit assertions, including that error-supplied metadata cannot overwrite identity fields. No test pins that precedence. That precedence is what makes err() spread createdIdentityFromError(error) last, and it currently lets a recorded identity replace a caller-supplied agent_id. A test makes the intended winner explicit.
The non-Error input path is also uncovered. asError returns a new Error and sets cause to the original value, so attach returns a different object than it received. Callers that discard the return value lose the identity.
💚 Proposed additional cases
+ it("keeps recorded identity authoritative over caller-supplied metadata", () => {
+ const scope = new CreatedIdentityScope();
+ scope.record({ surface: "surface:7" });
+ const merged = {
+ surface: "surface:stale",
+ ...createdIdentityFromError(scope.attach(new Error("later failure"))),
+ };
+
+ expect(merged.surface).toBe("surface:7");
+ });
+
+ it("wraps a non-Error throw and preserves it as the cause", () => {
+ const scope = new CreatedIdentityScope();
+ scope.record({ surface: "surface:7" });
+
+ const attached = scope.attach("socket closed");
+
+ expect(attached).toBeInstanceOf(Error);
+ expect(attached.cause).toBe("socket closed");
+ expect(createdIdentityFromError(attached)).toEqual({
+ surface: "surface:7",
+ });
+ });📝 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.
| describe("CreatedIdentityScope", () => { | |
| it("attaches recorded identity without changing the error type or cause", () => { | |
| const cause = new Error("socket stderr"); | |
| const error = new UnclassifiedPostCreationError("later failure", { | |
| cause, | |
| }); | |
| const scope = new CreatedIdentityScope(); | |
| scope.record({ surface: "surface:7", workspace: "workspace:2" }); | |
| const attached = scope.attach(error); | |
| expect(attached).toBe(error); | |
| expect(attached).toBeInstanceOf(UnclassifiedPostCreationError); | |
| expect(attached.cause).toBe(cause); | |
| expect(createdIdentityFromError(attached)).toEqual({ | |
| surface: "surface:7", | |
| workspace: "workspace:2", | |
| }); | |
| }); | |
| it("does not invent identity before creation", () => { | |
| const scope = new CreatedIdentityScope(); | |
| expect(createdIdentityFromError(scope.attach(new Error("preflight")))).toEqual( | |
| {}, | |
| ); | |
| }); | |
| it("accumulates prior batch identities and updates the failing member", () => { | |
| const scope = new CreatedIdentityScope(); | |
| const sameSurface = ( | |
| left: Record<string, unknown>, | |
| right: Record<string, unknown>, | |
| ) => left.surface_id === right.surface_id; | |
| scope.append( | |
| "agents", | |
| { agent_id: "pending-a", surface_id: "surface:a" }, | |
| sameSurface, | |
| ); | |
| scope.append( | |
| "agents", | |
| { agent_id: "agent-a", surface_id: "surface:a" }, | |
| sameSurface, | |
| ); | |
| scope.append( | |
| "agents", | |
| { agent_id: "agent-b", surface_id: "surface:b" }, | |
| sameSurface, | |
| ); | |
| expect(createdIdentityFromError(scope.attach(new Error("batch")))).toEqual({ | |
| agents: [ | |
| { agent_id: "agent-a", surface_id: "surface:a" }, | |
| { agent_id: "agent-b", surface_id: "surface:b" }, | |
| ], | |
| }); | |
| }); | |
| }); | |
| describe("CreatedIdentityScope", () => { | |
| it("attaches recorded identity without changing the error type or cause", () => { | |
| const cause = new Error("socket stderr"); | |
| const error = new UnclassifiedPostCreationError("later failure", { | |
| cause, | |
| }); | |
| const scope = new CreatedIdentityScope(); | |
| scope.record({ surface: "surface:7", workspace: "workspace:2" }); | |
| const attached = scope.attach(error); | |
| expect(attached).toBe(error); | |
| expect(attached).toBeInstanceOf(UnclassifiedPostCreationError); | |
| expect(attached.cause).toBe(cause); | |
| expect(createdIdentityFromError(attached)).toEqual({ | |
| surface: "surface:7", | |
| workspace: "workspace:2", | |
| }); | |
| }); | |
| it("does not invent identity before creation", () => { | |
| const scope = new CreatedIdentityScope(); | |
| expect(createdIdentityFromError(scope.attach(new Error("preflight")))).toEqual( | |
| {}, | |
| ); | |
| }); | |
| it("accumulates prior batch identities and updates the failing member", () => { | |
| const scope = new CreatedIdentityScope(); | |
| const sameSurface = ( | |
| left: Record<string, unknown>, | |
| right: Record<string, unknown>, | |
| ) => left.surface_id === right.surface_id; | |
| scope.append( | |
| "agents", | |
| { agent_id: "pending-a", surface_id: "surface:a" }, | |
| sameSurface, | |
| ); | |
| scope.append( | |
| "agents", | |
| { agent_id: "agent-a", surface_id: "surface:a" }, | |
| sameSurface, | |
| ); | |
| scope.append( | |
| "agents", | |
| { agent_id: "agent-b", surface_id: "surface:b" }, | |
| sameSurface, | |
| ); | |
| expect(createdIdentityFromError(scope.attach(new Error("batch")))).toEqual({ | |
| agents: [ | |
| { agent_id: "agent-a", surface_id: "surface:a" }, | |
| { agent_id: "agent-b", surface_id: "surface:b" }, | |
| ], | |
| }); | |
| }); | |
| it("keeps recorded identity authoritative over caller-supplied metadata", () => { | |
| const scope = new CreatedIdentityScope(); | |
| scope.record({ surface: "surface:7" }); | |
| const merged = { | |
| surface: "surface:stale", | |
| ...createdIdentityFromError(scope.attach(new Error("later failure"))), | |
| }; | |
| expect(merged.surface).toBe("surface:7"); | |
| }); | |
| it("wraps a non-Error throw and preserves it as the cause", () => { | |
| const scope = new CreatedIdentityScope(); | |
| scope.record({ surface: "surface:7" }); | |
| const attached = scope.attach("socket closed"); | |
| expect(attached).toBeInstanceOf(Error); | |
| expect(attached.cause).toBe("socket closed"); | |
| expect(createdIdentityFromError(attached)).toEqual({ | |
| surface: "surface:7", | |
| }); | |
| }); | |
| }); |
🤖 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/created-identity.test.ts` around lines 9 - 65, Add tests in the
CreatedIdentityScope suite covering both missing behaviors: verify
error-supplied identity metadata, including agent_id, takes precedence over
recorded identity, and verify attaching a non-Error value produces a new Error
with the original value as cause and preserves identity on the returned error.
Use attach and createdIdentityFromError to assert the documented outcomes.
| @@ -0,0 +1,22 @@ | |||
| # Hermetic CI snapshot of the launcher clauses cmuxlayer's spawn schema relies on. | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Record the fixture's provenance and refresh procedure.
This file is a hand-maintained snapshot of clauses from an out-of-repo launcher. tests/model-policy-drift.test.ts reads it whenever ~/.config/ralphtools/golem-dispatch.zsh is absent, which is the CI case. Nothing in the repository states which file it mirrors, when it was captured, or how to refresh it, so drift between the fixture and the real launcher is silent and undiagnosable from this file alone.
Extend the header comment with the source path, the capture date, and the refresh instruction.
♻️ Proposed header
-# Hermetic CI snapshot of the launcher clauses cmuxlayer's spawn schema relies on.
+# Hermetic CI snapshot of the launcher clauses cmuxlayer's spawn schema relies on.
+#
+# Source: ~/.config/ralphtools/golem-dispatch.zsh (not part of this repository)
+# Captured: 2026-08-12
+# Refresh: re-copy the `_claude_model` default branch, `_golem_parse_codex_flags`,
+# and `_golem_launch_cursor` clauses verbatim, then run
+# `tests/model-policy-drift.test.ts` on a machine that has the launcher
+# installed so the parity suite compares against the real file.
+# This file is read as text only. It is never executed.📝 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.
| # Hermetic CI snapshot of the launcher clauses cmuxlayer's spawn schema relies on. | |
| # Hermetic CI snapshot of the launcher clauses cmuxlayer's spawn schema relies on. | |
| # | |
| # Source: ~/.config/ralphtools/golem-dispatch.zsh (not part of this repository) | |
| # Captured: 2026-08-12 | |
| # Refresh: re-copy the `_claude_model` default branch, `_golem_parse_codex_flags`, | |
| # and `_golem_launch_cursor` clauses verbatim, then run | |
| # `tests/model-policy-drift.test.ts` on a machine that has the launcher | |
| # installed so the parity suite compares against the real file. | |
| # This file is read as text only. It is never executed. |
🤖 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/fixtures/golem-dispatch-contract.zsh` at line 1, Extend the header
comment in golem-dispatch-contract.zsh to document the mirrored launcher’s
source path, the fixture capture date, and the procedure for refreshing it from
the real launcher. Keep the fixture contents unchanged and make the provenance
and refresh guidance self-contained in the header.
| describe("hermetic golem-dispatch contract", () => { | ||
| it("keeps the CI fallback parseable and aligned", () => { | ||
| const fixture = readFileSync(contractFixturePath, "utf8"); | ||
| expect(parseClaudeDefault(fixture)).toBe( | ||
| MODEL_POLICY_CONTRACT.cli.claude.defaultModel, | ||
| ); | ||
| expect(parseCodexEffortValues(fixture)).toEqual(CODEX_EFFORT_VALUES); | ||
| expect(parseCursorLauncher(fixture)).toContain( | ||
| "_golem_refuse_agent_model_override", | ||
| ); | ||
| expect(fixture).toContain(MODEL_OVERRIDE_ENV); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the parser error messages now that both sources use them.
The new hermetic suite calls parseCodexEffortValues(fixture) and parseCursorLauncher(fixture). Those helpers report "could not parse Codex effort ladder from installed launcher" and "could not parse _golem_launch_cursor from golem-dispatch". When the fixture is the input, both messages name the wrong file and send a debugger to ~/.config/ralphtools/golem-dispatch.zsh instead of tests/fixtures/golem-dispatch-contract.zsh.
Pass the source label into the helpers, or state both candidate files in the message.
♻️ Proposed fix
-function parseCodexEffortValues(dispatchText: string): string[] {
+function parseCodexEffortValues(dispatchText: string, source: string): string[] {
const parser = dispatchText.match(
/_golem_parse_codex_flags\(\)\s*\{([\s\S]*?)\n\}/,
);
expect(
parser,
- "could not parse _golem_parse_codex_flags from installed launcher",
+ `could not parse _golem_parse_codex_flags from ${source}`,
).not.toBeNull();
const ladder = parser![1].match(
/\n\s*([a-z]+(?:\|[a-z]+)+)\)\s+_flag_codex_effort="\$2"/,
);
expect(
ladder,
- "could not parse Codex effort ladder from installed launcher",
+ `could not parse Codex effort ladder from ${source}`,
).not.toBeNull();
return ladder![1].split("|");
}Thread the same source argument through parseClaudeDefault and parseCursorLauncher, then pass contractFixturePath from the hermetic suite and dispatchPath from the parity suite.
🤖 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/model-policy-drift.test.ts` around lines 113 - 124, Update
parseCodexEffortValues and parseCursorLauncher to accept a source label and use
it in parse-failure messages, so errors identify the actual input file. Thread
the same source argument through parseClaudeDefault and parseCursorLauncher as
needed, passing contractFixturePath in the hermetic suite and dispatchPath in
the parity suite; preserve the existing parsing behavior.
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_42f7ed7a-9ee1-421f-9dcd-4815105b4884) |
|
Iteration pushed at
— cmuxlayerCodex (worker) · codex/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: 064c0ebbf2
ℹ️ 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 adjacentLine = lines.at(-2)?.trim() ?? ""; | ||
| return /(?:command not found|no such file(?: or directory)?|permission denied|traceback \(most recent call last\)|invalid (?:option|argument|model|effort))/i.test( | ||
| adjacentLine, |
There was a problem hiding this comment.
When the terminal already ends with an earlier command not found/permission denied followed by its prompt, the first post-submit read can still show that unchanged screen while the new launcher is starting. waitForAgentLaunchReady supplies no pre-submit baseline, so this matcher immediately attributes the stale adjacent line to the current command; spawn_agent then closes the valid surface and rolls back its worktree. Compare against the pre-submit snapshot or require the failure evidence to appear after submission.
Useful? React with 👍 / 👎.
| // Preserve the app-server's established contract: any decorated prompt | ||
| // ending in $, %, or # is ready. Pending input follows the terminator and | ||
| // therefore cannot match this suffix-only fallback. | ||
| if (/^.+[$%#]$/u.test(normalized)) { | ||
| return { input: "" }; |
There was a problem hiding this comment.
On the MCP spawn path, a fresh terminal whose startup output currently ends in text such as Downloading 100% or Cost: $ is now considered shell-ready by this unrestricted fallback. waitForLaunchShellReady can consequently send the launcher command while a startup program is still active, causing the input to be consumed or lost; the previous server matcher required recognizable prompt structure. Restrict this fallback to prompt-shaped lines rather than every line ending in $, %, or #.
Useful? React with 👍 / 👎.
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_ea7d66fd-f58c-48aa-a2b5-dbb99221e446) |
|
Delta round 2 fixes pushed at
— cmuxlayerCodex (worker) · codex/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: a9b2ea0d49
ℹ️ 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".
| @@ -0,0 +1,76 @@ | |||
| const SHELL_PROMPT_TERMINATOR = "[$%#>❯›»]"; | |||
There was a problem hiding this comment.
When a ready Gemini/Kiro/Claude/Codex screen ends in its normal bare >, ❯, or › composer and the adjacent CLI output contains a matched phrase such as permission denied, this terminator set makes launcherFailureFromShell treat the composer as a returned shell prompt. Both readiness loops check that failure before checking CLI readiness, so spawn_agent can close a valid agent surface and roll back its newly created worktree. Shell-exit detection needs to distinguish actual shell prompts from the CLI composer glyphs.
Useful? React with 👍 / 👎.
| surface_id: result.surface_id, | ||
| workspace_id: result.workspace_id ?? workspace ?? null, | ||
| }; | ||
| creation.record(activeSpawnIdentity); |
There was a problem hiding this comment.
When one spawn_in_workspace member succeeds and a later engine.spawnAgent call fails before on_surface_created runs, activeSpawnIdentity is correctly reset but the scope still retains the prior member's scalar fields from this call. Because err() spreads the recorded identity last, the failure response reports the already-successful first member as the top-level agent_id and surface_id for the later failure. Keep prior members only in agents, or clear the scalar identity at the start of each iteration.
Useful? React with 👍 / 👎.
1 participant
Summary
Verification
bun run typecheckbun run buildbun run test— 113 files passed; 2,597 tests passed; 1 intentional skiprun_tests.sh— passedspawn_agentCloses #340.
Closes #348.
Closes #349.
Closes #381.
— cmuxlayerCodex (worker) · codex/gpt-5.6-sol
Note
High Risk
Changes core MCP spawn_agent/new_split behavior including automatic surface close and worktree rollback on launcher failures; misclassification of shell state could close healthy tabs or roll back valid worktrees.
Overview
Hardens every MCP surface-creating spawn path so failures stay diagnosable and partially created resources can be recovered instead of leaving orphaned tabs, worktrees, or opaque timeouts.
Shared shell readiness moves prompt detection into
shell-prompt.ts(ASCII and Unicode terminators, strict vs loose matching) and uses it from the MCP server and app-server runtime. Readiness polling now treats a launcher that lands back on a shell with adjacent failure text asLauncherReadinessErrorwithlast_10_lines, and tightens pending-input detection so progress lines like62%are not mistaken for prompts.Created identity on errors adds
CreatedIdentityScopeto record workspace/surface/agent IDs right after cmux creation and attach them to thrown errors without changinginstanceoforcause; the sharederr()formatter merges that metadata (including batchedspawn_in_workspaceagents) into structured tool failures.Launcher submit verification and cleanup after Return, if readiness still fails and the screen still shows the launcher command at the prompt, the flow fails fast with terminal tail evidence. On launch-phase
AgentLaunchError/LauncherReadinessError,spawn_agentcloses the surface (stable UUID), marks the agent terminal when needed, and rolls back a newly created worktree/branch; generic timeouts still leave resources for manual recovery.AgentLaunchErrorgainslaunch_phase, standardError.cause, andon_surface_creatednow includesagent_id.Tests and CI add broad Vitest coverage for prompts, identity, rollback, and pending Return; model-policy drift tests use a committed
golem-dispatch-contract.zshfixture when the installed launcher is absent.Reviewed by Cursor Bugbot for commit a9b2ea0. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Note
Harden agent spawn failures with launcher diagnostics, rollback, and identity propagation
launcherFailureFromShellinshell-prompt.tsto detect early launcher exit-to-shell errors (command not found, permission denied, etc.) and surfacelast_10_linesin error responses.CreatedIdentityScopeincreated-identity.tsto attach structured identity (agent, surface, workspace IDs) to thrown errors without alteringinstanceoforcause.spawn_agentnow closes failed launcher surfaces and rolls back newly created worktrees/branches when aLauncherReadinessErroroccurs during the launch phase.screenShowsPendingShellInput: non-launcher contexts no longer treat arbitrary output ending in%/#/$as a shell prompt.shell-prompt.tsmodule, removing duplicate local implementations fromserver.tsandapp-server-runtime.ts.Macroscope summarized a9b2ea0.