diff --git a/packages/AgentCore/Sources/AgentCore/CacheHints.swift b/packages/AgentCore/Sources/AgentCore/CacheHints.swift index 32718b2..5f18e27 100644 --- a/packages/AgentCore/Sources/AgentCore/CacheHints.swift +++ b/packages/AgentCore/Sources/AgentCore/CacheHints.swift @@ -43,6 +43,20 @@ public struct CacheHints: Sendable, Equatable { /// Once the system prompt grows past the threshold (e.g. memory /// hydration adds context, presence enrichment lengthens the prompt) /// the hint resumes automatically. + /// + /// **Current production state (as of v0.1):** `selfAwarePreamble` is + /// ~700 chars and the orchestrator is constructed with an `.empty` + /// initial context, so the threshold is never crossed on a healthy + /// boot — `eligibleForSystemPrompt` returns `nil` on every turn and + /// the prompt cache is effectively dormant. This is intentional + /// cost-acceptance: padding the preamble to ≥4096 chars would + /// require real prompt-engineering work and risks bleeding + /// irrelevant context into the model. Cache hits resume naturally + /// once Memory M-4 (v0.8 milestone) lands `searchFacts`-driven + /// hydration that grows the prompt past 1024 tokens organically; + /// see #23 for the audit context. Until then, every turn pays full + /// input-token cost — a ~30–80% cost reduction is on the table once + /// M-4 ships. public static func eligibleForSystemPrompt( _ systemPromptText: String, ttl: CacheTTL = .extended1h diff --git a/packages/AgentCore/Sources/AgentOrchestrator/AgentOrchestrator.swift b/packages/AgentCore/Sources/AgentOrchestrator/AgentOrchestrator.swift index ef9b006..ba8c912 100644 --- a/packages/AgentCore/Sources/AgentOrchestrator/AgentOrchestrator.swift +++ b/packages/AgentCore/Sources/AgentOrchestrator/AgentOrchestrator.swift @@ -508,6 +508,15 @@ public actor AgentOrchestrator { // for the current pass. Reset on each pass restart (initial entry + // T1→T2 escalation re-enter via `continue outer`). var assistantTextSoFar: String = "" + // #24: text accumulated since the most recent toolUse append (or + // since the start of the round if no toolUse yet). Distinct from + // `assistantTextSoFar` which stays cumulative for D-02 vision + // routing on endTurn. This buffer feeds the assistant message + // history alongside each toolUse block so the model sees its own + // narration ("Let me check..." → tool_use(get_time)) on subsequent + // round-trips; previously the text was dropped entirely from + // history and the model would deny ever saying it. + var assistantTextSinceLastFlush: String = "" // D-02: escalation gets ONE shot. The second low-confidence outcome // stays on T1 (the user got the response we have). This mirrors the // AGENT-09 retry budget but is independent of it. @@ -575,6 +584,10 @@ public actor AgentOrchestrator { // can hand it to VisionRouter.evaluatePostResponse. // Reset on `continue outer` from the escalation arm. assistantTextSoFar += s + // #24: also accumulate into the per-tool-use flush + // buffer so the next toolUseRequested attaches the + // preceding narration to its assistant message. + assistantTextSinceLastFlush += s await replayLog.record(.textDelta(s), for: currentTurnId) await events.send(.tokenDelta(turnId: currentTurnId, text: s)) @@ -601,9 +614,29 @@ public actor AgentOrchestrator { // history so providers that require the original // tool_use round-trip (Anthropic) can resolve the // tool_use_id when the next tool_result lands. - messages.append(LLMMessage(role: .assistant, content: [ - .toolUse(id: req.id, name: req.name, argsJSON: req.argsJSON), - ])) + // + // #24: bundle any text the model emitted since the + // last flush — Opus 4.7 commonly narrates ("Let me + // check the time...") before calling a tool. Without + // this the text was dropped from history and the + // model would deny ever saying it on a follow-up. + // Trim whitespace-only text to avoid emitting empty + // text blocks (which Anthropic rejects with a 400). + var assistantContent: [LLMMessage.ContentBlock] = [] + let pendingText = assistantTextSinceLastFlush + .trimmingCharacters(in: .whitespacesAndNewlines) + if !pendingText.isEmpty { + assistantContent.append(.text(pendingText)) + } + assistantContent.append( + .toolUse(id: req.id, name: req.name, argsJSON: req.argsJSON) + ) + messages.append(LLMMessage(role: .assistant, content: assistantContent)) + // Consumed — the next toolUseRequested in the same + // round (rare but possible) gets only the text that + // came BETWEEN the two tool_use blocks. D-02's + // cumulative `assistantTextSoFar` is untouched. + assistantTextSinceLastFlush = "" do { let resultData = try await toolDispatcher.dispatch(toolUse: req) diff --git a/packages/AgentCore/Tests/AgentOrchestratorTests/AssistantTextWithToolUseTests.swift b/packages/AgentCore/Tests/AgentOrchestratorTests/AssistantTextWithToolUseTests.swift new file mode 100644 index 0000000..83dd575 --- /dev/null +++ b/packages/AgentCore/Tests/AgentOrchestratorTests/AssistantTextWithToolUseTests.swift @@ -0,0 +1,210 @@ +import XCTest +import AgentCore +import Config +import Replay +@testable import AgentOrchestrator + +/// **#24 — assistant text blocks alongside tool_use must not be dropped.** +/// +/// Opus 4.7 frequently narrates before invoking a tool ("Let me check the +/// time for you..." → `tool_use(get_time)`). Before this fix, the +/// orchestrator appended only the `.toolUse` block to `messages`, so on +/// the next round-trip the model had no record of saying anything — a +/// user follow-up like "what did you say before checking the time?" +/// would get a denial. +/// +/// These tests assert the assistant message at the round-trip boundary +/// contains BOTH the preceding text and the tool_use block. D-02's +/// cumulative `assistantTextSoFar` is preserved separately; only the +/// per-flush buffer is consumed. +@MainActor +final class AssistantTextWithToolUseTests: XCTestCase { + + var tempHome: TempReplayHome! + + override func setUp() async throws { + try await super.setUp() + tempHome = TempReplayHome.make() + } + + override func tearDown() async throws { + tempHome?.cleanup() + try await super.tearDown() + } + + private func buildOrchestrator( + provider: MockLLMProvider, + dispatcher: any ToolDispatcher + ) async throws -> AgentOrchestrator { + let replay = try ReplayLog(databaseURL: tempHome.dbURL) + let session = try await replay.beginSession(appVersion: "test", buildSHA: "deadbeef") + return AgentOrchestrator( + configStore: makeConfigStore(), + providerFactory: { _ in provider }, + toolDispatcher: dispatcher, + replayLog: replay, + sessionId: session, + systemPrompt: "you are jarvis", + availableTools: [] + ) + } + + private func collectUntilIdle(orch: AgentOrchestrator, timeout: TimeInterval = 5.0) async { + let deadline = Date().addingTimeInterval(timeout) + for await event in orch.events { + if case .stateChange(.idle) = event { return } + if Date() > deadline { return } + } + } + + // MARK: - ATU-1 + + /// Single text-then-tool sequence: model emits "Let me check..." then + /// `tool_use(get_time)`. The assistant message in the second provider + /// call must contain a `.text` block AND a `.toolUse` block. + func test_ATU1_textBeforeToolUseBundledIntoSameAssistantMessage() async throws { + let toolReq = ToolUseRequest(id: "tu1", name: "get_time", argsJSON: Data("{}".utf8)) + let mock = MockLLMProvider(scripts: [ + .init(events: [ + .messageStart(LLMMessageStart(messageId: "m1", model: "x", usagePrefix: nil)), + .textDelta("Let me check the time for you... "), + .toolUseRequested(toolReq), + .stopReason(.toolUse), + .messageStop, + ]), + .init(events: [ + .messageStart(LLMMessageStart(messageId: "m2", model: "x", usagePrefix: nil)), + .textDelta("It's 12:34."), + .stopReason(.endTurn), + .messageStop, + ]), + ]) + let dispatcher = StubToolDispatcher(dispatch: { _ in Data("12:34".utf8) }) + + let orch = try await buildOrchestrator(provider: mock, dispatcher: dispatcher) + _ = await orch.submit(.text("what time")) + await collectUntilIdle(orch: orch) + + let calls = await mock.getRecordedCalls() + XCTAssertEqual(calls.count, 2, "expected 2 provider calls (tool-use loop)") + + // Find the assistant message containing the tool_use block in the + // second call's history. + let secondCall = calls[1].messages + guard let assistantWithTool = secondCall.first(where: { msg in + msg.role == .assistant && msg.content.contains(where: { block in + if case .toolUse(id: "tu1", _, _) = block { return true } + return false + }) + }) else { + return XCTFail("no assistant message with the tool_use block found in second call") + } + + // Block ordering: text first, then tool_use. + let blockKinds = assistantWithTool.content.map { block -> String in + switch block { + case .text: return "text" + case .toolUse: return "toolUse" + case .toolResult: return "toolResult" + } + } + XCTAssertEqual(blockKinds, ["text", "toolUse"], + "assistant block sequence must be [text, toolUse] — got \(blockKinds)") + + // The text content matches what the model streamed (trimmed). + if case .text(let txt) = assistantWithTool.content[0] { + XCTAssertEqual(txt, "Let me check the time for you...", + "text block must contain the streamed narration (trimmed)") + } else { + XCTFail("expected first block to be .text") + } + } + + // MARK: - ATU-2 + + /// If the model emits a tool_use with NO preceding text (Opus + /// occasionally does this — straight to tool), the assistant message + /// must contain ONLY the toolUse block (no empty `.text("")` block, + /// which Anthropic rejects with a 400). + func test_ATU2_noPrecedingTextOnlyToolUseBlock() async throws { + let toolReq = ToolUseRequest(id: "tu2", name: "get_time", argsJSON: Data("{}".utf8)) + let mock = MockLLMProvider(scripts: [ + .init(events: [ + .messageStart(LLMMessageStart(messageId: "m1", model: "x", usagePrefix: nil)), + .toolUseRequested(toolReq), + .stopReason(.toolUse), + .messageStop, + ]), + .init(events: [ + .messageStart(LLMMessageStart(messageId: "m2", model: "x", usagePrefix: nil)), + .textDelta("done"), + .stopReason(.endTurn), + .messageStop, + ]), + ]) + let dispatcher = StubToolDispatcher(dispatch: { _ in Data("12:34".utf8) }) + + let orch = try await buildOrchestrator(provider: mock, dispatcher: dispatcher) + _ = await orch.submit(.text("time")) + await collectUntilIdle(orch: orch) + + let calls = await mock.getRecordedCalls() + guard let assistantWithTool = calls[1].messages.first(where: { msg in + msg.role == .assistant && msg.content.contains(where: { block in + if case .toolUse(id: "tu2", _, _) = block { return true } + return false + }) + }) else { + return XCTFail("expected assistant message with tool_use block") + } + + XCTAssertEqual(assistantWithTool.content.count, 1, + "no preceding text — assistant message must contain only the toolUse block, got \(assistantWithTool.content.count) blocks") + if case .toolUse = assistantWithTool.content[0] { + // ok + } else { + XCTFail("expected sole content block to be .toolUse") + } + } + + // MARK: - ATU-3 + + /// Whitespace-only preceding text must NOT be emitted as a `.text` + /// block (Anthropic rejects empty/whitespace-only text blocks). The + /// implementation trims whitespace before testing for emptiness. + func test_ATU3_whitespaceOnlyTextNotEmitted() async throws { + let toolReq = ToolUseRequest(id: "tu3", name: "get_time", argsJSON: Data("{}".utf8)) + let mock = MockLLMProvider(scripts: [ + .init(events: [ + .messageStart(LLMMessageStart(messageId: "m1", model: "x", usagePrefix: nil)), + .textDelta(" \n\t "), // whitespace only + .toolUseRequested(toolReq), + .stopReason(.toolUse), + .messageStop, + ]), + .init(events: [ + .messageStart(LLMMessageStart(messageId: "m2", model: "x", usagePrefix: nil)), + .stopReason(.endTurn), + .messageStop, + ]), + ]) + let dispatcher = StubToolDispatcher(dispatch: { _ in Data("ok".utf8) }) + + let orch = try await buildOrchestrator(provider: mock, dispatcher: dispatcher) + _ = await orch.submit(.text("hi")) + await collectUntilIdle(orch: orch) + + let calls = await mock.getRecordedCalls() + guard let assistantWithTool = calls[1].messages.first(where: { msg in + msg.role == .assistant && msg.content.contains(where: { block in + if case .toolUse(id: "tu3", _, _) = block { return true } + return false + }) + }) else { + return XCTFail("expected assistant message with tool_use block") + } + + XCTAssertEqual(assistantWithTool.content.count, 1, + "whitespace-only preceding text must be dropped, leaving only the toolUse block") + } +}