fix(ci): size the trailer gate's fetch to the PR so a release is scanned whole (#16931)

.github/workflows/no-commit-trailers.yml

@@ -50,7 +50,15 @@ jobs:
           # The depth is a guess about the future, so the scan refuses to run rather
           # than trust it: the reachability check below turns "too shallow" into a
           # loud failure instead of an empty range that reports clean.
-          fetch-depth: ${{ github.event_name == 'merge_group' && '0' || '100' }}
+          #
+          # #16931: sized per PR. A release promotion (main -> release) carries every
+          # commit since the last release -- #16801 had 808 -- so it exceeds any fixed
+          # depth chosen for ordinary PRs, and the guard below refused it. Full history
+          # is fetched only for a PR deeper than 90 commits: exactly the set for which
+          # depth 100 is guaranteed to fail anyway, so this never spends the #16207 cost
+          # on a PR that would have passed. Ordinary PRs keep depth 100. Expressions
+          # have comparison but no arithmetic, hence a threshold, not `commits + N`.
+          fetch-depth: ${{ (github.event_name == 'merge_group' || github.event.pull_request.commits > 90) && '0' || '100' }}
 
       - name: Scan PR commits for banned trailers
         env:
@@ -67,7 +75,7 @@ jobs:
           # seconds the shallow fetch saves, so it is checked explicitly (#16207).
           for _endpoint in "${BASE_SHA}" "${HEAD_SHA}"; do
             if ! git cat-file -e "${_endpoint}^{commit}" 2>/dev/null; then
-              echo "::error::${_endpoint} is not present at fetch-depth 100 -- this PR is"
+              echo "::error::${_endpoint} is not present in the checkout -- this PR is"
               echo "::error::deeper than the checkout. Raise fetch-depth in this workflow;"
               echo "::error::do NOT widen it back to 0 without re-measuring (#16207)."
               exit 1

changelog/unreleased/16931-trailer-gate-release-depth.md

@@ -0,0 +1,7 @@
+---
+type: fix
+scope: ci
+issue: 16931
+pr: 0000
+---
+The commit-trailer gate now fetches full history for a PR deeper than 90 commits. A release promotion carries every commit since the last release, so a fixed `fetch-depth: 100` left most of it unexamined and the gate — correctly — refused to report the truncated range as clean. Ordinary PRs keep depth 100, so the cost saving from #16207 is unchanged.

repo_tests/_github_expressions.py

@@ -0,0 +1,197 @@
+# Copyright 2025-2026 mrveiss
+# SPDX-License-Identifier: Apache-2.0
+# AutoBot - AI-Powered Automation Platform
+# Author: mrveiss
+"""A minimal evaluator for GitHub Actions' ``${{ }}`` expression syntax (#16931 review).
+
+The trailer gate's `fetch-depth` is chosen by an expression, not a literal:
+``(github.event_name == 'merge_group' || github.event.pull_request.commits > 90) && '0' || '100'``.
+Nothing exercised that expression -- an off-by-one (``>=`` for ``>``), a wrong
+field name, or a string-vs-number literal would go unnoticed by every existing
+guard on this workflow (#16932 review). Pinning the expression's *text* would
+not catch any of those, since the text would simply change along with the bug.
+
+This module evaluates the real fragment instead: parse it, walk it against a
+context dict shaped like GitHub's own `github` object, and return the value it
+actually produces -- including GitHub's JS-style short-circuiting, where `&&`
+and `||` return an *operand*, not a boolean (`'x' && '0'` is ``'0'``, not
+``True``). Reimplementing that loosely (e.g. coercing to Python bool) would
+silently pass a mutant that this workflow's own logic depends on: the `'0'`
+string is truthy specifically *because* GitHub does not fold it to a number.
+
+Scope is deliberately the fragment this one workflow needs: string/number
+literals, ``==``, ``>``, ``>=``, ``&&``, ``||``, parentheses, and dotted
+context lookups where a missing segment is null. Add operators only when a
+real expression needs them -- this is an anchor for one gate, not a GitHub
+Actions expression engine.
+"""
+
+from __future__ import annotations
+
+import re
+
+__all__ = ["evaluate", "strip_expression_braces"]
+
+_TOKEN_PATTERN = re.compile(
+    r"""
+      (?P<ws>\s+)
+    | (?P<op>==|>=|&&|\|\||>|\(|\))
+    | (?P<string>'(?:[^']|'')*')
+    | (?P<number>-?\d+(?:\.\d+)?)
+    | (?P<ident>[A-Za-z_][A-Za-z0-9_.]*)
+    """,
+    re.VERBOSE,
+)
+
+
+def strip_expression_braces(raw: str) -> str:
+    """Return the inner text of a ``${{ ... }}`` wrapper; raise if it is not one.
+
+    Raising rather than returning the raw text unchanged means a workflow edit
+    that drops the wrapper (e.g. hardcodes a literal) fails loudly here instead
+    of silently evaluating whatever text was left.
+    """
+    match = re.fullmatch(r"\$\{\{\s*(.*?)\s*\}\}", raw.strip())
+    if not match:
+        raise ValueError(f"not a GitHub expression wrapper: {raw!r}")
+    return match.group(1)
+
+
+def _tokenize(expr: str) -> list[tuple[str, str]]:
+    """Split an expression into ``(kind, text)`` tokens; reject unrecognized text.
+
+    A gap between consecutive matches (rather than looping on ``.match`` at a
+    cursor) would silently skip characters `finditer` could not match -- this
+    walks the match positions and fails on any gap instead.
+    """
+    tokens: list[tuple[str, str]] = []
+    pos = 0
+    for match in _TOKEN_PATTERN.finditer(expr):
+        if match.start() != pos:
+            raise ValueError(
+                f"unrecognized text at {pos} in {expr!r}: {expr[pos : match.start()]!r}"
+            )
+        pos = match.end()
+        if match.lastgroup != "ws":
+            tokens.append((match.lastgroup, match.group()))
+    if pos != len(expr):
+        raise ValueError(f"trailing unparsed text in {expr!r}: {expr[pos:]!r}")
+    return tokens
+
+
+def _lookup(context: dict, path: str) -> object:
+    """Dotted context lookup. A missing segment anywhere returns null (``None``)."""
+    value: object = context
+    for part in path.split("."):
+        if not isinstance(value, dict) or part not in value:
+            return None
+        value = value[part]
+    return value
+
+
+def _truthy(value: object) -> bool:
+    """GitHub's boolean coercion: null/false/0/'' are falsy, everything else is truthy.
+
+    Order matters: ``bool`` is a subclass of ``int`` in Python, so the bool
+    check must come first or ``False`` would fall into the numeric branch by
+    coincidence rather than by this function's own rule.
+    """
+    if isinstance(value, bool):
+        return value
+    if value is None:
+        return False
+    if isinstance(value, (int, float)):
+        return value != 0
+    if isinstance(value, str):
+        return value != ""
+    return True
+
+
+def _compare(op: str, left: object, right: object) -> bool:
+    """Evaluate ``==`` / ``>`` / ``>=``. A null operand compares false to anything."""
+    if left is None or right is None:
+        return False
+    if op == "==":
+        return left == right
+    if op == ">":
+        return left > right
+    if op == ">=":
+        return left >= right
+    raise ValueError(f"unsupported operator {op!r}")
+
+
+class _Parser:
+    """Recursive-descent parser over ``||`` / ``&&`` / comparison / primary.
+
+    That is GitHub's own precedence for this subset (loosest to tightest):
+    ``||``, then ``&&``, then the comparison operators, then literals/lookups/
+    parens.
+    """
+
+    def __init__(self, tokens: list[tuple[str, str]], context: dict) -> None:
+        self._tokens = tokens
+        self._pos = 0
+        self._context = context
+
+    def parse(self) -> object:
+        value = self._or()
+        if self._pos != len(self._tokens):
+            raise ValueError(f"trailing tokens: {self._tokens[self._pos :]!r}")
+        return value
+
+    def _peek_op(self, *ops: str) -> bool:
+        if self._pos >= len(self._tokens):
+            return False
+        kind, text = self._tokens[self._pos]
+        return kind == "op" and text in ops
+
+    def _or(self) -> object:
+        """``A || B``: GitHub returns ``A`` if truthy, else ``B`` -- not a boolean."""
+        value = self._and()
+        while self._peek_op("||"):
+            self._pos += 1
+            rhs = self._and()
+            value = value if _truthy(value) else rhs
+        return value
+
+    def _and(self) -> object:
+        """``A && B``: GitHub returns ``A`` if falsy, else ``B`` -- not a boolean."""
+        value = self._cmp()
+        while self._peek_op("&&"):
+            self._pos += 1
+            rhs = self._cmp()
+            value = rhs if _truthy(value) else value
+        return value
+
+    def _cmp(self) -> object:
+        value = self._primary()
+        if self._peek_op("==", ">", ">="):
+            op = self._tokens[self._pos][1]
+            self._pos += 1
+            value = _compare(op, value, self._primary())
+        return value
+
+    def _primary(self) -> object:
+        kind, text = self._tokens[self._pos]
+        self._pos += 1
+        if kind == "op" and text == "(":
+            return self._parenthesized()
+        if kind == "string":
+            return text[1:-1].replace("''", "'")
+        if kind == "number":
+            return float(text) if "." in text else int(text)
+        if kind == "ident":
+            return _lookup(self._context, text)
+        raise ValueError(f"unexpected token ({kind!r}, {text!r})")
+
+    def _parenthesized(self) -> object:
+        value = self._or()
+        if not self._peek_op(")"):
+            raise ValueError("unbalanced parentheses")
+        self._pos += 1
+        return value
+
+
+def evaluate(expr: str, context: dict) -> object:
+    """Evaluate a GitHub-expression fragment (no ``${{ }}`` wrapper) against a context dict."""
+    return _Parser(_tokenize(expr), context).parse()

repo_tests/no_commit_trailers_fetch_depth_test.py

@@ -0,0 +1,167 @@
+# Copyright 2025-2026 mrveiss
+# SPDX-License-Identifier: Apache-2.0
+# AutoBot - AI-Powered Automation Platform
+# Author: mrveiss
+"""The trailer gate's fetch-depth must see a release promotion whole (#16931 review).
+
+`no-commit-trailers.yml` used to check out at a fixed `fetch-depth: 100`. #16801
+(main -> release, 808 commits) exceeded it, and the gate refused to report a
+truncated range as clean -- correctly, by design (#16207). #16931 replaced the
+fixed depth with an expression: full history for `merge_group` or a PR over 90
+commits, depth 100 otherwise.
+
+Nothing exercised that expression before this file. The #16932 review flagged
+it as the one part of that PR shipping with zero automated evidence: an
+off-by-one (`>=` for `>`), a wrong field name, or a string-vs-number literal
+would go unnoticed by every other guard on this workflow, and the only thing
+that would ever notice is a live release run -- which is exactly the failure
+mode #16931 exists to stop happening *again*.
+
+This suite reads the live expression out of the workflow's YAML (`yaml.safe_load`,
+not a duplicated string) and evaluates it with `repo_tests._github_expressions`,
+a minimal GitHub-expression evaluator built for this purpose (no existing one was
+found in `repo_tests/` -- see the #16932 review thread). Pinning the expression's
+*text* would not catch a semantic regression, since the text changes along with
+the bug; evaluating it does.
+"""
+
+from __future__ import annotations
+
+from functools import lru_cache
+
+import pytest
+import yaml
+from repo_tests._github_expressions import evaluate, strip_expression_braces
+from repo_tests._paths import repo_root
+
+WORKFLOW = repo_root() / ".github" / "workflows" / "no-commit-trailers.yml"
+_CHECKOUT_STEP_NAME = "Checkout"
+
+
+@lru_cache(maxsize=1)
+def _fetch_depth_expr() -> str:
+    """The live `fetch-depth` expression, read from the checkout step's `with:`.
+
+    Reading through `yaml.safe_load` rather than a regex on the raw text means
+    this survives any YAML-legal reformatting of the step (key order, quoting)
+    that a text pin would not.
+    """
+    document = yaml.safe_load(WORKFLOW.read_text(encoding="utf-8"))
+    steps = document["jobs"]["check-trailers"]["steps"]
+    checkout = next(
+        (step for step in steps if step.get("name") == _CHECKOUT_STEP_NAME), None
+    )
+    assert checkout is not None, (
+        f"no {_CHECKOUT_STEP_NAME!r} step found -- this test cannot see what it checks"
+    )
+    raw = checkout["with"]["fetch-depth"]
+    return strip_expression_braces(raw)
+
+
+def _github_context(*, event_name: str, event: dict) -> dict:
+    """Wrap a fragment as `{"github": {...}}` -- the expression reads `github.*`."""
+    return {"github": {"event_name": event_name, "event": event}}
+
+
+def _merge_group_context() -> dict:
+    return _github_context(event_name="merge_group", event={})
+
+
+def _pull_request_context(*, commits: int) -> dict:
+    """A `pull_request` event context carrying only the field the expression reads."""
+    return _github_context(
+        event_name="pull_request", event={"pull_request": {"commits": commits}}
+    )
+
+
+def _push_context() -> dict:
+    """An event with no `pull_request` object at all.
+
+    This workflow is not wired to `push` (#16931 did not change that), but the
+    expression must not crash when `github.event.pull_request` is absent --
+    GitHub's own null-comparison rule is what keeps this safe, and that rule is
+    exactly what a naive reimplementation (e.g. raising on a missing key,
+    or coercing null to 0 and comparing) would get wrong silently.
+    """
+    return _github_context(event_name="push", event={})
+
+
+_FETCH_DEPTH_CASES = [
+    pytest.param(_merge_group_context(), "0", id="merge_group"),
+    pytest.param(
+        _pull_request_context(commits=89), "100", id="pull_request-89-commits"
+    ),
+    pytest.param(
+        _pull_request_context(commits=90), "100", id="pull_request-90-commits-boundary"
+    ),
+    pytest.param(
+        _pull_request_context(commits=91), "0", id="pull_request-91-commits-boundary"
+    ),
+    pytest.param(
+        _pull_request_context(commits=682),
+        "0",
+        id="pull_request-682-commits-release-sized",
+    ),
+    pytest.param(_push_context(), "100", id="push-no-pull-request-object"),
+]
+
+
+@pytest.mark.parametrize(("context", "expected"), _FETCH_DEPTH_CASES)
+def test_fetch_depth_matches_expected_value(context: dict, expected: str) -> None:
+    result = evaluate(_fetch_depth_expr(), context)
+    assert result == expected, (
+        f"context={context!r}: fetch-depth evaluated to {result!r}, expected {expected!r}"
+    )
+
+
+def test_mutated_operator_breaks_the_90_commit_boundary() -> None:
+    """Negative control: MUTATION TARGET is `>` -> `>=` in the live expression.
+
+    A 90-commit PR is the boundary #16931 drew: `> 90` keeps it at depth 100 (it
+    fits), `>= 90` would silently drop it to depth 0. That is exactly the class
+    of regression the #16932 review said nothing would catch. This constructs
+    the mutant from the LIVE text (not a hand-written copy, so it tracks any
+    wording change) and shows it produces a different, wrong value at the exact
+    boundary the suite above pins -- i.e. that boundary assertion would fail
+    against this mutant.
+    """
+    real = _fetch_depth_expr()
+    mutant = real.replace("commits > 90", "commits >= 90", 1)
+    assert mutant != real, (
+        "mutation target 'commits > 90' not found in the live expression -- update this test"
+    )
+    boundary = _pull_request_context(commits=90)
+    assert evaluate(real, boundary) == "100", (
+        "sanity: the real expression at the boundary should be depth 100"
+    )
+    assert evaluate(mutant, boundary) == "0", (
+        "a >= mutant silently drops a 90-commit PR to depth 0 -- undetected by "
+        "anything except this comparison, since #16932's own CI never ran a "
+        "90-commit PR through it"
+    )
+
+
+def test_mutated_field_name_breaks_the_91_commit_boundary() -> None:
+    """Negative control: MUTATION TARGET is the `pull_request.commits` field name.
+
+    A wrong field name (a typo, or a rename GitHub never ships) reads null from
+    every real payload, and null compares false to anything -- so the PR-depth
+    arm would never fire, and the mutant would ship as "always depth 100",
+    silently reintroducing the truncation #16931 was filed to fix. This shows
+    that at 91 commits, past the boundary, the mutant disagrees with the
+    correct value.
+    """
+    real = _fetch_depth_expr()
+    mutant = real.replace("pull_request.commits", "pull_request.changed_files", 1)
+    assert mutant != real, (
+        "mutation target 'pull_request.commits' not found in the live expression -- update this test"
+    )
+    past_boundary = _pull_request_context(commits=91)
+    assert evaluate(real, past_boundary) == "0", (
+        "sanity: the real expression past the boundary should be depth 0"
+    )
+    assert evaluate(mutant, past_boundary) == "100", (
+        "a wrong field name reads null and silently keeps depth 100 for a "
+        "91-commit PR -- the exact truncation #16931 was filed for, and the "
+        "boundary assertion above would not have caught it"
+    )