From 26be6c13fe93e9246611a4f652443d6987da2af3 Mon Sep 17 00:00:00 2001 From: Amrish Thakkar Date: Thu, 22 Oct 2026 14:03:02 -0500 Subject: [PATCH 1/2] feat(ci): comment on pull requests with an incomplete checklist Authors open pull requests without completing the template's required items and nothing catches it, so a PR waits for a review that never starts. Add a check that reads the description, scopes the checklist by the files the PR touches, and posts one comment listing what is outstanding. Split in two because GITHUB_TOKEN is read-only for pull_request events from a fork and most contributions here come from forks, so the validating job cannot label or comment on its own PR. Same pattern as validate-skill-evals and its companion. Drafts, bot authors and pull requests opened before the gated template reached main are reported but not enforced, and the outcome carries a reason so the comment never tells an author they are done when they are not. Also document the two things the template requires that CONTRIBUTING.md never explained: the domain SME sign-off, and the checklist check itself with its n/a escape and its labels. --- .../workflows/label-pr-checklist-result.yml | 128 ++++++++ .github/workflows/validate-pr-checklist.yml | 306 ++++++++++++++++++ CONTRIBUTING.md | 160 +++++++++ 3 files changed, 594 insertions(+) create mode 100644 .github/workflows/label-pr-checklist-result.yml create mode 100644 .github/workflows/validate-pr-checklist.yml diff --git a/.github/workflows/label-pr-checklist-result.yml b/.github/workflows/label-pr-checklist-result.yml new file mode 100644 index 00000000..fe0a5be0 --- /dev/null +++ b/.github/workflows/label-pr-checklist-result.yml @@ -0,0 +1,128 @@ +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 clears 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 — and most contributions here come from forks. 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. +# +# The PR number is read from an artifact rather than from the workflow_run +# payload, because `workflow_run.pull_requests` is empty for fork PRs. + +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 (a push landing while an edit is still being checked) could both reach +# the commenting step and race. The PR number is not in this payload for fork PRs, +# but the head repository and branch are, and together they 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 {owner, repo} = context.repo; + + let prNumber, outcome, reason = "complete", missing = ""; + try { + prNumber = parseInt(fs.readFileSync("result/pr-number", "utf8").replace(/\D/g, ""), 10); + outcome = fs.readFileSync("result/outcome", "utf8").trim(); + if (fs.existsSync("result/reason")) reason = fs.readFileSync("result/reason", "utf8").trim(); + if (fs.existsSync("result/missing.md")) missing = fs.readFileSync("result/missing.md", "utf8").trim(); + } catch (e) { + core.notice("No outcome artifact found; nothing to label."); + return; + } + if (!prNumber || (outcome !== "pass" && outcome !== "fail")) { + core.warning(`Unexpected artifact contents (pr='${prNumber}' outcome='${outcome}').`); + 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 pr = await github.rest.pulls.get({owner, repo, pull_number: prNumber}); + const hasLabel = (pr.data.labels || []).some(l => l.name === LABEL); + + const comments = await github.paginate(github.rest.issues.listComments, {owner, repo, issue_number: prNumber}); + const prev = comments.find(c => (c.body || "").includes(MARKER)); + + if (outcome === "fail") { + if (!hasLabel) await github.rest.issues.addLabels({owner, repo, issue_number: prNumber, labels: [LABEL]}); + const comment = [ + MARKER, + "### This PR is on hold — checklist incomplete", + "", + "A maintainer will not review it until every required item below is checked **and actually done**:", + "", + missing, + "", + "Complete the work, tick the boxes in the PR description, and this check clears automatically. See CONTRIBUTING.md for the detail behind 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: prNumber, body: comment}); + core.notice(`Labeled PR #${prNumber} with ${LABEL}.`); + } else { + // Passing is not the same as complete. Only say "complete" when the + // check actually found nothing unmet; otherwise say why it is not + // being enforced, so an author is never told they are done when they + // are not. A draft or a bot PR that was never labeled gets no comment + // at all — there is nothing to correct and nothing to chase. + if (hasLabel) { + try { + await github.rest.issues.removeLabel({owner, repo, issue_number: prNumber, 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 #${prNumber}: ${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 and will list them.", + 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.", + }; + const text = bodies[reason] || bodies.complete; + if (prev) await github.rest.issues.updateComment({owner, repo, comment_id: prev.id, body: `${MARKER}\n${text}`}); + core.notice(`PR #${prNumber} 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..538e6094 --- /dev/null +++ b/.github/workflows/validate-pr-checklist.yml @@ -0,0 +1,306 @@ +name: Validate PR checklist +# Gates pull requests on the required checkboxes in the PR description being +# checked. Reads the PR body from the event payload, works out which checklists +# apply from the FILES the PR changes (not from a checkbox, which is ambiguous — +# the repo's "Update to an existing skill, agent, or MCP server" option names more +# than one type), finds unchecked required items, writes the outcome to an +# artifact, and fails the job when anything required is missing. A companion +# workflow (label-pr-checklist-result.yml) then applies or removes the +# `needs-checklist` label and posts the comment. +# +# Why split into two workflows: for pull_request events from a fork, GITHUB_TOKEN +# is read-only regardless of the `permissions:` block, and most contributions to +# this repo come from forks. So this job cannot label or comment on its own PR. +# It records the result in an artifact; the workflow_run companion runs in the +# base repository's context with write access and acts on it. This mirrors the +# validate-skill-evals.yml / label-skill-evals-result.yml pair. +# +# Scope: this check owns the checklist only. Eval evidence is NOT re-checked here +# — validate-skill-evals.yml already gates that. The two checks are independent. +# +# Two things keep the gate from blocking work it was never meant to judge: +# +# 1. Scope by what the PR touches. Most of "Required before review" describes a +# tool contribution — layout, SME sign-off, eval evidence, README, CHANGELOG +# and a named owner. None of it means anything on a PR that only edits +# CONTRIBUTING.md or a workflow. On a PR that touches no tool directory, only +# the items that apply to every contribution are enforced (no internal-only +# content, and the licence confirmation). A tool PR is held to all of them. +# +# 2. An explicit not-applicable escape. Inside a tool PR an individual item can +# still be genuinely moot — an MCP server with no IAM to scope, say. Write +# `n/a: ` on the item and it is treated as met, with the reason +# visible in the description for a reviewer to judge. Three items can never +# be waived this way: the domain SME sign-off (the template calls it +# "Required, not optional"), the internal-content item, and the licence. +# Without the escape the only ways out are to tick something untrue or to +# delete the line, and deleting it is worse: a deleted item is invisible. +# +# 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` is required: ticking a box edits the body and must re-run the check. + # `labeled` lets a maintainer force a run on an already-open PR, which + # otherwise reports no status at all and sits blocked without a log, and makes + # `enforce-checklist` take effect the moment it is applied. Note that a label + # applied by GITHUB_TOKEN raises no `labeled` event, so the companion workflow + # cannot re-trigger this one. + types: [opened, edited, synchronize, reopened, ready_for_review, labeled] + # 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. +# Read-only: the job only reads the event payload and the PR's file list via the +# API, so fork PRs work without elevated permissions. All labeling and commenting +# happens in the companion. +permissions: + contents: read + pull-requests: 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: + - name: Check the required checklist items + id: check + # Never fails the step: the outcome is recorded and published for the + # labeling workflow, 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 + uses: actions/github-script@v7 + # Every value from the event payload is passed through the environment + # rather than interpolated into the script. A PR body is wholly + # attacker-controlled, and label names are attacker-controllable in + # principle, so neither is ever spliced into code. + env: + PR_BODY: ${{ github.event.pull_request.body }} + PR_NUMBER: ${{ github.event.pull_request.number }} + PR_CREATED_AT: ${{ github.event.pull_request.created_at }} + PR_LABELS: ${{ toJSON(github.event.pull_request.labels.*.name) }} + # A bot cannot fill in a template, and a draft is not submitted for + # review yet. Both are reported but not enforced; see EXEMPT below. + PR_AUTHOR_TYPE: ${{ github.event.pull_request.user.type }} + PR_DRAFT: ${{ github.event.pull_request.draft }} + with: + script: | + const fs = require("fs"); + + // Pull requests opened before this check existed cannot have used the + // template it enforces, so enforcing it on them would fail every open + // PR at once with nothing the author did wrong. They report their + // findings as warnings and pass. This is the same accommodation + // validate_skill_evals.py makes with its PRS_PREDATING_CHECK list, but + // keyed on the PR's creation time rather than a list of numbers, so + // there is nothing to maintain and nothing to forget to remove. + // + // This is the moment the gated PR template reached `main`, in commit + // 49f31c1 by way of PR #135 — not the day this check merges. The rule + // the check enforces is the template's, so it applies from when the + // template became available to contributors, and a PR opened after + // that had the checklist in front of its author. + // + // Deliberately earlier than this check's own merge date, so PRs opened + // in the interval are enforced rather than grandfathered. Note the + // commit is dated 2026-10-12 but only merged the following morning; + // the merge time is what a contributor could actually see, so that is + // the cutoff. No currently-open PR falls between the two. + // + // A PR opened before this reports its findings as warnings and passes, + // unless a maintainer applies the `enforce-checklist` label. + const CHECK_INTRODUCED = "2026-10-13T07:33:08Z"; + const ENFORCE_LABEL = "enforce-checklist"; + + // Directories that hold a contributed tool. A PR touching none of them + // is not a tool contribution, and the tool-specific items do not apply. + const TOOL_PREFIXES = ["skills/", "mcp/", "custom-agents/"]; + + // Items that apply to every contribution, whatever it changes. Matched + // on a stable phrase rather than the full line, so rewording the + // template's prose does not silently drop the item from the gate. + const UNIVERSAL_ITEMS = [ + /internal[-\s]only or proprietary/i, + /Apache License 2\.0/i, + ]; + + // Items that may never be waived with `n/a:`. + const NO_WAIVER = [ + /domain SME|subject[-\s]matter expert/i, + /internal[-\s]only or proprietary/i, + /Apache License 2\.0/i, + ]; + + // `n/a:` followed by a reason. A bare "n/a" is not enough — the reason + // is the part a reviewer judges. + const NA_WITH_REASON = /\bn\/a\s*:\s*\S/i; + + const body = process.env.PR_BODY || ""; + const number = parseInt(process.env.PR_NUMBER, 10); + const createdAt = process.env.PR_CREATED_AT || ""; + + // Two kinds of PR are reported but never failed or labeled. + // + // A bot author. Dependabot and friends write their own description + // and will never carry the template, so enforcing it would put a + // `needs-checklist` label and a "complete the checklist" comment on + // every dependency bump, forever, with no one able to act on it. + // + // A draft. A draft is explicitly not submitted for review, which is + // what the checklist gates. Marking it ready raises + // `ready_for_review`, this check re-runs, and it is enforced then — + // so nothing is lost by staying quiet until the author says ready. + // + // Both still write their findings to the step summary, and both report + // `pass`, so the companion clears a label left over from an earlier run + // (a PR converted back to draft, for instance). + const isBot = (process.env.PR_AUTHOR_TYPE || "") === "Bot"; + const isDraft = (process.env.PR_DRAFT || "") === "true"; + let labels = []; + try { labels = JSON.parse(process.env.PR_LABELS || "[]"); } catch { labels = []; } + const { owner, repo } = context.repo; + + const isUnchecked = (line) => /^\s*-\s*\[\s*\]/.test(line); + const matchesAny = (text, patterns) => patterns.some((p) => p.test(text)); + + // Split the body into sections keyed by "## Heading". + const sections = {}; + let current = "_preamble"; + for (const line of body.split(/\r?\n/)) { + const h = line.match(/^##\s+(.*)$/); + if (h) { current = h[1].trim(); sections[current] = []; continue; } + (sections[current] ||= []).push(line); + } + + // Which checklists apply is decided by the FILES the PR touches, not by + // a checkbox. This matches how validate-skill-evals scopes itself, and + // cannot be fooled by the combined "Update to an existing skill, agent, + // or MCP server" option. + const files = await github.paginate(github.rest.pulls.listFiles, { + owner, repo, pull_number: number, per_page: 100, + }); + const paths = files.map((f) => f.filename); + const touches = (prefix) => paths.some((p) => p.startsWith(prefix)); + const wantsSkill = touches("skills/"); + const wantsMcp = touches("mcp/"); + const wantsAgent = touches("custom-agents/"); + const isToolPr = TOOL_PREFIXES.some((p) => touches(p)); + + // Sections every PR must carry. On a non-tool PR only the universal + // items inside them are enforced; the rest are out of scope. + const required = ["Required before review", "License confirmation"]; + // Sections that only mean something for a contributed tool. + if (isToolPr) required.push("Security and safety"); + if (wantsSkill) required.push("Skills — complete if this PR adds or changes a skill"); + if (wantsMcp) required.push("MCP servers — complete if this PR adds or changes an MCP server"); + if (wantsAgent) required.push("Custom agents — complete if this PR adds or changes a custom agent"); + + const missing = []; + const waived = []; + for (const name of required) { + const sec = sections[name]; + if (!sec) { + // A section the files say must apply, but the author removed it from + // the description. Treat as unmet rather than skipping, so deleting + // a required section cannot bypass the gate. + missing.push(`- **${name}** → section is required for the files this PR changes but is missing from the PR description. Restore it and complete it.`); + continue; + } + for (const line of sec) { + if (!isUnchecked(line)) continue; + const label = line.replace(/^\s*-\s*\[\s*\]\s*/, "").replace(/\*\*/g, "").trim(); + if (!label) continue; + // Out of scope: a tool-specific item on a PR that touches no tool. + if (!isToolPr && !matchesAny(label, UNIVERSAL_ITEMS)) continue; + // Waived with a stated reason, where waiving is allowed. + if (NA_WITH_REASON.test(label)) { + if (matchesAny(label, NO_WAIVER)) { + missing.push(`- **${name}** → ${label} ← this item cannot be marked n/a`); + } else { + waived.push(`- **${name}** → ${label}`); + } + continue; + } + missing.push(`- **${name}** → ${label}`); + } + } + + // Grandfathering. A PR opened before the check existed reports its + // findings and passes, unless a maintainer has asked for enforcement. + const enforced = labels.includes(ENFORCE_LABEL); + const predating = createdAt !== "" && createdAt < CHECK_INTRODUCED && !enforced; + + // A bot or a draft is never enforced, and unlike grandfathering the + // `enforce-checklist` label does not override it: labeling a draft + // would nag an author who has already said they are not finished. + const exempt = isBot || isDraft; + + const failing = missing.length > 0 && !predating && !exempt; + const outcome = failing ? "fail" : "pass"; + + // Why it passed, not just that it did. The companion posts the comment + // and cannot see any of this, so without a reason it would tell the + // author of a draft or a grandfathered PR "checklist complete" while + // items are still unmet — the one message that must never be wrong. + const reason = missing.length === 0 ? "complete" + : isBot ? "bot" + : isDraft ? "draft" + : predating ? "predating" + : "incomplete"; + + fs.mkdirSync("checklist-result", { recursive: true }); + fs.writeFileSync("checklist-result/pr-number", String(number) + "\n"); + fs.writeFileSync("checklist-result/outcome", outcome + "\n"); + fs.writeFileSync("checklist-result/reason", reason + "\n"); + fs.writeFileSync("checklist-result/missing.md", missing.join("\n") + "\n"); + + // Step summary, so a predating PR's findings are visible somewhere even + // though nothing is labeled or commented on. + const summary = []; + summary.push("## PR checklist"); + summary.push(""); + summary.push(isToolPr + ? "This PR touches a tool directory, so every required item applies." + : "This PR touches no tool directory, so only the items that apply to every contribution are enforced (internal content, licence)."); + if (waived.length) { + summary.push("", "Marked not applicable, with a reason:", "", ...waived); + } + if (missing.length) { + let why = "Unmet required items:"; + if (isBot) why = "Opened by a bot, so the checklist is not enforced. Reported only:"; + else if (isDraft) why = "Still a draft, so the checklist is not enforced. It is enforced when you mark the PR ready for review. Reported only:"; + else if (predating) why = `Opened before this check existed (${createdAt}), so these are warnings and the check passes. Apply the \`${ENFORCE_LABEL}\` label to require them.`; + summary.push("", why, "", ...missing); + } else { + summary.push("", "All applicable required items are met."); + } + await core.summary.addRaw(summary.join("\n")).write(); + + if (exempt && missing.length) { + core.info(`PR #${number} is ${isBot ? "bot-authored" : "a draft"}; ${missing.length} item(s) reported, not enforced.`); + } else if (predating && missing.length) { + for (const m of missing) core.warning(m.replace(/^- /, "")); + core.info(`PR #${number} predates this check; ${missing.length} item(s) reported as warnings.`); + } else if (failing) { + core.setFailed(`Checklist incomplete: ${missing.length} required item(s) unmet.`); + } else { + core.info("All applicable required checklist items are met."); + } + - uses: actions/upload-artifact@v4 + with: + name: checklist-check-result + path: checklist-result/ + retention-days: 1 + - name: Fail the job when the checklist is incomplete + if: steps.check.outcome != 'success' + run: | + echo "::error::PR checklist incomplete — see the comment on the PR." + exit 1 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index df95c361..71569a09 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -262,6 +262,67 @@ 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, and the template +calls it required rather than optional for that reason. + +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. + +This item cannot be waived with the `n/a:` escape described under "The Pull +Request Checklist". There is no tool contribution for which domain correctness is +genuinely not applicable. + ## 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 +438,105 @@ 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 listing 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, wherever they sit: 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. + +Three items can never be waived this way: + +- the **domain SME sign-off**, for the reason given in its own section above +- **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 carries the same list. + +The check runs on the PR being opened, edited, pushed to, reopened, marked ready +for review, and labelled. 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. From 2ce9947a9840d18d2e246cbcef9b3ade241e13d0 Mon Sep 17 00:00:00 2001 From: Amrish Thakkar Date: Mon, 26 Oct 2026 14:06:40 -0500 Subject: [PATCH 2/2] fix(ci): Harden the PR checklist check Address review feedback on #156. - The labeler no longer posts or trusts artifact text. It resolves the PR from the workflow_run payload (head repository, branch and commit), so a fork cannot point the comment at another PR, and the comment is fixed text that links to the run's summary for the unmet items. A stale run matches no PR and does nothing. - Three outcomes instead of two. A check that cannot run reports `error`, says so, and leaves the label and comment alone. - The domain SME item can be marked n/a with a reason, for edits that change no technical claim. The template says so too. - The rules move to .github/scripts/check_pr_checklist.py with tests, including tests that pin the check to the PR template. - Section headings match by prefix; `unlabeled` re-runs the check; CONTRIBUTING no longer says universal items are found anywhere. --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- .github/scripts/check_pr_checklist.py | 210 ++++++++++++ .../workflows/label-pr-checklist-result.yml | 133 +++++--- .github/workflows/validate-pr-checklist.yml | 323 +++--------------- CONTRIBUTING.md | 26 +- tests/test_check_pr_checklist.py | 185 ++++++++++ 6 files changed, 545 insertions(+), 334 deletions(-) create mode 100644 .github/scripts/check_pr_checklist.py create mode 100644 tests/test_check_pr_checklist.py 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 index fe0a5be0..a2824e03 100644 --- a/.github/workflows/label-pr-checklist-result.yml +++ b/.github/workflows/label-pr-checklist-result.yml @@ -1,7 +1,7 @@ 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 clears the +# "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 @@ -9,12 +9,25 @@ name: Label PR checklist result # # 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 — and most contributions here come from forks. 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. +# 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. # -# The PR number is read from an artifact rather than from the workflow_run -# payload, because `workflow_run.pull_requests` is empty for fork PRs. +# 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: @@ -28,10 +41,9 @@ permissions: # 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 (a push landing while an edit is still being checked) could both reach -# the commenting step and race. The PR number is not in this payload for fork PRs, -# but the head repository and branch are, and together they identify the PR. -# `cancel-in-progress: false` so a labeler mid-write is never killed; runs queue. +# 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 @@ -59,20 +71,36 @@ jobs: 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 ""; } + }; - let prNumber, outcome, reason = "complete", missing = ""; - try { - prNumber = parseInt(fs.readFileSync("result/pr-number", "utf8").replace(/\D/g, ""), 10); - outcome = fs.readFileSync("result/outcome", "utf8").trim(); - if (fs.existsSync("result/reason")) reason = fs.readFileSync("result/reason", "utf8").trim(); - if (fs.existsSync("result/missing.md")) missing = fs.readFileSync("result/missing.md", "utf8").trim(); - } catch (e) { - core.notice("No outcome artifact found; nothing to label."); + 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 (!prNumber || (outcome !== "pass" && outcome !== "fail")) { - core.warning(`Unexpected artifact contents (pr='${prNumber}' outcome='${outcome}').`); + 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; } @@ -80,49 +108,44 @@ jobs: 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 pr = await github.rest.pulls.get({owner, repo, pull_number: prNumber}); - const hasLabel = (pr.data.labels || []).some(l => l.name === LABEL); - - const comments = await github.paginate(github.rest.issues.listComments, {owner, repo, issue_number: prNumber}); - const prev = comments.find(c => (c.body || "").includes(MARKER)); + 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: prNumber, labels: [LABEL]}); + 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", "", - "A maintainer will not review it until every required item below is checked **and actually done**:", - "", - missing, + "Required items in the PR description are unchecked. A maintainer will not review this PR until each one is checked **and actually done**.", "", - "Complete the work, tick the boxes in the PR description, and this check clears automatically. See CONTRIBUTING.md for the detail behind each item.", + `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: prNumber, body: comment}); - core.notice(`Labeled PR #${prNumber} with ${LABEL}.`); - } else { - // Passing is not the same as complete. Only say "complete" when the - // check actually found nothing unmet; otherwise say why it is not - // being enforced, so an author is never told they are done when they - // are not. A draft or a bot PR that was never labeled gets no comment - // at all — there is nothing to correct and nothing to chase. - if (hasLabel) { - try { - await github.rest.issues.removeLabel({owner, repo, issue_number: prNumber, 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 #${prNumber}: ${e.message}`); - } + 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 and will list them.", - 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.", - }; - const text = bodies[reason] || bodies.complete; - if (prev) await github.rest.issues.updateComment({owner, repo, comment_id: prev.id, body: `${MARKER}\n${text}`}); - core.notice(`PR #${prNumber} passed the checklist check (${reason}).`); } + 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 index 538e6094..b2242fa2 100644 --- a/.github/workflows/validate-pr-checklist.yml +++ b/.github/workflows/validate-pr-checklist.yml @@ -1,61 +1,30 @@ name: Validate PR checklist # Gates pull requests on the required checkboxes in the PR description being -# checked. Reads the PR body from the event payload, works out which checklists -# apply from the FILES the PR changes (not from a checkbox, which is ambiguous — -# the repo's "Update to an existing skill, agent, or MCP server" option names more -# than one type), finds unchecked required items, writes the outcome to an -# artifact, and fails the job when anything required is missing. A companion -# workflow (label-pr-checklist-result.yml) then applies or removes the -# `needs-checklist` label and posts the comment. +# 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. # -# Why split into two workflows: for pull_request events from a fork, GITHUB_TOKEN -# is read-only regardless of the `permissions:` block, and most contributions to -# this repo come from forks. So this job cannot label or comment on its own PR. -# It records the result in an artifact; the workflow_run companion runs in the -# base repository's context with write access and acts on it. This mirrors the -# validate-skill-evals.yml / label-skill-evals-result.yml pair. -# -# Scope: this check owns the checklist only. Eval evidence is NOT re-checked here -# — validate-skill-evals.yml already gates that. The two checks are independent. -# -# Two things keep the gate from blocking work it was never meant to judge: -# -# 1. Scope by what the PR touches. Most of "Required before review" describes a -# tool contribution — layout, SME sign-off, eval evidence, README, CHANGELOG -# and a named owner. None of it means anything on a PR that only edits -# CONTRIBUTING.md or a workflow. On a PR that touches no tool directory, only -# the items that apply to every contribution are enforced (no internal-only -# content, and the licence confirmation). A tool PR is held to all of them. -# -# 2. An explicit not-applicable escape. Inside a tool PR an individual item can -# still be genuinely moot — an MCP server with no IAM to scope, say. Write -# `n/a: ` on the item and it is treated as met, with the reason -# visible in the description for a reviewer to judge. Three items can never -# be waived this way: the domain SME sign-off (the template calls it -# "Required, not optional"), the internal-content item, and the licence. -# Without the escape the only ways out are to tick something untrue or to -# delete the line, and deleting it is worse: a deleted item is invisible. +# 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` is required: ticking a box edits the body and must re-run the check. - # `labeled` lets a maintainer force a run on an already-open PR, which - # otherwise reports no status at all and sits blocked without a log, and makes - # `enforce-checklist` take effect the moment it is applied. Note that a label - # applied by GITHUB_TOKEN raises no `labeled` event, so the companion workflow - # cannot re-trigger this one. - types: [opened, edited, synchronize, reopened, ready_for_review, labeled] + # `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. -# Read-only: the job only reads the event payload and the PR's file list via the -# API, so fork PRs work without elevated permissions. All labeling and commenting -# happens in the companion. permissions: contents: read - pull-requests: read concurrency: group: validate-pr-checklist-${{ github.event.pull_request.number }} cancel-in-progress: true @@ -71,236 +40,56 @@ jobs: # 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 - id: check - # Never fails the step: the outcome is recorded and published for the - # labeling workflow, and only then turned into a job failure. Without this - # the artifact would not upload on failure — the case that needs a label. + # 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 - uses: actions/github-script@v7 - # Every value from the event payload is passed through the environment - # rather than interpolated into the script. A PR body is wholly - # attacker-controlled, and label names are attacker-controllable in - # principle, so neither is ever spliced into code. + # 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_BODY: ${{ github.event.pull_request.body }} PR_NUMBER: ${{ github.event.pull_request.number }} - PR_CREATED_AT: ${{ github.event.pull_request.created_at }} - PR_LABELS: ${{ toJSON(github.event.pull_request.labels.*.name) }} - # A bot cannot fill in a template, and a draft is not submitted for - # review yet. Both are reported but not enforced; see EXEMPT below. - PR_AUTHOR_TYPE: ${{ github.event.pull_request.user.type }} - PR_DRAFT: ${{ github.event.pull_request.draft }} - with: - script: | - const fs = require("fs"); - - // Pull requests opened before this check existed cannot have used the - // template it enforces, so enforcing it on them would fail every open - // PR at once with nothing the author did wrong. They report their - // findings as warnings and pass. This is the same accommodation - // validate_skill_evals.py makes with its PRS_PREDATING_CHECK list, but - // keyed on the PR's creation time rather than a list of numbers, so - // there is nothing to maintain and nothing to forget to remove. - // - // This is the moment the gated PR template reached `main`, in commit - // 49f31c1 by way of PR #135 — not the day this check merges. The rule - // the check enforces is the template's, so it applies from when the - // template became available to contributors, and a PR opened after - // that had the checklist in front of its author. - // - // Deliberately earlier than this check's own merge date, so PRs opened - // in the interval are enforced rather than grandfathered. Note the - // commit is dated 2026-10-12 but only merged the following morning; - // the merge time is what a contributor could actually see, so that is - // the cutoff. No currently-open PR falls between the two. - // - // A PR opened before this reports its findings as warnings and passes, - // unless a maintainer applies the `enforce-checklist` label. - const CHECK_INTRODUCED = "2026-10-13T07:33:08Z"; - const ENFORCE_LABEL = "enforce-checklist"; - - // Directories that hold a contributed tool. A PR touching none of them - // is not a tool contribution, and the tool-specific items do not apply. - const TOOL_PREFIXES = ["skills/", "mcp/", "custom-agents/"]; - - // Items that apply to every contribution, whatever it changes. Matched - // on a stable phrase rather than the full line, so rewording the - // template's prose does not silently drop the item from the gate. - const UNIVERSAL_ITEMS = [ - /internal[-\s]only or proprietary/i, - /Apache License 2\.0/i, - ]; - - // Items that may never be waived with `n/a:`. - const NO_WAIVER = [ - /domain SME|subject[-\s]matter expert/i, - /internal[-\s]only or proprietary/i, - /Apache License 2\.0/i, - ]; - - // `n/a:` followed by a reason. A bare "n/a" is not enough — the reason - // is the part a reviewer judges. - const NA_WITH_REASON = /\bn\/a\s*:\s*\S/i; - - const body = process.env.PR_BODY || ""; - const number = parseInt(process.env.PR_NUMBER, 10); - const createdAt = process.env.PR_CREATED_AT || ""; - - // Two kinds of PR are reported but never failed or labeled. - // - // A bot author. Dependabot and friends write their own description - // and will never carry the template, so enforcing it would put a - // `needs-checklist` label and a "complete the checklist" comment on - // every dependency bump, forever, with no one able to act on it. - // - // A draft. A draft is explicitly not submitted for review, which is - // what the checklist gates. Marking it ready raises - // `ready_for_review`, this check re-runs, and it is enforced then — - // so nothing is lost by staying quiet until the author says ready. - // - // Both still write their findings to the step summary, and both report - // `pass`, so the companion clears a label left over from an earlier run - // (a PR converted back to draft, for instance). - const isBot = (process.env.PR_AUTHOR_TYPE || "") === "Bot"; - const isDraft = (process.env.PR_DRAFT || "") === "true"; - let labels = []; - try { labels = JSON.parse(process.env.PR_LABELS || "[]"); } catch { labels = []; } - const { owner, repo } = context.repo; - - const isUnchecked = (line) => /^\s*-\s*\[\s*\]/.test(line); - const matchesAny = (text, patterns) => patterns.some((p) => p.test(text)); - - // Split the body into sections keyed by "## Heading". - const sections = {}; - let current = "_preamble"; - for (const line of body.split(/\r?\n/)) { - const h = line.match(/^##\s+(.*)$/); - if (h) { current = h[1].trim(); sections[current] = []; continue; } - (sections[current] ||= []).push(line); - } - - // Which checklists apply is decided by the FILES the PR touches, not by - // a checkbox. This matches how validate-skill-evals scopes itself, and - // cannot be fooled by the combined "Update to an existing skill, agent, - // or MCP server" option. - const files = await github.paginate(github.rest.pulls.listFiles, { - owner, repo, pull_number: number, per_page: 100, - }); - const paths = files.map((f) => f.filename); - const touches = (prefix) => paths.some((p) => p.startsWith(prefix)); - const wantsSkill = touches("skills/"); - const wantsMcp = touches("mcp/"); - const wantsAgent = touches("custom-agents/"); - const isToolPr = TOOL_PREFIXES.some((p) => touches(p)); - - // Sections every PR must carry. On a non-tool PR only the universal - // items inside them are enforced; the rest are out of scope. - const required = ["Required before review", "License confirmation"]; - // Sections that only mean something for a contributed tool. - if (isToolPr) required.push("Security and safety"); - if (wantsSkill) required.push("Skills — complete if this PR adds or changes a skill"); - if (wantsMcp) required.push("MCP servers — complete if this PR adds or changes an MCP server"); - if (wantsAgent) required.push("Custom agents — complete if this PR adds or changes a custom agent"); - - const missing = []; - const waived = []; - for (const name of required) { - const sec = sections[name]; - if (!sec) { - // A section the files say must apply, but the author removed it from - // the description. Treat as unmet rather than skipping, so deleting - // a required section cannot bypass the gate. - missing.push(`- **${name}** → section is required for the files this PR changes but is missing from the PR description. Restore it and complete it.`); - continue; - } - for (const line of sec) { - if (!isUnchecked(line)) continue; - const label = line.replace(/^\s*-\s*\[\s*\]\s*/, "").replace(/\*\*/g, "").trim(); - if (!label) continue; - // Out of scope: a tool-specific item on a PR that touches no tool. - if (!isToolPr && !matchesAny(label, UNIVERSAL_ITEMS)) continue; - // Waived with a stated reason, where waiving is allowed. - if (NA_WITH_REASON.test(label)) { - if (matchesAny(label, NO_WAIVER)) { - missing.push(`- **${name}** → ${label} ← this item cannot be marked n/a`); - } else { - waived.push(`- **${name}** → ${label}`); - } - continue; - } - missing.push(`- **${name}** → ${label}`); - } - } - - // Grandfathering. A PR opened before the check existed reports its - // findings and passes, unless a maintainer has asked for enforcement. - const enforced = labels.includes(ENFORCE_LABEL); - const predating = createdAt !== "" && createdAt < CHECK_INTRODUCED && !enforced; - - // A bot or a draft is never enforced, and unlike grandfathering the - // `enforce-checklist` label does not override it: labeling a draft - // would nag an author who has already said they are not finished. - const exempt = isBot || isDraft; - - const failing = missing.length > 0 && !predating && !exempt; - const outcome = failing ? "fail" : "pass"; - - // Why it passed, not just that it did. The companion posts the comment - // and cannot see any of this, so without a reason it would tell the - // author of a draft or a grandfathered PR "checklist complete" while - // items are still unmet — the one message that must never be wrong. - const reason = missing.length === 0 ? "complete" - : isBot ? "bot" - : isDraft ? "draft" - : predating ? "predating" - : "incomplete"; - - fs.mkdirSync("checklist-result", { recursive: true }); - fs.writeFileSync("checklist-result/pr-number", String(number) + "\n"); - fs.writeFileSync("checklist-result/outcome", outcome + "\n"); - fs.writeFileSync("checklist-result/reason", reason + "\n"); - fs.writeFileSync("checklist-result/missing.md", missing.join("\n") + "\n"); - - // Step summary, so a predating PR's findings are visible somewhere even - // though nothing is labeled or commented on. - const summary = []; - summary.push("## PR checklist"); - summary.push(""); - summary.push(isToolPr - ? "This PR touches a tool directory, so every required item applies." - : "This PR touches no tool directory, so only the items that apply to every contribution are enforced (internal content, licence)."); - if (waived.length) { - summary.push("", "Marked not applicable, with a reason:", "", ...waived); - } - if (missing.length) { - let why = "Unmet required items:"; - if (isBot) why = "Opened by a bot, so the checklist is not enforced. Reported only:"; - else if (isDraft) why = "Still a draft, so the checklist is not enforced. It is enforced when you mark the PR ready for review. Reported only:"; - else if (predating) why = `Opened before this check existed (${createdAt}), so these are warnings and the check passes. Apply the \`${ENFORCE_LABEL}\` label to require them.`; - summary.push("", why, "", ...missing); - } else { - summary.push("", "All applicable required items are met."); - } - await core.summary.addRaw(summary.join("\n")).write(); - - if (exempt && missing.length) { - core.info(`PR #${number} is ${isBot ? "bot-authored" : "a draft"}; ${missing.length} item(s) reported, not enforced.`); - } else if (predating && missing.length) { - for (const m of missing) core.warning(m.replace(/^- /, "")); - core.info(`PR #${number} predates this check; ${missing.length} item(s) reported as warnings.`); - } else if (failing) { - core.setFailed(`Checklist incomplete: ${missing.length} required item(s) unmet.`); - } else { - core.info("All applicable required checklist items are met."); - } + 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 when the checklist is incomplete - if: steps.check.outcome != 'success' + - name: Fail the job unless the checklist passed + if: steps.verdict.outputs.outcome != 'pass' + env: + OUTCOME: ${{ steps.verdict.outputs.outcome }} run: | - echo "::error::PR checklist incomplete — see the comment on the PR." + 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 71569a09..7f7eedb5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -266,8 +266,7 @@ Test relevant scenarios with and without the custom agent, multiple times. Focus **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, and the template -calls it required rather than optional for that reason. +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 @@ -319,9 +318,11 @@ 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. -This item cannot be waived with the `n/a:` escape described under "The Pull -Request Checklist". There is no tool contribution for which domain correctness is -genuinely not applicable. +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 @@ -443,7 +444,7 @@ NOTE: a red check does not block merging on its own. An admin must add the job's 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 listing what is +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. @@ -458,7 +459,7 @@ 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, wherever they sit: no internal-only content, and the licence confirmation | +| 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 | @@ -490,9 +491,8 @@ 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. -Three items can never be waived this way: +Two items can never be waived this way: -- the **domain SME sign-off**, for the reason given in its own section above - **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 @@ -531,10 +531,14 @@ Three cases report their findings in the check's step summary and pass: 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 carries the same list. +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, and labelled. Ticking a box edits the description, so the check +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 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()