fix(core): skip approval for patches with zero changes #4298
Conversation
Fixes openai#4228 - Add zero-change detection to avoid unnecessary approval prompts when patches result in 0 additions and 0 deletions. - Added has_no_actual_changes() function to detect empty patches - Added calculate_changes_from_diff() to count diff changes - Modified apply_patch() to return early for zero-change patches - Added comprehensive unit tests for edge cases
|
I have read the CLA Document and I hereby sign the CLA mgiovani seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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 👍.
|
I have read the CLA Document and I hereby sign the CLA |
Addresses review feedback - fixed bug where lines starting with '+++'
or '---' in code content were incorrectly filtered as file headers.
- Changed from starts_with('+++') to starts_with('+++ ')
- Changed from starts_with('---') to starts_with('--- ')
- File headers always have space after prefix: '+++ b/file'
- Content lines like '+++counter;' are now correctly counted
- Added tests for increment/decrement operator edge cases
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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 review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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 review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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 review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. |
Addresses second review feedback - fixed bug where file renames with no content changes were incorrectly skipped. - Check move_path in Update operations before checking diff content - Rename operations are real changes even with identical content - Added test for rename-only patch scenario - Ensures file renames are properly applied, not skipped
- Never skip Add operations (even empty files like .gitkeep are real changes) - Never skip Delete operations (any file deletion is a real change) - Only skip Update operations with no diff AND no rename - Remove unnecessary comments for staff-level code quality - Add comprehensive test coverage for all edge cases Addresses all P1 review feedback: - Fixed increment/decrement operator parsing bug - Fixed rename-only patch regression - Fixed whitespace-only add/delete operations being incorrectly skipped
…issues - Fix test compilation errors by refactoring tests to use available APIs - Add metadata detection for permission, binary, and rename changes in diffs - Enhance diff analysis to detect mode changes, binary diffs, and other metadata - Only skip approval for truly empty Update operations (no content, no rename, no metadata) - Never skip Add/Delete operations as they always represent real file system changes - Add comprehensive test coverage for all edge cases including permission changes Resolves P0 compilation failure and P1 metadata detection bugs Maintains conservative approach - only skips genuine no-op operations
Details are in `responses-api-proxy/README.md`, but the key contribution of this PR is a new subcommand, `codex responses-api-proxy`, which reads the auth token for use with the OpenAI Responses API from `stdin` at startup and then proxies `POST` requests to `/v1/responses` over to `https://api.openai.com/v1/responses`, injecting the auth token as part of the `Authorization` header. The expectation is that `codex responses-api-proxy` is launched by a privileged user who has access to the auth token so that it can be used by unprivileged users of the Codex CLI on the same host. If the client only has one user account with `sudo`, one option is to: - run `sudo codex responses-api-proxy --http-shutdown --server-info /tmp/server-info.json` to start the server - record the port written to `/tmp/server-info.json` - relinquish their `sudo` privileges (which is irreversible!) like so: ``` sudo deluser $USER sudo || sudo gpasswd -d $USER sudo || true ``` - use `codex` with the proxy (see `README.md`) - when done, make a `GET` request to the server using the `PORT` from `server-info.json` to shut it down: ```shell curl --fail --silent --show-error "http://127.0.0.1:$PORT/shutdown" ``` To protect the auth token, we: - allocate a 1024 byte buffer on the stack and write `"Bearer "` into it to start - we then read from `stdin`, copying to the contents into the buffer after the prefix - after verifying the input looks good, we create a `String` from that buffer (so the data is now on the heap) - we zero out the stack-allocated buffer using https://crates.io/crates/zeroize so it is not optimized away by the compiler - we invoke `.leak()` on the `String` so we can treat its contents as a `&'static str`, as it will live for the rest of the processs - on UNIX, we `mlock(2)` the memory backing the `&'static str` - when using the `&'static str` when building an HTTP request, we use `HeaderValue::from_static()` to avoid copying the `&str` - we also invoke `.set_sensitive(true)` on the `HeaderValue`, which in theory indicates to other parts of the HTTP stack that the header should be treated with "special care" to avoid leakage: https://github.com/hyperium/http/blob/439d1c50d71e3be3204b6c4a1bf2255ed78e1f93/src/header/value.rs#L346-L376
Adds a 1-per-turn todo-list item and item.updated event
```jsonl
{"type":"item.started","item":{"id":"item_6","item_type":"todo_list","items":[{"text":"Record initial two-step plan now","completed":false},{"text":"Update progress to next step","completed":false}]}}
{"type":"item.updated","item":{"id":"item_6","item_type":"todo_list","items":[{"text":"Record initial two-step plan now","completed":true},{"text":"Update progress to next step","completed":false}]}}
{"type":"item.completed","item":{"id":"item_6","item_type":"todo_list","items":[{"text":"Record initial two-step plan now","completed":true},{"text":"Update progress to next step","completed":false}]}}
```
…penai#4252) The [official Rust SDK](https://github.com/modelcontextprotocol/rust-sdk/tree/57fc428c578a1a3fe851ee0838bf068bda120eb3) has come a long way since we first started our mcp client implementation 5 months ago and, today, it is much more complete than our own stdio-only implementation. This PR introduces a new config flag `experimental_use_rmcp_client` which will use a new mcp client powered by the sdk instead of our own. To keep this PR simple, I've only implemented the same stdio MCP functionality that we had but will expand on it with future PRs. --------- Co-authored-by: pakrym-oai <pakrym@openai.com>
- Render `send a message to load usage data` in the beginning of the session - Render `data not available yet` if received no rate limits - nit case - Deleted stall snapshots that were moved to `codex-rs/tui/src/status/snapshots`
If we detect a dangerous command but approval_policy is Never, simply reject the command.
mgiovani
force-pushed
the
fix/issue-4228-skip-approval-zero-changes
branch
from
91e5386 to
988b375
Compare
What
Skip approval prompts when patches have zero actual changes (0 additions, 0 deletions).
Why
Fixes #4228 - Users shouldn't be prompted to approve changes that result in no actual modifications to the codebase.
How
has_no_actual_changes()function to detect empty patches across all change types:calculate_changes_from_diff()function to parse unified diff format and count actual line changesapply_patch()function to return early with success message for zero-change patchesTesting
Note: Full test suite cannot run due to pre-existing compilation issues with unstable
letexpressions in other crates (unrelated to this change).Impact