From 0336666a739a68925de8f15dd6b0381faabba0b0 Mon Sep 17 00:00:00 2001 From: Adrian Cole <64215+codefromthecrypt@users.noreply.github.com> Date: Sat, 13 Dec 2025 16:30:11 +0100 Subject: [PATCH] fix(acp): return JSON-RPC error for fs/read_text_file on missing files (#2849) Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com> --- lua/avante/libs/acp_client.lua | 37 +++-- lua/avante/llm.lua | 11 +- lua/avante/utils/init.lua | 12 +- tests/libs/acp_client_spec.lua | 240 +++++++++++++++++++++++++++++++++ 4 files changed, 281 insertions(+), 19 deletions(-) create mode 100644 tests/libs/acp_client_spec.lua diff --git a/lua/avante/libs/acp_client.lua b/lua/avante/libs/acp_client.lua index 5faeb3d..dc55881 100644 --- a/lua/avante/libs/acp_client.lua +++ b/lua/avante/libs/acp_client.lua @@ -195,19 +195,21 @@ local ACPClient = {} -- ACP Error codes ACPClient.ERROR_CODES = { - TRANSPORT_ERROR = -32000, - PROTOCOL_ERROR = -32001, - TIMEOUT_ERROR = -32002, - AUTH_REQUIRED = -32003, - SESSION_NOT_FOUND = -32004, - PERMISSION_DENIED = -32005, - INVALID_REQUEST = -32006, + -- JSON-RPC 2.0 + PARSE_ERROR = -32700, + INVALID_REQUEST = -32600, + METHOD_NOT_FOUND = -32601, + INVALID_PARAMS = -32602, + INTERNAL_ERROR = -32603, + -- ACP + AUTH_REQUIRED = -32000, + RESOURCE_NOT_FOUND = -32002, } ---@class ACPHandlers ---@field on_session_update? fun(update: avante.acp.UserMessageChunk | avante.acp.AgentMessageChunk | avante.acp.AgentThoughtChunk | avante.acp.ToolCallUpdate | avante.acp.PlanUpdate | avante.acp.AvailableCommandsUpdate) ---@field on_request_permission? fun(tool_call: table, options: table[], callback: fun(option_id: string | nil)): nil ----@field on_read_file? fun(path: string, line: integer | nil, limit: integer | nil, callback: fun(content: string)): nil +---@field on_read_file? fun(path: string, line: integer | nil, limit: integer | nil, callback: fun(content: string), error_callback: fun(message: string, code: integer|nil)): nil ---@field on_write_file? fun(path: string, content: string, callback: fun(error: string|nil)): nil ---@field on_error? fun(error: table) @@ -562,7 +564,7 @@ end ---@param code? number ---@return nil function ACPClient:_send_error(id, message, code) - code = code or self.ERROR_CODES.TRANSPORT_ERROR + code = code or self.ERROR_CODES.INTERNAL_ERROR local msg = { jsonrpc = "2.0", id = id, error = { code = code, message = message } } local data = vim.json.encode(msg) @@ -666,7 +668,10 @@ function ACPClient:_handle_read_text_file(message_id, params) local session_id = params.sessionId local path = params.path - if not session_id or not path then return end + if not session_id or not path then + self:_send_error(message_id, "Invalid fs/read_text_file params", ACPClient.ERROR_CODES.INVALID_PARAMS) + return + end if self.config.handlers and self.config.handlers.on_read_file then vim.schedule(function() @@ -674,9 +679,12 @@ function ACPClient:_handle_read_text_file(message_id, params) path, params.line ~= vim.NIL and params.line or nil, params.limit ~= vim.NIL and params.limit or nil, - function(content) self:_send_result(message_id, { content = content }) end + function(content) self:_send_result(message_id, { content = content }) end, + function(err, code) self:_send_error(message_id, err or "Failed to read file", code) end ) end) + else + self:_send_error(message_id, "fs/read_text_file handler not configured", ACPClient.ERROR_CODES.METHOD_NOT_FOUND) end end @@ -688,7 +696,10 @@ function ACPClient:_handle_write_text_file(message_id, params) local path = params.path local content = params.content - if not session_id or not path or not content then return end + if not session_id or not path or not content then + self:_send_error(message_id, "Invalid fs/write_text_file params", ACPClient.ERROR_CODES.INVALID_PARAMS) + return + end if self.config.handlers and self.config.handlers.on_write_file then vim.schedule(function() @@ -698,6 +709,8 @@ function ACPClient:_handle_write_text_file(message_id, params) function(error) self:_send_result(message_id, error == nil and vim.NIL or error) end ) end) + else + self:_send_error(message_id, "fs/write_text_file handler not configured", ACPClient.ERROR_CODES.METHOD_NOT_FOUND) end end diff --git a/lua/avante/llm.lua b/lua/avante/llm.lua index 34b6c49..12be03b 100644 --- a/lua/avante/llm.lua +++ b/lua/avante/llm.lua @@ -1238,9 +1238,16 @@ function M._stream_acp(opts) permission_options = options, }, opts.session_ctx, tool_call.kind) end, - on_read_file = function(path, line, limit, callback) + on_read_file = function(path, line, limit, callback, error_callback) local abs_path = Utils.to_absolute_path(path) - local lines = Utils.read_file_from_buf_or_disk(abs_path) + local lines, err, errname = Utils.read_file_from_buf_or_disk(abs_path) + if err then + if error_callback then + local code = errname == "ENOENT" and ACPClient.ERROR_CODES.RESOURCE_NOT_FOUND or nil + error_callback(err, code) + end + return + end lines = lines or {} if line ~= nil and limit ~= nil then lines = vim.list_slice(lines, line, line + limit) end local content = table.concat(lines, "\n") diff --git a/lua/avante/utils/init.lua b/lua/avante/utils/init.lua index 65b3b5d..2786750 100644 --- a/lua/avante/utils/init.lua +++ b/lua/avante/utils/init.lua @@ -1383,6 +1383,7 @@ end ---@param filepath string ---@return string[]|nil lines ---@return string|nil error +---@return string|nil errname function M.read_file_from_buf_or_disk(filepath) local abs_path = filepath:sub(1, 7) == "term://" and filepath or M.join_paths(M.get_project_root(), filepath) --- Lookup if the file is loaded in a buffer @@ -1391,12 +1392,13 @@ function M.read_file_from_buf_or_disk(filepath) if bufnr ~= -1 and vim.api.nvim_buf_is_loaded(bufnr) then -- If buffer exists and is loaded, get buffer content local lines = vim.api.nvim_buf_get_lines(bufnr, 0, -1, false) - return lines, nil + return lines, nil, nil end end - local stat = vim.uv.fs_stat(abs_path) - if stat and stat.type == "directory" then return {}, "Cannot read a directory as file" .. filepath end + local stat, stat_err, stat_errname = vim.uv.fs_stat(abs_path) + if not stat then return {}, stat_err, stat_errname end + if stat.type == "directory" then return {}, "Cannot read a directory as file" .. filepath, nil end -- Fallback: read file from disk local file, open_err = io.open(abs_path, "r") @@ -1404,9 +1406,9 @@ function M.read_file_from_buf_or_disk(filepath) local content = file:read("*all") file:close() content = content:gsub("\r\n", "\n") - return vim.split(content, "\n"), nil + return vim.split(content, "\n"), nil, nil else - return {}, open_err + return {}, open_err, nil end end diff --git a/tests/libs/acp_client_spec.lua b/tests/libs/acp_client_spec.lua new file mode 100644 index 0000000..9f8030d --- /dev/null +++ b/tests/libs/acp_client_spec.lua @@ -0,0 +1,240 @@ +local ACPClient = require("avante.libs.acp_client") +local stub = require("luassert.stub") + +describe("ACPClient", function() + local schedule_stub + local setup_transport_stub + + before_each(function() + schedule_stub = stub(vim, "schedule") + schedule_stub.invokes(function(fn) fn() end) + setup_transport_stub = stub(ACPClient, "_setup_transport") + end) + + after_each(function() + schedule_stub:revert() + setup_transport_stub:revert() + end) + + describe("_handle_read_text_file", function() + it("should call error_callback when file read fails", function() + local sent_error = nil + local handler_called = false + local mock_config = { + transport_type = "stdio", + handlers = { + on_read_file = function(path, line, limit, success_callback, err_callback) + handler_called = true + err_callback("File not found", ACPClient.ERROR_CODES.RESOURCE_NOT_FOUND) + end, + }, + } + + local client = ACPClient:new(mock_config) + client._send_error = stub().invokes( + function(self, id, message, code) sent_error = { id = id, message = message, code = code } end + ) + + client:_handle_read_text_file(123, { sessionId = "test-session", path = "/nonexistent/file.txt" }) + + assert.is_true(handler_called) + assert.is_not_nil(sent_error) + assert.equals(123, sent_error.id) + assert.equals("File not found", sent_error.message) + assert.equals(ACPClient.ERROR_CODES.RESOURCE_NOT_FOUND, sent_error.code) + end) + + it("should use default error message when error_callback called with nil", function() + local sent_error = nil + local mock_config = { + transport_type = "stdio", + handlers = { + on_read_file = function(path, line, limit, success_callback, err_callback) err_callback(nil, nil) end, + }, + } + + local client = ACPClient:new(mock_config) + client._send_error = stub().invokes( + function(self, id, message, code) sent_error = { id = id, message = message, code = code } end + ) + + client:_handle_read_text_file(456, { sessionId = "test-session", path = "/bad/file.txt" }) + + assert.is_not_nil(sent_error) + assert.equals(456, sent_error.id) + assert.equals("Failed to read file", sent_error.message) + assert.is_nil(sent_error.code) + end) + + it("should call success_callback when file read succeeds", function() + local sent_result = nil + local mock_config = { + transport_type = "stdio", + handlers = { + on_read_file = function(path, line, limit, success_callback, err_callback) success_callback("file contents") end, + }, + } + + local client = ACPClient:new(mock_config) + client._send_result = stub().invokes(function(self, id, result) sent_result = { id = id, result = result } end) + + client:_handle_read_text_file(789, { sessionId = "test-session", path = "/existing/file.txt" }) + + assert.is_not_nil(sent_result) + assert.equals(789, sent_result.id) + assert.equals("file contents", sent_result.result.content) + end) + + it("should send error when params are invalid (missing sessionId)", function() + local sent_error = nil + local mock_config = { + transport_type = "stdio", + handlers = { + on_read_file = function() end, + }, + } + + local client = ACPClient:new(mock_config) + client._send_error = stub().invokes( + function(self, id, message, code) sent_error = { id = id, message = message, code = code } end + ) + + client:_handle_read_text_file(100, { path = "/file.txt" }) + + assert.is_not_nil(sent_error) + assert.equals(100, sent_error.id) + assert.equals("Invalid fs/read_text_file params", sent_error.message) + assert.equals(ACPClient.ERROR_CODES.INVALID_PARAMS, sent_error.code) + end) + + it("should send error when params are invalid (missing path)", function() + local sent_error = nil + local mock_config = { + transport_type = "stdio", + handlers = { + on_read_file = function() end, + }, + } + + local client = ACPClient:new(mock_config) + client._send_error = stub().invokes( + function(self, id, message, code) sent_error = { id = id, message = message, code = code } end + ) + + client:_handle_read_text_file(200, { sessionId = "test-session" }) + + assert.is_not_nil(sent_error) + assert.equals(200, sent_error.id) + assert.equals("Invalid fs/read_text_file params", sent_error.message) + assert.equals(ACPClient.ERROR_CODES.INVALID_PARAMS, sent_error.code) + end) + + it("should send error when handler is not configured", function() + local sent_error = nil + local mock_config = { + transport_type = "stdio", + handlers = {}, + } + + local client = ACPClient:new(mock_config) + client._send_error = stub().invokes( + function(self, id, message, code) sent_error = { id = id, message = message, code = code } end + ) + + client:_handle_read_text_file(300, { sessionId = "test-session", path = "/file.txt" }) + + assert.is_not_nil(sent_error) + assert.equals(300, sent_error.id) + assert.equals("fs/read_text_file handler not configured", sent_error.message) + assert.equals(ACPClient.ERROR_CODES.METHOD_NOT_FOUND, sent_error.code) + end) + end) + + describe("_handle_write_text_file", function() + it("should send error when params are invalid (missing sessionId)", function() + local sent_error = nil + local mock_config = { + transport_type = "stdio", + handlers = { + on_write_file = function() end, + }, + } + + local client = ACPClient:new(mock_config) + client._send_error = stub().invokes( + function(self, id, message, code) sent_error = { id = id, message = message, code = code } end + ) + + client:_handle_write_text_file(400, { path = "/file.txt", content = "data" }) + + assert.is_not_nil(sent_error) + assert.equals(400, sent_error.id) + assert.equals("Invalid fs/write_text_file params", sent_error.message) + assert.equals(ACPClient.ERROR_CODES.INVALID_PARAMS, sent_error.code) + end) + + it("should send error when params are invalid (missing path)", function() + local sent_error = nil + local mock_config = { + transport_type = "stdio", + handlers = { + on_write_file = function() end, + }, + } + + local client = ACPClient:new(mock_config) + client._send_error = stub().invokes( + function(self, id, message, code) sent_error = { id = id, message = message, code = code } end + ) + + client:_handle_write_text_file(500, { sessionId = "test-session", content = "data" }) + + assert.is_not_nil(sent_error) + assert.equals(500, sent_error.id) + assert.equals("Invalid fs/write_text_file params", sent_error.message) + assert.equals(ACPClient.ERROR_CODES.INVALID_PARAMS, sent_error.code) + end) + + it("should send error when params are invalid (missing content)", function() + local sent_error = nil + local mock_config = { + transport_type = "stdio", + handlers = { + on_write_file = function() end, + }, + } + + local client = ACPClient:new(mock_config) + client._send_error = stub().invokes( + function(self, id, message, code) sent_error = { id = id, message = message, code = code } end + ) + + client:_handle_write_text_file(600, { sessionId = "test-session", path = "/file.txt" }) + + assert.is_not_nil(sent_error) + assert.equals(600, sent_error.id) + assert.equals("Invalid fs/write_text_file params", sent_error.message) + assert.equals(ACPClient.ERROR_CODES.INVALID_PARAMS, sent_error.code) + end) + + it("should send error when handler is not configured", function() + local sent_error = nil + local mock_config = { + transport_type = "stdio", + handlers = {}, + } + + local client = ACPClient:new(mock_config) + client._send_error = stub().invokes( + function(self, id, message, code) sent_error = { id = id, message = message, code = code } end + ) + + client:_handle_write_text_file(700, { sessionId = "test-session", path = "/file.txt", content = "data" }) + + assert.is_not_nil(sent_error) + assert.equals(700, sent_error.id) + assert.equals("fs/write_text_file handler not configured", sent_error.message) + assert.equals(ACPClient.ERROR_CODES.METHOD_NOT_FOUND, sent_error.code) + end) + end) +end)