Skip to content

Commit 299548f

Browse files
alexey1312claude
andcommitted
fix(cli): address PR #66 review issues for export report
Fix TOCTOU race in capturePreState, extract duplicated determineAction logic, add diagnostics for nil checksums and decode failures, create parent directories before report write, improve error descriptions for non-LocalizedError types, and add ReportStats factory methods. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 27f7067 commit 299548f

12 files changed

Lines changed: 308 additions & 44 deletions

Sources/ExFigCLI/Report/AssetManifest.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ struct ManifestEntry: Encodable {
1414
/// What happened to this file during export.
1515
let action: FileAction
1616

17-
/// FNV-1a 64-bit content checksum (16-char hex), `nil` for deleted files.
17+
/// FNV-1a 64-bit content checksum (16-char hex), `nil` for deleted files or unreadable content.
1818
let checksum: String?
1919

2020
/// Type of asset (e.g., "color", "icon", "image", "typography").

Sources/ExFigCLI/Report/ExportReport.swift

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,32 @@ struct ExportReport: Encodable {
4242
/// Current report schema version.
4343
static let currentVersion = 1
4444

45+
init(
46+
version: Int = ExportReport.currentVersion,
47+
command: String,
48+
config: String,
49+
startTime: String,
50+
endTime: String,
51+
duration: TimeInterval,
52+
success: Bool,
53+
error: String?,
54+
stats: ReportStats,
55+
warnings: [String],
56+
manifest: AssetManifest?
57+
) {
58+
self.version = version
59+
self.command = command
60+
self.config = config
61+
self.startTime = startTime
62+
self.endTime = endTime
63+
self.duration = duration
64+
self.success = success
65+
self.error = error
66+
self.stats = stats
67+
self.warnings = warnings
68+
self.manifest = manifest
69+
}
70+
4571
/// Serializes the report to pretty-printed JSON with sorted keys.
4672
func jsonData() throws -> Data {
4773
try JSONCodec.encodePrettySorted(self)
@@ -59,4 +85,20 @@ struct ReportStats: Encodable {
5985
let typography: Int
6086

6187
static let zero = ReportStats(colors: 0, icons: 0, images: 0, typography: 0)
88+
89+
static func colors(_ count: Int) -> ReportStats {
90+
.init(colors: count, icons: 0, images: 0, typography: 0)
91+
}
92+
93+
static func icons(_ count: Int) -> ReportStats {
94+
.init(colors: 0, icons: count, images: 0, typography: 0)
95+
}
96+
97+
static func images(_ count: Int) -> ReportStats {
98+
.init(colors: 0, icons: 0, images: count, typography: 0)
99+
}
100+
101+
static func typography(_ count: Int) -> ReportStats {
102+
.init(colors: 0, icons: 0, images: 0, typography: count)
103+
}
62104
}

Sources/ExFigCLI/Report/ExportReportHelper.swift

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ func withExportReport(
6060
endTime: formatter.string(from: endTime),
6161
duration: endTime.timeIntervalSince(startTime),
6262
success: exportError == nil,
63-
error: exportError?.localizedDescription,
63+
error: exportError.map { describeExportError($0) },
6464
stats: buildStats(exportCount),
6565
warnings: warningCollector.getAll(),
6666
manifest: manifestTracker.buildManifest(previousReportPath: reportPath)
@@ -73,3 +73,12 @@ func withExportReport(
7373
}
7474

7575
// swiftlint:enable function_parameter_count
76+
77+
private func describeExportError(_ error: any Error) -> String {
78+
if let localized = error as? LocalizedError,
79+
let description = localized.errorDescription
80+
{
81+
return description
82+
}
83+
return String(describing: error)
84+
}

Sources/ExFigCLI/Report/ExportReportWriter.swift

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,18 @@ import Foundation
55
/// Same pattern as `Batch.swift` report writing (lines 710-716):
66
/// wrap write in do/catch, warn on failure, never propagate the error.
77
func writeExportReport(_ report: ExportReport, to path: String, ui: TerminalUI) {
8+
let data: Data
9+
do {
10+
data = try report.jsonData()
11+
} catch {
12+
ui.warning("Failed to serialize export report: \(error.localizedDescription)")
13+
return
14+
}
15+
816
do {
9-
let data = try report.jsonData()
1017
let url = URL(fileURLWithPath: path)
18+
let directory = url.deletingLastPathComponent()
19+
try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true)
1120
try data.write(to: url)
1221
ui.info("Report written to: \(path)")
1322
} catch {

Sources/ExFigCLI/Report/ManifestTracker.swift

Lines changed: 43 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,11 @@ final class ManifestTracker: Sendable {
2020
/// Default asset type for all recorded entries.
2121
let defaultAssetType: String
2222

23+
private let workingDirectory: String
24+
2325
init(assetType: String) {
2426
defaultAssetType = assetType
27+
workingDirectory = FileManager.default.currentDirectoryPath
2528
}
2629

2730
/// Pre-write filesystem state for a file path.
@@ -35,13 +38,11 @@ final class ManifestTracker: Sendable {
3538
/// Must be called BEFORE the file is written to disk, so that existing content
3639
/// can be compared for action detection (created vs. modified vs. unchanged).
3740
func capturePreState(for path: String) -> PreWriteState {
38-
let fileExisted = FileManager.default.fileExists(atPath: path)
39-
let existingChecksum: String? = if fileExisted, let existingData = FileManager.default.contents(atPath: path) {
40-
FNV1aHasher.hashToHex(existingData)
41+
if let existingData = FileManager.default.contents(atPath: path) {
42+
PreWriteState(fileExisted: true, existingChecksum: FNV1aHasher.hashToHex(existingData))
4143
} else {
42-
nil
44+
PreWriteState(fileExisted: false, existingChecksum: nil)
4345
}
44-
return PreWriteState(fileExisted: fileExisted, existingChecksum: existingChecksum)
4546
}
4647

4748
/// Record a file write operation after successful write.
@@ -55,14 +56,7 @@ final class ManifestTracker: Sendable {
5556
let assetType = assetType ?? defaultAssetType
5657
let relativePath = makeRelativePath(path)
5758
let newChecksum = FNV1aHasher.hashToHex(data)
58-
59-
let action: FileAction = if !preState.fileExisted {
60-
.created
61-
} else if let existingChecksum = preState.existingChecksum {
62-
existingChecksum == newChecksum ? .unchanged : .modified
63-
} else {
64-
.modified
65-
}
59+
let action = determineAction(preState: preState, newChecksum: newChecksum)
6660

6761
entries.withLock {
6862
$0.append(ManifestEntry(
@@ -94,14 +88,12 @@ final class ManifestTracker: Sendable {
9488
nil
9589
}
9690

97-
let action: FileAction = if !preState.fileExisted {
98-
.created
99-
} else if let existingChecksum = preState.existingChecksum, let newChecksum {
100-
existingChecksum == newChecksum ? .unchanged : .modified
101-
} else {
102-
.modified
91+
if newChecksum == nil {
92+
WarningCollectorStorage.current?.add("Manifest: could not compute checksum for \(relativePath)")
10393
}
10494

95+
let action = determineAction(preState: preState, newChecksum: newChecksum)
96+
10597
entries.withLock {
10698
$0.append(ManifestEntry(
10799
path: relativePath,
@@ -125,30 +117,46 @@ final class ManifestTracker: Sendable {
125117
var allEntries = entries.withLock { $0 }
126118

127119
if let previousPath = previousReportPath,
128-
let previousData = FileManager.default.contents(atPath: previousPath),
129-
let previousReport = try? JSONCodec.decode(PreviousReportManifest.self, from: previousData)
120+
let previousData = FileManager.default.contents(atPath: previousPath)
130121
{
131-
let currentPaths = Set(allEntries.map(\.path))
132-
for previousEntry in previousReport.manifest?.files ?? []
133-
where !currentPaths.contains(previousEntry.path)
134-
{
135-
allEntries.append(ManifestEntry(
136-
path: previousEntry.path,
137-
action: .deleted,
138-
checksum: nil,
139-
assetType: previousEntry.assetType
140-
))
122+
do {
123+
let previousReport = try JSONCodec.decode(PreviousReportManifest.self, from: previousData)
124+
let currentPaths = Set(allEntries.map(\.path))
125+
for previousEntry in previousReport.manifest?.files ?? []
126+
where !currentPaths.contains(previousEntry.path)
127+
{
128+
allEntries.append(ManifestEntry(
129+
path: previousEntry.path,
130+
action: .deleted,
131+
checksum: nil,
132+
assetType: previousEntry.assetType
133+
))
134+
}
135+
} catch {
136+
let message = "Could not read previous report at \(previousPath): "
137+
+ "\(error.localizedDescription). Deleted file detection skipped."
138+
WarningCollectorStorage.current?.add(message)
141139
}
142140
}
143141

144142
return AssetManifest(files: allEntries)
145143
}
146144

147-
/// Make path relative to current working directory.
145+
/// Determine file action based on pre-write state and new checksum.
146+
private func determineAction(preState: PreWriteState, newChecksum: String?) -> FileAction {
147+
if !preState.fileExisted {
148+
.created
149+
} else if let existingChecksum = preState.existingChecksum, let newChecksum {
150+
existingChecksum == newChecksum ? .unchanged : .modified
151+
} else {
152+
.modified
153+
}
154+
}
155+
156+
/// Make path relative to working directory captured at init time.
148157
private func makeRelativePath(_ absolutePath: String) -> String {
149-
let cwd = FileManager.default.currentDirectoryPath
150-
if absolutePath.hasPrefix(cwd + "/") {
151-
return String(absolutePath.dropFirst(cwd.count + 1))
158+
if absolutePath.hasPrefix(workingDirectory + "/") {
159+
return String(absolutePath.dropFirst(workingDirectory.count + 1))
152160
}
153161
return absolutePath
154162
}

Sources/ExFigCLI/Subcommands/ExportColors.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ extension ExFigCommand {
5656
reportPath: report,
5757
configInput: options.input,
5858
ui: ui,
59-
buildStats: { ReportStats(colors: $0, icons: 0, images: 0, typography: 0) },
59+
buildStats: { .colors($0) },
6060
export: { try await performExport(client: client, ui: ui) }
6161
)
6262
}

Sources/ExFigCLI/Subcommands/ExportIcons.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ extension ExFigCommand {
5757
reportPath: report,
5858
configInput: options.input,
5959
ui: ui,
60-
buildStats: { ReportStats(colors: 0, icons: $0, images: 0, typography: 0) },
60+
buildStats: { .icons($0) },
6161
export: { try await performExport(client: client, ui: ui) }
6262
)
6363
}

Sources/ExFigCLI/Subcommands/ExportImages.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ extension ExFigCommand {
5656
reportPath: report,
5757
configInput: options.input,
5858
ui: ui,
59-
buildStats: { ReportStats(colors: 0, icons: 0, images: $0, typography: 0) },
59+
buildStats: { .images($0) },
6060
export: { try await performExport(client: client, ui: ui) }
6161
)
6262
}

Sources/ExFigCLI/Subcommands/ExportTypography.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ extension ExFigCommand {
4545
reportPath: report,
4646
configInput: options.input,
4747
ui: ui,
48-
buildStats: { ReportStats(colors: 0, icons: 0, images: 0, typography: $0) },
48+
buildStats: { .typography($0) },
4949
export: { try await performExport(client: client, ui: ui) }
5050
)
5151
}

Tests/ExFigTests/Report/ManifestTrackerTests.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -279,7 +279,7 @@ final class AssetManifestTests: XCTestCase {
279279
ManifestEntry(path: "Same.swift", action: .unchanged, checksum: "fedcba9876543210", assetType: "image"),
280280
])
281281

282-
let data = try JSONEncoder().encode(manifest)
282+
let data = try JSONCodec.encode(manifest)
283283
let json = try JSONSerialization.jsonObject(with: data) as? [String: Any]
284284
let files = json?["files"] as? [[String: Any]]
285285

@@ -297,7 +297,7 @@ final class AssetManifestTests: XCTestCase {
297297

298298
func testEmptyManifest() throws {
299299
let manifest = AssetManifest(files: [])
300-
let data = try JSONEncoder().encode(manifest)
300+
let data = try JSONCodec.encode(manifest)
301301
let json = try JSONSerialization.jsonObject(with: data) as? [String: Any]
302302
let files = json?["files"] as? [Any]
303303

0 commit comments

Comments
 (0)