mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
dev
16 Commits
-
test: run app-server integration tests under Wine (#29788)
## Why Made a mistake when carving #29746 out of my local changes and the test was missing from the build graph. Oops! ## What Enable the app-server Wine exec test target. Remove the `manual` tag from generated Wine-exec test variants so wildcard Bazel test invocations select them. Refactor the smoke test to ensure it passes with current Windows support.
Adam Perry @ OpenAI ·
2026-06-24 05:23:29 +00:00 -
build: run buildifier from just fmt (#28125)
## Intent Keep Bazel and Starlark files consistently formatted without requiring contributors to install or version buildifier themselves. ## Implementation - Add a SHA-256-pinned, cross-platform DotSlash manifest for buildifier v8.5.1. - Run buildifier from the shared `just fmt` and `just fmt-check` driver, with Windows-safe explicit DotSlash invocation. - Provision DotSlash in formatting CI and contributor devcontainers, and document the source-build prerequisite. - Apply the initial mechanical buildifier formatting baseline.
2026-06-13 21:43:39 -07:00 -
[codex] Add hermetic Wine exec-server test (#27937)
## Why We want to make it possible for an app-server orchestrator on one OS to control an exec-server on another host running a different OS. In practice this kinda already works if you get lucky and the two hosts have the same path format, but we mangle quite a lot of operations if either end is Windows. This test starts exercising that interaction, although right now the initial bootstrap fails. Future changes will expand the test's assertions to match improved support. ## What Stacked on #27964. This adds a small Windows exec-server fixture and a Linux protocol smoke test using the reusable Wine harness, covering Windows environment discovery, non-TTY `cmd.exe` execution, output, exit status, and working directory. Once we've got the full codex binary cross-building under Bazel we could consider moving to the real binary instead of the stripped down exec-server-only binary used here.
2026-06-12 20:20:23 -07:00 -
lint: allow self-documenting builder arguments (#27507)
Builder-style setters often repeat the setting name in both the method and its sole argument. Calls such as `.enabled(false)` are already self-documenting, so requiring `/*enabled*/` adds noise without clarifying the call. ## What changed - Exempt a method's sole non-self argument when its resolved parameter name matches the method name. - Continue validating any explicit argument comment against the resolved parameter name. - Continue requiring comments when method and parameter names differ or when a method has multiple non-self arguments. - Document the exception in `AGENTS.md` and the lint's own behavior documentation. ## Examples Before this change we'd need redundant comments like this: ```rust builder.enabled(/*false*/ false); builder.retry_count(/*retry_count*/ 3); builder.base_url(/*base_url*/ None); ``` Now can be written like this: ```rust builder.enabled(false); builder.retry_count(3); builder.base_url(None); ``` Still disallowed: ```rust client.set_flag(true); // Method name does not match parameter `enabled`. options.enabled(false, /*retry_count*/ 3); // More than one non-self argument. options.enabled(/*value*/ false); // Explicit comment does not match `enabled`. ``` ## Validation Added UI coverage for boolean, numeric, and `None` builder arguments, multi-argument methods, and explicit comment mismatches. Ran `rustup run nightly-2025-09-18 cargo test` in `tools/argument-comment-lint`.
2026-06-11 10:24:42 -07:00 -
fix(tui): preserve wrapped prose beside URLs (#21760)
## Why Mixed prose lines that contained URLs started taking the URL-preserving wrapping path, but that path could split ordinary words mid-token. A follow-up issue remained in scrollback insertion: when already-rendered indented rows were wrapped again, continuation rows could lose their margin and fall back to terminal hard wrapping. Together those bugs made normal Markdown output look broken around links, lists, blockquotes, and indented content. Separately, the local argument-comment lint wrappers failed under environments that set `PYTHONSAFEPATH=1`, because Python no longer adds the script directory to `sys.path` automatically. That prevented the lint from reaching Rust callsites at all. <img width="1778" height="1558" alt="CleanShot 2026-05-09 at 11 51 38" src="https://github.com/user-attachments/assets/9274d150-1757-4f1a-89ac-5bdc9997d8cb" /> ## What Changed - Preserve URL tokens without turning every neighboring prose word into a character-level split point. - Add a mixed URL/prose wrapper that keeps ordinary words whole, preserves leading whitespace, and re-splits long non-URL tokens against the actual width available on continuation rows. - Reuse a rendered history row's leading whitespace as the continuation indent when scrollback insertion has to pre-wrap it again. - Add regression coverage for markdown wrapping, history-cell rendering, scrollback continuation margins, leading-indent width accounting, and continuation-row re-splitting. - Make both argument-comment lint entrypoints explicitly add their own directory to `sys.path`, so sibling imports still work when `PYTHONSAFEPATH=1`. ## How to Test 1. Start Codex and render a long Markdown response that mixes prose with inline links, blockquotes, lists, and indented code-like text. 2. Confirm that ordinary words next to links stay whole instead of breaking mid-word. 3. Resize or replay the transcript and confirm wrapped continuation rows keep their expected left margin for blockquotes, lists, and indented content. 4. Run the source argument-comment lint from a shell with `PYTHONSAFEPATH=1` and confirm it starts normally instead of failing to import `wrapper_common`. Targeted tests: - `cargo test -p codex-tui mixed_line --lib` - `cargo test -p codex-tui preserves_prefix_on_wrapped_rows --lib` - `cargo test -p codex-tui agent_markdown_cell_does_not_split_words_after_inline_markdown --lib` - `cargo test -p codex-tui mixed_url_markdown_wraps_prose_without_splitting_words_snapshot --lib` - `python3 tools/argument-comment-lint/test_wrapper_common.py` - `just argument-comment-lint-from-source -p codex-tui -- --lib` Notes: - `cargo test -p codex-tui` currently reaches the new tests successfully, then still aborts in the pre-existing `tests::fork_last_filters_latest_session_by_cwd_unless_show_all` stack-overflow failure.
Felipe Coury ·
2026-05-09 13:58:10 -03:00 -
Ensure all mentions of cargo-install are --locked (#21592)
There's already a preference for this in the codebase, but a few of them have drifted away. Generally `--locked` is preferred to reduce exposure to supply-chain attacks (and just generally improve reproducibility). In an ideal world these dependencies would maybe even be pinned to versions but Cargo is kinda bad at that for devtools. Still better to use --locked than not.
Aria Desires ·
2026-05-07 15:30:37 -07:00 -
ci: cross-compile Windows Bazel tests (#20585)
## Status This is the Bazel PR-CI cross-compilation follow-up to #20485. It is intentionally split from the Cargo/cargo-xwin release-build PoC so #20485 can stay as the historical release-build exploration. The unrelated async-utils test cleanup has been moved to #20686, so this PR is focused on the Windows Bazel CI path. The intended tradeoff is now explicit in `.github/workflows/bazel.yml`: pull requests get the fast Windows cross-compiled Bazel test leg, while post-merge pushes to `main` run both that fast cross leg and a fully native Windows Bazel test leg. The native main-only job keeps full V8/code-mode coverage and gets a 40-minute timeout because it is less latency-sensitive than PR CI. All other Bazel jobs remain at 30 minutes. ## Why Windows Bazel PR CI currently does the expensive part of the build on Windows. A native Windows Bazel test job on `main` completed in about 28m12s, leaving very little headroom under the 30-minute job timeout and making Windows the slowest PR signal. #20485 showed that Windows cross-compilation can be materially faster for Cargo release builds, but PR CI needs Bazel because Bazel owns our test sharding, flaky-test retries, and integration-test layout. This PR applies the same high-level shape we already use for macOS Bazel CI: compile with remote Linux execution, then run platform-specific tests on the platform runner. The compromise is deliberately signal-aware: code-mode/V8 changes are rare enough that PR CI can accept losing the direct V8/code-mode smoke-test signal temporarily, while `main` still runs the native Windows job post-merge to catch that class of regression. A follow-up PR should investigate making the cross-built Windows gnullvm V8 archive pass the direct V8/code-mode tests so this tradeoff can eventually go away. ## What Changed - Adds a `ci-windows-cross` Bazel config that targets `x86_64-pc-windows-gnullvm`, uses Linux RBE for build actions, and keeps `TestRunner` actions local on the Windows runner. - Adds explicit Windows platform definitions for `windows_x86_64_gnullvm`, `windows_x86_64_msvc`, and a bridge toolchain that lets gnullvm test targets execute under the Windows MSVC host platform. - Updates the Windows Bazel PR test leg to opt into the cross-compile path via `--windows-cross-compile` and `--remote-download-toplevel`. - Adds a `test-windows-native-main` job that runs only for `push` events on `refs/heads/main`, uses the native Windows Bazel path, includes V8/code-mode smoke tests, and has `timeout-minutes: 40`. - Keeps fork/community PRs without `BUILDBUDDY_API_KEY` on the previous local Windows MSVC-host fallback, including `--host_platform=//:local_windows_msvc` and `--jobs=8`. - Preserves the existing integration-test shape on non-gnullvm platforms, while generating Windows-cross wrapper targets only for `windows_gnullvm`. - Resolves `CARGO_BIN_EXE_*` values from runfiles at test runtime, avoiding hard-coded Cargo paths and duplicate test runfiles. - Extends the V8 Bazel patches enough for the `x86_64-pc-windows-gnullvm` target and Linux remote execution path. - Makes the Windows sandbox test cwd derive from `INSTA_WORKSPACE_ROOT` at runtime when Bazel provides it, because cross-compiled binaries may contain Linux compile-time paths. - Keeps the direct V8/code-mode unit smoke tests out of the Windows cross PR path for now while native Windows CI continues to cover them post-merge. ## Command Shape The fast Windows PR test leg invokes the normal Bazel CI wrapper like this: ```shell ./.github/scripts/run-bazel-ci.sh \ --print-failed-action-summary \ --print-failed-test-logs \ --windows-cross-compile \ --remote-download-toplevel \ -- \ test \ --test_tag_filters=-argument-comment-lint \ --test_verbose_timeout_warnings \ --build_metadata=COMMIT_SHA=${GITHUB_SHA} \ -- \ //... \ -//third_party/v8:all \ -//codex-rs/code-mode:code-mode-unit-tests \ -//codex-rs/v8-poc:v8-poc-unit-tests ``` With the BuildBuddy secret available on Windows, the wrapper selects `--config=ci-windows-cross` and appends the important Windows-cross overrides after rc expansion: ```shell --host_platform=//:rbe --shell_executable=/bin/bash --action_env=PATH=/usr/bin:/bin --host_action_env=PATH=/usr/bin:/bin --test_env=PATH=${CODEX_BAZEL_WINDOWS_PATH} ``` The native post-merge Windows job intentionally omits `--windows-cross-compile` and does not exclude the V8/code-mode unit targets: ```shell ./.github/scripts/run-bazel-ci.sh \ --print-failed-action-summary \ --print-failed-test-logs \ -- \ test \ --test_tag_filters=-argument-comment-lint \ --test_verbose_timeout_warnings \ --build_metadata=COMMIT_SHA=${GITHUB_SHA} \ --build_metadata=TAG_windows_native_main=true \ -- \ //... \ -//third_party/v8:all ``` ## Research Notes The existing macOS Bazel CI config already uses the model we want here: build actions run remotely with `--strategy=remote`, but `TestRunner` actions execute on the macOS runner. This PR mirrors that pattern for Windows with `--strategy=TestRunner=local`. The important Bazel detail is that `rules_rs` is already targeting `x86_64-pc-windows-gnullvm` for Windows Bazel PR tests. This PR changes where the build actions execute; it does not switch the Bazel PR test target to Cargo, `cargo-nextest`, or the MSVC release target. Cargo release builds differ from this Bazel path for V8: the normal Windows Cargo release target is MSVC, and `rusty_v8` publishes prebuilt Windows MSVC `.lib.gz` archives. The Bazel PR path targets `windows-gnullvm`; `rusty_v8` does not publish a prebuilt Windows GNU/gnullvm archive, so this PR builds that archive in-tree. That Linux-RBE-built gnullvm archive currently crashes in direct V8/code-mode smoke tests, which is why the workflow keeps native Windows coverage on `main`. The less obvious Bazel detail is test wrapper selection. Bazel chooses the Windows test wrapper (`tw.exe`) from the test action execution platform, not merely from the Rust target triple. The outer `workspace_root_test` therefore declares the default test toolchain and uses the bridge toolchain above so the test action executes on Windows while its inner Rust binary is built for gnullvm. The V8 investigation exposed a Windows-client gotcha: even when an action execution platform is Linux RBE, Bazel can still derive the genrule shell path from the Windows client. That produced remote commands trying to run `C:\Program Files\Git\usr\bin\bash.exe` on Linux workers. The wrapper now passes `--shell_executable=/bin/bash` with `--host_platform=//:rbe` for the Windows cross path. The same Windows-client/Linux-RBE boundary also affected `third_party/v8:binding_cc`: a multiline genrule command can carry CRLF line endings into Linux remote bash, which failed as `$'\r'`. That genrule now keeps the `sed` command on one physical shell line while using an explicit Starlark join so the shell arguments stay readable. ## Verification Local checks included: ```shell bash -n .github/scripts/run-bazel-ci.sh bash -n workspace_root_test_launcher.sh.tpl ruby -e "require %q{yaml}; YAML.load_file(%q{.github/workflows/bazel.yml}); puts %q{ok}" RUNNER_OS=Linux ./scripts/list-bazel-clippy-targets.sh RUNNER_OS=Windows ./scripts/list-bazel-clippy-targets.sh RUNNER_OS=Linux ./tools/argument-comment-lint/list-bazel-targets.sh RUNNER_OS=Windows ./tools/argument-comment-lint/list-bazel-targets.sh ``` The Linux clippy and argument-comment target lists contain zero `*-windows-cross-bin` labels, while the Windows lists still include 47 Windows-cross internal test binaries. CI evidence: - Baseline native Windows Bazel test on `main`: success in about 28m12s, https://github.com/openai/codex/actions/runs/25206257208/job/73907325959 - Green Windows-cross Bazel run on the split PR before adding the main-only native leg: Windows test 9m16s, Windows release verify 5m10s, Windows clippy 4m43s, https://github.com/openai/codex/actions/runs/25231890068 - The latest SHA adds the explicit PR-vs-main tradeoff in `bazel.yml`; CI is rerunning on that focused diff. ## Follow-Up A subsequent PR should investigate making a cross-built Windows binary work with V8/code-mode enabled. Likely options are either making the Linux-RBE-built `windows-gnullvm` V8 archive correct at runtime, or evaluating whether a Bazel MSVC target/toolchain can reuse the same prebuilt MSVC `rusty_v8` archive shape that Cargo release builds already use.
Michael Bolin ·
2026-05-01 15:55:28 -07:00 -
ci: reuse Bazel CI startup for target-discovery queries (#19232)
## Why A rerun of the Windows Bazel clippy job after [#19161](https://github.com/openai/codex/pull/19161) had exactly the cache behavior we wanted in BuildBuddy: zero action-cache misses. Even so, the GitHub job still took a little over five minutes. The problem was that the job was paying for two separate Bazel startup paths: 1. a `bazel query` to discover extra lint targets 2. the real `bazel build --config=clippy ...` invocation On Windows, that query was bypassing the CI Bazel wrapper, so it did not reuse the same `--output_user_root`, CI config, or remote-cache setup as the real build. In practice that meant the rerun could still cold-start a separate Bazel server before the actual clippy build even began. ## What - add `.github/scripts/run-bazel-query-ci.sh` to run CI-side Bazel queries with the same startup and cache-related flags as the main Bazel command - switch `scripts/list-bazel-clippy-targets.sh` to use that helper for manual `rust_test` target discovery - switch `tools/argument-comment-lint/list-bazel-targets.sh` to use the same helper - simplify `.github/scripts/run-argument-comment-lint-bazel.sh` so its Windows-only query path also goes through the shared helper This keeps the target-discovery queries aligned with the later build/test invocation instead of treating them as a separate cold Bazel session. ## Verification - `bash -n .github/scripts/run-bazel-query-ci.sh` - `bash -n scripts/list-bazel-clippy-targets.sh` - `bash -n tools/argument-comment-lint/list-bazel-targets.sh` - `bash -n .github/scripts/run-argument-comment-lint-bazel.sh` - mocked a Windows invocation of `run-bazel-query-ci.sh` and verified it forwards `--output_user_root`, `--config=ci-windows`, the BuildBuddy auth header, and the repository cache flags ## Docs No documentation updates are needed.
2026-04-23 23:26:17 -07:00 -
fix: close Bazel argument-comment-lint CI gaps (#16253)
## Why The Bazel-backed `argument-comment-lint` CI path had two gaps: - Bazel wildcard target expansion skipped inline unit-test crates from `src/` modules because the generated `*-unit-tests-bin` `rust_test` targets are tagged `manual`. - `argument-comment-mismatch` was still only a warning in the Bazel and packaged-wrapper entrypoints, so a typoed `/*param_name*/` comment could still pass CI even when the lint detected it. That left CI blind to real linux-sandbox examples, including the missing `/*local_port*/` comment in `codex-rs/linux-sandbox/src/proxy_routing.rs` and typoed argument comments in `codex-rs/linux-sandbox/src/landlock.rs`. ## What Changed - Added `tools/argument-comment-lint/list-bazel-targets.sh` so Bazel lint runs cover `//codex-rs/...` plus the manual `rust_test` `*-unit-tests-bin` targets. - Updated `just argument-comment-lint`, `rust-ci.yml`, and `rust-ci-full.yml` to use that helper. - Promoted both `argument-comment-mismatch` and `uncommented-anonymous-literal-argument` to errors in every strict entrypoint: - `tools/argument-comment-lint/lint_aspect.bzl` - `tools/argument-comment-lint/src/bin/argument-comment-lint.rs` - `tools/argument-comment-lint/wrapper_common.py` - Added wrapper/bin coverage for the stricter lint flags and documented the behavior in `tools/argument-comment-lint/README.md`. - Fixed the now-covered callsites in `codex-rs/linux-sandbox/src/proxy_routing.rs`, `codex-rs/linux-sandbox/src/landlock.rs`, and `codex-rs/core/src/shell_snapshot_tests.rs`. This keeps the Bazel target expansion narrow while making the Bazel and prebuilt-linter paths enforce the same strict lint set. ## Verification - `python3 -m unittest discover -s tools/argument-comment-lint -p 'test_*.py'` - `cargo +nightly-2025-09-18 test --manifest-path tools/argument-comment-lint/Cargo.toml` - `just argument-comment-lint`
2026-03-30 11:59:50 -07:00 -
build: migrate argument-comment-lint to a native Bazel aspect (#16106)
## 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
2026-03-28 12:41:56 -07:00 -
refactor: rewrite argument-comment lint wrappers in Python (#16063)
## Why The `argument-comment-lint` entrypoints had grown into two shell wrappers with duplicated parsing, environment setup, and Cargo forwarding logic. The recent `--` separator regression was a good example of the problem: the behavior was subtle, easy to break, and hard to verify. This change rewrites those wrappers in Python so the control flow is easier to follow, the shared behavior lives in one place, and the tricky argument/defaulting paths have direct test coverage. ## What changed - replaced `tools/argument-comment-lint/run.sh` and `tools/argument-comment-lint/run-prebuilt-linter.sh` with Python entrypoints: `run.py` and `run-prebuilt-linter.py` - moved shared wrapper behavior into `tools/argument-comment-lint/wrapper_common.py`, including: - splitting lint args from forwarded Cargo args after `--` - defaulting repo runs to `--manifest-path codex-rs/Cargo.toml --workspace --no-deps` - defaulting non-`--fix` runs to `--all-targets` unless the caller explicitly narrows the target set - setting repo defaults for `DYLINT_RUSTFLAGS` and `CARGO_INCREMENTAL` - kept the prebuilt wrapper thin: it still just resolves the packaged DotSlash entrypoint, keeps `rustup` shims first on `PATH`, infers `RUSTUP_HOME` when needed, and then launches the packaged `cargo-dylint` path - updated `justfile`, `rust-ci.yml`, and `tools/argument-comment-lint/README.md` to use the Python entrypoints - updated `rust-ci` so the package job runs Python syntax checks plus the new wrapper unit tests, and the OS-specific lint jobs invoke the wrappers through an explicit Python interpreter This is a follow-up to #16054: it keeps the current lint semantics while making the wrapper logic maintainable enough to iterate on safely. ## Validation - `python3 -m py_compile tools/argument-comment-lint/wrapper_common.py tools/argument-comment-lint/run.py tools/argument-comment-lint/run-prebuilt-linter.py tools/argument-comment-lint/test_wrapper_common.py` - `python3 -m unittest discover -s tools/argument-comment-lint -p 'test_*.py'` - `python3 ./tools/argument-comment-lint/run-prebuilt-linter.py -p codex-terminal-detection -- --lib` - `python3 ./tools/argument-comment-lint/run.py -p codex-terminal-detection -- --lib`
2026-03-27 19:42:30 -07:00 -
chore: clean up argument-comment lint and roll out all-target CI on macOS (#16054)
## Why `argument-comment-lint` was green in CI even though the repo still had many uncommented literal arguments. The main gap was target coverage: the repo wrapper did not force Cargo to inspect test-only call sites, so examples like the `latest_session_lookup_params(true, ...)` tests in `codex-rs/tui_app_server/src/lib.rs` never entered the blocking CI path. This change cleans up the existing backlog, makes the default repo lint path cover all Cargo targets, and starts rolling that stricter CI enforcement out on the platform where it is currently validated. ## What changed - mechanically fixed existing `argument-comment-lint` violations across the `codex-rs` workspace, including tests, examples, and benches - updated `tools/argument-comment-lint/run-prebuilt-linter.sh` and `tools/argument-comment-lint/run.sh` so non-`--fix` runs default to `--all-targets` unless the caller explicitly narrows the target set - fixed both wrappers so forwarded cargo arguments after `--` are preserved with a single separator - documented the new default behavior in `tools/argument-comment-lint/README.md` - updated `rust-ci` so the macOS lint lane keeps the plain wrapper invocation and therefore enforces `--all-targets`, while Linux and Windows temporarily pass `-- --lib --bins` That temporary CI split keeps the stricter all-targets check where it is already cleaned up, while leaving room to finish the remaining Linux- and Windows-specific target-gated cleanup before enabling `--all-targets` on those runners. The Linux and Windows failures on the intermediate revision were caused by the wrapper forwarding bug, not by additional lint findings in those lanes. ## Validation - `bash -n tools/argument-comment-lint/run.sh` - `bash -n tools/argument-comment-lint/run-prebuilt-linter.sh` - shell-level wrapper forwarding check for `-- --lib --bins` - shell-level wrapper forwarding check for `-- --tests` - `just argument-comment-lint` - `cargo test` in `tools/argument-comment-lint` - `cargo test -p codex-terminal-detection` ## Follow-up - Clean up remaining Linux-only target-gated callsites, then switch the Linux lint lane back to the plain wrapper invocation. - Clean up remaining Windows-only target-gated callsites, then switch the Windows lint lane back to the plain wrapper invocation.
2026-03-27 19:00:44 -07:00 -
Use released DotSlash package for argument-comment lint (#15199)
## Why The argument-comment lint now has a packaged DotSlash artifact from [#15198](https://github.com/openai/codex/pull/15198), so the normal repo lint path should use that released payload instead of rebuilding the lint from source every time. That keeps `just clippy` and CI aligned with the shipped artifact while preserving a separate source-build path for people actively hacking on the lint crate. The current alpha package also exposed two integration wrinkles that the repo-side prebuilt wrapper needs to smooth over: - the bundled Dylint library filename includes the host triple, for example `@nightly-2025-09-18-aarch64-apple-darwin`, and Dylint derives `RUSTUP_TOOLCHAIN` from that filename - on Windows, Dylint's driver path also expects `RUSTUP_HOME` to be present in the environment Without those adjustments, the prebuilt CI jobs fail during `cargo metadata` or driver setup. This change makes the checked-in prebuilt wrapper normalize the packaged library name to the plain `nightly-2025-09-18` channel before invoking `cargo-dylint`, and it teaches both the wrapper and the packaged runner source to infer `RUSTUP_HOME` from `rustup show home` when the environment does not already provide it. After the prebuilt Windows lint job started running successfully, it also surfaced a handful of existing anonymous literal callsites in `windows-sandbox-rs`. This PR now annotates those callsites so the new cross-platform lint job is green on the current tree. ## What Changed - checked in the current `tools/argument-comment-lint/argument-comment-lint` DotSlash manifest - kept `tools/argument-comment-lint/run.sh` as the source-build wrapper for lint development - added `tools/argument-comment-lint/run-prebuilt-linter.sh` as the normal enforcement path, using the checked-in DotSlash package and bundled `cargo-dylint` - updated `just clippy` and `just argument-comment-lint` to use the prebuilt wrapper - split `.github/workflows/rust-ci.yml` so source-package checks live in a dedicated `argument_comment_lint_package` job, while the released lint runs in an `argument_comment_lint_prebuilt` matrix on Linux, macOS, and Windows - kept the pinned `nightly-2025-09-18` toolchain install in the prebuilt CI matrix, since the prebuilt package still relies on rustup-provided toolchain components - updated `tools/argument-comment-lint/run-prebuilt-linter.sh` to normalize host-qualified nightly library filenames, keep the `rustup` shim directory ahead of direct toolchain `cargo` binaries, and export `RUSTUP_HOME` when needed for Windows Dylint driver setup - updated `tools/argument-comment-lint/src/bin/argument-comment-lint.rs` so future published DotSlash artifacts apply the same nightly-filename normalization and `RUSTUP_HOME` inference internally - fixed the remaining Windows lint violations in `codex-rs/windows-sandbox-rs` by adding the required `/*param*/` comments at the reported callsites - documented the checked-in DotSlash file, wrapper split, archive layout, nightly prerequisite, and Windows `RUSTUP_HOME` requirement in `tools/argument-comment-lint/README.md`
2026-03-20 03:19:22 +00:00 -
Publish runnable DotSlash package for argument-comment lint (#15198)
## Why To date, the argument-comment linter introduced in https://github.com/openai/codex/pull/14651 had to be built from source to run, which can be a bit slow (both for local dev and when it is run in CI). Because of the potential slowness, I did not wire it up to run as part of `just clippy` or anything like that. As a result, I have seen a number of occasions where folks put up PRs that violate the lint, see it fail in CI, and then have to put up their PR again. The goal of this PR is to pre-build a runnable version of the linter and then make it available via a DotSlash file. Once it is available, I will update `just clippy` and other touchpoints to make it a natural part of the dev cycle so lint violations should get flagged _before_ putting up a PR for review. To get things started, we will build the DotSlash file as part of an alpha release. Though I don't expect the linter to change often, so I'll probably change this to only build as part of mainline releases once we have a working DotSlash file. (Ultimately, we should probably move the linter into its own repo so it can have its own release cycle.) ## What Changed - add a reusable `rust-release-argument-comment-lint.yml` workflow that builds host-specific archives for macOS arm64, Linux arm64/x64, and Windows x64 - wire `rust-release.yml` to publish the `argument-comment-lint` DotSlash manifest on all releases for now, including alpha tags - package a runnable layout instead of a bare library The Unix archive layout is: ```text argument-comment-lint/ bin/ argument-comment-lint cargo-dylint lib/ libargument_comment_lint@nightly-2025-09-18-<target>.dylib|so ``` On Windows the same layout is published as a `.zip`, with `.exe` and `.dll` filenames instead. DotSlash resolves the package entrypoint to `argument-comment-lint/bin/argument-comment-lint`. That runner finds the sibling bundled `cargo-dylint` binary plus the single packaged Dylint library under `lib/`, then invokes `cargo-dylint dylint --lib-path <that-library>` with the repo's default lint settings.
2026-03-19 18:59:02 +00:00 -
Apply argument comment lint across codex-rs (#14652)
## Why Once the repo-local lint exists, `codex-rs` needs to follow the checked-in convention and CI needs to keep it from drifting. This commit applies the fallback `/*param*/` style consistently across existing positional literal call sites without changing those APIs. The longer-term preference is still to avoid APIs that require comments by choosing clearer parameter types and call shapes. This PR is intentionally the mechanical follow-through for the places where the existing signatures stay in place. After rebasing onto newer `main`, the rollout also had to cover newly introduced `tui_app_server` call sites. That made it clear the first cut of the CI job was too expensive for the common path: it was spending almost as much time installing `cargo-dylint` and re-testing the lint crate as a representative test job spends running product tests. The CI update keeps the full workspace enforcement but trims that extra overhead from ordinary `codex-rs` PRs. ## What changed - keep a dedicated `argument_comment_lint` job in `rust-ci` - mechanically annotate remaining opaque positional literals across `codex-rs` with exact `/*param*/` comments, including the rebased `tui_app_server` call sites that now fall under the lint - keep the checked-in style aligned with the lint policy by using `/*param*/` and leaving string and char literals uncommented - cache `cargo-dylint`, `dylint-link`, and the relevant Cargo registry/git metadata in the lint job - split changed-path detection so the lint crate's own `cargo test` step runs only when `tools/argument-comment-lint/*` or `rust-ci.yml` changes - continue to run the repo wrapper over the `codex-rs` workspace, so product-code enforcement is unchanged Most of the code changes in this commit are intentionally mechanical comment rewrites or insertions driven by the lint itself. ## Verification - `./tools/argument-comment-lint/run.sh --workspace` - `cargo test -p codex-tui-app-server -p codex-tui` - parsed `.github/workflows/rust-ci.yml` locally with PyYAML --- * -> #14652 * #14651
2026-03-16 16:48:15 -07:00 -
2026-03-14 08:18:04 -07:00