diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index c66b582d..69b00522 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -116,3 +116,13 @@ Agent conversations, runs, visible messages, tool calls/results, and reviews are stored in SwiftData after truncation and redaction. Launch marks unfinished runs `interrupted`; history is restored but remote actions are never resumed or replayed. + +A process that dies mid-batch — force quit, crash, the OS reclaiming the app — +leaves tool calls that nothing answered, which the next request would be +rejected for. Restore closes that in two steps. Results are recorded as each +tool finishes but their transcript rows wait for the run's next snapshot save, +so `loadConversations` first materializes any recorded result whose transcript +row was never written. Whatever is still unanswered is answered at the model +boundary by `AgentUnansweredToolCallRepair`, which says the outcome is unknown +rather than that the tool did not run: the process died after the command was +dispatched, and a model told otherwise would repeat it. diff --git a/Termind/Application/Agent/AgentContextPipeline.swift b/Termind/Application/Agent/AgentContextPipeline.swift index 51696301..85692980 100644 --- a/Termind/Application/Agent/AgentContextPipeline.swift +++ b/Termind/Application/Agent/AgentContextPipeline.swift @@ -37,16 +37,18 @@ nonisolated struct AgentContextPipeline: Sendable { /// What a chat run uses. /// - /// Order matters. Ordering runs before the orphan repair so a reordered - /// block is what gets checked; trimming runs after both so it measures the - /// results that will actually be sent; injection runs last because it is - /// the only step that adds a message, and every earlier step reasons about + /// Order matters. The unanswered repair runs first so the results it + /// invents are sorted into place by the step after it; ordering runs before + /// the orphan repair so a reordered block is what gets checked; trimming + /// runs after all three so it measures the results that will actually be + /// sent; injection runs last because everything before it reasons about /// tool results sitting directly behind their assistant turn. static func chat( sessionContext: AgentTranscriptMessage?, before promptID: AgentTranscriptMessage.ID? ) -> AgentContextPipeline { AgentContextPipeline([ + AgentUnansweredToolCallRepair(), AgentToolResultOrdering(), AgentOrphanedToolResultRepair(), AgentToolResultTrimming(), @@ -149,6 +151,71 @@ nonisolated struct AgentOrphanedToolResultRepair: AgentContextTransforming { } } +// MARK: - Unanswered calls + +/// Answers tool calls that nothing ever answered. +/// +/// The mirror of `AgentOrphanedToolResultRepair`, and the same class of fatal: +/// where a result with no call is rejected, so is a call with no result. OpenAI +/// says `No tool output found for function call call_…`, and the other two +/// reject the equivalent — the conversation cannot be continued at all after +/// that, only deleted. +/// +/// The gap this closes is a process that dies mid-batch. A live run always +/// answers every call, cancellation included — see `AgentToolScheduler` — but a +/// force quit, a crash, or the OS reclaiming the app takes the run's remaining +/// writes with it, and the history restored at launch has an assistant turn +/// with nothing behind it. +/// +/// The wording matters as much as the presence. It must not say the tool did +/// not run: the process died *after* the command was dispatched, so whether the +/// remote side executed it is genuinely unknown, and a model told "this never +/// ran" will happily run it again. Telling it to check first is the only honest +/// instruction, and the only safe one when the call was `rm`. +nonisolated struct AgentUnansweredToolCallRepair: AgentContextTransforming { + func transform(_ context: AgentModelContext) -> AgentModelContext { + var result = context + result.messages = Self.apply(result.messages) + return result + } + + static func apply(_ messages: [AgentTranscriptMessage]) -> [AgentTranscriptMessage] { + guard messages.contains(where: { $0.role == .assistant && !$0.toolCalls.isEmpty }) + else { return messages } + + var repaired: [AgentTranscriptMessage] = [] + repaired.reserveCapacity(messages.count) + var index = messages.startIndex + while index < messages.endIndex { + let message = messages[index] + repaired.append(message) + index += 1 + guard message.role == .assistant, !message.toolCalls.isEmpty else { continue } + + var answered: Set = [] + while index < messages.endIndex, messages[index].role == .tool { + if let id = messages[index].toolCallID { answered.insert(id) } + repaired.append(messages[index]) + index += 1 + } + for call in message.toolCalls where !answered.contains(call.id) { + repaired.append(AgentTranscriptMessage( + role: .tool, text: interrupted, toolCallID: call.id, + toolName: call.name, isError: true + )) + } + } + return repaired + } + + private static let interrupted = String( + localized: """ + Termind stopped before this tool call's result was recorded. Whether the action \ + completed is unknown; check the current state before doing it again. + """ + ) +} + // MARK: - Trimming /// Bounds what replaying old tool results costs. diff --git a/Termind/Application/Agent/AgentRuntime.swift b/Termind/Application/Agent/AgentRuntime.swift index 6e3a22f1..ad7de27f 100644 --- a/Termind/Application/Agent/AgentRuntime.swift +++ b/Termind/Application/Agent/AgentRuntime.swift @@ -120,10 +120,13 @@ actor AgentRuntime { state: .running, permissionMode: request.permissionMode, // Applied to incoming history as well as at the model boundary, - // which is what makes it a repair and not just a guard: this - // snapshot is persisted, so filtering here stops a stored orphan - // from being written forward into the next run. - messages: AgentOrphanedToolResultRepair.apply(request.priorMessages) + // which is what makes these repairs and not just guards: this + // snapshot is persisted, so a call left unanswered by a process + // that died — or a result left behind by one — is fixed once here + // rather than re-derived on every future turn of the conversation. + messages: AgentOrphanedToolResultRepair.apply( + AgentUnansweredToolCallRepair.apply(request.priorMessages) + ) ) activeRunID = snapshot.id let runID = snapshot.id diff --git a/Termind/Application/FeatureModels/AssistantLibraryModel.swift b/Termind/Application/FeatureModels/AssistantLibraryModel.swift index a07d8390..067e5005 100644 --- a/Termind/Application/FeatureModels/AssistantLibraryModel.swift +++ b/Termind/Application/FeatureModels/AssistantLibraryModel.swift @@ -66,7 +66,12 @@ final class AssistantConversationStore { let state: AgentToolCard.State if card.result?.isError == true { state = .failed } else if card.result != nil || card.state == "finished" { state = .completed } - else if card.state == "started" { state = .running } + // Not `.running`, which is what this used to restore to. Every + // unfinished run is marked interrupted at launch, so a card + // that was mid-flight when the process died is not running + // now and never will be — it came back as a spinner that + // nothing would ever stop. + else if card.state == "started" { state = .failed } else { state = .proposed } return AgentToolCard( invocation: card.invocation, descriptor: card.descriptor, diff --git a/Termind/Core/Persistence/AgentPersistence.swift b/Termind/Core/Persistence/AgentPersistence.swift index 62fe6814..d014bf85 100644 --- a/Termind/Core/Persistence/AgentPersistence.swift +++ b/Termind/Core/Persistence/AgentPersistence.swift @@ -283,7 +283,13 @@ final class SwiftDataAgentRunRepository: AgentRunPersisting, Sendable { .sorted { $0.createdAt < $1.createdAt } .compactMap { storedToolCard($0, results: results, reviews: reviews) } return StoredAgentConversation( - id: conversation.id, messages: transcript, toolCards: cards, + id: conversation.id, + messages: Self.recovering( + transcript, + results: results.filter { runIDs.contains($0.runID) }, + invocations: invocations.filter { runIDs.contains($0.runID) } + ), + toolCards: cards, lastRunState: state, title: conversation.title ) } @@ -338,6 +344,59 @@ final class SwiftDataAgentRunRepository: AgentRunPersisting, Sendable { try context.save() } + /// Puts back tool results that were recorded but never made it into the + /// transcript. + /// + /// The two are written on different schedules: a result row lands as its + /// tool finishes, while its transcript row waits for the run's next + /// snapshot save. A process killed in between — force quit, crash, the OS + /// reclaiming the app — leaves the call answered in storage and unanswered + /// in history, and an assistant turn with an unanswered call is rejected + /// outright on the next request. + /// + /// `AgentUnansweredToolCallRepair` would invent a placeholder for the same + /// gap, and does for anything still missing after this. Doing it here first + /// is what gives the model the output the tool actually produced instead of + /// being told the outcome is unknown when it is sitting in the database. + static func recovering( + _ transcript: [AgentTranscriptMessage], + results: [PersistentAgentToolResult], + invocations: [PersistentAgentToolInvocation] + ) -> [AgentTranscriptMessage] { + guard transcript.contains(where: { $0.role == .assistant && !$0.toolCalls.isEmpty }) + else { return transcript } + + let names = Dictionary( + invocations.map { ($0.callID, $0.toolName) }, uniquingKeysWith: { first, _ in first } + ) + var recovered: [AgentTranscriptMessage] = [] + recovered.reserveCapacity(transcript.count) + var index = transcript.startIndex + while index < transcript.endIndex { + let message = transcript[index] + recovered.append(message) + index += 1 + guard message.role == .assistant, !message.toolCalls.isEmpty else { continue } + + var answered: Set = [] + while index < transcript.endIndex, transcript[index].role == .tool { + if let id = transcript[index].toolCallID { answered.insert(id) } + recovered.append(transcript[index]) + index += 1 + } + for call in message.toolCalls where !answered.contains(call.id) { + // The last recorded one: a call id is unique to its invocation, + // so more than one row can only mean a rewritten result. + guard let stored = results.last(where: { $0.callID == call.id }) else { continue } + recovered.append(AgentTranscriptMessage( + role: .tool, text: stored.content, toolCallID: call.id, + toolName: names[call.id] ?? call.name, isError: stored.isError + )) + } + } + return recovered + } + private func upsertInvocation( _ invocation: AgentToolInvocation, state: String ) throws { diff --git a/TermindTests/AgentInterruptedRunRecoveryTests.swift b/TermindTests/AgentInterruptedRunRecoveryTests.swift new file mode 100644 index 00000000..02a0a344 --- /dev/null +++ b/TermindTests/AgentInterruptedRunRecoveryTests.swift @@ -0,0 +1,158 @@ +import Foundation +import SwiftData +import Testing + +@testable import Termind + +/// What a force quit during a tool batch leaves behind, and what it takes to +/// make that conversation usable again. +/// +/// The failure it guards is total, not partial: an assistant turn carrying a +/// tool call that nothing answers is rejected outright by every provider — +/// OpenAI says `No tool output found for function call call_…` — so the +/// conversation cannot be continued at all, only deleted. +@Suite +struct AgentInterruptedRunRecoveryTests { + private func assistant(calling ids: [String]) -> AgentTranscriptMessage { + AgentTranscriptMessage( + role: .assistant, text: "Checking both.", + toolCalls: ids.map { AgentToolCall(id: $0, name: "ssh_exec", arguments: .object([:])) } + ) + } + + private func answer(_ id: String, _ text: String) -> AgentTranscriptMessage { + AgentTranscriptMessage(role: .tool, text: text, toolCallID: id, toolName: "ssh_exec") + } + + @Test + func everyToolCallEndsUpAnsweredAtTheModelBoundary() { + let messages = [ + AgentTranscriptMessage(role: .user, text: "check both hosts"), + assistant(calling: ["call-a", "call-b"]), + answer("call-a", "up 3 days"), + ] + + let sent = AgentContextPipeline.chat(sessionContext: nil, before: nil)( + AgentModelContext(systemPrompt: "s", messages: messages, tools: []) + ).messages + + let answered = Set(sent.compactMap(\.toolCallID)) + #expect(answered == ["call-a", "call-b"]) + // Placed against the call it answers, not appended to the end. + #expect(sent.map(\.role) == [.user, .assistant, .tool, .tool]) + // The recovered output is untouched; only the missing one is invented. + #expect(sent[2].text == "up 3 days") + #expect(sent[3].isError) + } + + /// It must not claim the action did not happen. The process died after the + /// command was sent, so whether the remote side ran it is genuinely + /// unknown, and a model told "this never ran" will cheerfully run it again. + @Test + func theSynthesizedAnswerDoesNotClaimTheToolNeverRan() { + let repaired = AgentUnansweredToolCallRepair.apply([assistant(calling: ["call-a"])]) + + let text = try! #require(repaired.last?.text) + #expect(text.contains("unknown")) + #expect(!text.lowercased().contains("never ran")) + } + + @Test + func repairingIsIdempotentAndLeavesCompleteTurnsAlone() { + let messages = [assistant(calling: ["call-a"]), answer("call-a", "ok")] + + let once = AgentUnansweredToolCallRepair.apply(messages) + #expect(once == messages) + #expect(AgentUnansweredToolCallRepair.apply(once) == once) + } + + /// Results are recorded as each tool finishes, but their transcript rows are + /// only written when the run next saves its snapshot. A process killed in + /// between leaves the call answered in storage and unanswered in history — + /// so restore should hand back the real output, not a placeholder for it. + @MainActor + @Test + func restoreRecoversResultsRecordedBeforeTheProcessDied() async throws { + let configuration = ModelConfiguration( + "AgentInterruptedRunRecoveryTests", schema: HostPersistence.schema, + isStoredInMemoryOnly: true + ) + let container = try ModelContainer( + for: HostPersistence.schema, configurations: [configuration] + ) + let repository = SwiftDataAgentRunRepository(context: container.mainContext) + + // The snapshot as it stood when the assistant turn was persisted: two + // calls, no results yet. + let snapshot = AgentRunSnapshot( + conversationID: UUID(), state: .running, permissionMode: .askForApproval, + messages: [ + AgentTranscriptMessage(role: .user, text: "check both hosts"), + assistant(calling: ["call-a", "call-b"]), + ] + ) + try await repository.save(snapshot) + + // One tool finished and was recorded; the other was still running when + // the app was killed, so no snapshot save ever followed. + let finished = AgentToolInvocation( + runID: snapshot.id, + call: AgentToolCall(id: "call-a", name: "ssh_exec", arguments: .object([:])) + ) + try await repository.record(.toolStarted(finished), runID: snapshot.id) + try await repository.record( + .toolFinished(finished, AgentToolResult(callID: "call-a", content: "up 3 days")), + runID: snapshot.id + ) + let running = AgentToolInvocation( + runID: snapshot.id, + call: AgentToolCall(id: "call-b", name: "ssh_exec", arguments: .object([:])) + ) + try await repository.record(.toolStarted(running), runID: snapshot.id) + + // Relaunch. + let reopened = SwiftDataAgentRunRepository(context: container.mainContext) + let restored = try #require(try await reopened.loadConversations().first) + + let recovered = try #require(restored.messages.first { $0.toolCallID == "call-a" }) + #expect(recovered.text == "up 3 days", "the recorded output should come back") + #expect(!recovered.isError) + #expect(restored.lastRunState == .interrupted) + + // The one that never finished is still unanswered here — storage has + // nothing to recover — and is repaired at the model boundary. + let sent = AgentContextPipeline.chat(sessionContext: nil, before: nil)( + AgentModelContext(systemPrompt: "s", messages: restored.messages, tools: []) + ).messages + #expect(Set(sent.compactMap(\.toolCallID)) == ["call-a", "call-b"]) + } + + /// A card left mid-flight by a force quit must not come back spinning. + /// Every run is marked interrupted at launch, so `running` is never a state + /// a restored card can legitimately be in. + @MainActor + @Test + func restoredCardsDoNotComeBackRunning() { + let store = AssistantConversationStore() + let invocation = AgentToolInvocation( + runID: UUID(), + call: AgentToolCall(id: "call-b", name: "ssh_exec", arguments: .object([:])) + ) + store.restore([StoredAgentConversation( + id: UUID(), + messages: [], + toolCards: [StoredAgentToolCard( + invocation: invocation, + descriptor: AgentToolDescriptor( + name: "ssh_exec", summary: "exec", inputSchema: .object([:]), + target: .host, safety: .requiresAuthorization + ), + state: "started", result: nil + )], + lastRunState: .interrupted + )]) + + let card = try! #require(store.conversations.first?.toolCards.first) + #expect(card.state != .running) + } +}