Roblox Studio Companion Plugin support #148
Roblox Studio Companion Plugin support #148BigTows wants to merge 2 commits intoAleksandrSl:mainfrom
Conversation
|
Thank you, I'll take a look this week |
There was a problem hiding this comment.
Pull request overview
Adds support for the Roblox Studio “Luau Language Server Companion” by introducing a local HTTP server that forwards companion plugin events into the Luau LSP, plus project settings and notifications to manage it.
Changes:
- Add a project-level Companion Plugin service + startup activity that starts/stops an HTTP server based on project settings.
- Add settings (enable flag + port) and a new notification group/messages for companion plugin status/errors.
- Extend the LSP server interface to accept companion plugin notifications, and fix test resource path handling via
toURI().path.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/kotlin/com/github/aleksandrsl/intellijluau/declarations/LuauDeclarationsParserTest.kt | Use toURI().path for resource paths in tests. |
| src/main/resources/META-INF/com.github.aleksandrsl.intellijluau-lsp.xml | Register notification group + startup activity for companion service. |
| src/main/resources/messages/LuauBundle.properties | Add companion plugin notification strings. |
| src/main/kotlin/com/github/aleksandrsl/intellijluau/settings/ProjectSettingsState.kt | Persist companion plugin enablement + port in project settings. |
| src/main/kotlin/com/github/aleksandrsl/intellijluau/settings/ProjectSettingsComponent.kt | Add UI controls for enabling companion plugin and selecting port. |
| src/main/kotlin/com/github/aleksandrsl/intellijluau/settings/LuauDefaultSettingsState.kt | Include companion settings in “save as default” state. |
| src/main/kotlin/com/github/aleksandrsl/intellijluau/lsp/LuauLspServerSupportProvider.kt | Define LSP notifications ($/plugin/full, $/plugin/clear) and set lsp4jServerClass. |
| src/main/kotlin/com/github/aleksandrsl/intellijluau/lsp/CompanionServer.kt | Implement local HTTP server endpoints and LSP notification forwarding. |
| src/main/kotlin/com/github/aleksandrsl/intellijluau/lsp/CompanionPluginStartupActivity.kt | Initialize companion plugin service on project startup. |
| src/main/kotlin/com/github/aleksandrsl/intellijluau/lsp/CompanionPluginService.kt | Manage server lifecycle based on settings changes + notifications. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val contentLength = exchange.requestHeaders.getFirst("Content-Length")?.toLongOrNull() ?: 0 | ||
| if (contentLength > MAX_BODY_SIZE) { | ||
| exchange.sendResponse(413, "Request body too large. Limit: ${MAX_BODY_SIZE / 1024 / 1024}MB") | ||
| return | ||
| } | ||
|
|
||
| val body = try { | ||
| val inputStream = decompressIfNeeded(exchange) | ||
| inputStream.bufferedReader().readText() | ||
| } catch (e: Exception) { |
There was a problem hiding this comment.
handleFull enforces MAX_BODY_SIZE only via the Content-Length header, which can be missing/incorrect (e.g., chunked transfer) and still allow an arbitrarily large body to be read into memory with readText(). Consider enforcing the limit while reading/parsing (e.g., bounded stream/reader) so the cap applies regardless of headers and compression.
| sendResponse(500, "Internal Server Error") | ||
| } catch (_: Throwable) { | ||
| // Response may have already been sent | ||
| } |
There was a problem hiding this comment.
HttpExchange.use doesn't call exchange.close() in a finally block. Per HttpExchange contract, handlers should close the exchange to release the request/response streams; currently only responseBody is closed in sendResponse, and early returns/exception paths may leak the request body / connection resources.
| } | |
| } | |
| } finally { | |
| try { | |
| close() | |
| } catch (_: Throwable) { | |
| // Ignore close failures while cleaning up the exchange | |
| } |
| private fun decompressIfNeeded(exchange: HttpExchange): InputStream { | ||
| val encoding = exchange.requestHeaders.getFirst("Content-Encoding") | ||
| return if (encoding != null && encoding.equals("gzip", ignoreCase = true)) { | ||
| GZIPInputStream(exchange.requestBody) | ||
| } else { | ||
| exchange.requestBody | ||
| } |
There was a problem hiding this comment.
decompressIfNeeded allows gzip bodies but the size check is based on Content-Length (compressed size) and the code reads the full decompressed text into memory. This makes the endpoint vulnerable to decompression bombs; enforce a maximum on decompressed bytes (bounded GZIPInputStream / counting stream) before converting to a String.
| override fun dispose() { | ||
| messageBusConnection?.disconnect() | ||
| messageBusConnection = null | ||
| server?.stop() | ||
| server = null | ||
| operationQueue.close() | ||
| } | ||
|
|
||
| private sealed class Operation { | ||
| data class Update( | ||
| val change: ProjectSettingsConfigurable.SettingsChangedEvent? = null, | ||
| ) : Operation() | ||
|
|
||
| object Stop : Operation() | ||
| } |
There was a problem hiding this comment.
Operation.Stop is never enqueued (dispose() stops the server directly and only closes the channel). Either enqueue Operation.Stop in dispose() (like SourcemapGeneratorService) or remove the unused operation/branch to avoid dead code and keep lifecycle handling consistent.
| fun start() { | ||
| val httpServer = HttpServer.create(InetSocketAddress("127.0.0.1", port), 0) | ||
| httpServer.createContext("/full") { exchange -> handleFull(exchange) } | ||
| httpServer.createContext("/clear") { exchange -> handleClear(exchange) } | ||
| httpServer.createContext("/get-file-paths") { exchange -> handleGetFilePaths(exchange) } | ||
| httpServer.start() |
There was a problem hiding this comment.
The companion HTTP server exposes /full, /clear, and /get-file-paths without any authentication/authorization. Even though it binds to 127.0.0.1, any local process can send requests (including retrieving absolute project file paths). Consider adding a per-project shared secret (random token) and require it via header/query param, or otherwise restricting accepted clients.
| try { | ||
| val sendNotification = lspServer.javaClass.getMethod( | ||
| "sendNotification", | ||
| kotlin.jvm.functions.Function1::class.java | ||
| ) | ||
| val callback: (org.eclipse.lsp4j.services.LanguageServer) -> Unit = { languageServer -> | ||
| (languageServer as? LuauLanguageServer)?.let(action) | ||
| } | ||
| sendNotification.invoke(lspServer, callback) | ||
| } catch (_: NoSuchMethodException) { | ||
| // Fallback for older IntelliJ versions with lsp4jServer | ||
| try { | ||
| val getLsp4jServer = lspServer.javaClass.getMethod("getLsp4jServer") | ||
| val server = getLsp4jServer.invoke(lspServer) as? LuauLanguageServer ?: return false | ||
| action(server) | ||
| } catch (e: Throwable) { | ||
| LOG.warn("Failed to access LSP server API", e) | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
sendLspNotification only catches NoSuchMethodException for the sendNotification path. If getMethod succeeds but invoke fails (e.g., IllegalAccessException, InvocationTargetException, signature mismatch at runtime), the exception will escape and turn the HTTP handler into a 500. Consider catching Throwable around the reflection invoke and returning false (similar to the fallback path).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #148 +/- ##
==========================================
- Coverage 48.50% 47.31% -1.19%
==========================================
Files 198 201 +3
Lines 6204 6334 +130
Branches 1630 1663 +33
==========================================
- Hits 3009 2997 -12
- Misses 2463 2604 +141
- Partials 732 733 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Adds support for the Luau Language Server Companion