debug socket: offscreen ground-truth surface screenshot (debug.surface.screenshot) - #9708
Closed
lawrencecchen wants to merge 5 commits into
Closed
lawrencecchen wants to merge 5 commits into
lawrencecchen wants to merge 5 commits into
Conversation
…e.screenshot) Add a focus-free, occlusion-proof, native-resolution capture path for the embedded Ghostty terminal, for the ghostty-web pixel-parity harness. Mechanism: the socket worker submits one forced tokened render through the new ghostty fork API ghostty_surface_request_render_with_token (renderer-thread mailbox, safe from any thread, deliberately ignores the occlusion gate). Libghostty acknowledges the token on the main thread in the same block that assigns the frame's IOSurface to the layer; the waiter then deep-copies that IOSurface, the exact bytes the compositor would composite for the terminal grid area, at native backing scale (2x on Retina), tagged Display P3 (the renderer's Metal target color space). No Screen Recording permission, no window-server involvement, never activates the app or reorders windows; works fully occluded, on another Space, or never ordered front. - ghostty submodule: manaflow-ai/ghostty#181 (tokened renderer-thread forced draw; must merge before this PR) - CmuxTerminal: install the render-presented callback per runtime surface; token waiter registry on TerminalSurface - debug.surface.screenshot params: surface_id, scale (default native), label/path; response includes pixel+point dims, native_scale, color_space, window_occlusion_visible, app_active, window_frame so callers can prove capture conditions - runs on the socket worker (blocks on a main-thread-delivered acknowledgment; main-actor execution would deadlock) - tests_v2/test_offscreen_surface_screenshot.py: standalone proof script (pattern write, overlay full occlusion, non-frontmost, 2x, Display P3, pixel-identical repeat captures, scale=1)
📝 WalkthroughWalkthroughThe PR adds a DEBUG-only ChangesSurface screenshot capture
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DebugClient
participant TerminalController
participant TerminalSurface
participant GhosttyTerminalView
participant PNGOutput
DebugClient->>TerminalController: Send debug.surface.screenshot
TerminalController->>TerminalSurface: Request presented frame token
TerminalSurface->>GhosttyTerminalView: Trigger tokened render
GhosttyTerminalView-->>TerminalSurface: Report presented frame
TerminalController->>GhosttyTerminalView: Copy presented IOSurface
TerminalController->>PNGOutput: Write PNG and capture metadata
PNGOutput-->>DebugClient: Return screenshot response
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 1 warning)
✅ Passed checks (20 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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
`@Packages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandExecutionPolicyTests.swift`:
- Line 36: Add an explicit assertion in fixedWorkerSetRunsOnTheSocketWorker for
debug.surface.screenshot confirming mainThreadCallable is false, alongside the
existing runsOnSocketWorker assertion. Ensure the test fails if this method is
added to mainThreadCallableSocketWorkerMethods.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 11326-11331: Update the IOSurface pixel-reading flow around
IOSurfaceLock to check its return status before registering the IOSurfaceUnlock
defer; return nil when locking fails. Also validate that IOSurfaceGetBaseAddress
returns a non-nil address before constructing Data, returning nil otherwise.
In `@Sources/TerminalController`+SurfaceScreenshot.swift:
- Around line 29-41: Document the `@unchecked` Sendable rationale for
V2SurfaceScreenshotFrame and V2SurfaceScreenshotOutcome, stating that their
payload is immutable after construction and handed off exactly once between the
main-thread callback and worker thread.
- Around line 100-111: Sanitize the untrusted label in the default output path
branch before interpolating it into the filename. Update the label-derived base
value near outputURL so path separators and traversal components such as “..”
cannot escape the cmux-screenshots directory, while preserving the explicitPath
behavior as a caller-selected destination.
In `@tests_v2/cmux.py`:
- Around line 1086-1098: Validate panel at the start of surface_screenshot
before calling _resolve_surface_id: reject None and empty-string values, while
preserving valid string and integer panel handling. Ensure invalid input exits
through the existing validation/error convention rather than passing a null
surface_id or resolving the focused surface.
In `@tests_v2/test_offscreen_surface_screenshot.py`:
- Around line 173-175: Update the default-socket guard in the offscreen
screenshot test to compare resolved paths on both sides, resolving the literal
default socket path as well as args.socket. Preserve the existing refusal
message and return code when the paths identify the same socket.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2b2abf9c-4bae-450f-845d-b602e015a0e5
📒 Files selected for processing (14)
Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlCommandExecutionPolicy.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandExecutionPolicyTests.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+Debug.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+PresentedFrameCapture.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+RuntimeLifecycle.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface.swiftSources/GhosttyTerminalView.swiftSources/TerminalController+DebugMethodNames.swiftSources/TerminalController+SurfaceScreenshot.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojghosttytests_v2/cmux.pytests_v2/test_offscreen_surface_screenshot.py
| "workspace.remote.pty_bridge", "workspace.env", "sidebar.custom.reload", | ||
| "sidebar.custom.open", | ||
| "debug.sidebar.simulate_drag", "debug.mobile.transport.disconnect", | ||
| "debug.surface.screenshot", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Pin mainThreadCallable: false for debug.surface.screenshot.
fixedWorkerSetRunsOnTheSocketWorker only asserts runsOnSocketWorker. The policy comment states that a main-thread run deadlocks for the full capture timeout. Add an explicit assertion so a later edit cannot add the method to mainThreadCallableSocketWorkerMethods without failing a test.
💚 Proposed assertion
`@Test` func onlyPureProbesAreMainThreadCallable() {
`#expect`(ControlCommandExecutionPolicy(forMethod: "system.ping") == .socketWorker(mainThreadCallable: true))
`#expect`(ControlCommandExecutionPolicy(forMethod: "system.capabilities") == .socketWorker(mainThreadCallable: true))
`#expect`(ControlCommandExecutionPolicy(forMethod: "system.top") == .socketWorker(mainThreadCallable: false))
+ // A main-thread run would block on the renderer's main-thread
+ // presented-frame acknowledgment for the whole capture timeout.
+ `#expect`(ControlCommandExecutionPolicy(forMethod: "debug.surface.screenshot") == .socketWorker(mainThreadCallable: false))🤖 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
`@Packages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandExecutionPolicyTests.swift`
at line 36, Add an explicit assertion in fixedWorkerSetRunsOnTheSocketWorker for
debug.surface.screenshot confirming mainThreadCallable is false, alongside the
existing runsOnSocketWorker assertion. Ensure the test fails if this method is
added to mainThreadCallableSocketWorkerMethods.
Comment on lines
+11326
to
+11331
| IOSurfaceLock(surfaceRef, [.readOnly], nil) | ||
| defer { IOSurfaceUnlock(surfaceRef, [.readOnly], nil) } | ||
|
|
||
| let base = IOSurfaceGetBaseAddress(surfaceRef) | ||
| let size = bytesPerRow * height | ||
| let data = Data(bytes: base, count: size) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle IOSurfaceLock failure before reading pixels.
IOSurfaceLock can fail. The current path then reads the base address and unlocks an IOSurface that was not successfully locked. Return nil unless the lock succeeds and the base address exists.
Proposed fix
- IOSurfaceLock(surfaceRef, [.readOnly], nil)
+ guard IOSurfaceLock(surfaceRef, [.readOnly], nil) == KERN_SUCCESS,
+ let base = IOSurfaceGetBaseAddress(surfaceRef) else {
+ return nil
+ }
defer { IOSurfaceUnlock(surfaceRef, [.readOnly], nil) }
- let base = IOSurfaceGetBaseAddress(surfaceRef)
let size = bytesPerRow * height#!/bin/bash
set -euo pipefail
sdk_root="$(xcrun --sdk macosx --show-sdk-path)"
fd -a 'IOSurface*.h' "$sdk_root" | while IFS= read -r header; do
rg -n -C 3 'IOSurfaceLock|IOSurfaceGetBaseAddress|IOSurfaceUnlock' "$header"
done🤖 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 `@Sources/GhosttyTerminalView.swift` around lines 11326 - 11331, Update the
IOSurface pixel-reading flow around IOSurfaceLock to check its return status
before registering the IOSurfaceUnlock defer; return nil when locking fails.
Also validate that IOSurfaceGetBaseAddress returns a non-nil address before
constructing Data, returning nil otherwise.
Comment on lines
+29
to
+41
| private struct V2SurfaceScreenshotFrame: @unchecked Sendable { | ||
| let surfaceId: UUID | ||
| let image: CGImage | ||
| let backingScale: CGFloat | ||
| let windowOcclusionVisible: Bool | ||
| let appActive: Bool | ||
| let windowFrame: CGRect? | ||
| } | ||
|
|
||
| private enum V2SurfaceScreenshotOutcome: @unchecked Sendable { | ||
| case frame(V2SurfaceScreenshotFrame) | ||
| case failure(code: String, message: String) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Document the @unchecked Sendable rationale.
The coding guidelines require a clear safety explanation for @unchecked Sendable. V2SurfaceScreenshotFrame and V2SurfaceScreenshotOutcome carry CGImage and CGRect across the main-thread callback into the worker thread. Add one sentence that states the payload is immutable after construction and is handed off exactly once.
As per coding guidelines: "Do not mark shared mutable reference types as Sendable unless they use actor isolation, MainActor isolation, a lock with a documented rationale, or @unchecked Sendable with a clear safety explanation."
🤖 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 `@Sources/TerminalController`+SurfaceScreenshot.swift around lines 29 - 41,
Document the `@unchecked` Sendable rationale for V2SurfaceScreenshotFrame and
V2SurfaceScreenshotOutcome, stating that their payload is immutable after
construction and handed off exactly once between the main-thread callback and
worker thread.
Source: Coding guidelines
Comment on lines
+100
to
+111
| let outputURL: URL | ||
| if !explicitPath.isEmpty { | ||
| outputURL = URL(fileURLWithPath: (explicitPath as NSString).expandingTildeInPath) | ||
| } else { | ||
| let outputDir = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent("cmux-screenshots") | ||
| try? FileManager.default.createDirectory(at: outputDir, withIntermediateDirectories: true) | ||
| let timestampMs = Int(Date().timeIntervalSince1970 * 1000) | ||
| let shortId = String(UUID().uuidString.prefix(8)) | ||
| let base = label.isEmpty ? "surface" : label | ||
| outputURL = outputDir.appendingPathComponent("\(base)-\(timestampMs)-\(shortId).png") | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Sanitize label before you use it in the output filename.
label comes straight from the socket request and is interpolated into the file name. A label such as ../../evil or a/b escapes cmux-screenshots and writes the PNG to an arbitrary path. explicitPath has the same property, but that parameter is explicitly a caller-chosen destination; label is not. Restrict label to a safe character set, or reject any label that contains a path separator or ...
🛡️ Proposed fix
- let base = label.isEmpty ? "surface" : label
+ let safeLabel = label.filter { $0.isLetter || $0.isNumber || $0 == "-" || $0 == "_" }
+ let base = safeLabel.isEmpty ? "surface" : safeLabel
outputURL = outputDir.appendingPathComponent("\(base)-\(timestampMs)-\(shortId).png")📝 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
| let outputURL: URL | |
| if !explicitPath.isEmpty { | |
| outputURL = URL(fileURLWithPath: (explicitPath as NSString).expandingTildeInPath) | |
| } else { | |
| let outputDir = FileManager.default.temporaryDirectory | |
| .appendingPathComponent("cmux-screenshots") | |
| try? FileManager.default.createDirectory(at: outputDir, withIntermediateDirectories: true) | |
| let timestampMs = Int(Date().timeIntervalSince1970 * 1000) | |
| let shortId = String(UUID().uuidString.prefix(8)) | |
| let base = label.isEmpty ? "surface" : label | |
| outputURL = outputDir.appendingPathComponent("\(base)-\(timestampMs)-\(shortId).png") | |
| } | |
| let outputURL: URL | |
| if !explicitPath.isEmpty { | |
| outputURL = URL(fileURLWithPath: (explicitPath as NSString).expandingTildeInPath) | |
| } else { | |
| let outputDir = FileManager.default.temporaryDirectory | |
| .appendingPathComponent("cmux-screenshots") | |
| try? FileManager.default.createDirectory(at: outputDir, withIntermediateDirectories: true) | |
| let timestampMs = Int(Date().timeIntervalSince1970 * 1000) | |
| let shortId = String(UUID().uuidString.prefix(8)) | |
| let safeLabel = label.filter { $0.isLetter || $0.isNumber || $0 == "-" || $0 == "_" } | |
| let base = safeLabel.isEmpty ? "surface" : safeLabel | |
| outputURL = outputDir.appendingPathComponent("\(base)-\(timestampMs)-\(shortId).png") | |
| } |
🤖 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 `@Sources/TerminalController`+SurfaceScreenshot.swift around lines 100 - 111,
Sanitize the untrusted label in the default output path branch before
interpolating it into the filename. Update the label-derived base value near
outputURL so path separators and traversal components such as “..” cannot escape
the cmux-screenshots directory, while preserving the explicitPath behavior as a
caller-selected destination.
Comment on lines
+1086
to
+1098
| panel: Union[str, int], | ||
| scale: Optional[float] = None, | ||
| label: str = "", | ||
| path: str = "", | ||
| timeout_s: float = 20.0, | ||
| ) -> dict: | ||
| """Ground-truth capture of one terminal surface via the renderer's own | ||
| presented IOSurface (debug.surface.screenshot). Works while the window | ||
| is occluded, on another Space, or never ordered front; never focuses or | ||
| reorders anything. Native backing scale (2x on Retina) by default; | ||
| pixels are Display P3 (the PNG is tagged accordingly).""" | ||
| sid = self._resolve_surface_id(panel) | ||
| params: Dict[str, Any] = {"surface_id": sid} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject an invalid panel before resolving the surface.
_resolve_surface_id(None) selects the focused surface. An empty string returns None. As written, surface_screenshot(None) can capture the wrong surface, and an empty panel sends "surface_id": null.
Reject missing or empty panels before calling _resolve_surface_id.
Proposed fix
) -> dict:
"""Ground-truth capture of one terminal surface via the renderer's own
presented IOSurface (debug.surface.screenshot). Works while the window
is occluded, on another Space, or never ordered front; never focuses or
reorders anything. Native backing scale (2x on Retina) by default;
pixels are Display P3 (the PNG is tagged accordingly)."""
+ if panel is None:
+ raise cmuxError("surface_screenshot requires a surface")
sid = self._resolve_surface_id(panel)
+ if not sid:
+ raise cmuxError(f"Invalid surface: {panel!r}")
params: Dict[str, Any] = {"surface_id": sid}🤖 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_v2/cmux.py` around lines 1086 - 1098, Validate panel at the start of
surface_screenshot before calling _resolve_surface_id: reject None and
empty-string values, while preserving valid string and integer panel handling.
Ensure invalid input exits through the existing validation/error convention
rather than passing a null surface_id or resolving the focused surface.
Comment on lines
+173
to
+175
| if os.path.realpath(args.socket) == "/tmp/cmux-debug.sock": | ||
| print("FAIL: refusing to run against the user's default socket", file=sys.stderr) | ||
| return 2 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
The default-socket guard never fires on macOS.
os.path.realpath resolves symlinks. On macOS /tmp is a symlink to /private/tmp, so os.path.realpath("/tmp/cmux-debug.sock") returns /private/tmp/cmux-debug.sock. That value never equals the literal /tmp/cmux-debug.sock, so the script proceeds against the user's own instance. The script then sends a sleep 300 and a ctrl-c into a live pane. Resolve both sides before you compare.
🐛 Proposed fix
- if os.path.realpath(args.socket) == "/tmp/cmux-debug.sock":
+ if os.path.realpath(args.socket) == os.path.realpath("/tmp/cmux-debug.sock"):
print("FAIL: refusing to run against the user's default socket", file=sys.stderr)
return 2📝 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
| if os.path.realpath(args.socket) == "/tmp/cmux-debug.sock": | |
| print("FAIL: refusing to run against the user's default socket", file=sys.stderr) | |
| return 2 | |
| if os.path.realpath(args.socket) == os.path.realpath("/tmp/cmux-debug.sock"): | |
| print("FAIL: refusing to run against the user's default socket", file=sys.stderr) | |
| return 2 |
🤖 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_v2/test_offscreen_surface_screenshot.py` around lines 173 - 175, Update
the default-socket guard in the offscreen screenshot test to compare resolved
paths on both sides, resolving the literal default socket path as well as
args.socket. Preserve the existing refusal message and return code when the
paths identify the same socket.
…-screenshot # Conflicts: # Packages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandExecutionPolicyTests.swift # Sources/TerminalController.swift # ghostty
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3b1cf0f. Configure here.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
… offscreen render)
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 11327-11330: Update debugCopyPresentedFrameImage() to read
contents directly from modelLayer and remove the presentation-layer fallback,
ensuring the returned image uses the acknowledged IOSurface associated with the
render request token.
In `@Sources/TerminalController`+SurfaceScreenshot.swift:
- Around line 150-157: The surface lookup in the terminal panel flow should not
require the selected tab for UUID targets. Update the logic around
resolveSurfaceId and terminalInputTarget(forPanelID:) to use
AppDelegate.shared?.locateSurface(surfaceId:) for UUID-based surface resolution,
while retaining selected-tab index aliases only where their scoped behavior is
intentional.
In `@tests_v2/test_offscreen_surface_screenshot.py`:
- Around line 254-255: Bound the readiness wait around overlay.stdout in the
offscreen surface screenshot test instead of calling blocking readline()
directly. Wait until a readiness line is available using a deadline, then fail
with a clear timeout error if Swift compilation or helper startup stalls;
preserve the existing readiness-line processing when output arrives.
- Around line 211-217: Update the shell script in the screenshot test to replace
the fixed `sleep 300` in the command built by the test with `read -r _`. Ensure
the existing Ctrl-C cleanup releases the waiting read so capture behavior
remains deterministic without a fixed-duration delay.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e604aa1b-bb55-4a48-addd-cd56e0377bd7
📒 Files selected for processing (14)
Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlCommandExecutionPolicy.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandExecutionPolicyTests.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+Debug.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+PresentedFrameCapture.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+RuntimeLifecycle.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface.swiftSources/GhosttyTerminalView.swiftSources/TerminalController+DebugMethodNames.swiftSources/TerminalController+SurfaceScreenshot.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojghosttytests_v2/cmux.pytests_v2/test_offscreen_surface_screenshot.py
Comment on lines
+11327
to
+11330
| func debugCopyPresentedFrameImage() -> DebugPresentedFrameImage? { | ||
| guard let modelLayer = surfaceView.layer else { return nil } | ||
| let layer = modelLayer.presentation() ?? modelLayer | ||
| guard let contents = layer.contents else { return nil } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Read the acknowledged model-layer IOSurface.
Line 11329 prefers modelLayer.presentation(). That layer can still reference the preceding frame when the callback runs. The callback contract only guarantees that modelLayer.contents has the acknowledged IOSurface. Capture modelLayer.contents directly so the response matches the tokened render request.
Proposed fix
- let layer = modelLayer.presentation() ?? modelLayer
- guard let contents = layer.contents else { return nil }
+ guard let contents = modelLayer.contents else { return nil }
...
- backingScale: max(1.0, layer.contentsScale)
+ backingScale: max(1.0, modelLayer.contentsScale)📝 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
| func debugCopyPresentedFrameImage() -> DebugPresentedFrameImage? { | |
| guard let modelLayer = surfaceView.layer else { return nil } | |
| let layer = modelLayer.presentation() ?? modelLayer | |
| guard let contents = layer.contents else { return nil } | |
| func debugCopyPresentedFrameImage() -> DebugPresentedFrameImage? { | |
| guard let modelLayer = surfaceView.layer else { return nil } | |
| guard let contents = modelLayer.contents else { return nil } |
🤖 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 `@Sources/GhosttyTerminalView.swift` around lines 11327 - 11330, Update
debugCopyPresentedFrameImage() to read contents directly from modelLayer and
remove the presentation-layer fallback, ensuring the returned image uses the
acknowledged IOSurface associated with the render request token.
Comment on lines
+150
to
+157
| guard let tabId = tabManager.selectedTabId, | ||
| let tab = tabManager.tabs.first(where: { $0.id == tabId }) else { | ||
| finish(.failure(code: "not_found", message: "No tab selected")) | ||
| return | ||
| } | ||
| guard let panelId = resolveSurfaceId(from: surfaceArg, tab: tab), | ||
| let terminalPanel = tab.terminalInputTarget(forPanelID: panelId)?.panel else { | ||
| finish(.failure(code: "not_found", message: "Terminal surface not found")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve surface_id without selected-tab bias.
This code only searches the selected tab. A valid UUID for a surface in another tab or window returns not_found, so the API cannot capture the requested occluded or inactive surface.
Resolve UUID targets through the shared structured surface locator before obtaining the terminal panel. Keep index aliases only when their selected-tab scope is intentional and documented.
Based on learnings: panel-only routes must fall back to AppDelegate.shared?.locateSurface(surfaceId:) to avoid active-window bias.
🤖 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 `@Sources/TerminalController`+SurfaceScreenshot.swift around lines 150 - 157,
The surface lookup in the terminal panel flow should not require the selected
tab for UUID targets. Update the logic around resolveSurfaceId and
terminalInputTarget(forPanelID:) to use
AppDelegate.shared?.locateSurface(surfaceId:) for UUID-based surface resolution,
while retaining selected-tab index aliases only where their scoped behavior is
intentional.
Source: Learnings
Comment on lines
+211
to
+217
| script = ( | ||
| "clear; " | ||
| "for i in 1 2 3 4; do printf '\\e[48;2;255;0;0m%80s\\e[0m\\\\n' ''; done; " | ||
| "for i in 1 2 3 4; do printf '\\e[48;2;0;255;0m%80s\\e[0m\\\\n' ''; done; " | ||
| "for i in 1 2 3 4; do printf '\\e[48;2;0;0;255m%80s\\e[0m\\\\n' ''; done; " | ||
| f"printf '{SENTINEL}\\\\n'; printf '\\e[?25l'; sleep 300" | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Replace the fixed shell sleep with an event wait.
Line 216 uses sleep 300 to hold the shell. Use read -r _ and release it with the existing Ctrl-C cleanup. This keeps capture determinism without a fixed wall-clock wait.
Proposed fix
- f"printf '{SENTINEL}\\\\n'; printf '\\e[?25l'; sleep 300"
+ f"printf '{SENTINEL}\\\\n'; printf '\\e[?25l'; read -r _"As per coding guidelines, “Do not use fixed sleeps” in correctness tests.
📝 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
| script = ( | |
| "clear; " | |
| "for i in 1 2 3 4; do printf '\\e[48;2;255;0;0m%80s\\e[0m\\\\n' ''; done; " | |
| "for i in 1 2 3 4; do printf '\\e[48;2;0;255;0m%80s\\e[0m\\\\n' ''; done; " | |
| "for i in 1 2 3 4; do printf '\\e[48;2;0;0;255m%80s\\e[0m\\\\n' ''; done; " | |
| f"printf '{SENTINEL}\\\\n'; printf '\\e[?25l'; sleep 300" | |
| ) | |
| script = ( | |
| "clear; " | |
| "for i in 1 2 3 4; do printf '\\e[48;2;255;0;0m%80s\\e[0m\\\\n' ''; done; " | |
| "for i in 1 2 3 4; do printf '\\e[48;2;0;255;0m%80s\\e[0m\\\\n' ''; done; " | |
| "for i in 1 2 3 4; do printf '\\e[48;2;0;0;255m%80s\\e[0m\\\\n' ''; done; " | |
| f"printf '{SENTINEL}\\\\n'; printf '\\e[?25l'; read -r _" | |
| ) |
🤖 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_v2/test_offscreen_surface_screenshot.py` around lines 211 - 217, Update
the shell script in the screenshot test to replace the fixed `sleep 300` in the
command built by the test with `read -r _`. Ensure the existing Ctrl-C cleanup
releases the waiting read so capture behavior remains deterministic without a
fixed-duration delay.
Source: Coding guidelines
Comment on lines
+254
to
+255
| assert overlay.stdout is not None | ||
| ready_line = overlay.stdout.readline().strip() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Bound the overlay readiness wait.
readline() can block forever if Swift compilation or helper startup stalls. Wait for stdout readiness with a deadline, then fail the test with a clear error.
Proposed fix
+import select
...
- ready_line = overlay.stdout.readline().strip()
+ ready, _, _ = select.select([overlay.stdout], [], [], 20)
+ if not ready:
+ print("FAIL: overlay helper did not become ready", file=sys.stderr)
+ return 2
+ ready_line = overlay.stdout.readline().strip()As per coding guidelines, completion signals must use deadline-bounded waits.
📝 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
| assert overlay.stdout is not None | |
| ready_line = overlay.stdout.readline().strip() | |
| assert overlay.stdout is not None | |
| ready, _, _ = select.select([overlay.stdout], [], [], 20) | |
| if not ready: | |
| print("FAIL: overlay helper did not become ready", file=sys.stderr) | |
| return 2 | |
| ready_line = overlay.stdout.readline().strip() |
🤖 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_v2/test_offscreen_surface_screenshot.py` around lines 254 - 255, Bound
the readiness wait around overlay.stdout in the offscreen surface screenshot
test instead of calling blocking readline() directly. Wait until a readiness
line is available using a deadline, then fail with a clear timeout error if
Swift compilation or helper startup stalls; preserve the existing readiness-line
processing when output arrives.
Source: Coding guidelines
This was referenced Aug 9, 2026
This was referenced Sep 11, 2026
lawrencecchen
added
the
stale-revisit
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
debug.surface.screenshot, a focus-free, occlusion-proof, native-resolution ground-truth capture of one embedded Ghostty terminal surface over the debug socket, for the ghostty-web pixel-parity harness. The existingdebug.window.screenshotcaptures at 1x viaCGWindowListCreateImage(.nominalResolution) and goes stale when the window is occluded, forcing the harness to raise the tagged window over the user's work.Chosen design and why
Option (a), render-on-demand with direct read-back, implemented through the renderer's own IOSurface swap chain rather than an offscreen texture, because the fork's renderer already presents by assigning IOSurfaces to the layer (
ghostty/src/renderer/metal/IOSurfaceLayer.zig): reading the presented IOSurface IS the compositor's input, byte for byte, so no second render path can drift from what the window would show. Option (b) (occlusion-override + ScreenCaptureKit) was rejected: SCScreenshotManager needs Screen Recording TCC consent per tagged bundle id (every dev tag is a fresh TCC identity, so every parity build would prompt the user), and it captures display-profile-converted output rather than the renderer's own pixels.Mechanism, end to end:
ghostty_surface_request_render_with_token(renderer: thread-safe tokened forced draw for offscreen capture ghostty#181). It fills a single-slot pending presentation on the renderer thread and rings the existingdraw_nowasync, so it is thread-safe while the renderer OS thread is live, unlikeghostty_surface_render_now_with_token(iOS external-drain only). The forced draw rebuilds frame data from current terminal state (updateFrame) and deliberately skips the occlusionflags.visiblegate.IOSurfaceLayer.setSurfaceCallback), so the waiter's read oflayer.contentsis sequenced strictly after the assignment. No sleeps, no polling: the sync primitive is the renderer's own frame-completion callback (requirement 3).Target.zigcreates its render targets withdisplayP3; the leased-frame API enum documents the same). The PNG is written through ImageIO so the P3 ICC profile is embedded. The harness already converts P3 desktop captures to sRGB; it can keep doing so, now keyed off the response'scolor_space: "display-p3".scaleresamples in P3 (scale: 1gives logical-point resolution). The response carrieswidth_px/height_px,points_width/points_height,native_scale, plus capture-time evidence (window_occlusion_visible,app_active,window_frame) so callers can prove the window was occluded and the app inactive at the moment of capture.The path never activates the app, never changes key window, never reorders windows, and needs no Screen Recording permission (it never touches the window server).
Invocation
Raw wire form:
{"id":1,"method":"debug.surface.screenshot","params":{"surface_id":"<uuid|index>","scale":2,"path":"/tmp/out.png"}}.Proof
tests_v2/test_offscreen_surface_screenshot.py --socket /tmp/cmux-debug-oscap.sock(standalone; launches nothing but a tiny swift overlay window used as the occluder, killed on exit):read_textwindow_occlusion_visible == falsescale=1returns logical-size outputRun against the tagged
oscapbuild of this PR head (window 1000x700 fully covered by the overlay, app inactive throughout; all evidence values come from the RPC response captured at the same instant as the pixels):Ordering and limitations
unavailableerror, rather than sharing a frame between tokens).debug.panel_snapshot; the harness already selects its workspace before capturing.timeoutafter 10s; visible panes in occluded windows keep realized renderers, which is the parity case.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Adds
debug.surface.screenshotto capture a terminal surface from its presented IOSurface at native resolution, unaffected by focus or occlusion. Outputs a Display P3 PNG and returns capture metadata for verification.New Features
debug.surface.screenshotforces a tokened render and deep-copies the presented IOSurface; works while occluded or on another Space; no Screen Recording or window activation/reordering.scale,label,path; response includes pixel/point dims,native_scale,color_space,window_occlusion_visible,app_active, andwindow_frame.tests_v2/test_offscreen_surface_screenshot.pyandtests_v2/cmux.py.surface_screenshot.Dependencies
ghosttysubmodule and pins GhosttyKit checksums to the fork with tokened offscreen render; requires renderer: thread-safe tokened forced draw for offscreen capture ghostty#181 (tokened renderer-thread forced draw) to be merged first.Written for commit 6b43d63. Summary will update on new commits.
Summary by CodeRabbit
New Features
debug.surface.screenshotcommand for capturing terminal surfaces as PNGs, including offscreen or inactive windows.cmux.surface_screenshotPython helper.Bug Fixes
Tests
Note
Medium Risk
New debug-only control-socket path with main-thread/render synchronization and IOSurface readback; depends on fork Ghostty APIs and could deadlock if mis-routed to the main actor (policy explicitly avoids this).
Overview
Adds DEBUG-only
debug.surface.screenshotso harnesses can capture a terminal surface from the renderer’s presented IOSurface instead of the window server.The socket worker triggers a tokened forced render (
ghostty_surface_request_render_with_token), waits for the main-thread render-presented callback, then deep-copies the layer’s IOSurface into a Display P3 PNG at native backing scale (optionalscaleresampling). Capture does not focus, activate, or reorder windows and avoids Screen Recording; the response includes occlusion/active metadata for proof.TerminalSurface gains presented-frame waiter bookkeeping and installs the render-presented callback at runtime creation. GhosttyTerminalView adds
debugCopyPresentedFrameImage(). Execution policy routes the method on the socket worker (not main actor) to avoid deadlock. Python client and an occluded-window integration test are included.Reviewed by Cursor Bugbot for commit 3b1cf0f. Bugbot is set up for automated code reviews on this repo. Configure here.