version: "1.0.3" name: mbc-review description: Review code for MBC CQRS Serverless best practices and anti-patterns. Use this when reviewing code that uses MBC CQRS Serverless framework, checking for common mistakes, or validating implementation patterns.

Pre-flight Check (Version Update)

Before executing this skill, check for updates:

1. Run mbc install-skills --check to check if a newer version is available
2. If the output shows "Update available: X.Y.Z → A.B.C", ask the user:

• "A newer version of MBC skills is available (X.Y.Z → A.B.C). Would you like to update before proceeding?"

1. If the user agrees, run mbc install-skills --force to update
2. If the user declines or skills are up-to-date, proceed with the skill

Note: Skip this check if the user explicitly says to skip updates or if you've already checked in this session.

MBC CQRS Serverless Code Review

This skill reviews code for MBC CQRS Serverless best practices and identifies anti-patterns.

⚠️ Important: Two Anti-Pattern Code Systems

This codebase currently has two independent `AP00X` numbering systems that are NOT interchangeable:

1. Skill `AP` codes (this document) — used in human-facing reviews and documentation. Stable identifiers for discussion.
2. Detector `AP` codes (output of the mbc_check_anti_patterns tool, defined in src/tools/analyze.ts) — used by the static analysis tool. Numbered in detector implementation order.

Only AP016, AP017, AP018, AP019, and AP021 happen to refer to the same concept in both systems. All other AP numbers diverge — for example, the detector's AP005: Hardcoded Tenant corresponds to this document's AP002: Missing tenantCode. Do not assume the codes match.

Cross-reference table (detector → skill doc):

When you receive mbc_check_anti_patterns output, look up the detector code in this table to find the corresponding skill-doc section for full context and recommended fixes. Future versions of this framework should consolidate the two systems; until then, treat them as separate identifier spaces.

Anti-Patterns to Detect

AP001: Using publishSync Instead of publishAsync

Severity: Warning

Pattern:

// Bad
await this.commandService.publishSync(command, options)
// Good
await this.commandService.publishAsync(command, options)

Explanation: publishAsync is the recommended default. Only use publishSync when immediate consistency is absolutely required, as it blocks until processing completes.

AP002: Missing tenantCode in Multi-Tenant Operations

Severity: Error

Pattern:

// Bad
const pk = `ORDER#${code}`
// Good
const { tenantCode } = getUserContext(invokeContext)
const pk = `ORDER#${tenantCode}`

Explanation: All operations must include tenantCode for proper tenant isolation.

AP003: Hardcoded Version Numbers

Severity: Error

Pattern:

// Bad
version: 1,
version: 0,
// Good
version: VERSION_FIRST,  // For new entities (0)
version: existingItem.version,  // For updates

Explanation: Use VERSION_FIRST constant for new entities. For updates, always fetch the current version to enable optimistic locking.

AP004: Missing DataSyncHandler Registration

Severity: Error

Pattern:

// Bad
CommandModule.register({
  tableName: 'order',
  // dataSyncHandlers missing!
}),
// Good
CommandModule.register({
  tableName: 'order',
  dataSyncHandlers: [OrderDataSyncRdsHandler],
}),

Explanation: If you have DataSyncHandlers, they must be registered in the module.

AP005: Not Handling ConditionalCheckFailedException

Severity: Warning

Pattern:

// Bad
await this.commandService.publishAsync(command, options)
// Good
try {
  await this.commandService.publishAsync(command, options)
} catch (error) {
  if (error.name === 'ConditionalCheckFailedException') {
    // Handle version conflict - retry or inform user
    throw new ConflictException('Data was modified by another user')
  }
  throw error
}

Explanation: Optimistic locking can cause version conflicts that should be handled gracefully.

AP006: Using Wrong PK/SK Format

Severity: Error

Pattern:

// Bad - inconsistent format
const pk = `order-${tenantCode}`
const sk = `order_${code}`
// Good - consistent ENTITY#value format
const pk = `ORDER#${tenantCode}`
const sk = `ORDER#${code}`

Explanation: Follow the ENTITY#value convention for partition and sort keys.

AP007: Missing invokeContext in Service Methods

Severity: Error

Pattern:

// Bad
async create(dto: CreateOrderDto) {
  // No way to get user context
}
// Good
async create(dto: CreateOrderDto, invokeContext: IInvoke) {
  const { tenantCode, userId } = getUserContext(invokeContext);
}

Explanation: invokeContext is required to extract user information and tenant context.

AP008: Not Using generateId for Entity IDs

Severity: Warning

Pattern:

// Bad
id: uuid(),
id: `${pk}-${sk}`,
// Good
id: generateId(pk, sk),

Explanation: Use generateId() for consistent ID generation across the framework.

AP009: Missing DTO Validation Decorators

Severity: Warning

Pattern:

// Bad
export class CreateOrderDto {
  code: string
  name: string
}
// Good
export class CreateOrderDto {
  @IsString()
  @IsNotEmpty()
  code: string
  @IsString()
  @IsNotEmpty()
  name: string
}

Explanation: Always use class-validator decorators for input validation.

AP010: Deprecated Method Usage

Severity: Warning

Pattern:

// Bad - deprecated
await this.commandService.publish(command, options)
await this.commandService.publishPartialUpdate(command, options)
// Good - use Async variants
await this.commandService.publishAsync(command, options)
await this.commandService.publishPartialUpdateAsync(command, options)

Explanation: The non-Async methods are deprecated. Use publishAsync and publishPartialUpdateAsync.

AP011: Missing getCommandSource for Tracing

Severity: Warning

Pattern:

// Bad - no source tracking
await this.commandService.publishAsync(command, {
  invokeContext,
})
// Good - with source tracking
const commandSource = getCommandSource(
  basename(__dirname),
  this.constructor.name,
  'create',
)
await this.commandService.publishAsync(command, {
  source: commandSource,
  invokeContext,
})

Explanation: Always include source in publish options for debugging and audit trails.

AP012: Direct DynamoDB Access Instead of DataService

Severity: Warning

Pattern:

// Bad - direct DynamoDB access
import { DynamoDBClient, GetItemCommand } from '@aws-sdk/client-dynamodb'
const client = new DynamoDBClient({})
const result = await client.send(
  new GetItemCommand({
    TableName: 'my-table',
    Key: { pk: { S: 'ORDER#tenant' }, sk: { S: 'ORDER#123' } },
  }),
)
// Good - use DataService
const result = await this.dataService.getItem({ pk, sk })

Explanation: Use DataService for read operations. It provides caching, consistent interfaces, and proper error handling.

AP013: Missing Type Declaration in DataSyncHandler

Severity: Error

Pattern:

// Bad - missing type
@DataSyncHandler({})
export class OrderDataSyncRdsHandler implements IDataSyncHandler {
  // ...
}
// Good - with type declaration
@DataSyncHandler({ type: 'ORDER' })
export class OrderDataSyncRdsHandler implements IDataSyncHandler {
  // ...
}

Explanation: The type property in @DataSyncHandler decorator must match your entity's type field to route events correctly.

AP014: Not Using DetailKey Type

Severity: Info

Pattern:

// Bad - inline key definition
async findOne(pk: string, sk: string) {
  return this.dataService.getItem({ pk, sk });
}
// Good - use DetailKey type
import { DetailKey } from '@mbc-cqrs-serverless/core';
async findOne(key: DetailKey) {
  return this.dataService.getItem(key);
}

Explanation: Use the DetailKey type for consistency and type safety across the application.

AP015: Hardcoded Table Names

Severity: Warning

Pattern:

// Bad - hardcoded table name
CommandModule.register({
  tableName: 'production-orders',
  dataSyncHandlers: [OrderDataSyncRdsHandler],
}),
// Good - use environment variable or configuration
CommandModule.register({
  tableName: process.env.ORDER_TABLE_NAME || 'orders',
  dataSyncHandlers: [OrderDataSyncRdsHandler],
}),

Explanation: Table names should come from environment variables to support multiple environments (dev, staging, production).

AP016: Missing Error Logging

Severity: Warning

Pattern:

// Bad - silently catching errors
try {
  await this.commandService.publishAsync(command, options);
} catch (error) {
  throw new InternalServerErrorException();
}
// Good - log before rethrowing
private readonly logger = new Logger(OrderService.name);
try {
  await this.commandService.publishAsync(command, options);
} catch (error) {
  this.logger.error(`Failed to create order: ${error.message}`, error.stack);
  if (error.name === 'ConditionalCheckFailedException') {
    throw new ConflictException('Data was modified by another user');
  }
  throw error;
}

Explanation: Always log errors with context before handling them for debugging purposes.

AP017: Incorrect Attribute Merging

Severity: Error

Pattern:

// Bad - overwrites all attributes
await this.commandService.publishPartialUpdateAsync(
  {
    pk: key.pk,
    sk: key.sk,
    version: existingItem.version,
    attributes: dto.attributes, // Overwrites existing attributes!
  },
  options,
)
// Good - merge attributes properly
await this.commandService.publishPartialUpdateAsync(
  {
    pk: key.pk,
    sk: key.sk,
    version: existingItem.version,
    attributes: { ...existingItem.attributes, ...dto.attributes },
  },
  options,
)

Explanation: When updating, merge new attributes with existing ones to avoid data loss.

AP018: Missing Swagger Documentation

Severity: Info

Pattern:

// Bad - no documentation
@Controller('orders')
export class OrderController {
  @Post()
  async create(@Body() dto: CreateOrderDto) {}
}
// Good - with Swagger documentation
@ApiTags('orders')
@Controller('orders')
export class OrderController {
  @Post()
  @ApiOperation({ summary: 'Create a new order' })
  @ApiResponse({ status: 201, description: 'Order created successfully' })
  @ApiResponse({ status: 409, description: 'Order already exists' })
  async create(@Body() dto: CreateOrderDto) {}
}

Explanation: Add Swagger decorators for API documentation and better developer experience.

AP019: Not Handling Pagination Correctly

Severity: Warning

Pattern:

// Bad - no pagination support
async search(dto: SearchOrderDto, invokeContext: IInvoke) {
  const { tenantCode } = getUserContext(invokeContext);
  return this.dataService.listByPk({
    pk: `ORDER#${tenantCode}`,
  }); // Returns all items!
}
// Good - with proper pagination
async search(dto: SearchOrderDto, invokeContext: IInvoke) {
  const { tenantCode } = getUserContext(invokeContext);
  return this.dataService.listByPk({
    pk: `ORDER#${tenantCode}`,
    limit: dto.limit || 20,
    cursor: dto.cursor,
  });
}

Explanation: Always implement pagination to avoid returning large datasets and causing performance issues.

AP020: Circular Module Dependencies

Severity: Error

Pattern:

// Bad - circular dependency
// order.module.ts
@Module({
  imports: [ProductModule], // ProductModule imports OrderModule
})
export class OrderModule {}
// product.module.ts
@Module({
  imports: [OrderModule], // Circular!
})
export class ProductModule {}
// Good - use forwardRef or restructure
// order.module.ts
@Module({
  imports: [forwardRef(() => ProductModule)],
})
export class OrderModule {}
// Better - extract shared logic to a common module
@Module({
  imports: [SharedModule],
})
export class OrderModule {}

Explanation: Avoid circular dependencies. Use forwardRef() as a last resort, or better, restructure your modules.

AP021: Emitting Events Directly in XxxCommandService After publishAsync

Severity: Error

Pattern:

// Bad - emit immediately after publishAsync (data table not yet written)
async createOrder(params: CreateOrderParams, context: UserContext): Promise<string> {
  await this.commandService.publishAsync({ pk, sk, ... }, { invokeContext });
  // WRONG: at this point, DataService.getItem() returns null
  // because the data table is populated asynchronously via DynamoDB Streams
  this.eventEmitter.emit('order.created', { orderId, tenantCode, ... });
  return orderId;
}
// Bad - @OnEvent handler tries to read from DataService but finds nothing
@OnEvent('order.created')
async handleOrderCreated(event: OrderCreatedEvent) {
  const order = await this.dataService.getItem({ pk, sk }); // Returns null!
}
// Good - emit inside IDataSyncHandler.up() AFTER data table write
@Injectable()
export class OrderDataSyncHandler implements IDataSyncHandler {
  constructor(private readonly eventEmitter: EventEmitter2) {}
  async up(cmd: CommandModel): Promise<void> {
    if (!cmd.pk.startsWith('ORDER#')) return;
    if (cmd.isDeleted) return;
    if (cmd.version === VERSION_FIRST + 1) {
      // Data table is already written at this point — DataService.getItem() works
      this.eventEmitter.emit('order.created', {
        orderId: cmd.id,
        tenantCode: cmd.tenantCode,
        // ... other fields from cmd.attributes
      });
    }
  }
  async down(cmd: CommandModel): Promise<void> {
    this.eventEmitter.emit('order.deleted', { orderId: cmd.id, tenantCode: cmd.tenantCode });
  }
}

Explanation: publishAsync writes only to the DynamoDB command table. The data table (read by DataService.getItem()) is populated asynchronously via DynamoDB Streams → Step Functions → IDataSyncHandler.up(). Emitting events in XxxCommandService immediately after publishAsync means any @OnEvent handler that calls DataService.getItem() will find no data, causing "entity not found" errors.

The correct pattern is to implement a custom IDataSyncHandler and emit events in up() / down(). By the time these methods run, the data table write has already completed. Register the handler in CommandModule.register({ dataSyncHandlers: [...] }).

If you need to include previous field values for change-detection (e.g., statusChanged), embed them as attributes._prev in the publishAsync call and read them in the handler. Strip _prev from RDS sync handlers to prevent internal metadata from leaking into the database.

Related (v1.2.6+): If you only need read-your-writes consistency for the same user within the same request flow (e.g. POST → immediate GET in the API client), the Repository's RYW session mechanism (when RYW_SESSION_TTL_MINUTES is enabled) is sufficient — it merges the pending command-table write into the next read by the originating user, no event handler required.

The IDataSyncHandler.up() pattern above is still required when other users, background jobs, or cross-module event subscribers need to react to the change — RYW only covers the writer's own subsequent reads.

AP022: Incorrect Group-Based Role Resolver Implementation (v1.3.1+)

Detector counterpart: AP027 (mbc_check_anti_patterns). Note this skill-doc AP022 is unrelated to the detector's AP022 (eval/Function) — see the cross-reference table above.

Problem: Group-based roles (@GroupRoleResolver, custom:groups) are implemented in ways that break bootstrap or leak across requests.

❌ Incorrect — adding `@Injectable()` alongside `@GroupRoleResolver()`:

@GroupRoleResolver()
@Injectable({ scope: Scope.REQUEST }) // ❌ overrides the singleton scope; breaks bootstrap
export class AppGroupRoleResolver implements IGroupRoleResolver {
  /* ... */
}

❌ Incorrect — swallowing resolver errors to "fail open":

async resolveRoles(input) {
  try {
    return await this.lookup(input); // ❌ catching and returning roles on error
  } catch {
    return ['admin']; // ❌ grants access on backend failure
  }
}

❌ Incorrect — more than one resolver, or stateful instance shared across requests:

@GroupRoleResolver() class ResolverA ... // ❌ only one @GroupRoleResolver() per app
@GroupRoleResolver() class ResolverB ... //    (bootstrap throws)

✅ Correct:

import {
  GroupRoleResolver,
  IGroupRoleResolver,
  ResolveGroupRolesInput,
} from '@mbc-cqrs-serverless/core'
// No @Injectable() — @GroupRoleResolver() already registers a singleton provider.
@GroupRoleResolver()
export class AppGroupRoleResolver implements IGroupRoleResolver {
  async resolveRoles({
    tenantCode,
    groupIds,
    claims,
  }: ResolveGroupRolesInput): Promise<string[]> {
    return this.mapGroupsToRoles(tenantCode, groupIds) // stateless; let failures throw
  }
}

Explanation: @GroupRoleResolver() already applies @Injectable() with the default (singleton) scope. Adding another @Injectable() — especially with REQUEST/TRANSIENT scope — overrides it and breaks bootstrap, which resolves a single instance once at startup. Exactly one resolver is allowed per app. The default RolesGuard deliberately rethrows resolver failures so a backend outage surfaces as a 5xx instead of a silent 403; do not catch-and-grant inside the resolver. Use tenantRoles / tenantGroupIds from getUserContext() for direct/group context; do not rely on the deprecated RolesGuard.getUserRole().

AP023: Duplicate DataSyncHandler Registration Across Modules

Detector counterpart: AP028 (mbc_check_anti_patterns).

Severity: High

Pattern:

// Bad — same handler class listed as a provider in multiple modules
// tenant.module.ts  ← correct owner
@Module({
  providers: [TenantConfigRdsHandler],
})
export class TenantModule {}
// agent.module.ts  ← WRONG: registered here too
@Module({
  providers: [TenantConfigRdsHandler],
})
export class AgentModule {}
// Good — register the handler in exactly one module
// tenant.module.ts
@Module({
  providers: [TenantConfigRdsHandler],
})
export class TenantModule {}
// AgentModule must NOT list TenantConfigRdsHandler in its providers.

Explanation: ExplorerService.exploreDataSyncHandlers() scans the entire NestJS ModulesContainer and collects every provider whose class carries the @DataSyncHandler(tableName) decorator — without deduplication by class reference. If the same handler class appears as a provider in N modules, it is collected N times. CommandService.onModuleInit() then resolves each entry via moduleRef.get(handler, { strict: false }) (which always returns the same singleton instance) and pushes it to the internal handler list N times. When a command event arrives, handler.up() is executed N times in parallel inside Promise.all() — causing N concurrent DB writes that produce duplicate rows or P2002 unique-constraint errors.

This is a copy-paste mistake driven by the misconception that "each module must register the handler to pick it up." In reality, ExplorerService scans globally, so a single registration in any one module is sufficient.

Common symptoms:

• RDS records created in duplicate after a single command
• P2002 / unique constraint violations in Step Functions logs
• Alert/notification handlers firing multiple times per event

Fix: Register each @DataSyncHandler class as a provider in exactly one NestJS module — the module that logically owns that handler's concern. No other module should list it in its providers array or in the dataSyncHandlers option of CommandModule.register().

AP024: Reserved DataSyncHandler Type ('dynamodb')

Detector counterpart: AP029 (mbc_check_anti_patterns).

Severity: High

Pattern:

// Bad — 'dynamodb' is reserved for the internal DataSyncDdsHandler
@DataSyncHandler('my-table')
export class TenantConfigRdsHandler implements IDataSyncHandler {
  readonly type = 'dynamodb'  // ← silently excluded from publishSync pipeline
  async up(cmd: CommandModel) { ... }
  async down(cmd: CommandModel) { ... }
}
// Good — use any other string, or omit the type property entirely
@DataSyncHandler('my-table')
export class TenantConfigRdsHandler implements IDataSyncHandler {
  readonly type = 'rds'  // ← included in publishSync pipeline
  async up(cmd: CommandModel) { ... }
  async down(cmd: CommandModel) { ... }
}

Explanation: CommandService.publishSync() filters out handlers whose type is 'dynamodb' before executing the sync pipeline (this.dataSyncHandlers?.filter(h => h.type !== 'dynamodb')). The string 'dynamodb' is the type used exclusively by the internal DataSyncDdsHandler (the handler that writes the Data table). If a custom handler accidentally sets readonly type = 'dynamodb', it is silently excluded — handler.up() is never called, data is never written, and no error or warning is produced.

Common symptoms:

• Custom RDS / OpenSearch sync appears to work (no errors) but data is never written
• Integration tests pass (mock handlers are not filtered) but production sync is missing
• The symptom first appears in publishSync paths; async Step Functions path is unaffected (the async path calls all handlers by name, not by type)

Fix: Remove readonly type = 'dynamodb' from custom handlers and use a different value (e.g., 'rds', 'opensearch', 'custom'). If no type is needed, omit the property — a missing type evaluates to undefined, which satisfies !== 'dynamodb' and is included in the pipeline.

AP025: Fully-Qualified Table Name in @DataSyncHandler

Detector counterpart: AP030 (mbc_check_anti_patterns).

Severity: High

Pattern:

// Bad — fully-qualified DynamoDB table name (with env prefix + -command suffix)
@DataSyncHandler('dev-my-table-command')  // ← wrong: this is the DynamoDB table name
export class MyRdsHandler implements IDataSyncHandler { ... }
// Good — raw table name as passed to CommandModule.register()
@DataSyncHandler('my-table')  // ← correct: matches tableName in CommandModule.register
export class MyRdsHandler implements IDataSyncHandler { ... }
// In the module:
CommandModule.register({ tableName: 'my-table', dataSyncHandlers: [] })

Explanation: @DataSyncHandler(commandTableName) stores commandTableName as Reflect metadata on the class. ExplorerService matches handlers to CommandService instances by comparing this metadata value to the tableName option passed to CommandModule.register(). tableName is the raw logical name (e.g., 'my-table'), not the DynamoDB table name (e.g., 'dev-my-table-command' which the framework builds internally with environment prefix and -command suffix). A mismatch causes ExplorerService to find no match and silently skip the handler — no error, no warning, no data sync.

Common symptoms:

• Custom DataSyncHandler is correctly defined and registered as a provider, but handler.up() is never called
• Data appears to sync correctly for the DynamoDB (Data table) path but the RDS / secondary sync is missing
• No errors in CloudWatch; Step Functions shows SYNC_DATA state with an empty handler list

Fix: Pass the same raw tableName string to @DataSyncHandler that you pass to CommandModule.register({ tableName: '...' }). Do not include environment prefixes (dev-, stg-, prod-) or the -command suffix — those are added internally by the framework when constructing the actual DynamoDB table name.

Review Checklist

When reviewing MBC CQRS Serverless code, check:

Module Structure

• [ ] Module properly imports CommandModule.register()
• [ ] DataSyncHandlers are registered
• [ ] Services and controllers are properly provided

Service Layer

• [ ] All methods receive invokeContext: IInvoke
• [ ] getUserContext() is used to extract tenant info
• [ ] publishAsync is used (not publishSync)
• [ ] VERSION_FIRST is used for new entities
• [ ] Existing version is fetched for updates
• [ ] generateId() is used for ID generation
• [ ] getCommandSource() is used for tracing
• [ ] eventEmitter.emit() is NOT called directly after publishAsync (use IDataSyncHandler instead)

DTOs

• [ ] class-validator decorators are present
• [ ] Swagger decorators for API documentation
• [ ] CommandDto extends the base class

Controller

• [ ] @INVOKE_CONTEXT() decorator is used
• [ ] Proper HTTP methods (POST for create, PUT for update, etc.)
• [ ] API tags and documentation

Error Handling

• [ ] ConditionalCheckFailedException is handled
• [ ] Proper HTTP exceptions are thrown
• [ ] Errors are logged appropriately

Data Sync Handler

• [ ] @DataSyncHandler({ type: 'ENTITY' }) decorator is present (for RDS sync handlers)
• [ ] Implements IDataSyncHandler
• [ ] Handles both create/update and delete cases
• [ ] Registered as a provider in exactly one module — not duplicated across multiple modules (AP023)
• [ ] Event-emitting handlers implement IDataSyncHandler and emit in up() / down()
• [ ] _prev metadata is stripped before RDS upsert if used for change-detection

Notification Transport (when implementing INotificationTransport)

• [ ] Uses @NotificationTransport('transport-name') decorator instead of @Injectable() (AP026)
• [ ] Transport name is registered in NOTIFICATION_TRANSPORTS env var to be active

Group Role Resolver (when implementing IGroupRoleResolver, v1.3.1+) (AP022)

• [ ] Uses @GroupRoleResolver() only — no extra @Injectable() on the same class
• [ ] Resolver is singleton-scoped (no REQUEST/TRANSIENT scope)
• [ ] Exactly one @GroupRoleResolver() per application
• [ ] Resolver is stateless and does NOT catch-and-grant on failure (let errors propagate as 5xx)
• [ ] Registered in the module providers
• [ ] Uses tenantRoles / tenantGroupIds from getUserContext(), not the deprecated RolesGuard.getUserRole()

Output Format

When reviewing, provide output in this format:

## Code Review: [File Name]
 
### Issues Found
 
#### [AP00X] Issue Title
- **Severity:** Error/Warning/Info
- **Location:** Line XX
- **Current Code:**
  ```typescript
  // problematic code

• Suggested Fix:

``typescript // corrected code ``

• Explanation: Why this is an issue

Summary

Recommendations

1. Priority fixes...
2. Suggested improvements...