-
Notifications
You must be signed in to change notification settings - Fork 7.9k
make codex better at git #10145
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
make codex better at git #10145
Conversation
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 884de2d53b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6057f2d49a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
9326378 to
05e0857
Compare
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30781cde87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
30781cd to
b705c11
Compare
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c43909332
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
3c43909 to
efbfa4b
Compare
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 413d8435f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
413d843 to
121f081
Compare
a77b812 to
3a47181
Compare
codex-rs/core/src/git_info.rs
Outdated
| ) -> Option<std::process::Output> { | ||
| let mut command = Command::new("git"); | ||
| command.args(args).current_dir(cwd); | ||
| command.kill_on_drop(true); |
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.
added kill_on_drop as a general safeguard - it doesn’t look like any callers rely on git continuing after the future is dropped
b290663 to
2bc0f86
Compare
codex-rs/core/src/client.rs
Outdated
| websocket_last_items: Vec::new(), | ||
| transport_manager: self.state.transport_manager.clone(), | ||
| turn_state: Arc::new(OnceLock::new()), | ||
| turn_metadata_header, |
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 can build the header on demand instead of storing it on the session
aibrahim-oai
left a comment
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.
Let's add an integration test to test the flow e2e
800cfc9 to
c73c582
Compare
da20ff0 to
f1052e0
Compare
| .unwrap_or_else(|_| cwd.to_path_buf()) | ||
| .to_string_lossy() | ||
| .into_owned(); | ||
| for _attempt in 0..10 { |
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.
this test is a bit involved (~200 lines) because i have to make multiple requests to verify that headers are eventually sent (because of the async prewarm)
aibrahim-oai
left a comment
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.
LGTM, we have a lot of helper tests for integration tests that you can use to make the test shorter
|
/merge |
For posterity, I tried using helper tests to make this test shorter, but in this case they obscured the runtime behavior and made the teest less reliable around the async prewarm timing. So I kept the test explicit on purpose so the behavior is clear and stable - i'll keep an eye on it for follow-ups |
adds basic git context to the session prefix so the model can anchor git actions and be a bit more version-aware. structured it in a multiroot-friendly shape even though we only have one root today