feat(ci): assert .github/workflows holds only workflows, correctly named (#37616)

* feat(ci): assert .github/workflows holds only workflows, correctly named

* style(tests): annotate the hygiene test module's names with Final

* fix(ci): report a .yaml workflow as a naming finding, not a stray

GitHub reads .yml and .yaml alike, so WF001 telling you to move a valid
.yaml workflow to .github/scripts/ was wrong advice. WF001 now covers only
files that are not workflows at all, and the .yml spelling this directory
keeps moves to WF004, which says to rename rather than relocate.

WF001 also never looked into subdirectories, since GitHub does not read
them either; the message now says so. The directory is injected rather
than read off a module constant, so the cases are testable without
monkeypatching.

.github/scripts/assert_workflow_dir_hygiene.py

@@ -0,0 +1,149 @@
+#!/usr/bin/env python3
+"""Three invariants about what lives in .github/workflows/ and what its names mean.
+
+`.github/workflows/` is a directory GitHub reads, not a place to keep things. Every
+file at its top level is parsed as a workflow, so a script or a data file parked there
+is either an invalid workflow or an orphan nobody can find. A subdirectory is not read
+at all, so helper files may live in one. GitHub accepts both `.yml` and `.yaml`, and
+this repo spells them `.yml`, which is a naming rule rather than a validity one and is
+reported separately. And the `_` prefix is the repo's only signal that a workflow is a
+reusable building block rather than something that runs on its own, which is worth
+nothing unless it is true both ways.
+
+    WF001   a top-level file in .github/workflows/ that is not a workflow at all
+    WF002   a workflow whose only trigger is `workflow_call` but is not `_`-prefixed
+    WF003   a `_`-prefixed workflow that no other workflow can call
+    WF004   a real workflow spelled `.yaml` where this directory spells them `.yml`
+
+A workflow with `workflow_call` alongside a human trigger is deliberately dual-mode
+and belongs under its plain name, so only the call-only ones are held to WF002.
+
+Usage
+-----
+    python assert_workflow_dir_hygiene.py
+
+Exit code 1 if any violation is found.
+"""
+
+from __future__ import annotations
+
+import pathlib
+import sys
+from dataclasses import dataclass
+from typing import Final
+
+import yaml
+
+REPO_ROOT: Final = pathlib.Path(__file__).resolve().parents[2]
+WORKFLOW_DIR: Final = REPO_ROOT / ".github" / "workflows"
+SCRIPT_HOME: Final = ".github/scripts/"
+REUSABLE_PREFIX: Final = "_"
+CALL_TRIGGER: Final = "workflow_call"
+CANONICAL_SUFFIX: Final = ".yml"
+WORKFLOW_SUFFIXES: Final = frozenset((CANONICAL_SUFFIX, ".yaml"))
+
+
+@dataclass(frozen=True, slots=True)
+class Finding:
+    subject: str
+    code: str
+    detail: str
+
+    def render(self) -> str:
+        return f"  - {self.subject}: {self.code} {self.detail}"
+
+
+def _triggers(document: object) -> frozenset[str]:
+    if not isinstance(document, dict):
+        return frozenset()
+    raw: Final = document.get("on", document.get(True))
+    if isinstance(raw, str):
+        return frozenset({raw})
+    if isinstance(raw, dict):
+        return frozenset(str(key) for key in raw)
+    if isinstance(raw, list):
+        return frozenset(str(item) for item in raw)
+    return frozenset()
+
+
+def _workflows(directory: pathlib.Path) -> tuple[pathlib.Path, ...]:
+    return tuple(
+        path
+        for path in sorted(directory.iterdir())
+        if path.is_file() and path.suffix in WORKFLOW_SUFFIXES
+    )
+
+
+def _strays(directory: pathlib.Path) -> tuple[Finding, ...]:
+    return tuple(
+        Finding(
+            path.name,
+            "WF001",
+            f"is not a workflow, and GitHub parses every top-level file here as one; "
+            f"move it to {SCRIPT_HOME} or into a subdirectory, which GitHub does not read",
+        )
+        for path in sorted(directory.iterdir())
+        if path.is_file() and path.suffix not in WORKFLOW_SUFFIXES
+    )
+
+
+def _misspelled(directory: pathlib.Path) -> tuple[Finding, ...]:
+    return tuple(
+        Finding(
+            path.name,
+            "WF004",
+            f"is a real workflow and GitHub reads it, but this directory spells them "
+            f"{CANONICAL_SUFFIX}; rename it to {path.stem}{CANONICAL_SUFFIX}",
+        )
+        for path in _workflows(directory)
+        if path.suffix != CANONICAL_SUFFIX
+    )
+
+
+def _misnamed(directory: pathlib.Path) -> tuple[Finding, ...]:
+    return tuple(
+        finding
+        for path in _workflows(directory)
+        for finding in _naming_findings(path, _triggers(yaml.safe_load(path.read_text(encoding="utf-8"))))
+    )
+
+
+def _naming_findings(path: pathlib.Path, triggers: frozenset[str]) -> tuple[Finding, ...]:
+    underscored: Final = path.name.startswith(REUSABLE_PREFIX)
+    if triggers == frozenset({CALL_TRIGGER}) and not underscored:
+        return (
+            Finding(
+                path.name,
+                "WF002",
+                f"is only callable by another workflow, so name it {REUSABLE_PREFIX}{path.name}",
+            ),
+        )
+    if underscored and CALL_TRIGGER not in triggers:
+        return (
+            Finding(
+                path.name,
+                "WF003",
+                f"is named as a reusable workflow but has no {CALL_TRIGGER} trigger; "
+                "add one or drop the prefix",
+            ),
+        )
+    return ()
+
+
+def main() -> int:
+    findings: Final = _strays(WORKFLOW_DIR) + _misspelled(WORKFLOW_DIR) + _misnamed(WORKFLOW_DIR)
+    if not findings:
+        total: Final = len(_workflows(WORKFLOW_DIR))
+        sys.stdout.write(
+            f"OK: {total} workflows, every file in .github/workflows/ is one, and the "
+            f"{REUSABLE_PREFIX} prefix means callable in both directions.\n"
+        )
+        return 0
+    sys.stdout.write("ERROR: .github/workflows/ holds files that break its own conventions\n")
+    for finding in findings:
+        sys.stdout.write(f"{finding.render()}\n")
+    return 1
+
+
+if __name__ == "__main__":
+    sys.exit(main())

.github/workflows/ci-coverage.yml

@@ -46,3 +46,6 @@ jobs:
       # nowhere while counting as covered, which is how the caching suite went unrun.
       - name: Assert no -k expression deselects a file from every job that globs it
         run: python .github/scripts/assert_ci_coverage.py --slices
+
+      - name: Assert .github/workflows/ holds only workflows, correctly named
+        run: python .github/scripts/assert_workflow_dir_hygiene.py

tests/test_litellm/test_assert_workflow_dir_hygiene.py

@@ -0,0 +1,115 @@
+"""Tests for .github/scripts/assert_workflow_dir_hygiene.py."""
+
+import importlib.util
+import sys
+from pathlib import Path
+from typing import Final
+
+import pytest
+
+_REPO_ROOT: Final = Path(__file__).resolve().parents[2]
+_MODULE_PATH: Final = _REPO_ROOT / ".github" / "scripts" / "assert_workflow_dir_hygiene.py"
+_spec: Final = importlib.util.spec_from_file_location("assert_workflow_dir_hygiene", _MODULE_PATH)
+hygiene: Final = importlib.util.module_from_spec(_spec)
+sys.modules[_spec.name] = hygiene  # @dataclass(slots=True) rebuilds via sys.modules
+_spec.loader.exec_module(hygiene)
+
+
+def _codes(path_name, triggers):
+    return [f.code for f in hygiene._naming_findings(Path(path_name), frozenset(triggers))]
+
+
+def test_a_call_only_workflow_without_the_prefix_is_flagged():
+    assert _codes("deploy.yml", {"workflow_call"}) == ["WF002"]
+
+
+def test_a_call_only_workflow_with_the_prefix_is_clean():
+    assert _codes("_deploy.yml", {"workflow_call"}) == []
+
+
+def test_a_dual_mode_workflow_keeps_its_plain_name():
+    # workflow_call plus a human trigger is deliberate: the `_` prefix would hide a
+    # workflow someone is meant to be able to dispatch.
+    assert _codes("create-release-branch.yml", {"workflow_call", "workflow_dispatch"}) == []
+
+
+def test_a_prefixed_workflow_nobody_can_call_is_flagged():
+    assert _codes("_helper.yml", {"push"}) == ["WF003"]
+
+
+def test_a_plain_workflow_with_ordinary_triggers_is_clean():
+    assert _codes("test-unit.yml", {"pull_request", "push"}) == []
+
+
+@pytest.mark.parametrize(
+    "raw, expected",
+    [
+        ({"on": "push"}, {"push"}),
+        ({"on": ["push", "pull_request"]}, {"push", "pull_request"}),
+        ({"on": {"workflow_call": None}}, {"workflow_call"}),
+        ({True: {"pull_request": None}}, {"pull_request"}),
+        ({"jobs": {}}, set()),
+        ("not a mapping", set()),
+    ],
+)
+def test_triggers_reads_every_shape_the_on_key_takes(raw, expected):
+    # YAML 1.1 turns a bare `on:` key into the boolean True, which is why the loaded
+    # document has to be read both ways.
+    assert hygiene._triggers(raw) == frozenset(expected)
+
+
+def test_the_repo_as_it_stands_holds_only_workflows_in_the_workflow_dir():
+    assert [f.subject for f in hygiene._strays(hygiene.WORKFLOW_DIR)] == []
+
+
+def test_the_repo_as_it_stands_names_every_reusable_workflow_with_the_prefix():
+    assert [f.subject for f in hygiene._misnamed(hygiene.WORKFLOW_DIR)] == []
+
+
+def test_the_repo_as_it_stands_spells_every_workflow_yml():
+    assert [f.subject for f in hygiene._misspelled(hygiene.WORKFLOW_DIR)] == []
+
+
+_WORKFLOW: Final = "name: ci\non: [push]\njobs:\n  a:\n    runs-on: ubuntu-latest\n    steps: [{run: 'true'}]\n"
+
+
+def _populate(directory, files):
+    for name, body in files.items():
+        target = directory / name
+        target.parent.mkdir(parents=True, exist_ok=True)
+        target.write_text(body, encoding="utf-8")
+    return directory
+
+
+def _findings(directory):
+    return [
+        (f.subject, f.code)
+        for f in hygiene._strays(directory) + hygiene._misspelled(directory) + hygiene._misnamed(directory)
+    ]
+
+
+def test_a_script_at_the_top_level_is_a_stray(tmp_path):
+    directory = _populate(tmp_path, {"ci.yml": _WORKFLOW, "render.py": "print(1)\n"})
+    assert _findings(directory) == [("render.py", "WF001")]
+
+
+def test_a_script_inside_a_subdirectory_is_left_alone(tmp_path):
+    directory = _populate(tmp_path, {"ci.yml": _WORKFLOW, "helpers/render.py": "print(1)\n"})
+    assert _findings(directory) == []
+
+
+def test_a_yaml_workflow_is_a_naming_finding_not_a_stray(tmp_path):
+    directory = _populate(tmp_path, {"test-model-map.yaml": _WORKFLOW})
+    assert _findings(directory) == [("test-model-map.yaml", "WF004")]
+
+
+def test_the_yaml_message_names_the_rename_and_not_the_scripts_directory(tmp_path):
+    directory = _populate(tmp_path, {"test-model-map.yaml": _WORKFLOW})
+    detail = hygiene._misspelled(directory)[0].detail
+    assert "test-model-map.yml" in detail
+    assert hygiene.SCRIPT_HOME not in detail
+
+
+def test_a_yaml_workflow_is_still_held_to_the_prefix_rules(tmp_path):
+    directory = _populate(tmp_path, {"deploy.yaml": "on: {workflow_call: null}\njobs: {}\n"})
+    assert _findings(directory) == [("deploy.yaml", "WF004"), ("deploy.yaml", "WF002")]