Add address-review and code-review skills#806
Add address-review and code-review skills#806jerrysxie wants to merge 1 commit intoOpenDevicePartnership:mainfrom
Conversation
Add two Copilot skills for PR workflows: - address-review: evaluates review comments for validity before applying fixes, with a table-based workflow for developer confirmation - code-review: produces structured PR reviews with summary, step-by-step guide, and potential issues table for human reviewers Both skills are adapted for the embedded-services repository with no_std constraints, Embassy async patterns, and the Service trait.
jerrysxie
requested review from
RobertZ2011,
Copilot,
felipebalbi,
tullom and
williampMSFT
There was a problem hiding this comment.
Pull request overview
Adds two GitHub Copilot “skills” intended to standardize PR workflows in this repository: one for generating structured PR code reviews, and another for validating review comments before applying fixes (with a confirmation step).
Changes:
- Introduce a
code-reviewskill that outputs a summary, guided review steps, and a potential-issues table tailored to embedded-services constraints. - Introduce an
address-reviewskill that evaluates review comments for validity via a table-driven workflow, requests developer confirmation, then applies confirmed fixes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/skills/code-review/SKILL.md | Adds instructions for producing a structured, architecture-focused PR review output. |
| .github/skills/address-review/SKILL.md | Adds a workflow for validating review comments before making code changes, with explicit confirmation. |
|
|
||
| ## Review rules | ||
|
|
||
| - Do NOT flag formatting, style, or compilation errors — `cargo fmt`, `cargo clippy`, and CI handle those. |
There was a problem hiding this comment.
The rule "Do NOT flag ... compilation errors" is likely counterproductive/inconsistent with the rest of this skill: compilation failures (especially from incorrect #[cfg(...)] gating or cross-target builds) are important for reviewers to surface even if CI will eventually catch them. Consider narrowing this to formatting/style only, or explicitly allowing compile/build failures that affect targets/features used in this repo (thumb + host, feature combos).
| - Do NOT flag formatting, style, or compilation errors — `cargo fmt`, `cargo clippy`, and CI handle those. | |
| - Do NOT flag formatting or style issues, and do not spend review comments on purely mechanical compiler diagnostics that `cargo fmt`, `cargo clippy`, or CI will report directly. | |
| - DO flag compilation/build failures when they indicate a meaningful review issue, especially incorrect `#[cfg(...)]` gating, broken host or `thumbv8m.main-none-eabihf` builds, invalid feature combinations, or `no_std` violations. |
| - Consider the crate's `no_std` context, strict clippy policy, and feature-gated code when evaluating suggestions. | ||
| - If a reviewer suggests using `unwrap()`, `panic!()`, or direct indexing, flag it as invalid — these are denied by clippy config. | ||
| - If a comment is about style or formatting, mark it as invalid — CI enforces these automatically. | ||
| - Create new fixup commits for each change — do NOT amend existing commits. The author will squash them before merge. |
There was a problem hiding this comment.
This guideline mandates a specific git workflow ("Create new fixup commits... The author will squash them") but the repo's CONTRIBUTING.md does not specify fixup-commit or squash requirements. To avoid encoding an unconfirmed process, consider removing this line or rephrasing it as optional / "follow the project's or PR author's preferred workflow".
| - Create new fixup commits for each change — do NOT amend existing commits. The author will squash them before merge. | |
| - Follow the project's documented git workflow, or the PR author's preferred workflow, when preparing commits for confirmed changes. |
Add two Copilot skills for PR workflows:
Both skills are adapted for the embedded-services repository with no_std constraints, Embassy async patterns, and the Service trait.