diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 71ecca07..7d611308 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -16,7 +16,7 @@ Use this only when the PR should auto-close an issue: ## Quality Bar Checklist ✅ -**All items must be checked before merging.** +**All applicable items must be checked before merging.** - [ ] **Standards**: I have read `docs/contributors/quality-bar.md` and `docs/contributors/security-guardrails.md`. - [ ] **Metadata**: The `SKILL.md` frontmatter is valid (checked with `npm run validate`). @@ -25,6 +25,7 @@ Use this only when the PR should auto-close an issue: - [ ] **Security**: If this is an _offensive_ skill, I included the "Authorized Use Only" disclaimer. - [ ] **Safety scan**: If this PR adds or modifies `SKILL.md` command guidance, remote/network examples, or token-like strings, I ran `npm run security:docs` (or equivalent hardening check) and addressed any findings. - [ ] **Automated Skill Review**: If this PR changes `SKILL.md`, I checked the `skill-review` GitHub Actions result and addressed any actionable feedback. +- [ ] **Manual Logic Review**: If this PR changes `SKILL.md` or risky guidance, I manually reviewed the logic, safety, failure modes, and `risk:` label instead of relying on automated checks alone. - [ ] **Local Test**: I have verified the skill works locally. - [ ] **Repo Checks**: I ran `npm run validate:references` if my change affected docs, workflows, or infrastructure. - [ ] **Source-Only PR**: I did not manually include generated registry artifacts (`CATALOG.md`, `skills_index.json`, `data/*.json`) in this PR. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f03fa3c4..b3ba9048 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -36,6 +36,8 @@ Open the PR with the default template and enable **Allow edits from maintainers* If your PR adds or edits `SKILL.md`, GitHub will also run the automated `skill-review` workflow on the pull request. Community PRs should stay **source-only**: do not include generated registry artifacts such as `CATALOG.md`, `skills_index.json`, or `data/*.json`. +Automated validation is necessary, but it does **not** replace manual logic review. If your PR adds or changes a skill, or introduces command, network, credential, mutation, install, or security guidance, review the logic and failure modes manually even when every automated check passes. + If you only want to improve docs, editing directly in GitHub is still perfectly fine. --- @@ -237,6 +239,13 @@ npm run validate GitHub will also run the automated `skill-review` check for PRs that touch `SKILL.md`. +Passing `npm run validate` or `skill-review` is not enough on its own for skill changes. Before you open the PR, manually review the skill for: + +- trigger clarity and whether the skill would fire in the right situations, +- correctness of the instructions and examples, +- obvious failure modes, unsafe assumptions, and user-facing edge cases, +- whether the declared `risk:` level still matches the actual behavior. + For **docs / workflows / infra changes**: ```bash @@ -451,6 +460,7 @@ Before submitting your contribution: - [ ] I've included examples - [ ] I've tested the skill with an AI assistant - [ ] I've run `npm run validate` +- [ ] If I changed `SKILL.md` or risky guidance, I manually reviewed the logic, safety, and likely failure modes instead of relying on automated checks alone - [ ] I've run `npm run validate:references` and `npm test` when my change affects docs, workflows, or infrastructure - [ ] I ran the docs security scan (`npm run security:docs`) for any skill containing commands, network access, credentials, or destructive guidance - [ ] I did **not** include generated registry artifacts (`CATALOG.md`, `skills_index.json`, `data/*.json`) in this PR diff --git a/docs/contributors/quality-bar.md b/docs/contributors/quality-bar.md index b9072ad0..3505548e 100644 --- a/docs/contributors/quality-bar.md +++ b/docs/contributors/quality-bar.md @@ -47,6 +47,7 @@ A list of known edge cases or things the skill _cannot_ do. If a skill includes command examples, remote fetch steps, secrets, or mutation guidance, the PR must document the risk and pass `npm run security:docs` in addition to normal validation. For pull requests that add or modify `SKILL.md`, GitHub also runs the automated `skill-review` workflow. Treat that review as part of the normal PR quality gate and address any actionable findings before merge. +Automated checks are necessary, but they do **not** replace manual reviewer judgment on logic, safety, and likely failure modes. `npm run security:docs` enforces a repo-wide scan for: @@ -95,5 +96,6 @@ Notes: - `npm run audit:skills` is the maintainer-facing compliance/usability report for the full library. - `npm run security:docs` is required for command-heavy or risky skill content. - PRs that touch `SKILL.md` also get an automated `skill-review` GitHub Actions check. +- Skill changes and risky guidance still require a manual logic review before merge, even when the automated gates pass. - `npm run validate:strict` is a useful hardening pass, but the repository still contains legacy skills that do not yet satisfy strict validation. - Examples and limitations remain part of the quality bar even when they are not fully auto-enforced by the current validator.