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>
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
240
tests/libs/acp_client_spec.lua
Normal file
240
tests/libs/acp_client_spec.lua
Normal file
@@ -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)
|
||||
Reference in New Issue
Block a user