feat(cli): add optional cwd argument to serve#601
feat(cli): add optional cwd argument to serve#601Olusammytee wants to merge 2 commits intoKilo-Org:devfrom
Conversation
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
| }), | ||
| describe: "starts a headless kilo server", // kilocode_change | ||
| handler: async (args) => { | ||
| const baseCwd = process.env.PWD ?? process.cwd() |
There was a problem hiding this comment.
[WARNING]: Missing kilocode_change markers on Kilo-specific code in shared file
This file is shared with upstream opencode. Per AGENTS.md, Kilo-specific changes in shared code should be marked with kilocode_change comments. Lines 6, 9, and 17–25 are new Kilo-specific additions but lack markers. Other lines in this file (5, 13, 15, 32–47) already have them.
Consider wrapping this block:
// kilocode_change start - optional project path support
const baseCwd = process.env.PWD ?? process.cwd()
const cwd = args.project ? path.resolve(baseCwd, args.project) : process.cwd()
try {
process.chdir(cwd)
} catch {
console.error(`Failed to change directory to ${cwd}`)
process.exitCode = 1
return
}
// kilocode_change end
Also add // kilocode_change to the import path on line 6 and the command change on line 9.
| const cwd = args.project ? path.resolve(baseCwd, args.project) : process.cwd() | ||
| try { | ||
| process.chdir(cwd) | ||
| } catch { |
There was a problem hiding this comment.
[SUGGESTION]: Capture the error in the catch clause for better diagnostics
The catch clause discards the error. Including the actual error reason (e.g., ENOENT vs EACCES) in the message would help users debug path issues. The existing pattern in thread.ts:107 captures (e) as well.
| } catch { | |
| } catch (e) { |
Code Review SummaryStatus: No New Issues Found | Recommendation: Merge OverviewThis PR adds an optional
The 4 existing inline comments from prior reviews have been noted. The current code already addresses the error handling concerns raised — the catch block now properly captures the error, checks Files Reviewed (1 file)
|
There was a problem hiding this comment.
Pull request overview
This pull request adds an optional positional project path argument to the kilo serve command, enabling users to start the server in a specific directory without needing to manually change directories first. This is particularly useful for debugging scenarios where different project directories need to be tested.
Changes:
- Added optional
[project]positional argument to the serve command - Implemented path resolution that respects the shell's working directory using
process.env.PWD - Added error handling for invalid directory paths with graceful exit
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| process.chdir(cwd) | ||
| } catch { | ||
| console.error(`Failed to change directory to ${cwd}`) |
There was a problem hiding this comment.
The error handling in the serve command doesn't match the pattern used elsewhere in the codebase. In the TUI thread command (tui/thread.ts:108), UI.error is used for error messages which provides consistent formatting with the TEXT_DANGER_BOLD style. The serve command should use UI.error instead of console.error for consistency.
| } catch { | ||
| console.error(`Failed to change directory to ${cwd}`) |
There was a problem hiding this comment.
The catch block silently ignores the error object. Consider capturing and logging the error for better debugging, similar to how other commands in the codebase handle errors. For example, in tui/thread.ts:107, the error is captured as 'e' even though it's not used in the error message. This helps with debugging if the error needs to be inspected later.
| } catch { | |
| console.error(`Failed to change directory to ${cwd}`) | |
| } catch (e) { | |
| console.error(`Failed to change directory to ${cwd}`, e) |
|
Pushed follow-up commit 73347af to harden serve [project] path handling on Windows: we now resolve from process.cwd() and only call process.chdir when project is provided, with a clearer error message on failure. |
Summary
Fixes #128