From af2f06cc216c583b02eeeea9218e2e42785f6215 Mon Sep 17 00:00:00 2001 From: Oliver Drobnik Date: Sat, 9 May 2026 13:04:17 +0200 Subject: [PATCH 1/2] SubprocessModule: bridge `import Subprocess` to Shell.processLauncher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the SwiftScript half of the polyglot subprocess unification. ShellKit shipped `Shell.processLauncher` (Cocoanetics/ShellKit#1) and SwiftBash registered `BashProcessLauncher` (Cocoanetics/SwiftBash#19); this bridge lets a SwiftScript script write idiomatic swift-subprocess code and have it route through whatever launcher the bound shell carries. ```swift import Subprocess let r = try await Subprocess.run( Executable.name("git"), arguments: ["status"], output: Output.string(limit: 4096)) if r.terminationStatus.isSuccess { print(r.standardOutput ?? "") } ``` Routing: - **Standalone** (no embedder) → `DefaultProcessLauncher` → swift-subprocess `run(...)` → real `posix_spawn` (macOS / Linux / Windows / Android). - **Under SwiftBash** → `BashProcessLauncher` → resolves against the bash command registry. No `posix_spawn`, ever — pure-Swift builtins and registered SwiftPorts CLIs. - **Sandbox-bound** without a virtualised launcher → `ProcessLaunchDenied`, surfaced to the script as a thrown error. What v1 ships: * `Executable.name(_:)` / `.path(_:)` * `Output.string(limit:)` / `Output.discarded` and the symmetric `ErrorOutput.string(limit:)` / `ErrorOutput.discarded`. Modeled as separate type names because Swift's `Error` protocol identifier collides with the swift-subprocess `Error` generic parameter when used script-side. * `Subprocess.run(_:arguments:output:)` and `Subprocess.run(_:arguments:output:error:)` overloads, dispatched by argument count behind a single empty-paren bridge key (matches the existing convention for static methods like `Double.minimum`). * `ExecutionRecord.{terminationStatus, standardOutput, standardError, processIdentifier}`. `standardOutput` / `standardError` are Optional — `nil` for `.discarded`, the captured UTF-8 text for `.string(limit:)`. * `TerminationStatus.{isSuccess, exitedCode, signaledCode}`. The case-extracting accessors stand in for pattern-matching the underlying enum (`if case .exited(let code) = ...`), which SwiftScript doesn't model for opaque enum values. What v1 defers (script-author-visible items the bridge could grow later, ordered by likely demand): `environment:` / `workingDirectory:` / `input:` parameters on `run`, closure-form `run(...) { execution, stdout in ... }` overloads, streaming `AsyncSequence` outputs, `PlatformOptions`. For v1 the environment + cwd come from `ShellKit.Shell.current` and stdin is empty — covers the 95%-case shell-out pattern. Implementation note: the bridge body wraps the `Output.string(limit:)` adapter in a buffering `OutputSink` that the launcher streams into, then folds the captured bytes back into the returned `ExecutionRecord`. Honors the `.discarded` contract by dropping launcher-supplied bytes even when `DefaultProcessLauncher` populated its own record. Tests: `SubprocessBridgeTests` covers dispatch routing through a recording launcher (executable name/path passthrough, argv shape, exit-status mapping, stdout/stderr capture, sandbox-deny error surfacing) plus one platform-gated test (`#if os(macOS) || os(Linux)`) that goes end-to-end through `DefaultProcessLauncher` → real swift-subprocess. Suite: 10 tests, all pass; full SwiftScript suite 446/446 still green. Package.resolved bumps ShellKit pin to the `Shell.processLauncher`-bearing commit. SwiftScript itself does NOT add swift-subprocess as a direct dep — ShellKit owns that relationship; this module uses ShellKit's mirroring types (`Executable`, `Arguments`, `Environment`, `ExecutionRecord`, `TerminationStatus`). Co-Authored-By: Claude Opus 4.7 (1M context) --- Package.resolved | 22 +- .../Builtins/Registry.swift | 7 + .../Modules/SubprocessModule.swift | 344 ++++++++++++++++++ .../SubprocessBridgeTests.swift | 257 +++++++++++++ 4 files changed, 628 insertions(+), 2 deletions(-) create mode 100644 Sources/SwiftScriptInterpreter/Modules/SubprocessModule.swift create mode 100644 Tests/SwiftScriptInterpreterTests/SubprocessBridgeTests.swift diff --git a/Package.resolved b/Package.resolved index a210e83..0ae14b5 100644 --- a/Package.resolved +++ b/Package.resolved @@ -1,5 +1,5 @@ { - "originHash" : "723f256dacf5a9bc49a4e3f3e3199b1f10b82b02c55d2ad4ae5a576846570b90", + "originHash" : "0cd8515406137deb69d604344cc3fc16f80b2518f4dd27a5eeda64f7673d3cbd", "pins" : [ { "identity" : "shellkit", @@ -7,7 +7,7 @@ "location" : "https://github.com/Cocoanetics/ShellKit", "state" : { "branch" : "main", - "revision" : "3901e20158a2260586a4332ae244b74449da893a" + "revision" : "4395986d5c90e763daf8a92af739c5382b675bea" } }, { @@ -19,6 +19,15 @@ "version" : "1.7.1" } }, + { + "identity" : "swift-subprocess", + "kind" : "remoteSourceControl", + "location" : "https://github.com/swiftlang/swift-subprocess", + "state" : { + "revision" : "13d087685b95d64d6aac9b94500d347bbe84c39b", + "version" : "0.4.0" + } + }, { "identity" : "swift-syntax", "kind" : "remoteSourceControl", @@ -27,6 +36,15 @@ "revision" : "9de99a78f099e59caf2b2beec65a4c45d54b2081", "version" : "603.0.1" } + }, + { + "identity" : "swift-system", + "kind" : "remoteSourceControl", + "location" : "https://github.com/apple/swift-system", + "state" : { + "revision" : "7c6ad0fc39d0763e0b699210e4124afd5041c5df", + "version" : "1.6.4" + } } ], "version" : 3 diff --git a/Sources/SwiftScriptInterpreter/Builtins/Registry.swift b/Sources/SwiftScriptInterpreter/Builtins/Registry.swift index 66e9906..898262c 100644 --- a/Sources/SwiftScriptInterpreter/Builtins/Registry.swift +++ b/Sources/SwiftScriptInterpreter/Builtins/Registry.swift @@ -83,6 +83,13 @@ extension Interpreter { registerOnImport("Glibc", module: identityModule) registerOnImport("ucrt", module: identityModule) registerOnImport("WinSDK", module: identityModule) + // `Subprocess.run(...)` — collected-output bridge that mirrors + // swift-subprocess's API shape and routes every call through + // `ShellKit.Shell.current.processLauncher.launch(...)`. + // Standalone gets `DefaultProcessLauncher` (real exec via + // swift-subprocess); under SwiftBash gets `BashProcessLauncher` + // (resolves against the bash command registry, no `posix_spawn`). + registerOnImport("Subprocess", module: SubprocessModule()) } func registerBuiltin(name: String, body: @escaping ([Value]) async throws -> Value) { diff --git a/Sources/SwiftScriptInterpreter/Modules/SubprocessModule.swift b/Sources/SwiftScriptInterpreter/Modules/SubprocessModule.swift new file mode 100644 index 0000000..35ec158 --- /dev/null +++ b/Sources/SwiftScriptInterpreter/Modules/SubprocessModule.swift @@ -0,0 +1,344 @@ +import Foundation +import ShellKit + +/// `import Subprocess` bridge — a minimal v1 surface that mirrors +/// swift-subprocess's collected-output `run(...)` shape and routes +/// every call through `ShellKit.Shell.current.processLauncher.launch(...)`. +/// +/// ```swift +/// import Subprocess +/// +/// let r = try await Subprocess.run( +/// .name("echo"), +/// arguments: ["hello"], +/// output: .string(limit: 4096)) +/// +/// if r.terminationStatus.isSuccess { +/// print(r.standardOutput ?? "") +/// } +/// ``` +/// +/// Routing: +/// - **Standalone** SwiftScript scripts → `DefaultProcessLauncher` → +/// real `posix_spawn` via swift-subprocess (macOS / Linux / Windows +/// / Android). +/// - **Under SwiftBash** → `BashProcessLauncher` → resolves against +/// the bash command registry (no `posix_spawn`, ever). +/// - **Sandbox-bound** without a virtualised launcher → +/// `ProcessLaunchDenied`, surfaced to the script as a thrown +/// error. +/// +/// What v1 ships: +/// - `Executable.name(_:)` / `.path(_:)` +/// - `Output.string(limit:)` / `Output.discarded` and the symmetric +/// `ErrorOutput.string(limit:)` / `ErrorOutput.discarded` +/// - `Subprocess.run(_:arguments:output:)` and +/// `Subprocess.run(_:arguments:output:error:)` +/// - `ExecutionRecord.{terminationStatus, standardOutput, +/// standardError, processIdentifier}` +/// - `TerminationStatus.{isSuccess, exitedCode, signaledCode}` +/// +/// What v1 defers (script-author-visible items the bridge could grow +/// later, ordered by likely demand): `environment:` / +/// `workingDirectory:` / `input:` parameters on `run`, +/// closure-form `run(...) { execution, stdout in ... }` overloads, +/// streaming `AsyncSequence` outputs, `PlatformOptions` (spawn +/// attrs / file actions). For v1, environment + cwd come from +/// `ShellKit.Shell.current` and stdin is empty — this matches the +/// 95%-case shell-out pattern. +struct SubprocessModule: BuiltinModule { + let name = "Subprocess" + + func register(into i: Interpreter) { + registerExecutable(into: i) + registerOutputStrategies(into: i) + registerRun(into: i) + registerExecutionRecord(into: i) + registerTerminationStatus(into: i) + } + + // MARK: Executable + + private func registerExecutable(into i: Interpreter) { + // Bridge keys for static methods use empty `()` — the static- + // member resolver looks up the function value with empty + // labels, then the caller's positional args flow into the + // body verbatim. (See `Double.minimum` / `Bool.random` for the + // existing precedent.) + i.bridges["static func Executable.name()"] = .staticMethod { args in + guard args.count == 1 else { + throw RuntimeError.invalid("Executable.name(_:): expected 1 argument(s), got \(args.count)") + } + return boxOpaque(Executable.name(try unboxString(args[0])), typeName: "Executable") + } + i.bridges["static func Executable.path()"] = .staticMethod { args in + guard args.count == 1 else { + throw RuntimeError.invalid("Executable.path(_:): expected 1 argument(s), got \(args.count)") + } + return boxOpaque(Executable.path(try unboxString(args[0])), typeName: "Executable") + } + } + + // MARK: Output / ErrorOutput strategies + + private func registerOutputStrategies(into i: Interpreter) { + // Two parallel surfaces — `Output` and `ErrorOutput` — wrap + // the same `SubprocessIOStrategy` enum but carry distinct + // type names so the dispatch table can tell them apart at + // call-site label resolution. Mirrors swift-subprocess's + // `OutputProtocol` / `ErrorOutputProtocol` split (renamed + // here because `Error` collides with Swift's built-in + // `Error` protocol identifier in script syntax). + i.bridges["static func Output.string()"] = .staticMethod { args in + guard args.count == 1 else { + throw RuntimeError.invalid("Output.string(limit:): expected 1 argument(s), got \(args.count)") + } + return boxOpaque(SubprocessIOStrategy.string(limit: try unboxInt(args[0])), typeName: "Output") + } + i.bridges["static let Output.discarded"] = .staticValue( + boxOpaque(SubprocessIOStrategy.discarded, typeName: "Output")) + + i.bridges["static func ErrorOutput.string()"] = .staticMethod { args in + guard args.count == 1 else { + throw RuntimeError.invalid("ErrorOutput.string(limit:): expected 1 argument(s), got \(args.count)") + } + return boxOpaque(SubprocessIOStrategy.string(limit: try unboxInt(args[0])), typeName: "ErrorOutput") + } + i.bridges["static let ErrorOutput.discarded"] = .staticValue( + boxOpaque(SubprocessIOStrategy.discarded, typeName: "ErrorOutput")) + } + + // MARK: Subprocess.run overloads + + private func registerRun(into i: Interpreter) { + // Single bridge entry — the body dispatches by argument count + // for the two supported overload shapes: + // 3 args: (executable, arguments, output) + // 4 args: (executable, arguments, output, error) + // Caller-supplied labels (`arguments:` / `output:` / `error:`) + // are stripped by the static-member resolver before we see + // them, so positional order is the contract. + i.bridges["static func Subprocess.run()"] = .staticMethod { args in + switch args.count { + case 3: + return try await runImpl( + executable: args[0], + arguments: args[1], + output: args[2], + error: nil) + case 4: + return try await runImpl( + executable: args[0], + arguments: args[1], + output: args[2], + error: args[3]) + default: + throw RuntimeError.invalid( + "Subprocess.run: expected 3 or 4 arguments, got \(args.count)") + } + } + } + + // MARK: ExecutionRecord + + private func registerExecutionRecord(into i: Interpreter) { + i.bridges["var ExecutionRecord.terminationStatus"] = .computed { recv in + let r: ExecutionRecord = try unboxOpaque(recv, as: ExecutionRecord.self, typeName: "ExecutionRecord") + return boxOpaque(r.terminationStatus, typeName: "TerminationStatus") + } + i.bridges["var ExecutionRecord.standardOutput"] = .computed { recv in + let r: ExecutionRecord = try unboxOpaque(recv, as: ExecutionRecord.self, typeName: "ExecutionRecord") + // Optional — `nil` if the output strategy was + // `.discarded` (no buffer to read). + if r.standardOutput.isEmpty { + return .optional(nil) + } + return .optional(.string(String(decoding: r.standardOutput, as: UTF8.self))) + } + i.bridges["var ExecutionRecord.standardError"] = .computed { recv in + let r: ExecutionRecord = try unboxOpaque(recv, as: ExecutionRecord.self, typeName: "ExecutionRecord") + if r.standardError.isEmpty { + return .optional(nil) + } + return .optional(.string(String(decoding: r.standardError, as: UTF8.self))) + } + i.bridges["var ExecutionRecord.processIdentifier"] = .computed { recv in + let r: ExecutionRecord = try unboxOpaque(recv, as: ExecutionRecord.self, typeName: "ExecutionRecord") + return .int(Int(r.processIdentifier)) + } + } + + // MARK: TerminationStatus + + private func registerTerminationStatus(into i: Interpreter) { + i.bridges["var TerminationStatus.isSuccess"] = .computed { recv in + let s: TerminationStatus = try unboxOpaque(recv, as: TerminationStatus.self, typeName: "TerminationStatus") + return .bool(s.isSuccess) + } + // Pattern matching against opaque enum values is out of scope + // for the v1 interpreter, so flatten the two cases into + // optional accessors. Scripts read `r.terminationStatus.exitedCode` + // (nil if signaled) and `.signaledCode` (nil if exited) + // instead of `if case .exited(let c) = r.terminationStatus`. + i.bridges["var TerminationStatus.exitedCode"] = .computed { recv in + let s: TerminationStatus = try unboxOpaque(recv, as: TerminationStatus.self, typeName: "TerminationStatus") + if case .exited(let code) = s { return .optional(.int(Int(code))) } + return .optional(nil) + } + i.bridges["var TerminationStatus.signaledCode"] = .computed { recv in + let s: TerminationStatus = try unboxOpaque(recv, as: TerminationStatus.self, typeName: "TerminationStatus") + if case .signaled(let sig) = s { return .optional(.int(Int(sig))) } + return .optional(nil) + } + } +} + +// MARK: - Internals + +/// IO strategy used by ``Subprocess.run`` to decide how to handle each +/// stream. v1 only models `.string(limit:)` (capture into a UTF-8 +/// string up to `limit` bytes) and `.discarded`. Stream-based +/// (`AsyncSequence`) and file-descriptor strategies that +/// swift-subprocess offers are deferred. +enum SubprocessIOStrategy: Sendable { + case discarded + case string(limit: Int) +} + +/// Shared body for every `Subprocess.run` overload. Resolves the +/// strategies, allocates buffering ``OutputSink``s where the strategy +/// asks for captured bytes, dispatches through +/// `Shell.current.processLauncher.launch(...)`, and folds the buffered +/// bytes into a returned ``ExecutionRecord``. +private func runImpl( + executable: Value, + arguments: Value, + output: Value, + error: Value? +) async throws -> Value { + let exe = try unboxOpaque(executable, as: Executable.self, typeName: "Executable") + + // Accept `[String]` (the ergonomic call-site form swift-subprocess + // gets via `ExpressibleByArrayLiteral` on `Arguments`) or a boxed + // `Arguments` opaque (in case a future bridge adds the explicit + // type). + let args: Arguments + if case .array(let arr) = arguments { + let strs = try arr.map { try unboxString($0) } + args = Arguments(strs) + } else if case .opaque(let n, let any) = arguments, + n == "Arguments", + let a = any as? Arguments { + args = a + } else { + throw RuntimeError.invalid("Subprocess.run: arguments must be [String] (got \(arguments))") + } + + let outStrategy = try unboxOpaque(output, as: SubprocessIOStrategy.self, typeName: "Output") + let errStrategy: SubprocessIOStrategy + if let e = error { + errStrategy = try unboxOpaque(e, as: SubprocessIOStrategy.self, typeName: "ErrorOutput") + } else { + errStrategy = .discarded + } + + let outBuf = StrategyBuffer(strategy: outStrategy) + let errBuf = StrategyBuffer(strategy: errStrategy) + + let shell = ShellKit.Shell.current + let record = try await shell.processLauncher.launch( + exe, + arguments: args, + environment: shell.environment, + workingDirectory: nil, + input: .empty, + output: outBuf.sink, + error: errBuf.sink) + + // Strategy semantics: + // - `.discarded` → script-visible buffer is empty regardless of + // what the launcher returned. The launcher MAY still populate + // `record.standardOutput` (DefaultProcessLauncher does); we + // honour the script's "don't capture" contract by dropping it. + // - `.string(limit:)` → prefer the bridge-supplied buffer (what + // actually landed in the user's allocation), fall back to the + // launcher's record buffer. `BashProcessLauncher` streams to the + // sink only and leaves the record empty; `DefaultProcessLauncher` + // populates both. Both styles produce the same script-visible + // string. + let outBytes: Data + switch outStrategy { + case .discarded: + outBytes = Data() + case .string: + outBytes = outBuf.capturedBytes ?? record.standardOutput + } + let errBytes: Data + switch errStrategy { + case .discarded: + errBytes = Data() + case .string: + errBytes = errBuf.capturedBytes ?? record.standardError + } + + let merged = ExecutionRecord( + processIdentifier: record.processIdentifier, + terminationStatus: record.terminationStatus, + standardOutput: outBytes, + standardError: errBytes) + + return boxOpaque(merged, typeName: "ExecutionRecord") +} + +/// Pair of `OutputSink` + optional capture buffer driven by a +/// ``SubprocessIOStrategy``. `.discarded` returns a no-op sink with +/// no buffer; `.string(limit:)` returns a sink that appends every +/// chunk to a length-bounded byte buffer the run body reads back. +private struct StrategyBuffer { + let sink: OutputSink + /// Captured bytes if the strategy was `.string(limit:)`; nil for + /// `.discarded`. Empty `Data` (vs. `nil`) distinguishes "captured + /// nothing" from "didn't capture" so the run body can decide + /// whether to fall back to the launcher's own record buffer. + var capturedBytes: Data? { box?.read() } + + private let box: BufferBox? + + init(strategy: SubprocessIOStrategy) { + switch strategy { + case .discarded: + self.sink = .discard + self.box = nil + case .string(let limit): + let b = BufferBox(limit: limit) + self.box = b + // Capturing `b` (a class) by value gives every write the + // same instance; per-write append is locked. + self.sink = OutputSink(onWrite: { d in b.append(d) }) + } + } +} + +private final class BufferBox: @unchecked Sendable { + private let lock = NSLock() + private var buf = Data() + private let limit: Int + + init(limit: Int) { self.limit = limit } + + func append(_ d: Data) { + lock.lock(); defer { lock.unlock() } + let remaining = limit - buf.count + guard remaining > 0 else { return } + if d.count <= remaining { + buf.append(d) + } else { + buf.append(d.prefix(remaining)) + } + } + + func read() -> Data { + lock.lock(); defer { lock.unlock() } + return buf + } +} diff --git a/Tests/SwiftScriptInterpreterTests/SubprocessBridgeTests.swift b/Tests/SwiftScriptInterpreterTests/SubprocessBridgeTests.swift new file mode 100644 index 0000000..80f106c --- /dev/null +++ b/Tests/SwiftScriptInterpreterTests/SubprocessBridgeTests.swift @@ -0,0 +1,257 @@ +import Testing +import Foundation +import ShellKit +@testable import SwiftScriptInterpreter + +/// Tests for the `import Subprocess` bridge in SwiftScript. Each +/// installs a recording ``ProcessLauncher`` on the bound shell so the +/// tests don't depend on the host filesystem; one platform-gated test +/// at the end exercises the standalone path through real exec via +/// `DefaultProcessLauncher`. +@Suite("Subprocess bridge") +struct SubprocessBridgeTests { + + // MARK: Dispatch routing + + @Test func runRoutesThroughBoundProcessLauncher() async throws { + let launcher = RecordingLauncher(stubStdout: "captured\n") + let shell = TestShell(launcher: launcher) + try await shell.shellKit.withCurrent { + let interp = Interpreter() + _ = try await interp.eval(#""" + import Subprocess + let r = try await Subprocess.run( + Executable.name("any-name"), + arguments: ["a", "b"], + output: Output.string(limit: 4096)) + """#) + } + #expect(launcher.lastExecutableDescription == "any-name") + #expect(launcher.lastArguments == ["a", "b"]) + } + + @Test func executablePathPassesThroughVerbatim() async throws { + let launcher = RecordingLauncher() + let shell = TestShell(launcher: launcher) + try await shell.shellKit.withCurrent { + let interp = Interpreter() + _ = try await interp.eval(#""" + import Subprocess + _ = try await Subprocess.run( + Executable.path("/usr/local/bin/myprog"), + arguments: [], + output: Output.discarded) + """#) + } + #expect(launcher.lastExecutableDescription == "/usr/local/bin/myprog") + } + + // MARK: Output capture + + @Test func outputStringCapturesStdoutToStandardOutput() async throws { + let launcher = RecordingLauncher(stubStdout: "hi\n") + let shell = TestShell(launcher: launcher) + try await shell.shellKit.withCurrent { + let interp = Interpreter() + let r = try await interp.eval(#""" + import Subprocess + let result = try await Subprocess.run( + Executable.name("greet"), + arguments: [], + output: Output.string(limit: 4096)) + result.standardOutput + """#) + #expect(r == .optional(.string("hi\n"))) + } + } + + @Test func outputDiscardedReturnsNil() async throws { + let launcher = RecordingLauncher(stubStdout: "ignored\n") + let shell = TestShell(launcher: launcher) + try await shell.shellKit.withCurrent { + let interp = Interpreter() + let r = try await interp.eval(#""" + import Subprocess + let result = try await Subprocess.run( + Executable.name("greet"), + arguments: [], + output: Output.discarded) + result.standardOutput + """#) + // `.discarded` skips the buffer, the launcher's stub + // bytes never make it back into the script-visible + // record. + #expect(r == .optional(nil)) + } + } + + @Test func errorStringCapturesStderr() async throws { + let launcher = RecordingLauncher(stubStderr: "boom\n") + let shell = TestShell(launcher: launcher) + try await shell.shellKit.withCurrent { + let interp = Interpreter() + let r = try await interp.eval(#""" + import Subprocess + let result = try await Subprocess.run( + Executable.name("noisy"), + arguments: [], + output: Output.discarded, + error: ErrorOutput.string(limit: 4096)) + result.standardError + """#) + #expect(r == .optional(.string("boom\n"))) + } + } + + // MARK: TerminationStatus + + @Test func terminationStatusIsSuccessForExitZero() async throws { + let launcher = RecordingLauncher(termination: .exited(0)) + let shell = TestShell(launcher: launcher) + try await shell.shellKit.withCurrent { + let interp = Interpreter() + let r = try await interp.eval(#""" + import Subprocess + let result = try await Subprocess.run( + Executable.name("ok"), arguments: [], output: Output.discarded) + result.terminationStatus.isSuccess + """#) + #expect(r == .bool(true)) + } + } + + @Test func terminationStatusExitedCodeForNonZero() async throws { + let launcher = RecordingLauncher(termination: .exited(7)) + let shell = TestShell(launcher: launcher) + try await shell.shellKit.withCurrent { + let interp = Interpreter() + let r = try await interp.eval(#""" + import Subprocess + let result = try await Subprocess.run( + Executable.name("rc"), arguments: [], output: Output.discarded) + result.terminationStatus.exitedCode + """#) + #expect(r == .optional(.int(7))) + } + } + + @Test func terminationStatusSignaledCodeForSignaled() async throws { + let launcher = RecordingLauncher(termination: .signaled(15)) + let shell = TestShell(launcher: launcher) + try await shell.shellKit.withCurrent { + let interp = Interpreter() + let r = try await interp.eval(#""" + import Subprocess + let result = try await Subprocess.run( + Executable.name("term"), arguments: [], output: Output.discarded) + (result.terminationStatus.signaledCode, result.terminationStatus.exitedCode) + """#) + #expect(r == .tuple([.optional(.int(15)), .optional(nil)], labels: [])) + } + } + + // MARK: Sandbox refusal + + @Test func sandboxedDenyLauncherSurfacesAsThrownError() async { + let shell = TestShell(launcher: SandboxedDenyLauncher(reason: "no exec under test")) + var caught: String? + await shell.shellKit.withCurrent { + let interp = Interpreter() + do { + _ = try await interp.eval(#""" + import Subprocess + _ = try await Subprocess.run( + Executable.name("anything"), + arguments: [], + output: Output.discarded) + """#) + } catch { + caught = String(describing: error) + } + } + // The launcher's `ProcessLaunchDenied` propagates to the + // script as an error — typed-error pattern matching against + // ShellKit types is not available script-side, but the bridge + // does not eat it either. + #expect(caught != nil) + } + + // MARK: Real-exec path (standalone) + + #if os(macOS) || os(Linux) + @Test func standalonePathDelegatesToRealExec() async throws { + // The default shell uses `DefaultProcessLauncher`, which goes + // through `swift-subprocess.run(...)` to the real OS. Confirms + // the bridge plumbs all the way through end-to-end on + // platforms where real exec is available. + let shell = TestShell() // default launcher + try await shell.shellKit.withCurrent { + let interp = Interpreter() + let r = try await interp.eval(#""" + import Subprocess + let result = try await Subprocess.run( + Executable.path("/bin/echo"), + arguments: ["hi"], + output: Output.string(limit: 4096)) + result.standardOutput + """#) + #expect(r == .optional(.string("hi\n"))) + } + } + #endif +} + +// MARK: - Recording launcher + +private final class RecordingLauncher: ProcessLauncher, @unchecked Sendable { + private let stubStdout: Data + private let stubStderr: Data + private let termination: TerminationStatus + + // Plain stored properties — every test runs the launcher from a + // single Task, so the cross-task coordination NSLock would buy + // is unneeded. `@unchecked Sendable` advertises that. + var lastExecutableDescription: String? + var lastArguments: [String]? + + init( + stubStdout: String = "", + stubStderr: String = "", + termination: TerminationStatus = .exited(0) + ) { + self.stubStdout = Data(stubStdout.utf8) + self.stubStderr = Data(stubStderr.utf8) + self.termination = termination + } + + func launch( + _ executable: Executable, + arguments: Arguments, + environment: Environment, + workingDirectory: String?, + input: InputSource, + output: OutputSink, + error: OutputSink + ) async throws -> ExecutionRecord { + lastExecutableDescription = executable.description + lastArguments = arguments.values + + if !stubStdout.isEmpty { output.write(stubStdout) } + if !stubStderr.isEmpty { error.write(stubStderr) } + + return ExecutionRecord( + processIdentifier: 999, + terminationStatus: termination, + standardOutput: stubStdout, + standardError: stubStderr) + } +} + +// MARK: - TestShell variant that takes a launcher + +private extension TestShell { + convenience init(launcher: any ProcessLauncher) { + self.init() + self.shellKit.processLauncher = launcher + } +} From 84f4a3aecb7f7447bbaa615bdb7c84c9ccefafe7 Mon Sep 17 00:00:00 2001 From: Oliver Drobnik Date: Sat, 9 May 2026 14:02:56 +0200 Subject: [PATCH 2/2] Address Codex review on PR #5: distinguish discarded-vs-empty + throw on overflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two issues from the automated review on https://github.com/Cocoanetics/SwiftScript/pull/5: 1. **P1 — Preserve empty captured output instead of returning `nil`.** `Subprocess.run(..., output: .string(limit:))` with zero captured bytes was mapping `ExecutionRecord.standardOutput` to `nil` because of a naive `isEmpty` check in the accessor. That conflated two different states: `.discarded` (script asked for nothing → `nil`) versus `.string(limit:)` with an empty capture (script asked for capture, command was silent → `Optional("")`). A script could not tell a silent command apart from a discarded stream. The same pattern applied to `standardError`. Fix: the bridge now wraps the launcher's `ExecutionRecord` in a private `ScriptExecutionRecord` that carries per-stream "captured" flags (`outputCaptured` / `errorCaptured`). `.string(limit:)` sets the flag; `.discarded` doesn't. The property accessors return `nil` only when the flag is false, `Optional(String(...))` otherwise — even for an empty buffer. 2. **P1 — Throw on output-limit overflow instead of silently truncating.** `Output.string(limit:)` is documented in swift-subprocess to throw if emitted bytes exceed the limit. The previous bridge used `d.prefix(remaining)` and continued silently. A script parsing JSON or a status payload would get incomplete data with no error signal. Fix: `BufferBox` now records an `overflowed` flag when an incoming chunk would push past the limit. The run body inspects the captured state after `launch(...)` returns and throws a new `SubprocessOutputLimitExceeded` error (carrying the offending stream + the configured limit) when either stream overflowed. The threshold check also covers the fall-through case where the launcher populated `record.standardOutput` directly without going through the bridge sink. `OutputSink.onWrite` is non-throwing (`(Data) -> Void`), so the throw has to land in the run body rather than the sink closure — overflow is recorded in-band and surfaced at the first opportunity. Tests: - `outputStringPreservesEmptyCaptureAsEmptyString` — silent command + `.string(limit: 4096)` → `Optional("")`. - `outputStringThrowsOnLimitOverflow` — 50-byte stub against a 10-byte limit throws; error message names the stream and the limit. - `errorStringThrowsOnLimitOverflow` — symmetric coverage on the stderr side. Suite: 13/13 (was 10). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Modules/SubprocessModule.swift | 139 +++++++++++++----- .../SubprocessBridgeTests.swift | 73 +++++++++ 2 files changed, 178 insertions(+), 34 deletions(-) diff --git a/Sources/SwiftScriptInterpreter/Modules/SubprocessModule.swift b/Sources/SwiftScriptInterpreter/Modules/SubprocessModule.swift index 35ec158..97f3146 100644 --- a/Sources/SwiftScriptInterpreter/Modules/SubprocessModule.swift +++ b/Sources/SwiftScriptInterpreter/Modules/SubprocessModule.swift @@ -143,28 +143,31 @@ struct SubprocessModule: BuiltinModule { private func registerExecutionRecord(into i: Interpreter) { i.bridges["var ExecutionRecord.terminationStatus"] = .computed { recv in - let r: ExecutionRecord = try unboxOpaque(recv, as: ExecutionRecord.self, typeName: "ExecutionRecord") - return boxOpaque(r.terminationStatus, typeName: "TerminationStatus") + let r: ScriptExecutionRecord = try unboxOpaque( + recv, as: ScriptExecutionRecord.self, typeName: "ExecutionRecord") + return boxOpaque(r.record.terminationStatus, typeName: "TerminationStatus") } + // `.discarded` → `nil`. `.string(limit:)` → `Optional(String)` + // even if the captured byte buffer is empty — that's how a + // script distinguishes a silent command from a discarded + // stream (matches swift-subprocess's `Optional` with + // empty-string-on-no-output semantics). i.bridges["var ExecutionRecord.standardOutput"] = .computed { recv in - let r: ExecutionRecord = try unboxOpaque(recv, as: ExecutionRecord.self, typeName: "ExecutionRecord") - // Optional — `nil` if the output strategy was - // `.discarded` (no buffer to read). - if r.standardOutput.isEmpty { - return .optional(nil) - } - return .optional(.string(String(decoding: r.standardOutput, as: UTF8.self))) + let r: ScriptExecutionRecord = try unboxOpaque( + recv, as: ScriptExecutionRecord.self, typeName: "ExecutionRecord") + guard r.outputCaptured else { return .optional(nil) } + return .optional(.string(String(decoding: r.record.standardOutput, as: UTF8.self))) } i.bridges["var ExecutionRecord.standardError"] = .computed { recv in - let r: ExecutionRecord = try unboxOpaque(recv, as: ExecutionRecord.self, typeName: "ExecutionRecord") - if r.standardError.isEmpty { - return .optional(nil) - } - return .optional(.string(String(decoding: r.standardError, as: UTF8.self))) + let r: ScriptExecutionRecord = try unboxOpaque( + recv, as: ScriptExecutionRecord.self, typeName: "ExecutionRecord") + guard r.errorCaptured else { return .optional(nil) } + return .optional(.string(String(decoding: r.record.standardError, as: UTF8.self))) } i.bridges["var ExecutionRecord.processIdentifier"] = .computed { recv in - let r: ExecutionRecord = try unboxOpaque(recv, as: ExecutionRecord.self, typeName: "ExecutionRecord") - return .int(Int(r.processIdentifier)) + let r: ScriptExecutionRecord = try unboxOpaque( + recv, as: ScriptExecutionRecord.self, typeName: "ExecutionRecord") + return .int(Int(r.record.processIdentifier)) } } @@ -205,6 +208,39 @@ enum SubprocessIOStrategy: Sendable { case string(limit: Int) } +/// Wraps the launcher's `ExecutionRecord` with per-stream "captured" +/// flags so ``ExecutionRecord/standardOutput`` / +/// ``ExecutionRecord/standardError`` can return `Optional("")` for a +/// `.string(limit:)` capture that produced zero bytes — distinct from +/// `nil` for `.discarded` which never captured at all. Without this +/// the script can't tell a silent command apart from a discarded +/// stream (Codex P1 on PR #5). +struct ScriptExecutionRecord: Sendable { + let record: ExecutionRecord + let outputCaptured: Bool // false when `Output.discarded` + let errorCaptured: Bool // false when `ErrorOutput.discarded` +} + +/// Thrown when a stream produced more bytes than the supplied +/// `Output.string(limit:)` / `ErrorOutput.string(limit:)` budget. +/// Matches swift-subprocess's "throw on overflow" contract — silently +/// truncating would hand scripts incomplete data with no way to +/// detect the loss (Codex P1 on PR #5). +struct SubprocessOutputLimitExceeded: Error, CustomStringConvertible, Sendable { + /// Which stream overflowed. + let stream: Stream + /// The limit that was exceeded. + let limit: Int + + enum Stream: String, Sendable { + case standardOutput, standardError + } + + var description: String { + "Subprocess.\(stream.rawValue) exceeded the configured limit of \(limit) bytes" + } +} + /// Shared body for every `Subprocess.run` overload. Resolves the /// strategies, allocates buffering ``OutputSink``s where the strategy /// asks for captured bytes, dispatches through @@ -256,29 +292,50 @@ private func runImpl( error: errBuf.sink) // Strategy semantics: - // - `.discarded` → script-visible buffer is empty regardless of - // what the launcher returned. The launcher MAY still populate - // `record.standardOutput` (DefaultProcessLauncher does); we - // honour the script's "don't capture" contract by dropping it. + // - `.discarded` → no captured bytes; script reads `nil` regardless + // of what the launcher returned. Honours the script's "don't + // capture" contract — `DefaultProcessLauncher` does populate + // `record.standardOutput` even when the supplied sink is + // discarding, so we drop the launcher copy here too. // - `.string(limit:)` → prefer the bridge-supplied buffer (what // actually landed in the user's allocation), fall back to the // launcher's record buffer. `BashProcessLauncher` streams to the // sink only and leaves the record empty; `DefaultProcessLauncher` - // populates both. Both styles produce the same script-visible - // string. + // populates both. If the buffer overflowed the configured limit, + // throw rather than silently truncate (matches swift-subprocess). let outBytes: Data switch outStrategy { case .discarded: outBytes = Data() - case .string: - outBytes = outBuf.capturedBytes ?? record.standardOutput + case .string(let limit): + if let captured = outBuf.captured { + if captured.overflowed { + throw SubprocessOutputLimitExceeded(stream: .standardOutput, limit: limit) + } + outBytes = captured.data + } else { + outBytes = record.standardOutput + if outBytes.count > limit { + throw SubprocessOutputLimitExceeded(stream: .standardOutput, limit: limit) + } + } } let errBytes: Data switch errStrategy { case .discarded: errBytes = Data() - case .string: - errBytes = errBuf.capturedBytes ?? record.standardError + case .string(let limit): + if let captured = errBuf.captured { + if captured.overflowed { + throw SubprocessOutputLimitExceeded(stream: .standardError, limit: limit) + } + errBytes = captured.data + } else { + errBytes = record.standardError + if errBytes.count > limit { + throw SubprocessOutputLimitExceeded(stream: .standardError, limit: limit) + } + } } let merged = ExecutionRecord( @@ -286,8 +343,12 @@ private func runImpl( terminationStatus: record.terminationStatus, standardOutput: outBytes, standardError: errBytes) + let scriptRecord = ScriptExecutionRecord( + record: merged, + outputCaptured: { if case .string = outStrategy { return true } else { return false } }(), + errorCaptured: { if case .string = errStrategy { return true } else { return false } }()) - return boxOpaque(merged, typeName: "ExecutionRecord") + return boxOpaque(scriptRecord, typeName: "ExecutionRecord") } /// Pair of `OutputSink` + optional capture buffer driven by a @@ -296,11 +357,13 @@ private func runImpl( /// chunk to a length-bounded byte buffer the run body reads back. private struct StrategyBuffer { let sink: OutputSink - /// Captured bytes if the strategy was `.string(limit:)`; nil for - /// `.discarded`. Empty `Data` (vs. `nil`) distinguishes "captured - /// nothing" from "didn't capture" so the run body can decide - /// whether to fall back to the launcher's own record buffer. - var capturedBytes: Data? { box?.read() } + /// Captured bytes plus an overflow flag if the strategy was + /// `.string(limit:)`; nil for `.discarded`. The presence-vs-nil + /// distinction lets the run body tell "no buffer was allocated" + /// (fall back to the launcher's record) from "buffer was + /// allocated and produced zero bytes" (script-visible empty + /// string). + var captured: (data: Data, overflowed: Bool)? { box?.read() } private let box: BufferBox? @@ -319,15 +382,23 @@ private struct StrategyBuffer { } } +/// Length-bounded byte buffer that records overflow without +/// throwing — `OutputSink.onWrite` is non-throwing, so the throw +/// has to happen later out of the run body when it inspects the +/// captured state. private final class BufferBox: @unchecked Sendable { private let lock = NSLock() private var buf = Data() + private var overflowed = false private let limit: Int init(limit: Int) { self.limit = limit } func append(_ d: Data) { lock.lock(); defer { lock.unlock() } + if buf.count + d.count > limit { + overflowed = true + } let remaining = limit - buf.count guard remaining > 0 else { return } if d.count <= remaining { @@ -337,8 +408,8 @@ private final class BufferBox: @unchecked Sendable { } } - func read() -> Data { + func read() -> (data: Data, overflowed: Bool) { lock.lock(); defer { lock.unlock() } - return buf + return (buf, overflowed) } } diff --git a/Tests/SwiftScriptInterpreterTests/SubprocessBridgeTests.swift b/Tests/SwiftScriptInterpreterTests/SubprocessBridgeTests.swift index 80f106c..9ba8e47 100644 --- a/Tests/SwiftScriptInterpreterTests/SubprocessBridgeTests.swift +++ b/Tests/SwiftScriptInterpreterTests/SubprocessBridgeTests.swift @@ -65,6 +65,79 @@ struct SubprocessBridgeTests { } } + @Test func outputStringPreservesEmptyCaptureAsEmptyString() async throws { + // `.string(limit:)` with zero captured bytes returns + // `Optional("")` — distinct from `nil` for `.discarded`. Lets + // a script tell a silent command apart from a discarded + // stream. + let launcher = RecordingLauncher(stubStdout: "") + let shell = TestShell(launcher: launcher) + try await shell.shellKit.withCurrent { + let interp = Interpreter() + let r = try await interp.eval(#""" + import Subprocess + let result = try await Subprocess.run( + Executable.name("silent"), + arguments: [], + output: Output.string(limit: 4096)) + result.standardOutput + """#) + #expect(r == .optional(.string(""))) + } + } + + @Test func outputStringThrowsOnLimitOverflow() async throws { + // `.string(limit: N)` is documented to throw if emitted bytes + // exceed N — silently truncating would hand the script + // partial data with no way to detect the loss. Configure a + // launcher that produces 50 bytes against a 10-byte limit and + // verify the bridge surfaces the overflow as an error. + let launcher = RecordingLauncher(stubStdout: String(repeating: "x", count: 50)) + let shell = TestShell(launcher: launcher) + var caught: String? + await shell.shellKit.withCurrent { + let interp = Interpreter() + do { + _ = try await interp.eval(#""" + import Subprocess + _ = try await Subprocess.run( + Executable.name("noisy"), + arguments: [], + output: Output.string(limit: 10)) + """#) + } catch { + caught = String(describing: error) + } + } + #expect(caught != nil) + #expect(caught?.contains("standardOutput") == true) + #expect(caught?.contains("10 bytes") == true) + } + + @Test func errorStringThrowsOnLimitOverflow() async throws { + // Same overflow contract on the stderr side. + let launcher = RecordingLauncher(stubStderr: String(repeating: "y", count: 100)) + let shell = TestShell(launcher: launcher) + var caught: String? + await shell.shellKit.withCurrent { + let interp = Interpreter() + do { + _ = try await interp.eval(#""" + import Subprocess + _ = try await Subprocess.run( + Executable.name("noisy"), + arguments: [], + output: Output.discarded, + error: ErrorOutput.string(limit: 5)) + """#) + } catch { + caught = String(describing: error) + } + } + #expect(caught != nil) + #expect(caught?.contains("standardError") == true) + } + @Test func outputDiscardedReturnsNil() async throws { let launcher = RecordingLauncher(stubStdout: "ignored\n") let shell = TestShell(launcher: launcher)