Conversation
💡 Codex ReviewUpdating this crate to Rust 2024 in Cargo without updating Bazel leaves the two build paths out of sync: ℹ️ 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 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
c08ee08 to
33e587e
Compare
40849a1 to
f6e90ad
Compare
## Why `codex-utils-pty` and `codex-windows-sandbox` were the remaining crates in `codex-rs` that still overrode the workspace's Rust 2024 edition. Moving them forward in a separate PR keeps the baseline edition update isolated from the follow-on Bazel clippy workflow in #15955, while making linting and formatting behavior consistent with the rest of the workspace. This PR also needs Cargo and Bazel to agree on the edition for `codex-windows-sandbox`. Without the Bazel-side sync, the experimental Bazel app-server builds fail once they compile `windows-sandbox-rs`. ## What changed - switch `codex-rs/utils/pty` and `codex-rs/windows-sandbox-rs` to `edition = "2024"` - update `codex-utils-pty` callsites and tests to use the collapsed `if let` form that Clippy expects under the new edition - fix the Rust 2024 fallout in `windows-sandbox-rs`, including the reserved `gen` identifier, `unsafe extern` requirements, and new Clippy findings that surfaced under the edition bump - keep the edition bump separate from a larger unsafe cleanup by temporarily allowing `unsafe_op_in_unsafe_fn` in the Windows entrypoint modules that now report it under Rust 2024 - update `codex-rs/windows-sandbox-rs/BUILD.bazel` to `crate_edition = "2024"` so Bazel compiles the crate with the same edition as Cargo --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/15954). * #15976 * #15955 * __->__ #15954
620f7a4 to
fb4e76b
Compare
| - name: Make DotSlash available in PATH (Unix) | ||
| if: inputs.install-test-prereqs == 'true' && runner.os != 'Windows' | ||
| shell: bash | ||
| run: cp "$(which dotslash)" /usr/local/bin |
There was a problem hiding this comment.
That sounds a bit hacky no? Why not just updating PATH? 🤔
Also, I though which was considered flaky in bash (we might not care as we are in controlled env though)
There was a problem hiding this comment.
Fwiw, we were already doing this in .github/workflows/bazel.yml.
I believe I did this originally because updating PATH here doesn't propagate to subsequent steps and the bookkeeping to do it that way isn't worth it.
Or at least, facebook/install-dotslash@v2 normally does seem to work that way, but something (Bazel?) I think uses a more sanitized version of PATH that excludes the entry where GitHub Actions normally introduce executables.
| with: | ||
| path: | | ||
| ~/.cache/bazel-repo-cache | ||
| key: bazel-cache-${{ inputs.target }}-${{ hashFiles('MODULE.bazel', 'codex-rs/Cargo.lock', 'codex-rs/Cargo.toml') }} |
There was a problem hiding this comment.
OOC, why not using MODULE.bazel.lock ?
There was a problem hiding this comment.
Either should be fine, especially since the lock file is checked in here. MODULE.bazel.lock will probably get you more granular buckets which is nice (but since items in the repository cache are keyed under a hash of contents, it shouldn't be a big deal either way).
.github/scripts/run-bazel-ci.sh
Outdated
| #!/usr/bin/env bash | ||
|
|
||
| set -euo pipefail | ||
| set -o pipefail |
There was a problem hiding this comment.
OOC, isn't it redundant with the previous line?
.github/scripts/run-bazel-ci.sh
Outdated
|
|
||
| bazel_startup_args=() | ||
| if [[ -n "${BAZEL_STARTUP_ARGS:-}" ]]; then | ||
| read -r -a bazel_startup_args <<< "${BAZEL_STARTUP_ARGS}" |
There was a problem hiding this comment.
I think this will break if you have space in the startup args
Something like
--host_jvm_args=-Dfoo="bar baz"
Probably not a big deal though
There was a problem hiding this comment.
Great catch! Asked Codex to fix.
Though maybe the real fix is to rewrite this file in Python...
Why
bazel.ymlalready builds and tests the Bazel graph, butrust-ci.ymlstill runscargo clippyseparately. This PR starts the transition to a Bazel-backed lint lane forcodex-rsso we can eventually replace the duplicate Rust build, test, and lint work with Bazel while explicitly keeping the V8 Bazel path out of scope for now.To make that lane practical, the workflow also needs to look like the Bazel job we already trust. That means sharing the common Bazel setup and invocation logic instead of hand-copying it, and covering the arm64 macOS path in addition to Linux.
Landing the workflow green also required fixing the first lint findings that Bazel surfaced and adding the matching local entrypoint.
What changed
build:clippyconfig to.bazelrcand exportcodex-rs/clippy.tomlfromcodex-rs/BUILD.bazelso Bazel can run the repository's existing Clippy policyjust bazel-clippyso the local developer entrypoint matches the new CI lane.github/workflows/bazel.ymlwith a dedicated Bazel clippy job forcodex-rs, scoped to//codex-rs/... -//codex-rs/v8-poc:all.github/actions/setup-bazel-ci/action.ymland the shared Bazel invocation logic into.github/scripts/run-bazel-ci.shso the clippy and build/test jobs stay alignedcmsghdr::cmsg_lennormalization incodex-rs/shell-escalation/src/unix/socket.rsand the no-voice-inputdead-code warnings incodex-rs/tuiandcodex-rs/tui_app_serverVerification
just bazel-clippyRUNNER_OS=macOS ./.github/scripts/run-bazel-ci.sh -- build --config=clippy --build_metadata=COMMIT_SHA=local-check --build_metadata=TAG_job=clippy -- //codex-rs/... -//codex-rs/v8-poc:allbazel build --config=clippy //codex-rs/shell-escalation:shell-escalationCARGO_TARGET_DIR=/tmp/codex4-shell-escalation-test cargo test -p codex-shell-escalationruby -e 'require "yaml"; YAML.load_file(".github/workflows/bazel.yml"); YAML.load_file(".github/actions/setup-bazel-ci/action.yml")'Notes
CARGO_TARGET_DIR=/tmp/codex4-tui-app-server-test cargo test -p codex-tui-app-serverstill hits existing guardian-approvals test and snapshot failures unrelated to this PR's Bazel-clippy changes.Related: #15954