[ams-thakkar]

Description

Authors open PRs without completing the required items in the PR template, and nothing catches it, so a PR sits waiting for a review that won't start. This adds a check that reads the PR description, works out which items apply from the files changed, and puts PRs with unmet items on hold with a label and a comment. It also documents the two things the template asks for that CONTRIBUTING.md never explained.

File	What it does
.github/scripts/check_pr_checklist.py	The rules: which sections apply, the n/a: <reason> escape, which PRs are reported but not enforced. Writes outcome (pass/fail/error) and reason; the unmet items go to the step summary only
tests/test_check_pr_checklist.py	14 tests, including ones that pin every section name and universal item against PULL_REQUEST_TEMPLATE.md
.github/workflows/validate-pr-checklist.yml	Runs the script and publishes the verdict. Anything other than a verdict the script wrote itself is error, reported as "did not complete", never as the PR's fault
.github/workflows/label-pr-checklist-result.yml	workflow_run with write access. Applies or removes needs-checklist and posts or updates one fixed comment that links to the run's summary
CONTRIBUTING.md → Domain SME Sign-Off	Who qualifies, how to record it, what to do when you can't find one, the n/a: exception for edits that change no technical claim, and how it differs from the approving review main requires
CONTRIBUTING.md → The Pull Request Checklist	Which items apply to which PR, the n/a: escape and the two items it can't waive, both labels, and the three cases that aren't enforced
.github/PULL_REQUEST_TEMPLATE.md	The SME item names the n/a: exception, so the template and the check agree

It's split into two workflows because GITHUB_TOKEN is read-only for pull_request events from a fork, and most contributions here come from forks, so the validating job can't label or comment on its own PR. It's the same pattern as validate-skill-evals / label-skill-evals-result.

Type of change

• New skill
• New custom agent
• New MCP server
• Update to an existing skill, agent, or MCP server
• Documentation or infrastructure change

Required before review

• Repository layout is correct — n/a: no tool directory is touched; this changes two workflows, a script and its tests, the PR template and CONTRIBUTING.md.
• A domain SME has reviewed and signed off. — n/a: no tool domain content; the judgement here is about repository process, which is a maintainer call.
• Evaluation evidence is included (see Testing). — n/a: no skill. Behavioural evidence is under Testing.
• No internal-only or proprietary content — checked by hand and by scan_aws_identifiers.py (0 findings on 806 added lines).
• README is complete — n/a: no new tool. The behaviour is documented in CONTRIBUTING.md instead.
• CHANGELOG updated and the version bumped. — n/a: no versioned tool is modified.
• Named owner for ongoing maintenance — @ams-thakkar.

Testing

• python3 -I -m unittest discover -s tests: 202 tests on the branch, 14 of them new; 224 after a clean trial merge with current main.
• The template tests fail when a section is renamed in the template, and when the template's own prose reads as an n/a: waiver. The second caught a real mistake: an early wording of the SME line would have waived it for every author.
• Parity with the first version. The nine post-cutoff PRs open on 2026-10-22 (docs(aws-routing): TGW one-path-per-DXGW + same-ASN for cross-DXGW ECMP #137, feat(aws-waf-log-analysis): add AWS WAF log analysis skill #139, feat: add GPU diagnostics MCP for node-level NVIDIA triage #140, Add serverless-operation-review skill and custom agent #143, chore(deps-dev): bump handlebars from 4.7.8 to 4.7.10 in /mcp/aws-eks-node-diagnostics-mcp #144, Add aws-natgw-traffic-analysis skill #145, feat(custom-agents): add confluence-memory-sync custom agent #147, Add Coralogix DevOps Agent connector #150, IaC-change-cost-preview Skill for Release Readiness Review #155) give the same outcome, reason and unmet items under the script as under the original inline JS, with one exception. On Add serverless-operation-review skill and custom agent #143 and Add aws-natgw-traffic-analysis skill #145 the old exact heading match reported a shortened ## Skills heading as a missing section, and prefix matching fixes that. 7 of the 9 fail; feat(aws-waf-log-analysis): add AWS WAF log analysis skill #139 (draft) and chore(deps-dev): bump handlebars from 4.7.8 to 4.7.10 in /mcp/aws-eks-node-diagnostics-mcp #144 (dependabot) pass and are reported only.
• The labeler, against a mocked API, for an honest fail, a forged PR number, a stale commit, error, a missing artifact, a forged reason, a pass paired with incomplete, a draft after a hold, and a deleted fork. Only the honest fail and the draft write anything. Neither comment contains any artifact text.
• validate_skills.py --base-ref origin/main: 34 skills, 0 errors. scan_aws_identifiers.py: 0 findings.

Not verified: neither workflow has run for real, since a workflow_run listener only fires once it's on main.

Rollout

CHECK_INTRODUCED is 2026-10-13T07:33:08Z, the moment commit 49f31c1 put the gated template on main via #135. That's deliberately earlier than this PR, so a PR opened in between is enforced rather than grandfathered: its author had the checklist in front of them.

enforce-checklist already exists. needs-checklist is created by the labeler on first use.

Follow-ups, not in this PR:

1. The check is advisory until an admin adds validate-pr-checklist to the required status checks for main.
2. Merging doesn't comment on PRs that are already open, because pull_request events only fire on new activity. Those need a deliberate one-off backfill.
3. The three workflow_run labelers key their concurrency groups differently. That'll be filed separately.

Security and safety

• Read-only by default: the validating workflow takes contents: read only. Write access lives in the labeler, which runs from the default branch and never executes PR content.
• Customer data stays in the customer's account: not applicable, no customer data.
• No destructive operations: the only mutations are adding or removing one label and creating or updating one comment.

The validating job runs a workflow definition the PR controls, so the labeler treats its artifact as untrusted. The PR is resolved from the workflow_run payload (head repository, branch and commit), and the artifact's number only has to name one of those matches. outcome and reason are checked against fixed lists, and the comment is fixed text. The bot never posts text taken from a PR description, and can't be pointed at another PR.

License confirmation

• By submitting this pull request, I confirm that my contribution is made under the terms of the Apache License 2.0.

[udid-aws]

Reviewed the full diff against the existing check pairs (validate-skill-evals/label-skill-evals-result, scan-aws-identifiers/label-aws-identifiers-result), the PR template, and CONTRIBUTING.

I did not execute either workflow — a workflow_run listener only fires once it's on main — and I couldn't re-run the 41-PR evidence, since the harness in project-nebula/checklist-harness/ isn't in the repo. So the behavioural claims below are from reading the scripts, not from running them.

The design is right and the write-up is careful. Scoping by changed files rather than the "Type of change" boxes is the correct call and the reasoning for it holds. The reason channel is a real fix — a companion that can only see pass/fail will eventually tell a draft author they're done, and catching that before merge rather than after is the difference between a good check and one people learn to ignore. Passing every payload value through env: is right. And declining to put an if: guard on the job, because a skipped required check counts as satisfied, is the detail these things usually get wrong.

Three things I'd want resolved before this merges.

1. The companion posts attacker-controlled text as the bot, to an attacker-chosen PR

For pull_request events the workflow definition comes from the PR's own ref, not from main. A fork PR can therefore rewrite validate-pr-checklist.yml to put any pr-number and any missing.md into the artifact. label-pr-checklist-result.yml runs from main with pull-requests: write, trusts both, and splices missing straight into a comment body:

prNumber = parseInt(fs.readFileSync("result/pr-number", "utf8").replace(/\D/g, ""), 10);
...
missing = fs.readFileSync("result/missing.md", "utf8").trim();

Both existing labelers avoid this on both counts — they sanitize (pr=$(tr -dc '0-9' < result/pr-number)) and they never echo artifact text; the only mutation is a fixed label.

I don't think the issue is self-passing. CONTRIBUTING already accepts that a PR runs its own copy of a check and could edit it to pass, with code-owner review as the control, and the existing labelers already accept an artifact-supplied PR number. The new exposure is that the bot can be made to post arbitrary markdown on an arbitrary PR in the repo — a phishing and denial-of-review surface rather than a way past this check. Run approval doesn't cover it either: it's required for first-time contributors only.

Two cheap fixes:

• Resolve prNumber from the workflow_run payload (head_repository.full_name + head_branch via pulls.list) and use the artifact's number only as a cross-check.
• Render the comment from a fixed template, bounding missing to a line and character cap and stripping HTML comments and link syntax before embedding it.

2. A non-waivable SME sign-off on every PR that touches skills/

NO_WAIVER includes the SME item, and any path under skills/ sets isToolPr, so the full tool checklist applies. A typo fix in one skill README then needs a named domain expert other than the author, with no n/a: escape.

Not hypothetical: a961f30 (merged as #154) rewrote README/CHANGELOG/SKILL.md across ~40 skills to correct agent-type names. Under this check it would have been held on an SME sign-off, as would #153. "Five of the seven are stuck on the SME sign-off" reads as confirmation the gate works; it reads equally as evidence it's too blunt for edits that change no technical claim.

Worth either scoping the SME requirement to PRs that change substantive content (SKILL.md body, SYSTEM_PROMPT.md, MCP server source) rather than any path under a tool directory, or allowing n/a: on it and letting the reviewer judge the stated reason. "There is no tool contribution for which domain correctness is genuinely not applicable" is true of a new skill and not of a label rename.

3. Two-state outcome, where the sibling check established three

scan-aws-identifiers.yml writes pass, fail, or error, and its labeler handles error by leaving the label alone — needs-id-redact being "a claim nobody is in a position to make here." CONTRIBUTING states the principle: never report a could-not-run as the PR's fault.

This check has no error state. If the script throws before its writes — github.paginate(github.rest.pulls.listFiles) is the candidate — checklist-result/ never exists, the upload warns, the companion returns "nothing to label", and the final step still prints:

::error::PR checklist incomplete — see the comment on the PR.

A red required check, a message blaming the author, and no comment to read. Wrap the script body in try/catch, write outcome=error, fail the job with a message saying the check didn't complete, and have the companion leave both label and comment untouched on error.

Smaller points

No tests, and the evidence isn't reproducible. 306 lines of inline JS with no in-repo test. The convention here is the opposite: rules as pure functions in .github/scripts/, unit tests in tests/, conformance cases, and "a rule change needs a matching case change in the same PR." Keeping the harness out of tree means no reviewer can verify the 41-PR result and nobody can safely edit this in six months. Extracting to .github/scripts/check_pr_checklist.py with tests would mirror validate_skill_evals.py.

Asymmetric matching. Items are matched on stable phrases, with a comment explaining that rewording the template mustn't silently drop one. Headings are matched as exact strings, em dash included — reword a template heading and every tool PR fails with "section is required … but is missing." Match headings on a prefix too, or add a test pinning the template's headings against the validator's list.

Concurrency inconsistency left in place. The companion's comment correctly explains that keying on workflow_run.id makes the group unique and the setting a no-op — which is what label-skill-evals-result.yml and label-aws-identifiers-result.yml both still do. Three labelers, two patterns, and an in-tree comment calling one of them wrong. Fix them here or file it.

Missing unlabeled trigger. labeled is there so enforce-checklist takes effect at once; removing it doesn't re-run, so a grandfathered PR stays failing until the next push.

CONTRIBUTING overstates the universal items. The table says they're enforced "wherever they sit", but the loop only scans the required sections — an item moved out of "Required before review" isn't seen.

Unaddressed scope question. A PR editing connectors/*/connector.yaml changes which skills ship but touches no TOOL_PREFIXES path, so it gets the two-item treatment. validate_skills.py already treats that file as skill-affecting. Worth a deliberate decision either way.

Everything under "Rollout" — advisory until branch protection is updated, enforce-checklist needing creation, the open PRs needing a deliberate backfill — is accurate, and I'd leave those as follow-ups.

[ams-thakkar]

Thanks. All three were right. Reworked in 2ce9947.

1. Artifact text posted to an arbitrary PR. The labeler now trusts nothing from the artifact beyond a fixed list.

• The PR is resolved from the workflow_run payload: an open PR whose head is the run's head repository, branch and commit. The artifact's number only has to name one of those matches, so it can't redirect the label or comment. A run whose commit is no longer the head matches nothing, which also stops a stale run from overwriting a newer one.
• The comment is fixed text keyed by outcome and reason, both allowlisted. It links to the run's summary for the unmet items instead of repeating them. missing.md is gone, so the bot never posts PR-body text.

I didn't go with sanitizing, because bare URLs and @mentions get through markdown stripping. Not echoing anything was simpler than filtering it.

2. SME on every skills/ PR. I went with n/a:. The item can now be waived with a reason for edits that change no technical claim, and the reviewer judges the reason. Scoping by substantive content would mean parsing the diff (#152 only changed frontmatter, and #154 touched SKILL.md), so I didn't try. The template line and CONTRIBUTING now agree.

3. No error state. There are three outcomes now. Anything other than a verdict the script wrote itself (a failed fetch, a crash, a script that never ran) publishes error. The job then fails with "did not complete, nothing was concluded", and the labeler leaves the label and comment alone.

Smaller points

• Tests: the rules moved to .github/scripts/check_pr_checklist.py, with 14 tests in tests/test_check_pr_checklist.py, which validate-skills runs. I re-ran the nine harness fixtures against the port. Outcome, reason and items all match the old JS except on Add serverless-operation-review skill and custom agent #143 and Add aws-natgw-traffic-analysis skill #145, where the old exact match reported a shortened ## Skills heading as a missing section.
• Headings: matched by prefix, and a test pins every section name and both universal items against PULL_REQUEST_TEMPLATE.md.
• unlabeled: added.
• "wherever they sit": corrected to name the two sections.
• connectors/: out of scope for now. Submissions aren't open and the template has no connector section to enforce. The script has a comment saying so.
• Concurrency across the three labelers: not changed here. I'll file it separately.

What I verified: 202 tests on the branch, and 224 after a clean trial merge with current main. I ran the companion against a mocked API for an honest fail, a forged PR number, a stale commit, error, a missing artifact, a forged reason, a draft after a hold, and a deleted fork. scan_aws_identifiers found nothing and validate_skills reported 0 errors.

Not verified: neither workflow has run for real, since workflow_run only fires from main. That's the same limit as before.

[udid-aws]

Approving. All three blockers are properly addressed, and a couple of the fixes are better than what I suggested.

What I verified on 2ce9947:

Artifact trust. The PR is resolved from the workflow_run payload, and the artifact's number only selects among open PRs whose head repository, branch and commit match the run — so a forged number can only ever name the author's own PR. The comment is fixed text keyed on an allowlisted outcome/reason, and missing.md is gone, so no PR-body text reaches the bot. Not sanitizing was the right call: bare URLs and @mentions do survive markdown stripping, so not echoing anything is the only version that holds. The head-sha check also gives you the stale-run protection the concurrency group couldn't, which I hadn't considered.

SME waiver. The template line, the new exception paragraph in the SME section, and the "Two items can never be waived this way" list all agree. I checked the last one specifically — that's where a stale "three" would have survived.

Error state. error covers a crash before the first write and a failed fetch as well as an exception, the job message separates "did not complete" from "incomplete", and the labeler leaves label and comment alone. Pinned by test_an_unreadable_event_is_an_error_not_a_finding.

Tests. 202 and green on the branch, and validate-skills does run them. test_every_section_is_a_template_heading asserting each name prefixes exactly one heading catches a reworded template and an ambiguous prefix in one assertion, which is stronger than what I asked for. test_untouched_template_waives_nothing is a good instinct — template prose reading as n/a: would have been a silent universal bypass.

None of the following blocks merge. Worth a follow-up PR:

1. The test suite leaks a GitHub annotation into validate-skills. test_an_unreadable_event_is_an_error_not_a_finding lets main()'s print(f"::error::...") reach stdout, and nothing in the test file captures it. Since validate-skills runs the suite, that line lands in its job log and GitHub renders a bare ::error:: as an error annotation — so every PR gets a red annotation on an otherwise green check. Wrapping the call in contextlib.redirect_stdout(io.StringIO()) fixes it and lets the test assert on the captured text, which is a better test than the current one. The ::warning:: lines on the predating path have the same shape, though no test reaches them yet.

2. find_section matches License confirmation, but CONTRIBUTING's prose says "licence" throughout, including where it names the item. Someone retyping the heading from the docs gets "section missing from the description; restore it from the template" — fail-loud and actionable, so low stakes, but the matcher and the prose disagree on spelling. Either accept both in the prefix or use the template's spelling when naming the section.

3. The labeler concurrency inconsistency. Deferring is fine; drop the issue link in this thread when you file it, so whoever next reads the workflow_run.id comment in label-skill-evals-result.yml can find the reasoning.

Two things I looked at and am not asking you to change. Moving to git diff base...HEAD means the job now depends on refs/pull/N/merge, which is stale or absent on a conflicted PR, so it can die at the checkout step — your error semantics handle that safely, and it's the same pattern validate-skill-evals.yml already uses, so it isn't new here. Worth knowing that a conflicted PR will report a git error rather than a verdict once this is required. And reason is still artifact-supplied, so a PR could write pass/draft and post the "not enforced while a draft" text on itself — that's the self-passing class CONTRIBUTING already accepts, now confined to the author's own PR.

Same caveat as before on my side: I read the scripts and ran the test suite, but neither workflow has run for real, and the 41-PR harness result still isn't reproducible from the repo. The in-repo tests cover the rules now, which is the part that matters for whoever edits this next.