diff --git a/.github/workflows/rust-ci-full.yml b/.github/workflows/rust-ci-full.yml index a09c7fd433..cdedfb49e3 100644 --- a/.github/workflows/rust-ci-full.yml +++ b/.github/workflows/rust-ci-full.yml @@ -116,6 +116,7 @@ jobs: BUILDBUDDY_API_KEY: ${{ secrets.BUILDBUDDY_API_KEY }} shell: bash run: | + bazel_targets="$(./tools/argument-comment-lint/list-bazel-targets.sh)" ./.github/scripts/run-bazel-ci.sh \ -- \ build \ @@ -123,7 +124,7 @@ jobs: --keep_going \ --build_metadata=COMMIT_SHA=${GITHUB_SHA} \ -- \ - //codex-rs/... + ${bazel_targets} - name: Run argument comment lint on codex-rs via packaged wrapper if: ${{ runner.os == 'Windows' }} shell: bash diff --git a/.github/workflows/rust-ci.yml b/.github/workflows/rust-ci.yml index a655b3071e..82591d251b 100644 --- a/.github/workflows/rust-ci.yml +++ b/.github/workflows/rust-ci.yml @@ -176,6 +176,7 @@ jobs: BUILDBUDDY_API_KEY: ${{ secrets.BUILDBUDDY_API_KEY }} shell: bash run: | + bazel_targets="$(./tools/argument-comment-lint/list-bazel-targets.sh)" ./.github/scripts/run-bazel-ci.sh \ -- \ build \ @@ -183,7 +184,7 @@ jobs: --keep_going \ --build_metadata=COMMIT_SHA=${GITHUB_SHA} \ -- \ - //codex-rs/... + ${bazel_targets} - name: Run argument comment lint on codex-rs via packaged wrapper if: ${{ runner.os == 'Windows' }} shell: bash diff --git a/codex-rs/core/src/shell_snapshot_tests.rs b/codex-rs/core/src/shell_snapshot_tests.rs index 90288300ff..ff700ff7a6 100644 --- a/codex-rs/core/src/shell_snapshot_tests.rs +++ b/codex-rs/core/src/shell_snapshot_tests.rs @@ -313,9 +313,15 @@ async fn timed_out_snapshot_shell_is_terminated() -> Result<()> { shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), }; - let err = run_script_with_timeout(&shell, &script, Duration::from_secs(1), true, dir.path()) - .await - .expect_err("snapshot shell should time out"); + let err = run_script_with_timeout( + &shell, + &script, + Duration::from_secs(1), + /*use_login_shell*/ true, + dir.path(), + ) + .await + .expect_err("snapshot shell should time out"); assert!( err.to_string().contains("timed out"), "expected timeout error, got {err:?}" diff --git a/codex-rs/linux-sandbox/src/landlock.rs b/codex-rs/linux-sandbox/src/landlock.rs index 307f956a4d..d257edf933 100644 --- a/codex-rs/linux-sandbox/src/landlock.rs +++ b/codex-rs/linux-sandbox/src/landlock.rs @@ -274,7 +274,10 @@ mod tests { #[test] fn managed_network_enforces_seccomp_even_for_full_network_policy() { assert_eq!( - should_install_network_seccomp(NetworkSandboxPolicy::Enabled, true), + should_install_network_seccomp( + NetworkSandboxPolicy::Enabled, + /*allow_network_for_proxy*/ true + ), true ); } @@ -282,7 +285,10 @@ mod tests { #[test] fn full_network_policy_without_managed_network_skips_seccomp() { assert_eq!( - should_install_network_seccomp(NetworkSandboxPolicy::Enabled, false), + should_install_network_seccomp( + NetworkSandboxPolicy::Enabled, + /*allow_network_for_proxy*/ false + ), false ); } @@ -291,18 +297,22 @@ mod tests { fn restricted_network_policy_always_installs_seccomp() { assert!(should_install_network_seccomp( NetworkSandboxPolicy::Restricted, - false + /*allow_network_for_proxy*/ false )); assert!(should_install_network_seccomp( NetworkSandboxPolicy::Restricted, - true + /*allow_network_for_proxy*/ true )); } #[test] fn managed_proxy_routes_use_proxy_routed_seccomp_mode() { assert_eq!( - network_seccomp_mode(NetworkSandboxPolicy::Enabled, true, true), + network_seccomp_mode( + NetworkSandboxPolicy::Enabled, + /*allow_network_for_proxy*/ true, + /*proxy_routed_network*/ true + ), Some(NetworkSeccompMode::ProxyRouted) ); } @@ -310,7 +320,11 @@ mod tests { #[test] fn restricted_network_without_proxy_routing_uses_restricted_mode() { assert_eq!( - network_seccomp_mode(NetworkSandboxPolicy::Restricted, false, false), + network_seccomp_mode( + NetworkSandboxPolicy::Restricted, + /*allow_network_for_proxy*/ false, + /*proxy_routed_network*/ false + ), Some(NetworkSeccompMode::Restricted) ); } @@ -318,7 +332,11 @@ mod tests { #[test] fn full_network_without_managed_proxy_skips_network_seccomp_mode() { assert_eq!( - network_seccomp_mode(NetworkSandboxPolicy::Enabled, false, false), + network_seccomp_mode( + NetworkSandboxPolicy::Enabled, + /*allow_network_for_proxy*/ false, + /*proxy_routed_network*/ false + ), None ); } diff --git a/codex-rs/linux-sandbox/src/proxy_routing.rs b/codex-rs/linux-sandbox/src/proxy_routing.rs index f57472c8a1..07e1893ee3 100644 --- a/codex-rs/linux-sandbox/src/proxy_routing.rs +++ b/codex-rs/linux-sandbox/src/proxy_routing.rs @@ -718,7 +718,8 @@ mod tests { #[test] fn rewrites_proxy_url_to_local_loopback_port() { let rewritten = - rewrite_proxy_env_value("socks5h://127.0.0.1:8081", 43210).expect("rewritten value"); + rewrite_proxy_env_value("socks5h://127.0.0.1:8081", /*local_port*/ 43210) + .expect("rewritten value"); assert_eq!(rewritten, "socks5h://127.0.0.1:43210"); } diff --git a/justfile b/justfile index a39d2dd6ba..1a1295020f 100644 --- a/justfile +++ b/justfile @@ -74,7 +74,7 @@ bazel-clippy: [no-cd] bazel-argument-comment-lint: - bazel build --config=argument-comment-lint -- //codex-rs/... + bazel build --config=argument-comment-lint -- $(./tools/argument-comment-lint/list-bazel-targets.sh) bazel-remote-test: bazel test --test_tag_filters=-argument-comment-lint //... --config=remote --platforms=//:rbe --keep_going @@ -102,7 +102,7 @@ write-hooks-schema: [no-cd] argument-comment-lint *args: if [ "$#" -eq 0 ]; then \ - bazel build --config=argument-comment-lint -- //codex-rs/...; \ + bazel build --config=argument-comment-lint -- $(./tools/argument-comment-lint/list-bazel-targets.sh); \ else \ ./tools/argument-comment-lint/run-prebuilt-linter.py "$@"; \ fi diff --git a/tools/argument-comment-lint/README.md b/tools/argument-comment-lint/README.md index 78e28f308f..1b4895e325 100644 --- a/tools/argument-comment-lint/README.md +++ b/tools/argument-comment-lint/README.md @@ -129,7 +129,8 @@ Run the lint against `codex-rs` from the repo root: ```bash just argument-comment-lint -bazel build --config=argument-comment-lint -- //codex-rs/... +bazel build --config=argument-comment-lint -- \ + $(./tools/argument-comment-lint/list-bazel-targets.sh) ./tools/argument-comment-lint/run-prebuilt-linter.py -p codex-core just argument-comment-lint -p codex-core ``` @@ -138,10 +139,13 @@ If no package selection is provided, `just argument-comment-lint` now defaults to the Bazel aspect path over `//codex-rs/...`. The Python wrappers remain the package-scoped escape hatch and still default the underlying Cargo invocation to `--all-targets` unless you explicitly narrow the target set, so targeted -wrapper runs cover test-only call sites by default. +wrapper runs cover test-only call sites by default. The Bazel entrypoints use +`tools/argument-comment-lint/list-bazel-targets.sh` to add the internal +manual `*-unit-tests-bin` Rust targets explicitly, so inline `#[cfg(test)]` +call sites are covered without pulling in unrelated manual release targets. -Repo runs also promote `uncommented_anonymous_literal_argument` to an error by -default: +Repo runs also promote `argument_comment_mismatch` and +`uncommented_anonymous_literal_argument` to errors by default: ```bash ./tools/argument-comment-lint/run-prebuilt-linter.py -p codex-core @@ -154,7 +158,7 @@ rustc incremental compilation ICE locally. To override that behavior for an ad hoc run: ```bash -DYLINT_RUSTFLAGS="-A uncommented-anonymous-literal-argument" \ +DYLINT_RUSTFLAGS="-A argument-comment-mismatch -A uncommented-anonymous-literal-argument" \ CARGO_INCREMENTAL=1 \ ./tools/argument-comment-lint/run.py -p codex-core ``` diff --git a/tools/argument-comment-lint/lint_aspect.bzl b/tools/argument-comment-lint/lint_aspect.bzl index 2f0878d9f8..b3de768af1 100644 --- a/tools/argument-comment-lint/lint_aspect.bzl +++ b/tools/argument-comment-lint/lint_aspect.bzl @@ -16,6 +16,7 @@ load( ) _STRICT_LINT_FLAGS = [ + "-Dargument-comment-mismatch", "-Duncommented-anonymous-literal-argument", "-Aunknown-lints", ] diff --git a/tools/argument-comment-lint/list-bazel-targets.sh b/tools/argument-comment-lint/list-bazel-targets.sh new file mode 100755 index 0000000000..cba07f6080 --- /dev/null +++ b/tools/argument-comment-lint/list-bazel-targets.sh @@ -0,0 +1,13 @@ +#!/usr/bin/env bash + +set -euo pipefail + +repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +cd "${repo_root}" + +# Bazel wildcard builds skip manual targets, which misses the internal +# `*-unit-tests-bin` rust_test targets generated by `codex_rust_crate()`. +# Add only those manual rust_test targets explicitly so inline `#[cfg(test)]` +# call sites are linted without pulling in unrelated manual release targets. +printf '%s\n' "//codex-rs/..." +bazel query 'kind("rust_test rule", attr(tags, "manual", //codex-rs/...))' diff --git a/tools/argument-comment-lint/src/bin/argument-comment-lint.rs b/tools/argument-comment-lint/src/bin/argument-comment-lint.rs index 5a40d0efb3..9dfe83b0fb 100644 --- a/tools/argument-comment-lint/src/bin/argument-comment-lint.rs +++ b/tools/argument-comment-lint/src/bin/argument-comment-lint.rs @@ -8,6 +8,11 @@ use std::process::ExitCode; use std::time::SystemTime; use std::time::UNIX_EPOCH; +const STRICT_LINTS: [&str; 2] = [ + "argument-comment-mismatch", + "uncommented-anonymous-literal-argument", +]; + fn main() -> ExitCode { match run() { Ok(code) => code, @@ -85,14 +90,13 @@ fn has_library_selection(args: &[OsString]) -> bool { fn set_default_env(command: &mut Command) -> Result<(), String> { if let Some(flags) = env::var_os("DYLINT_RUSTFLAGS") { let mut flags = flags.to_string_lossy().to_string(); - append_flag_if_missing(&mut flags, "-D uncommented-anonymous-literal-argument"); + for strict_lint in STRICT_LINTS { + append_flag_if_missing(&mut flags, &format!("-D {strict_lint}")); + } append_flag_if_missing(&mut flags, "-A unknown_lints"); command.env("DYLINT_RUSTFLAGS", flags); } else { - command.env( - "DYLINT_RUSTFLAGS", - "-D uncommented-anonymous-literal-argument -A unknown_lints", - ); + command.env("DYLINT_RUSTFLAGS", strict_rustflags()); } if env::var_os("CARGO_INCREMENTAL").is_none() { @@ -108,6 +112,15 @@ fn set_default_env(command: &mut Command) -> Result<(), String> { Ok(()) } +fn strict_rustflags() -> String { + let strict_flags = STRICT_LINTS + .iter() + .map(|lint| format!("-D {lint}")) + .collect::>() + .join(" "); + format!("{strict_flags} -A unknown_lints") +} + fn append_flag_if_missing(flags: &mut String, flag: &str) { if flags.contains(flag) { return; @@ -253,6 +266,7 @@ fn exit_code_from_status(code: Option) -> ExitCode { #[cfg(test)] mod tests { use super::normalize_nightly_library_filename; + use super::strict_rustflags; use std::path::Path; #[test] @@ -276,4 +290,12 @@ mod tests { None ); } + + #[test] + fn strict_rustflags_promotes_both_enforced_lints() { + assert_eq!( + strict_rustflags(), + "-D argument-comment-mismatch -D uncommented-anonymous-literal-argument -A unknown_lints" + ); + } } diff --git a/tools/argument-comment-lint/test_wrapper_common.py b/tools/argument-comment-lint/test_wrapper_common.py index 3da6da604b..dbea24b534 100644 --- a/tools/argument-comment-lint/test_wrapper_common.py +++ b/tools/argument-comment-lint/test_wrapper_common.py @@ -103,6 +103,19 @@ class WrapperCommonTest(unittest.TestCase): ], ) + def test_default_lint_env_promotes_both_strict_lints(self) -> None: + env: dict[str, str] = {} + + wrapper_common.set_default_lint_env(env) + + self.assertEqual( + env["DYLINT_RUSTFLAGS"], + "-D argument-comment-mismatch " + "-D uncommented-anonymous-literal-argument " + "-A unknown_lints", + ) + self.assertEqual(env["CARGO_INCREMENTAL"], "0") + if __name__ == "__main__": unittest.main() diff --git a/tools/argument-comment-lint/wrapper_common.py b/tools/argument-comment-lint/wrapper_common.py index 75e03ecfbe..2473029b8e 100644 --- a/tools/argument-comment-lint/wrapper_common.py +++ b/tools/argument-comment-lint/wrapper_common.py @@ -13,7 +13,10 @@ import sys import tempfile from typing import MutableMapping, Sequence -STRICT_LINT = "uncommented-anonymous-literal-argument" +STRICT_LINTS = [ + "argument-comment-mismatch", + "uncommented-anonymous-literal-argument", +] NOISE_LINT = "unknown_lints" TOOLCHAIN_CHANNEL = "nightly-2025-09-18" @@ -129,7 +132,8 @@ def append_env_flag(env: MutableMapping[str, str], key: str, flag: str) -> None: def set_default_lint_env(env: MutableMapping[str, str]) -> None: - append_env_flag(env, "DYLINT_RUSTFLAGS", f"-D {STRICT_LINT}") + for strict_lint in STRICT_LINTS: + append_env_flag(env, "DYLINT_RUSTFLAGS", f"-D {strict_lint}") append_env_flag(env, "DYLINT_RUSTFLAGS", f"-A {NOISE_LINT}") if not env.get("CARGO_INCREMENTAL"): env["CARGO_INCREMENTAL"] = "0"