chore: add TDD, E2E, and feature skills, improve code review and pre-commit (#478)
This commit is contained in:
@@ -5,7 +5,7 @@ description: Review changed code in the Kandev monorepo for quality, security, a
|
||||
|
||||
# Code Review
|
||||
|
||||
Review the current changes in the Kandev codebase (Go + Next.js monorepo).
|
||||
Review the current changes in the Kandev codebase (Go + Next.js monorepo). Every finding needs a `file_path:line_number` reference, an explanation of *why* it matters, and a concrete fix.
|
||||
|
||||
## Steps
|
||||
|
||||
@@ -13,54 +13,112 @@ Review the current changes in the Kandev codebase (Go + Next.js monorepo).
|
||||
|
||||
Invoke the `/verify` skill to ensure all formatters, linters, typechecks, and tests pass. Do NOT proceed with review until verify passes clean.
|
||||
|
||||
### 2. Identify changed files
|
||||
### 2. Identify changed files and check scope
|
||||
|
||||
Run `git diff --name-only` (and `git diff --cached --name-only` for staged changes) to get the list of modified files. Read each changed file.
|
||||
Determine the right diff scope:
|
||||
- **Local changes**: `git diff --name-only` (unstaged) and `git diff --cached --name-only` (staged)
|
||||
- **PR review**: `git diff origin/<base_branch>...HEAD --name-only` to diff against the base branch
|
||||
|
||||
Read each changed file in full — understand surrounding code, not just the diff. Navigate callers, interfaces, and tests to understand changes end-to-end.
|
||||
|
||||
For each file, identify which requirement or intent it serves. Flag any changes that don't map to the task — scope creep is a blocker.
|
||||
|
||||
### 3. Review for issues
|
||||
|
||||
Check every changed file for the following. Report issues with `file_path:line_number` references.
|
||||
Check every changed file for the following layers. Skip layers that don't apply to the change.
|
||||
|
||||
**Complexity limits** (these are enforced by CI — catch them early):
|
||||
- Go: functions ≤80 lines, ≤50 statements, cyclomatic ≤15, cognitive ≤30, nesting ≤5
|
||||
- TS: files ≤600 lines, functions ≤100 lines, cyclomatic ≤15, cognitive ≤20, nesting ≤4
|
||||
- If a file is too large or a function too complex, split it into smaller cohesive files/functions grouped by responsibility
|
||||
|
||||
**Duplicated code:**
|
||||
- Look for copy-pasted logic across changed files and their surrounding code
|
||||
- Extract shared helpers, constants, or utility functions when duplication is found
|
||||
|
||||
**Dead code:**
|
||||
- Unused functions, variables, imports, types, or unreachable branches introduced by the changes
|
||||
- Remove them — don't leave commented-out code
|
||||
|
||||
**Security:**
|
||||
**Security** (blockers if found):
|
||||
- No secrets, tokens, or credentials in code
|
||||
- Proper input validation at system boundaries (user input, API handlers, external data)
|
||||
- Input validation at system boundaries (user input, API handlers, external data)
|
||||
- No SQL injection, XSS, command injection, or path traversal risks
|
||||
- Authentication and authorization checks in place for new endpoints
|
||||
- No insecure crypto (MD5/SHA1 for passwords, weak random)
|
||||
|
||||
**Architecture violations:**
|
||||
**Architecture:**
|
||||
- Frontend: no direct data fetching in components (must go through store), shadcn imports from `@kandev/ui` not `@/components/ui/*`
|
||||
- Backend: provider pattern for DI, context passed through call chains, event bus for cross-component communication
|
||||
- New abstractions justified — no over-engineering
|
||||
- Concerns cleanly separated (single responsibility)
|
||||
|
||||
**Missing unit tests:**
|
||||
**Logic & correctness:**
|
||||
- Edge cases handled (empty input, nil/null, zero, max values)
|
||||
- Error paths covered and not silently swallowed
|
||||
- Race conditions or concurrency issues in concurrent code
|
||||
|
||||
**Performance:**
|
||||
- No N+1 queries (loop with individual DB calls)
|
||||
- No memory leaks (unclosed connections, streams, listeners)
|
||||
- Missing database indexes for new query patterns
|
||||
- Algorithm complexity appropriate for the data scale
|
||||
|
||||
**Complexity limits** (CI also enforces these, but catch them early to avoid pushing and waiting):
|
||||
- Go: functions ≤80 lines, ≤50 statements, cyclomatic ≤15, cognitive ≤30, nesting ≤5
|
||||
- TS: files ≤600 lines, functions ≤100 lines, cyclomatic ≤15, cognitive ≤20, nesting ≤4
|
||||
- If too large or complex, split into smaller cohesive files/functions
|
||||
|
||||
**Code quality:**
|
||||
- No duplicated logic — extract shared helpers or constants
|
||||
- No dead code, unused imports, or commented-out code
|
||||
- Check for orphaned code: if the PR refactored or removed callers, grep for functions/types/exports that lost their last consumer
|
||||
- No speculative code — unused flags/options, "reserved for future" scaffolding, one-off abstractions with a single call site, options parsed but never used
|
||||
- Naming clear and consistent with project conventions
|
||||
- Deep nesting (>3 levels) — use early returns
|
||||
|
||||
**AI slop detection:**
|
||||
- Comments that restate code or narrate obvious steps
|
||||
- Unnecessary try/catch that swallow errors or return silent defaults in trusted internal paths
|
||||
- Redundant validation where inputs are already parsed/typed
|
||||
- `as any` or `as unknown as X` casts used to dodge type errors instead of fixing types
|
||||
- Defensive checks abnormal for the area of the codebase — compare with surrounding code patterns
|
||||
|
||||
**Testing:**
|
||||
- Backend (Go): new or changed functions/methods should have corresponding tests
|
||||
- Frontend (JS/TS libs only): new utility functions, hooks, API clients, and store slices should have tests
|
||||
- We do NOT test React components — skip those
|
||||
- Flag untested logic but don't block on it; suggest what tests to add
|
||||
|
||||
**Improvements:**
|
||||
- Suggest simplifications, better naming, or more idiomatic patterns — but only if the improvement is clear and worth the churn
|
||||
- Don't nitpick formatting or style that linters already cover
|
||||
- Flag untested logic but don't block on it; suggest what tests to add and recommend `/tdd`
|
||||
|
||||
### 4. Fix or report
|
||||
|
||||
- **Fix directly** any issues you can resolve confidently (dead code, unused imports, simple duplication, missing early returns)
|
||||
- **Report with explanation** any issues that need the author's input (architectural decisions, ambiguous test coverage, design trade-offs)
|
||||
- **Report** issues that need the author's input — always explain *why* the issue matters and provide a concrete suggested fix
|
||||
|
||||
### 5. Summary
|
||||
### 5. Output
|
||||
|
||||
Provide a brief summary:
|
||||
- Number of issues found and fixed
|
||||
- Any remaining items that need author attention
|
||||
- Overall assessment: ready to merge, needs minor fixes, or needs rework
|
||||
Use this format:
|
||||
|
||||
---
|
||||
|
||||
### Findings
|
||||
|
||||
#### Blocker (must fix before merge)
|
||||
*Security holes, data loss risk, broken logic, crashes*
|
||||
|
||||
1. **[Title]** — `file.go:42`
|
||||
- Issue: what's wrong
|
||||
- Why: why it matters
|
||||
- Fix: concrete suggestion or code snippet
|
||||
|
||||
#### Suggestion (recommended, doesn't block)
|
||||
*Performance problems, poor error handling, architectural concerns, missing tests*
|
||||
|
||||
### Summary
|
||||
|
||||
| Severity | Count |
|
||||
|----------|-------|
|
||||
| Blocker | N |
|
||||
| Suggestion | N |
|
||||
|
||||
**Verdict:** Ready to merge / Ready with suggestions / Blocked — fix blockers first
|
||||
|
||||
---
|
||||
|
||||
**Rules:**
|
||||
- Only report findings you're >=80% confident about — quality over quantity
|
||||
- Don't mark style preferences as blockers — linters cover formatting
|
||||
- Every criticism needs a suggested fix
|
||||
- Don't give feedback on code you didn't read
|
||||
- Omit empty severity sections
|
||||
|
||||
**Not a finding (skip these):**
|
||||
- Pre-existing issues on lines the change didn't modify
|
||||
- Things linters, typecheckers, or CI already catch (imports, types, formatting) — exception: still report complexity-limit violations since they require code changes to fix
|
||||
|
||||
@@ -0,0 +1,82 @@
|
||||
---
|
||||
name: e2e
|
||||
description: Write and run web E2E tests (Playwright) using TDD — locations, patterns, commands, and debugging.
|
||||
---
|
||||
|
||||
# E2E Tests
|
||||
|
||||
Write E2E tests using `/tdd` (Red-Green-Refactor). Always run the tests you create and watch them fail before implementing.
|
||||
|
||||
**Location:** `apps/web/e2e/`
|
||||
|
||||
```
|
||||
apps/web/e2e/
|
||||
├── fixtures/
|
||||
│ ├── backend.ts # Worker-scoped backend + frontend process
|
||||
│ └── test-base.ts # Extended fixture (apiClient, seedData, testPage)
|
||||
├── helpers/
|
||||
│ └── api-client.ts # HTTP client for seeding data (read for available methods)
|
||||
├── pages/ # Page objects (read for available pages and methods)
|
||||
└── tests/ # Spec files (*.spec.ts)
|
||||
```
|
||||
|
||||
Each worker gets an isolated backend, frontend, database, and mock agent — no Docker, no API keys needed.
|
||||
|
||||
## Run commands
|
||||
|
||||
```bash
|
||||
make test-e2e # all tests, headless
|
||||
make test-e2e-headed # with visible browser
|
||||
make test-e2e-ui # Playwright UI mode
|
||||
cd apps && pnpm --filter @kandev/web e2e -- tests/my-test.spec.ts # single file
|
||||
cd apps && pnpm --filter @kandev/web e2e -- --grep "task creation" # by name
|
||||
```
|
||||
|
||||
Prerequisites: `make build-backend build-web` (Make targets do this automatically).
|
||||
|
||||
## Writing a test
|
||||
|
||||
1. Read `helpers/api-client.ts` and `pages/` to discover available seed methods and page objects
|
||||
2. Import fixtures from `../fixtures/test-base` — provides `testPage`, `apiClient`, and `seedData` (pre-created workspace with default workflow)
|
||||
3. Use `data-testid` attributes for selectors — add them to components as needed
|
||||
4. Use page objects for common interactions; create new ones for new pages
|
||||
5. For GitHub features, use `apiClient.mockGitHub*()` methods to seed mock data
|
||||
|
||||
Example:
|
||||
|
||||
```typescript
|
||||
import { test, expect } from "../fixtures/test-base";
|
||||
import { KanbanPage } from "../pages/kanban-page";
|
||||
|
||||
test.describe("my feature", () => {
|
||||
test("does something", async ({ testPage, seedData, apiClient }) => {
|
||||
const task = await apiClient.createTask(seedData.workspaceId, "Test Task", "Description");
|
||||
const kanban = new KanbanPage(testPage);
|
||||
await kanban.goto(seedData.workspaceId);
|
||||
await expect(kanban.taskCardByTitle("Test Task")).toBeVisible();
|
||||
});
|
||||
});
|
||||
```
|
||||
|
||||
## Debugging failures
|
||||
|
||||
```bash
|
||||
E2E_DEBUG=1 make test-e2e # see backend/frontend stderr
|
||||
make test-e2e-ui # step through interactively
|
||||
make test-e2e-report # open HTML report from last run
|
||||
```
|
||||
|
||||
- **"Backend did not become healthy"** — run `make build-backend build-web`, check with `E2E_DEBUG=1`
|
||||
- **"Cannot find module"** — run `cd apps && pnpm install`
|
||||
- **Port conflicts** — backends use 18080+, frontends use 13000+ (per worker)
|
||||
- **Flaky timeouts** — health checks 30s, tests 60s. Check `fixtures/backend.ts` or `playwright.config.ts`
|
||||
- Screenshots on failure, video on first retry (CI)
|
||||
|
||||
## TDD workflow
|
||||
|
||||
Follow `/tdd` when writing E2E tests:
|
||||
|
||||
1. **RED** — Write the spec, run it, watch it fail (missing `data-testid`, feature not implemented, etc.)
|
||||
2. **GREEN** — Implement the feature/fix, add `data-testid` attributes, run the test until green
|
||||
3. **REFACTOR** — Extract page objects, clean up selectors, keep tests green
|
||||
4. Run `/verify` when done
|
||||
@@ -0,0 +1,75 @@
|
||||
---
|
||||
name: feature
|
||||
description: Guided feature development — brainstorm, explore codebase, design architecture, implement with TDD, and review. Use for new features or significant changes.
|
||||
---
|
||||
|
||||
# Feature Development
|
||||
|
||||
Systematic feature development: understand the problem, explore the codebase, design the solution, implement with TDD, verify it works, and review.
|
||||
|
||||
**No implementation before design approval.**
|
||||
|
||||
## Phase 1: Understand the problem
|
||||
|
||||
1. If the request is vague, ask clarifying questions — one at a time, prefer multiple choice
|
||||
2. Challenge the premise: is this the right feature, or is the real need something different?
|
||||
3. Identify: what problem is being solved, who benefits, what are the constraints
|
||||
4. Summarize understanding and confirm with the user
|
||||
|
||||
## Phase 2: Explore the codebase
|
||||
|
||||
1. Launch 2-3 Explore agents in parallel targeting different aspects:
|
||||
- Similar features and their implementation patterns
|
||||
- Architecture and abstractions in the relevant area
|
||||
- Integration points, data flow, and extension points
|
||||
2. Read all key files the agents identify
|
||||
3. Present a summary of existing patterns and conventions to reuse
|
||||
|
||||
## Phase 3: Design
|
||||
|
||||
1. **Scope check:** If the feature spans multiple independent subsystems, break it into sub-features first. Each should produce working, testable software on its own.
|
||||
2. Identify remaining ambiguities: edge cases, error handling, scope boundaries, backward compatibility
|
||||
3. Ask the user to resolve them — do not assume
|
||||
4. Propose 2-3 approaches with trade-offs and your recommendation:
|
||||
- Minimal change (smallest diff, maximum reuse)
|
||||
- Clean architecture (maintainability, elegant abstractions)
|
||||
- Pragmatic balance (speed + quality)
|
||||
5. **Map the file structure:** Before getting approval, list which files will be created or modified and what each is responsible for. This locks in decomposition decisions.
|
||||
6. Get explicit user approval on the approach before proceeding
|
||||
|
||||
## Phase 4: Implement with TDD
|
||||
|
||||
Follow `/tdd` strictly. Decide how to execute based on task independence:
|
||||
|
||||
**Independent tasks** (separate files/packages, no shared state): dispatch a subagent per task. Each gets a fresh context with the task description, relevant file paths from Phase 3, and codebase conventions from Phase 2. This prevents context pollution and keeps each task focused.
|
||||
|
||||
**Coupled tasks** (shared types, sequential data flow, integration work): implement inline in the current session to maintain shared context.
|
||||
|
||||
For each behavior or component:
|
||||
1. **RED** — write a failing test (unit, or E2E via `/e2e` for user-facing flows)
|
||||
2. **GREEN** — write the minimum code to pass
|
||||
3. **REFACTOR** — clean up while staying green
|
||||
4. Follow codebase conventions from Phase 2
|
||||
5. Build incrementally — one behavior at a time, tests passing at every step
|
||||
|
||||
**When things go wrong during implementation:**
|
||||
- **Bugs or missing validation discovered:** fix inline, add a test, continue
|
||||
- **Blocker (missing dependency, unclear requirement, test fails repeatedly):** stop and ask the user
|
||||
- **Fix requires architectural change (new DB table, new service layer, switching libraries):** stop and ask — don't make structural decisions silently
|
||||
- **3 failed fix attempts on the same issue:** stop, question the approach, ask the user
|
||||
|
||||
## Phase 5: QA
|
||||
|
||||
Run `/qa` as a subagent (keeps the main context clean) to verify the feature works end-to-end:
|
||||
- Trace the wiring (exports used, APIs called, data flows)
|
||||
- Test the happy path
|
||||
- Try to break it (boundary values, error paths, concurrency)
|
||||
- Write tests for any gaps found
|
||||
|
||||
## Phase 6: Review
|
||||
|
||||
1. Run `/simplify` to clean up the implementation
|
||||
2. Run `/verify` to ensure fmt, typecheck, test, and lint all pass
|
||||
3. Run `/code-review` as a subagent on the changes
|
||||
4. Fix any blockers, present suggestions to the user
|
||||
5. Summarize: what was built, key decisions, files modified, suggested next steps
|
||||
@@ -0,0 +1,67 @@
|
||||
---
|
||||
name: investigate
|
||||
description: Systematic bug investigation — reproduce, bisect to root cause, minimal fix with regression test. Use when something is broken and you don't know why.
|
||||
---
|
||||
|
||||
# Investigate
|
||||
|
||||
Systematic debugging when something is broken and the cause is unknown. Iron law: **no fix without root cause investigation first.**
|
||||
|
||||
## Steps
|
||||
|
||||
### 1. Reproduce
|
||||
|
||||
Before anything else, reproduce the bug reliably. Pick the right method based on where the bug lives:
|
||||
|
||||
- **Backend** (API, logic, data): write a Go test that calls the function/endpoint and asserts the wrong behavior. Run with `go test -v -run TestName ./path/...`
|
||||
- **Frontend** (UI, state, interaction): write a Playwright E2E test using `/e2e` that navigates to the page and triggers the bug. Run with `make test-e2e-headed` to see it visually.
|
||||
- **Full-stack** (user flow breaks end-to-end): Playwright E2E test that exercises the full path from UI through API to DB and back.
|
||||
- **Unclear where it lives**: start by reading the code path from the reported symptom (a page, an error message, a wrong value) back to its source. Then write the test at the appropriate level.
|
||||
|
||||
If it can't be reproduced, add logging/assertions to gather more info — don't guess at a fix.
|
||||
Find the minimal reproduction case: strip away everything that isn't needed to trigger the bug.
|
||||
|
||||
### 2. Narrow down the cause
|
||||
|
||||
Don't guess and patch — systematically narrow the scope:
|
||||
|
||||
**Trace the code path:** Follow the data from input to the failure point. Add assertions or logging at the midpoint of the call chain. Is the data correct there? If yes, the bug is downstream. If no, upstream. Repeat until you find the exact line where things go wrong.
|
||||
|
||||
**Narrow the input:** What's the smallest input that triggers the bug? What's the largest input that succeeds? Strip away everything that isn't needed to trigger it.
|
||||
|
||||
**Check history (only if it used to work):** If a feature regressed, use `git bisect` to find the commit that broke it. Skip this for bugs that were always present.
|
||||
|
||||
### 3. Confirm root cause
|
||||
|
||||
Before fixing, state the root cause clearly:
|
||||
- What is the actual cause (not the symptom)?
|
||||
- Why does it happen? (e.g., "empty string bypasses validation and reaches the DB layer")
|
||||
- Under what conditions? (e.g., "only when the input is whitespace-only")
|
||||
|
||||
If you can't state this clearly, you haven't found the root cause yet — go back to step 2.
|
||||
|
||||
### 4. Fix with TDD
|
||||
|
||||
Follow `/tdd`:
|
||||
1. Write a test that reproduces the exact bug — confirm it fails
|
||||
2. Write the minimal fix — change only what's necessary, don't refactor surrounding code
|
||||
3. Confirm the test passes and no other tests regress
|
||||
|
||||
### 5. Verify
|
||||
|
||||
- Run `/verify` to ensure nothing else broke
|
||||
- Check that the fix addresses the root cause, not just the symptom
|
||||
- If the same category of bug could occur elsewhere, grep for similar patterns
|
||||
|
||||
## Stop conditions
|
||||
|
||||
- **3 failed fix attempts:** Stop fixing and question the architecture. The bug may be a symptom of a deeper design issue.
|
||||
- **Can't reproduce:** Don't guess. Add observability (logging, assertions) and wait for it to happen again.
|
||||
- **Fix is larger than expected:** If the minimal fix touches many files, the root cause may be architectural. Discuss with the user before proceeding.
|
||||
|
||||
## What not to do
|
||||
|
||||
- Don't add try/catch to suppress the error — that hides the bug, it doesn't fix it
|
||||
- Don't "fix" by adding defensive checks everywhere — fix the one place that's wrong
|
||||
- Don't refactor while fixing — separate commits for separate concerns
|
||||
- Don't claim "fixed" without a regression test proving it
|
||||
@@ -1,6 +1,6 @@
|
||||
---
|
||||
name: pr-fixup
|
||||
description: Wait for CI checks and CodeRabbit review on a PR, fix failures and address comments, then push.
|
||||
description: Wait for CI checks and automated reviews (CodeRabbit, Greptile) on a PR, fix failures and address comments, then push.
|
||||
---
|
||||
|
||||
# PR Fixup
|
||||
@@ -38,20 +38,28 @@ gh pr checks <number>
|
||||
- Cap at **10 minutes** (20 polls). If still running after 10 min, report which checks are stuck and proceed.
|
||||
- Once done, note which checks passed and which failed.
|
||||
|
||||
### 3. Wait for CodeRabbit review
|
||||
### 3. Wait for automated reviews
|
||||
|
||||
Check if CodeRabbit has posted or is generating a review. Look at comments from `coderabbitai`:
|
||||
Check if CodeRabbit and Greptile have posted or are generating reviews.
|
||||
|
||||
**Stop waiting immediately if:**
|
||||
- A comment contains `<!-- rate limited by coderabbit.ai -->` — CodeRabbit is rate-limited and won't review this PR.
|
||||
- A comment contains `<!-- walkthrough_start -->` — the review is already complete.
|
||||
**Bot usernames:** `coderabbitai`, `greptile-apps[bot]`
|
||||
|
||||
**CodeRabbit — stop waiting if:**
|
||||
- A comment contains `<!-- rate limited by coderabbit.ai -->` — rate-limited, won't review.
|
||||
- A comment contains `<!-- walkthrough_start -->` — review complete.
|
||||
|
||||
**Greptile — stop waiting if:**
|
||||
- A review from `greptile-apps[bot]` exists (posts via the GitHub review API, not issue comments).
|
||||
|
||||
**Keep polling if:**
|
||||
- No `coderabbitai` comment exists AND `gh pr checks` shows a `CodeRabbit` check that is still `pending` — the review is being generated.
|
||||
- A bot hasn't commented yet AND `gh pr checks` shows its check is still `pending`.
|
||||
|
||||
Poll every **30 seconds**, cap at **10 minutes**. Fetch comments each poll:
|
||||
Poll every **30 seconds**, cap at **10 minutes**. Fetch both issue comments and reviews each poll:
|
||||
```bash
|
||||
gh pr view <number> --json comments --jq '.comments[] | select(.author.login == "coderabbitai") | .body'
|
||||
# CodeRabbit posts issue comments
|
||||
gh pr view <number> --json comments --jq '.comments[] | select(.author.login == "coderabbitai") | {author: .author.login, body: .body}'
|
||||
# Greptile posts reviews (with inline review comments)
|
||||
gh api repos/:owner/:repo/pulls/<number>/reviews --jq '.[] | select(.user.login == "greptile-apps[bot]") | {user: .user.login, state: .state}'
|
||||
```
|
||||
|
||||
### 4. Fix failing CI checks
|
||||
@@ -68,36 +76,61 @@ If any CI checks failed:
|
||||
|
||||
### 5. Triage review comments
|
||||
|
||||
Fetch all review comments — both human reviewers and CodeRabbit:
|
||||
Fetch all review comments — human reviewers, CodeRabbit, and Greptile:
|
||||
|
||||
```bash
|
||||
gh pr view <number> --json reviews,comments
|
||||
gh api repos/:owner/:repo/pulls/<number>/comments
|
||||
```
|
||||
|
||||
For each comment, decide:
|
||||
**Verify before implementing.** Do not blindly accept review feedback — evaluate each comment technically:
|
||||
|
||||
For each comment:
|
||||
1. Restate the requirement in your own words — if you can't, ask for clarification
|
||||
2. Check against the codebase: is the suggestion correct for THIS code?
|
||||
3. Check if it breaks existing functionality or conflicts with architectural decisions
|
||||
4. YAGNI check: if the suggestion adds unused features ("implement properly"), grep for actual usage first
|
||||
|
||||
Then classify:
|
||||
- **Valid and actionable** — real issue (bug, missing edge case, naming, architecture, code quality). Fix it.
|
||||
- **Already addressed** — the code already handles what the comment suggests. Skip.
|
||||
- **Nitpick or preference** — subjective style not covered by linters. Skip unless the reviewer insists.
|
||||
- **Wrong or outdated** — misunderstands the code or refers to old state. Skip.
|
||||
- **Wrong or outdated** — misunderstands the code, refers to old state, or is technically incorrect. Push back with reasoning.
|
||||
|
||||
**Push back when:**
|
||||
- The suggestion breaks existing functionality
|
||||
- The reviewer lacks full context (explain what they're missing)
|
||||
- It violates YAGNI (the feature is unused)
|
||||
- It's technically incorrect for this stack
|
||||
- It conflicts with architectural decisions
|
||||
|
||||
### 6. Address each comment
|
||||
|
||||
Every comment must get a response — either a fix or a reply explaining why it was skipped.
|
||||
|
||||
**Important: issue comments vs review comments use different APIs:**
|
||||
- **Review comments** (inline, from `gh api repos/:owner/:repo/pulls/<number>/comments`) — reply via `/pulls/<number>/comments/<comment_id>/replies`, react via `/pulls/comments/<comment_id>/reactions`
|
||||
- **Issue comments** (conversation timeline, from `gh pr view --json comments` — e.g., CodeRabbit walkthrough) — reply by posting a new comment via `gh pr comment <number> --body "..."`, react via `/issues/comments/<comment_id>/reactions`
|
||||
|
||||
**For valid comments:**
|
||||
1. Read the file at the referenced line
|
||||
2. Implement the fix
|
||||
3. React with thumbs up:
|
||||
```bash
|
||||
# For review comments:
|
||||
gh api repos/:owner/:repo/pulls/comments/<comment_id>/reactions -f content="+1"
|
||||
# For issue comments:
|
||||
gh api repos/:owner/:repo/issues/comments/<comment_id>/reactions -f content="+1"
|
||||
```
|
||||
4. Resolve the review thread (see below for thread ID retrieval)
|
||||
|
||||
**For skipped comments** (already addressed, nitpick, wrong, or outdated):
|
||||
1. Reply to the comment explaining why it was skipped:
|
||||
```bash
|
||||
# For review comments:
|
||||
gh api repos/:owner/:repo/pulls/<number>/comments/<comment_id>/replies -f body="<explanation>"
|
||||
# For issue comments:
|
||||
gh pr comment <number> --body "<explanation>"
|
||||
```
|
||||
Examples:
|
||||
- "This is already handled by X on line Y."
|
||||
|
||||
@@ -0,0 +1,81 @@
|
||||
---
|
||||
name: qa
|
||||
description: Verify a feature works after implementation. Actively try to break it — edge cases, error paths, integration wiring, and real usage flows.
|
||||
---
|
||||
|
||||
# QA
|
||||
|
||||
Verify that a feature works as intended after implementation. Assume bugs exist and hunt for them.
|
||||
|
||||
Mindset: you are not confirming it works — you are discovering where it breaks.
|
||||
|
||||
## Steps
|
||||
|
||||
### 1. Understand the intent
|
||||
|
||||
Read the task description, PR, or recent commits to understand what was built and what it should do. Identify:
|
||||
- The expected behavior (happy path)
|
||||
- System boundaries (user input, API endpoints, external data)
|
||||
- Integration points (what calls what, data flow end-to-end)
|
||||
|
||||
### 2. Trace the wiring
|
||||
|
||||
Before testing behavior, verify the feature is actually connected:
|
||||
- Exports are imported and used (not just defined)
|
||||
- API routes have consumers (frontend calls them, or tests exercise them)
|
||||
- Data flows end-to-end: input -> handler -> storage -> response -> display
|
||||
- New config/env vars are documented and have defaults
|
||||
|
||||
If something is orphaned or unwired, stop and report it — no point testing disconnected code.
|
||||
|
||||
### 3. Test the happy path
|
||||
|
||||
Run the feature as a user would. For backend changes, call the API. For frontend changes, trace the UI flow. For both, follow the full path:
|
||||
- Does the basic use case work?
|
||||
- Does the response/output match expectations?
|
||||
- Is the data persisted correctly?
|
||||
|
||||
### 4. Try to break it
|
||||
|
||||
Systematically test these categories (skip what doesn't apply):
|
||||
|
||||
**Boundary values:**
|
||||
- Empty input, nil/null, zero, negative numbers, max values
|
||||
- Empty arrays/maps, single element, very large collections
|
||||
- Strings: empty, whitespace-only, special characters, very long
|
||||
|
||||
**Error paths:**
|
||||
- What happens when dependencies fail (DB down, API timeout, invalid response)?
|
||||
- Are errors surfaced clearly or silently swallowed?
|
||||
- Does the system recover or get stuck in a bad state?
|
||||
|
||||
**Concurrency:**
|
||||
- What happens with simultaneous requests to the same resource?
|
||||
- Race conditions: create/update/delete at the same time
|
||||
- Does it handle duplicate submissions?
|
||||
|
||||
**Authorization:**
|
||||
- Can the feature be accessed without proper auth?
|
||||
- Does it respect permission boundaries?
|
||||
|
||||
### 5. Verify test coverage
|
||||
|
||||
Check that the implementation has tests covering the behaviors you just verified:
|
||||
- Are the happy path and key error paths tested?
|
||||
- Are edge cases from step 4 covered?
|
||||
- If tests are missing, write them using `/tdd`
|
||||
|
||||
### 6. Report
|
||||
|
||||
Summarize what was tested and what was found:
|
||||
|
||||
**Verified working:**
|
||||
- List of behaviors confirmed working
|
||||
|
||||
**Issues found:**
|
||||
- file:line - description, how to reproduce, severity (blocker/suggestion)
|
||||
|
||||
**Missing test coverage:**
|
||||
- Behaviors that work but have no automated test
|
||||
|
||||
**Verdict:** Feature complete / Has issues — fix before merge
|
||||
@@ -0,0 +1,58 @@
|
||||
---
|
||||
name: simplify
|
||||
description: Simplify recently changed code — inline one-off abstractions, remove speculative code, reduce nesting, replace cleverness with clarity. Run after implementing a feature.
|
||||
---
|
||||
|
||||
# Simplify
|
||||
|
||||
Post-implementation simplification pass. Review recently changed code and actively simplify it while preserving all behavior.
|
||||
|
||||
The best code is code you don't have to write. The second best is code anyone can read.
|
||||
|
||||
## Steps
|
||||
|
||||
### 1. Identify what to simplify
|
||||
|
||||
Run `git diff --name-only` (or `git diff origin/<base>...HEAD --name-only` for a branch) to get the changed files. Read each one.
|
||||
|
||||
### 2. Apply simplifications
|
||||
|
||||
Work through each changed file. For each simplification, verify tests still pass before moving on.
|
||||
|
||||
**Inline one-off abstractions:**
|
||||
- Helper functions with a single call site — inline them
|
||||
- Wrapper types that add no behavior — remove the wrapper
|
||||
- Interfaces with a single implementation and no test mock — remove the interface
|
||||
|
||||
**Remove speculative code:**
|
||||
- Unused function parameters or return values
|
||||
- Config options that are parsed but never read
|
||||
- "Reserved for future" scaffolding, empty extension points
|
||||
- Feature flags with no toggle mechanism
|
||||
|
||||
**Reduce nesting:**
|
||||
- Replace nested if/else with early returns (guard clauses)
|
||||
- Replace nested ternaries with if/else or switch
|
||||
- Extract deeply nested blocks into named functions
|
||||
|
||||
**Replace cleverness with clarity:**
|
||||
- Dense one-liners that hide intent — expand to readable multi-line
|
||||
- Overly generic code where a concrete implementation is simpler
|
||||
- Abstractions that add indirection without reducing duplication
|
||||
|
||||
**Remove noise:**
|
||||
- Comments that restate code (`// increment counter` above `counter++`)
|
||||
- Redundant type annotations where inference works
|
||||
- Empty error handlers, unused catch variables
|
||||
- Leftover debug logging
|
||||
|
||||
### 3. Verify
|
||||
|
||||
Run `/verify` to ensure all tests, lints, and typechecks still pass. If anything breaks, the simplification changed behavior — revert it.
|
||||
|
||||
### 4. Summary
|
||||
|
||||
Report what was simplified:
|
||||
- Files modified
|
||||
- What was removed/inlined/simplified
|
||||
- Lines removed (net)
|
||||
@@ -0,0 +1,96 @@
|
||||
---
|
||||
name: tdd
|
||||
description: Implement changes using Test-Driven Development (Red-Green-Refactor). Use for bug fixes, new features, or any code change that should have test coverage.
|
||||
---
|
||||
|
||||
# TDD
|
||||
|
||||
Implement code changes using strict Red-Green-Refactor. Iron law: **no production code without a failing test first.**
|
||||
|
||||
Wrote code before a test? Delete it. Start over from a failing test.
|
||||
|
||||
## When to use
|
||||
|
||||
- Bug fixes — write a test that reproduces the bug before fixing
|
||||
- New functions, methods, or utilities
|
||||
- Refactoring existing logic that lacks tests
|
||||
|
||||
**Skip** for: pure UI components (we don't test React components), config files, generated code.
|
||||
|
||||
## Determine test scope
|
||||
|
||||
- **Go unit** (`apps/backend/`): test file next to source as `*_test.go`. Run:
|
||||
```bash
|
||||
cd apps/backend && go test -v -run TestName ./internal/path/to/package/...
|
||||
```
|
||||
- **TypeScript unit** (`apps/web/lib/`): test file next to source as `*.test.ts`. Run:
|
||||
```bash
|
||||
cd apps && pnpm --filter @kandev/web test -- --run path/to/file.test.ts
|
||||
```
|
||||
- **Web E2E** (`apps/web/e2e/`): Playwright tests with full-stack isolation (backend + frontend + DB per worker). Uses page objects in `e2e/pages/` and fixtures in `e2e/fixtures/`. Run:
|
||||
```bash
|
||||
make test-e2e # all tests, headless
|
||||
cd apps && pnpm --filter @kandev/web e2e -- tests/my-test.spec.ts # single file
|
||||
make test-e2e-headed # with visible browser
|
||||
```
|
||||
|
||||
Choose the right level: unit tests for isolated logic, web E2E for user-facing flows.
|
||||
|
||||
## Steps
|
||||
|
||||
### 1. RED — Write a failing test
|
||||
|
||||
1. Identify the single behavior to implement or bug to reproduce
|
||||
2. Write the **smallest test** that asserts the expected behavior — one assertion, clear name
|
||||
3. Run the test and confirm it **fails with the expected assertion error** (not a compile/import error)
|
||||
4. If it passes immediately, the test is not testing new behavior — revise it
|
||||
|
||||
### 2. GREEN — Minimal code to pass
|
||||
|
||||
1. Write the **minimum production code** to make the failing test pass
|
||||
2. Do not add extra logic, handle other edge cases, or refactor yet
|
||||
3. Run the test again and confirm it **passes**
|
||||
4. If it fails, fix the production code (not the test)
|
||||
|
||||
### 3. REFACTOR — Clean up
|
||||
|
||||
1. Improve production code: extract helpers, rename, simplify — without changing behavior
|
||||
2. Improve tests: table-driven tests (Go) or `describe`/`it` blocks (TS), remove duplication
|
||||
3. Run the test after each change to confirm still green
|
||||
|
||||
### 4. Repeat
|
||||
|
||||
Return to step 1 for the next behavior or edge case. Continue until the feature or fix is complete.
|
||||
|
||||
### 5. Final verification
|
||||
|
||||
Run `/verify` to ensure all formatters, linters, typechecks, and tests pass across the monorepo.
|
||||
|
||||
## Testing anti-patterns
|
||||
|
||||
**Don't test mock behavior:**
|
||||
- If your assertion checks a mock element (`*-mock` test ID, mock return value), you're testing the mock, not the code. Test real behavior or don't mock it.
|
||||
|
||||
**Don't add test-only methods to production code:**
|
||||
- `destroy()`, `reset()`, `_testHelper()` that only tests call — put these in test utilities, not production classes.
|
||||
|
||||
**Mock minimally and understand dependencies:**
|
||||
- Before mocking, ask: what side effects does the real method have? Does the test depend on any of them?
|
||||
- Mock the slow/external part (network, disk), not the method the test depends on.
|
||||
- If mock setup is longer than test logic, consider an integration test instead.
|
||||
|
||||
**Don't use incomplete mocks:**
|
||||
- Mock the complete data structure as it exists in reality, not just fields your test uses. Partial mocks hide bugs when downstream code accesses omitted fields.
|
||||
|
||||
**Never swallow errors in tests:**
|
||||
- `try/catch` that silently ignores failures in test helpers or setup — these hide real failures.
|
||||
|
||||
## Red flags
|
||||
|
||||
- Writing production code before a failing test exists — delete and start over
|
||||
- Test passes on first run — it tests nothing new, revise the test
|
||||
- Fixing a test to make it pass instead of fixing the production code
|
||||
- Large jumps — multiple behaviors implemented between test runs
|
||||
- Skipping the refactor step
|
||||
- Mock setup longer than test logic — consider integration test
|
||||
- Asserting on mock elements instead of real behavior
|
||||
+6
-49
@@ -19,18 +19,11 @@ repos:
|
||||
files: ^apps/backend/.*\.go$
|
||||
pass_filenames: true
|
||||
|
||||
- id: go-tests
|
||||
name: Go tests (backend)
|
||||
entry: bash -c 'cd apps/backend && go test ./...'
|
||||
language: system
|
||||
files: \.go$
|
||||
pass_filenames: false
|
||||
|
||||
- id: go-lint
|
||||
name: Go Lint (backend)
|
||||
name: Go lint (changed files only)
|
||||
entry: bash -c 'unset GIT_DIR GIT_WORK_TREE; cd apps/backend && golangci-lint run --new-from-rev=HEAD'
|
||||
language: system
|
||||
files: \.go$
|
||||
files: ^apps/backend/.*\.go$
|
||||
pass_filenames: false
|
||||
|
||||
# Web app (TypeScript/React) hooks
|
||||
@@ -38,50 +31,14 @@ repos:
|
||||
hooks:
|
||||
- id: prettier-format
|
||||
name: Prettier format (web/cli/packages)
|
||||
entry: bash -c 'cd apps && pnpm exec prettier --write --ignore-unknown "${@/#apps\//}"' --
|
||||
entry: bash -c 'cd apps || exit 1; args=(); for f in "$@"; do args+=("${f#apps/}"); done; pnpm exec prettier --write --ignore-unknown "${args[@]}"' --
|
||||
language: system
|
||||
files: ^apps/(web|cli|packages)/.*\.(ts|tsx|js|jsx|mjs|cjs|json|md|mdx|yml|yaml|css|scss)$
|
||||
pass_filenames: true
|
||||
|
||||
- id: web-lint
|
||||
name: Web lint
|
||||
entry: bash -c 'cd apps && pnpm --filter @kandev/web lint'
|
||||
language: node
|
||||
name: Web lint (changed files only)
|
||||
entry: bash -c 'cd apps/web || exit 1; args=(); for f in "$@"; do args+=("${f#apps/web/}"); done; pnpm exec eslint --max-warnings 0 "${args[@]}"' --
|
||||
language: system
|
||||
files: ^apps/web/.*\.(ts|tsx|js|jsx)$
|
||||
pass_filenames: true
|
||||
additional_dependencies: []
|
||||
|
||||
- id: web-tests
|
||||
name: Web tests
|
||||
entry: bash -c 'cd apps && pnpm --filter @kandev/web test'
|
||||
language: node
|
||||
files: ^apps/web/.*\.(ts|tsx|js|jsx)$
|
||||
pass_filenames: false
|
||||
additional_dependencies: []
|
||||
|
||||
- id: web-typecheck
|
||||
name: Typecheck web app
|
||||
entry: bash -c 'cd apps && pnpm --filter @kandev/web exec tsc -p tsconfig.json --noEmit'
|
||||
language: node
|
||||
files: ^apps/web/.*\.(ts|tsx)$
|
||||
pass_filenames: false
|
||||
additional_dependencies: []
|
||||
|
||||
# Landing page hooks
|
||||
- repo: local
|
||||
hooks:
|
||||
# - id: landing-lint
|
||||
# name: Landing lint
|
||||
# entry: bash -c 'cd apps && pnpm --filter @kandev/landing lint'
|
||||
# language: node
|
||||
# files: ^apps/landing/.*\.(ts|tsx|js|jsx)$
|
||||
# pass_filenames: true
|
||||
# additional_dependencies: []
|
||||
|
||||
- id: landing-typecheck
|
||||
name: Typecheck landing page
|
||||
entry: bash -c 'cd apps && pnpm --filter @kandev/landing exec tsc -p tsconfig.json --noEmit'
|
||||
language: node
|
||||
files: ^apps/landing/.*\.(ts|tsx)$
|
||||
pass_filenames: false
|
||||
additional_dependencies: []
|
||||
|
||||
@@ -6,54 +6,74 @@ STEP 1: Determine what to review
|
||||
- If the working directory is clean: review ONLY the commits from this branch
|
||||
- Use: git log --oneline $(git merge-base origin/main HEAD)..HEAD to list the branch commits
|
||||
- Use: git diff $(git merge-base origin/main HEAD) to see the cumulative changes
|
||||
- Do NOT diff directly against origin/main or master — that would include unrelated changes if the branch is outdated
|
||||
- Do NOT diff directly against origin/main or master - that would include unrelated changes if the branch is outdated
|
||||
- Read each changed file in full — understand surrounding code, not just the diff
|
||||
- Navigate callers, interfaces, and tests to understand changes end-to-end
|
||||
- Check git blame on modified sections to understand why code was written a certain way
|
||||
- Only REPORT issues on code modified in this changeset, but USE the full codebase for context
|
||||
|
||||
STEP 2: Review the changes, then output your findings in EXACTLY 4 sections: BUG, IMPROVEMENT, NITPICK, PERFORMANCE.
|
||||
If a code review skill is available (e.g. /code-review, /review), invoke it instead of using the fallback below.
|
||||
|
||||
Rules:
|
||||
- Each section is OPTIONAL - only include it if you have findings for that category
|
||||
- If a section has no findings, omit it entirely
|
||||
- Format each finding as: filename:line_number - Description
|
||||
- Be specific and reference exact line numbers
|
||||
- Keep descriptions concise but actionable
|
||||
- Sort findings by severity within each section
|
||||
- Focus on logic and design issues, NOT formatting or style that automated tools handle
|
||||
STEP 2: Review the changes across these layers (skip layers that don't apply):
|
||||
|
||||
Section definitions:
|
||||
SECURITY (blockers if found):
|
||||
- No secrets, tokens, or credentials in code
|
||||
- Input validation at system boundaries (user input, API handlers, external data)
|
||||
- No SQL injection, XSS, command injection, or path traversal
|
||||
- Auth and authorization checks on new endpoints
|
||||
- No insecure crypto (MD5/SHA1 for passwords, weak random)
|
||||
|
||||
BUG: Critical issues that will cause runtime errors, crashes, incorrect behavior, data corruption, or logic errors
|
||||
- Examples: null/nil dereference, race conditions, incorrect algorithms, type mismatches, resource leaks, deadlocks
|
||||
LOGIC & CORRECTNESS:
|
||||
- Edge cases handled (empty input, nil/null, zero, max values)
|
||||
- Error paths covered and not silently swallowed
|
||||
- Race conditions or concurrency issues (unprotected shared state, missing synchronization, goroutine leaks)
|
||||
- Broken invariants — state that can become invalid
|
||||
|
||||
IMPROVEMENT: Code quality, architecture, security, or maintainability concerns
|
||||
- Examples: missing error handling, incorrect access modifiers (public/private/exported), SQL injection vulnerabilities, hardcoded credentials, tight coupling, missing validation, incorrect concurrency patterns
|
||||
PERFORMANCE:
|
||||
- No N+1 queries (loop with individual DB calls)
|
||||
- No memory leaks (unclosed connections, streams, listeners)
|
||||
- Algorithm complexity appropriate for data scale (O(n^2) where O(n) is possible)
|
||||
- Unnecessary allocations in loops, regex compilation in hot paths, unbounded resource growth
|
||||
- Prefer structured concurrency (errgroup, conc) over raw primitives
|
||||
|
||||
NITPICK: Significant readability or maintainability issues that impact code understanding
|
||||
- Examples: misleading variable/function names, overly complex logic that should be refactored, missing critical comments for complex algorithms, inconsistent error handling patterns
|
||||
- EXCLUDE: formatting, whitespace, import ordering, trivial naming preferences, style issues handled by linters/formatters
|
||||
CODE QUALITY:
|
||||
- No dead code, unused imports, or commented-out code
|
||||
- Check for orphaned code: if the change refactored or removed callers, grep for functions/types/exports that lost their last consumer
|
||||
- No speculative code (unused flags, one-off abstractions with single call site)
|
||||
- No duplicated logic — extract shared helpers or constants
|
||||
- Deep nesting (>3 levels) — use early returns
|
||||
|
||||
PERFORMANCE: Algorithmic or resource usage problems with measurable impact
|
||||
- Examples: O(n²) where O(n) or O(1) is possible, unnecessary allocations in loops, missing indexes for database queries, blocking I/O in hot paths, regex compilation in loops, unbounded resource growth
|
||||
- Concurrency-specific: unprotected shared state, missing synchronization, improper use of locks, goroutine leaks, missing context cancellation
|
||||
- Prefer structured concurrency libraries (e.g., errgroup, conc) over raw primitives for better error handling and panic recovery
|
||||
AI SLOP DETECTION:
|
||||
- Comments that restate code or narrate obvious steps
|
||||
- Unnecessary try/catch that swallow errors or return silent defaults
|
||||
- as any / as unknown casts to dodge type errors instead of fixing types
|
||||
- Redundant validation where inputs are already parsed/typed
|
||||
- Defensive checks abnormal for the area — compare with surrounding code patterns
|
||||
|
||||
Example format:
|
||||
## BUG
|
||||
- src/handler.go:45 - Dereferencing pointer without nil check will panic when user is not found
|
||||
- lib/parser.rs:123 - Loop condition uses <= instead of < causing out-of-bounds access
|
||||
STEP 3: Output your findings.
|
||||
|
||||
## IMPROVEMENT
|
||||
- api/db.go:67 - Database query error ignored, will silently fail and return stale data
|
||||
- services/auth.py:34 - Password comparison vulnerable to timing attacks, use constant-time comparison
|
||||
- internal/user.go:15 - Type exported but only used internally, should be unexported
|
||||
Every finding needs: file:line, what's wrong, why it matters, and how to fix it.
|
||||
Only report findings you're >=80% confident about.
|
||||
|
||||
## NITPICK
|
||||
- components/processor.ts:12 - Function name 'doStuff' doesn't describe what it actually does (transforms user input to API format)
|
||||
- utils/cache.go:89 - Error wrapped multiple times making original cause hard to trace
|
||||
Use these sections (omit empty ones):
|
||||
|
||||
## PERFORMANCE
|
||||
- src/repository.go:156 - Linear search through slice on every request, use map for O(1) lookup
|
||||
- handlers/api.py:45 - Compiling regex inside handler function, compile once at module level
|
||||
- workers/processor.go:78 - Launching unbounded goroutines without limit, use worker pool or semaphore pattern
|
||||
- db/queries.go:34 - N+1 query pattern, fetch all related records in single query with join
|
||||
## BLOCKER
|
||||
Must fix before merge — security holes, data loss risk, broken logic, crashes.
|
||||
|
||||
Now review the changes.
|
||||
- file:line - Description. Why it matters. How to fix.
|
||||
|
||||
## SUGGESTION
|
||||
Recommended but doesn't block — performance, architecture, missing tests.
|
||||
|
||||
- file:line - Description. Why it matters. How to fix.
|
||||
|
||||
End with a verdict: Ready to merge / Ready with suggestions / Blocked — fix blockers first
|
||||
|
||||
NOT A FINDING (skip these):
|
||||
- Issues on lines or files the change didn't modify — even if they are real bugs
|
||||
- Pre-existing code patterns that this change didn't introduce
|
||||
- Things linters, typecheckers, or CI catch (imports, types, formatting)
|
||||
- Intentional functionality changes directly related to the task
|
||||
- Issues explicitly suppressed in code (lint-ignore, nolint comments)
|
||||
- Pedantic nitpicks a senior engineer wouldn't flag
|
||||
- General "add more tests" without specifying what logic is untested
|
||||
|
||||
@@ -22,21 +22,81 @@ steps:
|
||||
show_in_command_panel: true
|
||||
allow_manual_move: true
|
||||
prompt: |
|
||||
Please review the changes in this pull request.
|
||||
|
||||
STEP 1: Determine what to review
|
||||
Review ONLY the changes in this pull request. The task description below contains the PR details and the exact git commands to scope the diff — follow those instructions.
|
||||
|
||||
STEP 2: Review the changes, then output your findings in EXACTLY 4 sections: BUG, IMPROVEMENT, NITPICK, PERFORMANCE.
|
||||
|
||||
Rules:
|
||||
- Each section is OPTIONAL — only include it if you have findings for that category
|
||||
- If a section has no findings, omit it entirely
|
||||
- Format each finding as: filename:line_number - Description
|
||||
- Be specific and reference exact line numbers
|
||||
- Keep descriptions concise but actionable
|
||||
Review the changes in this pull request.
|
||||
|
||||
{{task_prompt}}
|
||||
|
||||
STEP 1: DETERMINE WHAT TO REVIEW
|
||||
|
||||
The PR branch is checked out and the full codebase is available. You can and should read surrounding code to understand changes in context.
|
||||
- Extract the PR number from the task description (e.g. from the GitHub URL)
|
||||
- Run: gh pr view <number> --json headRefName,baseRefName to confirm the branch and base
|
||||
- Run: git diff origin/<base_branch>...HEAD --name-only to get the list of changed files
|
||||
- Run: git diff origin/<base_branch>...HEAD to see the full diff
|
||||
- Read each changed file in full — understand the surrounding code, not just the diff
|
||||
- Navigate related files (callers, interfaces, tests) to understand the change end-to-end
|
||||
- Only REPORT issues on code that was modified in this PR, but USE the full codebase for context
|
||||
|
||||
STEP 2: REVIEW THE CHANGES
|
||||
|
||||
Check if any code review skill is available (e.g. /code-review, /review, or similar).
|
||||
If one exists, invoke it — it has project-specific rules.
|
||||
Otherwise, use the fallback review below.
|
||||
|
||||
FALLBACK REVIEW (skip layers that don't apply):
|
||||
|
||||
Security (blockers if found):
|
||||
- No secrets, tokens, or credentials in code
|
||||
- Input validation at system boundaries (user input, API handlers, external data)
|
||||
- No SQL injection, XSS, command injection, or path traversal
|
||||
- Auth and authorization checks on new endpoints
|
||||
|
||||
Logic & correctness:
|
||||
- Edge cases handled (empty input, nil/null, zero, max values)
|
||||
- Error paths covered and not silently swallowed
|
||||
- Race conditions in concurrent code
|
||||
|
||||
Performance:
|
||||
- No N+1 queries (loop with individual DB calls)
|
||||
- No memory leaks (unclosed connections, streams, listeners)
|
||||
- Algorithm complexity appropriate for the data scale
|
||||
|
||||
Architecture:
|
||||
- New abstractions justified — no over-engineering
|
||||
- Concerns cleanly separated (single responsibility)
|
||||
|
||||
Code quality:
|
||||
- No dead code, unused imports, or commented-out code
|
||||
- No speculative code (unused flags, one-off abstractions with single call site)
|
||||
- Deep nesting (>3 levels) — suggest early returns
|
||||
|
||||
AI slop detection:
|
||||
- Comments that restate code or narrate obvious steps
|
||||
- Unnecessary try/catch that swallow errors or return silent defaults
|
||||
- as any / as unknown casts to dodge type errors instead of fixing types
|
||||
- Redundant validation where inputs are already parsed/typed
|
||||
|
||||
OUTPUT FORMAT:
|
||||
|
||||
For each finding, provide: file:line - description, why it matters, and how to fix it.
|
||||
Only report findings you're >=80% confident about.
|
||||
|
||||
Use these sections (omit empty ones):
|
||||
|
||||
BLOCKER (must fix before merge — security, data loss, broken logic):
|
||||
- file:line - description
|
||||
|
||||
SUGGESTION (recommended, doesn't block — performance, architecture, missing tests):
|
||||
- file:line - description
|
||||
|
||||
End with a verdict: Ready to merge / Ready with suggestions / Blocked
|
||||
|
||||
NOT A FINDING (skip these):
|
||||
- Issues on lines or files the PR didn't modify — even if they are real bugs
|
||||
- Pre-existing code patterns that this PR didn't introduce or change
|
||||
- Things linters, typecheckers, or CI catch (imports, types, formatting)
|
||||
- Intentional functionality changes directly related to the PR's purpose
|
||||
- Issues explicitly suppressed in code (lint-ignore, nolint comments)
|
||||
events:
|
||||
on_enter:
|
||||
- type: auto_start_agent
|
||||
|
||||
Reference in New Issue
Block a user