diff --git a/.github/skills/pr-review/SKILL.md b/.github/skills/pr-review/SKILL.md new file mode 100644 index 00000000000..061c32825b0 --- /dev/null +++ b/.github/skills/pr-review/SKILL.md @@ -0,0 +1,203 @@ +--- +name: pr-review +description: >- + This skill should be used when the user asks to "review a PR", + "check this pull request", "PR review", "review pull request", + "check PR against checklist", or needs to review a pull request + against the project's PR checklist and coding standards. +--- + +# PR Review + +Review pull requests against the project's PR checklist and coding standards. Produce a numbered list of findings for the user, then post inline review comments and a summary comment on the PR when approved. + +## Prerequisites + +- `gh` CLI authenticated with access to the repository. + +## Workflow + +### Phase 1: Gather PR Context + +1. If no PR number or URL was provided, prompt the user for it. Do not proceed without one. +2. Extract the PR number. Accept formats: full URL, `#123`, or plain number. +3. Run the following to collect PR metadata and diff: + +```bash +gh pr view --json number,title,body,author,files,additions,deletions,baseRefName,headRefName +gh pr diff +``` + +4. Identify the type of change (new command, bug fix, documentation, refactor, etc.) from the PR title, body, and changed files. This determines which checklist items apply. + +### Phase 2: Review + +Read `references/pr-checklist.md` for the full checklist. + +Review the diff against **all three categories** below. Not every checklist item applies to every PR — evaluate applicability based on the change type identified in Phase 1. + +#### A. Checklist Compliance + +Walk through each applicable item in the checklist and verify it against the diff. Key items to check: + +- **Single quotes** for strings in command files (not double quotes). For test files, this is less strictly enforced. +- **Command class naming**: `[Service][Command]Command` pattern. +- **Command name in `commands.ts`**: Must be placed in alphabetical order. +- **`force` option** on remove commands. +- **`async/await`** instead of `promise/then`. +- **`handleRejectedODataJsonPromise`** when `responseType: 'json'` is used. +- **`defaultProperties`** in list commands instead of conditional JSON output. +- **Custom Zod validation** for mutually exclusive options. +- **Verbose logging**: Every command should include at least one verbose log message that provides useful execution context. +- **User input escaped** in XML and URLs using `formatting.encodeQueryParameter()` or `formatting.escapeXml()`. +- **`GetFileByServerRelativePath`/`GetFolderByServerRelativePath`** in `spo` commands (not `...Url` variants). +- **No `any` types**, no commented-out code. +- **Bug fixes** include a test for the fixed case. +- **Documentation** included where needed. + +#### B. Code Quality + +Review the implementation for: + +- Logic errors, off-by-one mistakes, unhandled edge cases. +- Security issues (injection, unvalidated input, leaked credentials). +- Performance concerns (unnecessary API calls, missing pagination). +- TypeScript best practices (proper typing, no implicit any). + +#### C. Test Quality + +Check test files (`.spec.ts`) for: + +- Coverage of happy path and error cases. +- Proper use of mocks and assertions. +- Tests that actually verify behavior (not just that code runs). +- Tests for command name, description not being `null`, and schema validation. +- Use `sinon.restore()` in `after()` to reset stubs/spies. + +#### D. Documentation Quality + +Check documentation files (`.mdx`) for: + +- Every command needs a reference page at `docs/docs/cmd//.mdx`. +- The `.mdx` file name matches the command file name. +- Must include: title, description, usage, options table, examples, permissions, and response. A remarks section can be used for additional information, but is not mandatory. +- When a command has a response, include sample output for all four output formats: JSON, Markdown, text, and CSV. +- Include at least 2 examples for the examples section, covering different option combinations. +- Import and use `` for standard CLI options. +- Examples should use `m365` prefix and long option names (not short aliases). +- Document the minimum required permissions that allow success with any option combination. +- Docs must build without warnings. +- When importing components in the docs, use absolute imports from `@site/src/components/` instead of relative paths. +- When importing global options in the docs, use a relative import from `../_global.mdx`. + +### Phase 3: Present Findings + +Compile all findings into a **numbered list** and present it to the user in chat. Each finding must include: + +1. **File and line reference** (linked). +2. **Category**: Checklist | Code Quality | Test Quality. +3. **Severity**: Error (must fix) | Warning (should fix) | Suggestion (nice to have). +4. **Description**: What the issue is and why it matters. +5. **Suggestion**: What to change (be specific). + +If there are **zero findings**, state that the PR looks good. + +Sort findings: Errors first, then Warnings, then Suggestions. + +### Phase 4: Post Comments + +Always ask the user for confirmation before posting any comments on the PR. Present the planned comments and wait for explicit approval. Then: + +1. **Submit a review** with inline comments and the appropriate verdict: + - **Zero findings** → `APPROVE` + - **Any findings** (error, warning, or suggestion) → `REQUEST_CHANGES` + +Post inline comments and the summary as a single review submission. Always target the actual line where the issue exists. + +**API limitation**: The GitHub API can only target lines within diff hunk ranges (changed lines + surrounding context lines). If the target line falls in a gap between hunks, do **not** post it on a nearby line. Instead, include that finding in the summary (the review body) with the file path and line number. Only use `suggestion` blocks for findings posted on the exact target line. + +**Line number accuracy**: When posting inline comments, verify line numbers against the actual diff hunk content. The line number refers to the **new file** line number (RIGHT side). Count lines in the diff hunk carefully — off-by-one errors cause comments to land on the wrong line and suggestion blocks to replace the wrong code. + +```bash +gh api repos/{owner}/{repo}/pulls/{number}/reviews \ + --method POST \ + --input - <<'EOF' +{ + "event": "APPROVE or REQUEST_CHANGES", + "body": "", + "comments": [ + { + "path": "", + "line": , + "side": "RIGHT", + "body": "" + } + ] +} +EOF +``` + +The `comments` array can contain multiple entries. Omit it entirely when approving with zero findings. + +#### Comment Tone Guidelines + +These comments appear on a public repository and represent the user responding to a contributor's work. Every comment must be: + +- **Constructive**: Frame issues as suggestions, not demands. Use "Consider..." or "It might be worth..." instead of "You must..." or "This is wrong." +- **Specific**: Reference exact lines, show the expected code, explain *why* a change matters. +- **Appreciative**: Acknowledge good work. If the PR is mostly solid, say so. Contributors volunteer their time. +- **Brief**: One or two sentences per inline comment. Save detail for the summary. +- **Professional**: No sarcasm, no passive-aggressive phrasing, no exclamation marks on criticism. + +For inline comments, use this format: +``` +[Category | Severity] Description. + +Suggestion: `` +``` + +When confident a specific code change is correct, include a GitHub suggestion block to make it easy for the contributor to apply: + +```` +```suggestion + +``` +```` + +Only use suggestion blocks when the fix is unambiguous and self-contained. Do not guess. + +For the summary comment, use this format: +```markdown +## PR Review Summary + +**PR**: # - +**Author**: @<author> + +### Overview +<1-2 sentence assessment> + +### Findings +<numbered list of findings with severity> + +### Checklist Coverage +<brief note on which checklist categories were verified> + +--- +*Reviewed against the [PR checklist](docs/docs/contribute/pr-checklist.mdx).* +``` + +If the PR has zero issues, the summary should simply acknowledge the quality of the work and approve. + +## Additional Resources + +### Reference Files + +- **`references/pr-checklist.md`** — Full PR checklist with all items organized by category (General, Coding Standards, Documentation). + +### Reference Commands + +When providing feedback, point contributors to well-implemented commands as examples. Use these as reference implementations: + +- **`src/m365/spo/commands/list/list-list.ts`** — Example of a list command with `defaultProperties` and proper text output. +- **`src/m365/spo/commands/web/web-get.ts`** — Example of a get command with verbose logging and proper error handling. +- **`src/m365/entra/commands/user/user-get.ts`** — Example of a command using Zod schema validation. diff --git a/.github/skills/pr-review/references/pr-checklist.md b/.github/skills/pr-review/references/pr-checklist.md new file mode 100644 index 00000000000..5aaa51c7375 --- /dev/null +++ b/.github/skills/pr-review/references/pr-checklist.md @@ -0,0 +1,42 @@ +# PR Checklist + +## General guidelines + +- Ensure the build passes. +- No ESLint errors. +- Achieve 100% code coverage. +- The submission should match the specification. +- Maintain a single commit, or squash multiple commits. +- Use single quotes `' '` for strings in command files. For test files this is less strictly enforced, since responses copied from API requests often contain double quotes. +- If submitting a sample, ensure it is properly formatted and indented. + +## Coding standards + +- Command options should follow the naming convention (kebab-case for CLI flags). +- The command should have a correct name. +- The command name added to `commands.ts` should be placed so that commands are sorted alphabetically. +- The command class is named following the pattern `[Service][Command]Command`. For example, `SpoWebRemoveCommand`. +- Verify the command works as expected. +- List commands must have readable output in `text` mode, with each item fitting in one row of 130 characters preferably. +- Avoid commented-out code and usage of `any` types, preferring specific types. +- Remove commands should include a `force` option. +- For bug fixes, include a test for the fixed use case. +- Avoid unnecessary retrieval of form digest. +- Handle failed promises properly when `responseType: 'json'` is used by using `handleRejectedODataJsonPromise`. +- Escape user input in XML and URLs using `formatting.encodeQueryParameter()` or `formatting.escapeXml()`. +- Verbose and debug outputs are logged to stdErr (`logger.logToStderr` instead of `logger.log`). +- Do not do conditional output in JSON output mode; use `defaultProperties` for defining default properties in list commands. +- For commands with multiple options where the user is required to choose one, define these options using a custom Zod validation. +- Use `async/await` instead of `promise/then`. +- When working with `spo` commands, use `GetFileByServerRelativePath` and `GetFolderByServerRelativePath` API endpoint instead of `GetFileByServerRelativeUrl` and `GetFolderByServerRelativeUrl`. +- `npm test` must pass without errors. + +## Documentation + +- Include an `mdx` help page. +- Reference the `mdx` help page in the sidebar navigation. +- Start all code samples with `m365`. +- Ensure samples use long names of options rather than short ones. +- Include the marker to incorporate global options rather than listing them explicitly. +- If there is an option modifying the output, include responses for both default and modified output. +- For a command page, ensure it includes the following sections: title, description, usage, options, examples, permissions, and response. In some cases a remarks section is also allowed.