fix: resolve command to full exec path for Windows PATHEXT#272
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
participant Transport
participant Neovim
participant ACPProcess
Transport->>Neovim: Resolve config.command
Neovim-->>Transport: Return executable path
Transport->>ACPProcess: Spawn resolved command
ACPProcess-->>Transport: Return handle or spawn error
Transport->>Transport: Close pipes and set error state on failure
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lua/agentic/acp/acp_transport.lua`:
- Around line 96-100: The error path in acp_transport.lua now leaks the
allocated stdin/stdout/stderr libuv handles when
`vim.fn.exepath(config.command)` returns empty inside the transport startup
logic. Before calling `error(...)` in that branch, make sure the newly created
pipes are explicitly closed/cleaned up and any partial transport state is torn
down, using the existing startup code around `callbacks.on_state_change` and the
handle allocation site in `acp_transport.lua`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0b72ae05-2606-4b46-94d9-5340df8ee438
📒 Files selected for processing (1)
lua/agentic/acp/acp_transport.lua
940b069 to
ac7c1bc
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
carlos-algms
left a comment
carlos-algms
left a comment
There was a problem hiding this comment.
:claude: One change requested — see inline comment on the exepath line.
ac7c1bc to
92fd78a
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lua/agentic/acp/acp_transport.lua (1)
197-200: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose pipes on
uv.spawnfailure to prevent handle leaks.Although you successfully eliminated the main pipe leak by moving the
exepathcheck up, ifuv.spawnfails for other reasons (e.g.,EACCESlack of execution permissions), thestdin,stdout, andstderrpipes allocated on lines 89-91 will still leak.Additionally, since
uv.spawnreturns the error name as its second return value upon failure, it can be included in the error message for better debugging.🔧 Proposed fix
- if not handle then + if not handle then + stdin:close() + stdout:close() + stderr:close() callbacks.on_state_change("error") - error("Failed to spawn ACP agent process") + error(string.format("Failed to spawn ACP agent process: %s", tostring(pid))) end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lua/agentic/acp/acp_transport.lua` around lines 197 - 200, Update the uv.spawn failure branch in the ACP process startup flow to close the allocated stdin, stdout, and stderr pipes before raising the error. Capture uv.spawn’s second return value and include it in the failure message, while preserving the existing on_state_change("error") callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lua/agentic/acp/acp_transport.lua`:
- Around line 197-200: Update the uv.spawn failure branch in the ACP process
startup flow to close the allocated stdin, stdout, and stderr pipes before
raising the error. Capture uv.spawn’s second return value and include it in the
failure message, while preserving the existing on_state_change("error")
callback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b3fe158-8ca1-4597-8051-5aa2cbd6a652
📒 Files selected for processing (1)
lua/agentic/acp/acp_transport.lua
92fd78a to
78d4572
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lua/agentic/acp/acp_transport.lua`:
- Around line 195-209: Move the “Spawned ACP agent process” debug log in the
uv.spawn flow to after the `if not handle` failure guard, so it only logs
successful spawns and never treats the failure message in `pid` as a process ID.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d646a34-8f2e-4ae8-986e-ec1fcfc97204
📒 Files selected for processing (1)
lua/agentic/acp/acp_transport.lua
78d4572 to
c2ea276
Compare
2 participants
resolves #270
On Windows,
uv.spawn()does not automatically applyPATHEXTresolution. This means when the plugin tries to spawn provider commands likeclaude-agent-acp, it fails on Windows because the actual exectuable isclaude-agent-acp.CMDbutuv.spawn()does not know to look for the.CMDextension.Issue
The raw command name from config is being passed directly.
Solution
vim.fn.exepath()function provides a cross-platform command resolver, which includes the extension on Windows.Risk
Low,
vim.fn.exepath()is available in all supported Neovim versions (0.11+). Full paths are accepted byuv.spawn().