From 8332ed787f6bdc9a8f1ee15736961857c132eeaa Mon Sep 17 00:00:00 2001 From: Philip Niedertscheider Date: Wed, 14 Oct 2026 12:54:26 +0200 Subject: [PATCH] fix(skills): Recover interrupted installation operations --- .../skills/AgentSkillsInstallCommand.swift | 6 + .../skills/AgentSkillsUninstallCommand.swift | 6 + .../main/AgentSkillErrorPresentation.swift | 5 + .../AgentSkillInstallationRenderer.swift | 15 +- .../AgentSkillInstallationOutcome.swift | 9 + .../AgentSkillInstallationService.swift | 134 +++++--------- .../skills/AgentSkillInstallationStore.swift | 166 ++++++++++++++++++ .../AgentSkillsRecoveryCommandTests.swift | 43 +++++ .../AgentSkillInstallationBatchTests.swift | 37 ++++ .../AgentSkillInstallationRecoveryTests.swift | 164 +++++++++++++++++ .../AgentSkillRecoverySafetyTests.swift | 120 +++++++++++++ .../FaultInjectingSkillFileSystem.swift | 45 +++++ 12 files changed, 652 insertions(+), 98 deletions(-) create mode 100644 Sources/CLI/skills/AgentSkillInstallationStore.swift create mode 100644 Tests/CLITests/cmd/agent/skills/AgentSkillsRecoveryCommandTests.swift create mode 100644 Tests/CLITests/skills/AgentSkillInstallationBatchTests.swift create mode 100644 Tests/CLITests/skills/AgentSkillInstallationRecoveryTests.swift create mode 100644 Tests/CLITests/skills/AgentSkillRecoverySafetyTests.swift create mode 100644 Tests/CLITests/skills/FaultInjectingSkillFileSystem.swift diff --git a/Sources/CLI/cmd/agent/skills/AgentSkillsInstallCommand.swift b/Sources/CLI/cmd/agent/skills/AgentSkillsInstallCommand.swift index 860b2fa..33f75a6 100644 --- a/Sources/CLI/cmd/agent/skills/AgentSkillsInstallCommand.swift +++ b/Sources/CLI/cmd/agent/skills/AgentSkillsInstallCommand.swift @@ -45,6 +45,12 @@ struct AgentSkillsInstallCommand: ParsableCommand, GlobalOptionsProviding { dryRun: selection.dryRun, force: force ) deps.commandOutputWriter.write(AgentSkillInstallationRenderer().render(outcomes)) + } catch let failure as AgentSkillBatchFailure { + deps.commandOutputWriter.write(AgentSkillInstallationRenderer().render(failure.completed)) + if let error = failure.underlying as? AgentSkillInstallationError { + throw ValidationError(error.localizedDescription) + } + throw failure.underlying } catch let error as AgentSkillInstallationError { throw ValidationError(error.localizedDescription) } diff --git a/Sources/CLI/cmd/agent/skills/AgentSkillsUninstallCommand.swift b/Sources/CLI/cmd/agent/skills/AgentSkillsUninstallCommand.swift index 09a1817..b908e60 100644 --- a/Sources/CLI/cmd/agent/skills/AgentSkillsUninstallCommand.swift +++ b/Sources/CLI/cmd/agent/skills/AgentSkillsUninstallCommand.swift @@ -48,6 +48,12 @@ struct AgentSkillsUninstallCommand: ParsableCommand, GlobalOptionsProviding { dryRun: selection.dryRun ) deps.commandOutputWriter.write(AgentSkillInstallationRenderer().render(outcomes)) + } catch let failure as AgentSkillBatchFailure { + deps.commandOutputWriter.write(AgentSkillInstallationRenderer().render(failure.completed)) + if let error = failure.underlying as? AgentSkillInstallationError { + throw ValidationError(error.localizedDescription) + } + throw failure.underlying } catch let error as AgentSkillInstallationError { throw ValidationError(error.localizedDescription) } diff --git a/Sources/CLI/main/AgentSkillErrorPresentation.swift b/Sources/CLI/main/AgentSkillErrorPresentation.swift index 5f2c8f3..1ad75b6 100644 --- a/Sources/CLI/main/AgentSkillErrorPresentation.swift +++ b/Sources/CLI/main/AgentSkillErrorPresentation.swift @@ -15,6 +15,11 @@ extension AgentSkillInstallationError: LocalizedError { return "Expected a directory, not a symlink or other file, at '\(url.path)'." case .invalidFile(let url): return "Expected a regular file, not a symlink or directory, at '\(url.path)'." + case .invalidPendingOperation(let url): + return "Invalid pending skill operation at '\(url.path)'. No recovery was performed." + case .recoveryConflict(let url): + return "Skill files changed during an interrupted operation at '\(url.path)'. " + + "Back up and inspect them before retrying." } } } diff --git a/Sources/CLI/renderer/AgentSkillInstallationRenderer.swift b/Sources/CLI/renderer/AgentSkillInstallationRenderer.swift index ee6602b..5f74d8e 100644 --- a/Sources/CLI/renderer/AgentSkillInstallationRenderer.swift +++ b/Sources/CLI/renderer/AgentSkillInstallationRenderer.swift @@ -1,16 +1,19 @@ struct AgentSkillInstallationRenderer { func render(_ outcomes: [AgentSkillInstallationOutcome]) -> String { outcomes.map { outcome in + let action: String switch outcome.status { case .installed, .updated: - return "Installed: \(outcome.name) at \(outcome.file.path)" + action = "Installed: \(outcome.name) at \(outcome.file.path)" case .wouldInstall, .wouldUpdate: - return "Would install: \(outcome.name) at \(outcome.file.path)" - case .unchanged: return "Unchanged: \(outcome.name)" - case .uninstalled: return "Uninstalled: \(outcome.name)" - case .wouldUninstall: return "Would uninstall: \(outcome.name)" - case .notInstalled: return "Not installed: \(outcome.name)" + action = "Would install: \(outcome.name) at \(outcome.file.path)" + case .unchanged: action = "Unchanged: \(outcome.name)" + case .uninstalled: action = "Uninstalled: \(outcome.name)" + case .wouldUninstall: action = "Would uninstall: \(outcome.name)" + case .notInstalled: action = "Not installed: \(outcome.name)" } + let recovery = outcome.requiresRecovery ? "Would recover pending operation: \(outcome.name)\n" : "" + return recovery + action }.joined(separator: "\n") } } diff --git a/Sources/CLI/skills/AgentSkillInstallationOutcome.swift b/Sources/CLI/skills/AgentSkillInstallationOutcome.swift index 0892ccc..43d5cb3 100644 --- a/Sources/CLI/skills/AgentSkillInstallationOutcome.swift +++ b/Sources/CLI/skills/AgentSkillInstallationOutcome.swift @@ -9,6 +9,7 @@ struct AgentSkillInstallationOutcome: Equatable, Sendable { let name: String let file: URL let status: Status + var requiresRecovery = false } enum AgentSkillInstallationError: Error, Equatable { @@ -18,4 +19,12 @@ enum AgentSkillInstallationError: Error, Equatable { case invalidReceipt(URL) case invalidDirectory(URL) case invalidFile(URL) + case invalidPendingOperation(URL) + case recoveryConflict(URL) +} + +struct AgentSkillBatchFailure: Error { + let completed: [AgentSkillInstallationOutcome] + let failedSkill: String + let underlying: any Error } diff --git a/Sources/CLI/skills/AgentSkillInstallationService.swift b/Sources/CLI/skills/AgentSkillInstallationService.swift index 536d5fa..2e2b102 100644 --- a/Sources/CLI/skills/AgentSkillInstallationService.swift +++ b/Sources/CLI/skills/AgentSkillInstallationService.swift @@ -1,20 +1,16 @@ import Foundation import Logging -/// Owns only SKILL.md and its installation receipt, never an entire skill directory. +/// Owns only SKILL.md, its receipt, and its recovery journal, never an entire skill directory. struct DefaultAgentSkillInstallationService { - private struct Receipt: Codable { - let name: String - let content: Data - } - private struct Installation { let skill: BundledAgentSkill - let directory: URL - let file: URL - let receiptFile: URL - let content: Data? - let receipt: Receipt? + let store: AgentSkillInstallationStore + let inspection: AgentSkillInstallationStore.Inspection + + var content: Data? { inspection.state.content } + var receipt: AgentSkillInstallationStore.Receipt? { inspection.state.receipt } + var file: URL { store.file } } private let fileManager: AgentSkillFileSystem @@ -55,7 +51,7 @@ struct DefaultAgentSkillInstallationService { } logger.debug( "Skill installation preflight completed", metadata: ["count": .stringConvertible(installations.count)]) - return try installations.map { try install($0, dryRun: dryRun, force: force) } + return try execute(installations) { try install($0, dryRun: dryRun, force: force) } } catch { if !(error is AgentSkillInstallationError) { logger.error( @@ -91,7 +87,7 @@ struct DefaultAgentSkillInstallationService { } logger.debug( "Skill removal preflight completed", metadata: ["count": .stringConvertible(installations.count)]) - return try installations.map { try uninstall($0, dryRun: dryRun) } + return try execute(installations) { try uninstall($0, dryRun: dryRun) } } catch { if !(error is AgentSkillInstallationError) { logger.error( @@ -101,15 +97,33 @@ struct DefaultAgentSkillInstallationService { } } + private func execute( + _ installations: [Installation], operation: (Installation) throws -> AgentSkillInstallationOutcome + ) throws -> [AgentSkillInstallationOutcome] { + var completed: [AgentSkillInstallationOutcome] = [] + for installation in installations { + do { + completed.append(try operation(installation)) + } catch { + guard !completed.isEmpty else { throw error } + throw AgentSkillBatchFailure( + completed: completed, failedSkill: installation.skill.name, underlying: error) + } + } + return completed + } + private func install( _ installation: Installation, dryRun: Bool, force: Bool ) throws -> AgentSkillInstallationOutcome { let metadata: Logger.Metadata = ["skill": .string(installation.skill.name)] let content = Data(installation.skill.content.utf8) + if !dryRun { try installation.store.recover(installation.inspection) } if installation.content == content, installation.receipt?.content == content { logger.debug("Skill installation unchanged", metadata: metadata) return AgentSkillInstallationOutcome( - name: installation.skill.name, file: installation.file, status: .unchanged) + name: installation.skill.name, file: installation.file, status: .unchanged, + requiresRecovery: dryRun && installation.inspection.pending != nil) } if force, let previousContent = installation.content, previousContent != content { logger.notice( @@ -117,41 +131,34 @@ struct DefaultAgentSkillInstallationService { ) } if !dryRun { - try fileManager.createDirectory( - at: installation.directory, withIntermediateDirectories: true, attributes: nil) - try fileManager.writeSkillData(content, to: installation.file) - let receipt = Receipt(name: installation.skill.name, content: content) - try fileManager.writeSkillData(JSONEncoder().encode(receipt), to: installation.receiptFile) + try installation.store.update( + from: installation.inspection.state, + to: .init(content: content, receipt: .init(name: installation.skill.name, content: content))) } logger.info(dryRun ? "Would install skill" : "Installed skill", metadata: metadata) let updating = installation.content != nil return AgentSkillInstallationOutcome( name: installation.skill.name, file: installation.file, - status: dryRun ? (updating ? .wouldUpdate : .wouldInstall) : (updating ? .updated : .installed)) + status: dryRun ? (updating ? .wouldUpdate : .wouldInstall) : (updating ? .updated : .installed), + requiresRecovery: dryRun && installation.inspection.pending != nil) } private func uninstall(_ installation: Installation, dryRun: Bool) throws -> AgentSkillInstallationOutcome { let metadata: Logger.Metadata = ["skill": .string(installation.skill.name)] + if !dryRun { try installation.store.recover(installation.inspection) } guard installation.receipt != nil else { logger.debug("Skill not installed", metadata: metadata) return AgentSkillInstallationOutcome( - name: installation.skill.name, file: installation.file, status: .notInstalled) + name: installation.skill.name, file: installation.file, status: .notInstalled, + requiresRecovery: dryRun && installation.inspection.pending != nil) } if !dryRun { - if installation.content != nil { - try fileManager.removeItem(at: installation.file) - } - try fileManager.removeItem(at: installation.receiptFile) - if try fileManager.contentsOfDirectory(atPath: installation.directory.path).isEmpty { - try fileManager.removeItem(at: installation.directory) - logger.trace("Removed empty skill directory", metadata: metadata) - } else { - logger.debug("Preserved unrelated skill directory contents", metadata: metadata) - } + try installation.store.update(from: installation.inspection.state, to: .init()) } logger.info(dryRun ? "Would uninstall skill" : "Uninstalled skill", metadata: metadata) return AgentSkillInstallationOutcome( - name: installation.skill.name, file: installation.file, status: dryRun ? .wouldUninstall : .uninstalled) + name: installation.skill.name, file: installation.file, status: dryRun ? .wouldUninstall : .uninstalled, + requiresRecovery: dryRun && installation.inspection.pending != nil) } private func inspect(_ skills: [BundledAgentSkill], root: String) throws -> [Installation] { @@ -163,66 +170,9 @@ struct DefaultAgentSkillInstallationService { let rootURL = URL(fileURLWithPath: (root as NSString).expandingTildeInPath).standardizedFileURL return try skills.map { skill in logger.debug("Inspecting skill installation", metadata: ["skill": .string(skill.name)]) - let directory = rootURL.appendingPathComponent("skills").appendingPathComponent(skill.name) - let file = directory.appendingPathComponent("SKILL.md") - let receiptFile = directory.appendingPathComponent(".apple-docs-managed.json") - try validateDirectory(directory, root: rootURL) - let content = try readRegularFile(file) - let receiptData = try readRegularFile(receiptFile) - let receipt: Receipt? - if let receiptData { - guard let decoded = try? JSONDecoder().decode(Receipt.self, from: receiptData), - decoded.name == skill.name - else { - logger.warning("Invalid skill installation receipt", metadata: ["skill": .string(skill.name)]) - throw AgentSkillInstallationError.invalidReceipt(receiptFile) - } - logger.trace("Validated skill installation receipt", metadata: ["skill": .string(skill.name)]) - receipt = decoded - } else { - logger.trace("No skill installation receipt", metadata: ["skill": .string(skill.name)]) - receipt = nil - } - return Installation( - skill: skill, directory: directory, file: file, receiptFile: receiptFile, - content: content, receipt: receipt - ) - } - } - - private func validateDirectory(_ directory: URL, root: URL) throws { - logger.trace("Validating skill directory ancestors") - var ancestor = directory - while true { - if let type = try fileType(ancestor), type != .typeDirectory { - logger.warning("Refusing symlink or non-directory in skill installation path") - throw AgentSkillInstallationError.invalidDirectory(ancestor) - } - // Ancestors above the user-selected root may be OS aliases such as /var on macOS. - if ancestor.path == root.path { return } - ancestor.deleteLastPathComponent() - } - } - - private func readRegularFile(_ url: URL) throws -> Data? { - logger.trace("Reading skill installation file", metadata: ["file": .string(url.lastPathComponent)]) - guard let type = try fileType(url) else { return nil } - guard type == .typeRegular else { - logger.warning( - "Refusing symlink or non-regular skill installation file", - metadata: ["file": .string(url.lastPathComponent)]) - throw AgentSkillInstallationError.invalidFile(url) - } - return try fileManager.readSkillData(at: url) - } - - private func fileType(_ url: URL) throws -> FileAttributeType? { - logger.trace("Inspecting filesystem entry type") - do { - return try fileManager.attributesOfItem(atPath: url.path)[.type] as? FileAttributeType - } catch let error as CocoaError where error.code == .fileReadNoSuchFile { - logger.trace("Filesystem entry absent") - return nil + let store = AgentSkillInstallationStore( + name: skill.name, root: rootURL, fileManager: fileManager, logger: logger) + return Installation(skill: skill, store: store, inspection: try store.inspect()) } } } diff --git a/Sources/CLI/skills/AgentSkillInstallationStore.swift b/Sources/CLI/skills/AgentSkillInstallationStore.swift new file mode 100644 index 0000000..b407309 --- /dev/null +++ b/Sources/CLI/skills/AgentSkillInstallationStore.swift @@ -0,0 +1,166 @@ +import Foundation +import Logging + +struct AgentSkillInstallationStore { + struct Receipt: Codable, Equatable { + let name: String + let content: Data + } + + struct Snapshot: Codable, Equatable { + var content: Data? + var receipt: Receipt? + } + + struct Pending: Codable, Equatable { + let name: String + let previous: Snapshot + let next: Snapshot + } + + struct Inspection { + let state: Snapshot + let pending: Pending? + } + + let directory: URL + var file: URL { directory.appendingPathComponent("SKILL.md") } + private var receiptFile: URL { directory.appendingPathComponent(".apple-docs-managed.json") } + private var pendingFile: URL { directory.appendingPathComponent(".apple-docs-pending.json") } + private let name: String + private let root: URL + private let fileManager: AgentSkillFileSystem + private let logger: Logger + + init(name: String, root: URL, fileManager: AgentSkillFileSystem, logger: Logger) { + self.name = name + self.root = root + self.fileManager = fileManager + self.logger = logger + directory = root.appendingPathComponent("skills").appendingPathComponent(name) + } + + func inspect() throws -> Inspection { + try validateDirectory() + let current = try snapshot() + guard let pending = try pending() else { return Inspection(state: current, pending: nil) } + try validate(current, against: pending) + return Inspection(state: pending.next, pending: pending) + } + + func recover(_ inspection: Inspection) throws { + let current = try inspect() + guard current.pending == inspection.pending, current.state == inspection.state else { + throw AgentSkillInstallationError.recoveryConflict(directory) + } + if let pending = current.pending { + try apply(pending) + logger.info("Recovered interrupted skill operation", metadata: ["skill": .string(name)]) + } + } + + func update(from previous: Snapshot, to next: Snapshot) throws { + let current = try inspect() + guard current.pending == nil, current.state == previous else { + throw AgentSkillInstallationError.recoveryConflict(directory) + } + try fileManager.createDirectory(at: directory, withIntermediateDirectories: true, attributes: nil) + let pending = Pending(name: name, previous: previous, next: next) + try fileManager.writeSkillData(JSONEncoder().encode(pending), to: pendingFile) + try apply(pending) + } + + private func apply(_ pending: Pending) throws { + try validateDirectory() + guard try self.pending() == pending else { throw AgentSkillInstallationError.recoveryConflict(directory) } + let current = try snapshot() + try validate(current, against: pending) + if current.content != pending.next.content { + if let content = pending.next.content { + try fileManager.writeSkillData(content, to: file) + } else { + try fileManager.removeItem(at: file) + } + } + if current.receipt != pending.next.receipt { + if let receipt = pending.next.receipt { + try fileManager.writeSkillData(JSONEncoder().encode(receipt), to: receiptFile) + } else { + try fileManager.removeItem(at: receiptFile) + } + } + guard try snapshot() == pending.next else { throw AgentSkillInstallationError.recoveryConflict(directory) } + try fileManager.removeItem(at: pendingFile) + if pending.next == Snapshot(), try fileManager.contentsOfDirectory(atPath: directory.path).isEmpty { + try fileManager.removeItem(at: directory) + } + } + + private func pending() throws -> Pending? { + guard let data = try regularData(pendingFile) else { return nil } + guard let pending = try? JSONDecoder().decode(Pending.self, from: data), pending.name == name, + pending.previous.receipt.map({ $0.name == name }) ?? true, + pending.previous.content == nil || pending.previous.receipt != nil, + pending.next == Snapshot() + || (pending.next.receipt.map { $0.name == name && pending.next.content == $0.content } == true) + else { + logger.warning("Invalid pending skill operation", metadata: ["skill": .string(name)]) + throw AgentSkillInstallationError.invalidPendingOperation(pendingFile) + } + return pending + } + + private func validate(_ current: Snapshot, against pending: Pending) throws { + guard current.content == pending.previous.content || current.content == pending.next.content, + current.receipt == pending.previous.receipt || current.receipt == pending.next.receipt + else { + logger.warning("Refusing recovery over changed skill files", metadata: ["skill": .string(name)]) + throw AgentSkillInstallationError.recoveryConflict(directory) + } + } + + private func snapshot() throws -> Snapshot { + let content = try regularData(file) + let receipt: Receipt? + if let data = try regularData(receiptFile) { + guard let decoded = try? JSONDecoder().decode(Receipt.self, from: data), decoded.name == name else { + logger.warning("Invalid skill installation receipt", metadata: ["skill": .string(name)]) + throw AgentSkillInstallationError.invalidReceipt(receiptFile) + } + receipt = decoded + } else { + receipt = nil + } + return Snapshot(content: content, receipt: receipt) + } + + private func validateDirectory() throws { + var ancestor = directory + while true { + if let type = try fileType(ancestor), type != .typeDirectory { + logger.warning("Refusing symlink or non-directory in skill installation path") + throw AgentSkillInstallationError.invalidDirectory(ancestor) + } + // Ancestors above the selected root may be OS aliases such as /var on macOS. + if ancestor.path == root.path { return } + ancestor.deleteLastPathComponent() + } + } + + private func regularData(_ url: URL) throws -> Data? { + guard let type = try fileType(url) else { return nil } + guard type == .typeRegular else { + logger.warning("Refusing symlink or non-regular skill installation file") + throw AgentSkillInstallationError.invalidFile(url) + } + return try fileManager.readSkillData(at: url) + } + + private func fileType(_ url: URL) throws -> FileAttributeType? { + do { + return try fileManager.attributesOfItem(atPath: url.path)[.type] as? FileAttributeType + } catch let error as CocoaError where error.code == .fileReadNoSuchFile { + return nil + } + } +} diff --git a/Tests/CLITests/cmd/agent/skills/AgentSkillsRecoveryCommandTests.swift b/Tests/CLITests/cmd/agent/skills/AgentSkillsRecoveryCommandTests.swift new file mode 100644 index 0000000..00f4211 --- /dev/null +++ b/Tests/CLITests/cmd/agent/skills/AgentSkillsRecoveryCommandTests.swift @@ -0,0 +1,43 @@ +import Foundation +import Logging +import Testing + +@testable import CLI + +@Suite("Skill command partial completion") +struct AgentSkillsRecoveryCommandTests { + @Test("prints completed outcomes before propagating a later filesystem failure") + func reportsPartialCompletion() throws { + // -- Arrange -- + let root = FileManager.default.temporaryDirectory.resolvingSymlinksInPath() + .appendingPathComponent(UUID().uuidString) + defer { try? FileManager.default.removeItem(at: root) } + let command = try AgentSkillsInstallCommand.parse([ + "apple-docs", "apple-docs-discover-api", "--dir", root.path, + ]) + let deps = RecoveryDependencies() + + // -- Act -- + #expect(throws: InjectedSkillFailure.self) { try command.run(deps: deps) } + + // -- Assert -- + #expect(deps.output.lines.count == 1) + #expect(deps.output.lines.first?.contains("Installed: apple-docs at") == true) + #expect(deps.output.lines.first?.contains("Installed: apple-docs-discover-api at") == false) + } +} + +private struct RecoveryDependencies: AgentSkillInstallationServiceProvider, CommandOutputWriterProvider { + let output = RecordingCommandOutputWriter() + var commandOutputWriter: RecordingCommandOutputWriter { output } + + func agentSkillFileManager() -> FaultInjectingSkillFileSystem { + FaultInjectingSkillFileSystem( + operation: .write, filename: ".apple-docs-managed.json", skillName: "apple-docs-discover-api") + } + + func agentSkillInstallationService() -> DefaultAgentSkillInstallationService { + DefaultAgentSkillInstallationService( + logger: Logger(label: "test") { _ in SwiftLogNoOpLogHandler() }, fileManager: agentSkillFileManager()) + } +} diff --git a/Tests/CLITests/skills/AgentSkillInstallationBatchTests.swift b/Tests/CLITests/skills/AgentSkillInstallationBatchTests.swift new file mode 100644 index 0000000..a6e8951 --- /dev/null +++ b/Tests/CLITests/skills/AgentSkillInstallationBatchTests.swift @@ -0,0 +1,37 @@ +import Foundation +import Logging +import Testing + +@testable import CLI + +@Suite("Skill batch failures") +struct AgentSkillInstallationBatchTests { + @Test("reports completed skills when a later installation is interrupted") + func preservesCompletedOutcomes() throws { + // -- Arrange -- + let root = FileManager.default.temporaryDirectory.resolvingSymlinksInPath() + .appendingPathComponent(UUID().uuidString) + defer { try? FileManager.default.removeItem(at: root) } + let skills = ["first", "second"].map { BundledAgentSkill(name: $0, shortDescription: "Fixture", content: $0) } + let service = DefaultAgentSkillInstallationService( + logger: Logger(label: "test") { _ in SwiftLogNoOpLogHandler() }, + fileManager: FaultInjectingSkillFileSystem( + operation: .write, filename: ".apple-docs-managed.json", skillName: "second")) + + // -- Act -- + do { + _ = try service.install(skills, root: root.path, dryRun: false, force: false) + Issue.record("Expected the second installation to fail") + } catch let failure as AgentSkillBatchFailure { + // -- Assert -- + #expect(failure.completed.map(\.name) == ["first"]) + #expect(failure.completed.map(\.status) == [.installed]) + #expect(failure.failedSkill == "second") + #expect(failure.underlying is InjectedSkillFailure) + #expect(try Data(contentsOf: root.appendingPathComponent("skills/first/SKILL.md")) == Data("first".utf8)) + #expect( + FileManager.default.fileExists( + atPath: root.appendingPathComponent("skills/second/.apple-docs-pending.json").path)) + } + } +} diff --git a/Tests/CLITests/skills/AgentSkillInstallationRecoveryTests.swift b/Tests/CLITests/skills/AgentSkillInstallationRecoveryTests.swift new file mode 100644 index 0000000..8f1b7cd --- /dev/null +++ b/Tests/CLITests/skills/AgentSkillInstallationRecoveryTests.swift @@ -0,0 +1,164 @@ +import Foundation +import Logging +import Testing + +@testable import CLI + +@Suite("Interrupted skill installation recovery") +struct AgentSkillInstallationRecoveryTests { + @Test("recovers a fresh installation interrupted before the receipt write") + func recoversFreshInstallation() throws { + // -- Arrange -- + let root = temporaryRoot() + defer { try? FileManager.default.removeItem(at: root) } + let skill = fixture("first version") + let failing = installer(operation: .write, filename: ".apple-docs-managed.json") + + // -- Act -- + #expect(throws: InjectedSkillFailure.self) { + try failing.install([skill], root: root.path, dryRun: false, force: false) + } + let outcome = try healthy.install([skill], root: root.path, dryRun: false, force: false) + + // -- Assert -- + #expect(outcome.map(\.status) == [.unchanged]) + #expect(try Data(contentsOf: file(root)) == Data("first version".utf8)) + #expect(FileManager.default.fileExists(atPath: receipt(root).path)) + #expect(!FileManager.default.fileExists(atPath: pending(root).path)) + } + + @Test("completes an interrupted update before honoring a subsequent uninstall") + func recoversManagedUpdate() throws { + // -- Arrange -- + let root = temporaryRoot() + defer { try? FileManager.default.removeItem(at: root) } + _ = try healthy.install([fixture("old")], root: root.path, dryRun: false, force: false) + let unrelated = file(root).deletingLastPathComponent().appendingPathComponent("notes.txt") + try Data("user notes".utf8).write(to: unrelated) + let failing = installer(operation: .write, filename: ".apple-docs-managed.json") + + // -- Act -- + #expect(throws: InjectedSkillFailure.self) { + try failing.install([fixture("new")], root: root.path, dryRun: false, force: true) + } + let outcome = try healthy.uninstall([fixture("new")], root: root.path, dryRun: false) + + // -- Assert -- + #expect(outcome.map(\.status) == [.uninstalled]) + #expect(!FileManager.default.fileExists(atPath: file(root).path)) + #expect(!FileManager.default.fileExists(atPath: receipt(root).path)) + #expect(!FileManager.default.fileExists(atPath: pending(root).path)) + #expect(try Data(contentsOf: unrelated) == Data("user notes".utf8)) + } + + @Test("refuses recovery over external edits even with force") + func preservesExternalEdits() throws { + // -- Arrange -- + let root = temporaryRoot() + defer { try? FileManager.default.removeItem(at: root) } + let failing = installer(operation: .write, filename: ".apple-docs-managed.json") + #expect(throws: InjectedSkillFailure.self) { + try failing.install([fixture("bundled")], root: root.path, dryRun: false, force: false) + } + try Data("external edits".utf8).write(to: file(root)) + + // -- Act -- + #expect(throws: (any Error).self) { + try healthy.install([fixture("bundled")], root: root.path, dryRun: false, force: true) + } + + // -- Assert -- + #expect(try Data(contentsOf: file(root)) == Data("external edits".utf8)) + #expect(FileManager.default.fileExists(atPath: pending(root).path)) + #expect(!FileManager.default.fileExists(atPath: receipt(root).path)) + } + + @Test("retries interrupted removal without deleting unrelated files") + func recoversRemoval() throws { + // -- Arrange -- + let root = temporaryRoot() + defer { try? FileManager.default.removeItem(at: root) } + let skill = fixture("bundled") + _ = try healthy.install([skill], root: root.path, dryRun: false, force: false) + let failing = installer(operation: .remove, filename: ".apple-docs-managed.json") + + // -- Act -- + #expect(throws: InjectedSkillFailure.self) { try failing.uninstall([skill], root: root.path, dryRun: false) } + let outcome = try healthy.uninstall([skill], root: root.path, dryRun: false) + + // -- Assert -- + #expect(outcome.map(\.status) == [.notInstalled]) + #expect(!FileManager.default.fileExists(atPath: file(root).deletingLastPathComponent().path)) + } + + @Test("a dry run leaves an interrupted installation byte-for-byte unchanged") + func previewsRecoveryWithoutWriting() throws { + // -- Arrange -- + let root = temporaryRoot() + defer { try? FileManager.default.removeItem(at: root) } + let skill = fixture("bundled") + let failing = installer(operation: .write, filename: ".apple-docs-managed.json") + #expect(throws: InjectedSkillFailure.self) { + try failing.install([skill], root: root.path, dryRun: false, force: false) + } + let previousJournal = try Data(contentsOf: pending(root)) + let previousContent = try Data(contentsOf: file(root)) + + // -- Act -- + let outcomes = try healthy.install([skill], root: root.path, dryRun: true, force: false) + let output = AgentSkillInstallationRenderer().render(outcomes) + + // -- Assert -- + #expect(output.contains("Would recover pending operation: apple-docs")) + #expect(try Data(contentsOf: pending(root)) == previousJournal) + #expect(try Data(contentsOf: file(root)) == previousContent) + #expect(!FileManager.default.fileExists(atPath: receipt(root).path)) + } + + @Test("retries recovery when journal cleanup was interrupted") + func retriesJournalCleanup() throws { + // -- Arrange -- + let root = temporaryRoot() + defer { try? FileManager.default.removeItem(at: root) } + let skill = fixture("bundled") + let failing = installer(operation: .remove, filename: ".apple-docs-pending.json") + + // -- Act -- + #expect(throws: InjectedSkillFailure.self) { + try failing.install([skill], root: root.path, dryRun: false, force: false) + } + let outcome = try healthy.install([skill], root: root.path, dryRun: false, force: false) + + // -- Assert -- + #expect(outcome.map(\.status) == [.unchanged]) + #expect(!FileManager.default.fileExists(atPath: pending(root).path)) + #expect(try Data(contentsOf: file(root)) == Data("bundled".utf8)) + } + + private var healthy: DefaultAgentSkillInstallationService { + DefaultAgentSkillInstallationService(logger: Logger(label: "test") { _ in SwiftLogNoOpLogHandler() }) + } + + private func installer( + operation: FaultInjectingSkillFileSystem.Operation, filename: String + ) -> DefaultAgentSkillInstallationService { + DefaultAgentSkillInstallationService( + logger: Logger(label: "test") { _ in SwiftLogNoOpLogHandler() }, + fileManager: FaultInjectingSkillFileSystem(operation: operation, filename: filename)) + } + + private func fixture(_ content: String) -> BundledAgentSkill { + BundledAgentSkill(name: "apple-docs", shortDescription: "Fixture", content: content) + } + + private func temporaryRoot() -> URL { + FileManager.default.temporaryDirectory.resolvingSymlinksInPath().appendingPathComponent(UUID().uuidString) + } + private func file(_ root: URL) -> URL { root.appendingPathComponent("skills/apple-docs/SKILL.md") } + private func receipt(_ root: URL) -> URL { + root.appendingPathComponent("skills/apple-docs/.apple-docs-managed.json") + } + private func pending(_ root: URL) -> URL { + root.appendingPathComponent("skills/apple-docs/.apple-docs-pending.json") + } +} diff --git a/Tests/CLITests/skills/AgentSkillRecoverySafetyTests.swift b/Tests/CLITests/skills/AgentSkillRecoverySafetyTests.swift new file mode 100644 index 0000000..d37cc23 --- /dev/null +++ b/Tests/CLITests/skills/AgentSkillRecoverySafetyTests.swift @@ -0,0 +1,120 @@ +import Foundation +import Logging +import Testing + +@testable import CLI + +@Suite("Skill recovery safety") +struct AgentSkillRecoverySafetyTests { + @Test("preflights all selected skills before recovering an earlier interrupted operation") + func preflightsBeforeRecovery() throws { + // -- Arrange -- + let root = temporaryRoot() + defer { try? FileManager.default.removeItem(at: root) } + let first = fixture("first") + let second = fixture("second") + let failing = DefaultAgentSkillInstallationService( + logger: logger, + fileManager: FaultInjectingSkillFileSystem(operation: .write, filename: ".apple-docs-managed.json")) + #expect(throws: InjectedSkillFailure.self) { + try failing.install([first], root: root.path, dryRun: false, force: false) + } + let pending = root.appendingPathComponent("skills/first/.apple-docs-pending.json") + let previousJournal = try Data(contentsOf: pending) + let unmanaged = root.appendingPathComponent("skills/second/SKILL.md") + try FileManager.default.createDirectory( + at: unmanaged.deletingLastPathComponent(), withIntermediateDirectories: true) + try Data("user skill".utf8).write(to: unmanaged) + + // -- Act -- + #expect(throws: AgentSkillInstallationError.unmanagedSkill(name: "second", removing: false)) { + try healthy.install([first, second], root: root.path, dryRun: false, force: true) + } + + // -- Assert -- + #expect(try Data(contentsOf: pending) == previousJournal) + #expect( + !FileManager.default.fileExists( + atPath: root.appendingPathComponent("skills/first/.apple-docs-managed.json").path)) + #expect(try Data(contentsOf: unmanaged) == Data("user skill".utf8)) + } + + @Test("refuses symlinked operation journals without touching their targets") + func rejectsJournalSymlink() throws { + // -- Arrange -- + let root = temporaryRoot() + defer { try? FileManager.default.removeItem(at: root) } + let skill = fixture("skill") + _ = try healthy.install([skill], root: root.path, dryRun: false, force: false) + let outside = root.appendingPathComponent("outside.txt") + try Data("private content".utf8).write(to: outside) + let pending = root.appendingPathComponent("skills/skill/.apple-docs-pending.json") + try FileManager.default.createSymbolicLink(at: pending, withDestinationURL: outside) + + // -- Act -- + #expect(throws: AgentSkillInstallationError.invalidFile(pending)) { + try healthy.install([skill], root: root.path, dryRun: false, force: true) + } + + // -- Assert -- + #expect(try Data(contentsOf: outside) == Data("private content".utf8)) + #expect(try Data(contentsOf: root.appendingPathComponent("skills/skill/SKILL.md")) == Data("skill".utf8)) + } + + @Test("refuses malformed journals instead of inferring ownership") + func rejectsMalformedJournal() throws { + // -- Arrange -- + let root = temporaryRoot() + defer { try? FileManager.default.removeItem(at: root) } + let skill = fixture("skill") + _ = try healthy.install([skill], root: root.path, dryRun: false, force: false) + let pending = root.appendingPathComponent("skills/skill/.apple-docs-pending.json") + try Data("{}".utf8).write(to: pending) + + // -- Act -- + #expect(throws: AgentSkillInstallationError.invalidPendingOperation(pending)) { + try healthy.uninstall([skill], root: root.path, dryRun: false) + } + + // -- Assert -- + #expect(try Data(contentsOf: pending) == Data("{}".utf8)) + #expect(try Data(contentsOf: root.appendingPathComponent("skills/skill/SKILL.md")) == Data("skill".utf8)) + } + + @Test("an external receipt edit prevents recovery even when the content is unchanged") + func preservesReceiptEdits() throws { + // -- Arrange -- + let root = temporaryRoot() + defer { try? FileManager.default.removeItem(at: root) } + let skill = fixture("skill") + let failing = DefaultAgentSkillInstallationService( + logger: logger, + fileManager: FaultInjectingSkillFileSystem(operation: .remove, filename: ".apple-docs-pending.json")) + #expect(throws: InjectedSkillFailure.self) { + try failing.install([skill], root: root.path, dryRun: false, force: false) + } + let receipt = root.appendingPathComponent("skills/skill/.apple-docs-managed.json") + let edited = Data(#"{"name":"skill","content":"ZXh0ZXJuYWw="}"#.utf8) + try edited.write(to: receipt) + + // -- Act -- + #expect(throws: AgentSkillInstallationError.recoveryConflict(receipt.deletingLastPathComponent())) { + try healthy.install([skill], root: root.path, dryRun: false, force: true) + } + + // -- Assert -- + #expect(try Data(contentsOf: receipt) == edited) + #expect( + FileManager.default.fileExists( + atPath: root.appendingPathComponent("skills/skill/.apple-docs-pending.json").path)) + } + + private var logger: Logger { Logger(label: "test") { _ in SwiftLogNoOpLogHandler() } } + private var healthy: DefaultAgentSkillInstallationService { DefaultAgentSkillInstallationService(logger: logger) } + private func fixture(_ name: String) -> BundledAgentSkill { + BundledAgentSkill(name: name, shortDescription: "Fixture", content: name) + } + private func temporaryRoot() -> URL { + FileManager.default.temporaryDirectory.resolvingSymlinksInPath().appendingPathComponent(UUID().uuidString) + } +} diff --git a/Tests/CLITests/skills/FaultInjectingSkillFileSystem.swift b/Tests/CLITests/skills/FaultInjectingSkillFileSystem.swift new file mode 100644 index 0000000..6c018cf --- /dev/null +++ b/Tests/CLITests/skills/FaultInjectingSkillFileSystem.swift @@ -0,0 +1,45 @@ +import Foundation + +@testable import CLI + +enum InjectedSkillFailure: Error { + case filesystem +} + +struct FaultInjectingSkillFileSystem: AgentSkillFileSystem { + enum Operation { case write, remove } + let operation: Operation + let filename: String + var skillName: String? + private let fileManager = FileManager.default + + var currentDirectoryPath: String { fileManager.currentDirectoryPath } + var homeDirectoryForCurrentUser: URL { fileManager.homeDirectoryForCurrentUser } + func fileExists(atPath path: String) -> Bool { fileManager.fileExists(atPath: path) } + func createDirectory( + at url: URL, withIntermediateDirectories createIntermediates: Bool, attributes: [FileAttributeKey: Any]? + ) throws { + try fileManager.createDirectory( + at: url, withIntermediateDirectories: createIntermediates, attributes: attributes) + } + func removeItem(at url: URL) throws { + if operation == .remove, matches(url) { throw InjectedSkillFailure.filesystem } + try fileManager.removeItem(at: url) + } + func contentsOfDirectory(atPath path: String) throws -> [String] { + try fileManager.contentsOfDirectory(atPath: path) + } + func attributesOfItem(atPath path: String) throws -> [FileAttributeKey: Any] { + try fileManager.attributesOfItem(atPath: path) + } + func readSkillData(at url: URL) throws -> Data { try Data(contentsOf: url) } + func writeSkillData(_ data: Data, to url: URL) throws { + if operation == .write, matches(url) { throw InjectedSkillFailure.filesystem } + try data.write(to: url, options: .atomic) + } + + private func matches(_ url: URL) -> Bool { + url.lastPathComponent == filename + && (skillName == nil || url.deletingLastPathComponent().lastPathComponent == skillName) + } +}