From 8e3ec4163a0225f59780f45e156120f407b0ab11 Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Sun, 5 Jul 2026 17:39:13 +0900 Subject: [PATCH 01/18] fix(mcp): return complete review results --- .gitignore | 1 + Package.resolved | 10 +- Package.swift | 15 ++- README.md | 20 +++ .../AppServerCodexReviewBackend.swift | 6 +- .../Store/CodexReviewStoreReviews.swift | 6 +- .../CodexReviewMCPToolRequests.swift | 11 +- .../CodexReviewMCPToolResults.swift | 2 +- .../CodexReviewMCPToolSchemas.swift | 36 +++--- .../ReviewMCPLogProjection.swift | 2 +- .../ReviewRunIDArgument.swift | 43 +++++++ .../AppServerClientTests.swift | 118 +++++++++++++++--- .../CodexReviewMCPHTTPServerTests.swift | 110 ++++++++++++++-- .../xcshareddata/swiftpm/Package.resolved | 10 +- 14 files changed, 304 insertions(+), 86 deletions(-) create mode 100644 Sources/CodexReviewMCPServer/ReviewRunIDArgument.swift diff --git a/.gitignore b/.gitignore index 2da8eaa9..86e0893f 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,4 @@ DerivedData/ .swiftpm/xcode/xcuserdata/ .netrc *.profraw +/dependencies diff --git a/Package.resolved b/Package.resolved index 9a802ed2..33a2e255 100644 --- a/Package.resolved +++ b/Package.resolved @@ -1,14 +1,6 @@ { - "originHash" : "711aa87f833d42050080f043e999477df164429e02013f117e52aea172c59432", + "originHash" : "f841cc4390200080cc2f30006b6fa4aaefc669c3275d211221033777fe8c42e5", "pins" : [ - { - "identity" : "codexkit", - "kind" : "remoteSourceControl", - "location" : "https://github.com/lynnswap/CodexKit.git", - "state" : { - "revision" : "d2a694d7f633c1f01d4d260585863c0a3eae21af" - } - }, { "identity" : "eventsource", "kind" : "remoteSourceControl", diff --git a/Package.swift b/Package.swift index 39c8d66f..38844d3d 100644 --- a/Package.swift +++ b/Package.swift @@ -1,7 +1,18 @@ // swift-tools-version: 6.3 +import Foundation import PackageDescription +let packageDirectory = URL(fileURLWithPath: #filePath) + .deletingLastPathComponent() +let localCodexKitPath = packageDirectory + .appendingPathComponent("dependencies/CodexKit", isDirectory: true) + .path +let codexKitDependency: Package.Dependency = + FileManager.default.fileExists(atPath: "\(localCodexKitPath)/Package.swift") + ? .package(path: localCodexKitPath) + : .package(url: "https://github.com/lynnswap/CodexKit.git", branch: "main") + let package = Package( name: "CodexReviewKit", platforms: [ @@ -33,9 +44,7 @@ let package = Package( .package(url: "https://github.com/modelcontextprotocol/swift-sdk.git", exact: "0.12.1"), .package(url: "https://github.com/apple/swift-nio.git", from: "2.97.1"), .package(url: "https://github.com/lynnswap/ObservationBridge.git", .upToNextMinor(from: "0.12.0")), - // CodexKit has no tagged releases yet; pin the reviewed native-login - // and final review response contract revision. - .package(url: "https://github.com/lynnswap/CodexKit.git", revision: "d2a694d7f633c1f01d4d260585863c0a3eae21af"), + codexKitDependency, ], targets: [ .target( diff --git a/README.md b/README.md index 0b6a21da..cee65324 100644 --- a/README.md +++ b/README.md @@ -100,6 +100,26 @@ This Claude Code setting is process-wide. It is not scoped to the `codex_review` MCP server, so the same idle timeout applies to all MCP tools used by that Claude Code session. +## Local CodexKit Development + +`Package.swift` uses a local `dependencies/CodexKit` checkout when that +directory contains a `Package.swift`. If the local checkout is absent, SwiftPM +resolves `CodexKit` from the remote `main` branch. + +```bash +mkdir -p dependencies +git clone git@github.com:lynnswap/CodexKit.git dependencies/CodexKit +swift test --build-system swiftbuild --no-parallel +``` + +After creating or removing `dependencies/CodexKit`, run SwiftPM with manifest +caching disabled once if resolution still points at the previous dependency +kind: + +```bash +swift package --manifest-cache none resolve +``` + ## More Detail - [Architecture](Docs/architecture.md): package boundaries, runtime flow, and diff --git a/Sources/CodexReviewAppServer/AppServerCodexReviewBackend.swift b/Sources/CodexReviewAppServer/AppServerCodexReviewBackend.swift index 8f720dd9..b2039272 100644 --- a/Sources/CodexReviewAppServer/AppServerCodexReviewBackend.swift +++ b/Sources/CodexReviewAppServer/AppServerCodexReviewBackend.swift @@ -597,10 +597,8 @@ private enum AppServerTypedReviewEventAdapter { message: response.status?.rawValue ?? "Failed." ) } - guard let finalReview = response.finalAnswer?.nilIfEmpty - ?? response.transcript.finalAnswer?.nilIfEmpty - else { - return [.failed("Review completed without a final response.")] + guard let finalReview = response.transcript.reviewOutputText?.nilIfEmpty else { + return [.failed("Review completed without review output.")] } return [ .completed(finalReview: finalReview) diff --git a/Sources/CodexReviewKit/Store/CodexReviewStoreReviews.swift b/Sources/CodexReviewKit/Store/CodexReviewStoreReviews.swift index 3763bc68..5929fffb 100644 --- a/Sources/CodexReviewKit/Store/CodexReviewStoreReviews.swift +++ b/Sources/CodexReviewKit/Store/CodexReviewStoreReviews.swift @@ -685,7 +685,7 @@ extension CodexReviewStore { if completePendingCancellationIfNeeded(for: runRecord) { return recoveryState.currentRun } - markReviewFailed(runRecord, message: "Review completed without a final response.") + markReviewFailed(runRecord, message: "Review completed without review output.") } return recoveryState.currentRun } @@ -706,7 +706,7 @@ extension CodexReviewStore { if completePendingCancellationIfNeeded(for: runRecord) { return true } - markReviewFailed(runRecord, message: "Review completed without a final response.") + markReviewFailed(runRecord, message: "Review completed without review output.") } return true } @@ -858,7 +858,7 @@ extension CodexReviewStore { return } guard let finalReview = finalReview?.nilIfEmpty else { - markReviewFailed(runRecord, message: "Review completed without a final response.") + markReviewFailed(runRecord, message: "Review completed without review output.") return } let endedAt = clock.now() diff --git a/Sources/CodexReviewMCPServer/CodexReviewMCPToolRequests.swift b/Sources/CodexReviewMCPServer/CodexReviewMCPToolRequests.swift index f3c45428..32e991ad 100644 --- a/Sources/CodexReviewMCPServer/CodexReviewMCPToolRequests.swift +++ b/Sources/CodexReviewMCPServer/CodexReviewMCPToolRequests.swift @@ -37,7 +37,7 @@ func toolRequest( limit: arguments["limit"]?.intValue ) case .reviewCancel: - let runID = optionalRunID(in: arguments) + let runID = try ReviewRunIDArgument.optionalValue(in: arguments) let sessionID = sessionID( in: arguments, defaultSessionID: defaultSessionID, @@ -63,15 +63,8 @@ func sessionID( defaultSessionID ?? arguments["sessionID"]?.stringValue ?? fallback } -func optionalRunID(in arguments: [String: Value]) -> String? { - arguments["runID"]?.stringValue?.nilIfEmpty ?? arguments["runId"]?.stringValue?.nilIfEmpty -} - func requiredRunID(in arguments: [String: Value]) throws -> String { - guard let runID = optionalRunID(in: arguments) else { - throw MCPProtocolServerError.missingArgument("runID/runId") - } - return runID + try ReviewRunIDArgument.requiredValue(in: arguments) } func reviewTarget(from object: [String: Value]) throws -> CodexReviewAPI.Target { diff --git a/Sources/CodexReviewMCPServer/CodexReviewMCPToolResults.swift b/Sources/CodexReviewMCPServer/CodexReviewMCPToolResults.swift index 6b5d4d02..7d83fd74 100644 --- a/Sources/CodexReviewMCPServer/CodexReviewMCPToolResults.swift +++ b/Sources/CodexReviewMCPServer/CodexReviewMCPToolResults.swift @@ -63,7 +63,7 @@ private extension CodexReviewAPI.Read.Result { func structuredContentForStartOrAwait(log: ReviewMCPLogProjection) -> Value { structuredContent( - includeDetails: false, + includeDetails: core.lifecycle.status.isTerminal, includeNextAction: core.lifecycle.status.isTerminal == false, log: log ) diff --git a/Sources/CodexReviewMCPServer/CodexReviewMCPToolSchemas.swift b/Sources/CodexReviewMCPServer/CodexReviewMCPToolSchemas.swift index 05306d91..a72a8a39 100644 --- a/Sources/CodexReviewMCPServer/CodexReviewMCPToolSchemas.swift +++ b/Sources/CodexReviewMCPServer/CodexReviewMCPToolSchemas.swift @@ -26,28 +26,18 @@ func schema(for tool: CodexReviewMCP.Tool.Name) -> Value { case .reviewAwait: .object([ "type": .string("object"), - "properties": .object([ + "properties": .object(reviewRunIDProperties(extra: [ "sessionID": .object(["type": .string("string")]), - "runID": .object(["type": .string("string")]), - "runId": .object(["type": .string("string")]), - ]), - "anyOf": .array([ - .object(["required": .array([.string("runId")])]), - .object(["required": .array([.string("runID")])]), - ]), + ])), + "anyOf": ReviewRunIDArgument.requiredAnyOf(), ]) case .reviewRead: .object([ "type": .string("object"), - "properties": .object([ + "properties": .object(reviewRunIDProperties(extra: [ "sessionID": .object(["type": .string("string")]), - "runID": .object(["type": .string("string")]), - "runId": .object(["type": .string("string")]), - ]), - "anyOf": .array([ - .object(["required": .array([.string("runId")])]), - .object(["required": .array([.string("runID")])]), - ]), + ])), + "anyOf": ReviewRunIDArgument.requiredAnyOf(), ]) case .reviewList: .object([ @@ -62,14 +52,20 @@ func schema(for tool: CodexReviewMCP.Tool.Name) -> Value { case .reviewCancel: .object([ "type": .string("object"), - "properties": .object([ + "properties": .object(reviewRunIDProperties(extra: [ "sessionID": .object(["type": .string("string")]), - "runID": .object(["type": .string("string")]), - "runId": .object(["type": .string("string")]), "cwd": .object(["type": .string("string")]), "statuses": .object(["type": .string("array")]), "reason": .object(["type": .string("string")]), - ]), + ])), ]) } } + +private func reviewRunIDProperties(extra: [String: Value]) -> [String: Value] { + var properties = ReviewRunIDArgument.properties() + for (name, schema) in extra { + properties[name] = schema + } + return properties +} diff --git a/Sources/CodexReviewMCPServer/ReviewMCPLogProjection.swift b/Sources/CodexReviewMCPServer/ReviewMCPLogProjection.swift index acf10b01..3e1682c8 100644 --- a/Sources/CodexReviewMCPServer/ReviewMCPLogProjection.swift +++ b/Sources/CodexReviewMCPServer/ReviewMCPLogProjection.swift @@ -90,7 +90,7 @@ package struct ReviewMCPLogProjection: Sendable, Equatable { self.finalLifecycleMessage = status.isTerminal ? lifecycleMessage : nil self.finalResult = status == .succeeded - ? projectedItems.lastAssistantMessageText + ? result.core.finalReview ?? projectedItems.lastAssistantMessageText : nil } } diff --git a/Sources/CodexReviewMCPServer/ReviewRunIDArgument.swift b/Sources/CodexReviewMCPServer/ReviewRunIDArgument.swift new file mode 100644 index 00000000..30a4dd1c --- /dev/null +++ b/Sources/CodexReviewMCPServer/ReviewRunIDArgument.swift @@ -0,0 +1,43 @@ +import MCP + +enum ReviewRunIDArgument { + static let acceptedNames = ["runId", "runID", "jobId", "jobID"] + static let requiredDescription = acceptedNames.joined(separator: "/") + + static func properties() -> [String: Value] { + Dictionary(uniqueKeysWithValues: acceptedNames.map { name in + (name, Value.object(["type": .string("string")])) + }) + } + + static func requiredAnyOf() -> Value { + .array(acceptedNames.map { name in + .object(["required": .array([.string(name)])]) + }) + } + + static func optionalValue(in arguments: [String: Value]) throws -> String? { + let provided = acceptedNames.compactMap { name -> (name: String, value: String)? in + guard let value = arguments[name]?.stringValue?.nilIfEmpty else { + return nil + } + return (name, value) + } + guard let first = provided.first else { + return nil + } + if let conflict = provided.first(where: { $0.value != first.value }) { + throw MCPProtocolServerError.invalidArgument( + "Conflicting run identifier arguments: \(first.name) and \(conflict.name)." + ) + } + return first.value + } + + static func requiredValue(in arguments: [String: Value]) throws -> String { + guard let runID = try optionalValue(in: arguments) else { + throw MCPProtocolServerError.missingArgument(requiredDescription) + } + return runID + } +} diff --git a/Tests/CodexReviewAppServerTests/AppServerClientTests.swift b/Tests/CodexReviewAppServerTests/AppServerClientTests.swift index d1a8afbf..fc1b28c7 100644 --- a/Tests/CodexReviewAppServerTests/AppServerClientTests.swift +++ b/Tests/CodexReviewAppServerTests/AppServerClientTests.swift @@ -80,15 +80,6 @@ struct AppServerClientTests { ) ) ) - try await runtime.transport.emitServerNotification( - method: "item/agentMessage/delta", - params: TestDeltaNotification( - threadID: "review-thread", - turnID: "turn-1", - itemID: "msg-1", - delta: "Looks good." - ) - ) try await runtime.transport.emitServerNotification( method: "turn/completed", params: TestTurnNotification( @@ -97,7 +88,7 @@ struct AppServerClientTests { id: "turn-1", status: "completed", items: [ - .finalAnswer(id: "msg-1", text: "Looks good.") + .exitedReviewMode(id: "review-output", review: "Looks good.") ] ) ) @@ -108,7 +99,7 @@ struct AppServerClientTests { #expect(try await iterator.next() == .completed(finalReview: "Looks good.")) } - @Test func backendCompletesReviewFromFinalAnswerDeltaWhenTerminalPayloadIsSparse() async throws { + @Test func backendCompletesReviewFromExitedReviewModeWhenTerminalPayloadIsSparse() async throws { let runtime = try await CodexAppServerTestRuntime.start() try await runtime.transport.enqueueThreadStart(threadID: "thread-1", model: "gpt-5") try await runtime.transport.enqueueReviewStart(turnID: "turn-1", reviewThreadID: "review-thread") @@ -119,10 +110,44 @@ struct AppServerClientTests { var iterator = eventSequence(attempt).makeAsyncIterator() try await runtime.transport.emitServerNotification( - method: "item/agentMessage/delta", - params: TestDeltaNotification( + method: "item/completed", + params: TestItemNotification( threadID: "review-thread", turnID: "turn-1", + item: .init( + type: "exitedReviewMode", + id: "review-output", + review: "No issues found." + ) + ) + ) + try await runtime.transport.emitServerNotification( + method: "turn/completed", + params: TestTurnNotification( + threadID: "review-thread", + turn: .init(id: "turn-1", status: "completed") + ) + ) + + #expect( + try await iterator.next() == .started(turnID: "turn-1", reviewThreadID: "review-thread", model: "gpt-5")) + #expect(try await iterator.next() == .completed(finalReview: "No issues found.")) + } + + @Test func backendDoesNotPromoteThreadScopedFinalAnswerDeltaWithoutTurnID() async throws { + let runtime = try await CodexAppServerTestRuntime.start() + try await runtime.transport.enqueueThreadStart(threadID: "thread-1", model: "gpt-5") + try await runtime.transport.enqueueReviewStart(turnID: "turn-1", reviewThreadID: "review-thread") + await runtime.transport.waitForNotificationStreamCount(1) + let backend = AppServerCodexReviewBackend(appServer: runtime.server) + + let attempt = try await backend.startReview(makeReviewStart(target: .baseBranch("main"))) + var iterator = eventSequence(attempt).makeAsyncIterator() + + try await runtime.transport.emitServerNotification( + method: "item/agentMessage/delta", + params: TestDeltaWithoutTurnIDNotification( + threadID: "review-thread", itemID: "msg-1", delta: "Looks good.", phase: "final_answer" @@ -138,10 +163,37 @@ struct AppServerClientTests { #expect( try await iterator.next() == .started(turnID: "turn-1", reviewThreadID: "review-thread", model: "gpt-5")) - #expect(try await iterator.next() == .completed(finalReview: "Looks good.")) + #expect(try await iterator.next() == .failed("Review completed without review output.")) + } + + @Test func backendDoesNotPromoteThreadlessAgentMessageToInlineReview() async throws { + let runtime = try await CodexAppServerTestRuntime.start() + try await runtime.transport.enqueueThreadStart(threadID: "thread-1", model: "gpt-5") + try await runtime.transport.enqueueReviewStart(turnID: "turn-1", reviewThreadID: "thread-1") + await runtime.transport.waitForNotificationStreamCount(1) + let backend = AppServerCodexReviewBackend(appServer: runtime.server) + + let attempt = try await backend.startReview(makeReviewStart(target: .baseBranch("main"))) + var iterator = eventSequence(attempt).makeAsyncIterator() + + try await runtime.transport.emitServerNotification( + method: "agent/message", + params: TestAgentMessageNotification(message: "Looks good.") + ) + try await runtime.transport.emitServerNotification( + method: "turn/completed", + params: TestTurnNotification( + threadID: "thread-1", + turn: .init(id: "turn-1", status: "completed") + ) + ) + + #expect( + try await iterator.next() == .started(turnID: "turn-1", reviewThreadID: "thread-1", model: "gpt-5")) + #expect(try await iterator.next() == .failed("Review completed without review output.")) } - @Test func backendFailsCompletedReviewWithoutFinalAnswer() async throws { + @Test func backendFailsCompletedReviewWithoutReviewOutput() async throws { let runtime = try await CodexAppServerTestRuntime.start() try await runtime.transport.enqueueThreadStart(threadID: "thread-1", model: "gpt-5") try await runtime.transport.enqueueReviewStart(turnID: "turn-1", reviewThreadID: "review-thread") @@ -161,7 +213,7 @@ struct AppServerClientTests { #expect( try await iterator.next() == .started(turnID: "turn-1", reviewThreadID: "review-thread", model: "gpt-5")) - #expect(try await iterator.next() == .failed("Review completed without a final response.")) + #expect(try await iterator.next() == .failed("Review completed without review output.")) } @Test func backendIgnoresAgentMessageDeltasInLifecycleStream() async throws { @@ -202,7 +254,7 @@ struct AppServerClientTests { id: "turn-1", status: "completed", items: [ - .finalAnswer(id: "msg-1", text: "first") + .exitedReviewMode(id: "review-output-1", review: "first") ] ) ) @@ -215,7 +267,7 @@ struct AppServerClientTests { id: "turn-2", status: "completed", items: [ - .finalAnswer(id: "msg-1", text: "second") + .exitedReviewMode(id: "review-output-2", review: "second") ] ) ) @@ -276,7 +328,7 @@ struct AppServerClientTests { id: "turn-1", status: "completed", items: [ - .finalAnswer(id: "msg-1", text: "No issues found.") + .exitedReviewMode(id: "review-output", review: "No issues found.") ] ) ) @@ -660,6 +712,25 @@ private struct TestDeltaNotification: Encodable, Sendable { } } +private struct TestDeltaWithoutTurnIDNotification: Encodable, Sendable { + var threadID: String + var itemID: String + var delta: String + var phase: String? + + enum CodingKeys: String, CodingKey { + case threadID = "threadId" + case itemID = "itemId" + case delta + case phase + } +} + +private struct TestAgentMessageNotification: Encodable, Sendable { + var message: String + var phase: String? = nil +} + private struct TestItemNotification: Encodable, Sendable { var threadID: String var turnID: String @@ -717,4 +788,13 @@ private struct TestItem: Encodable, Sendable { status: "completed" ) } + + static func exitedReviewMode(id: String, review: String) -> TestItem { + .init( + type: "exitedReviewMode", + id: id, + review: review, + status: "completed" + ) + } } diff --git a/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift b/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift index d99e36a7..022a6d57 100644 --- a/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift +++ b/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift @@ -1,11 +1,12 @@ import Darwin import Foundation +import CodexKit import MCP @preconcurrency import NIOCore import Testing @_spi(Testing) @testable import CodexReviewKit import CodexReviewKit -import CodexReviewMCPServer +@testable import CodexReviewMCPServer import CodexReviewTesting @Suite("MCP Streamable HTTP server") @@ -50,11 +51,15 @@ struct CodexReviewMCPHTTPServerTests { let awaitSchema = try #require(reviewAwait["inputSchema"] as? [String: Any]) let awaitProperties = try #require(awaitSchema["properties"] as? [String: Any]) #expect(awaitProperties["runId"] != nil) + #expect(awaitProperties["jobId"] != nil) + #expect(awaitProperties["jobID"] != nil) #expect(awaitProperties["logOffset"] == nil) let awaitAnyOf = try #require(awaitSchema["anyOf"] as? [[String: Any]]) let requiredAliases = awaitAnyOf.compactMap { $0["required"] as? [String] } #expect(requiredAliases.contains(["runId"])) #expect(requiredAliases.contains(["runID"])) + #expect(requiredAliases.contains(["jobId"])) + #expect(requiredAliases.contains(["jobID"])) } } @@ -136,7 +141,31 @@ struct CodexReviewMCPHTTPServerTests { idGenerator: .init(next: { "run-1" }) ) - try await withHTTPServer(store: store) { server in + try await withHTTPServer( + store: store, + logProjectionProvider: { result in + ReviewMCPLogProjection( + result: result, + turnID: "turn-1", + threadItems: [ + .init( + id: "command-1", + kind: .commandExecution, + content: .command(.init(command: "swift test", output: "passed")) + ), + .init( + id: "assistant-1", + kind: .agentMessage, + content: .message(.init( + id: "assistant-1", + role: .assistant, + text: "No issues found." + )) + ), + ] + ) + } + ) { server in let endpoint = await server.url let sessionID = try await initializeSession(endpoint: endpoint) let requestBody = try makeJSONBody([ @@ -180,6 +209,14 @@ struct CodexReviewMCPHTTPServerTests { #expect( resolved.value(for: ["result", "structuredContent", "review", "reviewResult", "state"]) as? String == "noFindings") + let log = try #require( + resolved.value(for: ["result", "structuredContent", "log"]) as? [String: Any]) + let itemsPage = try #require(log["itemsPage"] as? [String: Any]) + #expect(itemsPage["total"] as? Int == 2) + #expect(itemsPage["limit"] as? Int == 100) + #expect(itemsPage["returned"] as? Int == 2) + let items = try #require(log["items"] as? [[String: Any]]) + #expect(items.compactMap { $0["kind"] as? String } == ["commandExecution", "agentMessage"]) let commands = await backend.recordedCommands() #expect( commands.contains( @@ -205,7 +242,23 @@ struct CodexReviewMCPHTTPServerTests { boundedReviewWaitDuration: .milliseconds(50) ) - try await withHTTPServer(store: store, configuration: configuration) { server in + try await withHTTPServer( + store: store, + configuration: configuration, + logProjectionProvider: { result in + ReviewMCPLogProjection( + result: result, + turnID: "turn-1", + threadItems: [ + .init( + id: "command-1", + kind: .commandExecution, + content: .command(.init(command: "git diff", output: "diff")) + ), + ] + ) + } + ) { server in let endpoint = await server.url let sessionID = try await initializeSession(endpoint: endpoint, clientName: "Claude Code") let requestBody = try makeJSONBody([ @@ -268,8 +321,12 @@ struct CodexReviewMCPHTTPServerTests { == "noFindings") #expect( awaited.value(for: ["result", "structuredContent", "log", "finalLifecycleMessage"]) as? String - == nil) - #expect(awaited.value(for: ["result", "structuredContent", "log", "finalResult"]) is NSNull) + == "Review completed.") + #expect( + awaited.value(for: ["result", "structuredContent", "log", "itemsPage", "returned"]) as? Int == 1) + let awaitedItems = try #require( + awaited.value(for: ["result", "structuredContent", "log", "items"]) as? [[String: Any]]) + #expect(awaitedItems.first?["kind"] as? String == "commandExecution") #expect(awaited.value(for: ["result", "structuredContent", "logs"]) == nil) } } @@ -455,7 +512,7 @@ struct CodexReviewMCPHTTPServerTests { "method": "tools/call", "params": [ "name": "review_read", - "arguments": ["runId": "run-in-session"], + "arguments": ["jobId": "run-in-session"], ], ] ) @@ -468,7 +525,7 @@ struct CodexReviewMCPHTTPServerTests { "method": "tools/call", "params": [ "name": "review_read", - "arguments": ["runID": "run-other-session"], + "arguments": ["jobID": "run-other-session"], ], ] ) @@ -486,6 +543,39 @@ struct CodexReviewMCPHTTPServerTests { } } + @Test func streamableHTTPRejectsConflictingRunIdentifierAliases() async throws { + let backend = FakeCodexReviewBackend() + let store = CodexReviewStore.makeTestingStore( + backend: TestingCodexReviewStoreBackend(reviewBackend: backend) + ) + + try await withHTTPServer(store: store) { server in + let sessionID = try await initializeSession(endpoint: await server.url) + let response = try await postJSONRPC( + endpoint: await server.url, + sessionID: sessionID, + body: [ + "jsonrpc": "2.0", + "id": 2, + "method": "tools/call", + "params": [ + "name": "review_read", + "arguments": [ + "runId": "run-1", + "jobId": "run-2", + ], + ], + ] + ) + + #expect(response.value(for: ["result", "isError"]) as? Bool == true) + let errorText = + (response.value(for: ["result", "content"]) as? [[String: Any]])?.first?["text"] + as? String + #expect(errorText == "Conflicting run identifier arguments: runId and jobId.") + } + } + @Test func streamableHTTPReviewReadLeavesLogEmptyWithoutChatProvider() async throws { let backend = FakeCodexReviewBackend() let store = CodexReviewStore.makeTestingStore( @@ -1098,9 +1188,13 @@ struct CodexReviewMCPHTTPServerTests { private func withHTTPServer( store: CodexReviewStore, configuration: CodexReviewMCPHTTPServer.Configuration = .init(port: 0), + logProjectionProvider: ReviewMCPLogProjectionProvider? = nil, operation: (CodexReviewMCPHTTPServer) async throws -> T ) async throws -> T { - let adapter = CodexReviewMCPServer(store: store) + let adapter = CodexReviewMCPServer( + store: store, + logProjectionProvider: logProjectionProvider + ) let server = CodexReviewMCPHTTPServer( adapter: adapter, configuration: configuration diff --git a/Tools/ReviewMonitor/CodexReviewMonitor.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved b/Tools/ReviewMonitor/CodexReviewMonitor.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved index a100014f..e9b8ccf7 100644 --- a/Tools/ReviewMonitor/CodexReviewMonitor.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved +++ b/Tools/ReviewMonitor/CodexReviewMonitor.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved @@ -1,14 +1,6 @@ { - "originHash" : "090f3e640b9c9ff9c1ef794fed79ee1dcdb35f013243ff248225a302aebbbb8a", + "originHash" : "11d1673255c652381957935c0aa0fcc8f6cba52200a3e3e3c7296d58e6207f76", "pins" : [ - { - "identity" : "codexkit", - "kind" : "remoteSourceControl", - "location" : "https://github.com/lynnswap/CodexKit.git", - "state" : { - "revision" : "d2a694d7f633c1f01d4d260585863c0a3eae21af" - } - }, { "identity" : "eventsource", "kind" : "remoteSourceControl", From d7acdc5cbfde58f2fd88768d3170224135904f27 Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Sun, 5 Jul 2026 19:11:09 +0900 Subject: [PATCH 02/18] fix(mcp): omit logs from review start results --- .../CodexReviewMCPToolResults.swift | 15 ++++++++++----- .../CodexReviewMCPHTTPServerTests.swift | 18 ++---------------- 2 files changed, 12 insertions(+), 21 deletions(-) diff --git a/Sources/CodexReviewMCPServer/CodexReviewMCPToolResults.swift b/Sources/CodexReviewMCPServer/CodexReviewMCPToolResults.swift index 7d83fd74..88453755 100644 --- a/Sources/CodexReviewMCPServer/CodexReviewMCPToolResults.swift +++ b/Sources/CodexReviewMCPServer/CodexReviewMCPToolResults.swift @@ -63,7 +63,8 @@ private extension CodexReviewAPI.Read.Result { func structuredContentForStartOrAwait(log: ReviewMCPLogProjection) -> Value { structuredContent( - includeDetails: core.lifecycle.status.isTerminal, + includeLog: false, + includeDetails: false, includeNextAction: core.lifecycle.status.isTerminal == false, log: log ) @@ -71,6 +72,7 @@ private extension CodexReviewAPI.Read.Result { func structuredContentForRead(log: ReviewMCPLogProjection) -> Value { structuredContent( + includeLog: true, includeDetails: true, includeNextAction: false, log: log @@ -78,6 +80,7 @@ private extension CodexReviewAPI.Read.Result { } func structuredContent( + includeLog: Bool, includeDetails: Bool, includeNextAction: Bool, log: ReviewMCPLogProjection @@ -93,10 +96,12 @@ private extension CodexReviewAPI.Read.Result { resolvedFinalReview: log.finalResult?.nilIfEmpty ?? core.finalReview ), ] - object["log"] = - includeDetails - ? log.structuredContentWithItems() - : log.structuredContent() + if includeLog { + object["log"] = + includeDetails + ? log.structuredContentWithItems() + : log.structuredContent() + } if includeNextAction { object["nextAction"] = .object([ "tool": .string(CodexReviewMCP.Tool.Name.reviewAwait.rawValue), diff --git a/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift b/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift index 022a6d57..f3df05cb 100644 --- a/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift +++ b/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift @@ -209,14 +209,7 @@ struct CodexReviewMCPHTTPServerTests { #expect( resolved.value(for: ["result", "structuredContent", "review", "reviewResult", "state"]) as? String == "noFindings") - let log = try #require( - resolved.value(for: ["result", "structuredContent", "log"]) as? [String: Any]) - let itemsPage = try #require(log["itemsPage"] as? [String: Any]) - #expect(itemsPage["total"] as? Int == 2) - #expect(itemsPage["limit"] as? Int == 100) - #expect(itemsPage["returned"] as? Int == 2) - let items = try #require(log["items"] as? [[String: Any]]) - #expect(items.compactMap { $0["kind"] as? String } == ["commandExecution", "agentMessage"]) + #expect(resolved.value(for: ["result", "structuredContent", "log"]) == nil) let commands = await backend.recordedCommands() #expect( commands.contains( @@ -319,14 +312,7 @@ struct CodexReviewMCPHTTPServerTests { #expect( awaited.value(for: ["result", "structuredContent", "review", "reviewResult", "state"]) as? String == "noFindings") - #expect( - awaited.value(for: ["result", "structuredContent", "log", "finalLifecycleMessage"]) as? String - == "Review completed.") - #expect( - awaited.value(for: ["result", "structuredContent", "log", "itemsPage", "returned"]) as? Int == 1) - let awaitedItems = try #require( - awaited.value(for: ["result", "structuredContent", "log", "items"]) as? [[String: Any]]) - #expect(awaitedItems.first?["kind"] as? String == "commandExecution") + #expect(awaited.value(for: ["result", "structuredContent", "log"]) == nil) #expect(awaited.value(for: ["result", "structuredContent", "logs"]) == nil) } } From 45b64f1c97ebd16a5901f7defa233b51318cedf9 Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Sun, 5 Jul 2026 19:25:29 +0900 Subject: [PATCH 03/18] fix(review-log): deduplicate review output messages --- .../ReviewMonitorCodexChatLogProjection.swift | 44 ++++++++++++++++--- .../ReviewMonitorCodexChatDetailTests.swift | 37 ++++++++++++++++ 2 files changed, 76 insertions(+), 5 deletions(-) diff --git a/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift b/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift index 22cdf98d..4e2c79d1 100644 --- a/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift +++ b/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift @@ -91,13 +91,21 @@ struct ReviewMonitorCodexChatLogProjection { let suppressUserMessages = items.contains { item in item.kind == .enteredReviewMode || item.kind == .exitedReviewMode } - let blocks = items.flatMap { - projectedBlocks( - from: $0, + let reviewOutputTexts = Set(items.compactMap(Self.reviewOutputText)) + var emittedReviewOutputTexts = Set() + let blocks = items.flatMap { item in + if let reviewOutputText = Self.reviewOutputText(for: item) { + guard emittedReviewOutputTexts.insert(reviewOutputText).inserted else { + return [] as [ReviewMonitorLogProjectedBlock] + } + } + return projectedBlocks( + from: item, turnStatus: turnStatus, chatCreatedAt: chatCreatedAt, chatUpdatedAt: chatUpdatedAt, - suppressUserMessages: suppressUserMessages + suppressUserMessages: suppressUserMessages, + reviewOutputTexts: reviewOutputTexts ) } guard blocks.isEmpty == false else { @@ -112,13 +120,19 @@ struct ReviewMonitorCodexChatLogProjection { turnStatus: CodexTurnStatus?, chatCreatedAt: Date?, chatUpdatedAt: Date?, - suppressUserMessages: Bool + suppressUserMessages: Bool, + reviewOutputTexts: Set ) -> [ReviewMonitorLogProjectedBlock] { switch item.content { case .message(let message): guard suppressUserMessages == false || message.role != .user else { return [] } + if message.role == .assistant, + reviewOutputTexts.contains(Self.normalizedReviewOutputText(message.text)) + { + return [] + } return [ projectedBlock( item, @@ -234,6 +248,26 @@ struct ReviewMonitorCodexChatLogProjection { } } + private static func reviewOutputText(for item: Item) -> String? { + guard item.kind == .exitedReviewMode else { + return nil + } + switch item.content { + case .message(let message): + return normalizedReviewOutputText(message.text).nilIfEmpty + case .diagnostic(let text), .log(let text): + return normalizedReviewOutputText(text).nilIfEmpty + case .unknown(let raw): + return raw.text.map(normalizedReviewOutputText)?.nilIfEmpty + case .plan, .reasoning, .command, .fileChange, .toolCall, .contextCompaction: + return nil + } + } + + private static func normalizedReviewOutputText(_ text: String) -> String { + text.trimmingCharacters(in: .whitespacesAndNewlines) + } + private func unknownText(title: String, detail: String?) -> String { guard let detail, detail != title else { return title diff --git a/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift b/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift index c0adff1e..633ce378 100644 --- a/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift +++ b/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift @@ -487,6 +487,43 @@ struct ReviewMonitorCodexChatDetailTests { #expect(document?.text == "Review the current code changes.") } + @Test func codexChatLogProjectionDeduplicatesReviewOutputAgentMessage() async throws { + var projection = ReviewMonitorCodexChatLogProjection() + let finalReview = """ + Review comment: + + - [P2] Handle complete agent messages without appending them as deltas. + """ + let turn = CodexTurnSnapshot( + id: CodexTurnID(rawValue: "turn-review"), + status: .completed, + items: [ + .init( + id: "review-output", + kind: .exitedReviewMode, + content: .log(finalReview) + ), + .init( + id: "review-output-agent", + kind: .agentMessage, + content: .message(.init( + id: "review-output-agent", + role: .assistant, + text: finalReview + )) + ), + ] + ) + let document = projection.render( + from: turn, + chatCreatedAt: nil, + chatUpdatedAt: nil + ) + + #expect(document?.text == finalReview) + #expect(document?.blocks.count == 1) + } + @Test func codexChatLogProjectionRendersUserMessageWithoutReviewModeLog() async throws { var projection = ReviewMonitorCodexChatLogProjection() let turn = CodexTurnSnapshot( From 2503b7a8e73e1aa868d8ea5a318c8f710a85857c Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Mon, 6 Jul 2026 00:03:13 +0900 Subject: [PATCH 04/18] fix(mcp): trim review finding payloads --- .../CodexReviewMCPToolResults.swift | 1 - .../CodexReviewMCPHTTPServerTests.swift | 49 +++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/Sources/CodexReviewMCPServer/CodexReviewMCPToolResults.swift b/Sources/CodexReviewMCPServer/CodexReviewMCPToolResults.swift index 88453755..b9b59c84 100644 --- a/Sources/CodexReviewMCPServer/CodexReviewMCPToolResults.swift +++ b/Sources/CodexReviewMCPServer/CodexReviewMCPToolResults.swift @@ -472,7 +472,6 @@ private extension ParsedReviewResult.Finding { "body": .string(body), "priority": priority.map(Value.int) ?? .null, "location": location.map { $0.structuredContent() } ?? .null, - "rawText": .string(rawText), ]) } } diff --git a/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift b/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift index f3df05cb..ab3a179f 100644 --- a/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift +++ b/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift @@ -223,6 +223,55 @@ struct CodexReviewMCPHTTPServerTests { } } + @Test func streamableHTTPOmitsRawFindingTextFromReviewStartResult() async throws { + let backend = FakeCodexReviewBackend() + let store = CodexReviewStore.makeTestingStore( + backend: TestingCodexReviewStoreBackend(reviewBackend: backend), + idGenerator: .init(next: { "run-1" }) + ) + + try await withHTTPServer(store: store) { server in + let endpoint = await server.url + let sessionID = try await initializeSession(endpoint: endpoint) + let requestBody = try makeJSONBody([ + "jsonrpc": "2.0", + "id": 2, + "method": "tools/call", + "params": [ + "name": "review_start", + "arguments": [ + "cwd": "/tmp/project", + "target": ["type": "uncommittedChanges"], + ], + ], + ]) + async let responseData = postJSONRPCData( + endpoint: endpoint, + sessionID: sessionID, + bodyData: requestBody + ) + await backend.yield(.completed(finalReview: """ + Review comment: + + - [P1] Pin CodexKit fallback — Package.swift:14-14 + Fresh clones can resolve a moving dependency and drift from the reviewed API. + """)) + let resolved = try decodeSSEJSON(from: try await responseData) + + #expect( + resolved.value(for: ["result", "structuredContent", "review", "finalReview"]) as? String + != nil) + let findings = try #require( + resolved.value(for: ["result", "structuredContent", "review", "reviewResult", "findings"]) + as? [[String: Any]]) + let finding = try #require(findings.first) + #expect(finding["title"] as? String == "[P1] Pin CodexKit fallback") + #expect(finding["body"] as? String == "Fresh clones can resolve a moving dependency and drift from the reviewed API.") + #expect(finding["location"] != nil) + #expect(finding["rawText"] == nil) + } + } + @Test func streamableHTTPBoundsClaudeReviewStartAndContinuesWithReviewAwait() async throws { let backend = FakeCodexReviewBackend() let store = CodexReviewStore.makeTestingStore( From 65b86df58e78a78bae0982cdd626558871ac0851 Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Mon, 6 Jul 2026 12:01:02 +0900 Subject: [PATCH 05/18] build(package): pin CodexKit fallback revision --- Package.swift | 3 ++- README.md | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/Package.swift b/Package.swift index 38844d3d..7b8d72c1 100644 --- a/Package.swift +++ b/Package.swift @@ -8,10 +8,11 @@ let packageDirectory = URL(fileURLWithPath: #filePath) let localCodexKitPath = packageDirectory .appendingPathComponent("dependencies/CodexKit", isDirectory: true) .path +let codexKitFallbackRevision = "58c2dab605c3dad806e0ac90c6d3d1f67c5fd36d" let codexKitDependency: Package.Dependency = FileManager.default.fileExists(atPath: "\(localCodexKitPath)/Package.swift") ? .package(path: localCodexKitPath) - : .package(url: "https://github.com/lynnswap/CodexKit.git", branch: "main") + : .package(url: "https://github.com/lynnswap/CodexKit.git", revision: codexKitFallbackRevision) let package = Package( name: "CodexReviewKit", diff --git a/README.md b/README.md index cee65324..4a0412cc 100644 --- a/README.md +++ b/README.md @@ -104,7 +104,9 @@ used by that Claude Code session. `Package.swift` uses a local `dependencies/CodexKit` checkout when that directory contains a `Package.swift`. If the local checkout is absent, SwiftPM -resolves `CodexKit` from the remote `main` branch. +resolves `CodexKit` from the pinned fallback revision in `Package.swift`. +Update that revision to a reviewed CodexKit `main` commit whenever +CodexReviewKit adopts new CodexKit APIs. ```bash mkdir -p dependencies From cf78964ef13b41a50616c035046a9fa4a7ab47ea Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Mon, 6 Jul 2026 13:16:56 +0900 Subject: [PATCH 06/18] fix(review): stabilize review result handling --- .../AppServerCodexReviewBackend.swift | 3 +- .../ReviewMonitorCodexChatLogProjection.swift | 42 ++++++++---- .../AppServerClientTests.swift | 1 + .../CodexReviewMCPHTTPServerTests.swift | 1 + .../ReviewMonitorCodexChatDetailTests.swift | 64 +++++++++++++++++++ 5 files changed, 99 insertions(+), 12 deletions(-) diff --git a/Sources/CodexReviewAppServer/AppServerCodexReviewBackend.swift b/Sources/CodexReviewAppServer/AppServerCodexReviewBackend.swift index b2039272..f6db3be1 100644 --- a/Sources/CodexReviewAppServer/AppServerCodexReviewBackend.swift +++ b/Sources/CodexReviewAppServer/AppServerCodexReviewBackend.swift @@ -152,7 +152,8 @@ package actor AppServerCodexReviewBackend: CodexReviewBackend, CodexModelActor { in: workspace, input: CodexReviewInput( target: request.request.target.appServerReviewTarget, - options: reviewThreadOptions(request) + options: reviewThreadOptions(request), + delivery: .inline ) ) .session diff --git a/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift b/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift index 4e2c79d1..7cd61216 100644 --- a/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift +++ b/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift @@ -91,11 +91,11 @@ struct ReviewMonitorCodexChatLogProjection { let suppressUserMessages = items.contains { item in item.kind == .enteredReviewMode || item.kind == .exitedReviewMode } - let reviewOutputTexts = Set(items.compactMap(Self.reviewOutputText)) - var emittedReviewOutputTexts = Set() + let reviewOutputKeys = Set(items.compactMap(Self.reviewOutputKey)) + var emittedReviewOutputKeys = Set() let blocks = items.flatMap { item in - if let reviewOutputText = Self.reviewOutputText(for: item) { - guard emittedReviewOutputTexts.insert(reviewOutputText).inserted else { + if let reviewOutputKey = Self.reviewOutputKey(for: item) { + guard emittedReviewOutputKeys.insert(reviewOutputKey).inserted else { return [] as [ReviewMonitorLogProjectedBlock] } } @@ -105,7 +105,7 @@ struct ReviewMonitorCodexChatLogProjection { chatCreatedAt: chatCreatedAt, chatUpdatedAt: chatUpdatedAt, suppressUserMessages: suppressUserMessages, - reviewOutputTexts: reviewOutputTexts + reviewOutputKeys: reviewOutputKeys ) } guard blocks.isEmpty == false else { @@ -121,7 +121,7 @@ struct ReviewMonitorCodexChatLogProjection { chatCreatedAt: Date?, chatUpdatedAt: Date?, suppressUserMessages: Bool, - reviewOutputTexts: Set + reviewOutputKeys: Set ) -> [ReviewMonitorLogProjectedBlock] { switch item.content { case .message(let message): @@ -129,7 +129,8 @@ struct ReviewMonitorCodexChatLogProjection { return [] } if message.role == .assistant, - reviewOutputTexts.contains(Self.normalizedReviewOutputText(message.text)) + let reviewOutputKey = Self.reviewOutputKey(for: item, text: message.text), + reviewOutputKeys.contains(reviewOutputKey) { return [] } @@ -248,22 +249,36 @@ struct ReviewMonitorCodexChatLogProjection { } } - private static func reviewOutputText(for item: Item) -> String? { + private static func reviewOutputKey(for item: Item) -> ReviewOutputKey? { guard item.kind == .exitedReviewMode else { return nil } switch item.content { case .message(let message): - return normalizedReviewOutputText(message.text).nilIfEmpty + return reviewOutputKey(for: item, text: message.text) case .diagnostic(let text), .log(let text): - return normalizedReviewOutputText(text).nilIfEmpty + return reviewOutputKey(for: item, text: text) case .unknown(let raw): - return raw.text.map(normalizedReviewOutputText)?.nilIfEmpty + return raw.text.flatMap { reviewOutputKey(for: item, text: $0) } case .plan, .reasoning, .command, .fileChange, .toolCall, .contextCompaction: return nil } } + private static func reviewOutputKey( + for item: Item, + text: String + ) -> ReviewOutputKey? { + guard let normalizedText = normalizedReviewOutputText(text).nilIfEmpty else { + return nil + } + return ReviewOutputKey(scopeID: reviewOutputScopeID(for: item), text: normalizedText) + } + + private static func reviewOutputScopeID(for item: Item) -> String { + item.projectionTurnID?.rawValue ?? item.sourceID + } + private static func normalizedReviewOutputText(_ text: String) -> String { text.trimmingCharacters(in: .whitespacesAndNewlines) } @@ -551,6 +566,11 @@ struct ReviewMonitorCodexChatLogProjection { } } +private struct ReviewOutputKey: Hashable { + var scopeID: String + var text: String +} + @MainActor private protocol CodexChatLogProjectionItem { var itemID: String { get } diff --git a/Tests/CodexReviewAppServerTests/AppServerClientTests.swift b/Tests/CodexReviewAppServerTests/AppServerClientTests.swift index fc1b28c7..7d4674ca 100644 --- a/Tests/CodexReviewAppServerTests/AppServerClientTests.swift +++ b/Tests/CodexReviewAppServerTests/AppServerClientTests.swift @@ -52,6 +52,7 @@ struct AppServerClientTests { let reviewStart = try #require(requests.first { $0.method == "review/start" }) let reviewParams = try jsonObject(from: reviewStart.params) #expect(reviewParams["threadId"] as? String == "thread-1") + #expect(reviewParams["delivery"] as? String == "inline") let target = try #require(reviewParams["target"] as? [String: Any]) #expect(target["type"] as? String == "uncommittedChanges") } diff --git a/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift b/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift index ab3a179f..5d711935 100644 --- a/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift +++ b/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift @@ -327,6 +327,7 @@ struct CodexReviewMCPHTTPServerTests { #expect(running.value(for: ["result", "structuredContent", "lifecycle", "status"]) as? String == "running") #expect(running.value(for: ["result", "structuredContent", "logs"]) == nil) #expect(running.value(for: ["result", "structuredContent", "rawLogText"]) == nil) + #expect(running.value(for: ["result", "structuredContent", "log"]) == nil) #expect( running.value(for: ["result", "structuredContent", "nextAction", "tool"]) as? String == "review_await") diff --git a/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift b/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift index 633ce378..4c653d69 100644 --- a/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift +++ b/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift @@ -524,6 +524,70 @@ struct ReviewMonitorCodexChatDetailTests { #expect(document?.blocks.count == 1) } + @Test func codexChatLogProjectionKeepsMatchingReviewOutputsFromDifferentTurns() async throws { + var projection = ReviewMonitorCodexChatLogProjection() + let finalReview = "No issues found." + let snapshot = makeCodexThreadSnapshotForTesting( + chatID: CodexThreadID(rawValue: "review-thread"), + turns: [ + .init( + id: CodexTurnID(rawValue: "turn-first"), + status: .completed, + items: [ + .init( + id: "review-output-first", + kind: .exitedReviewMode, + content: .log(finalReview) + ), + .init( + id: "review-output-agent-first", + kind: .agentMessage, + content: .message(.init( + id: "review-output-agent-first", + role: .assistant, + text: finalReview + )) + ), + ] + ), + .init( + id: CodexTurnID(rawValue: "turn-second"), + status: .completed, + items: [ + .init( + id: "review-output-second", + kind: .exitedReviewMode, + content: .log(finalReview) + ), + .init( + id: "review-output-agent-second", + kind: .agentMessage, + content: .message(.init( + id: "review-output-agent-second", + role: .assistant, + text: finalReview + )) + ), + ] + ), + ] + ) + + let projectedDocument = projection.render( + from: snapshot, + chatCreatedAt: nil, + chatUpdatedAt: nil + ) + let document = try #require(projectedDocument) + + #expect(document.blocks.count == 2) + let documentText = document.text as NSString + let renderedText = document.blocks.map { block in + documentText.substring(with: block.range) + } + #expect(renderedText == [finalReview, finalReview]) + } + @Test func codexChatLogProjectionRendersUserMessageWithoutReviewModeLog() async throws { var projection = ReviewMonitorCodexChatLogProjection() let turn = CodexTurnSnapshot( From 044e7063245227bd85867bf6bad76ad8d946eebe Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Mon, 6 Jul 2026 14:22:32 +0900 Subject: [PATCH 07/18] fix(review-ui): preserve incremental log updates --- ...wMonitorCommandOutputDisplayDocument.swift | 22 +- .../ReviewMonitorLogDocumentProjection.swift | 139 ++++++++--- .../Detail/ReviewMonitorLogProjection.swift | 87 +++++++ .../Detail/ReviewMonitorLogScrollView.swift | 20 +- .../ReviewMonitorCodexChatLogProjection.swift | 91 +++++++ .../ReviewMonitorCodexChatDetailTests.swift | 222 ++++++++++++++++++ Tests/ReviewUITests/ReviewUITests.swift | 88 +++++++ 7 files changed, 607 insertions(+), 62 deletions(-) diff --git a/Sources/ReviewChatLogUI/Detail/ReviewMonitorCommandOutputDisplayDocument.swift b/Sources/ReviewChatLogUI/Detail/ReviewMonitorCommandOutputDisplayDocument.swift index ac410d6a..27c78e28 100644 --- a/Sources/ReviewChatLogUI/Detail/ReviewMonitorCommandOutputDisplayDocument.swift +++ b/Sources/ReviewChatLogUI/Detail/ReviewMonitorCommandOutputDisplayDocument.swift @@ -1007,7 +1007,12 @@ enum ReviewMonitorCommandOutputDisplayDocument { case .replace(let replacement): guard replacement.textUTF16Length == (replacement.text as NSString).length, rangeIsValid(replacement.range, in: previousDisplay.text), - replacingText(in: previousDisplay.text, range: replacement.range, with: replacement.text) == displayText + ReviewMonitorUTF16TextReplacement.replacing( + previousDisplay.text, + range: replacement.range, + with: replacement.text, + equals: displayText + ) else { return nil } @@ -1073,21 +1078,6 @@ enum ReviewMonitorCommandOutputDisplayDocument { return range.location >= 0 && range.length >= 0 && NSMaxRange(range) <= length } - private static func replacingText(in text: String, range: NSRange, with replacement: String) -> String { - let string = text as NSString - guard range.location >= 0, - range.length >= 0, - NSMaxRange(range) <= string.length - else { - return text - } - let prefix = string.substring(with: NSRange(location: 0, length: range.location)) - let suffixLocation = NSMaxRange(range) - let suffix = string.substring( - with: NSRange(location: suffixLocation, length: string.length - suffixLocation) - ) - return prefix + replacement + suffix - } } private extension String { diff --git a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift index 4f64cd3d..af56113a 100644 --- a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift +++ b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift @@ -137,46 +137,121 @@ struct ReviewMonitorLogDocumentProjection: Sendable { previous: ReviewMonitorLog.Document, current: ReviewMonitorLog.Document ) -> ReviewMonitorLog.Replacement? { - for previousBlock in previous.blocks { - guard let currentBlock = current.blocks.first(where: { $0.id == previousBlock.id }), - currentBlock.range.location == previousBlock.range.location, - NSMaxRange(currentBlock.range) <= current.textUTF16Length - else { + guard let candidate = replacementCandidate(previous: previous, current: current) else { + return nil + } + + guard rangeIsValid(candidate.previous.range, upperBound: previous.textUTF16Length), + rangeIsValid(candidate.current.range, upperBound: current.textUTF16Length), + rangeIsValid(candidate.previous.sourceRange, upperBound: previous.sourceTextUTF16Length), + rangeIsValid(candidate.current.sourceRange, upperBound: current.sourceTextUTF16Length) + else { + return nil + } + + let replacementText = (current.text as NSString).substring(with: candidate.current.range) + let replacementSource = (current.sourceText as NSString).substring(with: candidate.current.sourceRange) + guard ReviewMonitorUTF16TextReplacement.replacing( + previous.text, + range: candidate.previous.range, + with: replacementText, + equals: current.text + ), + ReviewMonitorUTF16TextReplacement.replacing( + previous.sourceText, + range: candidate.previous.sourceRange, + with: replacementSource, + equals: current.sourceText + ) + else { + return nil + } + + return .init( + kind: candidate.current.kind, + blockID: candidate.current.id, + range: candidate.previous.range, + text: replacementText, + textUTF16Length: candidate.current.range.length + ) + } + + private static func replacementCandidate( + previous: ReviewMonitorLog.Document, + current: ReviewMonitorLog.Document + ) -> (previous: ReviewMonitorLog.Block, current: ReviewMonitorLog.Block)? { + guard previous.blocks.count == current.blocks.count else { + return nil + } + + var candidate: (previous: ReviewMonitorLog.Block, current: ReviewMonitorLog.Block)? + var displayDelta = 0 + var sourceDelta = 0 + for index in previous.blocks.indices { + let previousBlock = previous.blocks[index] + let currentBlock = current.blocks[index] + if candidate != nil { + guard unchangedBlockAfterReplacement( + previous: previousBlock, + current: currentBlock, + displayDelta: displayDelta, + sourceDelta: sourceDelta + ) else { + return nil + } continue } - let replacementText = (current.text as NSString).substring(with: currentBlock.range) - let candidate = replacingText( - in: previous.text, - range: previousBlock.range, - with: replacementText - ) - guard candidate == current.text else { + if previousBlock == currentBlock { continue } - return .init( - kind: currentBlock.kind, - blockID: currentBlock.id, - range: previousBlock.range, - text: replacementText, - textUTF16Length: currentBlock.range.length - ) + guard canReplaceSingleBlock(previous: previousBlock, current: currentBlock) else { + return nil + } + candidate = (previous: previousBlock, current: currentBlock) + displayDelta = currentBlock.range.length - previousBlock.range.length + sourceDelta = currentBlock.sourceRange.length - previousBlock.sourceRange.length } - return nil + return candidate } - private static func replacingText( - in text: String, - range: NSRange, - with replacement: String - ) -> String { - let string = text as NSString - let prefix = string.substring(with: NSRange(location: 0, length: range.location)) - let suffixLocation = NSMaxRange(range) - let suffix = string.substring( - with: NSRange(location: suffixLocation, length: string.length - suffixLocation) - ) - return prefix + replacement + suffix + private static func canReplaceSingleBlock( + previous: ReviewMonitorLog.Block, + current: ReviewMonitorLog.Block + ) -> Bool { + previous.id == current.id + && previous.kind == current.kind + && previous.groupID == current.groupID + && current.range.location == previous.range.location + && current.sourceRange.location == previous.sourceRange.location + && rangeIsValid(previous.range) + && rangeIsValid(current.range) + && rangeIsValid(previous.sourceRange) + && rangeIsValid(current.sourceRange) + } + + private static func unchangedBlockAfterReplacement( + previous: ReviewMonitorLog.Block, + current: ReviewMonitorLog.Block, + displayDelta: Int, + sourceDelta: Int + ) -> Bool { + previous.id == current.id + && previous.kind == current.kind + && previous.groupID == current.groupID + && previous.metadata == current.metadata + && current.range.location == previous.range.location + displayDelta + && current.range.length == previous.range.length + && current.sourceRange.location == previous.sourceRange.location + sourceDelta + && current.sourceRange.length == previous.sourceRange.length + } + + private static func rangeIsValid(_ range: NSRange) -> Bool { + range.location >= 0 && range.length >= 0 + } + + private static func rangeIsValid(_ range: NSRange, upperBound: Int) -> Bool { + range.location >= 0 && range.length >= 0 && NSMaxRange(range) <= upperBound } private static func contentChanged( diff --git a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogProjection.swift b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogProjection.swift index 31846963..0365d185 100644 --- a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogProjection.swift +++ b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogProjection.swift @@ -2,6 +2,93 @@ import Foundation package enum ReviewMonitorLog {} +enum ReviewMonitorUTF16TextReplacement { + static func replacing( + _ text: String, + range: NSRange, + with replacement: String, + equals target: String + ) -> Bool { + let source = text as NSString + let target = target as NSString + let replacement = replacement as NSString + let replacementLength = replacement.length + guard range.location >= 0, + range.length >= 0, + NSMaxRange(range) <= source.length, + source.length - range.length + replacementLength == target.length + else { + return false + } + + let prefixRange = NSRange(location: 0, length: range.location) + guard utf16SegmentsEqual(source, prefixRange, target, prefixRange) else { + return false + } + + let replacementTargetRange = NSRange(location: range.location, length: replacementLength) + guard utf16SegmentsEqual( + replacement, + NSRange(location: 0, length: replacementLength), + target, + replacementTargetRange + ) else { + return false + } + + let sourceSuffixLocation = NSMaxRange(range) + let suffixLength = source.length - sourceSuffixLocation + return utf16SegmentsEqual( + source, + NSRange(location: sourceSuffixLocation, length: suffixLength), + target, + NSRange(location: NSMaxRange(replacementTargetRange), length: suffixLength) + ) + } + + private static func utf16SegmentsEqual( + _ lhs: NSString, + _ lhsRange: NSRange, + _ rhs: NSString, + _ rhsRange: NSRange + ) -> Bool { + guard lhsRange.length == rhsRange.length, + rangeIsValid(lhsRange, in: lhs), + rangeIsValid(rhsRange, in: rhs) + else { + return false + } + guard lhsRange.length > 0 else { + return true + } + + let chunkSize = 4_096 + var lhsBuffer = [unichar](repeating: 0, count: chunkSize) + var rhsBuffer = [unichar](repeating: 0, count: chunkSize) + var offset = 0 + while offset < lhsRange.length { + let length = min(chunkSize, lhsRange.length - offset) + lhs.getCharacters( + &lhsBuffer, + range: NSRange(location: lhsRange.location + offset, length: length) + ) + rhs.getCharacters( + &rhsBuffer, + range: NSRange(location: rhsRange.location + offset, length: length) + ) + for index in 0.. Bool { + range.location >= 0 && range.length >= 0 && NSMaxRange(range) <= text.length + } +} + package extension ReviewMonitorLog { struct BlockID: Codable, Hashable, Sendable { var rawValue: String diff --git a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogScrollView.swift b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogScrollView.swift index c4312fc8..e4c59f3a 100644 --- a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogScrollView.swift +++ b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogScrollView.swift @@ -365,8 +365,12 @@ final class ReviewMonitorLogScrollView: NSScrollView { replacement.range.length >= 0, NSMaxRange(replacement.range) <= displayedUTF16Length, document.textUTF16Length == displayedUTF16Length - replacement.range.length + replacement.textUTF16Length, - let replacementResult = replacingDisplayedText(in: replacement.range, with: replacement.text), - replacementResult == document.text + ReviewMonitorUTF16TextReplacement.replacing( + displayedText, + range: replacement.range, + with: replacement.text, + equals: document.text + ) else { assertionFailure("Log replacement change is not synchronized with the display document.") return false @@ -896,18 +900,6 @@ final class ReviewMonitorLogScrollView: NSScrollView { displayedText = mutable as String } - private func replacingDisplayedText(in range: NSRange, with text: String) -> String? { - guard range.location >= 0, - range.length >= 0, - NSMaxRange(range) <= displayedUTF16Length - else { - return nil - } - let mutable = NSMutableString(string: displayedText) - mutable.replaceCharacters(in: range, with: text) - return mutable as String - } - private func invalidateDocumentLayout() { if syncDocumentFrameToTextLayout() { logDocumentView.invalidateIntrinsicContentSize() diff --git a/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift b/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift index 7cd61216..7119e830 100644 --- a/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift +++ b/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift @@ -93,12 +93,21 @@ struct ReviewMonitorCodexChatLogProjection { } let reviewOutputKeys = Set(items.compactMap(Self.reviewOutputKey)) var emittedReviewOutputKeys = Set() + var previousReasoningOutputKey: ReasoningOutputKey? let blocks = items.flatMap { item in if let reviewOutputKey = Self.reviewOutputKey(for: item) { guard emittedReviewOutputKeys.insert(reviewOutputKey).inserted else { return [] as [ReviewMonitorLogProjectedBlock] } } + if let reasoningOutputKey = Self.reasoningOutputKey(for: item) { + guard Self.shouldRenderReasoningOutput(reasoningOutputKey, after: previousReasoningOutputKey) else { + return [] as [ReviewMonitorLogProjectedBlock] + } + previousReasoningOutputKey = reasoningOutputKey + } else { + previousReasoningOutputKey = nil + } return projectedBlocks( from: item, turnStatus: turnStatus, @@ -279,10 +288,57 @@ struct ReviewMonitorCodexChatLogProjection { item.projectionTurnID?.rawValue ?? item.sourceID } + private static func reasoningOutputKey(for item: Item) -> ReasoningOutputKey? { + guard item.kind == .reasoning else { + return nil + } + guard case .reasoning(let reasoning) = item.content, + let normalizedText = normalizedReasoningOutputText(reasoning.text).nilIfEmpty + else { + return nil + } + return ReasoningOutputKey( + scopeID: reasoningOutputScopeID(for: item), + text: normalizedText, + payloadKind: rawPayloadKind(from: item.rawPayload) + ) + } + + private static func reasoningOutputScopeID(for item: Item) -> String { + item.projectionTurnID?.rawValue ?? "item:\(item.sourceID)" + } + + private static func shouldRenderReasoningOutput( + _ key: ReasoningOutputKey, + after previousKey: ReasoningOutputKey? + ) -> Bool { + guard let previousKey else { + return true + } + guard previousKey.scopeID == key.scopeID, previousKey.text == key.text else { + return true + } + let payloadKinds = Set([previousKey.payloadKind, key.payloadKind].compactMap { $0 }) + return payloadKinds != ["agent_reasoning", "reasoning"] + } + private static func normalizedReviewOutputText(_ text: String) -> String { text.trimmingCharacters(in: .whitespacesAndNewlines) } + private static func normalizedReasoningOutputText(_ text: String) -> String { + text.trimmingCharacters(in: .whitespacesAndNewlines) + } + + private static func rawPayloadKind(from data: Data?) -> String? { + guard let data, + let payload = try? JSONDecoder().decode(RawPayloadKind.self, from: data) + else { + return nil + } + return payload.kindValue + } + private func unknownText(title: String, detail: String?) -> String { guard let detail, detail != title else { return title @@ -571,6 +627,32 @@ private struct ReviewOutputKey: Hashable { var text: String } +private struct ReasoningOutputKey: Hashable { + var scopeID: String + var text: String + var payloadKind: String? +} + +private struct RawPayloadKind: Decodable { + struct NestedItem: Decodable { + var type: String? + var kind: String? + + var kindValue: String? { + type ?? kind + } + } + + var type: String? + var kind: String? + var item: NestedItem? + var payload: NestedItem? + + var kindValue: String? { + type ?? kind ?? item?.kindValue ?? payload?.kindValue + } +} + @MainActor private protocol CodexChatLogProjectionItem { var itemID: String { get } @@ -580,6 +662,7 @@ private protocol CodexChatLogProjectionItem { var kind: CodexThreadItem.Kind { get } var content: CodexThreadItem.Content { get } var itemStatus: CodexTurnStatus? { get } + var rawPayload: Data? { get } } extension CodexItem: CodexChatLogProjectionItem { @@ -632,6 +715,10 @@ private struct CodexChatModelLogItem: CodexChatLogProjectionItem { var itemStatus: CodexTurnStatus? { item.content.reviewMonitorLogItemStatus } + + var rawPayload: Data? { + item.rawPayload + } } @MainActor @@ -671,6 +758,10 @@ private struct CodexThreadSnapshotLogItem: CodexChatLogProjectionItem { item.content.reviewMonitorLogItemStatus } + var rawPayload: Data? { + item.rawPayload + } + private var semanticItemID: String { switch item.kind { case .enteredReviewMode: diff --git a/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift b/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift index 4c653d69..3d957a31 100644 --- a/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift +++ b/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift @@ -588,6 +588,224 @@ struct ReviewMonitorCodexChatDetailTests { #expect(renderedText == [finalReview, finalReview]) } + @Test func codexChatLogProjectionDeduplicatesSameTurnReasoningMirrorItems() async throws { + var projection = ReviewMonitorCodexChatLogProjection() + let reasoningText = """ + **Summarizing files with git** + + I need to summarize the files. + """ + let turn = CodexTurnSnapshot( + id: CodexTurnID(rawValue: "turn-review"), + status: .running, + items: [ + .init( + id: "event-reasoning", + kind: .reasoning, + content: .reasoning(.init(summary: reasoningText)), + rawPayload: rawPayload(type: "agent_reasoning") + ), + .init( + id: "response-reasoning", + kind: .reasoning, + content: .reasoning(.init(summary: reasoningText)), + rawPayload: rawPayload(type: "reasoning") + ), + ] + ) + + let projectedDocument = projection.render( + from: turn, + chatCreatedAt: nil, + chatUpdatedAt: nil + ) + let document = try #require(projectedDocument) + + #expect(document.blocks.count == 1) + #expect(document.text == "Summarizing files with git\n\nI need to summarize the files.") + } + + @Test func codexChatLogProjectionKeepsAdjacentMatchingReasoningWithoutMirrorPayloads() async throws { + var projection = ReviewMonitorCodexChatLogProjection() + let reasoningText = "Checking diff" + let turn = CodexTurnSnapshot( + id: CodexTurnID(rawValue: "turn-review"), + status: .running, + items: [ + .init( + id: "reasoning-a", + kind: .reasoning, + content: .reasoning(.init(summary: reasoningText)), + rawPayload: rawPayload(type: "reasoning") + ), + .init( + id: "reasoning-b", + kind: .reasoning, + content: .reasoning(.init(summary: reasoningText)), + rawPayload: rawPayload(type: "reasoning") + ), + ] + ) + + let projectedDocument = projection.render( + from: turn, + chatCreatedAt: nil, + chatUpdatedAt: nil + ) + let document = try #require(projectedDocument) + + #expect(document.blocks.count == 2) + #expect(document.text == "\(reasoningText)\n\n\(reasoningText)") + } + + @Test func codexChatLogProjectionKeepsMatchingReasoningSeparatedByCommandInSameTurn() async throws { + var projection = ReviewMonitorCodexChatLogProjection() + let reasoningText = "Checking diff" + let turn = CodexTurnSnapshot( + id: CodexTurnID(rawValue: "turn-review"), + status: .running, + items: [ + .init( + id: "reasoning-a", + kind: .reasoning, + content: .reasoning(.init(summary: reasoningText)), + rawPayload: rawPayload(type: "reasoning") + ), + .init( + id: "command-a", + kind: .commandExecution, + content: .command(.init(command: "/bin/zsh -lc")) + ), + .init( + id: "reasoning-b", + kind: .reasoning, + content: .reasoning(.init(summary: reasoningText)), + rawPayload: rawPayload(type: "reasoning") + ), + ] + ) + + let projectedDocument = projection.render( + from: turn, + chatCreatedAt: nil, + chatUpdatedAt: nil + ) + let document = try #require(projectedDocument) + + #expect(document.blocks.count == 3) + #expect(document.text.contains("\(reasoningText)\n\n$ /bin/zsh -lc\n\n\(reasoningText)")) + } + + @Test func codexChatLogProjectionKeepsMatchingReasoningAcrossTurns() async throws { + var projection = ReviewMonitorCodexChatLogProjection() + let reasoningText = "Checking diff" + let snapshot = makeCodexThreadSnapshotForTesting( + chatID: CodexThreadID(rawValue: "review-thread"), + turns: [ + .init( + id: CodexTurnID(rawValue: "turn-first"), + status: .completed, + items: [ + .init( + id: "reasoning-first", + kind: .reasoning, + content: .reasoning(.init(summary: reasoningText)) + ), + ] + ), + .init( + id: CodexTurnID(rawValue: "turn-second"), + status: .completed, + items: [ + .init( + id: "reasoning-second", + kind: .reasoning, + content: .reasoning(.init(summary: reasoningText)) + ), + ] + ), + ] + ) + + let projectedDocument = projection.render( + from: snapshot, + chatCreatedAt: nil, + chatUpdatedAt: nil + ) + let document = try #require(projectedDocument) + + #expect(document.blocks.count == 2) + #expect(document.text == "\(reasoningText)\n\n\(reasoningText)") + } + + @Test func codexChatLogProjectionIgnoresLateReasoningMirrorBeforeCommand() async throws { + var projection = ReviewMonitorCodexChatLogProjection() + let turnID = CodexTurnID(rawValue: "turn-review") + let reasoningText = """ + **Summarizing files with git** + + I need to summarize the files. + """ + let command = CodexThreadItem( + id: "command", + kind: .commandExecution, + content: .command(.init( + command: "/bin/zsh -lc", + status: .running, + startedAt: Date(timeIntervalSince1970: 4_000) + )) + ) + let initial = CodexTurnSnapshot( + id: turnID, + status: .running, + items: [ + .init( + id: "event-reasoning", + kind: .reasoning, + content: .reasoning(.init(summary: reasoningText)), + rawPayload: rawPayload(type: "agent_reasoning") + ), + command, + ] + ) + let mirrored = CodexTurnSnapshot( + id: turnID, + status: .running, + items: [ + .init( + id: "event-reasoning", + kind: .reasoning, + content: .reasoning(.init(summary: reasoningText)), + rawPayload: rawPayload(type: "agent_reasoning") + ), + .init( + id: "response-reasoning", + kind: .reasoning, + content: .reasoning(.init(summary: reasoningText)), + rawPayload: rawPayload(type: "reasoning") + ), + command, + ] + ) + + let projectedInitialDocument = projection.render( + from: initial, + chatCreatedAt: nil, + chatUpdatedAt: nil + ) + let initialDocument = try #require(projectedInitialDocument) + let projectedMirroredDocument = projection.render( + from: mirrored, + chatCreatedAt: nil, + chatUpdatedAt: nil + ) + let mirroredDocument = try #require(projectedMirroredDocument) + + #expect(mirroredDocument.text == initialDocument.text) + #expect(mirroredDocument.blocks == initialDocument.blocks) + #expect(mirroredDocument.revision == initialDocument.revision) + } + @Test func codexChatLogProjectionRendersUserMessageWithoutReviewModeLog() async throws { var projection = ReviewMonitorCodexChatLogProjection() let turn = CodexTurnSnapshot( @@ -897,6 +1115,10 @@ struct ReviewMonitorCodexChatDetailTests { try await modelContext.refresh(chat) return chat } + + private func rawPayload(type: String) -> Data { + Data("{\"type\":\"\(type)\"}".utf8) + } } private struct ThreadItemParams: Encodable, Sendable { diff --git a/Tests/ReviewUITests/ReviewUITests.swift b/Tests/ReviewUITests/ReviewUITests.swift index 69094a0e..a6dbc816 100644 --- a/Tests/ReviewUITests/ReviewUITests.swift +++ b/Tests/ReviewUITests/ReviewUITests.swift @@ -2831,6 +2831,94 @@ struct ReviewUITests { #expect(visibleText.contains("11sReviewing JSONRPC requests") == false) } + @Test func sourceDocumentReplacementTargetsMetadataChangedBlock() throws { + let startedAt = Date(timeIntervalSince1970: 200) + var projection = ReviewMonitorLogDocumentProjection() + let previousSource = projection.render(projectedBlocks: [ + .init( + id: .init("reasoning_1"), + kind: .rawReasoning, + groupID: "reasoning_1", + text: "Need to inspect files." + ), + .init( + id: .init("command_1"), + kind: .command, + groupID: "cmd_1", + text: "$ git diff", + metadata: .init( + sourceType: "commandExecution", + status: "running", + itemID: "cmd_1", + command: "git diff", + startedAt: startedAt, + commandStatus: "running" + ) + ), + ]) + + let currentSource = projection.render(projectedBlocks: [ + .init( + id: .init("reasoning_1"), + kind: .rawReasoning, + groupID: "reasoning_1", + text: "Need to inspect files." + ), + .init( + id: .init("command_1"), + kind: .command, + groupID: "cmd_1", + text: "$ git diff", + metadata: .init( + sourceType: "commandExecution", + status: "completed", + itemID: "cmd_1", + command: "git diff", + startedAt: startedAt, + completedAt: Date(timeIntervalSince1970: 211), + durationMs: 11_000, + commandStatus: "completed" + ) + ), + ]) + + guard case .replace(let replacement) = currentSource.lastChange else { + Issue.record("Expected command metadata change to remain a block replacement.") + return + } + #expect(replacement.blockID == ReviewMonitorLog.BlockID("command_1")) + #expect(applyingDisplayChange(currentSource.lastChange, to: previousSource.text) == currentSource.text) + } + + @Test func commandOutputOnlyDisplayBeforeMarkdownHeadingKeepsParagraphBoundary() throws { + var projection = ReviewMonitorLogDocumentProjection() + let source = projection.render(projectedBlocks: [ + .init( + id: .init("command_output_1"), + kind: .commandOutput, + groupID: "cmd_1", + text: "stdout", + metadata: .init( + sourceType: "commandExecution", + status: "completed", + durationMs: 43_000, + commandStatus: "completed" + ) + ), + .init( + id: .init("reasoning_1"), + kind: .rawReasoning, + groupID: "reasoning_1", + text: "**Summarizing files with git**\n\nI need to summarize the files." + ), + ]) + let display = ReviewMonitorCommandOutputDisplayDocument.make(from: source) + + let visibleText = ReviewMonitorCommandOutputDisplayDocument.userVisibleText(from: display.text) + #expect(visibleText.contains("Ran command for 43s\n\nSummarizing files with git")) + #expect(visibleText.contains("43sSummarizing files with git") == false) + } + @Test func coalescedRunningCommandBeforeMarkdownHeadingKeepsParagraphBoundary() async throws { let startedAt = Date(timeIntervalSince1970: 200) let chat = makeReviewChatFixtureForTesting( From 828dade5c6ccc3958c8aa573e865e6181ab17334 Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Mon, 6 Jul 2026 14:46:47 +0900 Subject: [PATCH 08/18] fix(review-ui): detect same-length log replacements --- .../ReviewMonitorLogDocumentProjection.swift | 58 +++++++++++++++++-- .../Detail/ReviewMonitorLogProjection.swift | 9 +++ Tests/ReviewUITests/ReviewUITests.swift | 40 +++++++++++++ 3 files changed, 103 insertions(+), 4 deletions(-) diff --git a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift index af56113a..9884911c 100644 --- a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift +++ b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift @@ -195,14 +195,21 @@ struct ReviewMonitorLogDocumentProjection: Sendable { previous: previousBlock, current: currentBlock, displayDelta: displayDelta, - sourceDelta: sourceDelta + sourceDelta: sourceDelta, + previousDocument: previous, + currentDocument: current ) else { return nil } continue } - if previousBlock == currentBlock { + if blockContentUnchanged( + previous: previousBlock, + current: currentBlock, + previousDocument: previous, + currentDocument: current + ) { continue } guard canReplaceSingleBlock(previous: previousBlock, current: currentBlock) else { @@ -234,9 +241,11 @@ struct ReviewMonitorLogDocumentProjection: Sendable { previous: ReviewMonitorLog.Block, current: ReviewMonitorLog.Block, displayDelta: Int, - sourceDelta: Int + sourceDelta: Int, + previousDocument: ReviewMonitorLog.Document, + currentDocument: ReviewMonitorLog.Document ) -> Bool { - previous.id == current.id + guard previous.id == current.id && previous.kind == current.kind && previous.groupID == current.groupID && previous.metadata == current.metadata @@ -244,6 +253,47 @@ struct ReviewMonitorLogDocumentProjection: Sendable { && current.range.length == previous.range.length && current.sourceRange.location == previous.sourceRange.location + sourceDelta && current.sourceRange.length == previous.sourceRange.length + else { + return false + } + return blockContentUnchanged( + previous: previous, + current: current, + previousDocument: previousDocument, + currentDocument: currentDocument + ) + } + + private static func blockContentUnchanged( + previous: ReviewMonitorLog.Block, + current: ReviewMonitorLog.Block, + previousDocument: ReviewMonitorLog.Document, + currentDocument: ReviewMonitorLog.Document + ) -> Bool { + guard previous.id == current.id, + previous.kind == current.kind, + previous.groupID == current.groupID, + previous.metadata == current.metadata, + rangeIsValid(previous.range, upperBound: previousDocument.textUTF16Length), + rangeIsValid(current.range, upperBound: currentDocument.textUTF16Length), + rangeIsValid(previous.sourceRange, upperBound: previousDocument.sourceTextUTF16Length), + rangeIsValid(current.sourceRange, upperBound: currentDocument.sourceTextUTF16Length), + ReviewMonitorUTF16TextReplacement.segmentsEqual( + previousDocument.text, + range: previous.range, + currentDocument.text, + range: current.range + ), + ReviewMonitorUTF16TextReplacement.segmentsEqual( + previousDocument.sourceText, + range: previous.sourceRange, + currentDocument.sourceText, + range: current.sourceRange + ) + else { + return false + } + return true } private static func rangeIsValid(_ range: NSRange) -> Bool { diff --git a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogProjection.swift b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogProjection.swift index 0365d185..b1612167 100644 --- a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogProjection.swift +++ b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogProjection.swift @@ -3,6 +3,15 @@ import Foundation package enum ReviewMonitorLog {} enum ReviewMonitorUTF16TextReplacement { + static func segmentsEqual( + _ lhs: String, + range lhsRange: NSRange, + _ rhs: String, + range rhsRange: NSRange + ) -> Bool { + utf16SegmentsEqual(lhs as NSString, lhsRange, rhs as NSString, rhsRange) + } + static func replacing( _ text: String, range: NSRange, diff --git a/Tests/ReviewUITests/ReviewUITests.swift b/Tests/ReviewUITests/ReviewUITests.swift index a6dbc816..afe70d19 100644 --- a/Tests/ReviewUITests/ReviewUITests.swift +++ b/Tests/ReviewUITests/ReviewUITests.swift @@ -2890,6 +2890,46 @@ struct ReviewUITests { #expect(applyingDisplayChange(currentSource.lastChange, to: previousSource.text) == currentSource.text) } + @Test func sourceDocumentReplacementHandlesSameLengthBlockTextRewrite() throws { + var projection = ReviewMonitorLogDocumentProjection() + let previousSource = projection.render(projectedBlocks: [ + .init( + id: .init("event_1"), + kind: .event, + groupID: "event_1", + text: "abcd" + ), + .init( + id: .init("event_2"), + kind: .event, + groupID: "event_2", + text: "tail" + ), + ]) + + let currentSource = projection.render(projectedBlocks: [ + .init( + id: .init("event_1"), + kind: .event, + groupID: "event_1", + text: "wxyz" + ), + .init( + id: .init("event_2"), + kind: .event, + groupID: "event_2", + text: "tail" + ), + ]) + + guard case .replace(let replacement) = currentSource.lastChange else { + Issue.record("Expected same-length block rewrite to remain a replacement.") + return + } + #expect(replacement.blockID == ReviewMonitorLog.BlockID("event_1")) + #expect(applyingDisplayChange(currentSource.lastChange, to: previousSource.text) == currentSource.text) + } + @Test func commandOutputOnlyDisplayBeforeMarkdownHeadingKeepsParagraphBoundary() throws { var projection = ReviewMonitorLogDocumentProjection() let source = projection.render(projectedBlocks: [ From e7eac9ee97ec02ddb5890e0944adca3fc90ded78 Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Mon, 6 Jul 2026 15:05:45 +0900 Subject: [PATCH 09/18] fix(review): preserve completion response contracts --- .../AppServerCodexReviewBackend.swift | 8 ++++- .../CodexReviewMCPToolResults.swift | 11 ++++++- .../AppServerClientTests.swift | 29 +++++++++++++++++++ .../CodexReviewMCPHTTPServerTests.swift | 20 +++++++++++-- 4 files changed, 63 insertions(+), 5 deletions(-) diff --git a/Sources/CodexReviewAppServer/AppServerCodexReviewBackend.swift b/Sources/CodexReviewAppServer/AppServerCodexReviewBackend.swift index f6db3be1..9032444b 100644 --- a/Sources/CodexReviewAppServer/AppServerCodexReviewBackend.swift +++ b/Sources/CodexReviewAppServer/AppServerCodexReviewBackend.swift @@ -598,7 +598,7 @@ private enum AppServerTypedReviewEventAdapter { message: response.status?.rawValue ?? "Failed." ) } - guard let finalReview = response.transcript.reviewOutputText?.nilIfEmpty else { + guard let finalReview = reviewCompletionText(for: response) else { return [.failed("Review completed without review output.")] } return [ @@ -606,6 +606,12 @@ private enum AppServerTypedReviewEventAdapter { ] } + private static func reviewCompletionText(for response: CodexResponse) -> String? { + response.transcript.reviewOutputText?.nilIfEmpty + ?? response.finalAnswer?.nilIfEmpty + ?? response.transcript.finalAnswer?.nilIfEmpty + } + private static func unknownStatusEvents( _: String, turnID _: String diff --git a/Sources/CodexReviewMCPServer/CodexReviewMCPToolResults.swift b/Sources/CodexReviewMCPServer/CodexReviewMCPToolResults.swift index b9b59c84..515cb0d4 100644 --- a/Sources/CodexReviewMCPServer/CodexReviewMCPToolResults.swift +++ b/Sources/CodexReviewMCPServer/CodexReviewMCPToolResults.swift @@ -63,7 +63,7 @@ private extension CodexReviewAPI.Read.Result { func structuredContentForStartOrAwait(log: ReviewMCPLogProjection) -> Value { structuredContent( - includeLog: false, + includeLog: true, includeDetails: false, includeNextAction: core.lifecycle.status.isTerminal == false, log: log @@ -115,6 +115,14 @@ private extension CodexReviewAPI.Read.Result { private extension ReviewMCPLogProjection { func structuredContent() -> Value { var truncatedFields: [String] = [] + let orderedEntryIDs = Array(self.orderedEntryIDs.suffix(Self.compactEntryIDLimit)) + let activeEntryIDs = Array(self.activeEntryIDs.suffix(Self.compactEntryIDLimit)) + if orderedEntryIDs.count < self.orderedEntryIDs.count { + truncatedFields.append("orderedEntryIds") + } + if activeEntryIDs.count < self.activeEntryIDs.count { + truncatedFields.append("activeEntryIds") + } var object: [String: Value] = [ "revision": .string(revision), "orderedEntryIds": .array(orderedEntryIDs.map(Value.string)), @@ -189,6 +197,7 @@ private extension ReviewMCPLogProjection { return .object(object) } + private static var compactEntryIDLimit: Int { 100 } private static var detailedItemsLimit: Int { 100 } } diff --git a/Tests/CodexReviewAppServerTests/AppServerClientTests.swift b/Tests/CodexReviewAppServerTests/AppServerClientTests.swift index 7d4674ca..bd00a8d2 100644 --- a/Tests/CodexReviewAppServerTests/AppServerClientTests.swift +++ b/Tests/CodexReviewAppServerTests/AppServerClientTests.swift @@ -135,6 +135,35 @@ struct AppServerClientTests { #expect(try await iterator.next() == .completed(finalReview: "No issues found.")) } + @Test func backendCompletesReviewFromCompatibleFinalAnswerWhenReviewOutputIsAbsent() async throws { + let runtime = try await CodexAppServerTestRuntime.start() + try await runtime.transport.enqueueThreadStart(threadID: "thread-1", model: "gpt-5") + try await runtime.transport.enqueueReviewStart(turnID: "turn-1", reviewThreadID: "review-thread") + await runtime.transport.waitForNotificationStreamCount(1) + let backend = AppServerCodexReviewBackend(appServer: runtime.server) + + let attempt = try await backend.startReview(makeReviewStart(target: .baseBranch("main"))) + var iterator = eventSequence(attempt).makeAsyncIterator() + + try await runtime.transport.emitServerNotification( + method: "turn/completed", + params: TestTurnNotification( + threadID: "review-thread", + turn: .init( + id: "turn-1", + status: "completed", + items: [ + .finalAnswer(id: "assistant-final", text: "No issues found.") + ] + ) + ) + ) + + #expect( + try await iterator.next() == .started(turnID: "turn-1", reviewThreadID: "review-thread", model: "gpt-5")) + #expect(try await iterator.next() == .completed(finalReview: "No issues found.")) + } + @Test func backendDoesNotPromoteThreadScopedFinalAnswerDeltaWithoutTurnID() async throws { let runtime = try await CodexAppServerTestRuntime.start() try await runtime.transport.enqueueThreadStart(threadID: "thread-1", model: "gpt-5") diff --git a/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift b/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift index 5d711935..64501b34 100644 --- a/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift +++ b/Tests/CodexReviewMCPServerTests/CodexReviewMCPHTTPServerTests.swift @@ -209,7 +209,7 @@ struct CodexReviewMCPHTTPServerTests { #expect( resolved.value(for: ["result", "structuredContent", "review", "reviewResult", "state"]) as? String == "noFindings") - #expect(resolved.value(for: ["result", "structuredContent", "log"]) == nil) + assertCompactLog(resolved, total: 2) let commands = await backend.recordedCommands() #expect( commands.contains( @@ -327,7 +327,7 @@ struct CodexReviewMCPHTTPServerTests { #expect(running.value(for: ["result", "structuredContent", "lifecycle", "status"]) as? String == "running") #expect(running.value(for: ["result", "structuredContent", "logs"]) == nil) #expect(running.value(for: ["result", "structuredContent", "rawLogText"]) == nil) - #expect(running.value(for: ["result", "structuredContent", "log"]) == nil) + assertCompactLog(running, total: 1) #expect( running.value(for: ["result", "structuredContent", "nextAction", "tool"]) as? String == "review_await") @@ -362,7 +362,7 @@ struct CodexReviewMCPHTTPServerTests { #expect( awaited.value(for: ["result", "structuredContent", "review", "reviewResult", "state"]) as? String == "noFindings") - #expect(awaited.value(for: ["result", "structuredContent", "log"]) == nil) + assertCompactLog(awaited, total: 1) #expect(awaited.value(for: ["result", "structuredContent", "logs"]) == nil) } } @@ -1458,6 +1458,20 @@ private nonisolated func testError(_ message: String) -> NSError { ) } +private func assertCompactLog(_ response: [String: Any], total: Int) { + let log = response.value(for: ["result", "structuredContent", "log"]) as? [String: Any] + #expect(log != nil) + #expect(log?["revision"] is String) + #expect((log?["orderedEntryIds"] as? [String])?.count == min(total, 100)) + #expect((log?["activeEntryIds"] as? [String]) != nil) + #expect(log?["activeEntryCount"] is Int) + let itemsPage = log?["itemsPage"] as? [String: Any] + #expect(itemsPage?["total"] as? Int == total) + #expect(itemsPage?["limit"] as? Int == 0) + #expect(itemsPage?["returned"] as? Int == 0) + #expect((log?["items"] as? [[String: Any]])?.isEmpty == true) +} + private extension [String: Any] { func value(for path: [String]) -> Any? { var current: Any? = self From c2b6caf15403076098a2e01488917357f0382842 Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Mon, 6 Jul 2026 16:01:17 +0900 Subject: [PATCH 10/18] fix(review-ui): make the log document own inter-block paragraph boundaries Block text with trailing newlines used to absorb the separator between blocks, so command panels (which replace block text with a placeholder) lost the boundary and rendered on the same line as neighboring text, and streamed output flipped the spacing around command rows. Normalize block text in the document builder and always separate visible blocks with one blank-line gap. Co-Authored-By: Claude Fable 5 --- .../ReviewMonitorLogDocumentProjection.swift | 27 ++-- Tests/ReviewUITests/ReviewUITests.swift | 122 ++++++++++++++++++ 2 files changed, 139 insertions(+), 10 deletions(-) diff --git a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift index 9884911c..5817834a 100644 --- a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift +++ b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift @@ -325,13 +325,19 @@ struct ReviewMonitorLogDocumentProjection: Sendable { return } - let renderedText = ReviewMonitorLogStyler.renderedText( - for: block.kind, - source: block.text, - blockID: block.id - ) + // Block text must not carry the inter-block separator: command + // panels replace their block text with a placeholder, so a + // separator living inside a neighboring block's trailing newlines + // would silently disappear from the display document. + let sourceText = Self.normalizedBlockText(block.text) + let renderedText = Self.normalizedBlockText( + ReviewMonitorLogStyler.renderedText( + for: block.kind, + source: sourceText, + blockID: block.id + )) let appended = appendedText(renderedText, after: document.text) - let appendedSource = appendedText(block.text, after: document.sourceText) + let appendedSource = appendedText(sourceText, after: document.sourceText) hasVisibleSections = true let previousLength = document.textUTF16Length @@ -339,7 +345,7 @@ struct ReviewMonitorLogDocumentProjection: Sendable { let suffixLength = ReviewMonitorLogDocumentProjection.utf16Length(appended) let sourceSuffixLength = ReviewMonitorLogDocumentProjection.utf16Length(appendedSource) let blockLength = ReviewMonitorLogDocumentProjection.utf16Length(renderedText) - let sourceBlockLength = ReviewMonitorLogDocumentProjection.utf16Length(block.text) + let sourceBlockLength = ReviewMonitorLogDocumentProjection.utf16Length(sourceText) let blockRange = NSRange( location: previousLength + max(0, suffixLength - blockLength), length: blockLength @@ -375,12 +381,13 @@ struct ReviewMonitorLogDocumentProjection: Sendable { if existingText.hasSuffix("\n\n") { return blockText } - if existingText.hasSuffix("\n") || blockText.hasPrefix("\n") { - return "\n" + blockText - } return "\n\n" + blockText } + private static func normalizedBlockText(_ text: String) -> String { + text.trimmingCharacters(in: .newlines) + } + private static func isVisible(kind: ReviewMonitorLog.Kind, text: String) -> Bool { if kind == .diagnostic { return true diff --git a/Tests/ReviewUITests/ReviewUITests.swift b/Tests/ReviewUITests/ReviewUITests.swift index afe70d19..f0343c75 100644 --- a/Tests/ReviewUITests/ReviewUITests.swift +++ b/Tests/ReviewUITests/ReviewUITests.swift @@ -2959,6 +2959,128 @@ struct ReviewUITests { #expect(visibleText.contains("43sSummarizing files with git") == false) } + @Test func commandOutputWithTrailingNewlinesBeforeMarkdownHeadingKeepsParagraphBoundary() throws { + var projection = ReviewMonitorLogDocumentProjection() + let source = projection.render(projectedBlocks: [ + .init( + id: .init("command_output_1"), + kind: .commandOutput, + groupID: "cmd_1", + text: "stdout\n\n", + metadata: .init( + sourceType: "commandExecution", + status: "completed", + durationMs: 43_000, + commandStatus: "completed" + ) + ), + .init( + id: .init("reasoning_1"), + kind: .rawReasoning, + groupID: "reasoning_1", + text: "**Summarizing files with git**\n\nI need to summarize the files." + ), + ]) + let display = ReviewMonitorCommandOutputDisplayDocument.make(from: source) + + let visibleText = ReviewMonitorCommandOutputDisplayDocument.userVisibleText(from: display.text) + #expect(visibleText.contains("Ran command for 43s\n\nSummarizing files with git")) + #expect(visibleText.contains("43sSummarizing files with git") == false) + #expect(visibleText.contains("43s\n\n\nSummarizing files with git") == false) + } + + @Test func adjacentCommandPanelsStayOnSeparateParagraphs() throws { + let startedAt = Date(timeIntervalSince1970: 200) + var projection = ReviewMonitorLogDocumentProjection() + let source = projection.render(projectedBlocks: [ + .init( + id: .init("command_1"), + kind: .command, + groupID: "cmd_1", + text: "$ git diff", + metadata: .init( + sourceType: "commandExecution", + status: "completed", + itemID: "cmd_1", + command: "git diff", + durationMs: 10_000, + commandStatus: "completed" + ) + ), + .init( + id: .init("command_output_1"), + kind: .commandOutput, + groupID: "cmd_1", + text: "diff\n", + metadata: .init( + sourceType: "commandExecution", + status: "completed", + itemID: "cmd_1", + command: "git diff", + durationMs: 10_000, + commandStatus: "completed" + ) + ), + .init( + id: .init("command_2"), + kind: .command, + groupID: "cmd_2", + text: "$ swift test", + metadata: .init( + sourceType: "commandExecution", + status: "running", + itemID: "cmd_2", + command: "swift test", + startedAt: startedAt, + commandStatus: "running" + ) + ), + ]) + let display = ReviewMonitorCommandOutputDisplayDocument.make( + from: source, + currentDate: Date(timeIntervalSince1970: 203) + ) + + let visibleText = ReviewMonitorCommandOutputDisplayDocument.userVisibleText(from: display.text) + #expect(visibleText.contains("Ran git diff for 10s\n\nRunning swift test")) + #expect(visibleText.contains("10sRunning swift test") == false) + } + + @Test func reasoningWithTrailingNewlinesBeforeCommandKeepsParagraphBoundary() throws { + let startedAt = Date(timeIntervalSince1970: 200) + var projection = ReviewMonitorLogDocumentProjection() + let source = projection.render(projectedBlocks: [ + .init( + id: .init("reasoning_1"), + kind: .rawReasoning, + groupID: "reasoning_1", + text: "Need to inspect files.\n\n" + ), + .init( + id: .init("command_1"), + kind: .command, + groupID: "cmd_1", + text: "$ git diff", + metadata: .init( + sourceType: "commandExecution", + status: "running", + itemID: "cmd_1", + command: "git diff", + startedAt: startedAt, + commandStatus: "running" + ) + ), + ]) + let display = ReviewMonitorCommandOutputDisplayDocument.make( + from: source, + currentDate: Date(timeIntervalSince1970: 203) + ) + + let visibleText = ReviewMonitorCommandOutputDisplayDocument.userVisibleText(from: display.text) + #expect(visibleText.contains("Need to inspect files.\n\nRunning git diff")) + #expect(visibleText.contains("files.Running git diff") == false) + } + @Test func coalescedRunningCommandBeforeMarkdownHeadingKeepsParagraphBoundary() async throws { let startedAt = Date(timeIntervalSince1970: 200) let chat = makeReviewChatFixtureForTesting( From 537a9e4a7335b273a205e7aa48548d9db2838c10 Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Mon, 6 Jul 2026 16:01:26 +0900 Subject: [PATCH 11/18] fix(review-ui): consume reasoning mirror payloads as pairs Adjacency-based mirror suppression missed reasoning mirrors that arrive after an intervening item (for example a command), duplicating the reasoning text, and compared later entries against a stale previous key. Track rendered reasoning entries per scope and text, and let each entry consume exactly one later mirror with the counterpart payload kind, so late mirrors are suppressed while legitimately repeated reasoning still renders. Co-Authored-By: Claude Fable 5 --- .../ReviewMonitorCodexChatLogProjection.swift | 55 +++++++++----- .../ReviewMonitorCodexChatDetailTests.swift | 71 +++++++++++++++++++ 2 files changed, 107 insertions(+), 19 deletions(-) diff --git a/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift b/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift index 7119e830..8e7aa083 100644 --- a/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift +++ b/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift @@ -93,7 +93,7 @@ struct ReviewMonitorCodexChatLogProjection { } let reviewOutputKeys = Set(items.compactMap(Self.reviewOutputKey)) var emittedReviewOutputKeys = Set() - var previousReasoningOutputKey: ReasoningOutputKey? + var pendingReasoningMirrors = PendingReasoningMirrors() let blocks = items.flatMap { item in if let reviewOutputKey = Self.reviewOutputKey(for: item) { guard emittedReviewOutputKeys.insert(reviewOutputKey).inserted else { @@ -101,12 +101,9 @@ struct ReviewMonitorCodexChatLogProjection { } } if let reasoningOutputKey = Self.reasoningOutputKey(for: item) { - guard Self.shouldRenderReasoningOutput(reasoningOutputKey, after: previousReasoningOutputKey) else { + guard pendingReasoningMirrors.shouldRender(reasoningOutputKey) else { return [] as [ReviewMonitorLogProjectedBlock] } - previousReasoningOutputKey = reasoningOutputKey - } else { - previousReasoningOutputKey = nil } return projectedBlocks( from: item, @@ -308,20 +305,6 @@ struct ReviewMonitorCodexChatLogProjection { item.projectionTurnID?.rawValue ?? "item:\(item.sourceID)" } - private static func shouldRenderReasoningOutput( - _ key: ReasoningOutputKey, - after previousKey: ReasoningOutputKey? - ) -> Bool { - guard let previousKey else { - return true - } - guard previousKey.scopeID == key.scopeID, previousKey.text == key.text else { - return true - } - let payloadKinds = Set([previousKey.payloadKind, key.payloadKind].compactMap { $0 }) - return payloadKinds != ["agent_reasoning", "reasoning"] - } - private static func normalizedReviewOutputText(_ text: String) -> String { text.trimmingCharacters(in: .whitespacesAndNewlines) } @@ -633,6 +616,40 @@ private struct ReasoningOutputKey: Hashable { var payloadKind: String? } +// Codex delivers one logical reasoning entry through two payload kinds +// (`agent_reasoning` event and `reasoning` item), and the mirror may arrive +// after unrelated items such as commands. Each rendered entry therefore +// consumes at most one later mirror with the counterpart payload kind, which +// suppresses late mirrors without dropping legitimately repeated reasoning. +private struct PendingReasoningMirrors { + private struct EntryKey: Hashable { + var scopeID: String + var text: String + } + + private static let mirrorPayloadKinds: Set = ["agent_reasoning", "reasoning"] + + private var pendingPayloadKindsByKey: [EntryKey: [String]] = [:] + + mutating func shouldRender(_ key: ReasoningOutputKey) -> Bool { + guard let payloadKind = key.payloadKind, + Self.mirrorPayloadKinds.contains(payloadKind) + else { + return true + } + let entryKey = EntryKey(scopeID: key.scopeID, text: key.text) + var pending = pendingPayloadKindsByKey[entryKey] ?? [] + if let mirrorIndex = pending.firstIndex(where: { $0 != payloadKind }) { + pending.remove(at: mirrorIndex) + pendingPayloadKindsByKey[entryKey] = pending.isEmpty ? nil : pending + return false + } + pending.append(payloadKind) + pendingPayloadKindsByKey[entryKey] = pending + return true + } +} + private struct RawPayloadKind: Decodable { struct NestedItem: Decodable { var type: String? diff --git a/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift b/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift index 3d957a31..16d3c0df 100644 --- a/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift +++ b/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift @@ -806,6 +806,77 @@ struct ReviewMonitorCodexChatDetailTests { #expect(mirroredDocument.revision == initialDocument.revision) } + @Test func codexChatLogProjectionDeduplicatesReasoningMirrorSeparatedByCommand() async throws { + var projection = ReviewMonitorCodexChatLogProjection() + let reasoningText = "Inspecting differences" + let turn = CodexTurnSnapshot( + id: CodexTurnID(rawValue: "turn-review"), + status: .running, + items: [ + .init( + id: "event-reasoning", + kind: .reasoning, + content: .reasoning(.init(summary: reasoningText)), + rawPayload: rawPayload(type: "agent_reasoning") + ), + .init( + id: "command-a", + kind: .commandExecution, + content: .command(.init(command: "/bin/zsh -lc")) + ), + .init( + id: "response-reasoning", + kind: .reasoning, + content: .reasoning(.init(summary: reasoningText)), + rawPayload: rawPayload(type: "reasoning") + ), + ] + ) + + let projectedDocument = projection.render( + from: turn, + chatCreatedAt: nil, + chatUpdatedAt: nil + ) + let document = try #require(projectedDocument) + + #expect(document.blocks.count == 2) + #expect(document.text == "\(reasoningText)\n\n$ /bin/zsh -lc") + } + + @Test func codexChatLogProjectionKeepsRepeatedReasoningAfterMirrorPairConsumed() async throws { + var projection = ReviewMonitorCodexChatLogProjection() + let reasoningText = "Inspecting differences" + func reasoningItem(id: String, payloadType: String) -> CodexThreadItem { + .init( + id: id, + kind: .reasoning, + content: .reasoning(.init(summary: reasoningText)), + rawPayload: rawPayload(type: payloadType) + ) + } + let turn = CodexTurnSnapshot( + id: CodexTurnID(rawValue: "turn-review"), + status: .running, + items: [ + reasoningItem(id: "event-reasoning-1", payloadType: "agent_reasoning"), + reasoningItem(id: "response-reasoning-1", payloadType: "reasoning"), + reasoningItem(id: "event-reasoning-2", payloadType: "agent_reasoning"), + reasoningItem(id: "response-reasoning-2", payloadType: "reasoning"), + ] + ) + + let projectedDocument = projection.render( + from: turn, + chatCreatedAt: nil, + chatUpdatedAt: nil + ) + let document = try #require(projectedDocument) + + #expect(document.blocks.count == 2) + #expect(document.text == "\(reasoningText)\n\n\(reasoningText)") + } + @Test func codexChatLogProjectionRendersUserMessageWithoutReviewModeLog() async throws { var projection = ReviewMonitorCodexChatLogProjection() let turn = CodexTurnSnapshot( From cd82a77f60293a6b0cdcbb4559ccd24114b16b0f Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Mon, 6 Jul 2026 16:01:26 +0900 Subject: [PATCH 12/18] fix(review): pin inline delivery for prepared review restarts Restarted reviews relied on the CodexKit default parameter for the delivery mode. Pass .inline explicitly so the restart path carries the same delivery contract as fresh review starts. Co-Authored-By: Claude Fable 5 --- Sources/CodexReviewAppServer/AppServerCodexReviewBackend.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Sources/CodexReviewAppServer/AppServerCodexReviewBackend.swift b/Sources/CodexReviewAppServer/AppServerCodexReviewBackend.swift index 9032444b..c6ad9a61 100644 --- a/Sources/CodexReviewAppServer/AppServerCodexReviewBackend.swift +++ b/Sources/CodexReviewAppServer/AppServerCodexReviewBackend.swift @@ -238,6 +238,7 @@ package actor AppServerCodexReviewBackend: CodexReviewBackend, CodexModelActor { let review = try await appServer.restartPreparedReview( appServerToken, target: request.request.target.appServerReviewTarget, + delivery: .inline, threadOptions: reviewThreadOptions(model: interruptedRun.model ?? request.model) ) let attemptID = makeAppServerReviewAttemptID() From b5580ac1d29ad1c05135b44eb1ab6eec5825aaec Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Mon, 6 Jul 2026 16:12:13 +0900 Subject: [PATCH 13/18] fix(mcp): import CodexReviewKit for the run ID argument helper ReviewRunIDArgument uses the package-scoped nilIfEmpty extension but relied on leaky member visibility from sibling files' imports. Co-Authored-By: Claude Fable 5 --- Sources/CodexReviewMCPServer/ReviewRunIDArgument.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Sources/CodexReviewMCPServer/ReviewRunIDArgument.swift b/Sources/CodexReviewMCPServer/ReviewRunIDArgument.swift index 30a4dd1c..5206e1b8 100644 --- a/Sources/CodexReviewMCPServer/ReviewRunIDArgument.swift +++ b/Sources/CodexReviewMCPServer/ReviewRunIDArgument.swift @@ -1,3 +1,4 @@ +import CodexReviewKit import MCP enum ReviewRunIDArgument { From 4c79ed515e0ab5254c3f6120013198c5846fcaa4 Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Mon, 6 Jul 2026 16:36:22 +0900 Subject: [PATCH 14/18] fix(review-ui): prefer nested item kind when decoding reasoning payloads A wrapper payload can carry both a top-level event envelope type and the nested item/payload type. Returning the envelope type first hid the reasoning payload kind from mirror deduplication, so wrapped live notifications rendered mirrored reasoning twice. Co-Authored-By: Claude Fable 5 --- .../ReviewMonitorCodexChatLogProjection.swift | 5 ++- .../ReviewMonitorCodexChatDetailTests.swift | 33 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift b/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift index 8e7aa083..2aa11aad 100644 --- a/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift +++ b/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift @@ -665,8 +665,11 @@ private struct RawPayloadKind: Decodable { var item: NestedItem? var payload: NestedItem? + // A nested item/payload kind identifies the item itself; a top-level + // type on the same wrapper is the event envelope kind, so the nested + // kind wins when both are present. var kindValue: String? { - type ?? kind ?? item?.kindValue ?? payload?.kindValue + item?.kindValue ?? payload?.kindValue ?? type ?? kind } } diff --git a/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift b/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift index 16d3c0df..fb56b0f8 100644 --- a/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift +++ b/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift @@ -844,6 +844,39 @@ struct ReviewMonitorCodexChatDetailTests { #expect(document.text == "\(reasoningText)\n\n$ /bin/zsh -lc") } + @Test func codexChatLogProjectionDeduplicatesReasoningMirrorWithWrappedItemPayload() async throws { + var projection = ReviewMonitorCodexChatLogProjection() + let reasoningText = "Inspecting differences" + let turn = CodexTurnSnapshot( + id: CodexTurnID(rawValue: "turn-review"), + status: .running, + items: [ + .init( + id: "event-reasoning", + kind: .reasoning, + content: .reasoning(.init(summary: reasoningText)), + rawPayload: rawPayload(type: "agent_reasoning") + ), + .init( + id: "response-reasoning", + kind: .reasoning, + content: .reasoning(.init(summary: reasoningText)), + rawPayload: Data("{\"type\":\"item.completed\",\"item\":{\"type\":\"reasoning\"}}".utf8) + ), + ] + ) + + let projectedDocument = projection.render( + from: turn, + chatCreatedAt: nil, + chatUpdatedAt: nil + ) + let document = try #require(projectedDocument) + + #expect(document.blocks.count == 1) + #expect(document.text == reasoningText) + } + @Test func codexChatLogProjectionKeepsRepeatedReasoningAfterMirrorPairConsumed() async throws { var projection = ReviewMonitorCodexChatLogProjection() let reasoningText = "Inspecting differences" From 9a37785c8d19c7f323a87d0598704396a551320c Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Mon, 6 Jul 2026 17:00:29 +0900 Subject: [PATCH 15/18] fix(review-ui): keep raw command output in the source document Normalizing block text inside the document builder also rewrote sourceText, so expanded command panels lost leading/trailing newlines and an all-newline output collapsed to an empty block. Move rendered normalization into the styler, keep sourceText raw, and read panel output through the block's source range. Co-Authored-By: Claude Fable 5 --- ...wMonitorCommandOutputDisplayDocument.swift | 16 +++++---- .../ReviewMonitorLogDocumentProjection.swift | 26 +++++++++----- .../Detail/ReviewMonitorLogProjection.swift | 5 +++ Tests/ReviewUITests/ReviewUITests.swift | 36 +++++++++++++++++++ 4 files changed, 69 insertions(+), 14 deletions(-) diff --git a/Sources/ReviewChatLogUI/Detail/ReviewMonitorCommandOutputDisplayDocument.swift b/Sources/ReviewChatLogUI/Detail/ReviewMonitorCommandOutputDisplayDocument.swift index 27c78e28..4ca0eeef 100644 --- a/Sources/ReviewChatLogUI/Detail/ReviewMonitorCommandOutputDisplayDocument.swift +++ b/Sources/ReviewChatLogUI/Detail/ReviewMonitorCommandOutputDisplayDocument.swift @@ -45,6 +45,7 @@ enum ReviewMonitorCommandOutputDisplayDocument { } let sourceString = source.text as NSString + let rawSourceString = source.sourceText as NSString var text = "" var textUTF16Length = 0 var blocks: [ReviewMonitorLog.Block] = [] @@ -110,7 +111,7 @@ enum ReviewMonitorCommandOutputDisplayDocument { ) let outputText = commandOutputText( for: panelSource, - sourceString: sourceString, + rawSourceString: rawSourceString, isExpanded: isExpanded ) let placeholder = commandOutputPlaceholder( @@ -147,7 +148,7 @@ enum ReviewMonitorCommandOutputDisplayDocument { outputSourceRange: panelSource.output?.sourceRange, lineCount: commandOutputLineCount( for: panelSource, - sourceString: sourceString, + rawSourceString: rawSourceString, isExpanded: isExpanded ), isExpanded: isExpanded, @@ -440,9 +441,12 @@ enum ReviewMonitorCommandOutputDisplayDocument { ) } + // Panel output reads the raw source text: rendered block text is + // normalized for stable paragraph boundaries, but expanded panels must + // show the command output exactly as captured. private static func commandOutputLineCount( for panelSource: CommandPanelSource, - sourceString: NSString, + rawSourceString: NSString, isExpanded: Bool ) -> Int { guard isExpanded else { @@ -451,12 +455,12 @@ enum ReviewMonitorCommandOutputDisplayDocument { guard let output = panelSource.output else { return 0 } - return ReviewMonitorLog.LineCounter.lineCount(in: sourceString, range: output.range) + return ReviewMonitorLog.LineCounter.lineCount(in: rawSourceString, range: output.sourceRange) } private static func commandOutputText( for panelSource: CommandPanelSource, - sourceString: NSString, + rawSourceString: NSString, isExpanded: Bool ) -> String { guard isExpanded else { @@ -465,7 +469,7 @@ enum ReviewMonitorCommandOutputDisplayDocument { guard let output = panelSource.output else { return "" } - return sourceString.substring(with: output.range) + return rawSourceString.substring(with: output.sourceRange) } private static func commandOutputTitle( diff --git a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift index 5817834a..db111c96 100644 --- a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift +++ b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift @@ -325,19 +325,19 @@ struct ReviewMonitorLogDocumentProjection: Sendable { return } - // Block text must not carry the inter-block separator: command - // panels replace their block text with a placeholder, so a - // separator living inside a neighboring block's trailing newlines - // would silently disappear from the display document. - let sourceText = Self.normalizedBlockText(block.text) + // Rendered block text must not carry the inter-block separator: + // command panels replace their block text with a placeholder, so + // a separator living inside a neighboring block's trailing + // newlines would silently disappear from the display document. + // Source text stays raw so panels and copy keep output fidelity. let renderedText = Self.normalizedBlockText( ReviewMonitorLogStyler.renderedText( for: block.kind, - source: sourceText, + source: block.text, blockID: block.id )) let appended = appendedText(renderedText, after: document.text) - let appendedSource = appendedText(sourceText, after: document.sourceText) + let appendedSource = appendedSourceText(block.text) hasVisibleSections = true let previousLength = document.textUTF16Length @@ -345,7 +345,7 @@ struct ReviewMonitorLogDocumentProjection: Sendable { let suffixLength = ReviewMonitorLogDocumentProjection.utf16Length(appended) let sourceSuffixLength = ReviewMonitorLogDocumentProjection.utf16Length(appendedSource) let blockLength = ReviewMonitorLogDocumentProjection.utf16Length(renderedText) - let sourceBlockLength = ReviewMonitorLogDocumentProjection.utf16Length(sourceText) + let sourceBlockLength = ReviewMonitorLogDocumentProjection.utf16Length(block.text) let blockRange = NSRange( location: previousLength + max(0, suffixLength - blockLength), length: blockLength @@ -384,6 +384,16 @@ struct ReviewMonitorLogDocumentProjection: Sendable { return "\n\n" + blockText } + private func appendedSourceText(_ blockText: String) -> String { + guard hasVisibleSections else { + return blockText + } + if blockText.isEmpty { + return "\n\n" + } + return "\n\n" + blockText + } + private static func normalizedBlockText(_ text: String) -> String { text.trimmingCharacters(in: .newlines) } diff --git a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogProjection.swift b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogProjection.swift index b1612167..bbe05c7d 100644 --- a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogProjection.swift +++ b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogProjection.swift @@ -563,6 +563,11 @@ enum ReviewMonitorLogStyler { source: String, blockID: ReviewMonitorLog.BlockID ) -> Presentation { + // Rendered block text must not carry outer newlines: the document + // builder owns inter-block paragraph boundaries, and presentation is + // re-derived from the raw source range later, so both paths need + // identical rendered geometry. + let source = source.trimmingCharacters(in: .newlines) switch kind { case .agentMessage, .reasoning, .reasoningSummary, .rawReasoning: return renderMarkdown(source, blockID: blockID) diff --git a/Tests/ReviewUITests/ReviewUITests.swift b/Tests/ReviewUITests/ReviewUITests.swift index f0343c75..37a96343 100644 --- a/Tests/ReviewUITests/ReviewUITests.swift +++ b/Tests/ReviewUITests/ReviewUITests.swift @@ -2989,6 +2989,42 @@ struct ReviewUITests { #expect(visibleText.contains("43s\n\n\nSummarizing files with git") == false) } + @Test func expandedCommandPanelPreservesRawOutputNewlines() throws { + var projection = ReviewMonitorLogDocumentProjection() + let metadata = ReviewMonitorLog.Metadata( + sourceType: "commandExecution", + status: "completed", + itemID: "cmd_1", + command: "git diff", + durationMs: 10_000, + commandStatus: "completed" + ) + let source = projection.render(projectedBlocks: [ + .init( + id: .init("command_1"), + kind: .command, + groupID: "cmd_1", + text: "$ git diff", + metadata: metadata + ), + .init( + id: .init("command_output_1"), + kind: .commandOutput, + groupID: "cmd_1", + text: "\nerror\n", + metadata: metadata + ), + ]) + let display = ReviewMonitorCommandOutputDisplayDocument.make( + from: source, + expandedBlockIDs: [ReviewMonitorLog.BlockID("commandOutput:cmd_1")] + ) + + let panel = try #require(display.commandOutputPanels.first) + #expect(panel.outputText == "\nerror\n") + #expect(panel.lineCount == 2) + } + @Test func adjacentCommandPanelsStayOnSeparateParagraphs() throws { let startedAt = Date(timeIntervalSince1970: 200) var projection = ReviewMonitorLogDocumentProjection() From bf3a65578ca9a8f3df36b302f5db713ea0d958ee Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Mon, 6 Jul 2026 17:01:18 +0900 Subject: [PATCH 16/18] fix(review-ui): apply cross-revision replacements after skipped renders Streaming updates supersede in-flight render tasks, so applied display documents can skip revisions. The scroll view only accepted the single-step lastChange or a pure tail append, so any mid-document change after a skipped revision fell back to a full reload, recreating command panel views and rebuilding TextKit layout on every burst. Recompute the block replacement against the display document that is actually on screen, and log when an incremental render still degrades to a reload. Co-Authored-By: Claude Fable 5 --- .../ReviewMonitorLogDocumentProjection.swift | 2 +- .../Detail/ReviewMonitorLogScrollView.swift | 35 ++++++++++++ Tests/ReviewUITests/ReviewUITests.swift | 56 +++++++++++++++++++ 3 files changed, 92 insertions(+), 1 deletion(-) diff --git a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift index db111c96..a8ea12c2 100644 --- a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift +++ b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift @@ -133,7 +133,7 @@ struct ReviewMonitorLogDocumentProjection: Sendable { return true } - private static func replacementChange( + static func replacementChange( previous: ReviewMonitorLog.Document, current: ReviewMonitorLog.Document ) -> ReviewMonitorLog.Replacement? { diff --git a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogScrollView.swift b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogScrollView.swift index e4c59f3a..55e5a543 100644 --- a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogScrollView.swift +++ b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogScrollView.swift @@ -1,5 +1,8 @@ import AppKit import ObjectiveC.runtime +import OSLog + +private let logger = Logger(subsystem: "CodexReviewKit", category: "log-scroll-view") @MainActor final class ReviewMonitorLogScrollView: NSScrollView { @@ -263,6 +266,19 @@ final class ReviewMonitorLogScrollView: NSScrollView { currentDisplayDocument = display return didRender } + if allowIncrementalUpdate, + let replacement = crossRevisionReplacement(for: display), + canApplyReplacement(replacement, to: display) { + let didRender = applyReplacement(replacement, document: display) + displayedRevision = display.revision + currentDisplayDocument = display + return didRender + } + if allowIncrementalUpdate { + logger.debug( + "Reloading displayed log without an incremental change displayedRevision=\(self.displayedRevision.map(String.init(describing:)) ?? "nil", privacy: .public) revision=\(display.revision, privacy: .public) blocks=\(display.blocks.count, privacy: .public)" + ) + } let didRender = applyReload( display.text, document: display, @@ -274,6 +290,25 @@ final class ReviewMonitorLogScrollView: NSScrollView { return didRender } + // Applied renders can skip revisions when a newer source document + // supersedes an in-flight render, so the single-step lastChange no longer + // describes what is on screen. Recompute the block replacement against + // the display document that was actually applied last. + private func crossRevisionReplacement( + for display: ReviewMonitorLog.Document + ) -> ReviewMonitorLog.Replacement? { + guard let previousDisplay = currentDisplayDocument, + previousDisplay.textUTF16Length == displayedUTF16Length, + previousDisplay.text == displayedText + else { + return nil + } + return ReviewMonitorLogDocumentProjection.replacementChange( + previous: previousDisplay, + current: display + ) + } + private func toggleCommandOutputPanel(_ blockID: ReviewMonitorLog.BlockID) { let restorationTarget = currentScrollRestorationTarget guard let sourceDocument diff --git a/Tests/ReviewUITests/ReviewUITests.swift b/Tests/ReviewUITests/ReviewUITests.swift index 37a96343..c5e3c112 100644 --- a/Tests/ReviewUITests/ReviewUITests.swift +++ b/Tests/ReviewUITests/ReviewUITests.swift @@ -3025,6 +3025,62 @@ struct ReviewUITests { #expect(panel.lineCount == 2) } + @Test func scrollViewAppliesCrossRevisionReplacementAfterSkippedRenders() throws { + let scrollView = ReviewMonitorLogScrollView() + var projection = ReviewMonitorLogDocumentProjection() + let startedAt = Date(timeIntervalSince1970: 200) + func commandBlocks( + status: String, + cwd: String? = nil, + completedAt: Date? = nil, + durationMs: Int? = nil + ) -> [ReviewMonitorLogProjectedBlock] { + [ + .init( + id: .init("reasoning_1"), + kind: .rawReasoning, + groupID: "reasoning_1", + text: "Analyzing the diff." + ), + .init( + id: .init("command_1"), + kind: .command, + groupID: "cmd_1", + text: "$ git diff", + metadata: .init( + sourceType: "commandExecution", + status: status, + itemID: "cmd_1", + command: "git diff", + cwd: cwd, + startedAt: startedAt, + completedAt: completedAt, + durationMs: durationMs, + commandStatus: status + ) + ), + ] + } + + let initial = projection.render(projectedBlocks: commandBlocks(status: "running")) + #expect(scrollView.render(document: initial, restoring: .bottom, allowIncrementalUpdate: false)) + #expect(scrollView.reloadCount == 1) + + // A superseded revision that is never applied to the scroll view. + let skipped = projection.render(projectedBlocks: commandBlocks(status: "running", cwd: "/tmp/project")) + let final = projection.render(projectedBlocks: commandBlocks( + status: "completed", + cwd: "/tmp/project", + completedAt: Date(timeIntervalSince1970: 211), + durationMs: 11_000 + )) + #expect(final.revision == skipped.revision &+ 1) + + #expect(scrollView.render(document: final, restoring: .bottom, allowIncrementalUpdate: true)) + #expect(scrollView.replaceCount == 1) + #expect(scrollView.reloadCount == 1) + } + @Test func adjacentCommandPanelsStayOnSeparateParagraphs() throws { let startedAt = Date(timeIntervalSince1970: 200) var projection = ReviewMonitorLogDocumentProjection() From 0bf7d4ac85820973c545e3464ea081780666ee80 Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Mon, 6 Jul 2026 17:41:39 +0900 Subject: [PATCH 17/18] build(package): commit the remote CodexKit resolution Package.resolved was generated with the local dependencies/CodexKit checkout present, so fresh checkouts resolving the remote fallback revision rewrote the resolved files and broke builds with automatic resolution disabled. Commit the resolved state of a checkout without the local dependency. Co-Authored-By: Claude Fable 5 --- Package.resolved | 10 +++++++++- .../xcshareddata/swiftpm/Package.resolved | 8 ++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/Package.resolved b/Package.resolved index 33a2e255..c52afdd3 100644 --- a/Package.resolved +++ b/Package.resolved @@ -1,6 +1,14 @@ { - "originHash" : "f841cc4390200080cc2f30006b6fa4aaefc669c3275d211221033777fe8c42e5", + "originHash" : "fcdb0a06dff142d609c149d0c2d9f06d33eb0a0f0e61129985312de846d0f6e5", "pins" : [ + { + "identity" : "codexkit", + "kind" : "remoteSourceControl", + "location" : "https://github.com/lynnswap/CodexKit.git", + "state" : { + "revision" : "58c2dab605c3dad806e0ac90c6d3d1f67c5fd36d" + } + }, { "identity" : "eventsource", "kind" : "remoteSourceControl", diff --git a/Tools/ReviewMonitor/CodexReviewMonitor.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved b/Tools/ReviewMonitor/CodexReviewMonitor.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved index e9b8ccf7..fdf4de33 100644 --- a/Tools/ReviewMonitor/CodexReviewMonitor.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved +++ b/Tools/ReviewMonitor/CodexReviewMonitor.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved @@ -1,6 +1,14 @@ { "originHash" : "11d1673255c652381957935c0aa0fcc8f6cba52200a3e3e3c7296d58e6207f76", "pins" : [ + { + "identity" : "codexkit", + "kind" : "remoteSourceControl", + "location" : "https://github.com/lynnswap/CodexKit.git", + "state" : { + "revision" : "58c2dab605c3dad806e0ac90c6d3d1f67c5fd36d" + } + }, { "identity" : "eventsource", "kind" : "remoteSourceControl", From 43b2d58d4e9c99e704aee397f9a2fb1ef7f9562a Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Mon, 6 Jul 2026 18:02:07 +0900 Subject: [PATCH 18/18] fix --- Package.resolved | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/Package.resolved b/Package.resolved index c52afdd3..2adb99a3 100644 --- a/Package.resolved +++ b/Package.resolved @@ -1,14 +1,6 @@ { - "originHash" : "fcdb0a06dff142d609c149d0c2d9f06d33eb0a0f0e61129985312de846d0f6e5", + "originHash" : "6e69018f1a7fb5b827d1f08721f3c54d5891b56910c97b1b90b3f9742625011b", "pins" : [ - { - "identity" : "codexkit", - "kind" : "remoteSourceControl", - "location" : "https://github.com/lynnswap/CodexKit.git", - "state" : { - "revision" : "58c2dab605c3dad806e0ac90c6d3d1f67c5fd36d" - } - }, { "identity" : "eventsource", "kind" : "remoteSourceControl",