feat: per-account resolver pipeline#19
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the nvim-mail plugin by modularizing the contact resolution logic into a new async, per-account resolver pipeline, adding a reply module for reply-all drafts, and improving signature stripping and spell-checking. The code review identified several critical issues in the new async resolver, including a nested condition that bypasses LDAP lookups when Notmuch is unconfigured, a lack of error handling for non-zero exit codes in LDAP queries, and a race condition that could overwrite user edits during async resolution. Additionally, the feedback suggests defensively checking decoded JSON types, preventing queries on empty strings, scoping account detection to the correct buffer, and robustly handling comma-separated names in the address splitter.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if i > #khard_queries then | ||
| -- Stage 2: notmuch (only if resolver has email_pattern + domain) | ||
| if resolver_cfg and resolver_cfg.email_pattern and resolver_cfg.domain then | ||
| notmuch_lookup(name, resolver_cfg, acct_cfg, function(email) | ||
| if email then cb(email); return end | ||
| -- Stage 3: LDAP (only if resolver has ldap config) | ||
| if resolver_cfg.ldap then | ||
| ldap_lookup(name, resolver_cfg.ldap, cb) | ||
| else | ||
| cb(nil) | ||
| end | ||
| end) | ||
| else | ||
| cb(nil) | ||
| end | ||
| return |
There was a problem hiding this comment.
If resolver_cfg.email_pattern or resolver_cfg.domain is not configured, the Stage 3 LDAP lookup is completely bypassed because it is nested inside the Stage 2 notmuch_lookup callback, and the outer if condition immediately calls cb(nil).
LDAP lookup should be independent of Notmuch configuration. We can refactor this to chain the lookups so that LDAP is still attempted if Notmuch is disabled or unconfigured.
if i > #khard_queries then
local function try_ldap()
if resolver_cfg and resolver_cfg.ldap then
ldap_lookup(name, resolver_cfg.ldap, cb)
else
cb(nil)
end
end
-- Stage 2: notmuch (only if resolver has email_pattern + domain)
if resolver_cfg and resolver_cfg.email_pattern and resolver_cfg.domain then
notmuch_lookup(name, resolver_cfg, acct_cfg, function(email)
if email then cb(email) else try_ldap() end
end)
else
try_ldap()
end
return
end
| vim.system(cmd, { text = true, timeout = ldap_cfg.timeout or 10000 }, function(result) | ||
| if result.code == 124 then | ||
| vim.schedule(function() | ||
| vim.notify('⚠ LDAP timeout for: ' .. name, vim.log.levels.WARN) | ||
| end) | ||
| cb(nil); return | ||
| end | ||
| local line = vim.split(result.stdout or '', '\n')[1] or '' |
There was a problem hiding this comment.
The LDAP lookup only checks for timeout (result.code == 124), but does not handle other non-zero exit codes (e.g., command not found, connection failure). If the command fails and prints an error message containing @ to stdout, it could be incorrectly parsed and saved as a valid email address.
We should explicitly check for result.code ~= 0 and return early.
vim.system(cmd, { text = true, timeout = ldap_cfg.timeout or 10000 }, function(result)
if result.code ~= 0 then
if result.code == 124 then
vim.schedule(function()
vim.notify('⚠ LDAP timeout for: ' .. name, vim.log.levels.WARN)
end)
end
cb(nil); return
end
local line = vim.split(result.stdout or '', '\n')[1] or ''
| local function finish() | ||
| done_count = done_count + 1 | ||
| if done_count < total then return end | ||
| for _, t in ipairs(tasks) do | ||
| if resolved_lines[t.idx] then lines[t.idx] = resolved_lines[t.idx] end | ||
| end | ||
| vim.schedule(function() | ||
| vim.api.nvim_buf_set_lines(bufnr, 0, header_end, false, vim.list_slice(lines, 1, header_end)) | ||
| if #unresolved > 0 then | ||
| vim.notify('⚠ No email found for: ' .. table.concat(unresolved, ', '), vim.log.levels.WARN) | ||
| else | ||
| vim.notify('✓ All contacts resolved', vim.log.levels.INFO) | ||
| end | ||
| end) | ||
| end |
There was a problem hiding this comment.
Since contact resolution is asynchronous and can take several seconds (especially with LDAP), the user might edit the buffer headers or body in the meantime. Overwriting the first header_end lines with the stale lines table captured at the start of the resolution will silently discard any edits made by the user.
To prevent this race condition, we should fetch the current buffer lines inside vim.schedule and only update the specific header lines that were successfully resolved.
local function finish()
done_count = done_count + 1
if done_count < total then return end
vim.schedule(function()
if not vim.api.nvim_buf_is_valid(bufnr) then return end
local current_lines = vim.api.nvim_buf_get_lines(bufnr, 0, -1, false)
for _, t in ipairs(tasks) do
if resolved_lines[t.idx] and current_lines[t.idx] then
if contacts.is_header_line(current_lines[t.idx]) then
current_lines[t.idx] = resolved_lines[t.idx]
end
end
end
local current_header_end = #current_lines
for i, l in ipairs(current_lines) do
if l == '' then current_header_end = i - 1; break end
end
vim.api.nvim_buf_set_lines(bufnr, 0, current_header_end, false, vim.list_slice(current_lines, 1, current_header_end))
if #unresolved > 0 then
vim.notify('⚠ No email found for: ' .. table.concat(unresolved, ', '), vim.log.levels.WARN)
else
vim.notify('✓ All contacts resolved', vim.log.levels.INFO)
end
end)
end
| local ok, data = pcall(vim.json.decode, output) | ||
| if not ok or not data then return {} end | ||
| local results = {} | ||
| for _, entry in ipairs(data) do |
There was a problem hiding this comment.
If vim.json.decode decodes a non-table value (e.g., a string, number, or boolean if the output is malformed), ipairs(data) will throw a runtime error.
We should defensively check that data is a table before iterating.
local ok, data = pcall(vim.json.decode, output)
if not ok or type(data) ~= 'table' then return {} end
local results = {}
for _, entry in ipairs(data) do
| --- Resolve all unresolved contacts in the mail headers of buffer `bufnr`. | ||
| function M.resolve_buffer(bufnr) | ||
| local contacts = require('nvim-mail.contacts') | ||
| local account_name = contacts.detect_account() |
There was a problem hiding this comment.
contacts.detect_account() currently always reads from the current buffer (0) and vim.b. However, resolve_buffer(bufnr) can be called with an explicit buffer number. If bufnr is not the current buffer, detect_account will inspect the wrong buffer.
It would be highly beneficial to update detect_account in contacts.lua to accept an optional bufnr parameter, and pass bufnr here.
local account_name = contacts.detect_account(bufnr)
| for _, e in ipairs(entries) do | ||
| e = vim.trim(e) | ||
| if not e:find('@') then names[#names + 1] = e end | ||
| end |
There was a problem hiding this comment.
If an entry in the header is empty or consists only of spaces, vim.trim(e) will result in "". This empty string will be added to names and queried against khard, which is inefficient and unnecessary.
We should check that the trimmed entry is not empty before adding it to the list of names to resolve.
for _, e in ipairs(entries) do
e = vim.trim(e)
if e ~= '' and not e:find('@') then names[#names + 1] = e end
end
| local function split_addresses(addr_str) | ||
| local result = {} | ||
| for part in (addr_str .. ','):gmatch('([^,]+),') do | ||
| local trimmed = part:match('^%s*(.-)%s*$') | ||
| if trimmed ~= '' then | ||
| result[#result + 1] = trimmed | ||
| end | ||
| end | ||
| return result | ||
| end |
There was a problem hiding this comment.
Splitting addresses purely by comma ([^,]+) will incorrectly split names that contain commas, such as "Doe, John <john@example.com>", into two separate invalid entries ("Doe" and "John <john@example.com>").
While this is an existing limitation in other parts of the plugin, since this is a new helper function, it would be highly beneficial to handle quoted names or names with commas correctly (e.g., by ignoring commas inside quotes or angle brackets).
6230bae to
b0b4440
Compare
Addresses gemini-code-assist review feedback on PR #19: - LDAP no longer bypassed when notmuch is unconfigured: extracted try_ldap() closure so Stage 3 runs whether or not Stage 2 was even attempted. Fixes the khard-only-account case where resolver.ldap is set but resolver.email_pattern/domain isn't. - Non-zero LDAP exit codes now short-circuit instead of parsing stdout as a possible email. Only exit 124 emits the timeout notify; other failures return nil silently. - finish() now runs entirely inside vim.schedule and re-fetches the current buffer state before writing, so a user editing headers while resolution runs no longer has their changes wiped. Header lines are only replaced if they still match /^%a+:/ shape.
|
Addressed in follow-up commit
Tests: |
- Add resolve.build_candidates(name, pattern, opts) supporting all five
email patterns: first.last, flast, first_last, firstlast, last.first.
opts.transliterate and opts.normalize_suffixes are opt-in; three-part
names get mid-name variants only for the first.last pattern.
Use vim.fn.tolower() for Unicode-correct downcasing (fixes Å/Ö etc.).
- Rewrite ericsson_candidates as a thin delegate to build_candidates
(backwards compatible: same output, same tests pass).
- Rewrite resolver.lua to be account-aware:
· get_resolver_config() reads resolver block from the detected account
· Stage 2 (notmuch) only fires when resolver.email_pattern + domain set
· Stage 3 (LDAP) only fires when resolver.ldap is configured
· Command, args, account_arg and timeout all come from config
· resolve_buffer() detects account via contacts.detect_account() and
passes it down through resolve_name()
· Legacy migration: if contacts.work_domain is set (non-default), a
compatible resolver is synthesised and a one-time deprecation warning
is emitted; existing setups continue to work unchanged
- contacts.lua: document resolver block shape in accounts config comment;
add deprecation note on work_domain
- tests/mail/resolve_spec.lua: 28 new build_candidates tests covering
all five patterns, transliterate on/off, normalize_suffixes on/off,
multi-part names, four-plus-part names, and Unicode/non-ASCII names
- README: document resolver block in Install example and account-aware
resolver section; update resolution pipeline description
Addresses gemini-code-assist review feedback on PR #19: - LDAP no longer bypassed when notmuch is unconfigured: extracted try_ldap() closure so Stage 3 runs whether or not Stage 2 was even attempted. Fixes the khard-only-account case where resolver.ldap is set but resolver.email_pattern/domain isn't. - Non-zero LDAP exit codes now short-circuit instead of parsing stdout as a possible email. Only exit 124 emits the timeout notify; other failures return nil silently. - finish() now runs entirely inside vim.schedule and re-fetches the current buffer state before writing, so a user editing headers while resolution runs no longer has their changes wiped. Header lines are only replaced if they still match /^%a+:/ shape.
Port the defensive pcall wraps from PR #24 (main-state fixes) onto the new account-aware resolver.lua, since #19 replaces the old file wholesale. Same rationale as the main-state fix: if khard, notmuch, or ldap_owa_query binaries are missing, vim.system throws an unhandled error and strands the resolution chain (counters never reach total, callbacks never fire). Each call is now wrapped in pcall; on failure the appropriate counter is bumped so the chain still completes.
5b1fee3 to
58cdbd1
Compare
Summary
Per-account
,mCresolver pipeline driven by an optionalresolverblock on eachcontacts.accountsentry. Closes #14.Changes
resolve.lua: newbuild_candidates(name, pattern, opts)supporting all five patterns (first.last,flast,first_last,firstlast,last.first);opts.transliterateandopts.normalize_suffixesare opt-in;ericsson_candidatesdelegates to it (backwards compatible). Usesvim.fn.tolower()for Unicode-correct downcasing.resolver.lua: fully account-aware pipeline.resolve_bufferdetects account viacontacts.detect_account(). Stage 2 (notmuch) fires only whenresolver.email_pattern+resolver.domainare set. Stage 3 (LDAP) fires only whenresolver.ldapis configured — command, args, account_arg, timeout all from config.contacts.lua:resolverblock shape documented in config comments;work_domainmarked deprecated.tests/mail/resolve_spec.lua: 28 newbuild_candidatestests (all 5 patterns, transliterate on/off, normalize_suffixes on/off, multi-part names, 4+-part names, Unicode names).README.md:resolverblock documented in Install example and account-aware resolver section.Migration
Existing setups using
contacts.work_domaincontinue to work unchanged — a legacy resolver is synthesised automatically and a one-time deprecation warning is shown. To migrate, movework_domaininto aresolverblock on the relevant account entry.Tests
Failed: 0 across all 10 spec files (~90 tests total).