fix: pre-push hook exit 0 on success path

[EtanHey]

Sister PR to voicelayer #182. cmux fork hook had same fall-off-end bug. Source: brainbar-85cb1121-307.

---

Summary by cubic

Fixes the pre-push git hook to exit 0 on success and adds a regression gate that runs a new test harness before pushes. Also introduces native MCP protocol support on the socket server and fixes a launch crash caused by Auto Layout cycles.

• New Features

  • Pre-push regression gate: .githooks/pre-push runs scripts/run_tests.sh; setup documented in CONTRIBUTING.
  • Native MCP support: detects Content-Length framing, implements initialize, tools/list, tools/call, and maps 20 tools to V2 methods; covered by unit tests.
  • IOSurface leak harness: tests/fixtures/rapid_spawn_kill.sh, tests/regression/test_iosurface_leak.sh, and an Xcode-driven fixture test integrated via scripts/run_tests.sh.

• Bug Fixes

  • Hook now exits 0 on success; warns and skips if the harness script is missing.
  • Prevents launch crash by deferring notification auth updates and skipping off-screen windows during layout.
  • Socket hardening: safe partial writes, require MCP initialize handshake, reject MCP when password auth is enabled, map MCP refs to *_id, parse multiple frames per read, close on malformed frames, and quote V1 args from MCP routes.

^{Written for commit 707a05e. Summary will update on new commits. Review in cubic}

Summary by CodeRabbit

Release Notes

• New Features

  • Added Model Context Protocol (MCP) support enabling tool-driven interactions over JSON-RPC messaging.

• Bug Fixes

  • Fixed Auto Layout constraint cycles triggered by off-screen window updates.
  • Improved authorization state update timing to prevent layout conflicts.

• Tests & Infrastructure

  • Added pre-push regression testing and memory leak detection framework.

[vercel]

@EtanHey is attempting to deploy a commit to the Manaflow Team on Vercel.

A member of the Team first needs to authorize it.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR introduces an MCP (Model Context Protocol) server implementation with JSON-RPC framing, adds pre-push regression testing via Git hooks and test scripts, implements IOSurface memory leak detection fixtures, and optimizes Auto Layout timing by deferring notification state updates and skipping off-screen window layout passes.

Changes

Cohort / File(s)	Summary
Pre-push Regression Gate .githooks/pre-push, scripts/run_tests.sh, CONTRIBUTING.md	Introduces Git pre-push hook that conditionally executes test script, blocking pushes on test failure. Adds test runner that verifies dependencies, initializes submodules, and runs XCTest suite and IOSurface leak regression tests.
MCP Server Implementation Sources/MCPServer.swift	Implements MCP-over-JSON-RPC protocol with MCPFraming for Content-Length parsing, MCPHandler for request routing (initialize, ping, tools/list, tools/call), and MCPToolSchemas mapping MCP tools to V2 methods with parameter conversion.
Socket Server MCP Integration Sources/TerminalController.swift	Adds per-connection MCP protocol detection on first read, parses framed MCP messages via MCPFraming, routes tool calls through V2 executor, implements binary-safe writeSocketData for partial writes and signal handling, and maintains fallback to existing V1/V2 command logic for non-MCP connections.
Auto Layout Timing Sources/TerminalNotificationStore.swift, Sources/Workspace.swift	Defers initial auth state refresh and subsequent updates via asyncAfter to prevent constraint update cycles; adds off-screen window layout skip in flushWorkspaceWindowLayouts for windows below Y threshold to prevent infinite Auto Layout loops.
Xcode Project Configuration GhosttyTabs.xcodeproj/project.pbxproj	Registers new Swift source files: MCPServer.swift in main target, MCPServerTests.swift and RapidSpawnKillFixtureTests.swift in test target.
IOSurface Leak Testing cmuxTests/MCPServerTests.swift, cmuxTests/RapidSpawnKillFixtureTests.swift, tests/fixtures/rapid_spawn_kill.sh, tests/fixtures/README.md, tests/regression/test_iosurface_leak.sh, tests/regression/README.md	Adds comprehensive MCP framing and handler tests, XCTest fixture for rapid spawn/kill cycles with IOSurface sampling, and bash regression test script measuring IOSurface allocations under leaks --atExit. Includes documentation for fixture behavior and environment variable configuration.

Sequence Diagram

sequenceDiagram
    participant Client as MCP Client
    participant TerminalCtrl as TerminalController
    participant MCPFrame as MCPFraming
    participant MCPHdlr as MCPHandler
    participant Schemas as MCPToolSchemas
    participant V2Exec as V2 Executor

    Client->>TerminalCtrl: Send Content-Length framed message
    TerminalCtrl->>MCPFrame: parse(data)
    MCPFrame-->>TerminalCtrl: ParseResult (complete/needMore/notMCP)
    
    alt Complete MCP Frame
        TerminalCtrl->>MCPHdlr: handleRequest(jsonRpcDict)
        
        alt Initialize Request
            MCPHdlr-->>TerminalCtrl: Server info + tool capabilities
            TerminalCtrl->>MCPFrame: encodeResponse(result)
            MCPFrame-->>TerminalCtrl: Content-Length framed JSON
            TerminalCtrl->>Client: Send framed response
        else Tools/List Request
            MCPHdlr->>Schemas: List available tools
            Schemas-->>MCPHdlr: Tool array
            MCPHdlr-->>TerminalCtrl: Tools response
            TerminalCtrl->>MCPFrame: encodeResponse(result)
            TerminalCtrl->>Client: Send framed response
        else Tools/Call Request
            MCPHdlr->>Schemas: route(toolName)
            Schemas-->>MCPHdlr: V2 method + param mapping
            MCPHdlr->>V2Exec: Execute mapped V2 method
            V2Exec-->>MCPHdlr: Result or error
            MCPHdlr-->>TerminalCtrl: MCP content response
            TerminalCtrl->>MCPFrame: encodeResponse(result)
            TerminalCtrl->>Client: Send framed response
        end
    else Non-MCP or Fallback
        TerminalCtrl->>TerminalCtrl: Route to V1/V2 command logic
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

• #1717: Both PRs modify Xcode project configuration to add new Swift source and test files, registering them in the build targets.
• #2398: Both PRs extend TerminalController socket handling and V2/JSON-RPC integration; this PR adds MCP protocol bridging while the related PR introduces V2 methods (surface.report_tty, surface.ports_kick) that MCP tools may dispatch to.

Poem

🐰 A testing rabbit hops with glee,
Pre-push checks guard quality,
MCP bridges dream to deed,
IOSurface leaks take heed,
Constraints dance, no infinite loops—hooray! 🎉

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is largely incomplete relative to the template. While it provides a comprehensive auto-generated summary of changes, it lacks key sections: minimal 'Summary' (what/why), no 'Testing' section, no demo video, missing checklist items, and review trigger instructions are absent.	Add 'Testing' section explaining how the fix was validated. Complete the checklist by checking boxes for local testing, test coverage, and documentation updates. Include or remove the review trigger block as appropriate.
Docstring Coverage	⚠️ Warning	Docstring coverage is 15.22% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main fix: the pre-push hook now correctly exits 0 on success, addressing the fall-off-end bug mentioned in the PR description.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

⚔️ Resolve merge conflicts

• Resolve merge conflict in branch fix/pre-push-exit0

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[greptile-apps]

Greptile Summary

This PR adds an MCP (Model Context Protocol) JSON-RPC 2.0 server to cmux, introduces a pre-push regression harness gate (scripts/run_tests.sh + .githooks/pre-push), fixes an Auto Layout infinite-constraint-cycle in TerminalNotificationStore and Workspace, and adds an IOSurface leak regression fixture.

• P1 — broken routing tests: testToolsCallRoutesToV2, testListSurfacesRouting, and testSendInputRouting in MCPServerTests.swift assert the MCP-side parameter names (surface, workspace) instead of the V2-mapped names (surface_id, workspace_id). These assertions evaluate nil == \"expected\" and will fail on CI.

Confidence Score: 3/5

Not safe to merge until the broken routing tests in MCPServerTests.swift are corrected.

A P1 defect — three XCTest assertions using wrong parameter key names that will always fail — directly breaks CI on the new MCPServer test suite. The rest of the change is well-structured, but the broken tests must be fixed first.

cmuxTests/MCPServerTests.swift — routing test parameter key names must be corrected before merge.

Important Files Changed

Filename	Overview
.githooks/pre-push	New pre-push hook that gates pushes on the regression harness; correctly includes explicit exit 0 on the success path (the stated fix).
Sources/MCPServer.swift	New MCP JSON-RPC 2.0 server with Content-Length framing, 20 tool schemas, and V2 method routing; implementation looks correct but MCP detection in TerminalController relies on the first read returning ≥15 bytes.
Sources/TerminalController.swift	Adds MCP protocol detection and dispatch alongside existing V1/V2 paths; detection relies solely on the first read() call returning ≥15 bytes, which is a fragile assumption.
cmuxTests/MCPServerTests.swift	Unit tests for MCPFraming and MCPHandler; multiple routing tests assert the wrong V2 parameter key names and will fail at runtime.
Sources/TerminalNotificationStore.swift	Defers authorization status updates via asyncAfter with hard-coded delays to avoid Auto Layout constraint cycles; fix is explained but fragile on very slow devices.
Sources/Workspace.swift	Adds a guard in flushWorkspaceWindowLayouts to skip invisible or off-screen SwiftUI system windows, preventing infinite Auto Layout constraint cycles.
scripts/run_tests.sh	New test runner orchestrating xcodebuild fixture test and IOSurface regression test; functionally exits 0 on success but lacks the explicit exit 0 that the hook it drives uses.
cmuxTests/RapidSpawnKillFixtureTests.swift	XCTest harness that runs rapid_spawn_kill.sh under leaks --atExit and asserts IOSurface footprint stays below a configurable threshold; exercises real runtime behavior.
tests/fixtures/rapid_spawn_kill.sh	Stress fixture that repeatedly spawns and kills cmux, samples IOSurface footprint via vmmap, and prints the peak value; clean, well-structured bash.
tests/regression/test_iosurface_leak.sh	Regression script running the fixture under leaks --atExit and sampling a live sentinel process; bytes_from_human is duplicated verbatim from rapid_spawn_kill.sh.

Comments Outside Diff (3)

1. cmuxTests/MCPServerTests.swift, line 1201-1202 (link)

   Routing test checks wrong parameter keys

   testToolsCallRoutesToV2 asserts executedParams?["surface"] but the read_screen route maps the MCP surface argument to the V2 key surface_id — executedParams["surface"] will always be nil, causing the assertion to fail. The same mismatch affects testListSurfacesRouting (workspace → workspace_id) and testSendInputRouting (surface → surface_id, workspace → workspace_id). All three tests will fail with nil is not equal to "<expected>" as written.

   

2. Sources/TerminalController.swift, line 1718-1720 (link)

   MCP detection silently falls back to V1 on short first read

   The detection window is only the first read() syscall. If that call returns fewer than 15 bytes (the length of "Content-Length:") — possible on any socket under load — firstBytes.starts(with:) returns false because Swift's implementation requires the receiver to be at least as long as the prefix. The connection silently drops into V1/V2 mode and the MCP session is lost. A safer approach is to defer the mode decision until enough bytes have accumulated, reusing the partial-prefix logic already in MCPFraming.parse.

   

3. scripts/run_tests.sh, line 1678-1680 (link)

   Missing explicit exit 0 on success path

   run_tests.sh exits cleanly in practice (a bash if with a false condition and no else exits 0), but the intent is ambiguous compared to the pre-push hook which the PR explicitly adds exit 0 to. Adding exit 0 makes the success path unambiguous and consistent with the stated fix.

   

_{Reviews (1): Last reviewed commit: "fix: pre-push hook exit 0 on success pat..." | Re-trigger Greptile}

[coderabbitai]

Actionable comments posted: 11

🧹 Nitpick comments (1)

cmuxTests/MCPServerTests.swift (1)

170-199: Consider documenting the expected tool count or making it more resilient.

The assertion XCTAssertEqual(tools.count, 20) will fail if any tools are added or removed. This is intentional as a regression guard, but a brief comment explaining why exactly 20 tools are expected would help future maintainers understand whether this is a strict requirement or needs updating when tools change.

Suggested improvement

     func testToolsListReturns20Tools() {
         let request = mcpRequest(id: 2, method: "tools/list", params: [:])
         let response = handler.handleRequest(request)!
         let result = response["result"] as! [String: Any]
         let tools = result["tools"] as! [[String: Any]]

+        // MCP spec exposes 20 tools; update this count when adding/removing tools
         XCTAssertEqual(tools.count, 20)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmuxTests/MCPServerTests.swift` around lines 170 - 199, The test
testToolsListReturns20Tools has a brittle assertion XCTAssertEqual(tools.count,
20) that will break when tools are added/removed; either document why 20 is a
strict requirement with a comment above that assertion referencing
testToolsListReturns20Tools and tools.count, or make the check resilient by
replacing the equality check with a minimum/threshold check (e.g.,
XCTAssertGreaterThanOrEqual(tools.count, 20)) and rely on the existing explicit
existence assertions for required tools (the toolNames contains checks) to
ensure critical tools remain present.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cmuxTests/RapidSpawnKillFixtureTests.swift`:
- Around line 148-152: The test is accidentally inheriting any exported
CMUX_RAPID_SPAWN_KILL_* variables from the parent environment when runProcess
builds mergedEnvironment; update runProcess (where mergedEnvironment is created
and assigned to process.environment) to first remove any keys matching the
CMUX_RAPID_SPAWN_KILL_* prefix (e.g. CMUX_RAPID_SPAWN_KILL_EXECUTABLE_PATH) from
ProcessInfo.processInfo.environment before applying the test's own environment
overrides so the fixture cannot be pointed at the wrong binary.
- Around line 168-185: The timeout handling may hang because process.terminate()
(SIGTERM) can be ignored and readDataToEndOfFile() will block; update the logic
around exitSignal, timedOut, and process.terminate() so that after the 2-second
grace period you check if the process is still running and then send a SIGKILL
(e.g., via process.interrupt/kill or posix_kill) and wait for
process.waitUntilExit()/exitSignal again, and/or close
stdoutPipe.fileHandleForReading and stderrPipe.fileHandleForReading before
calling readDataToEndOfFile() to ensure reads do not block; keep using
exitSignal and the timedOut flag but ensure you always wait for process
termination (or forcibly kill) before constructing ProcessResult.

In `@scripts/run_tests.sh`:
- Around line 14-18: DERIVED_DATA_PATH is currently a fixed shared directory
causing stale artifacts; change its default to create a unique per-run temp
directory (e.g., use mktemp -d with a /tmp/cmux-run-tests-derived.XXXX template)
and make BUILT_APP_PATH, FIXTURE_PATH, LEAK_TEST_PATH and any other uses (the
other blocks around the indicated ranges) reference that variable so each
invocation uses its own DerivedData; ensure any setup/cleanup logic that assumes
the old path is updated to handle the dynamic DERIVED_DATA_PATH.
- Line 13: The default DESTINATION value is hard-coded to
"platform=macOS,arch=arm64", which forces arm64 and breaks Intel Macs; update
the DESTINATION assignment in scripts/run_tests.sh (the DESTINATION variable) to
use only "platform=macOS" as the default (like other scripts), and apply the
same change for the other occurrences around lines 45-53 so xcodebuild can pick
the native architecture.

In `@Sources/MCPServer.swift`:
- Around line 166-170: The code currently coerces params["arguments"] to an
empty dictionary which hides protocol errors; change the handling so that if
params["arguments"] is absent you set toolArgs = [:], but if it is present and
not a [String: Any] you return jsonRPCError(id: id, code: -32602, message:
"Invalid 'arguments' - expected object"); implement this by replacing the let
toolArgs = ... line with a conditional cast/check on params["arguments"] and
returning the jsonRPCError when the value exists but fails to cast, referencing
the existing params, toolArgs variable, and jsonRPCError(id:code:message:) call.
- Around line 181-189: The current case .ok handling only JSON-serializes
[String: Any]; update it to emit stable JSON for any JSON-compatible top-level
value: first check JSONSerialization.isValidJSONObject(payload) and if true
serialize with JSONSerialization.data(withJSONObject:payload), otherwise detect
primitive JSON types (String, Int, Double, Bool, NSNull/Optional.none) and
serialize those by casting and encoding each with JSONEncoder (e.g., encode the
String/Int/Double/Bool value) to produce proper JSON text; if all serialization
attempts fail, fall back to String(describing: payload). Ensure these changes
replace the logic that used only a [String: Any] cast so v2Execute payloads that
are arrays or primitives are emitted as valid JSON.
- Around line 47-56: The parser currently accepts any non-negative
Content-Length which allows a peer to advertise arbitrarily large frames; add a
sanity cap (e.g. MAX_CONTENT_LENGTH or mcpMaxContentLength) and check it
immediately after parsing contentLength in the MCPServer.swift block (the guard
that binds headerStr and contentLength and before computing
bodyStart/totalNeeded); if contentLength is greater than the cap, reject the
frame (return the appropriate failure like .notMCP or an explicit invalid-frame
result used elsewhere) instead of falling through to the .needMore path so the
connection cannot force unbounded buffering.

In `@Sources/TerminalController.swift`:
- Around line 1603-1644: The code currently decides MCP vs NDJSON from a single
read by checking only the first chunk (firstBytes) which can split the
"Content-Length:" header; change this to accumulate an undecided preamble buffer
(e.g., mcpPreamble) by appending buffer[0..<bytesRead] while mcpDetected ==
false && mcpHandler == nil, and only decide MCP when either the preamble
contains the full "Content-Length:" prefix (check using the full prefix length)
or the preamble grows past a safe threshold (e.g., 64 bytes) or contains a
newline that proves it is NDJSON; when the prefix is detected set mcpDetected =
true and create mcpHandler and move any remaining bytes from mcpPreamble into
mcpBuffer, otherwise once ruled out stop accumulating and fall back to the
newline/NDJSON parser using the preserved preamble content (do not emit errors
for partial headers). Ensure all references use the existing symbols
mcpDetected, mcpHandler, mcpBuffer and replace firstBytes logic with the new
mcpPreamble accumulation and checks.
- Around line 2068-2085: The V1 MCP handlers for "report_meta" and
"set_progress" currently accept a params["surface"] and append "--surface=..."
which silently targets the active tab instead of the surface's owning workspace;
change the logic in the case "report_meta" and case "set_progress" blocks to
either reject surface (return .error(code: "invalid_params", message: "surface
not supported for v1")) or resolve the surface to its owning workspace ID and
pass "--tab=<workspace_id>" instead of "--surface=..."; implement this by
checking params["surface"] early in each case, calling your existing
workspace/surface lookup routine (or add a helper like
resolveOwnerWorkspace(forSurface:)) to get the workspace id, return an error if
resolution fails, and append "--tab=\(workspaceId)" (or error out) rather than
appending "--surface=\(…)".

In `@Sources/Workspace.swift`:
- Around line 8048-8051: The hardcoded offscreen Y threshold
(offscreenLayoutSkipYThreshold and the check using window.frame.origin.y >=
-2000) can incorrectly skip visible windows on tall/multi-display setups;
replace that heuristic with a proper on-screen test: determine whether the
window's frame intersects any connected NSScreen frame (or use window.screen and
convert the window rect to screen coordinates) and only treat it as off-screen
when it does not intersect any NSScreen bounds. Update the logic in Workspace
where offscreenLayoutSkipYThreshold is referenced (the window/frame check block)
to use this intersection test and remove or deprecate the -2000 constant. Ensure
the change handles nil window.screen and considers converted screen coordinates
so visible windows on stacked displays are not skipped.

In `@tests/regression/test_iosurface_leak.sh`:
- Around line 263-266: The success path in the test_iosurface_leak.sh script
currently logs a PASS but falls off the end of the main function without an
explicit exit code; add an explicit exit 0 on the successful branch (i.e.,
immediately after the log "PASS IOSurface footprint stayed within threshold" in
the main function) so the script returns a deterministic success status
consistent with the existing exit "$status"/exit 1 patterns.

---

Nitpick comments:
In `@cmuxTests/MCPServerTests.swift`:
- Around line 170-199: The test testToolsListReturns20Tools has a brittle
assertion XCTAssertEqual(tools.count, 20) that will break when tools are
added/removed; either document why 20 is a strict requirement with a comment
above that assertion referencing testToolsListReturns20Tools and tools.count, or
make the check resilient by replacing the equality check with a
minimum/threshold check (e.g., XCTAssertGreaterThanOrEqual(tools.count, 20)) and
rely on the existing explicit existence assertions for required tools (the
toolNames contains checks) to ensure critical tools remain present.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b23618fd-7437-4fc6-a39d-c77fd80f7558

📥 Commits

Reviewing files that changed from the base of the PR and between fac0ccc and 707a05e.

📒 Files selected for processing (14)

• .githooks/pre-push
• CONTRIBUTING.md
• GhosttyTabs.xcodeproj/project.pbxproj
• Sources/MCPServer.swift
• Sources/TerminalController.swift
• Sources/TerminalNotificationStore.swift
• Sources/Workspace.swift
• cmuxTests/MCPServerTests.swift
• cmuxTests/RapidSpawnKillFixtureTests.swift
• scripts/run_tests.sh
• tests/fixtures/README.md
• tests/fixtures/rapid_spawn_kill.sh
• tests/regression/README.md
• tests/regression/test_iosurface_leak.sh

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Clear inherited CMUX_RAPID_SPAWN_KILL_* overrides before launching the fixture.

runProcess merges the parent environment wholesale. In tests/fixtures/rapid_spawn_kill.sh, CMUX_RAPID_SPAWN_KILL_EXECUTABLE_PATH takes precedence over the app path, so an exported shell override can make this test pass against the wrong binary.

Suggested fix

         var mergedEnvironment = ProcessInfo.processInfo.environment
+        for key in [
+            "CMUX_RAPID_SPAWN_KILL_APP_PATH",
+            "CMUX_RAPID_SPAWN_KILL_EXECUTABLE_PATH",
+            "CMUX_RAPID_SPAWN_KILL_ITERATIONS",
+            "CMUX_RAPID_SPAWN_KILL_READY_TIMEOUT_MS",
+            "CMUX_RAPID_SPAWN_KILL_TMPDIR",
+        ] {
+            mergedEnvironment.removeValue(forKey: key)
+        }
         for (key, value) in environment {
             mergedEnvironment[key] = value
         }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	var mergedEnvironment = ProcessInfo.processInfo.environment
	for (key, value) in environment {
	mergedEnvironment[key] = value
	}
	process.environment = mergedEnvironment
	var mergedEnvironment = ProcessInfo.processInfo.environment
	for key in [
	"CMUX_RAPID_SPAWN_KILL_APP_PATH",
	"CMUX_RAPID_SPAWN_KILL_EXECUTABLE_PATH",
	"CMUX_RAPID_SPAWN_KILL_ITERATIONS",
	"CMUX_RAPID_SPAWN_KILL_READY_TIMEOUT_MS",
	"CMUX_RAPID_SPAWN_KILL_TMPDIR",
	] {
	mergedEnvironment.removeValue(forKey: key)
	}
	for (key, value) in environment {
	mergedEnvironment[key] = value
	}
	process.environment = mergedEnvironment

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmuxTests/RapidSpawnKillFixtureTests.swift` around lines 148 - 152, The test
is accidentally inheriting any exported CMUX_RAPID_SPAWN_KILL_* variables from
the parent environment when runProcess builds mergedEnvironment; update
runProcess (where mergedEnvironment is created and assigned to
process.environment) to first remove any keys matching the
CMUX_RAPID_SPAWN_KILL_* prefix (e.g. CMUX_RAPID_SPAWN_KILL_EXECUTABLE_PATH) from
ProcessInfo.processInfo.environment before applying the test's own environment
overrides so the fixture cannot be pointed at the wrong binary.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

❓ Verification inconclusive

Script executed:

git ls-files | grep -i "rapidspawn\|fixture.*test"

Repository: manaflow-ai/cmux

---

Repository: manaflow-ai/cmux
Exit code: 0

stdout:

cmuxTests/RapidSpawnKillFixtureTests.swift

---

Script executed:

fd -i rapidspawn

Repository: manaflow-ai/cmux

---

Repository: manaflow-ai/cmux
Exit code: 0

stdout:

cmuxTests/RapidSpawnKillFixtureTests.swift

---

Script executed:

fd "RapidSpawnKillFixtureTests" --type f

Repository: manaflow-ai/cmux

---

Repository: manaflow-ai/cmux
Exit code: 0

stdout:

cmuxTests/RapidSpawnKillFixtureTests.swift

---

Script executed:

find . -name "*RapidSpawn*" -o -name "*FixtureTest*" 2>/dev/null

Repository: manaflow-ai/cmux

---

Repository: manaflow-ai/cmux
Exit code: 0

stdout:

./cmuxTests/RapidSpawnKillFixtureTests.swift

---

Script executed:

wc -l ./cmuxTests/RapidSpawnKillFixtureTests.swift

Repository: manaflow-ai/cmux

---

Repository: manaflow-ai/cmux
Exit code: 0

stdout:

187 ./cmuxTests/RapidSpawnKillFixtureTests.swift

---

Script executed:

cat -n ./cmuxTests/RapidSpawnKillFixtureTests.swift | sed -n '150,200p'

Repository: manaflow-ai/cmux

---

Repository: manaflow-ai/cmux
Exit code: 0

stdout:

   150	            mergedEnvironment[key] = value
   151	        }
   152	        process.environment = mergedEnvironment
   153	        process.standardInput = FileHandle.nullDevice
   154	        process.standardOutput = stdoutPipe
   155	        process.standardError = stderrPipe
   156	
   157	        do {
   158	            try process.run()
   159	        } catch {
   160	            return ProcessResult(
   161	                status: -1,
   162	                stdout: "",
   163	                stderr: String(describing: error),
   164	                timedOut: false
   165	            )
   166	        }
   167	
   168	        let exitSignal = DispatchSemaphore(value: 0)
   169	        DispatchQueue.global(qos: .userInitiated).async {
   170	            process.waitUntilExit()
   171	            exitSignal.signal()
   172	        }
   173	
   174	        let timedOut = exitSignal.wait(timeout: .now() + timeout) == .timedOut
   175	        if timedOut {
   176	            process.terminate()
   177	            _ = exitSignal.wait(timeout: .now() + 2)
   178	        }
   179	
   180	        return ProcessResult(
   181	            status: process.terminationStatus,
   182	            stdout: String(data: stdoutPipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "",
   183	            stderr: String(data: stderrPipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "",
   184	            timedOut: timedOut
   185	        )
   186	    }
   187	}

---

Script executed:

cat -n ./cmuxTests/RapidSpawnKillFixtureTests.swift | sed -n '120,187p'

Repository: manaflow-ai/cmux

---

Repository: manaflow-ai/cmux
Exit code: 0

stdout:

   120	        return nil
   121	    }
   122	
   123	    private static func parseIOSurfaceFootprintMB(from output: String) -> Double? {
   124	        let pattern = #"VM: IOSurface\s*=\s*([0-9]+(?:\.[0-9]+)?)\s*MB"#
   125	        guard let regex = try? NSRegularExpression(pattern: pattern) else { return nil }
   126	        let range = NSRange(output.startIndex..<output.endIndex, in: output)
   127	        guard
   128	            let match = regex.firstMatch(in: output, range: range),
   129	            let valueRange = Range(match.range(at: 1), in: output)
   130	        else {
   131	            return nil
   132	        }
   133	        return Double(output[valueRange])
   134	    }
   135	
   136	    private func runProcess(
   137	        executablePath: String,
   138	        arguments: [String],
   139	        environment: [String: String],
   140	        timeout: TimeInterval
   141	    ) -> ProcessResult {
   142	        let process = Process()
   143	        let stdoutPipe = Pipe()
   144	        let stderrPipe = Pipe()
   145	
   146	        process.executableURL = URL(fileURLWithPath: executablePath)
   147	        process.arguments = arguments
   148	        var mergedEnvironment = ProcessInfo.processInfo.environment
   149	        for (key, value) in environment {
   150	            mergedEnvironment[key] = value
   151	        }
   152	        process.environment = mergedEnvironment
   153	        process.standardInput = FileHandle.nullDevice
   154	        process.standardOutput = stdoutPipe
   155	        process.standardError = stderrPipe
   156	
   157	        do {
   158	            try process.run()
   159	        } catch {
   160	            return ProcessResult(
   161	                status: -1,
   162	                stdout: "",
   163	                stderr: String(describing: error),
   164	                timedOut: false
   165	            )
   166	        }
   167	
   168	        let exitSignal = DispatchSemaphore(value: 0)
   169	        DispatchQueue.global(qos: .userInitiated).async {
   170	            process.waitUntilExit()
   171	            exitSignal.signal()
   172	        }
   173	
   174	        let timedOut = exitSignal.wait(timeout: .now() + timeout) == .timedOut
   175	        if timedOut {
   176	            process.terminate()
   177	            _ = exitSignal.wait(timeout: .now() + 2)
   178	        }
   179	
   180	        return ProcessResult(
   181	            status: process.terminationStatus,
   182	            stdout: String(data: stdoutPipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "",
   183	            stderr: String(data: stderrPipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "",
   184	            timedOut: timedOut
   185	        )
   186	    }
   187	}

---

The timeout enforcement is incomplete and can hang indefinitely.

The process.terminate() call sends only SIGTERM, which a process can ignore. After the 2-second grace period, readDataToEndOfFile() is called unconditionally. If the child process is still alive (refused SIGTERM or blocked on I/O), the call blocks indefinitely waiting for EOF, causing the helper to exceed the advertised timeout and leak subprocesses. Add SIGKILL after the grace period or close the pipes before reading.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmuxTests/RapidSpawnKillFixtureTests.swift` around lines 168 - 185, The
timeout handling may hang because process.terminate() (SIGTERM) can be ignored
and readDataToEndOfFile() will block; update the logic around exitSignal,
timedOut, and process.terminate() so that after the 2-second grace period you
check if the process is still running and then send a SIGKILL (e.g., via
process.interrupt/kill or posix_kill) and wait for
process.waitUntilExit()/exitSignal again, and/or close
stdoutPipe.fileHandleForReading and stderrPipe.fileHandleForReading before
calling readDataToEndOfFile() to ensure reads do not block; keep using
exitSignal and the timedOut flag but ensure you always wait for process
termination (or forcibly kill) before constructing ProcessResult.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n scripts/run_tests.sh | head -65

Repository: manaflow-ai/cmux

Length of output: 2765

---

🏁 Script executed:

# Check if there are other scripts or CI workflows that use this path simultaneously
rg -i "cmux-run-tests-derived|CMUX_RUN_TESTS_DERIVED_DATA_PATH" --type sh --type yaml --type yml

# Also check for any documentation about macOS destination format
rg -i "destination.*platform=macos" --type sh --type swift -A 2 -B 2 | head -40

Repository: manaflow-ai/cmux

Length of output: 1978

---

🏁 Script executed:

# Check if run_tests.sh is called from CI workflows that might run concurrently
rg -i "run_tests\.sh" --type yaml --type sh -B 2 -A 2

# Check for any locking mechanism or timestamping in the script
grep -n "lock\|timestamp\|mktemp\|unique" scripts/run_tests.sh

# Look for how other test scripts handle derived data
rg "DERIVED_DATA_PATH|derivedDataPath" scripts/ -A 1 -B 1

Repository: manaflow-ai/cmux

Length of output: 3130

---

Don't hard-code the default test destination to arm64.

platform=macOS,arch=arm64 makes the pre-push gate fail on Intel Macs. Align with other test scripts in the repo (test-unit.sh, run-tests-v1.sh, run-tests-v2.sh, reload.sh) which use platform=macOS to let xcodebuild select the native architecture.

Suggested fix

-DESTINATION="${CMUX_RUN_TESTS_DESTINATION:-platform=macOS,arch=arm64}"
+DESTINATION="${CMUX_RUN_TESTS_DESTINATION:-platform=macOS}"

Also applies to: 45-53

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/run_tests.sh` at line 13, The default DESTINATION value is hard-coded
to "platform=macOS,arch=arm64", which forces arm64 and breaks Intel Macs; update
the DESTINATION assignment in scripts/run_tests.sh (the DESTINATION variable) to
use only "platform=macOS" as the default (like other scripts), and apply the
same change for the other occurrences around lines 45-53 so xcodebuild can pick
the native architecture.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Use a unique DerivedData directory per run.

/tmp/cmux-run-tests-derived is shared across invocations. If a prior or concurrent run leaves a cmux DEV.app behind, the leak stage can end up exercising stale artifacts instead of the build produced by this invocation.

Suggested fix

-DERIVED_DATA_PATH="${CMUX_RUN_TESTS_DERIVED_DATA_PATH:-/tmp/cmux-run-tests-derived}"
+DERIVED_DATA_PATH_DEFAULT=0
+if [ -n "${CMUX_RUN_TESTS_DERIVED_DATA_PATH:-}" ]; then
+  DERIVED_DATA_PATH="$CMUX_RUN_TESTS_DERIVED_DATA_PATH"
+else
+  DERIVED_DATA_PATH="$(mktemp -d "${TMPDIR:-/tmp}/cmux-run-tests-derived.XXXXXX")"
+  DERIVED_DATA_PATH_DEFAULT=1
+fi
 BUILT_APP_PATH="$DERIVED_DATA_PATH/Build/Products/$CONFIGURATION/cmux DEV.app"
+
+cleanup() {
+  if [ "$DERIVED_DATA_PATH_DEFAULT" -eq 1 ]; then
+    rm -rf "$DERIVED_DATA_PATH"
+  fi
+}
+trap cleanup EXIT

Also applies to: 57-63, 88-98

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/run_tests.sh` around lines 14 - 18, DERIVED_DATA_PATH is currently a
fixed shared directory causing stale artifacts; change its default to create a
unique per-run temp directory (e.g., use mktemp -d with a
/tmp/cmux-run-tests-derived.XXXX template) and make BUILT_APP_PATH,
FIXTURE_PATH, LEAK_TEST_PATH and any other uses (the other blocks around the
indicated ranges) reference that variable so each invocation uses its own
DerivedData; ensure any setup/cleanup logic that assumes the old path is updated
to handle the dynamic DERIVED_DATA_PATH.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Cap accepted frame sizes before returning .needMore.

Right now any non-negative Content-Length is accepted, so a peer can advertise a huge body and make this connection buffer grow without bound. Please reject lengths above a sane maximum here instead of waiting indefinitely for the full payload.

Proposed fix

 enum MCPFraming {
+    private static let maxMessageBytes = 1 << 20
+
     /// Result of attempting to parse one MCP message from a buffer.
     enum ParseResult {
@@
-        guard let headerStr = String(data: headerData, encoding: .utf8),
-              let contentLength = Int(headerStr.trimmingCharacters(in: .whitespaces)),
-              contentLength >= 0 else {
+        guard let headerStr = String(data: headerData, encoding: .utf8),
+              let contentLength = Int(headerStr.trimmingCharacters(in: .whitespaces)),
+              contentLength >= 0,
+              contentLength <= maxMessageBytes else {
             return .notMCP
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/MCPServer.swift` around lines 47 - 56, The parser currently accepts
any non-negative Content-Length which allows a peer to advertise arbitrarily
large frames; add a sanity cap (e.g. MAX_CONTENT_LENGTH or mcpMaxContentLength)
and check it immediately after parsing contentLength in the MCPServer.swift
block (the guard that binds headerStr and contentLength and before computing
bodyStart/totalNeeded); if contentLength is greater than the cap, reject the
frame (return the appropriate failure like .notMCP or an explicit invalid-frame
result used elsewhere) instead of falling through to the .needMore path so the
connection cannot force unbounded buffering.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Serialize any JSON payload, not just dictionaries.

This only emits JSON text for [String: Any]. If v2Execute returns an array or other JSON-compatible top-level value, it falls back to String(describing:), which is not stable JSON and is much harder for MCP clients to consume.

Proposed fix

-            if let dict = payload as? [String: Any],
-               let jsonData = try? JSONSerialization.data(withJSONObject: dict, options: []),
+            if JSONSerialization.isValidJSONObject(payload),
+               let jsonData = try? JSONSerialization.data(withJSONObject: payload, options: [.sortedKeys]),
                let jsonStr = String(data: jsonData, encoding: .utf8) {
                 text = jsonStr
             } else {
                 text = String(describing: payload)
             }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	case .ok(let payload):
	let text: String
	if let dict = payload as? [String: Any],
	let jsonData = try? JSONSerialization.data(withJSONObject: dict, options: []),
	let jsonStr = String(data: jsonData, encoding: .utf8) {
	text = jsonStr
	} else {
	text = String(describing: payload)
	}
	case .ok(let payload):
	let text: String
	if JSONSerialization.isValidJSONObject(payload),
	let jsonData = try? JSONSerialization.data(withJSONObject: payload, options: [.sortedKeys]),
	let jsonStr = String(data: jsonData, encoding: .utf8) {
	text = jsonStr
	} else {
	text = String(describing: payload)
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/MCPServer.swift` around lines 181 - 189, The current case .ok
handling only JSON-serializes [String: Any]; update it to emit stable JSON for
any JSON-compatible top-level value: first check
JSONSerialization.isValidJSONObject(payload) and if true serialize with
JSONSerialization.data(withJSONObject:payload), otherwise detect primitive JSON
types (String, Int, Double, Bool, NSNull/Optional.none) and serialize those by
casting and encoding each with JSONEncoder (e.g., encode the
String/Int/Double/Bool value) to produce proper JSON text; if all serialization
attempts fail, fall back to String(describing: payload). Ensure these changes
replace the logic that used only a [String: Any] cast so v2Execute payloads that
are arrays or primitives are emitted as valid JSON.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Buffer enough bytes before locking into MCP mode.

This decides the protocol from a single read(). If the initial Content-Length: header is split across packets, the first chunk falls through to the newline parser, emits an unknown-command error for the partial header, and the connection is permanently desynchronized. Keep an undecided preamble buffer until you've either matched the full MCP prefix or seen enough data to rule it out.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/TerminalController.swift` around lines 1603 - 1644, The code
currently decides MCP vs NDJSON from a single read by checking only the first
chunk (firstBytes) which can split the "Content-Length:" header; change this to
accumulate an undecided preamble buffer (e.g., mcpPreamble) by appending
buffer[0..<bytesRead] while mcpDetected == false && mcpHandler == nil, and only
decide MCP when either the preamble contains the full "Content-Length:" prefix
(check using the full prefix length) or the preamble grows past a safe threshold
(e.g., 64 bytes) or contains a newline that proves it is NDJSON; when the prefix
is detected set mcpDetected = true and create mcpHandler and move any remaining
bytes from mcpPreamble into mcpBuffer, otherwise once ruled out stop
accumulating and fall back to the newline/NDJSON parser using the preserved
preamble content (do not emit errors for partial headers). Ensure all references
use the existing symbols mcpDetected, mcpHandler, mcpBuffer and replace
firstBytes logic with the new mcpPreamble accumulation and checks.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't silently ignore surface targeting in the V1 MCP bridge.

report_meta and set_progress only route by tab/workspace, but this bridge accepts surface and forwards --surface=... anyway. That returns success while mutating whichever tab is currently resolved, not the surface's owner. Either reject surface here or resolve it to the owning workspace and pass --tab=<workspace_id> instead.

♻️ Suggested fix

         case "report_meta":
             // Format: report_meta key value [--icon=X] [--color=#RRGGBB] [--surface=S]
             guard let key = params["key"] as? String, let value = params["value"] as? String else {
                 return .error(code: "invalid_params", message: "report_meta requires key and value")
             }
             args = "report_meta \(mcpQuoteV1(key)) \(mcpQuoteV1(value))"
             if let icon = params["icon"] as? String { args += " --icon=\(mcpQuoteV1(icon))" }
             if let color = params["color"] as? String { args += " --color=\(mcpQuoteV1(color))" }
-            if let surface = params["surface"] as? String { args += " --surface=\(mcpQuoteV1(surface))" }
+            if let surface = params["surface"] as? String {
+                guard let surfaceId = UUID(uuidString: surface) else {
+                    return .error(code: "invalid_params", message: "report_meta surface must be a UUID")
+                }
+                guard let located = v2MainSync({ AppDelegate.shared?.locateSurface(surfaceId: surfaceId) }) else {
+                    return .error(code: "not_found", message: "Surface not found")
+                }
+                args += " --tab=\(located.workspaceId.uuidString)"
+            }
         case "set_progress":
             // Format: set_progress value [--label=TEXT] [--surface=S]
             if let value = params["value"] as? Double {
                 args = "set_progress \(value)"
             } else if let value = params["value"] as? Int {
                 args = "set_progress \(value)"
             }
             if let label = params["label"] as? String { args += " --label=\(mcpQuoteV1(label))" }
-            if let surface = params["surface"] as? String { args += " --surface=\(mcpQuoteV1(surface))" }
+            if let surface = params["surface"] as? String {
+                guard let surfaceId = UUID(uuidString: surface) else {
+                    return .error(code: "invalid_params", message: "set_progress surface must be a UUID")
+                }
+                guard let located = v2MainSync({ AppDelegate.shared?.locateSurface(surfaceId: surfaceId) }) else {
+                    return .error(code: "not_found", message: "Surface not found")
+                }
+                args += " --tab=\(located.workspaceId.uuidString)"
+            }

Based on learnings: for panel_id-only routes in Sources/TerminalController.swift, resolve the real owner across windows instead of relying on the active window to avoid active-window bias.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/TerminalController.swift` around lines 2068 - 2085, The V1 MCP
handlers for "report_meta" and "set_progress" currently accept a
params["surface"] and append "--surface=..." which silently targets the active
tab instead of the surface's owning workspace; change the logic in the case
"report_meta" and case "set_progress" blocks to either reject surface (return
.error(code: "invalid_params", message: "surface not supported for v1")) or
resolve the surface to its owning workspace ID and pass "--tab=<workspace_id>"
instead of "--surface=..."; implement this by checking params["surface"] early
in each case, calling your existing workspace/surface lookup routine (or add a
helper like resolveOwnerWorkspace(forSurface:)) to get the workspace id, return
an error if resolution fails, and append "--tab=\(workspaceId)" (or error out)
rather than appending "--surface=\(…)".

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Hardcoded off-screen Y threshold can skip real visible windows

Line 8060 uses window.frame.origin.y >= -2000 as an off-screen filter. On vertically stacked multi-display setups, visible windows can legitimately be below -2000, so they’ll be skipped incorrectly.

Suggested fix

-    /// Windows positioned below this Y coordinate are treated as off-screen
-    /// SwiftUI system windows (settings, about, etc.) and skipped during layout
-    /// flushes to avoid triggering Auto Layout constraint cycles.
-    private static let offscreenLayoutSkipYThreshold: CGFloat = -2000
-
     private func flushWorkspaceWindowLayouts() {
         for window in NSApp.windows {
-            // Skip windows that are not visible or are off-screen SwiftUI
-            // system windows (settings, about, etc.). Calling
-            // layoutSubtreeIfNeeded on those can trigger an infinite
-            // constraint-update cycle (NSGenericException) when their
-            // SwiftUI layout has not converged.
-            guard window.isVisible, window.frame.origin.y >= Self.offscreenLayoutSkipYThreshold else { continue }
+            // Skip windows that are not visible or not on any connected screen.
+            // This avoids forcing layout on detached/system utility windows while
+            // keeping visible multi-display windows eligible for layout flush.
+            guard window.isVisible else { continue }
+            let isOnAnyScreen = NSScreen.screens.contains { $0.frame.intersects(window.frame) }
+            guard isOnAnyScreen else { continue }
             window.contentView?.layoutSubtreeIfNeeded()
             window.contentView?.displayIfNeeded()
         }
     }

Also applies to: 8055-8061

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/Workspace.swift` around lines 8048 - 8051, The hardcoded offscreen Y
threshold (offscreenLayoutSkipYThreshold and the check using
window.frame.origin.y >= -2000) can incorrectly skip visible windows on
tall/multi-display setups; replace that heuristic with a proper on-screen test:
determine whether the window's frame intersects any connected NSScreen frame (or
use window.screen and convert the window rect to screen coordinates) and only
treat it as off-screen when it does not intersect any NSScreen bounds. Update
the logic in Workspace where offscreenLayoutSkipYThreshold is referenced (the
window/frame check block) to use this intersection test and remove or deprecate
the -2000 constant. Ensure the change handles nil window.screen and considers
converted screen coordinates so visible windows on stacked displays are not
skipped.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Missing explicit exit 0 on success path — the same bug this PR fixes elsewhere.

The script logs "PASS" but falls off the end without an explicit exit 0. This is the identical "fall-off-end bug" pattern described in the PR objectives for the pre-push hook. The last command executed would be log "PASS ...", and while log (a printf) typically exits 0, relying on implicit exit status is fragile and inconsistent with the explicit exit 1 / exit "$status" patterns used elsewhere in this script.

Proposed fix

   log "PASS IOSurface footprint stayed within threshold"
+  exit 0
 }

 main "$@"

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	log "PASS IOSurface footprint stayed within threshold"
	}
	main "$@"
	log "PASS IOSurface footprint stayed within threshold"
	exit 0
	}
	main "$@"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/regression/test_iosurface_leak.sh` around lines 263 - 266, The success
path in the test_iosurface_leak.sh script currently logs a PASS but falls off
the end of the main function without an explicit exit code; add an explicit exit
0 on the successful branch (i.e., immediately after the log "PASS IOSurface
footprint stayed within threshold" in the main function) so the script returns a
deterministic success status consistent with the existing exit "$status"/exit 1
patterns.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 707a05e321

ℹ️ 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".

[chatgpt-codex-connector]

Detect MCP mode using buffered prefix matching

MCP detection currently checks only the current read() chunk with firstBytes.starts(with: "Content-Length:"). On stream sockets, the first read can legally contain only part of that header (for example Content-Len), which makes this branch miss MCP permanently and route the connection into newline mode; the MCP request then never gets parsed or answered. Detection needs to use accumulated bytes (or partial-prefix matching) to avoid intermittent MCP hangs.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Honor workspace when routing MCP set_status calls

This builder never maps the MCP workspace argument onto the V1 --tab option, so set_status writes metadata to the currently selected workspace instead of the workspace requested by the client. In multi-workspace sessions this causes status updates to land in the wrong workspace even when workspace is explicitly provided.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Remove hardcoded Y cutoff for visible window layout flush

The new guard skips any visible window whose frame origin is below -2000, but real user windows can have that coordinate on setups with a monitor positioned below the primary display (for example a 2160px-tall lower screen). In that case layout/display flushing is skipped for an active window, which can leave workspace UI geometry stale on that display.

Useful? React with 👍 / 👎.