From 82dd6e673db5dd8f4f8598edeb351adc824a3870 Mon Sep 17 00:00:00 2001 From: Jeremy Seago Date: Fri, 6 Mar 2026 12:09:13 -0600 Subject: [PATCH] fix(cli): prevent race condition between send and submit Add polling mechanism to ensure send queue is empty before submitting. Previously, submit could be called immediately after send, causing the carriage return to be sent before the message was fully transmitted. Changes: - Poll send queue with 100ms intervals before submitting - Add 5 second timeout with warning for stuck queues - Send carriage return directly to channel in submit() to avoid queueing - Add comprehensive integration tests for submit timing - Add unit tests for prompt resolution in send() --- lua/sidekick/cli/init.lua | 17 ++- lua/sidekick/cli/terminal.lua | 3 +- tests/cli_send_integration_spec.lua | 218 ++++++++++++++++++++++++++++ tests/cli_send_spec.lua | 128 ++++++++++++++++ 4 files changed, 364 insertions(+), 2 deletions(-) create mode 100644 tests/cli_send_integration_spec.lua create mode 100644 tests/cli_send_spec.lua diff --git a/lua/sidekick/cli/init.lua b/lua/sidekick/cli/init.lua index 9c0e9b50..4cbdb39e 100644 --- a/lua/sidekick/cli/init.lua +++ b/lua/sidekick/cli/init.lua @@ -194,7 +194,22 @@ function M.send(opts) msg = state.tool:format(text) state.session:send(msg .. "\n") if opts.submit then - state.session:submit() + -- Poll until send queue is empty, then submit + local max_attempts = 50 -- 5 second timeout + local attempts = 0 + local function try_submit() + attempts = attempts + 1 + if state.session.send_queue and #state.session.send_queue > 0 then + if attempts < max_attempts then + vim.defer_fn(try_submit, 100) + else + require("sidekick.util").warn("Submit timeout: send queue not empty after 5s") + end + else + state.session:submit() + end + end + vim.defer_fn(try_submit, 100) end end) end, { diff --git a/lua/sidekick/cli/terminal.lua b/lua/sidekick/cli/terminal.lua index 1053d1d4..4815bd01 100644 --- a/lua/sidekick/cli/terminal.lua +++ b/lua/sidekick/cli/terminal.lua @@ -491,7 +491,8 @@ function M:submit() if not self:is_running() then return end - self:send("\r") -- Updated to use the send method + -- Send carriage return directly to avoid queueing + vim.api.nvim_chan_send(self.job, "\r") end ---@param buf? integer diff --git a/tests/cli_send_integration_spec.lua b/tests/cli_send_integration_spec.lua new file mode 100644 index 00000000..debea4d5 --- /dev/null +++ b/tests/cli_send_integration_spec.lua @@ -0,0 +1,218 @@ +---@module 'luassert' + +local Cli = require("sidekick.cli") +local Config = require("sidekick.config") + +describe("cli.send integration", function() + local buf, win + + before_each(function() + buf = vim.api.nvim_create_buf(false, true) + vim.api.nvim_buf_set_lines(buf, 0, -1, false, { "test line" }) + vim.bo[buf].filetype = "lua" + win = vim.api.nvim_get_current_win() + vim.api.nvim_win_set_buf(win, buf) + vim.w[win].sidekick_visit = vim.uv.hrtime() + end) + + after_each(function() + if vim.api.nvim_buf_is_valid(buf) then + vim.api.nvim_buf_delete(buf, { force = true }) + end + end) + + describe("submit timing", function() + local events + local mock_state + + before_each(function() + events = {} + mock_state = { + tool = { + format = function(_, text) + return table.concat( + vim.tbl_map(function(t) + return type(t) == "table" and t[1] or t + end, text or {}), + "" + ) + end, + }, + session = { + send_queue = {}, + send = function(self, msg) + table.insert(events, { type = "send", msg = msg, time = vim.uv.hrtime() }) + table.insert(self.send_queue, msg) + -- Simulate async processing + vim.defer_fn(function() + table.remove(self.send_queue, 1) + end, 50) + end, + submit = function() + table.insert(events, { type = "submit", time = vim.uv.hrtime() }) + end, + }, + } + + local State = require("sidekick.cli.state") + _G._original_state_with = State.with + State.with = function(fn, _) + fn(mock_state) + end + end) + + after_each(function() + if _G._original_state_with then + require("sidekick.cli.state").with = _G._original_state_with + _G._original_state_with = nil + end + end) + + it("submits after queue is empty", function() + Cli.send({ msg = "test message", submit = true }) + + -- Wait for polling to complete + vim.wait(500, function() + return #vim.tbl_filter(function(e) + return e.type == "submit" + end, events) > 0 + end) + + local send_event = vim.tbl_filter(function(e) + return e.type == "send" + end, events)[1] + local submit_event = vim.tbl_filter(function(e) + return e.type == "submit" + end, events)[1] + + assert.is_not_nil(send_event, "send event should exist") + assert.is_not_nil(submit_event, "submit event should exist") + assert.is_true(submit_event.time > send_event.time, "submit should happen after send") + end) + + it("waits for multiple queued messages", function() + mock_state.session.send = function(self, msg) + table.insert(events, { type = "send", msg = msg, time = vim.uv.hrtime() }) + table.insert(self.send_queue, msg) + -- Simulate slower processing + vim.defer_fn(function() + if #self.send_queue > 0 then + table.remove(self.send_queue, 1) + end + end, 150) + end + + -- Add multiple messages to queue + table.insert(mock_state.session.send_queue, "queued1") + table.insert(mock_state.session.send_queue, "queued2") + + Cli.send({ msg = "test message", submit = true }) + + vim.wait(1000, function() + return #vim.tbl_filter(function(e) + return e.type == "submit" + end, events) > 0 + end) + + local submit_event = vim.tbl_filter(function(e) + return e.type == "submit" + end, events)[1] + + assert.is_not_nil(submit_event, "submit should eventually happen") + end) + + it("does not submit when submit=false", function() + Cli.send({ msg = "test message", submit = false }) + + vim.wait(300) + + local submit_events = vim.tbl_filter(function(e) + return e.type == "submit" + end, events) + + assert.are.equal(0, #submit_events, "submit should not be called") + end) + + it("handles empty queue immediately", function() + mock_state.session.send_queue = {} + + Cli.send({ msg = "test message", submit = true }) + + vim.wait(300, function() + return #vim.tbl_filter(function(e) + return e.type == "submit" + end, events) > 0 + end) + + local submit_event = vim.tbl_filter(function(e) + return e.type == "submit" + end, events)[1] + + assert.is_not_nil(submit_event, "submit should happen quickly with empty queue") + end) + + it("times out after 5 seconds with stuck queue", function() + local warnings = {} + local Util = require("sidekick.util") + local original_warn = Util.warn + Util.warn = function(msg) + table.insert(warnings, msg) + end + + -- Queue that never empties + mock_state.session.send = function(self, msg) + table.insert(events, { type = "send", msg = msg, time = vim.uv.hrtime() }) + table.insert(self.send_queue, msg) + -- Never remove from queue + end + + Cli.send({ msg = "test message", submit = true }) + + -- Wait for timeout (5 seconds + buffer) + vim.wait(5500, function() + return #warnings > 0 + end) + + Util.warn = original_warn + + assert.are.equal(1, #warnings) + assert.is_true(warnings[1]:match("timeout") ~= nil) + + -- Submit should not have been called + local submit_events = vim.tbl_filter(function(e) + return e.type == "submit" + end, events) + assert.are.equal(0, #submit_events) + end) + end) + + describe("terminal submit behavior", function() + it("sends carriage return directly to channel", function() + local Terminal = require("sidekick.cli.terminal") + local sent_data = {} + + -- Mock nvim_chan_send + local original_chan_send = vim.api.nvim_chan_send + vim.api.nvim_chan_send = function(chan, data) + table.insert(sent_data, { chan = chan, data = data }) + end + + -- Create a minimal terminal instance + local term = setmetatable({ + job = 123, + send_queue = {}, + is_running = function() + return true + end, + }, { __index = Terminal }) + + term:submit() + + vim.api.nvim_chan_send = original_chan_send + + assert.are.equal(1, #sent_data) + assert.are.equal(123, sent_data[1].chan) + assert.are.equal("\r", sent_data[1].data) + end) + end) +end) diff --git a/tests/cli_send_spec.lua b/tests/cli_send_spec.lua new file mode 100644 index 00000000..7a7dc8c2 --- /dev/null +++ b/tests/cli_send_spec.lua @@ -0,0 +1,128 @@ +---@module 'luassert' + +local Cli = require("sidekick.cli") +local Config = require("sidekick.config") + +describe("cli.send with prompt option", function() + local buf, win + + before_each(function() + buf = vim.api.nvim_create_buf(false, true) + vim.api.nvim_buf_set_lines(buf, 0, -1, false, { "test line" }) + vim.bo[buf].filetype = "lua" + win = vim.api.nvim_get_current_win() + vim.api.nvim_win_set_buf(win, buf) + vim.w[win].sidekick_visit = vim.uv.hrtime() + end) + + after_each(function() + if vim.api.nvim_buf_is_valid(buf) then + vim.api.nvim_buf_delete(buf, { force = true }) + end + end) + + describe("prompt resolution", function() + it("resolves string prompt by name", function() + Config.cli.prompts.test_simple = "Simple test prompt" + vim.api.nvim_win_set_cursor(win, { 1, 0 }) + local rendered = Cli.render({ prompt = "test_simple" }) + assert.is_not_nil(rendered) + Config.cli.prompts.test_simple = nil + end) + + it("resolves function prompt by name", function() + Config.cli.prompts.test_fn = function() + return "Function result" + end + local rendered = Cli.render({ prompt = "test_fn" }) + assert.is_not_nil(rendered) + Config.cli.prompts.test_fn = nil + end) + + it("returns nil for non-existent prompt", function() + local rendered = Cli.render({ prompt = "nonexistent" }) + assert.is_nil(rendered) + end) + end) + + describe("send with prompt option", function() + local sent_messages + local mock_state + + before_each(function() + sent_messages = {} + mock_state = { + tool = { + format = function(_, text) + return table.concat( + vim.tbl_map(function(t) + return type(t) == "table" and t[1] or t + end, text or {}), + "" + ) + end, + }, + session = { + send = function(_, msg) + table.insert(sent_messages, msg) + end, + submit = function() end, + }, + } + + local State = require("sidekick.cli.state") + _G._original_state_with = State.with + State.with = function(fn, _) + fn(mock_state) + end + end) + + after_each(function() + if _G._original_state_with then + require("sidekick.cli.state").with = _G._original_state_with + _G._original_state_with = nil + end + end) + + it("sends prompt content when prompt option is provided", function() + Cli.send({ prompt = "explain" }) + vim.wait(100) + assert.is_true(#sent_messages > 0) + assert.is_true(sent_messages[1]:match("Explain") ~= nil) + end) + + it("prefers msg over prompt when both provided", function() + Cli.send({ msg = "Direct message", prompt = "explain" }) + vim.wait(100) + assert.is_true(#sent_messages > 0) + assert.are.equal("Direct message\n", sent_messages[1]) + end) + + it("handles function-based prompts", function() + Config.cli.prompts.test_fn = function() + return "Function result" + end + Cli.send({ prompt = "test_fn" }) + vim.wait(100) + assert.is_true(#sent_messages > 0) + assert.is_true(sent_messages[1]:match("Function result") ~= nil) + Config.cli.prompts.test_fn = nil + end) + + it("warns on non-existent prompt", function() + local warned = false + local Util = require("sidekick.util") + local original_warn = Util.warn + Util.warn = function() + warned = true + end + + Cli.send({ prompt = "nonexistent" }) + vim.wait(100) + + Util.warn = original_warn + assert.is_true(warned) + assert.are.equal(0, #sent_messages) + end) + end) +end)