feat: add support for codex cli client#8158
feat: add support for codex cli client#8158kaplan-shaked wants to merge 1 commit intoscalameta:mainfrom
Conversation
📝 WalkthroughWalkthroughIntroduces TOML-based configuration support for MCP servers alongside existing JSON handling. Adds a new Changes
Poem
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@metals/src/main/scala/scala/meta/internal/metals/mcp/McpConfig.scala`:
- Around line 57-68: The deletion uses the raw projectName while writeConfig
writes the server under the transformed name produced by
client.projectName(projectName), so TomlConfig.removeServer should be called
with the same transformed key; update the delete path to pass
client.projectName(projectName) (the same symbol used in writeConfig) into
TomlConfig.removeServer so the TOML key matches and removal succeeds, and keep
the rest of the logic (configFile.readText, writeText/delete) unchanged.
In `@metals/src/main/scala/scala/meta/internal/metals/mcp/TomlConfig.scala`:
- Around line 47-52: The URL extraction logic in
section.find(_.trim.startsWith("url =")) and the subsequent line.split("=")(1)
truncates values containing '=' and misses `url=value` style entries; update the
predicate to find lines where l.trim.startsWith("url") && l.contains("=") (or
equivalent) to allow optional whitespace, then extract the RHS by splitting only
on the first '=' (limit=2) or by taking substring after the first '='; finally
trim the result and strip surrounding quotes before normalizing (preserve query
strings) and continue parsing the port as before (e.g., using
normalizedUrl.split(":").last.toInt).
🧹 Nitpick comments (2)
metals/src/main/scala/scala/meta/internal/metals/mcp/TomlConfig.scala (1)
58-69: Consider escaping special characters in TOML string values.The current implementation wraps values in quotes without escaping. If a value contains
"or\, the generated TOML will be malformed. While current usage appears to be URLs only, this could cause issues if the API is used more broadly.♻️ Proposed fix to escape special characters
private def generateSection( tableName: String, serverName: String, properties: Map[String, String], ): List[String] = { val header = s"[$tableName.$serverName]" val props = properties.map { case (k, v) => - s"""$k = "$v"""" + val escaped = v.replace("\\", "\\\\").replace("\"", "\\\"") + s"""$k = "$escaped"""" }.toList // Add a blank line before if needed? relying on split logic to handle spacing List(header) ++ props }tests/unit/src/test/scala/tests/mcp/TomlConfigSuite.scala (1)
69-79: Consider adding edge case tests forreadPort.The current test covers the happy path. Given the URL parsing fragility noted in
TomlConfig.scala, consider adding tests for:
- URLs with query parameters containing
=- URLs without the expected suffix
- Malformed URLs
test("read-port-with-query-params") { val toml = """ |[mcpServers.my-project-metals] |url = "http://localhost:8080/mcp?key=value" """.stripMargin val port = TomlConfig.readPort(toml, "mcpServers", "my-project-metals", "/mcp") assertEquals(port, Some(8080)) } test("read-port-not-found") { val toml = """ |[mcpServers.other] |url = "http://localhost:8080/mcp" """.stripMargin val port = TomlConfig.readPort(toml, "mcpServers", "my-project-metals", "/mcp") assertEquals(port, None) }
| if (configFile.toFile.getName.endsWith(".toml")) { | ||
| val configContent = configFile.readText | ||
| val updatedConfig = TomlConfig.removeServer( | ||
| configContent, | ||
| client.serverField, | ||
| projectName, | ||
| ) | ||
| if (updatedConfig.trim.isEmpty) { | ||
| configFile.delete() | ||
| } else { | ||
| configFile.writeText(updatedConfig) | ||
| } |
There was a problem hiding this comment.
Bug: Server name mismatch in TOML deletion will cause deletion to fail.
In writeConfig (line 31), the server is written using client.projectName(projectName) which transforms the name (e.g., adds -metals suffix for default clients). However, in deleteConfig (line 62), the raw projectName is passed to TomlConfig.removeServer.
This inconsistency means deletion will look for a different key than what was written, causing silent failure to remove the server entry.
🐛 Proposed fix
if (configFile.toFile.getName.endsWith(".toml")) {
val configContent = configFile.readText
val updatedConfig = TomlConfig.removeServer(
configContent,
client.serverField,
- projectName,
+ client.projectName(projectName),
)🤖 Prompt for AI Agents
In `@metals/src/main/scala/scala/meta/internal/metals/mcp/McpConfig.scala` around
lines 57 - 68, The deletion uses the raw projectName while writeConfig writes
the server under the transformed name produced by
client.projectName(projectName), so TomlConfig.removeServer should be called
with the same transformed key; update the delete path to pass
client.projectName(projectName) (the same symbol used in writeConfig) into
TomlConfig.removeServer so the TOML key matches and removal succeeds, and keep
the rest of the logic (configFile.readText, writeText/delete) unchanged.
There was a problem hiding this comment.
This looks like a legit issue.
There was a problem hiding this comment.
@tgodzik, thank you for confirming! The mismatch between client.projectName(projectName) in the write path and raw projectName in the delete path will indeed prevent proper cleanup of TOML server entries.
If you'd like, I can help create a plan to fix this issue across the codebase, or if you prefer to address it yourself, the fix is straightforward: use client.projectName(projectName) consistently in the deleteConfig method's TOML branch as shown in the bot's proposed diff.
|
There're so many agentic tools and favouring one after another is not right, especially a commercial one. |
|
@igor-ramazanov How is it different from supporting claude? or cursor? |
|
This is just an improvement, we can add more of them. It should no be an issue. |
|
I am adding #8184 to make the configuration useful. This means that we can add really any number of clients and make it easy for the user to set up |
tgodzik
left a comment
tgodzik
left a comment
There was a problem hiding this comment.
Thanks for working on it! And sorry for being late to get back to you
| val configFile = projectPath.resolve(s"${client.settingsPath}$filename") | ||
| if (configFile.exists) { | ||
| // TOML does not support the old endpoint, so we don't need to rewrite it | ||
| if (configFile.exists && !filename.endsWith(".toml")) { |
There was a problem hiding this comment.
Might be worth extracting filename.endsWith(".toml") isTomlFile method, we have similar isScal, isJava methods
| configFile.writeText(newConfig) | ||
| val config = if (configFile.exists) configFile.readText else "" | ||
| if (filename.endsWith(".toml")) { | ||
| val url = s"http://localhost:$port${MetalsMcpServer.mcpEndpoint}" |
There was a problem hiding this comment.
Might be worth to move the whole link to MetalsMcpServer and reuse everywhere.
| if (configFile.toFile.getName.endsWith(".toml")) { | ||
| val configContent = configFile.readText | ||
| val updatedConfig = TomlConfig.removeServer( | ||
| configContent, | ||
| client.serverField, | ||
| projectName, | ||
| ) | ||
| if (updatedConfig.trim.isEmpty) { | ||
| configFile.delete() | ||
| } else { | ||
| configFile.writeText(updatedConfig) | ||
| } |
There was a problem hiding this comment.
This looks like a legit issue.
| "metals", | ||
| Map("url" -> "http://localhost:123"), | ||
| ) | ||
| assert(updated.contains("[mcpServers.metals]")) |
There was a problem hiding this comment.
Could you use assertNoDiff with the full file output for the tests?
3 participants
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.