From 1f8d41736f4033c14ddbeb9f234733a9f31956f5 Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Sun, 26 Apr 2026 08:19:18 +0200 Subject: [PATCH 01/21] feat(git): add branch utilities for plan command --- lua/codereview/git.lua | 71 +++++++++++++++++++++++++++++++++++ tests/codereview/git_spec.lua | 38 +++++++++++++++++++ 2 files changed, 109 insertions(+) diff --git a/lua/codereview/git.lua b/lua/codereview/git.lua index 9b0bb78..5d60dab 100644 --- a/lua/codereview/git.lua +++ b/lua/codereview/git.lua @@ -69,4 +69,75 @@ function M.detect_project() return "https://" .. host, project end +function M.get_current_branch() + return shell("git rev-parse --abbrev-ref HEAD 2>/dev/null") +end + +function M.branch_exists(name) + local result = shell("git rev-parse --verify " .. name .. " 2>/dev/null") + return result ~= nil +end + +function M.get_default_base() + if M.branch_exists("main") then + return "main" + elseif M.branch_exists("master") then + return "master" + end + return nil +end + +function M.sanitize_branch_name(name) + if not name then + return nil + end + return name:gsub("/", "-") +end + +function M.diff_against_base(base) + local diff_output = shell("git diff " .. base .. "..HEAD --no-color 2>/dev/null") + if not diff_output or diff_output == "" then + return {} + end + + local files = {} + local current_file = nil + local current_diff = {} + + for line in (diff_output .. "\n"):gmatch("(.-)\n") do + local new_path = line:match("^%+%+%+ b/(.+)$") + local old_path = line:match("^%-%-%- a/(.+)$") + + if line:match("^diff %-%-git") then + if current_file then + table.insert(files, { + new_path = current_file.new_path, + old_path = current_file.old_path, + diff = table.concat(current_diff, "\n"), + }) + end + current_file = {} + current_diff = { line } + elseif old_path and current_file then + current_file.old_path = old_path + table.insert(current_diff, line) + elseif new_path and current_file then + current_file.new_path = new_path + table.insert(current_diff, line) + elseif current_file then + table.insert(current_diff, line) + end + end + + if current_file then + table.insert(files, { + new_path = current_file.new_path, + old_path = current_file.old_path, + diff = table.concat(current_diff, "\n"), + }) + end + + return files +end + return M diff --git a/tests/codereview/git_spec.lua b/tests/codereview/git_spec.lua index abbc6e4..e41bc51 100644 --- a/tests/codereview/git_spec.lua +++ b/tests/codereview/git_spec.lua @@ -42,3 +42,41 @@ describe("git.detect_project", function() config.reset() end) end) + +describe("git.get_current_branch", function() + it("returns branch name", function() + local branch = git.get_current_branch() + assert.is_string(branch) + assert.is_true(#branch > 0) + end) +end) + +describe("git.branch_exists", function() + it("returns true for HEAD", function() + assert.is_true(git.branch_exists("HEAD")) + end) + + it("returns false for non-existent branch", function() + assert.is_false(git.branch_exists("this-branch-does-not-exist-xyz")) + end) +end) + +describe("git.get_default_base", function() + it("returns main or master if exists", function() + local base = git.get_default_base() + -- In most repos, one of these exists + if base then + assert.is_true(base == "main" or base == "master") + end + end) +end) + +describe("git.sanitize_branch_name", function() + it("replaces slashes with dashes", function() + assert.equals("feat-auth-login", git.sanitize_branch_name("feat/auth/login")) + end) + + it("handles branch without slashes", function() + assert.equals("main", git.sanitize_branch_name("main")) + end) +end) From b75ae33aaee8524b3f357b4b7c41e8fb10a479db Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Sun, 26 Apr 2026 08:44:30 +0200 Subject: [PATCH 02/21] feat(plan): add prompt module for plan generation --- lua/codereview/plan/prompt.lua | 145 ++++++++++++++++++++++++++ tests/codereview/plan/prompt_spec.lua | 90 ++++++++++++++++ 2 files changed, 235 insertions(+) create mode 100644 lua/codereview/plan/prompt.lua create mode 100644 tests/codereview/plan/prompt_spec.lua diff --git a/lua/codereview/plan/prompt.lua b/lua/codereview/plan/prompt.lua new file mode 100644 index 0000000..f16cc46 --- /dev/null +++ b/lua/codereview/plan/prompt.lua @@ -0,0 +1,145 @@ +local M = {} +local ai_prompt = require("codereview.ai.prompt") + +function M.build_file_plan_prompt(file, summaries) + local path = file.new_path or file.old_path + local parts = { + "You are creating an implementation plan for changes in a file.", + "", + } + + local others = {} + for fpath, summary in pairs(summaries or {}) do + if fpath ~= path then + table.insert(others, string.format("- `%s`: %s", fpath, summary)) + end + end + if #others > 0 then + table.insert(parts, "## Branch Context (Other Changed Files)") + for _, line in ipairs(others) do + table.insert(parts, line) + end + table.insert(parts, "") + end + + table.insert(parts, "## File: " .. path) + table.insert(parts, "```diff") + table.insert(parts, ai_prompt.annotate_diff_with_lines(file.diff or "")) + table.insert(parts, "```") + table.insert(parts, "") + table.insert(parts, "## Instructions") + table.insert(parts, "") + table.insert(parts, "Analyze this diff and create tasks to complete or improve this implementation.") + table.insert(parts, "Output a JSON array in a ```json code block:") + table.insert(parts, '[{"file": "' .. path .. '", "line": , "task": "", "reason": ""}]') + table.insert(parts, "") + table.insert(parts, "Focus on: incomplete implementations, missing error handling, TODOs, edge cases, missing tests.") + table.insert(parts, "If the code looks complete, output `[]`.") + + return table.concat(parts, "\n") +end + +function M.parse_file_plan_output(output) + if not output or output == "" then + return {} + end + + local json_str = output:match("```json%s*\n(.+)\n```") + if not json_str then + json_str = output:match("%[.+%]") + end + if not json_str then + return {} + end + + local ok, data = pcall(vim.json.decode, json_str) + if not ok or type(data) ~= "table" then + return {} + end + + local tasks = {} + for _, item in ipairs(data) do + if type(item) == "table" and item.file and item.task then + table.insert(tasks, { + file = item.file, + line = tonumber(item.line), + task = item.task, + reason = item.reason or "", + }) + end + end + return tasks +end + +function M.build_combine_prompt(branch, base, tasks) + local parts = { + "You are writing a one-paragraph summary of an implementation plan.", + "", + "## Branch", + branch, + "", + "## Base", + base, + "", + "## Tasks", + } + + for i, t in ipairs(tasks) do + table.insert(parts, string.format("%d. `%s:%s` — %s", i, t.file, t.line or "?", t.task)) + end + + table.insert(parts, "") + table.insert(parts, "## Instructions") + table.insert(parts, "Write a brief (2-4 sentence) summary of what this implementation plan covers.") + table.insert(parts, "Output the summary in a ```markdown code block.") + + return table.concat(parts, "\n") +end + +function M.parse_summary(output) + if not output or output == "" then + return "" + end + local content = output:match("```markdown%s*\n(.-)\n```") + if content then + return vim.trim(content) + end + content = output:match("```[%w]*%s*\n(.-)\n```") + if content then + return vim.trim(content) + end + return vim.trim(output) +end + +function M.format_plan_markdown(branch, base, summary, tasks) + local date = os.date("%Y-%m-%d") + local parts = { + "# Implementation Plan: " .. branch, + "", + "Generated: " .. date, + "Branch: " .. branch, + "Base: " .. base, + "", + "## Summary", + "", + summary or "(No summary generated)", + "", + "## Tasks", + "", + } + + for i, t in ipairs(tasks) do + local line_str = t.line and tostring(t.line) or "?" + table.insert(parts, string.format("### %d. %s:%s", i, t.file, line_str)) + table.insert(parts, t.task) + table.insert(parts, "") + if t.reason and t.reason ~= "" then + table.insert(parts, "**Why:** " .. t.reason) + table.insert(parts, "") + end + end + + return table.concat(parts, "\n") +end + +return M diff --git a/tests/codereview/plan/prompt_spec.lua b/tests/codereview/plan/prompt_spec.lua new file mode 100644 index 0000000..0008546 --- /dev/null +++ b/tests/codereview/plan/prompt_spec.lua @@ -0,0 +1,90 @@ +local prompt = require("codereview.plan.prompt") + +describe("plan.prompt.build_file_plan_prompt", function() + it("includes file path and diff", function() + local file = { + new_path = "lua/test.lua", + diff = "+local x = 1", + } + local result = prompt.build_file_plan_prompt(file, {}) + assert.is_string(result) + assert.matches("lua/test.lua", result) + assert.matches("local x = 1", result) + end) + + it("includes other file summaries", function() + local file = { new_path = "a.lua", diff = "+x" } + local summaries = { ["b.lua"] = "Added helper function" } + local result = prompt.build_file_plan_prompt(file, summaries) + assert.matches("b.lua", result) + assert.matches("Added helper function", result) + end) +end) + +describe("plan.prompt.parse_file_plan_output", function() + it("parses JSON task array", function() + local output = [[ +```json +[{"file": "a.lua", "line": 10, "task": "Add validation", "reason": "Missing check"}] +``` +]] + local tasks = prompt.parse_file_plan_output(output) + assert.equals(1, #tasks) + assert.equals("a.lua", tasks[1].file) + assert.equals(10, tasks[1].line) + assert.equals("Add validation", tasks[1].task) + end) + + it("returns empty array for no issues", function() + local output = "```json\n[]\n```" + local tasks = prompt.parse_file_plan_output(output) + assert.equals(0, #tasks) + end) +end) + +describe("plan.prompt.build_combine_prompt", function() + it("includes all tasks", function() + local tasks = { + { file = "a.lua", line = 10, task = "Do X", reason = "Because Y" }, + } + local result = prompt.build_combine_prompt("feat-test", "main", tasks) + assert.matches("a.lua", result) + assert.matches("Do X", result) + end) +end) + +describe("plan.prompt.parse_summary", function() + it("extracts markdown block", function() + local output = "```markdown\nThis is a summary.\n```" + local result = prompt.parse_summary(output) + assert.equals("This is a summary.", result) + end) + + it("falls back to trimmed output", function() + local output = " Just plain text " + local result = prompt.parse_summary(output) + assert.equals("Just plain text", result) + end) +end) + +describe("plan.prompt.format_plan_markdown", function() + it("formats tasks as markdown", function() + local tasks = { + { file = "a.lua", line = 10, task = "Add validation", reason = "Missing" }, + } + local result = prompt.format_plan_markdown("feat-test", "main", "Summary here", tasks) + assert.matches("# Implementation Plan", result) + assert.matches("a.lua:10", result) + assert.matches("Add validation", result) + assert.matches("Summary here", result) + end) + + it("handles tasks without line numbers", function() + local tasks = { + { file = "b.lua", task = "Refactor module", reason = "Complexity" }, + } + local result = prompt.format_plan_markdown("fix-bug", "main", "Fix summary", tasks) + assert.matches("b.lua", result) + assert.matches("Refactor module", result) + end) +end) From f83744208dc68923bd2bb1a4f85ef3fa94769d86 Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Sun, 26 Apr 2026 08:45:06 +0200 Subject: [PATCH 03/21] feat(plan): add orchestration module for plan generation --- lua/codereview/plan/init.lua | 146 ++++++++++++++++++++++++++++ tests/codereview/plan/init_spec.lua | 30 ++++++ 2 files changed, 176 insertions(+) create mode 100644 lua/codereview/plan/init.lua create mode 100644 tests/codereview/plan/init_spec.lua diff --git a/lua/codereview/plan/init.lua b/lua/codereview/plan/init.lua new file mode 100644 index 0000000..0475283 --- /dev/null +++ b/lua/codereview/plan/init.lua @@ -0,0 +1,146 @@ +local M = {} +local git = require("codereview.git") +local ai_providers = require("codereview.ai.providers") +local ai_prompt = require("codereview.ai.prompt") +local plan_prompt = require("codereview.plan.prompt") +local spinner = require("codereview.ui.spinner") +local log = require("codereview.log") + +function M.resolve_base(arg) + if arg and arg ~= "" then + if git.branch_exists(arg) then + return arg, nil + end + return nil, "Branch '" .. arg .. "' does not exist" + end + + local default = git.get_default_base() + if default then + return default, nil + end + return nil, "No main/master branch found. Specify base: :CodeReviewPlan " +end + +function M.get_output_path(branch) + local date = os.date("%Y-%m-%d") + local sanitized = git.sanitize_branch_name(branch) + local root = git.get_repo_root() or "." + return root .. "/docs/plans/" .. date .. "-" .. sanitized .. "-plan.md" +end + +function M.start(base_arg) + local base, err = M.resolve_base(base_arg) + if err then + vim.notify(err, vim.log.levels.ERROR) + return + end + + local branch = git.get_current_branch() + if not branch then + vim.notify("Could not determine current branch", vim.log.levels.ERROR) + return + end + + if branch == base then + vim.notify("Current branch is the same as base (" .. base .. ")", vim.log.levels.WARN) + return + end + + local diffs = git.diff_against_base(base) + if #diffs == 0 then + vim.notify("No changes found between " .. base .. " and " .. branch, vim.log.levels.INFO) + return + end + + vim.notify(string.format("Generating plan: %s → %s (%d files)", base, branch, #diffs), vim.log.levels.INFO) + spinner.start(" Summarizing files… ") + + -- Phase 1: Summary + local summary_prompt = ai_prompt.build_summary_prompt({ title = branch, description = "" }, diffs) + ai_providers.get().run(summary_prompt, function(output, ai_err) + if ai_err then + spinner.stop() + vim.notify("Summary failed: " .. ai_err, vim.log.levels.ERROR) + return + end + + local summaries = ai_prompt.parse_summary_output(output) + M._run_phase2(branch, base, diffs, summaries) + end, { skip_agent = true }) +end + +function M._run_phase2(branch, base, diffs, summaries) + local all_tasks = {} + local completed = 0 + local total = #diffs + + spinner.set_label(string.format(" Planning 0/%d files… ", total)) + + for _, file in ipairs(diffs) do + local file_prompt = plan_prompt.build_file_plan_prompt(file, summaries) + + ai_providers.get().run(file_prompt, function(output, ai_err) + completed = completed + 1 + spinner.set_label(string.format(" Planning %d/%d files… ", completed, total)) + + if ai_err then + local path = file.new_path or file.old_path + log.warn("Plan failed for " .. path .. ": " .. ai_err) + else + local tasks = plan_prompt.parse_file_plan_output(output) + for _, t in ipairs(tasks) do + table.insert(all_tasks, t) + end + end + + if completed >= total then + M._run_phase3(branch, base, all_tasks) + end + end, { skip_agent = true }) + end +end + +function M._run_phase3(branch, base, tasks) + if #tasks == 0 then + spinner.stop() + vim.notify("No tasks identified — code looks complete!", vim.log.levels.INFO) + return + end + + spinner.set_label(" Writing plan… ") + + local combine_prompt = plan_prompt.build_combine_prompt(branch, base, tasks) + ai_providers.get().run(combine_prompt, function(output, ai_err) + spinner.stop() + + local summary = "" + if ai_err then + log.warn("Summary generation failed: " .. ai_err) + else + summary = plan_prompt.parse_summary(output) + end + + local markdown = plan_prompt.format_plan_markdown(branch, base, summary, tasks) + local path = M.get_output_path(branch) + + -- Ensure directory exists + local dir = path:match("(.+)/[^/]+$") + if dir then + vim.fn.mkdir(dir, "p") + end + + local file = io.open(path, "w") + if file then + file:write(markdown) + file:close() + vim.notify("Plan written to " .. path, vim.log.levels.INFO) + vim.schedule(function() + vim.cmd("edit " .. path) + end) + else + vim.notify("Failed to write plan to " .. path, vim.log.levels.ERROR) + end + end, { skip_agent = true }) +end + +return M diff --git a/tests/codereview/plan/init_spec.lua b/tests/codereview/plan/init_spec.lua new file mode 100644 index 0000000..3659a8e --- /dev/null +++ b/tests/codereview/plan/init_spec.lua @@ -0,0 +1,30 @@ +local plan = require("codereview.plan") + +describe("plan.resolve_base", function() + it("returns provided base if it exists", function() + -- HEAD always exists + local base, err = plan.resolve_base("HEAD") + assert.equals("HEAD", base) + assert.is_nil(err) + end) + + it("returns error for non-existent branch", function() + local base, err = plan.resolve_base("this-branch-does-not-exist-xyz123") + assert.is_nil(base) + assert.is_string(err) + assert.matches("does not exist", err) + end) +end) + +describe("plan.get_output_path", function() + it("generates path with sanitized branch name", function() + local path = plan.get_output_path("feat/auth") + assert.matches("docs/plans/", path) + assert.matches("feat%-auth%-plan.md", path) + end) + + it("includes date in path", function() + local path = plan.get_output_path("main") + assert.matches("%d%d%d%d%-%d%d%-%d%d", path) + end) +end) From 9988eeb5a9bbb7a9094b790cae6077a749b16d01 Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Sun, 26 Apr 2026 08:45:26 +0200 Subject: [PATCH 04/21] feat: add plan() entry point --- lua/codereview/init.lua | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lua/codereview/init.lua b/lua/codereview/init.lua index abf1a95..531654c 100644 --- a/lua/codereview/init.lua +++ b/lua/codereview/init.lua @@ -176,4 +176,8 @@ function M.toggle_scroll_mode() require("codereview.mr.diff_nav").toggle_scroll_mode(active.layout, active.state) end +function M.plan(base_arg) + require("codereview.plan").start(base_arg) +end + return M From b25407238ecdcb1617648f4ceb972bcce0d9d44f Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Sun, 26 Apr 2026 08:45:30 +0200 Subject: [PATCH 05/21] feat: add :CodeReviewPlan command --- plugin/codereview.lua | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/plugin/codereview.lua b/plugin/codereview.lua index 08ebe13..295342a 100644 --- a/plugin/codereview.lua +++ b/plugin/codereview.lua @@ -50,3 +50,7 @@ end, { desc = "Browse commits" }) vim.api.nvim_create_user_command("CodeReviewToggleScroll", function() require("codereview").toggle_scroll_mode() end, { desc = "Toggle scroll/per-file mode" }) + +vim.api.nvim_create_user_command("CodeReviewPlan", function(opts) + require("codereview").plan(opts.args ~= "" and opts.args or nil) +end, { nargs = "?", desc = "Generate implementation plan from current branch" }) From 736c084e08ad728a3fb8fe145b2b32ca6c900f03 Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Sun, 26 Apr 2026 08:45:59 +0200 Subject: [PATCH 06/21] test(plan): add integration tests --- tests/codereview/plan/integration_spec.lua | 29 ++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 tests/codereview/plan/integration_spec.lua diff --git a/tests/codereview/plan/integration_spec.lua b/tests/codereview/plan/integration_spec.lua new file mode 100644 index 0000000..efe9c49 --- /dev/null +++ b/tests/codereview/plan/integration_spec.lua @@ -0,0 +1,29 @@ +describe("CodeReviewPlan integration", function() + it("plan module loads without error", function() + local ok, plan = pcall(require, "codereview.plan") + assert.is_true(ok) + assert.is_table(plan) + assert.is_function(plan.start) + assert.is_function(plan.resolve_base) + assert.is_function(plan.get_output_path) + end) + + it("prompt module loads without error", function() + local ok, prompt = pcall(require, "codereview.plan.prompt") + assert.is_true(ok) + assert.is_function(prompt.build_file_plan_prompt) + assert.is_function(prompt.parse_file_plan_output) + assert.is_function(prompt.build_combine_prompt) + assert.is_function(prompt.parse_summary) + assert.is_function(prompt.format_plan_markdown) + end) + + it("git utilities are available", function() + local git = require("codereview.git") + assert.is_function(git.get_current_branch) + assert.is_function(git.branch_exists) + assert.is_function(git.get_default_base) + assert.is_function(git.sanitize_branch_name) + assert.is_function(git.diff_against_base) + end) +end) From 6a14acc570df6b09f2f907e090c9349c1416d5ad Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Tue, 28 Apr 2026 07:27:15 +0200 Subject: [PATCH 07/21] fix(plan): handle nil paths and limit concurrent AI calls - Add spinner.start/stop aliases for plan module compatibility - Extract path from diff --git line as fallback in git parser - Defensive nil handling for file paths in prompt builder - Limit to max 10 concurrent AI calls with queue-based processing --- lua/codereview/ai/prompt.lua | 3 +- lua/codereview/git.lua | 4 ++- lua/codereview/plan/init.lua | 53 ++++++++++++++++++++++------------- lua/codereview/ui/spinner.lua | 11 ++++++++ 4 files changed, 50 insertions(+), 21 deletions(-) diff --git a/lua/codereview/ai/prompt.lua b/lua/codereview/ai/prompt.lua index ca0e40c..f0ef745 100644 --- a/lua/codereview/ai/prompt.lua +++ b/lua/codereview/ai/prompt.lua @@ -439,7 +439,8 @@ function M.build_summary_prompt(review, diffs) } for _, file in ipairs(diffs) do - table.insert(parts, "### " .. (file.new_path or file.old_path)) + local path = file.new_path or file.old_path or "(unknown)" + table.insert(parts, "### " .. path) table.insert(parts, "```diff") table.insert(parts, file.diff or "") table.insert(parts, "```") diff --git a/lua/codereview/git.lua b/lua/codereview/git.lua index 5d60dab..d50ea31 100644 --- a/lua/codereview/git.lua +++ b/lua/codereview/git.lua @@ -116,7 +116,9 @@ function M.diff_against_base(base) diff = table.concat(current_diff, "\n"), }) end - current_file = {} + -- Extract path from "diff --git a/path b/path" as fallback + local git_path = line:match("^diff %-%-git a/(.+) b/") + current_file = { new_path = git_path, old_path = git_path } current_diff = { line } elseif old_path and current_file then current_file.old_path = old_path diff --git a/lua/codereview/plan/init.lua b/lua/codereview/plan/init.lua index 0475283..ca47a7e 100644 --- a/lua/codereview/plan/init.lua +++ b/lua/codereview/plan/init.lua @@ -69,35 +69,50 @@ function M.start(base_arg) end, { skip_agent = true }) end +local MAX_CONCURRENT = 10 + function M._run_phase2(branch, base, diffs, summaries) local all_tasks = {} local completed = 0 local total = #diffs + local next_idx = 1 + local active = 0 spinner.set_label(string.format(" Planning 0/%d files… ", total)) - for _, file in ipairs(diffs) do - local file_prompt = plan_prompt.build_file_plan_prompt(file, summaries) - - ai_providers.get().run(file_prompt, function(output, ai_err) - completed = completed + 1 - spinner.set_label(string.format(" Planning %d/%d files… ", completed, total)) - - if ai_err then - local path = file.new_path or file.old_path - log.warn("Plan failed for " .. path .. ": " .. ai_err) - else - local tasks = plan_prompt.parse_file_plan_output(output) - for _, t in ipairs(tasks) do - table.insert(all_tasks, t) + local function process_next() + while active < MAX_CONCURRENT and next_idx <= total do + local file = diffs[next_idx] + next_idx = next_idx + 1 + active = active + 1 + + local file_prompt = plan_prompt.build_file_plan_prompt(file, summaries) + + ai_providers.get().run(file_prompt, function(output, ai_err) + active = active - 1 + completed = completed + 1 + spinner.set_label(string.format(" Planning %d/%d files… ", completed, total)) + + if ai_err then + local path = file.new_path or file.old_path + log.warn("Plan failed for " .. path .. ": " .. ai_err) + else + local tasks = plan_prompt.parse_file_plan_output(output) + for _, t in ipairs(tasks) do + table.insert(all_tasks, t) + end end - end - if completed >= total then - M._run_phase3(branch, base, all_tasks) - end - end, { skip_agent = true }) + if completed >= total then + M._run_phase3(branch, base, all_tasks) + else + process_next() + end + end, { skip_agent = true }) + end end + + process_next() end function M._run_phase3(branch, base, tasks) diff --git a/lua/codereview/ui/spinner.lua b/lua/codereview/ui/spinner.lua index 538fac9..f34eb0d 100644 --- a/lua/codereview/ui/spinner.lua +++ b/lua/codereview/ui/spinner.lua @@ -79,4 +79,15 @@ function M.close() current_label = DEFAULT_LABEL end +--- Open spinner with optional initial label. +---@param label? string +function M.start(label) + if label then + current_label = label + end + M.open() +end + +M.stop = M.close + return M From 8dbff1de86a95afd04d4728843c6b89aaac806f9 Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Wed, 29 Apr 2026 07:21:46 +0200 Subject: [PATCH 08/21] refactor(ai): extract shared orchestrator from plan/review --- lua/codereview/ai/orchestrator.lua | 78 ++++++++ lua/codereview/plan/init.lua | 64 +++---- lua/codereview/review/init.lua | 86 ++++----- tests/codereview/ai/orchestrator_spec.lua | 219 ++++++++++++++++++++++ 4 files changed, 365 insertions(+), 82 deletions(-) create mode 100644 lua/codereview/ai/orchestrator.lua create mode 100644 tests/codereview/ai/orchestrator_spec.lua diff --git a/lua/codereview/ai/orchestrator.lua b/lua/codereview/ai/orchestrator.lua new file mode 100644 index 0000000..cdf3260 --- /dev/null +++ b/lua/codereview/ai/orchestrator.lua @@ -0,0 +1,78 @@ +local ai_providers = require("codereview.ai.providers") +local M = {} + +--- Run a parallel-batch AI orchestration loop. +--- +--- @param spec table +--- diffs: list of {new_path, old_path, diff} +--- build_prompt: fn(batch, opts) -> string|table +--- parse_output: fn(text) -> results array +--- on_result: fn(result)? fires once per parsed item +--- on_batch_complete fn(batch, parsed)? fires after each successful batch +--- on_error: fn(err, batch)? fires on per-batch failure +--- on_complete: fn(all_results) +--- provider_opts: table? forwarded to provider.run +--- max_concurrent: integer? (default 10) +function M.run(spec) + local diffs = spec.diffs or {} + local total = #diffs + if total == 0 then + if spec.on_complete then + spec.on_complete({}) + end + return + end + + -- Task 0: one file per batch. Task 5 swaps this for batch.build(diffs, ...). + local batches = {} + for _, f in ipairs(diffs) do + table.insert(batches, { f }) + end + + local results = {} + local completed, next_idx, active = 0, 1, 0 + local max_concurrent = spec.max_concurrent or 10 + + local function process_next() + while active < max_concurrent and next_idx <= #batches do + local batch = batches[next_idx] + next_idx = next_idx + 1 + active = active + 1 + + local prompt = spec.build_prompt(batch, {}) + ai_providers.get().run(prompt, function(output, err) + active = active - 1 + completed = completed + 1 + + if err then + if spec.on_error then + spec.on_error(err, batch) + end + else + local parsed = spec.parse_output(output) or {} + for _, r in ipairs(parsed) do + table.insert(results, r) + if spec.on_result then + spec.on_result(r) + end + end + if spec.on_batch_complete then + spec.on_batch_complete(batch, parsed) + end + end + + if completed >= #batches then + if spec.on_complete then + spec.on_complete(results) + end + else + process_next() + end + end, spec.provider_opts) + end + end + + process_next() +end + +return M diff --git a/lua/codereview/plan/init.lua b/lua/codereview/plan/init.lua index ca47a7e..82ef530 100644 --- a/lua/codereview/plan/init.lua +++ b/lua/codereview/plan/init.lua @@ -3,6 +3,7 @@ local git = require("codereview.git") local ai_providers = require("codereview.ai.providers") local ai_prompt = require("codereview.ai.prompt") local plan_prompt = require("codereview.plan.prompt") +local orchestrator = require("codereview.ai.orchestrator") local spinner = require("codereview.ui.spinner") local log = require("codereview.log") @@ -69,50 +70,33 @@ function M.start(base_arg) end, { skip_agent = true }) end -local MAX_CONCURRENT = 10 - function M._run_phase2(branch, base, diffs, summaries) - local all_tasks = {} - local completed = 0 local total = #diffs - local next_idx = 1 - local active = 0 - + local completed_count = 0 spinner.set_label(string.format(" Planning 0/%d files… ", total)) - local function process_next() - while active < MAX_CONCURRENT and next_idx <= total do - local file = diffs[next_idx] - next_idx = next_idx + 1 - active = active + 1 - - local file_prompt = plan_prompt.build_file_plan_prompt(file, summaries) - - ai_providers.get().run(file_prompt, function(output, ai_err) - active = active - 1 - completed = completed + 1 - spinner.set_label(string.format(" Planning %d/%d files… ", completed, total)) - - if ai_err then - local path = file.new_path or file.old_path - log.warn("Plan failed for " .. path .. ": " .. ai_err) - else - local tasks = plan_prompt.parse_file_plan_output(output) - for _, t in ipairs(tasks) do - table.insert(all_tasks, t) - end - end - - if completed >= total then - M._run_phase3(branch, base, all_tasks) - else - process_next() - end - end, { skip_agent = true }) - end - end - - process_next() + orchestrator.run({ + diffs = diffs, + build_prompt = function(batch) + return plan_prompt.build_file_plan_prompt(batch[1], summaries) + end, + parse_output = plan_prompt.parse_file_plan_output, + on_result = function() end, + on_batch_complete = function() + completed_count = completed_count + 1 + spinner.set_label(string.format(" Planning %d/%d files… ", completed_count, total)) + end, + on_error = function(err, batch) + local p = batch[1].new_path or batch[1].old_path + log.warn("Plan failed for " .. (p or "?") .. ": " .. err) + completed_count = completed_count + 1 + spinner.set_label(string.format(" Planning %d/%d files… ", completed_count, total)) + end, + on_complete = function(all_tasks) + M._run_phase3(branch, base, all_tasks) + end, + provider_opts = { skip_agent = true }, + }) end function M._run_phase3(branch, base, tasks) diff --git a/lua/codereview/review/init.lua b/lua/codereview/review/init.lua index e6311ae..5eb62fc 100644 --- a/lua/codereview/review/init.lua +++ b/lua/codereview/review/init.lua @@ -2,6 +2,7 @@ local ai_providers = require("codereview.ai.providers") local prompt_mod = require("codereview.ai.prompt") local diff_state_mod = require("codereview.mr.diff_state") +local orchestrator = require("codereview.ai.orchestrator") local log = require("codereview.log") local M = {} @@ -188,60 +189,61 @@ local function start_multi(review, diff_state, layout) local summaries = prompt_mod.parse_summary_output(output) - -- Phase 2: parallel per-file reviews + -- Phase 2: parallel per-file reviews via orchestrator local total = #diffs - local job_ids = {} - for _, file in ipairs(diffs) do - local path = file.new_path or file.old_path - local content = fetch_file_content(diff_state, review, path, file.deleted_file) - local file_prompt = prompt_mod.build_file_review_prompt(review, file, summaries, content) - - local file_job = ai_providers.get().run(file_prompt, function(file_output, file_err) - if file_err then - vim.notify("AI review failed for " .. path .. ": " .. file_err, vim.log.levels.WARN) - else - local suggestions = prompt_mod.parse_review_output(file_output) - suggestions = prompt_mod.filter_unchanged_lines(suggestions, { file }) - if #suggestions > 0 then - render_file_suggestions(diff_state, layout, suggestions) - end + orchestrator.run({ + diffs = diffs, + build_prompt = function(batch) + local file = batch[1] + local path = file.new_path or file.old_path + local content = fetch_file_content(diff_state, review, path, file.deleted_file) + return prompt_mod.build_file_review_prompt(review, file, summaries, content) + end, + parse_output = prompt_mod.parse_review_output, + on_result = function() end, + on_batch_complete = function(batch, parsed) + local file = batch[1] + local suggestions = prompt_mod.filter_unchanged_lines(parsed, { file }) + if #suggestions > 0 then + render_file_suggestions(diff_state, layout, suggestions) end - - -- Update progress local s = session.get() spinner.set_label(string.format(" AI reviewing… %d/%d files ", s.ai_completed + 1, s.ai_total)) - vim.schedule(function() local diff_mod = require("codereview.mr.diff") diff_mod.render_sidebar(layout.sidebar_buf, diff_state) end) - session.ai_file_done() - - -- All done? - if not session.get().ai_pending then - local count = #(diff_state.ai_suggestions or {}) - if count == 0 then - vim.schedule(function() - vim.notify("AI review: no issues found!", vim.log.levels.INFO) - end) - else - vim.schedule(function() - vim.notify(string.format("AI review: %d suggestions found", count), vim.log.levels.INFO) - end) - end - generate_summary_with_callbacks(diff_state, review, diffs) + end, + on_error = function(err, batch) + local path = batch[1].new_path or batch[1].old_path + vim.notify("AI review failed for " .. path .. ": " .. err, vim.log.levels.WARN) + local s = session.get() + spinner.set_label(string.format(" AI reviewing… %d/%d files ", s.ai_completed + 1, s.ai_total)) + vim.schedule(function() + local diff_mod = require("codereview.mr.diff") + diff_mod.render_sidebar(layout.sidebar_buf, diff_state) + end) + session.ai_file_done() + end, + on_complete = function() + local count = #(diff_state.ai_suggestions or {}) + if count == 0 then + vim.schedule(function() + vim.notify("AI review: no issues found!", vim.log.levels.INFO) + end) + else + vim.schedule(function() + vim.notify(string.format("AI review: %d suggestions found", count), vim.log.levels.INFO) + end) end - end) - - if file_job and file_job > 0 then - table.insert(job_ids, file_job) - end - end + generate_summary_with_callbacks(diff_state, review, diffs) + end, + }) - -- Store all job IDs for cancellation; update session with real counts - session.ai_start(job_ids, total) + -- Update session with real file count (no individual job IDs with orchestrator) + session.ai_start({}, total) spinner.set_label(string.format(" AI reviewing… 0/%d files ", total)) end, { skip_agent = true }) -- no --agent for summary call diff --git a/tests/codereview/ai/orchestrator_spec.lua b/tests/codereview/ai/orchestrator_spec.lua new file mode 100644 index 0000000..f6e10cb --- /dev/null +++ b/tests/codereview/ai/orchestrator_spec.lua @@ -0,0 +1,219 @@ +-- Stub vim globals for busted +_G.vim = _G.vim or {} +vim.json = vim.json or {} + +local ok, cjson = pcall(require, "cjson") +if ok then + vim.json.decode = vim.json.decode or function(s) + return cjson.decode(s) + end +else + vim.json.decode = vim.json.decode + or function(s) + s = s:match("^%s*(.-)%s*$") + if s == "[]" then + return {} + end + local inner = s:match("^%[(.*)%]$") + if not inner then + error("not an array: " .. s) + end + local result = {} + local depth = 0 + local obj_start = nil + for i = 1, #inner do + local c = inner:sub(i, i) + if c == "{" then + depth = depth + 1 + if depth == 1 then + obj_start = i + end + elseif c == "}" then + depth = depth - 1 + if depth == 0 and obj_start then + local obj_str = inner:sub(obj_start, i) + local obj = {} + for key, val in obj_str:gmatch('"([^"]+)"%s*:%s*(-?%d+)') do + obj[key] = tonumber(val) + end + for key, val in obj_str:gmatch('"([^"]+)"%s*:%s*"([^"]*)"') do + obj[key] = val + end + table.insert(result, obj) + obj_start = nil + end + end + end + return result + end +end + +describe("orchestrator", function() + local orchestrator = require("codereview.ai.orchestrator") + + it("calls build_prompt once per file (batch size 1) and parses output", function() + local prompts, results = {}, {} + local fake_provider = { + run = function(p, cb) + table.insert(prompts, p) + cb('[{"x":1}]') + end, + } + package.loaded["codereview.ai.providers"] = { + get = function() + return fake_provider + end, + } + package.loaded["codereview.ai.orchestrator"] = nil + orchestrator = require("codereview.ai.orchestrator") + + local done = false + orchestrator.run({ + diffs = { { new_path = "a", diff = "" }, { new_path = "b", diff = "" } }, + build_prompt = function(batch) + return "P:" .. batch[1].new_path + end, + parse_output = function(t) + return vim.json.decode(t) + end, + on_result = function(r) + table.insert(results, r) + end, + on_complete = function() + done = true + end, + max_concurrent = 2, + }) + assert.are.equal(2, #prompts) + assert.are.equal(2, #results) + assert.is_true(done) + end) + + it("on_error fires when provider returns error; on_complete still fires", function() + local fake = { + run = function(_, cb) + cb(nil, "boom") + end, + } + package.loaded["codereview.ai.providers"] = { + get = function() + return fake + end, + } + package.loaded["codereview.ai.orchestrator"] = nil + orchestrator = require("codereview.ai.orchestrator") + + local errs, done = {}, false + orchestrator.run({ + diffs = { { new_path = "a", diff = "" } }, + build_prompt = function() + return "" + end, + parse_output = function() + return {} + end, + on_result = function() end, + on_error = function(e) + table.insert(errs, e) + end, + on_complete = function() + done = true + end, + }) + assert.are.equal(1, #errs) + assert.is_true(done) + end) + + it("on_batch_complete fires once per successful batch with parsed results", function() + local batch_calls = {} + local fake_provider = { + run = function(_, cb) + cb('[{"x":1}]') + end, + } + package.loaded["codereview.ai.providers"] = { + get = function() + return fake_provider + end, + } + package.loaded["codereview.ai.orchestrator"] = nil + orchestrator = require("codereview.ai.orchestrator") + + orchestrator.run({ + diffs = { { new_path = "a", diff = "" }, { new_path = "b", diff = "" } }, + build_prompt = function(batch) + return batch[1].new_path + end, + parse_output = function(t) + return vim.json.decode(t) + end, + on_result = function() end, + on_batch_complete = function(batch, parsed) + table.insert(batch_calls, { path = batch[1].new_path, count = #parsed }) + end, + on_complete = function() end, + }) + assert.are.equal(2, #batch_calls) + assert.are.equal(1, batch_calls[1].count) + assert.are.equal(1, batch_calls[2].count) + end) + + it("on_batch_complete does NOT fire when provider errors", function() + local batch_calls = {} + local fake_provider = { + run = function(_, cb) + cb(nil, "err") + end, + } + package.loaded["codereview.ai.providers"] = { + get = function() + return fake_provider + end, + } + package.loaded["codereview.ai.orchestrator"] = nil + orchestrator = require("codereview.ai.orchestrator") + + orchestrator.run({ + diffs = { { new_path = "a", diff = "" } }, + build_prompt = function() + return "" + end, + parse_output = function() + return {} + end, + on_result = function() end, + on_batch_complete = function(batch, parsed) + table.insert(batch_calls, { batch = batch, parsed = parsed }) + end, + on_error = function() end, + on_complete = function() end, + }) + assert.are.equal(0, #batch_calls) + end) + + it("on_complete fires with empty results when diffs is empty", function() + package.loaded["codereview.ai.providers"] = { + get = function() + return { run = function() end } + end, + } + package.loaded["codereview.ai.orchestrator"] = nil + orchestrator = require("codereview.ai.orchestrator") + + local complete_results + orchestrator.run({ + diffs = {}, + build_prompt = function() + return "" + end, + parse_output = function() + return {} + end, + on_result = function() end, + on_complete = function(r) + complete_results = r + end, + }) + assert.same({}, complete_results) + end) +end) From d9b621a847e11c35c18a0cf93589762eafcdc31d Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Wed, 29 Apr 2026 07:25:35 +0200 Subject: [PATCH 09/21] feat(ai): skip lockfiles, generated, vendored and binary diffs --- lua/codereview/ai/file_filter.lua | 82 ++++++++++++++++++++++++ lua/codereview/ai/orchestrator.lua | 11 +++- lua/codereview/config.lua | 2 + tests/codereview/ai/file_filter_spec.lua | 41 ++++++++++++ 4 files changed, 135 insertions(+), 1 deletion(-) create mode 100644 lua/codereview/ai/file_filter.lua create mode 100644 tests/codereview/ai/file_filter_spec.lua diff --git a/lua/codereview/ai/file_filter.lua b/lua/codereview/ai/file_filter.lua new file mode 100644 index 0000000..8890d94 --- /dev/null +++ b/lua/codereview/ai/file_filter.lua @@ -0,0 +1,82 @@ +local M = {} + +M.DEFAULT_PATTERNS = { + "*.lock", + "package-lock.json", + "yarn.lock", + "pnpm-lock.yaml", + "Cargo.lock", + "Gemfile.lock", + "poetry.lock", + "uv.lock", + "go.sum", + "composer.lock", + "Pipfile.lock", + "*.min.js", + "*.min.css", + "*.map", + "*.pb.go", + "*_pb2.py", + "*.generated.*", + "node_modules/**", + "vendor/**", + "third_party/**", + "dist/**", + "build/**", + ".next/**", +} + +--- Convert a glob pattern to a Lua pattern. +--- Rules: escape magic chars, ** → .*, * → [^/]*, ? → [^/] +---@param glob string +---@return string +local function glob_to_pat(glob) + local pat = glob:gsub("([%%%.%(%)%[%]%+%-%^%$])", "%%%1") + pat = pat:gsub("%*%*", "\0DBL\0"):gsub("%*", "[^/]*"):gsub("%?", "[^/]"):gsub("\0DBL\0", ".*") + return "^" .. pat .. "$" +end + +---@param path string +---@param globs string[] +---@return boolean +local function matches_any(path, globs) + local base = path:match("([^/]+)$") or path + for _, g in ipairs(globs) do + local pat = glob_to_pat(g) + if path:match(pat) or base:match(pat) then + return true + end + end + return false +end + +---@param diff string? +---@return boolean +local function is_binary_diff(diff) + return diff ~= nil and diff:find("\nBinary files ", 1, true) ~= nil +end + +--- Filter diffs, removing lockfiles, generated files, vendored dirs, and binary diffs. +---@param diffs table[] list of {new_path, old_path, diff} +---@param user_patterns string[]? additional glob patterns to skip +---@return table[] +function M.apply(diffs, user_patterns) + local globs = {} + for _, g in ipairs(M.DEFAULT_PATTERNS) do + table.insert(globs, g) + end + for _, g in ipairs(user_patterns or {}) do + table.insert(globs, g) + end + + local out = {} + for _, f in ipairs(diffs) do + local path = f.new_path or f.old_path + if path and not matches_any(path, globs) and not is_binary_diff(f.diff) then + table.insert(out, f) + end + end + return out +end + +return M diff --git a/lua/codereview/ai/orchestrator.lua b/lua/codereview/ai/orchestrator.lua index cdf3260..9390b24 100644 --- a/lua/codereview/ai/orchestrator.lua +++ b/lua/codereview/ai/orchestrator.lua @@ -14,7 +14,16 @@ local M = {} --- provider_opts: table? forwarded to provider.run --- max_concurrent: integer? (default 10) function M.run(spec) - local diffs = spec.diffs or {} + local cfg = require("codereview.config").get() + local file_filter = require("codereview.ai.file_filter") + local before = #(spec.diffs or {}) + spec.diffs = file_filter.apply(spec.diffs or {}, (cfg.ai or {}).skip_patterns) + local skipped = before - #spec.diffs + if skipped > 0 then + vim.notify(string.format("Skipped %d file(s) (lockfiles/generated/binary)", skipped), vim.log.levels.INFO) + end + + local diffs = spec.diffs local total = #diffs if total == 0 then if spec.on_complete then diff --git a/lua/codereview/config.lua b/lua/codereview/config.lua index 1e09a02..ac0a35b 100644 --- a/lua/codereview/config.lua +++ b/lua/codereview/config.lua @@ -30,6 +30,7 @@ ---@field provider? "claude_cli"|"codex_cli"|"copilot_cli"|"gemini_cli"|"opencode_cli"|"qwen_cli"|"anthropic"|"openai"|"ollama"|"custom_cmd" AI Provider to use ---@field review_level? "info"|"suggestion"|"warning"|"error" controls the verbosity of AI code reviews (default: `info`) ---@field max_file_size? integer skip files larger than N lines (0 = unlimited) (default: 500) +---@field skip_patterns? string[] additional glob patterns to skip (additive to defaults) ---@field claude_cli? codereview.config.ai.ClaudeCLI Claude CLI options ---@field codex_cli? codereview.config.ai.CodexCLI Codex CLI options ---@field copilot_cli? codereview.config.ai.CopilotCLI Copilot CLI options @@ -115,6 +116,7 @@ local defaults = { openai = { api_key = nil, model = "gpt-4o", base_url = nil }, ollama = { model = "llama3", base_url = "http://localhost:11434" }, custom_cmd = { cmd = nil, args = {} }, + skip_patterns = {}, }, keymaps = {}, } diff --git a/tests/codereview/ai/file_filter_spec.lua b/tests/codereview/ai/file_filter_spec.lua new file mode 100644 index 0000000..2948d4b --- /dev/null +++ b/tests/codereview/ai/file_filter_spec.lua @@ -0,0 +1,41 @@ +local filter = require("codereview.ai.file_filter") + +describe("file_filter", function() + it("skips lockfiles", function() + local out = filter.apply({ + { new_path = "src/foo.lua", diff = "" }, + { new_path = "package-lock.json", diff = "" }, + { new_path = "Cargo.lock", diff = "" }, + { new_path = "pnpm-lock.yaml", diff = "" }, + }, nil) + assert.are.equal(1, #out) + end) + + it("skips minified, maps, vendored", function() + local out = filter.apply({ + { new_path = "x.min.js", diff = "" }, + { new_path = "x.css.map", diff = "" }, + { new_path = "node_modules/x/y.js", diff = "" }, + { new_path = "ok.js", diff = "" }, + }, nil) + assert.are.equal(1, #out) + end) + + it("skips binary diffs", function() + local out = filter.apply({ + { new_path = "img.png", diff = "diff --git a/img.png b/img.png\nBinary files differ" }, + { new_path = "ok.lua", diff = "@@" }, + }, nil) + assert.are.equal(1, #out) + end) + + it("user patterns additive", function() + local out = filter.apply({ { new_path = "x.json", diff = "" }, { new_path = "ok.lua", diff = "" } }, { "*.json" }) + assert.are.equal(1, #out) + end) + + it("nil path skipped", function() + local out = filter.apply({ { new_path = nil, old_path = nil, diff = "" } }, nil) + assert.are.equal(0, #out) + end) +end) From 3a0188bd0c016784ad02cc0f9db15382a78708fe Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Wed, 29 Apr 2026 07:30:06 +0200 Subject: [PATCH 10/21] refactor(ai): drop Phase 1 summary and per-file Branch Context --- lua/codereview/ai/prompt.lua | 70 +-------- lua/codereview/plan/init.lua | 20 +-- lua/codereview/plan/prompt.lua | 16 +-- lua/codereview/review/init.lua | 198 +++++++++++--------------- tests/codereview/ai/prompt_spec.lua | 89 ++---------- tests/codereview/plan/prompt_spec.lua | 11 +- tests/codereview/review/init_spec.lua | 40 ++---- 7 files changed, 114 insertions(+), 330 deletions(-) diff --git a/lua/codereview/ai/prompt.lua b/lua/codereview/ai/prompt.lua index f0ef745..cc08f5d 100644 --- a/lua/codereview/ai/prompt.lua +++ b/lua/codereview/ai/prompt.lua @@ -342,7 +342,7 @@ function M.parse_mr_draft(output) return vim.trim(title), vim.trim(description) end -function M.build_file_review_prompt(review, file, summaries, content) +function M.build_file_review_prompt(review, file, content) local path = file.new_path or file.old_path local parts = { "You are reviewing a single file in a merge request.", @@ -355,21 +355,6 @@ function M.build_file_review_prompt(review, file, summaries, content) "", } - -- Other changed files with summaries - local others = {} - for fpath, summary in pairs(summaries or {}) do - if fpath ~= path then - table.insert(others, string.format("- `%s`: %s", fpath, summary)) - end - end - if #others > 0 then - table.insert(parts, "## Other Changed Files in This MR") - for _, line in ipairs(others) do - table.insert(parts, line) - end - table.insert(parts, "") - end - if content and content ~= "" then table.insert(parts, "## Full File Content: " .. path) local ext = path:match("%.([^%.]+)$") or "" @@ -424,57 +409,4 @@ function M.build_file_review_prompt(review, file, summaries, content) return table.concat(parts, "\n") end -function M.build_summary_prompt(review, diffs) - local parts = { - "You are summarizing changes in a merge request for context.", - "", - "## MR Title", - review.title or "", - "", - "## MR Description", - review.description or "(no description)", - "", - "## Changed Files", - "", - } - - for _, file in ipairs(diffs) do - local path = file.new_path or file.old_path or "(unknown)" - table.insert(parts, "### " .. path) - table.insert(parts, "```diff") - table.insert(parts, file.diff or "") - table.insert(parts, "```") - table.insert(parts, "") - end - - table.insert(parts, "## Instructions") - table.insert(parts, "") - table.insert(parts, "For each file, write a one-sentence summary of what changed.") - table.insert(parts, "Output a JSON object in a ```json code block:") - table.insert(parts, '{"path/to/file.lua": "Summary of changes", ...}') - - return table.concat(parts, "\n") -end - -function M.parse_summary_output(output) - if not output or output == "" then - return {} - end - - local json_str = output:match("```json%s*\n(.+)\n```") - if not json_str then - json_str = output:match("%{.+%}") - end - if not json_str then - return {} - end - - local ok, data = pcall(vim.json.decode, json_str) - if not ok or type(data) ~= "table" then - return {} - end - - return data -end - return M diff --git a/lua/codereview/plan/init.lua b/lua/codereview/plan/init.lua index 82ef530..be74b97 100644 --- a/lua/codereview/plan/init.lua +++ b/lua/codereview/plan/init.lua @@ -1,7 +1,6 @@ local M = {} local git = require("codereview.git") local ai_providers = require("codereview.ai.providers") -local ai_prompt = require("codereview.ai.prompt") local plan_prompt = require("codereview.plan.prompt") local orchestrator = require("codereview.ai.orchestrator") local spinner = require("codereview.ui.spinner") @@ -54,23 +53,12 @@ function M.start(base_arg) end vim.notify(string.format("Generating plan: %s → %s (%d files)", base, branch, #diffs), vim.log.levels.INFO) - spinner.start(" Summarizing files… ") + spinner.start(" Planning files… ") - -- Phase 1: Summary - local summary_prompt = ai_prompt.build_summary_prompt({ title = branch, description = "" }, diffs) - ai_providers.get().run(summary_prompt, function(output, ai_err) - if ai_err then - spinner.stop() - vim.notify("Summary failed: " .. ai_err, vim.log.levels.ERROR) - return - end - - local summaries = ai_prompt.parse_summary_output(output) - M._run_phase2(branch, base, diffs, summaries) - end, { skip_agent = true }) + M._run_phase2(branch, base, diffs) end -function M._run_phase2(branch, base, diffs, summaries) +function M._run_phase2(branch, base, diffs) local total = #diffs local completed_count = 0 spinner.set_label(string.format(" Planning 0/%d files… ", total)) @@ -78,7 +66,7 @@ function M._run_phase2(branch, base, diffs, summaries) orchestrator.run({ diffs = diffs, build_prompt = function(batch) - return plan_prompt.build_file_plan_prompt(batch[1], summaries) + return plan_prompt.build_file_plan_prompt(batch[1]) end, parse_output = plan_prompt.parse_file_plan_output, on_result = function() end, diff --git a/lua/codereview/plan/prompt.lua b/lua/codereview/plan/prompt.lua index f16cc46..5480196 100644 --- a/lua/codereview/plan/prompt.lua +++ b/lua/codereview/plan/prompt.lua @@ -1,27 +1,13 @@ local M = {} local ai_prompt = require("codereview.ai.prompt") -function M.build_file_plan_prompt(file, summaries) +function M.build_file_plan_prompt(file) local path = file.new_path or file.old_path local parts = { "You are creating an implementation plan for changes in a file.", "", } - local others = {} - for fpath, summary in pairs(summaries or {}) do - if fpath ~= path then - table.insert(others, string.format("- `%s`: %s", fpath, summary)) - end - end - if #others > 0 then - table.insert(parts, "## Branch Context (Other Changed Files)") - for _, line in ipairs(others) do - table.insert(parts, line) - end - table.insert(parts, "") - end - table.insert(parts, "## File: " .. path) table.insert(parts, "```diff") table.insert(parts, ai_prompt.annotate_diff_with_lines(file.diff or "")) diff --git a/lua/codereview/review/init.lua b/lua/codereview/review/init.lua index 5eb62fc..a973dac 100644 --- a/lua/codereview/review/init.lua +++ b/lua/codereview/review/init.lua @@ -163,7 +163,7 @@ local function start_single(review, diff_state, layout) end end ---- Multi-file review: Phase 1 (summary) then Phase 2 (parallel per-file). +--- Multi-file review: parallel per-file reviews via orchestrator. local function start_multi(review, diff_state, layout) local diffs = diff_state.files local session = require("codereview.review.session") @@ -178,84 +178,64 @@ local function start_multi(review, diff_state, layout) diff_state.current_file = diff_state.current_file or 1 end - -- Phase 1: summary pre-pass - local summary_prompt = prompt_mod.build_summary_prompt(review, diffs) - local summary_job = ai_providers.get().run(summary_prompt, function(output, ai_err) - if ai_err then - session.ai_finish() - vim.notify("AI summary failed: " .. ai_err, vim.log.levels.ERROR) - return - end - - local summaries = prompt_mod.parse_summary_output(output) - - -- Phase 2: parallel per-file reviews via orchestrator - local total = #diffs - - orchestrator.run({ - diffs = diffs, - build_prompt = function(batch) - local file = batch[1] - local path = file.new_path or file.old_path - local content = fetch_file_content(diff_state, review, path, file.deleted_file) - return prompt_mod.build_file_review_prompt(review, file, summaries, content) - end, - parse_output = prompt_mod.parse_review_output, - on_result = function() end, - on_batch_complete = function(batch, parsed) - local file = batch[1] - local suggestions = prompt_mod.filter_unchanged_lines(parsed, { file }) - if #suggestions > 0 then - render_file_suggestions(diff_state, layout, suggestions) - end - local s = session.get() - spinner.set_label(string.format(" AI reviewing… %d/%d files ", s.ai_completed + 1, s.ai_total)) + local total = #diffs + + orchestrator.run({ + diffs = diffs, + build_prompt = function(batch) + local file = batch[1] + local path = file.new_path or file.old_path + local content = fetch_file_content(diff_state, review, path, file.deleted_file) + return prompt_mod.build_file_review_prompt(review, file, content) + end, + parse_output = prompt_mod.parse_review_output, + on_result = function() end, + on_batch_complete = function(batch, parsed) + local file = batch[1] + local suggestions = prompt_mod.filter_unchanged_lines(parsed, { file }) + if #suggestions > 0 then + render_file_suggestions(diff_state, layout, suggestions) + end + local s = session.get() + spinner.set_label(string.format(" AI reviewing… %d/%d files ", s.ai_completed + 1, s.ai_total)) + vim.schedule(function() + local diff_mod = require("codereview.mr.diff") + diff_mod.render_sidebar(layout.sidebar_buf, diff_state) + end) + session.ai_file_done() + end, + on_error = function(err, batch) + local path = batch[1].new_path or batch[1].old_path + vim.notify("AI review failed for " .. path .. ": " .. err, vim.log.levels.WARN) + local s = session.get() + spinner.set_label(string.format(" AI reviewing… %d/%d files ", s.ai_completed + 1, s.ai_total)) + vim.schedule(function() + local diff_mod = require("codereview.mr.diff") + diff_mod.render_sidebar(layout.sidebar_buf, diff_state) + end) + session.ai_file_done() + end, + on_complete = function() + local count = #(diff_state.ai_suggestions or {}) + if count == 0 then vim.schedule(function() - local diff_mod = require("codereview.mr.diff") - diff_mod.render_sidebar(layout.sidebar_buf, diff_state) + vim.notify("AI review: no issues found!", vim.log.levels.INFO) end) - session.ai_file_done() - end, - on_error = function(err, batch) - local path = batch[1].new_path or batch[1].old_path - vim.notify("AI review failed for " .. path .. ": " .. err, vim.log.levels.WARN) - local s = session.get() - spinner.set_label(string.format(" AI reviewing… %d/%d files ", s.ai_completed + 1, s.ai_total)) + else vim.schedule(function() - local diff_mod = require("codereview.mr.diff") - diff_mod.render_sidebar(layout.sidebar_buf, diff_state) + vim.notify(string.format("AI review: %d suggestions found", count), vim.log.levels.INFO) end) - session.ai_file_done() - end, - on_complete = function() - local count = #(diff_state.ai_suggestions or {}) - if count == 0 then - vim.schedule(function() - vim.notify("AI review: no issues found!", vim.log.levels.INFO) - end) - else - vim.schedule(function() - vim.notify(string.format("AI review: %d suggestions found", count), vim.log.levels.INFO) - end) - end - generate_summary_with_callbacks(diff_state, review, diffs) - end, - }) - - -- Update session with real file count (no individual job IDs with orchestrator) - session.ai_start({}, total) - spinner.set_label(string.format(" AI reviewing… 0/%d files ", total)) - end, { skip_agent = true }) -- no --agent for summary call - - if summary_job and summary_job > 0 then - -- Use summary job as initial tracking; will be replaced in Phase 2 - session.ai_start(summary_job) - spinner.set_label(" AI summarizing… ") - vim.notify("AI review started (summarizing files)…", vim.log.levels.INFO) - end + end + generate_summary_with_callbacks(diff_state, review, diffs) + end, + }) + + session.ai_start({}, total) + spinner.set_label(string.format(" AI reviewing… 0/%d files ", total)) + vim.notify("AI review started…", vim.log.levels.INFO) end ---- Single-file AI review: summarize all files, then review only the current file. +--- Single-file AI review: review only the current file. function M.start_file(review, diff_state, layout) local diffs = diff_state.files local file_idx = diff_state.current_file or 1 @@ -270,60 +250,42 @@ function M.start_file(review, diff_state, layout) local spinner = require("codereview.ui.spinner") session.start() - -- Phase 1: summary pre-pass - local summary_prompt = prompt_mod.build_summary_prompt(review, diffs) - local summary_job = ai_providers.get().run(summary_prompt, function(output, ai_err) - if ai_err then - session.ai_finish() - vim.notify("AI summary failed: " .. ai_err, vim.log.levels.ERROR) + local content = fetch_file_content(diff_state, review, target_path, target.deleted_file) + local file_prompt = prompt_mod.build_file_review_prompt(review, target, content) + local file_job = ai_providers.get().run(file_prompt, function(file_output, file_err) + session.ai_finish() + + if file_err then + vim.notify("AI review failed for " .. target_path .. ": " .. file_err, vim.log.levels.ERROR) return end - local summaries = prompt_mod.parse_summary_output(output) - - -- Phase 2: review the single target file - local content = fetch_file_content(diff_state, review, target_path, target.deleted_file) - local file_prompt = prompt_mod.build_file_review_prompt(review, target, summaries, content) - local file_job = ai_providers.get().run(file_prompt, function(file_output, file_err) - session.ai_finish() - - if file_err then - vim.notify("AI review failed for " .. target_path .. ": " .. file_err, vim.log.levels.ERROR) - return - end - - local suggestions = prompt_mod.parse_review_output(file_output) - suggestions = prompt_mod.filter_unchanged_lines(suggestions, { target }) - if #suggestions == 0 then - vim.notify("AI review: no issues found in " .. target_path, vim.log.levels.INFO) - return - end + local suggestions = prompt_mod.parse_review_output(file_output) + suggestions = prompt_mod.filter_unchanged_lines(suggestions, { target }) + if #suggestions == 0 then + vim.notify("AI review: no issues found in " .. target_path, vim.log.levels.INFO) + return + end - vim.notify(string.format("AI review: %d suggestions for %s", #suggestions, target_path), vim.log.levels.INFO) + vim.notify(string.format("AI review: %d suggestions for %s", #suggestions, target_path), vim.log.levels.INFO) - -- Replace only this file's suggestions (preserve others) - local kept = {} - for _, s in ipairs(diff_state.ai_suggestions or {}) do - if s.file ~= target_path then - table.insert(kept, s) - end + -- Replace only this file's suggestions (preserve others) + local kept = {} + for _, s in ipairs(diff_state.ai_suggestions or {}) do + if s.file ~= target_path then + table.insert(kept, s) end - diff_state.ai_suggestions = kept - - render_file_suggestions(diff_state, layout, suggestions) + end + diff_state.ai_suggestions = kept - generate_summary_with_callbacks(diff_state, review, diffs) - end) + render_file_suggestions(diff_state, layout, suggestions) - if file_job and file_job > 0 then - session.ai_start(file_job) - spinner.set_label(string.format(" AI reviewing %s… ", target_path)) - end - end, { skip_agent = true }) + generate_summary_with_callbacks(diff_state, review, diffs) + end) - if summary_job and summary_job > 0 then - session.ai_start(summary_job) - spinner.set_label(" AI summarizing… ") + if file_job and file_job > 0 then + session.ai_start(file_job) + spinner.set_label(string.format(" AI reviewing %s… ", target_path)) vim.notify(string.format("AI file review started for %s…", target_path), vim.log.levels.INFO) end end diff --git a/tests/codereview/ai/prompt_spec.lua b/tests/codereview/ai/prompt_spec.lua index d67c341..ce69b51 100644 --- a/tests/codereview/ai/prompt_spec.lua +++ b/tests/codereview/ai/prompt_spec.lua @@ -345,20 +345,13 @@ describe("ai.prompt", function() end) describe("build_file_review_prompt", function() - it("includes MR context, other file summaries, and target file diff", function() + it("includes MR context and target file diff", function() local review = { title = "Fix auth", description = "Token fix" } local file = { new_path = "src/auth.lua", diff = "@@ -1,2 +1,3 @@\n-old\n+new\n" } - local summaries = { - ["src/auth.lua"] = "Fixed token refresh", - ["src/config.lua"] = "Added timeout setting", - } - local result = prompt.build_file_review_prompt(review, file, summaries) + local result = prompt.build_file_review_prompt(review, file) -- Contains MR context assert.truthy(result:find("Fix auth")) assert.truthy(result:find("Token fix")) - -- Contains other file summaries (not the target file itself) - assert.truthy(result:find("src/config.lua")) - assert.truthy(result:find("Added timeout setting")) -- Contains the file's diff assert.truthy(result:find("%-old")) assert.truthy(result:find("%+new")) @@ -367,39 +360,17 @@ describe("ai.prompt", function() assert.truthy(result:find("severity")) end) - it("excludes target file from other files section", function() - local review = { title = "T", description = "D" } - local file = { new_path = "a.lua", diff = "diff" } - local summaries = { - ["a.lua"] = "Summary A", - ["b.lua"] = "Summary B", - } - local result = prompt.build_file_review_prompt(review, file, summaries) - -- Should contain b.lua summary but not a.lua in the "Other" section - assert.truthy(result:find("b.lua")) - assert.truthy(result:find("Summary B")) - -- The "Other Changed Files" section should not contain a.lua - local other_section = result:match("## Other Changed Files in This MR\n(.-)\n## File Under Review") - if other_section then - assert.falsy(other_section:find("a%.lua"), "Other files section should not contain the target file") - end - end) - - it("handles empty summaries", function() - local review = { title = "T", description = "D" } - local file = { new_path = "a.lua", diff = "diff" } - local result = prompt.build_file_review_prompt(review, file, {}) - assert.truthy(result:find("a.lua")) - -- Should still work, just no other files section - assert.truthy(result:find("JSON")) - -- Should NOT have "Other Changed Files" header - assert.falsy(result:find("Other Changed Files")) + it("review prompt has no Branch Context", function() + local review = { title = "t" } + local p = prompt.build_file_review_prompt(review, { new_path = "a", diff = "" }, "") + assert.is_nil(p:find("Branch Context")) + assert.is_nil(p:find("Other Changed Files")) end) it("annotates diff lines with line numbers in the file review prompt", function() local review = { title = "T", description = "D" } local file = { new_path = "src/foo.lua", diff = "@@ -3,2 +3,3 @@\n context\n-old\n+new\n+extra\n" } - local result = prompt.build_file_review_prompt(review, file, {}) + local result = prompt.build_file_review_prompt(review, file) -- Annotated context line at new line 3 assert.truthy(result:match("L%s*3:"), "prompt should contain L3 annotation") -- First added line at new line 4 @@ -411,7 +382,7 @@ describe("ai.prompt", function() config.setup({ ai = { review_level = "error" } }) local review = { title = "T", description = "D" } local file = { new_path = "f.lua", diff = "@@ -1,1 +1,1 @@\n-old\n+new\n" } - local result = prompt.build_file_review_prompt(review, file, {}) + local result = prompt.build_file_review_prompt(review, file) assert.truthy(result:find("Only report"), "should contain severity filter instruction") assert.truthy(result:find("error"), "should mention error level") config.reset() @@ -420,7 +391,7 @@ describe("ai.prompt", function() it("includes full file content section when content is provided", function() local review = { title = "T", description = "D" } local file = { new_path = "src/foo.lua", diff = "@@ -1,2 +1,3 @@\n-old\n+new\n" } - local result = prompt.build_file_review_prompt(review, file, {}, "local M = {}\nfunction M.foo()\nend\nreturn M") + local result = prompt.build_file_review_prompt(review, file, "local M = {}\nfunction M.foo()\nend\nreturn M") assert.truthy(result:find("## Full File Content")) assert.truthy(result:find("local M = {}")) assert.truthy(result:find("function M.foo")) @@ -430,14 +401,14 @@ describe("ai.prompt", function() it("omits full file content section when content is nil", function() local review = { title = "T", description = "D" } local file = { new_path = "src/foo.lua", diff = "@@ -1,2 +1,3 @@\n-old\n+new\n" } - local result = prompt.build_file_review_prompt(review, file, {}, nil) + local result = prompt.build_file_review_prompt(review, file, nil) assert.falsy(result:find("## Full File Content")) end) it("places full file content before the diff section", function() local review = { title = "T", description = "D" } local file = { new_path = "src/foo.lua", diff = "@@ -1,1 +1,1 @@\n-old\n+new\n" } - local result = prompt.build_file_review_prompt(review, file, {}, "full content here") + local result = prompt.build_file_review_prompt(review, file, "full content here") local content_pos = result:find("## Full File Content") local diff_pos = result:find("## File Under Review") assert.truthy(content_pos) @@ -470,40 +441,4 @@ describe("ai.prompt", function() assert.equals("Fixes the bug.", desc) end) end) - - describe("build_summary_prompt", function() - it("includes MR context and all file diffs", function() - local review = { title = "Fix auth", description = "Token fix" } - local diffs = { - { new_path = "src/auth.lua", diff = "@@ -1,2 +1,3 @@\n-old\n+new\n" }, - { new_path = "src/config.lua", diff = "@@ -5,1 +5,2 @@\n+added\n" }, - } - local result = prompt.build_summary_prompt(review, diffs) - assert.truthy(result:find("Fix auth")) - assert.truthy(result:find("src/auth.lua")) - assert.truthy(result:find("src/config.lua")) - assert.truthy(result:find("JSON")) - assert.truthy(result:find("one%-sentence summary")) - end) - end) - - describe("parse_summary_output", function() - it("extracts file-to-summary map from JSON block", function() - local output = - '```json\n{"src/auth.lua": "Fixed token refresh logic", "src/config.lua": "Added timeout setting"}\n```' - local summaries = prompt.parse_summary_output(output) - assert.equals("Fixed token refresh logic", summaries["src/auth.lua"]) - assert.equals("Added timeout setting", summaries["src/config.lua"]) - end) - - it("returns empty table on missing JSON", function() - local summaries = prompt.parse_summary_output("no json here") - assert.same({}, summaries) - end) - - it("returns empty table on nil input", function() - local summaries = prompt.parse_summary_output(nil) - assert.same({}, summaries) - end) - end) end) diff --git a/tests/codereview/plan/prompt_spec.lua b/tests/codereview/plan/prompt_spec.lua index 0008546..138e79a 100644 --- a/tests/codereview/plan/prompt_spec.lua +++ b/tests/codereview/plan/prompt_spec.lua @@ -6,18 +6,15 @@ describe("plan.prompt.build_file_plan_prompt", function() new_path = "lua/test.lua", diff = "+local x = 1", } - local result = prompt.build_file_plan_prompt(file, {}) + local result = prompt.build_file_plan_prompt(file) assert.is_string(result) assert.matches("lua/test.lua", result) assert.matches("local x = 1", result) end) - it("includes other file summaries", function() - local file = { new_path = "a.lua", diff = "+x" } - local summaries = { ["b.lua"] = "Added helper function" } - local result = prompt.build_file_plan_prompt(file, summaries) - assert.matches("b.lua", result) - assert.matches("Added helper function", result) + it("plan prompt has no Branch Context", function() + local p = prompt.build_file_plan_prompt({ new_path = "a", diff = "" }) + assert.is_nil(p:find("Branch Context")) end) end) diff --git a/tests/codereview/review/init_spec.lua b/tests/codereview/review/init_spec.lua index 947d41a..0a4982e 100644 --- a/tests/codereview/review/init_spec.lua +++ b/tests/codereview/review/init_spec.lua @@ -91,7 +91,7 @@ describe("review.init routing", function() captured_calls = {} end) - it("uses summary prompt then per-file prompts for multi-file MRs", function() + it("uses per-file prompts for multi-file MRs", function() local review = { title = "Multi", description = "desc" } local diff_state = { files = { @@ -108,15 +108,10 @@ describe("review.init routing", function() row_ai_cache = {}, } review_mod.start(review, diff_state, { main_buf = 0, sidebar_buf = 0, main_win = 0 }) - -- Phase 1: summary call with skip_agent - assert.truthy(#captured_calls >= 1, "should have at least one subprocess call") - local first = captured_calls[1] - assert.truthy(first.opts and first.opts.skip_agent, "summary call should skip agent") - assert.truthy(first.prompt:find("summariz"), "first prompt should be summary prompt") - -- Phase 2: per-file calls (one per file) - assert.truthy(#captured_calls >= 3, "should have summary + 2 file calls") + -- Per-file calls (one per file, no summary pre-pass) + assert.truthy(#captured_calls >= 2, "should have 2 per-file calls") assert.truthy( - captured_calls[2].prompt:find("a.lua") or captured_calls[3].prompt:find("a.lua"), + captured_calls[1].prompt:find("a.lua") or captured_calls[2].prompt:find("a.lua"), "per-file prompts should reference file paths" ) end) @@ -235,7 +230,7 @@ describe("review.start_file", function() vim.schedule = orig_schedule_sf end) - it("runs summary then single-file review with cross-file context", function() + it("reviews single target file without a summary pre-pass", function() local review = { title = "MR", description = "desc" } local diff_state = { files = { @@ -256,14 +251,9 @@ describe("review.start_file", function() review_mod.start_file(review, diff_state, layout) - -- Phase 1: summary call - assert.truthy(#captured_calls >= 1) - assert.truthy(captured_calls[1].opts and captured_calls[1].opts.skip_agent) - assert.truthy(captured_calls[1].prompt:find("summariz")) - - -- Phase 2: single file review (NOT 3 per-file calls) - assert.equals(2, #captured_calls, "should be exactly 2 calls: summary + 1 file") - assert.truthy(captured_calls[2].prompt:find("b.lua"), "should review the target file b.lua") + -- Single file review only (no summary pre-pass) + assert.equals(1, #captured_calls, "should be exactly 1 call: the file review") + assert.truthy(captured_calls[1].prompt:find("b.lua"), "should review the target file b.lua") end) it("replaces only the target file's suggestions", function() @@ -286,14 +276,8 @@ describe("review.start_file", function() } local orig_run = package.loaded["codereview.ai.subprocess"].run - local call_count = 0 package.loaded["codereview.ai.subprocess"].run = function(prompt, callback, opts) - call_count = call_count + 1 - if call_count == 1 then - callback('```json\n[{"file":"a.lua","summary":"does a"},{"file":"b.lua","summary":"does b"}]\n```') - else - callback('```json\n[{"file":"a.lua","line":10,"severity":"warning","comment":"new a"}]\n```') - end + callback('```json\n[{"file":"a.lua","line":10,"severity":"warning","comment":"new a"}]\n```') return 1 end @@ -346,9 +330,9 @@ describe("file content in per-file review", function() -- Should have fetched content for both files assert.equals(2, #content_fetch_calls) - -- Per-file prompts (calls 2 and 3) should contain full file content + -- Per-file prompts (calls 1 and 2) should contain full file content assert.truthy( - captured_calls[2].prompt:find("Full File Content") or captured_calls[3].prompt:find("Full File Content"), + captured_calls[1].prompt:find("Full File Content") or captured_calls[2].prompt:find("Full File Content"), "per-file prompts should include full file content" ) end) @@ -421,7 +405,7 @@ describe("file content in per-file review", function() assert.equals(2, #content_fetch_calls) -- But the per-file prompts should NOT have Full File Content for big files -- (both files return 501 lines which exceeds default 500) - for i = 2, #captured_calls do + for i = 1, #captured_calls do assert.falsy( captured_calls[i].prompt:find("Full File Content"), "per-file prompt should not include full file content for files exceeding max_file_size" From eacc4957377c38b411814591c8827c532f517d48 Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Wed, 29 Apr 2026 07:35:39 +0200 Subject: [PATCH 11/21] feat(ai): cache-friendly prompt order + Anthropic cache_control --- lua/codereview/ai/orchestrator.lua | 13 +++- lua/codereview/ai/prompt.lua | 62 +++++++------------ lua/codereview/ai/providers/anthropic.lua | 10 ++- lua/codereview/plan/init.lua | 1 + lua/codereview/plan/prompt.lua | 27 ++++---- lua/codereview/review/init.lua | 1 + tests/codereview/ai/prompt_spec.lua | 2 +- .../ai/providers/anthropic_spec.lua | 45 ++++++++++++++ tests/codereview/plan/prompt_spec.lua | 7 +++ 9 files changed, 110 insertions(+), 58 deletions(-) diff --git a/lua/codereview/ai/orchestrator.lua b/lua/codereview/ai/orchestrator.lua index 9390b24..5cb0060 100644 --- a/lua/codereview/ai/orchestrator.lua +++ b/lua/codereview/ai/orchestrator.lua @@ -48,8 +48,17 @@ function M.run(spec) next_idx = next_idx + 1 active = active + 1 - local prompt = spec.build_prompt(batch, {}) - ai_providers.get().run(prompt, function(output, err) + local prompt_str = spec.build_prompt(batch, {}) + if spec.cacheable then + local provider_name = (require("codereview.config").get().ai or {}).provider + if provider_name == "anthropic" then + local split = prompt_str:find("\n## Files?\n", 1, false) or prompt_str:find("\n## File:", 1, true) + if split then + prompt_str = { system = prompt_str:sub(1, split - 1), user = prompt_str:sub(split + 1) } + end + end + end + ai_providers.get().run(prompt_str, function(output, err) active = active - 1 completed = completed + 1 diff --git a/lua/codereview/ai/prompt.lua b/lua/codereview/ai/prompt.lua index cc08f5d..bf36450 100644 --- a/lua/codereview/ai/prompt.lua +++ b/lua/codereview/ai/prompt.lua @@ -353,58 +353,44 @@ function M.build_file_review_prompt(review, file, content) "## MR Description", review.description or "(no description)", "", + "## Instructions", + "", + "Each diff line is prefixed with its line number (e.g., L38:). Use the EXACT number from the L-prefix for the line field.", + "Review the file below. Output a JSON array in a ```json code block.", + 'Each item: {"file": "", "line": , "code": "", "severity": "error"|"warning"|"info"|"suggestion", "comment": "text"}', + 'The "code" field must contain the trimmed source code from the line you are commenting on (without the diff +/- prefix).', + 'Use \\n inside "comment" strings for line breaks.', + "If no issues, output `[]`.", + "ONLY comment on lines that are actual changes: lines prefixed with + (added) or - (removed) in the diff. Context lines (no +/- prefix) are for understanding only — NEVER comment on them.", + "Focus on: bugs, security, error handling, edge cases, naming, clarity.", + "Do NOT comment on style or formatting.", } + local sev_instr = severity_instruction() + if sev_instr then + table.insert(parts, sev_instr) + end + + table.insert( + parts, + "IMPORTANT: Find the L-prefix on the exact code line your comment applies to and use that number. Do NOT guess or count lines yourself." + ) + table.insert(parts, "") + if content and content ~= "" then table.insert(parts, "## Full File Content: " .. path) local ext = path:match("%.([^%.]+)$") or "" table.insert(parts, "```" .. ext) table.insert(parts, content) table.insert(parts, "```") + table.insert(parts, "The full file content is provided above for understanding only.") table.insert(parts, "") end - table.insert(parts, "## File Under Review: " .. path) + table.insert(parts, "## File: " .. path) table.insert(parts, "```diff") table.insert(parts, M.annotate_diff_with_lines(file.diff or "")) table.insert(parts, "```") - table.insert(parts, "") - table.insert(parts, "## Instructions") - table.insert(parts, "") - table.insert( - parts, - "Each diff line is prefixed with its line number (e.g., L38:). Use the EXACT number from the L-prefix for the line field." - ) - table.insert(parts, "Review this file. Output a JSON array in a ```json code block.") - table.insert( - parts, - 'Each item: {"file": "' - .. path - .. '", "line": , "code": "", "severity": "error"|"warning"|"info"|"suggestion", "comment": "text"}' - ) - table.insert( - parts, - 'The "code" field must contain the trimmed source code from the line you are commenting on (without the diff +/- prefix).' - ) - table.insert(parts, 'Use \\n inside "comment" strings for line breaks.') - table.insert(parts, "If no issues, output `[]`.") - if content and content ~= "" then - table.insert(parts, "The full file content is provided above for understanding only.") - end - table.insert( - parts, - "ONLY comment on lines that are actual changes: lines prefixed with + (added) or - (removed) in the diff. Context lines (no +/- prefix) are for understanding only — NEVER comment on them." - ) - table.insert(parts, "Focus on: bugs, security, error handling, edge cases, naming, clarity.") - table.insert(parts, "Do NOT comment on style or formatting.") - local sev_instr = severity_instruction() - if sev_instr then - table.insert(parts, sev_instr) - end - table.insert( - parts, - "IMPORTANT: Find the L-prefix on the exact code line your comment applies to and use that number. Do NOT guess or count lines yourself." - ) return table.concat(parts, "\n") end diff --git a/lua/codereview/ai/providers/anthropic.lua b/lua/codereview/ai/providers/anthropic.lua index 2156c7e..00dc56e 100644 --- a/lua/codereview/ai/providers/anthropic.lua +++ b/lua/codereview/ai/providers/anthropic.lua @@ -24,11 +24,15 @@ function M.run(prompt, callback, opts) local body = { model = pcfg.model or "claude-sonnet-4-20250514", max_tokens = 8192, - messages = { - { role = "user", content = prompt }, - }, } + if type(prompt) == "table" then + body.system = { { type = "text", text = prompt.system, cache_control = { type = "ephemeral" } } } + body.messages = { { role = "user", content = prompt.user } } + else + body.messages = { { role = "user", content = prompt } } + end + log.debug("AI anthropic: sending request, model=" .. body.model) return http.post_json(url, headers, body, function(response, err) if err then diff --git a/lua/codereview/plan/init.lua b/lua/codereview/plan/init.lua index be74b97..80cec1d 100644 --- a/lua/codereview/plan/init.lua +++ b/lua/codereview/plan/init.lua @@ -65,6 +65,7 @@ function M._run_phase2(branch, base, diffs) orchestrator.run({ diffs = diffs, + cacheable = true, build_prompt = function(batch) return plan_prompt.build_file_plan_prompt(batch[1]) end, diff --git a/lua/codereview/plan/prompt.lua b/lua/codereview/plan/prompt.lua index 5480196..8ca4184 100644 --- a/lua/codereview/plan/prompt.lua +++ b/lua/codereview/plan/prompt.lua @@ -6,22 +6,21 @@ function M.build_file_plan_prompt(file) local parts = { "You are creating an implementation plan for changes in a file.", "", + "## Instructions", + "", + "Analyze the diff below and create tasks to complete or improve this implementation.", + "Output a JSON array in a ```json code block:", + '[{"file": "", "line": , "task": "", "reason": ""}]', + "", + "Focus on: incomplete implementations, missing error handling, TODOs, edge cases, missing tests.", + "If the code looks complete, output `[]`.", + "", + "## File: " .. path, + "```diff", + ai_prompt.annotate_diff_with_lines(file.diff or ""), + "```", } - table.insert(parts, "## File: " .. path) - table.insert(parts, "```diff") - table.insert(parts, ai_prompt.annotate_diff_with_lines(file.diff or "")) - table.insert(parts, "```") - table.insert(parts, "") - table.insert(parts, "## Instructions") - table.insert(parts, "") - table.insert(parts, "Analyze this diff and create tasks to complete or improve this implementation.") - table.insert(parts, "Output a JSON array in a ```json code block:") - table.insert(parts, '[{"file": "' .. path .. '", "line": , "task": "", "reason": ""}]') - table.insert(parts, "") - table.insert(parts, "Focus on: incomplete implementations, missing error handling, TODOs, edge cases, missing tests.") - table.insert(parts, "If the code looks complete, output `[]`.") - return table.concat(parts, "\n") end diff --git a/lua/codereview/review/init.lua b/lua/codereview/review/init.lua index a973dac..feff4a7 100644 --- a/lua/codereview/review/init.lua +++ b/lua/codereview/review/init.lua @@ -182,6 +182,7 @@ local function start_multi(review, diff_state, layout) orchestrator.run({ diffs = diffs, + cacheable = true, build_prompt = function(batch) local file = batch[1] local path = file.new_path or file.old_path diff --git a/tests/codereview/ai/prompt_spec.lua b/tests/codereview/ai/prompt_spec.lua index ce69b51..9210d33 100644 --- a/tests/codereview/ai/prompt_spec.lua +++ b/tests/codereview/ai/prompt_spec.lua @@ -410,7 +410,7 @@ describe("ai.prompt", function() local file = { new_path = "src/foo.lua", diff = "@@ -1,1 +1,1 @@\n-old\n+new\n" } local result = prompt.build_file_review_prompt(review, file, "full content here") local content_pos = result:find("## Full File Content") - local diff_pos = result:find("## File Under Review") + local diff_pos = result:find("## File: ") assert.truthy(content_pos) assert.truthy(diff_pos) assert.truthy(content_pos < diff_pos, "Full file content should appear before diff section") diff --git a/tests/codereview/ai/providers/anthropic_spec.lua b/tests/codereview/ai/providers/anthropic_spec.lua index fd5c433..7df6717 100644 --- a/tests/codereview/ai/providers/anthropic_spec.lua +++ b/tests/codereview/ai/providers/anthropic_spec.lua @@ -86,4 +86,49 @@ describe("ai.providers.anthropic", function() assert.is_nil(result) assert.truthy(err:find("api_key")) end) + + it("sets cache_control on system when prompt is structured table", function() + package.loaded["codereview.config"].get = function() + return { + ai = { + enabled = true, + provider = "anthropic", + anthropic = { api_key = "sk-test", model = "claude-sonnet-4-20250514" }, + }, + } + end + local result, err + anthropic.run({ system = "You are an expert.", user = "Review this diff." }, function(o, e) + result = o + err = e + end) + assert.is_nil(err) + assert.equals("AI response", result) + assert.equals(1, #http_calls) + assert.not_nil(http_calls[1].body.system) + assert.equals("ephemeral", http_calls[1].body.system[1].cache_control.type) + assert.equals("You are an expert.", http_calls[1].body.system[1].text) + end) + + it("string prompt: no system field, backward compat", function() + package.loaded["codereview.config"].get = function() + return { + ai = { + enabled = true, + provider = "anthropic", + anthropic = { api_key = "sk-test", model = "claude-sonnet-4-20250514" }, + }, + } + end + local result, err + anthropic.run("plain string prompt", function(o, e) + result = o + err = e + end) + assert.is_nil(err) + assert.equals("AI response", result) + assert.equals(1, #http_calls) + assert.is_nil(http_calls[1].body.system) + assert.equals("plain string prompt", http_calls[1].body.messages[1].content) + end) end) diff --git a/tests/codereview/plan/prompt_spec.lua b/tests/codereview/plan/prompt_spec.lua index 138e79a..3943286 100644 --- a/tests/codereview/plan/prompt_spec.lua +++ b/tests/codereview/plan/prompt_spec.lua @@ -16,6 +16,13 @@ describe("plan.prompt.build_file_plan_prompt", function() local p = prompt.build_file_plan_prompt({ new_path = "a", diff = "" }) assert.is_nil(p:find("Branch Context")) end) + + it("plan prompt: instructions before diff", function() + local p = prompt.build_file_plan_prompt({ new_path = "a", diff = "@@\n+UNIQUE_TOKEN_XYZ" }) + local i_instr = p:find("Instructions") or 0 + local i_diff = p:find("UNIQUE_TOKEN_XYZ") or 0 + assert.is_true(i_instr > 0 and i_diff > i_instr) + end) end) describe("plan.prompt.parse_file_plan_output", function() From b2a41547620c6096028113b7d0b7fa8febcd45d2 Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Wed, 29 Apr 2026 07:49:04 +0200 Subject: [PATCH 12/21] feat(ai): per-file progress reporting for CLI providers Adds a tmp-file-backed progress tracker that CLI agents append to as they finish each file. The orchestrator polls every 250ms and emits on_progress(done, total) so plan and review spinners reflect real-time per-file progress. HTTP providers (anthropic, openai, ollama) are unaffected and continue using the existing per-batch on_batch_complete counter. --- lua/codereview/ai/orchestrator.lua | 32 ++++++++++++-- lua/codereview/ai/progress.lua | 58 ++++++++++++++++++++++++ lua/codereview/ai/prompt.lua | 18 +++++++- lua/codereview/plan/init.lua | 7 ++- lua/codereview/plan/prompt.lua | 4 +- lua/codereview/review/init.lua | 7 ++- tests/codereview/ai/progress_spec.lua | 63 +++++++++++++++++++++++++++ 7 files changed, 178 insertions(+), 11 deletions(-) create mode 100644 lua/codereview/ai/progress.lua create mode 100644 tests/codereview/ai/progress_spec.lua diff --git a/lua/codereview/ai/orchestrator.lua b/lua/codereview/ai/orchestrator.lua index 5cb0060..6f26339 100644 --- a/lua/codereview/ai/orchestrator.lua +++ b/lua/codereview/ai/orchestrator.lua @@ -1,6 +1,16 @@ local ai_providers = require("codereview.ai.providers") local M = {} +--- CLI providers support progress-file reporting; HTTP providers do not. +local CLI_PROVIDERS = { + claude_cli = true, + codex_cli = true, + copilot_cli = true, + gemini_cli = true, + opencode_cli = true, + qwen_cli = true, +} + --- Run a parallel-batch AI orchestration loop. --- --- @param spec table @@ -11,6 +21,7 @@ local M = {} --- on_batch_complete fn(batch, parsed)? fires after each successful batch --- on_error: fn(err, batch)? fires on per-batch failure --- on_complete: fn(all_results) +--- on_progress: fn(done, total)? fires every 250ms for CLI providers --- provider_opts: table? forwarded to provider.run --- max_concurrent: integer? (default 10) function M.run(spec) @@ -23,6 +34,7 @@ function M.run(spec) vim.notify(string.format("Skipped %d file(s) (lockfiles/generated/binary)", skipped), vim.log.levels.INFO) end + local provider_name = (cfg.ai or {}).provider local diffs = spec.diffs local total = #diffs if total == 0 then @@ -32,6 +44,18 @@ function M.run(spec) return end + -- Create a progress tracker for CLI providers when the caller wants progress callbacks. + -- HTTP providers (anthropic, openai, ollama) rely on the per-batch on_batch_complete counter instead. + local prog = nil + if CLI_PROVIDERS[provider_name] and spec.on_progress then + prog = require("codereview.ai.progress").new() + prog:watch(function(n) + spec.on_progress(n, total) + end) + end + + local prompt_opts = { progress_path = prog and prog.path or nil } + -- Task 0: one file per batch. Task 5 swaps this for batch.build(diffs, ...). local batches = {} for _, f in ipairs(diffs) do @@ -48,9 +72,8 @@ function M.run(spec) next_idx = next_idx + 1 active = active + 1 - local prompt_str = spec.build_prompt(batch, {}) - if spec.cacheable then - local provider_name = (require("codereview.config").get().ai or {}).provider + local prompt_str = spec.build_prompt(batch, prompt_opts) + if spec.cacheable and type(prompt_str) == "string" then if provider_name == "anthropic" then local split = prompt_str:find("\n## Files?\n", 1, false) or prompt_str:find("\n## File:", 1, true) if split then @@ -83,6 +106,9 @@ function M.run(spec) if spec.on_complete then spec.on_complete(results) end + if prog then + prog:cleanup() + end else process_next() end diff --git a/lua/codereview/ai/progress.lua b/lua/codereview/ai/progress.lua new file mode 100644 index 0000000..c0edd62 --- /dev/null +++ b/lua/codereview/ai/progress.lua @@ -0,0 +1,58 @@ +local M = {} +local Progress = {} +Progress.__index = Progress + +--- Create a new progress tracker backed by a tmp file. +--- The tmp file path is exposed as `p.path` so prompt builders can embed it. +--- +--- @return table Progress object with :count(), :watch(on_change), :cleanup() +function M.new() + local dir = vim.fn.stdpath("state") .. "/codereview" + vim.fn.mkdir(dir, "p") + local path = string.format("%s/run-%d-%d.progress", dir, os.time(), math.random(1000000)) + local f = io.open(path, "w") + if f then + f:close() + end + return setmetatable({ path = path }, Progress) +end + +--- Count completed-file lines written to the progress file. +--- @return integer +function Progress:count() + local f = io.open(self.path, "r") + if not f then + return 0 + end + local n = 0 + for _ in f:lines() do + n = n + 1 + end + f:close() + return n +end + +--- Poll the progress file every 250 ms and call on_change(count) when it changes. +--- @param on_change fun(count: integer) +function Progress:watch(on_change) + self._timer = vim.uv.new_timer() + self._timer:start( + 250, + 250, + vim.schedule_wrap(function() + on_change(self:count()) + end) + ) +end + +--- Stop the poll timer and delete the tmp file. +function Progress:cleanup() + if self._timer then + self._timer:stop() + self._timer:close() + self._timer = nil + end + os.remove(self.path) +end + +return M diff --git a/lua/codereview/ai/prompt.lua b/lua/codereview/ai/prompt.lua index bf36450..bdbef7c 100644 --- a/lua/codereview/ai/prompt.lua +++ b/lua/codereview/ai/prompt.lua @@ -342,7 +342,21 @@ function M.parse_mr_draft(output) return vim.trim(title), vim.trim(description) end -function M.build_file_review_prompt(review, file, content) +--- Return a prompt suffix that instructs a CLI agent to append a line to the +--- progress file after each file it analyzes. Returns "" when path is absent. +--- @param path string|nil Absolute path of the progress tmp file +--- @return string +function M.progress_suffix(path) + if not path or path == "" then + return "" + end + return string.format( + "\n\n## Progress reporting\nAfter analyzing each file, append exactly one line with its path to the progress file:\n```\nprintf '%%s\\n' '' >> %q\n```", + path + ) +end + +function M.build_file_review_prompt(review, file, content, opts) local path = file.new_path or file.old_path local parts = { "You are reviewing a single file in a merge request.", @@ -392,7 +406,7 @@ function M.build_file_review_prompt(review, file, content) table.insert(parts, M.annotate_diff_with_lines(file.diff or "")) table.insert(parts, "```") - return table.concat(parts, "\n") + return table.concat(parts, "\n") .. M.progress_suffix(opts and opts.progress_path) end return M diff --git a/lua/codereview/plan/init.lua b/lua/codereview/plan/init.lua index 80cec1d..d147af8 100644 --- a/lua/codereview/plan/init.lua +++ b/lua/codereview/plan/init.lua @@ -66,11 +66,14 @@ function M._run_phase2(branch, base, diffs) orchestrator.run({ diffs = diffs, cacheable = true, - build_prompt = function(batch) - return plan_prompt.build_file_plan_prompt(batch[1]) + build_prompt = function(batch, opts) + return plan_prompt.build_file_plan_prompt(batch[1], opts) end, parse_output = plan_prompt.parse_file_plan_output, on_result = function() end, + on_progress = function(done, t) + spinner.set_label(string.format(" Planning %d/%d files… ", done, t)) + end, on_batch_complete = function() completed_count = completed_count + 1 spinner.set_label(string.format(" Planning %d/%d files… ", completed_count, total)) diff --git a/lua/codereview/plan/prompt.lua b/lua/codereview/plan/prompt.lua index 8ca4184..be9cbbc 100644 --- a/lua/codereview/plan/prompt.lua +++ b/lua/codereview/plan/prompt.lua @@ -1,7 +1,7 @@ local M = {} local ai_prompt = require("codereview.ai.prompt") -function M.build_file_plan_prompt(file) +function M.build_file_plan_prompt(file, opts) local path = file.new_path or file.old_path local parts = { "You are creating an implementation plan for changes in a file.", @@ -21,7 +21,7 @@ function M.build_file_plan_prompt(file) "```", } - return table.concat(parts, "\n") + return table.concat(parts, "\n") .. ai_prompt.progress_suffix(opts and opts.progress_path) end function M.parse_file_plan_output(output) diff --git a/lua/codereview/review/init.lua b/lua/codereview/review/init.lua index feff4a7..cdb0c04 100644 --- a/lua/codereview/review/init.lua +++ b/lua/codereview/review/init.lua @@ -183,14 +183,17 @@ local function start_multi(review, diff_state, layout) orchestrator.run({ diffs = diffs, cacheable = true, - build_prompt = function(batch) + build_prompt = function(batch, opts) local file = batch[1] local path = file.new_path or file.old_path local content = fetch_file_content(diff_state, review, path, file.deleted_file) - return prompt_mod.build_file_review_prompt(review, file, content) + return prompt_mod.build_file_review_prompt(review, file, content, opts) end, parse_output = prompt_mod.parse_review_output, on_result = function() end, + on_progress = function(done, t) + spinner.set_label(string.format(" AI reviewing… %d/%d files ", done, t)) + end, on_batch_complete = function(batch, parsed) local file = batch[1] local suggestions = prompt_mod.filter_unchanged_lines(parsed, { file }) diff --git a/tests/codereview/ai/progress_spec.lua b/tests/codereview/ai/progress_spec.lua new file mode 100644 index 0000000..e4ca116 --- /dev/null +++ b/tests/codereview/ai/progress_spec.lua @@ -0,0 +1,63 @@ +local progress + +describe("progress", function() + before_each(function() + -- Stub vim APIs needed by progress.lua in the busted environment + vim.fn.stdpath = function(_) + return os.getenv("TMPDIR") or "/tmp" + end + vim.uv = vim.uv or {} + vim.uv.fs_stat = function(path) + local f = io.open(path, "r") + if f then + f:close() + return {} + end + return nil + end + vim.uv.new_timer = function() + return { + start = function() end, + stop = function() end, + close = function() end, + } + end + vim.schedule_wrap = vim.schedule_wrap or function(fn) + return fn + end + + package.loaded["codereview.ai.progress"] = nil + progress = require("codereview.ai.progress") + end) + + it("creates tmp file and counts appended lines", function() + local p = progress.new() + assert.is_string(p.path) + assert.are.equal(0, p:count()) + local f = io.open(p.path, "a") + f:write("a\n") + f:close() + f = io.open(p.path, "a") + f:write("b\n") + f:close() + assert.are.equal(2, p:count()) + p:cleanup() + assert.is_nil(vim.uv.fs_stat(p.path)) + end) + + it("count returns 0 when progress file has no lines", function() + local p = progress.new() + assert.are.equal(0, p:count()) + p:cleanup() + end) + + it("cleanup removes the tmp file", function() + local p = progress.new() + local path = p.path + -- File exists before cleanup + assert.is_not_nil(vim.uv.fs_stat(path)) + p:cleanup() + -- File gone after cleanup + assert.is_nil(vim.uv.fs_stat(path)) + end) +end) From f027d0dd3c31f8375beb6093084031761fe8abb1 Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Wed, 29 Apr 2026 08:07:25 +0200 Subject: [PATCH 13/21] feat(ai): batch files into single AI calls by token budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pack diffs into batches by char budget (default 80 000 chars / 15 files) before sending to the AI provider. Amortises instruction overhead across files, cutting AI calls on large MRs to a fraction of the old per-file count. Review uses a tighter per-spec budget (40 000 / 8) because full file content is included alongside diffs. - Add lua/codereview/ai/batch.lua: M.build(diffs, opts) greedy packer - Add ai.batch_char_budget (80 000) and ai.batch_max_files (15) defaults - Orchestrator: replace 1-file-per-batch loop with batch.build(); per-spec overrides (spec.batch_char_budget / spec.batch_max_files) take priority - plan/prompt: add build_batch_plan_prompt(files, opts) - ai/prompt: add build_batch_review_prompt(review, files, contents, opts) - plan/init: swap build_prompt to batch variant; spinner counts files not batches - review/init: swap to batch variant; pre-fetch content per file in batch; ai_file_done() called N times (once per file in batch) - Tests: batch_spec (TDD), orchestrator_spec updated, integration_spec asserts 5 small files → 1 batch --- lua/codereview/ai/batch.lua | 34 +++++++++++ lua/codereview/ai/orchestrator.lua | 9 ++- lua/codereview/ai/prompt.lua | 68 ++++++++++++++++++++++ lua/codereview/config.lua | 4 ++ lua/codereview/plan/init.lua | 8 +-- lua/codereview/plan/prompt.lua | 35 +++++++++++ lua/codereview/review/init.lua | 33 +++++++---- tests/codereview/ai/batch_spec.lua | 59 +++++++++++++++++++ tests/codereview/ai/orchestrator_spec.lua | 51 +++++++++++++++- tests/codereview/plan/integration_spec.lua | 13 +++++ 10 files changed, 294 insertions(+), 20 deletions(-) create mode 100644 lua/codereview/ai/batch.lua create mode 100644 tests/codereview/ai/batch_spec.lua diff --git a/lua/codereview/ai/batch.lua b/lua/codereview/ai/batch.lua new file mode 100644 index 0000000..46d66f1 --- /dev/null +++ b/lua/codereview/ai/batch.lua @@ -0,0 +1,34 @@ +local M = {} + +--- Group diffs into batches by character budget and file count cap. +--- An oversize file (diff alone exceeds budget) goes in its own batch. +--- +--- @param diffs table[] List of {new_path, old_path, diff} +--- @param opts table? {char_budget: integer, max_files: integer} +--- @return table[][] List of batches; each batch is a list of diffs +function M.build(diffs, opts) + local budget = (opts and opts.char_budget) or 80000 + local cap = (opts and opts.max_files) or 15 + local batches = {} + local cur = {} + local cur_size = 0 + + for _, f in ipairs(diffs) do + local sz = #(f.diff or "") + if (#cur > 0) and (cur_size + sz > budget or #cur >= cap) then + table.insert(batches, cur) + cur = {} + cur_size = 0 + end + table.insert(cur, f) + cur_size = cur_size + sz + end + + if #cur > 0 then + table.insert(batches, cur) + end + + return batches +end + +return M diff --git a/lua/codereview/ai/orchestrator.lua b/lua/codereview/ai/orchestrator.lua index 6f26339..1fc29c6 100644 --- a/lua/codereview/ai/orchestrator.lua +++ b/lua/codereview/ai/orchestrator.lua @@ -56,11 +56,10 @@ function M.run(spec) local prompt_opts = { progress_path = prog and prog.path or nil } - -- Task 0: one file per batch. Task 5 swaps this for batch.build(diffs, ...). - local batches = {} - for _, f in ipairs(diffs) do - table.insert(batches, { f }) - end + local batches = require("codereview.ai.batch").build(diffs, { + char_budget = spec.batch_char_budget or (cfg.ai and cfg.ai.batch_char_budget), + max_files = spec.batch_max_files or (cfg.ai and cfg.ai.batch_max_files), + }) local results = {} local completed, next_idx, active = 0, 1, 0 diff --git a/lua/codereview/ai/prompt.lua b/lua/codereview/ai/prompt.lua index bdbef7c..50bbe2b 100644 --- a/lua/codereview/ai/prompt.lua +++ b/lua/codereview/ai/prompt.lua @@ -305,6 +305,74 @@ function M.parse_review_output(output) return suggestions end +--- Build a review prompt for a batch of files (multiple diffs in one AI call). +--- Stable instructions first (cache-friendly); per-file content/diffs after "## Files". +--- +--- @param review table MR review object {title, description} +--- @param files table[] List of {new_path, old_path, diff} +--- @param contents_by_path table Full file content keyed by path (may be nil) +--- @param opts table? {progress_path?: string} +--- @return string +function M.build_batch_review_prompt(review, files, contents_by_path, opts) + local parts = { + "You are reviewing files in a merge request.", + "", + "## MR Title", + review.title or "", + "", + "## MR Description", + review.description or "(no description)", + "", + "## Instructions", + "", + "Each diff line is prefixed with its line number (e.g., L38:). Use the EXACT number from the L-prefix for the line field.", + "Review the files below. Output a JSON array in a ```json code block.", + 'Each item: {"file": "", "line": , "code": "", "severity": "error"|"warning"|"info"|"suggestion", "comment": "text"}', + 'The "code" field must contain the trimmed source code from the line you are commenting on (without the diff +/- prefix).', + 'Use \\n inside "comment" strings for line breaks.', + "If no issues, output `[]`.", + "ONLY comment on lines that are actual changes: lines prefixed with + (added) or - (removed) in the diff. Context lines (no +/- prefix) are for understanding only — NEVER comment on them.", + "Focus on: bugs, security, error handling, edge cases, naming, clarity.", + "Do NOT comment on style or formatting.", + } + + local sev_instr = severity_instruction() + if sev_instr then + table.insert(parts, sev_instr) + end + + table.insert( + parts, + "IMPORTANT: Find the L-prefix on the exact code line your comment applies to and use that number. Do NOT guess or count lines yourself." + ) + + table.insert(parts, "") + table.insert(parts, "## Files") + + for _, file in ipairs(files) do + local path = file.new_path or file.old_path + table.insert(parts, "") + + local content = contents_by_path and contents_by_path[path] + if content and content ~= "" then + local ext = path and path:match("%.([^%.]+)$") or "" + table.insert(parts, "### Full File Content: " .. path) + table.insert(parts, "```" .. ext) + table.insert(parts, content) + table.insert(parts, "```") + table.insert(parts, "The full file content is provided above for understanding only.") + table.insert(parts, "") + end + + table.insert(parts, "### File: " .. path) + table.insert(parts, "```diff") + table.insert(parts, M.annotate_diff_with_lines(file.diff or "")) + table.insert(parts, "```") + end + + return table.concat(parts, "\n") .. M.progress_suffix(opts and opts.progress_path) +end + function M.build_mr_prompt(branch, diff) return table.concat({ "I'm creating a merge request for branch: " .. branch, diff --git a/lua/codereview/config.lua b/lua/codereview/config.lua index ac0a35b..be1dc96 100644 --- a/lua/codereview/config.lua +++ b/lua/codereview/config.lua @@ -31,6 +31,8 @@ ---@field review_level? "info"|"suggestion"|"warning"|"error" controls the verbosity of AI code reviews (default: `info`) ---@field max_file_size? integer skip files larger than N lines (0 = unlimited) (default: 500) ---@field skip_patterns? string[] additional glob patterns to skip (additive to defaults) +---@field batch_char_budget? integer max diff chars per AI call (default: 80000) +---@field batch_max_files? integer max files per AI call (default: 15) ---@field claude_cli? codereview.config.ai.ClaudeCLI Claude CLI options ---@field codex_cli? codereview.config.ai.CodexCLI Codex CLI options ---@field copilot_cli? codereview.config.ai.CopilotCLI Copilot CLI options @@ -117,6 +119,8 @@ local defaults = { ollama = { model = "llama3", base_url = "http://localhost:11434" }, custom_cmd = { cmd = nil, args = {} }, skip_patterns = {}, + batch_char_budget = 80000, + batch_max_files = 15, }, keymaps = {}, } diff --git a/lua/codereview/plan/init.lua b/lua/codereview/plan/init.lua index d147af8..8dd121c 100644 --- a/lua/codereview/plan/init.lua +++ b/lua/codereview/plan/init.lua @@ -67,21 +67,21 @@ function M._run_phase2(branch, base, diffs) diffs = diffs, cacheable = true, build_prompt = function(batch, opts) - return plan_prompt.build_file_plan_prompt(batch[1], opts) + return plan_prompt.build_batch_plan_prompt(batch, opts) end, parse_output = plan_prompt.parse_file_plan_output, on_result = function() end, on_progress = function(done, t) spinner.set_label(string.format(" Planning %d/%d files… ", done, t)) end, - on_batch_complete = function() - completed_count = completed_count + 1 + on_batch_complete = function(batch) + completed_count = completed_count + #batch spinner.set_label(string.format(" Planning %d/%d files… ", completed_count, total)) end, on_error = function(err, batch) local p = batch[1].new_path or batch[1].old_path log.warn("Plan failed for " .. (p or "?") .. ": " .. err) - completed_count = completed_count + 1 + completed_count = completed_count + #batch spinner.set_label(string.format(" Planning %d/%d files… ", completed_count, total)) end, on_complete = function(all_tasks) diff --git a/lua/codereview/plan/prompt.lua b/lua/codereview/plan/prompt.lua index be9cbbc..dff8e4d 100644 --- a/lua/codereview/plan/prompt.lua +++ b/lua/codereview/plan/prompt.lua @@ -24,6 +24,41 @@ function M.build_file_plan_prompt(file, opts) return table.concat(parts, "\n") .. ai_prompt.progress_suffix(opts and opts.progress_path) end +--- Build a prompt for a batch of files (multiple diffs in one AI call). +--- Stable instructions first (cache-friendly); per-file diffs after "## Files". +--- +--- @param files table[] List of {new_path, old_path, diff} +--- @param opts table? {progress_path?: string} +--- @return string +function M.build_batch_plan_prompt(files, opts) + local parts = { + "You are creating an implementation plan for changes in files.", + "", + "## Instructions", + "", + "Analyze the diffs below and create tasks to complete or improve these implementations.", + "Output a JSON array in a ```json code block:", + '[{"file": "", "line": , "task": "", "reason": ""}]', + "", + "Focus on: incomplete implementations, missing error handling, TODOs, edge cases, missing tests.", + "If the code looks complete, output `[]`.", + } + + table.insert(parts, "") + table.insert(parts, "## Files") + + for _, file in ipairs(files) do + local path = file.new_path or file.old_path + table.insert(parts, "") + table.insert(parts, "### " .. path) + table.insert(parts, "```diff") + table.insert(parts, ai_prompt.annotate_diff_with_lines(file.diff or "")) + table.insert(parts, "```") + end + + return table.concat(parts, "\n") .. ai_prompt.progress_suffix(opts and opts.progress_path) +end + function M.parse_file_plan_output(output) if not output or output == "" then return {} diff --git a/lua/codereview/review/init.lua b/lua/codereview/review/init.lua index cdb0c04..36adc26 100644 --- a/lua/codereview/review/init.lua +++ b/lua/codereview/review/init.lua @@ -183,11 +183,19 @@ local function start_multi(review, diff_state, layout) orchestrator.run({ diffs = diffs, cacheable = true, + -- File content is much larger than diffs: use a tighter budget per batch. + batch_char_budget = 40000, + batch_max_files = 8, build_prompt = function(batch, opts) - local file = batch[1] - local path = file.new_path or file.old_path - local content = fetch_file_content(diff_state, review, path, file.deleted_file) - return prompt_mod.build_file_review_prompt(review, file, content, opts) + local contents_by_path = {} + for _, file in ipairs(batch) do + local path = file.new_path or file.old_path + local content = fetch_file_content(diff_state, review, path, file.deleted_file) + if content then + contents_by_path[path] = content + end + end + return prompt_mod.build_batch_review_prompt(review, batch, contents_by_path, opts) end, parse_output = prompt_mod.parse_review_output, on_result = function() end, @@ -195,29 +203,34 @@ local function start_multi(review, diff_state, layout) spinner.set_label(string.format(" AI reviewing… %d/%d files ", done, t)) end, on_batch_complete = function(batch, parsed) - local file = batch[1] - local suggestions = prompt_mod.filter_unchanged_lines(parsed, { file }) + local suggestions = prompt_mod.filter_unchanged_lines(parsed, batch) if #suggestions > 0 then render_file_suggestions(diff_state, layout, suggestions) end + local batch_size = #batch local s = session.get() - spinner.set_label(string.format(" AI reviewing… %d/%d files ", s.ai_completed + 1, s.ai_total)) + spinner.set_label(string.format(" AI reviewing… %d/%d files ", s.ai_completed + batch_size, s.ai_total)) vim.schedule(function() local diff_mod = require("codereview.mr.diff") diff_mod.render_sidebar(layout.sidebar_buf, diff_state) end) - session.ai_file_done() + for _ = 1, batch_size do + session.ai_file_done() + end end, on_error = function(err, batch) local path = batch[1].new_path or batch[1].old_path vim.notify("AI review failed for " .. path .. ": " .. err, vim.log.levels.WARN) + local batch_size = #batch local s = session.get() - spinner.set_label(string.format(" AI reviewing… %d/%d files ", s.ai_completed + 1, s.ai_total)) + spinner.set_label(string.format(" AI reviewing… %d/%d files ", s.ai_completed + batch_size, s.ai_total)) vim.schedule(function() local diff_mod = require("codereview.mr.diff") diff_mod.render_sidebar(layout.sidebar_buf, diff_state) end) - session.ai_file_done() + for _ = 1, batch_size do + session.ai_file_done() + end end, on_complete = function() local count = #(diff_state.ai_suggestions or {}) diff --git a/tests/codereview/ai/batch_spec.lua b/tests/codereview/ai/batch_spec.lua new file mode 100644 index 0000000..31a0043 --- /dev/null +++ b/tests/codereview/ai/batch_spec.lua @@ -0,0 +1,59 @@ +local batch = require("codereview.ai.batch") + +describe("batch.build", function() + it("packs by char budget", function() + local diffs = {} + for i = 1, 5 do + diffs[i] = { new_path = "f" .. i, diff = string.rep("x", 30) } + end + local out = batch.build(diffs, { char_budget = 100, max_files = 99 }) + assert.are.equal(2, #out) + assert.are.equal(3, #out[1]) + assert.are.equal(2, #out[2]) + end) + + it("oversize file goes alone", function() + local diffs = { + { new_path = "s", diff = string.rep("x", 10) }, + { new_path = "huge", diff = string.rep("x", 500) }, + { new_path = "t", diff = string.rep("x", 5) }, + } + local out = batch.build(diffs, { char_budget = 100, max_files = 99 }) + assert.are.equal(3, #out) + assert.are.equal("huge", out[2][1].new_path) + end) + + it("respects max_files cap", function() + local diffs = {} + for i = 1, 20 do + diffs[i] = { new_path = "f" .. i, diff = "x" } + end + local out = batch.build(diffs, { char_budget = 1e9, max_files = 5 }) + assert.are.equal(4, #out) + assert.are.equal(5, #out[1]) + end) + + it("single file returns single batch", function() + local diffs = { { new_path = "a.lua", diff = "@@\n+line" } } + local out = batch.build(diffs, { char_budget = 80000, max_files = 15 }) + assert.are.equal(1, #out) + assert.are.equal(1, #out[1]) + assert.are.equal("a.lua", out[1][1].new_path) + end) + + it("empty diffs returns empty batches", function() + local out = batch.build({}, { char_budget = 80000, max_files = 15 }) + assert.are.equal(0, #out) + end) + + it("uses defaults when opts omitted", function() + local diffs = {} + for i = 1, 3 do + diffs[i] = { new_path = "f" .. i, diff = "x" } + end + -- Should not error; all fit in one batch with generous defaults + local out = batch.build(diffs, nil) + assert.are.equal(1, #out) + assert.are.equal(3, #out[1]) + end) +end) diff --git a/tests/codereview/ai/orchestrator_spec.lua b/tests/codereview/ai/orchestrator_spec.lua index f6e10cb..5055458 100644 --- a/tests/codereview/ai/orchestrator_spec.lua +++ b/tests/codereview/ai/orchestrator_spec.lua @@ -51,7 +51,7 @@ end describe("orchestrator", function() local orchestrator = require("codereview.ai.orchestrator") - it("calls build_prompt once per file (batch size 1) and parses output", function() + it("calls build_prompt once per batch and parses output", function() local prompts, results = {}, {} local fake_provider = { run = function(p, cb) @@ -70,6 +70,8 @@ describe("orchestrator", function() local done = false orchestrator.run({ diffs = { { new_path = "a", diff = "" }, { new_path = "b", diff = "" } }, + -- Force 1 file per batch so we get 2 prompts (one per file) + batch_max_files = 1, build_prompt = function(batch) return "P:" .. batch[1].new_path end, @@ -89,6 +91,51 @@ describe("orchestrator", function() assert.is_true(done) end) + it("packs multiple files into one batch within budget", function() + local prompts = {} + local fake_provider = { + run = function(p, cb) + table.insert(prompts, p) + cb("[]") + end, + } + package.loaded["codereview.ai.providers"] = { + get = function() + return fake_provider + end, + } + package.loaded["codereview.ai.orchestrator"] = nil + orchestrator = require("codereview.ai.orchestrator") + + local done = false + orchestrator.run({ + diffs = { + { new_path = "a", diff = string.rep("x", 10) }, + { new_path = "b", diff = string.rep("x", 10) }, + { new_path = "c", diff = string.rep("x", 10) }, + }, + -- Budget large enough to fit all 3 → 1 AI call + batch_char_budget = 80000, + batch_max_files = 15, + build_prompt = function(batch) + local paths = {} + for _, f in ipairs(batch) do + table.insert(paths, f.new_path) + end + return table.concat(paths, ",") + end, + parse_output = function() + return {} + end, + on_result = function() end, + on_complete = function() + done = true + end, + }) + assert.are.equal(1, #prompts) + assert.is_true(done) + end) + it("on_error fires when provider returns error; on_complete still fires", function() local fake = { run = function(_, cb) @@ -141,6 +188,8 @@ describe("orchestrator", function() orchestrator.run({ diffs = { { new_path = "a", diff = "" }, { new_path = "b", diff = "" } }, + -- Force 1 file per batch so we get 2 on_batch_complete calls + batch_max_files = 1, build_prompt = function(batch) return batch[1].new_path end, diff --git a/tests/codereview/plan/integration_spec.lua b/tests/codereview/plan/integration_spec.lua index efe9c49..fcc11d1 100644 --- a/tests/codereview/plan/integration_spec.lua +++ b/tests/codereview/plan/integration_spec.lua @@ -12,6 +12,7 @@ describe("CodeReviewPlan integration", function() local ok, prompt = pcall(require, "codereview.plan.prompt") assert.is_true(ok) assert.is_function(prompt.build_file_plan_prompt) + assert.is_function(prompt.build_batch_plan_prompt) assert.is_function(prompt.parse_file_plan_output) assert.is_function(prompt.build_combine_prompt) assert.is_function(prompt.parse_summary) @@ -26,4 +27,16 @@ describe("CodeReviewPlan integration", function() assert.is_function(git.sanitize_branch_name) assert.is_function(git.diff_against_base) end) + + it("5 small files fit in 1 batch at default budget", function() + local batch = require("codereview.ai.batch") + local diffs = {} + for i = 1, 5 do + diffs[i] = { new_path = "file" .. i .. ".lua", diff = string.rep("+line\n", 10) } + end + -- 5 files × ~60 chars each = ~300 chars, well under default 80 000 budget + local batches = batch.build(diffs, { char_budget = 80000, max_files = 15 }) + assert.are.equal(1, #batches) + assert.are.equal(5, #batches[1]) + end) end) From 8564b30b311c946c362b2f880c32f27078460c45 Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Wed, 29 Apr 2026 12:57:59 +0200 Subject: [PATCH 14/21] fix(ai): address code review findings (filtered-files spinner, progress race, parser fallback) --- lua/codereview/ai/orchestrator.lua | 2 +- lua/codereview/ai/progress.lua | 4 +++ lua/codereview/ai/prompt.lua | 4 --- lua/codereview/plan/prompt.lua | 3 -- lua/codereview/review/init.lua | 9 +++-- tests/codereview/ai/prompt_spec.lua | 7 ++++ tests/codereview/review/init_spec.lua | 49 +++++++++++++++++++++++++++ 7 files changed, 68 insertions(+), 10 deletions(-) diff --git a/lua/codereview/ai/orchestrator.lua b/lua/codereview/ai/orchestrator.lua index 1fc29c6..dcb1056 100644 --- a/lua/codereview/ai/orchestrator.lua +++ b/lua/codereview/ai/orchestrator.lua @@ -74,7 +74,7 @@ function M.run(spec) local prompt_str = spec.build_prompt(batch, prompt_opts) if spec.cacheable and type(prompt_str) == "string" then if provider_name == "anthropic" then - local split = prompt_str:find("\n## Files?\n", 1, false) or prompt_str:find("\n## File:", 1, true) + local split = prompt_str:find("\n## Files?\n", 1, false) if split then prompt_str = { system = prompt_str:sub(1, split - 1), user = prompt_str:sub(split + 1) } end diff --git a/lua/codereview/ai/progress.lua b/lua/codereview/ai/progress.lua index c0edd62..9a920e2 100644 --- a/lua/codereview/ai/progress.lua +++ b/lua/codereview/ai/progress.lua @@ -40,6 +40,9 @@ function Progress:watch(on_change) 250, 250, vim.schedule_wrap(function() + if self._stopped then + return + end on_change(self:count()) end) ) @@ -47,6 +50,7 @@ end --- Stop the poll timer and delete the tmp file. function Progress:cleanup() + self._stopped = true if self._timer then self._timer:stop() self._timer:close() diff --git a/lua/codereview/ai/prompt.lua b/lua/codereview/ai/prompt.lua index 50bbe2b..8fdf579 100644 --- a/lua/codereview/ai/prompt.lua +++ b/lua/codereview/ai/prompt.lua @@ -246,10 +246,6 @@ function M.parse_review_output(output) -- Use greedy match (.+) to capture the full JSON block (handles nested fences/brackets) local json_str = output:match("```json%s*\n(.+)\n```") - if not json_str then - -- Fallback: greedy match for a JSON array (handles ] inside strings) - json_str = output:match("%[.+%]") - end if not json_str then log.debug("AI parse: no JSON block found in output (length=" .. #output .. ")") return {} diff --git a/lua/codereview/plan/prompt.lua b/lua/codereview/plan/prompt.lua index dff8e4d..10753e7 100644 --- a/lua/codereview/plan/prompt.lua +++ b/lua/codereview/plan/prompt.lua @@ -65,9 +65,6 @@ function M.parse_file_plan_output(output) end local json_str = output:match("```json%s*\n(.+)\n```") - if not json_str then - json_str = output:match("%[.+%]") - end if not json_str then return {} end diff --git a/lua/codereview/review/init.lua b/lua/codereview/review/init.lua index 36adc26..ae909ec 100644 --- a/lua/codereview/review/init.lua +++ b/lua/codereview/review/init.lua @@ -178,10 +178,15 @@ local function start_multi(review, diff_state, layout) diff_state.current_file = diff_state.current_file or 1 end - local total = #diffs + -- Pre-apply file filter so session.ai_start receives the correct post-filter total. + -- The orchestrator's own filter pass is then a no-op on already-filtered input. + local cfg = require("codereview.config").get() + local file_filter = require("codereview.ai.file_filter") + local filtered_diffs = file_filter.apply(diffs, (cfg.ai or {}).skip_patterns) + local total = #filtered_diffs orchestrator.run({ - diffs = diffs, + diffs = filtered_diffs, cacheable = true, -- File content is much larger than diffs: use a tighter budget per batch. batch_char_budget = 40000, diff --git a/tests/codereview/ai/prompt_spec.lua b/tests/codereview/ai/prompt_spec.lua index 9210d33..553dc8d 100644 --- a/tests/codereview/ai/prompt_spec.lua +++ b/tests/codereview/ai/prompt_spec.lua @@ -311,6 +311,13 @@ describe("ai.prompt", function() assert.equals(0, #suggestions) end) + it("ignores bare JSON array not wrapped in a code fence", function() + -- Without the greedy fallback, unfenced arrays must return {} + local output = '[{"file":"a.lua","line":1,"severity":"info","comment":"test"}]' + local suggestions = prompt.parse_review_output(output) + assert.equals(0, #suggestions) + end) + it("handles malformed JSON gracefully", function() local suggestions = prompt.parse_review_output("```json\n{broken\n```") assert.equals(0, #suggestions) diff --git a/tests/codereview/review/init_spec.lua b/tests/codereview/review/init_spec.lua index 0a4982e..ae67ab0 100644 --- a/tests/codereview/review/init_spec.lua +++ b/tests/codereview/review/init_spec.lua @@ -140,6 +140,55 @@ describe("review.init routing", function() end) end) +describe("review.start_multi filtered-file count", function() + local orig_ai_start, orig_filter + + before_each(function() + captured_calls = {} + orig_ai_start = package.loaded["codereview.review.session"].ai_start + orig_filter = package.loaded["codereview.ai.file_filter"] + end) + + after_each(function() + package.loaded["codereview.review.session"].ai_start = orig_ai_start + package.loaded["codereview.ai.file_filter"] = orig_filter + end) + + it("ai_start total uses post-filter count when file_filter drops files", function() + local ai_start_calls = {} + package.loaded["codereview.review.session"].ai_start = function(ids, total) + table.insert(ai_start_calls, { ids = ids, total = total }) + end + + -- Filter that keeps only the first file + package.loaded["codereview.ai.file_filter"] = { + apply = function(diffs, _) + return { diffs[1] } + end, + } + + local review = { title = "Multi", description = "desc" } + local diff_state = { + files = { + { new_path = "a.lua", diff = "diff-a" }, + { new_path = "package-lock.json", diff = "diff-lock" }, + }, + discussions = {}, + ai_suggestions = {}, + view_mode = "diff", + current_file = 1, + scroll_mode = false, + line_data_cache = {}, + row_disc_cache = {}, + row_ai_cache = {}, + } + review_mod.start(review, diff_state, { main_buf = 0, sidebar_buf = 0, main_win = 0 }) + + assert.equals(1, #ai_start_calls, "ai_start should be called once") + assert.equals(1, ai_start_calls[1].total, "ai_start total should equal filtered count (1), not original (2)") + end) +end) + describe("render_file_suggestions focus guard", function() local orig_run, orig_get_current_win, orig_set_current_win, orig_schedule From 41e0b1c004424ed5167fa278c27b577aef087bd6 Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Mon, 4 May 2026 13:37:46 +0200 Subject: [PATCH 15/21] feat(ai): add ai_skip_patterns parser to auth module --- ...2026-05-04-ai-skip-patterns-config-plan.md | 321 ++++++++++++++++++ lua/codereview/api/auth.lua | 24 ++ tests/codereview/auth_skip_patterns_spec.lua | 38 +++ 3 files changed, 383 insertions(+) create mode 100644 docs/plans/2026-05-04-ai-skip-patterns-config-plan.md create mode 100644 tests/codereview/auth_skip_patterns_spec.lua diff --git a/docs/plans/2026-05-04-ai-skip-patterns-config-plan.md b/docs/plans/2026-05-04-ai-skip-patterns-config-plan.md new file mode 100644 index 0000000..001f4b3 --- /dev/null +++ b/docs/plans/2026-05-04-ai-skip-patterns-config-plan.md @@ -0,0 +1,321 @@ +# AI Skip Patterns Config Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Allow per-repo AI skip patterns via `ai_skip_patterns` key in `.codereview.nvim` file. + +**Architecture:** Parse comma-separated patterns from dotfile, expose via auth module, merge with Lua config patterns at filter call sites. + +**Tech Stack:** Lua, Neovim plugin APIs + +--- + +## Task 1: Add pattern parser to auth.lua + +**Files:** +- Modify: `lua/codereview/api/auth.lua:115-118` +- Test: `tests/codereview/auth_skip_patterns_spec.lua` (create) + +**Step 1: Write failing test** + +Create `tests/codereview/auth_skip_patterns_spec.lua`: + +```lua +local auth = require("codereview.api.auth") + +describe("auth.get_ai_skip_patterns", function() + before_each(function() + auth.reset() + end) + + it("returns empty table when no config file", function() + local patterns = auth.get_ai_skip_patterns() + assert.same({}, patterns) + end) + + it("parses comma-separated patterns", function() + -- Mock will be needed; for now test the parse logic + local parse = auth._parse_skip_patterns_for_test + local result = parse("*.test.ts,docs/**,*.snap") + assert.same({ "*.test.ts", "docs/**", "*.snap" }, result) + end) + + it("trims whitespace around patterns", function() + local parse = auth._parse_skip_patterns_for_test + local result = parse(" *.test.ts , docs/** ,*.snap ") + assert.same({ "*.test.ts", "docs/**", "*.snap" }, result) + end) + + it("handles empty string", function() + local parse = auth._parse_skip_patterns_for_test + local result = parse("") + assert.same({}, result) + end) +end) +``` + +**Step 2: Run test to verify it fails** + +Run: `cd /Users/kleist/Sites/codereview.nvim && nvim --headless -u tests/minimal_init.lua -c "PlenaryBustedFile tests/codereview/auth_skip_patterns_spec.lua"` + +Expected: FAIL — `get_ai_skip_patterns` and `_parse_skip_patterns_for_test` not defined + +**Step 3: Implement in auth.lua** + +Add before `return M` (around line 117): + +```lua +local function parse_skip_patterns(value) + if not value or value == "" then + return {} + end + local patterns = {} + for pat in value:gmatch("[^,]+") do + local trimmed = pat:match("^%s*(.-)%s*$") + if trimmed ~= "" then + table.insert(patterns, trimmed) + end + end + return patterns +end + +function M.get_ai_skip_patterns() + local file_config = read_config_file() + if not file_config or not file_config.ai_skip_patterns then + return {} + end + return parse_skip_patterns(file_config.ai_skip_patterns) +end + +M._parse_skip_patterns_for_test = parse_skip_patterns +``` + +**Step 4: Run test to verify it passes** + +Run: `cd /Users/kleist/Sites/codereview.nvim && nvim --headless -u tests/minimal_init.lua -c "PlenaryBustedFile tests/codereview/auth_skip_patterns_spec.lua"` + +Expected: PASS + +**Step 5: Commit** + +```bash +git add lua/codereview/api/auth.lua tests/codereview/auth_skip_patterns_spec.lua +git commit -m "feat(ai): add ai_skip_patterns parser to auth module" +``` + +--- + +## Task 2: Update orchestrator.lua to merge patterns + +**Files:** +- Modify: `lua/codereview/ai/orchestrator.lua:28-31` + +**Step 1: Write failing test** + +Create `tests/codereview/ai/orchestrator_skip_patterns_spec.lua`: + +```lua +describe("orchestrator skip patterns merge", function() + local orchestrator = require("codereview.ai.orchestrator") + local auth = require("codereview.api.auth") + local config = require("codereview.config") + + before_each(function() + auth.reset() + end) + + it("merges dotfile patterns with config patterns", function() + -- This is an integration concern; verify via file_filter.apply call + -- The key behavior: both sources should be combined + local file_filter = require("codereview.ai.file_filter") + local diffs = { + { new_path = "src/app.ts", diff = "..." }, + { new_path = "src/app.test.ts", diff = "..." }, + } + -- With pattern *.test.ts, second file should be filtered + local result = file_filter.apply(diffs, { "*.test.ts" }) + assert.equals(1, #result) + assert.equals("src/app.ts", result[1].new_path) + end) +end) +``` + +**Step 2: Run test to verify baseline** + +Run: `cd /Users/kleist/Sites/codereview.nvim && nvim --headless -u tests/minimal_init.lua -c "PlenaryBustedFile tests/codereview/ai/orchestrator_skip_patterns_spec.lua"` + +Expected: PASS (baseline — file_filter works) + +**Step 3: Update orchestrator.lua** + +Change line 31 from: + +```lua + spec.diffs = file_filter.apply(spec.diffs or {}, (cfg.ai or {}).skip_patterns) +``` + +To: + +```lua + local auth = require("codereview.api.auth") + local lua_patterns = (cfg.ai or {}).skip_patterns or {} + local dotfile_patterns = auth.get_ai_skip_patterns() + local merged = {} + for _, p in ipairs(lua_patterns) do table.insert(merged, p) end + for _, p in ipairs(dotfile_patterns) do table.insert(merged, p) end + spec.diffs = file_filter.apply(spec.diffs or {}, merged) +``` + +**Step 4: Run test to verify it passes** + +Run: `cd /Users/kleist/Sites/codereview.nvim && nvim --headless -u tests/minimal_init.lua -c "PlenaryBustedFile tests/codereview/ai/orchestrator_skip_patterns_spec.lua"` + +Expected: PASS + +**Step 5: Commit** + +```bash +git add lua/codereview/ai/orchestrator.lua tests/codereview/ai/orchestrator_skip_patterns_spec.lua +git commit -m "feat(ai): merge dotfile skip patterns in orchestrator" +``` + +--- + +## Task 3: Update review/init.lua to merge patterns + +**Files:** +- Modify: `lua/codereview/review/init.lua:185` + +**Step 1: Locate and update** + +Change line 185 from: + +```lua + local filtered_diffs = file_filter.apply(diffs, (cfg.ai or {}).skip_patterns) +``` + +To: + +```lua + local auth = require("codereview.api.auth") + local lua_patterns = (cfg.ai or {}).skip_patterns or {} + local dotfile_patterns = auth.get_ai_skip_patterns() + local merged = {} + for _, p in ipairs(lua_patterns) do table.insert(merged, p) end + for _, p in ipairs(dotfile_patterns) do table.insert(merged, p) end + local filtered_diffs = file_filter.apply(diffs, merged) +``` + +**Step 2: Run existing tests** + +Run: `cd /Users/kleist/Sites/codereview.nvim && nvim --headless -u tests/minimal_init.lua -c "PlenaryBustedDirectory tests/codereview/"` + +Expected: All tests PASS + +**Step 3: Commit** + +```bash +git add lua/codereview/review/init.lua +git commit -m "feat(ai): merge dotfile skip patterns in review init" +``` + +--- + +## Task 4: Extract merge helper to file_filter.lua (DRY) + +**Files:** +- Modify: `lua/codereview/ai/file_filter.lua` +- Modify: `lua/codereview/ai/orchestrator.lua` +- Modify: `lua/codereview/review/init.lua` + +**Step 1: Add helper to file_filter.lua** + +Add before `return M`: + +```lua +function M.get_all_skip_patterns() + local config = require("codereview.config").get() + local auth = require("codereview.api.auth") + local lua_patterns = (config.ai or {}).skip_patterns or {} + local dotfile_patterns = auth.get_ai_skip_patterns() + local merged = {} + for _, p in ipairs(lua_patterns) do table.insert(merged, p) end + for _, p in ipairs(dotfile_patterns) do table.insert(merged, p) end + return merged +end +``` + +**Step 2: Simplify orchestrator.lua call site** + +Replace the merge logic with: + +```lua + spec.diffs = file_filter.apply(spec.diffs or {}, file_filter.get_all_skip_patterns()) +``` + +**Step 3: Simplify review/init.lua call site** + +Replace the merge logic with: + +```lua + local filtered_diffs = file_filter.apply(diffs, file_filter.get_all_skip_patterns()) +``` + +**Step 4: Run all tests** + +Run: `cd /Users/kleist/Sites/codereview.nvim && nvim --headless -u tests/minimal_init.lua -c "PlenaryBustedDirectory tests/codereview/"` + +Expected: All tests PASS + +**Step 5: Commit** + +```bash +git add lua/codereview/ai/file_filter.lua lua/codereview/ai/orchestrator.lua lua/codereview/review/init.lua +git commit -m "refactor(ai): extract get_all_skip_patterns helper to file_filter" +``` + +--- + +## Task 5: Update README documentation + +**Files:** +- Modify: `README.md` (configuration section) + +**Step 1: Find config docs section** + +Search for existing `.codereview.nvim` documentation in README. + +**Step 2: Add ai_skip_patterns docs** + +Add to the `.codereview.nvim` section: + +```markdown +### AI Skip Patterns + +Skip specific files from AI review by adding patterns to `.codereview.nvim`: + +``` +ai_skip_patterns=*.test.ts,docs/**,*.snap,fixtures/** +``` + +Patterns are comma-separated globs, merged with plugin config `ai.skip_patterns` and built-in defaults (lockfiles, minified files, generated code, vendor directories). +``` + +**Step 3: Commit** + +```bash +git add README.md +git commit -m "docs: add ai_skip_patterns configuration" +``` + +--- + +## Summary + +| Task | Description | Deps | +|------|-------------|------| +| 1 | Parser in auth.lua + tests | — | +| 2 | Merge in orchestrator.lua | 1 | +| 3 | Merge in review/init.lua | 1 | +| 4 | Extract DRY helper | 2, 3 | +| 5 | README docs | 4 | diff --git a/lua/codereview/api/auth.lua b/lua/codereview/api/auth.lua index dc6ea35..8407184 100644 --- a/lua/codereview/api/auth.lua +++ b/lua/codereview/api/auth.lua @@ -111,9 +111,33 @@ function M.refresh(platform) return nil, nil end +local function parse_skip_patterns(value) + if not value or value == "" then + return {} + end + local patterns = {} + for pat in value:gmatch("[^,]+") do + local trimmed = pat:match("^%s*(.-)%s*$") + if trimmed ~= "" then + table.insert(patterns, trimmed) + end + end + return patterns +end + +function M.get_ai_skip_patterns() + local file_config = read_config_file() + if not file_config or not file_config.ai_skip_patterns then + return {} + end + return parse_skip_patterns(file_config.ai_skip_patterns) +end + -- Exposed for testing only M._read_config_file_for_test = function() return read_config_file() end +M._parse_skip_patterns_for_test = parse_skip_patterns + return M diff --git a/tests/codereview/auth_skip_patterns_spec.lua b/tests/codereview/auth_skip_patterns_spec.lua new file mode 100644 index 0000000..efdbaa8 --- /dev/null +++ b/tests/codereview/auth_skip_patterns_spec.lua @@ -0,0 +1,38 @@ +local auth = require("codereview.api.auth") + +describe("auth.get_ai_skip_patterns", function() + before_each(function() + auth.reset() + end) + + it("returns empty table when no config file", function() + local saved_filereadable = vim.fn.filereadable + vim.fn.filereadable = function(path) + if path and path:match("%.codereview%.nvim$") then + return 0 + end + return saved_filereadable(path) + end + local patterns = auth.get_ai_skip_patterns() + vim.fn.filereadable = saved_filereadable + assert.same({}, patterns) + end) + + it("parses comma-separated patterns", function() + local parse = auth._parse_skip_patterns_for_test + local result = parse("*.test.ts,docs/**,*.snap") + assert.same({ "*.test.ts", "docs/**", "*.snap" }, result) + end) + + it("trims whitespace around patterns", function() + local parse = auth._parse_skip_patterns_for_test + local result = parse(" *.test.ts , docs/** ,*.snap ") + assert.same({ "*.test.ts", "docs/**", "*.snap" }, result) + end) + + it("handles empty string", function() + local parse = auth._parse_skip_patterns_for_test + local result = parse("") + assert.same({}, result) + end) +end) From d57128a10c2d5e20517268e6fdbe399f160b4a79 Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Mon, 4 May 2026 14:08:42 +0200 Subject: [PATCH 16/21] feat(ai): merge dotfile skip patterns in orchestrator --- lua/codereview/ai/orchestrator.lua | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lua/codereview/ai/orchestrator.lua b/lua/codereview/ai/orchestrator.lua index dcb1056..fba53de 100644 --- a/lua/codereview/ai/orchestrator.lua +++ b/lua/codereview/ai/orchestrator.lua @@ -28,7 +28,17 @@ function M.run(spec) local cfg = require("codereview.config").get() local file_filter = require("codereview.ai.file_filter") local before = #(spec.diffs or {}) - spec.diffs = file_filter.apply(spec.diffs or {}, (cfg.ai or {}).skip_patterns) + local auth = require("codereview.api.auth") + local lua_patterns = (cfg.ai or {}).skip_patterns or {} + local dotfile_patterns = auth.get_ai_skip_patterns() + local merged = {} + for _, p in ipairs(lua_patterns) do + table.insert(merged, p) + end + for _, p in ipairs(dotfile_patterns) do + table.insert(merged, p) + end + spec.diffs = file_filter.apply(spec.diffs or {}, merged) local skipped = before - #spec.diffs if skipped > 0 then vim.notify(string.format("Skipped %d file(s) (lockfiles/generated/binary)", skipped), vim.log.levels.INFO) From 9c4f355ddb32fd381c7590f98f63e5cc782529f8 Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Mon, 4 May 2026 14:09:00 +0200 Subject: [PATCH 17/21] feat(ai): merge dotfile skip patterns in review init --- lua/codereview/review/init.lua | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lua/codereview/review/init.lua b/lua/codereview/review/init.lua index ae909ec..a1af86c 100644 --- a/lua/codereview/review/init.lua +++ b/lua/codereview/review/init.lua @@ -182,7 +182,17 @@ local function start_multi(review, diff_state, layout) -- The orchestrator's own filter pass is then a no-op on already-filtered input. local cfg = require("codereview.config").get() local file_filter = require("codereview.ai.file_filter") - local filtered_diffs = file_filter.apply(diffs, (cfg.ai or {}).skip_patterns) + local auth = require("codereview.api.auth") + local lua_patterns = (cfg.ai or {}).skip_patterns or {} + local dotfile_patterns = auth.get_ai_skip_patterns() + local merged = {} + for _, p in ipairs(lua_patterns) do + table.insert(merged, p) + end + for _, p in ipairs(dotfile_patterns) do + table.insert(merged, p) + end + local filtered_diffs = file_filter.apply(diffs, merged) local total = #filtered_diffs orchestrator.run({ From e74b249534db50d3eb7c748dc8cc54b5528cc01b Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Mon, 4 May 2026 14:26:55 +0200 Subject: [PATCH 18/21] refactor(ai): extract get_all_skip_patterns helper to file_filter --- lua/codereview/ai/file_filter.lua | 15 +++++++++++++++ lua/codereview/ai/orchestrator.lua | 12 +----------- lua/codereview/review/init.lua | 13 +------------ tests/codereview/review/init_spec.lua | 3 +++ 4 files changed, 20 insertions(+), 23 deletions(-) diff --git a/lua/codereview/ai/file_filter.lua b/lua/codereview/ai/file_filter.lua index 8890d94..cea00e5 100644 --- a/lua/codereview/ai/file_filter.lua +++ b/lua/codereview/ai/file_filter.lua @@ -79,4 +79,19 @@ function M.apply(diffs, user_patterns) return out end +function M.get_all_skip_patterns() + local config = require("codereview.config").get() + local auth = require("codereview.api.auth") + local lua_patterns = (config.ai or {}).skip_patterns or {} + local dotfile_patterns = auth.get_ai_skip_patterns() + local merged = {} + for _, p in ipairs(lua_patterns) do + table.insert(merged, p) + end + for _, p in ipairs(dotfile_patterns) do + table.insert(merged, p) + end + return merged +end + return M diff --git a/lua/codereview/ai/orchestrator.lua b/lua/codereview/ai/orchestrator.lua index fba53de..e56c14b 100644 --- a/lua/codereview/ai/orchestrator.lua +++ b/lua/codereview/ai/orchestrator.lua @@ -28,17 +28,7 @@ function M.run(spec) local cfg = require("codereview.config").get() local file_filter = require("codereview.ai.file_filter") local before = #(spec.diffs or {}) - local auth = require("codereview.api.auth") - local lua_patterns = (cfg.ai or {}).skip_patterns or {} - local dotfile_patterns = auth.get_ai_skip_patterns() - local merged = {} - for _, p in ipairs(lua_patterns) do - table.insert(merged, p) - end - for _, p in ipairs(dotfile_patterns) do - table.insert(merged, p) - end - spec.diffs = file_filter.apply(spec.diffs or {}, merged) + spec.diffs = file_filter.apply(spec.diffs or {}, file_filter.get_all_skip_patterns()) local skipped = before - #spec.diffs if skipped > 0 then vim.notify(string.format("Skipped %d file(s) (lockfiles/generated/binary)", skipped), vim.log.levels.INFO) diff --git a/lua/codereview/review/init.lua b/lua/codereview/review/init.lua index a1af86c..47a5bfc 100644 --- a/lua/codereview/review/init.lua +++ b/lua/codereview/review/init.lua @@ -180,19 +180,8 @@ local function start_multi(review, diff_state, layout) -- Pre-apply file filter so session.ai_start receives the correct post-filter total. -- The orchestrator's own filter pass is then a no-op on already-filtered input. - local cfg = require("codereview.config").get() local file_filter = require("codereview.ai.file_filter") - local auth = require("codereview.api.auth") - local lua_patterns = (cfg.ai or {}).skip_patterns or {} - local dotfile_patterns = auth.get_ai_skip_patterns() - local merged = {} - for _, p in ipairs(lua_patterns) do - table.insert(merged, p) - end - for _, p in ipairs(dotfile_patterns) do - table.insert(merged, p) - end - local filtered_diffs = file_filter.apply(diffs, merged) + local filtered_diffs = file_filter.apply(diffs, file_filter.get_all_skip_patterns()) local total = #filtered_diffs orchestrator.run({ diff --git a/tests/codereview/review/init_spec.lua b/tests/codereview/review/init_spec.lua index ae67ab0..ddb75ef 100644 --- a/tests/codereview/review/init_spec.lua +++ b/tests/codereview/review/init_spec.lua @@ -162,6 +162,9 @@ describe("review.start_multi filtered-file count", function() -- Filter that keeps only the first file package.loaded["codereview.ai.file_filter"] = { + get_all_skip_patterns = function() + return {} + end, apply = function(diffs, _) return { diffs[1] } end, From 7be4028a220abf9bcc6ad3758de136cfab9a54c7 Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Mon, 4 May 2026 14:27:46 +0200 Subject: [PATCH 19/21] docs: add ai_skip_patterns configuration --- README.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/README.md b/README.md index 373b6e6..5902999 100644 --- a/README.md +++ b/README.md @@ -110,6 +110,7 @@ Lines starting with `#` are comments. Keys and values are trimmed of whitespace. | `project` | `owner/repo` (auto-detected from git remote if omitted) | | `base_url` | API URL override (e.g., self-hosted GitLab instance) | | `token` | Auth token for this project | +| `ai_skip_patterns` | Comma-separated glob patterns — files matching any pattern are excluded from AI review | > **Security:** Add `.codereview.nvim` to your `.gitignore` if it contains a token. @@ -229,6 +230,16 @@ The `ai.review_level` option controls the verbosity of AI code reviews. Higher l The AI is instructed to skip items below the configured level, saving tokens and reducing noise. To see lower-severity items again, change the level and re-run the AI review. +### AI Skip Patterns + +Skip specific files from AI review by adding patterns to `.codereview.nvim`: + +``` +ai_skip_patterns=*.test.ts,docs/**,*.snap,fixtures/** +``` + +Patterns are comma-separated globs, merged with plugin config `ai.skip_patterns` and built-in defaults (lockfiles, minified files, generated code, vendor directories). + ## Default Keymaps ### Navigation From b1307e4693824a97cdbfa32c12c5b4b7b43e4c3a Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Mon, 4 May 2026 14:46:06 +0200 Subject: [PATCH 20/21] fix(ai): URL encoding and glob pattern matching bugs - gitlab.lua: properly URL-encode file paths with spaces/special chars - file_filter.lua: fix glob_to_pat null-byte placeholder corruption The glob pattern `\0DBL\0` placeholder was interpreted by Lua's gsub as matching every position, corrupting patterns like `*.json` into garbage. Replaced with `<<>>` string placeholder. --- lua/codereview/ai/file_filter.lua | 2 +- lua/codereview/providers/gitlab.lua | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/lua/codereview/ai/file_filter.lua b/lua/codereview/ai/file_filter.lua index cea00e5..ea7560d 100644 --- a/lua/codereview/ai/file_filter.lua +++ b/lua/codereview/ai/file_filter.lua @@ -32,7 +32,7 @@ M.DEFAULT_PATTERNS = { ---@return string local function glob_to_pat(glob) local pat = glob:gsub("([%%%.%(%)%[%]%+%-%^%$])", "%%%1") - pat = pat:gsub("%*%*", "\0DBL\0"):gsub("%*", "[^/]*"):gsub("%?", "[^/]"):gsub("\0DBL\0", ".*") + pat = pat:gsub("%*%*", "<<>>"):gsub("%*", "[^/]*"):gsub("%?", "[^/]"):gsub("<<>>", ".*") return "^" .. pat .. "$" end diff --git a/lua/codereview/providers/gitlab.lua b/lua/codereview/providers/gitlab.lua index 81c7782..cb39da4 100644 --- a/lua/codereview/providers/gitlab.lua +++ b/lua/codereview/providers/gitlab.lua @@ -3,8 +3,14 @@ local M = {} M.name = "gitlab" +local function url_encode(str) + return str:gsub("([^%w%-._~])", function(c) + return string.format("%%%02X", string.byte(c)) + end) +end + local function encoded_project(ctx) - return ctx.project:gsub("/", "%%2F") + return url_encode(ctx.project) end local function mr_base(ctx, iid) @@ -199,7 +205,7 @@ function M.get_file_content(client, ctx, ref, path) if not headers then return nil, err end - local encoded_path = path:gsub("/", "%%2F") + local encoded_path = url_encode(path) local api_path = string.format("/api/v4/projects/%s/repository/files/%s/raw", encoded_project(ctx), encoded_path) local result, req_err = client.get(ctx.base_url, api_path, { headers = headers, From 701ab1cc1ca8b218f4aea48f259c682e813426f6 Mon Sep 17 00:00:00 2001 From: Thierry Michel Philippe Kleist Date: Wed, 13 May 2026 20:47:10 +0200 Subject: [PATCH 21/21] fix(api): convert JSON null to Lua nil on decode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitLab returns `description: null` for MRs with no description. By default `vim.json.decode` maps JSON null to `vim.NIL` (userdata), which is truthy — so existing `or ""` guards in `types.normalize_review` and `format_mr_preview` passed it through unchanged, causing `attempt to concatenate a userdata value` when the picker rendered the preview. Pass `{ luanil = { object = true, array = true } }` to all `vim.json.decode` call sites so null becomes nil throughout. Existing `~= vim.NIL` checks in github.lua remain correct (nil ~= vim.NIL). Fixes #22 --- lua/codereview/api/client.lua | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lua/codereview/api/client.lua b/lua/codereview/api/client.lua index e9b9a0e..c862d25 100644 --- a/lua/codereview/api/client.lua +++ b/lua/codereview/api/client.lua @@ -67,7 +67,7 @@ end local function process_response(response) local body = nil if response.body and response.body ~= "" then - local ok, decoded = pcall(vim.json.decode, response.body) + local ok, decoded = pcall(vim.json.decode, response.body, { luanil = { object = true, array = true } }) if ok then body = decoded else @@ -444,7 +444,7 @@ function M.graphql(url, headers, query, variables) return nil, string.format("HTTP %d: %s", response.status, response.body or "") end - local ok, data = pcall(vim.json.decode, response.body) + local ok, data = pcall(vim.json.decode, response.body, { luanil = { object = true, array = true } }) if not ok then return nil, "Failed to decode GraphQL response: " .. tostring(data) end @@ -481,7 +481,7 @@ function M.async_graphql(url, headers, query, variables) return nil, string.format("HTTP %d: %s", response.status, response.body or "") end - local ok, data = pcall(vim.json.decode, response.body) + local ok, data = pcall(vim.json.decode, response.body, { luanil = { object = true, array = true } }) if not ok then return nil, "Failed to decode GraphQL response: " .. tostring(data) end