-
Notifications
You must be signed in to change notification settings - Fork 620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stdio command path resolve #1386
base: main
Are you sure you want to change the base?
Stdio command path resolve #1386
Conversation
curious when you were running into this error - is it when using |
|
||
match std::env::current_dir() { | ||
Ok(cwd) => { | ||
let abs_path = cwd.join(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we wanna check if this is a valid path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah could be slightly better to exit earlier here if the file doesnt actually exist rather than letting if fail later in spawn_process
will add a check and then also will fix the lint errors from cargo fmt
I havent tried the cli argument, I run goose configure and add a stdio command line extension, enable it when i start a session! |
Fix command path resolution in StdioTransport
Problem
When using command-line extensions in Goose, the command path was being used as-is, which could cause failures when starting a Goose session from different directories since the paths were relative to the working directory.
Changes
StdioTransport::resolve_command_path()
to properly handle relative pathsTesting
Test cases to verify:
index.js
) are correctly resolved against the working directory