From 27bf8be86fe7287bb309b8a20415ab7aab7732ea Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Sat, 26 Sep 2026 02:22:00 -0400 Subject: [PATCH] Validate that bazel_args holds flags, not a command bazel_args is meant for the flags that make aquery mirror the real build (--config=release, -c opt). Anything that consumes it splices it into a bazel command line, but nothing checked its shape, and the SKILL.md text sits next to a literal `bazel aquery ...` line that is easy to paste in. Measured in any2bazel-lab: on a QGIS migration the agent recorded the whole aquery command as bazel_args. Its own diff never read the field, so the migration looked converged, while a consumer that spliced the field into `bazel aquery` failed on a second query expression and produced no parity result. config.load now validates the list and raises ValueError naming the first bad entry; diff.py turns that into a one-line error. An entry must be a flag, or the value of the flag just before it when that flag has no '='. So the well-formed ["-c", "opt"] is accepted, which a simpler "every entry starts with -" rule would reject. SKILL.md shows one right and one wrong example. In the lab's later QGIS run with this change, every bazel_args entry was a valid flag. Lab-Proposal: P22 Co-Authored-By: Claude Opus 5.5 --- SKILL.md | 9 ++++--- scripts/config.py | 41 +++++++++++++++++++++++++++++--- scripts/diff.py | 5 +++- tests/test_engine.py | 56 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 104 insertions(+), 7 deletions(-) diff --git a/SKILL.md b/SKILL.md index 62ad5c0..9010834 100644 --- a/SKILL.md +++ b/SKILL.md @@ -112,9 +112,12 @@ whether its frontend reads it: ``` Fields: -- **`bazel_args`** — the extra args aquery must run with to mirror the real - build (see [Extract the Bazel action graph](#extract-bazel-graph)). Recorded so - the comparison is reproducible; read by the operator, not the diff. +- **`bazel_args`** — the extra **flags** aquery must run with to mirror the + real build (see [Extract the Bazel action graph](#extract-bazel-graph)), e.g. + `["--config=release"]` or `["-c", "opt"]`; never a command + (`["aquery", "mnemonic(...)", ...]` is wrong). Recorded so the comparison is + reproducible; `diff.py` refuses an entry that is neither a flag nor a flag's + value. - **`target_map`** — `reference_name → bazel_name` for intentionally renamed **executables**. Bazel targets are keyed by full label, so the value is usually `:name` (e.g. `"bssl": ":bssl"`). Libraries need no mapping. diff --git a/scripts/config.py b/scripts/config.py index fd17cbd..4d2cbb2 100644 --- a/scripts/config.py +++ b/scripts/config.py @@ -100,9 +100,12 @@ class MigrationConfig: # Unlike `ignore` (flags/defines), this is the only lever for missing_tu / # missing_target on whole subtrees. Each entry is a reviewable decision. exclude_targets: Set[str] = field(default_factory=set) - # The bazel aquery invocation this migration is compared against, recorded + # The FLAGS appended to `bazel build` / `bazel aquery` so the Bazel side + # mirrors the real build (e.g. "--config=release", or "-c" "opt"), recorded # so the comparison is reproducible. Read by the skill/operator, not by the - # diff itself (the diff consumes the already-extracted model). + # diff itself (the diff consumes the already-extracted model); validated at + # load (see validate_bazel_args) because anything that consumes it splices + # it into a bazel command line. bazel_args: tuple = () # Opt-in: also diff TEST targets (role=test). OFF by default because test # diffing requires BOTH models to be extracted with tests enabled and the @@ -161,6 +164,38 @@ def target_excluded(self, name: str) -> bool: return name in self.exclude_targets +def validate_bazel_args(args) -> tuple: + """Check that `bazel_args` is a list of flags, and return it as a tuple. + + Each entry must be a flag (starts with '-') or the value of the flag just + before it, when that flag has no '=' (so ["-c", "opt"] and + ["--config", "release"] are fine). Anything else -- most often a whole + command such as ["aquery", "mnemonic(...)", "--output=jsonproto"] -- is + refused, naming the entry: spliced into `bazel aquery ` it + becomes a second query expression and breaks the command. A boolean flag + followed by a stray word slips through; the check is a guard against the + command-in-the-field mistake, not a Bazel option parser.""" + if isinstance(args, str) or not isinstance(args, (list, tuple)): + raise ValueError( + f"bazel_args must be a list of flags, got {type(args).__name__}") + value_allowed = False + for a in args: + if not isinstance(a, str): + raise ValueError(f"bazel_args entry {a!r} is not a string") + if a.startswith("-"): + value_allowed = "=" not in a + continue + if value_allowed: + value_allowed = False + continue + raise ValueError( + f"bazel_args entry {a!r} is not a flag or a flag's value. " + "bazel_args holds only the flags appended to `bazel build` / " + "`bazel aquery` to mirror the real build (e.g. [\"--config=release\"] " + "or [\"-c\", \"opt\"]), never the command or query itself.") + return tuple(args) + + def load(path: str) -> MigrationConfig: """Load config from an explicit file path, or return empty if absent.""" if not path or not os.path.isfile(path): @@ -182,7 +217,7 @@ def load(path: str) -> MigrationConfig: source_map=tuple( (e["from"], e["to"]) for e in ig.get("source_map", [])), exclude_targets=set(obj.get("exclude_targets", [])), - bazel_args=tuple(obj.get("bazel_args", [])), + bazel_args=validate_bazel_args(obj.get("bazel_args", [])), include_tests=bool(obj.get("include_tests", False)), ) diff --git a/scripts/diff.py b/scripts/diff.py index d602dcb..6ffeb8f 100644 --- a/scripts/diff.py +++ b/scripts/diff.py @@ -519,5 +519,8 @@ def summarize(discs: List[Discrepancy], from serialize import load_model a = load_model(sys.argv[1]) b = load_model(sys.argv[2]) - cfg = config_mod.load(sys.argv[3]) if len(sys.argv) > 3 else MigrationConfig() + try: + cfg = config_mod.load(sys.argv[3]) if len(sys.argv) > 3 else MigrationConfig() + except ValueError as e: + sys.exit(f"diff.py: {sys.argv[3]}: {e}") print(json.dumps(summarize(diff_models(a, b, cfg), a, b, cfg), indent=2)) diff --git a/tests/test_engine.py b/tests/test_engine.py index f50ef31..3d28f8c 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -640,6 +640,62 @@ def test_config_loads_all_fields_from_json(): assert cfg.map_include("external/absl+/base") == "@absl/base" # rewrite +def test_bazel_args_accepts_flags_and_flag_values(): + import config as config_mod + for ok in ([], ["--config=release"], ["-c", "opt"], + ["--config", "release", "--copt=-O3"], + ["--compilation_mode=opt", "--cxxopt=-std=c++17", "--force_pic"]): + assert config_mod.validate_bazel_args(ok) == tuple(ok), ok + + +def test_bazel_args_rejects_a_command_line(): + # A whole aquery command in the field (seen in practice): spliced into + # `bazel aquery ` it yields two query expressions. + import config as config_mod + bad_lists = ( + ["aquery", 'mnemonic("CppCompile|CppLink|CppArchive", //...)', + "--output=jsonproto"], + ["--config=release", "build"], # '=' flag takes no separate value + ["-c", "opt", "//..."], # one value per flag + "--config=release", # a string, not a list + ) + for bad in bad_lists: + try: + config_mod.validate_bazel_args(bad) + except ValueError as e: + if isinstance(bad, list): + assert repr(bad[-1] if bad[0].startswith("-") else bad[0]) in str(e), e + else: + raise AssertionError(f"accepted {bad!r}") + + +def test_bazel_args_rejects_a_non_string_entry(): + import config as config_mod + try: + config_mod.validate_bazel_args(["-c", 3]) + except ValueError as e: + assert "3" in str(e) and "not a string" in str(e), e + else: + raise AssertionError("accepted a non-string entry") + + +def test_config_load_refuses_malformed_bazel_args(): + import json + import tempfile + import config as config_mod + with tempfile.NamedTemporaryFile("w", suffix=".json", delete=False) as f: + json.dump({"bazel_args": ["aquery", "//...", "--output=jsonproto"]}, f) + path = f.name + try: + config_mod.load(path) + except ValueError as e: + assert "'aquery'" in str(e), e + else: + raise AssertionError("malformed bazel_args loaded without error") + finally: + os.unlink(path) + + def _test_models(cmake_test_srcs, bazel_test_srcs, n_cmake_bins=1, n_bazel_bins=1): """Build a/b models with a shared production lib plus test targets carrying the given sources, split across n_*_bins test executables."""