feat(chatdb): add ChatDB agent for text-to-SQL interactions #509
Conversation
|
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. WalkthroughThis PR introduces a comprehensive Text2SQL (ChatDB) Agent feature enabling natural language to SQL conversion with schema linking, multi-database support, SQL validation/revision, and optional visualization. It adds new AgentType.CHAT_DB with corresponding UI components, core agent implementations, NLP tokenization, database infrastructure, platform-specific SQL validators, and a multiplatform data source repository with SQLDelight persistence. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as ChatDB UI
participant VM as ChatDBViewModel
participant Agent as ChatDBAgent
participant Executor as ChatDBAgentExecutor
participant SchemaLinker as SchemaLinker
participant LLM as LLM Service
participant Validator as SQL Validator
participant Reviser as SqlReviseAgent
participant DB as Database
User->>UI: Enter natural language query
UI->>VM: sendMessage(text)
VM->>Agent: execute(ChatDBTask)
Agent->>Executor: execute(task)
rect rgb(200, 220, 255)
Note over Executor,DB: Schema Linking Phase
Executor->>SchemaLinker: link(query, schema)
SchemaLinker->>SchemaLinker: extractKeywords(query)
SchemaLinker->>DB: (optional) fetch schema context
SchemaLinker-->>Executor: SchemaLinkingResult
Executor->>VM: renderChatDBStep(SCHEMA_LINKING, SUCCESS)
end
rect rgb(200, 255, 220)
Note over Executor,LLM: SQL Generation Phase
Executor->>Executor: buildInitialUserMessage(task, schema)
Executor->>LLM: generateSQL(systemPrompt, userMessage)
LLM-->>Executor: response with SQL
Executor->>Executor: extractSqlFromResponse(response)
Executor->>VM: renderChatDBStep(GENERATE_SQL, SUCCESS)
end
rect rgb(255, 240, 200)
Note over Executor,Validator: Validation & Revision Phase
Executor->>Validator: validate(sql)
alt SQL Invalid
Executor->>Reviser: execute(SqlRevisionInput)
Reviser->>LLM: askLLMForRevision(context)
LLM-->>Reviser: revised sql
Reviser->>Validator: validate(revisedSql)
Validator-->>Reviser: result
Reviser-->>Executor: revised sql or error
Executor->>VM: renderChatDBStep(REVISE_SQL, SUCCESS)
else SQL Valid
Executor->>VM: renderChatDBStep(VALIDATE_SQL, SUCCESS)
end
end
rect rgb(255, 220, 220)
Note over Executor,DB: Execution Phase
Executor->>DB: executeQuery(sql)
DB-->>Executor: QueryResult
Executor->>VM: renderChatDBStep(EXECUTE_SQL, SUCCESS)
end
rect rgb(220, 220, 255)
Note over Executor,LLM: Visualization Phase (optional)
alt generateVisualization flag
Executor->>Executor: buildTableContext(result)
Executor->>LLM: generateVisualization(data)
LLM-->>Executor: PlotDSL code
Executor->>VM: renderChatDBStep(GENERATE_VISUALIZATION, SUCCESS)
end
end
Executor-->>Agent: ChatDBResult
Agent-->>VM: ToolResult.AgentResult
VM->>UI: update state with result
UI->>User: Display SQL, results, visualization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
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 |
|
augment review |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| if (match != null && currentConnection != null) { | ||
| val sql = match.groupValues[1].trim() | ||
| try { | ||
| val result = currentConnection!!.executeQuery(sql) |
There was a problem hiding this comment.
Consider enforcing read-only execution before running LLM-generated SQL; currently any SQL inside the code fence will be executed, which risks destructive statements despite the prompt guidance. Add a validation (e.g., ensure it starts with SELECT and contains no write/DDL keywords) before calling executeQuery.
🤖 Was this useful? React with 👍 or 👎
| dataSources = state.dataSources.filter { it.id != id }, | ||
| selectedDataSourceId = if (state.selectedDataSourceId == id) null else state.selectedDataSourceId | ||
| ) | ||
| if (state.selectedDataSourceId == id) { |
There was a problem hiding this comment.
The check for state.selectedDataSourceId == id happens after you set it to null, so disconnect() won’t run when deleting the currently selected source; check before updating state or cache the previous selection to ensure the connection is closed. This avoids leaking an open DB connection (also relates to resource cleanup) (Guideline: no_memory_leaks).
🤖 Was this useful? React with 👍 or 👎
| @Composable | ||
| fun ChatDBPage( | ||
| workspace: Workspace, | ||
| llmService: KoogLLMService?, |
There was a problem hiding this comment.
llmService parameter is currently unused; the ViewModel creates its own instance instead. Consider threading the provided llmService through to avoid duplicate initialization and ensure consistent configuration.
🤖 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: 9
🧹 Nitpick comments (10)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuMobile.kt (1)
256-263: Consider extracting duplicated icon mapping logic.The same
AgentType -> Iconmapping appears in three places now (desktop tab, mobile header, mobile submenu). Consider extracting a shared helper function to reduce duplication and ensure consistency.// In AutoDevComposeIcons.kt or a shared location fun AgentType.toIcon(): ImageVector = when (this) { AgentType.REMOTE -> AutoDevComposeIcons.Cloud AgentType.CODE_REVIEW -> AutoDevComposeIcons.RateReview AgentType.KNOWLEDGE -> AutoDevComposeIcons.Article AgentType.CODING -> AutoDevComposeIcons.Code AgentType.LOCAL_CHAT -> AutoDevComposeIcons.Chat AgentType.CHAT_DB -> AutoDevComposeIcons.Database }mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.kt (2)
23-23: Unused import.
Platformis imported but not used in this file. Consider removing it to keep imports clean.-import cc.unitmesh.agent.Platform
151-152: Consider handling Connecting and Error states explicitly.The empty
elsebranch silently ignoresConnectionStatus.ConnectingandConnectionStatus.Errorstates. While these may be handled elsewhere, the user may benefit from visual feedback in the chat pane during these states (e.g., a "Connecting..." indicator or error message banner).mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigDialog.kt (1)
212-213: Consider providing validation feedback to users.Currently, the Save button is simply disabled when validation fails. Users may not understand why they can't save. Consider showing inline error messages or validation hints for required fields.
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.kt (2)
231-245: Consider adding delete confirmation.The delete action executes immediately without confirmation. Users could accidentally delete data source configurations. Consider showing a confirmation dialog before deletion.
283-290: CircularProgressIndicator color may not be visible on disabled button.The indicator uses
MaterialTheme.colorScheme.onPrimary, but the button is disabled, which typically reduces the container color opacity. The indicator might be hard to see. Consider usingMaterialTheme.colorScheme.onSurfaceor the primary color instead.CircularProgressIndicator( modifier = Modifier.size(16.dp), strokeWidth = 2.dp, - color = MaterialTheme.colorScheme.onPrimary + color = MaterialTheme.colorScheme.primary )mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/model/DataSourceModels.kt (2)
16-23: Silent fallback to MYSQL may cause confusion.
fromStringsilently returnsMYSQLwhen no match is found. This could mask data corruption or configuration errors. Consider throwing an exception or returning null to make failures explicit.companion object { - fun fromString(value: String): DatabaseDialect { - return entries.find { + fun fromString(value: String): DatabaseDialect? { + return entries.find { it.name.equals(value, ignoreCase = true) || it.displayName.equals(value, ignoreCase = true) - } ?: MYSQL + } } }Alternatively, if a default is preferred, add logging or documentation:
fun fromString(value: String): DatabaseDialect { return entries.find { ... } ?: MYSQL.also { /* log warning about unknown dialect */ } }
30-35: Default port doesn't match dialect default.The default
port = 3306is hardcoded, but this doesn't match all dialects (e.g., PostgreSQL uses 5432). Whiledialect.defaultPortexists, the class default could mislead. This is minor since the dialog updates the port when dialect changes.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt (2)
64-66: Use structured logging instead of println.
printlnis not suitable for production logging. Consider using a proper logging framework that supports log levels and structured output.} catch (e: Exception) { - println("[ChatDB] Failed to initialize LLM service: ${e.message}") + // TODO: Use proper logging framework + // logger.error("[ChatDB] Failed to initialize LLM service", e) }
198-199: Pass system prompt as a separate message instead of concatenating with user input.The
streamPromptmethod acceptshistoryMessages: List<Message>which supports proper message roles viaMessageRole.SYSTEM. Instead of concatenating the system prompt with the user text (userPrompt = "$systemPrompt\n\nUser: $text"), pass it as a separate message:service.streamPrompt( userPrompt = text, historyMessages = listOf(Message(role = MessageRole.SYSTEM, content = systemPrompt)), compileDevIns = false ).collect { chunk -> // ... }This allows the LLM to properly distinguish system instructions from user queries, following the standard message role pattern already implemented in
KoogLLMService.buildPrompt.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/AgentType.kt(4 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentChatInterface.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt(2 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigDialog.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/model/DataSourceModels.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuDesktop.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuMobile.kt(2 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/icons/AutoDevComposeIcons.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualpattern for platform-specific code (for example:Platformimplementations)
Use concrete classes (not interfaces) andPromise(notFlow) for @JsExport annotations
Avoid emoji and UTF-8 characters in WASM code
When modifyingCodingAgentRenderer, update ALL implementations:DefaultCodingAgentRenderer,ComposeRenderer,JewelRenderer,ServerSideRenderer, andJsRendererAdapterin Kotlin
When modifyingCodingAgentRenderer, update JVM CLI implementations:CodingCliRendererandConsoleRenderer
UseAutoDevColorsorMaterialTheme.colorSchemefor Compose styling
Files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigDialog.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuDesktop.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/model/DataSourceModels.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/AgentType.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentChatInterface.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuMobile.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/icons/AutoDevComposeIcons.kt
**/*.{kt,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never use hardcoded colors - always use design tokens from the design system
Files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigDialog.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuDesktop.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/model/DataSourceModels.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/AgentType.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentChatInterface.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuMobile.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/icons/AutoDevComposeIcons.kt
🧠 Learnings (3)
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to mpp-vscode/src/**/*.ts : When modifying `CodingAgentRenderer`, update VSCode implementations: `mpp-vscode/src/bridge/mpp-core.ts` and `mpp-vscode/src/providers/chat-view.ts`
Applied to files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentChatInterface.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update ALL implementations: `DefaultCodingAgentRenderer`, `ComposeRenderer`, `JewelRenderer`, `ServerSideRenderer`, and `JsRendererAdapter` in Kotlin
Applied to files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentChatInterface.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update JVM CLI implementations: `CodingCliRenderer` and `ConsoleRenderer`
Applied to files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentChatInterface.kt
🧬 Code graph analysis (3)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.kt (2)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentMessageList.kt (1)
AgentMessageList(31-158)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt (1)
sendMessage(177-221)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt (3)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.kt (1)
DataSourcePanel(23-82)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.kt (1)
ChatDBChatPane(33-78)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigDialog.kt (1)
DataSourceConfigDialog(23-221)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt (1)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt (1)
ChatDBPage(22-126)
⏰ 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: Agent
- GitHub Check: Test mpp-core (KMP)
- GitHub Check: Build
🔇 Additional comments (19)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/icons/AutoDevComposeIcons.kt (1)
116-118: LGTM!The new
DatabaseandSchemaicon aliases are well-placed under a clear category comment and follow the established pattern of mapping to Material icons. UsingStoragefor database andTableChartfor schema are semantically appropriate choices.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuDesktop.kt (1)
283-290: LGTM!The
CHAT_DBicon mapping is consistent with other agent type mappings in thewhenexpression. Uses the design tokenAutoDevComposeIcons.Databaseas per coding guidelines.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuMobile.kt (1)
221-228: LGTM!The
CHAT_DBicon mapping is correctly added for the Agent Type header display, consistent with the desktop implementation.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt (1)
6-9: LGTM on new imports.The imports for
ChatDBPageandWorkspaceare necessary for the new routing logic.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/AgentType.kt (3)
35-38: LGTM!The
CHAT_DBenum constant is well-documented and follows the established pattern. The KDoc clearly describes its purpose for text-to-SQL interactions.
45-52: LGTM on display name mapping.The
getDisplayName()extension correctly returns "ChatDB" for the new agent type, maintaining consistency with the feature naming throughout the PR.
54-65: LGTM on string parsing.The
fromString()method handles both "chatdb" and "database" as valid inputs forCHAT_DB, providing flexibility for different input sources.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.kt (2)
264-271: Keyboard handling looks correct.Enter sends the message, Shift+Enter allows newlines. The implementation correctly prevents send when shift is pressed.
33-78: Well-structured composable layout.The ChatDBChatPane follows good Compose patterns with clear separation of concerns: status banner, message area with welcome overlay, and input area. The use of
MaterialTheme.colorSchemethroughout adheres to the design system guidelines.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigDialog.kt (2)
155-170: Password field implementation looks good.The password field correctly uses
PasswordVisualTransformationwith a visibility toggle. Not trimming the password value (line 207) is the correct behavior.
199-210: The empty ID is properly handled by the viewModel layer. WhenDataSourceConfigDialogpassesid = ""toonSave, it callsviewModel.addDataSource(), which generates a unique UUID before storing:id = if (config.id.isBlank()) Uuid.random().toString() else config.id. This ensures no empty IDs reach the LazyColumn, which useskey = { it.id }for recomposition. The design correctly separates concerns: UI layer doesn't generate IDs; viewModel ensures uniqueness before state updates.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt (2)
128-168: SchemaInfoDialog implementation is clean and functional.The dialog properly handles null schema, limits display to 10 tables with an overflow indicator, and uses Material theme styling correctly. Good implementation.
31-31: No issue:llmServiceis not a constructor parameter.The
ChatDBViewModelconstructor intentionally takes only theworkspaceparameter. ThellmServiceis initialized internally via theinitializeLLMService()method in theinitblock, which loads configuration fromConfigManagerand creates the service asynchronously. ThesendMessage()function properly handles the case where the service is null and displays a user-friendly error message (lines 185-190).Likely an incorrect or invalid review comment.
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.kt (1)
23-82: Well-structured panel component.The DataSourcePanel follows good practices with clear separation into private composables (DataSourceHeader, SearchField, DataSourceItem, ConnectionControls). Using
key = { it.id }in LazyColumn ensures efficient recomposition. The component correctly usesMaterialTheme.colorSchemefor all colors per the coding guidelines.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/model/DataSourceModels.kt (2)
92-105: Good use of sealed classes for type-safe state modeling.
ValidationResultandConnectionStatusare well-designed sealed classes that provide exhaustive pattern matching and clear state representation. This is idiomatic Kotlin.
144-167: ChatDBState is well-structured.The state class correctly uses derived properties for
selectedDataSourceandfilteredDataSources. For small to moderate data source lists, the on-access computation is acceptable. If performance becomes an issue with large lists, these could be cached.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt (3)
118-139: LGTM!The data source selection and dialog management logic is clean and correctly handles disconnection before switching data sources.
260-263: Redundant state update instopGeneration().Setting
isGenerating = falseis redundant since the cancelled job'sfinallyblock already sets it. However, it provides immediate feedback if the cancellation is slow, so this is acceptable.
267-271: LGTM!The
dispose()method properly cleans up all resources: stops generation, disconnects the database, and cancels the coroutine scope.
| AgentType.CODE_REVIEW, | ||
| AgentType.KNOWLEDGE -> { | ||
| // CODE_REVIEW and DOCUMENT_READER have their own full-page interfaces | ||
| AgentType.KNOWLEDGE, | ||
| AgentType.CHAT_DB -> { | ||
| // CODE_REVIEW, DOCUMENT_READER and CHAT_DB have their own full-page interfaces | ||
| // They should not reach here - handled by AgentInterfaceRouter | ||
| } |
There was a problem hiding this comment.
Fix comment inconsistency.
The comment references "DOCUMENT_READER" but the enum constant is KNOWLEDGE. Update for clarity.
AgentType.CODE_REVIEW,
AgentType.KNOWLEDGE,
AgentType.CHAT_DB -> {
- // CODE_REVIEW, DOCUMENT_READER and CHAT_DB have their own full-page interfaces
+ // CODE_REVIEW, KNOWLEDGE and CHAT_DB have their own full-page interfaces
// They should not reach here - handled by AgentInterfaceRouter
}🤖 Prompt for AI Agents
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentChatInterface.kt
around lines 200 to 205: the inline comment mentions "DOCUMENT_READER" which is
inconsistent with the enum constant used (`KNOWLEDGE`); update the comment to
reference `KNOWLEDGE` (or a neutral phrase like "DOCUMENT_READER/KNOWLEDGE" if
both are meaningful) so it matches the code and avoids confusion.
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt
Show resolved
Hide resolved
| if (state.connectionStatus is ConnectionStatus.Connected) { | ||
| var showSchemaDialog by remember { mutableStateOf(false) } | ||
| IconButton(onClick = { showSchemaDialog = true }) { | ||
| Icon(AutoDevComposeIcons.Schema, contentDescription = "View Schema") | ||
| } | ||
| if (showSchemaDialog) { | ||
| SchemaInfoDialog( | ||
| schema = viewModel.getSchema(), | ||
| onDismiss = { showSchemaDialog = false } | ||
| ) | ||
| } |
There was a problem hiding this comment.
State variable showSchemaDialog is scoped inside conditional block.
showSchemaDialog is declared inside the if (state.connectionStatus is ConnectionStatus.Connected) block using remember. This means the state will be recreated each time the condition becomes true again after being false, which could cause unexpected dialog dismissals. Hoist this state to the ChatDBPage level.
+ var showSchemaDialog by remember { mutableStateOf(false) }
+
Scaffold(
topBar = {
TopAppBar(
// ...
actions = {
// Schema info button when connected
if (state.connectionStatus is ConnectionStatus.Connected) {
- var showSchemaDialog by remember { mutableStateOf(false) }
IconButton(onClick = { showSchemaDialog = true }) {
Icon(AutoDevComposeIcons.Schema, contentDescription = "View Schema")
}
- if (showSchemaDialog) {
- SchemaInfoDialog(
- schema = viewModel.getSchema(),
- onDismiss = { showSchemaDialog = false }
- )
- }
}
}
)
- },
+ }
+ // ... after Scaffold closing brace
+ if (showSchemaDialog) {
+ SchemaInfoDialog(
+ schema = viewModel.getSchema(),
+ onDismiss = { showSchemaDialog = false }
+ )
+ }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/chatdb/ChatDBPage.kt
around lines 59 to 69, the remember-backed state variable showSchemaDialog is
declared inside the conditional that checks ConnectionStatus.Connected which
causes the state to be recreated when the condition toggles; move (hoist) the
declaration of showSchemaDialog to the ChatDBPage composable scope above the
conditional (e.g., before checking state.connectionStatus) so it persists across
recompositions and condition changes, then keep using the same variable inside
the conditional for IconButton and SchemaInfoDialog and update it via the same
onClick/onDismiss handlers.
| class ChatDBViewModel( | ||
| private val workspace: Workspace | ||
| ) { | ||
| private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default) |
There was a problem hiding this comment.
Compose state updates should occur on the main dispatcher.
The scope uses Dispatchers.Default, but the ViewModel modifies Compose mutableStateOf properties (state, isGenerating) from coroutines launched on this scope. Compose state should be updated from the main thread to avoid potential threading issues.
Consider using Dispatchers.Main for state updates:
- private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default)
+ private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Main)Alternatively, keep Dispatchers.Default for background work but switch to Main when updating state:
withContext(Dispatchers.Main) {
state = state.copy(...)
}
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt
Show resolved
Hide resolved
| fun connect() { | ||
| val dataSource = state.selectedDataSource ?: return | ||
|
|
||
| scope.launch { | ||
| state = state.copy(connectionStatus = ConnectionStatus.Connecting) | ||
|
|
||
| try { | ||
| val connection = createDatabaseConnection(dataSource.toDatabaseConfig()) | ||
| if (connection.isConnected()) { | ||
| currentConnection = connection | ||
| currentSchema = connection.getSchema() | ||
| state = state.copy(connectionStatus = ConnectionStatus.Connected) | ||
| _notificationEvent.emit("Connected" to "Successfully connected to ${dataSource.name}") | ||
| } else { | ||
| state = state.copy(connectionStatus = ConnectionStatus.Error("Failed to connect")) | ||
| } | ||
| } catch (e: Exception) { | ||
| state = state.copy(connectionStatus = ConnectionStatus.Error(e.message ?: "Unknown error")) | ||
| _notificationEvent.emit("Connection Failed" to (e.message ?: "Unknown error")) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential race condition with rapid connect() calls.
If connect() is called multiple times rapidly (e.g., user double-clicks), multiple connections could be created concurrently. Only the last connection is stored in currentConnection, leaving others orphaned and unclosed.
Consider cancelling any in-flight connection attempt before starting a new one:
+ private var connectionJob: Job? = null
+
fun connect() {
val dataSource = state.selectedDataSource ?: return
- scope.launch {
+ connectionJob?.cancel()
+ connectionJob = scope.launch {
state = state.copy(connectionStatus = ConnectionStatus.Connecting)
try {📝 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.
| fun connect() { | |
| val dataSource = state.selectedDataSource ?: return | |
| scope.launch { | |
| state = state.copy(connectionStatus = ConnectionStatus.Connecting) | |
| try { | |
| val connection = createDatabaseConnection(dataSource.toDatabaseConfig()) | |
| if (connection.isConnected()) { | |
| currentConnection = connection | |
| currentSchema = connection.getSchema() | |
| state = state.copy(connectionStatus = ConnectionStatus.Connected) | |
| _notificationEvent.emit("Connected" to "Successfully connected to ${dataSource.name}") | |
| } else { | |
| state = state.copy(connectionStatus = ConnectionStatus.Error("Failed to connect")) | |
| } | |
| } catch (e: Exception) { | |
| state = state.copy(connectionStatus = ConnectionStatus.Error(e.message ?: "Unknown error")) | |
| _notificationEvent.emit("Connection Failed" to (e.message ?: "Unknown error")) | |
| } | |
| } | |
| } | |
| private var connectionJob: Job? = null | |
| fun connect() { | |
| val dataSource = state.selectedDataSource ?: return | |
| connectionJob?.cancel() | |
| connectionJob = scope.launch { | |
| state = state.copy(connectionStatus = ConnectionStatus.Connecting) | |
| try { | |
| val connection = createDatabaseConnection(dataSource.toDatabaseConfig()) | |
| if (connection.isConnected()) { | |
| currentConnection = connection | |
| currentSchema = connection.getSchema() | |
| state = state.copy(connectionStatus = ConnectionStatus.Connected) | |
| _notificationEvent.emit("Connected" to "Successfully connected to ${dataSource.name}") | |
| } else { | |
| state = state.copy(connectionStatus = ConnectionStatus.Error("Failed to connect")) | |
| } | |
| } catch (e: Exception) { | |
| state = state.copy(connectionStatus = ConnectionStatus.Error(e.message ?: "Unknown error")) | |
| _notificationEvent.emit("Connection Failed" to (e.message ?: "Unknown error")) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt
around lines 141-162, connect() can be invoked concurrently causing multiple
in-flight connection coroutines and orphaned connections; fix by tracking the
active connect Job and cancelling it before starting a new one, and ensure any
previous temporary connection is closed when superseded: add a private var
connectJob: Job? and before scope.launch call connectJob?.cancel(), then assign
connectJob = scope.launch { ... }; additionally, if a new connection succeeds,
close any existing currentConnection before replacing it (and ensure cancelled
coroutine closes its partially created connection in a finally block), or
protect connection assignment with a Mutex to avoid races.
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt
Outdated
Show resolved
Hide resolved
| private suspend fun extractAndExecuteSQL(response: String) { | ||
| val sqlPattern = Regex("```sql\\n([\\s\\S]*?)```", RegexOption.IGNORE_CASE) | ||
| val match = sqlPattern.find(response) | ||
|
|
||
| if (match != null && currentConnection != null) { | ||
| val sql = match.groupValues[1].trim() | ||
| try { | ||
| val result = currentConnection!!.executeQuery(sql) | ||
| // Display query result as a new message | ||
| renderer.renderLLMResponseStart() | ||
| renderer.renderLLMResponseChunk("\n\n**Query Result:**\n```\n${result.toTableString()}\n```") | ||
| renderer.renderLLMResponseEnd() | ||
| } catch (e: Exception) { | ||
| renderer.renderError("Query Error: ${e.message}") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical security risk: No server-side SQL validation before execution.
The system prompt requests "only SELECT queries," but there's no actual validation. A malicious user could craft prompts that trick the LLM into generating destructive SQL (DROP, DELETE, UPDATE, etc.) via prompt injection. This is explicitly mentioned as a requirement in the linked issue: "AST-based SQL validation (prevent destructive statements)."
Add SQL validation before execution:
private suspend fun extractAndExecuteSQL(response: String) {
val sqlPattern = Regex("```sql\\n([\\s\\S]*?)```", RegexOption.IGNORE_CASE)
val match = sqlPattern.find(response)
if (match != null && currentConnection != null) {
val sql = match.groupValues[1].trim()
+
+ // Validate SQL is read-only (SELECT only)
+ val normalizedSql = sql.trim().uppercase()
+ if (!normalizedSql.startsWith("SELECT") ||
+ normalizedSql.contains(Regex("\\b(INSERT|UPDATE|DELETE|DROP|TRUNCATE|ALTER|CREATE)\\b"))) {
+ renderer.renderError("Security: Only SELECT queries are allowed")
+ return
+ }
+
try {
val result = currentConnection!!.executeQuery(sql)For production, consider AST-based SQL parsing for more robust validation.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new ChatDB agent type that enables text-to-SQL interactions, allowing users to connect to databases and query them using natural language. The implementation includes a full-featured UI with data source management, connection handling, and a chat interface for natural language queries that are automatically converted to SQL and executed.
Key Changes
- New AgentType: Added
CHAT_DBenum value with routing support for database chat functionality - Data Source Management: Complete UI for adding, editing, deleting, and connecting to multiple database types (MySQL, PostgreSQL, SQLite, Oracle, SQL Server, MariaDB)
- Text-to-SQL Chat: LLM-powered natural language to SQL conversion with automatic query extraction and execution, displaying results in formatted tables
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/AgentType.kt |
Added CHAT_DB enum value with display name and fromString mapping |
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/icons/AutoDevComposeIcons.kt |
Added Database and Schema icons using Material Icons |
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuDesktop.kt |
Added Database icon mapping for CHAT_DB agent type |
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuMobile.kt |
Added Database icon mapping for CHAT_DB agent type (2 locations) |
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/model/DataSourceModels.kt |
Defines data models for database configurations, connection status, messages, and UI state |
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.kt |
Left panel component for managing data source list, search, and connection controls |
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigDialog.kt |
Dialog for adding/editing database connection configurations with dialect-specific fields |
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.kt |
Right panel chat interface with connection status banner, welcome message, and input area |
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt |
ViewModel managing data sources, database connections, LLM interactions, and SQL execution |
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt |
Main page with split layout, top bar, and schema dialog |
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt |
Added CHAT_DB routing with workspace parameter |
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentChatInterface.kt |
Added CHAT_DB to the full-page interface exclusion list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val match = sqlPattern.find(response) | ||
|
|
||
| if (match != null && currentConnection != null) { | ||
| val sql = match.groupValues[1].trim() | ||
| try { | ||
| val result = currentConnection!!.executeQuery(sql) | ||
| // Display query result as a new message | ||
| renderer.renderLLMResponseStart() | ||
| renderer.renderLLMResponseChunk("\n\n**Query Result:**\n```\n${result.toTableString()}\n```") | ||
| renderer.renderLLMResponseEnd() | ||
| } catch (e: Exception) { | ||
| renderer.renderError("Query Error: ${e.message}") | ||
| } |
There was a problem hiding this comment.
The SQL extraction regex pattern uses a case-insensitive flag, but there's a potential security issue: the code only extracts and executes the first SQL block. If the LLM response contains multiple SQL blocks, only the first one will be executed. Consider either executing all found SQL blocks or making it explicit to the user that only the first block will be run.
| val match = sqlPattern.find(response) | |
| if (match != null && currentConnection != null) { | |
| val sql = match.groupValues[1].trim() | |
| try { | |
| val result = currentConnection!!.executeQuery(sql) | |
| // Display query result as a new message | |
| renderer.renderLLMResponseStart() | |
| renderer.renderLLMResponseChunk("\n\n**Query Result:**\n```\n${result.toTableString()}\n```") | |
| renderer.renderLLMResponseEnd() | |
| } catch (e: Exception) { | |
| renderer.renderError("Query Error: ${e.message}") | |
| } | |
| val matches = sqlPattern.findAll(response).toList() | |
| if (matches.isNotEmpty() && currentConnection != null) { | |
| renderer.renderLLMResponseStart() | |
| for ((index, match) in matches.withIndex()) { | |
| val sql = match.groupValues[1].trim() | |
| try { | |
| val result = currentConnection!!.executeQuery(sql) | |
| // Display query result as a new message | |
| val label = if (matches.size > 1) "\n\n**Query Result ${index + 1}:**\n" else "\n\n**Query Result:**\n" | |
| renderer.renderLLMResponseChunk("$label```\n${result.toTableString()}\n```") | |
| } catch (e: Exception) { | |
| renderer.renderError("Query Error: ${e.message}") | |
| } | |
| } | |
| renderer.renderLLMResponseEnd() |
| state = state.copy( | ||
| dataSources = state.dataSources.filter { it.id != id }, | ||
| selectedDataSourceId = if (state.selectedDataSourceId == id) null else state.selectedDataSourceId | ||
| ) | ||
| if (state.selectedDataSourceId == id) { | ||
| disconnect() | ||
| } |
There was a problem hiding this comment.
The disconnect operation should also be performed when the selected data source is deleted (line 108-110), but the current implementation calls disconnect() inside the condition that checks if the deleted source was selected. However, since state.selectedDataSourceId is updated before the check on line 108, the condition will never be true. Move the disconnect call before updating the state, or use the id parameter for the comparison.
| state = state.copy( | |
| dataSources = state.dataSources.filter { it.id != id }, | |
| selectedDataSourceId = if (state.selectedDataSourceId == id) null else state.selectedDataSourceId | |
| ) | |
| if (state.selectedDataSourceId == id) { | |
| disconnect() | |
| } | |
| if (state.selectedDataSourceId == id) { | |
| disconnect() | |
| } | |
| state = state.copy( | |
| dataSources = state.dataSources.filter { it.id != id }, | |
| selectedDataSourceId = if (state.selectedDataSourceId == id) null else state.selectedDataSourceId | |
| ) |
| renderer.renderLLMResponseStart() | ||
|
|
||
| service.streamPrompt( | ||
| userPrompt = "$systemPrompt\n\nUser: $text", |
There was a problem hiding this comment.
The system prompt is included in every user message (line 199), which increases token usage and costs significantly. Consider using a proper conversation history structure where the system prompt is sent once at the beginning, or use the LLM service's system message parameter if available.
| userPrompt = "$systemPrompt\n\nUser: $text", | |
| userPrompt = text, | |
| systemPrompt = systemPrompt, |
| @Serializable | ||
| data class ChatDBMessage( | ||
| val id: String, | ||
| val role: MessageRole, | ||
| val content: String, | ||
| val timestamp: Long, | ||
| val sql: String? = null, | ||
| val queryResult: QueryResultDisplay? = null | ||
| ) | ||
|
|
There was a problem hiding this comment.
The ChatDBMessage data class (lines 111-118) is defined but never used in the codebase. The chat functionality uses ComposeRenderer which has its own message representation. Consider removing this unused model or implementing proper message persistence if it's intended for future use.
| @Serializable | |
| data class ChatDBMessage( | |
| val id: String, | |
| val role: MessageRole, | |
| val content: String, | |
| val timestamp: Long, | |
| val sql: String? = null, | |
| val queryResult: QueryResultDisplay? = null | |
| ) |
| private fun loadDataSources() { | ||
| // TODO: Load from persistent storage | ||
| // For now, use empty list | ||
| state = state.copy(dataSources = emptyList()) | ||
| } | ||
|
|
||
| @OptIn(ExperimentalUuidApi::class) | ||
| fun addDataSource(config: DataSourceConfig) { | ||
| val newConfig = config.copy( | ||
| id = if (config.id.isBlank()) Uuid.random().toString() else config.id, | ||
| createdAt = kotlinx.datetime.Clock.System.now().toEpochMilliseconds(), | ||
| updatedAt = kotlinx.datetime.Clock.System.now().toEpochMilliseconds() | ||
| ) | ||
| state = state.copy( | ||
| dataSources = state.dataSources + newConfig, | ||
| isConfigDialogOpen = false, | ||
| editingDataSource = null | ||
| ) | ||
| saveDataSources() | ||
| } | ||
|
|
||
| fun updateDataSource(config: DataSourceConfig) { | ||
| val updated = config.copy( | ||
| updatedAt = kotlinx.datetime.Clock.System.now().toEpochMilliseconds() | ||
| ) | ||
| state = state.copy( | ||
| dataSources = state.dataSources.map { if (it.id == config.id) updated else it }, | ||
| isConfigDialogOpen = false, | ||
| editingDataSource = null | ||
| ) | ||
| saveDataSources() | ||
| } | ||
|
|
||
| fun deleteDataSource(id: String) { | ||
| state = state.copy( | ||
| dataSources = state.dataSources.filter { it.id != id }, | ||
| selectedDataSourceId = if (state.selectedDataSourceId == id) null else state.selectedDataSourceId | ||
| ) | ||
| if (state.selectedDataSourceId == id) { | ||
| disconnect() | ||
| } | ||
| saveDataSources() | ||
| } | ||
|
|
||
| private fun saveDataSources() { | ||
| // TODO: Persist to storage | ||
| } |
There was a problem hiding this comment.
Data source configurations are not persisted to storage (TODOs on lines 71, 115). This means users will lose all their configured database connections when the application restarts. Consider implementing persistence using ConfigManager or a similar storage mechanism before releasing this feature, as losing database configurations on restart significantly impacts usability.
| onNotification = onNotification | ||
| ) | ||
| } else { | ||
| // Fallback to coding if workspace is not available |
There was a problem hiding this comment.
The fallback to CODING agent when workspace is null (line 75) happens silently by calling onAgentTypeChange. This could be confusing to users who selected ChatDB. Consider showing a notification or error message explaining why ChatDB is unavailable before falling back to the CODING agent.
| // Fallback to coding if workspace is not available | |
| // Notify user and fallback to coding if workspace is not available | |
| onNotification( | |
| "ChatDB Unavailable", | |
| "The ChatDB agent requires an open workspace. Falling back to Coding agent." | |
| ) |
| onProjectChange: (String) -> Unit = {}, | ||
| onGitUrlChange: (String) -> Unit = {}, | ||
| onNotification: (String, String) -> Unit = { _, _ -> }, | ||
| workspace: Workspace? = null, |
There was a problem hiding this comment.
The workspace parameter is added to AgentInterfaceRouter but is not passed from AutoDevApp.kt where it's called (line 575-641). The currentWorkspace variable exists in AutoDevApp but needs to be added to the AgentInterfaceRouter call around line 640, otherwise ChatDB will always fall back to CODING mode.
| val result = currentConnection!!.executeQuery(sql) | ||
| // Display query result as a new message | ||
| renderer.renderLLMResponseStart() | ||
| renderer.renderLLMResponseChunk("\n\n**Query Result:**\n```\n${result.toTableString()}\n```") |
There was a problem hiding this comment.
Direct SQL execution from LLM output poses SQL injection risks. While the system prompt asks for SELECT-only queries, the LLM could still generate queries with malicious WHERE clauses or subqueries that could cause performance issues or data exposure. Consider using parameterized queries, implementing a SQL parser to validate and sanitize queries, or at minimum adding query timeouts and result size limits.
| private suspend fun extractAndExecuteSQL(response: String) { | ||
| val sqlPattern = Regex("```sql\\n([\\s\\S]*?)```", RegexOption.IGNORE_CASE) | ||
| val match = sqlPattern.find(response) | ||
|
|
||
| if (match != null && currentConnection != null) { | ||
| val sql = match.groupValues[1].trim() | ||
| try { | ||
| val result = currentConnection!!.executeQuery(sql) | ||
| // Display query result as a new message | ||
| renderer.renderLLMResponseStart() | ||
| renderer.renderLLMResponseChunk("\n\n**Query Result:**\n```\n${result.toTableString()}\n```") | ||
| renderer.renderLLMResponseEnd() | ||
| } catch (e: Exception) { | ||
| renderer.renderError("Query Error: ${e.message}") | ||
| } | ||
| } |
There was a problem hiding this comment.
The system prompt restricts SQL to SELECT queries only for safety, but this restriction is not enforced in the code. The extractAndExecuteSQL function will execute any SQL statement that matches the pattern. Consider adding SQL validation to ensure only SELECT queries are executed, or use a SQL parser to validate the query type before execution.
| try { | ||
| val connection = createDatabaseConnection(dataSource.toDatabaseConfig()) | ||
| if (connection.isConnected()) { | ||
| currentConnection = connection | ||
| currentSchema = connection.getSchema() | ||
| state = state.copy(connectionStatus = ConnectionStatus.Connected) | ||
| _notificationEvent.emit("Connected" to "Successfully connected to ${dataSource.name}") | ||
| } else { | ||
| state = state.copy(connectionStatus = ConnectionStatus.Error("Failed to connect")) | ||
| } | ||
| } catch (e: Exception) { | ||
| state = state.copy(connectionStatus = ConnectionStatus.Error(e.message ?: "Unknown error")) | ||
| _notificationEvent.emit("Connection Failed" to (e.message ?: "Unknown error")) | ||
| } | ||
| } |
There was a problem hiding this comment.
The connection error handling wraps the exception message, but doesn't close the potentially half-opened connection. Consider calling connection.close() in the catch block if the connection object was created but isConnected() returns false or throws an exception.
phodal
force-pushed
the
feature/chatdb-agent
branch
from
5229c24 to
f0fc6a4
Compare
- Add CHAT_DB agent type to AgentType enum - Create ChatDBPage with split layout (DataSourcePanel + ChatDBChatPane) - Create ChatDBViewModel for managing data sources and chat - Create DataSourcePanel for managing database connections - Create DataSourceConfigDialog for adding/editing data sources - Create ChatDBChatPane for chat interactions - Add Database and Schema icons to AutoDevComposeIcons - Update AgentInterfaceRouter to route to ChatDBPage - Update when expressions in AgentChatInterface, TopBarMenuDesktop, TopBarMenuMobile Closes #508
phodal
force-pushed
the
feature/chatdb-agent
branch
from
f0fc6a4 to
eb0d669
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (11)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.kt (1)
161-188: AgentType string persistence is correct, but ChatDB route never receives a workspace
The updated
wheninhandleAgentTypeChangethat maps:
REMOTE -> "Remote",LOCAL_CHAT -> "Local",CODING -> "Coding",CODE_REVIEW -> "CodeReview",KNOWLEDGE -> "Documents",CHAT_DB -> "ChatDB"
is a solid improvement over a genericelseand matches the mappings used inDesktopUiStateandIdeaAgentViewModel.However,
AgentInterfaceRouternow takesworkspace: Workspace? = null, and the call inAutoDevAppdoes not pass it, soworkspaceis alwaysnullinside the router. That means theAgentType.CHAT_DBbranch will always hit the placeholder"Please select a workspace to use ChatDB"even thoughAutoDevAppis already trackingcurrentWorkspace. In practice this makes ChatDB unusable from the desktop UI.You can fix this by wiring
currentWorkspacethrough to the router:- AgentInterfaceRouter( + AgentInterfaceRouter( llmService = llmService, isTreeViewVisible = isTreeViewVisible, onConfigWarning = { showModelConfigDialog = true }, onToggleTreeView = { /* 不需要，由全局状态管理 */ }, chatHistoryManager = chatHistoryManager, selectedAgentType = selectedAgentType, onAgentTypeChange = { type -> handleAgentTypeChange(type) }, onSessionSelected = { sessionId -> messages = chatHistoryManager.getMessages() currentStreamingOutput = "" }, onNewChat = { messages = emptyList() currentStreamingOutput = "" }, onInternalSessionSelected = { handler -> agentSessionSelectedHandler = handler }, onInternalNewChat = { handler -> agentNewChatHandler = handler }, hasHistory = messages.isNotEmpty(), hasDebugInfo = compilerOutput.isNotEmpty(), currentModelConfig = currentModelConfig, selectedAgent = selectedAgent, availableAgents = availableAgents, onOpenDirectory = { openDirectoryChooser() }, onClearHistory = { chatHistoryManager.clearCurrentSession() messages = emptyList() currentStreamingOutput = "" }, onModelConfigChange = { config -> currentModelConfig = config if (config.isValid()) { try { llmService = KoogLLMService.create(config) println("切换模型: ${config.provider.displayName} / ${config.modelName}") } catch (e: Exception) { println("切换模型失败: ${e.message}") } } }, onAgentChange = { agent -> selectedAgent = agent }, onConfigureRemote = { showRemoteConfigDialog = true }, onShowModelConfig = { showModelConfigDialog = true }, onShowToolConfig = { showToolConfigDialog = true }, - serverUrl = serverUrl, + serverUrl = serverUrl, useServerConfig = useServerConfig, projectId = remoteProjectId, gitUrl = remoteGitUrl, onProjectChange = { projectId -> remoteProjectId = projectId println("📁 Project ID: $projectId") }, onGitUrlChange = { url -> remoteGitUrl = url println("📦 Git URL: $url") }, - onNotification = onNotification, - modifier = Modifier.fillMaxSize() + onNotification = onNotification, + workspace = currentWorkspace, + modifier = Modifier.fillMaxSize() )As a follow-up (non-blocking), consider centralizing this AgentType→string mapping into a small extension or helper (e.g.,
AgentType.toConfigKey()in a shared module) to avoid having the samewhenduplicated across multiple files.Also applies to: 578-644
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigDialog.kt (1)
197-210: Validation logic should useDataSourceConfig.validate()method.The button enable state only checks basic field presence (lines 212-213), but this bypasses the
ValidationResultlogic defined inDataSourceConfig.validate(). This means invalid port ranges (e.g., > 65535) won't be caught until save time, and error messages won't be displayed to users.This issue was previously flagged in past review comments.
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt (2)
59-69: State variableshowSchemaDialogis scoped inside conditional block.The
showSchemaDialogstate is declared inside theif (state.connectionStatus is ConnectionStatus.Connected)block. This means the state will be recreated each time the connection status toggles, potentially causing unexpected dialog dismissals. Hoist this state to theChatDBPagelevel before theScaffold.This issue was previously raised in past review comments.
26-26:llmServiceparameter is unused.The
llmServiceparameter is passed toChatDBPagebut never used. TheChatDBViewModelappears to create its own instance. Consider either removing this parameter or threading it through to the ViewModel to avoid duplicate initialization.This issue was previously raised by augmentcode in past review comments.
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.kt (1)
277-291: Consider adding a cancel button for the Connecting state.The Connecting state shows a disabled button with a spinner, but users cannot cancel a hung connection attempt. This could leave users stuck if the connection times out slowly.
This issue was previously flagged in past review comments.
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/model/DataSourceModels.kt (2)
60-66: Clarify validation error message for non-SQLite ports.Validation checks
portonly whendialect != DatabaseDialect.SQLITE, but the error message says “Port must be between 1 and 65535” without indicating this is for non-SQLite databases. This can be slightly confusing in mixed-dialect UIs.You could make this explicit:
- if (port <= 0 || port > 65535) errors.add("Port must be between 1 and 65535") + if (port <= 0 || port > 65535) { + errors.add("Port must be between 1 and 65535 (for non-SQLite databases)") + }
29-42: Avoid serializing database passwords in plain text inDataSourceConfig.
DataSourceConfigis@Serializableand includespasswordas a plainString. If this is persisted, credentials will be stored in clear text (and can also end up in logs or crash dumps), which is a significant security risk.Consider one of:
- Exclude
passwordfrom serialization (e.g.,@kotlinx.serialization.Transient) and keep it in a dedicated secure store / IDE data source.- Store only an opaque credential ID here and resolve the actual secret via a secure provider.
- If you must serialize it, at least encrypt at rest and lock down file permissions, and document the limitation.
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt (4)
103-112: FixdeleteDataSourceso it actually disconnects the selected source.Right now you clear
selectedDataSourceIdbefore checking it, sodisconnect()is never called when deleting the active data source:
- Line 106 sets
selectedDataSourceIdtonullwhenidmatches.- Line 108 then checks
state.selectedDataSourceId == id, which is now alwaysfalse.This can leak an open DB connection.
A minimal fix:
fun deleteDataSource(id: String) { - state = state.copy( - dataSources = state.dataSources.filter { it.id != id }, - selectedDataSourceId = if (state.selectedDataSourceId == id) null else state.selectedDataSourceId - ) - if (state.selectedDataSourceId == id) { - disconnect() - } + val wasSelected = state.selectedDataSourceId == id + state = state.copy( + dataSources = state.dataSources.filter { it.id != id }, + selectedDataSourceId = if (wasSelected) null else state.selectedDataSourceId + ) + if (wasSelected) { + disconnect() + } saveDataSources() }
141-162: Makeconnect()idempotent and manage concurrent connection attempts.
connect()launches a new coroutine each call, even if a connection is already in progress or established. This can:
- Create multiple
DatabaseConnectioninstances if the user rapidly retries; only the last is stored incurrentConnection, leaving earlier ones unclosed.- Re-run
createDatabaseConnectionwhen alreadyConnected, which is unnecessary.- Leave partially created connections unclosed on exceptions.
Suggested improvements:
- Short-circuit if already
ConnectionStatus.ConnectingorConnected.- Track the active connect job and cancel/await it before starting a new one.
- Ensure any newly created
connectionis closed incatch/finallyif you don’t end up assigning it tocurrentConnection.For example:
- fun connect() { + private var connectJob: Job? = null + + fun connect() { val dataSource = state.selectedDataSource ?: return - - scope.launch { + if (state.connectionStatus is ConnectionStatus.Connecting || + state.connectionStatus is ConnectionStatus.Connected + ) return + + connectJob?.cancel() + connectJob = scope.launch { state = state.copy(connectionStatus = ConnectionStatus.Connecting) try { - val connection = createDatabaseConnection(dataSource.toDatabaseConfig()) + val connection = createDatabaseConnection(dataSource.toDatabaseConfig()) if (connection.isConnected()) { currentConnection = connection currentSchema = connection.getSchema()And close
connectionin afinallyblock if you decide not to keep it.Check the documentation for `cc.unitmesh.agent.database.DatabaseConnection` and `createDatabaseConnection` to see whether they manage pooling/lifecycle automatically or require explicit `close()` for each instance.
242-258: Enforce read-only SQL validation before executing LLM-generated queries.
extractAndExecuteSQL()executes whatever is inside the firstsqlblock viacurrentConnection!!.executeQuery(sql)with no validation. Even though the system prompt says “Only generate SELECT queries”, that’s not an enforceable guarantee and violates the “AST-based SQL validation (prevent destructive statements)” requirement.At minimum, add a guard to ensure the query is read-only:
if (match != null && currentConnection != null) { val sql = match.groupValues[1].trim() - try { - val result = currentConnection!!.executeQuery(sql) + // Basic read-only enforcement; replace with a proper SQL parser/AST in production + val normalized = sql.trim().uppercase() + if (!normalized.startsWith("SELECT") || + Regex("\\b(INSERT|UPDATE|DELETE|DROP|TRUNCATE|ALTER|CREATE)\\b").containsMatchIn(normalized) + ) { + renderer.renderError("Security: only SELECT queries are allowed.") + return + } + + try { + val result = currentConnection!!.executeQuery(sql) // Display query result as a new message renderer.renderLLMResponseStart() renderer.renderLLMResponseChunk("\n\n**Query Result:**\n```\n${result.toTableString()}\n```")For production, a real SQL parser/AST check and server-side sandboxing would be preferable.
Look up best practices for validating LLM-generated SQL (e.g., read-only enforcement, AST-based validation) before execution.
28-46: Use the main dispatcher for Compose state and renderer updates.
scopeusesDispatchers.Default, butstate,isGenerating, andComposeRendererupdates are Compose-facing state. Per Compose best practices, state mutations that trigger recomposition must occur on the main dispatcher to avoid race conditions and platform-specific threading issues. Updates tomutableStateOffrom background dispatchers can cause subtle synchronization problems.Use either:
- A main-based scope with background work isolated:
- private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default) + private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Main)and invoke
withContext(Dispatchers.Default)only for heavy operations (DB queries, LLM calls), or
- Introduce a separate
uiScopeonDispatchers.Mainfor state mutations while keeping a background scope for I/O.
🧹 Nitpick comments (3)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/components/header/IdeaAgentTabsHeader.kt (1)
92-93: CHAT_DB color/icon mapping is good; verify whether it should appear in the IDEA tab stripThe new
CHAT_DBmappings toIdeaAutoDevColors.Cyan.c400andIdeaComposeIcons.Databaselook consistent with the rest of the design.Note that the
agentTypeslist inSegmentedAgentTabs(Line 92) still only includesCODING,CODE_REVIEW,KNOWLEDGE, andREMOTE, soCHAT_DBcannot currently be selected from this header even if it is the active type from config. If IDEA is not meant to expose ChatDB yet, this is fine, but if it should be user-selectable you may want to appendAgentType.CHAT_DBto that list or document the intentional omission.Also applies to: 255-262, 267-274
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.kt (1)
151-152: Consider displaying Connecting and Error states in the banner.The
elsebranch silently ignoresConnectionStatus.ConnectingandConnectionStatus.Error. Users may not receive feedback when a connection attempt is in progress or has failed. Consider adding visual indicators for these states.- else -> { /* Connecting or Error handled elsewhere */ } + is ConnectionStatus.Connecting -> { + Card( + modifier = Modifier + .fillMaxWidth() + .padding(horizontal = 12.dp, vertical = 8.dp), + colors = CardDefaults.cardColors( + containerColor = MaterialTheme.colorScheme.tertiaryContainer.copy(alpha = 0.3f) + ) + ) { + Row( + modifier = Modifier.padding(12.dp), + horizontalArrangement = Arrangement.spacedBy(12.dp), + verticalAlignment = Alignment.CenterVertically + ) { + CircularProgressIndicator(modifier = Modifier.size(20.dp), strokeWidth = 2.dp) + Text("Connecting...", style = MaterialTheme.typography.bodySmall) + } + } + } + is ConnectionStatus.Error -> { + Card( + modifier = Modifier + .fillMaxWidth() + .padding(horizontal = 12.dp, vertical = 8.dp), + colors = CardDefaults.cardColors( + containerColor = MaterialTheme.colorScheme.errorContainer.copy(alpha = 0.3f) + ) + ) { + Row( + modifier = Modifier.padding(12.dp), + horizontalArrangement = Arrangement.spacedBy(12.dp), + verticalAlignment = Alignment.CenterVertically + ) { + Icon( + AutoDevComposeIcons.Error, + contentDescription = null, + modifier = Modifier.size(20.dp), + tint = MaterialTheme.colorScheme.error + ) + Text( + text = connectionStatus.message, + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.error + ) + } + } + }mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt (1)
70-75: Implement data source persistence before shipping the feature.
loadDataSources()andsaveDataSources()are TODOs, so all configured connections are lost on restart. For a DB agent, this severely impacts usability.Hook these into your existing config/storage layer (e.g.,
ConfigManageror workspace-scoped storage) so that:
loadDataSources()restores serializedDataSourceConfiginstances for the current workspace.saveDataSources()persists currentstate.dataSourceswhenever they change.Also applies to: 114-116
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/AgentType.kt(4 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/components/header/IdeaAgentTabsHeader.kt(2 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaComposeIcons.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentChatInterface.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt(2 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigDialog.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/model/DataSourceModels.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuDesktop.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuMobile.kt(2 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/icons/AutoDevComposeIcons.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/state/DesktopUiState.kt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuDesktop.kt
- mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentChatInterface.kt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualpattern for platform-specific code (for example:Platformimplementations)
Use concrete classes (not interfaces) andPromise(notFlow) for @JsExport annotations
Avoid emoji and UTF-8 characters in WASM code
When modifyingCodingAgentRenderer, update ALL implementations:DefaultCodingAgentRenderer,ComposeRenderer,JewelRenderer,ServerSideRenderer, andJsRendererAdapterin Kotlin
When modifyingCodingAgentRenderer, update JVM CLI implementations:CodingCliRendererandConsoleRenderer
UseAutoDevColorsorMaterialTheme.colorSchemefor Compose styling
Files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/components/header/IdeaAgentTabsHeader.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaComposeIcons.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuMobile.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/icons/AutoDevComposeIcons.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigDialog.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/state/DesktopUiState.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/AgentType.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/model/DataSourceModels.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt
**/*.{kt,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never use hardcoded colors - always use design tokens from the design system
Files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/components/header/IdeaAgentTabsHeader.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaComposeIcons.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuMobile.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/icons/AutoDevComposeIcons.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigDialog.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/state/DesktopUiState.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/AgentType.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/model/DataSourceModels.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt
🧠 Learnings (3)
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update ALL implementations: `DefaultCodingAgentRenderer`, `ComposeRenderer`, `JewelRenderer`, `ServerSideRenderer`, and `JsRendererAdapter` in Kotlin
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/components/header/IdeaAgentTabsHeader.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuMobile.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/state/DesktopUiState.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to mpp-vscode/src/**/*.ts : When modifying `CodingAgentRenderer`, update VSCode implementations: `mpp-vscode/src/bridge/mpp-core.ts` and `mpp-vscode/src/providers/chat-view.ts`
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/components/header/IdeaAgentTabsHeader.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update JVM CLI implementations: `CodingCliRenderer` and `ConsoleRenderer`
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/components/header/IdeaAgentTabsHeader.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/state/DesktopUiState.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt
🧬 Code graph analysis (1)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt (3)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.kt (1)
DataSourcePanel(23-82)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.kt (1)
ChatDBChatPane(33-78)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigDialog.kt (1)
DataSourceConfigDialog(23-221)
⏰ 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). (2)
- GitHub Check: Test mpp-core (KMP)
- GitHub Check: Build
🔇 Additional comments (14)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.kt (1)
240-253: CHAT_DB agent type persistence mapping looks correctThe new
AgentType.CHAT_DB -> "ChatDB"branch keeps thewhenexhaustive and aligns with the string keys used elsewhere for config persistence. No issues found here.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuMobile.kt (1)
221-228: Database icon wiring for CHAT_DB is consistent and completeBoth
whenexpressions now mapAgentType.CHAT_DBtoAutoDevComposeIcons.Database, keeping them exhaustive and aligning with the new ChatDB agent type without changing any behavior.Also applies to: 256-263
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaComposeIcons.kt (1)
1741-1794: Database ImageVector integrates cleanly with existing IDEA icon setThe new
DatabaseImageVectorfollows the same pattern (24×24 viewport, single black fill path, lazy initialization) as the other icons inIdeaComposeIcons. It should be safe to use fromIdeaAgentTabsHeaderwithout issues.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/state/DesktopUiState.kt (1)
5-9: Desktop agent-type persistence is consistent and low-riskUsing a small
CoroutineScope(Dispatchers.Default)inDesktopUiStateto persist the agent type viaAutoDevConfigWrapper.saveAgentTypePreferenceis reasonable for this lightweight disk write, and thewhenmapping covers all currentAgentTypevalues includingCHAT_DBwith the same string keys used elsewhere. I do not see functional issues here.Also applies to: 16-17, 47-64
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt (1)
3-11: CHAT_DB routing and workspace fallback are implemented safelyThe new
AgentType.CHAT_DBbranch looks good:
- When
workspaceis non-null, it delegates toChatDBPageand routesonBackback toCODINGwithout side effects in composition.- When
workspaceis null, it renders a simple placeholderBoxand no longer mutates agent type during composition, which avoids the recomposition issue highlighted in previous reviews.Once callers pass a non-null
Workspace(e.g., fromAutoDevApp’scurrentWorkspace), this should provide a clean ChatDB experience.Also applies to: 23-61, 63-86
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/icons/AutoDevComposeIcons.kt (1)
116-119: ChatDB-related icon aliases are correctly wired to Material iconsExposing
DatabaseasIcons.Default.StorageandSchemaasIcons.Default.TableChartinAutoDevComposeIconsgives the ChatDB UI a consistent way to reference these icons without scattering raw Material icon usages. This fits the existing icon pattern and introduces no functional risk.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/AgentType.kt (1)
35-38: LGTM!The
CHAT_DBenum value is properly added with consistent documentation, display name mapping, and string parsing (accepting both "chatdb" and "database" aliases). The implementation follows the established pattern of other agent types.Also applies to: 50-50, 62-62
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.kt (1)
33-78: LGTM!The main
ChatDBChatPanecomposable is well-structured with clear separation of concerns: connection banner, message list with welcome overlay, and input area. The conditional rendering based on connection status and message state is handled appropriately.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigDialog.kt (2)
23-102: LGTM!The dialog structure is well-organized with appropriate conditional rendering based on the database dialect. The dropdown for dialect selection correctly updates the default port, and the edit/add mode is properly distinguished.
199-201: No action required — ID generation is properly handled.The
addDataSourcemethod already generates a unique UUID for blank IDs before adding the data source to state (line 79:if (config.id.isBlank()) Uuid.random().toString()). No duplicate key conflicts will occur in the LazyColumn since each new data source receives a generated UUID before persistence.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt (2)
31-46: LGTM!The ViewModel lifecycle management is well-implemented:
rememberfor persistence across recompositions,LaunchedEffectfor collecting notifications, andDisposableEffectfor cleanup on dispose.
128-168: LGTM!The
SchemaInfoDialogappropriately limits table display to 10 entries with an overflow indicator, preventing UI bloat for large schemas. The null-safety check for schema is handled correctly.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.kt (2)
23-82: LGTM!The
DataSourcePanelcomposable is well-structured with clear separation into header, search field, list, and connection controls. The conditional rendering of connection controls based on selection state is handled appropriately.
145-250: LGTM!The
DataSourceItemcomposable correctly handles selection styling, status indicator colors mapped fromConnectionStatus, URL truncation with ellipsis, and a dropdown menu for edit/delete actions. The use ofMaterialTheme.colorSchemefor all colors follows the coding guidelines.
| fun sendMessage(text: String) { | ||
| if (isGenerating || text.isBlank()) return | ||
|
|
||
| currentExecutionJob = scope.launch { | ||
| isGenerating = true | ||
| renderer.addUserMessage(text) | ||
|
|
||
| try { | ||
| val service = llmService | ||
| if (service == null) { | ||
| renderer.renderError("LLM service not initialized. Please configure your model settings.") | ||
| isGenerating = false | ||
| return@launch | ||
| } | ||
|
|
||
| val schemaContext = currentSchema?.getDescription() ?: "No database connected" | ||
| val systemPrompt = buildSystemPrompt(schemaContext) | ||
|
|
||
| val response = StringBuilder() | ||
| renderer.renderLLMResponseStart() | ||
|
|
||
| service.streamPrompt( | ||
| userPrompt = "$systemPrompt\n\nUser: $text", | ||
| compileDevIns = false | ||
| ).collect { chunk -> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the KoogLLMService definition
fd -t f -e kt | xargs rg "class KoogLLMService|interface KoogLLMService" -A 20Repository: phodal/auto-dev
Length of output: 4495
🏁 Script executed:
# Also search for streamPrompt method signature
rg "fun streamPrompt" -A 10Repository: phodal/auto-dev
Length of output: 2242
🏁 Script executed:
# Read the specific lines in ChatDBViewModel.kt around 223-239
fd -t f "ChatDBViewModel.kt" | xargs cat -n | sed -n '220,245p'Repository: phodal/auto-dev
Length of output: 1058
🏁 Script executed:
# Read the full sendMessage() method context around lines 177-201
fd -t f "ChatDBViewModel.kt" | xargs cat -n | sed -n '170,245p'Repository: phodal/auto-dev
Length of output: 3197
🏁 Script executed:
# Check if there are other usages of streamPrompt() to understand the pattern
rg "streamPrompt\(" -B 2 -A 5 | head -100Repository: phodal/auto-dev
Length of output: 9375
🏁 Script executed:
# Check if historyMessages parameter accepts system messages in Message type
rg "class Message|data class Message" -A 5 -B 1Repository: phodal/auto-dev
Length of output: 8658
🏁 Script executed:
# Look for any Message types with role or system variants
rg "role.*system|system.*role" -t kt | head -20Repository: phodal/auto-dev
Length of output: 85
🏁 Script executed:
# Verify the exact Message class definition
cat -n mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/llm/Message.ktRepository: phodal/auto-dev
Length of output: 2184
Use historyMessages with MessageRole.SYSTEM to separate system instructions from user input.
sendMessage() concatenates the system prompt and user text into a single userPrompt string, which presents a prompt injection risk and resends the full system prompt on every turn:
service.streamPrompt(
userPrompt = "$systemPrompt\n\nUser: $text",
compileDevIns = false
)Instead, prepend a system message to historyMessages before calling streamPrompt():
val systemMessage = Message(MessageRole.SYSTEM, systemPrompt)
service.streamPrompt(
userPrompt = text,
historyMessages = listOf(systemMessage),
compileDevIns = false
)This provides structural separation between system instructions and user input (reducing prompt injection risk) and avoids resending the system prompt repeatedly. The Message type already supports MessageRole.SYSTEM for this purpose.
🤖 Prompt for AI Agents
In
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt
around lines 177-201, the code concatenates the system prompt and user text into
userPrompt (prompt injection risk and redundant resending); instead create a
Message with MessageRole.SYSTEM containing the systemPrompt and call
service.streamPrompt with userPrompt = text and historyMessages =
listOf(systemMessage) (preserve compileDevIns = false), removing the
"$systemPrompt\n\nUser: $text" concatenation so system instructions are sent as
a separate system message.
| private suspend fun extractAndExecuteSQL(response: String) { | ||
| val sqlPattern = Regex("```sql\\n([\\s\\S]*?)```", RegexOption.IGNORE_CASE) | ||
| val match = sqlPattern.find(response) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "ChatDBViewModel" --extension ktRepository: phodal/auto-dev
Length of output: 149
🏁 Script executed:
wc -l mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.ktRepository: phodal/auto-dev
Length of output: 153
🏁 Script executed:
sed -n '230,255p' mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.ktRepository: phodal/auto-dev
Length of output: 1174
🌐 Web query:
common markdown code block formatting LLM output ```sql newline styles spacing variants
💡 Result:
Below are common Markdown code-block styles/variants and small formatting tips for LLM-produced SQL outputs (examples use fenced blocks):
- Basic fenced block (preferred; enables syntax highlighting)
SELECT id, name
FROM users
WHERE active = true;- Use lowercase language tag "sql" for best compatibility. [1][2]
- Blank-line padding (recommended for readability in raw Markdown)
- Put a blank line before and after the fenced block. [1]
- Triple-tilde alternative (equivalent to backticks)
SELECT *
FROM orders;- Works where Markdown parser supports fenced blocks.
-
Inline / one-liner (for very short snippets)
SELECT COUNT(*) FROM users; -
Indented fenced block inside lists or block elements
- Indent the fence by 4 (or 8) spaces when inside a list so it renders correctly. [1]
- Clause-newline / expanded style (common SQL-formatter output — preferred for readability)
SELECT u.id,
u.name,
o.total
FROM users u
JOIN orders o ON o.user_id = u.id
WHERE u.active = TRUE
ORDER BY o.total DESC;- Put major clauses (FROM, JOIN, WHERE, GROUP BY, ORDER BY) each on their own line, and align or indent expressions. (Typical formatter output.) [2]
- Compact / single-statement style (for short queries)
SELECT id, name FROM users WHERE active = TRUE;- Keyword-casing variants (style choice)
- ALL CAPS keywords: SELECT, FROM, WHERE (common in SQL style guides).
- lower_case keywords: select, from, where (works everywhere). Use whichever matches project style; some renderers are case-sensitive for language tag but not SQL itself. [3][4]
- Trailing semicolon and newline
- Include semicolon at end of statement(s) and a final newline; many SQL tools expect it.
- Comments and metadata
-- Explain: Fetch active users with orders in last 30 days
SELECT ...- Use SQL comments to annotate LLM output intent.
- Diff / change annotation (for showing edits)
- WHERE active = FALSE
+ WHERE active = TRUE- Use
difffences when showing changes.
- Tools / syntax-highlighting notes
- GitHub uses Linguist for highlighting; "sql" (and aliases like pgsql, postgres, pgsql) are supported. Prefer lowercase language id for GitHub Pages/Jekyll. [1][5]
- Some platforms may treat uppercase language tags inconsistently (see some GitLab reports). Use lowercase to be safe. [4]
Quick practical checklist for LLM SQL output:
- Use ```sql fenced block with lower-case "sql". [1]
- Put blank line before/after fences for raw readability. [1]
- Use multi-line clause-per-line formatting for readability (JOIN/WHERE/GROUP BY on new lines). [2]
- Include semicolon and final newline.
- Add short SQL comments to explain intent.
- If inside lists, indent fences so Markdown renders correctly. [1]
Sources: GitHub fenced-code recommendations and language support; general Markdown code-block best-practice guides. [1][2][5][4]
References
[1] GitHub Docs — Creating and highlighting code blocks.
[2] Complete Guide to Markdown Code Blocks — best practices.
[3] GitHub supported languages (Linguist) list.
[4] GitLab issue re: case-sensitivity of LANGUAGE tag.
🏁 Script executed:
rg "sqlPattern|```sql" mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ -A 2 -B 2Repository: phodal/auto-dev
Length of output: 127
🏁 Script executed:
rg "sqlPattern" mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ -A 2 -B 2Repository: phodal/auto-dev
Length of output: 861
🏁 Script executed:
rg "extractAndExecuteSQL" mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ -B 2 -A 5Repository: phodal/auto-dev
Length of output: 2135
🏁 Script executed:
fd -t f "test" --extension kt | rg "ChatDB" -iRepository: phodal/auto-dev
Length of output: 41
🏁 Script executed:
rg "Regex.*\`\`\`" --type kt | head -20Repository: phodal/auto-dev
Length of output: 85
🏁 Script executed:
rg 'Regex.*```' | head -20Repository: phodal/auto-dev
Length of output: 2986
🏁 Script executed:
sed -n '240,250p' mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.ktRepository: phodal/auto-dev
Length of output: 513
Align SQL code block regex with established codebase patterns for robustness.
The current regex requires exactly \n after the language tag, which is inconsistent with similar code extraction patterns elsewhere in the codebase (PlanParser, PlotDSL extraction, ToolCallParser) that use \s*\n to handle optional whitespace. Additionally, LLM outputs may contain \r\n (Windows line endings) or extra spaces, causing valid SQL blocks to be missed.
Change to:
- val sqlPattern = Regex("```sql\\n([\\s\\S]*?)```", RegexOption.IGNORE_CASE)
+ val sqlPattern = Regex("```sql\\s+([\\s\\S]*?)```", RegexOption.IGNORE_CASE)This handles \n, \r\n, spaces, tabs, and their combinations while remaining aligned with similar extraction patterns in the codebase.
🤖 Prompt for AI Agents
In
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt
around lines 242-245, the SQL code-block regex currently requires an exact "\n"
after the "```sql" tag which misses blocks with extra whitespace or CRLFs;
update the pattern to accept one-or-more whitespace characters after the tag
(e.g., use "\s+" instead of "\n") so it matches "```sql" followed by any
combination of spaces/tabs/newlines (including "\r\n"), keeping
RegexOption.IGNORE_CASE; replace the existing Regex(...) instantiation
accordingly so it aligns with the project's other parsers.
| fun stopGeneration() { | ||
| currentExecutionJob?.cancel() | ||
| isGenerating = false | ||
| } | ||
|
|
||
| fun getSchema(): DatabaseSchema? = currentSchema | ||
|
|
||
| fun dispose() { | ||
| stopGeneration() | ||
| disconnect() | ||
| scope.cancel() | ||
| } |
There was a problem hiding this comment.
Ensure dispose() reliably closes DB connections before cancelling the scope.
dispose() calls disconnect() and then immediately scope.cancel():
fun dispose() {
stopGeneration()
disconnect()
scope.cancel()
}But disconnect() launches an async coroutine on scope; cancelling the scope right after can prevent that coroutine from running and leave currentConnection open.
Safer patterns:
- Either inline a synchronous close in
dispose():
fun dispose() {
stopGeneration()
- disconnect()
- scope.cancel()
+ try {
+ currentConnection?.close()
+ } catch (e: Exception) {
+ println("[ChatDB] Error closing connection: ${e.message}")
+ } finally {
+ currentConnection = null
+ currentSchema = null
+ }
+ scope.cancel()
}- Or make
disconnecta suspending function and ensure it completes before cancelling the scope.
🤖 Prompt for AI Agents
In
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt
around lines 260–271, dispose() cancels the scope immediately after calling
disconnect(), which launches an async coroutine on that scope and can prevent
the DB close from running; change dispose() so the DB connection is closed
reliably before cancelling the scope: either perform the close synchronously in
dispose() by directly checking and closing currentConnection (and clearing
currentConnection/currentSchema) before calling scope.cancel(), or make
disconnect() a suspending function and call it from dispose() in a
blocking/waiting manner (or otherwise await its completion) so the close
completes prior to scope.cancel(); ensure any exceptions during close are
handled/logged and currentConnection is nulled.
Refactor agent type string conversion to use getDisplayName() and make ChatDB workspace parameter optional. Remove SessionApp and streamline ChatDB routing logic.
Show SessionSidebar only for agent types that require it, hiding it on pages with their own navigation such as CHAT_DB, KNOWLEDGE, and CODE_REVIEW.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt (2)
26-31:llmServiceparameter is unused; ViewModel creates its own instance.The
llmServiceparameter is accepted but never passed to theChatDBViewModel. The ViewModel initializes its own LLM service viainitializeLLMService(). Either thread the provided service through to avoid duplicate initialization or remove the unused parameter.- val viewModel = remember { ChatDBViewModel(workspace) } + val viewModel = remember { ChatDBViewModel(workspace, llmService) }
59-69: HoistshowSchemaDialogstate outside the conditional block.The
showSchemaDialogstate is declared inside theif (state.connectionStatus is ConnectionStatus.Connected)block. When the connection status toggles, the state will be recreated, potentially causing unexpected dialog dismissals.Move the state declaration to the
ChatDBPagelevel:val viewModel = remember { ChatDBViewModel(workspace) } val state = viewModel.state + var showSchemaDialog by remember { mutableStateOf(false) } // ... later in actions block: if (state.connectionStatus is ConnectionStatus.Connected) { - var showSchemaDialog by remember { mutableStateOf(false) } IconButton(onClick = { showSchemaDialog = true }) {And move the dialog outside the
TopAppBaractions block.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt (7)
70-74: Data source persistence is not implemented.The
loadDataSources()andsaveDataSources()methods are placeholders (TODO comments). Users will lose all configured database connections when the application restarts. Consider implementing persistence usingConfigManageror similar storage before releasing this feature.Would you like me to help implement data source persistence, or should I open an issue to track this task?
Also applies to: 114-116
28-28: Compose state updates should occur on the main dispatcher.The
scopeusesDispatchers.Default, but the ViewModel modifies ComposemutableStateOfproperties (state,isGenerating) from coroutines launched on this scope. Compose state should be updated from the main thread.- private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default) + private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Main)Alternatively, wrap state updates with
withContext(Dispatchers.Main).
103-112: Logic bug:disconnect()is never called when deleting the selected data source.The check on line 108 uses
state.selectedDataSourceIdafter it was already set tonullon line 106. The condition will always befalsewhen deleting the selected data source, leaving the connection open.fun deleteDataSource(id: String) { + val wasSelected = state.selectedDataSourceId == id state = state.copy( dataSources = state.dataSources.filter { it.id != id }, selectedDataSourceId = if (state.selectedDataSourceId == id) null else state.selectedDataSourceId ) - if (state.selectedDataSourceId == id) { + if (wasSelected) { disconnect() } saveDataSources() }
141-162: Race condition with rapidconnect()calls can leak connections.If
connect()is called multiple times rapidly (e.g., user double-clicks), multiple connections could be created concurrently. Only the last connection is stored incurrentConnection, leaving others orphaned and unclosed.Track and cancel in-flight connection attempts:
+ private var connectionJob: Job? = null + fun connect() { val dataSource = state.selectedDataSource ?: return - scope.launch { + connectionJob?.cancel() + connectionJob = scope.launch { state = state.copy(connectionStatus = ConnectionStatus.Connecting) // ...
242-257: Critical security risk: No SQL validation before execution.The system prompt requests "only SELECT queries," but there's no actual validation. Malicious prompts could trick the LLM into generating destructive SQL (DROP, DELETE, UPDATE). The PR objectives explicitly require "AST-based SQL validation (prevent destructive statements)."
Add SQL validation before execution:
private suspend fun extractAndExecuteSQL(response: String) { - val sqlPattern = Regex("```sql\\n([\\s\\S]*?)```", RegexOption.IGNORE_CASE) + val sqlPattern = Regex("```sql\\s+([\\s\\S]*?)```", RegexOption.IGNORE_CASE) val match = sqlPattern.find(response) if (match != null && currentConnection != null) { val sql = match.groupValues[1].trim() + + // Validate SQL is read-only (SELECT only) + val normalizedSql = sql.trim().uppercase() + if (!normalizedSql.startsWith("SELECT") || + normalizedSql.contains(Regex("\\b(INSERT|UPDATE|DELETE|DROP|TRUNCATE|ALTER|CREATE|GRANT|REVOKE)\\b"))) { + renderer.renderError("Security: Only SELECT queries are allowed") + return + } + try { val result = currentConnection!!.executeQuery(sql)Note: Also updated regex from
\\nto\\s+to handle various whitespace patterns.
267-271:dispose()may not reliably close DB connections.
disconnect()launches an async coroutine onscope, butscope.cancel()is called immediately after, potentially preventing the disconnect coroutine from completing and leaving the connection open.Close the connection synchronously in
dispose():fun dispose() { stopGeneration() - disconnect() + try { + currentConnection?.close() + } catch (e: Exception) { + println("[ChatDB] Error closing connection: ${e.message}") + } finally { + currentConnection = null + currentSchema = null + } scope.cancel() }
198-201: UsehistoryMessageswithMessageRole.SYSTEMinstead of concatenation.Concatenating the system prompt with user input (
"$systemPrompt\n\nUser: $text") presents prompt injection risk and resends the full system prompt on every turn. Use theMessagetype withMessageRole.SYSTEMfor proper separation.+import cc.unitmesh.devins.llm.Message +import cc.unitmesh.devins.llm.MessageRole // In sendMessage(): +val systemMessage = Message(MessageRole.SYSTEM, systemPrompt) service.streamPrompt( - userPrompt = "$systemPrompt\n\nUser: $text", + userPrompt = text, + historyMessages = listOf(systemMessage), compileDevIns = false )
🧹 Nitpick comments (1)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt (1)
3-3: Unused import detected.The
fillMaxSizeimport is added but not used in this file. Consider removing it to keep imports clean.-import androidx.compose.foundation.layout.fillMaxSize
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/app/SessionApp.kt(0 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt(3 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/state/DesktopUiState.kt(2 hunks)
💤 Files with no reviewable changes (1)
- mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/app/SessionApp.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.kt
- mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/state/DesktopUiState.kt
- mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.kt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualpattern for platform-specific code (for example:Platformimplementations)
Use concrete classes (not interfaces) andPromise(notFlow) for @JsExport annotations
Avoid emoji and UTF-8 characters in WASM code
When modifyingCodingAgentRenderer, update ALL implementations:DefaultCodingAgentRenderer,ComposeRenderer,JewelRenderer,ServerSideRenderer, andJsRendererAdapterin Kotlin
When modifyingCodingAgentRenderer, update JVM CLI implementations:CodingCliRendererandConsoleRenderer
UseAutoDevColorsorMaterialTheme.colorSchemefor Compose styling
Files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt
**/*.{kt,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never use hardcoded colors - always use design tokens from the design system
Files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt
🧠 Learnings (3)
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to mpp-vscode/src/**/*.ts : When modifying `CodingAgentRenderer`, update VSCode implementations: `mpp-vscode/src/bridge/mpp-core.ts` and `mpp-vscode/src/providers/chat-view.ts`
Applied to files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update ALL implementations: `DefaultCodingAgentRenderer`, `ComposeRenderer`, `JewelRenderer`, `ServerSideRenderer`, and `JsRendererAdapter` in Kotlin
Applied to files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update JVM CLI implementations: `CodingCliRenderer` and `ConsoleRenderer`
Applied to files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt
🧬 Code graph analysis (1)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt (2)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt (1)
ChatDBPage(22-126)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.kt (1)
onAgentTypeChange(240-253)
⏰ 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). (2)
- GitHub Check: Test mpp-core (KMP)
- GitHub Check: Build
🔇 Additional comments (2)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt (1)
63-73: LGTM! Clean routing implementation for CHAT_DB.The new
CHAT_DBbranch properly routes toChatDBPage, passing the workspace (which is nullable by design), the LLM service, and forwarding the notification callback. TheonBackcallback correctly navigates back to theCODINGagent.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt (1)
128-168: LGTM! Well-structured schema dialog implementation.The
SchemaInfoDialogcomposable properly usesMaterialTheme.colorSchemefor the overflow text color (line 154), following the coding guidelines. The dialog appropriately limits display to 10 tables with an overflow indicator.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.kt (1)
448-454: Consider adding a property toAgentTypefor session sidebar visibility.The hardcoded list approach works correctly, but it could become a maintenance burden as more agent types are added. Each new type would require updating this conditional logic.
Consider adding a property to the
AgentTypeenum itself:enum class AgentType { CODING(requiresSessionSidebar = true), LOCAL_CHAT(requiresSessionSidebar = true), REMOTE(requiresSessionSidebar = true), CHAT_DB(requiresSessionSidebar = false), KNOWLEDGE(requiresSessionSidebar = false), CODE_REVIEW(requiresSessionSidebar = false); // ... }Then simplify the logic:
-val shouldShowSessionSidebar = selectedAgentType in listOf( - AgentType.CODING, - AgentType.LOCAL_CHAT, - AgentType.REMOTE -) +val shouldShowSessionSidebar = selectedAgentType.requiresSessionSidebar
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.kt(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualpattern for platform-specific code (for example:Platformimplementations)
Use concrete classes (not interfaces) andPromise(notFlow) for @JsExport annotations
Avoid emoji and UTF-8 characters in WASM code
When modifyingCodingAgentRenderer, update ALL implementations:DefaultCodingAgentRenderer,ComposeRenderer,JewelRenderer,ServerSideRenderer, andJsRendererAdapterin Kotlin
When modifyingCodingAgentRenderer, update JVM CLI implementations:CodingCliRendererandConsoleRenderer
UseAutoDevColorsorMaterialTheme.colorSchemefor Compose styling
Files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.kt
**/*.{kt,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never use hardcoded colors - always use design tokens from the design system
Files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.kt
🧠 Learnings (3)
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update ALL implementations: `DefaultCodingAgentRenderer`, `ComposeRenderer`, `JewelRenderer`, `ServerSideRenderer`, and `JsRendererAdapter` in Kotlin
Applied to files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update JVM CLI implementations: `CodingCliRenderer` and `ConsoleRenderer`
Applied to files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to mpp-vscode/src/**/*.ts : When modifying `CodingAgentRenderer`, update VSCode implementations: `mpp-vscode/src/bridge/mpp-core.ts` and `mpp-vscode/src/providers/chat-view.ts`
Applied to files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.kt
🧬 Code graph analysis (1)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.kt (1)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/SessionSidebar.kt (1)
SessionSidebar(39-106)
⏰ 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). (2)
- GitHub Check: Test mpp-core (KMP)
- GitHub Check: Build
🔇 Additional comments (2)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.kt (2)
461-488: LGTM! Session sidebar conditional rendering implemented correctly.The conditional rendering logic properly hides the
SessionSidebarfor agent types that have their own navigation (CHAT_DB, KNOWLEDGE, CODE_REVIEW). The session selection and new chat handlers are correctly preserved with appropriate fallback behavior.
175-176: No changes needed. The refactoring at lines 175-176 correctly usestype.getDisplayName()for config persistence, and the returned strings are compatible withAutoDevConfigWrapper.saveAgentTypePreference(). The format matches the hardcoded"Remote"string at line 736, confirming consistency across the codebase.Likely an incorrect or invalid review comment.
Introduce ChatDB agent models, executor, schema linker, and CLI entry point. Update build configuration for new modules.
- Add SqlReviseAgent as a standalone SubAgent for SQL self-correction - Add JSqlParserValidator for JVM-specific SQL validation using JSqlParser - Add comprehensive tests for SqlReviseAgent, JSqlParserValidator, and SchemaLinker - Update ChatDBAgentExecutor to use the new SqlReviseAgent - Fix CodeFence property reference (languageId instead of language) Implements the Revise Agent (自我修正闭环) feature from issue #508
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt (1)
239-249: Consider extracting shared SQL extraction logic.This
extractSqlFromResponsemethod duplicates similar logic found inChatDBAgentExecutor.kt(lines 197-214). Consider extracting this into a shared utility function to reduce duplication and ensure consistent behavior.Both implementations use
CodeFenceparsing followed by regex fallbacks. A shared utility could be placed in a common location likecc.unitmesh.devins.parseralongsideCodeFence.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/SchemaLinker.kt (1)
79-85: Keyword extraction may miss compound identifiers.The regex
[^a-z0-9\\s_]preserves underscores but the subsequent split on\\s+won't handle snake_case identifiers likeorder_items. Consider splitting on underscores too to extract individual words (e.g., "order" and "items") for better matching.fun extractKeywords(query: String): List<String> { return query.lowercase() .replace(Regex("[^a-z0-9\\s_]"), " ") - .split(Regex("\\s+")) + .split(Regex("[\\s_]+")) .filter { it.length > 2 && it !in stopWords } .distinct() }mpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/ChatDBCli.kt (2)
112-114: Hardcoded default temperature and maxTokens.Defaults of
0.7and4096are reasonable, but consider making these configurable via system properties for CLI flexibility, similar to other parameters.
174-175: Metadata access assumes string values for numeric fields.
result.metadata["revisionAttempts"]returns a String, and you're using?: "0"for display. This works but relies on the executor populating metadata correctly. TheChatDBResultalready has arevisionAttempts: Intfield—consider using that directly instead of metadata.- println("🔄 Revision attempts: ${result.metadata["revisionAttempts"] ?: "0"}") - println("📈 Has visualization: ${result.metadata["hasVisualization"] ?: "false"}") + println("🔄 Revision attempts: ${result.revisionAttempts}") + println("📈 Has visualization: ${result.plotDslCode != null}")mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidator.kt (1)
92-108: String-based warning detection may produce false positives.Using
statement.toString()andcontains("SELECT *")could match occurrences in string literals or comments. Consider using JSqlParser's AST visitor pattern to accurately detectSELECT *and missing WHERE clauses.For more accurate detection, you could check the parsed statement type:
private fun collectWarnings(statement: Statement): List<String> { val warnings = mutableListOf<String>() when (statement) { is net.sf.jsqlparser.statement.select.Select -> { val selectBody = statement.selectBody if (selectBody is net.sf.jsqlparser.statement.select.PlainSelect) { if (selectBody.selectItems.any { it is net.sf.jsqlparser.statement.select.AllColumns }) { warnings.add("Consider specifying explicit columns instead of SELECT *") } } } is net.sf.jsqlparser.statement.update.Update -> { if (statement.where == null) { warnings.add("UPDATE without WHERE clause will affect all rows") } } is net.sf.jsqlparser.statement.delete.Delete -> { if (statement.where == null) { warnings.add("DELETE without WHERE clause will affect all rows") } } } return warnings }mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt (1)
211-214: Last-resort SELECT extraction may capture incomplete SQL.The regex
(SELECT[\\s\\S]*?;)requires a semicolon terminator. If the LLM omits the trailing semicolon, valid SQL won't be extracted. Consider making the semicolon optional.- val selectPattern = Regex("(SELECT[\\s\\S]*?;)", RegexOption.IGNORE_CASE) + val selectPattern = Regex("(SELECT[\\s\\S]*?)(;|$)", RegexOption.IGNORE_CASE) val selectMatch = selectPattern.find(response) - return selectMatch?.groupValues?.get(1)?.trim() + return selectMatch?.groupValues?.get(1)?.trim()?.removeSuffix(";")?.plus(";")?.takeIf { it.length > 7 }Or simpler:
- val selectPattern = Regex("(SELECT[\\s\\S]*?;)", RegexOption.IGNORE_CASE) + val selectPattern = Regex("(SELECT[\\s\\S]*?;?)", RegexOption.IGNORE_CASE)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBModels.kt (1)
74-86: Potential redundancy betweenrevisionAttemptsfield andmetadatamap.
ChatDBResulthas a dedicatedrevisionAttempts: Intfield but also a genericmetadata: Map<String, String>. The CLI accessesmetadata["revisionAttempts"]instead of the direct field. Consider removing the duplication or documenting the intended usage of each.Either remove
metadataif all structured fields are explicit, or document thatmetadatais for future extensibility and consumers should prefer typed fields.mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgent.kt (5)
40-60: AlignmaxIterationswithRunConfig.maxTurnsto avoid config drift
maxIterationsis a constructor parameter/override and is passed intoChatDBAgentExecutor, whileRunConfigis hard-coded withmaxTurns = 10. If a caller provides a differentmaxIterations,RunConfigwill still cap turns at 10, which can be confusing.Consider deriving
RunConfig.maxTurnsfrommaxIterationsso they stay in sync, for example:- modelConfig = ModelConfig.default(), - runConfig = RunConfig(maxTurns = 10, maxTimeMinutes = 5) + modelConfig = ModelConfig.default(), + runConfig = RunConfig(maxTurns = maxIterations, maxTimeMinutes = 5)
82-88: Clarify and enforce lifecycle betweendatabaseConnection,executor, andclose()The lazy
executorcaptures aDatabaseConnectiononce:private val executor: ChatDBAgentExecutor by lazy { val connection = databaseConnection ?: createDatabaseConnection(databaseConfig) databaseConnection = connection ChatDBAgentExecutor(..., databaseConnection = connection, ...) }Later,
close()setsdatabaseConnectiontonullbut does not affect theexecutor, which still holds the original connection reference. Reusing the sameChatDBAgentafterclose()may then route calls through a closed connection with no explicit error at the agent level.It would be clearer to either:
- Treat
ChatDBAgentas single-use and throw ifexecuteis called afterclose(), or- Allow reuse by reinitializing the connection / executor after
close().For example, the simplest explicit option is to track a closed flag and guard
execute:- private var databaseConnection: DatabaseConnection? = null + private var databaseConnection: DatabaseConnection? = null + private var closed: Boolean = false @@ - suspend fun close() { - databaseConnection?.close() - databaseConnection = null - } + suspend fun close() { + closed = true + databaseConnection?.close() + databaseConnection = null + } @@ override suspend fun execute( input: ChatDBTask, onProgress: (String) -> Unit ): ToolResult.AgentResult { + check(!closed) { "ChatDBAgent has been closed and cannot be reused" } logger.info { "Starting ChatDB Agent for query: ${input.query}" }Also applies to: 145-148
99-108: MakevalidateInputmore defensive for stringly-typed inputs and bounds
validateInputassumesmaxRowsis aNumberandgenerateVisualizationaBoolean:maxRows = (input["maxRows"] as? Number)?.toInt() ?: 100, generateVisualization = input["generateVisualization"] as? Boolean ?: trueIf the map comes from JSON/CLI/etc., these may arrive as strings (e.g.
"100","false"), in which case the casts fail and the defaults are silently used. You might also want to clampmaxRowsto a reasonable positive range.Consider parsing strings and enforcing bounds, e.g.:
val maxRowsValue = input["maxRows"] val maxRows = when (maxRowsValue) { is Number -> maxRowsValue.toInt() is String -> maxRowsValue.toIntOrNull() else -> null }?.coerceIn(1, 10_000) ?: 100 val generateVisualization = when (val gv = input["generateVisualization"]) { is Boolean -> gv is String -> gv.toBooleanStrictOrNull() ?: true else -> true }This makes the agent more robust against loosely-typed callers while retaining sensible defaults.
120-128: Preserve native types inmetadataif the API allows itYou currently stringify all metadata values:
metadata = mapOf( "generatedSql" to (result.generatedSql ?: ""), "rowCount" to (result.queryResult?.rowCount?.toString() ?: "0"), "revisionAttempts" to result.revisionAttempts.toString(), "hasVisualization" to (result.plotDslCode != null).toString() )If
ToolResult.AgentResult.metadatasupports non-string values (e.g.Map<String, Any?>), it could be more convenient for consumers to keeprowCountasInt?,revisionAttemptsasInt, andhasVisualizationasBooleanrather than"0"/"true"/"false".If the type is already
Map<String, String>, then this is fine as-is and this comment can be ignored.
150-171: Prompt content looks solid; consider explicitly mentioning validation/sandboxing behaviorThe
SYSTEM_PROMPTclearly constrains the agent to read-onlySELECTqueries and adds good guidance on SQL formatting and PlotDSL blocks. Given you also have an AST-based validator and revise loop elsewhere in the system, you might optionally:
- Explicitly mention that destructive statements (DDL/DML like
INSERT,UPDATE,DELETE,DROP, etc.) will be rejected or revised.- Hint that the system may automatically adjust or reject unsafe SQL even if the user asks for it.
This can help align the model’s expectations with the actual safety mechanisms in the executor and validator.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBModels.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/SchemaLinker.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt(1 hunks)mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/chatdb/SchemaLinkerTest.kt(1 hunks)mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgentTest.kt(1 hunks)mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgent.kt(1 hunks)mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt(1 hunks)mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidator.kt(1 hunks)mpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidatorTest.kt(1 hunks)mpp-ui/build.gradle.kts(1 hunks)mpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/ChatDBCli.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualpattern for platform-specific code (for example:Platformimplementations)
Use concrete classes (not interfaces) andPromise(notFlow) for @JsExport annotations
Avoid emoji and UTF-8 characters in WASM code
When modifyingCodingAgentRenderer, update ALL implementations:DefaultCodingAgentRenderer,ComposeRenderer,JewelRenderer,ServerSideRenderer, andJsRendererAdapterin Kotlin
When modifyingCodingAgentRenderer, update JVM CLI implementations:CodingCliRendererandConsoleRenderer
UseAutoDevColorsorMaterialTheme.colorSchemefor Compose styling
Files:
mpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/ChatDBCli.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBModels.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/SchemaLinker.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidator.ktmpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidatorTest.ktmpp-core/src/commonTest/kotlin/cc/unitmesh/agent/chatdb/SchemaLinkerTest.ktmpp-core/src/commonTest/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgentTest.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgent.kt
**/*.{kt,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never use hardcoded colors - always use design tokens from the design system
Files:
mpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/ChatDBCli.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBModels.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/SchemaLinker.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidator.ktmpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidatorTest.ktmpp-core/src/commonTest/kotlin/cc/unitmesh/agent/chatdb/SchemaLinkerTest.ktmpp-core/src/commonTest/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgentTest.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgent.kt
🧠 Learnings (2)
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update JVM CLI implementations: `CodingCliRenderer` and `ConsoleRenderer`
Applied to files:
mpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/ChatDBCli.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Run `./gradlew :mpp-ui:generateI18n4kFiles` command when updating i18n resources
Applied to files:
mpp-ui/build.gradle.kts
🧬 Code graph analysis (5)
mpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidatorTest.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/test/DevInsTestCase.kt (4)
assertTrue(29-33)assertFalse(38-40)assertNotNull(54-59)assertNull(64-68)
mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/chatdb/SchemaLinkerTest.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/test/DevInsTestCase.kt (3)
assertTrue(29-33)assertNotNull(54-59)assertEquals(45-49)
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt (3)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/BaseAgentExecutor.kt (1)
getLLMResponse(49-85)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt (1)
extractSqlFromResponse(239-249)mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/test/AstFormatter.kt (1)
appendLine(206-209)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt (2)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/schema/ToolSchema.kt (2)
string(136-155)integer(157-170)mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt (1)
extractSqlFromResponse(198-215)
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgent.kt (1)
mpp-vscode/src/bridge/mpp-core.ts (1)
ToolRegistry(226-306)
⏰ 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). (2)
- GitHub Check: Test mpp-core (KMP)
- GitHub Check: Build
🔇 Additional comments (30)
mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgentTest.kt (4)
1-11: LGTM! Well-organized test structure.The test class is cleanly organized with clear section headers and comprehensive coverage of the
SqlRevisionInput,SqlReviseAgentSchema, andSqlValidationResultdata classes.
15-42: Good coverage of input validation and default values.Tests properly verify both explicit parameter passing and default value behavior for
maxAttempts.
46-75: Schema tests validate expected contract.The tests appropriately check that
getExampleUsagereturns expected format strings andtoJsonSchema()includes all required properties. The description check is appropriately flexible.
79-159: Comprehensive validation result and edge case coverage.Good coverage of success/failure states, warnings handling, empty schema edge case, and complex SQL input. The tests validate the data structures behave correctly across different scenarios.
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt (5)
20-48: LGTM! Well-defined tool schema.The
SqlReviseAgentSchemaproperly defines all required and optional properties using theDeclarativeToolSchemapattern with appropriate property types.
62-82: LGTM! Clean input validation with appropriate defaults.The
validateInputmethod properly extracts and validates required fields while providing sensible defaults for optional ones.
86-165: LGTM! Solid self-correction loop implementation.The execute method implements a clean revision loop with:
- Proper attempt tracking and max attempts guard
- Clear progress reporting
- Appropriate handling of both validator-present and validator-absent cases
- Meaningful metadata in results
268-276: LGTM! Clean, serializable data class.The
SqlRevisionInputdata class is properly annotated with@Serializableand provides sensible defaults.
281-293: LGTM! Simple validator interface and result types.The
SqlValidatorInterfaceprovides a clean contract for platform-specific implementations, andSqlValidationResultis properly structured with serialization support.mpp-ui/build.gradle.kts (1)
627-666: LGTM! Follows established CLI task patterns.The new
runChatDBClitask correctly follows the same structure as other CLI tasks in this file (runDocumentCli,runCodingCli, etc.), properly configuring classpath, main class, and system properties for database connection parameters.mpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidatorTest.kt (5)
1-11: LGTM! Well-structured JVM-specific test class.Good setup with a single validator instance shared across tests. The test organization with clear section headers makes the coverage easy to understand.
15-60: Good basic validation coverage.Tests properly verify valid SELECT queries, JOIN syntax, syntax errors (typo detection), and empty/blank SQL handling.
64-102: Excellent warning detection tests.Good coverage of security-sensitive patterns:
SELECT *usage warningsUPDATEwithoutWHEREclauseDELETEwithoutWHEREclause- Verification that safe statements don't trigger false warnings
106-141: Good complex query and DML coverage.Tests appropriately verify that complex SELECT queries (subqueries, GROUP BY, HAVING, ORDER BY) and INSERT statements are handled correctly.
145-184: Appropriate edge case handling.The multi-statement test correctly focuses on verifying no crash rather than asserting specific behavior (since JSqlParser behavior may vary). SQL comments handling is also tested.
mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/chatdb/SchemaLinkerTest.kt (5)
1-16: LGTM! Clean test setup.Good use of a shared
SchemaLinkerinstance and kotlin.test assertions.
20-54: Good keyword extraction coverage.Tests properly verify:
- Basic keyword extraction
- Stop word filtering
- Short word filtering (length <= 2)
- Lowercase normalization
58-90: Well-designed test fixture.The
createTestSchema()helper creates a realistic multi-table schema with appropriate columns, making tests readable and maintainable.
92-149: Comprehensive schema linking tests.Good coverage of:
- Direct table name matching
- Column name matching
- Fallback behavior when no matches found
- Partial matching (e.g., "users" matching "user_accounts")
153-203: Solid edge case coverage.Tests properly handle:
- Empty query returning empty keywords
- Empty schema returning empty results
- Special characters in queries
- Numeric content in queries
The
SchemaLinkingResulttest at the end validates the data class structure.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/SchemaLinker.kt (3)
146-151: Integer division yields zero threshold for short strings.When both strings are ≤2 characters,
min(s1.length, s2.length) / 3evaluates to 0, making fuzzy match succeed only on exact equality. This might be intentional, but verify it aligns with expected behavior for short identifiers like "id" or "pk".
156-167: Levenshtein implementation is correct and straightforward.The standard DP approach is appropriate here. For very large schema names this allocates O(n*m) memory, but that's unlikely to be an issue for typical table/column names.
37-74: Link method logic is sound with a sensible fallback.The fallback to include all tables when no matches are found (lines 62-64) ensures the LLM always has schema context. The confidence calculation correctly normalizes the average score.
mpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/ChatDBCli.kt (1)
40-48: Password handling looks safe but consider masking in debug scenarios.The password is read from system properties/args but not printed to stdout. However, if debug logging is added later, ensure the password isn't accidentally logged. Consider documenting that passwords can also be read from environment variables for better security practices.
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidator.kt (2)
17-32: Validation correctly returns structured results.The try-catch structure properly separates success/failure paths, and the
SqlValidationResultprovides clean error messages. Good separation of concerns.
63-87: Error message extraction handles common JSqlParser patterns well.The regex patterns for "Encountered" and "Was expecting" cover typical parse errors. The
.take(200)fallback prevents excessively long error messages.mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt (2)
48-152: Execute method is well-structured with clear step progression.The 8-step workflow (schema fetch → linking → context build → SQL generation → extraction → validation/revision → execution → visualization) is logical and well-documented with progress callbacks. Error handling accumulates issues without early termination, allowing partial results.
144-151: Success condition may be too strict.
success = errors.isEmpty() && queryResult != nullmeans any warning or non-fatal issue added toerrorswill mark the result as failed. Consider distinguishing between fatal errors and warnings, or only checking for query execution success.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBModels.kt (2)
12-40: ChatDBTask model is well-designed with sensible defaults.The optional
schemafield allows pre-fetched schemas while defaulting to runtime fetch. Documentation is clear and helpful.
45-86: ChatDBResult provides comprehensive execution information.The model captures all relevant execution details including SQL, results, visualization, errors, and metadata. The
contentproperty accessor pattern would be useful ifChatDBResultimplemented a common result interface.
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBModels.kt
Outdated
Show resolved
Hide resolved
| var validatedSql = generatedSql | ||
| val validation = jsqlValidator.validate(validatedSql!!) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Redundant non-null assertion after assignment.
validatedSql is assigned from generatedSql which was just null-checked at line 86-89. The !! is unnecessary and could mask logic errors if future refactoring changes the flow.
- var validatedSql = generatedSql
- val validation = jsqlValidator.validate(validatedSql!!)
+ var validatedSql = generatedSql!! // Safe: null case handled at line 86-89
+ val validation = jsqlValidator.validate(validatedSql)📝 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.
| var validatedSql = generatedSql | |
| val validation = jsqlValidator.validate(validatedSql!!) | |
| var validatedSql = generatedSql!! // Safe: null case handled at line 86-89 | |
| val validation = jsqlValidator.validate(validatedSql) |
🤖 Prompt for AI Agents
In mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt
around lines 92-93, remove the redundant non-null assertion on validatedSql when
calling jsqlValidator.validate — call validate with validatedSql directly (or
make validatedSql explicitly non-nullable at assignment if that better reflects
the prior null-check) so you don't use !! at the call site.
| private fun extractSqlFromResponse(response: String): String? { | ||
| val codeFence = CodeFence.parse(response) | ||
| if (codeFence.languageId.lowercase() == "sql" && codeFence.text.isNotBlank()) { | ||
| return codeFence.text.trim() | ||
| } | ||
|
|
||
| // Try to find SQL block manually | ||
| val sqlPattern = Regex("```sql\\s*([\\s\\S]*?)```", RegexOption.IGNORE_CASE) | ||
| val match = sqlPattern.find(response) | ||
| if (match != null) { | ||
| return match.groupValues[1].trim() | ||
| } | ||
|
|
||
| // Last resort: look for SELECT statement | ||
| val selectPattern = Regex("(SELECT[\\s\\S]*?;)", RegexOption.IGNORE_CASE) | ||
| val selectMatch = selectPattern.find(response) | ||
| return selectMatch?.groupValues?.get(1)?.trim() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the CodeFence class and understand parse vs parseAll
rg "fun parse\(|fun parseAll\(" --type=kt -B2 -A8Repository: phodal/auto-dev
Length of output: 85
🏁 Script executed:
# Search for SqlReviseAgent and its implementation
fd "SqlReviseAgent" --type=ktRepository: phodal/auto-dev
Length of output: 228
🏁 Script executed:
# Look for all usages of CodeFence.parse and CodeFence.parseAll
rg "CodeFence\.(parse|parseAll)" --type=kt -B2 -A2Repository: phodal/auto-dev
Length of output: 85
🏁 Script executed:
# Search for CodeFence class definition
rg "class CodeFence|fun parse\(|fun parseAll\(" -B2 -A8Repository: phodal/auto-dev
Length of output: 50373
🏁 Script executed:
# Search for SqlReviseAgent
rg "SqlReviseAgent" -B2 -A5Repository: phodal/auto-dev
Length of output: 11066
🏁 Script executed:
# Search for extractSqlFromResponse in different files
rg "extractSqlFromResponse" -B2 -A10Repository: phodal/auto-dev
Length of output: 5510
🏁 Script executed:
# Let's examine the CodeFence.parse implementation to understand the difference
rg -A 30 "fun parse\(content: String\): CodeFence" mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/parser/CodeFence.ktRepository: phodal/auto-dev
Length of output: 1389
🏁 Script executed:
# And look at parseAll implementation
rg -A 40 "fun parseAll\(content: String\): List<CodeFence>" mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/parser/CodeFence.ktRepository: phodal/auto-dev
Length of output: 2188
Use CodeFence.parseAll to handle multiple code blocks, consistent with SqlReviseAgent.
The current implementation uses CodeFence.parse which returns only the first code fence from the response. When an LLM response contains multiple code blocks (e.g., explanation followed by SQL), this misses the actual SQL block. SqlReviseAgent correctly uses CodeFence.parseAll with .find { it.languageId.lowercase() == "sql" } to locate the SQL fence among all blocks.
Update extractSqlFromResponse to use parseAll and filter for the SQL fence before falling back to regex patterns.
🤖 Prompt for AI Agents
In mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt
around lines 198-215, replace the single-code-fence usage with
CodeFence.parseAll: call CodeFence.parseAll(response).find {
it.languageId.lowercase() == "sql" && it.text.isNotBlank() } and return its
trimmed text if found; only if no SQL fence is found, continue to the existing
regex fallbacks (the ```sql block regex and the SELECT regex). Remove the use of
CodeFence.parse(response) and ensure languageId is lowercased for comparison and
text is trimmed before returning.
| val providerType = when (activeConfig.provider.lowercase()) { | ||
| "openai" -> LLMProviderType.OPENAI | ||
| "anthropic" -> LLMProviderType.ANTHROPIC | ||
| "google" -> LLMProviderType.GOOGLE | ||
| "deepseek" -> LLMProviderType.DEEPSEEK | ||
| "ollama" -> LLMProviderType.OLLAMA | ||
| "openrouter" -> LLMProviderType.OPENROUTER | ||
| "glm" -> LLMProviderType.GLM | ||
| "qwen" -> LLMProviderType.QWEN | ||
| "kimi" -> LLMProviderType.KIMI | ||
| else -> LLMProviderType.CUSTOM_OPENAI_BASE | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the LLMProviderType enum definition and check for companion methods
fd -t f -e kt | xargs rg -l "enum class LLMProviderType|object LLMProviderType" | head -5Repository: phodal/auto-dev
Length of output: 120
🏁 Script executed:
# Search for LLMProviderType definition with context
rg -A 30 "enum class LLMProviderType" --type=ktRepository: phodal/auto-dev
Length of output: 85
🏁 Script executed:
# Search for other instances of provider string mapping (similar when expressions or fromString calls)
rg -n "when.*provider.*lowercase\(\)|LLMProviderType\." --type=kt -B 2 -A 2Repository: phodal/auto-dev
Length of output: 85
🏁 Script executed:
# Read the ModelConfig.kt file to see LLMProviderType definition
cat mpp-core/src/commonMain/kotlin/cc/unitmesh/llm/ModelConfig.ktRepository: phodal/auto-dev
Length of output: 4867
🏁 Script executed:
# Search for provider string mapping without type flag
rg -n "when.*provider.*lowercase\(\)" -B 2 -A 10Repository: phodal/auto-dev
Length of output: 10599
🏁 Script executed:
# Search for fromString or similar companion methods related to provider
rg -n "fromString|valueOf" mpp-core/src/commonMain/kotlin/cc/unitmesh/llm/ModelConfig.ktRepository: phodal/auto-dev
Length of output: 41
Duplicate provider mapping across multiple CLI files; add a dedicated parsing method to LLMProviderType.
This when-expression is duplicated across at least five CLI files (ChatDBCli, DocumentCli, ReviewCli, CodingCli, DomainDictCli), and also in JsExports. While LLMProviderType has a fromDisplayName() companion method, it matches on capitalized display names ("OpenAI", "Anthropic") rather than lowercase strings ("openai", "anthropic"), so it cannot replace this logic.
Add a companion method like fun fromLowercaseString(name: String): LLMProviderType? to centralize the string-to-enum mapping and reduce duplication.
🤖 Prompt for AI Agents
In mpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/ChatDBCli.kt lines 94-105,
the provider lowercase-to-enum when-expression is duplicated across multiple CLI
files; add a companion method to LLMProviderType to centralize this mapping.
Implement fun fromLowercaseString(name: String): LLMProviderType in
LLMProviderType's companion object that matches the same lowercase keys
("openai", "anthropic", "google", "deepseek", "ollama", "openrouter", "glm",
"qwen", "kimi") and returns the corresponding enum (and fall back to
LLMProviderType.CUSTOM_OPENAI_BASE or null if you prefer explicit handling).
Replace the inline when-expressions in ChatDBCli and the other CLI/JsExports
files to call
LLMProviderType.fromLowercaseString(activeConfig.provider.lowercase()) and
handle the fallback/nullable result consistently.
- Fix ExecutorFactory.kt runBlocking issue for WASM by using expect/actual pattern - Move IdeaDatabaseConnection.kt from mpp-idea-core to ext-database module - Add AssertJ dependency to mpp-idea for test assertions - Replace AssertJ assertions with kotlin.test assertions in some test files - Update test files to use standard Kotlin test assertions
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
mpp-idea/mpp-idea-exts/ext-database/src/main/kotlin/cc/unitmesh/database/connection/IdeaDatabaseConnection.kt (2)
21-55: Resource leak:PreparedStatementandResultSetnot closed on exception.If an exception occurs during result processing (lines 30-50), neither
resultSetnorstmtwill be closed. Use Kotlin'suseextension to ensure proper cleanup.Apply this diff to fix the resource leak:
override suspend fun executeQuery(sql: String): QueryResult = withContext(Dispatchers.IO) { try { - val stmt = ideaConnection.prepareStatement( + ideaConnection.prepareStatement( sql, java.sql.ResultSet.TYPE_SCROLL_INSENSITIVE, java.sql.ResultSet.CONCUR_READ_ONLY - ) - val resultSet = stmt.executeQuery() - - val metaData = resultSet.metaData - val columnCount = metaData.columnCount - val columns = (1..columnCount).map { metaData.getColumnLabel(it) } - - val rows = mutableListOf<List<String>>() - while (resultSet.next()) { - val row = mutableListOf<String>() - for (i in 1..columnCount) { - val value = resultSet.getObject(i) - row.add(value?.toString() ?: "") + ).use { stmt -> + stmt.executeQuery().use { resultSet -> + val metaData = resultSet.metaData + val columnCount = metaData.columnCount + val columns = (1..columnCount).map { metaData.getColumnLabel(it) } + + val rows = mutableListOf<List<String>>() + while (resultSet.next()) { + val row = mutableListOf<String>() + for (i in 1..columnCount) { + val value = resultSet.getObject(i) + row.add(value?.toString() ?: "") + } + rows.add(row) + } + + QueryResult( + columns = columns, + rows = rows, + rowCount = rows.size + ) } - rows.add(row) } - - resultSet.close() - stmt.close() - - QueryResult( - columns = columns, - rows = rows, - rowCount = rows.size - ) } catch (e: Exception) { throw DatabaseException.queryFailed(sql, e.message ?: "Unknown error") } }
57-136: Resource leak:ResultSetobjects not closed on exception ingetSchema.Multiple
ResultSetobjects (tableRs,pkRs,columnRs) are closed only in the happy path. If an exception occurs mid-processing, these resources will leak. Use Kotlin'suseextension for eachResultSet.Apply this diff to fix the primary resource leak pattern (similar changes needed for
pkRsandcolumnRs):- val tableRs = metadata.getTables(null, null, "%", tableTypes) - - while (tableRs.next()) { + metadata.getTables(null, null, "%", tableTypes).use { tableRs -> + while (tableRs.next()) { // ... processing logic - } - tableRs.close() + } + }mpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/util/parser/MarkdownToHtmlConverterTest.kt (1)
9-10: Add documentation for @ignore or remove the annotation.This test was added with the
@Ignoreannotation in the recent CI fix commit, but there's no explanation for why it's ignored. Tests added to the codebase should either:
- Run and pass, or
- Include a comment explaining why they're ignored (e.g., "TODO: Fix converter to handle X case" or "Blocked by issue #123")
Currently, this ignored test provides no value and masks any future regressions in
convertMarkdownToHtml.
🧹 Nitpick comments (2)
mpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/sketch/run/ShellSafetyCheckTest.kt (1)
14-136: Assertion migration preserves semantics; consider minor readability tweaksAll migrated assertions correctly check
result.firstfor danger flags andresult.secondfor the expected messages, so behavior is unchanged and clear.If you want to reduce repetition, you could optionally destructure and/or add a tiny helper:
val (isDangerous, message) = ShellSafetyCheck.checkDangerousCommand(command) assertTrue(isDangerous) assertEquals("...", message)or
private fun assertDanger(command: String, expectedMsg: String) { val (isDangerous, message) = ShellSafetyCheck.checkDangerousCommand(command) assertTrue(isDangerous) assertEquals(expectedMsg, message) }Then each test becomes a one-liner call to
assertDanger(...). Not required, just a possible cleanup.mpp-core/src/commonMain/kotlin/cc/unitmesh/llm/ExecutorFactory.kt (1)
64-71: Clarify error message to distinguish platform limitations from provider failures.The current error message assumes
nullalways indicates a platform limitation (non-JVM), but on JVM,provider.createExecutor(config)could also returnnullif the provider fails to create an executor. This could mislead debugging.Consider refactoring to provide clearer error messages:
if (registryProvider != null) { - // Use platform-specific blocking call for registered providers - // This works because on JVM, the provider caches the API token - return createExecutorBlocking(registryProvider, config) - ?: throw IllegalStateException( - "Failed to create executor for ${config.provider.displayName}. " + - "Provider is registered but returned null. " + - "On non-JVM platforms, use createAsync() instead." - ) + val executor = createExecutorBlocking(registryProvider, config) + if (executor == null) { + throw IllegalStateException( + "Failed to create executor for ${config.provider.displayName}. " + + "Possible causes: " + + "1) Blocking executor creation not supported on this platform (JS/WASM) - use createAsync() instead. " + + "2) Provider failed to create executor - check provider configuration and logs." + ) + } + return executor }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
mpp-core/src/commonMain/kotlin/cc/unitmesh/llm/ExecutorFactory.kt(2 hunks)mpp-core/src/jsMain/kotlin/cc/unitmesh/llm/ExecutorFactory.js.kt(1 hunks)mpp-core/src/jvmMain/kotlin/cc/unitmesh/llm/ExecutorFactory.jvm.kt(2 hunks)mpp-core/src/wasmJsMain/kotlin/cc/unitmesh/llm/ExecutorFactory.wasmJs.kt(1 hunks)mpp-idea/build.gradle.kts(1 hunks)mpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/sketch/run/ShellSafetyCheckTest.kt(1 hunks)mpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/util/parser/MarkdownTest.kt(6 hunks)mpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/util/parser/MarkdownToHtmlConverterTest.kt(3 hunks)mpp-idea/mpp-idea-exts/ext-database/src/main/kotlin/cc/unitmesh/database/connection/IdeaDatabaseConnection.kt(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualpattern for platform-specific code (for example:Platformimplementations)
Use concrete classes (not interfaces) andPromise(notFlow) for @JsExport annotations
Avoid emoji and UTF-8 characters in WASM code
When modifyingCodingAgentRenderer, update ALL implementations:DefaultCodingAgentRenderer,ComposeRenderer,JewelRenderer,ServerSideRenderer, andJsRendererAdapterin Kotlin
When modifyingCodingAgentRenderer, update JVM CLI implementations:CodingCliRendererandConsoleRenderer
UseAutoDevColorsorMaterialTheme.colorSchemefor Compose styling
Files:
mpp-core/src/jvmMain/kotlin/cc/unitmesh/llm/ExecutorFactory.jvm.ktmpp-core/src/wasmJsMain/kotlin/cc/unitmesh/llm/ExecutorFactory.wasmJs.ktmpp-core/src/jsMain/kotlin/cc/unitmesh/llm/ExecutorFactory.js.ktmpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/util/parser/MarkdownTest.ktmpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/sketch/run/ShellSafetyCheckTest.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/llm/ExecutorFactory.ktmpp-idea/mpp-idea-exts/ext-database/src/main/kotlin/cc/unitmesh/database/connection/IdeaDatabaseConnection.ktmpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/util/parser/MarkdownToHtmlConverterTest.kt
**/*.{kt,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never use hardcoded colors - always use design tokens from the design system
Files:
mpp-core/src/jvmMain/kotlin/cc/unitmesh/llm/ExecutorFactory.jvm.ktmpp-core/src/wasmJsMain/kotlin/cc/unitmesh/llm/ExecutorFactory.wasmJs.ktmpp-core/src/jsMain/kotlin/cc/unitmesh/llm/ExecutorFactory.js.ktmpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/util/parser/MarkdownTest.ktmpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/sketch/run/ShellSafetyCheckTest.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/llm/ExecutorFactory.ktmpp-idea/mpp-idea-exts/ext-database/src/main/kotlin/cc/unitmesh/database/connection/IdeaDatabaseConnection.ktmpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/util/parser/MarkdownToHtmlConverterTest.kt
🧠 Learnings (1)
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : Use `expect`/`actual` pattern for platform-specific code (for example: `Platform` implementations)
Applied to files:
mpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/util/parser/MarkdownTest.ktmpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/sketch/run/ShellSafetyCheckTest.kt
🧬 Code graph analysis (4)
mpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/util/parser/MarkdownTest.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/test/DevInsTestCase.kt (2)
assertEquals(45-49)assertTrue(29-33)
mpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/sketch/run/ShellSafetyCheckTest.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/test/DevInsTestCase.kt (3)
assertTrue(29-33)assertEquals(45-49)assertFalse(38-40)
mpp-core/src/commonMain/kotlin/cc/unitmesh/llm/ExecutorFactory.kt (3)
mpp-core/src/jsMain/kotlin/cc/unitmesh/llm/ExecutorFactory.js.kt (1)
createExecutorBlocking(15-18)mpp-core/src/jvmMain/kotlin/cc/unitmesh/llm/ExecutorFactory.jvm.kt (1)
createExecutorBlocking(29-36)mpp-core/src/wasmJsMain/kotlin/cc/unitmesh/llm/ExecutorFactory.wasmJs.kt (1)
createExecutorBlocking(15-18)
mpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/util/parser/MarkdownToHtmlConverterTest.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/test/DevInsTestCase.kt (1)
assertEquals(45-49)
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: Test mpp-core (KMP)
🔇 Additional comments (10)
mpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/util/parser/MarkdownTest.kt (1)
8-8: Kotlin test assertions and import usage look goodUsing
kotlin.test.assertEqualsandkotlin.test.assertTrueconsistently across these tests is idiomatic and matches the rest of the file; the addedassertTrueimport is correct and there are no behavior changes introduced by these updates.Also applies to: 20-20, 43-43, 55-55, 70-70, 85-85
mpp-idea/build.gradle.kts (1)
175-175: AssertJ dependency is correctly scoped for existing mpp-idea tests.AssertJ 3.24.2 is already used in six existing mpp-idea test files (LineInfoTest, DiffSimplifierTest, PlanTaskTest, MarkdownPlanParserTest, ToolCallTest, PromptOptimizerTest). This change explicitly declares it as a shared testImplementation dependency for the mpp-idea module. The three new test classes (SchemaLinkerTest, SqlReviseAgentTest, JSqlParserValidatorTest) are in mpp-core and use kotlin.test assertions, not AssertJ—there is no contradiction.
mpp-idea/mpp-idea-exts/ext-database/src/main/kotlin/cc/unitmesh/database/connection/IdeaDatabaseConnection.kt (2)
138-144: LGTM!The
closemethod properly wraps the underlying connection close and handles exceptions appropriately.
146-172: Stub methods throwUnsupportedOperationException— consider documenting the implementation timeline.Both
createFromIdeaandgetAvailableDataSourcesthrow exceptions with messages indicating future implementation. Given the PR's objective to enable ChatDB functionality with IDEA data sources, these stubs may block full integration.Consider either:
- Adding a TODO/FIXME comment with an issue reference for tracking implementation.
- Implementing the actual IDEA Database API integration if it's needed for this PR.
companion object { + // TODO: Implement IDEA Database API integration - see issue #XXX /** * Create database connection from IDEA data sourcempp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/sketch/run/ShellSafetyCheckTest.kt (1)
4-6: Switch tokotlin.testassertions is appropriateUsing
kotlin.test.assertEquals/assertTrue/assertFalsealigns with Kotlin testing conventions and keeps the test dependency surface minimal. No issues here.mpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/util/parser/MarkdownToHtmlConverterTest.kt (1)
19-19: LGTM: Test data correction.The added blank lines in the markdown input correctly align with the blank lines already present in the expected HTML output (lines 40, 45, 50), making the test data more consistent and accurate.
Also applies to: 24-24, 29-29
mpp-core/src/jsMain/kotlin/cc/unitmesh/llm/ExecutorFactory.js.kt (1)
3-18: LGTM! JS platform correctly returns null for blocking executor creation.The implementation appropriately returns
nullfor the JS platform and provides clear documentation directing users to the asynchronous alternative.mpp-core/src/wasmJsMain/kotlin/cc/unitmesh/llm/ExecutorFactory.wasmJs.kt (1)
3-18: LGTM! WASM platform correctly returns null for blocking executor creation.The implementation is consistent with the JS platform approach and appropriately indicates non-support for blocking executor creation.
mpp-core/src/commonMain/kotlin/cc/unitmesh/llm/ExecutorFactory.kt (1)
20-30: LGTM! The expect declaration is well-documented and correctly typed.The platform-specific documentation clearly explains the behavior on each platform, and the nullable return type appropriately handles platforms that don't support blocking executor creation.
mpp-core/src/jvmMain/kotlin/cc/unitmesh/llm/ExecutorFactory.jvm.kt (1)
29-36: No timeout required—caching prevents repeated blocking calls.The
runBlockingdesign is intentional. TheGithubCopilotClientProvidercaches the API token with expiration checks, sogetApiToken()performs an HTTP request only on the first call or when the token expires. Subsequent calls return the cached token within a mutex lock—a fast operation suitable for synchronous blocking. Additionally, inKoogLLMService, the executor is initialized lazily, deferring the blocking call until first use. This is service-level initialization code, not a UI hot path, so brief blocking is acceptable.
| import org.assertj.core.api.Assertions.assertThat | ||
| import kotlin.test.Ignore | ||
| import kotlin.test.Test | ||
| import kotlin.test.assertEquals |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check assertion library usage patterns in the test suite
rg -n --type=kotlin 'import.*assertEquals|import.*assertThat' mpp-idea/mpp-idea-core/src/test/ | head -30Repository: phodal/auto-dev
Length of output: 174
🏁 Script executed:
# Check for any test configuration or conventions in the project
find . -name 'build.gradle*' -o -name 'pom.xml' -o -name 'settings.gradle*' | head -5Repository: phodal/auto-dev
Length of output: 197
🏁 Script executed:
# Look at the actual test file to confirm the changes and @Ignore status
cat -n mpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/util/parser/MarkdownToHtmlConverterTest.kt | head -70Repository: phodal/auto-dev
Length of output: 2174
🏁 Script executed:
# Check assertion library imports across all test files to determine project standard
rg --type=kotlin 'import.*assertEquals|import.*assertThat|import org\.assertj' mpp-idea/mpp-idea-core/src/test/ | cut -d: -f1 | sort | uniq -c | sort -rnRepository: phodal/auto-dev
Length of output: 2578
🏁 Script executed:
# Get a sample of other test files to see what assertion libraries are used
rg --type=kotlin -l 'class.*Test' mpp-idea/mpp-idea-core/src/test/ | head -10 | xargs -I {} sh -c 'echo "=== {} ===" && head -20 {}'Repository: phodal/auto-dev
Length of output: 6975
Remove @Ignore annotation to enable test validation.
The assertion library migration to kotlin.test.assertEquals is appropriate (line 5 and 59), with correct argument order (expected, actual). However, the @Ignore annotation (line 10) prevents this test from executing, leaving the changes unvalidated. Remove the annotation to verify the test passes with the markdown input corrections.
🤖 Prompt for AI Agents
In
mpp-idea/mpp-idea-core/src/test/kotlin/cc/unitmesh/devti/util/parser/MarkdownToHtmlConverterTest.kt
around lines 5 to 59, remove the @Ignore annotation on the test method (line
~10) so the test actually runs; the kotlin.test.assertEquals import and calls
are already correct (expected, actual) so simply delete the @Ignore annotation,
run the test suite to verify it passes, and commit the change.
- Add CHAT_DB branch to IdeaAgentApp.kt when expression - Add createExecutorBlocking actual implementation for iOS platform - Import SingleLLMPromptExecutor in iOS ExecutorFactory
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt (1)
355-361: ChatDB tab is exposed but only shows a “coming soon” placeholderThis branch compiles and follows existing layout patterns, but from a UX/feature‑parity standpoint it surfaces
AgentType.CHAT_DBin IDEA without any of the text‑to‑SQL or data‑source UI that exists in mpp‑ui.Consider either:
- Wiring this branch to a proper ChatDB ViewModel/compose surface (mirroring the mpp‑ui ChatDB page), or
- Temporarily hiding
AgentType.CHAT_DBfrom the IDEA header until the implementation is ready, or updating the message to clearly state that ChatDB is not yet available in the IDEA plugin.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mpp-core/src/iosMain/kotlin/cc/unitmesh/llm/ExecutorFactory.ios.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualpattern for platform-specific code (for example:Platformimplementations)
Use concrete classes (not interfaces) andPromise(notFlow) for @JsExport annotations
Avoid emoji and UTF-8 characters in WASM code
When modifyingCodingAgentRenderer, update ALL implementations:DefaultCodingAgentRenderer,ComposeRenderer,JewelRenderer,ServerSideRenderer, andJsRendererAdapterin Kotlin
When modifyingCodingAgentRenderer, update JVM CLI implementations:CodingCliRendererandConsoleRenderer
UseAutoDevColorsorMaterialTheme.colorSchemefor Compose styling
Files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.ktmpp-core/src/iosMain/kotlin/cc/unitmesh/llm/ExecutorFactory.ios.kt
**/*.{kt,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never use hardcoded colors - always use design tokens from the design system
Files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.ktmpp-core/src/iosMain/kotlin/cc/unitmesh/llm/ExecutorFactory.ios.kt
🧠 Learnings (3)
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to mpp-vscode/src/**/*.ts : When modifying `CodingAgentRenderer`, update VSCode implementations: `mpp-vscode/src/bridge/mpp-core.ts` and `mpp-vscode/src/providers/chat-view.ts`
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update ALL implementations: `DefaultCodingAgentRenderer`, `ComposeRenderer`, `JewelRenderer`, `ServerSideRenderer`, and `JsRendererAdapter` in Kotlin
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update JVM CLI implementations: `CodingCliRenderer` and `ConsoleRenderer`
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt
🧬 Code graph analysis (1)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt (1)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/components/timeline/IdeaTimelineContent.kt (1)
IdeaEmptyStateMessage(242-256)
⏰ 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). (2)
- GitHub Check: Test mpp-core (KMP)
- GitHub Check: Build
🔇 Additional comments (1)
mpp-core/src/iosMain/kotlin/cc/unitmesh/llm/ExecutorFactory.ios.kt (1)
3-3: Import forSingleLLMPromptExecutoris correct and necessaryThe import cleanly matches the new iOS actual signature and keeps the file’s dependencies minimal.
| /** | ||
| * iOS implementation: Blocking executor creation not supported | ||
| * iOS doesn't support runBlocking, so we return null | ||
| */ | ||
| internal actual fun createExecutorBlocking( | ||
| provider: LLMClientProvider, | ||
| config: ModelConfig | ||
| ): SingleLLMPromptExecutor? = null |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the expect declaration of createExecutorBlocking
find . -type f -name "*.kt" | xargs grep -l "expect.*createExecutorBlocking" | head -5Repository: phodal/auto-dev
Length of output: 126
🏁 Script executed:
# Search for all usages of createExecutorBlocking with context
rg -n 'createExecutorBlocking\(' -C3Repository: phodal/auto-dev
Length of output: 4745
🏁 Script executed:
# Look for ExecutorFactory files across platforms
fd "ExecutorFactory" -e ktRepository: phodal/auto-dev
Length of output: 471
🏁 Script executed:
# Read the expect declaration to see full KDoc
cat -n mpp-core/src/commonMain/kotlin/cc/unitmesh/llm/ExecutorFactory.kt | head -35Repository: phodal/auto-dev
Length of output: 1465
🏁 Script executed:
# Check Android implementation
cat -n mpp-core/src/androidMain/kotlin/cc/unitmesh/llm/ExecutorFactory.android.ktRepository: phodal/auto-dev
Length of output: 340
🏁 Script executed:
# Find all call sites where createExecutorBlocking is invoked
rg 'createExecutorBlocking\(' --type kt -B2 -A5Repository: phodal/auto-dev
Length of output: 85
🏁 Script executed:
# Retry without --type flag
rg 'createExecutorBlocking\(' -B2 -A5Repository: phodal/auto-dev
Length of output: 4684
🏁 Script executed:
# Check if Android file has more content (looks incomplete)
wc -l mpp-core/src/androidMain/kotlin/cc/unitmesh/llm/ExecutorFactory.android.ktRepository: phodal/auto-dev
Length of output: 135
🏁 Script executed:
# Get full Android file
cat mpp-core/src/androidMain/kotlin/cc/unitmesh/llm/ExecutorFactory.android.ktRepository: phodal/auto-dev
Length of output: 277
🏁 Script executed:
# Check if there's a common/default implementation or if Android uses inheritance
rg -A10 "createExecutorBlocking" mpp-core/src/androidMain/ --type-list | head -20Repository: phodal/auto-dev
Length of output: 448
🏁 Script executed:
# Look for build configuration to understand how Android source sets work
cat mpp-core/build.gradle.kts | grep -A5 -B5 "androidMain" | head -20Repository: phodal/auto-dev
Length of output: 446
Update expect declaration KDoc to document all platforms returning null
The expect function in mpp-core/src/commonMain/kotlin/cc/unitmesh/llm/ExecutorFactory.kt currently documents only "JS/WASM: Returns null" but should also explicitly mention iOS (and Android if applicable) to accurately reflect all non-JVM platforms. The single call site at line 66 correctly handles the nullable result using the Elvis operator with an explicit error message directing users to createAsync(), so null-safety is verified. Update the KDoc to:
* - JVM: Uses runBlocking to create executor synchronously
* - iOS/JS/WASM: Returns null (blocking calls not supported)
🤖 Prompt for AI Agents
In mpp-core/src/commonMain/kotlin/cc/unitmesh/llm/ExecutorFactory.kt (expect
declaration KDoc), update the documentation so it lists all platform behaviors:
replace the existing platform note that only mentions JS/WASM with the following
two bullets — "JVM: Uses runBlocking to create executor synchronously" and
"iOS/JS/WASM: Returns null (blocking calls not supported)"; keep the function
signature and nullable return as-is since call sites handle null via the Elvis
operator and direct users to createAsync().
#508 Introduce LlmSchemaLinker for LLM-powered schema linking with fallback, add table whitelist validation to SQL execution, and refactor SchemaLinker to support multiple strategies. Also improve SQL execution error handling and retries.
Update prompts and context for SQL generation and revision agents to enforce stricter schema usage and concise SQL-only responses.
Introduce DatabaseContentSchemaLinker to enhance schema linking accuracy using table sample data and system table filtering. Add support for English and Chinese keyword extraction and new database methods for retrieving sample rows and distinct values.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt (2)
98-111: FixvalidatedSqlnullability and redundant!!to avoid type issues.
validatedSqlis inferred asString?:var validatedSql = generatedSqlbut then:
val syntaxValidation = jsqlValidator.validate(validatedSql!!) val tableValidation = if (syntaxValidation.isValid) { jsqlValidator.validateWithTableWhitelist(validatedSql, allTableNames) } else { syntaxValidation }This both uses an unnecessary
!!and passes a nullablevalidatedSqltovalidateWithTableWhitelist(sql: String, ...), which expects a non‑nullString. The clean fix is to makevalidatedSqlnon‑nullable at the point we knowgeneratedSqlis non‑null:- var validatedSql = generatedSql - - // Get all table names from schema for whitelist validation + var validatedSql = generatedSql!! // safe: generatedSql null-checked above + + // Get all table names from schema for whitelist validation val allTableNames = schema.tables.map { it.name }.toSet() // First validate syntax, then validate table names - val syntaxValidation = jsqlValidator.validate(validatedSql!!) + val syntaxValidation = jsqlValidator.validate(validatedSql) val tableValidation = if (syntaxValidation.isValid) { - jsqlValidator.validateWithTableWhitelist(validatedSql, allTableNames) + jsqlValidator.validateWithTableWhitelist(validatedSql, allTableNames) } else { syntaxValidation }This removes the redundant assertion and aligns types with the validator API.
252-269: UseCodeFence.parseAlland search for the SQL fence, consistent withSqlReviseAgent.
extractSqlFromResponsecurrently usesCodeFence.parse(response)and assumes a single code fence withlanguageId == "sql". As previously noted in an earlier review, this misses valid SQL when the LLM returns multiple code blocks (e.g., explanation + SQL) and relies on a single fence.You already have a robust pattern in
SqlReviseAgent:val codeFences = CodeFence.parseAll(response) val sqlFence = codeFences.find { it.languageId.lowercase() == "sql" && it.text.isNotBlank() } if (sqlFence != null) return sqlFence.text.trim()Refactor
extractSqlFromResponseto useparseAlland only fall back to the regex/SELECTheuristics when no SQL fence is found.
🧹 Nitpick comments (7)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt (2)
235-245: Extract duplicated SQL extraction logic to shared utility.This
extractSqlFromResponsemethod duplicates similar logic found inChatDBAgentExecutor.kt(lines 251-268). The ChatDBAgentExecutor version includes an additional fallback that searches for SELECT statements, making it more robust.Consider extracting this to a shared utility class (e.g.,
SqlExtractoror similar) to maintain DRY principles and ensure consistent SQL extraction behavior across the codebase. The shared version should include all fallback strategies from both implementations.
226-232: Preserve error details when LLM revision fails.When an exception occurs, the error is logged but only
nullis returned. The caller receives a generic "Failed to generate revised SQL" message without insight into the actual failure reason (network error, timeout, rate limit, etc.).Consider propagating error details via the progress callback:
return try { val response = llmService.sendPrompt("$systemPrompt\n\n$userPrompt") extractSqlFromResponse(response) } catch (e: Exception) { logger.error(e) { "LLM revision failed: ${e.message}" } + onProgress("[ERROR] LLM call failed: ${e.message}") null }mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/SchemaLinker.kt (1)
27-67: Heuristics look solid; consider slightly relaxing stop words and extracting constants for reuse.Implementation is correct and self‑contained. Two minor tweaks you might consider:
- Some terms in
STOP_WORDSlike"total","amount","value","number"are common column names; filtering them may reduce recall when users ask about totals/amounts specifically.- The regex and
Regex("\\s+")are re‑created on every call toextractKeywords; moving them tocompanion objectconstants would avoid repeated compilation on hot paths.These are non‑blocking; current behavior is fine if you’re happy with the trade‑offs.
Also applies to: 124-186
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/LlmSchemaLinker.kt (1)
119-130: Strengthen keyword-extraction fallback when JSON parsing fails silently.
extractKeywordsonly falls back tofallbackLinker.extractKeywordson exceptions, butparseKeywordExtractionResponsecatchesExceptionand returns an emptyKeywordExtractionResult, so malformed JSON just yields an empty keyword list with no fallback.To keep behavior robust when the LLM drifts from the JSON contract, consider:
- Letting JSON parse errors bubble up so the outer
try/catchtriggers the fallback, or- Checking for an empty
(keywords + entities + attributes)list and, in that case, delegating tofallbackLinker.extractKeywords(query).This keeps LLM‑based extraction while guaranteeing at least keyword‑based behavior when the response is off‑format.
Also applies to: 141-147
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt (2)
140-187: Re-validate revised SQL before executing to keep whitelist and syntax guarantees.When initial validation fails, you call
SqlReviseAgentand, on success, set:validatedSql = revisionResult.content generatedSql = validatedSqlbut you don’t re-run
jsqlValidator.validate/validateWithTableWhiteliston the revised SQL; execution goes straight todatabaseConnection.executeQuery. This means revised SQL might bypass your AST-based checks (e.g., table whitelist).To keep the “validated before execution” guarantee, consider re-running the same validation pipeline on the final
validatedSqland short-circuiting execution if it’s still invalid. This is especially relevant if prompts or revise logic evolve over time.
271-305: Align PlotDSL extraction with multi-fence handling to mirror SQL extraction.
generateVisualizationcallsCodeFence.parse(response)and assumes the first fence isplotdsl. As with SQL extraction, LLM responses can contain multiple code blocks, so this may miss the actual visualization block.You can make this more robust by:
- Using
CodeFence.parseAll(response)and finding the first fence withlanguageId.lowercase() == "plotdsl"and non‑blank text.- Only then falling back to the regex
```plotdsl ... ```search.This keeps behavior consistent with the recommended SQL extraction path and
SqlReviseAgent.mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidator.kt (1)
20-184: Validator implementation matches intended behavior.The syntax validation, table-whitelist check, and warning extraction are all wired correctly and align with how
ChatDBAgentExecutorandSqlReviseAgentuse this class. I don’t see functional issues here.If you ever see perf pressure, you could consider sharing a parsed
Statementbetweenvalidate,validateWithTableWhitelist, andextractTableNames, but that’s not necessary now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/LlmSchemaLinker.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/SchemaLinker.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt(1 hunks)mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/chatdb/SchemaLinkerTest.kt(1 hunks)mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgent.kt(1 hunks)mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt(1 hunks)mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidator.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/chatdb/SchemaLinkerTest.kt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualpattern for platform-specific code (for example:Platformimplementations)
Use concrete classes (not interfaces) andPromise(notFlow) for @JsExport annotations
Avoid emoji and UTF-8 characters in WASM code
When modifyingCodingAgentRenderer, update ALL implementations:DefaultCodingAgentRenderer,ComposeRenderer,JewelRenderer,ServerSideRenderer, andJsRendererAdapterin Kotlin
When modifyingCodingAgentRenderer, update JVM CLI implementations:CodingCliRendererandConsoleRenderer
UseAutoDevColorsorMaterialTheme.colorSchemefor Compose styling
Files:
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/LlmSchemaLinker.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/SchemaLinker.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidator.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgent.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt
**/*.{kt,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never use hardcoded colors - always use design tokens from the design system
Files:
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/LlmSchemaLinker.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/SchemaLinker.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidator.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgent.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt
🧠 Learnings (3)
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update JVM CLI implementations: `CodingCliRenderer` and `ConsoleRenderer`
Applied to files:
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgent.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to mpp-vscode/src/**/*.ts : When modifying `CodingAgentRenderer`, update VSCode implementations: `mpp-vscode/src/bridge/mpp-core.ts` and `mpp-vscode/src/providers/chat-view.ts`
Applied to files:
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update ALL implementations: `DefaultCodingAgentRenderer`, `ComposeRenderer`, `JewelRenderer`, `ServerSideRenderer`, and `JsRendererAdapter` in Kotlin
Applied to files:
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt
🧬 Code graph analysis (5)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/LlmSchemaLinker.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/SchemaLinker.kt (2)
extractKeywords(25-25)extractKeywords(124-130)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/SchemaLinker.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/LlmSchemaLinker.kt (1)
extractKeywords(119-130)
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt (2)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/BaseAgentExecutor.kt (1)
getLLMResponse(49-85)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt (1)
extractSqlFromResponse(235-245)
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgent.kt (1)
mpp-vscode/src/bridge/mpp-core.ts (1)
ToolRegistry(226-306)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt (2)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/schema/ToolSchema.kt (2)
string(136-155)integer(157-170)mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt (1)
extractSqlFromResponse(252-269)
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: Test mpp-core (KMP)
🔇 Additional comments (1)
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgent.kt (1)
40-171: Agent wiring and configuration look consistent and correct.The ChatDBAgent setup (ToolRegistry, ToolOrchestrator, lazy ChatDBAgentExecutor, and SYSTEM_PROMPT) is coherent and matches the intended Text2SQL flow. Input validation and metadata mapping for
ToolResult.AgentResultare reasonable defaults.No changes needed from this file’s perspective.
| override suspend fun link(query: String, schema: DatabaseSchema): SchemaLinkingResult { | ||
| return try { | ||
| // Build schema description for LLM | ||
| val schemaDescription = buildSchemaDescription(schema) | ||
|
|
||
| // Ask LLM to identify relevant tables and columns | ||
| val prompt = SCHEMA_LINKING_PROMPT | ||
| .replace("{{SCHEMA}}", schemaDescription) | ||
| .replace("{{QUERY}}", query) | ||
| val response = llmService.sendPrompt(prompt) | ||
|
|
||
| // Parse LLM response | ||
| val llmResult = parseSchemaLinkingResponse(response) | ||
|
|
||
| // Validate that tables/columns exist in schema | ||
| val validTables = llmResult.tables.filter { tableName -> | ||
| schema.tables.any { it.name.equals(tableName, ignoreCase = true) } | ||
| } | ||
| val validColumns = llmResult.columns.filter { colRef -> | ||
| val parts = colRef.split(".") | ||
| if (parts.size == 2) { | ||
| val table = schema.tables.find { it.name.equals(parts[0], ignoreCase = true) } | ||
| table?.columns?.any { it.name.equals(parts[1], ignoreCase = true) } == true | ||
| } else false | ||
| } | ||
|
|
||
| // Extract keywords for the result | ||
| val keywords = extractKeywords(query) | ||
|
|
||
| // If LLM didn't find valid tables, fall back to keyword linker | ||
| if (validTables.isEmpty()) { | ||
| return fallbackLinker.link(query, schema) | ||
| } | ||
|
|
||
| SchemaLinkingResult( | ||
| relevantTables = validTables, | ||
| relevantColumns = validColumns, | ||
| keywords = keywords, | ||
| confidence = llmResult.confidence | ||
| ) | ||
| } catch (e: Exception) { |
There was a problem hiding this comment.
Normalize table/column names to schema casing after LLM validation.
You validate LLM‑returned tables/columns against DatabaseSchema using equals(..., ignoreCase = true) but keep the original LLM strings in validTables/validColumns. Downstream, ChatDBAgentExecutor.buildRelevantSchemaDescription filters with it.name in linkingResult.relevantTables (case‑sensitive), so users from the LLM won’t match Users in the schema and the relevant schema section may end up empty while ALLOWED TABLES still lists the LLM names.
Consider mapping back to canonical schema names during validation, e.g.:
val validTables = schema.tables
.filter { schemaTable -> llmResult.tables.any { it.equals(schemaTable.name, ignoreCase = true) } }
.map { it.name }and similarly normalizing columns to tableName.columnName using actual schema casing.
🤖 Prompt for AI Agents
In mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/LlmSchemaLinker.kt
around lines 70 to 110, the LLM-validated table and column names are kept with
the original LLM casing which can fail later because other code compares against
schema names case-sensitively; modify the validation to map LLM results back to
the canonical schema casing: for tables, iterate schema.tables and select those
whose name equals any LLM table name ignoring case, then return the
schemaTable.name; for columns, resolve each LLM column reference to the matching
schema table and column (ignore case for matching) and emit a normalized
"TableName.ColumnName" string using the schema's actual table and column names;
ensure fallback behavior remains the same and preserve llmResult.confidence and
keywords.
| "schemaDescription" to string( | ||
| description = "Database schema description for context", | ||
| required = true | ||
| ), |
There was a problem hiding this comment.
Inconsistency between schema requirement and data class default.
The schema marks schemaDescription as required = true (line 37), but SqlRevisionInput (line 269) provides a default empty string. This mismatch could cause confusion for tool consumers expecting the field to be mandatory.
Apply this diff to align the schema with the actual data class behavior:
"schemaDescription" to string(
description = "Database schema description for context",
- required = true
+ required = 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.
| "schemaDescription" to string( | |
| description = "Database schema description for context", | |
| required = true | |
| ), | |
| "schemaDescription" to string( | |
| description = "Database schema description for context", | |
| required = false | |
| ), |
🤖 Prompt for AI Agents
In mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt
around lines 35 to 38, the OpenAPI/schema entry marks "schemaDescription" as
required = true while the SqlRevisionInput data class provides a default empty
string, creating an inconsistency; update the schema to reflect the data class
by setting required = false (or remove the required flag entirely) so the field
is optional, and keep the description text as-is or clarify it indicates an
optional schema description.
| class SqlReviseAgent( | ||
| private val llmService: KoogLLMService, | ||
| private val sqlValidator: SqlValidatorInterface? = null | ||
| ) : SubAgent<SqlRevisionInput, ToolResult.AgentResult>( | ||
| definition = createDefinition() | ||
| ) { |
There was a problem hiding this comment.
Enforce SQL validator to prevent destructive queries.
Issue #508 specifies "AST-based SQL validation to block destructive queries" as an enterprise security requirement. However, SqlValidatorInterface is optional (nullable), meaning destructive SQL (DROP, DELETE, TRUNCATE, etc.) could be generated and returned without validation.
Consider making the validator mandatory or adding a default validator:
class SqlReviseAgent(
private val llmService: KoogLLMService,
- private val sqlValidator: SqlValidatorInterface? = null
+ private val sqlValidator: SqlValidatorInterface
) : SubAgent<SqlRevisionInput, ToolResult.AgentResult>(
definition = createDefinition()
) {Alternatively, if the validator must remain optional, add explicit warnings in logs and metadata when operating without validation.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt
around lines 62 to 67, the SqlValidatorInterface is nullable allowing
destructive SQL to bypass validation; make the validator mandatory by removing
the nullable type and requiring an instance in the constructor, or supply a
secure default validator implementation when null (e.g., an AST-based blocker
that rejects DROP/DELETE/TRUNCATE), and if you must keep it optional instead add
explicit runtime safeguards: log a prominent warning and set metadata flags
indicating validation is disabled, and ensure any execute/return path checks for
validator presence and refuses to return or execute queries that match
destructive patterns when no validator is present.
| onProgress("🔄 SQL Revise Agent - Starting revision") | ||
| onProgress("Original query: ${input.originalQuery.take(50)}...") | ||
| onProgress("Error: ${input.errorMessage.take(80)}...") | ||
|
|
||
| var currentSql = input.failedSql | ||
| var currentError = input.errorMessage | ||
| val attempts = input.previousAttempts.toMutableList() | ||
| var attemptCount = attempts.size | ||
|
|
||
| while (attemptCount < input.maxAttempts) { | ||
| attemptCount++ | ||
| onProgress("📝 Revision attempt $attemptCount/${input.maxAttempts}") | ||
|
|
||
| // Build revision context | ||
| val context = buildRevisionContext(input, currentSql, currentError, attempts) | ||
|
|
||
| // Ask LLM for revised SQL | ||
| val revisedSql = askLLMForRevision(context, onProgress) | ||
|
|
||
| if (revisedSql == null) { | ||
| onProgress("❌ Failed to generate revised SQL") | ||
| return ToolResult.AgentResult( | ||
| success = false, | ||
| content = "Failed to generate revised SQL after $attemptCount attempts", | ||
| metadata = mapOf( | ||
| "attempts" to attemptCount.toString(), | ||
| "lastError" to currentError | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| attempts.add(revisedSql) | ||
|
|
||
| // Validate the revised SQL if validator is available | ||
| if (sqlValidator != null) { | ||
| val validation = sqlValidator.validate(revisedSql) | ||
| if (validation.isValid) { | ||
| onProgress("✅ SQL validated successfully") | ||
| return ToolResult.AgentResult( | ||
| success = true, | ||
| content = revisedSql, | ||
| metadata = mapOf( | ||
| "attempts" to attemptCount.toString(), | ||
| "validated" to "true" | ||
| ) | ||
| ) | ||
| } else { | ||
| currentSql = revisedSql | ||
| currentError = validation.errors.joinToString("; ") | ||
| onProgress("⚠️ Validation failed: ${currentError.take(50)}...") | ||
| } | ||
| } else { | ||
| // No validator, return the revised SQL | ||
| onProgress("✅ SQL revised (no validation)") | ||
| return ToolResult.AgentResult( | ||
| success = true, | ||
| content = revisedSql, | ||
| metadata = mapOf( | ||
| "attempts" to attemptCount.toString(), | ||
| "validated" to "false" | ||
| ) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| onProgress("❌ Max revision attempts reached") |
There was a problem hiding this comment.
Remove emoji characters for WASM compatibility.
Progress messages throughout the execute method use emoji characters (🔄, 📝, ❌, ✅, commonMain, it will be compiled for all platforms including WASM.
Replace emoji with plain-text prefixes:
- onProgress("🔄 SQL Revise Agent - Starting revision")
+ onProgress("[INFO] SQL Revise Agent - Starting revision")
onProgress("Original query: ${input.originalQuery.take(50)}...")
onProgress("Error: ${input.errorMessage.take(80)}...")
var currentSql = input.failedSql
var currentError = input.errorMessage
val attempts = input.previousAttempts.toMutableList()
var attemptCount = attempts.size
while (attemptCount < input.maxAttempts) {
attemptCount++
- onProgress("📝 Revision attempt $attemptCount/${input.maxAttempts}")
+ onProgress("[ATTEMPT] Revision attempt $attemptCount/${input.maxAttempts}")
// Build revision context
val context = buildRevisionContext(input, currentSql, currentError, attempts)
// Ask LLM for revised SQL
val revisedSql = askLLMForRevision(context, onProgress)
if (revisedSql == null) {
- onProgress("❌ Failed to generate revised SQL")
+ onProgress("[ERROR] Failed to generate revised SQL")
return ToolResult.AgentResult(
success = false,
content = "Failed to generate revised SQL after $attemptCount attempts",
metadata = mapOf(
"attempts" to attemptCount.toString(),
"lastError" to currentError
)
)
}
attempts.add(revisedSql)
// Validate the revised SQL if validator is available
if (sqlValidator != null) {
val validation = sqlValidator.validate(revisedSql)
if (validation.isValid) {
- onProgress("✅ SQL validated successfully")
+ onProgress("[SUCCESS] SQL validated successfully")
return ToolResult.AgentResult(
success = true,
content = revisedSql,
metadata = mapOf(
"attempts" to attemptCount.toString(),
"validated" to "true"
)
)
} else {
currentSql = revisedSql
currentError = validation.errors.joinToString("; ")
- onProgress("⚠️ Validation failed: ${currentError.take(50)}...")
+ onProgress("[WARN] Validation failed: ${currentError.take(50)}...")
}
} else {
// No validator, return the revised SQL
- onProgress("✅ SQL revised (no validation)")
+ onProgress("[SUCCESS] SQL revised (no validation)")
return ToolResult.AgentResult(
success = true,
content = revisedSql,
metadata = mapOf(
"attempts" to attemptCount.toString(),
"validated" to "false"
)
)
}
}
- onProgress("❌ Max revision attempts reached")
+ onProgress("[ERROR] Max revision attempts reached")📝 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.
| onProgress("🔄 SQL Revise Agent - Starting revision") | |
| onProgress("Original query: ${input.originalQuery.take(50)}...") | |
| onProgress("Error: ${input.errorMessage.take(80)}...") | |
| var currentSql = input.failedSql | |
| var currentError = input.errorMessage | |
| val attempts = input.previousAttempts.toMutableList() | |
| var attemptCount = attempts.size | |
| while (attemptCount < input.maxAttempts) { | |
| attemptCount++ | |
| onProgress("📝 Revision attempt $attemptCount/${input.maxAttempts}") | |
| // Build revision context | |
| val context = buildRevisionContext(input, currentSql, currentError, attempts) | |
| // Ask LLM for revised SQL | |
| val revisedSql = askLLMForRevision(context, onProgress) | |
| if (revisedSql == null) { | |
| onProgress("❌ Failed to generate revised SQL") | |
| return ToolResult.AgentResult( | |
| success = false, | |
| content = "Failed to generate revised SQL after $attemptCount attempts", | |
| metadata = mapOf( | |
| "attempts" to attemptCount.toString(), | |
| "lastError" to currentError | |
| ) | |
| ) | |
| } | |
| attempts.add(revisedSql) | |
| // Validate the revised SQL if validator is available | |
| if (sqlValidator != null) { | |
| val validation = sqlValidator.validate(revisedSql) | |
| if (validation.isValid) { | |
| onProgress("✅ SQL validated successfully") | |
| return ToolResult.AgentResult( | |
| success = true, | |
| content = revisedSql, | |
| metadata = mapOf( | |
| "attempts" to attemptCount.toString(), | |
| "validated" to "true" | |
| ) | |
| ) | |
| } else { | |
| currentSql = revisedSql | |
| currentError = validation.errors.joinToString("; ") | |
| onProgress("⚠️ Validation failed: ${currentError.take(50)}...") | |
| } | |
| } else { | |
| // No validator, return the revised SQL | |
| onProgress("✅ SQL revised (no validation)") | |
| return ToolResult.AgentResult( | |
| success = true, | |
| content = revisedSql, | |
| metadata = mapOf( | |
| "attempts" to attemptCount.toString(), | |
| "validated" to "false" | |
| ) | |
| ) | |
| } | |
| } | |
| onProgress("❌ Max revision attempts reached") | |
| onProgress("[INFO] SQL Revise Agent - Starting revision") | |
| onProgress("Original query: ${input.originalQuery.take(50)}...") | |
| onProgress("Error: ${input.errorMessage.take(80)}...") | |
| var currentSql = input.failedSql | |
| var currentError = input.errorMessage | |
| val attempts = input.previousAttempts.toMutableList() | |
| var attemptCount = attempts.size | |
| while (attemptCount < input.maxAttempts) { | |
| attemptCount++ | |
| onProgress("[ATTEMPT] Revision attempt $attemptCount/${input.maxAttempts}") | |
| // Build revision context | |
| val context = buildRevisionContext(input, currentSql, currentError, attempts) | |
| // Ask LLM for revised SQL | |
| val revisedSql = askLLMForRevision(context, onProgress) | |
| if (revisedSql == null) { | |
| onProgress("[ERROR] Failed to generate revised SQL") | |
| return ToolResult.AgentResult( | |
| success = false, | |
| content = "Failed to generate revised SQL after $attemptCount attempts", | |
| metadata = mapOf( | |
| "attempts" to attemptCount.toString(), | |
| "lastError" to currentError | |
| ) | |
| ) | |
| } | |
| attempts.add(revisedSql) | |
| // Validate the revised SQL if validator is available | |
| if (sqlValidator != null) { | |
| val validation = sqlValidator.validate(revisedSql) | |
| if (validation.isValid) { | |
| onProgress("[SUCCESS] SQL validated successfully") | |
| return ToolResult.AgentResult( | |
| success = true, | |
| content = revisedSql, | |
| metadata = mapOf( | |
| "attempts" to attemptCount.toString(), | |
| "validated" to "true" | |
| ) | |
| ) | |
| } else { | |
| currentSql = revisedSql | |
| currentError = validation.errors.joinToString("; ") | |
| onProgress("[WARN] Validation failed: ${currentError.take(50)}...") | |
| } | |
| } else { | |
| // No validator, return the revised SQL | |
| onProgress("[SUCCESS] SQL revised (no validation)") | |
| return ToolResult.AgentResult( | |
| success = true, | |
| content = revisedSql, | |
| metadata = mapOf( | |
| "attempts" to attemptCount.toString(), | |
| "validated" to "false" | |
| ) | |
| ) | |
| } | |
| } | |
| onProgress("[ERROR] Max revision attempts reached") |
| appendLine("## Available Schema (USE ONLY THESE TABLES AND COLUMNS)") | ||
| appendLine("```") | ||
| appendLine(input.schemaDescription.take(2000)) | ||
| appendLine("```") |
There was a problem hiding this comment.
Schema truncation may break table/column definitions.
The hard-coded 2000-character limit could cut off schema information mid-table or mid-column, leaving the LLM with incomplete context. This may lead to incorrect SQL generation if critical tables or columns are truncated.
Consider one of the following solutions:
- Make the limit configurable via
SqlRevisionInput - Implement intelligent truncation at table boundaries
- Prioritize tables mentioned in the failed SQL
Example for solution 1:
data class SqlRevisionInput(
val originalQuery: String,
val failedSql: String,
val errorMessage: String,
val schemaDescription: String = "",
val previousAttempts: List<String> = emptyList(),
- val maxAttempts: Int = 3
+ val maxAttempts: Int = 3,
+ val maxSchemaLength: Int = 2000
)Then update line 182:
- appendLine(input.schemaDescription.take(2000))
+ appendLine(input.schemaDescription.take(input.maxSchemaLength))Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt
around lines 180 to 183, the schemaDescription is being naively truncated to
2000 characters which can cut table/column entries mid-definition; update the
code to use a configurable limit supplied via SqlRevisionInput (add a
maxSchemaChars:Int with a sensible default), or implement intelligent truncation
that slices whole table blocks (split by table/column delimiters and include
until limit) and/or prioritizes tables referenced in the failed SQL, and then
replace the hard-coded 2000 with the chosen max before appending the schema so
the LLM receives complete table/column contexts.
| private fun buildRelevantSchemaDescription( | ||
| schema: DatabaseSchema, | ||
| linkingResult: SchemaLinkingResult | ||
| ): String { | ||
| val relevantTables = schema.tables.filter { it.name in linkingResult.relevantTables } | ||
| return buildString { | ||
| appendLine("## Database Schema (USE ONLY THESE TABLES)") | ||
| appendLine() | ||
| relevantTables.forEach { table -> | ||
| appendLine("Table: ${table.name}") | ||
| appendLine("Columns: ${table.columns.joinToString(", ") { "${it.name} (${it.type})" }}") | ||
| appendLine() | ||
| } | ||
| } |
There was a problem hiding this comment.
Make schema description lookup case-insensitive to match schema linker behavior.
buildRelevantSchemaDescription filters tables with:
val relevantTables = schema.tables.filter { it.name in linkingResult.relevantTables }but LlmSchemaLinker validates tables using equals(..., ignoreCase = true) and returns the LLM’s original casing. If the LLM returns users but the schema table is Users, this filter yields an empty list and the generated schema description becomes blank even though the linker found relevant tables.
Either:
- Normalize
linkingResult.relevantTablesto schema casing in the linker (preferred), or - Change this filter to be case‑insensitive, e.g.:
val relevantTableNames = linkingResult.relevantTables.map { it.lowercase() }.toSet()
val relevantTables = schema.tables.filter { it.name.lowercase() in relevantTableNames }so the prompt always includes the intended tables.
🤖 Prompt for AI Agents
In mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt
around lines 214 to 227, the current filter compares table names with
linkingResult.relevantTables using a case-sensitive check which can miss matches
when the LLM returns different casing; update the logic to perform a
case-insensitive match by building a set of lowercased relevant table names from
linkingResult.relevantTables and then filter schema.tables by checking
table.name.lowercase() against that set so the intended tables are included
regardless of casing.
Enforce stricter rules for LLM SQL output and extract only the first SQL statement to prevent multiple statement errors. Also, filter table metadata by current database/catalog.
KeywordSchemaLinker was extracted from SchemaLinker.kt into its own file for better modularity and code organization. Updated references to use SchemaLinker type for fallback linker.
Introduce NlpTokenizer with MyNLP support on JVM for improved Chinese segmentation and fallback regex-based tokenization on other platforms. Updates KeywordSchemaLinker to use platform-specific extraction.
Moves fallback keyword extraction logic to a new FallbackNlpTokenizer object and updates all usages. Adds JVM tests for NLP tokenization and a test script for comparison.
Delete unused SqlRevisionContext to simplify ChatDBModels and reduce code clutter.
Introduce unit tests to verify FallbackNlpTokenizer behavior and update implementation as needed.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/LlmSchemaLinker.kt (1)
96-105: Case-sensitivity issue persists (duplicate of prior feedback).The validation at lines 96-105 still retains LLM casing for
validTablesandvalidColumns. This was flagged in a prior review. The fix should map to canonical schema names.mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt (3)
111-117: Redundant non-null assertion (duplicate of prior feedback).At line 111,
generatedSqlis guaranteed non-null due to the early return at lines 105-108. The!!at line 117 is redundant.
234-234: Case-sensitive table lookup (duplicate of prior feedback).Line 234 uses case-sensitive
inoperator. IflinkingResult.relevantTablescontains"users"but the schema has"Users", no tables will match. This was flagged previously.
286-303: UseCodeFence.parseAllto handle multiple code blocks (duplicate of prior feedback).The current implementation uses
CodeFence.parsewhich returns only the first code fence. When an LLM response contains multiple blocks (e.g., explanation followed by SQL), this may miss the actual SQL block. TheSqlReviseAgentcorrectly usesCodeFence.parseAllwith filtering.
🧹 Nitpick comments (7)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/FallbackNlpTokenizer.kt (1)
439-459: Consider making the dictionary extensible.The hardcoded dictionary (~100 words) is reasonable for a fallback but limits segmentation quality for domain-specific terms. The comment on line 441 already notes this could be loaded from external resources.
For future extensibility:
internal object ChineseSegmenter { private val baseDictionary = setOf(/* current words */) private val customDictionary = mutableSetOf<String>() val dictionary: Set<String> get() = baseDictionary + customDictionary fun addCustomWords(words: Collection<String>) { customDictionary.addAll(words) } // ... }mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/KeywordSchemaLinker.kt (1)
69-120: Consider deduplicating column scoring logic for clarityBoth
calculateTableRelevanceandcalculateColumnRelevanceembed very similar column-name scoring rules; extracting a shared helper for column scoring would reduce duplication and keep future tuning of weights in a single place.mpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizerTest.kt (1)
11-98: NLP tokenizer JVM tests provide good coverageThe test suite exercises Chinese-only, mixed, and English-only queries plus fallback comparison and several realistic DB-oriented prompts; structure and assertions look solid. You might eventually trim the
printlndiagnostics to keep test logs cleaner, but that’s optional.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/DatabaseContentSchemaLinker.kt (1)
156-164: Consider using a more robust JSON extraction pattern.The regex
\{[^{}]*\}works for flat JSON but will fail if the LLM includes nested objects. Since the current expected format is flat, this should work, but consider aligning withLlmSchemaLinker.extractJsonFromResponsefor consistency if that implementation differs.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/LlmSchemaLinker.kt (1)
185-192: Remove unused methodbuildSchemaDescriptionSync.This synchronous variant is defined but never called. If it's intended for future use, consider removing it to avoid dead code accumulation.
- /** - * Build schema description without sample data (synchronous version) - */ - private fun buildSchemaDescriptionSync(schema: DatabaseSchema): String { - return schema.tables.joinToString("\n\n") { table -> - val columns = table.columns.joinToString(", ") { col -> - "${col.name} (${col.type}${if (col.isPrimaryKey) ", PK" else ""})" - } - "Table: ${table.name}\nColumns: $columns" - } - }mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt (2)
44-50: Fallback chain may cause confusion.The schema linker initialization creates
LlmSchemaLinkeras the fallback forDatabaseContentSchemaLinker. However,LlmSchemaLinkeralready hasKeywordSchemaLinkeras its fallback. This creates a 3-level chain:DatabaseContentSchemaLinker → LlmSchemaLinker → KeywordSchemaLinker. While functional, consider documenting this chain or simplifying if the layered fallbacks add latency without proportional benefit.
353-361: Apply sameCodeFence.parseAllfix for PlotDSL extraction.Similar to
extractSqlFromResponse, this usesCodeFence.parsewhich may miss the PlotDSL block if the LLM includes multiple code fences. UseCodeFence.parseAllwith filtering for consistency.- val codeFence = CodeFence.parse(response) - if (codeFence.languageId.lowercase() == "plotdsl" && codeFence.text.isNotBlank()) { - return codeFence.text.trim() - } + val codeFences = CodeFence.parseAll(response) + val plotFence = codeFences.find { it.languageId.lowercase() == "plotdsl" && it.text.isNotBlank() } + if (plotFence != null) { + return plotFence.text.trim() + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
mpp-core/build.gradle.kts(1 hunks)mpp-core/src/androidMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.android.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBModels.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/DatabaseContentSchemaLinker.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/FallbackNlpTokenizer.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/KeywordSchemaLinker.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/LlmSchemaLinker.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/SchemaLinker.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/database/DatabaseConnection.kt(1 hunks)mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/chatdb/FallbackNlpTokenizerTest.kt(1 hunks)mpp-core/src/iosMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.ios.kt(1 hunks)mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.js.kt(1 hunks)mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt(1 hunks)mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.jvm.kt(1 hunks)mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/database/ExposedDatabaseConnection.kt(3 hunks)mpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizerTest.kt(1 hunks)mpp-core/src/wasmJsMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.wasmJs.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBModels.kt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualpattern for platform-specific code (for example:Platformimplementations)
Use concrete classes (not interfaces) andPromise(notFlow) for @JsExport annotations
Avoid emoji and UTF-8 characters in WASM code
When modifyingCodingAgentRenderer, update ALL implementations:DefaultCodingAgentRenderer,ComposeRenderer,JewelRenderer,ServerSideRenderer, andJsRendererAdapterin Kotlin
When modifyingCodingAgentRenderer, update JVM CLI implementations:CodingCliRendererandConsoleRenderer
UseAutoDevColorsorMaterialTheme.colorSchemefor Compose styling
Files:
mpp-core/src/androidMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.android.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/database/DatabaseConnection.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.ktmpp-core/src/iosMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.ios.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.jvm.ktmpp-core/src/commonTest/kotlin/cc/unitmesh/agent/chatdb/FallbackNlpTokenizerTest.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/database/ExposedDatabaseConnection.ktmpp-core/src/jsMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.js.ktmpp-core/src/wasmJsMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.wasmJs.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/DatabaseContentSchemaLinker.ktmpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizerTest.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/KeywordSchemaLinker.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/FallbackNlpTokenizer.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/LlmSchemaLinker.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/SchemaLinker.kt
**/*.{kt,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never use hardcoded colors - always use design tokens from the design system
Files:
mpp-core/src/androidMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.android.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/database/DatabaseConnection.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.ktmpp-core/src/iosMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.ios.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.jvm.ktmpp-core/src/commonTest/kotlin/cc/unitmesh/agent/chatdb/FallbackNlpTokenizerTest.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/database/ExposedDatabaseConnection.ktmpp-core/src/jsMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.js.ktmpp-core/src/wasmJsMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.wasmJs.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/DatabaseContentSchemaLinker.ktmpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizerTest.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/KeywordSchemaLinker.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/FallbackNlpTokenizer.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/LlmSchemaLinker.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/SchemaLinker.kt
🧠 Learnings (3)
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : Use `expect`/`actual` pattern for platform-specific code (for example: `Platform` implementations)
Applied to files:
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : Avoid emoji and UTF-8 characters in WASM code
Applied to files:
mpp-core/src/wasmJsMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.wasmJs.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update JVM CLI implementations: `CodingCliRenderer` and `ConsoleRenderer`
Applied to files:
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt
🧬 Code graph analysis (6)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/database/DatabaseConnection.kt (1)
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/database/ExposedDatabaseConnection.kt (1)
executeQuery(19-55)
mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/chatdb/FallbackNlpTokenizerTest.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/test/DevInsTestCase.kt (2)
assertEquals(45-49)assertTrue(29-33)
mpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizerTest.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/test/DevInsTestCase.kt (1)
assertTrue(29-33)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/KeywordSchemaLinker.kt (4)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.kt (1)
extractKeywords(18-18)mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.jvm.kt (1)
extractKeywords(37-50)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/LlmSchemaLinker.kt (1)
extractKeywords(130-141)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/SchemaLinker.kt (1)
extractKeywords(24-24)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/LlmSchemaLinker.kt (3)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/KeywordSchemaLinker.kt (1)
extractKeywords(62-64)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/DatabaseContentSchemaLinker.kt (1)
extractKeywords(152-154)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/SchemaLinker.kt (1)
extractKeywords(24-24)
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt (2)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/BaseAgentExecutor.kt (1)
getLLMResponse(49-85)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt (1)
extractSqlFromResponse(235-245)
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: Test mpp-core (KMP)
🔇 Additional comments (17)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.kt (1)
1-19: LGTM!The expect declaration correctly follows the
expect/actualpattern for platform-specific code. The documentation clearly explains the platform behavior differences. Based on learnings, this is the correct approach for Kotlin Multiplatform.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/FallbackNlpTokenizer.kt (2)
1-17: Well-structured fallback tokenizer implementation.The implementation correctly combines multiple NLP techniques (Porter Stemmer, BiMM, RAKE) with proper documentation and algorithm references. This provides reasonable cross-platform keyword extraction without heavy dependencies.
211-433: Porter Stemmer implementation is correct.The implementation follows the standard Porter Stemming Algorithm with proper boundary checks (e.g., line 406's
j >= 0check before array access). The code is well-organized with clear step separation.mpp-core/src/wasmJsMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.wasmJs.kt (1)
1-20: LGTM with UTF-8 consideration.The implementation correctly delegates to
FallbackNlpTokenizer. Note that per coding guidelines, UTF-8 characters should be used cautiously in WASM code. The Chinese characters are inFallbackNlpTokenizer(commonMain) as runtime data strings, which should work correctly, but verify Chinese text processing behaves as expected in WASM targets during testing.Based on learnings about avoiding UTF-8 characters in WASM code, verify Chinese keyword extraction works correctly in the WASM target.
mpp-core/build.gradle.kts (1)
215-217: Dependency placement is correct.The MyNLP dependency is properly placed in jvmMain since it's a JVM-only library. Version 4.0.0 is the latest stable release on Maven Central.
mpp-core/src/androidMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.android.kt (1)
3-21: AndroidNlpTokenizeractual correctly delegates to fallbackThe Android actual cleanly reuses
FallbackNlpTokenizerand keeps behavior aligned with other platforms; no issues spotted.mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.js.kt (1)
3-18: JSNlpTokenizeractual is consistent and minimalDelegation to
FallbackNlpTokenizerkeeps JS behavior simple and consistent with other non-JVM targets; implementation looks good.mpp-core/src/iosMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.ios.kt (1)
3-21: iOSNlpTokenizeractual is correctly wiredThe iOS implementation delegates to the shared fallback tokenizer and documents future enhancement paths; no functional issues detected.
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/NlpTokenizer.jvm.kt (1)
8-83: JVMNlpTokenizerhas robust init and fallback behaviorLazy lexer initialization, guarded MyNLP usage, and clean fallback to
FallbackNlpTokenizermake this implementation resilient; the mixed Chinese/English handling aligns with the JVM tests.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/SchemaLinker.kt (1)
14-66:SchemaLinkerbase and shared utilities look solidThe abstract contract, shared
STOP_WORDS, and Levenshtein/fuzzy-match helpers are well factored for reuse by concrete linkers; no correctness issues identified here.mpp-core/src/commonTest/kotlin/cc/unitmesh/agent/chatdb/FallbackNlpTokenizerTest.kt (1)
16-375: ComprehensiveFallbackNlpTokenizertest suiteThis file provides thorough coverage of the tokenizer’s responsibilities (stemming, segmentation, RAKE keywords, identifier formats, edge cases, and performance sanity), which should greatly help prevent regressions; no blocking issues spotted.
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/KeywordSchemaLinker.kt (1)
62-64: Fix unresolved references toSTOP_WORDSandfuzzyMatch
STOP_WORDSandfuzzyMatchlive onSchemaLinker's companion, so the current unqualified calls will not compile from this subclass.Use the class-qualified names (or import from the companion) to make these references valid:
- override suspend fun extractKeywords(query: String): List<String> { - return NlpTokenizer.extractKeywords(query, STOP_WORDS) - } + override suspend fun extractKeywords(query: String): List<String> { + return NlpTokenizer.extractKeywords(query, SchemaLinker.STOP_WORDS) + } @@ - // Fuzzy match (Levenshtein distance) - else if (fuzzyMatch(tableName, keyword)) { + // Fuzzy match (Levenshtein distance) + else if (SchemaLinker.fuzzyMatch(tableName, keyword)) { score += 0.3 } @@ - else if (fuzzyMatch(colName, keyword)) score += 0.3 + else if (SchemaLinker.fuzzyMatch(colName, keyword)) score += 0.3Alternatively, add imports at the top of the file:
import cc.unitmesh.agent.chatdb.SchemaLinker.Companion.STOP_WORDS import cc.unitmesh.agent.chatdb.SchemaLinker.Companion.fuzzyMatchAlso applies to: 74-99, 107-117
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/DatabaseContentSchemaLinker.kt (2)
1-27: LGTM!The class structure and fallback pattern are well-designed. The use of sample data for enriched schema linking aligns with the RSL-SQL research approach mentioned in the documentation.
72-106: LGTM!The system table filtering and sample data retrieval are well-implemented with appropriate error handling for cases where tables might not exist.
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/LlmSchemaLinker.kt (1)
23-27: LGTM!The constructor design with optional database connection and configurable fallback linker provides good flexibility for different usage scenarios.
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt (2)
157-199: LGTM!The execution retry loop with SQL revision attempts is well-structured. The retry logic appropriately backs off when revision doesn't produce different SQL, preventing infinite loops.
368-410: LGTM!The result building and continuation message override are straightforward and complete.
| val validTables = result.tables.filter { tableName -> | ||
| userTables.any { it.name.equals(tableName, ignoreCase = true) } | ||
| } |
There was a problem hiding this comment.
Normalize table names to schema casing after validation.
The validation uses case-insensitive matching but retains the LLM's original casing in validTables. If the LLM returns "users" but the schema table is "Users", downstream code using case-sensitive comparisons (e.g., buildRelevantSchemaDescription in ChatDBAgentExecutor) will fail to match.
Apply this diff to map to canonical schema names:
- val validTables = result.tables.filter { tableName ->
- userTables.any { it.name.equals(tableName, ignoreCase = true) }
- }
+ val validTables = userTables
+ .filter { schemaTable -> result.tables.any { it.equals(schemaTable.name, ignoreCase = true) } }
+ .map { it.name }📝 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.
| val validTables = result.tables.filter { tableName -> | |
| userTables.any { it.name.equals(tableName, ignoreCase = true) } | |
| } | |
| val validTables = userTables | |
| .filter { schemaTable -> result.tables.any { it.equals(schemaTable.name, ignoreCase = true) } } | |
| .map { it.name } |
🤖 Prompt for AI Agents
In
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/DatabaseContentSchemaLinker.kt
around lines 130 to 132, the code filters tables case-insensitively but keeps
the LLM-provided casing; change the logic so after filtering you map each
matched tableName to the canonical schema name from userTables (use a
case-insensitive comparison to find the userTables entry and take its .name),
replacing validTables with the list of canonical names; ensure the mapping
preserves only matched names and handles duplicates consistently.
| suspend fun getSampleRows(tableName: String, limit: Int = 3): QueryResult { | ||
| return try { | ||
| executeQuery("SELECT * FROM `$tableName` LIMIT $limit") | ||
| } catch (e: Exception) { | ||
| QueryResult(emptyList(), emptyList(), 0) | ||
| } | ||
| } |
There was a problem hiding this comment.
SQL dialect incompatibility and potential injection risk.
-
Dialect issue: Backtick quoting (
`tableName`) is MySQL-specific. PostgreSQL uses double quotes, and SQL Server uses square brackets. SinceDatabaseConfigsupports multiple dialects, this will fail on non-MySQL databases. -
Security: Table names should be validated against the schema to prevent injection. Consider using
getSchema().getTable(tableName)to verify the table exists before querying.
Consider abstracting dialect-specific quoting:
- suspend fun getSampleRows(tableName: String, limit: Int = 3): QueryResult {
+ suspend fun getSampleRows(tableName: String, limit: Int = 3): QueryResult {
return try {
- executeQuery("SELECT * FROM `$tableName` LIMIT $limit")
+ // Validate table exists in schema first
+ val schema = getSchema()
+ if (schema.getTable(tableName) == null) {
+ return QueryResult(emptyList(), emptyList(), 0)
+ }
+ executeQuery("SELECT * FROM $tableName LIMIT $limit")
} catch (e: Exception) {
QueryResult(emptyList(), emptyList(), 0)
}
}Note: Full dialect support would require passing dialect info or using a query builder.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/database/DatabaseConnection.kt
around lines 62-68, the code interpolates the table name using MySQL backticks
which is dialect-specific and risks SQL injection; instead, first validate the
table exists via getSchema().getTable(tableName) (or throw/return an empty
QueryResult if not found), then construct the identifier safely using the
configured dialect quoting helper (e.g., a
DatabaseConfig.quoteIdentifier(tableName) abstraction or a small switch on
DatabaseConfig.dialect to use backticks, double quotes, or square brackets), do
not accept arbitrary characters (validate against a safe identifier pattern if
no quote helper exists), and only then call executeQuery with the properly
quoted identifier; preserve the existing catch and return behavior on error.
| suspend fun getDistinctValues(tableName: String, columnName: String, limit: Int = 10): List<String> { | ||
| return try { | ||
| val result = executeQuery("SELECT DISTINCT `$columnName` FROM `$tableName` LIMIT $limit") | ||
| result.rows.map { it.firstOrNull() ?: "" }.filter { it.isNotEmpty() } | ||
| } catch (e: Exception) { | ||
| emptyList() | ||
| } | ||
| } |
There was a problem hiding this comment.
Same dialect and injection concerns apply here.
Both tableName and columnName are user-controllable and directly interpolated into SQL. Additionally, the silent catch block makes debugging difficult when queries fail unexpectedly.
Consider validating both parameters against the schema:
suspend fun getDistinctValues(tableName: String, columnName: String, limit: Int = 10): List<String> {
return try {
- val result = executeQuery("SELECT DISTINCT `$columnName` FROM `$tableName` LIMIT $limit")
+ val table = getSchema().getTable(tableName)
+ if (table == null || table.columns.none { it.name.equals(columnName, ignoreCase = true) }) {
+ return emptyList()
+ }
+ val result = executeQuery("SELECT DISTINCT $columnName FROM $tableName LIMIT $limit")
result.rows.map { it.firstOrNull() ?: "" }.filter { it.isNotEmpty() }
} catch (e: Exception) {
+ // Consider logging the exception for debugging
emptyList()
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/database/DatabaseConnection.kt
around lines 78 to 85, the code interpolates user-controlled tableName and
columnName into SQL (SQL injection risk) and swallows exceptions silently;
validate tableName and columnName against the known schema or a whitelist before
using them (identifiers cannot be parameterized), build the SQL only after
validation, avoid direct string interpolation of untrusted values, and replace
the empty catch with logging of the exception (and optionally rethrow or return
a clear error/result) so failures are visible for debugging.
| // Get current database/catalog name to filter tables | ||
| val currentCatalog = connection.catalog | ||
|
|
||
| // Get tables only from current database (catalog) | ||
| val tableTypes = arrayOf("TABLE", "VIEW") | ||
| val tableRs = metadata.getTables(null, null, "%", tableTypes) | ||
| val tableRs = metadata.getTables(currentCatalog, null, "%", tableTypes) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the file exists and read the getSchema method
find . -path "*/mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/database/ExposedDatabaseConnection.kt" -type fRepository: phodal/auto-dev
Length of output: 144
🏁 Script executed:
# Read the file to see the current implementation
cat -n mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/database/ExposedDatabaseConnection.ktRepository: phodal/auto-dev
Length of output: 8293
🏁 Script executed:
# Search for tests related to ExposedDatabaseConnection
find . -path "*/test/**" -name "*.kt" -type f | xargs grep -l "ExposedDatabaseConnection" 2>/dev/nullRepository: phodal/auto-dev
Length of output: 43
🏁 Script executed:
# Look for database-specific test configurations
find . -path "*/test/**" -name "*.kt" -type f | xargs grep -l "SQLite\|PostgreSQL\|MySQL\|Oracle" 2>/dev/null | head -20Repository: phodal/auto-dev
Length of output: 177
🏁 Script executed:
# Search more broadly for database-related tests
find . -path "*/test/**" -name "*Database*" -o -path "*/test/**" -name "*Connection*" | grep -v ".git"Repository: phodal/auto-dev
Length of output: 41
🏁 Script executed:
# Look for any test files in test directories
find . -path "*/test/**" -name "*.kt" -type f | head -20Repository: phodal/auto-dev
Length of output: 1896
🏁 Script executed:
# Check DatabaseConfig implementation
find . -name "DatabaseConfig.kt" -type fRepository: phodal/auto-dev
Length of output: 41
🏁 Script executed:
# Search for null handling patterns in the codebase with catalog or connection.catalog
rg -n "connection\.catalog" --type=ktRepository: phodal/auto-dev
Length of output: 85
🏁 Script executed:
# Look for JDBC metadata usage patterns in the codebase
rg -n "getTables\|getPrimaryKeys\|getColumns" --type=kt -B2 -A2Repository: phodal/auto-dev
Length of output: 85
🏁 Script executed:
# Search for connection.catalog with correct rg syntax
rg "connection\.catalog" -A2 -B2Repository: phodal/auto-dev
Length of output: 673
🏁 Script executed:
# Find DatabaseConfig
find . -name "*atabaseConfig*" -type fRepository: phodal/auto-dev
Length of output: 41
🏁 Script executed:
# Search for getTables, getPrimaryKeys, getColumns with correct syntax
rg "getTables|getPrimaryKeys|getColumns" -A2 -B2Repository: phodal/auto-dev
Length of output: 9675
🏁 Script executed:
# Search for any catalog-related initialization or configuration
rg "catalog" -iRepository: phodal/auto-dev
Length of output: 3302
🏁 Script executed:
# Search for any test or documentation about database connection setup
find . -name "*.md" -o -name "*.txt" | xargs grep -l "database\|catalog\|ExposedDatabaseConnection" 2>/dev/nullRepository: phodal/auto-dev
Length of output: 222
🏁 Script executed:
# Check if there's any DatabaseConfig or database initialization code
find . -name "*.kt" | xargs grep -l "DatabaseConfig" 2>/dev/null | head -10Repository: phodal/auto-dev
Length of output: 908
🏁 Script executed:
# Look for any tests or setup code that creates database connections
rg "ExposedDatabaseConnection.create|createDatabaseConnection" -A5Repository: phodal/auto-dev
Length of output: 7654
🏁 Script executed:
# Check if there are integration tests or examples
find . -path "*/example*" -o -path "*/sample*" -o -path "*/demo*" | head -20Repository: phodal/auto-dev
Length of output: 733
🏁 Script executed:
# Read the test file to see what scenarios are tested
cat -n mpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/database/DatabaseConnectionTest.ktRepository: phodal/auto-dev
Length of output: 7523
🏁 Script executed:
# Read the example file to understand usage patterns
cat -n mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/database/DatabaseConnectionExample.ktRepository: phodal/auto-dev
Length of output: 2071
🏁 Script executed:
# Check for any documentation about database support
cat -n mpp-core/README.md | head -100Repository: phodal/auto-dev
Length of output: 3239
Add null handling for connection.catalog and expand test coverage for different database types.
The code assumes connection.catalog is always non-null, but JDBC drivers can return null for databases like SQLite or when the catalog isn't explicitly configured. When null is passed to getTables(), getPrimaryKeys(), and getColumns(), driver behavior is inconsistent—some drivers interpret null as "all catalogs," which defeats the stated intent to filter by current database.
There are no integration tests verifying catalog-scoped metadata retrieval across the supported database types (MySQL, PostgreSQL, SQLite, Oracle, SQL Server, MariaDB). Add a null check with a fallback strategy:
val currentCatalog = connection.catalog?.takeIf { it.isNotEmpty() }Alternatively, add database-specific logic using metadata.databaseProductName to handle edge cases. Then add integration tests covering at least MySQL, PostgreSQL, and SQLite to verify the actual filtering behavior.
🤖 Prompt for AI Agents
In
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/database/ExposedDatabaseConnection.kt
around lines 63 to 68, the code assumes connection.catalog is non-null which can
cause inconsistent behavior across JDBC drivers; add null/empty handling and
fallback logic so we do not pass null to metadata methods. Replace the direct
use of connection.catalog with a safe value (e.g., connection.catalog?.takeIf {
it.isNotEmpty() } or an empty/placeholder string) and/or branch on
metadata.databaseProductName for vendor-specific behavior, and ensure the same
guarded value is used when calling metadata.getTables(), getPrimaryKeys(), and
getColumns(). Finally, add integration tests for MySQL, PostgreSQL, and SQLite
to assert that catalog-scoped metadata retrieval behaves as expected under each
driver.
Introduce an inline configuration pane for adding and editing data sources, replacing the previous dialog-based approach. The pane appears on the right side and supports saving and connecting actions.
Replaces JSqlParserValidator with platform-specific SqlValidator implementations and moves ChatDBAgent classes to commonMain for improved multiplatform support.
Deleted unused DataSourceConfigDialog to clean up codebase.
Implement cross-platform DataSourceRepository for saving, loading, and managing database connection configurations. Refactor ChatDBViewModel to use repository for persistence. Update DataSourceConfigPane for improved UI.
Deleted the SchemaInfoDialog composable as it is no longer used in ChatDBPage.
Render successful query results as assistant messages to support proper markdown table rendering in the timeline.
Render all agent progress updates and final results directly to the chat timeline via the renderer, providing clearer feedback to users during query execution. Removes redundant result rendering from the view model.
Add detailed rendering for schema, linking, SQL generation, validation, execution, and errors in LLM responses for improved clarity and debugging.
Introduce ChatDBStepItem and ChatDBStepCard for displaying database query execution steps with expandable details and status badges in the timeline. Refactor agent executor and renderer to support step-wise progress updates.
Introduce a "New Session" button that clears the current chat timeline. Also include generated SQL in query result details.
Display detailed table schemas and query result previews in ChatDB steps, including expandable table cards, column info, keyword chips, and data preview tables.
Introduce MultiDatabaseChatDBAgent and executor, update UI and models to handle multiple database sources in chatdb agent workflow.
Enhance MultiDatabaseChatDBExecutor with SQL validation, automatic revision on errors, execution retries, and optional PlotDSL visualization generation for query results.
Integrate PlotDSLAgent to generate chart visualizations for SQL query results. Improve table string formatting with row limits and truncation.
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (11)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt (1)
23-23:llmServiceparameter is unused.The
llmServiceparameter is passed toChatDBPagebut never used. TheChatDBViewModellikely creates its own LLM service instance internally. Either remove the parameter or pass it to the ViewModel to ensure consistent configuration.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/database/DatabaseConnection.kt (2)
62-68: SQL dialect incompatibility and potential injection risk.Backtick quoting is MySQL-specific and will fail on PostgreSQL (uses double quotes) or SQL Server (uses square brackets). Additionally,
tableNameis directly interpolated without validation. Validate against the schema and use dialect-aware quoting.
78-85: Same dialect and injection concerns apply here.Both
tableNameandcolumnNameare user-controllable and directly interpolated. The silent catch also makes debugging difficult.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.kt (1)
279-283: Consider revising the first suggestion to be more useful.The suggestion "Show me all tables in the database" may be redundant since schema information is already available via the connection banner. A more data-focused suggestion would be more helpful.
val suggestions = listOf( - "Show me all tables in the database", + "Show me the first 10 rows from the ${schema.tables.firstOrNull()?.name ?: "users"} table", "What are the columns in the ${schema.tables.firstOrNull()?.name ?: "users"} table?", "Count the total number of records" )mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt (3)
35-35: Compose state updates should occur on the main dispatcher.This was previously flagged. The
scopeusesDispatchers.Default, but the ViewModel modifies ComposemutableStateOfproperties (state,isGenerating) from coroutines launched on this scope. Compose state should be updated from the main thread.- private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default) + private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Main)
507-511:dispose()cancels scope beforedisconnectAll()completes.This was previously flagged.
disconnectAll()launches a coroutine onscope, butscope.cancel()is called immediately after, potentially preventing the disconnect coroutine from completing and leaving connections open.fun dispose() { stopGeneration() - disconnectAll() - scope.cancel() + // Close connections synchronously to ensure cleanup + connections.forEach { (id, connection) -> + try { + runBlocking { connection.close() } + } catch (e: Exception) { + println("[ChatDB] Error closing connection for $id: ${e.message}") + } + } + connections.clear() + schemas.clear() + scope.cancel() }
319-340: Connection not closed whenisConnected()returns false.If
createDatabaseConnectionsucceeds butisConnected()returns false, the connection object is not closed, potentially leaking resources.try { val connection = createDatabaseConnection(dataSource.toDatabaseConfig()) if (connection.isConnected()) { connections[id] = connection schemas[id] = connection.getSchema() state = state.copy( connectionStatuses = state.connectionStatuses + (id to ConnectionStatus.Connected), connectionStatus = if (state.hasAnyConnection || true) ConnectionStatus.Connected else state.connectionStatus ) _notificationEvent.emit("Connected" to "Successfully connected to ${dataSource.name}") } else { + try { connection.close() } catch (_: Exception) {} state = state.copy( connectionStatuses = state.connectionStatuses + (id to ConnectionStatus.Error("Failed to connect")) ) }mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt (3)
169-183: Avoid hard-coded schema truncation; make it configurable or smarter.
buildRevisionContexttruncatesschemaDescriptionwith a fixed 2000-character limit:appendLine(input.schemaDescription.take(2000))This can cut table/column definitions mid-block, potentially hiding key columns or relationships from the LLM and hurting revision quality.
Suggested directions:
- Make the limit part of
SqlRevisionInput(e.g.,maxSchemaLength: Int = 2000) and use that instead of a magic number.- Or implement a simple boundary-aware truncation (e.g., split schema into table blocks and include whole blocks until the limit).
Example minimal change:
data class SqlRevisionInput( ... - val maxAttempts: Int = 3 + val maxAttempts: Int = 3, + val maxSchemaLength: Int = 2000 )and:
- appendLine(input.schemaDescription.take(2000)) + appendLine(input.schemaDescription.take(input.maxSchemaLength))This keeps prompts bounded while preserving complete table contexts.
20-44: Align schema definition withSqlRevisionInputbehavior.
SqlReviseAgentSchemamarksschemaDescriptionasrequired = true, but:
validateInputtreats it as optional (schemaDescription = input["schemaDescription"] as? String ?: "").SqlRevisionInputprovides a defaultschemaDescription: String = "".This mismatch can confuse tool consumers and schema-based tooling.
Consider updating the schema to reflect optionality:
- "schemaDescription" to string( - description = "Database schema description for context", - required = true - ), + "schemaDescription" to string( + description = "Database schema description for context", + required = false + ),(Optionally also add a
previousAttemptsproperty to the schema if you expect it ininput.)Also applies to: 70-81, 265-272
86-101: Remove emoji from progress messages in commonMain for WASM targets.Progress strings use emoji markers (
🔄,📝,❌,✅,⚠️) inexecute, and this file lives in commonMain, so it will be compiled for WASM. Guidelines say to avoid emoji/UTF‑8 in WASM code.Recommend replacing them with textual prefixes, e.g.:
- onProgress("🔄 SQL Revise Agent - Starting revision") + onProgress("[INFO] SQL Revise Agent - Starting revision") ... - onProgress("📝 Revision attempt $attemptCount/${input.maxAttempts}") + onProgress("[ATTEMPT] Revision attempt $attemptCount/${input.maxAttempts}") ... - onProgress("❌ Failed to generate revised SQL") + onProgress("[ERROR] Failed to generate revised SQL") ... - onProgress("✅ SQL validated successfully") + onProgress("[SUCCESS] SQL validated successfully") ... - onProgress("⚠️ Validation failed: ${currentError.take(50)}...") + onProgress("[WARN] Validation failed: ${currentError.take(50)}...") ... - onProgress("✅ SQL revised (no validation)") + onProgress("[SUCCESS] SQL revised (no validation)") ... - onProgress("❌ Max revision attempts reached") + onProgress("[ERROR] Max revision attempts reached")This keeps semantics while complying with platform constraints.
Also applies to: 109-111, 127-128, 139-140, 143-144, 155-158
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt (1)
509-526: Make SQL extraction robust by usingCodeFence.parseAll(align withSqlReviseAgent).Current implementation only inspects a single code fence via
CodeFence.parse, so if the LLM returns multiple blocks (e.g., explanation + SQL), you may miss the actual SQL and fall through to brittle regexes.SqlReviseAgent.extractSqlFromResponsealready usesCodeFence.parseAlland selects thesqlfence, which is more reliable.Consider mirroring that logic here:
- private fun extractSqlFromResponse(response: String): String? { - val codeFence = CodeFence.parse(response) - if (codeFence.languageId.lowercase() == "sql" && codeFence.text.isNotBlank()) { - return extractFirstStatement(codeFence.text.trim()) - } - - // Try to find SQL block manually - val sqlPattern = Regex("```sql\\s*([\\s\\S]*?)```", RegexOption.IGNORE_CASE) - val match = sqlPattern.find(response) - if (match != null) { - return extractFirstStatement(match.groupValues[1].trim()) - } - - // Last resort: look for SELECT statement - val selectPattern = Regex("(SELECT[\\s\\S]*?;)", RegexOption.IGNORE_CASE) - val selectMatch = selectPattern.find(response) - return selectMatch?.groupValues?.get(1)?.trim() - } + private fun extractSqlFromResponse(response: String): String? { + val codeFences = CodeFence.parseAll(response) + val sqlFence = codeFences.find { it.languageId.lowercase() == "sql" && it.text.isNotBlank() } + if (sqlFence != null) { + return extractFirstStatement(sqlFence.text.trim()) + } + + // Try to find SQL block manually + val sqlPattern = Regex("```sql\\s*([\\s\\S]*?)```", RegexOption.IGNORE_CASE) + val match = sqlPattern.find(response) + if (match != null) { + return extractFirstStatement(match.groupValues[1].trim()) + } + + // Last resort: look for SELECT statement + val selectPattern = Regex("(SELECT[\\s\\S]*?;)", RegexOption.IGNORE_CASE) + val selectMatch = selectPattern.find(response) + return selectMatch?.groupValues?.get(1)?.trim() + }This keeps the existing regex fallbacks but makes the primary path consistent with the revision agent.
🟡 Minor comments (12)
mpp-ui/src/commonMain/sqldelight/cc/unitmesh/devins/db/DataSource.sq-35-39 (1)
35-39: Potential race condition inclearDefault+setDefaultsequence.When setting a new default, the caller must execute
clearDefaultfollowed bysetDefaultas two separate operations. Without a transaction wrapper, concurrent calls could leave the database in an inconsistent state (e.g., multiple defaults or no default).Consider documenting that callers must wrap these in a transaction, or providing a combined query:
setDefaultAtomic: UPDATE DataSource SET isDefault = CASE WHEN id = ? THEN 1 ELSE 0 END WHERE isDefault = 1 OR id = ?;mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/RendererModels.kt-248-257 (1)
248-257: Emoji characters iniconfield may cause issues on WASM platform.Per coding guidelines, emoji and UTF-8 characters should be avoided in WASM code. Consider using icon identifiers (strings or enum references) that map to platform-specific icon resources instead of embedding emoji directly.
-enum class ChatDBStepType(val displayName: String, val icon: String) { - FETCH_SCHEMA("Fetch Database Schema", "📊"), - SCHEMA_LINKING("Schema Linking", "🔗"), +enum class ChatDBStepType(val displayName: String, val iconId: String) { + FETCH_SCHEMA("Fetch Database Schema", "chart"), + SCHEMA_LINKING("Schema Linking", "link"), // ... map iconId to actual icons in UI layerCommittable suggestion skipped: line range outside the PR's diff.
mpp-ui/src/iosMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.ios.kt-71-81 (1)
71-81: Thread-safety concern in singleton initialization.The
getInstance()method is not thread-safe. If called concurrently from multiple threads, it could create multipleDataSourceRepositoryinstances and database connections.Consider using a thread-safe initialization pattern:
actual companion object { - private var instance: DataSourceRepository? = null + @Volatile + private var instance: DataSourceRepository? = null + private val lock = Any() actual fun getInstance(): DataSourceRepository { - return instance ?: run { - val driverFactory = DatabaseDriverFactory() - val database = createDatabase(driverFactory) - DataSourceRepository(database).also { instance = it } + return instance ?: synchronized(lock) { + instance ?: run { + val driverFactory = DatabaseDriverFactory() + val database = createDatabase(driverFactory) + DataSourceRepository(database).also { instance = it } + } } } }mpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.jvm.kt-49-52 (1)
49-52:setDefaultis not atomic — potential race condition.The
clearDefault()andsetDefault(id)calls are not wrapped in a transaction. If another thread reads between these calls, it may see no default data source. Consider using a transaction.actual fun setDefault(id: String) { - queries.clearDefault() - queries.setDefault(id) + queries.transaction { + queries.clearDefault() + queries.setDefault(id) + } }mpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.jvm.kt-71-82 (1)
71-82: Double-checked locking without@Volatilemay cause visibility issues.The
instancefield should be marked@Volatileto ensure proper visibility across threads in the double-checked locking pattern.actual companion object { - private var instance: DataSourceRepository? = null + @Volatile + private var instance: DataSourceRepository? = null actual fun getInstance(): DataSourceRepository {mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/database/DatabaseConnection.kt-186-190 (1)
186-190: Truncation logic may produce incorrect output whencolWidths[idx]is less than 4.If
colWidths[idx]is less than 4,str.take(colWidths[idx] - 3)takes negative or zero characters, producing "..." for any non-empty string. Consider adding a minimum width guard.- val displayStr = if (str.length > colWidths[idx]) { - str.take(colWidths[idx] - 3) + "..." + val colWidth = colWidths[idx] + val displayStr = if (str.length > colWidth && colWidth >= 4) { + str.take(colWidth - 3) + "..." + } else if (str.length > colWidth) { + str.take(colWidth) } else { str }mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt-62-68 (1)
62-68: Potential stale state access inonToggleDataSourcecallback.The lambda captures
statewhich is read at composition time. When the callback executes,state.dataSourcesandstate.isConfigPaneOpenmay be stale. Consider accessing state directly from the ViewModel or using a method that encapsulates this logic.onToggleDataSource = { id -> viewModel.toggleDataSource(id) - // When toggling a data source, optionally show its config in the pane - val selected = state.dataSources.find { it.id == id } - if (selected != null && state.isConfigPaneOpen) { - viewModel.openConfigPane(selected) - } + // Move this logic into ViewModel.toggleDataSource() to avoid stale state },Committable suggestion skipped: line range outside the PR's diff.
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/MultiDatabaseChatDBExecutor.kt-756-777 (1)
756-777: Potential bug in fallback SQL handling.At line 771, when
cleanSqlis blank, the code falls back to the originalsqlwhich still contains the-- database:comment. This comment could cause issues with some database dialects or parsers.} else if (databaseConnections.size == 1) { // If only one database, use it as default val defaultDb = databaseConnections.keys.first() - blocks.add(SqlBlock(defaultDb, cleanSql.ifBlank { sql })) + blocks.add(SqlBlock(defaultDb, if (cleanSql.isNotBlank()) cleanSql else sql.trim())) }Or better, if
cleanSqlis blank, consider skipping the block entirely since it likely means there's no actual SQL:} else if (databaseConnections.size == 1 && cleanSql.isNotBlank()) { val defaultDb = databaseConnections.keys.first() blocks.add(SqlBlock(defaultDb, cleanSql)) }mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/MultiDatabaseChatDBExecutor.kt-798-821 (1)
798-821: Column mismatch between databases not handled.When combining results from multiple databases, the code assumes all results have the same columns (line 812-814 only uses columns from the first result). If queries return different schemas, the combined result will have misaligned data.
Consider validating column consistency or handling mismatches:
private fun combineResults(results: Map<String, QueryResult>): QueryResult { if (results.isEmpty()) { return QueryResult(emptyList(), emptyList(), 0) } if (results.size == 1) { return results.values.first() } // For multiple results, add a "database" column and combine val allColumns = mutableListOf("_database") val allRows = mutableListOf<List<String>>() + var referenceColumns: List<String>? = null for ((dbName, result) in results) { - if (allColumns.size == 1) { + if (referenceColumns == null) { allColumns.addAll(result.columns) + referenceColumns = result.columns + } else if (result.columns != referenceColumns) { + // Log warning about column mismatch + logger.warn { "Column mismatch for $dbName: expected $referenceColumns, got ${result.columns}" } } for (row in result.rows) { - allRows.add(listOf(dbName) + row) + // Pad or truncate row to match expected column count + val paddedRow = row.take(referenceColumns?.size ?: row.size) + .toMutableList() + .apply { while (size < (referenceColumns?.size ?: 0)) add("") } + allRows.add(listOf(dbName) + paddedRow) } } return QueryResult(allColumns, allRows, allRows.size) }Committable suggestion skipped: line range outside the PR's diff.
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBStepCard.kt-596-596 (1)
596-596: Avoid emoji characters in commonMain code per coding guidelines.The coding guidelines state to "Avoid emoji and UTF-8 characters in WASM code." Since this is in
commonMain, it will be compiled for WASM targets where emoji rendering may be problematic.Consider replacing emoji with icon components:
- Text(text = "🗄️", fontSize = 12.sp) + Icon( + AutoDevComposeIcons.Database, + contentDescription = null, + modifier = Modifier.size(12.dp), + tint = MaterialTheme.colorScheme.onSurfaceVariant + )Apply similar changes to lines 690-692 (📋), 762 (🔑), and 765 (🔗).
Committable suggestion skipped: line range outside the PR's diff.
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/MultiDatabaseChatDBAgent.kt-149-155 (1)
149-155:close()should handle exceptions to ensure all connections are closed.If one connection's
close()throws, the loop exits and remaining connections stay open. Wrap each close in a try-catch.suspend fun close() { - databaseConnections.values.forEach { it.close() } + databaseConnections.values.forEach { connection -> + try { + connection.close() + } catch (e: Exception) { + logger.error { "Error closing database connection: ${e.message}" } + } + } databaseConnections.clear() }Committable suggestion skipped: line range outside the PR's diff.
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgent.kt-142-148 (1)
142-148: Add exception handling inclose().If
close()throws,databaseConnectionwon't be set to null, leaving the agent in an inconsistent state.suspend fun close() { - databaseConnection?.close() - databaseConnection = null + try { + databaseConnection?.close() + } catch (e: Exception) { + logger.error { "Error closing database connection: ${e.message}" } + } finally { + databaseConnection = null + } }Committable suggestion skipped: line range outside the PR's diff.
🧹 Nitpick comments (24)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentMessageList.kt (1)
240-244: Consider importingChatDBStepCardinstead of using fully qualified name.The fully qualified path
cc.unitmesh.devins.ui.compose.agent.chatdb.components.ChatDBStepCardworks but is verbose. Adding an import would improve readability.+import cc.unitmesh.devins.ui.compose.agent.chatdb.components.ChatDBStepCard // ... in the when block: is TimelineItem.ChatDBStepItem -> { - cc.unitmesh.devins.ui.compose.agent.chatdb.components.ChatDBStepCard( + ChatDBStepCard( step = timelineItem ) }mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/RendererModels.kt (1)
220-225: Consider usingMap<String, String>instead ofMap<String, Any>fordetails.
Map<String, Any>can cause serialization issues in multiplatform contexts and makes the API contract unclear. If structured data is needed, consider a sealed class orMap<String, String>with JSON-encoded values for complex types.data class ChatDBStepItem( val stepType: ChatDBStepType, val status: ChatDBStepStatus, val title: String, - val details: Map<String, Any> = emptyMap(), + val details: Map<String, String> = emptyMap(), val error: String? = null,mpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidatorTest.kt (1)
167-175: Strengthen assertion for multiple statements test.The test only verifies the result is not null, which doesn't validate the actual behavior. Consider asserting on specific expected behavior (e.g., whether both statements are validated, or if only the first is parsed).
@Test fun testMultipleStatements() { - // JSqlParser may handle this differently val sql = "SELECT * FROM users; SELECT * FROM orders" - val result = validator.validate(sql) - // Just verify it doesn't crash - behavior may vary - assertNotNull(result) + // Document expected behavior: JSqlParser typically validates only the first statement + // or may return an error for multiple statements depending on configuration + assertNotNull(result) + // Consider: assertTrue(result.isValid) or assertFalse depending on expected behavior }mpp-ui/src/jsMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.js.kt (1)
10-37: Inconsistent warning behavior across methods.Only
getAll()logs a warning viaconsole.warn. For consistency, either log warnings in all stub methods or remove the warning fromgetAll(). Silent no-ops can be confusing for developers debugging why data isn't persisting.Consider a consistent approach:
actual class DataSourceRepository { + private fun logNotImplemented(method: String) { + console.warn("DataSourceRepository.$method not implemented for JS platform") + } + actual fun getAll(): List<DataSourceConfig> { - console.warn("DataSourceRepository not implemented for JS platform") + logNotImplemented("getAll") return emptyList() } actual fun save(config: DataSourceConfig) { - // No-op + logNotImplemented("save") } // ... apply to other methodsmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.kt (1)
4-4: Remove unused import.
DatabaseDialectis imported but not used in this file.import cc.unitmesh.devins.ui.compose.agent.chatdb.model.DataSourceConfig -import cc.unitmesh.devins.ui.compose.agent.chatdb.model.DatabaseDialectmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigPane.kt (1)
51-64: Consider adding port range validation.The current validation only checks for required fields. Port numbers should be validated to be within the valid range (1-65535).
- val isValid = name.isNotBlank() && database.isNotBlank() && - (dialect == DatabaseDialect.SQLITE || host.isNotBlank()) + val portNum = port.toIntOrNull() + val isValidPort = portNum != null && portNum in 1..65535 + val isValid = name.isNotBlank() && database.isNotBlank() && + (dialect == DatabaseDialect.SQLITE || (host.isNotBlank() && isValidPort))mpp-core/src/iosMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.ios.kt (1)
49-83: Regex-based table extraction has known limitations.The patterns work for simple cases but won't handle:
- Schema-qualified tables (
schema.table)- Quoted identifiers (
"table name"or`table`)- Subqueries in FROM clause
- Table aliases with AS keyword
This is acceptable given the documented limitation that non-JVM platforms use basic regex-based extraction.
Consider improving the FROM pattern to handle schema-qualified names:
- val fromPattern = Regex("""FROM\s+(\w+)""", RegexOption.IGNORE_CASE) + val fromPattern = Regex("""FROM\s+(?:(\w+)\.)?(\w+)""", RegexOption.IGNORE_CASE) fromPattern.findAll(sql).forEach { match -> - tables.add(match.groupValues[1]) + val tableName = match.groupValues[2].ifEmpty { match.groupValues[1] } + tables.add(tableName) }mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt (1)
28-29: ViewModel not keyed onworkspace— won't recreate if workspace changes.If
workspacechanges after initial composition, theChatDBViewModelwon't be recreated becauserememberhas no key. Consider usingremember(workspace)to ensure the ViewModel is recreated when the workspace changes.- val viewModel = remember { ChatDBViewModel(workspace) } + val viewModel = remember(workspace) { ChatDBViewModel(workspace) }mpp-core/src/androidMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.android.kt (2)
85-123: Duplicate implementation with JVM variant — consider extracting to shared code.The
extractErrorMessageandcollectWarningsmethods are nearly identical to the JVM implementation inSqlValidator.jvm.kt. Since both platforms use JSqlParser, consider extracting these private helpers into a shared JVM/Android module (e.g.,jvmAndroidMain) to reduce duplication and maintenance burden.
109-123: String-based SQL analysis is fragile.Using
sql.contains("SELECT *")and similar checks can produce false positives when these patterns appear in string literals or comments. Consider using JSqlParser's AST visitor pattern for more accurate detection.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/database/DatabaseConnection.kt (1)
157-157: Unused variabletotalWidth.
totalWidthis calculated but never used. Remove it to avoid confusion.- val totalWidth = colWidths.sumOf { it + 3 } + 1 // +3 for " │ " per column, +1 for final │ - val sb = StringBuilder()mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.kt (1)
278-307: Add timeout and cancel mechanism for database connection attempts.When a data source is in the
Connectingstate, users see a progress indicator but have no way to cancel if the connection hangs. Industry best practices recommend 5–15 second timeouts for mobile and 15–30 seconds for desktop applications. Consider implementing a timeout (e.g., usingwithTimeoutOrNullin the coroutine) and allowing users to tap the spinner to cancel the connection attempt—this can reuse the existingdisconnectDataSourcecallback or introduce a newonCancelConnectcallback.mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.jvm.kt (2)
153-169: Warning detection uses naive string matching which may produce false positives.The
collectWarningsfunction usesstatement.toString()and then checks withcontains(). This approach has issues:
SELECT *check may match strings like'SELECT * is disabled'in string literals- The UPDATE/DELETE without WHERE check may trigger on subqueries or CTEs that legitimately don't have WHERE
Consider using JSqlParser's visitor pattern for more accurate detection:
private fun collectWarnings(statement: Statement): List<String> { val warnings = mutableListOf<String>() - - // Check for common issues that aren't errors but might be problematic - val sql = statement.toString() - - if (sql.contains("SELECT *")) { - warnings.add("Consider specifying explicit columns instead of SELECT *") - } - - if (!sql.contains("WHERE", ignoreCase = true) && - (sql.contains("UPDATE", ignoreCase = true) || sql.contains("DELETE", ignoreCase = true))) { - warnings.add("UPDATE/DELETE without WHERE clause will affect all rows") + + when (statement) { + is net.sf.jsqlparser.statement.select.Select -> { + // Check for SELECT * using PlainSelect + val selectBody = statement.selectBody + if (selectBody is net.sf.jsqlparser.statement.select.PlainSelect) { + if (selectBody.selectItems.any { it.toString() == "*" }) { + warnings.add("Consider specifying explicit columns instead of SELECT *") + } + } + } + is net.sf.jsqlparser.statement.update.Update -> { + if (statement.where == null) { + warnings.add("UPDATE without WHERE clause will affect all rows") + } + } + is net.sf.jsqlparser.statement.delete.Delete -> { + if (statement.where == null) { + warnings.add("DELETE without WHERE clause will affect all rows") + } + } } return warnings }
124-148: Duplicate code across JVM and Android implementations.The
extractErrorMessageandcollectWarningsfunctions are nearly identical to those inSqlValidator.android.kt(lines 84-122). Consider extracting shared logic to a common module to reduce maintenance burden. You could:
- Create a
SqlValidatorHelperobject in commonMain with shared utilities- Or use a delegation pattern where platform implementations delegate to shared code
Based on the coding guidelines recommending
expect/actualpatterns, the shared validation logic could be in commonMain.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/MultiDatabaseChatDBExecutor.kt (2)
529-543: Silent error swallowing may mask connection issues.When fetching schemas, exceptions are logged but the database is silently skipped. This could lead to confusing behavior where a user expects queries against a database that failed silently.
Consider propagating partial failure information to the caller:
private suspend fun fetchAndMergeSchemas(): MergedDatabaseSchema { val schemas = mutableMapOf<String, DatabaseSchema>() + val failedDatabases = mutableListOf<String>() for ((dbId, connection) in databaseConnections) { try { val schema = connection.getSchema() schemas[dbId] = schema logger.info { "Fetched schema for $dbId: ${schema.tables.size} tables" } } catch (e: Exception) { logger.error(e) { "Failed to fetch schema for $dbId" } + failedDatabases.add(dbId) } } + if (failedDatabases.isNotEmpty()) { + logger.warn { "Failed to fetch schemas for: ${failedDatabases.joinToString()}" } + } + return MergedDatabaseSchema(schemas) }Or render a warning step in the UI to inform the user.
380-425: Retry logic structure is confusing and could be simplified.The placement of
executionRetries++at line 424 after the revision logic makes the flow hard to follow. The conditionexecutionRetries < maxExecutionRetries - 1at line 381 requires mental gymnastics to verify correctness.Consider restructuring for clarity:
- // Try to revise SQL based on execution error - if (executionRetries < maxExecutionRetries - 1) { + executionRetries++ + + // Try to revise SQL based on execution error (except on last attempt) + if (executionRetries < maxExecutionRetries) { onProgress("🔄 Attempting to fix SQL based on execution error...") // ... revision logic ... } - executionRetries++This makes it clearer that we increment first, then decide whether to revise based on remaining attempts.
mpp-core/src/wasmJsMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.wasmJs.kt (1)
49-83: Table name extraction patterns are too simplistic.The regex patterns don't handle common SQL constructs:
- Schema-qualified names:
SELECT * FROM schema.userswon't extract correctly- Quoted identifiers:
"table-name"or`table name`won't match- The patterns may match keywords in subqueries or string literals
Consider improving the patterns:
actual override fun extractTableNames(sql: String): List<String> { val tables = mutableListOf<String>() - // Match FROM clause - val fromPattern = Regex("""FROM\s+(\w+)""", RegexOption.IGNORE_CASE) + // Match FROM clause (handles schema.table and quoted identifiers) + val fromPattern = Regex("""FROM\s+(?:\[?(\w+)\]?\.)?[\["`]?(\w+)[\]"`]?""", RegexOption.IGNORE_CASE) fromPattern.findAll(sql).forEach { match -> - tables.add(match.groupValues[1]) + val tableName = match.groupValues[2].ifEmpty { match.groupValues[1] } + if (tableName.isNotEmpty()) tables.add(tableName) }Given the limitations documented in the
expectclass ("regex-based extraction which may be less accurate"), this is acceptable for a lightweight WASM implementation, but consider adding a note about known limitations.mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.kt (1)
23-23: Remove unused import.
Platformis imported but not used in this file.-import cc.unitmesh.agent.Platformmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBStepCard.kt (1)
176-177: Consider type-safe step details to eliminate unchecked casts.The numerous
@Suppress("UNCHECKED_CAST")annotations indicate a design opportunity. Consider creating sealed classes or data classes for step details:sealed class ChatDBStepDetails { data class FetchSchema( val databaseName: String? = null, val totalTables: Int = 0, val databases: List<DatabaseInfo> = emptyList(), val tableSchemas: List<TableSchemaInfo> = emptyList() ) : ChatDBStepDetails() data class SchemaLinking( val databasesAnalyzed: List<String> = emptyList(), val keywords: List<String> = emptyList(), val relevantTableSchemas: List<TableSchemaInfo> = emptyList() ) : ChatDBStepDetails() // ... other step types }This would make the code more type-safe and self-documenting. However, this would require changes to the renderer API, so it's a larger refactor.
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/MultiDatabaseChatDBAgent.kt (2)
1-9: Unused import:DatabaseSchemaThe import
DatabaseSchemaon line 7 is not used in this file.import cc.unitmesh.agent.database.DatabaseConfig import cc.unitmesh.agent.database.DatabaseConnection -import cc.unitmesh.agent.database.DatabaseSchema import cc.unitmesh.agent.database.createDatabaseConnection
139-141:buildSystemPrompt()is trivial and could be inlined.The method simply returns
MULTI_DB_SYSTEM_PROMPT. Consider removing the method and using the constant directly inexecute(), or keeping it if you plan to add dynamic schema information later.mpp-ui/src/wasmJsMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.kt (1)
72-87: Silent failure insave()- consider propagating errors.The
save()method catches exceptions and only logs them. The caller has no way to know if the save succeeded. Consider throwing or returning a result type so the UI can notify users of save failures.- actual fun save(config: DataSourceConfig) { + actual fun save(config: DataSourceConfig): Boolean { try { val storage = loadStorage() val stored = config.toStoredDataSource() val existing = storage.dataSources.indexOfFirst { it.id == config.id } val updatedList = if (existing >= 0) { storage.dataSources.toMutableList().apply { set(existing, stored) } } else { storage.dataSources + stored } saveStorage(DataSourceStorage(updatedList)) console.log("WASM: Data source saved: ${config.id}") + return true } catch (e: Exception) { console.error("WASM: Error saving data source: ${e.message}") + return false } }mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt (1)
489-495:getSchema()only returns first schema in multi-datasource mode.The comment acknowledges this limitation. For multi-datasource support, consider returning a combined schema or deprecating this method in favor of
getAllSchemas().mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/model/DataSourceModels.kt (1)
57-66: Clarify port validation message for non-SQLite dialects.Validation skips host/port checks when
dialect == DatabaseDialect.SQLITE, but the error message says “Port must be between 1 and 65535” without that caveat.To avoid confusion, you could clarify the message:
- if (port <= 0 || port > 65535) errors.add("Port must be between 1 and 65535") + if (port <= 0 || port > 65535) { + errors.add("Port must be between 1 and 65535 for non-SQLite databases") + }This better reflects the special-case handling for SQLite.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
mpp-core/src/androidMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.android.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgent.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/MultiDatabaseChatDBAgent.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/MultiDatabaseChatDBExecutor.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/database/DatabaseConnection.kt(3 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/CodingAgentRenderer.kt(2 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/RendererModels.kt(3 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.kt(1 hunks)mpp-core/src/iosMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.ios.kt(1 hunks)mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.js.kt(1 hunks)mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.jvm.kt(1 hunks)mpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidatorTest.kt(1 hunks)mpp-core/src/wasmJsMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.wasmJs.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentMessageList.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.kt(5 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBStepCard.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigPane.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.kt(1 hunks)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/model/DataSourceModels.kt(1 hunks)mpp-ui/src/commonMain/sqldelight/cc/unitmesh/devins/db/DataSource.sq(1 hunks)mpp-ui/src/iosMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.ios.kt(1 hunks)mpp-ui/src/jsMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.js.kt(1 hunks)mpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.jvm.kt(1 hunks)mpp-ui/src/wasmJsMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualpattern for platform-specific code (for example:Platformimplementations)
Use concrete classes (not interfaces) andPromise(notFlow) for @JsExport annotations
Avoid emoji and UTF-8 characters in WASM code
When modifyingCodingAgentRenderer, update ALL implementations:DefaultCodingAgentRenderer,ComposeRenderer,JewelRenderer,ServerSideRenderer, andJsRendererAdapterin Kotlin
When modifyingCodingAgentRenderer, update JVM CLI implementations:CodingCliRendererandConsoleRenderer
UseAutoDevColorsorMaterialTheme.colorSchemefor Compose styling
Files:
mpp-core/src/iosMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.ios.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigPane.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentMessageList.ktmpp-ui/src/iosMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.ios.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBStepCard.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/MultiDatabaseChatDBExecutor.ktmpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidatorTest.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.jvm.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/database/DatabaseConnection.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgent.ktmpp-ui/src/jsMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.js.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/RendererModels.ktmpp-core/src/wasmJsMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.wasmJs.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.ktmpp-ui/src/wasmJsMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.ktmpp-core/src/androidMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.android.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/CodingAgentRenderer.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.ktmpp-core/src/jsMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.js.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/MultiDatabaseChatDBAgent.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/model/DataSourceModels.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.jvm.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.kt
**/*.{kt,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never use hardcoded colors - always use design tokens from the design system
Files:
mpp-core/src/iosMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.ios.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigPane.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentMessageList.ktmpp-ui/src/iosMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.ios.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBStepCard.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/MultiDatabaseChatDBExecutor.ktmpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidatorTest.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.jvm.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/database/DatabaseConnection.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgent.ktmpp-ui/src/jsMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.js.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/RendererModels.ktmpp-core/src/wasmJsMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.wasmJs.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.ktmpp-ui/src/wasmJsMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.ktmpp-core/src/androidMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.android.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/CodingAgentRenderer.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.ktmpp-core/src/jsMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.js.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/MultiDatabaseChatDBAgent.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/model/DataSourceModels.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.jvm.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.kt
🧠 Learnings (6)
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update ALL implementations: `DefaultCodingAgentRenderer`, `ComposeRenderer`, `JewelRenderer`, `ServerSideRenderer`, and `JsRendererAdapter` in Kotlin
Applied to files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentMessageList.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/RendererModels.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/CodingAgentRenderer.ktmpp-core/src/jsMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.js.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : Use `expect`/`actual` pattern for platform-specific code (for example: `Platform` implementations)
Applied to files:
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update JVM CLI implementations: `CodingCliRenderer` and `ConsoleRenderer`
Applied to files:
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/RendererModels.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/CodingAgentRenderer.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to mpp-vscode/src/**/*.ts : When modifying `CodingAgentRenderer`, update VSCode implementations: `mpp-vscode/src/bridge/mpp-core.ts` and `mpp-vscode/src/providers/chat-view.ts`
Applied to files:
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/CodingAgentRenderer.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.ts : When modifying `CodingAgentRenderer`, update ALL TypeScript implementations: `BaseRenderer.ts`, `CliRenderer.ts`, `ServerRenderer.ts`, and `TuiRenderer.ts`
Applied to files:
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/CodingAgentRenderer.kt
📚 Learning: 2025-12-07T08:40:10.850Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.850Z
Learning: Applies to **/*.kt : Avoid emoji and UTF-8 characters in WASM code
Applied to files:
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt
🧬 Code graph analysis (17)
mpp-core/src/iosMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.ios.kt (4)
mpp-core/src/wasmJsMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.wasmJs.kt (4)
performBasicValidation(85-138)validate(9-139)validate(11-20)extractTableNames(49-83)mpp-core/src/androidMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.android.kt (3)
validate(17-124)validate(19-34)extractTableNames(75-83)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.kt (3)
validate(20-51)validate(27-27)extractTableNames(50-50)mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.jvm.kt (3)
validate(17-184)validate(19-34)extractTableNames(85-93)
mpp-ui/src/iosMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.ios.kt (1)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/db/DatabaseDriverFactory.kt (1)
createDatabase(44-97)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/MultiDatabaseChatDBExecutor.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/BaseAgentExecutor.kt (1)
getLLMResponse(49-85)
mpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/subagent/JSqlParserValidatorTest.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/test/DevInsTestCase.kt (4)
assertTrue(29-33)assertFalse(38-40)assertNotNull(54-59)assertNull(64-68)
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.jvm.kt (1)
mpp-core/src/androidMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.android.kt (2)
collectWarnings(109-123)extractErrorMessage(85-107)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/database/DatabaseConnection.kt (1)
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/database/ExposedDatabaseConnection.kt (1)
executeQuery(19-55)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgent.kt (1)
mpp-vscode/src/bridge/mpp-core.ts (1)
ToolRegistry(226-306)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/RendererModels.kt (3)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStep.kt (1)
generateId(116-118)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/AgentPlan.kt (1)
generateId(135-137)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanTask.kt (1)
generateId(151-153)
mpp-core/src/wasmJsMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.wasmJs.kt (5)
mpp-core/src/iosMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.ios.kt (4)
performBasicValidation(85-138)validate(9-139)validate(11-20)extractTableNames(49-83)mpp-core/src/androidMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.android.kt (3)
validate(17-124)validate(19-34)extractTableNames(75-83)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt (3)
validate(277-281)validate(278-278)extractTableNames(280-280)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.kt (3)
validate(20-51)validate(27-27)extractTableNames(50-50)mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.jvm.kt (3)
validate(17-184)validate(19-34)extractTableNames(85-93)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/database/DatabaseConnection.kt (1)
createDatabaseConnection(379-379)
mpp-core/src/androidMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.android.kt (1)
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/subagent/SqlValidator.jvm.kt (2)
collectWarnings(153-169)extractErrorMessage(124-148)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.kt (2)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentMessageList.kt (1)
AgentMessageList(31-158)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt (1)
sendMessage(397-469)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt (4)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/base/ResizableSplitPane.kt (1)
ResizableSplitPane(38-186)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.kt (1)
DataSourcePanel(26-97)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigPane.kt (1)
DataSourceConfigPane(27-296)mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.kt (1)
ChatDBChatPane(35-91)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt (2)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/schema/ToolSchema.kt (2)
string(136-155)integer(157-170)mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/test/AstFormatter.kt (1)
appendLine(206-209)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/MultiDatabaseChatDBAgent.kt (1)
mpp-vscode/src/bridge/mpp-core.ts (1)
ToolRegistry(226-306)
mpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.jvm.kt (1)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/db/DatabaseDriverFactory.kt (1)
createDatabase(44-97)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt (2)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/BaseAgentExecutor.kt (1)
getLLMResponse(49-85)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt (1)
extractSqlFromResponse(235-245)
⏰ 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). (2)
- GitHub Check: Test mpp-core (KMP)
- GitHub Check: Build
| private val executor: ChatDBAgentExecutor by lazy { | ||
| val connection = databaseConnection ?: createDatabaseConnection(databaseConfig) | ||
| databaseConnection = connection | ||
|
|
||
| ChatDBAgentExecutor( | ||
| projectPath = projectPath, | ||
| llmService = llmService, | ||
| toolOrchestrator = toolOrchestrator, | ||
| renderer = renderer, | ||
| databaseConnection = connection, | ||
| maxIterations = maxIterations, | ||
| enableLLMStreaming = enableLLMStreaming | ||
| ) | ||
| } |
There was a problem hiding this comment.
Consider adding error handling for connection creation.
Similar to MultiDatabaseChatDBAgent, the lazy initialization of executor creates a database connection without error handling. If createDatabaseConnection fails, the agent becomes unusable.
private val executor: ChatDBAgentExecutor by lazy {
- val connection = databaseConnection ?: createDatabaseConnection(databaseConfig)
- databaseConnection = connection
+ val connection = databaseConnection ?: try {
+ createDatabaseConnection(databaseConfig)
+ } catch (e: Exception) {
+ logger.error { "Failed to create database connection: ${e.message}" }
+ throw IllegalStateException("Cannot create database connection", e)
+ }
+ databaseConnection = connection
ChatDBAgentExecutor(🤖 Prompt for AI Agents
In mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgent.kt around
lines 84 to 97, the lazy-initialized executor calls createDatabaseConnection
without handling failures; wrap the creation in a try/catch, log or surface the
error with context, and fail fast (rethrow or throw a descriptive custom
exception) so the agent does not silently become unusable; ensure
databaseConnection is not set when creation fails and preserve the original
exception as cause for easier debugging.
| onProgress("📊 Fetching database schema...") | ||
|
|
There was a problem hiding this comment.
Remove emoji from commonMain strings for WASM compatibility.
This commonMain file is compiled for WASM, but multiple user-facing strings use emoji characters (progress messages and headings like “✅/❌/📊/📈”). Project guidelines say to avoid emoji/UTF‑8 in WASM code; these should be replaced with plain-text prefixes (e.g., [INFO], [SUCCESS], [ERROR]) or similar.
Recommend systematically replacing emoji in:
onProgressmessages (schema fetching, schema linking, SQL generation, validation/revision, execution, visualization).buildResultheadings (“## ✅ Query Executed Successfully”, “## ❌ Query Failed”).
This keeps behavior while complying with platform constraints.
Also applies to: 103-104, 138-139, 200-201, 235-236, 271-272, 288-289, 306-307, 339-340, 369-370, 601-602, 637-642
🤖 Prompt for AI Agents
In
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/chatdb/ChatDBAgentExecutor.kt
around lines 80-81 and also at 103-104, 138-139, 200-201, 235-236, 271-272,
288-289, 306-307, 339-340, 369-370, 601-602 and 637-642, remove emoji characters
from all user-facing strings and replace them with plain-text prefixes for WASM
compatibility (e.g., replace "📊", "📈", "✅", "❌" in onProgress calls and
buildResult headings with "[INFO]", "[PROGRESS]", "[SUCCESS]", "[ERROR]" or
similar); update each onProgress message and the buildResult headings ("## ✅
Query Executed Successfully", "## ❌ Query Failed") to use the chosen plain-text
tokens while preserving the original message text and semantics.
| private val executor: MultiDatabaseChatDBExecutor by lazy { | ||
| // Create connections for all configured databases | ||
| databaseConfigs.forEach { (id, config) -> | ||
| if (!databaseConnections.containsKey(id)) { | ||
| databaseConnections[id] = createDatabaseConnection(config) | ||
| } | ||
| } | ||
|
|
||
| MultiDatabaseChatDBExecutor( | ||
| projectPath = projectPath, | ||
| llmService = llmService, | ||
| toolOrchestrator = toolOrchestrator, | ||
| renderer = renderer, | ||
| databaseConnections = databaseConnections, | ||
| databaseConfigs = databaseConfigs, | ||
| maxIterations = maxIterations, | ||
| enableLLMStreaming = enableLLMStreaming | ||
| ) | ||
| } |
There was a problem hiding this comment.
Lazy initialization creates connections without error handling.
If createDatabaseConnection throws for any config, the lazy initialization fails partway, leaving databaseConnections in an inconsistent state. Consider adding error handling or connecting in parallel with proper exception isolation.
private val executor: MultiDatabaseChatDBExecutor by lazy {
// Create connections for all configured databases
databaseConfigs.forEach { (id, config) ->
if (!databaseConnections.containsKey(id)) {
- databaseConnections[id] = createDatabaseConnection(config)
+ try {
+ databaseConnections[id] = createDatabaseConnection(config)
+ } catch (e: Exception) {
+ logger.error { "Failed to connect to database $id: ${e.message}" }
+ // Continue with other databases
+ }
}
}
+
+ if (databaseConnections.isEmpty()) {
+ throw IllegalStateException("Failed to connect to any configured database")
+ }
MultiDatabaseChatDBExecutor(Committable suggestion skipped: line range outside the PR's diff.
| /** | ||
| * Render a ChatDB execution step. | ||
| * This is an optional method primarily used by UI renderers (ComposeRenderer, JewelRenderer). | ||
| * Console renderers can ignore this or provide simple text output. | ||
| * | ||
| * @param stepType The type of step being executed | ||
| * @param status The current status of the step | ||
| * @param title The display title for the step (defaults to stepType.displayName) | ||
| * @param details Additional details about the step (e.g., table names, row counts, SQL) | ||
| * @param error Error message if the step failed | ||
| */ | ||
| fun renderChatDBStep( | ||
| stepType: ChatDBStepType, | ||
| status: ChatDBStepStatus, | ||
| title: String = stepType.displayName, | ||
| details: Map<String, Any> = emptyMap(), | ||
| error: String? = null | ||
| ) { | ||
| // Default: no-op for renderers that don't support ChatDB steps | ||
| } |
There was a problem hiding this comment.
Missing imports for ChatDBStepType and ChatDBStepStatus.
The method references ChatDBStepType and ChatDBStepStatus but these types are not imported. This will cause compilation errors.
Add the missing imports at the top of the file:
package cc.unitmesh.agent.render
import cc.unitmesh.agent.plan.PlanSummaryData
import cc.unitmesh.agent.tool.ToolResult
import cc.unitmesh.llm.compression.TokenInfo
+import cc.unitmesh.agent.render.ChatDBStepType
+import cc.unitmesh.agent.render.ChatDBStepStatusCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/CodingAgentRenderer.kt
around lines 78 to 97, the renderChatDBStep method references ChatDBStepType and
ChatDBStepStatus but they are not imported; add the correct import statements
for ChatDBStepType and ChatDBStepStatus at the top of the file (import their
package where those types are defined) so the file compiles, then re-run the
build to verify no unused import warnings remain.
| class SqlReviseAgent( | ||
| private val llmService: KoogLLMService, | ||
| private val sqlValidator: SqlValidatorInterface? = null | ||
| ) : SubAgent<SqlRevisionInput, ToolResult.AgentResult>( |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find SqlReviseAgent instantiations to check if validator is always provided
rg -n "SqlReviseAgent\(" mpp-core -S -C2Repository: phodal/auto-dev
Length of output: 1976
🏁 Script executed:
# Examine the SqlReviseAgent implementation to verify the validation logic
cat -n mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt | head -160Repository: phodal/auto-dev
Length of output: 7765
Make the SQL validator mandatory to prevent unvalidated query execution.
The SqlReviseAgent accepts a nullable sqlValidator parameter with a default value of null. When the validator is null (lines 141-151), the agent returns revised SQL immediately without any validation, which violates the security goal of using AST-based validation to block destructive queries like DROP, DELETE, or TRUNCATE.
While current instantiation sites in ChatDBAgentExecutor and MultiDatabaseChatDBExecutor do provide a SqlValidator, the API design itself permits the dangerous null path and should be fixed.
Recommended fix: Remove the nullable default and make the validator mandatory:
class SqlReviseAgent(
private val llmService: KoogLLMService,
private val sqlValidator: SqlValidatorInterface
) : SubAgent<SqlRevisionInput, ToolResult.AgentResult>(This ensures validation always occurs and eliminates the unintended code path at lines 141-151 that returns unvalidated SQL.
🤖 Prompt for AI Agents
In mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/SqlReviseAgent.kt
around lines 62-65 (and eliminate the null-path at ~141-151), the sqlValidator
parameter is nullable with a default of null allowing skipped validation; change
the constructor to require a non-null SqlValidatorInterface (remove the default
and nullable type) so callers must provide a validator, then remove or simplify
the conditional branch that returns revised SQL when validator is null (update
call sites if necessary) to ensure AST-based validation always runs.
| @Serializable | ||
| data class DataSourceConfig( | ||
| val id: String, | ||
| val name: String, | ||
| val dialect: DatabaseDialect = DatabaseDialect.MYSQL, | ||
| val host: String = "localhost", | ||
| val port: Int = 3306, | ||
| val database: String = "", | ||
| val username: String = "", | ||
| val password: String = "", | ||
| val description: String = "", | ||
| val isDefault: Boolean = false, | ||
| val createdAt: Long = 0L, | ||
| val updatedAt: Long = 0L | ||
| ) { |
There was a problem hiding this comment.
Avoid serializing database passwords in plain text.
DataSourceConfig is @Serializable and includes password: String, which means credentials will be persisted and possibly transmitted in clear text. This is a significant security risk (config files, sync, logs, backups, memory dumps).
Consider one of the following:
- Store only a reference/key to a secure credential store (OS keychain, encrypted storage) instead of the raw password.
- Exclude
passwordfrom the serialized form and handle it via a separate, secured channel/structure. - At minimum, clearly document the risk and ensure configs are stored in a protected location with strict permissions, and avoid logging any
DataSourceConfiginstances.
If short-term constraints force plain-text storage, treating this as technical debt with a clear roadmap to a secure store would be prudent.
Also applies to: 77-86
🤖 Prompt for AI Agents
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/model/DataSourceModels.kt
lines 29-43 (and similarly lines 77-86): the DataSourceConfig currently
serializes a plain-text password which is a security risk; change the model to
avoid persisting/transmitting raw credentials by (1) removing password from the
serialized form — mark it non-serializable (use kotlinx.serialization @Transient
or equivalent) or remove the property from the @Serializable data class, (2) add
a serialized credential reference field (e.g., credentialKey: String?) that
points to a secure store, and (3) handle the actual password only in-memory via
a separate secure mechanism (set transient password at runtime from the store);
also add a short TODO comment documenting migration to a secure credential store
and avoid logging or printing DataSourceConfig instances.
| username TEXT NOT NULL, | ||
| password TEXT NOT NULL, |
There was a problem hiding this comment.
Security: Password stored in plaintext.
Storing database passwords as plain text is a security risk. Consider encrypting the password before storage or using platform-specific secure storage mechanisms (e.g., Keychain on iOS, EncryptedSharedPreferences on Android, system credential managers on desktop).
At minimum, the password should be encrypted with a key derived from platform-specific secure storage. For a multiplatform solution, consider:
- Adding an
expect/actualencryption utility - Using SQLCipher for encrypted database storage
- Storing only a reference/alias to credentials in secure platform storage
| actual fun save(config: DataSourceConfig) { | ||
| queries.insertOrReplace( | ||
| id = config.id, | ||
| name = config.name, | ||
| dialect = config.dialect.name, | ||
| host = config.host, | ||
| port = config.port.toLong(), | ||
| databaseName = config.database, | ||
| username = config.username, | ||
| password = config.password, | ||
| description = config.description, | ||
| isDefault = if (config.isDefault) 1L else 0L, | ||
| createdAt = config.createdAt, | ||
| updatedAt = config.updatedAt | ||
| ) | ||
| } |
There was a problem hiding this comment.
Security concern: Password stored in plain text.
Database credentials are stored directly in SQLite without encryption. For enterprise security requirements mentioned in the PR objectives, consider using iOS Keychain for sensitive credential storage.
This is acceptable for an initial implementation, but for production use, you should:
- Store only a reference/key in SQLite
- Store the actual password in iOS Keychain using
Security.framework - Retrieve credentials from Keychain when building connection strings
Would you like me to open an issue to track implementing secure credential storage using iOS Keychain?
🤖 Prompt for AI Agents
mpp-ui/src/iosMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.ios.kt
around lines 24-39: the current save(...) persists the plaintext password into
SQLite; replace this by storing only a reference key in the DB and moving the
actual secret into the iOS Keychain via Security.framework: generate a stable
unique key (e.g. "datasource-{config.id}-password"), save the password to
Keychain using SecItemAdd/SecItemUpdate with appropriate accessibility and
attributes, persist that key string (not the password) in the database, on reads
retrieve the password from Keychain using SecItemCopyMatching when constructing
connection details, ensure no logging of secrets, handle Keychain errors and
migration for existing records (on save, if a plaintext password exists, write
it to Keychain and overwrite DB field with the reference, and on delete remove
the Keychain item).
| username = config.username, | ||
| password = config.password, |
There was a problem hiding this comment.
Password stored in plain text.
Database credentials including passwords are stored without encryption. This is a security concern, especially if the SQLite database file is accessible. Consider encrypting sensitive fields before storage or using a platform-specific secure storage mechanism.
| @Serializable | ||
| private data class StoredDataSource( | ||
| val id: String, | ||
| val name: String, | ||
| val dialect: String, | ||
| val host: String, | ||
| val port: Int, | ||
| val database: String, | ||
| val username: String, | ||
| val password: String, | ||
| val description: String, | ||
| val isDefault: Boolean, | ||
| val createdAt: Long, | ||
| val updatedAt: Long | ||
| ) |
There was a problem hiding this comment.
Security: Passwords stored in plain text in browser localStorage.
StoredDataSource stores password as plain text, which gets serialized to localStorage. This is a security risk as localStorage is accessible to any JavaScript on the same origin and persists across sessions.
Consider:
- Not storing passwords at all (prompt on connect)
- Using session storage instead
- Encrypting sensitive fields before storage
🤖 Prompt for AI Agents
In mpp-ui/src/wasmJsMain/kotlin/cc/unitmesh/devins/db/DataSourceRepository.kt
around lines 21 to 35, the StoredDataSource data class currently includes
password which gets serialized to localStorage — remove sensitive plaintext
storage: delete the password field from StoredDataSource and any serialization
that persists it, and instead (a) keep passwords out of persistent localStorage
by prompting the user on connect or storing ephemeral credentials only in
sessionStorage, or (b) if persistence is required, encrypt the password
client-side using Web Crypto (SubtleCrypto) with a user-derived key (e.g.,
PBKDF2) before serializing and decrypt on demand; update all code paths that
read/write StoredDataSource to use the new approach and migrate or purge any
existing plain-text entries in localStorage.
Change toTableString to generate Markdown table format for query results, replacing the previous ASCII table style. Adds escaping for special Markdown characters and updates tests accordingly.
Refactored MultiDatabaseChatDBAgent and related renderer logic to improve database handling and UI integration. Updated SQL validation and connection modules for better cross-platform support.
Refactored renderer and database connection modules for improved structure and maintainability. Updated related UI components and server-side renderer.
Render a success step when revised SQL passes dry run validation and update approval to use the latest dry run result.
Summary
This PR adds a new ChatDB agent type for text-to-SQL interactions, allowing users to connect to databases and use natural language to query them.
Changes
New Files
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBPage.kt- Main page with split layoutmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/ChatDBViewModel.kt- ViewModel for managing data sources and chatmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourcePanel.kt- Left panel for data source managementmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/DataSourceConfigDialog.kt- Dialog for adding/editing data sourcesmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/components/ChatDBChatPane.kt- Right panel for chat interactionsmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/chatdb/model/DataSourceModels.kt- Data models for data sourcesModified Files
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/AgentType.kt- Added CHAT_DB agent typempp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt- Added routing for ChatDBmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentChatInterface.kt- Added CHAT_DB to when expressionmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuDesktop.kt- Added CHAT_DB iconmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuMobile.kt- Added CHAT_DB iconmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/icons/AutoDevComposeIcons.kt- Added Database and Schema iconsFeatures
Screenshots
(UI implementation - screenshots can be added after testing)
Closes #508
Summary by CodeRabbit
Release Notes
New Features
Enhancements
Style
✏️ Tip: You can customize this high-level summary in your review settings.