ale icon indicating copy to clipboard operation
ale copied to clipboard

Send JSON-RPC error responses for unknown request methods

Open HerringtonDarkholme opened this issue 5 months ago • 5 comments

The Language Server Protocol requires that all requests receive responses. Previously, ALE would silently ignore unknown JSON-RPC requests from language servers (like workspace/configuration), violating the LSP specification.

Solution:

  • Modified the central message handler in ale#lsp#HandleMessage to track whether any handler processed each message
  • Added handler return values: handlers now return v:true when they process server-initiated requests/notifications
  • Only 2 handlers needed modification since most handle client request responses:
    • ale#lsp_linter#HandleLSPResponse (textDocument/publishDiagnostics, etc.)
    • ale#codefix#HandleLSPResponse (workspace/applyEdit)
  • Unknown requests now receive standard JSON-RPC error responses with code -32601 (Method Not Found)

This ensures ALE is compliant with the LSP specification requirement that "Every processed request must send a response back to the sender."

🤖 Generated with Claude Code

Fixes #4610

I have not added vader test, due to insufficient AI training data. Yet I think this pull request is good enough for an inspiration.

HerringtonDarkholme avatar Sep 05 '25 00:09 HerringtonDarkholme

Vibe coding it, like it's 2025! Well done.

I've pushed rymdbar:pr/5044 which adds two, hand-crafted LLM-free, details:

  • A trivial message informing the user an unknown call was made.
  • An attempt at a Vader test case detecting the above message.

One could imagine the user might be interesting in learning the names of unknown calls, so that might be a possible improvement.

The test case kind of works, but is a bit of a hack. I assume there are better ways of doing it, but this is what I came up with.

What worries me most is that merging this with my commit included might lead to users feeling flooded by messages caused by lsp:s ignoring announced capabilities and just yolo:ing calls.

Please feel free updating the PR to include this test case.

rymdbar avatar Sep 19 '25 07:09 rymdbar

I've pushed rymdbar:pr/5044 which adds two, hand-crafted LLM-free, details:

  • A trivial message informing the user an unknown call was made.

[snip]

What worries me most is that merging this with my commit included might lead to users feeling flooded by messages caused by lsp:s ignoring announced capabilities and just yolo:ing calls.

What if we put this behind a setting (g:ale…), and also included such errors in the :ALEInfo output? I for one would probably enable it so I can report server or client bugs (or to debug things), but I agree with worrying about the flood.

  • An attempt at a Vader test case detecting the above message.

[snip]

The test case kind of works, but is a bit of a hack. I assume there are better ways of doing it, but this is what I came up with.

I think there are test cases that simulate server requests and check responses?

One could imagine the user might be interesting in learning the names of unknown calls, so that might be a possible improvement.

❤️

benknoble avatar Sep 22 '25 11:09 benknoble

Hi ale team, I personally don't have time to add tests. But feel free to modify this PR or take the code to create a new PR. Cheers!

HerringtonDarkholme avatar Sep 22 '25 14:09 HerringtonDarkholme

I just tested this pull request after the recommendation from @rymdbar.

This fixes https://github.com/dense-analysis/ale/issues/4998, https://github.com/dense-analysis/ale/issues/5058 and https://github.com/biomejs/biome/issues/7908.

Can we get this merged?

Spixmaster avatar Oct 30 '25 15:10 Spixmaster

I just tested this pull request after the recommendation from @rymdbar.

My exact words were: You might wish to look at the conversation in that pull request and chip in your code to complete it?

Looking at the pull request was only half of the instruction.

Can we get this merged?

You mean in its current state? I think the mere fact that this pull request is open already answered that before the question was phrased.

rymdbar avatar Oct 31 '25 15:10 rymdbar