diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index b1ce9dbf..f3b1538c 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -23,7 +23,7 @@ Fill in every section that applies, and delete the entry-type sections that don' - [ ] **Repository layout is correct** — skills in `/skills`, MCP servers in `/mcp`, custom agents in `/custom-agents`; skills and MCP servers are never mixed in one directory. -- [ ] **A domain SME has reviewed and signed off.** A named subject-matter expert for this domain — someone other than the author — has reviewed the tool for technical correctness and approved it in this PR. Required, not optional. +- [ ] **A domain SME has reviewed and signed off.** A named subject-matter expert for this domain — someone other than the author — has reviewed the tool for technical correctness and approved it in this PR. Required for any change to what the tool tells the agent; an edit that changes no technical claim can be marked n/a with the reason, as CONTRIBUTING.md describes. - [ ] **Evaluation evidence is included** (see Testing). - [ ] **No internal-only or proprietary content** — no internal hostnames, account IDs, customer names, ticket numbers, internal URLs, or secrets in the tool, its examples, or its tests. - [ ] **README is complete** — prerequisites, setup/deployment, and at least one sample prompt or usage example. diff --git a/.github/scripts/check_pr_checklist.py b/.github/scripts/check_pr_checklist.py new file mode 100644 index 00000000..1db9fc0a --- /dev/null +++ b/.github/scripts/check_pr_checklist.py @@ -0,0 +1,210 @@ +#!/usr/bin/env python3 +"""Check that a pull request's description has its required checklist items done. + +Run by ``.github/workflows/validate-pr-checklist.yml``. Reads the ``pull_request`` +event payload and the files the pull request changes, lists the unmet items in the +job's step summary, and writes the verdict for ``label-pr-checklist-result.yml``:: + + /outcome pass | fail | error + /reason complete | incomplete | draft | bot | predating + +The unmet items are never written to ````. The labeling workflow runs with +write access and posts a comment, so it is handed only values it checks against a +fixed list; text from a PR description never reaches it. + +Which items apply comes from the files the PR changes, not from the "Type of +change" boxes, one of which names three tool types at once. A PR that touches no +tool directory is held only to the items that apply to every contribution. Inside +a tool PR an item that genuinely does not apply can be marked ``n/a:`` with a +reason on its own line, and the reviewer judges the reason; the universal items +cannot be waived that way. + +Exit status: 0 pass, 1 fail, 2 the check could not run. An error says nothing +about the PR, and the labeler leaves the PR's label and comment alone on it. +""" + +from __future__ import annotations + +import argparse +import json +import os +import re +import subprocess +import sys +from pathlib import Path + +# When the gated PR template reached main (commit 49f31c1, merged with PR #135), +# not when this check merged: a PR opened after it had the checklist in front of +# its author. A PR opened earlier passes with warnings unless ENFORCE_LABEL is on it. +CHECK_INTRODUCED = "2026-10-13T07:33:08Z" +ENFORCE_LABEL = "enforce-checklist" + +# Matched against the description's "## " headings by prefix, case-insensitively, +# so rewording the rest of a heading keeps its section. The tests pin each one +# against .github/PULL_REQUEST_TEMPLATE.md. +ALWAYS_SECTIONS = ("Required before review", "License confirmation") +TOOL_SECTION = "Security and safety" +TOOL_DIRS = {"skills/": "Skills", "mcp/": "MCP servers", "custom-agents/": "Custom agents"} +# connectors/ is not a tool directory here, deliberately: connector submissions are +# not open yet, and the template has no connector section to enforce. + +# Items every PR is held to, tool or not, and that can never be marked n/a: the +# repository is public, and the licence is a statement only the author can make. +UNIVERSAL_ITEMS = ( + re.compile(r"internal[-\s]only or proprietary", re.I), + re.compile(r"Apache License 2\.0", re.I), +) + +HEADING = re.compile(r"^##\s+(.*?)\s*$") +UNCHECKED = re.compile(r"^\s*-\s*\[\s*\]\s*") +# `n/a:` and a reason. A bare "n/a" waives nothing: the reason is what gets judged. +NA_WITH_REASON = re.compile(r"\bn/a\s*:\s*\S", re.I) + + +def sections(body: str) -> dict[str, list[str]]: + """The description's lines, grouped under their "## " heading.""" + found: dict[str, list[str]] = {} + current = None + for line in body.splitlines(): + heading = HEADING.match(line) + if heading: + current = heading.group(1) + found[current] = [] + elif current is not None: + found[current].append(line) + return found + + +def find_section(found: dict[str, list[str]], prefix: str) -> list[str] | None: + for heading, lines in found.items(): + if heading.lower().startswith(prefix.lower()): + return lines + return None + + +def required_sections(paths: list[str]) -> tuple[list[str], bool]: + """The sections the changed files call for, and whether this is a tool PR.""" + tool_dirs = [d for d in TOOL_DIRS if any(p.startswith(d) for p in paths)] + names = list(ALWAYS_SECTIONS) + if tool_dirs: + names.append(TOOL_SECTION) + names += [TOOL_DIRS[d] for d in tool_dirs] + return names, bool(tool_dirs) + + +def evaluate(body: str, paths: list[str]) -> tuple[list[str], list[str], bool]: + """(unmet, waived, is_tool_pr). Each entry reads "
→ ".""" + names, is_tool = required_sections(paths) + found = sections(body) + unmet: list[str] = [] + waived: list[str] = [] + for name in names: + lines = find_section(found, name) + if lines is None: + # Deleting a required section must not be a way past it. + unmet.append(f"{name} → section missing from the description; restore it from the template") + continue + for line in lines: + if not UNCHECKED.match(line): + continue + item = UNCHECKED.sub("", line).replace("**", "").strip() + if not item: + continue + universal = any(p.search(item) for p in UNIVERSAL_ITEMS) + if not is_tool and not universal: + continue + if NA_WITH_REASON.search(item): + if not universal: + waived.append(f"{name} → {item}") + continue + item += " (this item cannot be marked n/a)" + unmet.append(f"{name} → {item}") + return unmet, waived, is_tool + + +def decide(pr: dict, unmet: list[str]) -> tuple[str, str]: + """(outcome, reason). The reason lets the labeler say why a PR passed, so it + never tells the author of a draft or a grandfathered PR they are done.""" + if not unmet: + return "pass", "complete" + # A bot cannot fill in a template, and a draft is not submitted for review. The + # label does not override either: labeling a draft would nag an author who has + # said they are not finished. + if (pr.get("user") or {}).get("type") == "Bot": + return "pass", "bot" + if pr.get("draft"): + return "pass", "draft" + created = pr.get("created_at") or "" + labels = {label.get("name") for label in pr.get("labels") or []} + if created and created < CHECK_INTRODUCED and ENFORCE_LABEL not in labels: + return "pass", "predating" + return "fail", "incomplete" + + +def summary(unmet: list[str], waived: list[str], is_tool: bool, reason: str) -> str: + lines = ["## PR checklist", ""] + lines.append( + "This PR touches a tool directory, so every required item applies." + if is_tool + else "This PR touches no tool directory, so only the items that apply to every contribution are enforced." + ) + if waived: + lines += ["", "Marked n/a, with a reason:", ""] + [f"- {w}" for w in waived] + heading = { + "complete": "All applicable required items are met.", + "incomplete": "Unmet required items:", + "bot": "Opened by a bot, so the checklist is not enforced. Reported only:", + "draft": "A draft, so the checklist is not enforced until it is marked ready for review. Reported only:", + "predating": f"Opened before this check existed, so these are warnings. Apply `{ENFORCE_LABEL}` to require them:", + }[reason] + lines += ["", heading] + if unmet: + lines += [""] + [f"- {u}" for u in unmet] + return "\n".join(lines) + "\n" + + +def changed_paths(base_ref: str, head_ref: str) -> list[str]: + out = subprocess.run( + ["git", "diff", "--name-only", f"{base_ref}...{head_ref}"], + check=True, + capture_output=True, + text=True, + ).stdout + return [p for p in out.splitlines() if p] + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("--event", required=True, help="the pull_request event payload ($GITHUB_EVENT_PATH)") + parser.add_argument("--base-ref", required=True, help="the base branch, e.g. origin/main") + parser.add_argument("--head-ref", default="HEAD") + parser.add_argument("--out", type=Path, required=True, help="directory for outcome and reason") + args = parser.parse_args(argv) + args.out.mkdir(parents=True, exist_ok=True) + + try: + pr = json.loads(Path(args.event).read_text(encoding="utf-8"))["pull_request"] + unmet, waived, is_tool = evaluate(pr.get("body") or "", changed_paths(args.base_ref, args.head_ref)) + outcome, reason = decide(pr, unmet) + report = summary(unmet, waived, is_tool, reason) + step_summary = os.environ.get("GITHUB_STEP_SUMMARY") + if step_summary: + with open(step_summary, "a", encoding="utf-8") as f: + f.write(report) + except Exception as e: # noqa: BLE001 - failing to reach a verdict is an error, never a finding + (args.out / "outcome").write_text("error\n", encoding="utf-8") + print(f"::error::The PR checklist check could not run ({type(e).__name__}: {e}). " + "Nothing was concluded about this pull request.") + return 2 + + (args.out / "outcome").write_text(outcome + "\n", encoding="utf-8") + (args.out / "reason").write_text(reason + "\n", encoding="utf-8") + print(report) + if reason == "predating": + for item in unmet: + print(f"::warning::{item}") + return 0 if outcome == "pass" else 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/workflows/label-pr-checklist-result.yml b/.github/workflows/label-pr-checklist-result.yml new file mode 100644 index 00000000..a2824e03 --- /dev/null +++ b/.github/workflows/label-pr-checklist-result.yml @@ -0,0 +1,151 @@ +name: Label PR checklist result + +# Applies the `needs-checklist` label and an explanatory comment to a PR whose +# "Validate PR checklist" check failed, and removes the label and updates the +# comment once the check passes. +# +# `needs-checklist` is an *output* of the check and is owned by this workflow +# alone. Reviewers filter it out: a PR carrying it is on hold and not reviewed. +# +# Why a separate workflow: for pull_request events from a fork, GITHUB_TOKEN is +# read-only regardless of the `permissions:` block, so the validating job cannot +# label or comment on its own PR. A workflow_run job runs in the base repository's +# context with write access, and uses the workflow definition from the default +# branch rather than from the PR. +# +# Nothing from the artifact is trusted beyond a fixed list. The validating job runs +# a workflow definition the PR itself controls, so its artifact can say anything. +# Hence: +# - The PR is resolved from this event's payload, which GitHub writes: an open PR +# whose head is the run's head repository, branch and commit. The artifact's +# PR number only has to name one of those matches, so it cannot point the +# label or comment at another PR. A run whose commit is no longer the PR's head +# matches nothing and does nothing, which also keeps a stale run from +# overwriting a newer one. +# - The comment is fixed text chosen by `outcome` and `reason`, both checked +# against the lists below. It links to the run for the unmet items rather than +# repeating them, so no text from a PR description is ever posted by the bot. +# - `error` means the check did not reach a verdict. The label and comment are +# left as they are: neither "incomplete" nor "complete" is a claim anyone is in +# a position to make. The validating job has already failed, saying why. + +on: + workflow_run: + workflows: ["Validate PR checklist"] + types: [completed] + +permissions: + contents: read + actions: read # download the artifact from the triggering run + pull-requests: write # add / remove the label and comment + +# Serialized per pull request, not per run. Keying on `workflow_run.id` would make +# every group unique and the setting a no-op, so two validate runs finishing close +# together could both reach the commenting step and race. The head repository and +# branch identify the PR. `cancel-in-progress: false` so a labeler mid-write is +# never killed; runs queue. +concurrency: + group: label-pr-checklist-${{ github.event.workflow_run.head_repository.full_name }}-${{ github.event.workflow_run.head_branch }} + cancel-in-progress: false + +jobs: + label: + runs-on: ubuntu-latest + if: >- + github.event.workflow_run.conclusion == 'success' || + github.event.workflow_run.conclusion == 'failure' + steps: + - name: Download the check outcome + uses: actions/download-artifact@v4 + continue-on-error: true + with: + name: checklist-check-result + path: result + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ github.token }} + + - name: Apply or remove the label and comment + uses: actions/github-script@v7 + with: + script: | + const fs = require("fs"); + const LABEL = "needs-checklist"; + const MARKER = ""; + const REASONS = { pass: ["complete", "draft", "bot", "predating"], fail: ["incomplete"] }; + const run = context.payload.workflow_run; + const {owner, repo} = context.repo; + const read = (name) => { + try { return fs.readFileSync(`result/${name}`, "utf8").trim(); } catch { return ""; } + }; + + const outcome = read("outcome"); + const reason = read("reason"); + if (!Object.hasOwn(REASONS, outcome)) { + core.notice(`Outcome '${outcome || "none"}': the check reached no verdict, so the label and comment are left as they are.`); + return; + } + if (!REASONS[outcome].includes(reason)) { + core.warning(`Unexpected reason '${reason}' for outcome '${outcome}'; the PR is left as it is.`); + return; + } + + const head = run.head_repository; + const claimed = Number(read("pr-number")); + if (!head) { + core.notice("The run's head repository no longer exists; nothing to label."); + return; + } + const candidates = await github.paginate(github.rest.pulls.list, { + owner, repo, state: "open", head: `${head.owner.login}:${run.head_branch}`, per_page: 100, + }); + const pr = candidates.find((p) => p.number === claimed && p.head.sha === run.head_sha); + if (!pr) { + core.warning(`No open PR #${claimed} has ${head.full_name}:${run.head_branch} at ${run.head_sha} as its head; nothing labeled.`); + return; + } + + // Ensure the label exists. + try { await github.rest.issues.getLabel({owner, repo, name: LABEL}); } + catch { await github.rest.issues.createLabel({owner, repo, name: LABEL, color: "D93F0B", description: "PR checklist incomplete — not reviewed until resolved"}); } + + const hasLabel = (pr.labels || []).some((l) => l.name === LABEL); + const comments = await github.paginate(github.rest.issues.listComments, {owner, repo, issue_number: pr.number}); + const prev = comments.find((c) => (c.body || "").includes(MARKER)); + + if (outcome === "fail") { + if (!hasLabel) await github.rest.issues.addLabels({owner, repo, issue_number: pr.number, labels: [LABEL]}); + const comment = [ + MARKER, + "### This PR is on hold — checklist incomplete", + "", + "Required items in the PR description are unchecked. A maintainer will not review this PR until each one is checked **and actually done**.", + "", + `The [check's summary](${run.html_url}) lists them. Complete the work, tick the boxes, and this clears automatically. CONTRIBUTING.md explains each item.`, + ].join("\n"); + if (prev) await github.rest.issues.updateComment({owner, repo, comment_id: prev.id, body: comment}); + else await github.rest.issues.createComment({owner, repo, issue_number: pr.number, body: comment}); + core.notice(`Labeled PR #${pr.number} with ${LABEL}.`); + return; + } + + // Passing is not the same as complete. Say "complete" only when nothing + // was unmet, and otherwise say why it is not enforced, so an author is + // never told they are done when they are not. A PR that never had the + // comment gets none: there is nothing to correct. + if (hasLabel) { + try { + await github.rest.issues.removeLabel({owner, repo, issue_number: pr.number, name: LABEL}); + } catch (e) { + // 404 is normal: the label was already removed by hand or by a + // concurrent run. Anything else is a real failure worth seeing. + if (e.status !== 404) core.warning(`Could not remove ${LABEL} from #${pr.number}: ${e.message}`); + } + } + const bodies = { + complete: "### Checklist complete ✅\nAll required items are checked. This PR is ready for maintainer review.", + draft: "### Checklist not enforced while this PR is a draft\nSome required items are still unmet. Mark the PR ready for review and this check runs again.", + bot: "### Checklist not enforced for bot-authored PRs\nThis PR was opened by a bot, so the contribution checklist does not apply.", + predating: "### Checklist not enforced on this PR\nIt was opened before this check existed, so the unmet items are reported as warnings in the check's summary rather than blocking. A maintainer can apply `enforce-checklist` to require them.", + }; + if (prev) await github.rest.issues.updateComment({owner, repo, comment_id: prev.id, body: `${MARKER}\n${bodies[reason]}`}); + core.notice(`PR #${pr.number} passed the checklist check (${reason}).`); diff --git a/.github/workflows/validate-pr-checklist.yml b/.github/workflows/validate-pr-checklist.yml new file mode 100644 index 00000000..b2242fa2 --- /dev/null +++ b/.github/workflows/validate-pr-checklist.yml @@ -0,0 +1,95 @@ +name: Validate PR checklist +# Gates pull requests on the required checkboxes in the PR description being +# checked. The rules — which items apply, the `n/a:` escape, and which PRs are +# reported but not enforced — live in .github/scripts/check_pr_checklist.py, with +# tests in tests/test_check_pr_checklist.py. +# +# The job lists unmet items in its step summary and publishes only the verdict +# (pass, fail or error, and why) as an artifact. A companion workflow, +# label-pr-checklist-result.yml, then applies or removes the `needs-checklist` +# label and posts the comment. It has to be separate because GITHUB_TOKEN is +# read-only for pull_request events from forks, which is where most contributions +# here come from. This mirrors the validate-skill-evals.yml pair. +# +# NOTE: a red check does not block merge on its own. An admin must add this job's +# name ("validate-pr-checklist") to the required status checks for `main` in +# branch protection. Until then this workflow is advisory. +on: + pull_request: + # `edited`: ticking a box edits the body and must re-run the check. + # `labeled` / `unlabeled`: applying or removing `enforce-checklist` takes effect + # at once, and labeling forces a run on a PR that has none. A label applied by + # GITHUB_TOKEN raises no event, so the companion cannot re-trigger this one. + types: [opened, edited, synchronize, reopened, ready_for_review, labeled, unlabeled] + # Not filtered with paths: a required status check that never runs is reported + # as pending forever, which would permanently block PRs. The job always runs. +permissions: + contents: read +concurrency: + group: validate-pr-checklist-${{ github.event.pull_request.number }} + cancel-in-progress: true +jobs: + # The job id and its display name are kept identical, because the display name + # is the status check context that branch protection matches on. + validate-pr-checklist: + name: validate-pr-checklist + runs-on: ubuntu-latest + # No `if:` guard on the job, on purpose. Skipping it would report the required + # check as "skipped", which branch protection can treat as satisfied — so a + # guard keyed on a label would let anyone unblock a failing PR by labeling it. + # Labels are read by the script instead, where they can only make the check + # stricter and the job still reports a real status. + steps: + - uses: actions/checkout@v4 + with: + # Full history, so the script can diff against the merge base. + fetch-depth: 0 + - uses: actions/setup-python@v5 + with: + python-version: "3.12" + - name: Check the required checklist items + # Never fails the step: the verdict is published for the labeling workflow + # first and only then turned into a job failure. Without this the artifact + # would not upload on failure — the case that needs a label. + continue-on-error: true + # The PR body, labels and author are read from the event payload file by + # the script, never interpolated into this command. + env: + BASE_REF: ${{ github.event.pull_request.base.ref }} + run: | + git fetch --no-tags --quiet origin \ + "+refs/heads/${BASE_REF}:refs/remotes/origin/${BASE_REF}" + python3 .github/scripts/check_pr_checklist.py \ + --event "$GITHUB_EVENT_PATH" \ + --base-ref "origin/${BASE_REF}" \ + --out checklist-result + - name: Publish the verdict for the labeling workflow + id: verdict + env: + PR_NUMBER: ${{ github.event.pull_request.number }} + run: | + # Anything but a verdict the script wrote itself is an error: a failed + # fetch, a crash, or a script that never ran. An error is reported as the + # check not completing, never as the PR's checklist being incomplete. + mkdir -p checklist-result + outcome=$(cat checklist-result/outcome 2>/dev/null || true) + case "$outcome" in pass|fail|error) ;; *) outcome=error ;; esac + printf '%s\n' "$outcome" > checklist-result/outcome + printf '%s\n' "$PR_NUMBER" > checklist-result/pr-number + echo "outcome=${outcome}" >> "$GITHUB_OUTPUT" + - uses: actions/upload-artifact@v4 + with: + name: checklist-check-result + path: checklist-result/ + retention-days: 1 + - name: Fail the job unless the checklist passed + if: steps.verdict.outputs.outcome != 'pass' + env: + OUTCOME: ${{ steps.verdict.outputs.outcome }} + run: | + if [ "$OUTCOME" = "fail" ]; then + echo "::error::PR checklist incomplete — the check summary lists the unmet items." + else + echo "::error::The PR checklist check did not complete. Nothing was concluded about this pull request — see the log above." + fi + exit 1 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index df95c361..7f7eedb5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -262,6 +262,68 @@ If an issue lands with you, verify the skill against the checklist in it. When i Test relevant scenarios with and without the custom agent, multiple times. Focus on quality and consistency of the output, compared to asking DevOps Agent the same question using chat. When checking consistency, the output doesn't have to be the same verbatim - focus on the substance +## Domain SME Sign-Off + +**Every tool contribution needs a named subject-matter expert, other than the +author, to review it for technical correctness and say so in the pull request.** +It is the one required item a maintainer cannot do for you. + +Maintainers review what is reviewable from the repository: layout, the publishing +rules, the eval evidence, whether the content is safe to publish, and whether the +tool fits the catalog. What they cannot do, across every AWS service this +repository covers, is tell you that an OpenSearch shard-count threshold is right, +that an EKS upgrade order is safe, or that an RDS failover procedure matches how +the service actually behaves. A skill that is well-formed and confidently wrong is +worse than no skill, because the agent will act on it during an incident. The SME +is the only check on that, which is why a PR waits for one. + +### Who qualifies + +Someone with hands-on operational experience of the service or domain the tool +covers, who did not write it. They do not need to be a maintainer of this +repository, and they do not need to be an AWS employee. A colleague who runs the +service in production, the service team itself, or a specialist from your own +organisation all count. + +What does not count: the author, a reviewer who checked only spelling and +structure, or an unnamed group. "Reviewed internally" is not a sign-off, because a +future maintainer reading the PR cannot tell what was checked or by whom. + +### How to record it + +The SME needs to leave something in the pull request that names what they +verified. Either is fine: + +- A **GitHub review** on the PR, approving it, with a line on what they checked. +- A **comment** on the PR saying they reviewed it and what they looked at — + useful when the SME has no write access to the repository. + +Then tick the box. The box is what the checklist check reads; a note beside an +unticked box does not clear it, so a PR whose description says "SME review +pending" stays on hold whatever the prose says. + +**An SME sign-off is not the same as the approving review the branch requires.** +`main` requires one approving review from a maintainer before a PR can merge. +That is a separate gate, about whether the change should land in this repository +at all. An SME who is not a maintainer satisfies the checklist item without +satisfying branch protection, and a maintainer approving on structure does not +satisfy the checklist item. A PR usually needs both, from different people. + +### If you cannot find one + +Say so, and ask — do not tick the box and do not leave it pending indefinitely. +Raise it in your [tool request issue](https://github.com/aws/tools-for-devops-agent/issues/new?template=community-request.yml), +or comment on the PR asking a maintainer to help route it. Maintainers can often +reach the relevant service team or an internal specialist. A PR held up on a +sign-off nobody has been asked for is the most common reason a good contribution +stalls. + +The one exception is a change that alters no technical claim — a typo, a link, a +label renamed across many skills. Mark the item `n/a:` with that reason, as +described under "The Pull Request Checklist", and the reviewer judges whether it +holds. A new tool, or any change to what a tool tells the agent to do, still +needs the sign-off. + ## Redacting AWS Identifiers **Redact your own AWS identifiers before you commit.** This repository is public, and a commit is permanent: once an account ID, an ARN or an instance ID is on `main` it is in the history for good, and deleting it in a later commit does not remove it. Redacting is yours to do. Two pieces of tooling help — the skill evaluation tool's redaction pass, and the pull request check described in the next section — and neither covers everything. @@ -377,6 +439,108 @@ A failing check puts a `needs-id-redact` label on the pull request, which is rem NOTE: a red check does not block merging on its own. An admin must add the job's name (`scan-aws-identifiers`) to the required status checks for `main` in branch protection. Until then this check is advisory. +## The Pull Request Checklist + +The pull request template carries a set of required items, and a check +([`.github/workflows/validate-pr-checklist.yml`](.github/workflows/validate-pr-checklist.yml)) +reads your PR description and fails when a required box is unchecked. A failing +check puts a `needs-checklist` label on the PR and a comment pointing to what is +outstanding. A PR carrying that label is on hold and is not reviewed until it +clears. + +Nothing about this check judges your tool. It checks that the work the template +asks for has been done and said to have been done. + +### Which items apply to your PR + +Scope comes from the **files the PR changes**, not from the "Type of change" +boxes — those are for a human reader, and "Update to an existing skill, agent, or +MCP server" names three types at once, so they cannot decide it. + +| Your PR changes | Items enforced | +| --- | --- | +| Nothing under `skills/`, `mcp/` or `custom-agents/` | Only the two items that apply to every contribution, in Required before review and License confirmation: no internal-only content, and the licence confirmation | +| Anything under `skills/` | Every item in Required before review, Security and safety, License confirmation, and the Skills section | +| Anything under `mcp/` | The same, with the MCP servers section instead of Skills | +| Anything under `custom-agents/` | The same, with the Custom agents section | + +So a PR that only edits `CONTRIBUTING.md` or a workflow is not asked for a README, +a CHANGELOG or an SME sign-off, because none of it means anything for that change. +A PR touching more than one tool directory is held to each of their sections. + +Eval evidence is **not** re-checked here. `validate-skill-evals` already gates +that, and the two checks are independent — the checklist item is about whether you +included the evidence, and the eval check is about whether the evidence is there +and well-formed. + +### When an item genuinely does not apply + +Inside a tool contribution an individual item can still be moot — an MCP server +with no IAM to scope, say. Write `n/a:` and a reason on the item instead of +ticking it: + +```markdown +- [ ] **Least-privilege IAM** — n/a: this server calls no AWS APIs, only the vendor's HTTPS endpoint with a customer-supplied token. +``` + +The item is then treated as met, and the reason stays in the description where a +reviewer can judge it. + +Two things about the form. A bare `n/a` with no reason does not count — the reason +is the part being judged. And **it has to be on the same line as the checkbox**: +the check reads line by line, so a reason wrapped onto the next line leaves the +box looking plainly unchecked. Keep the item on one line however long it gets. + +Two items can never be waived this way: + +- **no internal-only or proprietary content**, because this repository is public + and the claim is about every file in the PR +- the **licence confirmation**, which is a statement only you can make + +Use the escape rather than either of the alternatives. Ticking something untrue is +caught at review and sends the PR back. Deleting the line is worse: the check +treats a missing required section as unmet and names it, precisely so that +removing an inconvenient item is not a way past it. + +### The labels + +| Label | Who sets it | Meaning | +| --- | --- | --- | +| `needs-checklist` | automation only | The last run found required items unmet. Removed automatically when the check passes. Not something to add or remove by hand | +| `enforce-checklist` | a maintainer | Requires the checklist on a pull request that would otherwise be exempt because it predates the check. Stays once applied, so the requirement does not lapse on the next push | + +### What is not enforced + +Three cases report their findings in the check's step summary and pass: + +- **A draft pull request.** A draft is not submitted for review, which is what the + checklist gates. Marking it ready for review re-runs the check and enforces it + from then on, so nothing is lost by the check staying quiet until you say ready. +- **A pull request opened by a bot.** Dependabot and similar write their own + description and never carry the template. +- **A pull request opened before this check existed**, meaning before + `2026-10-13T07:33:08Z`, when the template's review gates reached `main`. Those + authors contributed against the template as it stood at the time, so the check + reports warnings rather than failing. The cutoff is the moment the gated + template became visible to contributors rather than the day this check merged, + so a PR opened in between is enforced — its author had the checklist in front of + them. A maintainer can hold one of these to the full requirement with + `enforce-checklist`. + +### Seeing the result + +The check's summary on the workflow run page lists every item it found unmet, +which applied and why, and anything you marked `n/a:` with its reason — including +for the cases above that pass. The comment on the PR links to that summary rather +than repeating it, so the bot never posts text taken from a PR description. + +If the check itself cannot run — a failed fetch, say — it fails saying so, and +leaves the label and comment as they were: nothing was concluded about your PR. + +The check runs on the PR being opened, edited, pushed to, reopened, marked ready +for review, labelled, and unlabelled. Ticking a box edits the description, so the check +re-runs and clears on its own; there is nothing to ask a maintainer to re-trigger. + ## Reporting Bugs/Feature Requests We welcome you to use the GitHub issue tracker to report bugs or suggest features. diff --git a/tests/test_check_pr_checklist.py b/tests/test_check_pr_checklist.py new file mode 100644 index 00000000..f4a404d5 --- /dev/null +++ b/tests/test_check_pr_checklist.py @@ -0,0 +1,185 @@ +"""Tests for .github/scripts/check_pr_checklist.py.""" + +import json +import os +import re +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(ROOT / ".github" / "scripts")) + +import check_pr_checklist as cpc # noqa: E402 + +TEMPLATE = (ROOT / ".github" / "PULL_REQUEST_TEMPLATE.md").read_text(encoding="utf-8") +TICKED = re.sub(r"^(\s*-\s*)\[\s*\]", r"\1[x]", TEMPLATE, flags=re.M) + +SKILL_PR = ["skills/demo/SKILL.md"] +DOCS_PR = ["CONTRIBUTING.md"] + + +def body(*sections: str) -> str: + return "\n\n".join(sections) + "\n" + + +REQUIRED_ALL_TICKED = "## Required before review\n- [x] **No internal-only or proprietary content**" +LICENSE_TICKED = "## License confirmation\n- [x] I confirm the terms of the Apache License 2.0." +SECURITY_TICKED = "## Security and safety\n- [x] Read-only by default." +SKILLS_TICKED = "## Skills — complete if this PR adds or changes a skill\n- [x] frontmatter is valid" + + +class TemplateTest(unittest.TestCase): + """The check and the template must agree; a reworded template fails here first.""" + + def test_every_section_is_a_template_heading(self): + headings = [m.group(1) for m in map(cpc.HEADING.match, TEMPLATE.splitlines()) if m] + names = [*cpc.ALWAYS_SECTIONS, cpc.TOOL_SECTION, *cpc.TOOL_DIRS.values()] + for name in names: + with self.subTest(name): + matches = [h for h in headings if h.lower().startswith(name.lower())] + self.assertEqual(len(matches), 1, f"{name!r} should prefix exactly one heading, got {matches}") + + def test_every_universal_item_is_a_template_checkbox(self): + boxes = [line for line in TEMPLATE.splitlines() if cpc.UNCHECKED.match(line)] + for pattern in cpc.UNIVERSAL_ITEMS: + with self.subTest(pattern.pattern): + self.assertEqual(sum(bool(pattern.search(b)) for b in boxes), 1) + + def test_untouched_template_waives_nothing(self): + # Prose in the template that happens to read as `n/a: ` would let + # every author skip that item without writing anything. + unmet, waived, _ = cpc.evaluate(TEMPLATE, SKILL_PR) + self.assertEqual(waived, []) + self.assertTrue(any("domain SME" in u for u in unmet)) + + def test_untouched_template_on_a_docs_pr_holds_only_the_universal_items(self): + unmet, _, is_tool = cpc.evaluate(TEMPLATE, DOCS_PR) + self.assertFalse(is_tool) + self.assertEqual(len(unmet), 2) + self.assertIn("internal-only", unmet[0]) + self.assertIn("Apache License", unmet[1]) + + def test_fully_ticked_template_passes_on_every_tool_type(self): + paths = ["skills/a/SKILL.md", "mcp/b/README.md", "custom-agents/c/SYSTEM_PROMPT.md"] + self.assertEqual(cpc.evaluate(TICKED, paths)[0], []) + + +class EvaluateTest(unittest.TestCase): + def test_sme_item_can_be_marked_na_with_a_reason(self): + text = body( + "## Required before review\n" + "- [x] **No internal-only or proprietary content**\n" + "- [ ] **A domain SME has reviewed and signed off.** n/a: renames a label, changes no technical claim", + LICENSE_TICKED, SECURITY_TICKED, SKILLS_TICKED, + ) + unmet, waived, _ = cpc.evaluate(text, SKILL_PR) + self.assertEqual(unmet, []) + self.assertEqual(len(waived), 1) + self.assertIn("domain SME", waived[0]) + + def test_universal_items_cannot_be_marked_na(self): + text = body(REQUIRED_ALL_TICKED, "## License confirmation\n- [ ] Apache License 2.0 n/a: not mine to give") + unmet, waived, _ = cpc.evaluate(text, DOCS_PR) + self.assertEqual(waived, []) + self.assertEqual(len(unmet), 1) + self.assertIn("cannot be marked n/a", unmet[0]) + + def test_bare_na_waives_nothing(self): + text = body( + "## Required before review\n- [x] No internal-only or proprietary content\n- [ ] **CHANGELOG updated** n/a", + LICENSE_TICKED, SECURITY_TICKED, SKILLS_TICKED, + ) + unmet, waived, _ = cpc.evaluate(text, SKILL_PR) + self.assertEqual(waived, []) + self.assertEqual(len(unmet), 1) + + def test_a_deleted_section_is_unmet(self): + unmet, _, _ = cpc.evaluate(body(REQUIRED_ALL_TICKED, LICENSE_TICKED, SECURITY_TICKED), SKILL_PR) + self.assertEqual(len(unmet), 1) + self.assertTrue(unmet[0].startswith("Skills → section missing")) + + def test_a_shortened_heading_still_finds_its_section(self): + text = body(REQUIRED_ALL_TICKED, LICENSE_TICKED, SECURITY_TICKED, "## Skills\n- [x] frontmatter is valid") + self.assertEqual(cpc.evaluate(text, SKILL_PR)[0], []) + + def test_tool_items_do_not_apply_to_a_pr_touching_no_tool(self): + text = body("## Required before review\n- [x] No internal-only or proprietary content\n- [ ] README is complete", + LICENSE_TICKED) + for paths in (DOCS_PR, ["connectors/acme/connector.yaml"]): + with self.subTest(paths): + self.assertEqual(cpc.evaluate(text, paths)[0], []) + + +class DecideTest(unittest.TestCase): + AFTER = "2026-10-20T00:00:00Z" + BEFORE = "2026-10-01T00:00:00Z" + + def pr(self, **fields): + base = {"created_at": self.AFTER, "draft": False, "user": {"type": "User"}, "labels": []} + return {**base, **fields} + + def test_outcomes(self): + cases = [ + ("nothing unmet", self.pr(), [], ("pass", "complete")), + ("unmet", self.pr(), ["x"], ("fail", "incomplete")), + ("bot", self.pr(user={"type": "Bot"}), ["x"], ("pass", "bot")), + ("draft", self.pr(draft=True), ["x"], ("pass", "draft")), + ("draft with the label", self.pr(draft=True, labels=[{"name": cpc.ENFORCE_LABEL}]), ["x"], ("pass", "draft")), + ("predating", self.pr(created_at=self.BEFORE), ["x"], ("pass", "predating")), + ("predating with the label", self.pr(created_at=self.BEFORE, labels=[{"name": cpc.ENFORCE_LABEL}]), ["x"], + ("fail", "incomplete")), + ("no creation time", self.pr(created_at=""), ["x"], ("fail", "incomplete")), + ] + for name, pr, unmet, expected in cases: + with self.subTest(name): + self.assertEqual(cpc.decide(pr, unmet), expected) + + +class MainTest(unittest.TestCase): + def setUp(self): + self.tmp = Path(tempfile.mkdtemp()) + self.out = self.tmp / "out" + self.addCleanup(subprocess.run, ["rm", "-rf", str(self.tmp)]) + + def git(self, *args): + subprocess.run(["git", "-C", str(self.tmp / "repo"), *args], check=True, capture_output=True) + + def event(self, pr_body): + path = self.tmp / "event.json" + path.write_text(json.dumps({"pull_request": { + "body": pr_body, "number": 1, "created_at": "2026-10-20T00:00:00Z", + "draft": False, "user": {"type": "User"}, "labels": [], + }}), encoding="utf-8") + return str(path) + + def test_an_unreadable_event_is_an_error_not_a_finding(self): + code = cpc.main(["--event", str(self.tmp / "missing.json"), "--base-ref", "main", "--out", str(self.out)]) + self.assertEqual(code, 2) + self.assertEqual((self.out / "outcome").read_text().strip(), "error") + self.assertFalse((self.out / "reason").exists()) + + def test_a_skill_pr_with_the_untouched_template_fails(self): + repo = self.tmp / "repo" + repo.mkdir() + self.git("init", "-q", "-b", "main") + self.git("-c", "user.name=t", "-c", "user.email=t@example.com", "commit", "-q", "--allow-empty", "-m", "base") + self.git("checkout", "-q", "-b", "pr") + (repo / "skills" / "demo").mkdir(parents=True) + (repo / "skills" / "demo" / "SKILL.md").write_text("x\n", encoding="utf-8") + self.git("add", ".") + self.git("-c", "user.name=t", "-c", "user.email=t@example.com", "commit", "-q", "-m", "skill") + cwd = os.getcwd() + os.chdir(repo) + self.addCleanup(os.chdir, cwd) + code = cpc.main(["--event", self.event(TEMPLATE), "--base-ref", "main", "--out", str(self.out)]) + self.assertEqual(code, 1) + self.assertEqual((self.out / "outcome").read_text().strip(), "fail") + self.assertEqual((self.out / "reason").read_text().strip(), "incomplete") + self.assertEqual(sorted(p.name for p in self.out.iterdir()), ["outcome", "reason"]) + + +if __name__ == "__main__": + unittest.main()