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]

Proactively mirrored a fix from #423's identical `_iter_call_kwargs()`: a top-level list or dict expression opening the first positional argument — e.g. `archive_override(["apple_support"][0], ...)` — was descended into without ever recording `_POSITIONAL_KEY`, so `module_name` read as absent and `_has_apple_support_override()` missed the override.

Applied the same "mark unresolved before descending" guard to the `[` and `{` branches, plus two mirrored regression tests — list and dict (614a2f4).

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

🤖 Generated with Claude Code

[zzcgumn]

Addressed the latest Copilot finding: comments between "=" and the RHS.

The RHS lookahead after `ident =` only skipped whitespace via `_skip_ws()`, so a comment on its own line between `=` and the literal — valid Starlark formatting — made the RHS look non-literal. Reproduced both cases from the finding:

• `module_name =\n # pin past Xcode 27 fixes\n "rules_cc"` was marked `_UNRESOLVED`, falsely tripping `_has_apple_support_override()` for an unrelated module's override (returned `True`, should be `False`).
• The same shape on `bazel_dep`'s `version =` returned the sentinel instead of the actual version string.

Factored the comment-skipping loop already in `_is_complete_literal()` out into a shared `_skip_ws_and_comments()`, and used it for the RHS lookahead too (7d162ae). Both reproductions now resolve correctly, confirmed directly, plus two regression tests (one per function) and confirmed the apple_support-with-a-comment case still correctly returns `True` (not over-corrected into never matching).

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

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Valid indirect Starlark calls can bypass the new override guard.

1 open finding

• Reject callable indirection when scanning archive_override directives · New

🧠 Review effort: Balanced

[zzcgumn]

Addressed the latest Copilot finding: parenthesized-callable indirection.

Reproduced the example directly: `(archive_override)(module_name = "apple_support", urls = ["x"])` — valid Starlark, since grouping parens around any expression are legal and the result can be called immediately — returned `False` from `_has_apple_support_override()` before the fix (should be `True`).

Took the second option the finding offered (fail closed for this specific callable-indirection form, rather than a full Starlark-aware parser/evaluator, which would be a much larger change than this guard warrants): added `_match_parenthesized_call()`, which recognizes `"(" name ")("` as an alternate way to find the same call, wired into the outer scan alongside the existing direct `name(` match (43f561d).

Verified: the indirection form is now detected, a longer identifier like `(archive_override_foo)(...)` is correctly rejected (not a false match), an indirected override on an unrelated module doesn't false-trigger, and the direct `archive_override(...)` form is unaffected. Two regression tests added.

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

🤖 Generated with Claude Code

[zzcgumn]

Proactively mirrored a fix from #423's identical `_decode_escape()`: a backslash immediately followed by an actual LF/CRLF line break in the source (as opposed to the two-character `\n` escape for a newline value) fell through to the default "leave as literal backslash + next character" branch. A `module_name` literal split across two source lines by a trailing backslash — valid Starlark, which drops the continuation entirely — kept both characters, producing a value that never equals the real module name and letting the override bypass the fail-closed guard.

Added the same LF/CRLF line-continuation decoding (both collapse to `""`), plus two mirrored regression tests (0e22b4f).

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

🤖 Generated with Claude Code

[Copilot]

🔵 Needs a closer look

Valid Starlark comments and raw string literals can cause incorrect override detection.

0 open findings

1 resolved since last review

• Reject callable indirection when scanning archive_override directives

Previously missed (1)

In code that hasn't changed since last review

Raw string literals are misclassified as unresolved names

python/​tests/​ci_macos_asan_test.py:360

This treats only a quote as a string-literal RHS, but Starlark also supports raw literals such as r"rules_cc". For archive_override(module_name = r"rules_cc", ...), the parser marks module_name unresolved and _has_apple_support_override incorrectly returns true, blocking a valid unrelated override. Recognize the raw-string prefix throughout tokenization and skip escape decoding for its contents.

🧠 Review effort: Balanced

[zzcgumn]

Addressed the latest Copilot finding: raw string literals.

`module_name = r"rules_cc"` — valid Starlark, a raw string literal — was read as a bare identifier `r` followed by a separate quoted string, falling through to the bare-identifier "unresolved" path, which made the guard incorrectly return `True` (fail closed) for an override on a completely unrelated module. Not a guard bypass, but a false-positive CI failure, as the finding's headline says.

Added `_is_raw_string_start()`/`_skip_raw_string()`/`_skip_any_string()`: per the Starlark spec, a raw string only gives `<quote>` special meaning (a literal quote, the string continues) — every other backslash, including one before a newline, is kept completely literal rather than decoded. Wired `_skip_any_string()` into the two places that decide whether a token is the argument's literal value (keyword RHS, first-positional capture) (4468a31).

Verified: the unrelated-module case no longer false-triggers, an `apple_support` raw literal is still detected (both keyword and positional forms, confirmed `r`/`R` prefixes and single/double quotes), a raw literal with an escaped quote (`r"apple_"support"`, a different string from "apple_support") correctly resolves to its true value rather than matching, and the plain-literal and real-bare-identifier cases are unaffected. Four regression tests added.

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

🤖 Generated with Claude Code

[zzcgumn]

Proactively mirrored a fix from #423's identical `_match_parenthesized_call()`: it used `_skip_ws()` around the grouped callable name's tokens, so a comment inside the grouping — `(archive_override # note\n)(module_name = "apple_support", ...)`, which Starlark ignores identically to the plain `(name)(` form — made this scanner miss the call and let the override bypass the guard.

Switched to `_skip_ws_and_comments()` (already used elsewhere in this file for the same reason), plus a mirrored regression test (0a416ab).

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

🤖 Generated with Claude Code

[Copilot]

🔵 Needs a closer look

The scanner mishandles valid raw triple-quoted strings and misses multiply grouped override calls.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Triple-quoted raw strings trigger false override detection

python/​tests/​ci_macos_asan_test.py:171

Raw triple-quoted literals are valid Starlark, but this deliberately parses only one-character raw-string delimiters. For archive_override(module_name = r"""rules_cc""", ...), _skip_raw_string stops at the first quote and marks the value unresolved; the fail-closed check then incorrectly reports an apple_support override, so an unrelated valid override breaks this CI guard. Handle triple delimiters as _skip_string does and add an unrelated-module regression case.

🧠 Review effort: Balanced

[zzcgumn]

Addressed the latest Copilot finding: raw triple-quoted strings.

`_skip_raw_string()` only handled a single-character delimiter, so `module_name = r"""rules_cc"""` stopped at the first lone quote inside the triple-quote run, leaving the value unresolved and making the fail-closed guard incorrectly return `True` for an override on a completely unrelated module. Reproduced directly against the Copilot-supplied example (returned `True`, should be `False`).

Extended `_skip_raw_string()` to match a triple-quote delimiter as the 3-character sequence, the same way `_skip_string()` already does for ordinary strings, keeping the raw `<quote>` escape rule (a backslash before a single quote character is still a literal quote that doesn't end the string) (91b57ff). Added both regression cases the finding asked for — an unrelated override and the `bazel_dep` name — plus one confirming `apple_support` via a raw triple-quote still matches, and confirmed the plain and ordinary-raw-string cases are unaffected.

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

🤖 Generated with Claude Code

[Copilot]

🔵 Needs a closer look

The scanner misses or mishandles several valid Starlark call and string forms.

0 open findings

Previously missed (2)

In code that hasn't changed since last review

Handle backslash parity when parsing raw-string closing quotes

python/​tests/​ci_macos_asan_test.py:184

Raw-string quote handling must account for the parity of the preceding backslash run. With a valid raw literal ending in two backslashes, such as strip_prefix = r"foo\\\\", the second backslash is currently treated as escaping the closing quote; parsing then consumes later arguments and marks an unrelated override unresolved, so the apple_support guard fails spuriously. Count consecutive backslashes and treat the quote as escaped only after an odd run (while preserving raw backslashes in the value).

Capture bazel_dep version from the second positional argument

python/​tests/​ci_macos_asan_test.py:468

bazel_dep also permits version as its second positional argument, but the scanner records only the first positional value. Consequently the valid declaration bazel_dep("apple_support", "2.10.1") makes this helper raise instead of verifying the pin. Capture positional arguments by index, or otherwise handle the second positional value here.

🧠 Review effort: Balanced