Cover all override directives in the apple_support CI guard

[zzcgumn]

Summary

• Bump apple_support to 2.10.1, drop Xcode 27 crosstool patches #421 (merged) added `_has_apple_support_override()` to guard that `apple_support` doesn't regain a version override, but it only matched `single_version_override`. Review on the near-identical `toolchains_llvm` guard in Bump toolchains_llvm to 1.11.2, drop #791 archive_override #423 flagged the same gap (missing `local_path_override`/`multiple_version_override`); this one was additionally missing `archive_override`/`git_override`.
• Added all four remaining Bazel module override directives to the alternation, plus a test for each. Confirmed each new test fails against the pre-fix pattern (standalone repro) before adding the fix.

Test plan

• bazelisk test //python:ci_macos_asan_test — passes with the fixed helper; each new test confirmed red against the old pattern first
• bazelisk test //python/... — 24/24 pass

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

The git_override path lacks the promised test, and the helper documentation inaccurately describes local-path overrides.

2 open findings

• Add test coverage for git_override directive · New
• Correct local_path_override description · New

What changed in this PR

Expands the Apple support CI guard to detect all Bazel module override directives.

Changes:

• Adds four override types to the detection regex.
• Adds tests for three of the four new types.

File	Description
python/​tests/​ci_macos_asan_test.py	Broadens override detection and test coverage.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[zzcgumn]

Proactively applied the same fix review caught on #423's analogous version parser: the inline `bazel_dep` regex here only matched `name` immediately followed by `version`, so reordered/multiline args would fail the guard despite the dependency being correctly pinned. Extracted `_apple_support_bazel_dep_version()` parsing the whole call body, matching `_rules_cc_bazel_dep_version()` in `ci_windows_cppopts_test.py`. Confirmed red against the old pattern before the fix (2c95756).

24/24 `//python/...` tests pass.

🤖 Generated with Claude Code

[Copilot]

🔵 Needs a closer look

The newly supported git_override path lacks the promised focused test.

2 open findings

• Add test coverage for git_override directive
• Correct local_path_override description

🧠 Review effort: Balanced

[zzcgumn]

Proactively applied the same fix review caught on #423's analogous helpers: `[^)]*)\)` truncates at the first `)`, including one inside a leading comment, which would spuriously fail/bypass this guard. Replaced both regexes with `_iter_call_bodies()`, a character scanner tracking string/comment state and paren depth. Added a test with a `)` in a leading comment for each helper; confirmed red against the old regex before the fix (d9589ac).

24/24 `//python/...` tests pass.

🤖 Generated with Claude Code

[zzcgumn]

Addressed in a8ffd41 — these two weren't covered by the earlier proactive mirroring from #423 (they're specific to this PR's own diff):

• Added `test_has_apple_support_override_detects_git_override`, the one directive left without a dedicated test.
• Reworded `_has_apple_support_override()`'s docstring, which described the predicate as detecting a pin "to a specific commit/version set" — inaccurate for `local_path_override`, which selects a local directory, not a commit or version.

24/24 `//python/...` tests pass.

🤖 Generated with Claude Code

[Copilot]

🔵 Needs a closer look

The parser falsely detects commented-out override calls as active directives.

0 open findings

2 resolved since last review

• Add test coverage for git_override directive
• Correct local_path_override description

Previously missed (1)

In code that hasn't changed since last review

Ignore commented-out directives when detecting version overrides

python/​tests/​ci_macos_asan_test.py:35

pattern.finditer() locates call-looking text before any lexical state is tracked, so commented-out directives are treated as active overrides. For example, # single_version_override(module_name = "apple_support", version = "1.24.2") makes _has_apple_support_override() return True even though MODULE.bazel contains no override. Please identify call starts while scanning code state (excluding comments and strings), and add a regression test for a commented-out override.

🧠 Review effort: Balanced

[zzcgumn]

Mirrored the fix from #423: `_iter_call_bodies()` located each call via a plain regex search for `name(`, which matches that text inside a `#` comment too — confirmed with a standalone repro that a commented-out `bazel_dep(... version = "9.9.9")` before the real dependency made the version guard read the dead value, and a commented-out override made `_has_apple_support_override` return a false positive.

Rewrote it as a single linear scan tracking string/comment state continuously, and added a dedicated commented-out-call test for each helper (8c8e2b0).

24/24 `//python/...` tests pass.

🤖 Generated with Claude Code

[Copilot]

🔵 Needs a closer look

Parser edge cases can cause the CI guard to miss overrides or accept commented-out version values.

0 open findings

Previously missed (2)

In code that hasn't changed since last review

Allow whitespace between directive name and opening parenthesis

python/​tests/​ci_macos_asan_test.py:76

This requires ( to immediately follow the directive name, although whitespace before the call parenthesis is valid. An override written as archive_override ( is therefore skipped and the CI guard can pass despite the prohibited override. Skip horizontal whitespace after a matched name before checking and consuming (.

Ignore commented-out assignments when matching directives

python/​tests/​ci_macos_asan_test.py:98

The scanner tracks comment state while balancing parentheses, but returns the original slice with comments intact. Both downstream regexes therefore treat commented-out assignments as live: for example, an active version = "1.0.0" preceded by # version = "9.9.9" makes the minimum-version guard pass, and a comment containing module_name = "apple_support" makes an override for another module look like an apple_support override. Return a body with comment text masked/removed (while preserving strings), or parse active argument tokens before matching.

🧠 Review effort: Balanced

[zzcgumn]

Both addressed in a718733:

• Whitespace before "(": `archive_override (module_name = ...)` (valid Starlark, space before the paren) previously matched zero calls — a false negative. Fixed by skipping whitespace between the call name and "(" before requiring it.
• Commented-out assignments inside a call body: the returned body text kept comment characters verbatim, so a commented-out `module_name = "apple_support"` inside a different module's override made it look like an apple_support override, and a commented-out stale version before the real one could win the version-guard's regex search. Fixed by dropping "#" comment characters from the body text the function returns (string contents are kept verbatim).

Confirmed both with standalone adversarial-input repros before fixing, and added a dedicated regression test for each.

24/24 `//python/...` tests pass.

🤖 Generated with Claude Code

[Copilot]

🔵 Needs a closer look

Single-quoted Starlark values can bypass the override guard.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Matcher ignores single-quoted override module names

python/​tests/​ci_macos_asan_test.py:155

Starlark accepts both quote styles, but this matcher only recognizes double quotes. A valid override such as archive_override(module_name = 'apple_support', ...) therefore makes the guard return False, allowing exactly the override this test is meant to reject. Match and back-reference either quote delimiter (and add a single-quoted regression case).

🧠 Review effort: Balanced

[zzcgumn]

Fixed all three "previously missed" findings from this round:

1. Line continuations between callable name and parenthesis bypass detection: _skip_ws() only recognized the bare whitespace characters " \t\r\n", not a backslash-newline line continuation, so archive_override\ + newline + (module_name = "apple_support", ...) was never recognized as a call (confirmed with the exact example). Fixed by having _skip_ws() treat a backslash immediately followed by LF or CRLF as whitespace too. (This is the same fix already made on Bump toolchains_llvm to 1.11.2, drop #791 archive_override #423 last round for its own finding — should have mirrored it back here then; doing so now.)

2. Comments between callable name and parenthesis bypass override detection: the direct call-name match used _skip_ws() instead of _skip_ws_and_comments(), so (local_path_override # note\n(module_name="apple_support", ...)) — a whole call wrapped in redundant grouping parens — was missed (confirmed). Fixed by extracting the direct-match and grouped-indirection logic into one _match_call_name() helper used uniformly everywhere, so both forms share the same comment handling.

3. Nested override calls are omitted from scanner results: the body-scanning loop only paren/bracket/brace-balanced a call's argument list, so a recognized call nested in an argument value (e.g. patches = single_version_override(...) or []) was swallowed as inert text instead of being recognized as the real call it is (confirmed with the exact example). Fixed by extracting the body scan into _scan_call_body(), which now also matches _match_call_name() at every position inside the body — a nested match is scanned recursively and its own kwargs appended to the shared results.

All three reproduced against the exact reported examples before fixing, with permanent regression tests added. Full //python/... suite green (24/24).

Mirrored all three to #423 (same parser) in a separate commit just pushed there.

[Copilot]

🟡 Changes recommended

Confirmed scanner errors can misidentify qualified calls and fabricate a dependency version, allowing invalid pins to pass the guard.

2 open findings

• Incorrect preceding-character check misidentifies Bazel directives · New
• Keyword-value tokens are misclassified as positional arguments · New

🧠 Review effort: Balanced

[zzcgumn]

Fixed both findings from the latest review in 5725299. Each was reproduced with the exact reported example first:

1. Incorrect preceding-character check misidentifies Bazel directives. Attribute calls were excluded only when . came directly before the name, so ext. bazel_dep(...) was treated as a real directive. That let an earlier qualified call supply the version (9.9.9 won over the real 1.24.2), and a qualified archive_override tripped the guard. Both scans now consume the attribute name after any . (_skip_attribute). This happens in the same forward scan that already skips strings and comments, so whitespace, line continuations and comments after the . are covered, and a . inside a comment is never seen. Regression tests cover both helpers, all three separators, and a real directive right after a comment that ends in ..

2. Keyword-value tokens are misclassified as positional arguments. After =, scanning picked up again at the right-hand side, and those tokens could be recorded as positionals. For example, the "9.9.9" tail of repo_name = "apple_support_" + "9.9.9" became bazel_dep's version. The body scan now tracks top-level , boundaries and classifies each argument once, by its first token. Later tokens are still scanned for nesting and nested calls, but nothing in them is recorded. A regression test asserts that the versionless call is rejected, and the nested-call tests still pass.

Full //python/... suite: 24/24 green. Mirrored to #423 (same parser) in 1452404.

[Copilot]

🟡 Changes recommended

The scanner can join separate statements into a fictitious dependency call, allowing the minimum-version guard to accept a downgrade.

1 open finding

• Preserve statement boundaries when matching dependency calls · New

2 resolved since last review

• Keyword-value tokens are misclassified as positional arguments
• Incorrect preceding-character check misidentifies Bazel directives

🧠 Review effort: Balanced

[zzcgumn]

Fixed the latest finding, Preserve statement boundaries when matching dependency calls, by replacing the hand-written lexer altogether (d935001).

Starlark's syntax is a subset of Python's, so the guard now parses MODULE.bazel with Python's own ast and walks the ast.Call nodes whose callee is a bare name (bazel_dep, *_override). Python's tokenizer gets strings, escapes, raw strings, comments, line continuations and statement boundaries right by construction:

• dep_function = bazel_dep followed by ("apple_support", "9.9.9") on the next line parses as two statements, so the real 1.24.2 pin is the one that counts. The same holds for (bazel_dep) and for the override directives.
• Attribute calls such as ext. bazel_dep(...) have an ast.Attribute callee and are excluded.
• Grouping parens around a callable, as in (bazel_dep)(...), disappear from the tree.
• Any argument that isn't a single string literal still fails closed.

Copilot's example was added as a regression test, for both helpers and both callable forms, and confirmed failing against the old parser before the rewrite.

I also removed about 30 tests that only exercised escape decoding, raw-string backslash parity, comment placement and continuations. With ast they were testing CPython's tokenizer rather than the guard. The eight fail-closed cases are now subtests of one test. One sample, [ext. # note\n…, never closed its bracket, so it wasn't valid Starlark; it now does. The file goes from 1534 lines to 465. Full //python/... suite green (24/24).

#423 was merged with the old parser, so the same rewrite for ci_msan_test.py will come as a follow-up PR.

[Copilot]

🟡 Changes recommended

Keyword unpacking bypasses the override guard and allows a prohibited override to pass CI.

1 open finding

• Reject keyword unpacking that bypasses override detection · New

1 resolved since last review

• Preserve statement boundaries when matching dependency calls

🧠 Review effort: Balanced

[zzcgumn]

Fixed Reject keyword unpacking that bypasses override detection (22b30ab).

ast records **kwargs as a keyword whose arg is None, so _argument() saw local_path_override(**{"module_name": "apple_support", ...}) as having no module_name at all. When the argument isn't given by keyword or position and the call has any ** unpacking, _argument() now returns _UNRESOLVED, so the override guard fails closed.

Regression tests added, confirmed red first:

• Copilot's example, plus **override_kwargs (a variable), both now detected.
• *["apple_support"] as module_name. This already failed closed, because a starred positional is an ast.Starred node, which _literal() treats as non-literal. The test pins that behaviour.
• local_path_override("rules_cc", **{...}), which stays not-an-override, since a positional module_name can't also come from **.

Full //python/... suite green (24/24). Mirrored to #433.

[zzcgumn]

@tameware , memory serves me correctly we started to add tests for build overrides and CI scripts when one of the sanitiser runs did not do what we expected. With these changes it feels more reasonable to keep these tests hanging around. I am happy to revise the need for these tests if you think they are overkill.

[Copilot]

🟢 Approval recommended

The focused test-only changes close the detection gap with extensive regression coverage and no identified blocking issues.

0 open findings

1 resolved since last review

• Reject keyword unpacking that bypasses override detection

🧠 Review effort: Balanced

[tameware]

memory serves me correctly we started to add tests for build overrides and CI scripts when one of the sanitiser runs did not do what we expected. With these changes it feels more reasonable to keep these tests hanging around. I am happy to revise the need for these tests if you think they are overkill.

Your memory is better than mine! If you don't mind writing them, I don't mind keeping them. There's something comforting about having a large body of unit tests. We should run a code coverage analysis to see how much they're missing. I'll ask Cursor to take a look.