diff --git a/Packages/ICCeryCore/Sources/ICCeryCore/Argyll/ArgyllRunner.swift b/Packages/ICCeryCore/Sources/ICCeryCore/Argyll/ArgyllRunner.swift index 9dcabe5..7e4d499 100644 --- a/Packages/ICCeryCore/Sources/ICCeryCore/Argyll/ArgyllRunner.swift +++ b/Packages/ICCeryCore/Sources/ICCeryCore/Argyll/ArgyllRunner.swift @@ -87,6 +87,7 @@ public struct ArgyllRunner: Sendable { let binaryURL = binaryResolver.resolve("targen") let processId = ProcessID.targen(cleanBasename) + await ensureNotRunning(id: processId) let events = processManager.events() try await processManager.runStreaming( id: processId, @@ -121,6 +122,7 @@ public struct ArgyllRunner: Sendable { let binaryURL = binaryResolver.resolve("printtarg") let processId = ProcessID.printtarg(cleanBasename) + await ensureNotRunning(id: processId) let events = processManager.events() try await processManager.runStreaming( id: processId, @@ -169,6 +171,19 @@ public struct ArgyllRunner: Sendable { // MARK: - Shared collection + /// Cancels any previous child with the same id and waits for it to + /// finalize, so `runStreaming` / `runCaptured` never sees a + /// `duplicateID` from a leftover process (#50, #52). + private func ensureNotRunning(id: String) async { + guard await processManager.isRunning(id) else { return } + await processManager.kill(id: id) + var attempts = 0 + while await processManager.isRunning(id), attempts < 30 { + try? await Task.sleep(for: .milliseconds(100)) + attempts += 1 + } + } + private struct CollectedRun { var exitCode: Int32? var stdout: String @@ -179,10 +194,15 @@ public struct ArgyllRunner: Sendable { /// Drains the event stream until this child's `exit` event. /// stdout is accumulated both per-line (logs) and verbatim (for /// the manifest parse — the pretty JSON needs its newlines). + /// + /// When `flushPartialLines` is `true`, a background `Task` flushes + /// unterminated output every 500 ms so tools like `colprof` that + /// print dots without newlines still produce log batches. private func collect( id processId: String, events: AsyncStream, - onLogBatch: (@Sendable ([String]) -> Void)? + onLogBatch: (@Sendable ([String]) -> Void)?, + flushPartialLines: Bool = false ) async -> CollectedRun { var lines: [String] = [] var stdout = "" @@ -198,6 +218,17 @@ public struct ArgyllRunner: Sendable { onLogBatch?(out) } + var dotFlushTask: Task? + if flushPartialLines { + dotFlushTask = Task { [processManager] in + while !Task.isCancelled { + try? await Task.sleep(for: .milliseconds(500)) + if Task.isCancelled { break } + await processManager.flushPartialLine(id: processId) + } + } + } + for await event in events { guard event.id == processId else { continue } switch event { @@ -229,6 +260,12 @@ public struct ArgyllRunner: Sendable { break } } + + dotFlushTask?.cancel() + if let dotFlushTask { + _ = await dotFlushTask.value + } + return CollectedRun(exitCode: exitCode, stdout: stdout, stderr: stderr, lines: lines) } @@ -242,6 +279,7 @@ public struct ArgyllRunner: Sendable { let binaryURL = binaryResolver.resolve("instlist") let processId = ProcessID.instlist + await ensureNotRunning(id: processId) let events = processManager.events() try await processManager.runStreaming( id: processId, @@ -301,6 +339,7 @@ public struct ArgyllRunner: Sendable { let binaryURL = binaryResolver.resolve("average") let processId = ProcessID.average(config.basename) + await ensureNotRunning(id: processId) let events = processManager.events() try await processManager.runStreaming( id: processId, @@ -335,6 +374,7 @@ public struct ArgyllRunner: Sendable { let binaryURL = binaryResolver.resolve("colprof") let processId = ProcessID.colprof(cleanBasename) + await ensureNotRunning(id: processId) let events = processManager.events() try await processManager.runStreaming( id: processId, @@ -342,7 +382,12 @@ public struct ArgyllRunner: Sendable { arguments: args, workingDirectory: cwd ) - let run = await collect(id: processId, events: events, onLogBatch: onLogBatch) + let run = await collect( + id: processId, + events: events, + onLogBatch: onLogBatch, + flushPartialLines: true + ) guard run.exitCode == 0 else { throw ArgyllRunnerError.colprofFailed( @@ -368,10 +413,13 @@ public struct ArgyllRunner: Sendable { /// /// Runs `applycal` captured and performs an in-place replace via /// `{input}.applycal.tmp` then `replaceItemAt`. On failure the tmp - /// file is removed and the original is left untouched. + /// file is removed and the original is left untouched. The UI must + /// never request `unapply` (#52). public func runApplycal( config: ApplycalConfig ) async throws -> URL { + assert(!config.unapply, "runApplycal does not support unapply") + let inputURL = config.inputProfileURL let cwd = inputURL.deletingLastPathComponent() let binaryURL = binaryResolver.resolve("applycal") @@ -383,6 +431,8 @@ public struct ArgyllRunner: Sendable { // Remove any stale tmp from a previous crash. try? fm.removeItem(at: tmpURL) + await ensureNotRunning(id: processId) + let outputConfig = ApplycalConfig( calibrationPath: config.calibrationPath, inputProfileURL: inputURL, @@ -398,8 +448,11 @@ public struct ArgyllRunner: Sendable { workingDirectory: cwd ) - guard result.exitCode == 0 else { + guard result.exitCode == 0, !Task.isCancelled else { try? fm.removeItem(at: tmpURL) + if Task.isCancelled { + throw CancellationError() + } throw ArgyllRunnerError.applycalFailed( result.stderr.isEmpty ? "applycal exited with code \(result.exitCode)" @@ -413,6 +466,15 @@ public struct ArgyllRunner: Sendable { ) } + let attrs = try? fm.attributesOfItem(atPath: tmpURL.path) + let size = attrs?[.size] as? UInt64 ?? 0 + guard size >= 128 else { + try? fm.removeItem(at: tmpURL) + throw ArgyllRunnerError.applycalFailed( + "calibrated profile is too small (\(size) bytes)" + ) + } + do { if fm.fileExists(atPath: inputURL.path) { _ = try fm.replaceItemAt(inputURL, withItemAt: tmpURL) @@ -441,6 +503,7 @@ public struct ArgyllRunner: Sendable { let binaryURL = binaryResolver.resolve("iccgamut") let processId = ProcessID.iccgamut(stem: stem) + await ensureNotRunning(id: processId) let events = processManager.events() try await processManager.runStreaming( id: processId, @@ -480,6 +543,7 @@ public struct ArgyllRunner: Sendable { let binaryURL = binaryResolver.resolve("profcheck") let processId = ProcessID.profcheck(ti3Path: ti3Path) + await ensureNotRunning(id: processId) let events = processManager.events() try await processManager.runStreaming( id: processId, @@ -547,11 +611,21 @@ public struct ArgyllRunner: Sendable { let binaryURL = binaryResolver.resolve("chartread") let processId = ProcessID.chartread(cleanBasename) let processManager = self.processManager + let isXY = config.isXY return AsyncStream { continuation in let task = Task { + await ensureNotRunning(id: processId) let events = processManager.events() + // Register the XY parking hook before spawning. + await processManager.setPreKillHook(id: processId) { [processManager] in + if isXY { + try? await processManager.sendStdin(id: processId, bytes: ChartreadInput.quit.bytes) + try? await Task.sleep(for: .milliseconds(500)) + } + } + do { try await processManager.runStreaming( id: processId, @@ -582,20 +656,22 @@ public struct ArgyllRunner: Sendable { switch event { case .stdout(_, let line): - let classified = ChartreadClassifier.classify(line: line, previousState: state) + let previous = state + let classified = ChartreadClassifier.classify(line: line, previousState: previous) state = classified.state + if classified.isRemoveSheetNotice { continuation.yield(.removeSheetNotice) } - if classified.sheetNumber != nil || classified.alignmentPatch != nil { - continuation.yield(.prompt(classified)) - } else if state != previousOrContinuationState(state, classified) { - // Only emit prompt when the state meaningfully changes. - continuation.yield(.prompt(classified)) - } else if state == .tablePlaceSheet || state == .tableAlign { - // Continuation lines in table states are still prompts. - continuation.yield(.prompt(classified)) - } else if classified.requestedWarningKey != nil { + + let shouldPrompt = + classified.sheetNumber != nil + || classified.alignmentPatch != nil + || classified.requestedWarningKey != nil + || classified.state != previous + || classified.isTableContinuation + + if shouldPrompt { continuation.yield(.prompt(classified)) } @@ -632,6 +708,11 @@ public struct ArgyllRunner: Sendable { } } + if Task.isCancelled { + continuation.finish() + return + } + let canonical = cwd.appendingPathComponent("\(cleanBasename).ti3") if let code = exitCode, code == 0 { if FileManager.default.fileExists(atPath: canonical.path) { @@ -647,15 +728,13 @@ public struct ArgyllRunner: Sendable { continuation.onTermination = { _ in task.cancel() + Task { + await processManager.kill(id: processId) + } } } } - private func previousOrContinuationState(_ state: ChartreadState, _ classified: ChartreadClassifyResult) -> ChartreadState { - if classified.isTableContinuation { return .promptContinue } - return state - } - /// Send an exact input sequence to the running `chartread` child. public func sendChartreadInput(basename: String, input: ChartreadInput) async throws { let cleanBasename = try PathSecurity.sanitizeBasename(basename) @@ -665,17 +744,14 @@ public struct ArgyllRunner: Sendable { /// Terminate a running `chartread` child. /// - /// For XY tables, sends `q\n` first and waits ~500 ms so the head parks. + /// The actual XY parking is handled by the pre-kill hook registered in + /// `runChartread`. public func cancelChartread(basename: String, isXY: Bool = false) { let cleanBasename = try? PathSecurity.sanitizeBasename(basename) guard let cleanBasename else { return } let processId = ProcessID.chartread(cleanBasename) Task { - if isXY { - try? await processManager.sendStdin(id: processId, bytes: ChartreadInput.quit.bytes) - try? await Task.sleep(for: .milliseconds(500)) - } await processManager.kill(id: processId) } } diff --git a/Packages/ICCeryCore/Sources/ICCeryCore/Measurement/ChartreadClassifier.swift b/Packages/ICCeryCore/Sources/ICCeryCore/Measurement/ChartreadClassifier.swift index af4e90d..097c8d7 100644 --- a/Packages/ICCeryCore/Sources/ICCeryCore/Measurement/ChartreadClassifier.swift +++ b/Packages/ICCeryCore/Sources/ICCeryCore/Measurement/ChartreadClassifier.swift @@ -153,7 +153,7 @@ public enum ChartreadClassifier { let phrases = [ "'d' if/when done", "d to finish/save", "all strips/patches read", "all strips read", "all patches read", "done reading", - "'d' to save", "press d to", "hit 'd'" + "'d' to save", "press d to", "hit 'd'", "d to finish", "d to save" ] if phrases.contains(where: { text.contains($0) }) { return ChartreadClassifyResult(state: .allStripsRead) @@ -163,20 +163,28 @@ public enum ChartreadClassifier { // 7. Warnings / prompts needing a key. private static func warning(text: String, previous: ChartreadState) -> ChartreadClassifyResult? { + let lower = text let warningSignals = [ - "(warning)", "use it anyway", "seem to have read strip pass", - "unexpected response", "seem to have read", "misread", - "try again", "do you want to" + "(warning)", "use it anyway", "seem to have read strip", + "unexpected response", "try again", "do you want to", + "abort ? - are you sure", "are you sure" ] - guard warningSignals.contains(where: { text.contains($0) }) else { return nil } + + let isWarningPrompt = + warningSignals.contains(where: { lower.contains($0) }) + || lower.contains("(y/n)") + || lower.contains("'y' or 'n'") + || lower.contains("?") + + guard isWarningPrompt else { return nil } var key: String? - if text.contains("(y/n)") || text.contains("'y' or 'n'") { + if lower.contains("(y/n)") || lower.contains("'y' or 'n'") { // Default to asking the user; no automatic key. key = nil - } else if text.contains("'y'") || text.contains("press y") || text.contains("hit 'y'") { + } else if lower.contains("'y'") || lower.contains("press y") || lower.contains("hit 'y'") { key = "y" - } else if text.contains("'n'") || text.contains("press n") || text.contains("hit 'n'") { + } else if lower.contains("'n'") || lower.contains("press n") || lower.contains("hit 'n'") { key = "n" } @@ -195,12 +203,14 @@ public enum ChartreadClassifier { !hasLocate else { return nil } - if lowercased.contains("hit any key to continue") - || lowercased.contains("hit space to continue") - || lowercased.contains("calibration") - || lowercased.contains("calibrate") + if lowercased.contains("calibrat") + || lowercased.contains("white reference") || lowercased.contains("white tile") - || lowercased.contains("standard tile") { + || lowercased.contains("standard tile") + || lowercased.contains("reference") + || lowercased.contains("tile") + || lowercased.contains("hit any key to continue") + || lowercased.contains("hit space to continue") { return ChartreadClassifyResult(state: .calibrating) } return nil @@ -209,11 +219,29 @@ public enum ChartreadClassifier { // 9. Awaiting strip. private static func awaitingStrip(text: String, previous: ChartreadState) -> ChartreadClassifyResult? { let lowercased = text.lowercased() + + // These are explicit, multi-word prompts; we deliberately do NOT + // match bare "read strip" so that error lines like + // "failed to read strip" or "error reading strip" fall through to + // the error matcher. let phrases = [ - "hit ... read ... strip", "ready to read", "read ... strip ... key", - "hit any key to read", "ready to read strip", "hit a key to read", - "press any key to read", "read strip" + "ready to read", + "hit any key to read", + "hit a key to read", + "hit space to read", + "hit [space] to read", + "press any key to read", + "press space to read", + "trigger instrument", + "start reading", + "read next strip" ] + + // Also permit "hit X to read strip Y" or "ready to read strip Z". + if lowercased.range(of: #"(hit|press).+to\s+read\s+strip"#, options: .regularExpression) != nil { + return ChartreadClassifyResult(state: .awaitingStrip) + } + guard phrases.contains(where: { lowercased.contains($0) }) else { return nil } return ChartreadClassifyResult(state: .awaitingStrip) } @@ -228,16 +256,27 @@ public enum ChartreadClassifier { // 11. Error. private static func error(text: String, previous: ChartreadState) -> ChartreadClassifyResult? { - let phrases = ["error", "too fast", "too slow", "misread", "failed to read", "failed"] - // Avoid false positives inside harmless words by matching full words where possible. - let lower = text - guard phrases.contains(where: { phrase in - lower.contains(phrase) && !lower.contains("no error") - }) else { return nil } + let lower = text.lowercased() - if lower.contains("misread") || lower.contains("failed to read") || lower.contains("error") { + // Avoid false positives from confirmation prompts and "no error" status. + guard !lower.contains("no error") else { return nil } + guard !lower.contains("(y/n)") + && !lower.contains("'y' or 'n'") + && !lower.contains("?") + else { return nil } + + let phraseMatches = ["failed to read", "error reading", "too fast", "too slow", "misread"] + for phrase in phraseMatches { + if lower.contains(phrase) { + return ChartreadClassifyResult(state: .error) + } + } + + // Whole-word "error" only — bare "failed" alone is not enough. + if lower.range(of: #"\berror\b"#, options: .regularExpression) != nil { return ChartreadClassifyResult(state: .error) } + return nil } } diff --git a/Packages/ICCeryCore/Sources/ICCeryCore/Process/ProcessLineDecoder.swift b/Packages/ICCeryCore/Sources/ICCeryCore/Process/ProcessLineDecoder.swift index ab79ff6..25f9c79 100644 --- a/Packages/ICCeryCore/Sources/ICCeryCore/Process/ProcessLineDecoder.swift +++ b/Packages/ICCeryCore/Sources/ICCeryCore/Process/ProcessLineDecoder.swift @@ -35,6 +35,18 @@ public struct ProcessLineDecoder: Sendable { return rest.isEmpty ? nil : Self.decode(rest) } + /// Emits the current unterminated tail as a single line and clears it. + /// Used by `ProcessManager.flushPartialLine` for tools that emit + /// progress dots without newlines. + public mutating func flushPartial() -> String? { + guard !pending.isEmpty else { return nil } + var rest = pending + pending.removeAll(keepingCapacity: false) + if rest.last == 0x0D { rest = rest.dropLast() } + let text = Self.decode(rest) + return text.isEmpty ? nil : text + } + private static func decode(_ bytes: Data.SubSequence) -> String { String(decoding: bytes, as: UTF8.self) } diff --git a/Packages/ICCeryCore/Sources/ICCeryCore/Process/ProcessManager.swift b/Packages/ICCeryCore/Sources/ICCeryCore/Process/ProcessManager.swift index 4d692fc..88c7f2d 100644 --- a/Packages/ICCeryCore/Sources/ICCeryCore/Process/ProcessManager.swift +++ b/Packages/ICCeryCore/Sources/ICCeryCore/Process/ProcessManager.swift @@ -20,8 +20,11 @@ public struct CapturedResult: Sendable, Equatable { /// with the prefix stripped; all other stdout is `stdout` events. /// - `exit` is emitted exactly once per child, and only after both /// output pipes reach EOF — so no buffered output is lost on fast -/// exits or kills. -/// - `kill` drops the stdin handle so writers fail fast. +/// exits or kills. If EOFs never arrive, a watchdog finalizes. +/// - `kill` runs a pre-kill hook (e.g. XY `q\n` + 500 ms park) before +/// terminating. Hooks are removed once the child finalizes. +/// - `killAll` on `NSApplication.willTerminate` and last-window close +/// runs all hooks and terminates every child (#147, #149). public actor ProcessManager { public static let rowColorsPrefix = "ROW_COLORS_JSON: " @@ -92,12 +95,18 @@ public actor ProcessManager { /// pipes have also reached EOF. var pendingExitCode: Int32? var finalized = false + /// Watchdog that forces finalization if EOFs never arrive. + var finalizeTask: Task? } private var children: [String: RunningChild] = [:] /// Processes owned by `runCaptured` (dup detection + kill support). private var captured: [String: Process] = [:] + /// Hooks run by `kill` before terminating the child. + /// Used by `chartread` to park an XY head with `q\n`. + private var preKillHooks: [String: @Sendable () async -> Void] = [:] + /// Ids of currently-running children. public var runningIDs: [String] { Array(children.keys) + captured.keys } @@ -105,6 +114,14 @@ public actor ProcessManager { children[id] != nil || captured[id] != nil } + // MARK: - Pre-kill hooks + + /// Register a hook to run before `kill(id:)` terminates the child. + /// The hook is removed once the child finalizes. + public func setPreKillHook(id: String, hook: @escaping @Sendable () async -> Void) { + preKillHooks[id] = hook + } + // MARK: - Spawn (streaming) /// Spawns a streaming child. Returns after spawn; callers wait for @@ -142,14 +159,6 @@ public actor ProcessManager { stderrDecoder: ProcessLineDecoder() ) - do { - try process.run() - } catch { - children.removeValue(forKey: id) - emit(.error(id: id, message: error.localizedDescription)) - throw ProcessError.spawnFailed("\(binary.path): \(error.localizedDescription)") - } - let stdoutHandle = stdoutPipe.fileHandleForReading let stderrHandle = stderrPipe.fileHandleForReading stdoutHandle.readabilityHandler = { [weak self] handle in @@ -167,6 +176,15 @@ public actor ProcessManager { guard let self else { return } Task { await self.didTerminate(id: id, code: proc.terminationStatus) } } + + do { + try process.run() + } catch { + preKillHooks.removeValue(forKey: id) + children.removeValue(forKey: id) + emit(.error(id: id, message: error.localizedDescription)) + throw ProcessError.spawnFailed("\(binary.path): \(error.localizedDescription)") + } } // MARK: - Spawn (captured) @@ -198,41 +216,114 @@ public actor ProcessManager { "spawn(captured) \(id): \(binary.path) \(LogSanitizer.sanitizeArgs(arguments))" ) - // Register before run() so a concurrent duplicate spawn fails. + // Register and set up the termination hand-off before run() so + // a very fast exit is never missed (#50, #52). captured[id] = process + let capturedProcess = process + + // Box is local and synchronised with an NSLock; the @unchecked + // Sendable annotation is safe because all access is under the lock. + final class Box: @unchecked Sendable { + private let lock = NSLock() + private var status: Int32? + private var continuation: CheckedContinuation? + + /// Try to resume an already-stored continuation with the exit + /// status. Returns true if a continuation was resumed. + func resume(with status: Int32) -> Bool { + lock.lock() + if let cont = continuation { + continuation = nil + lock.unlock() + cont.resume(returning: status) + return true + } + self.status = status + lock.unlock() + return false + } + + /// Store a continuation, returning any status that arrived + /// before it. The caller must resume with the returned status. + func store(_ continuation: CheckedContinuation) -> Int32? { + lock.lock() + if let status = status { + self.status = nil + self.continuation = nil + lock.unlock() + return status + } + self.continuation = continuation + // A fast exit may have raced past the first nil-check. + if let status = status { + self.status = nil + self.continuation = nil + lock.unlock() + return status + } + lock.unlock() + return nil + } + } + let box = Box() + capturedProcess.terminationHandler = { proc in + _ = box.resume(with: proc.terminationStatus) + } + do { try process.run() } catch { + _ = box.resume(with: -1) captured.removeValue(forKey: id) + preKillHooks.removeValue(forKey: id) emit(.error(id: id, message: error.localizedDescription)) throw ProcessError.spawnFailed("\(binary.path): \(error.localizedDescription)") } - async let outData = Task.detached { - stdoutPipe.fileHandleForReading.readDataToEndOfFile() - }.value - async let errData = Task.detached { - stderrPipe.fileHandleForReading.readDataToEndOfFile() - }.value + // Close the parent write ends so readDataToEndOfFile() gets EOF + // as soon as the child exits; the child still has its own copies. + try? stdoutPipe.fileHandleForWriting.close() + try? stderrPipe.fileHandleForWriting.close() - let code = await withCheckedContinuation { continuation in - process.terminationHandler = { proc in - continuation.resume(returning: proc.terminationStatus) + return await withTaskCancellationHandler { + async let outData = Task.detached { + stdoutPipe.fileHandleForReading.readDataToEndOfFile() + }.value + async let errData = Task.detached { + stderrPipe.fileHandleForReading.readDataToEndOfFile() + }.value + + let code = await withCheckedContinuation { continuation in + if let status = box.store(continuation) { + continuation.resume(returning: status) + } + } + + let (out, err) = await (outData, errData) + + // Emit the real exit code once, regardless of whether kill() + // already removed the id from `captured`. + _ = captured.removeValue(forKey: id) + preKillHooks.removeValue(forKey: id) + emit(.exit(id: id, code: code)) + + return CapturedResult( + stdout: String(decoding: out, as: UTF8.self), + stderr: String(decoding: err, as: UTF8.self), + exitCode: code + ) + } onCancel: { [weak self] in + // If the awaiting Task is cancelled, terminate the child so + // callers like runApplycal never replace a good profile with + // a truncated tmp. + if capturedProcess.isRunning { + capturedProcess.terminate() + } + Task { [weak self] in + await self?.kill(id: id) } } - - let (out, err) = await (outData, errData) - // If kill() already reaped this child, its exit event went out. - if captured.removeValue(forKey: id) != nil { - emit(.exit(id: id, code: code)) - } - - return CapturedResult( - stdout: String(decoding: out, as: UTF8.self), - stderr: String(decoding: err, as: UTF8.self), - exitCode: code - ) } // MARK: - stdin @@ -255,39 +346,89 @@ public actor ProcessManager { try sendStdin(id: id, bytes: Data(text.utf8)) } + // MARK: - Partial-line flush + + /// Emits the current unterminated tail of a streaming child's stdout + /// and stderr as ordinary lines. Callers (e.g. `colprof`) use this + /// to flush progress dots without waiting for a newline. + public func flushPartialLine(id: String) { + guard var child = children[id], !child.finalized else { return } + + if let tail = child.stdoutDecoder.flushPartial() { + if tail.hasPrefix(Self.rowColorsPrefix) { + let payload = Data(tail.dropFirst(Self.rowColorsPrefix.count).utf8) + emit(.jsonRow(id: id, payload: payload)) + } else { + emit(.stdout(id: id, line: tail)) + } + } + if let tail = child.stderrDecoder.flushPartial() { + emit(.stderr(id: id, line: tail)) + } + + children[id] = child + } + // MARK: - Kill - /// Terminates a child. The `exit` event still fires exactly once. - /// stdin is dropped immediately so writers fail fast (docs/03 rule 7). - public func kill(id: String) { + /// Terminates a child. First runs any registered pre-kill hook, then + /// drops stdin and signals the process. For streaming children the + /// `exit` event is emitted once both stdout and stderr EOFs have been + /// seen (or the watchdog finalizes). For captured children the real + /// exit code is emitted by `runCaptured` itself. + public func kill(id: String) async { + if let hook = preKillHooks.removeValue(forKey: id) { + await hook() + } + if var child = children[id] { try? child.stdin?.close() child.stdin = nil children[id] = child + if child.process.isRunning { child.process.terminate() - } else { - Task { await self.didTerminate(id: id, code: child.process.terminationStatus) } + } else if child.pendingExitCode == nil { + // The process already exited but `didTerminate` has not + // run; synthesize it so `maybeFinalize` can fire. + didTerminate(id: id, code: child.process.terminationStatus) } return } + if let process = captured[id] { if process.isRunning { process.terminate() } - if captured.removeValue(forKey: id) != nil { - emit(.exit(id: id, code: process.terminationStatus)) - } + // Do not emit `.exit` here; `runCaptured` emits the real code + // after the process reaps. + return } } /// Terminates every running child; returns how many were signaled /// (`kill_all_processes`, docs/03). Mandatory on app exit (#147/#149). @discardableResult - public func killAll() -> Int { - let ids = Array(children.keys) + Array(captured.keys) - for id in ids { kill(id: id) } + public func killAll() async -> Int { + let ids = runningIDs + for id in ids { await kill(id: id) } return ids.count } + // MARK: - Force kill (SIGKILL fallback) + + /// Sends `SIGKILL` to a streaming child if it is still running. + /// Used by the finalization watchdog when a graceful `terminate()` + /// does not cause the process to exit. + public func forceKill(id: String) { + guard let child = children[id], + !child.finalized, + child.process.isRunning + else { return } + + let pid = child.process.processIdentifier + guard pid > 0 else { return } + _ = Darwin.kill(pid, SIGKILL) + } + // MARK: - Internals private func childEnvironment(extra: [String: String]) -> [String: String] { @@ -339,6 +480,16 @@ public actor ProcessManager { child.pendingExitCode = code try? child.stdin?.close() child.stdin = nil + + // Start a watchdog in case the `readabilityHandler` EOFs never + // arrive after the process exits (e.g. a hung pipe). + child.finalizeTask = Task { [weak self] in + try? await Task.sleep(for: .seconds(2)) + guard let self else { return } + await self.forceKill(id: id) + await self.forceFinalize(id: id) + } + children[id] = child maybeFinalize(id: id) } @@ -351,8 +502,12 @@ public actor ProcessManager { child.stdoutEOF, child.stderrEOF, !child.finalized else { return } + child.finalized = true + child.finalizeTask?.cancel() + child.finalizeTask = nil children.removeValue(forKey: id) + preKillHooks.removeValue(forKey: id) // Flush unterminated tail lines. if var decoder = Optional(child.stdoutDecoder), @@ -369,4 +524,20 @@ public actor ProcessManager { } emit(.exit(id: id, code: code)) } + + /// Forces finalization even when one or both EOFs are missing. + /// Used by the `didTerminate` watchdog. + private func forceFinalize(id: String) { + guard var child = children[id], !child.finalized else { return } + + if child.pendingExitCode == nil { + child.pendingExitCode = -9 + } + child.stdoutEOF = true + child.stderrEOF = true + child.finalizeTask?.cancel() + child.finalizeTask = nil + children[id] = child + maybeFinalize(id: id) + } } diff --git a/Packages/ICCeryCore/Sources/ICCeryCore/Profile/DriftAlert.swift b/Packages/ICCeryCore/Sources/ICCeryCore/Profile/DriftAlert.swift index f673bd3..382f053 100644 --- a/Packages/ICCeryCore/Sources/ICCeryCore/Profile/DriftAlert.swift +++ b/Packages/ICCeryCore/Sources/ICCeryCore/Profile/DriftAlert.swift @@ -2,32 +2,40 @@ import Foundation /// Computes a consecutive-breach warning from verification history. /// -/// A drift alert triggers when there are at least two `poor` records on -/// distinct calendar days, or two `poor` records at least one hour apart. +/// A drift alert triggers when the most recent chronologically consecutive +/// poor records form a run of at least two, and the first and last of that +/// run are on distinct UTC days or at least one hour apart. public enum DriftAlert { /// Returns an alert message, or `nil` when no consecutive breach exists. public static func compute(from records: [VerificationRecord]) -> String? { - let poor = records - .filter { $0.status == .poor } - .sorted { $0.timestamp < $1.timestamp } + // Work in chronological order. + let chronological = records.sorted { $0.timestamp < $1.timestamp } - guard poor.count >= 2 else { return nil } - - for i in 0..= 3600 - - if !sameDay || oneHour { - return "Drift alert: poor results between \(a.id) and \(b.id)." - } + // Build the longest suffix of consecutive `.poor` records. + // Non-poor records break the run, so we stop at the first non-poor + // encountered from the end. + var run: [VerificationRecord] = [] + for record in chronological.reversed() { + if record.status == .poor { + run.insert(record, at: 0) + } else { + break } } + guard run.count >= 2 else { return nil } + + let first = run.first! + let last = run.last! + + let sameDay = Calendar.utc.isDate(first.timestamp, inSameDayAs: last.timestamp) + let oneHour = last.timestamp.timeIntervalSince(first.timestamp) >= 3600 + + if !sameDay || oneHour { + return "Drift alert: poor results between \(first.id) and \(last.id)." + } + return nil } } diff --git a/Packages/ICCeryCore/Sources/ICCeryCore/Profile/ProfileInstaller.swift b/Packages/ICCeryCore/Sources/ICCeryCore/Profile/ProfileInstaller.swift index a81dda8..7cd22bc 100644 --- a/Packages/ICCeryCore/Sources/ICCeryCore/Profile/ProfileInstaller.swift +++ b/Packages/ICCeryCore/Sources/ICCeryCore/Profile/ProfileInstaller.swift @@ -33,10 +33,32 @@ public enum ProfileInstallError: LocalizedError, Equatable, Sendable { /// Installs an ICC/ICM profile into the OS colour store. public enum ProfileInstaller { + /// Resolves the destination URL that `install` would write to for the + /// given source and options, without copying anything. Useful for + /// collision previews in the UI. + public static func resolveDestinationURL( + for config: InstallProfileConfig, + fileManager: FileManager = .default + ) throws -> URL { + let sourceURL = config.sourceURL + let ext = sourceURL.pathExtension.lowercased() + guard ext == "icc" || ext == "icm" else { + throw ProfileInstallError.sourceNotProfile + } + + try validateSourceURL(sourceURL) + + let destDir = destinationDirectory(for: config.options, fileManager: fileManager) + return destDir.appendingPathComponent(sourceURL.lastPathComponent) + } + /// Installs `sourceURL` into `~/Library/ColorSync/Profiles` or /// `/Library/ColorSync/Profiles`. Always copies, never moves. - public static func install(config: InstallProfileConfig) throws -> InstallProfileResult { - let fm = FileManager.default + public static func install( + config: InstallProfileConfig, + fileManager: FileManager = .default + ) throws -> InstallProfileResult { + let fm = fileManager // Source validation. let sourceURL = config.sourceURL @@ -55,38 +77,37 @@ public enum ProfileInstaller { throw ProfileInstallError.sourceTooSmall } - // Stem security. - let stem = sourceURL.deletingPathExtension().lastPathComponent - guard !stem.contains("..") && !stem.contains("/") && !stem.contains("\\") else { - throw ProfileInstallError.unsafeStem(stem) - } + try validateSourceURL(sourceURL) // Destination directory. - let destDir: URL - if config.options.preferSystem { - destDir = URL(fileURLWithPath: "/Library/ColorSync/Profiles") - } else { - let home = fm.homeDirectoryForCurrentUser - destDir = home.appendingPathComponent("Library/ColorSync/Profiles") - } - - // Ensure parent exists. - try? fm.createDirectory(at: destDir, withIntermediateDirectories: true) - - let destURL = destDir.appendingPathComponent("\(stem).icc") + let destURL = try resolveDestinationURL(for: config, fileManager: fm) + try? fm.createDirectory( + at: destURL.deletingLastPathComponent(), + withIntermediateDirectories: true + ) // Collision resolution. let destExists = fm.fileExists(atPath: destURL.path) if destExists { if config.options.forceOverwrite { - // Continue to overwrite path. + return try performInstall( + from: sourceURL, + to: destURL, + options: config.options, + fileManager: fm, + overwritten: true, + renamed: false + ) } else if config.options.collisionPolicy == .rename { let epoch = Int(Date().timeIntervalSince1970) - let renamedURL = destDir.appendingPathComponent("\(stem)-\(epoch).icc") + let stem = sourceURL.deletingPathExtension().lastPathComponent + let renamedURL = destURL.deletingLastPathComponent() + .appendingPathComponent("\(stem)-\(epoch).\(ext)") return try performInstall( from: sourceURL, to: renamedURL, options: config.options, + fileManager: fm, overwritten: false, renamed: true ) @@ -102,19 +123,53 @@ public enum ProfileInstaller { from: sourceURL, to: destURL, options: config.options, - overwritten: destExists, + fileManager: fm, + overwritten: false, renamed: false ) } + // MARK: - Private helpers + + private static func validateSourceURL(_ sourceURL: URL) throws { + let path = sourceURL.path + let stem = sourceURL.deletingPathExtension().lastPathComponent + + // Reject backslashes anywhere in the path. + guard !path.contains("\\") else { + throw ProfileInstallError.unsafeStem(stem) + } + + // Reject any path component that is literally "." or "..". + // This allows names like "foo..bar" while blocking real traversal. + for component in sourceURL.pathComponents { + if component == "." || component == ".." { + throw ProfileInstallError.unsafeStem(stem) + } + } + } + + private static func destinationDirectory( + for options: InstallProfileOptions, + fileManager: FileManager + ) -> URL { + if options.preferSystem { + return URL(fileURLWithPath: "/Library/ColorSync/Profiles") + } else { + return fileManager.homeDirectoryForCurrentUser + .appendingPathComponent("Library/ColorSync/Profiles") + } + } + private static func performInstall( from sourceURL: URL, to destURL: URL, options: InstallProfileOptions, + fileManager: FileManager, overwritten: Bool, renamed: Bool ) throws -> InstallProfileResult { - let fm = FileManager.default + let fm = fileManager let tmpURL = destURL.appendingPathExtension("iccery-install.tmp") // Remove stale tmp. @@ -123,6 +178,13 @@ public enum ProfileInstaller { do { try fm.copyItem(at: sourceURL, to: tmpURL) + let attrs = try? fm.attributesOfItem(atPath: tmpURL.path) + let tmpSize = attrs?[.size] as? UInt64 ?? 0 + guard tmpSize >= 128 else { + try? fm.removeItem(at: tmpURL) + throw ProfileInstallError.sourceTooSmall + } + if fm.fileExists(atPath: destURL.path) { _ = try fm.replaceItemAt(destURL, withItemAt: tmpURL) } else { @@ -135,6 +197,10 @@ public enum ProfileInstaller { if destURL.path.hasPrefix("/Library/") && !fm.fileExists(atPath: destURL.path) { throw ProfileInstallError.systemRequiresAdminRights } + + if let installError = error as? ProfileInstallError { + throw installError + } throw ProfileInstallError.copyFailed(error.localizedDescription) } diff --git a/Packages/ICCeryCore/Sources/ICCeryCore/Profile/VerificationHistoryStore.swift b/Packages/ICCeryCore/Sources/ICCeryCore/Profile/VerificationHistoryStore.swift index 6a06d97..2910aca 100644 --- a/Packages/ICCeryCore/Sources/ICCeryCore/Profile/VerificationHistoryStore.swift +++ b/Packages/ICCeryCore/Sources/ICCeryCore/Profile/VerificationHistoryStore.swift @@ -57,10 +57,12 @@ public actor VerificationHistoryStore { /// Appends a record, trims to capacity, and writes atomically. /// - /// Returns the trimmed list, or `nil` if a write error occurs so the - /// caller can surface the failure without replacing the in-memory list. + /// Loads the existing history first and propagates any load error so an + /// unparseable file is never overwritten. @discardableResult public func append(_ record: VerificationRecord) throws -> [VerificationRecord] { + try load() + var updated = records updated.append(record) if updated.count > capacity { diff --git a/Sources/ICCery/MeasurementWorkflowViewModel.swift b/Sources/ICCery/MeasurementWorkflowViewModel.swift index 66d3103..541608d 100644 --- a/Sources/ICCery/MeasurementWorkflowViewModel.swift +++ b/Sources/ICCery/MeasurementWorkflowViewModel.swift @@ -305,6 +305,10 @@ final class MeasurementWorkflowViewModel { environment.runner.cancelChartread(basename: basename, isXY: selectedInstrument.isXY) chartreadTask?.cancel() isChartreadRunning = false + chartreadState = .idle + currentPrompt = nil + requestedWarningKey = nil + showRemoveSheetNotice = false } func sendWarningKey(_ key: String) { diff --git a/Sources/ICCery/ProfileWorkflowViewModel.swift b/Sources/ICCery/ProfileWorkflowViewModel.swift index d1f0093..320c774 100644 --- a/Sources/ICCery/ProfileWorkflowViewModel.swift +++ b/Sources/ICCery/ProfileWorkflowViewModel.swift @@ -81,6 +81,15 @@ final class ProfileWorkflowViewModel { init(wizard: WizardViewModel, environment: AppEnvironment) { self.wizard = wizard self.environment = environment + restoreCreatedProfileURL() + } + + /// Restores `createdProfileURL` from the wizard artefacts or by probing + /// the working directory for an existing `.icc`/`.icm` (#52). + func restoreCreatedProfileURL() { + let cwd = wizard.effectiveWorkingDirectory ?? PathSecurity.resolveSafeCwd(nil) + createdProfileURL = wizard.artefacts.profilePath + ?? ArtefactProbe.resolveProfile(basename: wizard.basename, cwd: cwd) } // MARK: - Derived @@ -206,6 +215,7 @@ final class ProfileWorkflowViewModel { calibrationPath: self.calibrationFile, inputProfileURL: url ) + assert(!applyConfig.unapply, "applycal unapply is not supported in v2.0") finalProfileURL = try await runner.runApplycal(config: applyConfig) self.colprofLog.append("Calibration embedded: \(self.calibrationFile)") } @@ -256,7 +266,15 @@ final class ProfileWorkflowViewModel { // MARK: - Stage 5: verify profile var knownPrinters: [String] { - Array(Set(verificationHistory.map { $0.printerName })).sorted() + var names = Set() + for record in verificationHistory { + if record.printerName.isEmpty { + names.insert("Unknown") + } else { + names.insert(record.printerName) + } + } + return Array(names).sorted() } func loadHistory() { @@ -333,10 +351,11 @@ final class ProfileWorkflowViewModel { let timestamp = Date() let id = "vr-\(Int(timestamp.timeIntervalSince1970))-\(Self.nextSeq())" + let printerName = wizard.printerName?.isEmpty == false ? wizard.printerName! : "Unknown" return VerificationRecord( id: id, profileName: createdProfileURL?.lastPathComponent ?? wizard.basename, - printerName: wizard.printerName ?? "", + printerName: printerName, avgDE: avg, maxDE: max, rmsDE: rms, @@ -381,24 +400,32 @@ final class ProfileWorkflowViewModel { openColorPanel: settings.openColorPanelAfterInstall ) - let destURL = installDestination(for: sourceURL, options: options) - let collision = FileManager.default.fileExists(atPath: destURL.path) + do { + let config = InstallProfileConfig(sourceURL: sourceURL, options: options) + let destURL = try ProfileInstaller.resolveDestinationURL(for: config) + let collision = FileManager.default.fileExists(atPath: destURL.path) - if collision && settings.askBeforeOverwriteProfile { - pendingInstallOptions = options - installCollisionMessage = "A profile named \(destURL.lastPathComponent) already exists." - showingInstallCollision = true - return + if collision && settings.askBeforeOverwriteProfile { + pendingInstallOptions = options + installCollisionMessage = "A profile named \(destURL.lastPathComponent) already exists." + showingInstallCollision = true + return + } + + runInstall(sourceURL: sourceURL, options: options) + } catch { + wizard.showNotice( + "Install failed: \(error.localizedDescription)", + kind: .error + ) } - - runInstall(sourceURL: sourceURL, options: options) } func resolveInstallCollision(policy: ProfileCollisionPolicy) { showingInstallCollision = false guard let sourceURL = createdProfileURL, var options = pendingInstallOptions else { return } - options.collisionPolicy = policy + if policy == .cancel { installResult = InstallProfileResult( destPath: "", @@ -410,20 +437,12 @@ final class ProfileWorkflowViewModel { ) return } - runInstall(sourceURL: sourceURL, options: options) - } - private func installDestination(for sourceURL: URL, options: InstallProfileOptions) -> URL { - let stem = sourceURL.deletingPathExtension().lastPathComponent - let fm = FileManager.default - let destDir: URL - if options.preferSystem { - destDir = URL(fileURLWithPath: "/Library/ColorSync/Profiles") - } else { - destDir = fm.homeDirectoryForCurrentUser - .appendingPathComponent("Library/ColorSync/Profiles") + options.collisionPolicy = policy + if policy == .overwrite { + options.forceOverwrite = true } - return destDir.appendingPathComponent("\(stem).icc") + runInstall(sourceURL: sourceURL, options: options) } private func runInstall(sourceURL: URL, options: InstallProfileOptions) { diff --git a/Sources/ICCery/Stage3View.swift b/Sources/ICCery/Stage3View.swift index 2509fb3..290b2d3 100644 --- a/Sources/ICCery/Stage3View.swift +++ b/Sources/ICCery/Stage3View.swift @@ -210,7 +210,9 @@ struct Stage3View: View { .accessibilityIdentifier("btnCalibrate") case .awaitingStrip: Button("Trigger") { model.calibrate() } - .accessibilityIdentifier("btnCalibrate") + .accessibilityIdentifier("btnTrigger") + Button("Done & Save") { model.doneAndSave() } + .accessibilityIdentifier("btnDoneReadEarly") case .tablePlaceSheet, .tableAlign, .promptContinue, .warning: Button(continueTitle) { model.accept() } .accessibilityIdentifier("btnAccept") @@ -224,16 +226,6 @@ struct Stage3View: View { EmptyView() } - if model.chartreadState == .awaitingStrip || model.chartreadState == .allStripsRead { - Button("Done & Save") { model.doneAndSave() } - .accessibilityIdentifier("btnDoneRead") - } - - if model.chartreadState == .error { - Button("Retry") { model.retry() } - .accessibilityIdentifier("btnRetry") - } - Button("Cancel") { model.cancelRead() } .accessibilityIdentifier("btnCancel") } @@ -374,7 +366,7 @@ struct Stage3View: View { Button("Finish & Average") { model.finishAndAverage() } - .disabled(!model.isFinished || model.isFinishing) + .disabled(!model.canFinish || model.isFinishing) .accessibilityIdentifier("btnFinishAndAverage") } @@ -386,6 +378,7 @@ struct Stage3View: View { } .padding(16) .background(Theme.panel) + .accessibilityElement(children: .contain) .accessibilityIdentifier("chartreadAveragingPanel") } } diff --git a/Sources/ICCery/Stage4View.swift b/Sources/ICCery/Stage4View.swift index 3fe4a08..e879abb 100644 --- a/Sources/ICCery/Stage4View.swift +++ b/Sources/ICCery/Stage4View.swift @@ -18,6 +18,7 @@ struct Stage4View: View { } .frame(maxWidth: .infinity, maxHeight: .infinity) .background(Theme.background) + .onAppear { model.restoreCreatedProfileURL() } } // MARK: - Header diff --git a/Sources/ICCery/Stage5View.swift b/Sources/ICCery/Stage5View.swift index af7b760..a6c18bd 100644 --- a/Sources/ICCery/Stage5View.swift +++ b/Sources/ICCery/Stage5View.swift @@ -21,7 +21,10 @@ struct Stage5View: View { } .frame(maxWidth: .infinity, maxHeight: .infinity) .background(Theme.background) - .onAppear { model.loadHistory() } + .onAppear { + model.restoreCreatedProfileURL() + model.loadHistory() + } .alert("Install profile", isPresented: $model.showingInstallCollision) { Button("Overwrite", role: .destructive) { model.resolveInstallCollision(policy: .overwrite) @@ -70,7 +73,7 @@ struct Stage5View: View { .accessibilityIdentifier("driftAlert") } - if let warning = model.profcheckWarning, !warning.isEmpty, model.driftAlert == nil { + if let warning = model.profcheckWarning, !warning.isEmpty { Text("⚠ \(warning)") .font(.caption) .padding(.horizontal, 8) diff --git a/Tests/ICCeryCoreTests/ApplycalArgsTests.swift b/Tests/ICCeryCoreTests/ApplycalArgsTests.swift index 9075509..fb10001 100644 --- a/Tests/ICCeryCoreTests/ApplycalArgsTests.swift +++ b/Tests/ICCeryCoreTests/ApplycalArgsTests.swift @@ -15,16 +15,16 @@ struct ApplycalArgsTests { #expect(args == ["-v", "-a", "/tmp/cal.cal", "/tmp/profile.icc"]) } - @Test("Unapply is never sent from build") - func unapplyNotEmitted() throws { + @Test("Unapply is emitted when the caller explicitly sets it") + func unapplyEmittedWhenConfigSet() throws { let config = ApplycalConfig( calibrationPath: "/tmp/cal.cal", inputProfileURL: URL(fileURLWithPath: "/tmp/profile.icc"), unapply: true ) let args = try ApplycalArgs.build(config: config) - // Builder intentionally emits -u because config can set it, but - // the UI layer never passes unapply: true in v2.0. + // Builder emits -u only when the caller explicitly sets unapply. + // The UI layer never passes unapply: true in v2.0. #expect(args == ["-v", "-u", "/tmp/cal.cal", "/tmp/profile.icc"]) } } diff --git a/Tests/ICCeryCoreTests/DriftAlertTests.swift b/Tests/ICCeryCoreTests/DriftAlertTests.swift index 51ab7fa..4eb2835 100644 --- a/Tests/ICCeryCoreTests/DriftAlertTests.swift +++ b/Tests/ICCeryCoreTests/DriftAlertTests.swift @@ -38,7 +38,7 @@ struct DriftAlertTests { #expect(DriftAlert.compute(from: [day1, day2]) != nil) } - @Test("Non-poor results do not trigger") + @Test("Non-poor records do not trigger") func nonPoor() { let records = [ record(avg: 1.0, at: 0), @@ -47,6 +47,48 @@ struct DriftAlertTests { #expect(DriftAlert.compute(from: records) == nil) } + @Test("Non-poor records break the consecutive poor run") + func nonPoorBreaksRun() { + let records = [ + record(avg: 4.0, at: 0), // poor + record(avg: 4.5, at: 86400), // poor, far apart + record(avg: 1.0, at: 90000), // good — breaks the run + record(avg: 4.0, at: 92000) // poor, recent but close to previous poor + ] + #expect(DriftAlert.compute(from: records) == nil) + } + + @Test("Only the final consecutive poor run is considered") + func onlySuffixRun() { + let records = [ + record(avg: 4.0, at: 0), // poor + record(avg: 4.5, at: 18000), // poor, > 1h from first + record(avg: 1.0, at: 20000), // good — breaks the run + record(avg: 4.0, at: 25000), // poor + record(avg: 4.5, at: 26000) // poor, < 1h and same day + ] + #expect(DriftAlert.compute(from: records) == nil) + } + + @Test("Final consecutive poor run alerts when far apart") + func suffixRunAlerts() { + let records = [ + record(avg: 1.0, at: 0), // good + record(avg: 4.0, at: 1000), // poor + record(avg: 4.5, at: 4600) // poor, 1h after previous + ] + #expect(DriftAlert.compute(from: records) != nil) + } + + @Test("A single final poor record after good records does not alert") + func singleFinalPoor() { + let records = [ + record(avg: 1.0, at: 0), + record(avg: 4.0, at: 86400) + ] + #expect(DriftAlert.compute(from: records) == nil) + } + private func record(avg: Double, at offset: TimeInterval) -> VerificationRecord { VerificationRecord( id: "vr-\(Int(offset))", diff --git a/Tests/ICCeryCoreTests/ProfileInstallerTests.swift b/Tests/ICCeryCoreTests/ProfileInstallerTests.swift index 640a6c5..7851582 100644 --- a/Tests/ICCeryCoreTests/ProfileInstallerTests.swift +++ b/Tests/ICCeryCoreTests/ProfileInstallerTests.swift @@ -2,42 +2,165 @@ import Foundation import Testing @testable import ICCeryCore +/// A `FileManager` subclass that reports a temporary directory as the +/// user home, so `ProfileInstaller` can be tested without writing to the +/// real `~/Library/ColorSync/Profiles`. +private final class TestFileManager: FileManager { + let tempHome: URL + + init(home: URL) { + self.tempHome = home + super.init() + } + + override var homeDirectoryForCurrentUser: URL { + tempHome + } +} + @Suite("ProfileInstaller") struct ProfileInstallerTests { - @Test("Copies .icc to user ColorSync folder") - func userInstall() throws { + private func makeTempDir() throws -> URL { let fm = FileManager.default let tmp = fm.temporaryDirectory.appendingPathComponent(UUID().uuidString) try fm.createDirectory(at: tmp, withIntermediateDirectories: true) + return tmp + } - let source = tmp.appendingPathComponent("test.icc") - let iccData = Data(repeating: 0, count: 256) - try iccData.write(to: source) + private func makeSource( + at dir: URL, + name: String, + bytes: [UInt8] = Array(repeating: 0, count: 256) + ) throws -> URL { + let url = dir.appendingPathComponent(name) + let data = Data(bytes) + try data.write(to: url) + return url + } - let colorsync = tmp.appendingPathComponent("Library/ColorSync/Profiles") + @Test("Installs .icc to user ColorSync folder") + func userInstall() throws { + let fm = FileManager.default + let tmp = try makeTempDir() + let testFM = TestFileManager(home: tmp) + let source = try makeSource(at: tmp, name: "test.icc") - // Inject a user profile install by replacing the home directory - // is not practical; instead exercise Core validation on a - // temp-only path via the file URL safety checks and the public - // install against a writable system-like path is tested below. + let result = try ProfileInstaller.install( + config: InstallProfileConfig(sourceURL: source), + fileManager: testFM + ) + + #expect(result.registered) + #expect(!result.overwritten) + #expect(!result.renamed) + #expect(result.destPath.hasSuffix("test.icc")) + #expect(fm.fileExists(atPath: result.destPath)) + } + + @Test("Overwrite succeeds and replaces the existing file") + func overwriteSucceeds() throws { + let fm = FileManager.default + let tmp = try makeTempDir() + let testFM = TestFileManager(home: tmp) + let source = try makeSource(at: tmp, name: "m5_profile.icc", bytes: (0..<256).map { UInt8($0) }) + + // First install. + let first = try ProfileInstaller.install( + config: InstallProfileConfig(sourceURL: source), + fileManager: testFM + ) + #expect(!first.overwritten) + + // Change the source contents. + let newBytes: [UInt8] = (0..<256).map { UInt8(($0 + 100) % 256) } + try Data(newBytes).write(to: source) + + let options = InstallProfileOptions( + forceOverwrite: true, + preferSystem: false, + collisionPolicy: .overwrite, + openColorPanel: false + ) + let second = try ProfileInstaller.install( + config: InstallProfileConfig(sourceURL: source, options: options), + fileManager: testFM + ) + + #expect(second.overwritten) + #expect(!second.renamed) + #expect(fm.fileExists(atPath: second.destPath)) + let installed = try Data(contentsOf: URL(fileURLWithPath: second.destPath)) + #expect(Array(installed) == newBytes) + } + + @Test("Preserves .icm source extension") + func preservesIcmExtension() throws { + let tmp = try makeTempDir() + let testFM = TestFileManager(home: tmp) + let source = try makeSource(at: tmp, name: "m5_profile.icm") + + let result = try ProfileInstaller.install( + config: InstallProfileConfig(sourceURL: source), + fileManager: testFM + ) + + #expect(URL(fileURLWithPath: result.destPath).pathExtension == "icm") + #expect(result.destPath.hasSuffix("m5_profile.icm")) + } + + @Test("Rejects parent traversal in source path") + func rejectsParentTraversal() throws { + let fm = FileManager.default + let tmp = try makeTempDir() + + // Create a real file in the parent of `tmp` with a path that contains + // a literal ".." component. + let parent = tmp.deletingLastPathComponent() + let naughtyName = "naughty-\(UUID().uuidString).icc" + let realFile = parent.appendingPathComponent(naughtyName) + _ = try makeSource(at: parent, name: naughtyName) + defer { try? fm.removeItem(at: realFile) } + + let sourceURL = tmp + .appendingPathComponent("..") + .appendingPathComponent(naughtyName) + #expect(fm.fileExists(atPath: sourceURL.path)) - // For this unit test, validate the stem security and source rules. - let unsafe = tmp.appendingPathComponent("bad..stem.icc") - try Data(repeating: 0, count: 256).write(to: unsafe) do { - _ = try ProfileInstaller.install(config: InstallProfileConfig(sourceURL: unsafe)) + _ = try ProfileInstaller.install(config: InstallProfileConfig(sourceURL: sourceURL)) Issue.record("Expected unsafeStem error") } catch let error as ProfileInstallError { if case .unsafeStem = error { } else { Issue.record("Expected unsafeStem, got \(error)") } } catch { Issue.record("Unexpected error type: \(error)") } + } + + @Test("Allows stems with consecutive dots like foo..bar") + func allowsDoubleDotStem() throws { + let fm = FileManager.default + let tmp = try makeTempDir() + let testFM = TestFileManager(home: tmp) + let source = try makeSource(at: tmp, name: "foo..bar.icc") + + let result = try ProfileInstaller.install( + config: InstallProfileConfig(sourceURL: source), + fileManager: testFM + ) + + #expect(result.destPath.hasSuffix("foo..bar.icc")) + #expect(fm.fileExists(atPath: result.destPath)) + } + + @Test("Rejects source files that are too small") + func rejectsSmallSource() throws { + let tmp = try makeTempDir() + let source = tmp.appendingPathComponent("tiny.icc") + try Data(repeating: 0, count: 64).write(to: source) - let small = tmp.appendingPathComponent("tiny.icc") - try Data(repeating: 0, count: 64).write(to: small) do { - _ = try ProfileInstaller.install(config: InstallProfileConfig(sourceURL: small)) + _ = try ProfileInstaller.install(config: InstallProfileConfig(sourceURL: source)) Issue.record("Expected sourceTooSmall error") } catch let error as ProfileInstallError { if case .sourceTooSmall = error { } else { Issue.record("Expected sourceTooSmall, got \(error)") } @@ -45,31 +168,4 @@ struct ProfileInstallerTests { Issue.record("Unexpected error type: \(error)") } } - - @Test("Installs into a temp user folder and preserves source") - func tempInstallPreservesSource() throws { - let fm = FileManager.default - let tmp = fm.temporaryDirectory.appendingPathComponent(UUID().uuidString) - try fm.createDirectory(at: tmp, withIntermediateDirectories: true) - - let source = tmp.appendingPathComponent("m5_profile.icc") - try Data(repeating: 0, count: 256).write(to: source) - - let destDir = tmp.appendingPathComponent("ColorSync/Profiles") - try fm.createDirectory(at: destDir, withIntermediateDirectories: true) - - // There is no public API to override the home directory, so - // test the copy mechanism directly via file operations. - let dest = destDir.appendingPathComponent("m5_profile.icc") - let tmpDest = dest.appendingPathExtension("iccery-install.tmp") - try fm.copyItem(at: source, to: tmpDest) - if fm.fileExists(atPath: dest.path) { - _ = try fm.replaceItemAt(dest, withItemAt: tmpDest) - } else { - try fm.moveItem(at: tmpDest, to: dest) - } - - #expect(fm.fileExists(atPath: source.path)) - #expect(fm.fileExists(atPath: dest.path)) - } } diff --git a/Tests/ICCeryCoreTests/VerificationHistoryStoreTests.swift b/Tests/ICCeryCoreTests/VerificationHistoryStoreTests.swift index c478f5f..91a63fc 100644 --- a/Tests/ICCeryCoreTests/VerificationHistoryStoreTests.swift +++ b/Tests/ICCeryCoreTests/VerificationHistoryStoreTests.swift @@ -51,6 +51,86 @@ struct VerificationHistoryStoreTests { } } + @Test("Append loads existing records first") + func appendLoadsExisting() async throws { + let fm = FileManager.default + let tmp = fm.temporaryDirectory.appendingPathComponent(UUID().uuidString) + try fm.createDirectory(at: tmp, withIntermediateDirectories: true) + let url = tmp.appendingPathComponent("verification_history.json") + + // Pre-populate the store on disk. + let existing = VerificationRecord( + id: "vr-existing", + profileName: "p", + printerName: "", + avgDE: 1.0, + maxDE: 1.0, + rmsDE: 1.0, + patchCount: 1, + status: .good, + timestamp: Date(timeIntervalSince1970: 0) + ) + let store1 = VerificationHistoryStore(url: url) + _ = try await store1.append(existing) + + // A fresh store appending a new record must keep the existing one. + let store2 = VerificationHistoryStore(url: url) + let new = VerificationRecord( + id: "vr-new", + profileName: "p", + printerName: "", + avgDE: 2.0, + maxDE: 2.0, + rmsDE: 2.0, + patchCount: 2, + status: .good, + timestamp: Date(timeIntervalSince1970: 10) + ) + _ = try await store2.append(new) + + let all = await store2.all() + #expect(all.count == 2) + #expect(all.contains { $0.id == "vr-existing" }) + #expect(all.contains { $0.id == "vr-new" }) + } + + @Test("Append does not overwrite an unparseable file") + func appendPreservesUnparseableFile() async { + let fm = FileManager.default + let tmp = fm.temporaryDirectory.appendingPathComponent(UUID().uuidString) + try? fm.createDirectory(at: tmp, withIntermediateDirectories: true) + let url = tmp.appendingPathComponent("verification_history.json") + + let badJSON = "not json" + try? badJSON.write(to: url, atomically: true, encoding: .utf8) + + let store = VerificationHistoryStore(url: url) + let record = VerificationRecord( + id: "vr-new", + profileName: "p", + printerName: "", + avgDE: 1.0, + maxDE: 1.0, + rmsDE: 1.0, + patchCount: 1, + status: .good, + timestamp: Date(timeIntervalSince1970: 0) + ) + + do { + _ = try await store.append(record) + Issue.record("append() should propagate the load error") + } catch { + #expect(fm.fileExists(atPath: url.path)) + if let data = try? Data(contentsOf: url), + let contents = String(data: data, encoding: .utf8) { + #expect(contents == badJSON) + } else { + Issue.record("Could not read preserved file") + } + } + } + @Test("CSV export quoting") func csvQuoting() async throws { let fm = FileManager.default diff --git a/Tests/ICCeryUITests/Fixtures/bin/chartread b/Tests/ICCeryUITests/Fixtures/bin/chartread index c540cd6..8f38d75 100755 --- a/Tests/ICCeryUITests/Fixtures/bin/chartread +++ b/Tests/ICCeryUITests/Fixtures/bin/chartread @@ -39,7 +39,7 @@ def main(): def read_input(): line = read_line() - if not line: + if line == "": sys.exit(1) return line.strip() diff --git a/Tests/ICCeryUITests/Milestone4UITests.swift b/Tests/ICCeryUITests/Milestone4UITests.swift index 2a50bdc..92759aa 100644 --- a/Tests/ICCeryUITests/Milestone4UITests.swift +++ b/Tests/ICCeryUITests/Milestone4UITests.swift @@ -93,7 +93,6 @@ final class Milestone4UITests: XCTestCase { /// End-to-end handheld chartread with the mock fixture produces a /// canonical .ti3 and unlocks Stage 4. func testHandheldFixtureChartreadAndAverage() throws { - try XCTSkipIf(true, "Full interactive chartread UI requires fixture timing tuning; skipped for CI stability. Core chartread/arteffact tests cover the model.") reachStage3() app.buttons["btnDetectInstruments"].click() @@ -113,19 +112,21 @@ final class Milestone4UITests: XCTestCase { app.buttons["btnCalibrate"].click() // Trigger strip A. - _ = waitFor("btnCalibrate", timeout: 20) - app.buttons["btnCalibrate"].click() + _ = waitFor("btnTrigger", timeout: 20) + app.buttons["btnTrigger"].click() // Trigger strip B. - _ = waitFor("btnCalibrate", timeout: 20) - app.buttons["btnCalibrate"].click() + _ = waitFor("btnTrigger", timeout: 20) + app.buttons["btnTrigger"].click() // All strips read → Done & Save appears. _ = waitFor("btnDoneRead", timeout: 20) app.buttons["btnDoneRead"].firstMatch.click() // Averaging panel appears with one pass snapshot. + _ = waitFor("chartreadAveragingPanel", timeout: 20) _ = waitFor("passCounterBadge", timeout: 20) + XCTAssertTrue(app.buttons["btnFinishAndAverage"].waitForExistence(timeout: 5)) XCTAssertTrue(app.buttons["btnFinishAndAverage"].isEnabled) app.buttons["btnFinishAndAverage"].click()