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-07-01 00:31:56 +08:00 · 2026-03-30 11:59:50 -07:00
parent 258ba436f1
commit 9313c49e4c
12 changed files with 111 additions and 27 deletions
@@ -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
@@ -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
@@ -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:?}"
@@ -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
        );
    }
@@ -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");
    }

@@ -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
@@ -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
 ```
@@ -16,6 +16,7 @@ load(
 )

 _STRICT_LINT_FLAGS = [
+    "-Dargument-comment-mismatch",
    "-Duncommented-anonymous-literal-argument",
    "-Aunknown-lints",
 ]
@@ -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/...))'
@@ -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::<Vec<_>>()
+        .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<i32>) -> 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"
+        );
+    }
 }
@@ -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()
@@ -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"