From 2c2ed51876044dd4646aec912d1b4b248836c5bd Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 17 Apr 2026 11:19:58 -0700 Subject: [PATCH] ci: make Windows Bazel clippy catch core test imports (#18350) ## Why Unused imports in `core/tests/suite/unified_exec.rs` in the Windows build were not caught by Bazel CI on https://github.com/openai/codex/pull/18096. I spot-checked https://github.com/openai/codex/actions/workflows/rust-ci-full.yml?query=branch%3Amain and noticed that builds were consistently red. This revealed that our Cargo builds _were_ properly catching these issues, identifying a Windows-specific coverage hole in the Bazel clippy job. The Windows Bazel clippy job uses `--skip_incompatible_explicit_targets` so it can lint a broad target set without failing immediately on targets that are genuinely incompatible with Windows. However, with the default Windows host platform, `rust_test` targets such as `//codex-rs/core:core-all-test` could be skipped before the clippy aspect reached their integration-test modules. As a result, the imports in `core/tests/suite/unified_exec.rs` were not being linted by the Windows Bazel clippy job at all. The clippy diagnostic that Windows Bazel should have surfaced was: ```text error: unused import: `codex_config::Constrained` --> core\tests\suite\unified_exec.rs:8:5 | 8 | use codex_config::Constrained; | ^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D unused-imports` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(unused_imports)]` error: unused import: `codex_protocol::permissions::FileSystemAccessMode` --> core\tests\suite\unified_exec.rs:11:5 | 11 | use codex_protocol::permissions::FileSystemAccessMode; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: unused import: `codex_protocol::permissions::FileSystemPath` --> core\tests\suite\unified_exec.rs:12:5 | 12 | use codex_protocol::permissions::FileSystemPath; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: unused import: `codex_protocol::permissions::FileSystemSandboxEntry` --> core\tests\suite\unified_exec.rs:13:5 | 13 | use codex_protocol::permissions::FileSystemSandboxEntry; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: unused import: `codex_protocol::permissions::FileSystemSandboxPolicy` --> core\tests\suite\unified_exec.rs:14:5 | 14 | use codex_protocol::permissions::FileSystemSandboxPolicy; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ``` ## What changed - Run the Windows Bazel clippy job with the MSVC host platform via `--windows-msvc-host-platform`, matching the Windows Bazel test job. This keeps `--skip_incompatible_explicit_targets` while ensuring Windows `rust_test` targets such as `//codex-rs/core:core-all-test` are still linted. - Remove the unused imports from `core/tests/suite/unified_exec.rs`. - Add `--print-failed-action-summary` to `.github/scripts/run-bazel-ci.sh` so Bazel action failures can be summarized after the build exits. ## Failure reporting Once the coverage issue was fixed, an intentionally reintroduced unused import made the Windows Bazel clippy job fail as expected. That exposed a separate usability problem: because the job keeps `--keep_going`, the top-level Bazel output could still end with: ```text ERROR: Build did NOT complete successfully FAILED: ``` without the underlying rustc/clippy diagnostic being visible in the obvious part of the GitHub Actions log. To keep `--keep_going` while making failures actionable, the wrapper now scans the captured Bazel console output for failed actions and prints the matching rustc/clippy diagnostic block. When a diagnostic block is found, it is emitted both as a GitHub `::error` annotation and as plain expanded log output, rather than being hidden in a collapsed group. ## Verification To validate the CI path, I intentionally introduced an unused import in `core/tests/suite/unified_exec.rs`. The Windows Bazel clippy job failed as expected, confirming that the integration-test module is now covered by Bazel clippy. The same failure also verified that the wrapper surfaces the matching clippy diagnostics directly in the Actions output. --- .github/scripts/run-bazel-ci.sh | 105 ++++++++++++++++++++-- .github/workflows/bazel.yml | 12 ++- codex-rs/core/tests/suite/unified_exec.rs | 11 +-- 3 files changed, 115 insertions(+), 13 deletions(-) diff --git a/.github/scripts/run-bazel-ci.sh b/.github/scripts/run-bazel-ci.sh index a1d42a057..e5376a812 100755 --- a/.github/scripts/run-bazel-ci.sh +++ b/.github/scripts/run-bazel-ci.sh @@ -3,6 +3,7 @@ set -euo pipefail print_failed_bazel_test_logs=0 +print_failed_bazel_action_summary=0 use_node_test_env=0 remote_download_toplevel=0 windows_msvc_host_platform=0 @@ -13,6 +14,10 @@ while [[ $# -gt 0 ]]; do print_failed_bazel_test_logs=1 shift ;; + --print-failed-action-summary) + print_failed_bazel_action_summary=1 + shift + ;; --use-node-test-env) use_node_test_env=1 shift @@ -37,7 +42,7 @@ while [[ $# -gt 0 ]]; do done if [[ $# -eq 0 ]]; then - echo "Usage: $0 [--print-failed-test-logs] [--use-node-test-env] [--remote-download-toplevel] [--windows-msvc-host-platform] -- -- " >&2 + echo "Usage: $0 [--print-failed-test-logs] [--print-failed-action-summary] [--use-node-test-env] [--remote-download-toplevel] [--windows-msvc-host-platform] -- -- " >&2 exit 1 fi @@ -136,6 +141,93 @@ print_bazel_test_log_tails() { done } +print_bazel_action_failure_summary() { + local console_log="$1" + local escaped_summary + local summary + + summary="$( + awk ' + function clean(line) { + gsub(sprintf("%c", 27) "\\[[0-9;]*m", "", line) + sub(/^.*\t[^\t]*\t[0-9TZ:._-]+ /, "", line) + return line + } + + function is_diagnostic(line) { + return line ~ /^(error(\[[^]]+\])?:|warning:|note:|help:)/ || + line ~ /^[[:space:]]+-->/ || + line ~ /^[[:space:]]*[0-9]+[[:space:]]+\|/ || + line ~ /^[[:space:]]*\|/ || + line ~ /^[[:space:]]+= (note|help):/ || + line ~ /^[[:space:]]*\^[[:space:]^~-]*$/ || + line ~ /^For more information/ || + line ~ /^error: aborting/ + } + + { + line = clean($0) + } + + line ~ /^ERROR: .* failed:/ { + if (printed) { + print "" + } + print line + in_failure = 1 + seen_diagnostic = 0 + printed = 1 + next + } + + in_failure && is_diagnostic(line) { + print line + seen_diagnostic = 1 + next + } + + in_failure && seen_diagnostic && line == "" { + print "" + next + } + + in_failure && seen_diagnostic { + in_failure = 0 + seen_diagnostic = 0 + next + } + ' "$console_log" + )" + + if [[ -z "$summary" ]]; then + summary="$(grep -E '^ERROR: |^FAILED: ' "$console_log" | tail -n 50 || true)" + fi + + if [[ -z "$summary" ]]; then + echo "No Bazel action failures were found in the captured console output." + return + fi + + if [[ "${GITHUB_ACTIONS:-}" == "true" ]]; then + escaped_summary="$( + printf '%s' "$summary" \ + | awk 'BEGIN { ORS = "" } { + gsub(/%/, "%25") + gsub(/\r/, "%0D") + print sep $0 + sep = "%0A" + }' + )" + echo "::error title=Bazel failed action diagnostics::${escaped_summary}" + fi + + echo + echo "Bazel failed action diagnostics:" + echo "--------------------------------" + printf '%s\n' "$summary" + echo "--------------------------------" +} + bazel_args=() bazel_targets=() found_target_separator=0 @@ -178,10 +270,10 @@ if [[ "${RUNNER_OS:-}" == "Windows" && $windows_msvc_host_platform -eq 1 ]]; the done if [[ $has_host_platform_override -eq 0 ]]; then - # Keep Windows Bazel targets on `windows-gnullvm` for cfg coverage, but opt - # specific jobs into an MSVC exec platform when they need helper binaries - # like Rust test wrappers and V8 generators to resolve a compatible host - # toolchain. + # Use the MSVC Windows platform for jobs that need helper binaries like + # Rust test wrappers and V8 generators to resolve a compatible toolchain. + # Callers that need a different Windows target platform should pass an + # explicit `--platforms=...` flag. post_config_bazel_args+=("--host_platform=//:local_windows_msvc") fi fi @@ -302,6 +394,9 @@ else fi if [[ ${bazel_status:-0} -ne 0 ]]; then + if [[ $print_failed_bazel_action_summary -eq 1 ]]; then + print_bazel_action_failure_summary "$bazel_console_log" + fi if [[ $print_failed_bazel_test_logs -eq 1 ]]; then print_bazel_test_log_tails "$bazel_console_log" fi diff --git a/.github/workflows/bazel.yml b/.github/workflows/bazel.yml index 0328ac207..842a3ce12 100644 --- a/.github/workflows/bazel.yml +++ b/.github/workflows/bazel.yml @@ -161,10 +161,14 @@ jobs: --build_metadata=COMMIT_SHA=${GITHUB_SHA} --build_metadata=TAG_job=clippy ) + bazel_wrapper_args=() if [[ "${RUNNER_OS}" == "Windows" ]]; then - # Some explicit targets pulled in through //codex-rs/... are - # intentionally incompatible with `//:local_windows`, but the lint - # aspect still traverses their compatible Rust deps. + # Keep this aligned with the Windows Bazel test job. With the + # default `//:local_windows` host platform, Windows `rust_test` + # targets such as `//codex-rs/core:core-all-test` can be skipped + # by `--skip_incompatible_explicit_targets`, which hides clippy + # diagnostics from integration-test modules. + bazel_wrapper_args+=(--windows-msvc-host-platform) bazel_clippy_args+=(--skip_incompatible_explicit_targets) fi @@ -175,6 +179,8 @@ jobs: done <<< "${bazel_target_lines}" ./.github/scripts/run-bazel-ci.sh \ + --print-failed-action-summary \ + "${bazel_wrapper_args[@]}" \ -- \ build \ "${bazel_clippy_args[@]}" \ diff --git a/codex-rs/core/tests/suite/unified_exec.rs b/codex-rs/core/tests/suite/unified_exec.rs index 77a15e92c..bf0eb4637 100644 --- a/codex-rs/core/tests/suite/unified_exec.rs +++ b/codex-rs/core/tests/suite/unified_exec.rs @@ -5,13 +5,8 @@ use std::sync::OnceLock; use anyhow::Context; use anyhow::Result; -use codex_config::Constrained; use codex_exec_server::CreateDirectoryOptions; use codex_features::Feature; -use codex_protocol::permissions::FileSystemAccessMode; -use codex_protocol::permissions::FileSystemPath; -use codex_protocol::permissions::FileSystemSandboxEntry; -use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ExecCommandSource; @@ -2349,6 +2344,12 @@ async fn unified_exec_runs_under_sandbox() -> Result<()> { #[cfg(unix)] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn unified_exec_enforces_glob_deny_read_policy() -> Result<()> { + use codex_config::Constrained; + use codex_protocol::permissions::FileSystemAccessMode; + use codex_protocol::permissions::FileSystemPath; + use codex_protocol::permissions::FileSystemSandboxEntry; + use codex_protocol::permissions::FileSystemSandboxPolicy; + skip_if_no_network!(Ok(())); skip_if_sandbox!(Ok(()));