Conversation
There was a problem hiding this comment.
Pull request overview
This PR refines the Docker Desktop MCP integration to make enabling Docker MCP more atomic (only persisting configuration after the MCP successfully starts), while also improving determinism and testability in a few MCP-adjacent areas.
Changes:
- Stage Docker MCP configuration in-memory, initialize the MCP client, then persist the config only after successful startup (with rollback paths).
- Make Docker MCP tool rendering deterministic by sorting map keys before rendering parameters.
- Improve MCP tool media/base64 handling and expand related test coverage; refactor Docker MCP availability checks to be test-stubbable.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/ui/model/ui.go | Switch Docker MCP enable flow to prepare → start → persist with rollback on failures. |
| internal/ui/chat/docker_mcp.go | Sort extra argument keys for stable tool rendering output. |
| internal/config/docker_mcp.go | Extract Docker MCP config creation; split enable into prepare vs persist; add injectable runner for availability checks. |
| internal/config/docker_mcp_test.go | Remove env-dependent skips via runner stubbing; add a “real docker when available” test. |
| internal/agent/tools/mcp/tools.go | Normalize/validate base64 output for MCP tool media results. |
| internal/agent/tools/mcp/tools_test.go | Add test cases for whitespace and unpadded base64 handling. |
| internal/agent/coordinator.go | Remove Coordinator.RefreshTools from interface and implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR adjusts Docker MCP enablement/presentation flows and tightens some MCP tool handling to improve UX reliability in the Docker Desktop integration.
Changes:
- Stage Docker MCP config in-memory first, initialize the MCP client, then persist config (with rollback on failure).
- Make Docker MCP tool parameter rendering deterministic by sorting extra-arg keys.
- Improve MCP tool media handling by normalizing base64 inputs and extending tests; refactor Docker MCP availability probing for testability.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/ui/model/ui.go | Enables Docker MCP using a staged config + initialize + persist sequence with rollback. |
| internal/ui/chat/docker_mcp.go | Sorts extra tool args for stable rendering output. |
| internal/config/docker_mcp.go | Introduces an overridable Docker MCP version runner + splits enablement into prepare/persist steps. |
| internal/config/docker_mcp_test.go | Makes Docker MCP tests deterministic by stubbing availability; adds an optional “real docker” test. |
| internal/agent/tools/mcp/tools.go | Normalizes base64 inputs and expands base64 validation to accept raw encoding. |
| internal/agent/tools/mcp/tools_test.go | Adds coverage for unpadded/whitespace base64 and decoding validation. |
| internal/agent/coordinator.go | Removes RefreshTools from the Coordinator interface/implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d1b2b8f to
876901b
Compare
andreynering
left a comment
There was a problem hiding this comment.
Looks good!
I made sure to test and it's still working well.
Odds and ends related to the Docker Desktop integration. The main changes here are: