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
59 changes: 59 additions & 0 deletions .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ name: Lint
#
# What it checks:
# - actionlint on every .github/workflows/*.yml (catches GHA-specific bugs)
# - draft-gate triggers: any workflow gated on `draft == false` (in its own jobs
# OR in a reusable it calls) must listen for `ready_for_review`, else its
# required check reports `skipped` — which GitHub counts as SATISFIED — and the
# PR merges green and UNREVIEWED. See selftest/check_draft_gate_triggers.py.
# - prettier --check on the markdown glob (default **/*.md)
# - shellcheck on standalone shell scripts (opt-in, default OFF)
#
Expand Down Expand Up @@ -70,6 +74,11 @@ on:
required: false
type: boolean
default: true
run_draft_gate_check:
description: "Fail if a workflow gates on draft == false but does not listen for ready_for_review. Default true. This one defaults ON, unlike the other opt-in checks, because the failure it catches is silent and fails OPEN — a skipped required check counts as satisfied, so the PR merges unreviewed."
required: false
type: boolean
default: true
run_shellcheck:
description: "Enable actionlint's shellcheck integration. Default false to avoid info-level style noise on first install."
required: false
Expand Down Expand Up @@ -144,6 +153,56 @@ jobs:
fi
shell: bash

draft-gate-triggers:
name: draft-gate triggers
# Defaults ON (unlike run_shellcheck etc.). Those default off to avoid style noise on
# first install; this one catches a silent FAIL-OPEN — a draft-gated workflow that
# never hears `ready_for_review`, so its required check reports `skipped`, which
# GitHub counts as SATISFIED, and the PR merges unreviewed. The fleet was swept clean
# on 2026-07-16 before this landed, so ON is not expected to redden any caller.
# Same format()/loose-coercion dodge as the actionlint job above.
if: ${{ format('{0}', inputs.run_draft_gate_check) != 'false' }}
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v6

# The checker lives in ci-workflows so consumers get fixes without a re-install --
# the exact gap that made this bug survive two central fixes (ci-workflows#115 and
# dotclaude#156 both landed 2026-07-15; a sweep on 2026-07-16 still found 21
# installed repos vulnerable, because neither was retroactive).
#
# Self-reference: in a reusable, `github.repository` is the CALLER's repo. When the
# caller IS ci-workflows (this workflow's own pull_request self-test), the checker is
# already in the checkout above, and fetching it from @main would be wrong twice
# over: it would miss the PR's changes, and on the PR that first adds the checker
# @main does not have the file at all -- the job died with "No such file or
# directory". So: use the local copy here, fetch @main everywhere else.
# ci-workflows is public, so a consumer's default GITHUB_TOKEN can check it out.
- name: Check out the checker
if: ${{ github.repository != 'topcoder1/ci-workflows' }}
uses: actions/checkout@v6
with:
repository: topcoder1/ci-workflows
ref: main
path: .ci-workflows-checker
persist-credentials: false

- name: Install PyYAML
run: python3 -m pip install --quiet pyyaml
shell: bash

- name: Check draft-gate triggers
env:
IS_SELF: ${{ github.repository == 'topcoder1/ci-workflows' }}
run: |
if [ "$IS_SELF" = "true" ]; then
checker=selftest/check_draft_gate_triggers.py
else
checker=.ci-workflows-checker/selftest/check_draft_gate_triggers.py
fi
python3 "$checker" .github/workflows
shell: bash

prettier:
name: prettier (markdown)
# Skip entirely on push events — prettier-autofix handles main-branch
Expand Down
5 changes: 5 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ dev = [
"pexpect>=4.9.0",
"pytest",
"pytest-cov",
# check_draft_gate_triggers.py parses workflow YAML rather than grepping it:
# the transitive draft gate lives in a called reusable, so the caller has no
# `draft` text to grep for. Without this declared, `uv run pytest` dies at
# COLLECTION with ModuleNotFoundError and takes the whole selftest suite with it.
"pyyaml",
]

[tool.pytest.ini_options]
Expand Down
198 changes: 198 additions & 0 deletions selftest/check_draft_gate_triggers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,198 @@
#!/usr/bin/env python3
"""Enforce: a workflow gated on `draft == false` MUST listen for `ready_for_review`.

WHY THIS EXISTS
---------------
A workflow whose job gates on `if: github.event.pull_request.draft == false`, but whose
`on.pull_request.types` omits `ready_for_review`, FAILS OPEN:

1. PR opened as a draft -> `opened` fires -> job skips on the gate (correct)
2. `gh pr ready` -> `ready_for_review` fires -> caller is not listening
3. nothing runs; the check stays `skipped`

GitHub counts a **skipped REQUIRED context as SATISFIED**, so the PR merges green and
UNREVIEWED. Fleet policy makes DRAFT the standard auto-merge opt-out, so every
manual-merge PR is draft->ready -- the recommended workflow was the one guaranteed to skip
the review. (It can also fail CLOSED, leaving a PR stuck on "Expected -- Waiting for status
to be reported", when no draft-phase run fired at all: domain-rank#33.)

WHY A CHECK AND NOT A COMMENT
-----------------------------
This exact mistake has been made twice fleet-wide: `reopened` was dropped first, then
`ready_for_review` was dropped *directly below the comment warning about `reopened`*. A
comment did not prevent the recurrence. And fixing it centrally does not fix consumers:
ci-workflows#115 and dotclaude#156 both landed 2026-07-15, yet a sweep on 2026-07-16 still
found 21 installed repos vulnerable, because neither central fix is retroactive. This check
runs inside the `lint.yml` reusable, against the CALLER's workflows, so installed-repo drift
is caught on every PR rather than by a periodic manual audit.

WHY IT PARSES INSTEAD OF GREPPING
---------------------------------
The transitive case is the one that bites: a caller like `pr-review.yml` contains NO `draft`
expression anywhere -- the gate lives in the reusable it calls. Grepping the caller cannot
see it. An explicit `types:` list is a DENYLIST BY OMISSION, and GitHub's default types
(opened/synchronize/reopened) exclude `ready_for_review`, so an ABSENT list is unsafe too.

Usage:
check_draft_gate_triggers.py <workflows-dir> [--extra-reusable name.yml ...]

Exits 0 when clean, 1 when any violation is found (annotated for GitHub Actions).
"""

from __future__ import annotations

import argparse
import sys
from pathlib import Path

import yaml

# GitHub's default `pull_request` activity types. `ready_for_review` is NOT among them --
# which is why an absent `types:` list is not a safe default for a draft-gated workflow.
GITHUB_DEFAULT_PR_TYPES: tuple[str, ...] = ("opened", "synchronize", "reopened")

# Reusables in topcoder1/ci-workflows whose jobs gate on `draft == false`. A caller inherits
# that gate transitively. Kept in sync with the repo by test_registry_matches_repo in
# selftest/test_check_draft_gate_triggers.py, which fails if a new draft-gating reusable is
# added here without being registered -- so the registry cannot silently rot.
DRAFT_GATED_REUSABLES: frozenset[str] = frozenset(
{
"claude-review.yml",
"claude-author-automerge.yml",
"safe-paths-automerge.yml",
"codex-review.yml",
"claude-adversarial-review.yml",
}
)

DRAFT_GATE_EXPR = "draft == false"


def load_workflow(path: Path) -> dict | None:
try:
doc = yaml.safe_load(path.read_text())
except yaml.YAMLError:
return None # actionlint owns YAML validity; don't double-report
return doc if isinstance(doc, dict) else None


def triggers(workflow: dict) -> dict:
"""Return the `on:` block.

PyYAML resolves the bare key `on:` to the boolean True (YAML 1.1 truthiness), so
`workflow["on"]` is a KeyError on every real GitHub workflow. Accept every spelling
rather than silently reading nothing and passing vacuously.
"""
for key in (True, "on", "On", "ON"):
if key in workflow:
raw = workflow[key]
break
else:
return {}
if isinstance(raw, str):
return {raw: {}}
if isinstance(raw, list):
return {str(k): {} for k in raw}
return raw if isinstance(raw, dict) else {}


def pr_types(workflow: dict) -> tuple[str, ...]:
pull_request = triggers(workflow).get("pull_request")
if not isinstance(pull_request, dict):
return GITHUB_DEFAULT_PR_TYPES
types = pull_request.get("types")
if isinstance(types, list) and types:
return tuple(str(t) for t in types)
return GITHUB_DEFAULT_PR_TYPES


def draft_gate_reason(workflow: dict, reusables: frozenset[str]) -> str | None:
"""Why this workflow is draft-gated, or None. Checks own jobs AND called reusables."""
jobs = workflow.get("jobs")
if not isinstance(jobs, dict):
return None
for job_id, job in jobs.items():
if not isinstance(job, dict):
continue
if DRAFT_GATE_EXPR in str(job.get("if", "")):
return f"job `{job_id}` gates on `{DRAFT_GATE_EXPR}`"
uses = job.get("uses")
if isinstance(uses, str):
base = uses.split("@")[0].rsplit("/", 1)[-1]
if base in reusables:
return f"job `{job_id}` calls `{base}`, which gates on `{DRAFT_GATE_EXPR}`"
return None


def check_dir(workflows_dir: Path, reusables: frozenset[str]) -> list[str]:
violations: list[str] = []
files = sorted(workflows_dir.glob("*.yml")) + sorted(workflows_dir.glob("*.yaml"))
for path in files:
workflow = load_workflow(path)
if workflow is None:
continue
if "pull_request" not in triggers(workflow):
continue
reason = draft_gate_reason(workflow, reusables)
if reason is None:
continue
types = pr_types(workflow)
if "ready_for_review" in types:
continue
pull_request = triggers(workflow).get("pull_request")
has_explicit_types = (
isinstance(pull_request, dict)
and isinstance(pull_request.get("types"), list)
and bool(pull_request["types"])
)
listed = (
f"types: {list(types)}"
if has_explicit_types
else f"no explicit `types:` list, so GitHub's defaults apply: {list(types)}"
)
violations.append(
f"{path.name}: {reason}, but does not listen for `ready_for_review` "
f"({listed}). A draft PR skips that job at `opened`, and `gh pr ready` would "
f"fire nothing this workflow hears -- so its check reports `skipped`, which "
f"GitHub counts as a SATISFIED required context, and the PR merges green and "
f"unreviewed. Fix: add `ready_for_review` to the types list. "
f"See ci-workflows#115, domain-rank#35."
)
return violations


def main(argv: list[str] | None = None) -> int:
parser = argparse.ArgumentParser(description=__doc__)
parser.add_argument("workflows_dir", type=Path)
parser.add_argument(
"--extra-reusable",
action="append",
default=[],
help="Additional draft-gating reusable filename to treat as transitive.",
)
args = parser.parse_args(argv)

if not args.workflows_dir.is_dir():
# Not an error: plenty of repos have no workflows dir.
print(f"no workflows directory at {args.workflows_dir}; nothing to check")
return 0

reusables = DRAFT_GATED_REUSABLES | set(args.extra_reusable)
violations = check_dir(args.workflows_dir, reusables)

for v in violations:
print(f"::error file=.github/workflows/{v.split(':')[0]}::{v}")
if violations:
print(
f"\n{len(violations)} workflow(s) gate on `{DRAFT_GATE_EXPR}` without listening "
f"for `ready_for_review`. This FAILS OPEN: the required check reports `skipped`, "
f"which GitHub counts as satisfied, so the PR merges unreviewed."
)
return 1

print("OK: every draft-gated workflow listens for `ready_for_review`")
return 0


if __name__ == "__main__": # pragma: no cover
sys.exit(main())
Loading
Loading