From 02e796522869071d658f899356608a242e2f85e7 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 5 Aug 2025 23:33:21 -0700 Subject: [PATCH 1/3] fix: add stricter checks and better error messages to create_github_release.sh (#1874) This script attempts to verify that: - You have no local, uncommitted changes. - You are on `main` - The commit you are on exists on `main` also exists on the origin `https://github.com/openai/codex`, i.e., it is not just a commit you have pushed to your local version of `main` As part of this, try to print better error message if/when these conditions are violated. --- codex-rs/scripts/create_github_release.sh | 28 ++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/codex-rs/scripts/create_github_release.sh b/codex-rs/scripts/create_github_release.sh index 84dcb95fa0..4bc4e25125 100755 --- a/codex-rs/scripts/create_github_release.sh +++ b/codex-rs/scripts/create_github_release.sh @@ -19,7 +19,33 @@ if ! git diff --quiet || ! git diff --cached --quiet || [ -n "$(git ls-files --o fi # Fail if in a detached HEAD state. -CURRENT_BRANCH=$(git symbolic-ref --short -q HEAD) +CURRENT_BRANCH=$(git symbolic-ref --short -q HEAD 2>/dev/null || true) +if [ -z "${CURRENT_BRANCH:-}" ]; then + echo "ERROR: Could not determine the current branch (detached HEAD?)." >&2 + echo " Please run this script from a checked-out branch." >&2 + exit 1 +fi + +# Ensure we are on the 'main' branch before proceeding. +if [ "${CURRENT_BRANCH}" != "main" ]; then + echo "ERROR: Releases must be created from the 'main' branch (current: '${CURRENT_BRANCH}')." >&2 + echo " Please switch to 'main' and try again." >&2 + exit 1 +fi + +# Ensure the current local commit on 'main' is present on 'origin/main'. +# This guarantees we only create releases from commits that are already on +# the canonical repository (https://github.com/openai/codex). +if ! git fetch --quiet origin main; then + echo "ERROR: Failed to fetch 'origin/main'. Ensure the 'origin' remote is configured and reachable." >&2 + exit 1 +fi + +if ! git merge-base --is-ancestor HEAD origin/main; then + echo "ERROR: Your local 'main' HEAD commit is not present on 'origin/main'." >&2 + echo " Please push your commits first (git push origin main) or check out a commit on 'origin/main'." >&2 + exit 1 +fi # Create a new branch for the release and make a commit with the new version. if [ $# -ge 1 ]; then From 7b3ab968a0ee599b4eae03956322caac1642f55c Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 5 Aug 2025 23:36:10 -0700 Subject: [PATCH 2/3] docs: add more detail to the codex-rust-review (#1875) This PR attempts to break `codex-rust-review.md` into sections so that it is easier to consume. It also adds a healthy new section on "Assertions in Tests" that has been on my mind for awhile. --- .github/codex/labels/codex-rust-review.md | 128 +++++++++++++++++++++- 1 file changed, 122 insertions(+), 6 deletions(-) diff --git a/.github/codex/labels/codex-rust-review.md b/.github/codex/labels/codex-rust-review.md index 2c2893a1fe..ae82d272d7 100644 --- a/.github/codex/labels/codex-rust-review.md +++ b/.github/codex/labels/codex-rust-review.md @@ -6,18 +6,134 @@ Then provide the **review** (1-2 sentences plus bullet points, friendly tone). Things to look out for when doing the review: +## General Principles + - **Make sure the pull request body explains the motivation behind the change.** If the author has failed to do this, call it out, and if you think you can deduce the motivation behind the change, propose copy. - Ideally, the PR body also contains a small summary of the change. For small changes, the PR title may be sufficient. - Each PR should ideally do one conceptual thing. For example, if a PR does a refactoring as well as introducing a new feature, push back and suggest the refactoring be done in a separate PR. This makes things easier for the reviewer, as refactoring changes can often be far-reaching, yet quick to review. -- If the nature of the change seems to have a visual component (which is often the case for changes to `codex-rs/tui`), recommend including a screenshot or video to demonstrate the change, if appropriate. -- Rust files should generally be organized such that the public parts of the API appear near the top of the file and helper functions go below. This is analagous to the "inverted pyramid" structure that is favored in journalism. -- Encourage the use of small enums or the newtype pattern in Rust if it helps readability without adding significant cognitive load or lines of code. -- Be wary of large files and offer suggestions for how to break things into more reasonably-sized files. -- When modifying a `Cargo.toml` file, make sure that dependency lists stay alphabetically sorted. Also consider whether a new dependency is added to the appropriate place (e.g., `[dependencies]` versus `[dev-dependencies]`) -- If you see opportunities for the changes in a diff to use more idiomatic Rust, please make specific recommendations. For example, favor the use of expressions over `return`. - When introducing new code, be on the lookout for code that duplicates existing code. When found, propose a way to refactor the existing code such that it should be reused. + +## Code Organization + - Each create in the Cargo workspace in `codex-rs` has a specific purpose: make a note if you believe new code is not introduced in the correct crate. - When possible, try to keep the `core` crate as small as possible. Non-core but shared logic is often a good candidate for `codex-rs/common`. +- Be wary of large files and offer suggestions for how to break things into more reasonably-sized files. +- Rust files should generally be organized such that the public parts of the API appear near the top of the file and helper functions go below. This is analagous to the "inverted pyramid" structure that is favored in journalism. + +## Assertions in Tests + +Assert the equality of the entire objects instead of doing "piecemeal comparisons," performing `assert_eq!()` on individual fields. + +Note that unit tests also function as "executable documentation." As shown in the following example, "piecemeal comparisons" are often more verbose, provide less coverage, and are not as useful as executable documentation. + +For example, suppose you have the following enum: + +```rust +#[derive(Debug, PartialEq)] +enum Message { + Request { + id: String, + method: String, + params: Option, + }, + Notification { + method: String, + params: Option, + }, +} +``` + +This is an example of a _piecemeal_ comparison: + +```rust +// BAD: Piecemeal Comparison + +#[test] +fn test_get_latest_messages() { + let messages = get_latest_messages(); + assert_eq!(messages.len(), 2); + + let m0 = &messages[0]; + match m0 { + Message::Request { id, method, params } => { + assert_eq!(id, "123"); + assert_eq!(method, "subscribe"); + assert_eq!( + *params, + Some(json!({ + "conversation_id": "x42z86" + })) + ) + } + Message::Notification { .. } => { + panic!("expected Request"); + } + } + + let m1 = &messages[1]; + match m1 { + Message::Request { .. } => { + panic!("expected Notification"); + } + Message::Notification { method, params } => { + assert_eq!(method, "log"); + assert_eq!( + *params, + Some(json!({ + "level": "info", + "message": "subscribed" + })) + ) + } + } +} +``` + +This is a _deep_ comparison: + +```rust +// GOOD: Verify the entire structure with a single assert_eq!(). + +use pretty_assertions::assert_eq; + +#[test] +fn test_get_latest_messages() { + let messages = get_latest_messages(); + + assert_eq!( + vec![ + Message::Request { + id: "123".to_string(), + method: "subscribe".to_string(), + params: Some(json!({ + "conversation_id": "x42z86" + })), + }, + Message::Notification { + method: "log".to_string(), + params: Some(json!({ + "level": "info", + "message": "subscribed" + })), + }, + ], + messages, + ); +} +``` + +## More Tactical Rust Things To Look Out For + +- Do not use `unsafe` (unless you have a really, really good reason like using an operating system API directly and no safe wrapper exists). For example, there are cases where it is tempting to use `unsafe` in order to use `std::env::set_var()`, but this indeed `unsafe` and has led to race conditions on multiple occasions. (When this happens, find a mechanism other than environment variables to use for configuration.) +- Encourage the use of small enums or the newtype pattern in Rust if it helps readability without adding significant cognitive load or lines of code. +- If you see opportunities for the changes in a diff to use more idiomatic Rust, please make specific recommendations. For example, favor the use of expressions over `return`. +- When modifying a `Cargo.toml` file, make sure that dependency lists stay alphabetically sorted. Also consider whether a new dependency is added to the appropriate place (e.g., `[dependencies]` versus `[dev-dependencies]`) + +## Pull Request Body + +- If the nature of the change seems to have a visual component (which is often the case for changes to `codex-rs/tui`), recommend including a screenshot or video to demonstrate the change, if appropriate. - References to existing GitHub issues and PRs are encouraged, where appropriate, though you likely do not have network access, so may not be able to help here. +# PR Information + {CODEX_ACTION_GITHUB_EVENT_PATH} contains the JSON that triggered this GitHub workflow. It contains the `base` and `head` refs that define this PR. Both refs are available locally. From 966480e4af7087e72025efb8e48e8aebd666082f Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 5 Aug 2025 23:58:26 -0700 Subject: [PATCH 3/3] fix: try to reduce public API of crates to speed up incremental builds --- codex-rs/core/src/lib.rs | 8 ++++---- codex-rs/tui/src/lib.rs | 9 +++++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index c728bd3125..185bdead8a 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -15,7 +15,7 @@ pub use codex::Codex; pub use codex::CodexSpawnOk; pub mod codex_wrapper; pub mod config; -pub mod config_profile; +mod config_profile; pub mod config_types; mod conversation_history; pub mod error; @@ -33,7 +33,7 @@ pub use model_provider_info::ModelProviderInfo; pub use model_provider_info::WireApi; pub use model_provider_info::built_in_model_providers; pub use model_provider_info::create_oss_provider_with_base_url; -pub mod model_family; +mod model_family; mod models; mod openai_model_info; mod openai_tools; @@ -43,9 +43,9 @@ pub mod protocol; mod rollout; pub(crate) mod safety; pub mod seatbelt; -pub mod shell; +mod shell; pub mod spawn; -pub mod turn_diff_tracker; +mod turn_diff_tracker; mod user_notification; pub mod util; pub use apply_patch::CODEX_APPLY_PATCH_ARG1; diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 50535e5967..d8808d212f 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -27,14 +27,23 @@ mod bottom_pane; mod chatwidget; mod citation_regex; mod cli; +#[cfg(feature = "vt100-tests")] pub mod custom_terminal; +#[cfg(not(feature = "vt100-tests"))] +mod custom_terminal; mod exec_command; mod file_search; mod get_git_diff; mod git_warning_screen; mod history_cell; +#[cfg(feature = "vt100-tests")] pub mod insert_history; +#[cfg(not(feature = "vt100-tests"))] +mod insert_history; +#[cfg(feature = "vt100-tests")] pub mod live_wrap; +#[cfg(not(feature = "vt100-tests"))] +mod live_wrap; mod log_layer; mod markdown; mod slash_command;