| name | project-code-review |
| description | Review code according to project standards |
Code review
Structured code review that checks changes for correctness,
type safety,
security,
style,
and maintainability.
Produces actionable findings categorized by severity.
All underlying rules referenced below are defined in AGENTS.
md.
Process
Read the diff or files under review,
then evaluate each category below.
Skip categories that do not apply to the language or change.
Correctness
- Logic errors,
off-by-one,
unhandled edge cases
- Missing null/undefined checks
- Race conditions in async code
- Incorrect use of APIs or library methods
- Broken error propagation (swallowed exceptions,
silent catch blocks)
Off-by-one and boundary errors
const lastItem = items[items.length];
const lastItem = items.at(-1,);
for (let index = 0; index < items.length - 1; index++) { ... }
for (const item of items) { ... }
Missing null/undefined checks
function getUser(id: string,): User {
const user = users.find(candidate => candidate.id === id);
return user;
}
function getUser(id: string,): User {
return notNullishOrThrow(users.find(function matchesId(candidate,) {
return candidate.id === id;
},),);
}
Race conditions
const cache: Map<string, Data> = new Map();
async function getData(key: string,): Promise<Data> {
if (!cache.has(key,)) {
const data = await fetchData(key,);
cache.set(key, data,);
}
return notNullishOrThrow(cache.get(key,),);
}
Broken error propagation
try {
await saveRecord(record,);
}
catch {
}
try {
await saveRecord(record,);
}
catch (error) {
console.error('Failed to save record:', error,);
throw new Error('Failed to save record', { cause: error, },);
}
Type safety (TypeScript)
- Explicit return types on all functions
unknown over any;
no bare Function type
- No non-null assertions (
!);
use assertion functions or narrowing
- Branded types for domain primitives where appropriate
satisfies over as when validating shape without widening
const generic parameters;
readonly array parameters
- Proper discriminated unions;
narrow
typeof === 'symbol' before identity checks
Explicit return types
function parseConfig(raw: string,) {
return JSON.parse(raw,);
}
function parseConfig(raw: string,): Config {
return JSON.parse(raw,) as Config;
}
unknown over any
function processInput(data: any): void { ... }
function runCallback(callback: Function): void { ... }
function processInput(data: unknown): void { ... }
function runCallback(callback: () => void): void { ... }
Domain-specific types
function getUser({ id }: { id: string }): User { ... }
function setPermissions({ level }: { level: number }): void { ... }
type UserId = string & { readonly __brand: 'UserId' };
function getUser({ id }: { id: UserId }): User { ... }
type PermissionLevel = 1 | 2 | 3;
function setPermissions({ level }: { level: PermissionLevel }): void { ... }
type SemVer = `${number}.${number}.${number}`;
function parseVersion({ version }: { version: SemVer }): VersionInfo { ... }
Explicit type annotations and satisfies
const config = { host: 'localhost', port: 8080, } as ServerConfig;
const config: ServerConfig = { host: 'localhost', port: 8080, };
export default { host: 'localhost', port: 8080, } satisfies ServerConfig;
Symbol union narrowing
Flag code that compares a value to a specific symbol and assumes the else branch is non-symbol:
if (out === NO_LITERAL) {
}
else {
use(out.parsed,);
}
if (typeof out === 'symbol') {
if (out === NO_LITERAL) {
}
else {
throw new Error(`Unexpected symbol: ${String(out,)}`,);
}
}
else {
use(out.parsed,);
}
Generics
Flag missing const or readonly modifiers:
function processItems<T extends { id: string; },>(items: T[],): T[];
function processItems<const T extends { id: string; },>(
items: readonly T[],
): readonly T[];
function myFn<const T,>(myArr: T[],): T[];
function myFn<const T,>(myArr: readonly T[],): T[];
Flag non-descriptive generic names:
<T extends Record<string, unknown>>
<TUser extends Record<string, unknown>>
Function signatures
- Functions with 2+ parameters must use a single destructured object parameter (named params)
- No rest parameters (
...args) in functions we control;
accept an array parameter instead
- No
const x = function() {} -- use a function declaration instead
- No calling functions before their declaration in source order
- Exempt:
callbacks whose signature is dictated by an external API or library (e.g.
.map(),
.sort(),
event handlers)
No variable function expressions
Flag function expressions assigned to variables:
const greet = function greet(name: string,): void {};
const greet = function(name: string,): void {};
function greet(name: string,): void {}
Named parameters
Flag any function declaration or named arrow function with 2+ positional parameters:
function clamp(value: number, min: number, max: number,): number {
return Math.max(min, Math.min(max, value,),);
}
function clamp(
{ value, min, max, }: { value: number; min: number; max: number; },
): number {
return Math.max(min, Math.min(max, value,),);
}
function fetchUser(id: string, signal: AbortSignal): Promise<User> { ... }
function fetchUser({ id, signal }: { id: string; signal: AbortSignal }): Promise<User> { ... }
Callbacks passed to external APIs are exempt because the caller dictates the signature:
const doubled = items.map(function doubleByIndex(item, index,) {
return multiply({ value: item, by: index, },);
},);
const sorted = items.sort(function byPriority(left, right,) {
return left.priority - right.priority;
},);
No use before declaration
Flag function calls that appear before the function's declaration in source order.
Hoisting makes this legal at runtime but breaks top-down readability:
const a = b();
function b(): string {
return 'hello';
}
function b(): string {
return 'hello';
}
const a = b();
Rest parameters
Flag rest parameters in functions we control:
function logMessages(...messages: readonly string[]): void { ... }
function logMessages({ messages }: { messages: readonly string[] }): void { ... }
Immutability and style
const over let;
no unnecessary mutation
- Justify or refactor:
any deviation from the preferred pattern must have a comment explaining why refactoring to the preferred approach is not feasible;
if no justification is given,
flag it as WARNING
- Functional patterns (
map/filter/reduce) over imperative loops
for...of when iteration is unavoidable,
never classic for loops
- No single-letter variables (except math formulas)
- Magic numbers/strings extracted to named constants (0,
1,
2,
-1,
-2 exempt);
when a named constant's value is composed from exempt-range arithmetic (e.g.
1 / (2 * 2) for 1/4,
(16 - 2 - 1) / 16 for 13/16),
do not flag the expression as a readability issue
- Extract and name complex conditions
const over let
let baseUrl = 'https://api.example.com';
const baseUrl = 'https://api.example.com';
let total = 0;
for (const item of items)
total += item.price;
const total = items.reduce(function addPrice(sum, item,) {
return sum + item.price;
}, 0,);
Magic numbers and strings
if (retries > 3) { ... }
await wait(5000);
const maxRetries = 3;
const retryDelayMs = 5000;
if (retries > maxRetries) { ... }
await wait(retryDelayMs);
const BORDER_RADIUS = 1 / (2 * 2);
const FONT_SIZE = (16 - 2 - 1) / 16;
for...of over classic for
for (let index = 0; index < items.length; index++)
process(items[index],);
for (const item of items)
process(item,);
Functional over imperative
Flag imperative patterns when functional alternatives exist:
let results = [];
for (let i = 0; i < items.length; i++) {
if (items[i].isActive)
results.push(items[i].value * 2,);
}
const results = items
.filter(function isActive(item,) {
return item.isActive;
},)
.map(function doubleValue(item,) {
return item.value * 2;
},);
Object iteration
Flag for...in loops on objects:
for (const key in obj) {
if (Object.prototype.hasOwnProperty.call(obj, key,))
result[key] = process(obj[key],);
}
Object.entries(obj,).forEach(function applyProcess([key, value,],) {
result[key] = process(value,);
},);
const result = Object.fromEntries(
Object.entries(obj,).map(function processEntry([key, value,],) {
return [key, process(value,),];
},),
);
No switch statements
Flag all switch statements.
Use if/else chains for branching or Record lookups for mapping a discriminant to a value.
switch adds break boilerplate,
risks accidental fallthrough,
and encourages large blocks.
If/else works naturally with early returns and needs no special syntax.
When the logic is a pure mapping from key to value,
a Record is more declarative and type-safe.
switch (toolName) {
case 'Read': {
return `Reading ${shortPath(filePath,)}`;
}
case 'Edit': {
return `Editing ${shortPath(filePath,)}`;
}
case 'Bash': {
return `Running ${shortCommand(command,)}`;
}
default: {
return toolName;
}
}
if (toolName === 'Read')
return `Reading ${shortPath(filePath,)}`;
else if (toolName === 'Edit')
return `Editing ${shortPath(filePath,)}`;
else if (toolName === 'Bash')
return `Running ${shortCommand(command,)}`;
else
return toolName;
const TOOL_LABELS: Record<string, (input: ToolInput,) => string> = {
Read: function readLabel({ filePath, },) {
return `Reading ${shortPath(filePath,)}`;
},
Edit: function editLabel({ filePath, },) {
return `Editing ${shortPath(filePath,)}`;
},
Bash: function bashLabel({ command, },) {
return `Running ${shortCommand(command,)}`;
},
};
const labelFn = TOOL_LABELS[toolName];
return labelFn !== undefined ? labelFn(input,) : toolName;
Naming extracted concepts
Flag inline complex conditions:
if (status === 'pending' && retries < maxRetries && !isTimeout) {
}
function canRetry(): boolean {
return status === 'pending' && retries < maxRetries && !isTimeout;
}
if (canRetry()) {
}
Single-letter variables
for (let i = 0; i < items.length; i++)
for (let itemIndex = 0; itemIndex < items.length; itemIndex++)
items.forEach(function processItem(item, itemIndex) { ... })
Security
- No hardcoded secrets,
API keys,
or credentials
- No unsanitized user input in SQL,
shell commands,
or HTML
- No overly permissive CORS,
file permissions,
or network exposure
- Secrets not logged,
even at debug level
Hardcoded secrets
const apiKey = 'sk-live-abc123def456';
const apiKey = notNullishOrThrow(process.env['API_KEY'],);
Unsanitized user input
const output = execSync(`grep ${userQuery} /var/log/app.log`,);
const output = execSync('grep', [userQuery, '/var/log/app.log',],);
const rows = db.query(`SELECT * FROM users WHERE name = '${name}'`,);
const rows = db.query('SELECT * FROM users WHERE name = ?', [name,],);
Secrets in logs
console.log('Authenticating with token:', token,);
const tokenPrefixLength = 4;
console.log('Authenticating with token:',
token.slice(0, tokenPrefixLength,) + '...',);
Naming and readability
- Semantic,
descriptive names for variables,
functions,
types
- Comments explain WHY,
not WHAT
- TSDoc on all declarations (functions,
types,
constants,
classes -- including locals inside function bodies) with
@param,
@returns,
@example;
do not skip declarations that seem "obvious from context"
@throws only on functions that actually throw;
do not add @throws to functions that never throw
- Comments on their own line above code,
never inline after code
- No narrative or promotional language in docs
- Region markers for substantial code blocks;
flag missing markers as WARNING for substantial blocks and NIT for smaller ones
Comments inside template literals
Embed comments inside template literals using the ${''} trick.
Do not use target-language comments (XML,
HTML,
etc.) or move the comment outside the template.
Reasons:
- Target-language comments require context switching between JS and the target language
- Editors cannot properly highlight target-language comments inside JS template literals
- Target-language comment syntax is easy to get wrong or forget across different languages
const xml = `
<os><type arch='x86_64'>hvm</type></os>
<!-- Guest CPU requires AVX -->
<cpu mode='host-passthrough'/>
`;
const xml = `
<os><type arch='x86_64'>hvm</type></os>
<cpu mode='host-passthrough'/>
`;
const xml = `
<os><type arch='x86_64'>hvm</type></os>
${
// Guest CPU requires AVX
''}
<cpu mode='host-passthrough'/>
`;
TSDoc quality
Flag WHY-less comments:
Flag TSDoc on non-declarations:
counter++;
counter++;
Escaping block comment terminators
Flag unescaped */ inside TSDoc blocks:
"
*/
// Good
/**
* Returns a string like "
Async patterns
async/await over raw promises or callbacks
- No
.then(),
.catch(),
.finally() -- use async/await with try/catch or let errors propagate naturally by throwing
Promise.all for independent concurrent operations
Promise.allSettled when partial failure is acceptable
- No
await in loops without justification and eslint-disable comment
AbortController for cancellable operations
- Streams and subprocesses properly consumed or cleaned up
No .then()/.catch()/.finally()
function loadConfig(): Promise<Config> {
return readFile('config.json', 'utf8',)
.then(raw => JSON.parse(raw,) as Config)
.catch(error => {
console.error('Failed:', error,);
throw error;
},);
}
async function loadConfig(): Promise<Config> {
const raw = await readFile('config.json', 'utf8',);
return JSON.parse(raw,) as Config;
}
No await in loops
for (const url of urls) {
const response = await fetch(url,);
results.push(await response.json(),);
}
async function fetchJson(url: string,): Promise<unknown> {
const response = await fetch(url,);
return response.json();
}
const results = await Promise.all(urls.map(fetchJson,),);
Manual promise creation
Flag explicit new Promise when utilities exist:
function delay(ms,) {
return new Promise(function resolveAfterDelay(resolve,) {
setTimeout(resolve, ms,);
},);
}
import { wait, } from '@monochromatic-dev/module-es';
Imports and modules
- Grouped:
builtins,
external deps,
workspace packages,
relative,
type-only
- Named imports over default imports
import type for type-only imports
- File extensions in relative imports when required by config
- No circular dependencies
Import grouping and style
import { readFile, } from 'node:fs/promises';
import { z, } from 'zod';
import config from './config';
import { Config, } from './types';
import { readFile, } from 'node:fs/promises';
import { z, } from 'zod';
import { parseConfig, } from '@monochromatic-dev/module-es';
import { loadSettings, } from './config.ts';
import type { Config, } from './types.ts';
Error handling
- Prefer letting errors propagate by throwing
- No
try...finally -- use using/await using with Symbol.dispose/Symbol.asyncDispose for cleanup
- Custom error classes extending
Error for domain errors
- No
process.exit();
throw instead
- Multi-line error messages use
outdent from @cspotcode/outdent
@throws in TSDoc only for functions that actually throw;
flag @throws on non-throwing functions as NIT
notNullishOrThrow instead of non-null assertion (!)
- Every catch block must log the error with
console.error()
using over try...finally
const handle = openResource();
try {
await process(handle,);
}
finally {
handle.close();
}
await using handle = openResource();
await process(handle,);
No process.exit()
if (!isValid) {
console.error('Invalid input',);
process.exit(1,);
}
if (!isValid)
throw new Error('Invalid input',);
outdent for multi-line error messages
throw new Error(
'Failed to process record.\n'
+ `Expected type: ${expectedType}\n`
+ `Received type: ${receivedType}`,
);
import { outdent, } from '@cspotcode/outdent';
throw new Error(outdent`
Failed to process record.
Expected type: ${expectedType}
Received type: ${receivedType}
`,);
Custom error classes
class ValidationError extends Error {
constructor(
message: string,
public readonly field: string,
) {
super(message,);
this.name = 'ValidationError';
}
}
Type guards
function isString(value: unknown,): value is string {
return typeof value === 'string';
}
Assertion functions
function assertIsString(value: unknown,): asserts value is string {
if (typeof value !== 'string')
throw new TypeError('Expected string',);
}
notNullishOrThrow over non-null assertion
const value = possiblyUndefined!;
import { notNullishOrThrow, } from '@monochromatic-dev/module-es';
const value = notNullishOrThrow(possiblyUndefined,);
Silent error handling
Flag empty catch blocks without logging:
catch (error) { }
catch (error) {
throw new Error('Failed to get index stats', { cause: error });
}
catch (error) {
console.error('Failed to get index stats:', error);
}
Flag silently discarded unexpected states:
if (!(event instanceof CustomEvent))
return;
if (!(event instanceof CustomEvent))
throw new TypeError('Expected CustomEvent',);
Markdown quality
- No tables in markdown files -- use nested headings or lists instead
- Flag any existing tables in changed markdown files as WARNING with a suggestion to convert
Tables to lists
<!-- Bad -- flag as WARNING -->
| Name | Type | Default |
| ---- | ------ | --------- |
| host | string | localhost |
| port | number | 8080 |
<!-- Good -->
- host -- `string`, default `localhost`
- port -- `number`, default `8080`
Testing gaps
- New logic paths missing corresponding test cases
- Edge cases not covered (empty input,
boundary values,
error paths)
- Mocking that hides real integration issues
- Flaky patterns (timing dependencies,
shared mutable state)
Commit messages
When reviewing PRs or multi-commit bundles,
also review commit messages.
Format
Conventional Commits:
<type>(<scope>): <subject> with optional body and footer.
Types:
feat,
fix,
docs,
style,
refactor,
perf,
test,
build,
ci,
chore,
revert.
Scope:
package name or * for multi-package changes.
Single type and scope
feat(module-es): add a
Description of a
Multiple types and/or scopes
feat(module-es): enhance error handling utilities
error.assert.throw: add assertion-based error throwing
- Implement conditional error throwing based on assertions
- Include TypeScript type narrowing support
error.throw: add unified error throwing utility
- Implement consistent error creation
- Provide better stack traces
- Include custom error types
test(module-es): achieve 100% coverage for error utilities
- Add comprehensive test cases
- Use parameterized tests for edge cases
- Ensure proper type inference testing
Rules
- Group related changes by type (feat,
fix,
test,
etc.)
- Do not mix different types in the same scope section
- Be specific about what changed,
not just which files
- Focus on "what" and "why"
- Flag partial commit messages (describing only some changes) as WARNING
CSS quality
When changes include CSS,
check:
- Native platform features over JS reimplementations (
<dialog>,
Popover API,
CSS nesting)
rem for all sizing;
calc() for derivation;
no px except device-pixel contexts
- Logical properties everywhere (no physical
left/right/top/bottom)
- No shorthand properties that combine unrelated axes or sub-properties;
single-axis/single-concept shorthands are fine (
padding-inline,
margin-block,
border-radius,
inset,
gap)
- Colors via CSS custom properties;
no
var() fallbacks (exception:
user-configurable)
- No
!important
:focus-visible on interactive elements;
48px minimum touch targets
- Shallow native nesting (3 levels max)
- Data attributes for state/variant styling,
not BEM modifiers
Logical properties
margin-left: 1rem;
padding-top: 0.5rem;
text-align: left;
top: 0;
right: 0;
margin-inline-start: 1rem;
padding-block-start: 0.5rem;
text-align: start;
inset-block-start: 0;
inset-inline-end: 0;
rem sizing
font-size: 14px;
padding-block: 8px;
border-radius: 4px;
font-size: calc(14 / 16 * 1rem);
padding-block: calc(8 / 16 * 1rem);
border-radius: calc(4 / 16 * 1rem);
:focus-visible and touch targets
button:focus {
outline-color: var(--focus-ring);
}
button {
min-inline-size: 2rem;
min-block-size: 2rem;
}
button {
min-inline-size: 3rem;
min-block-size: 3rem;
&:focus-visible {
outline-color: var(--focus-ring);
}
}
Shallow nesting
.card {
& .header {
& .title {
& span {
color: var(--accent-fg);
}
}
}
}
.card {
.title {
& span {
color: var(--accent-fg);
}
}
}
Shorthand properties
border: 1px solid #111;
padding: 0.5rem 1rem;
margin: 0 auto;
background: #fff url(...) no-repeat center;
border-width: calc(1 / 16 * 1rem);
border-style: solid;
border-color: var(--gray-fg);
padding-block: 0.5rem;
padding-inline: 1rem;
margin-block: 0;
margin-inline: auto;
border-radius: 0.25rem;
inset: 0;
gap: 1rem;
Color tokens
color: #111;
border-color: #a00;
color: var(--gray-fg);
border-color: var(--error-fg);
State styling
<span class="pill pill--loading">
<span class="pill" data-loading>
.pill--loading {
opacity: 0.5;
}
.pill {
&[data-loading] {
opacity: 0.5;
}
}
Logging
- All loggers must use the
tagged wrapper;
no raw console.* or untagged logger instances
- Tags at every function/module boundary using
myFn.name or subsystem name
- Composed tags for nested calls -- pass
tagged({ tag, l }) down the call chain
- No manual tag prefixes in message strings
Untagged logger
export const l: Logger = $;
export const l: Logger = tagged({ tag: 'rss', },);
Manual tag in message string
l.info('[cycle] capture complete',);
const l = tagged({ tag: 'cycle', l: parentLogger, },);
l.info('capture complete',);
Shallow tagging
function processItem({ item, l, }: { item: Item; l: Logger; },): void {
l.info('processing',);
transformItem({ item, l, },);
}
function processItem(
{ item, l: parentLogger, }: { item: Item; l: Logger; },
): void {
const l = tagged({ tag: processItem.name, l: parentLogger, },);
l.info('processing',);
transformItem({ item, l, },);
}
Script conventions
- No bash/shell scripts -- TypeScript only,
executed with Node
- Top-level code,
no
main() wrapper;
top-level await for async
- No
process.exit() -- throw errors instead
No main() wrapper
async function main(): Promise<void> {
const data = await loadData();
console.log(data,);
}
main();
const data = await loadData();
console.log(data,);
Region markers
Flag missing //region///endregion markers on substantial code blocks:
function loginUser(credentials: UserCredentials,): UserSession {
return {} as UserSession;
}
function registerUser(details: UserDetails,): UserProfile {
return {} as UserProfile;
}
Output format
Summary: <one-line description of the change and overall assessment>
Findings:
BLOCKER:
- <file:line> <description and suggested action>
WARNING:
- <file:line> <description and suggested action>
NIT:
- <file:line> <description and suggested action>
NON-ACTIONABLE:
- <file:line> <description and best-effort ideas>
Omit empty severity sections.
If no issues found,
write "No issues found" under Findings.
Every item in BLOCKER,
WARNING,
and NIT must include a concrete suggested action.
If you spot a real problem but cannot determine a specific fix,
place it under NON-ACTIONABLE with best-effort ideas -- still report it,
just be honest about uncertainty.