From 75e3c8e6156ed7541f8fc1c3c4efdbfcbeadb71e Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Mon, 27 Jul 2026 20:47:24 -0700 Subject: [PATCH 1/3] feat(bazel): enforce that BUILD files match their sources Phase 2 of docs/dev/bazel-consolidation.md. Nothing in CI noticed when a BUILD file fell behind its sources, and the consequences were not theoretical. Running Gazelle from the root found four test files that Bazel was never building. apikey_test.go, deploy_test.go and root_test.go were missing from //src/clis/nvcf-cli/cmd, and internal/apikeys had a client_test.go with no go_test target at all. They exist in the repository and have never run in CI. Both targets pass once declared, so this is coverage that was silently absent rather than tests that were failing. That is the same failure mode as nvsnap, whose BUILD file was last regenerated twelve days before the sources it was meant to build. tools/ci/check-gazelle runs Gazelle from the root in --mode=diff, so it reports rather than rewrites and leaves no dirty tree. It distinguishes "there is a diff" from "gazelle failed", because a fetch or Starlark error must not read as tidy BUILD files. Scope is Go packages only. Gazelle also wants to reorder attributes in the src/libraries/java BUILD files, which is cosmetic and would churn files the Java modules own for no functional gain. Those modules move into the root in Phase 6; widen this then, as part of that move. One manual fix on top of the generated output: Gazelle hoists the rules_shell load in src/libraries/go/lib and carried a comment with it that describes the sh_test target, not the load. The comment is back on sh_test, and the check is idempotent with it there. Known gaps, deliberately left: two BUILD files Gazelle cannot merge (nvsnap/ui, go/worker/test/testutils) and three imports under tests/bdd that are not valid module paths. Neither blocks this check. Runs in build-test, not bazel.yml. Co-authored-by: Balaji Ganesan Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/build-test.yml | 3 + src/clis/nvcf-cli/cmd/BUILD.bazel | 4 + .../nvcf-cli/internal/apikeys/BUILD.bazel | 12 ++- src/clis/nvcf-cli/internal/client/BUILD.bazel | 1 + .../nvcf-cli/scripts/admin-mock/BUILD.bazel | 14 ++++ src/libraries/go/lib/BUILD.bazel | 4 +- tools/ci/check-gazelle | 77 +++++++++++++++++++ 7 files changed, 112 insertions(+), 3 deletions(-) create mode 100644 src/clis/nvcf-cli/scripts/admin-mock/BUILD.bazel create mode 100755 tools/ci/check-gazelle diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml index 18aa4eaf9..ff1396e21 100644 --- a/.github/workflows/build-test.yml +++ b/.github/workflows/build-test.yml @@ -68,3 +68,6 @@ jobs: run: | bash tools/ci/test-check-nested-modules bash tools/ci/check-nested-modules + + - name: Check BUILD files match their sources + run: bash tools/ci/check-gazelle diff --git a/src/clis/nvcf-cli/cmd/BUILD.bazel b/src/clis/nvcf-cli/cmd/BUILD.bazel index b470885ed..22671a821 100644 --- a/src/clis/nvcf-cli/cmd/BUILD.bazel +++ b/src/clis/nvcf-cli/cmd/BUILD.bazel @@ -103,15 +103,18 @@ go_test( srcs = [ "admin_test.go", "agent_skill_test.go", + "apikey_test.go", "cluster_agent_maintenance_test.go", "cluster_agent_test.go", "cluster_agent_validate_test.go", "cluster_dump_test.go", "cluster_registration_test.go", + "deploy_test.go", "exit_code_error_test.go", "function_llm_model_test.go", "main_test.go", "registry_test.go", + "root_test.go", "self_hosted_check_test.go", "self_hosted_compute_plane_test.go", "self_hosted_control_plane_test.go", @@ -125,6 +128,7 @@ go_test( ], embed = [":cmd"], deps = [ + "//src/clis/nvcf-cli/internal/apikeys", "//src/clis/nvcf-cli/internal/client", "//src/clis/nvcf-cli/internal/clusteragent", "//src/clis/nvcf-cli/internal/clusterdump", diff --git a/src/clis/nvcf-cli/internal/apikeys/BUILD.bazel b/src/clis/nvcf-cli/internal/apikeys/BUILD.bazel index 58e79c879..7ae5e63cd 100644 --- a/src/clis/nvcf-cli/internal/apikeys/BUILD.bazel +++ b/src/clis/nvcf-cli/internal/apikeys/BUILD.bazel @@ -13,7 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -load("@rules_go//go:def.bzl", "go_library") +load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "apikeys", @@ -22,3 +22,13 @@ go_library( visibility = ["//src/clis/nvcf-cli:__subpackages__"], deps = ["//src/clis/nvcf-cli/internal/logging"], ) + +go_test( + name = "apikeys_test", + srcs = ["client_test.go"], + embed = [":apikeys"], + deps = [ + "@com_github_stretchr_testify//assert", + "@com_github_stretchr_testify//require", + ], +) diff --git a/src/clis/nvcf-cli/internal/client/BUILD.bazel b/src/clis/nvcf-cli/internal/client/BUILD.bazel index 33f0af610..72159f2ec 100644 --- a/src/clis/nvcf-cli/internal/client/BUILD.bazel +++ b/src/clis/nvcf-cli/internal/client/BUILD.bazel @@ -52,6 +52,7 @@ go_test( ], embed = [":client"], deps = [ + "//src/clis/nvcf-cli/internal/state", "@com_github_spf13_viper//:viper", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", diff --git a/src/clis/nvcf-cli/scripts/admin-mock/BUILD.bazel b/src/clis/nvcf-cli/scripts/admin-mock/BUILD.bazel new file mode 100644 index 000000000..f3aad336e --- /dev/null +++ b/src/clis/nvcf-cli/scripts/admin-mock/BUILD.bazel @@ -0,0 +1,14 @@ +load("@rules_go//go:def.bzl", "go_binary", "go_library") + +go_library( + name = "admin-mock_lib", + srcs = ["main.go"], + importpath = "nvcf-cli/scripts/admin-mock", + visibility = ["//visibility:private"], +) + +go_binary( + name = "admin-mock", + embed = [":admin-mock_lib"], + visibility = ["//visibility:public"], +) diff --git a/src/libraries/go/lib/BUILD.bazel b/src/libraries/go/lib/BUILD.bazel index 16552aee1..f52293c8d 100644 --- a/src/libraries/go/lib/BUILD.bazel +++ b/src/libraries/go/lib/BUILD.bazel @@ -27,6 +27,8 @@ load("@gazelle//:def.bzl", "gazelle_test") # rules with the same importpath (nverrors_go_proto and errors). # gazelle:resolve go github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/nvkit/api/errors/v1 //src/libraries/go/lib/pkg/nvkit/api/errors/v1:errors +load("@rules_shell//shell:sh_test.bzl", "sh_test") + gazelle_test( name = "gazelle_test", extra_args = ["src/libraries/go/lib"], @@ -36,8 +38,6 @@ gazelle_test( # Transitional non-hermetic wrapper: this shells out to `go run` through the # shared golangci-lint runner. Keep cache-disabled tags until lint inputs are # declared in Bazel. -load("@rules_shell//shell:sh_test.bzl", "sh_test") - sh_test( name = "golangci_lint", timeout = "long", diff --git a/tools/ci/check-gazelle b/tools/ci/check-gazelle new file mode 100755 index 000000000..0dad36651 --- /dev/null +++ b/tools/ci/check-gazelle @@ -0,0 +1,77 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Fail if Gazelle, run from the repository root, would change any BUILD file. +# +# Why this exists: nothing currently notices when a BUILD file falls behind its +# sources. nvsnap's checkpoint_v2.go and restore_v2.go landed 2026-07-14 while +# internal/agent/BUILD.bazel was last regenerated 2026-07-02, so the build was +# broken for twelve days and no job reported it. That was not unique: at the +# time this check was written, more than a dozen packages had a .go file newer +# than their BUILD.bazel. +# +# Root-run matters. Gazelle resolves dependencies against the module it runs +# in, so running it per-subtree produces different, and during consolidation +# increasingly wrong, results. The root run is the one whose output has to stay +# stable as services migrate into the root module. +# +# --mode=diff makes Gazelle report rather than rewrite, so CI never leaves a +# dirty tree and the failure output is the patch a developer needs to apply. +set -euo pipefail + +root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +cd "${root}" + +: "${BAZEL:=bazel}" + +diff_out="$(mktemp)" +trap 'rm -f "${diff_out}"' EXIT + +# Gazelle exits non-zero in diff mode when there is a diff, which is the signal +# we want rather than an error, so capture the status instead of letting set -e +# take it. +# Scope: Go packages only for now. Gazelle also wants to reorder attributes in +# the src/libraries/java BUILD files, which is cosmetic and would churn files +# the Java modules own for no functional gain. Those modules move into the root +# in Phase 6 of docs/dev/bazel-consolidation.md; widen this then, in the change +# that moves them, rather than reordering another team's files ahead of it. +GO_ROOTS=( + src/clis + src/libraries/go + src/compute-plane-services + src/control-plane-services + src/invocation-plane-services +) + +status=0 +"${BAZEL}" run //:gazelle -- --mode=diff "${GO_ROOTS[@]}" > "${diff_out}" 2>&1 || status=$? + +# A real failure (missing target, fetch error, Starlark error) is not the same +# as "there is a diff". Distinguish them, otherwise a broken build reads as +# tidy BUILD files. +if grep -qE '^(ERROR|FATAL):' "${diff_out}"; then + echo "error: gazelle did not run to completion" >&2 + cat "${diff_out}" >&2 + exit 2 +fi + +# Gazelle prints unified diffs for files it would change. No diff lines means +# the generated output already matches the tree. +if ! grep -qE '^(\+\+\+|---) ' "${diff_out}"; then + echo "gazelle: BUILD files are up to date" + exit 0 +fi + +{ + echo "error: BUILD files are out of date with their sources." + echo + echo "Regenerate from the repository root and commit the result:" + echo + echo " bazel run //:gazelle" + echo + echo "Gazelle would apply the following:" + echo + cat "${diff_out}" +} >&2 +exit 1 From 570ed16f40d86313fbe5170eb339f1e88ba99f90 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Tue, 28 Jul 2026 06:38:03 -0700 Subject: [PATCH 2/3] fix(bazel): fail closed when gazelle exits without a diff Review caught a false green. The script captured gazelle's exit status and never checked it, so any failure that printed neither an ERROR/FATAL prefix nor unified-diff markers fell through to the success path and reported "BUILD files are up to date" for a run that verified nothing. A crashed server, a killed process, an unrecognised failure mode: all of them read as tidy BUILD files. That is precisely the class of silent pass this check was added to eliminate, so it mattered more here than it would in most scripts. Non-zero with diff markers stays exit 1, since gazelle exits non-zero whenever it produces a diff and that is the normal "out of date" signal. Non-zero without them is now exit 2. tools/ci/test-check-gazelle covers all three outcomes with a stubbed BAZEL, so nothing touches a real Bazel or the network: clean exits 0, diff exits 1 whether gazelle reports zero or non-zero, and ERROR, FATAL and silent non-zero exits all fail closed with 2. Verified the test catches the regression rather than only passing today. Reverting the status check makes it fail with "want exit 2, got 0", which is the exact symptom reported. Co-authored-by: Balaji Ganesan Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/build-test.yml | 4 +- tools/ci/check-gazelle | 13 ++++- tools/ci/test-check-gazelle | 93 ++++++++++++++++++++++++++++++++ 3 files changed, 107 insertions(+), 3 deletions(-) create mode 100755 tools/ci/test-check-gazelle diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml index ff1396e21..e2284068a 100644 --- a/.github/workflows/build-test.yml +++ b/.github/workflows/build-test.yml @@ -70,4 +70,6 @@ jobs: bash tools/ci/check-nested-modules - name: Check BUILD files match their sources - run: bash tools/ci/check-gazelle + run: | + bash tools/ci/test-check-gazelle + bash tools/ci/check-gazelle diff --git a/tools/ci/check-gazelle b/tools/ci/check-gazelle index 0dad36651..06809ee34 100755 --- a/tools/ci/check-gazelle +++ b/tools/ci/check-gazelle @@ -56,9 +56,18 @@ if grep -qE '^(ERROR|FATAL):' "${diff_out}"; then exit 2 fi -# Gazelle prints unified diffs for files it would change. No diff lines means -# the generated output already matches the tree. +# Gazelle prints unified diffs for files it would change, and exits non-zero +# when it produces any. So a non-zero status with diff markers is the normal +# "out of date" signal, while a non-zero status without them is a failure that +# happened to print neither ERROR nor FATAL. Treating the latter as success +# would report tidy BUILD files for a run that never checked them, which is the +# exact false green this whole check exists to prevent. if ! grep -qE '^(\+\+\+|---) ' "${diff_out}"; then + if [ "${status}" -ne 0 ]; then + echo "error: gazelle exited ${status} without producing a diff" >&2 + cat "${diff_out}" >&2 + exit 2 + fi echo "gazelle: BUILD files are up to date" exit 0 fi diff --git a/tools/ci/test-check-gazelle b/tools/ci/test-check-gazelle new file mode 100755 index 000000000..c82614bd4 --- /dev/null +++ b/tools/ci/test-check-gazelle @@ -0,0 +1,93 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Behavioral test for tools/ci/check-gazelle. +# +# The check has to keep three outcomes apart: BUILD files are current, BUILD +# files are stale, and Gazelle did not run. Conflating the last two is the +# dangerous one, because a fetch or Starlark failure reported as "up to date" +# means CI claims BUILD files were verified when nothing verified them. +# +# BAZEL is stubbed so none of this touches a real Bazel or the network. +set -euo pipefail + +script="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/check-gazelle" +fail=0 + +stub_dir="$(mktemp -d)" +trap 'rm -rf "${stub_dir}"' EXIT + +# Build a fake bazel that prints a fixed payload and exits with a fixed status. +make_bazel() { # make_bazel + local status="$1" payload="$2" path="${stub_dir}/bazel" + { + echo '#!/usr/bin/env bash' + printf 'cat <<%s\n%s\n%s\n' "PAYLOAD_EOF" "${payload}" "PAYLOAD_EOF" + echo "exit ${status}" + } > "${path}" + chmod +x "${path}" + printf '%s' "${path}" +} + +run() { # run + local b; b="$(make_bazel "$1" "$2")" + set +e + out="$(BAZEL="${b}" bash "${script}" 2>&1)" + rc=$? + set -e +} + +expect() { # expect [substring] + local desc="$1" want="$2" substr="${3:-}" + if [ "${rc}" != "${want}" ]; then + printf 'FAIL %s: want exit %s, got %s\n%s\n' "${desc}" "${want}" "${rc}" "${out}" + fail=1; return + fi + if [ -n "${substr}" ] && ! printf '%s' "${out}" | grep -q -- "${substr}"; then + printf 'FAIL %s: output missing %q\n%s\n' "${desc}" "${substr}" "${out}" + fail=1; return + fi + printf 'ok %s\n' "${desc}" +} + +# Clean: Gazelle succeeds and prints no diff. +run 0 'no changes' +expect "clean tree exits 0" 0 "up to date" + +# Stale: Gazelle prints a unified diff. It exits non-zero when it does, which is +# the expected signal, not a failure. +run 1 '--- a/src/foo/BUILD.bazel ++++ b/src/foo/BUILD.bazel +@@ -1 +1,2 @@ ++ srcs = ["new_test.go"],' +expect "diff exits 1" 1 "out of date" +expect " ... and shows the patch" 1 "new_test.go" + +# A diff reported with a zero exit is still a diff. +run 0 '--- a/src/foo/BUILD.bazel ++++ b/src/foo/BUILD.bazel' +expect "diff with zero status still exits 1" 1 "out of date" + +# Invocation failure: Gazelle could not run. This must never read as clean. +run 1 'ERROR: no such package @@gazelle//: fetch failed' +expect "ERROR output exits 2" 2 "did not run to completion" + +run 1 'FATAL: Starlark evaluation failed' +expect "FATAL output exits 2" 2 "did not run to completion" + +# The case the status check exists for: a non-zero exit that prints neither a +# recognised error prefix nor a diff. Without checking the status this fell +# through to the success path and reported tidy BUILD files. +run 34 'Server terminated abruptly' +expect "silent non-zero exit fails closed" 2 "without producing a diff" + +# And its mirror: a zero exit with no diff is genuinely clean, not a failure. +run 0 '' +expect "empty output with zero status is clean" 0 "up to date" + +if [ "${fail}" -ne 0 ]; then + echo "check-gazelle: FAILED" >&2 + exit 1 +fi +echo "check-gazelle: all checks passed" From 82b4ae25b6d8ed2af40faf4f5f21fe5e65cdb88a Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Tue, 28 Jul 2026 13:52:57 -0700 Subject: [PATCH 3/3] test(ci): assert how gazelle is invoked, not just its output The stub ignored its arguments, so the suite verified only how check-gazelle classified gazelle's output. It would still have passed if the script dropped --mode=diff and started rewriting the tree, renamed the target, ran from the wrong directory, or changed which roots it scopes. Everything about the invocation was untested. The scope is the part that most needed a guard. Go-only is a deliberate decision: Gazelle also wants to reorder attributes in the src/libraries/java BUILD files, which is cosmetic and would churn files the Java modules own. Nothing recorded that intent in a way a change could trip over. The stub now records its working directory and argv, and the tests assert the target, --mode=diff, the repository root as cwd, each of the five Go roots, and that src/libraries/java is absent. Also switches assertions to fixed-string matching. The helper passed its needle to grep as a regex, so "new_test.go" would have accepted "newXtest.go", and it used a pipeline where grep exiting early could SIGPIPE the producer and trip pipefail on large output. Verified the new assertions fail when the behaviour changes rather than only passing today: adding src/libraries/java to the scope fails the exclusion check, and removing --mode=diff fails the diff-mode check. Co-authored-by: Balaji Ganesan Co-Authored-By: Claude Opus 5 (1M context) --- tools/ci/test-check-gazelle | 44 ++++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/tools/ci/test-check-gazelle b/tools/ci/test-check-gazelle index c82614bd4..3135af218 100755 --- a/tools/ci/test-check-gazelle +++ b/tools/ci/test-check-gazelle @@ -19,10 +19,16 @@ stub_dir="$(mktemp -d)" trap 'rm -rf "${stub_dir}"' EXIT # Build a fake bazel that prints a fixed payload and exits with a fixed status. +invocation_log="${stub_dir}/invocation" + make_bazel() { # make_bazel local status="$1" payload="$2" path="${stub_dir}/bazel" { echo '#!/usr/bin/env bash' + # Record the working directory and full argv so the tests can assert + # how gazelle was invoked, not only how its output was classified. + printf 'printf "cwd=%%s\\n" "$PWD" > %s\n' "${invocation_log}" + printf 'printf "%%s\\n" "$@" >> %s\n' "${invocation_log}" printf 'cat <<%s\n%s\n%s\n' "PAYLOAD_EOF" "${payload}" "PAYLOAD_EOF" echo "exit ${status}" } > "${path}" @@ -44,7 +50,10 @@ expect() { # expect [substring] printf 'FAIL %s: want exit %s, got %s\n%s\n' "${desc}" "${want}" "${rc}" "${out}" fail=1; return fi - if [ -n "${substr}" ] && ! printf '%s' "${out}" | grep -q -- "${substr}"; then + # -F so the needle is literal: "new_test.go" as a regex would also accept + # "newXtest.go". Herestring rather than a pipeline so grep exiting early + # cannot SIGPIPE the producer and trip pipefail on large output. + if [ -n "${substr}" ] && ! grep -Fq -- "${substr}" <<<"${out}"; then printf 'FAIL %s: output missing %q\n%s\n' "${desc}" "${substr}" "${out}" fail=1; return fi @@ -86,6 +95,39 @@ expect "silent non-zero exit fails closed" 2 "without producing a diff" run 0 '' expect "empty output with zero status is clean" 0 "up to date" +# How gazelle is invoked is part of the contract. Without these, the suite +# still passes if check-gazelle drops --mode=diff, renames the target, runs +# from the wrong directory, or silently widens its scope. +run 0 'no changes' +inv="$(cat "${invocation_log}")" + +expect_inv() { # expect_inv + if grep -Fq -- "$2" <<<"${inv}"; then + printf 'ok %s\n' "$1" + else + printf 'FAIL %s: invocation missing %q\n%s\n' "$1" "$2" "${inv}" + fail=1 + fi +} + +expect_inv "runs the root gazelle target" "//:gazelle" +expect_inv "asks for a diff, does not rewrite" "--mode=diff" +expect_inv "runs from the repository root" "cwd=$(cd "$(dirname "${script}")/../.." && pwd)" +for root in src/clis src/libraries/go src/compute-plane-services \ + src/control-plane-services src/invocation-plane-services; do + expect_inv "scopes ${root}" "${root}" +done + +# Scope is Go-only on purpose: Gazelle would reorder attributes in the Java +# BUILD files, which is cosmetic churn in files the Java modules own. Assert +# the exclusion so widening it is a deliberate edit, not an accident. +if grep -Fq -- "src/libraries/java" <<<"${inv}"; then + printf 'FAIL java is in scope; it is deliberately excluded\n%s\n' "${inv}" + fail=1 +else + printf 'ok leaves src/libraries/java out of scope\n' +fi + if [ "${fail}" -ne 0 ]; then echo "check-gazelle: FAILED" >&2 exit 1