diff --git a/.gitignore b/.gitignore index 2da8eaa..86e0893 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 9a802ed..2adb99a 100644 --- a/Package.resolved +++ b/Package.resolved @@ -1,14 +1,6 @@ { - "originHash" : "711aa87f833d42050080f043e999477df164429e02013f117e52aea172c59432", + "originHash" : "6e69018f1a7fb5b827d1f08721f3c54d5891b56910c97b1b90b3f9742625011b", "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 39c8d66..7b8d72c 100644 --- a/Package.swift +++ b/Package.swift @@ -1,7 +1,19 @@ // 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 codexKitFallbackRevision = "58c2dab605c3dad806e0ac90c6d3d1f67c5fd36d" +let codexKitDependency: Package.Dependency = + FileManager.default.fileExists(atPath: "\(localCodexKitPath)/Package.swift") + ? .package(path: localCodexKitPath) + : .package(url: "https://github.com/lynnswap/CodexKit.git", revision: codexKitFallbackRevision) + let package = Package( name: "CodexReviewKit", platforms: [ @@ -33,9 +45,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 0b6a21d..4a0412c 100644 --- a/README.md +++ b/README.md @@ -100,6 +100,28 @@ 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 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 +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 8f720dd..c6ad9a6 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 @@ -237,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() @@ -597,16 +599,20 @@ 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 = reviewCompletionText(for: response) else { + return [.failed("Review completed without review output.")] } return [ .completed(finalReview: finalReview) ] } + 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/CodexReviewKit/Store/CodexReviewStoreReviews.swift b/Sources/CodexReviewKit/Store/CodexReviewStoreReviews.swift index 3763bc6..5929fff 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 f3c4542..32e991a 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 6b5d4d0..515cb0d 100644 --- a/Sources/CodexReviewMCPServer/CodexReviewMCPToolResults.swift +++ b/Sources/CodexReviewMCPServer/CodexReviewMCPToolResults.swift @@ -63,6 +63,7 @@ private extension CodexReviewAPI.Read.Result { func structuredContentForStartOrAwait(log: ReviewMCPLogProjection) -> Value { structuredContent( + includeLog: true, 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), @@ -110,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)), @@ -184,6 +197,7 @@ private extension ReviewMCPLogProjection { return .object(object) } + private static var compactEntryIDLimit: Int { 100 } private static var detailedItemsLimit: Int { 100 } } @@ -467,7 +481,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/Sources/CodexReviewMCPServer/CodexReviewMCPToolSchemas.swift b/Sources/CodexReviewMCPServer/CodexReviewMCPToolSchemas.swift index 05306d9..a72a8a3 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 acf10b0..3e1682c 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 0000000..5206e1b --- /dev/null +++ b/Sources/CodexReviewMCPServer/ReviewRunIDArgument.swift @@ -0,0 +1,44 @@ +import CodexReviewKit +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/Sources/ReviewChatLogUI/Detail/ReviewMonitorCommandOutputDisplayDocument.swift b/Sources/ReviewChatLogUI/Detail/ReviewMonitorCommandOutputDisplayDocument.swift index ac410d6..4ca0eee 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( @@ -1007,7 +1011,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 +1082,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 4f64cd3..a8ea12c 100644 --- a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift +++ b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogDocumentProjection.swift @@ -133,50 +133,175 @@ struct ReviewMonitorLogDocumentProjection: Sendable { return true } - private static func replacementChange( + static func replacementChange( 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, + previousDocument: previous, + currentDocument: current + ) 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 blockContentUnchanged( + previous: previousBlock, + current: currentBlock, + previousDocument: previous, + currentDocument: current + ) { 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 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 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) + private static func unchangedBlockAfterReplacement( + previous: ReviewMonitorLog.Block, + current: ReviewMonitorLog.Block, + displayDelta: Int, + sourceDelta: Int, + previousDocument: ReviewMonitorLog.Document, + currentDocument: ReviewMonitorLog.Document + ) -> Bool { + guard 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 + else { + return false + } + return blockContentUnchanged( + previous: previous, + current: current, + previousDocument: previousDocument, + currentDocument: currentDocument ) - return prefix + replacement + suffix + } + + 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 { + 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( @@ -200,13 +325,19 @@ struct ReviewMonitorLogDocumentProjection: Sendable { return } - let renderedText = ReviewMonitorLogStyler.renderedText( - for: block.kind, - source: block.text, - blockID: block.id - ) + // 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: block.text, + blockID: block.id + )) let appended = appendedText(renderedText, after: document.text) - let appendedSource = appendedText(block.text, after: document.sourceText) + let appendedSource = appendedSourceText(block.text) hasVisibleSections = true let previousLength = document.textUTF16Length @@ -250,12 +381,23 @@ 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 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) + } + private static func isVisible(kind: ReviewMonitorLog.Kind, text: String) -> Bool { if kind == .diagnostic { return true diff --git a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogProjection.swift b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogProjection.swift index 3184696..bbe05c7 100644 --- a/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogProjection.swift +++ b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogProjection.swift @@ -2,6 +2,102 @@ 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, + 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 @@ -467,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/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogScrollView.swift b/Sources/ReviewChatLogUI/Detail/ReviewMonitorLogScrollView.swift index c4312fc..55e5a54 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 @@ -365,8 +400,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 +935,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 22cdf98..2aa11aa 100644 --- a/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift +++ b/Sources/ReviewChatLogUI/ReviewMonitorCodexChatLogProjection.swift @@ -91,13 +91,27 @@ struct ReviewMonitorCodexChatLogProjection { let suppressUserMessages = items.contains { item in item.kind == .enteredReviewMode || item.kind == .exitedReviewMode } - let blocks = items.flatMap { - projectedBlocks( - from: $0, + let reviewOutputKeys = Set(items.compactMap(Self.reviewOutputKey)) + var emittedReviewOutputKeys = Set() + var pendingReasoningMirrors = PendingReasoningMirrors() + 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 pendingReasoningMirrors.shouldRender(reasoningOutputKey) else { + return [] as [ReviewMonitorLogProjectedBlock] + } + } + return projectedBlocks( + from: item, turnStatus: turnStatus, chatCreatedAt: chatCreatedAt, chatUpdatedAt: chatUpdatedAt, - suppressUserMessages: suppressUserMessages + suppressUserMessages: suppressUserMessages, + reviewOutputKeys: reviewOutputKeys ) } guard blocks.isEmpty == false else { @@ -112,13 +126,20 @@ struct ReviewMonitorCodexChatLogProjection { turnStatus: CodexTurnStatus?, chatCreatedAt: Date?, chatUpdatedAt: Date?, - suppressUserMessages: Bool + suppressUserMessages: Bool, + reviewOutputKeys: Set ) -> [ReviewMonitorLogProjectedBlock] { switch item.content { case .message(let message): guard suppressUserMessages == false || message.role != .user else { return [] } + if message.role == .assistant, + let reviewOutputKey = Self.reviewOutputKey(for: item, text: message.text), + reviewOutputKeys.contains(reviewOutputKey) + { + return [] + } return [ projectedBlock( item, @@ -234,6 +255,73 @@ struct ReviewMonitorCodexChatLogProjection { } } + private static func reviewOutputKey(for item: Item) -> ReviewOutputKey? { + guard item.kind == .exitedReviewMode else { + return nil + } + switch item.content { + case .message(let message): + return reviewOutputKey(for: item, text: message.text) + case .diagnostic(let text), .log(let text): + return reviewOutputKey(for: item, text: text) + case .unknown(let raw): + 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 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 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 @@ -517,6 +605,74 @@ struct ReviewMonitorCodexChatLogProjection { } } +private struct ReviewOutputKey: Hashable { + var scopeID: String + var text: String +} + +private struct ReasoningOutputKey: Hashable { + var scopeID: String + var text: String + 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? + var kind: String? + + var kindValue: String? { + type ?? kind + } + } + + var type: String? + var kind: String? + 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? { + item?.kindValue ?? payload?.kindValue ?? type ?? kind + } +} + @MainActor private protocol CodexChatLogProjectionItem { var itemID: String { get } @@ -526,6 +682,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 { @@ -578,6 +735,10 @@ private struct CodexChatModelLogItem: CodexChatLogProjectionItem { var itemStatus: CodexTurnStatus? { item.content.reviewMonitorLogItemStatus } + + var rawPayload: Data? { + item.rawPayload + } } @MainActor @@ -617,6 +778,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/CodexReviewAppServerTests/AppServerClientTests.swift b/Tests/CodexReviewAppServerTests/AppServerClientTests.swift index d1a8afb..bd00a8d 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") } @@ -81,14 +82,69 @@ struct AppServerClientTests { ) ) try await runtime.transport.emitServerNotification( - method: "item/agentMessage/delta", - params: TestDeltaNotification( + method: "turn/completed", + params: TestTurnNotification( + threadID: "review-thread", + turn: .init( + id: "turn-1", + status: "completed", + items: [ + .exitedReviewMode(id: "review-output", review: "Looks good.") + ] + ) + ) + ) + + #expect( + try await iterator.next() == .started(turnID: "turn-1", reviewThreadID: "review-thread", model: "gpt-5")) + #expect(try await iterator.next() == .completed(finalReview: "Looks good.")) + } + + @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") + 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/completed", + params: TestItemNotification( threadID: "review-thread", turnID: "turn-1", - itemID: "msg-1", - delta: "Looks good." + 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 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( @@ -97,7 +153,7 @@ struct AppServerClientTests { id: "turn-1", status: "completed", items: [ - .finalAnswer(id: "msg-1", text: "Looks good.") + .finalAnswer(id: "assistant-final", text: "No issues found.") ] ) ) @@ -105,10 +161,10 @@ 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() == .completed(finalReview: "No issues found.")) } - @Test func backendCompletesReviewFromFinalAnswerDeltaWhenTerminalPayloadIsSparse() async throws { + @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") @@ -120,9 +176,8 @@ struct AppServerClientTests { try await runtime.transport.emitServerNotification( method: "item/agentMessage/delta", - params: TestDeltaNotification( + params: TestDeltaWithoutTurnIDNotification( threadID: "review-thread", - turnID: "turn-1", itemID: "msg-1", delta: "Looks good.", phase: "final_answer" @@ -138,10 +193,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 backendFailsCompletedReviewWithoutFinalAnswer() async throws { + @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 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 +243,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 +284,7 @@ struct AppServerClientTests { id: "turn-1", status: "completed", items: [ - .finalAnswer(id: "msg-1", text: "first") + .exitedReviewMode(id: "review-output-1", review: "first") ] ) ) @@ -215,7 +297,7 @@ struct AppServerClientTests { id: "turn-2", status: "completed", items: [ - .finalAnswer(id: "msg-1", text: "second") + .exitedReviewMode(id: "review-output-2", review: "second") ] ) ) @@ -276,7 +358,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 +742,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 +818,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 d99e36a..64501b3 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,7 @@ struct CodexReviewMCPHTTPServerTests { #expect( resolved.value(for: ["result", "structuredContent", "review", "reviewResult", "state"]) as? String == "noFindings") + assertCompactLog(resolved, total: 2) let commands = await backend.recordedCommands() #expect( commands.contains( @@ -193,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( @@ -205,7 +284,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([ @@ -232,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) + assertCompactLog(running, total: 1) #expect( running.value(for: ["result", "structuredContent", "nextAction", "tool"]) as? String == "review_await") @@ -266,10 +362,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 - == nil) - #expect(awaited.value(for: ["result", "structuredContent", "log", "finalResult"]) is NSNull) + assertCompactLog(awaited, total: 1) #expect(awaited.value(for: ["result", "structuredContent", "logs"]) == nil) } } @@ -455,7 +548,7 @@ struct CodexReviewMCPHTTPServerTests { "method": "tools/call", "params": [ "name": "review_read", - "arguments": ["runId": "run-in-session"], + "arguments": ["jobId": "run-in-session"], ], ] ) @@ -468,7 +561,7 @@ struct CodexReviewMCPHTTPServerTests { "method": "tools/call", "params": [ "name": "review_read", - "arguments": ["runID": "run-other-session"], + "arguments": ["jobID": "run-other-session"], ], ] ) @@ -486,6 +579,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 +1224,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 @@ -1328,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 diff --git a/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift b/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift index c0adff1..fb56b0f 100644 --- a/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift +++ b/Tests/ReviewUITests/ReviewMonitorCodexChatDetailTests.swift @@ -487,6 +487,429 @@ 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 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 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 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 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" + 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( @@ -796,6 +1219,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 69094a0..c5e3c11 100644 --- a/Tests/ReviewUITests/ReviewUITests.swift +++ b/Tests/ReviewUITests/ReviewUITests.swift @@ -2831,6 +2831,348 @@ 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 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: [ + .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 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 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 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() + 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( 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 a100014..fdf4de3 100644 --- a/Tools/ReviewMonitor/CodexReviewMonitor.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved +++ b/Tools/ReviewMonitor/CodexReviewMonitor.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved @@ -1,12 +1,12 @@ { - "originHash" : "090f3e640b9c9ff9c1ef794fed79ee1dcdb35f013243ff248225a302aebbbb8a", + "originHash" : "11d1673255c652381957935c0aa0fcc8f6cba52200a3e3e3c7296d58e6207f76", "pins" : [ { "identity" : "codexkit", "kind" : "remoteSourceControl", "location" : "https://github.com/lynnswap/CodexKit.git", "state" : { - "revision" : "d2a694d7f633c1f01d4d260585863c0a3eae21af" + "revision" : "58c2dab605c3dad806e0ac90c6d3d1f67c5fd36d" } }, {