Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c67cf0b9c4
ℹ️ 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".
.github/workflows/rust-ci.yml
Outdated
| @@ -184,10 +172,6 @@ jobs: | |||
| --build_metadata=COMMIT_SHA=${GITHUB_SHA} \ | |||
| -- \ | |||
| //codex-rs/... | |||
There was a problem hiding this comment.
Include manual unit-test targets in Bazel lint run
Switching this job to bazel build ... //codex-rs/... drops lint coverage for unit-test crates because Bazel wildcard patterns exclude targets tagged manual, and our unit-test rust_test targets are tagged manual in defs.bzl (tags = test_tags + ["manual"]). The previous wrapper path defaulted to Cargo --all-targets, so test-only call sites were checked; with this change, uncommented literal arguments inside unit tests can pass CI undetected unless those targets are explicitly listed.
Useful? React with 👍 / 👎.
6e69a6c to
5495a3f
Compare
## Why `argument-comment-lint` had become a PR bottleneck because the repo-wide lane was still effectively running a `cargo dylint`-style flow across the workspace instead of reusing Bazel's Rust dependency graph. That kept the lint enforced, but it threw away the main benefit of moving this job under Bazel in the first place: metadata reuse and cacheable per-target analysis in the same shape as Clippy. This change moves the repo-wide lint onto a native Bazel Rust aspect so Linux and macOS can lint `codex-rs` without rebuilding the world crate-by-crate through the wrapper path. ## What Changed - add a nightly Rust toolchain with `rustc-dev` for Bazel and a dedicated crate-universe repo for `tools/argument-comment-lint` - add `tools/argument-comment-lint/driver.rs` and `tools/argument-comment-lint/lint_aspect.bzl` so Bazel can run the lint as a custom `rustc_driver` - switch repo-wide `just argument-comment-lint` and the Linux/macOS `rust-ci` lanes to `bazel build --config=argument-comment-lint //codex-rs/...` - keep the Python/DotSlash wrappers as the package-scoped fallback path and as the current Windows CI path - gate the Dylint entrypoint behind a `bazel_native` feature so the Bazel-native library avoids the `dylint_*` packaging stack - update the aspect runtime environment so the driver can locate `rustc_driver` correctly under remote execution - keep the dedicated `tools/argument-comment-lint` package tests and wrapper unit tests in CI so the source and packaged entrypoints remain covered ## Verification - `python3 -m unittest discover -s tools/argument-comment-lint -p 'test_*.py'` - `cargo test` in `tools/argument-comment-lint` - `bazel build //tools/argument-comment-lint:argument-comment-lint-driver --@rules_rust//rust/toolchain/channel=nightly` - `bazel build --config=argument-comment-lint //codex-rs/utils/path-utils:all` - `bazel build --config=argument-comment-lint //codex-rs/rollout:rollout` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/16106). * #16120 * __->__ #16106
b803333 to
1bb20d3
Compare
871fc7f to
24a83a1
Compare
Why
Follow-up to #16106.
argument-comment-lintalready runs as a native Bazel aspect on Linux and macOS, but Windows is still the long pole inrust-ci. To move Windows onto the same native Bazel lane, the toolchain split has to let exec-side helper binaries build in an MSVC environment while still linting repo crates aswindows-gnullvm.Pushing the Windows lane onto the native Bazel path exposed a second round of Windows-only issues in the mixed exec-toolchain plumbing after the initial wrapper/target fixes landed.
What Changed
rust-ci.ymlandrust-ci-full.ymllocal_windows_msvcplatform for exec-side helper binaries while keepinglocal_windowsas thewindows-gnullvmtarget platformrules_rustsorepository_set(...)preserves explicit exec-platform constraints for the generated toolchains, keep the Windows-specific bootstrap/direct-link fixes needed for the nightly lint driver, and expose exec-siderustc-dev.rlibs to the MSVC sysrootx86_64-pc-windows-msvcandx86_64-pc-windows-gnullvmtargetsdev_componentson the custom Windows nightly repository set so the MSVC exec helper toolchain actually downloads the compiler-internal crates thatclippy_utilsneedsrun-argument-comment-lint-bazel.shto enumerate concrete Windows Rust rules, normalize the resulting labels, and skip explicitly requested incompatible targets instead of failing before the lint run startsrules_rustbuild-script env propagation so exec-sidewindows-msvchelper crates drop forwarded MinGW include and linker search paths as whole flag/path pairs instead of emitting malformedCFLAGS,CXXFLAGS, andLDFLAGSsetup-bazel-ciand pass the relevant variables throughrun-bazel-ci.shvia--action_env/--host_action_envso Bazel build scripts can see the MSVC and UCRT headers on native Windows runssetup-bazel-ciMSVC environment export step so it is easier to audit howvswhere,VsDevCmd.bat, and the filteredGITHUB_ENVexport fit togetheraws-lc-systo skip its standalonememcmpprobe under Bazelwindows-msvcbuild-script environments, which avoids a Windows-native toolchain mismatch that blocked the lint lane before it reached the aspect executionaws-lc-systo prefer its bundledprebuilt-nasmobjects for Bazelwindows-msvcbuild-script runs, which avoids missinggenerated-src/win-x86_64/*.asmrunfiles in the exec-side helper toolchaincodex-rs/linux-sandboxandcodex-rs/corethat the wider native lint coverage surfacedPatches
This PR introduces a large patch stack because the Windows Bazel lint lane currently depends on behavior that upstream dependencies do not provide out of the box in the mixed
windows-gnullvmtarget /windows-msvcexec-toolchain setup.rules_rustpatches look like upstream candidates rather than OpenAI-only policy. Preserving explicit exec-platform constraints, forwarding the right MSVC/UCRT environment into exec-side build scripts, exposing exec-siderustc-devartifacts, and keeping the Windows bootstrap/linker behavior coherent all look like fixes to the Bazel/Rust integration layer itself.aws-lc-syspatches are more tactical. They special-case Bazelwindows-msvcbuild-script environments to avoid amemcmpprobe mismatch and missing NASM runfiles. Those may be harder to upstream as-is because they rely on Bazel-specific detection instead of a general Cargo/build-script contract.rules_rustpatches are less controversial and should be broken out into focused upstream proposals, while theaws-lc-syspatches are more likely to be temporary escape hatches unless that crate wants a more general hook for hermetic build systems.Suggested follow-up plan:
rules_rustdeltas into upstream-sized PRs or issues with minimized repros.aws-lc-syspatches during the next dependency bump and see whether they can be replaced by an upstream fix, a crate upgrade, or a cleaner opt-in mechanism.Verification
./.github/scripts/run-argument-comment-lint-bazel.sh --config=argument-comment-lint --keep_goingRUNNER_OS=Windows ./.github/scripts/run-argument-comment-lint-bazel.sh --nobuild --config=argument-comment-lint --platforms=//:local_windows --keep_goingcargo test -p codex-linux-sandboxcargo test -p codex-core shell_snapshot_testsjust argument-comment-lintReferences