Repository navigation
docs: say stop only warns when it cannot delete the lock file - #537
Merged
Merged
Conversation
lockfile.remove can fail (a read-only lock directory, or a file that is already gone). stop and the start token mismatch only log a warning then, so the lock file stays (init.lua:642-647, :605-608). Three sentences in scripts/README.md said the plugin removes the lock without that condition. The lock directory sentence now says when the default applies: when CLAUDE_CONFIG_DIR is unset or empty, as README.md already does.
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
The Resource Cleanup comment said M.stop() removes the lock file. It tries to, and only logs a warning when it cannot (init.lua:642-647). The Lock File System section reads as how the plugin works today, but lockfile.lua has no VimLeavePre autocmd and removes the file with os.remove through lockfile.remove. One sentence after the sketch says where the real cleanup runs and that stop only warns when the removal fails.
ThomasK33
changed the title
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06ae6bcab1
ℹ️ 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".
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Codex P3: lockfile.remove also returns an error when the lock file is already gone (lockfile.lua:215-216), and stop logs the same warning then. A failed removal therefore does not always leave a file. Three sentences said that the file stays. They now say that a lock file that the removal failed to delete stays.
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
ThomasK33
changed the title
hareki
pushed a commit
to hareki/claudecode.nvim
that referenced
this pull request
Nov 13, 2026
…atch the code (coder#543) ## Why Two sections of `ARCHITECTURE.md` show code sketches that do not match the code. Both read as how the plugin works today. They sit under headings that name the module, and nothing calls them examples, so a contributor reads them as fact. - The Lock File System sketch names a `write_file` helper and a `vim.loop.fs_rename` call that `lockfile.lua` does not have. It puts a `VimLeavePre` autocmd and an unconditional `fs_unlink` in `lockfile.lua`, which has no autocmd. coder#537 added one sentence after it and left the rewrite for this PR. - The Diff System sketch writes a temp file with `write_file` and sets the keymaps `<leader>da` and `<leader>dq`. The `openDiff` tool writes no temp file, and the plugin sets no diff keymaps. ## What changed Two sections of `ARCHITECTURE.md`. Each is now short prose with citations, plus one shortened excerpt whose code lines are verbatim from the source file (a script checked each line). **Lock File System** (`lockfile.lua`), six bullets: - Where the file lives: `<port>.lock` in `$CLAUDE_CONFIG_DIR/ide`, or in `~/.claude/ide` when that variable is unset or empty (`lockfile.lua:11-18`). - What it holds: `pid`, `workspaceFolders`, `ideName`, `transport` and `authToken` (`lockfile.lua:129-135`). - Directory mode: a new lock directory gets `0700`, and `create` tries to tighten an existing one (`lockfile.lua:94`, `:104`). - The atomic write: a temp file in the same directory, opened with `wx` and mode `0600`, renamed with `os.rename`. A failed write or rename makes `create` try to remove the temp file, and it ignores a failed removal (`lockfile.lua:154-199`). - Removal: `remove` uses `os.remove` and returns an error when no readable lock file exists and when `os.remove` fails (`lockfile.lua:208-232`). - The lifecycle, which `init.lua` owns: `start` calls `create` after the server binds, `stop` calls `remove` and only logs a warning when it fails, and the `VimLeavePre` autocmd that `setup` creates calls `stop` when the server runs (`init.lua:514-522`, `:586`, `:635-648`). **Diff System** (`diff.lua`, `diff_inline.lua`), a paragraph and six bullets: - `openDiff` calls `open_diff_blocking`, which runs in the tool's coroutine and yields until the diff is resolved. A `ClaudeCodeDiffOpened` handler can resolve the diff during setup, and `open_diff_blocking` then returns the result at once without yielding. When the setup fails (for example, because the file has unsaved changes), it raises an error and does not yield (`tools/open_diff.lua:67`, `diff.lua:1893-1987`). - The layout: `vertical` (the default) and `horizontal` use Neovim's diff mode, with the proposal in a split to the right or below. For a new file, the old side is an empty buffer. `unified` is drawn inline by `diff_inline.lua` and is not a Neovim diff (`diff.lua:279-288`, `:1595`). - The proposal is an unlisted `acwrite` scratch buffer and not a temp file. Its name is `<tab_name> (proposed)` for a native diff of an existing file, `<tab_name> (NEW FILE - proposed)` for a new file, and `<tab_name> (unified diff)` for the unified layout (`diff.lua:1730-1748`, `diff_inline.lua:292-299`). - Tabs: a native diff prefers the current tab and goes to an editor window in another tab only when the current tab has none. It opens in a new tab when `diff_opts.open_in_new_tab` is set or the tab it picks, or the current tab, already has a diff window (`diff.lua:125-136`, `:1625`, `:1700`). - Accept and reject: in the proposal buffer, `:w` (also `:wq`, `:x` and `ZZ`) and `:ClaudeCodeDiffAccept` accept. `:ClaudeCodeDiffDeny`, closing the last window of the proposal, and deleting its buffer reject. The two commands read the diff from the current buffer and only warn in another buffer. The plugin sets no keymap. The README's example config maps `<leader>aa` and `<leader>ad` to the two commands (`README.md:66-67`). - A diff resolves once. Accept replies `FILE_SAVED` with the content, reject replies `DIFF_REJECTED`, and the plugin does not write the target file. Normally Claude writes it after `FILE_SAVED`. When that Claude session ends or drops the answer first, `:ClaudeCodeCloseAllDiffs` puts the accepted text into the file's buffer as unsaved changes. A diff whose buffer cannot take the text (for example, because it has unsaved changes) stays open (`diff.lua:1071`, `:1154`, `:2209-2271`, `:2309`). - Only the non-blocking `open_diff` writes a temp file, and the plugin does not call it. It goes under `stdpath("cache")/claudecode_diffs/<session folder>`, and `vim.fn.tempname()` supplies only a part of the folder name (`diff.lua:522-561`, `:791-860`). ### Statements of the old sections that the code contradicts | Old statement | What the code does | Evidence | |---|---|---| | Lock: `local temp_path = lock_path .. ".tmp"` | The temp path is `<port>.lock.tmp.<pid>.<hrtime>`. | `lockfile.lua:154` | | Lock: `write_file(temp_path, json_data)` | No `write_file` exists. The code opens the temp file with `fs_open "wx"` and mode 0600, loops `fs_write` until every byte is written, then closes it. | `grep -c write_file lockfile.lua` is 0. `lockfile.lua:156-188` | | Lock: `vim.loop.fs_rename(temp_path, lock_path)` | The code calls `os.rename`. | `grep -c fs_rename lockfile.lua` is 0. `lockfile.lua:195` | | Lock: a `VimLeavePre` autocmd in `lockfile.lua` calls `fs_unlink(lock_path)` | `lockfile.lua` has no autocmd. Its `fs_unlink` calls remove only the temp file. The lock file goes through `remove` with `os.remove`, which `stop` calls. The exit autocmd is in `init.lua`. | `grep -c VimLeavePre lockfile.lua` is 0. `lockfile.lua:191`, `:197`, `:221`. `init.lua:514-522`, `:642` | | Diff: "Create temp file with proposed changes" with `vim.fn.tempname()` and `write_file(temp_file, new_content)` | `openDiff` writes no temp file. The proposal is a scratch buffer. `write_file` appears nowhere under `lua/`. The only file write in `diff.lua` is `io.open(tmp_file, "w")` in `create_temp_file`, which the legacy `open_diff` calls. `tempname()` seeds part of the session folder name. | Probe: no `claudecode_diffs*` folder after an `openDiff` diff. `diff.lua:523-524`, `:553`, `:1730-1748` | | Diff: `edit original_file`, `diffthis`, `vsplit temp_file`, `diffthis` | The old file goes in a window with `diffthis`. `create_split` makes a split to the right (`vertical`) or below (`horizontal`). The proposal buffer goes there with `diffthis`. | Probe: right for `vertical`, below for `horizontal`. `diff.lua:279-288`, `:715-730` | | Diff: "Open diff in current tab to reduce clutter" | The current tab is preferred and not guaranteed. A new tab opens when `open_in_new_tab` is set or the picked tab or the current tab already has a diff window. With no usable editor window in the current tab, the diff uses one in another tab. | Probe: 1 tab by default, 2 tabs for each of the two cases. `diff.lua:125-136`, `:1625`, `:1700`. `tests/integration/diff_current_tab_spec.lua:136` | | Diff: `vim.keymap.set("n", "<leader>da", accept_all_changes)` and `<leader>dq` with `exit_diff_mode` | The plugin sets no diff keymap. `accept_all_changes` and `exit_diff_mode` do not exist. Accept and reject go through `:w`, the two commands, and closing the proposal. | `grep keymap diff.lua diff_inline.lua` finds nothing. `init.lua:1209-1219`. Probe of all five accept paths and three reject paths | | Diff: "Native Neovim diff implementation" | True for `vertical` and `horizontal`. It is incomplete: `unified` is an inline renderer and not a Neovim diff. | Probe: 0 windows with `'diff'` set for `unified`. `diff.lua:1595` | ## Scope Only these two sections of `ARCHITECTURE.md`. I did not audit the rest of the file. The line numbers in this description refer to `main` at `5acc1002`. `ARCHITECTURE.md` cites symbols and no line numbers, so later commits to `diff.lua` do not make it stale. coder#542 added three lines to `diff.lua` after this branch left `main`. - **coder#537** landed as `5acc1002` and conflicted with the lock section in one hunk. This branch keeps its own lifecycle bullet, which says what the sentence of coder#537 said, with citations. The comment fix of coder#537 in the Resource Cleanup snippet is kept. - **Resource Cleanup snippet** (not changed). It is a simplified form of the real autocmd. The real one has the `ClaudeCodeShutdown` group and calls `stop` only when a server runs (`init.lua:514-522`). It is not false. - **`README.md:251-252`** (not changed). It lists "Accept: `:w` or `<leader>aa`" and "Reject: `:q` or `<leader>ad`" without saying that the two keys come from the example config at `README.md:66-67`. Nothing under `lua/` or `plugin/` sets them. That is a possible follow-up for the README. ## Blast Radius One documentation file. Nothing loads it. A wrong sentence here misleads a contributor and breaks no behavior. ## Verification I ran the real modules in headless Neovim 0.12.3 with an isolated home (`env -i`, private `HOME`, `XDG_*` and `CLAUDE_CONFIG_DIR`, the stub `claude` first on `PATH`). Lock file section: - A new lock directory has mode 0700 and the lock file has mode 0600. - The file holds the keys `authToken`, `ideName`, `pid`, `transport` and `workspaceFolders`, with `ideName` `Neovim` and `transport` `ws`. - No temp file stays after `create`. - `fs_open "wx"` fails with `EEXIST` for an existing file and for a dangling symlink. - `remove` returns `Lock file does not exist` for a missing file and `Invalid port number` for a bad port. - With `CLAUDE_CONFIG_DIR` empty or unset, the lock directory is `~/.claude/ide`. Diff System section: - `vertical`: the proposal is right of the old file. `horizontal`: below it. Both windows have `'diff'` set, there is 1 tab, and the proposal buffer is `acwrite` and unlisted. No `claudecode_diffs` folder exists. - `:w`, `:wq`, `:x`, `ZZ` and `:ClaudeCodeDiffAccept` each reply `FILE_SAVED` with the new text, and the target file is unchanged. - `:ClaudeCodeDiffDeny`, closing the last window of the proposal, and `bwipeout` of its buffer each reply `DIFF_REJECTED`. - A `ClaudeCodeDiffOpened` handler that rejects the diff makes `open_diff_blocking` return `DIFF_REJECTED` at once: the coroutine is dead after its first resume and never yielded. - `:ClaudeCodeDiffDeny` and `:ClaudeCodeDiffAccept` in the old-file buffer only warn `No active diff found in current buffer`, and the diff stays pending. - The proposal buffers are named `n-exist (proposed)`, `n-new (NEW FILE - proposed)` and `u1 (unified diff)` for an existing file, a new file and the unified layout. - After `:w` and `close_saved_diffs`, the target file is unchanged and its buffer holds the accepted text as unsaved changes. When the buffer already has unsaved changes, the diff stays open (`kept` says `its buffer has unsaved changes`) and the plugin does not touch the buffer. - A dirty target buffer makes `open_diff_blocking` raise `Diff setup failed`. The coroutine is dead after its first resume. - A new-file diff started from an empty buffer reuses that buffer as its old side. Started from a file buffer, it shows an empty `nofile` placeholder named `<path> (NEW FILE)`. - `open_in_new_tab = true` gives 2 tabs. A diff already in the current tab gives 2 tabs. `unified` gives 1 tab, 0 windows with `'diff'` set, and an inline diff buffer. - The legacy `open_diff` writes `target.txt.new` under `stdpath("cache")/claudecode_diffs/<session folder>`. Checks: - Citation check over the seven READMEs and `ARCHITECTURE.md` (I added the file to the validator's list): 787 citations, 1 flagged. The flagged one is `[plugin/claudecode.lua:pcall(require, "claudecode")]`, an expression that `main` already cites. A negative control with two wrong symbols flagged both. - `mise run format-check`: exit 0 (355 files, 0 changed). - I did not run `mise run all`. The change is docs only, and no test, script or CI file reads `ARCHITECTURE.md` (I checked `tests`, `scripts`, `.github` and `mise.toml`). CI runs the full suite on the pushed head. - `git merge-tree --write-tree` of the head and `origin/main` is clean. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
hareki
pushed a commit
to hareki/claudecode.nvim
that referenced
this pull request
Nov 13, 2026
… missing (coder#557) ## 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 coder#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: <path>") 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: <path>", as before. A file that is there and that `io.open` cannot open gives "Lock file exists but cannot be read: <path>: 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: <path>" 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 coder#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: <path>") and not the C library's ("<path>: 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: <path>" 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`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The advisor on #533 found that three sentences in
scripts/README.mdsay the plugin removes the lock file with no condition. The removal can fail.lockfile.removereturns an error whenos.removefails (for example in a read-only lock directory) and whenvim.fn.filereadablereports the file as missing (the file is gone, or it exists but is unreadable; follow-up F19 fixes the unreadable case) (lua/claudecode/lockfile.lua:208-232).stopand the token mismatch path ofstartonly log a warning then, so a lock file that the removal failed to delete stays (lua/claudecode/init.lua:642-647,:605-608).README.mdandlua/claudecode/README.mdalready carry the condition.ARCHITECTURE.mdhas the same wording: a comment saysM.stop()will "remove lock file", and the Lock File System section shows an unconditionalfs_unlinkin aVimLeavePrecallback.The lock directory sentence for
repro_issue_277.shsaid "else~/.claude/ide". The default applies whenCLAUDE_CONFIG_DIRis unset or empty (lua/claudecode/lockfile.lua:11-18), andREADME.mdalready says so.What changed
Three bullets in
scripts/README.md, one sentence each, and two lines inARCHITECTURE.md:repro_issue_70_probe.lua(older text onmain). Other Claude CLIs see the lock file untilclaudecode.stop()removes it. New: when the removal fails,stoponly logs a warning, so a lock file that the removal failed to delete stays.repro_issue_70.sh. A probe stopped by SIGTERM "still tries to remove" itsPORT.lock. When that fails,stoponly logs a warning, so a lock file that the removal failed to delete stays. A SIGKILL leaves the file behind, as before.repro_issue_277.sh. The plugin "removes the lock, or only logs a warning when it cannot". The lock directory is$CLAUDE_CONFIG_DIR/ide, or~/.claude/idewhen that variable is unset or empty.I added the citation
[lua/claudecode/init.lua:M.stop]to the first bullet.ARCHITECTURE.md, Resource Cleanup. The comment now reads-- Stop server, try to remove lock file (only logs a warning when it cannot). The file has no[file:symbol]citations, so I added none.ARCHITECTURE.md, Lock File System. One sentence after the code block: the real cleanup runs frominit.lua, whoseVimLeavePreautocmd callsstopwhen the server runs,stopcallslockfile.remove, andstoponly logs a warning when the removal fails, so a lock file that the removal failed to delete stays.Scope
I grepped every tracked Markdown file for lock-file removal wording (remove, delete, unlink, clean up, stale, leaves). The other READMEs already carry the condition:
README.md:1126and:1154-1155, andlua/claudecode/README.md:109-111and:196.The
ARCHITECTURE.mdsketch (line 55) reads as how the plugin works today and not as an illustration. It sits under the headinglockfile.luaand no text calls it an example. It is also wrong:lockfile.luahas noVimLeavePreautocmd and removes the file withos.removeinlockfile.remove(lua/claudecode/lockfile.lua:221), and the exit cleanup is ininit.lua(lua/claudecode/init.lua:514-518). So I left the code block as it is and added the one sentence. I did not fix the other lines of the sketch (write_fileandfs_renameare not inlockfile.luaeither). That is a wider rewrite of the section.Not changed:
.agents/skills/verify-claudecode-nvim/features/server.md:9and:12describe the checked working case ("deletes<port>.lock"). They are test expectations.scripts/repro_issue_70_probe.lua:84has the code comment-- removes the lock file. It is code and not a README.Blast Radius
Two documentation files: three lines of
scripts/README.md, and two lines ofARCHITECTURE.md(one changed, one added). Nothing loads them. A wrong sentence here misleads a reader and breaks no behavior.Verification
I probed
scripts/repro_issue_70_probe.luaon Neovim 0.12.3 in an isolated home (env -i, privateHOME,XDG_*andCLAUDE_CONFIG_DIR, the stubclaudefirst onPATH, providernone). Each case signalled only thenvimthat it started.stopremoved it)Claude Code integration stoppedloggedIn the read-only case the probe prints no warning.
logger.warnruns throughvim.schedule(lua/claudecode/logger.lua:109-120), and the probe callsos.exitright afterstop(). So the README says whatstopdoes (it logs a warning) and does not say that the probe shows it.[plugin/claudecode.lua:pcall(require, "claudecode")], an expression thatmainalready cites.mise run format-check: exit 0 (355 files, 0 changed).mise run all: exit 0 (1682 successes, 0 failures, 0 errors, 24 pending,luacheck0 warnings and 0 errors in 178 files).lockfile.removealso fails when the file is already gone (or unreadable; see F19) (lua/claudecode/lockfile.lua:215-216), so a failed removal does not always leave a file. The three sentences that said "the file stays" now say that a lock file that the removal failed to delete stays.ARCHITECTURE.md(grep overtests,scriptsand.github). For them I ranmise run format-check(exit 0, 355 files, 0 changed) and the citation check (762 citations, same 1 flagged). I did not runmise run allagain. CI runs the full test suite on the final head.git merge-tree --write-treeof the head andorigin/mainis clean.Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh