From a2bd7e05c1eff919e533829eaa787fa65bc818d0 Mon Sep 17 00:00:00 2001 From: onevcat Date: Thu, 27 Aug 2026 09:38:12 +0900 Subject: [PATCH 1/4] feat: add ios-device screenshot backed by devicectl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Physical iOS devices could observe (ui) and act (tap) but not capture the screen — issue #115 even records screenshot as unsupportable on this channel. That is true for the accessibility audit channel, but CoreDevice offers it: `xcrun devicectl device capture screenshot` captures any screen, with no development-signing requirement. - `sim-use ios-device screenshot`: resolves the device like the other ios-device verbs (same DeviceSession discovery and errors, --device optional with exactly one attached), then shells out to devicectl for the capture. Output mirrors the simulator verb: path on stdout, confirmation on stderr. Non-.png output paths are rejected up front (devicectl writes PNG only). - --output path resolution (file / directory / default naming) is hoisted into a shared SimUseCore OutputFilePath, replacing the two existing per-target copies in IOSSimScreenshotCommand and VideoOutputFile; both keep their public API as thin delegates. - DeviceSession.resolveDevice exposes discovery + selection without opening the audit service, for verbs that hand the work to another channel. Screen recording exists on the same channel (capture screen-record) but CoreDevice reports the capability unsupported on the available test device, so it is not exposed yet. Co-Authored-By: Claude Fable 5 Signed-off-by: onevcat --- CHANGELOG.md | 1 + README.md | 12 ++- Sources/SimUseCore/OutputFilePath.swift | 69 ++++++++++++++++ Sources/SimUseVideo/VideoOutputFile.swift | 49 ++---------- .../Devicectl/Devicectl.swift | 78 +++++++++++++++++++ .../Transport/DeviceSession.swift | 28 +++++-- .../Verbs/IOSDeviceCommand.swift | 46 ++++++++++- .../Verbs/IOSSimScreenshotCommand.swift | 52 ++----------- Tests/IOSDeviceBackendTests.swift | 41 ++++++++++ skills/sim-use/SKILL.md | 7 +- 10 files changed, 276 insertions(+), 107 deletions(-) create mode 100644 Sources/SimUseCore/OutputFilePath.swift create mode 100644 Sources/iOSDeviceBackend/Devicectl/Devicectl.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 91704885..811a53ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `sim-use ios-device` (experimental): drive a development-signed app on a physical iPhone or iPad. `devices` lists attached devices, `ui` prints the foreground app's accessibility tree, and `tap --label` / `--label-contains` sends Activate to one unambiguous match. sim-use installs and signs no runner and needs no Developer Disk Image; the device must be unlocked and the target app must have `get-task-allow=true`. This channel intentionally omits coordinate tap, swipe and gesture because the daemon exposes no element geometry. - `sim-use ios-device ui` now renders each element's accessibility identifier as `#id`, and `sim-use ios-device tap` accepts it as a positional `#` or `--id` (mirroring the simulator tap). This is a stable handle to prefer when a label is dynamic — a navigation-bar back button is labelled with the previous screen's title but keeps `#BackButton`. The `@N` alias and coordinate forms remain unavailable on this channel (handles expire between processes; the daemon exposes no geometry). Label and identifier matching go through the same case-sensitive `SelectorTextMatcher` policy the simulator and Android surfaces already use, so a selector behaves identically across all three. +- `sim-use ios-device screenshot`: capture a PNG of a connected iPhone or iPad display. Capture runs over CoreDevice (`xcrun devicectl device capture screenshot`) rather than the accessibility audit channel, so — unlike `ui` and `tap` — it is not limited to development-signed foreground apps: whatever is on screen is captured, SpringBoard and system apps included. Device selection matches the other `ios-device` verbs (`--device` optional with exactly one attached), and `--output` follows the shared path semantics with a `Device Screenshot - - .png` default. The `--output` path resolution shared by the simulator screenshot and the video verbs is now factored into one `OutputFilePath` helper instead of two per-target copies. ### Fixed diff --git a/README.md b/README.md index c695fcbf..66242f91 100644 --- a/README.md +++ b/README.md @@ -156,7 +156,7 @@ All device-scoped commands accept `--device ` (optional when only one simula * **Top-level** — cross-platform verbs: `ui`, `tap`, `long-press`, `swipe`, `touch`, `multi-touch`, `type`, `paste`, `button`, `gesture`, `keyboard-state`, `screenshot`, `record-video`, `stream-video`, `app-state`. Same flags on iOS and Android. * **`sim-use ios `** — iOS-only: `key`, `key-combo`, `key-sequence`, `batch`. * **`sim-use android `** — Android-only: `init`, `devices`, `ping`, `scroll`. - * **`sim-use ios-device `** — physical iOS devices (experimental): `devices`, `ui`, `tap`. Separate from the top-level verbs because the capabilities differ — see [Physical iOS devices](#physical-ios-devices). + * **`sim-use ios-device `** — physical iOS devices (experimental): `devices`, `ui`, `screenshot`, `tap`. Separate from the top-level verbs because the capabilities differ — see [Physical iOS devices](#physical-ios-devices). Run `sim-use --help` or `sim-use --help` for the full flag set. @@ -423,6 +423,11 @@ sim-use ios-device tap --label-contains "Reply" --element-type Button # Or target the stable accessibility identifier shown as #id — the same # `#id` positional the simulator tap accepts (--id works too). sim-use ios-device tap '#BackButton' + +# Screenshot — any screen, not limited to development-signed apps. +sim-use ios-device screenshot +# Screenshot saved to ./Device Screenshot - My iPhone - 2026-08-27 at 09.34.10.png +sim-use ios-device screenshot --output shot.png ``` A device is addressed by UDID or ECID, and `--device` is optional only when exactly one is attached. Run `ui` again after every action: accessibility actions are fire-and-forget, so the follow-up read is the authoritative verification. @@ -433,14 +438,15 @@ This channel deliberately differs from the simulator backend: * **No element geometry.** There is no coordinate tap, `swipe`, `gesture` or `multi-touch`. Only the exposed `tap` accessibility action is currently supported; unsupported simulator verbs are not routed here. * **No `@N` aliases, but stable `#id`s.** Element handles encode a live pointer and expire with their DTX connection, so — like the missing geometry — the cross-invocation `@N` alias cannot be backed faithfully and the outline advertises none. The stable accessibility identifier *can*: the outline shows each element's `#id`, and `tap` accepts it as a positional `#` or `--id` (mirroring the simulator), alongside `--label` / `--label-contains` / `--element-type`. Prefer the `#id` when a label is dynamic — a navigation-bar back button is labelled with the previous screen's title but keeps `#BackButton`, and is an ordinary, tappable row in the outline. - * **Text output only.** The experimental `ios-device` commands do not yet support `--json`, screenshot or recording. + * **Screenshots go over CoreDevice, not the audit daemon.** `screenshot` shells out to `xcrun devicectl device capture screenshot`, a separate channel with different rules: it is not limited to development-signed foreground apps and captures whatever is on screen, SpringBoard and system apps included. Screen *recording* exists on the same channel (`devicectl device capture screen-record`) but is capability-gated per device (CoreDevice can report "Screen Recording is not supported by this device") and is not exposed yet. + * **Text output only.** The experimental `ios-device` commands do not yet support `--json` or recording. * **Slower snapshots.** A full tree costs a few seconds. `ui --fast` stops at labelled elements and is roughly 40% quicker, at the cost of about a quarter of the elements. * **Reading order, not screen order.** With no frames to sort by, the outline follows accessibility nesting and reading order. ## Architecture -sim-use drives iOS Simulators through the lower-level XCFrameworks of Facebook's [idb](https://github.com/facebook/idb) (statically linked), Apple's Accessibility APIs, and the simulator HID pipeline. Android devices are driven through an on-device bridge APK that exposes the AccessibilityService tree and input injection over HTTP, tunnelled via `adb forward`. Physical iOS devices go through a third path: idb's `FBDeviceControl` opens a lockdown service connection, over which sim-use speaks Apple's DTX message protocol to the accessibility audit daemon. Everything ships as a single binary. The established simulator and Android surfaces support `--json`; the experimental `ios-device` commands currently emit text only. +sim-use drives iOS Simulators through the lower-level XCFrameworks of Facebook's [idb](https://github.com/facebook/idb) (statically linked), Apple's Accessibility APIs, and the simulator HID pipeline. Android devices are driven through an on-device bridge APK that exposes the AccessibilityService tree and input injection over HTTP, tunnelled via `adb forward`. Physical iOS devices go through a third path: idb's `FBDeviceControl` opens a lockdown service connection, over which sim-use speaks Apple's DTX message protocol to the accessibility audit daemon (screen capture instead shells out to Xcode's `devicectl`). Everything ships as a single binary. The established simulator and Android surfaces support `--json`; the experimental `ios-device` commands currently emit text only. ## Viewer diff --git a/Sources/SimUseCore/OutputFilePath.swift b/Sources/SimUseCore/OutputFilePath.swift new file mode 100644 index 00000000..0764f183 --- /dev/null +++ b/Sources/SimUseCore/OutputFilePath.swift @@ -0,0 +1,69 @@ +// SPDX-License-Identifier: Apache-2.0 +import Foundation + +/// Output-path resolution shared by every file-producing verb (screenshots, +/// video recordings) across platform backends, so `--output` behaves the same +/// everywhere: trim and tilde-expand the supplied path, anchor relative paths +/// at the current directory, treat an existing directory as the destination +/// for a default-named file, create missing parent directories, and replace +/// an existing file. +public enum OutputFilePath { + /// Resolve the user-supplied `--output` argument into a concrete file + /// URL. `defaultFilename` is consulted when no path is supplied or when + /// the path names an existing directory; it is invoked at most once per + /// call so timestamped names stay consistent. + public static func resolve(output: String?, defaultFilename: () -> String) throws -> URL { + let fileManager = FileManager.default + var cachedDefault: String? + func defaultName() -> String { + if let cachedDefault { return cachedDefault } + let name = defaultFilename() + cachedDefault = name + return name + } + + let providedPath = output?.trimmingCharacters(in: .whitespacesAndNewlines) + let resolvedPath: String + if let providedPath, !providedPath.isEmpty { + resolvedPath = (providedPath as NSString).expandingTildeInPath + } else { + resolvedPath = defaultName() + } + + let baseURL: URL + if resolvedPath.hasPrefix("/") { + baseURL = URL(fileURLWithPath: resolvedPath) + } else { + baseURL = URL(fileURLWithPath: fileManager.currentDirectoryPath).appendingPathComponent(resolvedPath) + } + + var isDirectory: ObjCBool = false + if fileManager.fileExists(atPath: baseURL.path, isDirectory: &isDirectory), isDirectory.boolValue { + return baseURL.appendingPathComponent(defaultName()) + } + + let directoryURL = baseURL.deletingLastPathComponent() + if !fileManager.fileExists(atPath: directoryURL.path) { + try fileManager.createDirectory(at: directoryURL, withIntermediateDirectories: true, attributes: nil) + } + + if fileManager.fileExists(atPath: baseURL.path) { + var existingIsDirectory: ObjCBool = false + fileManager.fileExists(atPath: baseURL.path, isDirectory: &existingIsDirectory) + if existingIsDirectory.boolValue { + throw CLIError(errorDescription: "Output path \(baseURL.path) is a directory. Provide a file name or point to a different location.") + } + try fileManager.removeItem(at: baseURL) + } + + return baseURL + } + + /// Timestamp format shared by every screenshot default filename so paired + /// screenshots from cross-platform sessions sort together. + public static func screenshotTimestamp(_ date: Date) -> String { + let formatter = DateFormatter() + formatter.dateFormat = "yyyy-MM-dd 'at' HH.mm.ss" + return formatter.string(from: date) + } +} diff --git a/Sources/SimUseVideo/VideoOutputFile.swift b/Sources/SimUseVideo/VideoOutputFile.swift index e8a1cd12..6bc60b1a 100644 --- a/Sources/SimUseVideo/VideoOutputFile.swift +++ b/Sources/SimUseVideo/VideoOutputFile.swift @@ -8,51 +8,12 @@ public enum VideoOutputFile { /// Resolve the user-supplied `--output` argument into a concrete /// output file URL (default naming uses `fileExtension`). Every /// platform backend and the cross-platform forwarder use the same - /// path semantics. + /// path semantics (`OutputFilePath`). public static func prepareOutputURL(output: String?, fileExtension: String = "mp4") throws -> URL { - let fileManager = FileManager.default - let formatter = ISO8601DateFormatter() - formatter.formatOptions = [.withInternetDateTime] - - let providedPath = output?.trimmingCharacters(in: .whitespacesAndNewlines) - let resolvedPath: String - if let providedPath, !providedPath.isEmpty { - resolvedPath = (providedPath as NSString).expandingTildeInPath - } else { - resolvedPath = "sim-use-video-\(formatter.string(from: Date())).\(fileExtension)" - } - - let baseURL: URL - if resolvedPath.hasPrefix("/") { - baseURL = URL(fileURLWithPath: resolvedPath) - } else { - baseURL = URL(fileURLWithPath: fileManager.currentDirectoryPath).appendingPathComponent(resolvedPath) - } - - var isDirectory: ObjCBool = false - if fileManager.fileExists(atPath: baseURL.path, isDirectory: &isDirectory), isDirectory.boolValue { - let filename = "sim-use-video-\(formatter.string(from: Date())).\(fileExtension)" - let directoryURL = baseURL - if !fileManager.fileExists(atPath: directoryURL.path) { - try fileManager.createDirectory(at: directoryURL, withIntermediateDirectories: true, attributes: nil) - } - return directoryURL.appendingPathComponent(filename) + try OutputFilePath.resolve(output: output) { + let formatter = ISO8601DateFormatter() + formatter.formatOptions = [.withInternetDateTime] + return "sim-use-video-\(formatter.string(from: Date())).\(fileExtension)" } - - let directoryURL = baseURL.deletingLastPathComponent() - if !fileManager.fileExists(atPath: directoryURL.path) { - try fileManager.createDirectory(at: directoryURL, withIntermediateDirectories: true, attributes: nil) - } - - if fileManager.fileExists(atPath: baseURL.path) { - var existingIsDirectory: ObjCBool = false - fileManager.fileExists(atPath: baseURL.path, isDirectory: &existingIsDirectory) - if existingIsDirectory.boolValue { - throw CLIError(errorDescription: "Output path \(baseURL.path) is a directory. Provide a file name or point to a different location.") - } - try fileManager.removeItem(at: baseURL) - } - - return baseURL } } diff --git a/Sources/iOSDeviceBackend/Devicectl/Devicectl.swift b/Sources/iOSDeviceBackend/Devicectl/Devicectl.swift new file mode 100644 index 00000000..1a66f617 --- /dev/null +++ b/Sources/iOSDeviceBackend/Devicectl/Devicectl.swift @@ -0,0 +1,78 @@ +// SPDX-License-Identifier: Apache-2.0 +import Foundation + +/// Shells out to `xcrun devicectl` (CoreDevice) for capabilities the +/// accessibility audit channel does not offer — currently screen capture. +/// +/// Unlike the audit channel, CoreDevice capture is not limited to +/// development-signed foreground apps: it captures whatever is on screen, +/// including SpringBoard and system apps. Device *selection* still goes +/// through `DeviceSession.resolveDevice` so every `ios-device` verb sees the +/// same device set and errors; only the capture itself runs over CoreDevice. +enum Devicectl { + struct Failure: Error, LocalizedError, CustomStringConvertible { + let message: String + var errorDescription: String? { message } + var description: String { message } + } + + /// Argument vector for a screenshot capture, separated from the spawn so + /// tests can pin the invocation without a device. `--quiet` suppresses + /// devicectl's own progress output; the verb prints its own confirmation. + static func screenshotArguments(deviceIdentifier: String, destination: URL) -> [String] { + [ + "devicectl", "device", "capture", "screenshot", + "--device", deviceIdentifier, + "--destination", destination.path, + "--timeout", "30", + "--quiet", + ] + } + + /// Runs devicectl and fails with its stderr on a non-zero exit. + /// `executablePath` is injectable so tests can drive the drain and error + /// mapping against `/bin/sh`; production uses the default `xcrun`. + static func run(arguments: [String], executablePath: String = "/usr/bin/xcrun") throws { + let process = Process() + process.executableURL = URL(fileURLWithPath: executablePath) + process.arguments = arguments + + let stdout = Pipe() + let stderr = Pipe() + process.standardOutput = stdout + process.standardError = stderr + + // Drain both pipes while the child runs so a chatty error path cannot + // fill the ~64 KB pipe buffer and deadlock `waitUntilExit()`. Same + // drain as `SimctlDeviceLister.runSimctl` and `Adb.run`. + let bufferLock = NSLock() + var errBuffer = Data() + stdout.fileHandleForReading.readabilityHandler = { handle in + _ = handle.availableData + } + stderr.fileHandleForReading.readabilityHandler = { handle in + let chunk = handle.availableData + guard !chunk.isEmpty else { return } + bufferLock.lock(); errBuffer.append(chunk); bufferLock.unlock() + } + + do { + try process.run() + } catch { + throw Failure(message: "could not spawn xcrun devicectl: \(error.localizedDescription)") + } + process.waitUntilExit() + + stdout.fileHandleForReading.readabilityHandler = nil + stderr.fileHandleForReading.readabilityHandler = nil + bufferLock.lock() + errBuffer.append(stderr.fileHandleForReading.readDataToEndOfFile()) + bufferLock.unlock() + + guard process.terminationStatus == 0 else { + let err = (String(data: errBuffer, encoding: .utf8) ?? "") + .trimmingCharacters(in: .whitespacesAndNewlines) + throw Failure(message: "xcrun devicectl exited \(process.terminationStatus)\(err.isEmpty ? "" : ": \(err)")") + } + } +} diff --git a/Sources/iOSDeviceBackend/Transport/DeviceSession.swift b/Sources/iOSDeviceBackend/Transport/DeviceSession.swift index 667a41a1..559a2110 100644 --- a/Sources/iOSDeviceBackend/Transport/DeviceSession.swift +++ b/Sources/iOSDeviceBackend/Transport/DeviceSession.swift @@ -109,14 +109,17 @@ public enum DeviceSession { @MainActor public static func connectedDevices(logger: FBControlCoreLogger? = nil) async throws -> [DeviceSummary] { let set = try deviceSet(logger: logger) - return attachedDevices(set).map { - DeviceSummary( - udid: $0.identity, - name: $0.name, - osVersion: $0.osVersion.name.rawValue, - state: FBiOSTargetStateStringFromState($0.state).rawValue - ) - } + return attachedDevices(set).map(summary(of:)) + } + + /// Resolves a device the way `withClient` does — same discovery, same + /// selection and error surface — without opening the audit service, for + /// verbs that hand the actual work to another channel (e.g. `screenshot` + /// via `devicectl`). + @MainActor + public static func resolveDevice(udid: String?, logger: FBControlCoreLogger? = nil) throws -> DeviceSummary { + let set = try deviceSet(logger: logger) + return summary(of: try resolve(udid, among: attachedDevices(set))) } @MainActor @@ -210,6 +213,15 @@ public enum DeviceSession { return attached.isEmpty ? set.allDevices : attached } + private static func summary(of device: FBDevice) -> DeviceSummary { + DeviceSummary( + udid: device.identity, + name: device.name, + osVersion: device.osVersion.name.rawValue, + state: FBiOSTargetStateStringFromState(device.state).rawValue + ) + } + /// AMDevice does not publish the lockdown UDID until a session is opened, /// so a connected device is identified by its ECID until then. Accept /// either, and default to the only device when there is just one. diff --git a/Sources/iOSDeviceBackend/Verbs/IOSDeviceCommand.swift b/Sources/iOSDeviceBackend/Verbs/IOSDeviceCommand.swift index 6043999b..613b7007 100644 --- a/Sources/iOSDeviceBackend/Verbs/IOSDeviceCommand.swift +++ b/Sources/iOSDeviceBackend/Verbs/IOSDeviceCommand.swift @@ -118,9 +118,10 @@ public struct IOSDeviceCommand: AsyncParsableCommand { Element geometry is not available on this channel, so there is no coordinate tap, swipe or gesture here; interaction goes through - accessibility actions instead. + accessibility actions instead. The display itself can still be + captured with `screenshot`. """, - subcommands: [Devices.self, UI.self, Tap.self] + subcommands: [Devices.self, UI.self, Screenshot.self, Tap.self] ) public init() {} @@ -183,6 +184,45 @@ public struct IOSDeviceCommand: AsyncParsableCommand { } } + struct Screenshot: AsyncParsableCommand { + static let configuration = CommandConfiguration( + commandName: "screenshot", + abstract: "Capture a screenshot of the device display and save it as a PNG file.", + discussion: """ + Captures over CoreDevice (`xcrun devicectl device capture + screenshot`) rather than the accessibility audit channel, so it + is not limited to development-signed foreground apps: whatever is + on screen is captured, SpringBoard and system apps included. The + device is selected exactly like the other ios-device verbs. + """ + ) + + @OptionGroup var device: DeviceOptions + + @Option(help: "Output PNG file path. Defaults to 'Device Screenshot - - .png' in the current directory.") + var output: String? + + /// Mirrors the simulator's default naming so paired screenshots from + /// cross-platform sessions sort together. Static so tests can pin the + /// convention without a device. + static func defaultFilename(deviceName: String, at date: Date) -> String { + "Device Screenshot - \(deviceName) - \(OutputFilePath.screenshotTimestamp(date)).png" + } + + func run() async throws { + let summary = try await DeviceSession.resolveDevice(udid: device.udid) + let url = try OutputFilePath.resolve(output: output) { + Self.defaultFilename(deviceName: summary.name, at: Date()) + } + guard url.pathExtension.lowercased() == "png" else { + throw CLIError(errorDescription: "devicectl writes PNG only — use an output path ending in .png (got '\(url.lastPathComponent)')") + } + try Devicectl.run(arguments: Devicectl.screenshotArguments(deviceIdentifier: summary.udid, destination: url)) + print(url.path) + FileHandle.standardError.write(Data("Screenshot saved to \(url.path)\n".utf8)) + } + } + struct Tap: AsyncParsableCommand { static let configuration = CommandConfiguration( commandName: "tap", @@ -229,7 +269,7 @@ public struct IOSDeviceCommand: AsyncParsableCommand { } func validate() throws { - if let alias, id != nil { + if alias != nil, id != nil { throw ValidationError("specify the identifier once — either the positional `#id` or --id, not both") } if let alias, !alias.hasPrefix("#") { diff --git a/Sources/iOSSimBackend/Verbs/IOSSimScreenshotCommand.swift b/Sources/iOSSimBackend/Verbs/IOSSimScreenshotCommand.swift index 2ae943a4..128029d4 100644 --- a/Sources/iOSSimBackend/Verbs/IOSSimScreenshotCommand.swift +++ b/Sources/iOSSimBackend/Verbs/IOSSimScreenshotCommand.swift @@ -80,61 +80,19 @@ public struct IOSSimScreenshotCommand: SimUseExecutableCommand { /// Resolve the user-supplied `--output` argument into a concrete /// file URL using iOS naming conventions. Public so tests can /// pin the path expansion behaviour without spinning up an - /// FBSimulator. + /// FBSimulator. Path semantics live in `OutputFilePath`, shared + /// with the video verbs and the physical-device screenshot. public static func prepareOutputURL(output: String?, simulatorName: String) throws -> URL { - let fileManager = FileManager.default - - let providedPath = output?.trimmingCharacters(in: .whitespacesAndNewlines) - let resolvedPath: String - if let providedPath, !providedPath.isEmpty { - resolvedPath = (providedPath as NSString).expandingTildeInPath - } else { - let timestamp = formatTimestamp(Date()) - resolvedPath = "Simulator Screenshot - \(simulatorName) - \(timestamp).png" + try OutputFilePath.resolve(output: output) { + "Simulator Screenshot - \(simulatorName) - \(formatTimestamp(Date())).png" } - - let baseURL: URL - if resolvedPath.hasPrefix("/") { - baseURL = URL(fileURLWithPath: resolvedPath) - } else { - baseURL = URL(fileURLWithPath: fileManager.currentDirectoryPath).appendingPathComponent(resolvedPath) - } - - var isDirectory: ObjCBool = false - if fileManager.fileExists(atPath: baseURL.path, isDirectory: &isDirectory), isDirectory.boolValue { - let timestamp = formatTimestamp(Date()) - let filename = "Simulator Screenshot - \(simulatorName) - \(timestamp).png" - let directoryURL = baseURL - if !fileManager.fileExists(atPath: directoryURL.path) { - try fileManager.createDirectory(at: directoryURL, withIntermediateDirectories: true, attributes: nil) - } - return directoryURL.appendingPathComponent(filename) - } - - let directoryURL = baseURL.deletingLastPathComponent() - if !fileManager.fileExists(atPath: directoryURL.path) { - try fileManager.createDirectory(at: directoryURL, withIntermediateDirectories: true, attributes: nil) - } - - if fileManager.fileExists(atPath: baseURL.path) { - var existingIsDirectory: ObjCBool = false - fileManager.fileExists(atPath: baseURL.path, isDirectory: &existingIsDirectory) - if existingIsDirectory.boolValue { - throw CLIError(errorDescription: "Output path \(baseURL.path) is a directory. Provide a file name or point to a different location.") - } - try fileManager.removeItem(at: baseURL) - } - - return baseURL } /// Shared timestamp format used by both iOS and Android default /// filenames so paired screenshots from cross-platform sessions /// sort together. public static func formatTimestamp(_ date: Date) -> String { - let formatter = DateFormatter() - formatter.dateFormat = "yyyy-MM-dd 'at' HH.mm.ss" - return formatter.string(from: date) + OutputFilePath.screenshotTimestamp(date) } } \ No newline at end of file diff --git a/Tests/IOSDeviceBackendTests.swift b/Tests/IOSDeviceBackendTests.swift index 4deb38e4..bfddc3c4 100644 --- a/Tests/IOSDeviceBackendTests.swift +++ b/Tests/IOSDeviceBackendTests.swift @@ -301,6 +301,47 @@ struct IOSDeviceBackendTests { } } + @Test("screenshot pins the devicectl argument vector") + func screenshotArgumentsAreStable() { + let args = Devicectl.screenshotArguments( + deviceIdentifier: "00008130-00066D2A10EB8D3A", + destination: URL(fileURLWithPath: "/tmp/shot.png") + ) + #expect(args == [ + "devicectl", "device", "capture", "screenshot", + "--device", "00008130-00066D2A10EB8D3A", + "--destination", "/tmp/shot.png", + "--timeout", "30", + "--quiet", + ]) + } + + @Test("devicectl failures surface the exit code and stderr") + func devicectlFailureSurfacesStderr() { + do { + try Devicectl.run(arguments: ["-c", "echo boom >&2; exit 3"], executablePath: "/bin/sh") + Issue.record("expected a non-zero exit to throw") + } catch { + #expect(error.localizedDescription.contains("exited 3")) + #expect(error.localizedDescription.contains("boom")) + } + } + + @Test("devicectl success is silent") + func devicectlSuccessRuns() throws { + try Devicectl.run(arguments: ["-c", "echo ok"], executablePath: "/bin/sh") + } + + @Test("device screenshot default filename mirrors the simulator convention") + func screenshotDefaultFilenameConvention() { + let name = IOSDeviceCommand.Screenshot.defaultFilename( + deviceName: "iPhone One", + at: Date(timeIntervalSince1970: 0) + ) + #expect(name.hasPrefix("Device Screenshot - iPhone One - ")) + #expect(name.hasSuffix(".png")) + } + @Test("device selection errors tell the user how to recover") func deviceSelectionErrorsAreActionable() { let none = DeviceSessionError.noDevices.localizedDescription diff --git a/skills/sim-use/SKILL.md b/skills/sim-use/SKILL.md index 7baca42f..121a932f 100644 --- a/skills/sim-use/SKILL.md +++ b/skills/sim-use/SKILL.md @@ -88,7 +88,7 @@ Every byte of command output you read costs context. Defaults that keep the loop Use the separate `sim-use ios-device` surface. Never route a physical iPhone or iPad through the top-level or `sim-use ios` verbs. -**Hard requirement:** the device must be paired, trusted, unlocked and in Developer Mode, and the foreground target app must be development-signed with `get-task-allow=true`. A Release-configuration binary installed with a Development profile is supported. Distribution/Ad Hoc, TestFlight, App Store and system apps are unsupported; do not retry them or claim success. sim-use itself installs and signs no runner and needs no Developer Disk Image. +**Hard requirement for `ui` / `tap`:** the device must be paired, trusted, unlocked and in Developer Mode, and the foreground target app must be development-signed with `get-task-allow=true`. A Release-configuration binary installed with a Development profile is supported. Distribution/Ad Hoc, TestFlight, App Store and system apps are unsupported; do not retry them or claim success. sim-use itself installs and signs no runner and needs no Developer Disk Image. `screenshot` is exempt from the signing rule — it runs over CoreDevice and captures any screen, system apps included. ```bash # Physical-device preflight @@ -105,6 +105,9 @@ sim-use ios-device tap --label-contains "Reply" --element-type Button --device < # By stable identifier (the #id shown in ui) — positional or --id, like the simulator sim-use ios-device tap '#BackButton' --device + +# Screenshot — any screen, not limited to development-signed apps +sim-use ios-device screenshot --output shot.png --device ``` Rules for this experimental surface: @@ -112,7 +115,7 @@ Rules for this experimental surface: 1. **Treat hierarchy errors as capability failures.** If the command says the hierarchy is unavailable, confirm the screen is unlocked and inspect the installed app's final `get-task-allow` entitlement. Do not fall back to coordinates or focus walking. 2. **Tap by `#id` or label, not `@N`.** Element handles expire with the DTX connection, so there is no `@N` alias (nor coordinates — no geometry). Use the `#id` shown in the outline (positional `#` or `--id`) — it is stable and the best choice when a label is dynamic — or `--label` / `--label-contains`, with `--element-type` to disambiguate. The navigation-bar back button appears as a normal `Button "" #BackButton`; go back by tapping `#BackButton` (or the shown label) like any other element — no special "back" verb. 3. **Always verify.** Activate is fire-and-forget. Re-run `sim-use ios-device ui` and confirm the expected state before continuing. -4. **No geometry or simulator-only verbs.** Coordinate tap, swipe, gesture, multi-touch, type, screenshot, recording and `--json` are unavailable here. Do not substitute a similarly named top-level command. +4. **No geometry or simulator-only verbs.** Coordinate tap, swipe, gesture, multi-touch, type, recording and `--json` are unavailable here. Do not substitute a similarly named top-level command. `screenshot` *is* available and works on any screen (it captures over CoreDevice, not the accessibility channel). 5. **Budget seconds, not milliseconds.** A full tree takes a few seconds. `ui --fast` is quicker but omits nested elements; do not poll in a tight loop. If `ui` succeeds with zero elements or `tap` prints success for a missing/ambiguous label, treat it as a sim-use bug; the command is expected to fail loudly instead. From bb760b13f5ceec14aa758e0fbb36a4c612f94d7e Mon Sep 17 00:00:00 2001 From: onevcat Date: Thu, 27 Aug 2026 10:10:21 +0900 Subject: [PATCH 2/4] fix: stop a rejected non-PNG screenshot path from deleting the target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on the initial revision: - OutputFilePath.resolve() removed an existing file at the target before ios-device screenshot's .png validation ran, so a rejected `--output important.jpg` destroyed the user's important.jpg and then errored. Resolution and preparation are now separate steps: resolve() is read-only, and the new prepare() carries the destructive work (mkdir parent, replace existing file). The verb validates between the two; the simulator/video wrappers recombine them, so their replace-on- success behaviour (which predates this PR) is unchanged. Regression tests pin both: a rejected path leaves the existing file intact, an accepted path still replaces. - README example showed `./…` for the saved-path output; the command prints an absolute path (stdout: the path, stderr: confirmation). Example updated to match. Co-Authored-By: Claude Fable 5 Signed-off-by: onevcat --- README.md | 5 +-- Sources/SimUseCore/OutputFilePath.swift | 36 ++++++++++++------- Sources/SimUseVideo/VideoOutputFile.swift | 4 ++- .../Verbs/IOSDeviceCommand.swift | 17 ++++++--- .../Verbs/IOSSimScreenshotCommand.swift | 4 ++- Tests/IOSDeviceBackendTests.swift | 28 +++++++++++++++ 6 files changed, 74 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 66242f91..cd2c2eb3 100644 --- a/README.md +++ b/README.md @@ -424,9 +424,10 @@ sim-use ios-device tap --label-contains "Reply" --element-type Button # `#id` positional the simulator tap accepts (--id works too). sim-use ios-device tap '#BackButton' -# Screenshot — any screen, not limited to development-signed apps. +# Screenshot — any screen, not limited to development-signed apps. Prints the +# absolute saved path on stdout (plus a confirmation on stderr). sim-use ios-device screenshot -# Screenshot saved to ./Device Screenshot - My iPhone - 2026-08-27 at 09.34.10.png +# /Users/me/Device Screenshot - My iPhone - 2026-08-27 at 09.34.10.png sim-use ios-device screenshot --output shot.png ``` diff --git a/Sources/SimUseCore/OutputFilePath.swift b/Sources/SimUseCore/OutputFilePath.swift index 0764f183..eddf5e44 100644 --- a/Sources/SimUseCore/OutputFilePath.swift +++ b/Sources/SimUseCore/OutputFilePath.swift @@ -4,15 +4,21 @@ import Foundation /// Output-path resolution shared by every file-producing verb (screenshots, /// video recordings) across platform backends, so `--output` behaves the same /// everywhere: trim and tilde-expand the supplied path, anchor relative paths -/// at the current directory, treat an existing directory as the destination +/// at the current directory, treat an existing directory as a destination /// for a default-named file, create missing parent directories, and replace /// an existing file. +/// +/// Resolution and preparation are deliberately separate steps: `resolve` +/// never touches the filesystem beyond read-only stats, so a caller can +/// validate the resolved URL (e.g. enforce an extension) and reject it +/// without having destroyed an existing file at the target. public enum OutputFilePath { /// Resolve the user-supplied `--output` argument into a concrete file /// URL. `defaultFilename` is consulted when no path is supplied or when /// the path names an existing directory; it is invoked at most once per - /// call so timestamped names stay consistent. - public static func resolve(output: String?, defaultFilename: () -> String) throws -> URL { + /// call so timestamped names stay consistent. Performs no filesystem + /// mutation — call `prepare(_:)` before writing to the returned URL. + public static func resolve(output: String?, defaultFilename: () -> String) -> URL { let fileManager = FileManager.default var cachedDefault: String? func defaultName() -> String { @@ -42,21 +48,27 @@ public enum OutputFilePath { return baseURL.appendingPathComponent(defaultName()) } - let directoryURL = baseURL.deletingLastPathComponent() + return baseURL + } + + /// Destructive preparation of a resolved output URL: create missing + /// parent directories and remove an existing file at the target so the + /// subsequent write replaces it. + public static func prepare(_ url: URL) throws { + let fileManager = FileManager.default + + let directoryURL = url.deletingLastPathComponent() if !fileManager.fileExists(atPath: directoryURL.path) { try fileManager.createDirectory(at: directoryURL, withIntermediateDirectories: true, attributes: nil) } - if fileManager.fileExists(atPath: baseURL.path) { - var existingIsDirectory: ObjCBool = false - fileManager.fileExists(atPath: baseURL.path, isDirectory: &existingIsDirectory) - if existingIsDirectory.boolValue { - throw CLIError(errorDescription: "Output path \(baseURL.path) is a directory. Provide a file name or point to a different location.") + var isDirectory: ObjCBool = false + if fileManager.fileExists(atPath: url.path, isDirectory: &isDirectory) { + if isDirectory.boolValue { + throw CLIError(errorDescription: "Output path \(url.path) is a directory. Provide a file name or point to a different location.") } - try fileManager.removeItem(at: baseURL) + try fileManager.removeItem(at: url) } - - return baseURL } /// Timestamp format shared by every screenshot default filename so paired diff --git a/Sources/SimUseVideo/VideoOutputFile.swift b/Sources/SimUseVideo/VideoOutputFile.swift index 6bc60b1a..dfb77e82 100644 --- a/Sources/SimUseVideo/VideoOutputFile.swift +++ b/Sources/SimUseVideo/VideoOutputFile.swift @@ -10,10 +10,12 @@ public enum VideoOutputFile { /// platform backend and the cross-platform forwarder use the same /// path semantics (`OutputFilePath`). public static func prepareOutputURL(output: String?, fileExtension: String = "mp4") throws -> URL { - try OutputFilePath.resolve(output: output) { + let url = OutputFilePath.resolve(output: output) { let formatter = ISO8601DateFormatter() formatter.formatOptions = [.withInternetDateTime] return "sim-use-video-\(formatter.string(from: Date())).\(fileExtension)" } + try OutputFilePath.prepare(url) + return url } } diff --git a/Sources/iOSDeviceBackend/Verbs/IOSDeviceCommand.swift b/Sources/iOSDeviceBackend/Verbs/IOSDeviceCommand.swift index 613b7007..e5677015 100644 --- a/Sources/iOSDeviceBackend/Verbs/IOSDeviceCommand.swift +++ b/Sources/iOSDeviceBackend/Verbs/IOSDeviceCommand.swift @@ -209,14 +209,23 @@ public struct IOSDeviceCommand: AsyncParsableCommand { "Device Screenshot - \(deviceName) - \(OutputFilePath.screenshotTimestamp(date)).png" } - func run() async throws { - let summary = try await DeviceSession.resolveDevice(udid: device.udid) - let url = try OutputFilePath.resolve(output: output) { - Self.defaultFilename(deviceName: summary.name, at: Date()) + /// Resolve, validate, then prepare — in that order, so a rejected + /// non-PNG path never removes an existing file at the target. + /// Static so tests can pin that guarantee without a device. + static func resolveOutputURL(output: String?, deviceName: String) throws -> URL { + let url = OutputFilePath.resolve(output: output) { + defaultFilename(deviceName: deviceName, at: Date()) } guard url.pathExtension.lowercased() == "png" else { throw CLIError(errorDescription: "devicectl writes PNG only — use an output path ending in .png (got '\(url.lastPathComponent)')") } + try OutputFilePath.prepare(url) + return url + } + + func run() async throws { + let summary = try await DeviceSession.resolveDevice(udid: device.udid) + let url = try Self.resolveOutputURL(output: output, deviceName: summary.name) try Devicectl.run(arguments: Devicectl.screenshotArguments(deviceIdentifier: summary.udid, destination: url)) print(url.path) FileHandle.standardError.write(Data("Screenshot saved to \(url.path)\n".utf8)) diff --git a/Sources/iOSSimBackend/Verbs/IOSSimScreenshotCommand.swift b/Sources/iOSSimBackend/Verbs/IOSSimScreenshotCommand.swift index 128029d4..4c110ba8 100644 --- a/Sources/iOSSimBackend/Verbs/IOSSimScreenshotCommand.swift +++ b/Sources/iOSSimBackend/Verbs/IOSSimScreenshotCommand.swift @@ -83,9 +83,11 @@ public struct IOSSimScreenshotCommand: SimUseExecutableCommand { /// FBSimulator. Path semantics live in `OutputFilePath`, shared /// with the video verbs and the physical-device screenshot. public static func prepareOutputURL(output: String?, simulatorName: String) throws -> URL { - try OutputFilePath.resolve(output: output) { + let url = OutputFilePath.resolve(output: output) { "Simulator Screenshot - \(simulatorName) - \(formatTimestamp(Date())).png" } + try OutputFilePath.prepare(url) + return url } /// Shared timestamp format used by both iOS and Android default diff --git a/Tests/IOSDeviceBackendTests.swift b/Tests/IOSDeviceBackendTests.swift index bfddc3c4..f1432b38 100644 --- a/Tests/IOSDeviceBackendTests.swift +++ b/Tests/IOSDeviceBackendTests.swift @@ -342,6 +342,34 @@ struct IOSDeviceBackendTests { #expect(name.hasSuffix(".png")) } + @Test("a rejected non-PNG output path leaves the existing file intact") + func rejectedOutputPreservesExistingFile() throws { + let dir = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: dir) } + let existing = dir.appendingPathComponent("important.jpg") + let precious = Data("precious".utf8) + try precious.write(to: existing) + + #expect(throws: (any Error).self) { + _ = try IOSDeviceCommand.Screenshot.resolveOutputURL(output: existing.path, deviceName: "iPhone One") + } + #expect(try Data(contentsOf: existing) == precious) + } + + @Test("an accepted PNG output path removes the existing file so the write replaces it") + func acceptedOutputReplacesExistingFile() throws { + let dir = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: dir) } + let existing = dir.appendingPathComponent("shot.png") + try Data("stale".utf8).write(to: existing) + + let url = try IOSDeviceCommand.Screenshot.resolveOutputURL(output: existing.path, deviceName: "iPhone One") + #expect(url == existing) + #expect(!FileManager.default.fileExists(atPath: existing.path)) + } + @Test("device selection errors tell the user how to recover") func deviceSelectionErrorsAreActionable() { let none = DeviceSessionError.noDevices.localizedDescription From 3dd3fa3a605660ef322db1def107217ea79a8225 Mon Sep 17 00:00:00 2001 From: onevcat Date: Thu, 27 Aug 2026 10:21:19 +0900 Subject: [PATCH 3/4] fix: never remove the existing screenshot before capture succeeds Round-2 review finding: for a valid .png target the previous flow was validate -> prepare (removes existing file) -> devicectl capture, so a capture failure (device unplugged, devicectl timeout, CoreDevice error) destroyed the old file with nothing written. The prior regression test even pinned that early deletion as expected. ios-device screenshot now resolves and validates without touching the target (parent directories are still created up front), captures into a temporary sibling that keeps the .png suffix devicectl requires, and moves it over the target only on success; on failure the temporary is cleaned up and the existing file is untouched. Regression tests cover the injected-failure branch (existing content intact, no temporary left behind), atomic replacement, and fresh-file creation. OutputFilePath gains a non-destructive createParentDirectory(for:); prepare() (parent dirs + remove existing) remains for the simulator and video wrappers, whose replace-before-write behaviour predates this PR. Co-Authored-By: Claude Fable 5 Signed-off-by: onevcat --- CHANGELOG.md | 2 +- Sources/SimUseCore/OutputFilePath.swift | 19 ++++-- .../Verbs/IOSDeviceCommand.swift | 33 +++++++++-- Tests/IOSDeviceBackendTests.swift | 58 +++++++++++++++++-- 4 files changed, 98 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 811a53ec..f4585bff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `sim-use ios-device` (experimental): drive a development-signed app on a physical iPhone or iPad. `devices` lists attached devices, `ui` prints the foreground app's accessibility tree, and `tap --label` / `--label-contains` sends Activate to one unambiguous match. sim-use installs and signs no runner and needs no Developer Disk Image; the device must be unlocked and the target app must have `get-task-allow=true`. This channel intentionally omits coordinate tap, swipe and gesture because the daemon exposes no element geometry. - `sim-use ios-device ui` now renders each element's accessibility identifier as `#id`, and `sim-use ios-device tap` accepts it as a positional `#` or `--id` (mirroring the simulator tap). This is a stable handle to prefer when a label is dynamic — a navigation-bar back button is labelled with the previous screen's title but keeps `#BackButton`. The `@N` alias and coordinate forms remain unavailable on this channel (handles expire between processes; the daemon exposes no geometry). Label and identifier matching go through the same case-sensitive `SelectorTextMatcher` policy the simulator and Android surfaces already use, so a selector behaves identically across all three. -- `sim-use ios-device screenshot`: capture a PNG of a connected iPhone or iPad display. Capture runs over CoreDevice (`xcrun devicectl device capture screenshot`) rather than the accessibility audit channel, so — unlike `ui` and `tap` — it is not limited to development-signed foreground apps: whatever is on screen is captured, SpringBoard and system apps included. Device selection matches the other `ios-device` verbs (`--device` optional with exactly one attached), and `--output` follows the shared path semantics with a `Device Screenshot - - .png` default. The `--output` path resolution shared by the simulator screenshot and the video verbs is now factored into one `OutputFilePath` helper instead of two per-target copies. +- `sim-use ios-device screenshot`: capture a PNG of a connected iPhone or iPad display. Capture runs over CoreDevice (`xcrun devicectl device capture screenshot`) rather than the accessibility audit channel, so — unlike `ui` and `tap` — it is not limited to development-signed foreground apps: whatever is on screen is captured, SpringBoard and system apps included. Device selection matches the other `ios-device` verbs (`--device` optional with exactly one attached), and `--output` follows the shared path semantics with a `Device Screenshot - - .png` default. A rejected path or a capture that fails mid-flight never removes an existing file at `--output`: the image lands in a temporary sibling and replaces the target only on success. The `--output` path resolution shared by the simulator screenshot and the video verbs is now factored into one `OutputFilePath` helper instead of two per-target copies. ### Fixed diff --git a/Sources/SimUseCore/OutputFilePath.swift b/Sources/SimUseCore/OutputFilePath.swift index eddf5e44..dc2d4a2b 100644 --- a/Sources/SimUseCore/OutputFilePath.swift +++ b/Sources/SimUseCore/OutputFilePath.swift @@ -51,16 +51,25 @@ public enum OutputFilePath { return baseURL } - /// Destructive preparation of a resolved output URL: create missing - /// parent directories and remove an existing file at the target so the - /// subsequent write replaces it. - public static func prepare(_ url: URL) throws { + /// Create the resolved URL's missing parent directories. Non-destructive; + /// safe to run before a capture that may still fail. + public static func createParentDirectory(for url: URL) throws { let fileManager = FileManager.default - let directoryURL = url.deletingLastPathComponent() if !fileManager.fileExists(atPath: directoryURL.path) { try fileManager.createDirectory(at: directoryURL, withIntermediateDirectories: true, attributes: nil) } + } + + /// Destructive preparation of a resolved output URL: create missing + /// parent directories and remove an existing file at the target so the + /// subsequent write replaces it. Callers whose payload production can + /// still fail after this point should instead write to a temporary + /// sibling and replace on success, so a failed capture cannot destroy + /// the existing file. + public static func prepare(_ url: URL) throws { + let fileManager = FileManager.default + try createParentDirectory(for: url) var isDirectory: ObjCBool = false if fileManager.fileExists(atPath: url.path, isDirectory: &isDirectory) { diff --git a/Sources/iOSDeviceBackend/Verbs/IOSDeviceCommand.swift b/Sources/iOSDeviceBackend/Verbs/IOSDeviceCommand.swift index e5677015..2fb441bf 100644 --- a/Sources/iOSDeviceBackend/Verbs/IOSDeviceCommand.swift +++ b/Sources/iOSDeviceBackend/Verbs/IOSDeviceCommand.swift @@ -209,8 +209,9 @@ public struct IOSDeviceCommand: AsyncParsableCommand { "Device Screenshot - \(deviceName) - \(OutputFilePath.screenshotTimestamp(date)).png" } - /// Resolve, validate, then prepare — in that order, so a rejected - /// non-PNG path never removes an existing file at the target. + /// Resolve, validate the extension, then ensure the parent directory + /// exists — nothing here removes an existing file, so neither a + /// rejected path nor a later capture failure can destroy one. /// Static so tests can pin that guarantee without a device. static func resolveOutputURL(output: String?, deviceName: String) throws -> URL { let url = OutputFilePath.resolve(output: output) { @@ -219,14 +220,38 @@ public struct IOSDeviceCommand: AsyncParsableCommand { guard url.pathExtension.lowercased() == "png" else { throw CLIError(errorDescription: "devicectl writes PNG only — use an output path ending in .png (got '\(url.lastPathComponent)')") } - try OutputFilePath.prepare(url) + try OutputFilePath.createParentDirectory(for: url) return url } + /// Captures into a temporary sibling file and moves it over the final + /// target only on success, so a capture that fails mid-flight (device + /// unplugged, devicectl timeout) leaves an existing file at --output + /// untouched. The temporary name keeps the .png suffix devicectl + /// requires. Injectable capture so tests can pin the failure branch. + static func captureAtomically(to url: URL, capture: (URL) throws -> Void) throws { + let fileManager = FileManager.default + let temporary = url.deletingLastPathComponent() + .appendingPathComponent(".\(url.lastPathComponent).partial-\(UUID().uuidString).png") + do { + try capture(temporary) + if fileManager.fileExists(atPath: url.path) { + _ = try fileManager.replaceItemAt(url, withItemAt: temporary) + } else { + try fileManager.moveItem(at: temporary, to: url) + } + } catch { + try? fileManager.removeItem(at: temporary) + throw error + } + } + func run() async throws { let summary = try await DeviceSession.resolveDevice(udid: device.udid) let url = try Self.resolveOutputURL(output: output, deviceName: summary.name) - try Devicectl.run(arguments: Devicectl.screenshotArguments(deviceIdentifier: summary.udid, destination: url)) + try Self.captureAtomically(to: url) { temporary in + try Devicectl.run(arguments: Devicectl.screenshotArguments(deviceIdentifier: summary.udid, destination: temporary)) + } print(url.path) FileHandle.standardError.write(Data("Screenshot saved to \(url.path)\n".utf8)) } diff --git a/Tests/IOSDeviceBackendTests.swift b/Tests/IOSDeviceBackendTests.swift index f1432b38..de773def 100644 --- a/Tests/IOSDeviceBackendTests.swift +++ b/Tests/IOSDeviceBackendTests.swift @@ -357,17 +357,67 @@ struct IOSDeviceBackendTests { #expect(try Data(contentsOf: existing) == precious) } - @Test("an accepted PNG output path removes the existing file so the write replaces it") - func acceptedOutputReplacesExistingFile() throws { + @Test("an accepted PNG output path defers deletion — the existing file survives resolution") + func acceptedOutputLeavesExistingFileUntilCapture() throws { let dir = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) defer { try? FileManager.default.removeItem(at: dir) } let existing = dir.appendingPathComponent("shot.png") - try Data("stale".utf8).write(to: existing) + let stale = Data("stale".utf8) + try stale.write(to: existing) let url = try IOSDeviceCommand.Screenshot.resolveOutputURL(output: existing.path, deviceName: "iPhone One") #expect(url == existing) - #expect(!FileManager.default.fileExists(atPath: existing.path)) + #expect(try Data(contentsOf: existing) == stale) + } + + @Test("a failed capture leaves the existing screenshot intact and no temporary behind") + func failedCapturePreservesExistingFile() throws { + struct CaptureFailed: Error {} + let dir = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: dir) } + let target = dir.appendingPathComponent("important.png") + let precious = Data("precious".utf8) + try precious.write(to: target) + + #expect(throws: CaptureFailed.self) { + try IOSDeviceCommand.Screenshot.captureAtomically(to: target) { temporary in + try Data("partial".utf8).write(to: temporary) + throw CaptureFailed() + } + } + #expect(try Data(contentsOf: target) == precious) + #expect(try FileManager.default.contentsOfDirectory(atPath: dir.path) == ["important.png"]) + } + + @Test("a successful capture atomically replaces the existing screenshot") + func successfulCaptureReplacesExistingFile() throws { + let dir = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: dir) } + let target = dir.appendingPathComponent("shot.png") + try Data("stale".utf8).write(to: target) + + try IOSDeviceCommand.Screenshot.captureAtomically(to: target) { temporary in + try Data("fresh".utf8).write(to: temporary) + } + #expect(try Data(contentsOf: target) == Data("fresh".utf8)) + #expect(try FileManager.default.contentsOfDirectory(atPath: dir.path) == ["shot.png"]) + } + + @Test("a successful capture creates the target when none exists") + func successfulCaptureCreatesFreshFile() throws { + let dir = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: dir) } + let target = dir.appendingPathComponent("shot.png") + + try IOSDeviceCommand.Screenshot.captureAtomically(to: target) { temporary in + try Data("fresh".utf8).write(to: temporary) + } + #expect(try Data(contentsOf: target) == Data("fresh".utf8)) + #expect(try FileManager.default.contentsOfDirectory(atPath: dir.path) == ["shot.png"]) } @Test("device selection errors tell the user how to recover") From 15c9a7e10d4530240486c40546dc78f061bc6a71 Mon Sep 17 00:00:00 2001 From: onevcat Date: Thu, 27 Aug 2026 10:30:02 +0900 Subject: [PATCH 4/4] fix: keep max-length and slash-containing names inside the output contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two round-3 review findings on ios-device screenshot: - The temporary capture name embedded the full target basename plus a ~50-byte suffix, so a valid NAME_MAX-length (255-byte) *.png target failed with ENAMETOOLONG when devicectl created the temporary. The temporary basename is now fixed and short (.sim-use-screenshot-partial-.png) — same directory and the UUID already guarantee uniqueness; nothing needed the target name. - The default filename interpolated the user-editable device name verbatim, so a name like "My iPhone/Work" turned the default output into a directory hierarchy (and ".." segments could walk out of the current directory), breaking the documented "single file in the current directory" default. Device names now pass through OutputFilePath.safeFilenameComponent ("/" -> "-"); with separators gone, a ".." can never form a standalone path component, which closes the traversal case too. Regression tests: 255-byte target captures (also verified live on device), slash names stay one component, and a traversal-shaped device name resolves into the current directory. Co-Authored-By: Claude Fable 5 Signed-off-by: onevcat --- Sources/SimUseCore/OutputFilePath.swift | 9 +++++ .../Verbs/IOSDeviceCommand.swift | 18 ++++++---- Tests/IOSDeviceBackendTests.swift | 33 +++++++++++++++++++ 3 files changed, 54 insertions(+), 6 deletions(-) diff --git a/Sources/SimUseCore/OutputFilePath.swift b/Sources/SimUseCore/OutputFilePath.swift index dc2d4a2b..d4b4c82e 100644 --- a/Sources/SimUseCore/OutputFilePath.swift +++ b/Sources/SimUseCore/OutputFilePath.swift @@ -80,6 +80,15 @@ public enum OutputFilePath { } } + /// Collapse a free-form name (e.g. a user-editable device name) into a + /// single path component for default filenames: path separators become + /// "-" so a name like "My iPhone/Work" cannot introduce directory + /// hierarchy — or, via "..", escape the target directory — when the name + /// is embedded in a default filename. + public static func safeFilenameComponent(_ name: String) -> String { + name.replacingOccurrences(of: "/", with: "-") + } + /// Timestamp format shared by every screenshot default filename so paired /// screenshots from cross-platform sessions sort together. public static func screenshotTimestamp(_ date: Date) -> String { diff --git a/Sources/iOSDeviceBackend/Verbs/IOSDeviceCommand.swift b/Sources/iOSDeviceBackend/Verbs/IOSDeviceCommand.swift index 2fb441bf..7ad1979c 100644 --- a/Sources/iOSDeviceBackend/Verbs/IOSDeviceCommand.swift +++ b/Sources/iOSDeviceBackend/Verbs/IOSDeviceCommand.swift @@ -203,10 +203,13 @@ public struct IOSDeviceCommand: AsyncParsableCommand { var output: String? /// Mirrors the simulator's default naming so paired screenshots from - /// cross-platform sessions sort together. Static so tests can pin the - /// convention without a device. + /// cross-platform sessions sort together. The device name is + /// user-editable free text, so it is collapsed into a single safe + /// path component first — "My iPhone/Work" must not create a + /// directory hierarchy. Static so tests can pin the convention + /// without a device. static func defaultFilename(deviceName: String, at date: Date) -> String { - "Device Screenshot - \(deviceName) - \(OutputFilePath.screenshotTimestamp(date)).png" + "Device Screenshot - \(OutputFilePath.safeFilenameComponent(deviceName)) - \(OutputFilePath.screenshotTimestamp(date)).png" } /// Resolve, validate the extension, then ensure the parent directory @@ -227,12 +230,15 @@ public struct IOSDeviceCommand: AsyncParsableCommand { /// Captures into a temporary sibling file and moves it over the final /// target only on success, so a capture that fails mid-flight (device /// unplugged, devicectl timeout) leaves an existing file at --output - /// untouched. The temporary name keeps the .png suffix devicectl - /// requires. Injectable capture so tests can pin the failure branch. + /// untouched. The temporary basename is fixed and short — deriving it + /// from the target name would push a NAME_MAX-length (255-byte) + /// target over the per-component limit — and keeps the .png suffix + /// devicectl requires. Injectable capture so tests can pin the + /// failure branch. static func captureAtomically(to url: URL, capture: (URL) throws -> Void) throws { let fileManager = FileManager.default let temporary = url.deletingLastPathComponent() - .appendingPathComponent(".\(url.lastPathComponent).partial-\(UUID().uuidString).png") + .appendingPathComponent(".sim-use-screenshot-partial-\(UUID().uuidString).png") do { try capture(temporary) if fileManager.fileExists(atPath: url.path) { diff --git a/Tests/IOSDeviceBackendTests.swift b/Tests/IOSDeviceBackendTests.swift index de773def..f4223948 100644 --- a/Tests/IOSDeviceBackendTests.swift +++ b/Tests/IOSDeviceBackendTests.swift @@ -342,6 +342,39 @@ struct IOSDeviceBackendTests { #expect(name.hasSuffix(".png")) } + @Test("a device name containing path separators stays a single filename component") + func deviceNameWithSlashesStaysSingleComponent() { + let name = IOSDeviceCommand.Screenshot.defaultFilename( + deviceName: "My iPhone/Work", + at: Date(timeIntervalSince1970: 0) + ) + #expect(!name.contains("/")) + #expect(name.hasPrefix("Device Screenshot - My iPhone-Work - ")) + } + + @Test("a traversal-shaped device name cannot escape the current directory") + func traversalDeviceNameResolvesIntoCwd() throws { + let url = try IOSDeviceCommand.Screenshot.resolveOutputURL(output: nil, deviceName: "../../evil") + #expect(url.deletingLastPathComponent().path == FileManager.default.currentDirectoryPath) + #expect(!url.lastPathComponent.contains("/")) + } + + @Test("a NAME_MAX-length target name still captures — the temporary name is independent of it") + func maxLengthTargetNameCaptures() throws { + let dir = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: dir) } + let name = String(repeating: "a", count: 251) + ".png" + #expect(name.utf8.count == 255) + let target = dir.appendingPathComponent(name) + + try IOSDeviceCommand.Screenshot.captureAtomically(to: target) { temporary in + try Data("fresh".utf8).write(to: temporary) + } + #expect(try Data(contentsOf: target) == Data("fresh".utf8)) + #expect(try FileManager.default.contentsOfDirectory(atPath: dir.path) == [name]) + } + @Test("a rejected non-PNG output path leaves the existing file intact") func rejectedOutputPreservesExistingFile() throws { let dir = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString)