name	code-review
description	Automated PR review checklist covering security, performance, architecture, and code quality. Use when reviewing pull requests, examining code changes, or performing code quality assessments.

Code Review Skill

Use the Quick checklist template for a short paste into a PR description. Everything from Review process through Approval criteria is the full deep checklist.

Quick checklist template

## Security Review
- [ ] No hardcoded secrets or tokens
- [ ] Input validation present
- [ ] Error handling secure (no sensitive data in logs)

## Correctness Review  
- [ ] Logic correctly implemented
- [ ] Edge cases handled
- [ ] Error scenarios covered
- [ ] Async/await chains correct

## Architecture Review
- [ ] Proper code organization
- [ ] Design patterns followed
- [ ] Good modularity
- [ ] No circular dependencies

## Performance Review
- [ ] Efficient implementation
- [ ] No unnecessary API calls
- [ ] Memory leaks avoided
- [ ] Concurrency handled correctly

## Testing Review
- [ ] Adequate test coverage (80%+)
- [ ] Quality tests (not just passing)
- [ ] TDD compliance
- [ ] Both success and failure paths tested

## Code Conventions
- [ ] TypeScript strict mode
- [ ] Consistent naming conventions
- [ ] No unused imports or variables
- [ ] Documentation adequate

## Monorepo Checks
- [ ] Cross-package imports use published names
- [ ] Workspace dependencies declared correctly
- [ ] OCLIF manifest updated if commands changed
- [ ] No breaking changes to exported APIs

Review process

Severity Levels

🔴 Critical (must fix before merge):
- Security vulnerabilities
- Logic errors causing incorrect behavior
- Breaking API changes
- Hardcoded secrets or credentials
- Data loss scenarios
🟡 Important (should fix):
- Performance regressions
- Code maintainability issues
- Missing error handling
- Test coverage gaps
- Best practice violations
🟢 Suggestion (consider improving):
- Code readability improvements
- Minor optimizations
- Documentation enhancements
- Style inconsistencies

Security Review

Secrets and Credentials

No hardcoded API keys, tokens, or passwords
No secrets in environment variables exposed in logs
No secrets in test data or fixtures
Sensitive data not logged or exposed in error messages
Config files don't contain real credentials

Input Validation

// ✅ GOOD - Validate all user input
if (!region || typeof region !== 'string') {
  throw new CLIError('Region must be a non-empty string');
}

if (!['us', 'eu', 'au'].includes(region)) {
  throw new CLIError('Invalid region specified');
}

// ❌ BAD - No validation
const result = await client.setRegion(region);

Error Handling

No sensitive data in error messages
Stack traces don't leak system information
Error messages are user-friendly
All errors properly caught and handled
No raw error objects exposed to users

Authentication

Correctness Review

Logic Validation

Business logic correctly implemented
Algorithm is correct for the problem
State transitions valid
Data types used correctly
Comparisons use correct operators (=== not ==)

Error Scenarios

API failures handled (404, 429, 500, etc.)
Network timeouts managed
Empty/null responses handled
Invalid input rejected
Partial failures handled gracefully

Async/Await and Promises

// ✅ GOOD - Proper async handling
async run(): Promise<void> {
  try {
    const result = await this.fetchData();
    await this.processData(result);
  } catch (error) {
    handleAndLogError(error, this.contextDetails);
  }
}

// ❌ BAD - Missing await
async run(): Promise<void> {
  const result = this.fetchData(); // Missing await!
  await this.processData(result);
}

Data Integrity

Architecture Review

Code Organization

Classes/functions have single responsibility
No circular dependencies
Proper module structure
Clear separation of concerns
Commands delegate to services/utils

Design Patterns

// ✅ GOOD - Service layer separation
export default class MyCommand extends BaseCommand {
  async run(): Promise<void> {
    const service = new MyService(this.contextDetails);
    const result = await service.execute();
  }
}

// ❌ BAD - Logic in command
export default class MyCommand extends BaseCommand {
  async run(): Promise<void> {
    const data = await client.fetch();
    for (const item of data) {
      // Complex business logic here
    }
  }
}

Modularity

Type Safety

TypeScript strict mode enabled
No any types without justification
Interfaces defined for contracts
Generic types used appropriately
Type narrowing correct

Performance Review

API Calls

Rate limiting respected (10 req/sec for Contentstack)
No unnecessary API calls in loops
Pagination implemented for large datasets
Caching used where appropriate
Batch operations considered

Memory Management

No memory leaks in event listeners
Large arrays streamed not loaded fully
Cleanup in try/finally blocks
File handles properly closed
Resources released after use

Concurrency

// ✅ GOOD - Controlled concurrency
const results = await Promise.all(
  items.map(item => processItem(item))
);

// ❌ BAD - Unbounded concurrency
for (const item of items) {
  promises.push(processItem(item)); // No limit!
}

Efficiency

Algorithms are efficient for scale
String concatenation uses proper methods
Unnecessary computations avoided
Data structures appropriate
No repeated lookups without caching

Testing Review

Coverage

Test Quality

// ✅ GOOD - Quality test
it('should throw error when region is empty', () => {
  expect(() => validateRegion(''))
    .to.throw('Region is required');
});

// ❌ BAD - Weak test
it('should validate region', () => {
  validateRegion('us'); // No assertion!
});

Testing Patterns

TDD Compliance

Tests written before implementation
All tests pass
Code coverage requirements met
Tests are maintainable
Both success and failure paths covered

Code Conventions

TypeScript Standards

strict: true in tsconfig
No any types (use unknown if needed)
Proper null/undefined handling
Type annotations on public APIs
Generics used appropriately

Naming Conventions

Classes: PascalCase (MyClass)
Functions: camelCase (myFunction)
Constants: UPPER_SNAKE_CASE (MY_CONST)
Booleans start with verb (isActive, hasData)
Descriptive names (not x, temp, data)

Code Style

No unused imports
No unused variables
Proper indentation (consistent spacing)
Line length reasonable (<100 chars)
Imports organized (group by type)

Documentation

Complex logic documented
Public APIs have JSDoc comments
Non-obvious decisions explained
Examples provided where helpful
README updated if needed

Monorepo-Specific Checks

Cross-Package Imports

// ✅ GOOD - Use published package names
import { configHandler } from '@contentstack/cli-utilities';
import { BaseCommand } from '@contentstack/cli-command';

// ❌ BAD - Relative paths across packages
import { configHandler } from '../../../contentstack-utilities/src';

Workspace Dependencies

Dependencies declared in correct package.json
Versions consistent across packages
No circular dependencies between packages
Shared deps in root if used by multiple packages
No installing globally when local version exists

OCLIF Configuration

Command file paths correct in package.json
OCLIF manifest regenerated (pnpm build && pnpm oclif manifest)
New commands registered in plugin list if needed
Topic separators consistent (:)
Command names follow pattern

Build and Publishing

TypeScript compiles without errors
Build output in lib/ directory
No build artifacts committed
ESLint passes without warnings
Tests pass in CI environment

Tsgen plugin (`contentstack-cli-tsgen`)

Delivery-token wording correct (not CMA-centric); GraphQL paths require delivery token
Integration coverage for new flags when behavior is user-facing
See package code-review skill

Common Issues to Look For

Security

Config with default passwords
API keys in client-side code
SQL injection (N/A here, but parameterization pattern)
XSS in CLI output (escape HTML if needed)

Performance

API calls in loops
Synchronous file I/O on large files
Memory not released properly
Rate limiting ignored

Logic

Off-by-one errors in loops
Wrong comparison operators
Async/await chains broken
Null/undefined not handled

Testing

Tests pass locally but fail in CI
Mocks not properly restored
Race conditions in tests
Hardcoded timeouts

Approval criteria

APPROVE when:

✅ All 🔴 Critical items addressed
✅ Most 🟡 Important items addressed (document exceptions)
✅ Code follows team conventions
✅ Tests pass and coverage sufficient
✅ Monorepo integrity maintained

REQUEST CHANGES when:

❌ Any 🔴 Critical issues present
❌ Multiple 🟡 Important issues unaddressed
❌ Test coverage below 80%
❌ Architecture concerns not resolved
❌ Breaking changes not documented

COMMENT for:

💬 🟢 Suggestions (non-blocking)
💬 Questions about implementation
💬 Appreciation for good patterns

Additional PR review notes (former .cursor/commands/code-review.md)

Module organization: Proper separation of concerns

Review Execution

Automated Checks

Lint compliance: ESLint checks for code style
TypeScript compiler: Successful compilation to lib/ directories
Test execution: All tests pass successfully
Build verification: Build scripts complete without errors

Manual Review Focus Areas

Command usability: Clear help text and realistic examples
Error handling: Appropriate error messages and recovery options
Test quality: Comprehensive test coverage for critical paths
Monorepo consistency: Consistent patterns across all packages
Flag design: Intuitive flag names and combinations

Common Issues to Flag

Inconsistent TypeScript settings: Mixed strict mode without reason
Real API calls in tests: Unmocked external dependencies
Missing error handling: Commands that fail silently
Poor test organization: Tests without clear Arrange-Act-Assert
Build artifacts committed: lib/ directories in version control
Unclear error messages: Non-actionable error descriptions
Inconsistent flag naming: Similar flags with different names
Missing command examples: Examples not showing actual usage

Repository-Specific Checklist

For Modularized CLI

Before Merge

All review items addressed
No build artifacts in commit
Tests added for new functionality
Documentation updated if needed
No console.log() statements (use log.debug instead)
Error messages are user-friendly

FilesExpand file tree

SKILL.md

Latest commit

History