Skip to content

fix(server): use vim.system instead of jobstart to bypass shell #560

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jakubbortlik merged 4 commits into from
May 26, 2026
Merged

fix(server): use vim.system instead of jobstart to bypass shell #560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c1c926e
fix(server): use list-form jobstart to bypass shell
JonLD May 14, 2026
d9e326a
Switch to using vim.system instead of vim.fn.jobstart
JonLD May 20, 2026
33b735e
Buffer server stderr
JonLD May 25, 2026
f68b9ac
Trim trailing newline from stderr notification
JonLD May 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ To view these help docs and to get more detailed help information, please run `:

## Requirements

- <a href="https://neovim.io/">Neovim</a> >= v0.10
- <a href="https://go.dev/">Go</a> >= v1.25.1

## Quick Start
Expand Down
81 changes: 36 additions & 45 deletions lua/gitlab/server.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
-- This module contains the logic responsible for building and starting
-- the Golang server. The Go server is responsible for making API calls
-- to Gitlab and returning the data
local List = require("gitlab.utils.list")
local state = require("gitlab.state")
local u = require("gitlab.utils")
local job = require("gitlab.job")
Expand Down Expand Up @@ -62,58 +61,50 @@ M.start = function(callback)
state.chosen_mr_iid = 0 -- Do not let this interfere with subsequent reviewer.open() calls

local settings = vim.json.encode(go_server_settings)
if vim.fn.has("win32") then
settings = settings:gsub('"', '\\"')
end

local command = string.format('"%s" "%s"', state.settings.server.binary, settings)

local job_id = vim.fn.jobstart(command, {
on_stdout = function(_, data)
-- if port was not provided then we need to parse it from output of server
if parsed_port == nil then
for _, line in ipairs(data) do
port = line:match("Server started on port:%s+(%d+)")
if port ~= nil then
parsed_port = port
state.settings.server.port = port
break
end
end
end
local stderr_buf = ""

-- This assumes that first output of server will be parsable and port will be correctly set.
-- Make sure that this actually check if port was correctly parsed based on server output
-- because server outputs port only if it started successfully.
if parsed_port ~= nil and not callback_called then
state.go_server_running = true
callback()
callback_called = true
local ok, err = pcall(vim.system, { state.settings.server.binary, settings }, {
stdout = function(_, data)
if data == nil or parsed_port ~= nil then
return
end
end,
on_stderr = function(_, errors)
local err_msg = List.new(errors):reduce(function(agg, err)
if err ~= "" and err ~= nil then
agg = agg .. err .. "\n"
for line in data:gmatch("[^\r\n]+") do
local matched = line:match("Server started on port:%s+(%d+)")
if matched ~= nil then
parsed_port = matched
vim.schedule(function()
state.settings.server.port = matched
state.go_server_running = true
if not callback_called then
callback_called = true
callback()
end
end)
break
end
return agg
end, "")

if err_msg ~= "" then
u.notify(err_msg, vim.log.levels.ERROR)
end
end,
on_exit = function(job_id, exit_code)
if exit_code ~= 0 then
u.notify(
"Golang gitlab server exited: job_id: " .. job_id .. ", exit_code: " .. exit_code,
vim.log.levels.ERROR
)
stderr = function(_, data)
if data == nil or data == "" then
return
end
stderr_buf = stderr_buf .. data
end,
Comment on lines +88 to 93
Copy link
Copy Markdown
Collaborator

@jakubbortlik jakubbortlik May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @JonLD, I'va had Claude review this and it says:

  Previously stderr was accumulated with List.reduce and shown as one message. Now each
  chunk fires a separate u.notify.

  If the Go server writes a multi-line error across two chunks, the user sees two separate
   notifications. For a long stacktrace this can be noisy. Simple fix: concatenate into a
  module-scoped buffer and flush on nil:

  local stderr_buf = ""
  stderr = function(_, data)
    if data == nil then
      if stderr_buf ~= "" then
        local msg = stderr_buf
        stderr_buf = ""
        vim.schedule(function() u.notify(msg, vim.log.levels.ERROR) end)
      end
      return
    end
    stderr_buf = stderr_buf .. data
  end,

Could you please verify that your modification is not a regression from the previous behaviour? I can confirm that now when I for example mess up the server settings, I'm getting two messages:

gitlab.nvim: Failure parsing plugin settings: invalid character 'B' after top-level value
gitlab.nvim: Golang gitlab server exited: code: 1, signal: 0

With the following patch, I get only one message (gitlab.nvim: Golang gitlab server exited: code: 1, signal: 0, msg: Failure parsing plugin settings: invalid character 'B' after top-level value) which I would prefer to see, if it's just one problem:

diff --git a/lua/gitlab/server.lua b/lua/gitlab/server.lua
index b06c5060..9d2e6a4e 100644
--- a/lua/gitlab/server.lua
+++ b/lua/gitlab/server.lua
@@ -62,6 +62,7 @@ M.start = function(callback)
 
   local settings = vim.json.encode(go_server_settings)
 
+  local stderr_buf, msg = "", ""
   local ok, err = pcall(vim.system, { state.settings.server.binary, settings }, {
     stdout = function(_, data)
       if data == nil or parsed_port ~= nil then
@@ -84,18 +85,20 @@ M.start = function(callback)
       end
     end,
     stderr = function(_, data)
-      if data == nil or data == "" then
+      if data == nil then
+        if stderr_buf ~= "" then
+          msg = stderr_buf
+          stderr_buf = ""
+        end
         return
       end
-      vim.schedule(function()
-        u.notify(data, vim.log.levels.ERROR)
-      end)
+      stderr_buf = stderr_buf .. data
     end,
   }, function(out)
     if out.code ~= 0 then
       vim.schedule(function()
         u.notify(
-          "Golang gitlab server exited: code: " .. out.code .. ", signal: " .. (out.signal or 0),
+          "Golang gitlab server exited: code: " .. out.code .. ", signal: " .. (out.signal or 0) .. ", msg: " .. msg,
           vim.log.levels.ERROR
         )
       end)

Copy link
Copy Markdown
Contributor Author

@JonLD JonLD May 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jakubbortlik, nice spot. I don't think this is a regression with my changes as on the head of develop you still get separate notifications for "Golang gitlab server exited" and the failure message. That said it does make sense to just combine them into one notification so I've gone ahead and made the change. It's slightly different to the diff you suggested but achieves the same.

Copy link
Copy Markdown
Collaborator

@jakubbortlik jakubbortlik May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the point of the "regression" was that your original stderr implementation would have notified for each "chunk of data":

    stderr = function(_, data)
      if data == nil or data == "" then
        return
      end
      vim.schedule(function()
        u.notify(data, vim.log.levels.ERROR)
      end)
    end,

The fact that both stderr and on_exit notified separately was a separate (and indeed pre-existing) issue.

Now your solutio looks much cleaner than my suggestion! The only thing I've noticed is that now the message can contain a trailing new line character that e.g., in Snacks notification history show as blank lines:
image
Could you please trim the final new lines?

Copy link
Copy Markdown
Contributor Author

@JonLD JonLD May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I see your point with the regression, didn't notice that in any testing.

Trailing newline trimmed now.

})
if job_id <= 0 then
u.notify("Could not start gitlab.nvim binary", vim.log.levels.ERROR)
}, function(out)
if out.code ~= 0 then
vim.schedule(function()
local msg = "Golang gitlab server exited: code: " .. out.code .. ", signal: " .. (out.signal or 0)
if stderr_buf ~= "" then
msg = msg .. ", msg: " .. vim.trim(stderr_buf)
end
u.notify(msg, vim.log.levels.ERROR)
end)
end
end)

if not ok then
u.notify("Could not start gitlab.nvim binary: " .. tostring(err), vim.log.levels.ERROR)
end
end

Expand Down
Loading