skill(review): add architectural and security review skills#766
Conversation
- Archive: 71 decisions (>30d), 10 (7-30d) - Merge: 13 inbox decisions into canonical log - Summarize: Flight, EECOM, FIDO histories (kept recent entries) - Log: Bug-fix-pipeline session recorded - Deduplicate: 73 unique decisions - decisions.md: 385.8 KB -> 103.7 KB Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add two new PR review skills to .copilot/skills/: - architectural-review: module boundaries, dependency direction, export surface safety - security-review: credentials, injection, workflow permissions, supply chain Link from reviewer-protocol and copilot-instructions.md so Copilot references them on every PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds two new domain-specific PR review skills (architecture + security) and links them from the main reviewer guidance so reviewers consistently consult the relevant checklists.
Changes:
- Added
.copilot/skills/architectural-review/SKILL.mdwith architectural review invariants (module boundaries, bootstrap constraints, export surface, refactor safety, template sync, etc.). - Added
.copilot/skills/security-review/SKILL.mdwith a security review checklist (secrets/PII, injection risks, Actions security, supply chain, git safety). - Linked both skills from
.copilot/skills/reviewer-protocol/SKILL.mdand.github/copilot-instructions.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
.github/copilot-instructions.md |
Adds a “PR Review Skills” section pointing reviewers/authors to the new checklists. |
.copilot/skills/architectural-review/SKILL.md |
Introduces an architectural PR review checklist and examples (needs template-sync corrections). |
.copilot/skills/security-review/SKILL.md |
Introduces a security PR review checklist and structured output format. |
.copilot/skills/reviewer-protocol/SKILL.md |
Adds “Related Review Skills” links to the two new skills. |
| Template files exist in **four locations** and must stay consistent: | ||
|
|
||
| ``` | ||
| templates/ # Source of truth | ||
| .squad-templates/ # Local project templates | ||
| packages/squad-cli/templates/ # CLI-bundled templates | ||
| .github/workflows/ # Workflow templates | ||
| ``` |
There was a problem hiding this comment.
The “Template Sync Awareness” section documents the wrong source-of-truth and sync targets. In this repo the canonical templates live in .squad-templates/, with mirror targets templates/, packages/squad-cli/templates/, and packages/squad-sdk/templates/ plus a special target .github/agents/ for squad.agent.md (see test/template-sync.test.ts:4-6). This section currently claims templates/ is the source of truth, omits packages/squad-sdk/templates/, and lists .github/workflows/ as a template location (which is not part of the sync contract). Update the checklist/list to match the tested template sync contract so reviewers don’t enforce incorrect invariants.
| PR modifies templates/squad.agent.md but not .squad-templates/squad.agent.md | ||
|
|
||
| Finding: APPROVE with note — Template was updated in templates/ but the | ||
| same file in .squad-templates/ was not synced. Verify whether .squad-templates/ | ||
| should match or if it intentionally diverges. |
There was a problem hiding this comment.
Example 3 references templates/squad.agent.md, but the repo’s mirror target uses templates/squad.agent.md.template (the .template suffix is intentional to prevent CLI discovery), and .github/agents/squad.agent.md is the special synced copy (see test/template-sync.test.ts:86-95 and test/template-sync.test.ts:119-135). Updating the example paths will keep the skill consistent with actual file names and the sync script/test behavior.
| PR modifies templates/squad.agent.md but not .squad-templates/squad.agent.md | |
| Finding: APPROVE with note — Template was updated in templates/ but the | |
| same file in .squad-templates/ was not synced. Verify whether .squad-templates/ | |
| should match or if it intentionally diverges. | |
| PR modifies templates/squad.agent.md.template but not .github/agents/squad.agent.md | |
| Finding: APPROVE with note — Template was updated in templates/ but the | |
| synced copy in .github/agents/ was not updated. Verify whether the change | |
| should be mirrored or if this file intentionally diverges. |
|
Love this. Looked at how microsoft/aspire handles arch/security review - they rely on PR template checkboxes and CODEOWNERS. Your approach of codifying it as Copilot skills that get read on every PR is genuinely more sophisticated. The reviewer-protocol linking is smart - makes sure these don't get orphaned. LGTM, approving. Should merge after #762. |
tamirdresher
left a comment
There was a problem hiding this comment.
Approved. Merge after #762.
* [Scribe] Post-pipeline memory & decision consolidation - Archive: 71 decisions (>30d), 10 (7-30d) - Merge: 13 inbox decisions into canonical log - Summarize: Flight, EECOM, FIDO histories (kept recent entries) - Log: Bug-fix-pipeline session recorded - Deduplicate: 73 unique decisions - decisions.md: 385.8 KB -> 103.7 KB Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * skill(review): add architectural and security review skills Add two new PR review skills to .copilot/skills/: - architectural-review: module boundaries, dependency direction, export surface safety - security-review: credentials, injection, workflow permissions, supply chain Link from reviewer-protocol and copilot-instructions.md so Copilot references them on every PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chore: remove .squad file leakage Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add two new PR review skills to
.copilot/skills/:Also links both skills from:
.copilot/skills/reviewer-protocol/SKILL.md(new Related Review Skills section).github/copilot-instructions.md(new PR Review Skills section after PR Guidelines)This ensures Copilot reads the relevant security and architecture checklists on every PR.