Skip to content

Fix E816 malformed diffpatch output issue#287

Open
zawedcvg wants to merge 7 commits into
nickjvandyke:mainfrom
zawedcvg:main
Open

Fix E816 malformed diffpatch output issue#287
zawedcvg wants to merge 7 commits into
nickjvandyke:mainfrom
zawedcvg:main

Conversation

@zawedcvg

@zawedcvg zawedcvg commented Jun 7, 2026

Copy link
Copy Markdown

Tasks

  • I read CONTRIBUTING.md
  • I ensured my changes pass automated checks
  • I reviewed and understand the AI-generated code in my changes (if any)
  • I addressed actionable AI review feedback (if any)

Description

Issue basically was because of some incorrect file path handling. The current solution does work, but might not be the best, so let me know. Below is what the logs captured

2026-06-06 22:39:41 [opencode edits] opened edit diff buffer
{
  buf_name = "/home/user/.config/nvim/.config/nvim/lua/plugins/opencode.nvim/lua/opencode/events/permissions/edits.lua",
  buftype = "",
  current_buf = 38,
  current_tabpage = 2,
  current_win = 1038,
  error = "vim/_core/editor.lua:0: nvim_exec2(), line 1: Vim(diffpatch):E816: Cannot read patch output",
  filetype = "lua",
  modified = false,
  ok = false,
  windows_in_tab = { {
      buf = 38,
      diff = false,
      name = "/home/user/.config/nvim/.config/nvim/lua/plugins/opencode.nvim/lua/opencode/events/permissions/edits.lua",
      win = 1038
    } }
}

Testing

Related Issue(s)

Screenshots/Videos

@nickjvandyke

nickjvandyke commented Jun 7, 2026

Copy link
Copy Markdown
Owner

Thanks for taking a crack at this!

I'm a bit confused - why would this affect the diffpatch error? filepath is only used to open the current version of the edited file in tabnew. Whereas diffpatch receives patch_filepath, untouched by this PR.

@zawedcvg

zawedcvg commented Jun 7, 2026

Copy link
Copy Markdown
Author

I could be wrong but I think its due to this from the :help diffpatch

Note that {patchfile} should only contain a diff for one file,
the current file. If {patchfile} contains diffs for other
files as well, the results are unpredictable.

I think because its trying to diff with a file that doesn't exist, the error is being thrown. At least this is my guess for now.

@zawedcvg

zawedcvg commented Jun 8, 2026

Copy link
Copy Markdown
Author

Ok you can reproduce the E816 pretty easily. have a file and generate a random patch for it and store it somewhere. Then open a non existing file and diffpatch with this patch. You get E816.

Also interestingly, if you open diffpatch from an existing file, but the file is not the correct one, you get this, which is a thing that sometimes occurs with opencode
image

@nickjvandyke

Copy link
Copy Markdown
Owner

Ah I see, thanks for explaining! So basically OpenCode might send a filepath that's incorrect relative to how Neovim searches for the file to open it? And resolving it as an absolute path fixes that?

Why do we sometimes make it relative to the home directory? I don't see when the absolute path would fail and need to fallback to that.

Also interestingly, if you open diffpatch from an existing file, but the file is not the correct one, you get this, which is a thing that sometimes occurs with opencode

Ah thanks for figuring that out! It puzzled me. Does this change fix it too?

@nickjvandyke

Copy link
Copy Markdown
Owner

Given we have the server available in this scope, I wonder if local filepath = server.cwd .. event.properties.metadata.diff would be even more reliable? I presume OpenCode always sends the filepath relative to its CWD. Whereas Neovim could very rarely resolve the absolute path differently with very particular directory structures and CWD mismatches.

@zawedcvg

zawedcvg commented Jun 9, 2026

Copy link
Copy Markdown
Author

from what I remember the home directory branch was added because just making it absolute didn't completely solve. I might be misremembering but

local filepath = server.cwd .. event.properties.metadata.diff

doesn't solve it either. It has the same path issue because open code doesn't create paths wrt server.cwd.

Ah thanks for figuring that out! It puzzled me. Does this change fix it too?

Nope, it doesn't, just the malformed issue. I will spend sometime to see if I can find a better fix to the malformed issue and also fix the single line issue. Though the latter is a bit confusing in the sense that it opens the correct file, but the patch is not for that file? It doesn't make complete sense to me but I suspect it to be a similar file path issue. Sucks that diff patch doesn't have the greatest error handling but what can you do.

@zawedcvg

Copy link
Copy Markdown
Author

for the e816 error, I think this is the fix for now. Opencode sometimes sends the full path and sometimes everything but home, so I dont think there is a better solution.

I managed to figure out the bug with the single line thing, bad news is that there's nothing that can be done about it. So turns out that what opencode exposes in the diff, is a pretty printed version of the actual diff. It uses it just for display. It calls trimDiff on the actual unified patch, which can cause the patch to be malformed. Basically anytime the edit diff actually works with the plugin, it is due to luck. So yeah nothing can be done about this from the plugin side. I will open an issue and see if the opencode devs want to expose the actual patch, since it doesn't serve any purpose except for niche usecases like this.

@nickjvandyke

Copy link
Copy Markdown
Owner

/review

@github-actions

Copy link
Copy Markdown
Contributor

I've reviewed the changes. Here's my assessment:


What the code does: Resolves filepath by first trying :p (absolute from CWD), then falling back to $HOME + filepath. This prevents diffpatch from receiving a patch for a file path Neovim can't locate.

Correctness: The logic is sound — fnamemodify(":p") correctly handles already-absolute paths (returns as-is), and the HOME fallback handles the case where opencode sends paths like lua/opencode/events/... relative to $HOME. No bugs in the implementation.

Nit — silent failure when neither strategy works: If neither absolute_filepath nor home_filepath is readable, the code falls through with the original unresolved filepath. tabnew would create an empty "new file" buffer, and diffpatch would then fail with E816 all the same. Consider adding an early return with a vim.notify when the file can't be found, so the user gets a clear message instead of a cryptic E816.

Style — extraction: The resolution logic could be pulled into a small helper function for readability, e.g.:

local function resolve_filepath(filepath)
  local abs = vim.fn.fnamemodify(filepath, ":p")
  if vim.fn.filereadable(abs) == 1 then return vim.fn.fnameescape(abs) end
  local home = vim.fs.normalize(vim.fs.joinpath(vim.env.HOME or "", filepath))
  if vim.env.HOME and vim.fn.filereadable(home) == 1 then return vim.fn.fnameescape(home) end
  return nil
end

But this is purely stylistic — the current code works fine and is only 8 lines.

PR health: Good size (+14/-1), single file, focused on one issue. The discussion shows good collaboration between maintainer and contributor. The description could be stronger (clearer "why") but the conversation fills in the gaps.

New%20session%20-%202026-06-10T13%3A25%3A29.213Z
opencode session  |  github run

@zawedcvg

Copy link
Copy Markdown
Author

addressed the comments

Comment on lines +30 to +35
elseif vim.env.HOME and vim.env.HOME ~= "" then
local home_filepath = vim.fs.normalize(vim.fs.joinpath(vim.env.HOME, filepath))
if vim.fn.filereadable(home_filepath) == 1 then
filepath = home_filepath
end
else

@nickjvandyke nickjvandyke Jun 11, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It's unlikely, but I see a small bug here: the home_filepath readability check is in a nested if, so if it fails, we'll miss the outer else w/ the error notification.

Feel free to restructure how you like, but maybe after the absolute/home resolution attempts to update filepath, we put a standalone vim.fn.filereadable(filepath) check? Then no matter the code path, we check the final filepath before attempting to use it.

I think we also want an early return after the notification, so we don't get E816. And maybe vim.log.levels.ERROR.

end, { buffer = true, desc = "Close opencode edit diff" })
elseif event.type == "permission.replied" and current_edit_request_id == event.properties.requestID then
-- Entire edit was accepted or rejected, either in the plugin or TUI; close the diff
-- #NOTE: i don't think it works when accepting/rejecting from the TUI

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This worked last I tested, has that changed? Feel free to open a new bug report if so, but let's keep this PR focused 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants