feat(mpp-core): implement Task Management Tool for CodingAgent #38
Conversation
- Add SelectedFileItem, FileChip, TopToolbar, FileSearchPopup components - Add WorkspaceFileSearchProvider with pre-built file index for fast search - Add IndexingState enum for tracking indexing progress - Integrate file context into DevInEditorInput with buildAndSendMessage() - Add Prompt Enhancement button to BottomToolbar - Add AutoAwesome and History icons to AutoDevComposeIcons - Add FileContext to EditorCallbacks for file context submission Closes #35
…anagement - Add TaskStatus enum (TODO, IN_PROGRESS, COMPLETED, FAILED, BLOCKED) and PlanPhase enum (PDCA cycle) - Add CodeFileLink data class for markdown file link extraction - Add PlanStep, PlanTask, AgentPlan data models with serialization support - Add MarkdownPlanParser using pure Kotlin regex (no IntelliJ dependencies) - Add PlanStateService with StateFlow and listener pattern for reactive updates - Add PlanUpdateListener interface for UI notifications - Add comprehensive unit tests for parser and state service - Fix DocQLReturnAllTest missing Pending branch in when expressions Part of #37
- Add PlanManagementTool with CREATE, UPDATE, COMPLETE_STEP, FAIL_STEP, VIEW actions - Add PlanManagementParams, PlanManagementSchema, PlanManagementInvocation - Add comprehensive unit tests for all tool actions - Tool integrates with PlanStateService for state management Part of #37
- Add PlanManagementTool to BuiltinToolsProvider.provide() - Add executePlanManagementTool method in ToolOrchestrator - Add tests for plan and task-boundary tool registration Part of #37
…nderer - Create PlanPanel composable with task and step display - Add expandable task cards with progress tracking - Implement status icons and colors for plan visualization - Integrate plan state tracking in ComposeRenderer - Handle plan tool calls to update UI state Part of #37
… parameter parsing - Add Planning and Task Management section to CodingAgentTemplate (EN and ZH) - Document when to use planning, plan format, and plan actions - Update Task Completion Strategy to include planning step - Fix taskIndex/stepIndex parameter parsing in ToolOrchestrator to handle both Number and String types Part of #37
…tTool - Remove TaskBoundaryTool.kt as PlanManagementTool provides superset functionality - Remove TaskBoundaryTool from BuiltinToolsProvider and ToolOrchestrator - Update ToolRegistryTest to remove task-boundary test - Update comments referencing task-boundary - Enhance PlanManagementTool with detailed KDoc documentation - Fix tuiEmoji to use proper emoji character Part of #37
Lower the maximum wait time for shell command execution to 2 minutes and pass it as a timeout to the orchestrator context.
WalkthroughThis PR implements a comprehensive task management and planning system for CodingAgent, replacing the simple TaskBoundaryTool with a full-featured plan management infrastructure. Changes include new data models (AgentPlan, PlanTask, PlanStep, TaskStatus), reactive state management (PlanStateService), Markdown parsing/formatting, plan UI visualization, file context management with search integration, and enhanced editor input. A PlanManagementTool replaces TaskBoundaryTool, with supporting tests and updates to agent templates, executor configuration, and orchestration logic. Changes
Sequence DiagramsequenceDiagram
participant User
participant EditorInput as DevInEditor<br/>Input
participant CodingAgent as CodingAgent
participant PlanTool as PlanManagement<br/>Tool
participant PlanService as PlanState<br/>Service
participant Renderer as Compose<br/>Renderer
participant PlanPanel as Plan<br/>Panel UI
User->>EditorInput: Submit text + files
EditorInput->>CodingAgent: onSubmit(text, files)
CodingAgent->>PlanTool: Invoke with CREATE action
PlanTool->>PlanService: createPlanFromMarkdown()
PlanService->>PlanService: Parse markdown → AgentPlan
PlanService-->>PlanTool: AgentPlan created
PlanTool-->>CodingAgent: ToolResult.Success
CodingAgent->>Renderer: renderToolCall(PlanTool)
Renderer->>PlanService: Subscribe to currentPlan
PlanService-->>Renderer: Plan StateFlow updates
Renderer->>PlanPanel: updatePlanFromToolCall()
PlanPanel->>PlanPanel: Parse + render tasks/steps
User->>PlanPanel: Click step item
PlanPanel->>CodingAgent: Invoke PlanTool (COMPLETE_STEP)
CodingAgent->>PlanTool: Action: COMPLETE_STEP
PlanTool->>PlanService: completeStep(taskId, stepId)
PlanService->>PlanService: Update step status
PlanService->>PlanPanel: Notify via StateFlow
PlanPanel->>PlanPanel: Recompose with updated status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| FAILED -> "!" | ||
| IN_PROGRESS -> "*" | ||
| TODO -> " " | ||
| BLOCKED -> "B" |
There was a problem hiding this comment.
BLOCKED maps to marker "B" here, but the markdown parser’s fromMarker doesn’t recognize "B" (it falls back to TODO). Consider aligning markers (parser should accept "B" or use a parsed marker that round-trips) to avoid losing BLOCKED status when formatting/parsing plans.
🤖 Was this useful? React with 👍 or 👎
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/DevInEditorInput.kt (1)
379-384: Avoid emojis in code for WASM compatibility.Per coding guidelines for
**/*.kt, emojis (✨, ℹ️, ❌) should be avoided for WASM platform compatibility. Consider using text-only log messages.Apply this diff:
- println("✨ Enhanced: \"${textFieldValue.text.trim()}\" -> \"$enhanced\"") + println("[Enhancement] Enhanced: \"${textFieldValue.text.trim()}\" -> \"$enhanced\"") } else { - println("ℹ️ No enhancement needed or failed") + println("[Enhancement] No enhancement needed or failed") } } catch (e: Exception) { - println("❌ Enhancement failed: ${e.message}") + println("[Enhancement] Failed: ${e.message}")
🧹 Nitpick comments (16)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/CodingAgentExecutor.kt (1)
215-216: Good addition of timeout parameter for consistency.Passing the timeout to
OrchestratorContextensures consistent timeout enforcement across tool executions. However, the comment "Use max timeout for shell commands" is misleading since this context is used for all tool types (read-file, write-file, grep, shell, etc.), not just shell commands.Consider updating the comment to reflect the broader applicability:
val executionContext = OrchestratorContext( workingDirectory = projectPath, environment = emptyMap(), - timeout = asyncShellConfig.maxWaitTimeoutMs // Use max timeout for shell commands + timeout = asyncShellConfig.maxWaitTimeoutMs // Maximum timeout for all tool executions )mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/editor/EditorCallbacks.kt (1)
7-12: Consider adding@Serializableannotation for multiplatform compatibility.Given that the PR objectives emphasize multiplatform support with kotlinx.serialization, and
FileContextis a data class that may need to be serialized for cross-platform communication, consider adding the@Serializableannotation.+import kotlinx.serialization.Serializable + +@Serializable data class FileContext( val name: String, val path: String, val relativePath: String = name, val isDirectory: Boolean = false )mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/FileChip.kt (1)
92-100: Consider adding ripple indication for better accessibility.The remove button uses
clickable(onClick = onRemove)without an indication, which may reduce visual feedback for users. Consider adding a ripple effect for better UX.Icon( imageVector = AutoDevComposeIcons.Close, contentDescription = "Remove from context", modifier = Modifier .size(14.dp) - .clickable(onClick = onRemove), + .clickable( + interactionSource = remember { MutableInteractionSource() }, + indication = ripple(bounded = false, radius = 8.dp), + onClick = onRemove + ), tint = LocalContentColor.current.copy(alpha = 0.6f) )This applies to both
FileChip(line 96-98) andFileChipExpanded(line 163-165).mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/WorkspaceFileSearchProvider.kt (2)
129-133: Blocking callers when index is not ready.When
searchFilesis called and the index is not ready, it synchronously callsbuildIndex()which could be slow for large workspaces. This may cause unexpected delays for callers.Consider either:
- Returning an empty list with a status indicator when not ready
- Documenting this behavior clearly
- Using a non-blocking approach that returns partial results
136-136: Consider reducing log verbosity for user queries.Logging the raw user query (
query='$query') at INFO level could be verbose in production and may contain sensitive information depending on the context. Consider using DEBUG level or omitting the query content.-AutoDevLogger.info(TAG) { "searchFiles: query='$query', indexSize=${fileIndex.size}" } +AutoDevLogger.debug(TAG) { "searchFiles: query='$query', indexSize=${fileIndex.size}" }mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/plan/MarkdownPlanParserTest.kt (1)
125-143: Consider asserting specific marker formats in the formatToMarkdown test.The test verifies that content is present but doesn't assert the exact marker format (e.g.,
[✓]for completed,[ ]for TODO). This could miss regressions where markers are formatted incorrectly.val markdown = MarkdownPlanParser.formatToMarkdown(tasks) assertTrue(markdown.contains("1. First task")) - assertTrue(markdown.contains("Do something")) - assertTrue(markdown.contains("Do another thing")) + assertTrue(markdown.contains("[✓]") || markdown.contains("[x]"), "Should contain completed marker") + assertTrue(markdown.contains("[ ]"), "Should contain TODO marker") + assertTrue(markdown.contains("Do something")) + assertTrue(markdown.contains("Do another thing")) }mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/TaskStatus.kt (1)
26-32: Minor: Fix spacing in string literal.There's an extra space before the comma in the
whenexpression.fun fromMarker(marker: String): TaskStatus = when (marker.trim().lowercase()) { "x", "✓" -> COMPLETED "!" -> FAILED "*" -> IN_PROGRESS - "" , " " -> TODO + "", " " -> TODO else -> TODO }mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/TopToolbar.kt (1)
96-125: Consider edge case when expanded with empty files.When
isExpandedis true butselectedFilesbecomes empty (e.g., after clearing all files), neither the chips row nor the spacer is rendered, potentially causing layout issues. TheisExpandedstate should reset when files are cleared.+ // Reset expanded state when no files + LaunchedEffect(selectedFiles.isEmpty()) { + if (selectedFiles.isEmpty()) { + isExpanded = false + } + } + Column(modifier = modifier.fillMaxWidth()) {This requires adding the import:
import androidx.compose.runtime.LaunchedEffectmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.kt (1)
247-276: Refactor duplicate logic in COMPLETE_STEP and FAIL_STEP handlers.Both handlers share nearly identical code for index validation and plan updates, differing only in the method called (
complete()vsfail()). Consider extracting a helper function.+ private fun updateStepStatus( + taskIndex: Int, + stepIndex: Int, + updateAction: (PlanStep) -> Unit + ) { + _currentPlan?.let { plan -> + if (taskIndex in 1..plan.tasks.size) { + val task = plan.tasks[taskIndex - 1] + if (stepIndex in 1..task.steps.size) { + val step = task.steps[stepIndex - 1] + updateAction(step) + task.updateStatusFromSteps() + _currentPlan = plan.copy(updatedAt = Clock.System.now().toEpochMilliseconds()) + } + } + } + } + private fun updatePlanFromToolCall(params: Map<String, String>) { val action = params["action"]?.uppercase() ?: return val planMarkdown = params["planMarkdown"] ?: "" when (action) { "CREATE", "UPDATE" -> { if (planMarkdown.isNotBlank()) { _currentPlan = MarkdownPlanParser.parseToPlan(planMarkdown) } } "COMPLETE_STEP" -> { val taskIndex = params["taskIndex"]?.toIntOrNull() ?: return val stepIndex = params["stepIndex"]?.toIntOrNull() ?: return - _currentPlan?.let { plan -> - if (taskIndex in 1..plan.tasks.size) { - val task = plan.tasks[taskIndex - 1] - if (stepIndex in 1..task.steps.size) { - val step = task.steps[stepIndex - 1] - step.complete() - task.updateStatusFromSteps() - _currentPlan = plan.copy(updatedAt = Clock.System.now().toEpochMilliseconds()) - } - } - } + updateStepStatus(taskIndex, stepIndex) { it.complete() } } "FAIL_STEP" -> { val taskIndex = params["taskIndex"]?.toIntOrNull() ?: return val stepIndex = params["stepIndex"]?.toIntOrNull() ?: return - _currentPlan?.let { plan -> - if (taskIndex in 1..plan.tasks.size) { - val task = plan.tasks[taskIndex - 1] - if (stepIndex in 1..task.steps.size) { - val step = task.steps[stepIndex - 1] - step.fail() - task.updateStatusFromSteps() - _currentPlan = plan.copy(updatedAt = Clock.System.now().toEpochMilliseconds()) - } - } - } + updateStepStatus(taskIndex, stepIndex) { it.fail() } } } }mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/tool/impl/PlanManagementToolTest.kt (1)
51-74: Consider adding edge case tests for out-of-bounds indices.The test uses
taskIndex = 1, stepIndex = 1which assumes 1-based indexing. Consider adding tests for:
- Out-of-bounds
taskIndex(e.g.,taskIndex = 99)- Out-of-bounds
stepIndex(e.g.,stepIndex = 99)- Invalid action string (e.g.,
action = "INVALID")These edge cases would help ensure robust error handling in
PlanManagementTool.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/AgentPlan.kt (1)
119-137: Thread-safety concern withidCounter.The
idCounteris a simplevarwith++increment, which is not atomic. In concurrent scenarios (multiple coroutines creating plans), this could lead to duplicate IDs.Consider using
atomicfufor thread-safe increment:+import kotlinx.atomicfu.atomic + companion object { - private var idCounter = 0L + private val idCounter = atomic(0L) fun generateId(): String { - return "plan_${++idCounter}_${Clock.System.now().toEpochMilliseconds()}" + return "plan_${idCounter.incrementAndGet()}_${Clock.System.now().toEpochMilliseconds()}" } }Alternatively, since the timestamp is already included, the counter's role in uniqueness is secondary, but atomicity would still be preferable for correctness.
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanTask.kt (1)
149-153: Same thread-safety concern withidCounteras inAgentPlan.The
idCounterhere also uses non-atomic increment. Consider usingatomicfufor consistency and thread-safety across bothAgentPlanandPlanTask.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/PlanPanel.kt (1)
71-78: ReturningUnitfrom a composable is unconventional.The
planIcon()function returnsUnitexplicitly while its purpose is to render anIcon. This is functional but confusing.Consider returning nothing (implicit Unit) or restructuring as a regular composable:
@Composable -fun TaskStatus.planIcon(): Unit = when (this) { +fun TaskStatus.PlanIcon() { + when (this) { TaskStatus.TODO -> Icon(Icons.Default.RadioButtonUnchecked, null, tint = planColor) // ... + } }mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/impl/PlanManagementTool.kt (1)
103-104: Force-unwrap!!afterupdateStepStatuscould theoretically NPE.While unlikely in practice, if
updateStepStatusor a concurrent operation clears the plan, the!!will throw. This is a minor defensive coding concern.Use safe access with early return:
- val updatedPlan = planStateService.currentPlan.value!! + val updatedPlan = planStateService.currentPlan.value + ?: return ToolResult.Error("Plan was unexpectedly cleared", ToolErrorType.FILE_NOT_FOUND.code) val updatedStep = updatedPlan.tasks[taskIdx].steps[stepIdx]mpp-core/src/jvmMain/kotlin/cc/unitmesh/devins/filesystem/DefaultFileSystem.jvm.kt (2)
108-113: Logging and exclusion behaviour insearchFilesThe project-root guard and
.git/buildexclusion look reasonable and should keep generated artifacts out of results. The amount ofprintln-based tracing (entry, gitignore reload, timing, error stack) in a core filesystem class may be noisy in CLI/IDE contexts and can pollute stdout channels that expect structured output; consider routing this through your logging abstraction or at least guarding it behind a debug flag if you plan to keep the diagnostics.Also applies to: 139-146, 187-192
119-133: Glob-to-regex transformation is improved; consider escaping remaining regex metacharactersThe placeholder-based conversion for
**,*,?, and{a,b}looks solid and should handle common glob patterns better. For future robustness, you may want to also escape other regex metacharacters that could appear literally in patterns (e.g.[ ] ( ) + ^ $ |) by first escaping the whole string and then re-injecting your placeholders, to avoid surprising matches if such characters ever show up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
mpp-core/build.gradle.kts(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/CodingAgentTemplate.kt(2 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/CodingAgentExecutor.kt(2 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/orchestrator/ToolOrchestrator.kt(4 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/AgentPlan.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/CodeFileLink.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/MarkdownPlanParser.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStateService.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStep.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanTask.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanUpdateListener.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/TaskStatus.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/RendererModels.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/impl/PlanManagementTool.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/impl/TaskBoundaryTool.kt(0 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/registry/BuiltinToolsProvider.kt(3 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/editor/EditorCallbacks.kt(2 hunks)mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/plan/MarkdownPlanParserTest.kt(1 hunks)mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/plan/PlanStateServiceTest.kt(1 hunks)mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/tool/ToolRegistryTest.kt(1 hunks)mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/tool/impl/PlanManagementToolTest.kt(1 hunks)mpp-core/src/jvmMain/kotlin/cc/unitmesh/devins/filesystem/DefaultFileSystem.jvm.kt(3 hunks)mpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/tool/impl/DocQLReturnAllTest.kt(2 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.kt(4 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/PlanPanel.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/BottomToolbar.kt(2 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/DevInEditorInput.kt(9 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/FileChip.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/FileSearchPopup.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/SelectedFileItem.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/TopToolbar.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/WorkspaceFileSearchProvider.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/icons/AutoDevComposeIcons.kt(1 hunks)
💤 Files with no reviewable changes (1)
- mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/impl/TaskBoundaryTool.kt
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/icons/AutoDevComposeIcons.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/BottomToolbar.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/devins/editor/EditorCallbacks.ktmpp-core/src/commonTest/kotlin/cc/unitmesh/agent/tool/ToolRegistryTest.ktmpp-core/src/commonTest/kotlin/cc/unitmesh/agent/plan/MarkdownPlanParserTest.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/RendererModels.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/SelectedFileItem.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/FileChip.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/registry/BuiltinToolsProvider.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/TopToolbar.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/MarkdownPlanParser.ktmpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/tool/impl/DocQLReturnAllTest.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/impl/PlanManagementTool.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanTask.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStateService.ktmpp-core/src/commonTest/kotlin/cc/unitmesh/agent/tool/impl/PlanManagementToolTest.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/CodingAgentTemplate.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/devins/filesystem/DefaultFileSystem.jvm.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStep.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/TaskStatus.ktmpp-core/src/commonTest/kotlin/cc/unitmesh/agent/plan/PlanStateServiceTest.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/AgentPlan.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanUpdateListener.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/FileSearchPopup.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/DevInEditorInput.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/WorkspaceFileSearchProvider.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/CodeFileLink.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/orchestrator/ToolOrchestrator.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/CodingAgentExecutor.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/PlanPanel.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/icons/AutoDevComposeIcons.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/BottomToolbar.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/SelectedFileItem.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/FileChip.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/TopToolbar.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/FileSearchPopup.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/DevInEditorInput.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/WorkspaceFileSearchProvider.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/PlanPanel.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/icons/AutoDevComposeIcons.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/BottomToolbar.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/SelectedFileItem.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/FileChip.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/TopToolbar.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/FileSearchPopup.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/DevInEditorInput.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/WorkspaceFileSearchProvider.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/PlanPanel.kt
🧠 Learnings (5)
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Applies to **/compose/**/*.{kt,kts} : In Compose (Desktop/Android), use `AutoDevColors` from `cc.unitmesh.devins.ui.compose.theme` or `MaterialTheme.colorScheme`
Applied to files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/icons/AutoDevComposeIcons.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/FileChip.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/DevInEditorInput.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/PlanPanel.kt
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Applies to **/*.kt : Use `expect`/`actual` for platform-specific code (e.g., file I/O on JVM/JS/Wasm) in Kotlin Multiplatform projects
Applied to files:
mpp-core/build.gradle.kts
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Always consider the multiplatform aspect: JS, WASM, Desktop JVM, Android, iOS in Kotlin multiplatform projects
Applied to files:
mpp-core/build.gradle.kts
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Applies to **/idea/**/*.kt : For SwingPanel z-index issues with Compose popups, enable Jewel's custom popup renderer: `JewelFlags.useCustomPopupRenderer = true` in `IdeaAgentToolWindowFactory`
Applied to files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/FileSearchPopup.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/PlanPanel.kt
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Applies to **/idea/**/*.kt : For popup/dropdown menus in IntelliJ plugins, use Jewel's `PopupMenu` instead of `androidx.compose.ui.window.Popup`
Applied to files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/FileSearchPopup.kt
🧬 Code graph analysis (7)
mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/tool/ToolRegistryTest.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/test/DevInsTestCase.kt (3)
assertNotNull(54-59)assertEquals(45-49)assertTrue(29-33)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/TopToolbar.kt (2)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/FileSearchPopup.kt (1)
FileSearchPopup(49-239)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/FileChip.kt (2)
FileChip(29-104)FileChipExpanded(110-173)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/MarkdownPlanParser.kt (1)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/sketch/IdeaPlanRenderer.kt (1)
PlanStep(134-143)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/impl/PlanManagementTool.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/schema/ToolSchema.kt (1)
string(136-155)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanTask.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStateService.kt (1)
updateStepStatus(112-117)
mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/tool/impl/PlanManagementToolTest.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/test/DevInsTestCase.kt (2)
assertTrue(29-33)assertEquals(45-49)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/DevInEditorInput.kt (1)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/TopToolbar.kt (1)
TopToolbar(24-184)
⏰ 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 (41)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/RendererModels.kt (2)
33-35: KDoc updated to reflect plan management tool terminology.The comment change aligns with the PR's shift from the deprecated TaskBoundaryTool to the new PlanManagementTool. This is appropriate.
36-42: Consider marking TaskInfo and TaskStatus with @serializable for multiplatform serialization.Since this is multiplatform common code (commonMain) and the PR objectives emphasize @serializable models for data interchange across platforms, verify whether TaskInfo and TaskStatus need kotlinx.serialization annotations. If these models are used for serialization (e.g., saving plan state, cross-platform communication), they should be marked accordingly.
Also applies to: 47-59
mpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/tool/impl/DocQLReturnAllTest.kt (2)
171-176: Verify thatPendingresults don't affect test assertions.Similar to the previous test, this when expression handles
Pendingbut doesn't verify the result type. IfqueryAllDocumentsreturnsPendinginstead ofSuccess, the assertion on lines 181-184 might fail incorrectly ifPending.messagedoesn't contain the expected "more available" truncation message.Apply the same fix as suggested for the previous test case:
val content = when (resultDefault) { is ToolResult.Success -> resultDefault.content is ToolResult.Error -> resultDefault.message is ToolResult.AgentResult -> resultDefault.content is ToolResult.Pending -> resultDefault.message } + +// Verify expected result type for test validity +assertTrue( + resultDefault is ToolResult.Success || resultDefault is ToolResult.AgentResult, + "Expected Success or AgentResult but got: ${resultDefault::class.simpleName}" +)
125-130: TheToolResult.Pendingcase in the when expression is unreachable.DocQLDirectQueryExecutor.queryAllDocuments()only returnsSuccessorError—neverPending. The defensive handling adds unnecessary complexity without addressing any actual test concern. Remove the unusedPendingbranch for clarity:val content = when (resultAll) { is ToolResult.Success -> resultAll.content is ToolResult.Error -> resultAll.message is ToolResult.AgentResult -> resultAll.content - is ToolResult.Pending -> resultAll.message }Likely an incorrect or invalid review comment.
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/CodingAgentExecutor.kt (1)
27-28: Clarify the comment on line 216 — it applies to all tool types, not just shell commands.The timeout change is reasonable and aligns with industry tools. The actual timeout behavior differs from what the original comment implies: operations don't terminate after 2 minutes. Instead, the
initialWaitTimeoutMs(1 minute) triggers feedback to the AI that a process is still running (as shown inhandlePendingResult), while the process continues in the background. ThemaxWaitTimeoutMs(2 minutes) is passed toToolExecutionContext, which applies to all tool execution, not just shell commands as the comment suggests.Likely an incorrect or invalid review comment.
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/BottomToolbar.kt (2)
28-29: LGTM! New enhancement parameters are well-integrated.The new
onEnhanceClickandisEnhancingparameters follow the existing pattern of other toolbar actions with sensible defaults.
154-174: Enhancement button implementation looks good.The button correctly disables during enhancement and shows a progress indicator. Uses
MaterialTheme.colorSchemefor theming as per coding guidelines.One minor consideration: the
clickablemodifier on the progress indicator's parentIconButtonis already handling the click, so the disabled state works correctly.mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/editor/EditorCallbacks.kt (1)
26-32: LGTM! Backward-compatible overload with file context.The default implementation correctly delegates to the parameterless
onSubmit, ensuring existing implementations continue to work without modification.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/FileChip.kt (1)
29-104: LGTM! FileChip follows theming guidelines.The component correctly uses
MaterialTheme.colorSchemeandLocalContentColorfor all colors, avoiding hardcoded values as per the coding guidelines. Based on learnings, this is the correct approach for Compose components.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/SelectedFileItem.kt (1)
46-57: LGTM! Cross-platform path handling.The
fromPathfactory correctly handles both forward slashes and backslashes for cross-platform compatibility.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/WorkspaceFileSearchProvider.kt (1)
47-84: LGTM! Robust indexing implementation.The
buildIndexfunction handles edge cases well:
- Checks for null workspace and root path
- Skips re-indexing if already indexed for the same workspace
- Uses
withContext(Dispatchers.Default)appropriately for CPU-bound work- Proper error handling with state transitions
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/icons/AutoDevComposeIcons.kt (1)
62-62: LGTM!The AutoAwesome icon alias follows the established pattern and is appropriately categorized under Communication & AI.
mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/tool/ToolRegistryTest.kt (1)
72-78: LGTM!The test properly validates that the PlanManagementTool is registered with the expected name and description.
mpp-core/build.gradle.kts (1)
138-138: LGTM!Moving kotlinx-datetime from
runtimeOnlytoimplementationcorrectly provides compile-time access to Clock.System and related APIs needed by the new plan management models (AgentPlan, PlanTask, PlanStep).mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/registry/BuiltinToolsProvider.kt (1)
9-9: LGTM!The replacement of TaskBoundaryTool with PlanManagementTool is clean and properly integrated into the built-in tools provider.
Also applies to: 38-38, 57-58
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanUpdateListener.kt (1)
1-47: LGTM!The PlanUpdateListener interface and DefaultPlanUpdateListener base class provide a clean observer pattern implementation for plan lifecycle events. The design allows consumers to selectively override only the events they care about.
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/CodingAgentTemplate.kt (3)
39-70: LGTM!The planning and task management guidance is clear and provides concrete examples of when to use the
/plantool, the expected format, and available actions. This will help the agent understand when and how to leverage planning capabilities.
76-79: LGTM!The updated task completion strategy appropriately emphasizes planning for complex tasks before execution, with step-by-step tracking during implementation.
197-228: LGTM!The Chinese version of the planning guidance and task completion strategy is consistent with the English version and properly localized.
Also applies to: 234-238
mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/plan/PlanStateServiceTest.kt (1)
1-173: LGTM!Comprehensive test coverage for PlanStateService with 10 well-structured tests covering plan creation, state updates, listener notifications, and progress calculation. The test names are descriptive and the assertions are clear.
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/CodeFileLink.kt (1)
1-44: LGTM!The CodeFileLink data class provides a clean abstraction for markdown-style code file links with serialization support. The regex pattern correctly captures markdown link syntax
[text](path)and the extractFromText utility method enables parsing links from step descriptions.mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/plan/MarkdownPlanParserTest.kt (1)
1-165: LGTM!The test suite provides good coverage of the parser functionality including parsing, status markers, file links, task status propagation, markdown formatting, and edge cases. The tests are well-structured with descriptive names.
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/TopToolbar.kt (1)
1-223: LGTM!The component properly uses
MaterialTheme.colorSchemefor all colors, follows Compose best practices with proper state management, and provides good accessibility with content descriptions. The toolbar UI is well-structured with appropriate animations.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.kt (1)
80-82: LGTM!The plan state integration follows the existing patterns in ComposeRenderer with proper Compose state management. The
currentPlanproperty is correctly exposed for UI binding.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/MarkdownPlanParser.kt (1)
1-128: LGTM!The parser implementation is well-structured with clear regex patterns and proper handling of task headers, steps with checkboxes, and unordered items. The code correctly handles edge cases like empty content and properly updates task status from steps. The separation between
parse(),formatToMarkdown(), andparseToPlan()follows good design principles.mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/tool/impl/PlanManagementToolTest.kt (1)
12-14: LGTM on test structure and isolation.Each test gets a fresh
PlanStateServiceinstance viacreateTool(), ensuring proper test isolation. The use ofrunTestfromkotlinx.coroutines.testis appropriate for testing suspend functions.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/DevInEditorInput.kt (4)
83-84: LGTM on the newfileSearchProviderparameter.The optional parameter with default
nullmaintains backward compatibility. The effective provider fallback toWorkspaceFileSearchProvider(line 109) is appropriate.
104-139: File context management is well-structured.The
buildAndSendMessagefunction properly:
- Generates DevIns commands for selected files
- Appends file context to the message
- Clears input and selection state after submission
One consideration: If
it.toDevInsCommand()throws for malformed file items, the entire send operation would fail silently (returns early on blank text). Consider adding defensive handling ifSelectedFileItemcan have invalid state.
491-504: LGTM on TopToolbar desktop integration.The toolbar is correctly:
- Conditionally rendered only on desktop (
!isMobile)- Wired to file selection state with proper add/remove/clear callbacks
- Using the effective search provider
630-639: Consistent message submission across all paths.All submission paths (ENTER key, IME send action, and BottomToolbar send button) now route through
buildAndSendMessage, ensuring file context is always included.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/orchestrator/ToolOrchestrator.kt (1)
385-385: Clean dispatch path for plan management tool.The tool name mapping from
"plan"toexecutePlanManagementToolis straightforward and aligns with the new plan management architecture.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/AgentPlan.kt (2)
20-33: Be aware of mutable state in data class.Using
MutableListandvarin a data class means:
equals/hashCodeare based on initial values, not current statecopy()creates a shallow copy sharing the sameMutableListinstanceThis is acceptable if
AgentPlaninstances are managed as single sources of truth (e.g., inPlanStateService), but callers should avoid relying oncopy()for independent clones.
34-45: LGTM on status derivation logic.The priority order for status aggregation is sensible:
- All completed → COMPLETED
- Any failed → FAILED (stops progress)
- Any in-progress → IN_PROGRESS
- Any blocked → BLOCKED
- Otherwise → TODO
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanTask.kt (2)
110-118: LGTM onupdateStatuswith cascading step completion.The
updateStepsparameter provides flexibility for either updating only the task status or cascading the completion to all steps. The defaultfalseprevents accidental mass updates.
120-130: LGTM on markdown serialization.The
toMarkdownmethod correctly produces the documented format with proper indentation and 1-based task indexing.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/PlanPanel.kt (1)
199-231: LGTM: PlanStepItem implementation is well-structured.The infinite transition animation for in-progress steps and the conditional styling for completed steps (strikethrough) are implemented correctly. The conditional click handling is appropriately applied.
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStep.kt (1)
14-35: LGTM: PlanStep data class design.The
@Serializabledata class with mutablestatusis a reasonable design choice for tracking step state. The computedisCompletedproperty and mutation methods provide a clean API. ThecodeFileLinksintegration for extracting file references is well-implemented.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/FileSearchPopup.kt (2)
82-92: LGTM: LaunchedEffect for loading recent files.The initialization logic correctly resets state when the popup opens and handles focus request failures gracefully. The dependency on
indexingStateensures recent files are only loaded when indexing is complete.
145-237: LGTM: Comprehensive UI state handling.The
whenblock covers all relevant states (no workspace, indexing, error, loading, empty results, recent files, search results) with appropriate UI feedback. Color usage correctly leveragesMaterialTheme.colorScheme.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/impl/PlanManagementTool.kt (2)
70-84: LGTM:updatePlanlogic with minor note.The fallback to
createPlanwhen no plan exists is appropriate. The logic correctly parses markdown and updates the plan state.The
!!on line 81 has the same theoretical risk as noted forupdateStepStatus. Consider applying the same defensive pattern if desired.
157-159: LGTM: Tool structure and dependency injection.The
PlanManagementToolcorrectly encapsulatesPlanStateServicewith a default constructor for convenience, while allowing injection for testing. ThegetPlanStateService()accessor enables external access when needed.
| private suspend fun executePlanManagementTool( | ||
| tool: Tool, | ||
| params: Map<String, Any>, | ||
| context: cc.unitmesh.agent.tool.ToolExecutionContext | ||
| ): ToolResult { | ||
| val taskBoundaryTool = tool as cc.unitmesh.agent.tool.impl.TaskBoundaryTool | ||
|
|
||
| val taskName = params["taskName"] as? String | ||
| ?: return ToolResult.Error("taskName parameter is required") | ||
| val status = params["status"] as? String | ||
| ?: return ToolResult.Error("status parameter is required") | ||
| val summary = params["summary"] as? String ?: "" | ||
|
|
||
| val taskBoundaryParams = cc.unitmesh.agent.tool.impl.TaskBoundaryParams( | ||
| taskName = taskName, | ||
| status = status, | ||
| summary = summary | ||
| val planTool = tool as cc.unitmesh.agent.tool.impl.PlanManagementTool | ||
|
|
||
| val action = params["action"] as? String | ||
| ?: return ToolResult.Error("action parameter is required") | ||
| val planMarkdown = params["planMarkdown"] as? String ?: "" | ||
|
|
||
| // Handle taskIndex and stepIndex - can be Number or String | ||
| val taskIndex = when (val v = params["taskIndex"]) { | ||
| is Number -> v.toInt() | ||
| is String -> v.toIntOrNull() ?: 0 | ||
| else -> 0 | ||
| } | ||
| val stepIndex = when (val v = params["stepIndex"]) { | ||
| is Number -> v.toInt() | ||
| is String -> v.toIntOrNull() ?: 0 | ||
| else -> 0 | ||
| } | ||
|
|
||
| val planParams = cc.unitmesh.agent.tool.impl.PlanManagementParams( | ||
| action = action, | ||
| planMarkdown = planMarkdown, | ||
| taskIndex = taskIndex, | ||
| stepIndex = stepIndex | ||
| ) | ||
| val invocation = taskBoundaryTool.createInvocation(taskBoundaryParams) | ||
|
|
||
| val invocation = planTool.createInvocation(planParams) | ||
| return invocation.execute(context) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "PlanManagementTool.kt" --exec cat -n {}Repository: phodal/auto-dev-sketch
Length of output: 10270
Address invalid default indices for step operations.
The taskIndex and stepIndex default to 0, but PlanManagementTool enforces 1-based indexing and validates that both must be positive (line 87 in PlanManagementTool.kt). This causes COMPLETE_STEP and FAIL_STEP actions to fail with error "taskIndex and stepIndex must be positive" whenever indices are not explicitly provided. Consider:
- Making these parameters required for operations that need them (COMPLETE_STEP, FAIL_STEP)
- Or use
nullas default and validate at invocation creation time - Or document that defaults only apply to CREATE/UPDATE/VIEW operations
🤖 Prompt for AI Agents
In
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/orchestrator/ToolOrchestrator.kt
around lines 678 to 710, the current code defaults taskIndex and stepIndex to 0
which breaks PlanManagementTool because it expects 1-based positive indices;
change handling so indices are nullable by default and validate/require them for
actions that need them: parse params["taskIndex"] and params["stepIndex"] into
Int? (null if missing or unparsable), then when building PlanManagementParams
set taskIndex/stepIndex to the nullable values (or call a constructor that
accepts nullable), and before creating the invocation check the action: for
COMPLETE_STEP and FAIL_STEP (and any other actions that require indices) return
ToolResult.Error if the corresponding index is null or <=0; for
CREATE/UPDATE/VIEW keep behavior that treats null as absent. Ensure validation
error messages match PlanManagementTool expectations.
|
|
||
| val step = PlanStep( | ||
| id = "step_${++stepIdCounter}", | ||
| description = description, | ||
| status = TaskStatus.fromMarker(marker), | ||
| codeFileLinks = codeFileLinks | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine PlanTask.generateId() implementation
rg "fun generateId\(\)" --context 5 -A 10Repository: phodal/auto-dev-sketch
Length of output: 2618
🏁 Script executed:
#!/bin/bash
# Find PlanStep class definition
rg "class PlanStep" --context 3 -A 15Repository: phodal/auto-dev-sketch
Length of output: 3367
🏁 Script executed:
#!/bin/bash
# Check if there are other references to stepIdCounter or step ID usage
rg "stepIdCounter|step_" --context 2Repository: phodal/auto-dev-sketch
Length of output: 3039
🏁 Script executed:
#!/bin/bash
# Find where parse() method is called to understand usage patterns
rg "\.parse\(" --context 2 | head -50Repository: phodal/auto-dev-sketch
Length of output: 3616
🏁 Script executed:
#!/bin/bash
# Check line 59 to see how PlanTask.generateId() is used
sed -n '55,65p' mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/MarkdownPlanParser.ktRepository: phodal/auto-dev-sketch
Length of output: 454
🏁 Script executed:
#!/bin/bash
# Look at full PlanStep.kt to see the generateId method and idCounter
cat -n mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStep.kt | head -80Repository: phodal/auto-dev-sketch
Length of output: 2363
Use PlanStep.generateId() to ensure step IDs are globally unique across parse calls.
Currently, step IDs are generated using a local stepIdCounter (line 72-75, 88), producing IDs like step_1, step_2 within each parse call. These IDs lack timestamps and will collide if the same markdown is parsed multiple times or steps are merged from different parses. Unlike PlanTask.generateId() which is used on line 59, PlanStep already has a companion object method generateId() that returns "step_${++idCounter}_${currentTimeMillis()}", ensuring true uniqueness. Use PlanStep.generateId() instead of manual counter construction.
🤖 Prompt for AI Agents
In mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/MarkdownPlanParser.kt
around lines 72-78 (and also update the similar construction at line 88), the
parser currently builds step IDs with a local stepIdCounter
("step_${++stepIdCounter}") which can collide across parse calls; change these
to call PlanStep.generateId() for each new PlanStep instead (i.e., set id =
PlanStep.generateId()), and remove any reliance on the local counter for step ID
generation so step IDs are globally unique across parses.
| */ | ||
| val currentPlan: StateFlow<AgentPlan?> = _currentPlan.asStateFlow() | ||
|
|
||
| private val listeners = mutableListOf<PlanUpdateListener>() |
There was a problem hiding this comment.
Thread safety: listeners list is not synchronized.
The mutableListOf is not thread-safe. If addListener/removeListener are called from different coroutine contexts or threads while notifications are being dispatched, this can cause ConcurrentModificationException.
Consider using a thread-safe collection:
- private val listeners = mutableListOf<PlanUpdateListener>()
+ private val listeners = mutableListOf<PlanUpdateListener>().toMutableList()
+ // Or use: private val listeners = java.util.concurrent.CopyOnWriteArrayList<PlanUpdateListener>()Alternatively, copy the list before iterating in notification methods:
private fun notifyPlanCreated(plan: AgentPlan) {
listeners.toList().forEach { it.onPlanCreated(plan) }
}🤖 Prompt for AI Agents
In mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStateService.kt
around line 26, the mutableListOf assigned to `listeners` is not thread-safe and
can throw ConcurrentModificationException when listeners are added/removed
concurrently; fix by either replacing it with a thread-safe collection (e.g.,
CopyOnWriteArrayList or a synchronized collection) or ensure safe iteration by
copying the list before notifying (e.g., use `listeners.toList()` inside
notification methods) and guard add/remove operations with a mutex or
synchronized block to ensure atomicity.
| fun updatePlan(tasks: List<PlanTask>) { | ||
| val plan = _currentPlan.value | ||
| if (plan != null) { | ||
| plan.tasks.clear() | ||
| plan.tasks.addAll(tasks) | ||
| notifyPlanUpdated(plan) | ||
| } else { | ||
| createPlan(tasks) | ||
| } | ||
| } |
There was a problem hiding this comment.
StateFlow won't emit updates when mutating plan.tasks in-place.
Mutating plan.tasks.clear() and plan.tasks.addAll() does not trigger a new emission from _currentPlan StateFlow because the reference hasn't changed. Subscribers watching currentPlan won't receive the update.
Reassign the StateFlow value to ensure emission:
fun updatePlan(tasks: List<PlanTask>) {
val plan = _currentPlan.value
if (plan != null) {
plan.tasks.clear()
plan.tasks.addAll(tasks)
+ _currentPlan.value = plan // Force emission
notifyPlanUpdated(plan)
} else {
createPlan(tasks)
}
}Alternatively, create a new AgentPlan instance with updated tasks for true immutability.
🤖 Prompt for AI Agents
In mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStateService.kt
around lines 62 to 71, you're mutating plan.tasks in-place which doesn't cause
StateFlow to emit because the object reference stays the same; instead create
and assign a new AgentPlan (or use plan.copy with the new tasks list) to
_currentPlan.value so subscribers see the update, e.g. build an immutable copy
with tasks = tasks and set _currentPlan.value to that (you can remove or keep
notifyPlanUpdated if it duplicates emission).
| fun addTask(task: PlanTask) { | ||
| val plan = _currentPlan.value ?: createPlan(emptyList()) | ||
| plan.addTask(task) | ||
| notifyPlanUpdated(plan) | ||
| } |
There was a problem hiding this comment.
Same StateFlow emission issue in addTask.
After plan.addTask(task), the StateFlow value reference is unchanged, so no emission occurs.
Apply the same fix:
fun addTask(task: PlanTask) {
val plan = _currentPlan.value ?: createPlan(emptyList())
plan.addTask(task)
+ _currentPlan.value = plan // Force emission
notifyPlanUpdated(plan)
}🤖 Prompt for AI Agents
In mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStateService.kt
around lines 84-88, addTask mutates the existing Plan but leaves the StateFlow
holding the same object reference so no emission happens; after calling
plan.addTask(task) create a new Plan instance (e.g., via plan.copy(...) or by
copying its tasks into a fresh Plan) and assign that new instance to
_currentPlan.value (or pass the new copy into notifyPlanUpdated) so the
StateFlow sees a different reference and emits the update.
| """.trimIndent() | ||
|
|
||
| override val metadata: ToolMetadata = ToolMetadata( | ||
| displayName = "Plan Management", tuiEmoji = "📋", composeIcon = "plan", |
There was a problem hiding this comment.
Emoji 📋 may cause issues on WASM platform.
The tuiEmoji field uses an emoji character, which the coding guidelines advise against for WASM: "For WASM platform, avoid using emoji and UTF-8 in code."
Consider using an ASCII fallback or platform-specific handling:
- tuiEmoji = "📋", composeIcon = "plan",
+ tuiEmoji = "[P]", composeIcon = "plan", // ASCII fallback for WASM compatibilityOr use expect/actual to provide platform-specific emoji handling. Based on coding guidelines.
📝 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.
| displayName = "Plan Management", tuiEmoji = "📋", composeIcon = "plan", | |
| displayName = "Plan Management", tuiEmoji = "[P]", composeIcon = "plan", // ASCII fallback for WASM compatibility |
🤖 Prompt for AI Agents
In
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/impl/PlanManagementTool.kt
around line 177, the tuiEmoji field currently uses the literal emoji "📋" which
may break on WASM; replace it with a platform-safe approach by introducing a
small helper that returns an ASCII fallback for WASM (e.g., "[P]" or "PLAN") and
the emoji for other platforms, or declare an expect/actual string (e.g., expect
val planTuiEmoji: String in common and actual implementations per platform) and
use that here so WASM builds avoid UTF-8 emoji while other platforms keep the
emoji.
| val TaskStatus.planColor: Color | ||
| get() = when (this) { | ||
| TaskStatus.TODO -> Color(0xFF9E9E9E) | ||
| TaskStatus.IN_PROGRESS -> Color(0xFF2196F3) | ||
| TaskStatus.COMPLETED -> Color(0xFF4CAF50) | ||
| TaskStatus.FAILED -> Color(0xFFF44336) | ||
| TaskStatus.BLOCKED -> Color(0xFFFF9800) | ||
| } |
There was a problem hiding this comment.
Hardcoded colors violate coding guidelines.
The planColor extension uses hardcoded hex colors (Color(0xFF...)), which violates the guideline: "DO NOT hardcode colors...Always use design tokens for consistency across platforms."
As per coding guidelines and learnings, use AutoDevColors or MaterialTheme.colorScheme:
+import cc.unitmesh.devins.ui.compose.theme.AutoDevColors
+
val TaskStatus.planColor: Color
+ @Composable
get() = when (this) {
- TaskStatus.TODO -> Color(0xFF9E9E9E)
- TaskStatus.IN_PROGRESS -> Color(0xFF2196F3)
- TaskStatus.COMPLETED -> Color(0xFF4CAF50)
- TaskStatus.FAILED -> Color(0xFFF44336)
- TaskStatus.BLOCKED -> Color(0xFFFF9800)
+ TaskStatus.TODO -> MaterialTheme.colorScheme.outline
+ TaskStatus.IN_PROGRESS -> MaterialTheme.colorScheme.primary
+ TaskStatus.COMPLETED -> AutoDevColors.success // or define in theme
+ TaskStatus.FAILED -> MaterialTheme.colorScheme.error
+ TaskStatus.BLOCKED -> AutoDevColors.warning // or define in theme
}Note: This will require making the property @Composable or defining semantic colors in AutoDevColors.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/PlanPanel.kt
around lines 62 to 69, the TaskStatus.planColor extension currently returns
hardcoded Color(...) values; replace these with design-token colors (either
AutoDevColors semantic tokens or MaterialTheme.colorScheme tokens) and make the
property @Composable if you choose MaterialTheme. Update the when branches to
return the appropriate semantic tokens (e.g.,
AutoDevColors.surface/primary/positive/negative/warning or
colorScheme.primary/tertiary/secondaryContainer/error/warning equivalents), add
the @Composable annotation to the property, and adjust imports accordingly so no
hardcoded hex literals remain. Ensure the mapping follows the intended semantics
(TODO→neutral, IN_PROGRESS→primary, COMPLETED→positive, FAILED→negative,
BLOCKED→warning).
| // Debounced search function | ||
| fun performSearch(query: String) { | ||
| if (query.length < 2 || currentWorkspace == null || indexingState != IndexingState.READY) { | ||
| searchResults = emptyList() | ||
| isLoading = false | ||
| return | ||
| } | ||
|
|
||
| isLoading = true | ||
| scope.launch { | ||
| delay(150) // Debounce | ||
| try { | ||
| searchResults = searchProvider.searchFiles(query) | ||
| } catch (e: Exception) { | ||
| searchResults = emptyList() | ||
| } finally { | ||
| isLoading = false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Debounce implementation has race condition.
Each call to performSearch launches a new coroutine without canceling previous ones. If the user types "abc" quickly, three coroutines are launched, and all three may complete and update searchResults in rapid succession after 150ms.
Use a Job to cancel previous search requests:
+ var searchJob by remember { mutableStateOf<kotlinx.coroutines.Job?>(null) }
+
fun performSearch(query: String) {
if (query.length < 2 || currentWorkspace == null || indexingState != IndexingState.READY) {
searchResults = emptyList()
isLoading = false
return
}
isLoading = true
+ searchJob?.cancel()
- scope.launch {
+ searchJob = scope.launch {
delay(150) // Debounce
try {
searchResults = searchProvider.searchFiles(query)
} catch (e: Exception) {
searchResults = emptyList()
} finally {
isLoading = false
}
}
}📝 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.
| // Debounced search function | |
| fun performSearch(query: String) { | |
| if (query.length < 2 || currentWorkspace == null || indexingState != IndexingState.READY) { | |
| searchResults = emptyList() | |
| isLoading = false | |
| return | |
| } | |
| isLoading = true | |
| scope.launch { | |
| delay(150) // Debounce | |
| try { | |
| searchResults = searchProvider.searchFiles(query) | |
| } catch (e: Exception) { | |
| searchResults = emptyList() | |
| } finally { | |
| isLoading = false | |
| } | |
| } | |
| } | |
| var searchJob by remember { mutableStateOf<kotlinx.coroutines.Job?>(null) } | |
| // Debounced search function | |
| fun performSearch(query: String) { | |
| if (query.length < 2 || currentWorkspace == null || indexingState != IndexingState.READY) { | |
| searchResults = emptyList() | |
| isLoading = false | |
| return | |
| } | |
| isLoading = true | |
| searchJob?.cancel() | |
| searchJob = scope.launch { | |
| delay(150) // Debounce | |
| try { | |
| searchResults = searchProvider.searchFiles(query) | |
| } catch (e: Exception) { | |
| searchResults = emptyList() | |
| } finally { | |
| isLoading = false | |
| } | |
| } | |
| } |
| val truncatedPath: String | ||
| get() { | ||
| val parentPath = relativePath.substringBeforeLast("/", "") | ||
| if (parentPath.isEmpty()) return "" | ||
| if (parentPath.length <= 40) return parentPath | ||
|
|
||
| val parts = parentPath.split("/") | ||
| if (parts.size <= 2) return "...$parentPath" | ||
|
|
||
| val keepParts = parts.takeLast(4) | ||
| return "...${keepParts.joinToString("/")}" | ||
| } |
There was a problem hiding this comment.
Inconsistency between truncatedPath property and truncatePath function.
The truncatedPath property keeps the last 4 parts (line 37: parts.takeLast(4)), while the top-level truncatePath function keeps the last 3 parts (line 73: parts.takeLast(3)). They also use different max lengths (40 vs 30). This may cause inconsistent display across the UI.
Consider unifying the logic or having one delegate to the other.
val truncatedPath: String
get() {
- val parentPath = relativePath.substringBeforeLast("/", "")
- if (parentPath.isEmpty()) return ""
- if (parentPath.length <= 40) return parentPath
-
- val parts = parentPath.split("/")
- if (parts.size <= 2) return "...$parentPath"
-
- val keepParts = parts.takeLast(4)
- return "...${keepParts.joinToString("/")}"
+ return truncatePath(relativePath, maxLength = 40)
}And adjust truncatePath to accept configurable keepParts if needed.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/SelectedFileItem.kt
around lines 28-39: the truncatedPath getter uses different truncation rules
(keeps last 4 parts and max length 40) than the top-level truncatePath function
(keeps last 3 parts and max length 30), causing inconsistent UI; refactor so
both use the same logic by making truncatePath accept configurable parameters
(maxLength and keepParts) and have truncatedPath call truncatePath(relativePath,
maxLength = 40, keepParts = 4) or vice versa (choose one canonical max/keep
values), then remove duplicated logic and ensure both call the single
implementation so the display is consistent.
| class InMemoryRecentFilesProvider(private val maxSize: Int = 20) : RecentFilesProvider { | ||
| private val recentFiles = mutableListOf<SelectedFileItem>() | ||
|
|
||
| override suspend fun getRecentFiles(): List<SelectedFileItem> = recentFiles.toList() | ||
|
|
||
| override fun addRecentFile(file: SelectedFileItem) { | ||
| recentFiles.removeAll { it.path == file.path } | ||
| recentFiles.add(0, file) | ||
| if (recentFiles.size > maxSize) recentFiles.removeAt(recentFiles.lastIndex) | ||
| } |
There was a problem hiding this comment.
Thread-safety issue in InMemoryRecentFilesProvider.
The recentFiles mutableList is accessed/modified without synchronization. If addRecentFile is called from a different coroutine context than getRecentFiles, this could lead to ConcurrentModificationException or data corruption.
class InMemoryRecentFilesProvider(private val maxSize: Int = 20) : RecentFilesProvider {
- private val recentFiles = mutableListOf<SelectedFileItem>()
+ private val recentFiles = mutableListOf<SelectedFileItem>()
+ private val lock = Any()
- override suspend fun getRecentFiles(): List<SelectedFileItem> = recentFiles.toList()
+ override suspend fun getRecentFiles(): List<SelectedFileItem> = synchronized(lock) {
+ recentFiles.toList()
+ }
- override fun addRecentFile(file: SelectedFileItem) {
+ override fun addRecentFile(file: SelectedFileItem) = synchronized(lock) {
recentFiles.removeAll { it.path == file.path }
recentFiles.add(0, file)
if (recentFiles.size > maxSize) recentFiles.removeAt(recentFiles.lastIndex)
}
}Alternatively, use a ConcurrentLinkedDeque or Kotlin's Mutex for coroutine-safe synchronization.
📝 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.
| class InMemoryRecentFilesProvider(private val maxSize: Int = 20) : RecentFilesProvider { | |
| private val recentFiles = mutableListOf<SelectedFileItem>() | |
| override suspend fun getRecentFiles(): List<SelectedFileItem> = recentFiles.toList() | |
| override fun addRecentFile(file: SelectedFileItem) { | |
| recentFiles.removeAll { it.path == file.path } | |
| recentFiles.add(0, file) | |
| if (recentFiles.size > maxSize) recentFiles.removeAt(recentFiles.lastIndex) | |
| } | |
| class InMemoryRecentFilesProvider(private val maxSize: Int = 20) : RecentFilesProvider { | |
| private val recentFiles = mutableListOf<SelectedFileItem>() | |
| private val lock = Any() | |
| override suspend fun getRecentFiles(): List<SelectedFileItem> = synchronized(lock) { | |
| recentFiles.toList() | |
| } | |
| override fun addRecentFile(file: SelectedFileItem) = synchronized(lock) { | |
| recentFiles.removeAll { it.path == file.path } | |
| recentFiles.add(0, file) | |
| if (recentFiles.size > maxSize) recentFiles.removeAt(recentFiles.lastIndex) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/editor/context/WorkspaceFileSearchProvider.kt
around lines 191 to 200, the InMemoryRecentFilesProvider uses a plain
mutableList (recentFiles) accessed from suspend and non-suspend functions
without synchronization, which can cause ConcurrentModificationException or
corruption; make accesses coroutine/thread-safe by protecting reads and writes
with a Kotlin Mutex (or use a concurrent collection appropriate for Kotlin
Multiplatform), e.g., declare a Mutex and use mutex.withLock { ... } around
modifications in addRecentFile and around the snapshot copy in getRecentFiles,
ensuring you keep the same semantics (remove duplicates, add to front, trim to
maxSize) while holding the lock.
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive Task Management system for the CodingAgent, replacing the simpler TaskBoundaryTool with a full-featured PlanManagementTool. The implementation follows a clean architecture with data models, parser, state management, tool interface, and UI components.
Key Changes:
- Plan Management System: Complete implementation with markdown parsing, state tracking via StateFlow, and reactive updates
- UI Components: New
PlanPanelfor visualizing plans, plus file context management toolbar (TopToolbar,FileSearchPopup) for better user experience - Tool Integration: Replaced
TaskBoundaryToolwithPlanManagementToolsupporting CREATE, UPDATE, COMPLETE_STEP, FAIL_STEP, and VIEW actions
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
TaskStatus.kt, PlanStep.kt, PlanTask.kt, AgentPlan.kt |
Core data models with status tracking and markdown conversion |
MarkdownPlanParser.kt |
Multiplatform parser for plan markdown with support for task/step status markers |
PlanStateService.kt |
State management using StateFlow with listener pattern for reactive updates |
PlanManagementTool.kt |
AI-callable tool with 5 actions for plan CRUD operations |
PlanPanel.kt |
Compose UI with expandable tasks, status icons, and progress tracking |
WorkspaceFileSearchProvider.kt |
File indexing and search for context management |
TopToolbar.kt, FileSearchPopup.kt, FileChip.kt |
File context UI components |
DevInEditorInput.kt |
Integration of file context with message submission |
ComposeRenderer.kt |
Plan state tracking from tool calls |
ToolOrchestrator.kt |
Execution logic for plan management tool |
CodingAgentTemplate.kt |
Updated system prompt with planning guidance |
BuiltinToolsProvider.kt |
Registered new tool, removed old TaskBoundaryTool |
| Test files | Comprehensive unit tests (30 passing) for parser, state service, and tool |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fun buildAndSendMessage(text: String) { | ||
| if (text.isBlank()) return | ||
|
|
||
| // Generate DevIns commands for selected files | ||
| val filesText = selectedFiles.joinToString("\n") { it.toDevInsCommand() } | ||
| val fullText = if (filesText.isNotEmpty()) "$text\n$filesText" else text | ||
|
|
||
| // Send with file contexts | ||
| callbacks?.onSubmit(fullText, getFileContexts()) | ||
|
|
||
| // Clear input and files | ||
| textFieldValue = TextFieldValue("") | ||
| selectedFiles = emptyList() | ||
| showCompletion = false | ||
| } |
There was a problem hiding this comment.
The buildAndSendMessage function always appends file context commands to the user's message, even when the text already contains file references. This could lead to duplicate file references if the user manually included /file: or /dir: commands in their message.
Consider checking if file commands already exist in the text before appending:
fun buildAndSendMessage(text: String) {
if (text.isBlank()) return
val filesText = selectedFiles.joinToString("\n") { it.toDevInsCommand() }
// Only append if text doesn't already contain these file references
val fullText = if (filesText.isNotEmpty() && !text.contains(filesText)) {
"$text\n$filesText"
} else {
text
}
// ...
}| if (currentDepth >= maxDepth || files.size >= 5000) return | ||
|
|
||
| val fileSystem = workspace.fileSystem | ||
| val pathToList = if (currentPath.isEmpty()) "." else currentPath | ||
|
|
||
| try { | ||
| val entries = fileSystem.listFiles(pathToList, null) | ||
| if (currentDepth == 0) { | ||
| AutoDevLogger.info(TAG) { "indexFilesRecursively: root entries=${entries.size}" } | ||
| } | ||
| for (entry in entries) { | ||
| val name = entry.substringAfterLast('/') | ||
|
|
||
| // Skip hidden files and common ignored directories | ||
| if (name.startsWith(".") || name in IGNORED_DIRS) continue | ||
|
|
||
| val isDir = fileSystem.isDirectory(fileSystem.resolvePath(entry)) | ||
| files.add(IndexedFile(name, entry, isDir)) | ||
|
|
||
| if (isDir && files.size < 5000) { | ||
| indexFilesRecursively(workspace, entry, files, maxDepth, currentDepth + 1) |
There was a problem hiding this comment.
The indexFilesRecursively function has a hard-coded limit of 5000 files (lines 93, 112) that could cause issues in large projects. When this limit is reached, the indexing silently stops, which might confuse users who can't find files they expect to see.
Consider:
- Adding a warning log when the limit is reached
- Making the limit configurable
- Documenting this limitation in the class documentation
Example:
if (files.size >= maxResults) {
AutoDevLogger.warn(TAG) { "File index limit of $maxResults reached. Some files may not be searchable." }
return
}| fun onSubmit(text: String) {} | ||
|
|
||
| /** | ||
| * 当用户提交内容时调用，包含文件上下文 | ||
| * 默认实现调用不带文件上下文的 onSubmit | ||
| */ | ||
| fun onSubmit(text: String, files: List<FileContext>) { | ||
| onSubmit(text) | ||
| } |
There was a problem hiding this comment.
[nitpick] The new onSubmit(text: String, files: List<FileContext>) method has a default implementation that calls the original onSubmit(text: String), ignoring the file context. This could lead to subtle bugs where implementers might forget to override the new method, causing file context to be silently lost.
Consider adding a @Deprecated annotation to the old method or making the design more explicit:
/**
* @deprecated Override onSubmit(text: String, files: List<FileContext>) instead
*/
fun onSubmit(text: String) {
onSubmit(text, emptyList())
}
fun onSubmit(text: String, files: List<FileContext>) {}This would encourage implementers to use the new API while maintaining backward compatibility.
| fun onSubmit(text: String) {} | |
| /** | |
| * 当用户提交内容时调用，包含文件上下文 | |
| * 默认实现调用不带文件上下文的 onSubmit | |
| */ | |
| fun onSubmit(text: String, files: List<FileContext>) { | |
| onSubmit(text) | |
| } | |
| /** | |
| * @deprecated Override onSubmit(text: String, files: List<FileContext>) instead | |
| */ | |
| @Deprecated( | |
| message = "Override onSubmit(text: String, files: List<FileContext>) instead", | |
| replaceWith = ReplaceWith("onSubmit(text, emptyList())") | |
| ) | |
| fun onSubmit(text: String) { | |
| onSubmit(text, emptyList()) | |
| } | |
| /** | |
| * 当用户提交内容时调用，包含文件上下文 | |
| * 默认实现为空实现 | |
| */ | |
| fun onSubmit(text: String, files: List<FileContext>) {} |
| is Number -> v.toInt() | ||
| is String -> v.toIntOrNull() ?: 0 | ||
| else -> 0 | ||
| } | ||
| val stepIndex = when (val v = params["stepIndex"]) { | ||
| is Number -> v.toInt() | ||
| is String -> v.toIntOrNull() ?: 0 | ||
| else -> 0 |
There was a problem hiding this comment.
The executePlanManagementTool function uses inconsistent parameter conversion that could lead to errors. When taskIndex or stepIndex are missing from the params map, they default to 0, which will cause validation errors later (line 87 checks for <= 0).
Consider explicitly handling missing parameters:
val taskIndex = when (val v = params["taskIndex"]) {
null -> return ToolResult.Error("taskIndex parameter is required for COMPLETE_STEP and FAIL_STEP")
is Number -> v.toInt()
is String -> v.toIntOrNull() ?: return ToolResult.Error("taskIndex must be a valid integer")
else -> return ToolResult.Error("taskIndex has invalid type")
}This would provide clearer error messages to the agent.
| is Number -> v.toInt() | |
| is String -> v.toIntOrNull() ?: 0 | |
| else -> 0 | |
| } | |
| val stepIndex = when (val v = params["stepIndex"]) { | |
| is Number -> v.toInt() | |
| is String -> v.toIntOrNull() ?: 0 | |
| else -> 0 | |
| null -> return ToolResult.Error("taskIndex parameter is required for COMPLETE_STEP and FAIL_STEP") | |
| is Number -> v.toInt() | |
| is String -> v.toIntOrNull() ?: return ToolResult.Error("taskIndex must be a valid integer") | |
| else -> return ToolResult.Error("taskIndex has invalid type") | |
| } | |
| val stepIndex = when (val v = params["stepIndex"]) { | |
| null -> return ToolResult.Error("stepIndex parameter is required for COMPLETE_STEP and FAIL_STEP") | |
| is Number -> v.toInt() | |
| is String -> v.toIntOrNull() ?: return ToolResult.Error("stepIndex must be a valid integer") | |
| else -> return ToolResult.Error("stepIndex has invalid type") |
| fun fromMarker(marker: String): TaskStatus = when (marker.trim().lowercase()) { | ||
| "x", "✓" -> COMPLETED | ||
| "!" -> FAILED | ||
| "*" -> IN_PROGRESS | ||
| "" , " " -> TODO | ||
| else -> TODO | ||
| } | ||
|
|
||
| /** | ||
| * Get the markdown marker for this status | ||
| */ | ||
| fun TaskStatus.toMarker(): String = when (this) { | ||
| COMPLETED -> "✓" | ||
| FAILED -> "!" | ||
| IN_PROGRESS -> "*" | ||
| TODO -> " " | ||
| BLOCKED -> "B" | ||
| } |
There was a problem hiding this comment.
The fromMarker function in TaskStatus.kt has inconsistent handling of the checkmark character. Line 27 checks for both "x" and "✓", but the comment on line 10 says [x] for COMPLETED while toMarker() returns "✓" (line 38). This could cause round-trip inconsistencies when parsing and formatting plans.
Consider:
- Documenting that "x" and "✓" are both accepted for COMPLETED status
- Standardizing on one format for output (currently "✓")
- Ensuring the examples in comments match the actual behavior
This is consistent with GitHub-flavored markdown which uses [x] for completed checkboxes.
| // Build index if not ready | ||
| if (_indexingState.value != IndexingState.READY) { | ||
| AutoDevLogger.info(TAG) { "searchFiles: Index not ready, building..." } | ||
| buildIndex() | ||
| } |
There was a problem hiding this comment.
In the searchFiles function, when the index is not ready, buildIndex() is called synchronously (line 132). Since this is a suspend function called from within another suspend function, this could block the UI thread if the indexing takes a long time.
Consider:
- Building the index asynchronously in the background when the workspace opens
- Showing a loading state immediately instead of blocking
- Adding a timeout for index building
The current implementation might cause the file search popup to freeze while indexing thousands of files.
| val infiniteTransition = rememberInfiniteTransition() | ||
| val angle by infiniteTransition.animateFloat( | ||
| initialValue = 0f, | ||
| targetValue = 360f, | ||
| animationSpec = infiniteRepeatable(animation = tween(2000, easing = LinearEasing), repeatMode = RepeatMode.Restart) | ||
| ) |
There was a problem hiding this comment.
The PlanStepItem composable creates an infinite rotation animation for IN_PROGRESS steps (lines 201-206) that runs continuously. This animation is recreated every time the composable is recomposed, which could lead to:
- Multiple concurrent animations if the composable recomposes frequently
- Unnecessary CPU/GPU usage for all in-progress steps simultaneously
Consider using LaunchedEffect or making the animation conditional:
val infiniteTransition = rememberInfiniteTransition(label = "stepRotation")
val angle by if (step.status == TaskStatus.IN_PROGRESS) {
infiniteTransition.animateFloat(...)
} else {
remember { mutableStateOf(0f) }
}| val infiniteTransition = rememberInfiniteTransition() | |
| val angle by infiniteTransition.animateFloat( | |
| initialValue = 0f, | |
| targetValue = 360f, | |
| animationSpec = infiniteRepeatable(animation = tween(2000, easing = LinearEasing), repeatMode = RepeatMode.Restart) | |
| ) | |
| val angle by if (step.status == TaskStatus.IN_PROGRESS) { | |
| val infiniteTransition = rememberInfiniteTransition(label = "stepRotation") | |
| infiniteTransition.animateFloat( | |
| initialValue = 0f, | |
| targetValue = 360f, | |
| animationSpec = infiniteRepeatable(animation = tween(2000, easing = LinearEasing), repeatMode = RepeatMode.Restart) | |
| ) | |
| } else { | |
| remember { mutableStateOf(0f) } | |
| } |
| ## Plan Actions | ||
| - `CREATE`: Create a new plan with markdown content | ||
| - `COMPLETE_STEP`: Mark a step as done (taskIndex=1, stepIndex=1 for first step of first task) | ||
| - `VIEW`: View current plan status | ||
| Example: | ||
| <devin> | ||
| /plan | ||
| ```json | ||
| {"action": "CREATE", "planMarkdown": "1. Setup\n - [ ] Create entity class\n - [ ] Create repository\n\n2. Implementation\n - [ ] Create service\n - [ ] Create controller"} | ||
| ``` | ||
| </devin> | ||
There was a problem hiding this comment.
[nitpick] The system prompt guidance for the plan tool uses 1-based indexing (line 60: "taskIndex=1, stepIndex=1 for first step of first task"), but this should be more clearly documented in the tool's own description to avoid confusion. The PlanManagementTool description (lines 169-170) mentions "1-based" but could be more prominent.
Consider adding explicit examples with different index values to make it crystal clear:
Examples:
- First task, first step: taskIndex=1, stepIndex=1
- First task, second step: taskIndex=1, stepIndex=2
- Second task, first step: taskIndex=2, stepIndex=1
| sb.appendLine("$index. $title") | ||
| steps.forEach { step -> | ||
| sb.appendLine(" ${step.toMarkdown()}") |
There was a problem hiding this comment.
[nitpick] The toMarkdown() method adds a trailing newline after each task (line 128 calls appendLine for each step, and line 125 for the title). When multiple tasks are formatted together in MarkdownPlanParser.formatToMarkdown() or AgentPlan.toMarkdown(), this could result in extra blank lines between tasks.
This is likely intentional for readability, but consider documenting this behavior or ensuring consistent formatting. The current output would be:
1. Task 1
- [ ] Step 1
2. Task 2
- [ ] Step 2
Note the trailing blank line at the end and between tasks.
| sb.appendLine("$index. $title") | |
| steps.forEach { step -> | |
| sb.appendLine(" ${step.toMarkdown()}") | |
| sb.append("$index. $title\n") | |
| if (steps.isNotEmpty()) { | |
| sb.append(steps.joinToString(separator = "\n") { " ${it.toMarkdown()}" }) |
| fun performSearch(query: String) { | ||
| if (query.length < 2 || currentWorkspace == null || indexingState != IndexingState.READY) { | ||
| searchResults = emptyList() | ||
| isLoading = false | ||
| return | ||
| } | ||
|
|
||
| isLoading = true | ||
| scope.launch { | ||
| delay(150) // Debounce | ||
| try { | ||
| searchResults = searchProvider.searchFiles(query) | ||
| } catch (e: Exception) { | ||
| searchResults = emptyList() | ||
| } finally { | ||
| isLoading = false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] The performSearch function has a hardcoded debounce delay of 150ms (line 104) which might be too aggressive for large projects. When indexing is complete but search is slow (e.g., searching through 5000 files), the user experience could be poor.
Consider:
- Making the debounce delay configurable
- Showing a loading indicator immediately when typing starts
- Cancelling the previous search when a new one is triggered
The current implementation could result in multiple concurrent searches if the user types faster than the search completes.
- TaskStatus: add 'b' marker for BLOCKED, use ASCII 'x'/'v' instead of UTF-8 checkmark for WASM compatibility - PlanStateService: fix StateFlow emission by reassigning value, add thread-safe listener management - MarkdownPlanParser: use PlanStep.generateId() for globally unique IDs, update regex to ASCII-only markers - PlanStep: make generateId() public, add thread-safe ID generation with synchronized block - PlanTask: add thread-safe ID generation with synchronized block - ToolOrchestrator: validate taskIndex/stepIndex are positive for COMPLETE_STEP/FAIL_STEP actions - PlanPanel: only create infinite animation for IN_PROGRESS steps to reduce CPU/GPU usage
Summary
Implement a complete Task Management system for
CodingAgentinmpp-core, similar to Augment's task management capabilities. This allows the AI agent to create, update, and track task plans during execution.Closes #37
Changes
Phase 1: Core Data Models & Parser
TaskStatus.kt- Enum with 5 states (TODO, IN_PROGRESS, COMPLETED, FAILED, BLOCKED) and marker conversion functionsPlanStep.kt- Individual step within a task with status and file linksPlanTask.kt- Task containing multiple steps with computed statusAgentPlan.kt- Top-level plan container with progress trackingMarkdownPlanParser.kt- Pure Kotlin multiplatform parser (no IntelliJ dependencies)PlanStateService.kt- State management using StateFlow with listener patternPhase 2: PlanManagementTool Implementation
PlanManagementTool.kt- AI-callable tool with 5 actions:CREATE: Create a new plan from markdownUPDATE: Update existing planCOMPLETE_STEP: Mark a step as completed (1-based taskIndex/stepIndex)FAIL_STEP: Mark a step as failedVIEW: View current plan statusPhase 3: UI Components
PlanPanel.kt- Compose Multiplatform UI component with:Phase 4: Integration
CodingAgentTemplate.ktwith planning guidance in system promptBuiltinToolsProviderToolOrchestratorComposeRendererCleanup
TaskBoundaryToolasPlanManagementToolprovides superset functionalityTest Results
Unit Tests (30 tests passing)
MarkdownPlanParserTest- 11 testsPlanStateServiceTest- 10 testsPlanManagementToolTest- 9 testsIntegration Tests with CodingCli
Plan Format Example
Commits
feat(mpp-core): add plan management data models and parserfeat(mpp-core): add PlanManagementTool for AI agent task planningfeat(mpp-core): register PlanManagementTool in BuiltinToolsProviderfeat(mpp-ui): add PlanPanel UI component and integrate with ComposeRendererfeat(mpp-core): add plan management guidance to system prompt and fix parameter parsingrefactor(mpp-core): remove TaskBoundaryTool in favor of PlanManagementToolPull Request opened by Augment Code with guidance from the PR author
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.