Repository navigation
Add cmux shot for screenshotting a cmux window - #15394
Open
teamleaderleo wants to merge 19 commits into
Open
teamleaderleo wants to merge 19 commits into
teamleaderleo wants to merge 19 commits into
Conversation
A clip needs its parameters settled before any capture starts: the format and its default frame rate, the scale and width caps, the crop rectangle in window points, the file name, and how the frame size follows a window that is resized mid-clip. None of that needs a window or a screen, so it lives in CmuxFoundation with tests instead of inside the capture session. WindowRecordingFrameGeometry aspect-fits each captured image into the frame size chosen at the start, so a resize letterboxes rather than stretches, and a crop is adopted once and then kept for the rest of the clip. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
An agent working in its own cmux can describe what it changed, but it cannot show it. `cmux record start` films a cmux window, or a region of one, and writes an mp4 or a gif that can go straight into a pull request. `cmux record note` drops a caption into the clip as it runs, so a reader can follow what was being done. Capture uses ScreenCaptureKit's own-process content, so only cmux's own windows are reachable and no Screen Recording permission is asked for. Encoding is AVAssetWriter for mp4 and ImageIO for gif; frame times come from when each frame was sampled, so playback matches what happened. A recording stops itself at --max-seconds, and one runs at a time, so an agent that goes away cannot leave a capture running. The window.record.* socket methods run on the socket worker and are not main-thread callable: the window being filmed has to keep drawing while the sampler runs. They are deliberately absent from the remote relay allowlist, which stays default-deny, because a clip is local screen content rather than an object of the peer's workspace. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A review of the recorder found seven defects the existing tests missed. These are the reds, before the fixes: - `--fps nan`, `--fps inf` and `--fps 1e30` reach `Int(Double)` in the request decoder and trap, which takes the whole app down with the socket. The package suite now crashes with "Double value cannot be converted to Int" instead of finishing. - a gif whose recording stopped long before its frame budget cannot be finalized, because the budget is handed to ImageIO as the number of images the file will contain, so the documented `--gif` flow leaves no file at all. - `record stop` after a clip reached its own `--max-seconds` limit reported "no recording is running" rather than the finished clip. - an unknown recording id, and a note for a clip that already stopped, had no coverage at all. - the mp4 writer's no-frames path was not checked for leaving a stub file behind, and the two-frames-one-tick test only asserted a nonzero duration rather than the timestamps it exists to pin down. - `fit` upscales a window that shrank mid-clip, which its own name says it does not do. `remember` on the registry stops being private so a test can set up a finished clip without a window on screen. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Seven defects a review found, in the order they bite a caller: - `--fps nan` and `--fps 1e30` trapped in `Int(Double)` and took the app down. The decoder now refuses a value that is not finite or does not fit an `Int`. - a gif was created with the recording's frame budget as its declared image count, so a clip stopped before that budget could not be finalized and left no file at all. The count is a floor rather than a cap, and how many frames a clip ends with is only known when it stops, so declare one and add as many as were captured. - `stop` could close the file while an append was suspended inside the session actor, which is an uncatchable AVFoundation exception for the mp4 writer and concurrent state for the gif writer. Appends and the close now take turns. - two `record start` calls could both pass the one-at-a-time check, because the check was followed by the await that opens the capture. The slot is claimed before that await. - `record stop` after a clip reached its own `--max-seconds` limit said no recording was running; it reports the finished clip, as `status` already did. - the mp4 writer cancelled a writer it had never started when a clip captured no frames, which raises rather than returns an error. - the output file was deleted before the first frame was written, so a failed recording destroyed whatever was at `--out`. Frames go to a hidden sibling file and only move into place once the clip closes, a directory or device at that path is refused, and a partial file is cleaned up on every failure path. Also: a window closed mid-clip reported an empty rectangle and was captured as a one-pixel frame for the rest of the clip, so an empty rectangle ends the recording and keeps what came before; `fit` no longer magnifies a window that shrank mid-clip, which is what its name always claimed; and `window.record.*` is listed in `system.capabilities` for local callers, where the relay policy still filters it out. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
`cmux record start --label --gif` named the clip "--gif" and recorded an mp4, because the shared option parser takes whatever follows a flag. The record command now hands a value that looks like a flag back to the unexpected-arguments check, which names both, and the positional recording id for `stop`/`status` no longer accepts one either. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
`outputNotAFile` was added to `WindowRecordingSessionError` in the fix
commit for the output-path handling, but the router's switch over that
enum was never extended, so the app target stopped compiling:
Sources/TerminalController+WindowRecording.swift:175:13:
error: switch must be exhaustive
Nothing on the pull request caught it. The checks that run on a pull
request build the packages and the tooling, not the app target, so this
first appeared in a dispatched UI test run:
https://github.com/manaflow-ai/cmux/actions/runs/36412249068
The missing case is `invalid_params`: `--out` naming a directory, a
device or anything else the recorder may not replace is the caller's
parameter, not a cmux failure, and an agent that reads `internal_error`
retries instead of fixing its flag.
The mapping had no test at all, which is why a missing case could sit
here. `recordingErrorCode(for:)` is now internal and every case of the
three error types it understands is pinned, including the unrecognized
fallback. A compile error cannot be committed as a red test first, since
the test would not build either; the build log above is the red
evidence, and the test is what keeps the codes from drifting.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
`window.screenshot` takes the same region, the same scale and width caps and the same rule about an output path as `window.record.start`, and a caller who finds a rectangle in a clip should be able to shoot it with the same four numbers. Written twice, the two would drift: one would round a region's edges differently, or accept a relative path, or trap on `--max-width 1e30` where the other refuses it. So the decoding moves into `WindowCaptureValueDecoding`, which both requests translate into their own public failures. The recorder's API and every message it produces are unchanged; `WindowRecordingRequest.make` now decodes the format first and maps the shared failure back into its own, which is why the format still names itself in a path error. `WindowRecordingFrameGeometry.plan` gains an overload taking a region, a scale and a width quantum rather than a recording request, so a still can plan its crop with the code a clip uses. H.264's even-width rounding becomes that quantum instead of being read off the format. `WindowRecordingLabel` takes a fallback, so a label that sanitizes away to nothing is filed as a screenshot rather than under the word "recording", and `WindowRecordingOutputNaming` gains the screenshot directory the DEBUG command has always used plus a filename overload taking an extension. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A record start that timed out on the socket kept running and could register a recording the caller had been told failed. The registry now tracks each start by token; on timeout the socket handler abandons it and waits for the session to release its writer and partial file before answering. A start that is stopped or abandoned mid-capture no longer opens a writer, and fail() releases a writer even after the session reached a terminal state. record note now strips only a leading -- terminator, and cli.usage.record carries every catalog locale. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
# Conflicts: # cmux.xcodeproj/project.pbxproj
The package conventions lint rejects all-static public types, so the limits and the default output naming become extensions on the request. The mp4 test names its encoded clip length plainly so the determinism check does not read it as a measured wall-clock time. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
An agent that films a window with `cmux record` still has no way to grab one frame of it. A clip is the wrong artifact for "what does this sheet look like now": it is slower to produce, heavier to attach, and a reviewer cannot read a label off it. `window.screenshot` captures one of cmux's own windows to a png or a jpeg, through the same ScreenCaptureKit path the recorder uses. No Screen Recording permission is involved, so it works on a release build and inside CI, where the DEBUG-only `screenshot` command does not exist at all. The two commands share every decision they could disagree about: parameter decoding, crop and scale planning, frame capture, output-file handling and window resolution. `cmux record --region 0,0,420,900` and `cmux shot --region 0,0,420,900` frame the same rectangle of the same window, so a detail spotted in a clip can be shot with the same four numbers. The image is encoded beside the output path and moved into place, so an existing file there is replaced only once there is a complete image to replace it with, and `--out` pointed at a directory comes back as `invalid_params` rather than deleting it. Local socket only: `window.screenshot` is not on the `cmux ssh` relay allowlist, since it reads pixels of windows the remote session does not own. ## Changelog Added: `cmux shot` captures a cmux window, or a region of one, to a png or a jpeg. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 5 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (43)
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 |
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
Optional tuples are not Equatable, so the caption test did not compile. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The recorder branch moved its limits and output naming off all-static public types, which the namespace lint rejects, and this branch had generalized that same naming for stills. Resolve it once: WindowCaptureOutputName holds the timestamped identifier and builds a filename from a label plus an extension, and each request type carries its own output directory and filename in an extension. The screenshot limits move onto WindowScreenshotRequest the same way the recording ones moved onto WindowRecordingRequest. Also takes the recorder's `--` handling for record note through this branch's shared parseCaptureOption, its discardWriter, and its Equatable Pixel in the pipeline tests, which is what the last dispatch failed on. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
3 of 6 tasks
Catch-up merge by scripts/ci/catch_up_pr.py (RFC manaflow-ai#14631). Merged by scripts/merge-main.sh: origin/main at 56eacd4. Resolved conflicts: - Resources/Localizable.xcstrings: xcstrings key-level union - cmux.xcodeproj/project.pbxproj: union of added entries, then normalize-pbxproj.py Catch-up-previous-head: 4d5f2a3 Catch-up-base: 56eacd4
|
Deployment failed for project cmux with the following error: |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
CGImageDestinationFinalize only succeeds when the destination received exactly the image count it was created with, so a gif opened with a count of one could not take a second frame. The gif writer now keeps each frame as PNG data and builds the gif in finish. The mp4 timing test skips the empty marker buffers the asset reader returns. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
# Conflicts: # cmux.xcodeproj/project.pbxproj
Four defects from the review: - `--region 0,0,1e19,1e19` killed the app. `Int(_: Double)` traps past Int's range, and only fps, scale and max-width were bounded. A region is bounded now, and the geometry clamps rather than traps, so a request built in code cannot take the app down either. - `--out` on a read-only volume answered `internal_error`. The writer's own failures had no mapping, and opening the file is the caller's parameter. - A stop that lands while the start is still opening the clip reported "the recording has already finished", an event that never happened, and the start answered `internal_error`. Both say stopped-while-starting now, with the code `conflict`. - The backpressure wait spun once its task was cancelled, which is exactly when a stop is waiting behind it. It uses a delay a cancelled task still waits out. The contract also says where the frames go while a clip is open, since quitting cmux mid-recording leaves that file behind. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
`cmux shot --region 0,0,420,900` wrote the whole window. The still path skipped composing whenever the crop offset was zero, and `cropsNothing` only asked about the offset, so a region anchored at the window's origin looked like "no crop" no matter how small it was. `cropsNothing` now takes the source size and answers the question the callers actually have, and the composer uses the same method instead of its own three-part condition, so the still and recording paths cannot disagree again. A timed-out screenshot no longer writes its file: the capture task is cancelled when the socket wait gives up, and the write is preceded by a cancellation check, so a "timeout" reply does not land a file on top of whatever the caller put at `out` since. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…indow-screenshot # Conflicts: # Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/WindowRecording/WindowRecordingFrameGeometry.swift # Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/WindowRecording/WindowRecordingRequest.swift # cmux.xcodeproj/project.pbxproj # docs/cli-contract.md
teamleaderleo
added
the
needs a call
Collaborator
Author
|
Product decision pending before merge; leaving this feature open for that decision. |
This branch has not been deployed
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.
Stacked on #15277: that PR's commits are the first three here, and only
c0179bd134bandc55f4b1256fare new. Review those two, and merge this after #15277.An agent that can film a window with
cmux recordstill has no way to grab one frame of it. A clip is the wrong artifact for "what does this sheet look like right now": slower to produce, heavier to attach, and a reviewer cannot read a label off it. Thescreenshotcommand that exists today is#if DEBUG, so it is missing from every build we dogfood, which is where an agent actually runs.cmux shotcaptures one of cmux's own windows to a png or a jpeg overwindow.screenshot, on the same ScreenCaptureKit path the recorder uses:Only cmux's own windows are captured, so no Screen Recording permission is involved and this works on a release build and inside CI.
Shared with the recorder
The two commands share every decision they could disagree about, at five layers: parameter decoding (
WindowCaptureValueDecoding), crop and scale planning (WindowRecordingFrameGeometry.plan), frame capture (OwnWindowFrameCapture, lifted out ofWindowRecordingSession), output-file handling (WindowCaptureOutputFile) and window resolution (TerminalController.captureWindowID). Socmux record --region 0,0,420,900andcmux shot --region 0,0,420,900frame the same rectangle of the same window, and a detail spotted in a clip can be shot with the same four numbers.Two differences are deliberate. A still has no encoder demanding even dimensions, so it plans with
widthQuantum: 1and the caller gets the pixels they asked for. And a shot with nothing to crop, scale or caption skips the compose pass rather than paying a copy and the window's color space for no change.The image is encoded beside the output path and moved into place, so an existing file there is replaced only once there is a complete image to replace it with, and
--outpointed at a directory comes back asinvalid_paramsinstead of deleting it.Relay
window.screenshotis registered as.socketWorker(mainThreadCallable: false)and is denied on thecmux sshrelay, with a test pinning both. It reads pixels of windows the remote session does not own, and there is no scoping that would make that safe, so it stays local-socket only.Testing
cmuxTests/WindowScreenshotWriteTests.swift(new) covers the half of this that needs no window on screen: a png and a jpeg are written at the image size and identified as those types by ImageIO, jpeg--qualityactually changes the file size, the output directory is created, an existing file is replaced, a directory is refused with nothing left behind, the working file is a hidden sibling of the output, and everyscreenshotErrorCodecase is pinned so a caller cannot be sent down the wrong path by a new failure defaulting tointernal_error.Packages/macOS/CmuxFoundation/Tests/.../WindowScreenshotRequestTests.swift(new, 14 tests) pins the request limits, the format aliases and the--jpgshorthand, plus geometry and naming tests for the shared code.python3 scripts/verify-local.py --affected origin/main: 7/7 selected checks passed (xcstrings, localization, project, wire-app-sources, test-wiring, package-groups, feature-flags). Native compilation and app tests are not in that scope and run in CI; a focused UI test is dispatched for the app-host compile, and the capture path itself needs a window server, so the dogfood evidence comes from a fleet build.Changelog
Added:
cmux shotcaptures a cmux window, or a region of one, to a png or a jpeg.🤖 Generated with Claude Code
Summary by cubic
Adds
cmux recordandcmux shot, which capture cmux's own windows to an mp4/gif and to a single png/jpeg respectively, so an agent can attach a clip or a still of what it did. Both use ScreenCaptureKit's own-process capture, so no Screen Recording permission is involved and they work in Release builds and inside CI.Recording and shots
recordfilms a window or region,noteadds captions drawn into later frames, and one recording runs at a time and stops itself at--max-seconds.shotwrites one frame as a png or jpeg; both commands share decoding, geometry planning, capture, and output-file handling, so the same--regionnumbers frame the same rectangle.Limits and safety
cmux sshrelay; request limits, error codes, and encoded formats are pinned by tests.Written for commit 3ba7009. Summary will update on new commits.