mirror of
https://github.com/block/goose.git
synced 2026-07-17 12:56:20 +02:00
add shared agent skills for code-review and create-pr (#32)
Skills are stored in .agents/skills/ as the single source of truth, with symlinks from .cursor/, .claude/, and .codex/ so all three coding agents can discover them. Made-with: Cursor
This commit is contained in:
morgmart
@@ -0,0 +1,190 @@
|
||||
---
|
||||
name: code-review
|
||||
description: >-
|
||||
Senior engineer code review focused on catching issues before they become PR
|
||||
comments. Reviews only changed lines, categorizes issues by priority, and fixes
|
||||
them one by one. Use when the user says "code review", "review my code",
|
||||
"review this branch", or wants pre-PR feedback.
|
||||
---
|
||||
|
||||
# Pre-PR Code Review
|
||||
|
||||
You are a senior engineer conducting a thorough code review. Review **only the lines that changed** in this branch (via `git diff main...HEAD`) and provide actionable feedback on code quality. Do not flag issues in unchanged code.
|
||||
|
||||
## Determine Files to Review
|
||||
|
||||
**Before starting the review**, identify which files to review by checking:
|
||||
|
||||
1. **Run git commands** to check both:
|
||||
- Committed changes: `git diff --name-only main...HEAD`
|
||||
- Unstaged/staged changes: `git status --short`
|
||||
|
||||
2. **Ask the user which set to review** if both exist:
|
||||
- If there are both committed changes AND unstaged/staged changes, ask: "I see you have both committed changes and unstaged/staged changes. Which would you like me to review?"
|
||||
- **Option A**: Committed changes in this branch (compare against main)
|
||||
- **Option B**: Current unstaged/staged changes
|
||||
- **Option C**: Both
|
||||
|
||||
3. **Proceed automatically** if only one set exists:
|
||||
- If only committed changes exist → review those
|
||||
- If only unstaged/staged changes exist → review those
|
||||
- If neither exist → inform the user there are no changes to review
|
||||
|
||||
4. **Get the file list** based on the user's choice:
|
||||
- For committed changes: Use `git diff --name-only main...HEAD`
|
||||
- For unstaged/staged: Use `git diff --name-only` and `git diff --cached --name-only`
|
||||
- Filter to only include files that exist (some may be deleted)
|
||||
|
||||
**Only proceed with the review once you have the specific list of files to review.**
|
||||
|
||||
## Review Checklist
|
||||
|
||||
### React Best Practices
|
||||
- **Components**: Are functional components with hooks used consistently?
|
||||
- **State Management**: Is `useState` and `useEffect` used properly? Any unnecessary re-renders?
|
||||
- **Props**: Are prop types properly defined with TypeScript interfaces?
|
||||
- **Keys**: Are list items using proper unique keys (not array indices)?
|
||||
- **Hooks Rules**: Are hooks called at the top level and in the correct order?
|
||||
- **Custom Hooks**: Could any logic be extracted into reusable custom hooks?
|
||||
|
||||
### TypeScript Best Practices
|
||||
- **const vs let vs var**: Is `const` used by default? Is `let` only used when reassignment is needed? Is `var` avoided entirely?
|
||||
- **Type Safety**: Are types explicit and avoiding `any`? Are proper interfaces/types defined?
|
||||
- **Type Assertions**: Are type assertions (`as`) used sparingly and only when necessary?
|
||||
- **Non-null Assertions**: Are non-null assertions (`!`) avoided? They bypass TypeScript's null safety and hide bugs. Use proper null checks or optional chaining instead.
|
||||
- **React Ref Types**: Are React refs properly typed as nullable (`useRef<T>(null)` with `RefObject<T | null>`)? Refs are null on first render and during unmount.
|
||||
- **Optional Chaining**: Is optional chaining (`?.`) used appropriately for potentially undefined values?
|
||||
- **Enums vs Union Types**: Are union types preferred over enums where appropriate?
|
||||
|
||||
### Design System & Styling
|
||||
- **Component Usage**: Are design system components used instead of raw HTML elements (`<Button>` not `<button>`, `<Input>` not `<input>`)?
|
||||
- **No Custom Styling**: Is custom inline styling or CSS avoided in favor of design system utilities?
|
||||
- **Tailwind Classes**: Are Tailwind utility classes used properly and consistently?
|
||||
- **Tailwind JIT Compilation**: Are Tailwind classes using static strings? JavaScript variables in template literals (e.g., `` `max-w-[${variable}]` ``) break JIT compilation. Use static strings or conditional logic instead (e.g., `condition ? 'max-w-[100px]' : 'max-w-[200px]'`).
|
||||
- **Theme Tokens**: Are theme tokens used for colors that adapt to light/dark mode (e.g., `text-foreground`, `bg-card`, `text-muted-foreground`) instead of hardcoded colors (e.g., `text-black`, `bg-white`)?
|
||||
- **Variants**: Could any components benefit from additional variants or properties in the design system?
|
||||
- **Light and Dark Mode Support**: Are colors working properly in both light and dark modes? No broken colors?
|
||||
- **Responsive Layout**: Does the layout work correctly at all breakpoints? No broken layout on mobile, tablet, or desktop?
|
||||
|
||||
### Code Simplicity (DRY Principle)
|
||||
- **Duplication**: Is there any repeated code that could be extracted into functions or components?
|
||||
- **Complexity**: Are there overly complex functions that could be broken down?
|
||||
- **Logic**: Is the logic straightforward and easy to follow?
|
||||
- **Abstractions**: Are abstractions appropriate (not too early, not too late)?
|
||||
- **Guard Clauses**: Are early-return guards used to keep code shallow and readable?
|
||||
|
||||
### Code Cleanliness
|
||||
- **Comments**: Are there unnecessary comments explaining obvious code? (Remove them)
|
||||
- **Console Logs**: Are there leftover `console.log` statements? (Remove them)
|
||||
- **Dead Code**: Is there unused code, commented-out code, or unused imports?
|
||||
- **Naming**: Are variable and function names clear and descriptive?
|
||||
- **Magic Numbers**: Are there magic numbers without explanation? Should they be named constants?
|
||||
|
||||
### Animation & UI Polish
|
||||
- **Race Conditions**: Are there any animation race conditions or timing issues?
|
||||
- **Single Source of Truth**: Is state managed in one place to avoid conflicts?
|
||||
- **AnimatePresence**: Is it used properly with unique keys for dialog/modal transitions?
|
||||
- **Reduced Motion**: Is `useReducedMotion()` respected for accessibility?
|
||||
|
||||
### General Code Quality
|
||||
- **Error Handling**: Are errors handled gracefully with user-friendly messages?
|
||||
- **Loading States**: Are loading states shown during async operations?
|
||||
- **Accessibility**: Are ARIA labels, keyboard navigation, and screen reader support considered?
|
||||
- **Performance**: Are there any obvious performance issues (unnecessary re-renders, heavy computations)?
|
||||
- **Git Hygiene**: Are there any files that shouldn't be committed (env files, etc.)?
|
||||
- **Unrelated Changes**: Are there any stray files or changes that don't relate to the branch's main purpose? (Accidental commits, unrelated fixes)
|
||||
|
||||
## Review & Fix Process
|
||||
|
||||
### Step 0: Run Quality Checks
|
||||
|
||||
Before reading any code, run the project's CI gate to establish a baseline:
|
||||
|
||||
```bash
|
||||
just ci
|
||||
```
|
||||
|
||||
This runs: `pnpm check` (Biome lint/format + file sizes), `pnpm typecheck`, `just clippy` (Rust linting), `pnpm test`, `pnpm build`, and `just tauri-check` (Rust type checking).
|
||||
|
||||
Report the results as pass/fail. Any failures are automatically **P0** issues and should appear at the top of the findings list. Do not skip this step even if the user only wants a quick review.
|
||||
|
||||
### Step 1: Conduct Review
|
||||
|
||||
For each file in the list:
|
||||
|
||||
1. Run `git diff main...HEAD -- <file>` to get the exact lines that changed
|
||||
2. Review **only those changed lines** against the Review Checklist — do not flag issues in unchanged code
|
||||
3. Note the file path and line numbers from the diff output for each issue found
|
||||
|
||||
### Step 2: Categorize Issues
|
||||
|
||||
Assign each issue a priority level:
|
||||
- **P0**: Breaks functionality, TypeScript errors, security issues
|
||||
- **P1–P2**: Performance problems, accessibility issues, code quality, unnecessary complexity, poor practices, design system violations
|
||||
- **P3**: Style inconsistencies, minor improvements, missing type safety, animation issues, theme token usage
|
||||
- **P4**: Cleanup — console logs, unused imports, dead code, unnecessary comments, unrelated changes
|
||||
|
||||
If many high-severity issues exist in a file, assess whether a full refactor would be simpler than individual fixes.
|
||||
|
||||
### Step 3: Present Findings
|
||||
|
||||
After reviewing all files, provide:
|
||||
- **Summary**: Total files reviewed, overall quality rating (1-5 stars)
|
||||
- **Issues**: A single numbered list ordered by priority (P0 first, P4 last). Each issue must follow this exact format:
|
||||
|
||||
```
|
||||
1. Short Issue Title (P0) [Must Fix]
|
||||
- Description of the issue and why it matters
|
||||
- Recommended fix
|
||||
|
||||
2. Short Issue Title (P3) [Your Call]
|
||||
- Description of the issue and why it may or may not need addressing
|
||||
- Recommended fix if the user chooses to act on it
|
||||
```
|
||||
|
||||
Use a short, descriptive title (3–6 words max) so issues can be referenced by number (e.g. "fix issue 3").
|
||||
|
||||
### Step 3b: Self-Check
|
||||
|
||||
Before presenting findings to the user, silently review the issue list two more times:
|
||||
|
||||
1. **Pass 1**: For each issue, ask — is this genuinely a problem, or could it be intentional/acceptable? Remove false positives.
|
||||
2. **Pass 2**: For each remaining issue, ask — does the recommended fix actually improve the code, or is it a matter of preference?
|
||||
|
||||
After both passes, tag each surviving issue as one of:
|
||||
- **[Must Fix]** — clear violation, will likely get flagged in PR review
|
||||
- **[Your Call]** — valid concern but may be intentional or a reasonable tradeoff (e.g. stepping outside the design system for a specific reason). Present it but let the user decide.
|
||||
|
||||
Only present issues that survived both passes.
|
||||
|
||||
### Step 4: Fix Issues
|
||||
|
||||
**Before fixing**, ask: "Would you like me to fix these issues in order? Or do you have questions about any of them first? I will fix each issue one by one and ask for approval before moving to the next one."
|
||||
|
||||
**When approved**, work through issues one at a time in numbered order (P0 → P4). After each fix:
|
||||
1. Explain what was changed and why
|
||||
2. Ask: "Does that look good? Ready to move on to issue [N]?"
|
||||
3. Wait for confirmation before proceeding to the next issue
|
||||
|
||||
**Important**: When adding documentation comments:
|
||||
- Only add comments for non-obvious things: magic numbers, complex logic, design decisions, or workarounds
|
||||
- If you call out something as confusing or hard-coded in your review and suggest adding documentation, it's acceptable to add a comment when approved
|
||||
- Don't add comments that just restate what the code does
|
||||
|
||||
Explain each change as you make it. If an issue is too subjective or minor, skip it and note why.
|
||||
|
||||
**Remember**: Cleanup tasks like removing comments should always be done LAST, because earlier fixes might introduce new comments that also need removal.
|
||||
|
||||
### Step 5: Ready to Ship
|
||||
|
||||
Once all issues are fixed, display:
|
||||
|
||||
---
|
||||
|
||||
**✅ Code review complete! All issues have been addressed.**
|
||||
|
||||
Your code is ready to commit and push. Lefthook will run the full CI gate (`just ci`) automatically when you push.
|
||||
|
||||
Next steps: generate a PR summary that explains the intent of this change, what files were modified and why, and how to verify the changes work.
|
||||
|
||||
---
|
||||
@@ -0,0 +1,88 @@
|
||||
---
|
||||
name: create-pr
|
||||
description: >-
|
||||
Create a GitHub PR from the current branch: handle uncommitted changes, generate
|
||||
a summary, and submit via gh CLI. Use when the user says "create PR", "open PR",
|
||||
"submit PR", "push PR", or wants to create a pull request.
|
||||
---
|
||||
|
||||
# Create PR
|
||||
|
||||
Create a GitHub PR from the current branch: handle uncommitted changes, generate a summary, and submit.
|
||||
|
||||
## Step 1: Rebase Reminder
|
||||
|
||||
Before doing anything else, remind the user to rebase onto main if they haven't already. Ask if they'd like to proceed or rebase first.
|
||||
|
||||
## Step 2: Check for Uncommitted Changes
|
||||
|
||||
Run `git status` to check for staged, unstaged, or untracked changes.
|
||||
|
||||
- If there are uncommitted changes, show the user what's outstanding and ask if they'd like to commit them before creating the PR.
|
||||
- If the user says yes, stage the relevant files, draft a concise commit message based on the changes, and commit.
|
||||
- If there are no uncommitted changes, move on.
|
||||
|
||||
## Step 3: Gather Branch Context
|
||||
|
||||
Run these commands in parallel to understand the branch:
|
||||
|
||||
1. `git log main..HEAD --oneline` to see all commits on this branch.
|
||||
2. `git diff main..HEAD --stat` to get the list of changed files.
|
||||
3. `git diff main..HEAD` to understand what changed in each file.
|
||||
4. `git rev-parse --abbrev-ref HEAD` to get the current branch name.
|
||||
5. `git status` to check if the branch has been pushed to remote.
|
||||
|
||||
## Step 4: Generate PR Title and Summary
|
||||
|
||||
**Title:** Generate a concise PR title (under 72 characters) that captures the intent of the change. Use conventional style: lowercase, imperative mood (e.g., "prevent chat list from reordering when renaming sessions").
|
||||
|
||||
**Body:** Generate a PR summary with these four sections:
|
||||
|
||||
### Section 1: Overview
|
||||
|
||||
Start with metadata tags, then a Problem/Solution block:
|
||||
|
||||
- `**Category:**` — one of: `new-feature`, `improvement`, `fix`, `infrastructure`
|
||||
- `**User Impact:**` — one sentence describing what changed from the user's perspective. Write this as a standalone sentence a non-technical stakeholder would understand (e.g., "Users can now create and schedule repeatable tasks directly from the desktop app."). This line is used for project changelogs.
|
||||
- `**Problem:**` — describe the user-facing confusion, mismatch, or friction this PR addresses.
|
||||
- `**Solution:**` — explain how the change resolves that UX problem and, if applicable, why the approach was chosen.
|
||||
|
||||
Keep Problem + Solution to 2-4 sentences total. Prioritize intent and expected user experience, but include brief high-level implementation rationale when it explains reliability, maintainability, or code quality.
|
||||
|
||||
### Section 2: Changes
|
||||
|
||||
Wrap this section in a collapsible `<details>` block with the summary "File changes".
|
||||
|
||||
Inside, list every changed file. For each file, use the filename as a bold header, then underneath write one or two sentences about what was changed and why. Focus on intent, not implementation details.
|
||||
|
||||
Format:
|
||||
```
|
||||
<details>
|
||||
<summary>File changes</summary>
|
||||
|
||||
**path/to/file.ts**
|
||||
What changed and why.
|
||||
|
||||
**path/to/other.rs**
|
||||
What changed and why.
|
||||
|
||||
</details>
|
||||
```
|
||||
|
||||
### Section 3: Reproduction Steps
|
||||
|
||||
Numbered steps in plain English for how an engineer can see the outcome of this PR. Assume they know how to run the project. Focus on where to look and what they should see.
|
||||
|
||||
### Section 4: Screenshots/Demos (for UX changes)
|
||||
|
||||
If the PR includes visual changes, include before/after screenshots or a short demo. If there are no visual changes, omit this section entirely.
|
||||
|
||||
## Step 5: Push and Create PR
|
||||
|
||||
1. Push the branch to remote if it hasn't been pushed yet: `git push -u origin HEAD`
|
||||
2. Create the PR using `gh pr create` with the generated title and body. Use a HEREDOC for the body to preserve formatting.
|
||||
3. Output the PR URL as a clickable hyperlink so the user can open it directly.
|
||||
|
||||
## Tone
|
||||
|
||||
Write from the perspective of a product designer explaining their thinking to engineers. Be clear and concise — just enough to establish intent. They can read the code; your job is to guide their understanding of the "why."
|
||||
+1
@@ -0,0 +1 @@
|
||||
../../.agents/skills/code-review
|
||||
+1
@@ -0,0 +1 @@
|
||||
../../.agents/skills/create-pr
|
||||
+1
@@ -0,0 +1 @@
|
||||
../../.agents/skills/code-review
|
||||
+1
@@ -0,0 +1 @@
|
||||
../../.agents/skills/create-pr
|
||||
+1
@@ -0,0 +1 @@
|
||||
../../.agents/skills/code-review
|
||||
+1
@@ -0,0 +1 @@
|
||||
../../.agents/skills/create-pr
|
||||
Reference in New Issue
Block a user