Merge e02c331b47 into 47cfd3f5ca

.github/scripts/check_pr_body.py

@@ -0,0 +1,166 @@
+#!/usr/bin/env python3
+"""Gate PR bodies on the machine-checkable rules that live inside HTML comments in
+.github/pull_request_template.md, since those comments are invisible in rendered PRs
+and are stripped from the template copies that agent harnesses inject into context."""
+
+from __future__ import annotations
+
+import argparse
+import os
+import re
+import sys
+from dataclasses import dataclass
+from functools import reduce
+from pathlib import Path
+from typing import Final
+
+BULLET_WORD_TARGET: Final = 10
+BULLET_WORD_CAP: Final = 14
+BULLET_SECTIONS: Final = ("tldr", "caveats (if any)")
+QA_RUNBOOK_SECTION: Final = "qa runbook"
+E2E_PREFIX: Final = "tests/e2e/"
+PLACEHOLDER_TOKENS: Final = ("<blah>",)
+NO_CAVEATS_PATTERN: Final = re.compile(r"^(none|n/?a)[.!]?$", re.IGNORECASE)
+HTML_COMMENT_PATTERN: Final = re.compile(r"<!--.*?-->", re.DOTALL)
+FENCE_PATTERN: Final = re.compile(r"^\s{0,3}(?:```|~~~)")
+HEADING_PATTERN: Final = re.compile(r"^#{2,6}\s+(?P<title>.+?)\s*$")
+BULLET_PATTERN: Final = re.compile(r"^\s*(?:[-*+]|\d+[.)])\s+(?P<text>.*)$")
+LABEL_PATTERN: Final = re.compile(r"^[^-*+].*:\s*$")
+
+
+@dataclass(frozen=True, slots=True)
+class Violation:
+    section: str
+    detail: str
+
+    def __str__(self) -> str:
+        return f"{self.section}: {self.detail}"
+
+
+@dataclass(frozen=True, slots=True)
+class Section:
+    title: str
+    lines: tuple[str, ...]
+
+
+def strip_html_comments(body: str) -> str:
+    return HTML_COMMENT_PATTERN.sub("", body.replace("\r\n", "\n"))
+
+
+def mask_fenced_blocks(body: str) -> str:
+    def step(acc: tuple[tuple[str, ...], bool], line: str) -> tuple[tuple[str, ...], bool]:
+        lines, in_fence = acc
+        if FENCE_PATTERN.match(line):
+            return (*lines, ""), not in_fence
+        return (*lines, "" if in_fence else line), in_fence
+
+    masked, _ = reduce(step, body.split("\n"), ((), False))
+    return "\n".join(masked)
+
+
+def split_sections(body: str) -> tuple[Section, ...]:
+    lines: Final = tuple(body.split("\n"))
+    headings: Final = tuple(
+        (index, match.group("title")) for index, line in enumerate(lines) if (match := HEADING_PATTERN.match(line))
+    )
+    ends: Final = tuple(index for index, _ in headings[1:]) + (len(lines),)
+    return tuple(Section(title=title, lines=lines[start + 1 : end]) for (start, title), end in zip(headings, ends))
+
+
+def group_bullets(lines: tuple[str, ...]) -> tuple[tuple[str, ...], tuple[str, ...]]:
+    def classify(acc: tuple[tuple[str, ...], tuple[str, ...]], line: str) -> tuple[tuple[str, ...], tuple[str, ...]]:
+        bullets, prose = acc
+        stripped: Final = line.strip()
+        if not stripped:
+            return acc
+        bullet_match: Final = BULLET_PATTERN.match(line)
+        if bullet_match:
+            return (*bullets, bullet_match.group("text").strip()), prose
+        if bullets and line[:1].isspace():
+            return (*bullets[:-1], f"{bullets[-1]} {stripped}"), prose
+        return bullets, (*prose, stripped)
+
+    return reduce(classify, lines, ((), ()))
+
+
+def check_bullet_section(section: Section) -> tuple[Violation, ...]:
+    bullets, prose = group_bullets(section.lines)
+    prose_violations: Final = tuple(
+        Violation(section.title, f'prose line "{line[:60]}" must be a short bullet instead')
+        for line in prose
+        if not LABEL_PATTERN.match(line) and not NO_CAVEATS_PATTERN.match(line)
+    )
+    length_violations: Final = tuple(
+        Violation(
+            section.title,
+            f'bullet "{bullet[:60]}" has {len(bullet.split())} words;'
+            f" keep bullets to roughly {BULLET_WORD_TARGET} words ({BULLET_WORD_CAP} max)",
+        )
+        for bullet in bullets
+        if len(bullet.split()) > BULLET_WORD_CAP
+    )
+    return prose_violations + length_violations
+
+
+def is_placeholder_line(line: str) -> bool:
+    bullet_match: Final = BULLET_PATTERN.match(line)
+    content: Final = bullet_match.group("text").strip() if bullet_match else line.strip()
+    return content == "..." or any(token in line for token in PLACEHOLDER_TOKENS)
+
+
+def check_placeholders(sections: tuple[Section, ...]) -> tuple[Violation, ...]:
+    return tuple(
+        Violation(section.title, f'template placeholder left in: "{line.strip()[:60]}"')
+        for section in sections
+        for line in section.lines
+        if is_placeholder_line(line)
+    )
+
+
+def check_qa_runbook(sections: tuple[Section, ...], changed_files: tuple[str, ...]) -> tuple[Violation, ...]:
+    has_runbook: Final = any(section.title.lower() == QA_RUNBOOK_SECTION for section in sections)
+    touches_e2e: Final = any(path.startswith(E2E_PREFIX) for path in changed_files)
+    if has_runbook and not touches_e2e:
+        return (
+            Violation(
+                "QA runbook",
+                "delete this section; the template only wants it when the PR edits tests/e2e",
+            ),
+        )
+    return ()
+
+
+def check_body(body: str, changed_files: tuple[str, ...]) -> tuple[Violation, ...]:
+    sections: Final = split_sections(mask_fenced_blocks(strip_html_comments(body)))
+    bullet_violations: Final = tuple(
+        violation
+        for section in sections
+        if section.title.lower() in BULLET_SECTIONS
+        for violation in check_bullet_section(section)
+    )
+    return bullet_violations + check_placeholders(sections) + check_qa_runbook(sections, changed_files)
+
+
+def main() -> int:
+    parser: Final = argparse.ArgumentParser()
+    parser.add_argument("--changed-files", type=Path, required=True)
+    args: Final = parser.parse_args()
+    body: Final = os.environ.get("PR_BODY", "")
+    changed_files: Final = tuple(
+        line.strip() for line in args.changed_files.read_text(encoding="utf-8").splitlines() if line.strip()
+    )
+    violations: Final = check_body(body, changed_files)
+    for violation in violations:
+        print(f"::error title=PR body template::{violation}")
+    if violations:
+        print(
+            "\nThe rules above come from the HTML comments inside .github/pull_request_template.md;"
+            " open that file to see every rule next to its section."
+        )
+        return 1
+    print("PR body follows the template comment rules.")
+    return 0
+
+
+if __name__ == "__main__":
+    sys.exit(main())

.github/workflows/pr-body-template.yml

@@ -0,0 +1,58 @@
+name: PR Body Template
+
+# The imperative rules in .github/pull_request_template.md live inside HTML
+# comments, which are invisible in the rendered PR and get stripped from the
+# template copies that agent harnesses inject into context, so authors (human
+# and AI) routinely miss them. This workflow turns the machine-checkable
+# subset into a real gate: TLDR and Caveats must be short bullets, no template
+# placeholders may remain, and the QA runbook section must be deleted unless
+# the PR edits tests/e2e.
+#
+# Skip with the ignore-pr-body-template label (mirrors conventional-commits).
+
+on:
+  pull_request:
+    types: [opened, edited, reopened, synchronize, labeled, unlabeled]
+
+permissions:
+  contents: read
+  pull-requests: read
+
+concurrency:
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.sha }}
+  cancel-in-progress: ${{ github.event_name == 'pull_request' }}
+
+jobs:
+  lint-pr-body:
+    name: Validate PR body
+    if: ${{ !endsWith(github.event.pull_request.user.login, '[bot]') && !contains(github.event.pull_request.labels.*.name, 'ignore-pr-body-template') }}
+    runs-on: ubuntu-latest
+    steps:
+      # The judging copy of the script comes from the base branch so a PR
+      # cannot weaken the checker that judges it. No fallback to a
+      # PR-controlled copy: if the base branch does not carry the script yet,
+      # the check step below skips with a notice instead.
+      - name: Checkout the base branch's lint script
+        uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0
+        with:
+          ref: ${{ github.event.pull_request.base.sha }}
+          sparse-checkout: .github/scripts
+          persist-credentials: false
+
+      - name: List changed files
+        env:
+          GH_TOKEN: ${{ github.token }}
+          REPO: ${{ github.repository }}
+          PR_NUMBER: ${{ github.event.pull_request.number }}
+        run: gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename' > changed_files.txt
+
+      - name: Check body against template comment rules
+        env:
+          PR_BODY: ${{ github.event.pull_request.body }}
+        run: |
+          SCRIPT=.github/scripts/check_pr_body.py
+          if [ ! -f "$SCRIPT" ]; then
+            echo "::notice title=PR body template::Base branch has no checker script yet; skipping until it merges"
+            exit 0
+          fi
+          python3 "$SCRIPT" --changed-files changed_files.txt

tests/code_coverage_tests/check_workflow_startup_safety.py

@@ -11,7 +11,8 @@ because CI cannot enforce them on itself.
    ``${{ a + b }}`` is a startup failure, not a value. Only ``+`` and ``*`` are
    flagged: ``-`` appears in hyphenated input names like ``inputs.timeout-minutes``
    and ``/`` inside ref strings, so neither can be told apart from arithmetic by
-   inspection alone.
+   inspection alone. A ``.*`` is the object-filter dereference (as in
+   ``labels.*.name``), not multiplication, so it is exempt.
 2. Callers of the reusable unit-test workflow keep the job timeout at or above
    the test budget plus the setup ceilings plus the runner overhead below.
    Otherwise the job deadline preempts pytest inside its own advertised budget,
@@ -43,6 +44,7 @@ JOB_OVERHEAD_MINUTES: Final = 5
 
 EXPRESSION: Final = re.compile(r"\$\{\{(?P<body>.*?)\}\}", re.DOTALL)
 QUOTED: Final = re.compile(r"'[^']*'")
+OBJECT_FILTER: Final = re.compile(r"\.\*")
 ARITHMETIC: Final = re.compile(r"[+*]")
 MATRIX_REF: Final = re.compile(r"^\$\{\{\s*matrix\.(?P<key>[\w-]+)\s*\}\}$")
 
@@ -75,7 +77,7 @@ def parse_workflow(text: str) -> WorkflowFile | str:
 def arithmetic_expressions(text: str) -> Iterator[str]:
     for match in EXPRESSION.finditer(text):
         body: Final = match.group("body")
-        if ARITHMETIC.search(QUOTED.sub("", body)):
+        if ARITHMETIC.search(OBJECT_FILTER.sub("", QUOTED.sub("", body))):
             yield body.strip()
 
 

tests/test_litellm/test_github_check_pr_body.py

@@ -0,0 +1,130 @@
+"""Unit tests for `.github/scripts/check_pr_body.py`."""
+
+from __future__ import annotations
+
+import importlib.util
+import sys
+from pathlib import Path
+
+import pytest
+
+SCRIPT_PATH = Path(__file__).resolve().parents[2] / ".github" / "scripts" / "check_pr_body.py"
+
+
+@pytest.fixture(scope="module")
+def checker():
+    spec = importlib.util.spec_from_file_location("check_pr_body", SCRIPT_PATH)
+    assert spec and spec.loader, f"Could not load spec for {SCRIPT_PATH}"
+    module = importlib.util.module_from_spec(spec)
+    sys.modules["check_pr_body"] = module
+    spec.loader.exec_module(module)
+    return module
+
+
+COMPLIANT_BODY = """## TLDR
+
+Problem this solves:
+
+- Any counts keep drifting toward their ceilings
+
+How it solves it:
+
+- Real types at each Any source across 56 files
+
+## Caveats (if any)
+
+- Rate limiter no longer crashes on usage-less responses
+
+### Final Attestation
+
+- [ ] The tests check the right things, including the edge cases, and regressions in the respective real-world customer use-cases are not possible after this PR
+"""
+
+
+def test_compliant_body_passes(checker):
+    assert checker.check_body(COMPLIANT_BODY, ("litellm/main.py",)) == ()
+
+
+def test_prose_caveats_fails(checker):
+    body = "## Caveats (if any)\n\nThree micro-hardenings ride along with the typing because honest annotation exposed them.\n"
+    violations = checker.check_body(body, ())
+    assert len(violations) == 1
+    assert "must be a short bullet" in violations[0].detail
+
+
+def test_overlong_bullet_fails(checker):
+    bullet = "- " + " ".join(f"word{i}" for i in range(15))
+    violations = checker.check_body(f"## Caveats (if any)\n\n{bullet}\n", ())
+    assert len(violations) == 1
+    assert "15 words" in violations[0].detail
+
+
+def test_wrapped_bullet_continuation_counts_into_its_bullet(checker):
+    body = "## Caveats (if any)\n\n- short bullet that\n  wraps onto an indented line\n"
+    assert checker.check_body(body, ()) == ()
+
+
+def test_none_line_in_caveats_passes(checker):
+    assert checker.check_body("## Caveats (if any)\n\nNone\n", ()) == ()
+
+
+def test_qa_runbook_without_e2e_changes_fails(checker):
+    body = "## QA runbook\n\n- some manual step\n"
+    violations = checker.check_body(body, ("litellm/main.py",))
+    assert len(violations) == 1
+    assert "delete this section" in violations[0].detail
+
+
+def test_qa_runbook_with_e2e_changes_passes(checker):
+    body = "## QA runbook\n\n- some manual step\n"
+    assert checker.check_body(body, ("tests/e2e/test_thing.py",)) == ()
+
+
+def test_leftover_placeholder_fails(checker):
+    violations = checker.check_body("## TLDR\n\nProblem this solves:\n\n- <blah>\n- ...\n", ())
+    assert len(violations) == 2
+    assert all("placeholder" in violation.detail for violation in violations)
+
+
+def test_html_comments_are_ignored(checker):
+    body = "## Caveats (if any)\n\n<!-- Short bullet points, just like the TLDR: one line per bullet -->\n"
+    assert checker.check_body(body, ()) == ()
+
+
+def test_final_attestation_checkbox_is_not_a_caveat(checker):
+    body = (
+        "## Caveats (if any)\n\n- a real caveat bullet\n\n### Final Attestation\n\n"
+        "- [ ] The tests check the right things, including the edge cases, and regressions"
+        " in the respective real-world customer use-cases are not possible after this PR\n"
+    )
+    assert checker.check_body(body, ()) == ()
+
+
+def test_empty_body_passes(checker):
+    assert checker.check_body("", ()) == ()
+
+
+def test_ellipsis_inside_code_fence_is_not_a_placeholder(checker):
+    body = "## Screenshots / Proof of Fix\n\n```\n$ curl http://localhost:4000/health\n...\n{\"status\": \"ok\"}\n```\n"
+    assert checker.check_body(body, ()) == ()
+
+
+def test_headings_inside_code_fence_do_not_split_sections(checker):
+    body = (
+        "## Screenshots / Proof of Fix\n\nThe old body looked like this:\n\n```\n## TLDR\n\n- <blah>\n- ...\n\n"
+        "## Caveats (if any)\n\nA long prose paragraph that would fail the bullet rule if parsed.\n```\n"
+    )
+    assert checker.check_body(body, ()) == ()
+
+
+def test_ellipsis_outside_fences_still_fails(checker):
+    violations = checker.check_body("## TLDR\n\nProblem this solves:\n\n- ...\n", ())
+    assert len(violations) == 1
+    assert "placeholder" in violations[0].detail
+
+
+def test_multiline_html_comment_spanning_section_is_stripped(checker):
+    body = "## QA runbook\n\n<!-- Only needed when your PR edits tests/e2e; delete this section otherwise\n\nExample:\n\n- step one\n-->\n"
+    violations = checker.check_body(body, ("litellm/main.py",))
+    assert len(violations) == 1
+    assert violations[0].section == "QA runbook"