fix: address PR review comments for MCP tools compliance

- Fix closeAllDiffTabs potential duplicate window closing by using set-based approach instead of array
- Add comprehensive test coverage for openFile parameters (makeFrontmost, preview mode, line/text selection)
- Update CLAUDE.md documentation to mention endLine parameter for openFile tool
- Enhance JSON encoder/decoder in test suite with proper escape sequence handling
- Add missing vim API mocks for complex openFile functionality testing

All 325 tests now pass with complete coverage of new MCP tool features.

Change-Id: I15bceb2bb44552205ea63c5ef1cb83722f7b5893
Signed-off-by: Thomas Kosiewski <[email protected]>

Author: ThomasK33

CLAUDE.md

@@ -71,7 +71,7 @@ The WebSocket server implements secure authentication using:
 
 **MCP-Exposed Tools** (with JSON schemas):
 
-- `openFile` - Opens files with optional line/text selection, preview mode, and text pattern matching
+- `openFile` - Opens files with optional line/text selection (startLine/endLine), preview mode, text pattern matching, and makeFrontmost flag
 - `getCurrentSelection` - Gets current text selection from active editor
 - `getLatestSelection` - Gets most recent text selection (even from inactive editors)
 - `getOpenEditors` - Lists currently open files with VS Code-compatible `tabs` structure

lua/claudecode/tools/close_all_diff_tabs.lua

@@ -19,32 +19,38 @@ local function handler(_params) -- Prefix unused params with underscore
 
   -- Get all windows
   local windows = vim.api.nvim_list_wins()
-  local windows_to_close = {}
+  local windows_to_close = {} -- Use set to avoid duplicates
 
   for _, win in ipairs(windows) do
     local buf = vim.api.nvim_win_get_buf(win)
     local buftype = vim.api.nvim_buf_get_option(buf, "buftype")
     local diff_mode = vim.api.nvim_win_get_option(win, "diff")
+    local should_close = false
 
     -- Check if this is a diff window
     if diff_mode then
-      table.insert(windows_to_close, win)
+      should_close = true
     end
 
     -- Also check for diff-related buffer names or types
     local buf_name = vim.api.nvim_buf_get_name(buf)
     if buf_name:match("%.diff$") or buf_name:match("diff://") then
-      table.insert(windows_to_close, win)
+      should_close = true
     end
 
     -- Check for special diff buffer types
     if buftype == "nofile" and buf_name:match("^fugitive://") then
-      table.insert(windows_to_close, win)
+      should_close = true
+    end
+
+    -- Add to close set only once (prevents duplicates)
+    if should_close then
+      windows_to_close[win] = true
     end
   end
 
   -- Close the identified diff windows
-  for _, win in ipairs(windows_to_close) do
+  for win, _ in pairs(windows_to_close) do
     if vim.api.nvim_win_is_valid(win) then
       local success = pcall(vim.api.nvim_win_close, win, false)
       if success then

tests/busted_setup.lua

@@ -155,7 +155,14 @@ _G.json_encode = function(data)
 
     return table.concat(parts)
   elseif type(data) == "string" then
-    return '"' .. data:gsub('"', '\\"') .. '"'
+    -- Handle escape sequences properly
+    local escaped = data
+      :gsub("\\", "\\\\") -- Escape backslashes first
+      :gsub('"', '\\"') -- Escape quotes
+      :gsub("\n", "\\n") -- Escape newlines
+      :gsub("\r", "\\r") -- Escape carriage returns
+      :gsub("\t", "\\t") -- Escape tabs
+    return '"' .. escaped .. '"'
   elseif type(data) == "boolean" then
     return data and "true" or "false"
   elseif type(data) == "number" then
@@ -197,7 +204,13 @@ _G.json_decode = function(str)
         end
         pos = pos + 1
       end
-      local value = str:sub(start, pos - 1):gsub('\\"', '"')
+      local value = str
+        :sub(start, pos - 1)
+        :gsub('\\"', '"') -- Unescape quotes
+        :gsub("\\\\", "\\") -- Unescape backslashes
+        :gsub("\\n", "\n") -- Unescape newlines
+        :gsub("\\r", "\r") -- Unescape carriage returns
+        :gsub("\\t", "\t") -- Unescape tabs
       pos = pos + 1
       return value
     elseif char == "{" then

tests/unit/tools/open_file_spec.lua

@@ -57,6 +57,30 @@ describe("Tool: open_file", function()
     _G.vim.api.nvim_get_current_win = spy.new(function()
       return 1000
     end)
+    _G.vim.api.nvim_get_current_buf = spy.new(function()
+      return 1 -- Mock current buffer ID
+    end)
+    _G.vim.api.nvim_buf_get_name = spy.new(function(buf)
+      return "test.txt" -- Mock buffer name
+    end)
+    _G.vim.api.nvim_buf_line_count = spy.new(function(buf)
+      return 10 -- Mock line count
+    end)
+    _G.vim.api.nvim_buf_set_mark = spy.new(function(buf, name, line, col, opts)
+      -- Mock mark setting
+    end)
+    _G.vim.api.nvim_buf_get_lines = spy.new(function(buf, start, end_line, strict)
+      -- Mock buffer lines for search
+      return {
+        "local function test()",
+        "  print('hello')",
+        "  return true",
+        "end",
+      }
+    end)
+    _G.vim.api.nvim_win_set_cursor = spy.new(function(win, pos)
+      -- Mock cursor setting
+    end)
   end)
 
   after_each(function()
@@ -129,10 +153,80 @@ describe("Tool: open_file", function()
     expect(_G.vim.cmd_history[1]).to_be("edit /Users/testuser/.config/nvim/init.lua")
   end)
 
-  -- TODO: Add tests for selection by line numbers (params.startLine, params.endLine)
-  -- This will require mocking vim.api.nvim_win_set_cursor or similar for selection
-  -- and potentially vim.api.nvim_buf_get_lines if text content matters for selection.
+  it("should handle makeFrontmost=false to return detailed JSON", function()
+    local params = { filePath = "test.txt", makeFrontmost = false }
+    local success, result = pcall(open_file_handler, params)
+
+    expect(success).to_be_true()
+    expect(result.content).to_be_table()
+    expect(result.content[1].type).to_be("text")
+
+    -- makeFrontmost=false should return JSON-encoded detailed info
+    local parsed_result = require("tests.busted_setup").json_decode(result.content[1].text)
+    expect(parsed_result.success).to_be_true()
+    expect(parsed_result.filePath).to_be("test.txt")
+  end)
+
+  it("should handle preview mode parameter", function()
+    local params = { filePath = "test.txt", preview = true }
+    local success, result = pcall(open_file_handler, params)
+
+    expect(success).to_be_true()
+    expect(result.content[1].text).to_be("Opened file: test.txt")
+    -- Preview mode affects window behavior but basic functionality should work
+  end)
+
+  it("should handle line selection parameters", function()
+    -- Mock additional functions needed for line selection
+    _G.vim.api.nvim_win_set_cursor = spy.new(function(win, pos)
+      -- Mock cursor setting
+    end)
+    _G.vim.fn.setpos = spy.new(function(mark, pos)
+      -- Mock position setting
+    end)
+
+    local params = { filePath = "test.txt", startLine = 5, endLine = 10 }
+    local success, result = pcall(open_file_handler, params)
 
-  -- TODO: Add tests for selection by text patterns (params.startText, params.endText)
-  -- This will require more complex mocking of buffer content and search functions.
+    expect(success).to_be_true()
+    expect(result.content).to_be_table()
+    expect(result.content[1].type).to_be("text")
+    expect(result.content[1].text).to_be("Opened file and selected lines 5 to 10")
+  end)
+
+  it("should handle text pattern selection when pattern found", function()
+    local params = {
+      filePath = "test.txt",
+      startText = "function",
+      endText = "end",
+      selectToEndOfLine = true,
+    }
+
+    local success, result = pcall(open_file_handler, params)
+
+    expect(success).to_be_true()
+    expect(result.content).to_be_table()
+    expect(result.content[1].type).to_be("text")
+    -- Since the mock buffer contains "function" and "end", selection should work
+    expect(result.content[1].text).to_be('Opened file and selected text from "function" to "end"')
+  end)
+
+  it("should handle text pattern selection when pattern not found", function()
+    -- Mock search to return 0 (not found)
+    _G.vim.fn.search = spy.new(function(pattern)
+      return 0 -- Pattern not found
+    end)
+
+    local params = {
+      filePath = "test.txt",
+      startText = "nonexistent",
+    }
+
+    local success, result = pcall(open_file_handler, params)
+
+    expect(success).to_be_true()
+    expect(result.content).to_be_table()
+    expect(result.content[1].type).to_be("text")
+    assert_contains(result.content[1].text, "not found")
+  end)
 end)