-
Notifications
You must be signed in to change notification settings - Fork 5.5k
SDK: support working directory and skipGitRepoCheck options #4563
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
Conversation
Change exitCode promise to reject instead of throwing error when child process exits with non-zero code. Update run.test.ts to properly test error handling and remove unnecessary restore call.
sdk/typescript/src/exec.ts
Outdated
resolve(code); | ||
} else { | ||
throw new Error(`Codex Exec exited with code ${code}`); | ||
reject(new Error(`Codex Exec exited with code ${code}: ${stderr}`)); |
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.
It might be useful to surface stderr
somehow even when things succeed because there could be warnings printed there that are important?
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.
Agree but not sure where to put it yet.
if (args.skipGitRepoCheck) { | ||
commandArgs.push("--skip-git-repo-check"); | ||
} | ||
|
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.
I can't comment on the below, but we don't honor OPENAI_BASE_URL
or OPENAI_API_KEY
anymore, do we?
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.
I can't comment on line 53, but I would recommend feeding the user's prompt via stdin on the off chance that is extremely large and would exceed PATH_ARG_MAX
.
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.
We very much honor OPENAI_BASE_URL that's how all our mock tests work.
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.
I'll look into API key.
Updated to prompt via stdin
- Move user input to stdin for both normal and resume execution - Refine argument construction for threadId resume - Capture stderr as Buffer chunks and improve error reporting - Add error handling for missing stdin
Make options not required, add support for working directory and skipGitRepoCheck options on the turn