diff --git a/.github/workflows/actionlint.yml b/.github/workflows/actionlint.yml index 35f484b..4b052f4 100644 --- a/.github/workflows/actionlint.yml +++ b/.github/workflows/actionlint.yml @@ -6,6 +6,7 @@ on: pull_request: paths: - ".github/workflows/**" + - "scripts/**" permissions: contents: read @@ -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 diff --git a/.github/workflows/gate-attestation.yml b/.github/workflows/gate-attestation.yml index d8538a1..231fcce 100644 --- a/.github/workflows/gate-attestation.yml +++ b/.github/workflows/gate-attestation.yml @@ -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 diff --git a/.github/workflows/hybrid-gate.yml b/.github/workflows/hybrid-gate.yml index 7a8be40..29fdf25 100644 --- a/.github/workflows/hybrid-gate.yml +++ b/.github/workflows/hybrid-gate.yml @@ -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: @@ -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 — @@ -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 diff --git a/.github/workflows/release-please.yml b/.github/workflows/release-please.yml index c34ef28..daa2dbd 100644 --- a/.github/workflows/release-please.yml +++ b/.github/workflows/release-please.yml @@ -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 }} diff --git a/scripts/check_event_shape_guards.py b/scripts/check_event_shape_guards.py new file mode 100644 index 0000000..9815093 --- /dev/null +++ b/scripts/check_event_shape_guards.py @@ -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\s+)env:\s*$") +MAPPING_ENTRY = re.compile( + r"^(?P\s+)(?P[A-Za-z_][A-Za-z0-9_]*):\s*(?P\S.*?)\s*$" +) +EXPRESSION = re.compile(r"\$\{\{(?P.+?)\}\}") +STEP_START = re.compile(r"^(?P\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[A-Za-z_][A-Za-z0-9_]*)=" + r"""["']?\$\{?(?P[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 `|| ` 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)) diff --git a/scripts/check_gate_evaluation.py b/scripts/check_gate_evaluation.py new file mode 100644 index 0000000..8965a61 --- /dev/null +++ b/scripts/check_gate_evaluation.py @@ -0,0 +1,94 @@ +#!/usr/bin/env python3 +"""Execute hybrid-gate's `gate` verdict script against a fixed event matrix. + +WHY this exists as a script rather than a review checklist: the verdict is a +shell ladder of early exits, and which arm fires depends on `github.event_name` +— a value actionlint type-checks but never evaluates. The failure it guards is +silent by construction: the gate reports success, so nothing downstream has a +red result to point at. Only running the ladder distinguishes the two shapes. + +WHY it runs the text out of the workflow instead of a copy: a copy drifts, and +a drifted copy passing is worse than no check at all. +""" + +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +import yaml + +# (trailer_found, docs_only, build_result, attribution_result, event_name), +# expected exit status, description. +# +# WHY the pull_request rows are here even though none of them exercise the +# event guard: they are what makes a regression legible. Tightening the push +# path is only correct if the pre-merge path is untouched, and these rows fail +# if a future edit tightens both. +CASES: list[tuple[tuple[str, str, str, str, str], int, str]] = [ + (("true", "false", "skipped", "success", "pull_request"), 0, + "pull_request, tip stamped: passes without a build"), + (("false", "false", "success", "success", "pull_request"), 0, + "pull_request, no trailer, build green: passes"), + (("false", "false", "failure", "success", "pull_request"), 1, + "pull_request, no trailer, build red: fails"), + (("false", "true", "skipped", "success", "pull_request"), 0, + "pull_request, docs-only: exempt"), + (("true", "false", "skipped", "failure", "pull_request"), 1, + "pull_request, attribution red: fails even when stamped"), + (("true", "false", "success", "success", "push"), 0, + "push, tip stamped, build green: passes"), + (("true", "false", "failure", "success", "push"), 1, + "push, tip stamped, build RED: must fail"), + (("true", "false", "skipped", "success", "push"), 1, + "push, tip stamped, build SKIPPED: must fail"), + (("false", "false", "failure", "success", "push"), 1, + "push, no trailer, build red: fails"), + (("false", "true", "skipped", "success", "push"), 0, + "push, docs-only: exempt"), +] + +KEYS = ("TRAILER_FOUND", "DOCS_ONLY", "BUILD_RESULT", "ATTRIBUTION_RESULT", + "EVENT_NAME") + + +def verdict_script(workflow: Path) -> str: + doc = yaml.safe_load(workflow.read_text()) + steps = doc["jobs"]["gate"]["steps"] + for step in steps: + if step.get("name") == "Evaluate gate result": + return step["run"] + raise SystemExit( + f"{workflow}: no 'Evaluate gate result' step in the gate job. " + "The step was renamed or removed; update this check to match." + ) + + +def main() -> int: + workflow = Path(sys.argv[1] if len(sys.argv) > 1 + else ".github/workflows/hybrid-gate.yml") + script = verdict_script(workflow) + + failures = 0 + for values, expected, description in CASES: + env = dict(zip(KEYS, values)) + env["PATH"] = "/usr/bin:/bin" + result = subprocess.run(["bash", "-c", script], env=env, + capture_output=True, text=True, check=False) + if result.returncode == expected: + print(f"ok {description}") + continue + failures += 1 + print(f"FAIL {description}") + print(f" expected exit {expected}, got {result.returncode}") + print(f" env: {env}") + for line in (result.stdout + result.stderr).splitlines(): + print(f" | {line}") + + print(f"\n{len(CASES) - failures}/{len(CASES)} cases behaved as specified.") + return 1 if failures else 0 + + +if __name__ == "__main__": + sys.exit(main())