docs(contributing): Require manual logic review

Clarify that validate and automated skill-review are necessary but not sufficient for skill and risky guidance changes. Add the requirement consistently to contributing guidance, the quality bar, and the PR checklist so maintainers explicitly review logic, safety, failure modes, and risk labeling before merge.
This commit is contained in:
sickn33
2026-03-29 10:27:16 +02:00
parent 9e1e9c97a1
commit eb3df2a577
3 changed files with 14 additions and 1 deletions

View File

@@ -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.

View File

@@ -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

View File

@@ -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.