Paul King created GROOVY-12633:
----------------------------------
Summary: Follow-ups from a post-merge review of groovy-mcp
(GROOVY-12487)
Key: GROOVY-12633
URL: https://issues.apache.org/jira/browse/GROOVY-12633
Project: Groovy
Issue Type: Bug
Reporter: Paul King
h2. Description
Follow-ups from a post-merge review of {{groovy-mcp}} (GROOVY-12487). The
module works as described for ordinary use. The items below are a claim the
server makes about itself that doesn't hold, two interoperability problems,
behaviour problems found by driving the server over stdin, missing tests, and
documentation of how the module is kept in step with the rest of Groovy.
h2. 1. A {{--classpath}} jar's global AST transformations run on every query
A jar on {{--classpath}} that registers a global AST transformation
({{META-INF/services/org.codehaus.groovy.transform.ASTTransformation}}) runs on
every compile the server does. A transformation whose static initializer wrote
a marker file ran on a single {{groovy_diagnostics}} call on {{text}}. Local
transformations triggered by annotations in the queried source, and macro
methods, run the same way.
Only {{@Grab}} and {{@ASTTest}} are disabled. Yet:
* {{groovy://server/limits}} hard-codes {{"execution": false}}.
* ARCHITECTURE.md says "Compiling is not executing".
* The user guide says the server "does not run the source it is shown", which
is true for the source but not for the classpath.
A model cannot widen the classpath, so this is the operator's trust decision.
The docs, and the limits resource, should say plainly that code on
{{--classpath}} can run at compile time. Either rename the {{execution}} field
or describe exactly what it covers.
h2. 2. A {{--root}} reached through a symlink rejects the server's own URIs
{{PathPolicy}} checks the lexical path against the root as given, while results
carry real-path URIs. With {{--root /tmp/x}} on macOS, where {{/tmp}} is a link
to {{/private/tmp}}:
* {{groovy_symbols}} returns {{file:///private/tmp/x/Sample.groovy}}.
* Passing that URI, or the same absolute path, back to a tool gives "path is
outside the workspace root".
It fails closed, but breaks the normal round trip under macOS {{/tmp}} and
{{$TMPDIR}}, and Windows junctions. Accept a path whose real path is inside
{{root.toRealPath()}}.
h2. 3. Tool {{inputSchema}} uses a top-level {{oneOf}}
The 12 source tools publish an {{inputSchema}} whose top level is a {{oneOf}}
over {{text}} / {{path}} / {{uri}}. Some LLM tool APIs reject {{oneOf}},
{{anyOf}} and {{allOf}} at the top level of an input schema. In a client that
passes the schema through unchanged, those tools are unusable. Publishing plain
{{properties}} and enforcing the combination rules at run time, as is already
done, avoids this. This has not been checked against a live client.
h2. 4. Deeply nested input reports no diagnostics
A buffer with about 2000 nested parentheses followed by a real syntax error
returns {{"diagnostics": []}} with {{isError: false}}. {{groovy_tokens}}
returns {{[]}}. With 200 parentheses, the same buffer correctly reports
{{Unexpected '}'}}. The compiler on its own reports a parse failure, so the
error is being swallowed on the groovy-lsp side, and a model is told that
broken code is clean. A failed compile should produce a diagnostic, or an
{{isError}} result.
h2. 5. Open buffers never expire and are compiled together
Every {{text}} call with a new {{groovy-buffer:}} URI, and every {{path}},
stays open until {{groovy_close}}, and each query recompiles all open buffers
together.
* {{class Foo}} in {{groovy-buffer:a}}, then {{class Foo}} in the default
snippet, reports "Invalid duplicate class definition" naming both buffers.
* There is no cap on the number of buffers. With N buffers of up to 512 Ki
characters each, heap use and per-call cost grow with N.
Options: cap the number of open buffers, replace rather than accumulate the
default snippet, or compile a {{text}} query on its own. At a minimum, the user
guide should say that buffers persist and are compiled together.
h2. 6. Protocol details
* A large integer id is truncated: {{18446744073709551617}} is answered with
{{"id":1}} ({{JsonValues.asInt}} calls {{longValue()}}). It should be
{{-32600}} with a null id, as for other invalid ids.
* Trailing content after a JSON value is accepted:
{{\{"jsonrpc":"2.0","id":"s","method":"ping"\} trailing}} returns a result. It
should be {{-32700}}.
* A *request* (with an id) whose method starts with {{notifications/}} gets
{{result: \{\}}}. It should be {{-32601}}.
* A response object sent by the client ({{\{"id":10,"result":\{\}\}}}) gets an
error reply. JSON-RPC says not to reply to responses.
* "Not initialized" uses {{-32002}}, which MCP uses for "resource not found".
Choose an unassigned code.
* Prompt descriptors carry {{annotations}}, which the Prompt type may not
define. Confirm this against the spec, or drop it.
h2. 7. File reads
{{PathPolicy.resolveFile}} checks the size and the real path, and then
{{engine.open}} reads the path again without a size limit. A local process that
can write inside the root could replace the file with a symlink, or grow it, in
between. Read once, after the check, from the resolved real path, and limit the
read itself.
The result-size cap covers one copy of the payload, but each tool result
carries it twice ({{content}} text and {{structuredContent}}).
h2. 8. API surface
The module has no published baseline yet, so these can still change cheaply:
* Public constants {{PROTOCOL_VERSION}}, {{SUPPORTED_PROTOCOLS}} and
{{MODERN_PROTOCOL}} will change with each MCP revision. Make them non-public,
or document them as not stable.
* {{McpServerConfig}} is a record, so adding a cap later changes its canonical
constructor. Consider a builder.
* {{GroovyLanguageEngine.context()}} was made public only so the limits
resource can read the Grape and ASTTest flags and the classpath size. A narrow
accessor would avoid exposing {{LanguageServerContext}}.
* Neither {{groovy-mcp}} nor {{groovy-lsp}} carries {{@Incubating}} or a prose
incubating marker, although both track external protocols that change several
times a year.
h2. 9. Tests
* The {{@ASTTest}} test checks only that a string is absent from the output.
Use an observable side effect, such as a system property or a marker file.
* Add a test that {{@Grab}} does not resolve.
* Add a test that pins what {{--classpath}} transformations do, to match the
documentation from item 1.
* Add tests for items 2, 4, 5 and 6.
* In groovy-lsp, the new compile-epoch logic in {{LanguageServerContext}},
where a started debounced compile yields to an explicit {{recompile()}}, has no
test.
h2. 10. Documentation: keeping the module in step
Everything {{groovy-mcp}} uses is in {{groovy-lsp}} {{internal}} packages. An
API change there breaks the build, which is fine. These changes break nothing
and go unnoticed:
* *New engine features.* A capability added to {{GroovyLanguageEngine}} reaches
MCP clients only if someone adds a tool, with a hand-written schema.
* *New compile-time execution paths.* The safety claims rest on groovy-lsp
disabling {{GrabAnnotationTransformation}} and the {{@ASTTest}} thread flag. A
future core switch of the same kind would need adding there.
* *MCP spec revisions.* The supported versions are hard-coded, and revisions
arrive several times a year.
Add a "Keeping in step" section to {{subprojects/groovy-mcp/ARCHITECTURE.md}}
listing these triggers. Point at it from {{subprojects/groovy-lsp/AGENTS.md}}.
Add a client-registration example (command and arguments) to the user guide,
which currently has none.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
