Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions .github/workflows/actionlint.yml
Original file line numberDiff line numberDiff line change
Expand Up@@ -6,6 +6,7 @@ on:
pull_request:
paths:
- ".github/workflows/**"
- "scripts/**"

permissions:
contents: read
Expand All@@ -32,3 +33,40 @@ jobs:
chmod +x actionlint
- name: Run actionlint
run: ./actionlint -color .github/workflows/*.yml

# WHY here rather than in a job of its own: actionlint answers "is this
# workflow well-formed before 11 repos inherit it", and so does this —
# same question, same trigger, no second checkout. actionlint type-checks
# the expressions but never evaluates them, so the gate's verdict ladder
# is outside what it can see.
#
# WARNING: this workflow is path-filtered. Do not add it to required
# status checks without dropping the `paths:` filter first — a required
# check that never runs blocks its PR permanently, with nothing failing
# and nothing pending to point at.
- name: Check the gate verdict against the event matrix
run: |
python3 -m pip install --quiet --disable-pip-version-check pyyaml
python3 scripts/check_gate_evaluation.py \
.github/workflows/hybrid-gate.yml

event-shape-guards:
name: event-shape-guards
# WHY: actionlint type-checks expressions but cannot see that a
# pull_request-only context is empty on a push, which is the one class of
# defect that has shipped from this repo twice (#25 and the check-trailer
# step it missed). Runs beside actionlint because both answer "is this
# workflow correct before 11 repos inherit it".
#
# WARNING: this workflow is path-filtered, so it does not run on every PR.
# Do not add either job to required status checks without dropping the
# `paths:` filter first — a required check that never runs blocks its PR
# permanently, with nothing failing and nothing pending to point at.
runs-on: ubuntu-latest
timeout-minutes: 5
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
persist-credentials: false
- name: Check pull_request-only values reaching git
run: python3 scripts/check_event_shape_guards.py .github/workflows
12 changes: 11 additions & 1 deletion .github/workflows/gate-attestation.yml
Original file line numberDiff line numberDiff line change
Expand Up@@ -65,7 +65,17 @@ jobs:
env:
# WHY: head.sha reaches the script via env, not shell interpolation
# (expression-injection rule). It is server-populated + content-addressed.
PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }}
# WHY the fallback: the context is populated on pull_request events
# only, and `git log -1 --format="%b" ""` exits 128 with
# `fatal: ambiguous argument ''`, so a caller triggering this reusable
# on a push got an unreadable git crash instead of a verdict.
# WARNING: the fallback makes a push-triggered run legible, not green.
# This reusable has no full-gate-build fallback, so it fails whenever
# the pushed commit carries no trailer — and squash-merged commits
# usually do not. Callers wanting default-branch coverage call
# hybrid-gate.yml, which routes an untrailered tip to a real build
# instead.
PR_HEAD_SHA: ${{ github.event.pull_request.head.sha || github.sha }}
run: |
body=$(git log -1 --format="%b" "$PR_HEAD_SHA")
if echo "$body" | grep -q "^Gate-Passed:"; then
Expand Down
23 changes: 21 additions & 2 deletions .github/workflows/hybrid-gate.yml
Original file line numberDiff line numberDiff line change
Expand Up@@ -264,7 +264,18 @@ jobs:
# fails safe -- empty != 'true', so the full build always runs -- but it would have disabled the
# docs-only exemption fleet-wide with nothing going red.
needs: [check-trailer, docs-only]
if: needs.check-trailer.outputs.found != 'true' && needs.docs-only.outputs.docs_only != 'true'
# WHY the event_name arm: a Gate-Passed trailer attests the tree it was
# computed on, and the tree a commit has once it is on the default branch
# is not that tree — landing rebases, squashes or merges it against
# whatever else arrived first. Before the merge the trailer is the best
# evidence available and skipping the build is the point; after it, the
# attested tree no longer exists and honouring the trailer would report
# the default branch as built without building it. A push therefore always
# takes the build path, whatever the pushed tip carries.
if: >-
needs.docs-only.outputs.docs_only != 'true'
&& (github.event_name == 'push'
|| needs.check-trailer.outputs.found != 'true')
runs-on: ubuntu-latest
timeout-minutes: ${{ inputs.full_gate_timeout_minutes }}
env:
Expand DownExpand Up@@ -519,6 +530,7 @@ jobs:
DOCS_ONLY: ${{ needs.docs-only.outputs.docs_only }}
BUILD_RESULT: ${{ needs.full-gate-build.result }}
ATTRIBUTION_RESULT: ${{ needs.ai-attribution.result }}
EVENT_NAME: ${{ github.event_name }}
run: |
# WHY there is no automation-author waiver here: bot PRs
# (dependabot, release-please) pass the same ladder as everyone —
Expand All@@ -535,7 +547,14 @@ jobs:
exit 1
fi

if [ "$TRAILER_FOUND" = "true" ]; then
# WHY the event guard: this arm exits before BUILD_RESULT is read, so
# without it a push whose tip happened to carry a trailer would pass
# the gate green while full-gate-build was skipped — or, once the job
# is forced to run on a push, while it was failing. The trailer
# attests a pre-merge tree; on the default branch only the build
# speaks for what is actually there. Mirrors the full-gate-build
# routing condition, which must stay in step with this one.
if [ "$TRAILER_FOUND" = "true" ] && [ "$EVENT_NAME" != "push" ]; then
echo "Gate-Passed trailer verified."
exit 0
fi
Expand Down
10 changes: 10 additions & 0 deletions .github/workflows/release-please.yml
Original file line numberDiff line numberDiff line change
Expand Up@@ -32,6 +32,16 @@ permissions:
jobs:
release-please:
runs-on: ubuntu-latest
# WHY an explicit bound: with none, this job inherits GitHub's 360-minute
# ceiling, and a caller's `uses:` line cannot supply one for it — GitHub
# Actions only accepts `timeout-minutes` on a job with its own `steps:`,
# never on the caller's side of a reusable-workflow call. This is the one
# job in this repo's own workflows that had no bound at all. 10 minutes
# matches gate-attestation.yml and actionlint.yml: the work here is one
# action call that reads conventional commits and opens or updates a PR
# through the GitHub API, the same order of cost as those, not the
# compile-and-test work full-gate-build is timed for.
timeout-minutes: 10
outputs:
release_created: ${{ steps.release.outputs.release_created }}
tag_name: ${{ steps.release.outputs.tag_name }}
Expand Down
228 changes: 228 additions & 0 deletions scripts/check_event_shape_guards.py
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,228 @@
#!/usr/bin/env python3
"""Guard: a pull_request-only value must not reach `git` unguarded.

WHY: `github.event.pull_request.*`, `github.head_ref` and `github.base_ref` are
populated for `pull_request` events only. On a `push` they interpolate to the
empty string. Passing an empty string where git expects a revision aborts the
command with `fatal: ambiguous argument ''`, which takes the step, the job and
the required check with it -- so a workflow that is fine on every PR fails on
every push to the default branch.

That failure has shipped from this repo twice. The `docs-only` and
`ai-attribution` steps of `hybrid-gate.yml` both died this way until #25 derived
their diff ranges from the event that actually fired; #25 left the neighbouring
`check-trailer` step passing `$PR_HEAD_SHA` straight to `git log`.

WARNING: this guard only sees values that reach a `git` command line in the same
step. An empty value consumed by `grep`, `case` or an echo is harmless and is
deliberately not reported -- reporting it would make this check fire on ~18 sites
fleet-wide, none of them defects, and a check that cannot pass teaches its
readers to skim past it.

A site is compliant when the expression carries a `||` fallback, or the step's
`run:` script guards the variable before using it -- either an explicit `[ -n ]`
/ `[ -z ]` test or a `${NAME:-fallback}` default expansion.

The value is tracked through intermediate assignments: `tip="$PR_HEAD_SHA"`
followed by `git log "$tip"` is reported, because one level of aliasing hides
the site without fixing it.
"""

from __future__ import annotations

import re
import sys
from pathlib import Path

# WHY: the contexts GitHub populates for pull_request events and leaves empty on
# a push. The `pull_request.` prefix covers head.sha, user.login, title, body.
PR_ONLY_CONTEXT = re.compile(
r"github\.event\.pull_request\.|github\.head_ref|github\.base_ref"
)

ENV_KEY = re.compile(r"^(?P<indent>\s+)env:\s*$")
MAPPING_ENTRY = re.compile(
r"^(?P<indent>\s+)(?P<name>[A-Za-z_][A-Za-z0-9_]*):\s*(?P<value>\S.*?)\s*$"
)
EXPRESSION = re.compile(r"\$\{\{(?P<expr>.+?)\}\}")
STEP_START = re.compile(r"^(?P<indent>\s*)-\s")
GIT_COMMAND = re.compile(r"(?:^|[|;&(`]|\$\()\s*git\s")


class Finding:
"""One pull_request-only value reaching `git` with no guard."""

def __init__(self, path: Path, line: int, name: str, git_line: str) -> None:
self.path = path
self.line = line
self.name = name
self.git_line = git_line

def __str__(self) -> str:
return (
f"{self.path}:{self.line}: `{self.name}` interpolates a "
"pull_request-only context with no `||` fallback, and reaches git "
f"with no emptiness test: `{self.git_line}`. On a push event this "
"aborts with `fatal: ambiguous argument ''`."
)


def env_vars(lines: list[str], start: int, end: int) -> dict[str, str]:
"""Map env var name to expression text for every `env:` entry in a span."""
found: dict[str, str] = {}
env_indent: int | None = None
for line in lines[start:end]:
key = ENV_KEY.match(line)
if key:
env_indent = len(key.group("indent"))
continue
if env_indent is None:
continue
entry = MAPPING_ENTRY.match(line)
if entry is None:
if line.strip() and not line.lstrip().startswith("#"):
env_indent = None
continue
if len(entry.group("indent")) <= env_indent:
env_indent = None
continue
expression = EXPRESSION.search(entry.group("value"))
if expression is not None:
found[entry.group("name")] = expression.group("expr")
return found


def step_spans(lines: list[str]) -> list[tuple[int, int]]:
"""Return the [start, end) span of every YAML sequence item."""
starts = [
(index, len(match.group("indent")))
for index, line in enumerate(lines)
if (match := STEP_START.match(line))
]
spans: list[tuple[int, int]] = []
for position, (index, indent) in enumerate(starts):
end = len(lines)
for next_index, next_indent in starts[position + 1 :]:
if next_indent <= indent:
end = next_index
break
spans.append((index, end))
return spans


def guards_emptiness(body: str, name: str) -> bool:
"""True when `body` tests `$name` for being set or empty before use."""
test_guard = re.compile(
r"""\[\[?\s+-[nz]\s+"?\$\{?""" + re.escape(name) + r"""\}?"?\s+\]\]?"""
)
# WHY: `${NAME:-fallback}` and `${NAME:=fallback}` substitute the fallback
# when NAME is empty, so they resolve the empty-string hazard as completely
# as an explicit `[ -n ]` test. Treating them as unguarded would fire on the
# fix in hybrid-gate.yml's own check-trailer step, and a guard that rejects
# a correct repair teaches its readers to work around it.
default_guard = re.compile(
r"\$\{" + re.escape(name) + r":[-=][^}]*\}"
)
return bool(test_guard.search(body) or default_guard.search(body))


def tainted_names(body: str, name: str) -> set[str]:
"""Return `name` plus every shell variable assigned from it unguarded.

WHY: the value routinely reaches git through an intermediate — `tip="$PR_HEAD_SHA"`
then `git log "$tip"`. Matching only the original name reports that site clean,
which is the exact defect class this guard exists to catch escaping through one
assignment. A guarded assignment is not followed: it is handled by
`guards_emptiness`, which marks the whole site compliant before this runs.
"""
tainted = {name}
# WHY: fixpoint rather than one hop, so an alias of an alias cannot hide the
# site either. The set is bounded by the assignments in a single step.
while True:
alias_pattern = re.compile(
r"^\s*(?P<alias>[A-Za-z_][A-Za-z0-9_]*)="
r"""["']?\$\{?(?P<src>[A-Za-z_][A-Za-z0-9_]*)\}?["']?\s*$"""
)
grown = False
for line in body.splitlines():
match = alias_pattern.match(line)
if match is None:
continue
if match.group("src") in tainted and match.group("alias") not in tainted:
tainted.add(match.group("alias"))
grown = True
if not grown:
return tainted


def check_file(path: Path) -> tuple[list[Finding], int]:
"""Return the findings in `path` and how many PR-only values it examined."""
lines = path.read_text(encoding="utf-8").splitlines()
findings: list[Finding] = []
examined = 0
for start, end in step_spans(lines):
body = "\n".join(lines[start:end])
for name, expression in env_vars(lines, start, end).items():
if not PR_ONLY_CONTEXT.search(expression):
continue
examined += 1
if "||" in expression or guards_emptiness(body, name):
continue
reference = re.compile(
r"\$\{?(?:"
+ "|".join(re.escape(alias) for alias in sorted(tainted_names(body, name)))
+ r")\}?\b"
)
for offset, line in enumerate(lines[start:end]):
if GIT_COMMAND.search(line) and reference.search(line):
findings.append(
Finding(path, start + offset + 1, name, line.strip())
)
break
return findings, examined


def main(argv: list[str]) -> int:
root = Path(argv[1]) if len(argv) > 1 else Path(".github/workflows")
paths = sorted(root.glob("*.yml"))
if not paths:
print(f"ERROR: no workflow files under {root}", file=sys.stderr)
return 1

findings: list[Finding] = []
examined = 0
for path in paths:
path_findings, path_examined = check_file(path)
findings.extend(path_findings)
examined += path_examined

# WHY: a guard that examined nothing reports the same green as a guard that
# examined everything. Refuse to pass vacuously.
if examined == 0:
print(
"ERROR: no pull_request-only interpolation was examined, so this "
"guard no longer measures anything. Either the workflows stopped "
"using those contexts, or the parser stopped matching them.",
file=sys.stderr,
)
return 1

for finding in findings:
print(f"ERROR: {finding}", file=sys.stderr)
if findings:
print(
f"\n{len(findings)} unguarded site(s). Add a `|| <fallback>` to the "
"expression (github.sha is the push-event equivalent of a PR tip), "
"or test the variable for emptiness before the git call.",
file=sys.stderr,
)
return 1

print(
f"OK: {examined} pull_request-only value(s) examined, none reaches git unguarded."
)
return 0


if __name__ == "__main__":
sys.exit(main(sys.argv))
Loading