From 103b6b256b977c7c58ceb6a2669fec0f0e45ff60 Mon Sep 17 00:00:00 2001 From: Johnny IV Young Ospino Date: Fri, 9 Oct 2026 14:35:11 -0500 Subject: [PATCH 1/3] fix(export): preserve concurrent outputs during publication --- .../NonReplacingFilePublisherTests.swift | 30 +++++++ Tests/CLI/CLINoClobberPublicationTests.swift | 88 +++++++++++++++++++ Vitrine/CLI/CLIBatchRenderer.swift | 18 ++-- Vitrine/CLI/CLIOutputWriter.swift | 24 ++--- Vitrine/Export/BatchOutputFiles.swift | 78 +--------------- Vitrine/Export/CarouselPaginator.swift | 2 +- .../Export/NonReplacingFilePublisher.swift | 84 ++++++++++++++++++ docs/AGENT-COOKBOOK.md | 10 +++ docs/ARCHITECTURE.md | 10 ++- 9 files changed, 251 insertions(+), 93 deletions(-) create mode 100644 RenderingTests/NonReplacingFilePublisherTests.swift create mode 100644 Tests/CLI/CLINoClobberPublicationTests.swift create mode 100644 VitrineRendering/Export/NonReplacingFilePublisher.swift diff --git a/RenderingTests/NonReplacingFilePublisherTests.swift b/RenderingTests/NonReplacingFilePublisherTests.swift new file mode 100644 index 00000000..45691f0e --- /dev/null +++ b/RenderingTests/NonReplacingFilePublisherTests.swift @@ -0,0 +1,30 @@ +import Foundation +import Testing + +@testable import VitrineRendering + +@Suite("Shared publication failure ownership") +struct NonReplacingFilePublisherTests { + enum Failure: Error, Equatable { case write } + + @Test func fallbackFailureDoesNotDeleteAReplacementOwnedByAnotherProducer() throws { + let directory = FileManager.default.temporaryDirectory + .appendingPathComponent("publication-failure-\(UUID().uuidString)") + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: directory) } + let output = directory.appendingPathComponent("output.png") + let winner = Data("replacement owned by another producer".utf8) + + #expect(throws: Failure.write) { + try NonReplacingFilePublisher.publishByExclusiveCreate(Data("new".utf8), to: output) { + handle, bytes in + try handle.write(contentsOf: bytes.prefix(1)) + try FileManager.default.removeItem(at: output) + try winner.write(to: output) + throw Failure.write + } + } + + #expect(try Data(contentsOf: output) == winner) + } +} diff --git a/Tests/CLI/CLINoClobberPublicationTests.swift b/Tests/CLI/CLINoClobberPublicationTests.swift new file mode 100644 index 00000000..7d0e03be --- /dev/null +++ b/Tests/CLI/CLINoClobberPublicationTests.swift @@ -0,0 +1,88 @@ +import Foundation +import Testing +import VitrineDomain +import VitrineRendering + +@testable import VitrineCLICore + +@Suite("CLI no-clobber applies at publication") +struct CLINoClobberPublicationTests: CLITestSupport { + @Test(arguments: ["png", "txt", "md", "html"]) + func postPreflightWinnersSurviveRealRendering(extensionName: String) throws { + let directory = try makeTempDirectory() + defer { try? FileManager.default.removeItem(at: directory) } + let output = directory.appendingPathComponent("card.png") + let winner = directory.appendingPathComponent("card.\(extensionName)") + let original = Data("another producer's completed artifact".utf8) + let options = try CLIArguments.parse([ + "render", "snippet.swift", "--out", output.path, + "--no-overwrite", "--sidecars", "all", + ]) + try CLIOutputWriter.guardNoOverwriteTargetsAvailable(beside: output, options: options) + // The other producer commits after preflight, before our real render/write path. + try original.write(to: winner) + var config = SnapshotConfig() + config.code = "let value = 42" + config.language = .swift + + #expect(throws: CLIError.writeFailed(path: winner.path)) { + _ = try CLIOutputWriter.renderAndWriteArtifact(config, options: options, to: output) + } + #expect(try Data(contentsOf: winner) == original) + if extensionName != "png" { + #expect(try decodePNG(at: output.path).width > 0) + } + } + + @Test(arguments: ["--manifest", "--skipped-report"]) + func batchReportsPreserveWinnersCreatedAfterRendering(flag: String) throws { + let root = try makeTempDirectory() + defer { try? FileManager.default.removeItem(at: root) } + let input = root.appendingPathComponent("input", isDirectory: true) + let output = root.appendingPathComponent("output", isDirectory: true) + let report = root.appendingPathComponent("report.json") + try FileManager.default.createDirectory(at: input, withIntermediateDirectories: true) + _ = try writeInput("let value = 42", named: "Card.swift", in: input) + let options = try CLIArguments.parse([ + "batch", input.path, "--out", output.path, "--no-overwrite", flag, report.path, + ]) + let original = Data("another producer's report".utf8) + var reachedPublication = false + + #expect(throws: CLIError.writeFailed(path: report.path)) { + try CLIBatchRenderer.run(options, beforeReports: { + reachedPublication = true + try original.write(to: report) + }) + } + + #expect(reachedPublication) + #expect(try Data(contentsOf: report) == original) + #expect(try decodePNG(at: output.appendingPathComponent("Card.png").path).width > 0) + } + + @Test func lateDanglingSymlinkIsPreservedAndNeverFollowed() throws { + let directory = try makeTempDirectory() + defer { try? FileManager.default.removeItem(at: directory) } + let output = directory.appendingPathComponent("manifest.json") + let target = directory.appendingPathComponent("other.json") + try FileManager.default.createSymbolicLink(at: output, withDestinationURL: target) + + #expect(throws: CLIError.writeFailed(path: output.path)) { + try CLIOutputWriter.write(Data("new".utf8), to: output, noOverwrite: true) + } + #expect(try FileManager.default.destinationOfSymbolicLink(atPath: output.path) == target.path) + #expect(!FileManager.default.fileExists(atPath: target.path)) + } + + @Test func defaultOverwriteModeStillReplacesAnExistingOutput() throws { + let directory = try makeTempDirectory() + defer { try? FileManager.default.removeItem(at: directory) } + let output = directory.appendingPathComponent("report.json") + try Data("old".utf8).write(to: output) + + try CLIOutputWriter.write(Data("new".utf8), to: output, noOverwrite: false) + + #expect(try Data(contentsOf: output) == Data("new".utf8)) + } +} diff --git a/Vitrine/CLI/CLIBatchRenderer.swift b/Vitrine/CLI/CLIBatchRenderer.swift index 40fcd110..1b1bfb90 100644 --- a/Vitrine/CLI/CLIBatchRenderer.swift +++ b/Vitrine/CLI/CLIBatchRenderer.swift @@ -60,7 +60,8 @@ enum CLIBatchRenderer { try FileManager.default.contentsOfDirectory( at: $0, includingPropertiesForKeys: [.isRegularFileKey], options: [.skipsHiddenFiles]) - } + }, + beforeReports: (() throws -> Void)? = nil ) throws -> String { let background = try CLIRenderResources.prepareBackground(options) defer { background.removeTemporaryFiles() } @@ -183,8 +184,11 @@ enum CLIBatchRenderer { ) let summary = "\(action) \(rendered) image\(rendered == 1 ? "" : "s") to \(outputDirectory.path)" - try writeSkippedReport(skippedReport, path: options.skippedReportPath) - try writeBatchManifest(manifest, path: options.batchManifestPath) + try beforeReports?() + try writeSkippedReport( + skippedReport, path: options.skippedReportPath, noOverwrite: options.noOverwrite) + try writeBatchManifest( + manifest, path: options.batchManifestPath, noOverwrite: options.noOverwrite) if rendered == 0, options.failOnEmpty { throw CLIError.batchEmpty(skipped: skipped) } @@ -559,7 +563,7 @@ enum CLIBatchRenderer { /// useful CI artifact (`[]`) because it proves the batch scanned without omissions. private static func writeSkippedReport( _ skippedReport: [SkippedReportEntry], - path: String? + path: String?, noOverwrite: Bool ) throws { guard let path, !path.isEmpty else { return } let url = URL(fileURLWithPath: path) @@ -576,7 +580,7 @@ enum CLIBatchRenderer { encoded.append(0x0A) data = encoded } - try data.write(to: url, options: .atomic) + try CLIOutputWriter.write(data, to: url, noOverwrite: noOverwrite) } catch { throw CLIError.writeFailed(path: path) } @@ -586,7 +590,7 @@ enum CLIBatchRenderer { /// for CI because it proves discovery completed even when no inputs matched. private static func writeBatchManifest( _ manifest: [BatchManifestEntry], - path: String? + path: String?, noOverwrite: Bool ) throws { guard let path, !path.isEmpty else { return } let url = URL(fileURLWithPath: path) @@ -603,7 +607,7 @@ enum CLIBatchRenderer { encoded.append(0x0A) data = encoded } - try data.write(to: url, options: .atomic) + try CLIOutputWriter.write(data, to: url, noOverwrite: noOverwrite) } catch { throw CLIError.writeFailed(path: path) } diff --git a/Vitrine/CLI/CLIOutputWriter.swift b/Vitrine/CLI/CLIOutputWriter.swift index bbb80bb8..7110183d 100644 --- a/Vitrine/CLI/CLIOutputWriter.swift +++ b/Vitrine/CLI/CLIOutputWriter.swift @@ -171,7 +171,7 @@ enum CLIOutputWriter { } catch let error { throw CLIError.renderFailure(error) } - try write(payload.data, to: url) + try write(payload.data, to: url, noOverwrite: options.noOverwrite) if options.textSidecar { try writeTextSidecar(for: config, options: options, beside: url) } if options.markdownSidecar { try writeMarkdownSidecar(for: config, options: options, beside: url) @@ -211,7 +211,7 @@ enum CLIOutputWriter { ) throws { let sidecarURL = imageURL.deletingPathExtension().appendingPathExtension("txt") do { - try Data(config.sidecarText.utf8).write(to: sidecarURL, options: .atomic) + try write(Data(config.sidecarText.utf8), to: sidecarURL, noOverwrite: options.noOverwrite) } catch { let nsError = error as NSError Log.export.error( @@ -233,7 +233,7 @@ enum CLIOutputWriter { let contents = markdownSidecarContents( for: config, imageName: imageURL.lastPathComponent) do { - try Data(contents.utf8).write(to: sidecarURL, options: .atomic) + try write(Data(contents.utf8), to: sidecarURL, noOverwrite: options.noOverwrite) } catch { let nsError = error as NSError Log.export.error( @@ -253,7 +253,7 @@ enum CLIOutputWriter { let sidecarURL = imageURL.deletingPathExtension().appendingPathExtension("html") let contents = htmlSidecarContents(for: config, imageName: imageURL.lastPathComponent) do { - try Data(contents.utf8).write(to: sidecarURL, options: .atomic) + try write(Data(contents.utf8), to: sidecarURL, noOverwrite: options.noOverwrite) } catch { let nsError = error as NSError Log.export.error( @@ -284,13 +284,17 @@ enum CLIOutputWriter { /// Writes `data` to `url`, mapping any I/O failure to `CLIError.writeFailed`. /// - /// Atomic, like every other write the batch renderer and the app's exchange files - /// perform: an interrupted run (a full disk, or Ctrl-C between two `write` calls) - /// must not leave a truncated image or sidecar that looks complete to whatever - /// script consumes it. - private static func write(_ data: Data, to url: URL) throws { + /// No-clobber is enforced at publication, including a destination created after + /// preflight. Each file commits independently; a later sidecar failure preserves + /// earlier completed outputs and reports failure. The shared fallback on volumes + /// without exclusive rename can leave a partial file on I/O failure. + static func write(_ data: Data, to url: URL, noOverwrite: Bool) throws { do { - try data.write(to: url, options: .atomic) + if noOverwrite { + try NonReplacingFilePublisher.publish(data, to: url) + } else { + try data.write(to: url, options: .atomic) + } } catch { // Log only the format, never the (user-chosen) path (privacy policy). let nsError = error as NSError diff --git a/Vitrine/Export/BatchOutputFiles.swift b/Vitrine/Export/BatchOutputFiles.swift index 83d791bc..de291dd0 100644 --- a/Vitrine/Export/BatchOutputFiles.swift +++ b/Vitrine/Export/BatchOutputFiles.swift @@ -1,5 +1,5 @@ -import Darwin import Foundation +import VitrineRendering /// One non-replacing plan for every image and optional sidecar in a GUI batch. nonisolated enum BatchOutputFiles { @@ -38,86 +38,16 @@ nonisolated enum BatchOutputFiles { } } - /// Complete bytes are staged in an owned private sibling directory, then committed - /// with one exclusive rename. A race-created output is preserved, including symlinks. - /// Cleanup removes only the private stage; a previously published image is never undone. + /// Publishes through the same exclusive commit used by CLI no-clobber output. static func publish( _ data: Data, to destination: URL, beforeCommit: (() throws -> Void)? = nil ) throws { - try Task.checkCancellation() - // Stage and commit through the same file-system representation, so the private - // directory and the later rename name identical bytes on every volume. - var template: [CChar] = destination.deletingLastPathComponent() - .appendingPathComponent(".vitrine-export-XXXXXX") - .withUnsafeFileSystemRepresentation { path in - path.map { Array(UnsafeBufferPointer(start: $0, count: strlen($0) + 1)) } ?? [] - } - guard !template.isEmpty else { throw POSIXError(.EINVAL) } - guard mkdtemp(&template) != nil else { - throw POSIXError(POSIXErrorCode(rawValue: errno) ?? .EIO) - } - let stage = URL( - fileURLWithFileSystemRepresentation: template, isDirectory: true, relativeTo: nil) - defer { try? FileManager.default.removeItem(at: stage) } - let payload = stage.appendingPathComponent("payload") - try data.write(to: payload) - try beforeCommit?() - try Task.checkCancellation() - let status: Int32 = payload.withUnsafeFileSystemRepresentation { source in - destination.withUnsafeFileSystemRepresentation { target in - guard let source, let target else { return EINVAL } - return renamex_np(source, target, UInt32(RENAME_EXCL)) == 0 ? 0 : errno - } - } - switch status { - case 0: - return - case ENOTSUP, EOPNOTSUPP: - // Some network and FAT-family volumes refuse `RENAME_EXCL`. Fall back to an - // exclusive create, which still never replaces or follows an existing name. - try publishByExclusiveCreate(data, to: destination) - default: - throw POSIXError(POSIXErrorCode(rawValue: status) ?? .EIO) - } + try NonReplacingFilePublisher.publish(data, to: destination, beforeCommit: beforeCommit) } - /// Non-replacing publication for volumes without exclusive rename: `O_EXCL` refuses an - /// existing name (including a symlink, via `O_NOFOLLOW`), and a failed write removes - /// only the file this call created. Unlike the rename path, the file is visible while - /// it is written, so it is used only when the volume cannot rename exclusively. static func publishByExclusiveCreate(_ data: Data, to destination: URL) throws { - let descriptor: Int32 = destination.withUnsafeFileSystemRepresentation { path in - guard let path else { return -1 } - let flags = O_WRONLY | O_CREAT | O_EXCL | O_NOFOLLOW | O_CLOEXEC - // 0o666 minus the umask matches `Data.write`, so both paths produce equal modes. - return Darwin.open(path, flags, 0o666) - } - guard descriptor >= 0 else { - throw POSIXError(POSIXErrorCode(rawValue: errno) ?? .EIO) - } - let handle = FileHandle(fileDescriptor: descriptor, closeOnDealloc: true) - do { - try handle.write(contentsOf: data) - } catch { - try? handle.close() - removeCreatedFile(at: destination) - throw error - } - do { - // A failed close has already released the descriptor; never close it twice, - // because a concurrent export may have reused that descriptor number. - try handle.close() - } catch { - removeCreatedFile(at: destination) - throw error - } - } - - private static func removeCreatedFile(at destination: URL) { - _ = destination.withUnsafeFileSystemRepresentation { path in - path.map { Darwin.unlink($0) } - } + try NonReplacingFilePublisher.publishByExclusiveCreate(data, to: destination) } private static func key(_ filename: String) -> String { diff --git a/Vitrine/Export/CarouselPaginator.swift b/Vitrine/Export/CarouselPaginator.swift index b022afc8..9c8fbff1 100644 --- a/Vitrine/Export/CarouselPaginator.swift +++ b/Vitrine/Export/CarouselPaginator.swift @@ -15,7 +15,7 @@ enum CarouselPaginator { /// Splits `code` into slides of at most `maxLinesPerSlide` consecutive lines, /// **balanced**: the slide count is fixed by the cap, then lines are distributed /// evenly so pages differ by at most one line (a 25-line snippet at 12 becomes - /// 13 + 12, never 12 + 12 + 1). Empty input yields no slides. + /// 9 + 8 + 8, never 12 + 12 + 1). Empty input yields no slides. static func pages(for code: String, maxLinesPerSlide: Int) -> [String] { slides(for: code, maxLinesPerSlide: maxLinesPerSlide).map(\.text) } diff --git a/VitrineRendering/Export/NonReplacingFilePublisher.swift b/VitrineRendering/Export/NonReplacingFilePublisher.swift new file mode 100644 index 00000000..7c988cbc --- /dev/null +++ b/VitrineRendering/Export/NonReplacingFilePublisher.swift @@ -0,0 +1,84 @@ +import Darwin +import Foundation + +/// Shared app/CLI output publication; rendering and filename planning stay with callers. +nonisolated public enum NonReplacingFilePublisher { + /// Complete bytes are staged in an owned private sibling directory, then committed + /// with one exclusive rename. A race-created output is preserved, including symlinks. + /// Cleanup removes only the private stage; a previously published image is never undone. + public static func publish( + _ data: Data, to destination: URL, + beforeCommit: (() throws -> Void)? = nil + ) throws { + try Task.checkCancellation() + // Stage and commit through the same file-system representation, so the private + // directory and the later rename name identical bytes on every volume. + var template: [CChar] = destination.deletingLastPathComponent() + .appendingPathComponent(".vitrine-export-XXXXXX") + .withUnsafeFileSystemRepresentation { path in + path.map { Array(UnsafeBufferPointer(start: $0, count: strlen($0) + 1)) } ?? [] + } + guard !template.isEmpty else { throw POSIXError(.EINVAL) } + guard mkdtemp(&template) != nil else { + throw POSIXError(POSIXErrorCode(rawValue: errno) ?? .EIO) + } + let stage = URL( + fileURLWithFileSystemRepresentation: template, isDirectory: true, relativeTo: nil) + defer { try? FileManager.default.removeItem(at: stage) } + let payload = stage.appendingPathComponent("payload") + try data.write(to: payload) + try beforeCommit?() + try Task.checkCancellation() + let status: Int32 = payload.withUnsafeFileSystemRepresentation { source in + destination.withUnsafeFileSystemRepresentation { target in + guard let source, let target else { return EINVAL } + return renamex_np(source, target, UInt32(RENAME_EXCL)) == 0 ? 0 : errno + } + } + switch status { + case 0: + return + case ENOTSUP, EOPNOTSUPP: + // Some network and FAT-family volumes refuse `RENAME_EXCL`. Fall back to an + // exclusive create, which still never replaces or follows an existing name. + try publishByExclusiveCreate(data, to: destination) + default: + throw POSIXError(POSIXErrorCode(rawValue: status) ?? .EIO) + } + } + + /// Non-replacing publication for volumes without exclusive rename: `O_EXCL` refuses an + /// existing name (including a symlink, via `O_NOFOLLOW`). An I/O failure is surfaced; + /// a partial file may remain because cleanup must not unlink a public name another + /// process can replace. Unlike the rename path, bytes are visible while being written. + public static func publishByExclusiveCreate(_ data: Data, to destination: URL) throws { + try publishByExclusiveCreate(data, to: destination) { handle, bytes in + try handle.write(contentsOf: bytes) + } + } + + /// A narrow fault seam for real-file error-path tests; the default writer stays public. + static func publishByExclusiveCreate( + _ data: Data, to destination: URL, writeContents: (FileHandle, Data) throws -> Void + ) throws { + let descriptor: Int32 = destination.withUnsafeFileSystemRepresentation { path in + guard let path else { return -1 } + let flags = O_WRONLY | O_CREAT | O_EXCL | O_NOFOLLOW | O_CLOEXEC + // 0o666 minus the umask matches `Data.write`, so both paths produce equal modes. + return Darwin.open(path, flags, 0o666) + } + guard descriptor >= 0 else { + throw POSIXError(POSIXErrorCode(rawValue: errno) ?? .EIO) + } + let handle = FileHandle(fileDescriptor: descriptor, closeOnDealloc: true) + do { + try writeContents(handle, data) + } catch { + try? handle.close() + throw error + } + // A failed close has already released the descriptor; never close it twice, + // because a concurrent export may have reused that descriptor number. + try handle.close() + } +} diff --git a/docs/AGENT-COOKBOOK.md b/docs/AGENT-COOKBOOK.md index c509ada4..e124eb6d 100644 --- a/docs/AGENT-COOKBOOK.md +++ b/docs/AGENT-COOKBOOK.md @@ -142,3 +142,13 @@ Tests execute these parser/renderer/authorization contracts with temporary files synthetic content, and an ephemeral signed-token verifier; no real activation or production credential is required. Wrapper tests simulate failures and incomplete responses. Neither proves external-provider privacy, real demand, or human acceptance. + +## Output collisions and partial runs + +The source implementation enforces `--no-overwrite` again when each image, sidecar, +manifest, or skipped report is published, including destinations created after preflight. +Files commit independently; a late collision or I/O failure returns failure and keeps +earlier complete outputs. Do not treat a failed command as a complete output bundle. +On filesystems without exclusive rename, the exclusive-create fallback can leave a +partial file after an I/O failure. Inspect failed output directories before retrying. +These source changes require a new qualified release before the published binary has them. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 1ffd95ed..6b1dd550 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -1227,7 +1227,15 @@ before dismissing, so an inconsistent writer result cannot silently claim succes Preset and carousel exports plan all image/sidecar names together before rendering, allocating numeric suffixes around occupied names. Complete files are staged privately and published with exclusive rename, preserving destinations created after planning. -Volumes that refuse exclusive rename fall back to an exclusive, non-following create. +`NonReplacingFilePublisher` in `VitrineRendering` owns the shared filesystem commit; +GUI batch planning stays in the app, while CLI `--no-overwrite` applies it to images, +sidecars, manifests, and skipped reports. Preflight remains useful for early diagnostics, +but publication enforces non-replacement when a destination appears during rendering. +Outputs commit independently: a late failure retains earlier completed files and returns +failure, without claiming whole-batch rollback. Volumes that refuse exclusive rename +fall back to an exclusive, non-following create. An I/O failure in that fallback can +leave a partial file. It is deliberately retained rather than unlinking a public name +that another process might have replaced. A requested nonempty sidecar failure leaves its completed image in place and reports an incomplete export. Cancellation stops further admission and publication; already committed files remain available. The From c9050c0eace8e27e4b496d8ed10ca3d52b985927 Mon Sep 17 00:00:00 2001 From: Johnny IV Young Ospino Date: Fri, 9 Oct 2026 14:41:05 -0500 Subject: [PATCH 2/3] fix(tests): import image geometry and format publication checks --- Tests/CLI/CLINoClobberPublicationTests.swift | 14 +++++++++----- Vitrine/CLI/CLIOutputWriter.swift | 3 ++- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/Tests/CLI/CLINoClobberPublicationTests.swift b/Tests/CLI/CLINoClobberPublicationTests.swift index 7d0e03be..63cfc50e 100644 --- a/Tests/CLI/CLINoClobberPublicationTests.swift +++ b/Tests/CLI/CLINoClobberPublicationTests.swift @@ -1,3 +1,4 @@ +import CoreGraphics import Foundation import Testing import VitrineDomain @@ -50,10 +51,12 @@ struct CLINoClobberPublicationTests: CLITestSupport { var reachedPublication = false #expect(throws: CLIError.writeFailed(path: report.path)) { - try CLIBatchRenderer.run(options, beforeReports: { - reachedPublication = true - try original.write(to: report) - }) + try CLIBatchRenderer.run( + options, + beforeReports: { + reachedPublication = true + try original.write(to: report) + }) } #expect(reachedPublication) @@ -71,7 +74,8 @@ struct CLINoClobberPublicationTests: CLITestSupport { #expect(throws: CLIError.writeFailed(path: output.path)) { try CLIOutputWriter.write(Data("new".utf8), to: output, noOverwrite: true) } - #expect(try FileManager.default.destinationOfSymbolicLink(atPath: output.path) == target.path) + #expect( + try FileManager.default.destinationOfSymbolicLink(atPath: output.path) == target.path) #expect(!FileManager.default.fileExists(atPath: target.path)) } diff --git a/Vitrine/CLI/CLIOutputWriter.swift b/Vitrine/CLI/CLIOutputWriter.swift index 7110183d..cb2543a2 100644 --- a/Vitrine/CLI/CLIOutputWriter.swift +++ b/Vitrine/CLI/CLIOutputWriter.swift @@ -211,7 +211,8 @@ enum CLIOutputWriter { ) throws { let sidecarURL = imageURL.deletingPathExtension().appendingPathExtension("txt") do { - try write(Data(config.sidecarText.utf8), to: sidecarURL, noOverwrite: options.noOverwrite) + try write( + Data(config.sidecarText.utf8), to: sidecarURL, noOverwrite: options.noOverwrite) } catch { let nsError = error as NSError Log.export.error( From 8084a05f0c25f965fbecf188ca1dc0b581bda715 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 20:33:04 +0000 Subject: [PATCH 3/3] fix(cli): report dangling no-overwrite targets during preflight - Check no-overwrite image, sidecar, and batch report targets with lstat so a dangling symlink fails preflight as an existing output instead of a late write failure - Drop the duplicate sidecar error logging now that the shared writer logs and maps failures - Add regression tests and a changelog entry for publication-time no-clobber --- CHANGELOG.md | 2 + Tests/CLI/CLINoClobberPublicationTests.swift | 39 +++++++++++++++++ Vitrine/CLI/CLIBatchRenderer.swift | 2 +- Vitrine/CLI/CLIOutputWriter.swift | 46 +++++++------------- 4 files changed, 57 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 87e74c14..3597906a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,8 @@ can never drift. ### Fixed +- CLI `--no-overwrite` also refuses images, sidecars, manifests, and skipped reports that + another process creates while a run is rendering, instead of replacing them at write time. - Preserve existing files during multi-size and carousel exports by choosing available names and refusing race-created destinations. - Live-file Reload catches a save that lands while the file is being read, without losing a later local draft. - Reject observed file changes during bounded reads, including same-length saves, instead of accepting mixed text. diff --git a/Tests/CLI/CLINoClobberPublicationTests.swift b/Tests/CLI/CLINoClobberPublicationTests.swift index 63cfc50e..d34decf3 100644 --- a/Tests/CLI/CLINoClobberPublicationTests.swift +++ b/Tests/CLI/CLINoClobberPublicationTests.swift @@ -79,6 +79,45 @@ struct CLINoClobberPublicationTests: CLITestSupport { #expect(!FileManager.default.fileExists(atPath: target.path)) } + @Test(arguments: ["png", "txt"]) + func preflightReportsADanglingSymlinkBeforeRendering(extensionName: String) throws { + let directory = try makeTempDirectory() + defer { try? FileManager.default.removeItem(at: directory) } + let output = directory.appendingPathComponent("card.png") + let occupied = directory.appendingPathComponent("card.\(extensionName)") + try FileManager.default.createSymbolicLink( + at: occupied, withDestinationURL: directory.appendingPathComponent("missing")) + let options = try CLIArguments.parse([ + "render", "snippet.swift", "--out", output.path, "--no-overwrite", "--sidecars", + "text", + ]) + + #expect(throws: CLIError.outputExists(path: occupied.path)) { + try CLIOutputWriter.guardNoOverwriteTargetsAvailable(beside: output, options: options) + } + } + + @Test(arguments: ["--manifest", "--skipped-report"]) + func batchPreflightReportsADanglingSymlinkReport(flag: String) throws { + let root = try makeTempDirectory() + defer { try? FileManager.default.removeItem(at: root) } + let input = root.appendingPathComponent("input", isDirectory: true) + let output = root.appendingPathComponent("output", isDirectory: true) + let report = root.appendingPathComponent("report.json") + try FileManager.default.createDirectory(at: input, withIntermediateDirectories: true) + _ = try writeInput("let value = 42", named: "Card.swift", in: input) + try FileManager.default.createSymbolicLink( + at: report, withDestinationURL: root.appendingPathComponent("missing.json")) + let options = try CLIArguments.parse([ + "batch", input.path, "--out", output.path, "--no-overwrite", flag, report.path, + ]) + + #expect(throws: CLIError.outputExists(path: report.path)) { + try CLIBatchRenderer.run(options) + } + #expect(!FileManager.default.fileExists(atPath: output.path)) + } + @Test func defaultOverwriteModeStillReplacesAnExistingOutput() throws { let directory = try makeTempDirectory() defer { try? FileManager.default.removeItem(at: directory) } diff --git a/Vitrine/CLI/CLIBatchRenderer.swift b/Vitrine/CLI/CLIBatchRenderer.swift index 1b1bfb90..9f4dc002 100644 --- a/Vitrine/CLI/CLIBatchRenderer.swift +++ b/Vitrine/CLI/CLIBatchRenderer.swift @@ -263,7 +263,7 @@ enum CLIBatchRenderer { "The batch report at \"\(report.path)\" conflicts with an input resource " + "or another output. Choose distinct paths for resources and outputs.") } - if options.noOverwrite, FileManager.default.fileExists(atPath: report.path) { + if options.noOverwrite, CLIOutputWriter.entryExists(at: report) { throw CLIError.outputExists(path: report.path) } try guardExistingInputReport( diff --git a/Vitrine/CLI/CLIOutputWriter.swift b/Vitrine/CLI/CLIOutputWriter.swift index cb2543a2..29cd6591 100644 --- a/Vitrine/CLI/CLIOutputWriter.swift +++ b/Vitrine/CLI/CLIOutputWriter.swift @@ -103,9 +103,14 @@ enum CLIOutputWriter { /// Finds the first existing output target when `--no-overwrite` is active. static func existingNoOverwriteTarget(beside imageURL: URL, options: CLIOptions) -> URL? { guard options.noOverwrite else { return nil } - return outputTargets(beside: imageURL, options: options).first { - FileManager.default.fileExists(atPath: $0.path) - } + return outputTargets(beside: imageURL, options: options).first(where: entryExists(at:)) + } + + /// Whether any directory entry occupies `url`, without following a final symlink. + /// Publication refuses a dangling symlink too, so preflight must report it before + /// rendering rather than letting the run fail late with a generic write error. + static func entryExists(at url: URL) -> Bool { + (try? FileManager.default.attributesOfItem(atPath: url.path)) != nil } /// Enforces `--no-overwrite` before rendering/copying so a run fails without @@ -210,16 +215,9 @@ enum CLIOutputWriter { for config: SnapshotConfig, options: CLIOptions, beside imageURL: URL ) throws { let sidecarURL = imageURL.deletingPathExtension().appendingPathExtension("txt") - do { - try write( - Data(config.sidecarText.utf8), to: sidecarURL, noOverwrite: options.noOverwrite) - } catch { - let nsError = error as NSError - Log.export.error( - "CLI text-sidecar write failed (\(nsError.domain, privacy: .public) \(nsError.code, privacy: .public))" - ) - throw CLIError.writeFailed(path: sidecarURL.path) - } + // `write` already logs the underlying error and maps it to `writeFailed`. + try write( + Data(config.sidecarText.utf8), to: sidecarURL, noOverwrite: options.noOverwrite) } /// Writes the Markdown sidecar next to the rendered image at `imageURL` @@ -233,15 +231,8 @@ enum CLIOutputWriter { let sidecarURL = imageURL.deletingPathExtension().appendingPathExtension("md") let contents = markdownSidecarContents( for: config, imageName: imageURL.lastPathComponent) - do { - try write(Data(contents.utf8), to: sidecarURL, noOverwrite: options.noOverwrite) - } catch { - let nsError = error as NSError - Log.export.error( - "CLI markdown-sidecar write failed (\(nsError.domain, privacy: .public) \(nsError.code, privacy: .public))" - ) - throw CLIError.writeFailed(path: sidecarURL.path) - } + // `write` already logs the underlying error and maps it to `writeFailed`. + try write(Data(contents.utf8), to: sidecarURL, noOverwrite: options.noOverwrite) } /// Writes the HTML sidecar next to the rendered image at `imageURL` @@ -253,15 +244,8 @@ enum CLIOutputWriter { ) throws { let sidecarURL = imageURL.deletingPathExtension().appendingPathExtension("html") let contents = htmlSidecarContents(for: config, imageName: imageURL.lastPathComponent) - do { - try write(Data(contents.utf8), to: sidecarURL, noOverwrite: options.noOverwrite) - } catch { - let nsError = error as NSError - Log.export.error( - "CLI html-sidecar write failed (\(nsError.domain, privacy: .public) \(nsError.code, privacy: .public))" - ) - throw CLIError.writeFailed(path: sidecarURL.path) - } + // `write` already logs the underlying error and maps it to `writeFailed`. + try write(Data(contents.utf8), to: sidecarURL, noOverwrite: options.noOverwrite) } /// Builds the Markdown sidecar body: `![alt](image)` + a fenced code block.