fix(server): handle processes without ports#202
fix(server): handle processes without ports#202madmaxieee wants to merge 1 commit intonickjvandyke:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the CLI process/server typing and server discovery logic to support opencode processes that do not expose an HTTP port, avoiding invalid client calls and providing a clearer failure mode.
Changes:
- Marked
Process.portas optional in the process type definition. - Updated
get_server(port)to explicitly reject whenportis missing, instead of attempting client calls withnil.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
lua/opencode/cli/server.lua |
Adds an early rejection path when get_server is called without a port. |
lua/opencode/cli/process/init.lua |
Updates the Process type annotation to make port optional. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lua/opencode/cli/server.lua
Outdated
| return Promise.new(function(_, reject) | ||
| reject("This process is not listening on any port.") | ||
| end) |
There was a problem hiding this comment.
Instead of constructing a new Promise only to immediately reject, consider using Promise.reject(...) here for a simpler implementation and consistent semantics with the promise module’s helper API.
| return Promise.new(function(_, reject) | |
| reject("This process is not listening on any port.") | |
| end) | |
| return Promise.reject("This process is not listening on any port.") |
| @@ -25,10 +25,15 @@ local M = {} | |||
|
|
|||
| ---Verify that an `opencode` process is responding on the given port, | |||
| ---and fetch some details about it. | |||
There was a problem hiding this comment.
The function-level doc comment still reads like a port is always provided. Since port is now optional and the promise is rejected when it’s missing, it would be clearer to document that behavior (and why) in the comment above get_server.
| ---and fetch some details about it. | |
| ---and fetch some details about it. | |
| ---If `port` is `nil`, the returned promise is rejected immediately with a message | |
| ---indicating that this process is not listening on any port; this function does | |
| ---not perform any port discovery, and callers must supply a known listening port. |
| ---@class opencode.cli.process.Process | ||
| ---@field pid number | ||
| ---@field port number | ||
| ---@field port number? |
There was a problem hiding this comment.
For class fields, this repo commonly marks optional fields with ---@field name? type (e.g. opencode.cli.server.Opts.port? number in cli/server.lua). To keep annotations consistent, consider changing this to ---@field port? number.
| ---@field port number? | |
| ---@field port? number |
Mark the port field as optional in the process definition and update the server logic to reject the promise with a clear error message if a port is not provided.
madmaxieee
force-pushed
the
fix-processes-without-ports
branch
from
17cbc5e to
f51e628
Compare
|
When do you experience this? The process-finding code should already filter out processes that aren't listening on ports. That's why the field is non-nullable. |
|
You're right. It is fixed in 0.5.1. I was using 0.5.0, which is like one commit before this got fixed lol. Sorry for the bother. Thank you for maintaining such a great plugin. |
|
No worries! Thanks for trying the most recent version to verify 🙂 |
Description
Mark the port field as optional in the process definition and update the server logic to reject the promise with a clear error message if a port is not provided because not all processes listen to ports.
Related Issue(s)
Screenshots/Videos