Skip to content

fix(lockfile): treat a lock file that cannot be read as existing, not missing - #557

Merged
ThomasK33 merged 10 commits into
mainfrom
fix/lockfile-remove-unreadable
Nov 12, 2026
Merged

ThomasK33 merged 10 commits into
mainfrom
fix/lockfile-remove-unreadable

Conversation

@ThomasK33

Copy link
Copy Markdown
Member

Why

The goal: the lock file code must tell a lock file that exists from one that is missing, whether or not the plugin can read it, and lockfile.remove must never delete anything that is not a lock file. lockfile.remove, lockfile.get_auth_token and :checkhealth claudecode checked for the file with vim.fn.filereadable. For a lock file with mode 000, filereadable is 0, so all three said that the lock file did not exist.

  1. remove skipped a delete that the system can do. Deleting a file needs write permission on its folder, not read permission on the file. stop only logs the error as a warning, so the lock file stayed and the warning was false. The docs: say stop only warns when it cannot delete the lock file #537 advisor's probe found this (case 4: lock file mode 000, file stays, warning "Lock file does not exist").
  2. get_auth_token must still refuse a file that it cannot read, but its error said that the file did not exist.
  3. :checkhealth claudecode reported "Lock file missing" for such a file, so a user who ran it was sent to restart the integration instead of fixing a permission.

Making remove delete such a file also needed a safe delete. os.remove is the C remove, which deletes an empty folder. Another process of the same user can replace the lock file with an empty folder between the check and the delete. filereadable did not stop that either. It checks for a folder and for read permission at two moments, so a folder that appears between them passes as readable, and os.remove deletes it. The round b verifier ran a second process that swaps the file and a folder in a loop: main deleted 73,836 folders in 20 s with a mode-000 file (Neovim 0.12.3). So the delete itself must refuse a folder.

What changed

  1. lockfile.remove looks for the name with vim.loop.fs_lstat (it works on Neovim 0.8) and deletes it with vim.loop.fs_unlink instead of os.remove. fs_unlink refuses a folder (EISDIR), so the delete never removes a folder, also when another process puts one there after the check. fs_lstat does not follow a link, so a link at the lock path, even one that points nowhere, is a name that remove deletes. It deletes the link and never its target. "Lock file does not exist" is returned only when fs_lstat finds nothing or a folder. The error returns keep their shape: a raise or a nil result from the delete gives false, "Failed to remove lock file: <message>". The message is now libuv's ("EACCES: permission denied: ") and not the C library's.

  2. lockfile.get_auth_token tells the cases apart with vim.loop.fs_stat. A missing path or a folder gives "Lock file does not exist: ", as before. A file that is there and that io.open cannot open gives "Lock file exists but cannot be read: : Permission denied" (the system message). It still returns false, nil, error. The old "Failed to open lock file" error appeared when io.open failed after filereadable accepted the path, including a bound Unix socket without a race. The new error replaces it.

  3. lua/claudecode/health.lua keeps filereadable for "Lock file present". When it is 0, vim.loop.fs_stat tells the cases apart: a file that is there gives the error "Lock file exists but cannot be read: " with advice to make it readable, and nothing or a folder gives "Lock file missing" as before.

  4. Tests in tests/lockfile_test.lua, with real files in the suite's private temp home. For remove:

    • an unreadable lock file is deleted and remove returns success,
    • a missing file gives the "does not exist" error, and a folder named like a lock file stays,
    • a read-only folder gives the fs_unlink error and the file stays,
    • a folder that takes the place of the file after the existence check stays, and remove returns false (for a file with mode 000 and one with mode 600),
    • a link that points nowhere, a link to the lock file of another port, a hard link and a link to a folder lose only their name, and the target stays,
    • a FIFO and a socket at the lock path are deleted, as before, and the tests assert that remove returns success and that the name is gone,
    • the failure injection mocks fs_unlink, the call that is used: a nil result with an error, and a raise, both give false with "Failed to remove lock file".

    For get_auth_token: a missing file gives the exact "does not exist" message, an unreadable file is refused with the "cannot be read" message (no token, no "does not exist"), a FIFO with mode 000 and a socket give the same message, and a folder and a link that points nowhere give the "does not exist" message. In tests/unit/health_spec.lua: a lock file that is there but unreadable gives "exists but cannot be read", and a folder at the lock path gives "Lock file missing". The mode 000 tests, the FIFO test and the read-only test are pending when the user is root, who can read and write anyway. The FIFO and socket tests are pending where mkfifo or python3 is missing. The tests assert on messages and booleans only, never on a token or on the content of a lock file.

  5. Test mocks changed. The filereadable mock says what Neovim says (:h filereadable(): 1 only for a file that exists, can be read and is not a folder) with the shell's test -r and test ! -d, so it never opens the file (opening a FIFO waits for a writer). The vim.loop mock gets an fs_stat and an fs_lstat that need no read permission on the file and know a file, a folder, a FIFO, a socket and a link. Its fs_unlink calls unlink(2) through ffi, so it refuses a folder and fails like libuv, where it used to call os.remove and always return true.

  6. Docs. ARCHITECTURE.md said that remove "returns an error when no readable lock file exists" (added by docs: rewrite the lock file and diff sketches of ARCHITECTURE.md to match the code #543). That is false now. It says that remove deletes the file with vim.loop.fs_unlink, which refuses a folder, returns an error when fs_lstat finds nothing or a folder and when fs_unlink fails, still deletes a mode 000 lock file, and removes a link as a link. Its line on blocking I/O names fs_unlink, fs_stat, fs_lstat and the io.open of get_auth_token, and no longer os.remove. lua/claudecode/README.md says how lockfile.remove finds and deletes the name, has a line for the two errors of lockfile.get_auth_token, and says that a lock file in a lock folder that cannot be searched gives "does not exist", because fs_lstat and fs_stat find nothing. It also names the three states of the health check (present, missing, there but not readable). README.md says that :checkhealth claudecode checks that the lock file exists and can be read. The lines that say that removal can fail stay true, so I did not touch them (README.md around line 1126 and 1154).

  7. covered.lock is re-recorded for the new hashes of lockfile.lua and health.lua. No block and no call site changed.

Scope

Only lockfile.remove, lockfile.get_auth_token and the lock file line of :checkhealth claudecode, their tests and the doc lines. get_auth_token has no caller in lua/. It is part of the module's API, and the tests are its only users here.

Not changed, with the reason for each:

  • get_auth_token still opens a readable FIFO at the lock path with io.open, which waits for a writer. Main does the same, and no user flow puts a FIFO there. This head also reaches it when another process swaps a file with mode 000 for a FIFO after the fs_stat call. get_auth_token has no caller in lua/.
  • remove deletes the name <port>.lock in the lock folder as it resolves at that moment. If another process replaces the lock folder itself with a link to another folder between fs_lstat and fs_unlink, remove deletes a <port>.lock file there (never a folder). Main does the same for a readable file. Only the Neovim that holds a port owns a live lock file for it, and stop removes the lock file before the server stops, so no live lock file is lost. The other process needs write access to the config folder. A fix needs unlinkat with an open folder handle.
  • :checkhealth claudecode says "Lock file present" for a Unix socket at the lock path, because filereadable is 1 for it. Main says the same, and no user flow puts a socket there.

Blast Radius

lockfile.remove runs from stop and from the token-mismatch path of start in init.lua. It differs from main in these ways:

  • A lock file that exists but cannot be read (mode 000) is now deleted. Before, it stayed and stop logged a wrong warning.
  • Any other thing that is not a folder is now handed to the delete. filereadable also refused a file that it could not read, and nothing else.
  • A link at the lock path is removed as a link and never its target. A link to an existing readable file was removed this way before. Now a link to a file that cannot be read, a link that points nowhere and a link to a folder are removed too, and their targets stay.
  • The delete is fs_unlink, which refuses a folder. On main, a folder that another process puts at the path after a readable file was checked is deleted when it is empty (os.remove). Here it stays, and remove returns false.
  • The text of a delete failure is libuv's ("EACCES: permission denied: ") and not the C library's (": Permission denied"). The prefix "Failed to remove lock file" and the false, error shape are the same.

lockfile.get_auth_token differs in two ways. First, unreadable regular files now give the "cannot be read" error instead of "does not exist", and paths that filereadable accepted but io.open rejected, such as bound Unix sockets, now give that error instead of "Failed to open lock file". The failure tuple remains false, nil, error, so no caller that checks the first value changes. Second, a file that becomes readable between the fs_stat call and io.open (another process runs chmod, or renames a readable file over the path) now gives its token, where main said that the file did not exist. get_auth_token reads what is at the path when it opens it, as main does for a readable file, and no code in lua/ calls it.

:checkhealth claudecode differs in one way: a lock file that exists but cannot be read gives the error "Lock file exists but cannot be read: " and not "Lock file missing". A readable lock file still gives "Lock file present", and nothing or a folder at the path still gives "Lock file missing".

A lock file that exists in a lock folder that cannot be searched (mode 000 on the folder) is the one case that all three still report as missing: the stat call finds nothing there, as filereadable did. The README says so.

Unchanged: a missing path gives the same error in both, a folder gives the same error and stays, a read-only folder gives an error from the delete and the file stays, a raise or a nil result from the delete gives the same shape of error, and a readable lock file gives the same token.

The diff-lifecycle check does not apply: lockfile.lua and health.lua cannot reach diff state, an openDiff answer, a disconnect or a diff command. check.sh still passes (main: 89,205 distinct states, live-DisconnectedCallsAnswered: 5,269), and covered.lock is re-recorded.

Security

The lock file holds the auth token. This change does not change who can read it, where it lives, or what the plugin reads:

  • Who can read it: unchanged. The plugin writes it with mode 0600, and this change adds no read. The stat calls read no content. get_auth_token still refuses a file that it cannot read, and it reads only a file that io.open opens, as before.
  • Where: unchanged. The path is still lock_dir, a slash, the port number and .lock. A link at that path is removed as a link and its target stays: a link to an existing readable non-directory was removed this way before, and now a link to an unreadable target, a dangling link and a link to a folder are removed too, with their targets left alone (a test for each).
  • How long: shorter in one case. A lock file that the plugin cannot read, with a token for a server that has stopped, is deleted now. Before, it stayed on disk.
  • Deleting only a name: remove can no longer delete a folder, including one that another process of the same user puts at the path after the check, because fs_unlink refuses a folder. That process needs write access to the private lock folder already, and this is not an escalation. It closes a way to delete an empty folder that is not a lock file.
  • What an error shows: the new get_auth_token error holds the path and the system message from io.open ("Permission denied"). The new health error holds the path. Neither holds a token or any content.
  • The tests and the probes print booleans and messages only. They never print a token or any content of a lock file.

Verification

Red on main, green here. The same test files run with earlier code and with the fix:

Step Result
busted tests/lockfile_test.lua, remove tests only, main's lockfile.lua 19 successes / 1 failure: remove returned false: Lock file does not exist: …/12353.lock (the unreadable-file test)
get_auth_token tests added, the first remove fix only 22 successes / 1 failure: Lock file does not exist: …/12361.lock (the unreadable-file test)
busted tests/unit/health_spec.lua, main's health.lua 9 successes / 1 failure: the test that an unreadable lock file is not reported as missing
the final tests/lockfile_test.lua against the first version of the fix (os.remove, fs_stat) 27 successes / 5 failures: the folder that takes the place of the file, the link that points nowhere, the link to a folder, and the two failure injections of fs_unlink
the final tests/lockfile_test.lua against get_auth_token with fs_lstat instead of fs_stat 32 successes / 1 failure: the link that points nowhere
this head, busted tests/lockfile_test.lua 33 successes / 0 failures / 0 errors / 0 pending
this head, busted tests/unit/health_spec.lua 10 successes / 0 failures / 0 errors / 0 pending

The other new tests pass before and after, as controls (a missing file, a folder, a read-only folder, a hard link, a link to the lock file of another port, and the FIFO and socket tests that pin what filereadable let through).

The folder race on real files (a real Neovim, not the mock). Another process runs rm -f and mkdir on the lock path right after the first fs_stat, fs_lstat or filereadable call that names it. remove must return false and the folder must stay. Both a file with mode 000 and one with mode 600 are tried:

Code Neovim 0.12.3 Neovim 0.8.0
main (filereadable, os.remove) mode 000: folder stays. mode 600: remove returns true and deletes the folder same
the first version of this PR (fs_stat, os.remove) remove returns true and deletes the folder, for both modes same
this head (fs_lstat, fs_unlink) remove returns false and the folder stays, for both modes same

The hook acts right after the check. The main row for mode 000 holds for that order only. Against a second process that swaps the file and a folder in a loop, main also deletes folders for mode 000 (73,836 in 20 s, round b verifier). This head deleted none in four 20 s runs (Neovim 0.12.3 and 0.8.0, mode 600 and 000).

vim.loop.fs_unlink gives the same results on Neovim 0.8.0 and 0.12.3: it deletes a file with mode 000, a link to a file, a dangling link, a link to a folder (the folder and its content stay), a FIFO and one name of a hard link (the other name stays), and it refuses an empty folder with EISDIR, a missing path with ENOENT, and a read-only folder with EACCES.

Mutants of remove and get_auth_token against the final test file (all killed, the control passes 33 of 33): remove back to filereadable, fs_stat instead of fs_lstat, os.remove instead of fs_unlink, no folder check, no existence check, every entry that is not a regular file treated as missing, an error for every call, a nil result counted as success, and no pcall. get_auth_token back to filereadable, fs_lstat instead of fs_stat, the old "Failed to open" message, no folder check, the "does not exist" message for an unreadable file, no system message, a message without the reason, and every entry that is not a regular file treated as missing.

Real Neovim 0.12.3, real files in a probe folder (the same scripts run against main's code and the fix, and print booleans and messages only):

Case main here
remove, lock file mode 000 false, "Lock file does not exist", file stays true, file deleted
remove, missing file false, "Lock file does not exist" same
remove, folder <port>.lock false, "Lock file does not exist", folder stays same
remove, read-only lock folder false, "Failed to remove lock file: …: Permission denied", file stays false, "Failed to remove lock file: EACCES: permission denied: …", file stays
get_auth_token, lock file mode 000 false, nil, "Lock file does not exist" false, nil, "Lock file exists but cannot be read: …: Permission denied"
get_auth_token, missing file false, nil, "Lock file does not exist" same
get_auth_token, folder false, nil, "Lock file does not exist" same
get_auth_token, readable file true, a token same
:checkhealth claudecode, readable lock file "Lock file present" same
:checkhealth claudecode, lock file mode 000 "Lock file missing" "Lock file exists but cannot be read"
:checkhealth claudecode, lock file missing "Lock file missing" same
:checkhealth claudecode, folder at the lock path "Lock file missing" same

The :checkhealth rows ran with a real server and a real lock file, with XDG_* and CLAUDE_CONFIG_DIR in a probe folder.

Gates on this head (e6e3a170, with main at f7f89cb5 merged):

  • mise exec -- busted -v tests/lockfile_test.lua: 33 successes / 0 failures / 0 errors / 0 pending. busted -v tests/unit/health_spec.lua: 10 successes / 0 failures / 0 errors / 0 pending.
  • mise run format-check: formatted 426 files (0 changed).
  • .agents/skills/verify-diff-lifecycle/check.sh (full): RESULT: PASS (main: 89205 states, live-DisconnectedCallsAnswered: 5269 states). check.sh --hash-only: covered.lock matches the code.
  • mise run all: Total: 0 warnings / 0 errors in 185 files, 1756 successes / 0 failures / 0 errors / 28 pending.
  • scripts/run_integration_tests_individually.sh: Total Success: 527, Total Failed: 0, Total Errors: 0.
  • git merge-tree --write-tree HEAD origin/main (main at 523ee200): clean. check.sh --hash-only on that merge: covered.lock matches the code. The two commits that main has since f7f89cb5 touch none of the 8 files of this PR.

Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh

`lockfile.remove` used `vim.fn.filereadable` to decide whether the lock
file exists. For a lock file with mode 000 it answered "Lock file does not
exist" and skipped a delete that `os.remove` can do, because deleting needs
write permission on the folder, not read permission on the file. The lock
file stayed.

It now checks with `vim.loop.fs_stat` and accepts a regular file only, so a
folder with that name still stays. `os.remove` reports its own error, as for
a read-only folder.

The test mock of `filereadable` now answers 0 for a folder, as Vim does, and
the mock of `vim.loop` gets an `fs_stat` that does not need read permission.
…able did

filereadable is 0 for a missing file, an unreadable file and a folder. The
first commit treated a folder like a regular file's absence only by type, which
left a FIFO or a socket named <port>.lock out. Exclude exactly what filereadable
excluded besides the read permission: a missing path and a folder.
`lockfile.get_auth_token` used `vim.fn.filereadable` to decide whether the
lock file exists, so a lock file with mode 000 gave "Lock file does not
exist". It must still refuse that file, but the error was false.

It now tells the two apart with `vim.loop.fs_stat`. A missing path or a folder
gives "Lock file does not exist". A file that is there and that `io.open`
cannot open gives "Lock file exists but cannot be read" with the system
message. The tests assert on the messages and on booleans only, never on the
token or the content.
…ad, and test a FIFO and a socket

ARCHITECTURE.md said that remove returns an error when no readable lock file
exists. After the fs_stat check, that is false: it deletes a mode 000 file. The
README lines for remove and get_auth_token also say what fs_stat finds, and
that a lock folder that cannot be searched gives the does not exist error.

The fs_stat mock now knows a FIFO and a socket. Tests: a FIFO and a socket at the
lock path are deleted by remove, and get_auth_token says that a FIFO with mode
000 and a socket exist but cannot be read. A mutant that treats every entry that
is not a regular file as missing now fails them. The comment on the filereadable
mock quotes :h filereadable() instead of guessing its reason.
:checkhealth claudecode used filereadable, which is 0 for a lock file with mode
000, so it reported Lock file missing. It now tells the cases apart with
vim.loop.fs_stat, as lockfile.remove and lockfile.get_auth_token do. A folder
at the lock path is still reported as missing. covered.lock is re-recorded for
the new hash of health.lua.
…older

Another process of the same user can replace a lock file with an empty folder
between the existence check and the delete. os.remove (C remove) deletes an
empty folder, so remove could delete one that is not a lock file, now also for a
file that cannot be read. fs_unlink refuses a folder (EISDIR), so the delete
itself never removes one. The error returns are the same shape: a raise or a
nil result is false with Failed to remove lock file and the message.

The check uses fs_lstat, so a link at the lock path, even one that points
nowhere, is a name that remove deletes, as a link and never its target. Tests:
a folder that takes the place of the file after the check stays, a dangling
link, a link to the lock file of another port, a hard link and a link to a folder
lose only their name, and the failure injection mocks fs_unlink, the call that
is used. The fs_unlink mock calls unlink(2), and the filereadable mock no longer
opens the file, so a FIFO cannot hang a test. A FIFO and a socket test now also
assert that the name is gone. ARCHITECTURE.md and the README name fs_unlink.
…, and fix three stale lines

get_auth_token uses fs_stat, so a link that points nowhere is no lock file
and gets the "does not exist" message. A mutant that used fs_lstat there
passed every test. A new test now pins the message, and that mutant fails it.

Three lines were stale after the fs_unlink change. A test title said
"os.remove" and now says "fs_unlink". A comment in get_auth_token said that
a failure of io.open always means a refusal to read, but another process can
delete the file after the stat call. The I/O line of ARCHITECTURE.md did not
name the io.open of get_auth_token.

covered.lock is recorded again for the new hash of lockfile.lua.
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-12T11:45:21.127934Z e6e3a17 Manual request
🔒 Security Review ✅ Completed 2026-11-12T11:46:26.158668Z e6e3a17 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: e6e3a17073

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: e6e3a17073

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@ThomasK33
ThomasK33 added this pull request to the merge queue Nov 12, 2026
Merged via the queue into main with commit 05b9785 Nov 12, 2026
6 checks passed
@ThomasK33
ThomasK33 deleted the fix/lockfile-remove-unreadable branch November 12, 2026 12:34
hareki pushed a commit to hareki/claudecode.nvim that referenced this pull request Nov 13, 2026
… other path characters (coder#561)

## Why

`cwd.git_root` must find the repository of a folder whatever its name
is. A repository in a folder with a space or a comma in its path was not
found when git could not run: the fallback gave nil, or the repository
of Neovim's current folder. A folder named `[x] {y}` or `$HOME-like` was
lost or changed even when git answered. A verifier of F12 found the
first case (finding F12-P1, a space or comma in the path makes the
`.git` fallback return nil).

## What changed

Four problems in `lua/claudecode/cwd.lua`. The first two took a path as
a pattern. The third stepped one folder down at a trailing `..`. The
fourth is a flaw in my first version of the second step.

1. **The `.git` fallback** called `vim.fn.finddir(".git", start_dir ..
";")`. `finddir` reads its path as a list in `'path'` option syntax. A
space or a comma split the path, and a `;` ended it. The fallback now
walks up from the full path of `start_dir` and tests each folder for a
`.git` folder with `vim.fn.isdirectory`. A relative `start_dir` now
reaches the folders above the current one too. Before, a relative start
gave a wrong answer or nil in some cases (for example
`git_root("sub/deeper")` in the repository root).
2. **`normalize_dir`** passed every path through `vim.fn.expand`.
`expand` reads `[`, `{`, `*`, `?`, `$` and `\` in a name as patterns or
variables. A folder named `[x] {y}` or `it's` came back with a trailing
slash. A folder named `$HOME-like` or `back\slash` came back nil. This
also happened when `git rev-parse` answered. `normalize_dir` now returns
a directory that exists as written, and expands any other string (`~`,
`$VAR`). See step 4 for text that starts with `~` or `$`.

3. **`..` before Neovim 0.11.** Before 0.11, `fnamemodify(path, ":p")`
keeps a last `..`, and `:h` of `a/b/..` is `a/b`. So a walk from a start
folder that ends in `..` stepped one folder down and returned the wrong
root. The walk now calls `simplify()` on the full path first. A verifier
found this.

4. **Text that starts with `~` or `$`.** The first version of step 2
took a path that names an existing folder as written, on the raw text.
`vim.fn.isdirectory` does not expand, so with `cwd = "~"` or `"$HOME"`
and a stray folder of that literal name in Neovim's current folder (for
example one that an unexpanded `mkdir "~/x"` made), `git_root` and
`resolve` returned the stray folder where main returned the home folder.
`normalize_dir` now sends text that starts with `~` or `$` through
`expand` first, as a shell and `:cd` do. A verifier of the terminal
change found this.

`lua/claudecode/README.md` gets one new bullet that states this, with
the rule for text that starts with `~` or `$`. It sits after the
"Terminal cwd precedence" bullet. It names `git_root` and `resolve`
only, and it says that the terminal still passes a static `cwd` and a
`cwd_provider` result through `vim.fn.expand` itself.

## Scope

- Only `cwd.lua` changes in code. Steps 1 and 2 are one change of idea:
a path is a literal, not a pattern. Step 3 is a quirk of `fnamemodify`
before Neovim 0.11. Step 4 closes a hole that step 2 opened.
- `normalize_dir` serves `cwd.git_root` and `cwd.resolve`. Nothing in
`lua/` calls `cwd.resolve`. The terminal calls `cwd.git_root` when
`git_repo_cwd` is set, so this PR fixes folder names with `$` and `\` on
that path: when git does not answer, the terminal now gets `$HOME-like`,
`back\slash`, `[x] {y}`, a space and a comma right, where main gave nil
or the repository of Neovim's current folder. A static `cwd` and a
`cwd_provider` result do not go through `normalize_dir`:
`terminal.lua:build_config` passes them through `vim.fn.expand` itself,
so this PR does not change what the terminal does with those. An
absolute start path that ends in `..` is fixed here, through the
`simplify()` call above.
- Two gaps stay for separate follow-ups. Main already has both, and this
PR does not reach them. First, the fallback finds a `.git` folder only,
not a `.git` file in a worktree or submodule. Second, the walk is
lexical, so a link into another repository answers with the repository
of the link's own folder, where git answers with the link target's
repository. The terminal's `expand` of a static `cwd` and of a provider
result is a third follow-up, and it has its own change.
- `~` and `$VAR` still expand in `cwd.resolve` (the spec checks both).
The spec's cases for `cwd.resolve` are named as `cwd.resolve` cases,
because they do not test the terminal.
- `git rev-parse` still runs first. Its result for a plain path is the
same string as before.

## Blast Radius

- User-visible: a repository in a folder with a space, a comma, a quote
or another path character is now found when git cannot run. With git,
`git_root` returns a folder with `[`, `{`, `*`, `?`, `$` or `\` in its
name exactly. It does not reach the terminal through a `cwd_provider`:
`build_config` still expands the provider's result, so the root README's
example still loses `$` and `\` there (follow-up). A plain path gives
the same result as before.
- The fallback result is an absolute path with no `..` part. Before it
could be relative. The one exception is a relative start whose name
begins with `~`: Vim reads `~` as the home folder and `~name` as a
user's home folder, so `:p` leaves `~name` relative and the result can
be the relative `~name`. That is a pathological case, and no caller in
`lua/` passes a relative path (the terminal passes the file's folder
from `expand("%:p")` or `getcwd()`).
- A string that names an existing folder is taken as written, except
text that starts with `~` or `$`. That text always goes through `expand`
first, so a folder named `~` or `$HOME` in Neovim's current folder does
not take the place of the home folder, as on main. The cost: a relative
folder name that starts with `$` and reads as a variable (`$HOME-like`,
`$odd/sub`) is expanded and not found, as on main. A lone relative
`$odd` that names no variable is left unchanged by Vim and is still
found. An absolute path never starts with `~` or `$`, so every folder
name this PR fixes keeps working. The change applies to `git_root`'s
`start_dir` and to `cwd.resolve`. The terminal's own `expand` of a
static `cwd` and a provider result is a separate change.
- Neovim 0.8: the code uses `vim.fn.isdirectory`, `vim.fn.fnamemodify`
and `vim.fn.simplify`, which exist there. I ran the new spec on Neovim
0.8.0, 0.10.0 and 0.12.3. The `..` case fails on 0.8.0 and 0.10.0
without the `simplify()` call, and passes on 0.12.3 either way.
- `covered.lock`: `cwd.lua` is the terminal's working-directory lookup.
`check.sh` printed STALE for the file hash of `cwd.lua` only, with no
changed block or call site. The change cannot reach diff state, an
openDiff answer, a disconnect or a diff command. The model passed
(`main`: 89205 distinct states, `live-DisconnectedCallsAnswered`: 5269).
`check.sh --record` wrote the new lock and `check.sh --hash-only`
reports that it matches.

### Merge with coder#539 (F12)

coder#539 landed first. This PR's merge commit with `origin/main` resolves
the interplay: it re-records `covered.lock` and changes the one stale
test in coder#539's `cwd_spec.lua` (see Verification). F11 (coder#535) merges with
this PR without a conflict.

## Verification

Red before the fix, green after (same spec, only
`lua/claudecode/cwd.lua` swapped):

| Spec | origin/main | with the fix |
| --- | --- | --- |
| `tests/integration/cwd_path_characters_spec.lua` (37 cases, real
Neovim, 0.8.0 and 0.12.3) | 20 successes, 17 failures | 37 successes, 0
failures |
| the new case "goes up from a start folder that ends in .." at
`ede2f733` (before the `simplify()` call) | fails on 0.8.0 and 0.10.0,
passes on 0.12.3 | passes on 0.8.0, 0.10.0 and 0.12.3 (35 successes when
added, 37 at the final head, 0 failures on each) |
| the two cases "expands a leading ~ / $ even when Neovim's cwd holds a
folder named ~ / $HOME", with `cwd.lua` from `ca9e61c0` (before the
guard) | 35 successes, 2 failures on 0.8.0 and 0.12.3 | 37 successes, 0
failures on each |

The spec makes real folders and runs `cwd.git_root` on them, in two
modes. In the first mode git is off `PATH` (the shell keeps its absolute
path), so `git` ends with "not found" and a list command raises E475, as
on a host without git. In the second mode a real `git init` makes the
repository. The folder names are: a space, a comma, a semicolon, single
and double quotes, brackets and braces, parentheses, non-ASCII letters,
a star and a question mark, a percent sign and a hash, a backslash, a
dollar sign, a leading tilde, and a plain name. Each name runs from the
repository folder and from a folder two levels below it, and the answer
must equal the repository path with no trailing slash. A folder that is
not a repository returns nil, whatever its name. Seven more cases cover
a relative start folder, a start folder that ends in `..`, `cwd.resolve`
with `git_repo_cwd`, a static `cwd` with brackets and a star in
`cwd.resolve`, `~` and `$VAR` in a setting of `cwd.resolve`, and two
cases where Neovim's current folder holds a folder named `~` or `$HOME`
and the setting must still expand. The 17 red cases are 10 of the no-git
names, 4 of the git names, the relative start, `resolve`, and the static
`cwd`.

Gates on the final head `f12368bd`, which contains a merge of
`origin/main` (05b9785, with coder#551, coder#535, coder#494, coder#545, coder#553, coder#555 and
coder#557) (private XDG folders). An independent verifier passed `c73ab044`.
Since then the branch has three changes of its own and two merges with
main. The changes are a docs commit (the README sentence about where the
walk starts), the fix for text that starts with `~` or `$` (step 4, with
two spec cases and the README clause), and the `covered.lock` record for
it. For the lock I ran the full `check.sh`: the model passed (89205
distinct states, 5269 live states, every fact TRUE), and only the file
hash of `cwd.lua` was stale, with no changed block or call site.

- `mise run all`: native exit 0. luacheck: 0 warnings / 0 errors in 186
files. busted: 1756 successes / 0 failures / 0 errors / 29 pending.
treefmt: 0 files changed. Since coder#545, `mise run all` also runs the
static proofs of the `verify-claudecode-nvim` skill, and they pass.
- `mise run format-check`: native exit 0 (428 files, 0 changed).
- `scripts/run_integration_tests_individually.sh` on Neovim 0.12.3: 564
successes, 0 failed, 0 errors, native exit 0. On Neovim 0.8.0: 427
successes, 0 failed, 0 errors, native exit 0 (the 0.8.0 run skips the
specs that need 0.9). The new spec gives 37 successes on both. It starts
about 100 processes (git and a shell), so it takes about 6 s on a quiet
host and can pass the runner's 40 s limit at a load average above 100. A
TIMEOUT line there means the host was slow, not that the spec hung.
- All four `.agents/skills/verify-*/check.sh --hash-only` match their
`covered.lock`.

The merge with `origin/main` had two parts to resolve in an earlier
commit. `covered.lock` conflicted on the hash line of `cwd.lua`, so I
ran the full `check.sh` and `check.sh --record`. coder#539's
`tests/unit/cwd_spec.lua` had a test that stubbed `vim.fn.finddir`,
which the fallback no longer calls, so that test now walks up real
folders. It does not depend on where `TMPDIR` is: it passes with
`TMPDIR` inside a git checkout too. `cwd.lua` itself merged without a
text conflict. The two latest merges needed no `covered.lock` change:
coder#551 conflicted only in `tests/mocks/vim.lua`, where the branch keeps
both its `simplify` entry and coder#551's `wordcount`, and coder#535 and coder#494
merged without a conflict.

An independent verifier found the `..` case, the README wording and the
other points that this body now states. The `terminal.lua` expand and
`.git` files in worktrees are gaps that main already has. They are
follow-ups, not part of this PR.

---
_Generated with [`xum`](https://github.com/coder/xum) • Model:
`anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
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.

1 participant