mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-10 22:41:41 +00:00
fix: evaluate job name expressions in the check-run collision guard
The guard only substituted a bare `${{ matrix.key }}`, so any name built from a
larger expression stayed in the string as its own template. `_test-unit-base.yml`
names its job with a ternary over `format()`, which meant every shard published
an opaque name and 23 of the 33 required contexts, all of them `<shard> / Run
tests`, were invisible to the very check meant to protect them.
Job names are now evaluated per matrix combination over the pieces a name can
hold: string literals, `matrix.<key>`, `format()`, `==` and `!=`, and the
`<cond> && <a> || <b>` idiom. All 33 required contexts now resolve, and nothing
in the repo leaves an expression unresolved. An expression the evaluator does not
understand still falls back to its verbatim template, so two jobs sharing one
stays a collision.
The test also drops its `sys.path.insert`, which the test-quality budget counts
under TQ003; pytest already puts the file's own directory on the path.
This commit is contained in:
mateo-berri
parent
44b7a76439
commit
baee7d8175
2 changed files with 152 additions and 20 deletions
|
|
@ -12,13 +12,17 @@ and commit ed5761daef4ae17152446d182c860630c38b7268 carried both check runs.
|
|||
This invariant has to be enforced here because CI cannot enforce it on itself.
|
||||
|
||||
A job publishes its `name:` when it sets one and its job id otherwise. Two shapes
|
||||
expand that further. A `${{ matrix.key }}` reference publishes one check run per
|
||||
value the matrix supplies, so two shard lists that overlap collide even though
|
||||
their templates read differently. A job calling a local reusable workflow
|
||||
publishes one check run per job of the callee, named `<caller> / <callee>`, which
|
||||
is why a caller's name never collides with a plain job that happens to match it.
|
||||
A reference nothing resolves stays in the string, so two jobs carrying the same
|
||||
unresolved template still compare equal and their collision is still caught.
|
||||
expand that further. A name carrying `${{ ... }}` publishes one check run per
|
||||
matrix combination it reads, so two shard lists that overlap collide even though
|
||||
their templates read differently. Each expression is evaluated per combination
|
||||
over the pieces a job name can hold: string literals, `matrix.<key>`, `format()`,
|
||||
`==` and `!=`, and the `<cond> && <a> || <b>` idiom, which is how the shards
|
||||
reach their real `<shard> / Run tests` names rather than staying opaque. A job
|
||||
calling a local reusable workflow publishes one check run per job of the callee,
|
||||
named `<caller> / <callee>`, which is why a caller's name never collides with a
|
||||
plain job that happens to match it. An expression nothing resolves stays in the
|
||||
string, so two jobs carrying the same unresolved template still compare equal and
|
||||
their collision is still caught.
|
||||
"""
|
||||
|
||||
import itertools
|
||||
|
|
@ -27,6 +31,7 @@ import re
|
|||
import sys
|
||||
from collections.abc import Iterator, Mapping, Sequence
|
||||
from pathlib import Path
|
||||
from types import MappingProxyType
|
||||
from typing import Final
|
||||
|
||||
import yaml
|
||||
|
|
@ -34,7 +39,13 @@ from pydantic import BaseModel, Field, ValidationError
|
|||
|
||||
REPO_ROOT: Final = Path(__file__).resolve().parent.parent.parent
|
||||
WORKFLOWS_DIR: Final = REPO_ROOT / ".github" / "workflows"
|
||||
MATRIX_REF: Final = re.compile(r"\$\{\{\s*matrix\.(?P<key>[\w-]+)\s*\}\}")
|
||||
EXPRESSION: Final = re.compile(r"\$\{\{(?P<body>.*?)\}\}", re.DOTALL)
|
||||
MATRIX_KEY: Final = re.compile(r"matrix\.(?P<key>[\w-]+)")
|
||||
MATRIX_REF: Final = re.compile(r"^matrix\.(?P<key>[\w-]+)$")
|
||||
LITERAL: Final = re.compile(r"^'(?P<text>[^']*)'$")
|
||||
FORMAT_CALL: Final = re.compile(r"^format\((?P<args>.*)\)$", re.DOTALL)
|
||||
COMPARISON: Final = re.compile(r"^(?P<left>.+?)\s*(?P<operator>==|!=)\s*(?P<right>.+)$", re.DOTALL)
|
||||
NO_MATRIX: Final[Mapping[str, str]] = MappingProxyType({})
|
||||
LOCAL_CALL_PREFIX: Final = "./"
|
||||
|
||||
|
||||
|
|
@ -97,19 +108,85 @@ def matrix_values(job: Job, key: str) -> tuple[str, ...]:
|
|||
return tuple(dict.fromkeys(from_list + from_rows))
|
||||
|
||||
|
||||
def substituted(template: str, values: Mapping[str, str]) -> str:
|
||||
return MATRIX_REF.sub(lambda ref: values.get(ref.group("key"), ref.group(0)), template)
|
||||
def scanned(state: tuple[int, bool], char: str) -> tuple[int, bool]:
|
||||
depth, quoted = state
|
||||
if char == "'":
|
||||
return depth, not quoted
|
||||
if quoted:
|
||||
return depth, quoted
|
||||
return depth + int(char == "(") - int(char == ")"), quoted
|
||||
|
||||
|
||||
def split_outside(text: str, token: str) -> tuple[str, ...]:
|
||||
"""`text` cut on every `token` that sits outside quotes and parentheses."""
|
||||
states: Final = tuple(itertools.accumulate(text, scanned, initial=(0, False)))
|
||||
cuts: Final = tuple(
|
||||
index
|
||||
for index in range(len(text) - len(token) + 1)
|
||||
if text.startswith(token, index) and states[index] == (0, False)
|
||||
)
|
||||
starts: Final = (0, *(cut + len(token) for cut in cuts))
|
||||
return tuple(text[start:end] for start, end in zip(starts, (*cuts, len(text))))
|
||||
|
||||
|
||||
def value_of(text: str, values: Mapping[str, str]) -> str | None:
|
||||
expression: Final = text.strip()
|
||||
literal: Final = LITERAL.match(expression)
|
||||
if literal is not None:
|
||||
return literal.group("text")
|
||||
reference: Final = MATRIX_REF.match(expression)
|
||||
if reference is not None:
|
||||
return values.get(reference.group("key"))
|
||||
call: Final = FORMAT_CALL.match(expression)
|
||||
if call is None:
|
||||
return None
|
||||
arguments: Final = tuple(value_of(part, values) for part in split_outside(call.group("args"), ","))
|
||||
resolved: Final = tuple(argument for argument in arguments if argument is not None)
|
||||
if not resolved or len(resolved) != len(arguments):
|
||||
return None
|
||||
return resolved[0].format(*resolved[1:])
|
||||
|
||||
|
||||
def holds(condition: str, values: Mapping[str, str]) -> bool | None:
|
||||
comparison: Final = COMPARISON.match(condition.strip())
|
||||
if comparison is None:
|
||||
return None
|
||||
left: Final = value_of(comparison.group("left"), values)
|
||||
right: Final = value_of(comparison.group("right"), values)
|
||||
if left is None or right is None:
|
||||
return None
|
||||
return (left == right) == (comparison.group("operator") == "==")
|
||||
|
||||
|
||||
def evaluate(body: str, values: Mapping[str, str]) -> str | None:
|
||||
"""The single string this expression yields, or None when its shape is not understood."""
|
||||
branches: Final = tuple(split_outside(alternative, "&&") for alternative in split_outside(body, "||"))
|
||||
outcomes: Final = tuple(tuple(holds(part, values) for part in branch[:-1]) for branch in branches)
|
||||
if any(outcome is None for branch in outcomes for outcome in branch):
|
||||
return None
|
||||
taken: Final = next((branch[-1] for branch, outcome in zip(branches, outcomes) if all(outcome)), None)
|
||||
return None if taken is None else value_of(taken, values)
|
||||
|
||||
|
||||
def resolved_span(span: re.Match[str], values: Mapping[str, str]) -> str:
|
||||
substitution: Final = evaluate(span.group("body"), values)
|
||||
return span.group(0) if substitution is None else substitution
|
||||
|
||||
|
||||
def rendered(template: str, values: Mapping[str, str]) -> str:
|
||||
return EXPRESSION.sub(lambda span: resolved_span(span, values), template)
|
||||
|
||||
|
||||
def expand(template: str, job: Job) -> tuple[str, ...]:
|
||||
keys: Final = tuple(dict.fromkeys(ref.group("key") for ref in MATRIX_REF.finditer(template)))
|
||||
candidates: Final = tuple((key, matrix_values(job, key)) for key in keys)
|
||||
resolvable: Final = tuple((key, values) for key, values in candidates if values)
|
||||
keys: Final = tuple(dict.fromkeys(ref.group("key") for ref in MATRIX_KEY.finditer(template)))
|
||||
resolvable: Final = tuple((key, values) for key in keys if (values := matrix_values(job, key)))
|
||||
if not resolvable:
|
||||
return (template,)
|
||||
return (rendered(template, NO_MATRIX),)
|
||||
return tuple(
|
||||
substituted(template, dict(zip((key for key, _ in resolvable), combination)))
|
||||
for combination in itertools.product(*(values for _, values in resolvable))
|
||||
dict.fromkeys(
|
||||
rendered(template, dict(zip((key for key, _ in resolvable), combination)))
|
||||
for combination in itertools.product(*(values for _, values in resolvable))
|
||||
)
|
||||
)
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -1,10 +1,30 @@
|
|||
import sys
|
||||
from pathlib import Path
|
||||
from typing import Final
|
||||
|
||||
sys.path.insert(0, str(Path(__file__).resolve().parent))
|
||||
from check_workflow_job_name_collisions import callee_path, collisions, parse, published, workflow_sources
|
||||
|
||||
from check_workflow_job_name_collisions import callee_path, collisions, parse, workflow_sources # noqa: E402
|
||||
REUSABLE_BASE: Final = """on:
|
||||
workflow_call:
|
||||
jobs:
|
||||
run:
|
||||
name: >-
|
||||
${{ matrix.python-version == '3.12' && 'Run tests'
|
||||
|| format('Run tests (Python {0})', matrix.python-version) }}
|
||||
runs-on: ubuntu-latest
|
||||
strategy:
|
||||
matrix:
|
||||
python-version: ["3.12", "3.13"]
|
||||
"""
|
||||
|
||||
SHARD_CALLER: Final = """on: pull_request
|
||||
jobs:
|
||||
unit:
|
||||
name: ${{ matrix.shard }}
|
||||
uses: ./.github/workflows/base.yml
|
||||
strategy:
|
||||
matrix:
|
||||
include:
|
||||
- shard: core-utils
|
||||
"""
|
||||
|
||||
|
||||
def test_every_workflow_in_the_repo_publishes_a_unique_check_run_name() -> None:
|
||||
|
|
@ -209,3 +229,38 @@ def test_a_file_that_is_not_a_workflow_is_ignored() -> None:
|
|||
}
|
||||
|
||||
assert collisions(sources) == ()
|
||||
|
||||
|
||||
def test_a_conditional_name_expands_to_the_branch_each_matrix_value_takes() -> None:
|
||||
names: Final = frozenset(
|
||||
name for name, _ in published({".github/workflows/base.yml": REUSABLE_BASE, "unit.yml": SHARD_CALLER})
|
||||
)
|
||||
|
||||
assert names == frozenset({"core-utils / Run tests", "core-utils / Run tests (Python 3.13)"})
|
||||
|
||||
|
||||
def test_a_conditional_name_never_publishes_the_branch_its_condition_rules_out() -> None:
|
||||
names: Final = frozenset(
|
||||
name for name, _ in published({".github/workflows/base.yml": REUSABLE_BASE, "unit.yml": SHARD_CALLER})
|
||||
)
|
||||
|
||||
assert "core-utils / Run tests (Python 3.12)" not in names
|
||||
|
||||
|
||||
def test_a_conditional_reusable_name_collides_with_a_plain_job_publishing_the_same_name() -> None:
|
||||
sources: Final = {
|
||||
".github/workflows/base.yml": REUSABLE_BASE,
|
||||
"unit.yml": SHARD_CALLER,
|
||||
"postgres.yml": ("on: pull_request\njobs:\n legacy:\n name: core-utils / Run tests\n"),
|
||||
}
|
||||
|
||||
found: Final = collisions(sources)
|
||||
|
||||
assert len(found) == 1
|
||||
assert "`core-utils / Run tests` is published by 2 jobs" in found[0]
|
||||
|
||||
|
||||
def test_every_workflow_in_the_repo_resolves_every_expression_in_its_job_names() -> None:
|
||||
unresolved: Final = tuple(f"{owner}: {name}" for name, owner in published(workflow_sources()) if "${{" in name)
|
||||
|
||||
assert unresolved == ()
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue