<label> [decorations]: <subject>

[discussion]

// ✅ Good: Clear, specific, actionable

praise: Excellent use of TypeScript generics here!

This makes the function much more reusable while maintaining type safety.

---

nitpick [non-blocking]: Consider using const instead of let

This variable is never reassigned, so `const` would be more appropriate:
```typescript
const MAX_RETRIES = 3;

try \{
  const data = await fetchUser(userId);
  // ...
\} catch (error) \{
  logger.error('Failed to fetch user', \{ userId, error \});
  throw new UserNotFoundError(userId);
\}

router.get('/api/admin/users', requireAdmin, getUsers);

const CACHE_TTL_SECONDS = 3600;
cache.set(key, value, CACHE_TTL_SECONDS);

---

## Review Process

### 1. Before Reviewing

**Check Context:**
- Read the PR/MR description
- Understand the purpose and scope
- Review linked tickets or issues
- Check CI/CD pipeline status

**Verify Automated Checks:**
- [ ] Tests are passing
- [ ] Linting has no errors
- [ ] Type checking passes
- [ ] Code coverage meets targets
- [ ] No merge conflicts

**Set Aside Time:**
- Small PR (< 200 lines): 15-30 minutes
- Medium PR (200-500 lines): 30-60 minutes
- Large PR (> 500 lines): 1-2 hours (or ask to split)

### 2. During Review

**Follow a Pattern:**

1. **High-Level Review** (5-10 minutes)
   - Read PR description and understand intent
   - Skim all changed files to get overview
   - Verify approach makes sense architecturally
   - Check that changes align with stated purpose

2. **Detailed Review** (20-45 minutes)
   - Line-by-line code review
   - Check logic, edge cases, error handling
   - Verify tests cover new code
   - Look for security vulnerabilities
   - Ensure code follows team conventions

3. **Testing Considerations** (5-10 minutes)
   - Are tests comprehensive?
   - Do tests test the right things?
   - Are edge cases covered?
   - Is test data realistic?

4. **Documentation Check** (5 minutes)
   - Are complex sections commented?
   - Is public API documented?
   - Are breaking changes noted?
   - Is README updated if needed?

### 3. After Reviewing

**Provide Clear Decision:**
- ✅ **Approve**: Code is ready to merge
- 💬 **Comment**: Feedback provided, no action required
- 🔄 **Request Changes**: Issues must be addressed before merge

**Respond to Author:**
- Answer questions promptly
- Re-review after changes made
- Approve when issues resolved
- Thank author for addressing feedback

---

## Review Checklists

### General Code Quality

- [ ] **Readability**: Code is easy to understand
- [ ] **Naming**: Variables and functions have clear, descriptive names
- [ ] **Comments**: Complex logic is explained
- [ ] **Formatting**: Code follows team style guide
- [ ] **DRY**: No unnecessary duplication
- [ ] **SOLID Principles**: Code follows SOLID where applicable
- [ ] **Function Size**: Functions are focused and < 50 lines
- [ ] **Cyclomatic Complexity**: Functions have complexity < 10

### Security

- [ ] **Authentication**: Protected endpoints require auth
- [ ] **Authorization**: Users can only access their own data
- [ ] **Input Sanitization**: SQL injection, XSS prevented
- [ ] **Secrets Management**: No hardcoded credentials or API keys
- [ ] **Encryption**: Sensitive data encrypted at rest and in transit
- [ ] **Rate Limiting**: Endpoints protected from abuse

---

## Quick Start Guide

**For Reviewers:**
1. Read PR description and understand intent
2. Check that automated checks pass
3. Do high-level review (architecture, approach)
4. Do detailed review (logic, edge cases, tests)
5. Use conventional comments for clear communication
6. Provide decision: Approve, Comment, or Request Changes

**For Authors:**
1. Write clear PR description
2. Perform self-review before requesting review
3. Ensure all automated checks pass
4. Keep PR focused and reasonably sized (< 400 lines)
5. Respond to feedback promptly and respectfully
6. Make requested changes or explain reasoning

---

**Skill Version**: 2.0.0
**Last Updated**: 2026-01-08
**Maintained by**: AI Agent Hub Team

## Related Skills

- `ork:architecture-patterns` - Enforce testing and architectural best practices during code review
- `security-scanning` - Automated security checks to complement manual review
- `ork:testing-patterns` - Comprehensive testing patterns to verify during review

## Capability Details

### review-process
**Keywords:** code review, pr review, review process, feedback
**Solves:**
- How to review PRs
- Conventional comments format
- Review best practices

### quality-checks
**Keywords:** readability, solid, dry, complexity, naming
**Solves:**
- Check code quality
- SOLID principles review
- Cyclomatic complexity

### security-review
**Keywords:** security, authentication, authorization, injection, xss
**Solves:**
- Security review checklist
- Find vulnerabilities
- Auth validation

### language-specific
**Keywords:** typescript, python, type hints, async await, pep8
**Solves:**
- TypeScript review
- Python review
- Language-specific patterns

### pr-template
**Keywords:** pr template, pull request, description
**Solves:**
- PR description format
- Review checklist

## Rules

Each category has individual rule files in `rules/` loaded on-demand:

| Category | Rule | Impact | Key Pattern |
|----------|------|--------|-------------|
| TypeScript Quality | `rules/typescript-quality.md` | HIGH | No `any`, Zod validation, exhaustive switches, React 19 |
| Python Quality | `rules/python-quality.md` | HIGH | Pydantic v2, ruff, mypy strict, async timeouts |
| Security Baseline | `rules/security-baseline.md` | CRITICAL | No secrets, auth on endpoints, input validation |
| Linting | `rules/linting-biome-setup.md` | HIGH | Biome setup, ESLint migration, gradual adoption |
| Linting | `rules/linting-biome-rules.md` | HIGH | Biome config, type-aware rules, CI integration |

**Total: 5 rules across 4 categories**

## Available Scripts

- **`scripts/review-pr.md`** - Dynamic PR review with auto-fetched GitHub data
  - Auto-fetches: PR title, author, state, changed files, diff stats, comments count
  - Usage: `/ork:review-pr [PR-number]`
  - Requires: GitHub CLI (`gh`)
  - Uses `$ARGUMENTS` and `!command` for live PR data

- **`assets/review-feedback-template.md`** - Static review feedback template
- **`assets/pr-template.md`** - PR description template


---

## Rules (5)

### Biome Rule Configuration and CI Integration — HIGH


## Biome Rule Configuration and CI Integration

**Incorrect — default config without key rules enabled:**
```json
{
  "linter": { "enabled": true }
  // Missing: noUnusedVariables, noUnusedImports, noExplicitAny
  // Missing: type-aware rules (Biome 2.0+)
}

{
  "$schema": "https://biomejs.dev/schemas/2.0.0/schema.json",
  "formatter": {
    "enabled": true,
    "indentStyle": "space",
    "indentWidth": 2,
    "lineWidth": 100
  },
  "linter": {
    "enabled": true,
    "rules": {
      "recommended": true,
      "correctness": {
        "noUnusedVariables": "error",
        "noUnusedImports": "error"
      },
      "suspicious": {
        "noExplicitAny": "warn"
      },
      "nursery": {
        "noFloatingPromises": "error"
      }
    }
  },
  "javascript": {
    "formatter": {
      "quoteStyle": "single",
      "trailingCommas": "all"
    }
  }
}

# .github/workflows/lint.yml
name: Lint
on: [push, pull_request]
jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - uses: biomejs/setup-biome@v2
      - run: biome ci .

// 4+ config files: .eslintrc, .prettierrc, .prettierignore, .editorconfig
// 127+ npm packages for ESLint + Prettier + plugins
// 3-5s lint time for 10k lines

# Install (single binary, no plugins needed)
npm install --save-dev --save-exact @biomejs/biome

# Initialize config
npx @biomejs/biome init

# Check (lint + format in one command)
npx @biomejs/biome check .

# Fix all auto-fixable issues
npx @biomejs/biome check --write .

# CI mode (strict, fails on errors)
npx @biomejs/biome ci .

# Auto-migrate ESLint configuration
npx @biomejs/biome migrate eslint --write

{
  "overrides": [
    {
      "include": ["*.test.ts", "*.spec.ts"],
      "linter": {
        "rules": {
          "suspicious": { "noExplicitAny": "off" }
        }
      }
    },
    {
      "include": ["legacy/**"],
      "linter": { "enabled": false }
    }
  ]
}

# VIOLATION: No validation on input models
class UserInput(BaseModel):
    email: str      # Accepts any string
    age: int        # Accepts negative numbers

# CORRECT: Constrained fields + validators
from pydantic import BaseModel, Field, model_validator

class UserInput(BaseModel):
    email: str = Field(pattern=r'^[\w\.-]+@[\w\.-]+\.\w+$')
    age: int = Field(ge=0, le=150)

    @model_validator(mode='after')
    def validate_fields(self) -> 'UserInput':
        if self.age < 13 and '@' not in self.email:
            raise ValueError('Minors require valid parent email')
        return self

# VIOLATION: Missing type hints
def process(data):
    result = []
    for item in data:
        result.append(item.name)
    return result

# CORRECT: Full type hints
def process(data: list[UserModel]) -> list[str]:
    result: list[str] = []
    for item in data:
        result.append(item.name)
    return result

# VIOLATION: No timeout on external calls
async def fetch_user(user_id: str) -> User:
    response = await httpx.get(f"/users/{user_id}")
    return User(**response.json())

# CORRECT: Timeout protection
import asyncio

async def fetch_user(user_id: str) -> User:
    async with asyncio.timeout(10):
        response = await httpx.get(f"/users/{user_id}")
        response.raise_for_status()
        return User.model_validate(response.json())

# All Python files must pass:
# ruff check --select ALL
# ruff format --check

# Key rules enforced:
# - No unused imports
# - No f-strings in logging (use % formatting)
# - No bare except clauses
# - No mutable default arguments

class UserInput(BaseModel):
    email: str  # No validation!
    age: int    # Accepts negative

async def fetch_user(user_id: str) -> User:
    # No timeout! May hang forever
    response = await httpx.get(f"/users/{user_id}")
    return User(**response.json())

from pydantic import BaseModel, Field, model_validator
import asyncio

class UserInput(BaseModel):
    email: str = Field(pattern=r'^[\w\.-]+@[\w\.-]+\.\w+$')
    age: int = Field(ge=0, le=150)

    @model_validator(mode='after')
    def validate_fields(self) -> 'UserInput':
        if self.age < 13 and '@' not in self.email:
            raise ValueError('Minors require valid parent email')
        return self

async def fetch_user(user_id: str) -> User:
    async with asyncio.timeout(10):  # Timeout protection
        response = await httpx.get(f"/users/{user_id}")
        response.raise_for_status()
        return User.model_validate(response.json())

# VIOLATION: Secrets in code
API_KEY = "sk-1234567890abcdef"
DB_PASSWORD = "admin123"
JWT_SECRET = "mysecret"

# CORRECT: Environment variables
API_KEY = os.environ["API_KEY"]
DB_PASSWORD = os.environ.get("DB_PASSWORD")

// VIOLATION: Secrets in code
const apiKey = "sk-1234567890abcdef";

// CORRECT: Environment variables
const apiKey = process.env.API_KEY;

# VIOLATION: Unprotected endpoint
@app.get("/api/admin/users")
async def list_users():
    return await db.get_all_users()

# CORRECT: Auth middleware
@app.get("/api/admin/users")
async def list_users(user: User = Depends(require_admin)):
    return await db.get_all_users()

// VIOLATION: No auth
router.get('/api/users', getUsers);

// CORRECT: Auth middleware
router.get('/api/users', requireAuth, getUsers);

# VIOLATION: SQL injection
query = f"SELECT * FROM users WHERE id = {user_id}"

query = "SELECT * FROM users WHERE id = $1"
await db.execute(query, user_id)

// VIOLATION: XSS — raw HTML insertion
element.innerHTML = userInput;

element.textContent = userInput;

# Must run before merge:
npm audit          # JavaScript/TypeScript
pip-audit          # Python

# VIOLATION: Debug code in production
import pdb; pdb.set_trace()
print(f"DEBUG: user password is {password}")
set -x  # In scripts with secrets in scope

# CORRECT: Remove before commit
logger.debug("User authenticated", extra={"user_id": user.id})

# Hardcoded secret
API_KEY = "sk-1234567890abcdef"

# No auth protection
@app.get("/api/admin/users")
async def list_users():
    return await db.get_all_users()

# SQL injection vulnerability
query = f"SELECT * FROM users WHERE id = {user_id}"

# Environment variables
API_KEY = os.environ["API_KEY"]

# Auth middleware
@app.get("/api/admin/users")
async def list_users(user: User = Depends(require_admin)):
    return await db.get_all_users()

# Parameterized query
query = "SELECT * FROM users WHERE id = $1"
await db.execute(query, user_id)

// VIOLATION: any defeats the type system
function processData(data: any) { ... }
const result: any = await fetch(url);

// CORRECT: Use proper types or unknown
function processData(data: UserInput) { ... }
const result: unknown = await fetch(url);

// VIOLATION: Trust the network
const data = await response.json();
const data = await response.json() as User;  // Type assertion, not validation

// CORRECT: Validate at boundary
import { z } from 'zod';
const UserSchema = z.object({
  id: z.string().uuid(),
  email: z.string().email(),
  role: z.enum(['admin', 'user']),
});
const data = UserSchema.parse(await response.json());

// VIOLATION: Non-exhaustive — adding a new status silently falls through
switch (status) {
  case 'active': return 'Active';
  case 'inactive': return 'Inactive';
}

// CORRECT: Compiler catches missing cases
function assertNever(x: never): never {
  throw new Error(`Unexpected value: ${x}`);
}

switch (status) {
  case 'active': return 'Active';
  case 'inactive': return 'Inactive';
  default: return assertNever(status);
}

// REQUIRE: useOptimistic for mutations
const [optimistic, addOptimistic] = useOptimistic(state, reducer);

// REQUIRE: useFormStatus in form submit buttons
const { pending } = useFormStatus();

// REQUIRE: use() for Suspense-aware data fetching
const data = use(promise);

// REQUIRE: Skeleton loading, not spinners
function CardSkeleton() {
  return <div className="animate-pulse">...</div>;
}

// Defeats type system
function processData(data: any) { return data.email; }

// Trust the network - no validation!
const data = await response.json();

// Non-exhaustive switch
switch (status) {
  case 'active': return 'Active';
  case 'inactive': return 'Inactive';
}  // Adding 'pending' silently breaks

import { z } from 'zod';

// Proper types
const UserSchema = z.object({
  id: z.string().uuid(),
  email: z.string().email(),
});
function processData(data: z.infer<typeof UserSchema>) { return data.email; }

// Validate at boundary
const data = UserSchema.parse(await response.json());

// Exhaustive switch with assertNever
function assertNever(x: never): never {
  throw new Error(`Unexpected: ${x}`);
}
switch (status) {
  case 'active': return 'Active';
  case 'inactive': return 'Inactive';
  default: return assertNever(status);  // Compiler catches missing cases
}

issue (blocking): SQL injection risk - sanitize input
suggestion (non-blocking): Consider memoizing this calculation
nitpick (if-minor): Add newline at end of file

LGTM! CI green, change is isolated.

nitpick: Consider adding a test case for edge case X

issue (blocking): Missing error handling in fetchAnalysis()
- What happens if API returns 500?
- Add try/catch and show user-friendly error message

suggestion: Consider extracting validation logic to Zod schema
- Reusable across API + frontend
- Centralized validation rules

praise: Great test coverage! Love the edge case tests.

security (blocking): Multiple OWASP Top 10 violations detected

1. SQL Injection (A03):
   - Line 45: User input concatenated into SQL query
   - Fix: Use parameterized queries (SQLAlchemy already supports this)

2. Broken Access Control (A01):
   - Line 78: No check if user owns this analysis
   - Fix: Add ownership check before allowing update

3. Security Logging Failures (A09):
   - No audit log for data deletion
   - Fix: Log user_id, analysis_id, timestamp to audit table

performance: Potential N+1 query in loop (lines 102-110)
- Fetching chunks individually instead of batch query
- Fix: Use `SELECT WHERE id IN (...)` to fetch all at once
- Expected impact: 50ms → 5ms for 10 chunks

suggestion: Add feature flag for this rollout
- New analysis pipeline is high-risk change
- Allows gradual rollout + quick rollback
- Example: `if feature_flags.is_enabled('new_pipeline', user_id):`

question: How does this handle concurrent updates?
- Two users editing same analysis simultaneously
- Do we need optimistic locking (version field)?

praise: Excellent error messages! These will make debugging much easier.

# BAD: Anyone can delete any analysis
@router.delete("/analyses/{analysis_id}")
async def delete_analysis(analysis_id: int):
    await repo.delete(analysis_id)  # ❌ No auth check!

# GOOD: Check ownership first
@router.delete("/analyses/{analysis_id}")
async def delete_analysis(
    analysis_id: int,
    current_user: User = Depends(get_current_user)
):
    analysis = await repo.get(analysis_id)
    if analysis.user_id != current_user.id:
        raise HTTPException(403, "Not authorized")
    await repo.delete(analysis_id)

# BAD: Password in plaintext
user = User(email=email, password=password)  # ❌

# GOOD: Hash password
from passlib.context import CryptContext
pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto")
user = User(email=email, password_hash=pwd_context.hash(password))

# BAD: SQL injection risk
query = f"SELECT * FROM analyses WHERE url = '{user_url}'"  # ❌
results = db.execute(query)

# GOOD: Parameterized query
query = "SELECT * FROM analyses WHERE url = :url"
results = db.execute(query, {"url": user_url})

# BEST: Use ORM
results = db.query(Analysis).filter(Analysis.url == user_url).all()

<label> (<category>): <subject>

<discussion>

issue (security): SQL injection vulnerability in user lookup.

The `user_id` parameter is concatenated directly into the query.

Instead of:
  cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")

Use parameterized queries:
  cursor.execute("SELECT * FROM users WHERE id = %s", (user_id,))

issue (bug): This will crash when `items` is empty.

`items[0]` throws IndexError. Add a guard:
  if not items:
      return default_value

suggestion (performance): Consider using `dict.get()` for O(1) lookup.

Current loop is O(n) for each check:
  for item in items:
      if item['id'] == target_id: ...

With a dict:
  items_by_id = {item['id']: item for item in items}
  result = items_by_id.get(target_id)

suggestion (readability): Extract this into a well-named function.

This 15-line block calculates shipping cost. A function like
`calculate_shipping_cost(order, destination)` would make the
caller's intent clearer and enable reuse.

nitpick (style): Prefer `is None` over `== None` per PEP 8.

  if value is None:  # ✓
  if value == None:  # ✗

nitpick (naming): `data` is generic. Consider `user_profile` or `response_payload`.

praise: Excellent test coverage! These edge cases would have caught
real bugs. The property-based test for serialization roundtrips is
particularly clever.

praise: This refactor reduced complexity from 15 to 4. Much easier
to reason about now. Great work!

question (design): Why did we choose Redis over PostgreSQL for sessions?

Not blocking, just want to understand the tradeoff for the ADR.

question: Is `timeout=30` intentional? Other endpoints use 60s.

thought: We might want to add rate limiting here eventually.
Not for this PR, but worth a follow-up issue.

This code is bad.

issue (complexity): This function has 6 levels of nesting.
Consider early returns or extracting helper functions.

You need to fix this. This is wrong.

suggestion: Consider using X because Y. What do you think?

def authenticate_request(request):
    token = request.headers.get('Authorization')
    if not token:
        raise AuthError('Missing token')

    payload = jwt.decode(token, SECRET_KEY)  # ← Issue: No algorithm specified
    request.user = payload['user_id']
    return request

🔴 **issue (security):** JWT decode without algorithm is vulnerable to algorithm confusion attacks.

An attacker could forge tokens by switching to 'none' algorithm.

**Suggestion:**
​```python
payload = jwt.decode(token, SECRET_KEY, algorithms=['HS256'])
​```

Reference: https://auth0.com/blog/critical-vulnerabilities-in-json-web-token-libraries/

🟡 **suggestion (error-handling):** Consider catching specific JWT exceptions.

​```python
try:
    payload = jwt.decode(token, SECRET_KEY, algorithms=['HS256'])
except jwt.ExpiredSignatureError:
    raise AuthError('Token expired', code=401)
except jwt.InvalidTokenError:
    raise AuthError('Invalid token', code=401)
​```

This gives users actionable error messages.

🟢 **praise:** Clean separation of auth logic into middleware. This will make testing much easier!

⚪ **nitpick (style):** Consider using `get()` with default for cleaner None check:

​```python
token = request.headers.get('Authorization', '')
if not token:
​```

## Review Summary

Thanks for adding auth! The implementation is clean and well-structured.

### Must Fix (Blocking)
- [ ] Add `algorithms` parameter to `jwt.decode()` (security)

### Should Consider
- [ ] Specific JWT exception handling for better UX

### Nice to Have
- [ ] Minor style improvements

LGTM once the security issue is addressed! 🔐