feat(terminal): add user cancellation tracking and ANSI stripping for terminal output #27
Conversation
… terminal output - Add cancelledByUser flag to ManagedSession for tracking user-initiated cancellations - Add markSessionCancelledByUser() method to ShellSessionManager for non-suspend context - Register sessions to ShellSessionManager in ToolOrchestrator for cancel event handling - Sync output to ManagedSession in PtyShellExecutor.waitForSession() - Add AnsiStripper utility to clean ANSI escape sequences from terminal output - Strip ANSI codes before sending output to AI for cleaner, readable text - Update CodingAgentRenderer interface with cancelledByUser parameter - Update all renderer implementations (JewelRenderer, ComposeRenderer, CodingCliRenderer) - Include captured output in error messages when commands timeout or fail
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds ANSI-stripping utilities, live PTY session registration and cancellation tracking, integrates managed sessions into PTY orchestration and execution, propagates a cancelledByUser flag through execution results and renderers, and enriches error/result flows with cleaned output and cancellation metadata. Changes
Sequence DiagramsequenceDiagram
actor User
participant IDE as IDE / ViewModel
participant ToolOrch as ToolOrchestrator
participant PtyExec as PtyShellExecutor
participant SessionMgr as ShellSessionManager
participant Ansi as AnsiStripper
participant Renderer as Renderer
User->>IDE: start shell command
IDE->>ToolOrch: executeShellTool(...)
ToolOrch->>SessionMgr: registerLiveSession(sessionId, handlers)
ToolOrch->>PtyExec: start PTY process
PtyExec->>SessionMgr: forward output lines to ManagedSession
PtyExec->>ToolOrch: notify async session started (background)
User->>IDE: cancel command
IDE->>SessionMgr: markSessionCancelledByUser(sessionId)
IDE->>PtyExec: kill PTY process
PtyExec-->>ToolOrch: completion/exception + collected output
ToolOrch->>Ansi: strip(output)
ToolOrch->>SessionMgr: query cancelledByUser(sessionId)
ToolOrch->>Renderer: updateLiveTerminalStatus(sessionId, exitCode, time, cleanedOutput, cancelledByUser)
Renderer->>Renderer: build CANCELLED_BY_USER or success/error result
Renderer-->>IDE: render result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.kt📄 CodeRabbit inference engine (AGENTS.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.kt (1)
413-416: LGTM! Correct ordering of cancellation marking before process termination.The session is properly marked as cancelled by user before destroying the process, which ensures
ToolOrchestrator.startSessionMonitoring()can detect the user-initiated cancellation.Consider adding an import for
ShellSessionManagerto avoid the fully-qualified name:import cc.unitmesh.agent.tool.shell.ShellSessionManagerThen simplify line 415 to:
ShellSessionManager.markSessionCancelledByUser(cancelEvent.sessionId)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/shell/AnsiStripper.kt (1)
108-116: Redundant\rhandling instripAndNormalize.The
strip()function already removes all carriage return characters (\r) on lines 54-57. Therefore, calling.replace("\r\n", "\n").replace("\r", "")afterstrip()is redundant since the returned string will never contain\r.If the intent is to preserve
\r\nas\n(normalize before stripping), the order should be reversed:fun stripAndNormalize(text: String): String { - return strip(text) - .replace("\r\n", "\n") - .replace("\r", "") + return strip( + text.replace("\r\n", "\n") + .replace("\r", "") + ) }Or, if the current behavior (strip all
\r) is intended, simplify to:fun stripAndNormalize(text: String): String { - return strip(text) - .replace("\r\n", "\n") - .replace("\r", "") + return strip(text) }mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/CodingAgentRenderer.kt (1)
53-60: KDoc comment may be misleading about exit code 137.The comment states that
cancelledByUsercorresponds to exit code 137 (SIGKILL), but user cancellation could result in different exit codes depending on the signal used (e.g., SIGTERM=143, SIGINT=130). ThecancelledByUserflag should be independent of the specific exit code.Consider updating the documentation:
- * @param cancelledByUser Whether the command was cancelled by the user (exit code 137) + * @param cancelledByUser Whether the command was cancelled by the usermpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/CodingCli.kt (1)
294-342: Consider extracting duplicate cancellation result construction.The cancellation error message construction (lines 300-320) and result creation is nearly identical to the code in
awaitProcess(lines 418-438). This duplication could lead to inconsistencies if one is updated but not the other.Consider extracting a helper function:
private fun createCancelledByUserResult( command: String, exitCode: Int, sessionId: String, output: String, executionTimeMs: Long? = null ): ToolResult.Error { val message = buildString { appendLine("⚠️ Command cancelled by user") appendLine() appendLine("Command: $command") appendLine("Exit code: $exitCode (SIGKILL)") appendLine() if (output.isNotEmpty()) { appendLine("Output before cancellation:") appendLine(output) } else { appendLine("(no output captured before cancellation)") } } val metadata = mutableMapOf( "exit_code" to exitCode.toString(), "session_id" to sessionId, "cancelled" to "true", "output" to output ) executionTimeMs?.let { metadata["execution_time_ms"] = it.toString() } return ToolResult.Error(message, errorType = "CANCELLED_BY_USER", metadata = metadata) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/orchestrator/ToolOrchestrator.kt(4 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/CodingAgentRenderer.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/shell/AnsiStripper.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/shell/ShellSessionManager.kt(2 hunks)mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/tool/shell/PtyShellExecutor.kt(2 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt(2 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.kt(2 hunks)mpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/CodingCli.kt(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualfor platform-specific code (e.g., file I/O on JVM/JS/Wasm) in Kotlin Multiplatform projects
Check export first if some functions are not working well with CLI (TypeScript)
In Kotlin/JS @JsExport: AvoidFlow, usePromiseinstead
In Kotlin/JS @JsExport: Use concrete classes as return types and parameter types; avoid interface types
For WASM platform, avoid using emoji and UTF-8 in code
Files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/shell/ShellSessionManager.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/shell/AnsiStripper.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/tool/shell/PtyShellExecutor.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/CodingAgentRenderer.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/CodingCli.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/orchestrator/ToolOrchestrator.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt
**/idea/**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/idea/**/*.kt: For SwingPanel z-index issues with Compose popups, enable Jewel's custom popup renderer:JewelFlags.useCustomPopupRenderer = trueinIdeaAgentToolWindowFactory
For popup/dropdown menus in IntelliJ plugins, use Jewel'sPopupMenuinstead ofandroidx.compose.ui.window.Popup
For dialogs in IntelliJ plugins, use IntelliJ'sDialogWrapperwithorg.jetbrains.jewel.bridge.composeinstead ofandroidx.compose.ui.window.Dialog
Files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt
**/compose/**/*.{kt,kts}
📄 CodeRabbit inference engine (AGENTS.md)
In Compose (Desktop/Android), use
AutoDevColorsfromcc.unitmesh.devins.ui.compose.themeorMaterialTheme.colorScheme
Files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.kt
{**/compose/**/*.kt,mpp-ui/src/jsMain/typescript/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (AGENTS.md)
DO NOT hardcode colors (e.g.,
Color(0xFF...)or#hex). Always use design tokens for consistency across platforms
Files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.kt
🧬 Code graph analysis (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/orchestrator/ToolOrchestrator.kt (2)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/logging/LoggingInitializer.kt (2)
getLogger(93-98)getLogger(100-105)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/logging/AutoDevLogger.kt (1)
getLogger(38-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Agent
- GitHub Check: Test mpp-core (KMP)
- GitHub Check: Build (223)
- GitHub Check: Build (241)
🔇 Additional comments (10)
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/tool/shell/PtyShellExecutor.kt (1)
307-314: LGTM! Proper completion sequence with output synchronization.The implementation correctly:
- Waits for process completion with timeout
- Allows output reading job to finish with a short timeout
- Marks both the
LiveShellSessionandManagedSessionas completedThe 1-second timeout for output reading is a reasonable safeguard.
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/shell/ShellSessionManager.kt (1)
118-122: LGTM on the property addition conceptually.The
cancelledByUserflag provides a clear semantic distinction between user-initiated cancellations and other failure modes. The thread-safety concern for this property is addressed in the comment above.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/shell/AnsiStripper.kt (1)
21-66: LGTM! Solid ANSI escape sequence stripping implementation.The implementation correctly handles:
- CSI sequences (ESC[...X) for colors, cursor movement
- OSC sequences (ESC]...BEL/ST) for window titles
- Character set selection (ESC(X, ESC)X)
- Simple escape sequences
The early return optimization on line 22-24 is a nice touch for performance.
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.kt (1)
450-501: LGTM! Clean implementation of cancellation-aware terminal status updates.The changes properly:
- Add
cancelledByUserparameter to distinguish user-initiated cancellations- Generate appropriate error messages based on the cancellation source
- Include
cancelledandoutputin the metadata for downstream consumersThe branching logic on lines 482-487 provides clear, user-friendly messaging.
mpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/CodingCli.kt (1)
220-238: LGTM!The status symbol and message logic correctly handles the three states (cancelled, success, failure) with appropriate visual indicators.
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/orchestrator/ToolOrchestrator.kt (4)
116-129: LGTM on session registration logic.The session is properly registered with
ShellSessionManagerbefore the renderer is notified. The process handlers (isAlive,kill) are correctly delegated to theLiveShellSession, enabling proper cancellation control.
296-311: Clean implementation of ANSI stripping and cancellation propagation.The output is properly sanitized before being sent to the renderer, and the cancellation status is correctly retrieved from the managed session and propagated.
314-355: Good defensive error handling with output capture.The error path properly:
- Retrieves the managed session to check cancellation status
- Falls back through multiple output sources (managedSession → liveSession)
- Strips ANSI codes before constructing the error message
- Includes helpful debug logging for troubleshooting
This ensures the AI receives useful context even when errors occur.
373-394: LGTM on the when expression simplification.The inline
toolName.toToolType()call is clean and readable.mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt (1)
455-456: LGTM on the API update.The method signature correctly accepts the new
cancelledByUserparameter with proper integration into the result construction logic.
|
|
||
| /** | ||
| * Mark a session as cancelled by user. | ||
| * This is a non-suspend function for use in UI callbacks where coroutine context may not be available. | ||
| * Note: This accesses the sessions map without locking, which is safe for this specific use case | ||
| * because we're only setting a boolean flag on an existing session. | ||
| */ | ||
| fun markSessionCancelledByUser(sessionId: String) { | ||
| sessions[sessionId]?.cancelledByUser = true | ||
| } |
There was a problem hiding this comment.
Data race: accessing sessions map without synchronization.
The comment claims this is safe because "we're only setting a boolean flag," but the real issue is that sessions[sessionId] performs an unsynchronized read on a mutableMapOf (backed by HashMap) which is not thread-safe. Concurrent calls to registerSession, removeSession, or getSession could cause:
ConcurrentModificationException- Reading stale or corrupted data
- The session being removed between the read and the flag assignment
Additionally, cancelledByUser is not @Volatile, so the write may not be visible to other threads.
Consider one of these fixes:
Option 1: Use @Volatile for the flag and synchronize map access:
- fun markSessionCancelledByUser(sessionId: String) {
- sessions[sessionId]?.cancelledByUser = true
- }
+ fun markSessionCancelledByUser(sessionId: String) {
+ synchronized(sessions) {
+ sessions[sessionId]?.cancelledByUser = true
+ }
+ }And in ManagedSession:
- var cancelledByUser: Boolean = false
+ @Volatile var cancelledByUser: Boolean = falseOption 2: Use a concurrent map:
-private val sessions = mutableMapOf<String, ManagedSession>()
+private val sessions = java.util.concurrent.ConcurrentHashMap<String, ManagedSession>()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Mark a session as cancelled by user. | |
| * This is a non-suspend function for use in UI callbacks where coroutine context may not be available. | |
| * Note: This accesses the sessions map without locking, which is safe for this specific use case | |
| * because we're only setting a boolean flag on an existing session. | |
| */ | |
| fun markSessionCancelledByUser(sessionId: String) { | |
| sessions[sessionId]?.cancelledByUser = true | |
| } | |
| /** | |
| * Mark a session as cancelled by user. | |
| * This is a non-suspend function for use in UI callbacks where coroutine context may not be available. | |
| * Note: This accesses the sessions map without locking, which is safe for this specific use case | |
| * because we're only setting a boolean flag on an existing session. | |
| */ | |
| fun markSessionCancelledByUser(sessionId: String) { | |
| synchronized(sessions) { | |
| sessions[sessionId]?.cancelledByUser = true | |
| } | |
| } |
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/tool/shell/PtyShellExecutor.kt
Outdated
Show resolved
Hide resolved
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR adds user cancellation tracking for terminal commands and implements ANSI escape sequence stripping to provide clean output to AI models. The changes span multiple components including session management, shell execution, rendering layers, and output processing.
Key Changes:
- Added
cancelledByUserflag toManagedSessionfor distinguishing user cancellations from command failures - Implemented
AnsiStripperutility to remove terminal control sequences (colors, cursor movements) from output - Updated all renderer implementations to handle cancellation status and display appropriate user feedback
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
AnsiStripper.kt |
New utility for stripping ANSI escape sequences from terminal output |
ShellSessionManager.kt |
Added cancellation tracking with cancelledByUser flag and markSessionCancelledByUser() method |
PtyShellExecutor.kt |
Added output synchronization to ManagedSession during command execution |
ToolOrchestrator.kt |
Integrated ANSI stripping and cancellation detection in session monitoring |
CodingAgentRenderer.kt |
Updated interface to include cancelledByUser parameter in updateLiveTerminalStatus() |
CodingCli.kt |
Implemented cancellation handling with special status symbols and detailed error messages |
JewelRenderer.kt |
Added cancellation-aware result handling with captured output |
ComposeRenderer.kt |
Updated to distinguish user cancellations from command failures |
IdeaAgentViewModel.kt |
Marks sessions as cancelled before terminating processes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| message = buildString { | ||
| appendLine("⚠️ Command cancelled by user") | ||
| appendLine() | ||
| appendLine("Command: ${session.command}") | ||
| appendLine("Exit code: $exitCode (SIGKILL)") | ||
| appendLine() | ||
| if (output.isNotEmpty()) { | ||
| appendLine("Output before cancellation:") | ||
| appendLine(output) | ||
| } else { | ||
| appendLine("(no output captured before cancellation)") | ||
| } | ||
| }, |
There was a problem hiding this comment.
Code Duplication: Similar to the first occurrence, this error message construction is duplicated. Both instances (lines 301-313 and 418-430) build essentially the same cancelled message structure.
See the previous comment for the suggested helper function to eliminate this duplication.
| val errorMessage = buildString { | ||
| appendLine("⚠️ Command cancelled by user") | ||
| appendLine() | ||
| appendLine("Exit code: $exitCode (SIGKILL)") | ||
| appendLine() | ||
| if (!output.isNullOrEmpty()) { | ||
| appendLine("Output before cancellation:") | ||
| appendLine(output) | ||
| } else { | ||
| appendLine("(no output captured before cancellation)") | ||
| } | ||
| } |
There was a problem hiding this comment.
Code Duplication: The cancelled command error message building is also duplicated in JewelRenderer (lines 483-494). This is the third instance of this pattern across different renderers.
Solution: Consider extracting this to a shared utility function in the CodingAgentRenderer interface or a common utility class, so all renderers can use the same logic.
| // Use new ExecutableTool architecture for most tools | ||
| // Only special-case tools that need custom handling (shell with PTY, etc.) | ||
| return when (val toolType = toolName.toToolType()) { | ||
| return when (toolName.toToolType()) { |
There was a problem hiding this comment.
[nitpick] Unnecessary Variable: The toolType variable was removed but the code still works because the when expression now directly evaluates toolName.toToolType(). However, if this method is called multiple times in error paths or has side effects, it could be inefficient.
While this change works, if toToolType() is expensive or has side effects, consider keeping the variable. Otherwise, this is a minor improvement.
| return when (toolName.toToolType()) { | |
| val toolType = toolName.toToolType() | |
| return when (toolType) { |
| // Start output reading job to sync to ManagedSession | ||
| // This ensures output is available for ToolOrchestrator.startSessionMonitoring() | ||
| // Note: For IDEA, ProcessOutputCollector in IdeaLiveTerminalBubble also reads output, | ||
| // but that's OK because they read from different streams or the same stream is already consumed. | ||
| val outputJob = launch { | ||
| try { | ||
| ptyHandle.inputStream.bufferedReader().use { reader -> | ||
| var line = reader.readLine() | ||
| while (line != null && isActive) { | ||
| val lineWithNewline = line + "\n" | ||
| // Sync to LiveShellSession (for backward compatibility) | ||
| session.appendStdout(lineWithNewline) | ||
| // Sync to ManagedSession (for ToolOrchestrator) | ||
| managedSession?.appendOutput(lineWithNewline) |
There was a problem hiding this comment.
Potential Data Race: Multiple coroutines may be reading from the same PTY input stream concurrently. The comment mentions "ProcessOutputCollector in IdeaLiveTerminalBubble also reads output", but reading from the same InputStream concurrently is unsafe. Only one reader should consume from ptyHandle.inputStream at a time, or the output will be split unpredictably between readers.
Solution: Either:
- Ensure only one reader exists per process (coordinate between this code and ProcessOutputCollector)
- Use a broadcast/multicast pattern where one reader distributes to multiple consumers
- Document clearly which component is responsible for reading in each scenario
| message = buildString { | ||
| appendLine("⚠️ Command cancelled by user") | ||
| appendLine() | ||
| appendLine("Command: ${session.command}") | ||
| appendLine("Exit code: $exitCode (SIGKILL)") | ||
| appendLine() | ||
| if (output.isNotEmpty()) { | ||
| appendLine("Output before cancellation:") | ||
| appendLine(output) | ||
| } else { | ||
| appendLine("(no output captured before cancellation)") | ||
| } | ||
| }, |
There was a problem hiding this comment.
Code Duplication: The error message building logic for cancelled commands is duplicated in two places (lines 301-313 and 418-430). This same pattern appears with nearly identical structure and content.
Solution: Extract this into a helper function:
private fun buildCancelledMessage(command: String, exitCode: Int, output: String): String {
return buildString {
appendLine("⚠️ Command cancelled by user")
appendLine()
appendLine("Command: $command")
appendLine("Exit code: $exitCode (SIGKILL)")
appendLine()
if (output.isNotEmpty()) {
appendLine("Output before cancellation:")
appendLine(output)
} else {
appendLine("(no output captured before cancellation)")
}
}
}| package cc.unitmesh.agent.tool.shell | ||
|
|
||
| /** | ||
| * Utility object for stripping ANSI escape sequences from terminal output. | ||
| * This converts raw terminal output with color codes, cursor movements, etc. | ||
| * into clean, readable ASCII text. | ||
| */ | ||
| object AnsiStripper { | ||
| private const val ESC = '\u001B' | ||
|
|
||
| /** | ||
| * Strip all ANSI escape sequences from the given text. | ||
| * Handles: | ||
| * - CSI sequences (ESC[...X) - colors, cursor movement, erase | ||
| * - OSC sequences (ESC]...BEL/ST) - window title, etc. | ||
| * - Simple escape sequences (ESC X) | ||
| * | ||
| * @param text The text containing ANSI escape sequences | ||
| * @return Clean text with all escape sequences removed | ||
| */ | ||
| fun strip(text: String): String { | ||
| if (!text.contains(ESC)) { | ||
| return text | ||
| } | ||
|
|
||
| val result = StringBuilder() | ||
| var i = 0 | ||
|
|
||
| while (i < text.length) { | ||
| val ch = text[i] | ||
|
|
||
| when { | ||
| ch == ESC && i + 1 < text.length -> { | ||
| val next = text[i + 1] | ||
| when (next) { | ||
| '[' -> { | ||
| // CSI sequence: ESC[...X (ends with a letter) | ||
| i = skipCsiSequence(text, i + 2) | ||
| } | ||
| ']' -> { | ||
| // OSC sequence: ESC]...BEL or ESC]...ST | ||
| i = skipOscSequence(text, i + 2) | ||
| } | ||
| '(' , ')' -> { | ||
| // Character set selection: ESC(X or ESC)X | ||
| i = if (i + 2 < text.length) i + 3 else text.length | ||
| } | ||
| else -> { | ||
| // Simple escape sequence: ESC X | ||
| i += 2 | ||
| } | ||
| } | ||
| } | ||
| ch == '\r' -> { | ||
| // Carriage return - skip it (will be handled with newlines) | ||
| i++ | ||
| } | ||
| else -> { | ||
| result.append(ch) | ||
| i++ | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return result.toString() | ||
| } | ||
|
|
||
| /** | ||
| * Skip a CSI sequence starting at the given position. | ||
| * CSI sequences end with a letter (0x40-0x7E). | ||
| */ | ||
| private fun skipCsiSequence(text: String, start: Int): Int { | ||
| var i = start | ||
| while (i < text.length) { | ||
| val ch = text[i] | ||
| if (ch in '@'..'~') { | ||
| // Found the terminating character | ||
| return i + 1 | ||
| } | ||
| i++ | ||
| } | ||
| return text.length | ||
| } | ||
|
|
||
| /** | ||
| * Skip an OSC sequence starting at the given position. | ||
| * OSC sequences end with BEL (0x07) or ST (ESC\). | ||
| */ | ||
| private fun skipOscSequence(text: String, start: Int): Int { | ||
| var i = start | ||
| while (i < text.length) { | ||
| val ch = text[i] | ||
| when { | ||
| ch == '\u0007' -> { | ||
| // BEL character terminates OSC | ||
| return i + 1 | ||
| } | ||
| ch == ESC && i + 1 < text.length && text[i + 1] == '\\' -> { | ||
| // ST (String Terminator) terminates OSC | ||
| return i + 2 | ||
| } | ||
| } | ||
| i++ | ||
| } | ||
| return text.length | ||
| } | ||
|
|
||
| /** | ||
| * Strip ANSI sequences and also normalize line endings. | ||
| * Converts \r\n to \n and removes standalone \r. | ||
| */ | ||
| fun stripAndNormalize(text: String): String { | ||
| return strip(text) | ||
| .replace("\r\n", "\n") | ||
| .replace("\r", "") | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing Test Coverage: The new AnsiStripper utility is not covered by tests. Given the complexity of ANSI escape sequence handling (CSI, OSC, character sets, etc.) and the presence of a critical bug (infinite loop in skipOscSequence), this component requires comprehensive test coverage.
Recommendation: Add tests covering:
- CSI sequences (colors, cursor movement)
- OSC sequences (with BEL and ST terminators)
- Character set selection sequences
- Edge cases (malformed sequences, unterminated sequences)
- The
stripAndNormalize()method
| /** | ||
| * Mark a session as cancelled by user. | ||
| * This is a non-suspend function for use in UI callbacks where coroutine context may not be available. | ||
| * Note: This accesses the sessions map without locking, which is safe for this specific use case | ||
| * because we're only setting a boolean flag on an existing session. | ||
| */ | ||
| fun markSessionCancelledByUser(sessionId: String) { | ||
| sessions[sessionId]?.cancelledByUser = true | ||
| } |
There was a problem hiding this comment.
Missing Test Coverage: The cancelledByUser flag and markSessionCancelledByUser() method are not covered by tests. Given the thread-safety concerns with this implementation and its critical role in distinguishing user cancellations from other failures, this functionality needs test coverage.
Recommendation: Add tests verifying:
- Setting and retrieving the
cancelledByUserflag - Thread-safe access to the flag
- Integration with the cancellation flow
| * This is a non-suspend function for use in UI callbacks where coroutine context may not be available. | ||
| * Note: This accesses the sessions map without locking, which is safe for this specific use case | ||
| * because we're only setting a boolean flag on an existing session. | ||
| */ | ||
| fun markSessionCancelledByUser(sessionId: String) { | ||
| sessions[sessionId]?.cancelledByUser = true |
There was a problem hiding this comment.
Thread Safety Issue: The markSessionCancelledByUser() method accesses the mutable sessions map without acquiring the mutex lock. While the comment claims this is "safe" because it only sets a boolean flag, this is actually not thread-safe. The map itself could be modified (e.g., by removeSession() or registerSession()) concurrently, which could cause:
- A
ConcurrentModificationExceptionon the JVM - Undefined behavior on other platforms
- The session might be removed between the map access and the flag set
Solution: Either:
- Make
cancelledByUseranAtomicBooleanor use another thread-safe mechanism - Keep the method as suspend and use
mutex.withLock { sessions[sessionId]?.cancelledByUser = true }
| * This is a non-suspend function for use in UI callbacks where coroutine context may not be available. | |
| * Note: This accesses the sessions map without locking, which is safe for this specific use case | |
| * because we're only setting a boolean flag on an existing session. | |
| */ | |
| fun markSessionCancelledByUser(sessionId: String) { | |
| sessions[sessionId]?.cancelledByUser = true | |
| * This is a suspend function and must be called from a coroutine context. | |
| * The access to the sessions map is now protected by the mutex for thread safety. | |
| */ | |
| suspend fun markSessionCancelledByUser(sessionId: String) { | |
| mutex.withLock { | |
| sessions[sessionId]?.cancelledByUser = true | |
| } |
| // OSC sequence: ESC]...BEL or ESC]...ST | ||
| i = skipOscSequence(text, i + 2) | ||
| } | ||
| '(' , ')' -> { |
There was a problem hiding this comment.
Spacing Issue: Extra space in the when branch between '(' and ','. This should be '(', ')' without the space for consistency with Kotlin formatting conventions.
'(', ')' -> {| '(' , ')' -> { | |
| '(', ')' -> { |
| val statusMessage = when { | ||
| cancelledByUser -> "Cancelled by user" | ||
| exitCode == 0 -> "Exit code: $exitCode" | ||
| else -> "Exit code: $exitCode" |
There was a problem hiding this comment.
Redundant status message logic: Lines 229-233 have identical logic for both the success case (exitCode == 0) and the else case. Both branches produce "Exit code: $exitCode", making the distinction unnecessary.
Solution: Simplify to:
val statusMessage = if (cancelledByUser) {
"Cancelled by user"
} else {
"Exit code: $exitCode"
}| val statusMessage = when { | |
| cancelledByUser -> "Cancelled by user" | |
| exitCode == 0 -> "Exit code: $exitCode" | |
| else -> "Exit code: $exitCode" | |
| val statusMessage = if (cancelledByUser) { | |
| "Cancelled by user" | |
| } else { | |
| "Exit code: $exitCode" |
…commands - Skip AnalysisAgent for cancelled commands in CodingAgentExecutor - Skip error rendering for user-cancelled scenarios - Preserve metadata in ToolExecutionResult.failure() for cancelled flag propagation - Apply same fix to DocumentAgentExecutor When user cancels a command (e.g., bootRun), the flow now: 1. Skips triggering AnalysisAgent for the cancelled output 2. Skips displaying extra 'Tool execution failed' error message 3. Only shows the concise cancellation confirmation This eliminates unnecessary double-messaging (Summary + Error) for intentional cancellations.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/CodingAgentExecutor.kt (1)
74-74: Replace emoji with plain text alternatives in commonMain code targeted at WASM.This file is in
commonMain, which compiles to all platforms including WASM. The coding guidelines explicitly state: "For WASM platform, avoid using emoji and UTF-8 in code."Replace emojis at lines 74 and 345:
- Line 74: Replace
🚀 CodingAgent startedwithRocket CodingAgent startedorCodingAgent started- Line 345: Replace
⏳ Process is still runningwithProcess is still running
🧹 Nitpick comments (3)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/DocumentAgentExecutor.kt (1)
322-327: Cancellation guard is correct; consider centralizing metadata flag parsingThe early return on
executionResult.metadata["cancelled"] == "true"is consistent with the existingisLiveSessioncheck and correctly prevents unnecessary analysis for user‑cancelled commands while still preserving raw output.To reduce duplication and make future changes to metadata representation safer, consider a small helper like:
private fun Map<String, String>.flagTrue(key: String): Boolean = this[key]?.toBooleanStrictOrNull() ?: falseand then use:
val isLiveSession = executionResult.metadata.flagTrue("isLiveSession") val wasCancelledByUser = executionResult.metadata.flagTrue("cancelled")This keeps the
"true"convention in one place and hardens parsing if the producer ever emitstrue/falseinstead of"true".mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/CodingAgentExecutor.kt (2)
282-284: Consider using a constant for the metadata key and value.The string comparison
metadata["cancelled"] == "true"is repeated in the codebase (also at line 474). This pattern is fragile—a typo or case mismatch could silently break the logic.Consider defining constants at the class or companion object level:
+ companion object { + private const val METADATA_KEY_CANCELLED = "cancelled" + private const val METADATA_VALUE_TRUE = "true" + } + // 跳过用户取消的场景 - 用户取消是明确的意图，不需要显示额外的错误消息 - val wasCancelledByUser = executionResult.metadata["cancelled"] == "true" + val wasCancelledByUser = executionResult.metadata[METADATA_KEY_CANCELLED] == METADATA_VALUE_TRUE if (!executionResult.isSuccess && !executionResult.isPending && !wasCancelledByUser) {Alternatively, consider storing boolean values directly in metadata rather than string representations if the metadata map supports mixed types.
472-477: Logic is consistent and correct.The cancellation check correctly bypasses content analysis for user-cancelled operations, which aligns with the intent expressed at line 283. The early return pattern is appropriate.
Optional: The Chinese comments (lines 282, 472-473) may hinder international collaboration. Consider translating to English for consistency with the rest of the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/CodingAgentExecutor.kt(2 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/DocumentAgentExecutor.kt(2 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/orchestrator/ToolExecutionResult.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualfor platform-specific code (e.g., file I/O on JVM/JS/Wasm) in Kotlin Multiplatform projects
Check export first if some functions are not working well with CLI (TypeScript)
In Kotlin/JS @JsExport: AvoidFlow, usePromiseinstead
In Kotlin/JS @JsExport: Use concrete classes as return types and parameter types; avoid interface types
For WASM platform, avoid using emoji and UTF-8 in code
Files:
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/DocumentAgentExecutor.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/CodingAgentExecutor.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/orchestrator/ToolExecutionResult.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test mpp-core (KMP)
- GitHub Check: Build (241)
- GitHub Check: Build (223)
🔇 Additional comments (2)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/DocumentAgentExecutor.kt (1)
303-304: Doc note for cancelled commands matches implementationThe added KDoc note about skipping summarization for user‑cancelled commands accurately reflects the control flow below and clarifies behavior for future readers. No changes needed.
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/orchestrator/ToolExecutionResult.kt (1)
89-108: Error-path metadata propagation is consistent and aligned with goalsPassing
metadataintoToolResult.Error(error, metadata = metadata)while also storing it onToolExecutionResult.metadatacleanly aligns the failure path with the success path and enables downstream checks (e.g., cancellation flags) without changing public APIs. This looks correct and self-explanatory with the added comment.
|
augment review |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| executionTimeMs: Long, | ||
| output: String? | ||
| output: String?, | ||
| cancelledByUser: Boolean |
There was a problem hiding this comment.
ComposeRenderer builds ToolResult.Error without setting an errorType, whereas JewelRenderer and the CLI set one (e.g., CANCELLED_BY_USER or COMMAND_FAILED). Consider including an errorType here to keep downstream handling consistent across renderers.
🤖 Was this useful? React with 👍 or 👎
- Fix thread-safety in ShellSessionManager.markSessionCancelledByUser() with synchronized block
- Fix condition ordering in JewelRenderer: check cancelledByUser before exitCode == 0
- Add execution_time_ms to awaitManagedSession for metadata consistency
- Extract buildCancelledMessage helper to reduce code duplication
- Simplify statusMessage logic in CodingCli (remove redundant when branches)
- Add errorType to ComposeRenderer for cancelled results
- Fix spacing in AnsiStripper ('(' , ')' -> '(', ')')
- Remove duplicate inputStream reader in PtyShellExecutor to fix data race
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt (1)
471-518: Condition ordering corrected; consider extracting shared message builder.The
whenexpression now correctly checkscancelledByUserfirst, addressing the previous review concern. However, the cancelled message building logic (lines 475-486) is duplicated acrossJewelRenderer,ComposeRenderer, andCodingCli. Consider extracting to a shared utility inRendererUtilsor theCodingAgentRendererinterface.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/shell/AnsiStripper.kt (1)
1-117: Add focused unit tests for ANSI/OSC stripping and normalizationThis utility is central to making PTY output AI‑friendly and involves several subtle cases (CSI, OSC with BEL/ST, charset selects, simple ESC, malformed/unterminated sequences, CR handling, and
stripAndNormalize). It deserves dedicated tests covering:
- Typical CSI color/movement sequences.
- OSC sequences terminated by both BEL and
ESC\.- Charset selection escapes and simple
ESC Xsequences.- Malformed/unterminated CSI/OSC sequences.
- Line‑ending behavior for
stripvsstripAndNormalize.This echoes the earlier review feedback and would help guard against regressions in these parsing routines.
🧹 Nitpick comments (5)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/shell/ShellSessionManager.kt (1)
119-123: Consider adding@VolatiletocancelledByUserfor cross-thread visibility.The
synchronizedblock inmarkSessionCancelledByUserprotects map access, but without@Volatile, the boolean write may not be visible to other threads reading this property (e.g., inawaitManagedSessionorawaitProcess).- var cancelledByUser: Boolean = false + @Volatile var cancelledByUser: Boolean = falsempp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.kt (1)
488-492: Inconsistent error type usage: string literal vs enum.Line 489 uses a string literal
"CANCELLED_BY_USER"while line 491 usesToolErrorType.COMMAND_FAILED.code. For consistency and maintainability, consider definingCANCELLED_BY_USERas a constant or adding it toToolErrorTypeenum.- errorType = "CANCELLED_BY_USER" + errorType = cc.unitmesh.agent.tool.ToolErrorType.CANCELLED_BY_USER.codeThis requires adding
CANCELLED_BY_USERto theToolErrorTypeenum if not already present.mpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/CodingCli.kt (1)
413-418: Minor duplication: status symbol logic repeated.The status symbol computation (lines 224-228 and 413-417) is duplicated. Consider extracting to a small helper function.
private fun statusSymbol(cancelledByUser: Boolean, exitCode: Int): String = when { cancelledByUser -> "⚠" exitCode == 0 -> "✓" else -> "✗" }mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/shell/AnsiStripper.kt (2)
21-66: ANSI stripping loop looks good; consider handling trailing lone ESC more defensivelyThe main loop correctly recognizes CSI/OSC/simple ESC sequences and skips them, but a lone
ESCat the very end of the string currently falls through and is emitted as a raw control byte. If you prefer to treat that as a malformed escape and drop it, you can makech == ESCunconditional and special‑case the “no next char” case:- when { - ch == ESC && i + 1 < text.length -> { - val next = text[i + 1] - when (next) { - '[' -> { - // CSI sequence: ESC[...X (ends with a letter) - i = skipCsiSequence(text, i + 2) - } - ']' -> { - // OSC sequence: ESC]...BEL or ESC]...ST - i = skipOscSequence(text, i + 2) - } - '(', ')' -> { - // Character set selection: ESC(X or ESC)X - i = if (i + 2 < text.length) i + 3 else text.length - } - else -> { - // Simple escape sequence: ESC X - i += 2 - } - } - } + when { + ch == ESC -> { + if (i + 1 >= text.length) { + // Trailing lone ESC – drop it + i++ + } else { + val next = text[i + 1] + when (next) { + '[' -> { + // CSI sequence: ESC[...X (ends with a letter) + i = skipCsiSequence(text, i + 2) + } + ']' -> { + // OSC sequence: ESC]...BEL or ESC]...ST + i = skipOscSequence(text, i + 2) + } + '(', ')' -> { + // Character set selection: ESC(X or ESC)X + i = if (i + 2 < text.length) i + 3 else text.length + } + else -> { + // Simple escape sequence: ESC X + i += 2 + } + } + } + }The rest of the stripping logic looks solid for the CSI/OSC/simple ESC cases described in the KDoc.
54-57: Clarify where line-ending normalization happens (stripvsstripAndNormalize)Right now
stripalready drops'\r', so by the timestripAndNormalizeruns, thereplace("\r\n", "\n").replace("\r", "")chain is effectively a no‑op. Either:
- Keep CR stripping in
stripand adjust the KDocs to saystripalso normalizes\r, or- Make
strippurely about ANSI removal and leave all line‑ending normalization tostripAndNormalize, e.g.:- ch == '\r' -> { - // Carriage return - skip it (will be handled with newlines) - i++ - } + ch == '\r' -> { + result.append(ch) + i++ + }so that
stripAndNormalizeis the single place that converts\r\n→\nand removes standalone\r. Either way, having a single, well‑documented layer responsible for EOL handling will make behavior easier to reason about.Also applies to: 112-116
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/shell/AnsiStripper.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/shell/ShellSessionManager.kt(2 hunks)mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/tool/shell/PtyShellExecutor.kt(2 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt(2 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.kt(2 hunks)mpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/CodingCli.kt(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualfor platform-specific code (e.g., file I/O on JVM/JS/Wasm) in Kotlin Multiplatform projects
Check export first if some functions are not working well with CLI (TypeScript)
In Kotlin/JS @JsExport: AvoidFlow, usePromiseinstead
In Kotlin/JS @JsExport: Use concrete classes as return types and parameter types; avoid interface types
For WASM platform, avoid using emoji and UTF-8 in code
Files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/shell/ShellSessionManager.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/CodingCli.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/tool/shell/PtyShellExecutor.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/shell/AnsiStripper.kt
**/compose/**/*.{kt,kts}
📄 CodeRabbit inference engine (AGENTS.md)
In Compose (Desktop/Android), use
AutoDevColorsfromcc.unitmesh.devins.ui.compose.themeorMaterialTheme.colorScheme
Files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.kt
{**/compose/**/*.kt,mpp-ui/src/jsMain/typescript/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (AGENTS.md)
DO NOT hardcode colors (e.g.,
Color(0xFF...)or#hex). Always use design tokens for consistency across platforms
Files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.kt
**/idea/**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/idea/**/*.kt: For SwingPanel z-index issues with Compose popups, enable Jewel's custom popup renderer:JewelFlags.useCustomPopupRenderer = trueinIdeaAgentToolWindowFactory
For popup/dropdown menus in IntelliJ plugins, use Jewel'sPopupMenuinstead ofandroidx.compose.ui.window.Popup
For dialogs in IntelliJ plugins, use IntelliJ'sDialogWrapperwithorg.jetbrains.jewel.bridge.composeinstead ofandroidx.compose.ui.window.Dialog
Files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test mpp-core (KMP)
🔇 Additional comments (3)
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/tool/shell/PtyShellExecutor.kt (1)
266-289: LGTM! Data race concern addressed.The code correctly avoids creating a duplicate reader for the PTY input stream, relying on the UI's
ProcessOutputCollectorto handle output collection. The managed session sync and exit code propagation look correct.mpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/CodingCli.kt (1)
336-348: Good refactor:buildCancelledMessagehelper addresses duplication.The helper function consolidates the cancelled message building logic, addressing the past review concern about code duplication. The metadata now consistently includes
execution_time_msin bothawaitManagedSessionandawaitProcess.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/shell/AnsiStripper.kt (1)
72-83: CSI/OSC skipping helpers are bounded and handle unterminated sequences safely
skipCsiSequenceterminates on@..~with a fallback totext.length, andskipOscSequenceterminates on BEL orESC\with a similar fallback while incrementingion every iteration. That addresses the prior infinite‑loop risk and safely copes with truncated sequences in captured PTY output.Also applies to: 89-105
| fun markSessionCancelledByUser(sessionId: String) { | ||
| synchronized(sessions) { | ||
| sessions[sessionId]?.cancelledByUser = true | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine the specific lines
find . -name "ShellSessionManager.kt" -type fRepository: phodal/auto-dev-sketch
Length of output: 150
🏁 Script executed:
# Search for expect/actual synchronization patterns
rg -n "expect.*Mutex|actual.*Mutex|expect.*synchronized|actual.*synchronized|expect.*Lock|actual.*Lock" --type=ktRepository: phodal/auto-dev-sketch
Length of output: 92
🏁 Script executed:
# Check for synchronized usage in commonMain
rg -n "synchronized\(" mpp-core/src/commonMain --type=kt -B 2 -A 2Repository: phodal/auto-dev-sketch
Length of output: 92
🏁 Script executed:
# Check if there's any Mutex or synchronization utilities in the codebase
rg -n "Mutex|RwMutex|Semaphore" --type=kt | head -20Repository: phodal/auto-dev-sketch
Length of output: 92
🏁 Script executed:
# Read the actual file to confirm the code
cat -n ./mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/shell/ShellSessionManager.kt | sed -n '55,75p'Repository: phodal/auto-dev-sketch
Length of output: 838
🏁 Script executed:
# Search for synchronized usage in commonMain without file type filter
rg -n "synchronized\(" mpp-core/src/commonMain -B 2 -A 2Repository: phodal/auto-dev-sketch
Length of output: 658
🏁 Script executed:
# Search for expect/actual patterns across the entire codebase
rg -n "expect fun.*synchronized|actual fun.*synchronized|expect.*Mutex|actual.*Mutex" -B 1 -A 1Repository: phodal/auto-dev-sketch
Length of output: 48
🏁 Script executed:
# Check for any Mutex usage in commonMain
rg -n "Mutex" mpp-core/src/commonMain -B 1 -A 1Repository: phodal/auto-dev-sketch
Length of output: 1148
🏁 Script executed:
# Get an overview of all synchronized usage in the entire mpp-core
rg -n "synchronized\(" mpp-coreRepository: phodal/auto-dev-sketch
Length of output: 184
Use Mutex instead of synchronized() for thread-safe access in multiplatform code.
This function uses synchronized(sessions) on line 63, which is JVM-specific and will not provide synchronization guarantees on Kotlin/JS or Kotlin/Native platforms. The class already declares private val mutex = Mutex() (line 18) and uses it correctly elsewhere (e.g., line 72 in getActiveSessions()). Replace this function with a suspend variant using mutex.withLock(), or if a non-suspend function is required for UI callbacks, use expect/actual declarations to provide platform-specific implementations per the coding guidelines.
🤖 Prompt for AI Agents
In
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/shell/ShellSessionManager.kt
around lines 62-66, replace the JVM-only synchronized(sessions) block with a
multiplatform-safe approach: make markSessionCancelledByUser a suspend function
and perform the sessions modification inside mutex.withLock {
sessions[sessionId]?.cancelledByUser = true }; if you cannot make it suspend
(e.g., used by UI callbacks), instead introduce an expect declaration for
markSessionCancelledByUser in commonMain and provide platform-specific actual
implementations that use appropriate synchronization on each target (JVM uses
synchronized or Mutex, JS/Native use Mutex.withLock or platform primitives), and
update call sites accordingly.
synchronized is JVM-only. Direct access is safe because: - JS/WASM are single-threaded - On JVM, boolean assignment is atomic and cancelledByUser is only written once
- Fix thread-safety in ShellSessionManager.markSessionCancelledByUser() with synchronized block
- Fix condition ordering in JewelRenderer: check cancelledByUser before exitCode == 0
- Add execution_time_ms to awaitManagedSession for metadata consistency
- Extract buildCancelledMessage helper to reduce code duplication
- Simplify statusMessage logic in CodingCli (remove redundant when branches)
- Add errorType to ComposeRenderer for cancelled results
- Fix spacing in AnsiStripper ('(' , ')' -> '(', ')')
- Remove duplicate inputStream reader in PtyShellExecutor to fix data race
- Fix thread-safety in ShellSessionManager.markSessionCancelledByUser() with synchronized block
- Fix condition ordering in JewelRenderer: check cancelledByUser before exitCode == 0
- Add execution_time_ms to awaitManagedSession for metadata consistency
- Extract buildCancelledMessage helper to reduce code duplication
- Simplify statusMessage logic in CodingCli (remove redundant when branches)
- Add errorType to ComposeRenderer for cancelled results
- Fix spacing in AnsiStripper ('(' , ')' -> '(', ')')
- Remove duplicate inputStream reader in PtyShellExecutor to fix data race
Summary
This PR adds proper handling for terminal command cancellation and cleans up ANSI escape sequences from terminal output before sending to AI.
Changes
User Cancellation Tracking
cancelledByUserflag toManagedSessionfor tracking user-initiated cancellationsmarkSessionCancelledByUser()method toShellSessionManagerfor non-suspend contextShellSessionManagerinToolOrchestratorfor cancel event handlingManagedSessioninPtyShellExecutor.waitForSession()ANSI Stripping
AnsiStripperutility to clean ANSI escape sequences from terminal outputRenderer Updates
CodingAgentRendererinterface withcancelledByUserparameterSkip Analysis & Error for Cancelled Commands (New)
CodingAgentExecutorandDocumentAgentExecutorToolExecutionResult.failure()for cancelled flag propagationBefore
Terminal output sent to AI contained raw ANSI escape sequences:
After
Clean, readable output:
When user cancels a command:
Testing
Tested with
CodingClirunning./gradlew bootRuncommand - output is now clean and readable.Pull Request opened by Augment Code with guidance from the PR author
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.