Skip to content

Comments

pr#2

Merged
zcg merged 24 commits intobyebye-code:mainfrom
openai:main
Dec 3, 2025
Merged

pr#2
zcg merged 24 commits intobyebye-code:mainfrom
openai:main

Conversation

@zcg
Copy link

@zcg zcg commented Dec 3, 2025

External (non-OpenAI) Pull Request Requirements

Before opening this Pull Request, please read the dedicated "Contributing" markdown file or your PR may be closed:
https://github.com/openai/codex/blob/main/docs/contributing.md

If your PR conforms to our contribution guidelines, replace this text with a detailed and high quality description of your changes.

Include a link to a bug report or enhancement request.

owenlin0 and others added 24 commits December 2, 2025 17:35
These will be more ergonomic to work with in Typescript.
# Ghost snapshot ignores

This PR should close #7067, #7395, #7405.

Prior to this change the ghost snapshot task ran `git status
--ignored=matching` so the report picked up literally every ignored
file. When a directory only contained entries matched by patterns such
as `dozens/*.txt`, `/test123/generated/*.html`, or `/wp-includes/*`, Git
still enumerated them and the large-untracked-dir detection treated the
parent directory as “large,” even though everything inside was
intentionally ignored.

By removing `--ignored=matching` we only capture true untracked paths
now, so those patterns stay out of the snapshot report and no longer
trigger the “large untracked directories” warning.

---------

Signed-off-by: lionelchg <lionel.cheng@hotmail.fr>
Co-authored-by: lionelchg <lionel.cheng@hotmail.fr>
- Show context window usage in tokens instead of percentage when the
window length is unknown.
this PR 
- adds a helper function to amend `.codexpolicy` files with new prefix
rules
- adds a utility to `Policy` allowing prefix rules to be added to
existing `Policy` structs

both additions will be helpful as we thread codexpolicy into the TUI
workflow
I noticed this while doing a post-commit review of #7467.
in response to incoming changes to responses headers where the header
may sometimes not contain credits info (no longer forcing a credit
check)
The problem with using `serde(flatten)` on Turn status is that it
conditionally serializes the `error` field, which is not the pattern we
want in API v2 where all fields on an object should always be returned.

```
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
#[serde(rename_all = "camelCase")]
#[ts(export_to = "v2/")]
pub struct Turn {
    pub id: String,
    /// Only populated on a `thread/resume` response.
    /// For all other responses and notifications returning a Turn,
    /// the items field will be an empty list.
    pub items: Vec<ThreadItem>,
    #[serde(flatten)]
    pub status: TurnStatus,
}

#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
#[serde(tag = "status", rename_all = "camelCase")]
#[ts(tag = "status", export_to = "v2/")]
pub enum TurnStatus {
    Completed,
    Interrupted,
    Failed { error: TurnError },
    InProgress,
}
```

serializes to:
```
{
  "id": "turn-123",
  "items": [],
  "status": "completed"
}

{
  "id": "turn-123",
  "items": [],
  "status": "failed",
  "error": {
    "message": "Tool timeout",
    "codexErrorInfo": null
  }
}
```

Instead we want:
```
{
  "id": "turn-123",
  "items": [],
  "status": "completed",
  "error": null
}

{
  "id": "turn-123",
  "items": [],
  "status": "failed",
  "error": {
    "message": "Tool timeout",
    "codexErrorInfo": null
  }
}
```
This PR honors the `history.max_bytes` configuration parameter by
trimming `history.jsonl` whenever it grows past the configured limit.
While appending new entries we retain the newest record, drop the oldest
lines to stay within the byte budget, and serialize the compacted file
back to disk under the same lock to keep writers safe.
I think this might help with #7033
because `create_approval_requirement_for_command()` will soon need
access to `Session.state`, which is a `tokio::sync::Mutex` that needs to
be accessed via `async`.
Pull FrameRequester out of tui.rs into its own module and make a
FrameScheduler struct. This is effectively an Actor/Handler approach
(see https://ryhl.io/blog/actors-with-tokio/). Adds tests and docs.

Small refactor of pending_viewport_area logic.
I find it helpful to easily verify which version is running.

Tested:

```shell
~/code/codex3/codex-rs/exec-server$ cargo run --bin codex-exec-mcp-server -- --help
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.19s
     Running `/Users/mbolin/code/codex3/codex-rs/target/debug/codex-exec-mcp-server --help`
Usage: codex-exec-mcp-server [OPTIONS]

Options:
      --execve <EXECVE_WRAPPER>  Executable to delegate execve(2) calls to in Bash
      --bash <BASH_PATH>         Path to Bash that has been patched to support execve() wrapping
  -h, --help                     Print help
  -V, --version                  Print version
~/code/codex3/codex-rs/exec-server$ cargo run --bin codex-exec-mcp-server -- --version
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.17s
     Running `/Users/mbolin/code/codex3/codex-rs/target/debug/codex-exec-mcp-server --version`
codex-exec-server 0.0.0
```
Fix issue #7430 
Generate unique numbered placeholders for multiple large pastes of the
same length so deleting one no longer removes the others.

Signed-off-by: Joshua <joshua1s@protonmail.com>
When running `npx @openai/codex-shell-tool-mcp`, the old code derived
`__dirname` from `process.argv[1]`, which points to npx’s transient
wrapper script in
`~/.npm/_npx/134d0fb7e1a27652/node_modules/.bin/codex-shell-tool-mcp`.
That made `vendorRoot` resolve to `<npx cache>/vendor`, so the startup
checks failed with "Required binary missing" because it looked for
`codex-execve-wrapper` in the wrong place.

By relying on the real module `__dirname` and `path.resolve(__dirname,
"..", "vendor")`, the package now anchors to its installed location
under `node_modules/@openai/codex-shell-tool-mcp/`, so the bundled
binaries are found and npx launches correctly.
Reading the tail can be costly if we have a very big rollout item. we
can just read the file metadata
I was debugging something else and noticed we could eliminate an
instance of `#[allow(dead_code)]` pretty easily.
- [x] Update device code auth strings.
#7486)

# Unified Exec Shell Selection on Windows

## Problem

reference issue #7466

The `unified_exec` handler currently deserializes model-provided tool
calls into the `ExecCommandArgs` struct:

```rust
#[derive(Debug, Deserialize)]
struct ExecCommandArgs {
    cmd: String,
    #[serde(default)]
    workdir: Option<String>,
    #[serde(default = "default_shell")]
    shell: String,
    #[serde(default = "default_login")]
    login: bool,
    #[serde(default = "default_exec_yield_time_ms")]
    yield_time_ms: u64,
    #[serde(default)]
    max_output_tokens: Option<usize>,
    #[serde(default)]
    with_escalated_permissions: Option<bool>,
    #[serde(default)]
    justification: Option<String>,
}
```

The `shell` field uses a hard-coded default:

```rust
fn default_shell() -> String {
    "/bin/bash".to_string()
}
```

When the model returns a tool call JSON that only contains `cmd` (which
is the common case), Serde fills in `shell` with this default value.
Later, `get_command` uses that value as if it were a model-provided
shell path:

```rust
fn get_command(args: &ExecCommandArgs) -> Vec<String> {
    let shell = get_shell_by_model_provided_path(&PathBuf::from(args.shell.clone()));
    shell.derive_exec_args(&args.cmd, args.login)
}
```

On Unix, this usually resolves to `/bin/bash` and works as expected.
However, on Windows this behavior is problematic:

- The hard-coded `"/bin/bash"` is not a valid Windows path.
- `get_shell_by_model_provided_path` treats this as a model-specified
shell, and tries to resolve it (e.g. via `which::which("bash")`), which
may or may not exist and may not behave as intended.
- In practice, this leads to commands being executed under a non-default
or non-existent shell on Windows (for example, WSL bash), instead of the
expected Windows PowerShell or `cmd.exe`.

The core of the issue is that **"model did not specify `shell`" is
currently interpreted as "the model explicitly requested `/bin/bash`"**,
which is both Unix-specific and wrong on Windows.

## Proposed Solution

Instead of hard-coding `"/bin/bash"` into `ExecCommandArgs`, we should
distinguish between:

1. **The model explicitly specifying a shell**, e.g.:

   ```json
   {
     "cmd": "echo hello",
     "shell": "pwsh"
   }
   ```

In this case, we *do* want to respect the model’s choice and use
`get_shell_by_model_provided_path`.

2. **The model omitting the `shell` field entirely**, e.g.:

   ```json
   {
     "cmd": "echo hello"
   }
   ```

In this case, we should *not* assume `/bin/bash`. Instead, we should use
`default_user_shell()` and let the platform decide.

To express this distinction, we can:

1. Change `shell` to be optional in `ExecCommandArgs`:

   ```rust
   #[derive(Debug, Deserialize)]
   struct ExecCommandArgs {
       cmd: String,
       #[serde(default)]
       workdir: Option<String>,
       #[serde(default)]
       shell: Option<String>,
       #[serde(default = "default_login")]
       login: bool,
       #[serde(default = "default_exec_yield_time_ms")]
       yield_time_ms: u64,
       #[serde(default)]
       max_output_tokens: Option<usize>,
       #[serde(default)]
       with_escalated_permissions: Option<bool>,
       #[serde(default)]
       justification: Option<String>,
   }
   ```

Here, the absence of `shell` in the JSON is represented as `shell:
None`, rather than a hard-coded string value.
Closes #3404 

## Summary
On windows, ctrl+v does not work for the same reason that cmd+v does not
work on macos. This PR adds alt/option+v detection, which allows windows
users to paste images from the clipboard using.

We could swap between just ctrl on mac and just alt on windows, but this
felt simpler - I don't feel strongly about it.

Note that this will NOT address image pasting in WSL environments, due
to issues with WSL <> Windows clipboards. I'm planning to address that
in a separate PR since it will likely warrant some discussion.

## Testing
- [x] Tested locally on a Mac and Windows laptop
If an image can't be read by the API, it will poison the entire history,
preventing any new turn on the conversation.
This detect such cases and replace the image by a placeholder
@zcg zcg merged commit 5c54c38 into byebye-code:main Dec 3, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.