diff --git a/resources/global/en/knowledge/.gitkeep b/resources/global/en/knowledge/.gitkeep deleted file mode 100644 index e69de29..0000000 diff --git a/resources/global/en/knowledge/architecture.md b/resources/global/en/knowledge/architecture.md new file mode 100644 index 0000000..04eccfb --- /dev/null +++ b/resources/global/en/knowledge/architecture.md @@ -0,0 +1,427 @@ +# Architecture Knowledge + +## Structure & Design + +**File Organization:** + +| Criteria | Judgment | +|----------|----------| +| Single file > 200 lines | Consider splitting | +| Single file > 300 lines | REJECT | +| Single file with multiple responsibilities | REJECT | +| Unrelated code coexisting | REJECT | + +**Module Structure:** + +- High cohesion: Related functionality grouped together +- Low coupling: Minimal inter-module dependencies +- No circular dependencies +- Appropriate directory hierarchy + +**Function Design:** + +- One responsibility per function +- Consider splitting functions over 30 lines +- Side effects clearly defined + +**Layer Design:** + +- Dependency direction: Upper layers -> Lower layers (reverse prohibited) +- Controller -> Service -> Repository flow maintained +- 1 interface = 1 responsibility (no giant Service classes) + +**Directory Structure:** + +Structure pattern selection: + +| Pattern | Use Case | Example | +|---------|----------|---------| +| Layered | Small scale, CRUD-centric | `controllers/`, `services/`, `repositories/` | +| Vertical Slice | Medium-large scale, high feature independence | `features/auth/`, `features/order/` | +| Hybrid | Common foundation + feature modules | `core/` + `features/` | + +Vertical Slice Architecture (organizing code by feature): + +``` +src/ +├── features/ +│ ├── auth/ +│ │ ├── LoginCommand.ts +│ │ ├── LoginHandler.ts +│ │ ├── AuthRepository.ts +│ │ └── auth.test.ts +│ └── order/ +│ ├── CreateOrderCommand.ts +│ ├── CreateOrderHandler.ts +│ └── ... +└── shared/ # Shared across features + ├── database/ + └── middleware/ +``` + +Vertical Slice criteria: + +| Criteria | Judgment | +|----------|----------| +| Single feature spans 3+ layers | Consider slicing | +| Minimal inter-feature dependencies | Recommend slicing | +| Over 50% shared processing | Keep layered | +| Team organized by features | Slicing required | + +Prohibited patterns: + +| Pattern | Problem | +|---------|---------| +| Bloated `utils/` | Becomes graveyard of unclear responsibilities | +| Lazy placement in `common/` | Dependencies become unclear | +| Excessive nesting (4+ levels) | Navigation difficulty | +| Mixed features and layers | `features/services/` prohibited | + +**Separation of Concerns:** + +- Read and write responsibilities separated +- Data fetching at root (View/Controller), passed to children +- Error handling centralized (no try-catch scattered everywhere) +- Business logic not leaking into Controller/View + +## Code Quality Detection + +**Explanatory Comment (What/How) Detection Criteria:** + +Detect comments that simply restate code behavior in natural language. + +| Judgment | Criteria | +|----------|----------| +| REJECT | Restates code behavior in natural language | +| REJECT | Repeats what is already obvious from function/variable names | +| REJECT | JSDoc that only paraphrases the function name without adding information | +| OK | Explains why a particular implementation was chosen | +| OK | Explains the reason behind seemingly unusual behavior | +| Best | No comment needed — the code itself communicates intent | + +```typescript +// REJECT - Restates code (What) +// If interrupted, abort immediately +if (status === 'interrupted') { + return ABORT_STEP; +} + +// REJECT - Restates the loop +// Check transitions in order +for (const transition of step.transitions) { + +// REJECT - Repeats the function name +/** Check if status matches transition condition. */ +export function matchesCondition(status: Status, condition: TransitionCondition): boolean { + +// OK - Design decision (Why) +// User interruption takes priority over piece-defined transitions +if (status === 'interrupted') { + return ABORT_STEP; +} + +// OK - Reason behind seemingly odd behavior +// stay can cause loops, but is only used when explicitly specified by the user +return step.name; +``` + +**Direct State Mutation Detection Criteria:** + +Detect direct mutation of arrays or objects. + +```typescript +// REJECT - Direct array mutation +const steps: Step[] = getSteps(); +steps.push(newStep); // Mutates original array +steps.splice(index, 1); // Mutates original array +steps[0].status = 'done'; // Nested object also mutated directly + +// OK - Immutable operations +const withNew = [...steps, newStep]; +const without = steps.filter((_, i) => i !== index); +const updated = steps.map((s, i) => + i === 0 ? { ...s, status: 'done' } : s +); + +// REJECT - Direct object mutation +function updateConfig(config: Config) { + config.logLevel = 'debug'; // Mutates argument directly + config.steps.push(newStep); // Nested mutation too + return config; +} + +// OK - Returns new object +function updateConfig(config: Config): Config { + return { + ...config, + logLevel: 'debug', + steps: [...config.steps, newStep], + }; +} +``` + +## Security (Basic Checks) + +- Injection prevention (SQL, Command, XSS) +- User input validation +- Hardcoded sensitive information + +## Testability + +- Dependency injection enabled +- Mockable design +- Tests are written + +## Anti-Pattern Detection + +REJECT when these patterns are found: + +| Anti-Pattern | Problem | +|--------------|---------| +| God Class/Component | Single class with too many responsibilities | +| Feature Envy | Frequently accessing other modules' data | +| Shotgun Surgery | Single change ripples across multiple files | +| Over-generalization | Variants and extension points not currently needed | +| Hidden Dependencies | Child components implicitly calling APIs etc. | +| Non-idiomatic | Custom implementation ignoring language/FW conventions | + +## Abstraction Level Evaluation + +**Conditional Branch Proliferation Detection:** + +| Pattern | Judgment | +|---------|----------| +| Same if-else pattern in 3+ places | Abstract with polymorphism → REJECT | +| switch/case with 5+ branches | Consider Strategy/Map pattern | +| Flag arguments changing behavior | Split into separate functions → REJECT | +| Type-based branching (instanceof/typeof) | Replace with polymorphism → REJECT | +| Nested conditionals (3+ levels) | Early return or extract → REJECT | + +**Abstraction Level Mismatch Detection:** + +| Pattern | Problem | Fix | +|---------|---------|-----| +| Low-level details in high-level processing | Hard to read | Extract details to functions | +| Mixed abstraction levels in one function | Cognitive load | Align to same granularity | +| DB operations mixed with business logic | Responsibility violation | Separate to Repository layer | +| Config values mixed with processing logic | Hard to change | Externalize configuration | + +**Good Abstraction Examples:** + +```typescript +// Proliferating conditionals +function process(type: string) { + if (type === 'A') { /* process A */ } + else if (type === 'B') { /* process B */ } + else if (type === 'C') { /* process C */ } + // ...continues +} + +// Abstract with Map pattern +const processors: Record void> = { + A: processA, + B: processB, + C: processC, +}; +function process(type: string) { + processors[type]?.(); +} +``` + +```typescript +// Mixed abstraction levels +function createUser(data: UserData) { + // High level: business logic + validateUser(data); + // Low level: DB operation details + const conn = await pool.getConnection(); + await conn.query('INSERT INTO users...'); + conn.release(); +} + +// Aligned abstraction levels +function createUser(data: UserData) { + validateUser(data); + await userRepository.save(data); // Details hidden +} +``` + +## Workaround Detection + +Don't overlook compromises made to "just make it work." + +| Pattern | Example | +|---------|---------| +| Unnecessary package additions | Mystery libraries added just to make things work | +| Test deletion/skipping | `@Disabled`, `.skip()`, commented out | +| Empty implementations/stubs | `return null`, `// TODO: implement`, `pass` | +| Mock data in production | Hardcoded dummy data | +| Swallowed errors | Empty `catch {}`, `rescue nil` | +| Magic numbers | Unexplained `if (status == 3)` | + +## Strict TODO Comment Prohibition + +"We'll do it later" never gets done. What's not done now is never done. + +TODO comments are immediate REJECT. + +```kotlin +// REJECT - Future-looking TODO +// TODO: Add authorization check by facility ID +fun deleteCustomHoliday(@PathVariable id: String) { + deleteCustomHolidayInputPort.execute(input) +} + +// APPROVE - Implement now +fun deleteCustomHoliday(@PathVariable id: String) { + val currentUserFacilityId = getCurrentUserFacilityId() + val holiday = findHolidayById(id) + require(holiday.facilityId == currentUserFacilityId) { + "Cannot delete holiday from another facility" + } + deleteCustomHolidayInputPort.execute(input) +} +``` + +Only acceptable TODO cases: + +| Condition | Example | Judgment | +|-----------|---------|----------| +| External dependency prevents implementation + Issued | `// TODO(#123): Implement after API key obtained` | Acceptable | +| Technical constraint prevents + Issued | `// TODO(#456): Waiting for library bug fix` | Acceptable | +| "Future implementation", "add later" | `// TODO: Add validation` | REJECT | +| "No time for now" | `// TODO: Refactor` | REJECT | + +Correct handling: +- Needed now → Implement now +- Not needed now → Delete the code +- External blocker → Create issue and include ticket number in comment + +## DRY Violation Detection + +Detect duplicate code. + +| Pattern | Judgment | +|---------|----------| +| Same logic in 3+ places | Immediate REJECT - Extract to function/method | +| Same validation in 2+ places | Immediate REJECT - Extract to validator function | +| Similar components 3+ | Immediate REJECT - Create shared component | +| Copy-paste derived code | Immediate REJECT - Parameterize or abstract | + +AHA principle (Avoid Hasty Abstractions) balance: +- 2 duplications → Wait and see +- 3 duplications → Extract immediately +- Different domain duplications → Don't abstract (e.g., customer validation vs admin validation are different) + +## Spec Compliance Verification + +Verify that changes comply with the project's documented specifications. + +Verification targets: + +| Target | What to Check | +|--------|---------------| +| CLAUDE.md / README.md | Conforms to schema definitions, design principles, constraints | +| Type definitions / Zod schemas | New fields reflected in schemas | +| YAML/JSON config files | Follows documented format | + +Specific checks: + +1. When config files (YAML, etc.) are modified or added: + - Cross-reference with schema definitions in CLAUDE.md, etc. + - No ignored or invalid fields present + - No required fields missing + +2. When type definitions or interfaces are modified: + - Documentation schema descriptions are updated + - Existing config files are compatible with new schema + +REJECT when these patterns are found: + +| Pattern | Problem | +|---------|---------| +| Fields not in the spec | Ignored or unexpected behavior | +| Invalid values per spec | Runtime error or silently ignored | +| Violation of documented constraints | Against design intent | + +## Call Chain Verification + +When new parameters/fields are added, verify not just the changed file but also callers. + +Verification steps: +1. When finding new optional parameters or interface fields, `Grep` all callers +2. Check if all callers pass the new parameter +3. If fallback value (`?? default`) exists, verify if fallback is used as intended + +Danger patterns: + +| Pattern | Problem | Detection | +|---------|---------|-----------| +| `options.xxx ?? fallback` where all callers omit `xxx` | Feature implemented but always falls back | grep callers | +| Tests set values directly with mocks | Don't go through actual call chain | Check test construction | +| `executeXxx()` doesn't receive `options` it uses internally | No route to pass value from above | Check function signature | + +```typescript +// Missing wiring: No route to receive projectCwd +export async function executePiece(config, cwd, task) { + const engine = new PieceEngine(config, cwd, task); // No options +} + +// Wired: Can pass projectCwd +export async function executePiece(config, cwd, task, options?) { + const engine = new PieceEngine(config, cwd, task, options); +} +``` + +Logically dead code due to caller constraints: + +Call chain verification applies not only to "missing wiring" but also to the reverse — unnecessary guards for conditions that callers already guarantee. + +| Pattern | Problem | Detection | +|---------|---------|-----------| +| TTY check when all callers require TTY | Unreachable branch remains | grep all callers' preconditions | +| Null guard when callers already check null | Redundant defense | Trace caller constraints | +| Runtime type check when TypeScript types constrain | Not trusting type safety | Check TypeScript type constraints | + +Verification steps: +1. When finding defensive branches (TTY check, null guard, etc.), grep all callers +2. If all callers already guarantee the condition, guard is unnecessary → REJECT +3. If some callers don't guarantee it, keep the guard + +## Quality Attributes + +| Attribute | Review Point | +|-----------|--------------| +| Scalability | Design handles increased load | +| Maintainability | Easy to modify and fix | +| Observability | Logging and monitoring enabled | + +## Big Picture + +Don't get lost in minor "clean code" nitpicks. + +Verify: +- How will this code evolve in the future +- Is scaling considered +- Is technical debt being created +- Does it align with business requirements +- Is naming consistent with the domain + +## Change Scope Assessment + +Check change scope and include in report (non-blocking). + +| Scope Size | Lines Changed | Action | +|------------|---------------|--------| +| Small | ~200 lines | Review as-is | +| Medium | 200-500 lines | Review as-is | +| Large | 500+ lines | Continue review. Suggest splitting if possible | + +Note: Some tasks require large changes. Don't REJECT based on line count alone. + +Verify: +- Changes are logically cohesive (no unrelated changes mixed in) +- Coder's scope declaration matches actual changes + +Include as suggestions (non-blocking): +- If splittable, present splitting proposal diff --git a/resources/global/en/knowledge/cqrs-es.md b/resources/global/en/knowledge/cqrs-es.md new file mode 100644 index 0000000..ba226a9 --- /dev/null +++ b/resources/global/en/knowledge/cqrs-es.md @@ -0,0 +1,417 @@ +# CQRS+ES Knowledge + +## Aggregate Design + +Aggregates hold only fields necessary for decision-making. + +Command Model (Aggregate) role is to "receive commands, make decisions, and emit events". Query data is handled by Read Model (Projection). + +"Necessary for decision" means: +- Used in `if`/`require` conditional branches +- Field value referenced when emitting events in instance methods + +| Criteria | Judgment | +|----------|----------| +| Aggregate spans multiple transaction boundaries | REJECT | +| Direct references between Aggregates (not ID references) | REJECT | +| Aggregate exceeds 100 lines | Consider splitting | +| Business invariants exist outside Aggregate | REJECT | +| Holding fields not used for decisions | REJECT | + +Good Aggregate: +```kotlin +// Only fields necessary for decisions +data class Order( + val orderId: String, // Used when emitting events + val status: OrderStatus // Used for state checking +) { + fun confirm(confirmedBy: String): OrderConfirmedEvent { + require(status == OrderStatus.PENDING) { "Cannot confirm in this state" } + return OrderConfirmedEvent( + orderId = orderId, + confirmedBy = confirmedBy, + confirmedAt = LocalDateTime.now() + ) + } +} + +// Holding fields not used for decisions (NG) +data class Order( + val orderId: String, + val customerId: String, // Not used for decisions + val shippingAddress: Address, // Not used for decisions + val status: OrderStatus +) +``` + +Aggregates with no additional operations have ID only: +```kotlin +// When only creation, no additional operations +data class Notification(val notificationId: String) { + companion object { + fun create(customerId: String, message: String): NotificationCreatedEvent { + return NotificationCreatedEvent( + notificationId = UUID.randomUUID().toString(), + customerId = customerId, + message = message + ) + } + } +} +``` + +## Event Design + +| Criteria | Judgment | +|----------|----------| +| Event not in past tense (Created → Create) | REJECT | +| Event contains logic | REJECT | +| Event contains internal state of other Aggregates | REJECT | +| Event schema not version controlled | Warning | +| CRUD-style events (Updated, Deleted) | Needs review | + +Good Events: +```kotlin +// Good: Domain intent is clear +OrderPlaced, PaymentReceived, ItemShipped + +// Bad: CRUD style +OrderUpdated, OrderDeleted +``` + +Event Granularity: +- Too fine: `OrderFieldChanged` → Domain intent unclear +- Appropriate: `ShippingAddressChanged` → Intent is clear +- Too coarse: `OrderModified` → What changed is unclear + +## Command Handlers + +| Criteria | Judgment | +|----------|----------| +| Handler directly manipulates DB | REJECT | +| Handler modifies multiple Aggregates | REJECT | +| No command validation | REJECT | +| Handler executes queries to make decisions | Needs review | + +Good Command Handler: +``` +1. Receive command +2. Restore Aggregate from event store +3. Apply command to Aggregate +4. Save emitted events +``` + +## Projection Design + +| Criteria | Judgment | +|----------|----------| +| Projection issues commands | REJECT | +| Projection references Write model | REJECT | +| Single projection serves multiple use cases | Needs review | +| Design that cannot be rebuilt | REJECT | + +Good Projection: +- Optimized for specific read use case +- Idempotently reconstructible from events +- Completely independent from Write model + +## Query Side Design + +Controller uses QueryGateway. Does not use Repository directly. + +Types between layers: +- `application/query/` - Query result types (e.g., `OrderDetail`) +- `adapter/protocol/` - REST response types (e.g., `OrderDetailResponse`) +- QueryHandler returns application layer types, Controller converts to adapter layer types + +```kotlin +// application/query/OrderDetail.kt +data class OrderDetail( + val orderId: String, + val customerName: String, + val totalAmount: Money +) + +// adapter/protocol/OrderDetailResponse.kt +data class OrderDetailResponse(...) { + companion object { + fun from(detail: OrderDetail) = OrderDetailResponse(...) + } +} + +// QueryHandler - returns application layer type +@QueryHandler +fun handle(query: GetOrderDetailQuery): OrderDetail? { + val entity = repository.findById(query.id) ?: return null + return OrderDetail(...) +} + +// Controller - converts to adapter layer type +@GetMapping("/{id}") +fun getById(@PathVariable id: String): ResponseEntity { + val detail = queryGateway.query( + GetOrderDetailQuery(id), + OrderDetail::class.java + ).join() ?: throw NotFoundException("...") + + return ResponseEntity.ok(OrderDetailResponse.from(detail)) +} +``` + +Structure: +``` +Controller (adapter) → QueryGateway → QueryHandler (application) → Repository + ↓ ↓ +Response.from(detail) OrderDetail +``` + +## Eventual Consistency + +| Situation | Response | +|-----------|----------| +| UI expects immediate updates | Redesign or polling/WebSocket | +| Consistency delay exceeds tolerance | Reconsider architecture | +| Compensating transactions undefined | Request failure scenario review | + +## Saga vs EventHandler + +Saga is used only for "operations between multiple aggregates where contention occurs". + +Cases where Saga is needed: +``` +When multiple actors compete for the same resource +Example: Inventory reservation (10 people ordering the same product simultaneously) + +OrderPlacedEvent + ↓ InventoryReservationSaga +ReserveInventoryCommand → Inventory aggregate (serializes concurrent execution) + ↓ +InventoryReservedEvent → ConfirmOrderCommand +InventoryReservationFailedEvent → CancelOrderCommand +``` + +Cases where Saga is not needed: +``` +Non-competing operations +Example: Inventory release on order cancellation + +OrderCancelledEvent + ↓ InventoryReleaseHandler (simple EventHandler) +ReleaseInventoryCommand + ↓ +InventoryReleasedEvent +``` + +Decision criteria: + +| Situation | Saga | EventHandler | +|-----------|------|--------------| +| Resource contention exists | Use | - | +| Compensating transaction needed | Use | - | +| Non-competing simple coordination | - | Use | +| Retry on failure is sufficient | - | Use | + +Anti-pattern: +```kotlin +// NG - Using Saga for lifecycle management +@Saga +class OrderLifecycleSaga { + // Tracking all order state transitions in Saga + // PLACED → CONFIRMED → SHIPPED → DELIVERED +} + +// OK - Saga only for operations requiring eventual consistency +@Saga +class InventoryReservationSaga { + // Only for inventory reservation concurrency control +} +``` + +Saga is not a lifecycle management tool. Create it per "operation" that requires eventual consistency. + +## Exception vs Event (Failure Handling) + +Failures not requiring audit use exceptions, failures requiring audit use events. + +Exception approach (recommended: most cases): +```kotlin +// Domain model: Throws exception on validation failure +fun reserveInventory(orderId: String, quantity: Int): InventoryReservedEvent { + if (availableQuantity < quantity) { + throw InsufficientInventoryException("Insufficient inventory") + } + return InventoryReservedEvent(productId, orderId, quantity) +} + +// Saga: Catch with exceptionally and perform compensating action +commandGateway.send(command) + .exceptionally { ex -> + commandGateway.send(CancelOrderCommand( + orderId = orderId, + reason = ex.cause?.message ?: "Inventory reservation failed" + )) + null + } +``` + +Event approach (rare cases): +```kotlin +// Only when audit is required +data class PaymentFailedEvent( + val paymentId: String, + val reason: String, + val attemptedAmount: Money +) : PaymentEvent +``` + +Decision criteria: + +| Question | Exception | Event | +|----------|-----------|-------| +| Need to check this failure later? | No | Yes | +| Required by regulations/compliance? | No | Yes | +| Only Saga cares about the failure? | Yes | No | +| Is there value in keeping it in Event Store? | No | Yes | + +Default is exception approach. Consider events only when audit requirements exist. + +## Abstraction Level Evaluation + +**Conditional branch proliferation detection:** + +| Pattern | Judgment | +|---------|----------| +| Same if-else pattern in 3+ places | Abstract with polymorphism → REJECT | +| switch/case with 5+ branches | Consider Strategy/Map pattern | +| Event type branching proliferating | Separate event handlers → REJECT | +| Complex state branching in Aggregate | Consider State Pattern | + +**Abstraction level mismatch detection:** + +| Pattern | Problem | Fix | +|---------|---------|-----| +| DB operation details in CommandHandler | Responsibility violation | Separate to Repository layer | +| Business logic in EventHandler | Responsibility violation | Extract to domain service | +| Persistence in Aggregate | Layer violation | Change to EventStore route | +| Calculation logic in Projection | Hard to maintain | Extract to dedicated service | + +Good abstraction examples: + +```kotlin +// Event type branching proliferation (NG) +@EventHandler +fun on(event: DomainEvent) { + when (event) { + is OrderPlacedEvent -> handleOrderPlaced(event) + is OrderConfirmedEvent -> handleOrderConfirmed(event) + is OrderShippedEvent -> handleOrderShipped(event) + // ...keeps growing + } +} + +// Separate handlers per event (OK) +@EventHandler +fun on(event: OrderPlacedEvent) { ... } + +@EventHandler +fun on(event: OrderConfirmedEvent) { ... } + +@EventHandler +fun on(event: OrderShippedEvent) { ... } +``` + +```kotlin +// Complex state branching (NG) +fun process(command: ProcessCommand) { + when (status) { + PENDING -> if (command.type == "approve") { ... } else if (command.type == "reject") { ... } + APPROVED -> if (command.type == "ship") { ... } + // ...gets complex + } +} + +// Abstracted with State Pattern (OK) +sealed class OrderState { + abstract fun handle(command: ProcessCommand): List +} +class PendingState : OrderState() { + override fun handle(command: ProcessCommand) = when (command) { + is ApproveCommand -> listOf(OrderApprovedEvent(...)) + is RejectCommand -> listOf(OrderRejectedEvent(...)) + else -> throw InvalidCommandException() + } +} +``` + +## Anti-pattern Detection + +REJECT if found: + +| Anti-pattern | Problem | +|--------------|---------| +| CRUD Disguise | Just splitting CRUD into Command/Query | +| Anemic Domain Model | Aggregate is just a data structure | +| Event Soup | Meaningless events proliferate | +| Temporal Coupling | Implicit dependency on event order | +| Missing Events | Important domain events are missing | +| God Aggregate | All responsibilities in one Aggregate | + +## Test Strategy + +Separate test strategies by layer. + +Test Pyramid: +``` + ┌─────────────┐ + │ E2E Test │ ← Few: Overall flow confirmation + ├─────────────┤ + │ Integration │ ← Command→Event→Projection→Query coordination + ├─────────────┤ + │ Unit Test │ ← Many: Each layer tested independently + └─────────────┘ +``` + +Command side (Aggregate): +```kotlin +// Using AggregateTestFixture +@Test +fun `confirm command emits event`() { + fixture + .given(OrderPlacedEvent(...)) + .`when`(ConfirmOrderCommand(orderId, confirmedBy)) + .expectSuccessfulHandlerExecution() + .expectEvents(OrderConfirmedEvent(...)) +} +``` + +Query side: +```kotlin +// Direct Read Model setup + QueryGateway +@Test +fun `can get order details`() { + // Given: Setup Read Model directly + orderRepository.save(OrderEntity(...)) + + // When: Execute query via QueryGateway + val detail = queryGateway.query(GetOrderDetailQuery(orderId), ...).join() + + // Then + assertEquals(expectedDetail, detail) +} +``` + +Checklist: + +| Aspect | Judgment | +|--------|----------| +| Aggregate tests verify events not state | Required | +| Query side tests don't create data via Command | Recommended | +| Integration tests consider Axon async processing | Required | + +## Infrastructure Layer + +Check: +- Is event store choice appropriate? +- Does messaging infrastructure meet requirements? +- Is snapshot strategy defined? +- Is event serialization format appropriate? diff --git a/resources/global/en/knowledge/frontend.md b/resources/global/en/knowledge/frontend.md new file mode 100644 index 0000000..8674246 --- /dev/null +++ b/resources/global/en/knowledge/frontend.md @@ -0,0 +1,497 @@ +# Frontend Knowledge + +## Component Design + +Do not write everything in one file. Always split components. + +Required splits: +- Has its own state → Must split +- JSX over 50 lines → Split +- Reusable → Split +- Multiple responsibilities → Split +- Independent section within page → Split + +| Criteria | Judgment | +|----------|----------| +| Component over 200 lines | Consider splitting | +| Component over 300 lines | REJECT | +| Display and logic mixed | Consider separation | +| Props drilling (3+ levels) | Consider state management | +| Component with multiple responsibilities | REJECT | + +Good Component: +- Single responsibility: Does one thing well +- Self-contained: Dependencies are clear +- Testable: Side effects are isolated + +Component Classification: + +| Type | Responsibility | Example | +|------|----------------|---------| +| Container | Data fetching, state management | `UserListContainer` | +| Presentational | Display only | `UserCard` | +| Layout | Arrangement, structure | `PageLayout`, `Grid` | +| Utility | Common functionality | `ErrorBoundary`, `Portal` | + +Directory Structure: +``` +features/{feature-name}/ +├── components/ +│ ├── {feature}-view.tsx # Main view (composes children) +│ ├── {sub-component}.tsx # Sub-components +│ └── index.ts +├── hooks/ +├── types.ts +└── index.ts +``` + +## State Management + +Child components do not modify their own state. They bubble events to parent, and parent manipulates state. + +```tsx +// ❌ Child modifies its own state +const ChildBad = ({ initialValue }: { initialValue: string }) => { + const [value, setValue] = useState(initialValue) + return setValue(e.target.value)} /> +} + +// ✅ Parent manages state, child notifies via callback +const ChildGood = ({ value, onChange }: { value: string; onChange: (v: string) => void }) => { + return onChange(e.target.value)} /> +} + +const Parent = () => { + const [value, setValue] = useState('') + return +} +``` + +Exception (OK for child to have local state): +- UI-only temporary state (hover, focus, animation) +- Completely local state that doesn't need to be communicated to parent + +| Criteria | Judgment | +|----------|----------| +| Unnecessary global state | Consider localizing | +| Same state managed in multiple places | Needs normalization | +| State changes from child to parent (reverse data flow) | REJECT | +| API response stored as-is in state | Consider normalization | +| Inappropriate useEffect dependencies | REJECT | + +State Placement Guidelines: + +| State Nature | Recommended Placement | +|--------------|----------------------| +| Temporary UI state (modal open/close, etc.) | Local (useState) | +| Form input values | Local or form library | +| Shared across multiple components | Context or state management library | +| Server data cache | Data fetching library (TanStack Query, etc.) | + +## Data Fetching + +API calls are made in root (View) components and passed to children via props. + +```tsx +// ✅ CORRECT - Fetch at root, pass to children +const OrderDetailView = () => { + const { data: order, isLoading, error } = useGetOrder(orderId) + const { data: items } = useListOrderItems(orderId) + + if (isLoading) return + if (error) return + + return ( + + ) +} + +// ❌ WRONG - Child fetches its own data +const OrderSummary = ({ orderId }) => { + const { data: order } = useGetOrder(orderId) + // ... +} +``` + +When UI state changes affect parameters (week switching, filters, etc.): + +Manage state at View level and pass callbacks to components. + +```tsx +// ✅ CORRECT - State managed at View level +const ScheduleView = () => { + const [currentWeek, setCurrentWeek] = useState(startOfWeek(new Date())) + const { data } = useListSchedules({ + from: format(currentWeek, 'yyyy-MM-dd'), + to: format(endOfWeek(currentWeek), 'yyyy-MM-dd'), + }) + + return ( + + ) +} + +// ❌ WRONG - Component manages state + data fetching +const WeeklyCalendar = ({ facilityId }) => { + const [currentWeek, setCurrentWeek] = useState(...) + const { data } = useListSchedules({ facilityId, from, to }) + // ... +} +``` + +Exceptions (component-level fetching allowed): + +| Case | Reason | +|------|--------| +| Infinite scroll | Depends on scroll position (internal UI state) | +| Search autocomplete | Real-time search based on input value | +| Independent widget | Notification badge, weather, etc. Completely unrelated to parent data | +| Real-time updates | WebSocket/Polling auto-updates | +| Modal detail fetch | Fetch additional data only when opened | + +Decision criteria: "Is there no point in parent managing this / Does it not affect parent?" + +| Criteria | Judgment | +|----------|----------| +| Direct fetch in component | Separate to Container layer | +| No error handling | REJECT | +| Loading state not handled | REJECT | +| No cancellation handling | Warning | +| N+1 query-like fetching | REJECT | + +## Shared Components and Abstraction + +Common UI patterns should be shared components. Copy-paste of inline styles is prohibited. + +```tsx +// ❌ WRONG - Copy-pasted inline styles + + +// ✅ CORRECT - Use shared component + + + +``` + +Patterns to make shared components: +- Icon buttons (close, edit, delete, etc.) +- Loading/error displays +- Status badges +- Tab switching +- Label + value display (detail screens) +- Search input +- Color legends + +Avoid over-generalization: + +```tsx +// ❌ WRONG - Forcing stepper variant into IconButton +export const iconButtonVariants = cva('...', { + variants: { + variant: { + default: '...', + outlined: '...', // ← Stepper-specific, not used elsewhere + }, + size: { + medium: 'p-2', + stepper: 'w-8 h-8', // ← Only used with outlined + }, + }, +}) + +// ✅ CORRECT - Purpose-specific component +export function StepperButton(props) { + return ( + + ) +} +``` + +Signs to make separate components: +- Implicit constraints like "this variant is always with this size" +- Added variant is clearly different from original component's purpose +- Props specification becomes complex on the usage side + +## Abstraction Level Evaluation + +**Conditional branch bloat detection:** + +| Pattern | Judgment | +|---------|----------| +| Same conditional in 3+ places | Extract to shared component → **REJECT** | +| Props-based branching with 5+ types | Consider component split | +| Nested ternaries in render | Early return or component separation → **REJECT** | +| Type-based render branching | Consider polymorphic components | + +**Abstraction level mismatch detection:** + +| Pattern | Problem | Fix | +|---------|---------|-----| +| Data fetching logic mixed in JSX | Hard to read | Extract to custom hook | +| Business logic mixed in component | Responsibility violation | Separate to hooks/utils | +| Style calculation logic scattered | Hard to maintain | Extract to utility function | +| Same transformation in multiple places | DRY violation | Extract to common function | + +Good abstraction examples: + +```tsx +// ❌ Conditional bloat +function UserBadge({ user }) { + if (user.role === 'admin') { + return Admin + } else if (user.role === 'moderator') { + return Moderator + } else if (user.role === 'premium') { + return Premium + } else { + return User + } +} + +// ✅ Abstracted with Map +const ROLE_CONFIG = { + admin: { label: 'Admin', className: 'bg-red-500' }, + moderator: { label: 'Moderator', className: 'bg-yellow-500' }, + premium: { label: 'Premium', className: 'bg-purple-500' }, + default: { label: 'User', className: 'bg-gray-500' }, +} + +function UserBadge({ user }) { + const config = ROLE_CONFIG[user.role] ?? ROLE_CONFIG.default + return {config.label} +} +``` + +```tsx +// ❌ Mixed abstraction levels +function OrderList() { + const [orders, setOrders] = useState([]) + useEffect(() => { + fetch('/api/orders') + .then(res => res.json()) + .then(data => setOrders(data)) + }, []) + + return orders.map(order => ( +
{order.total.toLocaleString()} USD
+ )) +} + +// ✅ Aligned abstraction levels +function OrderList() { + const { data: orders } = useOrders() // Hide data fetching + + return orders.map(order => ( + + )) +} +``` + +## Frontend and Backend Separation of Concerns + +### Display Format Responsibility + +Backend returns "data", frontend converts to "display format". + +```tsx +// ✅ Frontend: Convert to display format +export function formatPrice(amount: number): string { + return `$${amount.toLocaleString()}` +} + +export function formatDate(date: Date): string { + return format(date, 'MMM d, yyyy') +} +``` + +| Criteria | Judgment | +|----------|----------| +| Backend returns display strings | Suggest design review | +| Same format logic copy-pasted | Unify to utility function | +| Inline formatting in component | Extract to function | + +### Domain Logic Placement (Smart UI Elimination) + +Domain logic (business rules) belongs in the backend. Frontend only displays and edits state. + +What is domain logic: +- Aggregate business rules (stock validation, price calculation, status transitions) +- Business constraint validation +- Invariant enforcement + +Frontend responsibilities: +- Display state received from server +- Collect user input and send commands to backend +- Manage UI-only temporary state (focus, hover, modal open/close) +- Display format conversion (formatting, sorting, filtering) + +| Criteria | Judgment | +|----------|----------| +| Price calculation/stock validation in frontend | Move to backend → **REJECT** | +| Status transition rules in frontend | Move to backend → **REJECT** | +| Business validation in frontend | Move to backend → **REJECT** | +| Recalculating server-computable values in frontend | Redundant → **REJECT** | + +Good vs Bad Examples: + +```tsx +// ❌ BAD - Business rules in frontend +function OrderForm({ order }: { order: Order }) { + const totalPrice = order.items.reduce((sum, item) => + sum + item.price * item.quantity, 0 + ) + const canCheckout = totalPrice >= 100 && order.items.every(i => i.stock > 0) + + return +} + +// ✅ GOOD - Display state received from server +function OrderForm({ order }: { order: Order }) { + // totalPrice, canCheckout are received from server + return ( + <> +
{formatPrice(order.totalPrice)}
+ + + ) +} +``` + +```tsx +// ❌ BAD - Status transition logic in frontend +function TaskCard({ task }: { task: Task }) { + const canStart = task.status === 'pending' && task.assignee !== null + const canComplete = task.status === 'in_progress' && /* complex conditions... */ + + return ( + <> + + + + ) +} + +// ✅ GOOD - Server returns allowed actions +function TaskCard({ task }: { task: Task }) { + // task.allowedActions = ['start', 'cancel'], etc., calculated by server + const canStart = task.allowedActions.includes('start') + const canComplete = task.allowedActions.includes('complete') + + return ( + <> + + + + ) +} +``` + +Exceptions (OK to have logic in frontend): + +| Case | Reason | +|------|--------| +| UI-only validation | UX feedback like "required field", "max length" (must also validate on server) | +| Client-side filter/sort | Changing display order of lists received from server | +| Display condition branching | UI control like "show details if logged in" | +| Real-time feedback | Preview display during input | + +Decision criteria: "Would the business break if this calculation differs from the server?" +- YES → Place in backend (domain logic) +- NO → OK in frontend (display logic) + +## Performance + +| Criteria | Judgment | +|----------|----------| +| Unnecessary re-renders | Needs optimization | +| Large lists without virtualization | Warning | +| Unoptimized images | Warning | +| Unused code in bundle | Check tree-shaking | +| Excessive memoization | Verify necessity | + +Optimization Checklist: +- Are `React.memo` / `useMemo` / `useCallback` appropriate? +- Are large lists using virtual scroll? +- Is Code Splitting appropriate? +- Are images lazy loaded? + +Anti-patterns: + +```tsx +// ❌ New object every render + + +// ✅ Constant or useMemo +const style = useMemo(() => ({ color: 'red' }), []); + +``` + +## Accessibility + +| Criteria | Judgment | +|----------|----------| +| Interactive elements without keyboard support | REJECT | +| Images without alt attribute | REJECT | +| Form elements without labels | REJECT | +| Information conveyed by color only | REJECT | +| Missing focus management (modals, etc.) | REJECT | + +Checklist: +- Using semantic HTML? +- Are ARIA attributes appropriate (not excessive)? +- Is keyboard navigation possible? +- Does it make sense with a screen reader? +- Is color contrast sufficient? + +## TypeScript/Type Safety + +| Criteria | Judgment | +|----------|----------| +| Use of `any` type | REJECT | +| Excessive type assertions (as) | Needs review | +| No Props type definition | REJECT | +| Inappropriate event handler types | Needs fix | + +## Frontend Security + +| Criteria | Judgment | +|----------|----------| +| dangerouslySetInnerHTML usage | Check XSS risk | +| Unsanitized user input | REJECT | +| Sensitive data stored in frontend | REJECT | +| CSRF token not used | Needs verification | + +## Testability + +| Criteria | Judgment | +|----------|----------| +| No data-testid, etc. | Warning | +| Structure difficult to test | Consider separation | +| Business logic embedded in UI | REJECT | + +## Anti-pattern Detection + +REJECT if found: + +| Anti-pattern | Problem | +|--------------|---------| +| God Component | All features concentrated in one component | +| Prop Drilling | Deep props bucket brigade | +| Inline Styles abuse | Maintainability degradation | +| useEffect hell | Dependencies too complex | +| Premature Optimization | Unnecessary memoization | +| Magic Strings | Hardcoded strings | +| Hidden Dependencies | Child components with hidden API calls | +| Over-generalization | Components forced to be generic | diff --git a/resources/global/en/knowledge/security.md b/resources/global/en/knowledge/security.md new file mode 100644 index 0000000..6c9d844 --- /dev/null +++ b/resources/global/en/knowledge/security.md @@ -0,0 +1,164 @@ +# Security Knowledge + +## AI-Generated Code Security Issues + +AI-generated code has unique vulnerability patterns. + +| Pattern | Risk | Example | +|---------|------|---------| +| Plausible but dangerous defaults | High | `cors: { origin: '*' }` looks fine but is dangerous | +| Outdated security practices | Medium | Using deprecated encryption, old auth patterns | +| Incomplete validation | High | Validates format but not business rules | +| Over-trusting inputs | Critical | Assumes internal APIs are always safe | +| Copy-paste vulnerabilities | High | Same dangerous pattern repeated in multiple files | + +Require extra scrutiny: +- Auth/authorization logic (AI tends to miss edge cases) +- Input validation (AI may check syntax but miss semantics) +- Error messages (AI may expose internal details) +- Config files (AI may use dangerous defaults from training data) + +## Injection Attacks + +**SQL Injection:** + +- SQL construction via string concatenation → REJECT +- Not using parameterized queries → REJECT +- Unsanitized input in ORM raw queries → REJECT + +```typescript +// NG +db.query(`SELECT * FROM users WHERE id = ${userId}`) + +// OK +db.query('SELECT * FROM users WHERE id = ?', [userId]) +``` + +**Command Injection:** + +- Unvalidated input in `exec()`, `spawn()` → REJECT +- Insufficient escaping in shell command construction → REJECT + +```typescript +// NG +exec(`ls ${userInput}`) + +// OK +execFile('ls', [sanitizedInput]) +``` + +**XSS (Cross-Site Scripting):** + +- Unescaped output to HTML/JS → REJECT +- Improper use of `innerHTML`, `dangerouslySetInnerHTML` → REJECT +- Direct embedding of URL parameters → REJECT + +## Authentication & Authorization + +**Authentication issues:** + +- Hardcoded credentials → Immediate REJECT +- Plaintext password storage → Immediate REJECT +- Weak hash algorithms (MD5, SHA1) → REJECT +- Improper session token management → REJECT + +**Authorization issues:** + +- Missing permission checks → REJECT +- IDOR (Insecure Direct Object Reference) → REJECT +- Privilege escalation possibility → REJECT + +```typescript +// NG - No permission check +app.get('/user/:id', (req, res) => { + return db.getUser(req.params.id) +}) + +// OK +app.get('/user/:id', authorize('read:user'), (req, res) => { + if (req.user.id !== req.params.id && !req.user.isAdmin) { + return res.status(403).send('Forbidden') + } + return db.getUser(req.params.id) +}) +``` + +## Data Protection + +**Sensitive information exposure:** + +- Hardcoded API keys, secrets → Immediate REJECT +- Sensitive info in logs → REJECT +- Internal info exposure in error messages → REJECT +- Committed `.env` files → REJECT + +**Data validation:** + +- Unvalidated input values → REJECT +- Missing type checks → REJECT +- No size limits set → REJECT + +## Cryptography + +- Use of weak crypto algorithms → REJECT +- Fixed IV/Nonce usage → REJECT +- Hardcoded encryption keys → Immediate REJECT +- No HTTPS (production) → REJECT + +## File Operations + +**Path Traversal:** + +- File paths containing user input → REJECT +- Insufficient `../` sanitization → REJECT + +```typescript +// NG +const filePath = path.join(baseDir, userInput) +fs.readFile(filePath) + +// OK +const safePath = path.resolve(baseDir, userInput) +if (!safePath.startsWith(path.resolve(baseDir))) { + throw new Error('Invalid path') +} +``` + +**File Upload:** + +- No file type validation → REJECT +- No file size limits → REJECT +- Allowing executable file uploads → REJECT + +## Dependencies + +- Packages with known vulnerabilities → REJECT +- Unmaintained packages → Warning +- Unnecessary dependencies → Warning + +## Error Handling + +- Stack trace exposure in production → REJECT +- Detailed error message exposure → REJECT +- Swallowing security events → REJECT + +## Rate Limiting & DoS Protection + +- No rate limiting (auth endpoints) → Warning +- Resource exhaustion attack possibility → Warning +- Infinite loop possibility → REJECT + +## OWASP Top 10 Checklist + +| Category | Check Items | +|----------|-------------| +| A01 Broken Access Control | Authorization checks, CORS config | +| A02 Cryptographic Failures | Encryption, sensitive data protection | +| A03 Injection | SQL, Command, XSS | +| A04 Insecure Design | Security design patterns | +| A05 Security Misconfiguration | Default settings, unnecessary features | +| A06 Vulnerable Components | Dependency vulnerabilities | +| A07 Auth Failures | Authentication mechanisms | +| A08 Software Integrity | Code signing, CI/CD | +| A09 Logging Failures | Security logging | +| A10 SSRF | Server-side requests | diff --git a/resources/global/en/personas/architecture-reviewer.md b/resources/global/en/personas/architecture-reviewer.md index f2b8e45..6fdb744 100644 --- a/resources/global/en/personas/architecture-reviewer.md +++ b/resources/global/en/personas/architecture-reviewer.md @@ -40,434 +40,6 @@ Code is read far more often than it is written. Poorly structured code destroys - Give vague feedback ("clean this up" is prohibited) - Review AI-specific issues (AI Reviewer's job) -## Review Target Distinction - -**Important**: Distinguish between source files and generated files. - -| Type | Location | Review Target | -|------|----------|---------------| -| Generated reports | `.takt/reports/` | Not a review target | -| Reports in git diff | `.takt/reports/` | **Ignore** | - -**About template files:** -- YAML and Markdown files in `resources/` are templates -- `{report_dir}`, `{task}` are placeholders (replaced at runtime) -- Even if expanded values appear in git diff for report files, they are NOT hardcoded - -**To avoid false positives:** -1. Before flagging "hardcoded values", **verify if the file is source or report** -2. Files under `.takt/reports/` are generated during piece execution - not review targets -3. Ignore generated files even if they appear in git diff - -## Review Perspectives - -### 1. Structure & Design - -**File Organization:** - -| Criteria | Judgment | -|----------|----------| -| Single file > 200 lines | Consider splitting | -| Single file > 300 lines | REJECT | -| Single file with multiple responsibilities | REJECT | -| Unrelated code coexisting | REJECT | - -**Module Structure:** -- High cohesion: Related functionality grouped together -- Low coupling: Minimal inter-module dependencies -- No circular dependencies -- Appropriate directory hierarchy - -**Function Design:** -- One responsibility per function -- Consider splitting functions over 30 lines -- Side effects clearly defined - -**Layer Design:** -- Dependency direction: Upper layers -> Lower layers (reverse prohibited) -- Controller -> Service -> Repository flow maintained -- 1 interface = 1 responsibility (no giant Service classes) - -**Directory Structure:** - -Structure pattern selection: - -| Pattern | Use Case | Example | -|---------|----------|---------| -| Layered | Small scale, CRUD-centric | `controllers/`, `services/`, `repositories/` | -| Vertical Slice | Medium-large scale, high feature independence | `features/auth/`, `features/order/` | -| Hybrid | Common foundation + feature modules | `core/` + `features/` | - -Vertical Slice Architecture (organizing code by feature): - -``` -src/ -├── features/ -│ ├── auth/ -│ │ ├── LoginCommand.ts -│ │ ├── LoginHandler.ts -│ │ ├── AuthRepository.ts -│ │ └── auth.test.ts -│ └── order/ -│ ├── CreateOrderCommand.ts -│ ├── CreateOrderHandler.ts -│ └── ... -└── shared/ # Shared across features - ├── database/ - └── middleware/ -``` - -Vertical Slice criteria: - -| Criteria | Judgment | -|----------|----------| -| Single feature spans 3+ layers | Consider slicing | -| Minimal inter-feature dependencies | Recommend slicing | -| Over 50% shared processing | Keep layered | -| Team organized by features | Slicing required | - -Prohibited patterns: - -| Pattern | Problem | -|---------|---------| -| Bloated `utils/` | Becomes graveyard of unclear responsibilities | -| Lazy placement in `common/` | Dependencies become unclear | -| Excessive nesting (4+ levels) | Navigation difficulty | -| Mixed features and layers | `features/services/` prohibited | - -**Separation of Concerns:** -- Read and write responsibilities separated -- Data fetching at root (View/Controller), passed to children -- Error handling centralized (no try-catch scattered everywhere) -- Business logic not leaking into Controller/View - -### 2. Code Quality - -**Mandatory checks:** -- Use of `any` type -> **Immediate REJECT** -- Overuse of fallback values (`?? 'unknown'`) -> **REJECT** (see examples below) -- Explanatory comments (What/How comments) -> **REJECT** (see examples below) -- Unused code ("just in case" code) -> **REJECT** (see examples below) -- Direct state mutation (not immutable) -> **REJECT** (see examples below) - -**Design principles:** -- Simple > Easy: Readability prioritized -- DRY: No more than 3 duplications -- YAGNI: Only what's needed now -- Fail Fast: Errors detected and reported early -- Idiomatic: Follows language/framework conventions - -**Explanatory Comment (What/How) Detection Criteria:** - -Comments must only explain design decisions not evident from code (Why), never restate what the code does (What/How). If the code is clear enough, no comment is needed at all. - -| Judgment | Criteria | -|----------|----------| -| **REJECT** | Restates code behavior in natural language | -| **REJECT** | Repeats what is already obvious from function/variable names | -| **REJECT** | JSDoc that only paraphrases the function name without adding information | -| OK | Explains why a particular implementation was chosen | -| OK | Explains the reason behind seemingly unusual behavior | -| Best | No comment needed — the code itself communicates intent | - -```typescript -// ❌ REJECT - Restates code (What) -// If interrupted, abort immediately -if (status === 'interrupted') { - return ABORT_STEP; -} - -// ❌ REJECT - Restates the loop -// Check transitions in order -for (const transition of step.transitions) { - -// ❌ REJECT - Repeats the function name -/** Check if status matches transition condition. */ -export function matchesCondition(status: Status, condition: TransitionCondition): boolean { - -// ✅ OK - Design decision (Why) -// User interruption takes priority over piece-defined transitions -if (status === 'interrupted') { - return ABORT_STEP; -} - -// ✅ OK - Reason behind seemingly odd behavior -// stay can cause loops, but is only used when explicitly specified by the user -return step.name; - -// ✅ Best - No comment needed. Code is self-evident -if (status === 'interrupted') { - return ABORT_STEP; -} -``` - -**Direct State Mutation Detection Criteria:** - -Directly mutating objects or arrays makes changes hard to track and causes unexpected side effects. Always use spread operators or immutable operations to return new objects. - -```typescript -// ❌ REJECT - Direct array mutation -const steps: Step[] = getSteps(); -steps.push(newStep); // Mutates original array -steps.splice(index, 1); // Mutates original array -steps[0].status = 'done'; // Nested object also mutated directly - -// ✅ OK - Immutable operations -const withNew = [...steps, newStep]; -const without = steps.filter((_, i) => i !== index); -const updated = steps.map((s, i) => - i === 0 ? { ...s, status: 'done' } : s -); - -// ❌ REJECT - Direct object mutation -function updateConfig(config: Config) { - config.logLevel = 'debug'; // Mutates argument directly - config.steps.push(newStep); // Nested mutation too - return config; -} - -// ✅ OK - Returns new object -function updateConfig(config: Config): Config { - return { - ...config, - logLevel: 'debug', - steps: [...config.steps, newStep], - }; -} -``` - -### 3. Security - -- Injection prevention (SQL, Command, XSS) -- User input validation -- Hardcoded sensitive information - -### 4. Testability - -- Dependency injection enabled -- Mockable design -- Tests are written - -### 5. Anti-Pattern Detection - -**REJECT** when these patterns are found: - -| Anti-Pattern | Problem | -|--------------|---------| -| God Class/Component | Single class with too many responsibilities | -| Feature Envy | Frequently accessing other modules' data | -| Shotgun Surgery | Single change ripples across multiple files | -| Over-generalization | Variants and extension points not currently needed | -| Hidden Dependencies | Child components implicitly calling APIs etc. | -| Non-idiomatic | Custom implementation ignoring language/FW conventions | -| Logically dead defensive code | Guards for conditions already guaranteed by all callers | - -**Logically dead defensive code:** - -Call chain verification applies not only to "missing wiring" but also to the reverse — **unnecessary guards for conditions that callers already guarantee**. - -| Pattern | Problem | Detection | -|---------|---------|-----------| -| TTY check when all callers require TTY | Unreachable branch remains | grep all callers' preconditions | -| Null guard when callers already check null | Redundant defense | Trace caller constraints | -| Runtime type check when TypeScript types constrain | Not trusting type safety | Check TypeScript type constraints | - -**Verification:** -1. When you find a defensive branch (TTY check, null guard, etc.), grep all callers -2. If all callers already guarantee the condition, the guard is unnecessary → **REJECT** -3. If some callers don't guarantee it, keep the guard - -### 6. Abstraction Level Evaluation - -**Conditional Branch Proliferation Detection:** - -| Pattern | Judgment | -|---------|----------| -| Same if-else pattern in 3+ places | Abstract with polymorphism → **REJECT** | -| switch/case with 5+ branches | Consider Strategy/Map pattern | -| Flag arguments changing behavior | Split into separate functions → **REJECT** | -| Type-based branching (instanceof/typeof) | Replace with polymorphism → **REJECT** | -| Nested conditionals (3+ levels) | Early return or extract → **REJECT** | - -**Abstraction Level Mismatch Detection:** - -| Pattern | Problem | Fix | -|---------|---------|-----| -| Low-level details in high-level processing | Hard to read | Extract details to functions | -| Mixed abstraction levels in one function | Cognitive load | Align to same granularity | -| DB operations mixed with business logic | Responsibility violation | Separate to Repository layer | -| Config values mixed with processing logic | Hard to change | Externalize configuration | - -**Good Abstraction Examples:** - -```typescript -// ❌ Proliferating conditionals -function process(type: string) { - if (type === 'A') { /* process A */ } - else if (type === 'B') { /* process B */ } - else if (type === 'C') { /* process C */ } - // ...continues -} - -// ✅ Abstract with Map pattern -const processors: Record void> = { - A: processA, - B: processB, - C: processC, -}; -function process(type: string) { - processors[type]?.(); -} -``` - -```typescript -// ❌ Mixed abstraction levels -function createUser(data: UserData) { - // High level: business logic - validateUser(data); - // Low level: DB operation details - const conn = await pool.getConnection(); - await conn.query('INSERT INTO users...'); - conn.release(); -} - -// ✅ Aligned abstraction levels -function createUser(data: UserData) { - validateUser(data); - await userRepository.save(data); // Details hidden -} -``` - -### 7. Workaround Detection - -**Don't overlook compromises made to "just make it work."** - -| Pattern | Example | -|---------|---------| -| Unnecessary package additions | Mystery libraries added just to make things work | -| Test deletion/skipping | `@Disabled`, `.skip()`, commented out | -| Empty implementations/stubs | `return null`, `// TODO: implement`, `pass` | -| Mock data in production | Hardcoded dummy data | -| Swallowed errors | Empty `catch {}`, `rescue nil` | -| Magic numbers | Unexplained `if (status == 3)` | - -**Always point these out.** Temporary fixes become permanent. - -### 8. Spec Compliance Verification - -**Verify that changes comply with the project's documented specifications.** - -**Verification targets:** - -| Target | What to Check | -|--------|---------------| -| CLAUDE.md / README.md | Conforms to schema definitions, design principles, constraints | -| Type definitions / Zod schemas | New fields reflected in schemas | -| YAML/JSON config files | Follows documented format | - -**Specific checks:** - -1. When config files (YAML, etc.) are modified or added: - - Cross-reference with schema definitions in CLAUDE.md, etc. - - No ignored or invalid fields present - - No required fields missing - -2. When type definitions or interfaces are modified: - - Documentation schema descriptions are updated - - Existing config files are compatible with new schema - -3. When piece definitions are modified: - - Correct fields used for step type (normal vs. parallel) - - No unnecessary fields remaining (e.g., `next` on parallel sub-steps) - -**REJECT when these patterns are found:** - -| Pattern | Problem | -|---------|---------| -| Fields not in the spec | Ignored or unexpected behavior | -| Invalid values per spec | Runtime error or silently ignored | -| Violation of documented constraints | Against design intent | -| Step type / field mismatch | Sign of copy-paste error | - -### 9. Quality Attributes - -| Attribute | Review Point | -|-----------|--------------| -| Scalability | Design handles increased load | -| Maintainability | Easy to modify and fix | -| Observability | Logging and monitoring enabled | - -### 10. Big Picture - -**Caution**: Don't get lost in minor "clean code" nitpicks. - -Verify: -- How will this code evolve in the future -- Is scaling considered -- Is technical debt being created -- Does it align with business requirements -- Is naming consistent with the domain - -### 11. Boy Scout Rule - -**Leave the code better than you found it.** If changed files have structural issues, flag them for refactoring within the task scope. - -**In scope:** -- Existing issues within changed files (dead code, poor naming, broken abstractions) -- Structural issues within changed modules (mixed responsibilities, unnecessary dependencies) - -**Out of scope:** -- Issues in unchanged files (record as existing issues only) -- Refactoring that significantly exceeds the task scope (suggest as non-blocking) - -**Judgment:** - -| Situation | Judgment | -|-----------|----------| -| Clear issues within changed files | **REJECT** — require fix together | -| Structural issues within changed modules | **REJECT** — fix if within scope | -| Issues in unchanged files | Record only (non-blocking) | - -**Following poor existing code as justification for leaving problems is not acceptable.** If existing code is bad, improve it rather than match it. - -### 12. Change Scope Assessment - -**Check change scope and include in report (non-blocking).** - -| Scope Size | Lines Changed | Action | -|------------|---------------|--------| -| Small | ~200 lines | Review as-is | -| Medium | 200-500 lines | Review as-is | -| Large | 500+ lines | Continue review. Suggest splitting if possible | - -**Note:** Some tasks require large changes. Don't REJECT based on line count alone. - -**Verify:** -- Changes are logically cohesive (no unrelated changes mixed in) -- Coder's scope declaration matches actual changes - -**Include as suggestions (non-blocking):** -- If splittable, present splitting proposal - -### 13. Circular Review Detection - -When review count is provided (e.g., "Review count: 3rd"), adjust judgment accordingly. - -**From the 3rd review onwards:** - -1. Check if the same type of issues are recurring -2. If recurring, suggest **alternative approaches** rather than detailed fixes -3. Even when REJECTing, include perspective that "a different approach should be considered" - -Example: When issues repeat on the 3rd review - -- Point out the normal issues -- Note that the same type of issues are recurring -- Explain the limitations of the current approach -- Present alternatives (e.g., redesign with a different pattern, introduce new technology) - -**Point**: Rather than repeating "fix this again", step back and suggest a different path. - ## Important **Be specific.** These are prohibited: diff --git a/resources/global/en/personas/cqrs-es-reviewer.md b/resources/global/en/personas/cqrs-es-reviewer.md index 8d4ed80..0eac101 100644 --- a/resources/global/en/personas/cqrs-es-reviewer.md +++ b/resources/global/en/personas/cqrs-es-reviewer.md @@ -28,117 +28,6 @@ The truth of a domain is inscribed in events. State is merely a temporary projec - Snapshot strategies - Replay and rebuild -## Review Criteria - -### 1. Aggregate Design - -**Required Checks:** - -| Criteria | Judgment | -|----------|----------| -| Aggregate spans multiple transaction boundaries | REJECT | -| Direct references between Aggregates (not ID references) | REJECT | -| Aggregate exceeds 100 lines | Consider splitting | -| Business invariants exist outside Aggregate | REJECT | - -**Good Aggregate:** -- Clear consistency boundary -- References other Aggregates by ID -- Receives commands, emits events -- Protects invariants internally - -### 2. Event Design - -**Required Checks:** - -| Criteria | Judgment | -|----------|----------| -| Event not in past tense (Created → Create) | REJECT | -| Event contains logic | REJECT | -| Event contains internal state of other Aggregates | REJECT | -| Event schema not version controlled | Warning | -| CRUD-style events (Updated, Deleted) | Needs review | - -**Good Events:** -``` -// Good: Domain intent is clear -OrderPlaced, PaymentReceived, ItemShipped - -// Bad: CRUD style -OrderUpdated, OrderDeleted -``` - -**Event Granularity:** -- Too fine: `OrderFieldChanged` → Domain intent unclear -- Appropriate: `ShippingAddressChanged` → Intent is clear -- Too coarse: `OrderModified` → What changed is unclear - -### 3. Command Handlers - -**Required Checks:** - -| Criteria | Judgment | -|----------|----------| -| Handler directly manipulates DB | REJECT | -| Handler modifies multiple Aggregates | REJECT | -| No command validation | REJECT | -| Handler executes queries to make decisions | Needs review | - -**Good Command Handler:** -``` -1. Receive command -2. Restore Aggregate from event store -3. Apply command to Aggregate -4. Save emitted events -``` - -### 4. Projection Design - -**Required Checks:** - -| Criteria | Judgment | -|----------|----------| -| Projection issues commands | REJECT | -| Projection references Write model | REJECT | -| Single projection serves multiple use cases | Needs review | -| Design that cannot be rebuilt | REJECT | - -**Good Projection:** -- Optimized for specific read use case -- Idempotently reconstructible from events -- Completely independent from Write model - -### 5. Eventual Consistency - -**Required Checks:** - -| Situation | Response | -|-----------|----------| -| UI expects immediate updates | Redesign or polling/WebSocket | -| Consistency delay exceeds tolerance | Reconsider architecture | -| Compensating transactions undefined | Request failure scenario review | - -### 6. Anti-pattern Detection - -**REJECT** if found: - -| Anti-pattern | Problem | -|--------------|---------| -| CRUD Disguise | Just splitting CRUD into Command/Query | -| Anemic Domain Model | Aggregate is just a data structure | -| Event Soup | Meaningless events proliferate | -| Temporal Coupling | Implicit dependency on event order | -| Missing Events | Important domain events are missing | -| God Aggregate | All responsibilities in one Aggregate | - -### 7. Infrastructure Layer - -**Check:** -- Is the event store choice appropriate? -- Does the messaging infrastructure meet requirements? -- Is snapshot strategy defined? -- Is event serialization format appropriate? - ## Important - **Don't overlook superficial CQRS**: Just splitting CRUD into Command/Query is meaningless diff --git a/resources/global/en/personas/frontend-reviewer.md b/resources/global/en/personas/frontend-reviewer.md index c9396b3..3eb912f 100644 --- a/resources/global/en/personas/frontend-reviewer.md +++ b/resources/global/en/personas/frontend-reviewer.md @@ -32,533 +32,6 @@ The user interface is the only point of contact between the system and users. No - WAI-ARIA compliance - Responsive design -## Review Criteria - -### 1. Component Design - -**Principle: Do not write everything in one file. Always split components.** - -**Required splits:** -- Has its own state → Must split -- JSX over 50 lines → Split -- Reusable → Split -- Multiple responsibilities → Split -- Independent section within page → Split - -**Required Checks:** - -| Criteria | Judgment | -|----------|----------| -| Component over 200 lines | Consider splitting | -| Component over 300 lines | REJECT | -| Display and logic mixed | Consider separation | -| Props drilling (3+ levels) | Consider state management | -| Component with multiple responsibilities | REJECT | - -**Good Component:** -- Single responsibility: Does one thing well -- Self-contained: Dependencies are clear -- Testable: Side effects are isolated - -**Component Classification:** - -| Type | Responsibility | Example | -|------|----------------|---------| -| Container | Data fetching, state management | `UserListContainer` | -| Presentational | Display only | `UserCard` | -| Layout | Arrangement, structure | `PageLayout`, `Grid` | -| Utility | Common functionality | `ErrorBoundary`, `Portal` | - -**Directory Structure:** -``` -features/{feature-name}/ -├── components/ -│ ├── {feature}-view.tsx # Main view (composes children) -│ ├── {sub-component}.tsx # Sub-components -│ └── index.ts -├── hooks/ -├── types.ts -└── index.ts -``` - -### 2. State Management - -**Principle: Child components do not modify their own state. They bubble events to parent, and parent manipulates state.** - -```tsx -// ❌ Child modifies its own state -const ChildBad = ({ initialValue }: { initialValue: string }) => { - const [value, setValue] = useState(initialValue) - return setValue(e.target.value)} /> -} - -// ✅ Parent manages state, child notifies via callback -const ChildGood = ({ value, onChange }: { value: string; onChange: (v: string) => void }) => { - return onChange(e.target.value)} /> -} - -const Parent = () => { - const [value, setValue] = useState('') - return -} -``` - -**Exception (OK for child to have local state):** -- UI-only temporary state (hover, focus, animation) -- Completely local state that doesn't need to be communicated to parent - -**Required Checks:** - -| Criteria | Judgment | -|----------|----------| -| Unnecessary global state | Consider localizing | -| Same state managed in multiple places | Needs normalization | -| State changes from child to parent (reverse data flow) | REJECT | -| API response stored as-is in state | Consider normalization | -| Inappropriate useEffect dependencies | REJECT | - -**State Placement Guidelines:** - -| State Nature | Recommended Placement | -|--------------|----------------------| -| Temporary UI state (modal open/close, etc.) | Local (useState) | -| Form input values | Local or form library | -| Shared across multiple components | Context or state management library | -| Server data cache | Data fetching library (TanStack Query, etc.) | - -### 3. Data Fetching - -**Principle: API calls are made in root (View) components and passed to children via props.** - -```tsx -// ✅ CORRECT - Fetch at root, pass to children -const OrderDetailView = () => { - const { data: order, isLoading, error } = useGetOrder(orderId) - const { data: items } = useListOrderItems(orderId) - - if (isLoading) return - if (error) return - - return ( - - ) -} - -// ❌ WRONG - Child fetches its own data -const OrderSummary = ({ orderId }) => { - const { data: order } = useGetOrder(orderId) - // ... -} -``` - -**Reasons:** -- Data flow is explicit and traceable -- Child components are pure presentation (easier to test) -- No hidden dependencies in child components - -**When UI state changes affect parameters (week switching, filters, etc.):** - -Manage state at View level and pass callbacks to components. - -```tsx -// ✅ CORRECT - State managed at View level -const ScheduleView = () => { - const [currentWeek, setCurrentWeek] = useState(startOfWeek(new Date())) - const { data } = useListSchedules({ - from: format(currentWeek, 'yyyy-MM-dd'), - to: format(endOfWeek(currentWeek), 'yyyy-MM-dd'), - }) - - return ( - - ) -} - -// ❌ WRONG - Component manages state + data fetching -const WeeklyCalendar = ({ facilityId }) => { - const [currentWeek, setCurrentWeek] = useState(...) - const { data } = useListSchedules({ facilityId, from, to }) - // ... -} -``` - -**Exceptions (component-level fetching allowed):** - -| Case | Reason | -|------|--------| -| Infinite scroll | Depends on scroll position (internal UI state) | -| Search autocomplete | Real-time search based on input value | -| Independent widget | Notification badge, weather, etc. Completely unrelated to parent data | -| Real-time updates | WebSocket/Polling auto-updates | -| Modal detail fetch | Fetch additional data only when opened | - -**Decision criteria: "Is there no point in parent managing this / Does it not affect parent?"** - -**Required Checks:** - -| Criteria | Judgment | -|----------|----------| -| Direct fetch in component | Separate to Container layer | -| No error handling | REJECT | -| Loading state not handled | REJECT | -| No cancellation handling | Warning | -| N+1 query-like fetching | REJECT | - -### 4. Shared Components and Abstraction - -**Principle: Common UI patterns should be shared components. Copy-paste of inline styles is prohibited.** - -```tsx -// ❌ WRONG - Copy-pasted inline styles - - -// ✅ CORRECT - Use shared component - - - -``` - -**Patterns to make shared components:** -- Icon buttons (close, edit, delete, etc.) -- Loading/error displays -- Status badges -- Tab switching -- Label + value display (detail screens) -- Search input -- Color legends - -**Avoid over-generalization:** - -```tsx -// ❌ WRONG - Forcing stepper variant into IconButton -export const iconButtonVariants = cva('...', { - variants: { - variant: { - default: '...', - outlined: '...', // ← Stepper-specific, not used elsewhere - }, - size: { - medium: 'p-2', - stepper: 'w-8 h-8', // ← Only used with outlined - }, - }, -}) - -// ✅ CORRECT - Purpose-specific component -export function StepperButton(props) { - return ( - - ) -} -``` - -**Signs to make separate components:** -- Implicit constraints like "this variant is always with this size" -- Added variant is clearly different from original component's purpose -- Props specification becomes complex on the usage side - -### 5. Abstraction Level Evaluation - -**Conditional branch bloat detection:** - -| Pattern | Judgment | -|---------|----------| -| Same conditional in 3+ places | Extract to shared component → **REJECT** | -| Props-based branching with 5+ types | Consider component split | -| Nested ternaries in render | Early return or component separation → **REJECT** | -| Type-based render branching | Consider polymorphic components | - -**Abstraction level mismatch detection:** - -| Pattern | Problem | Fix | -|---------|---------|-----| -| Data fetching logic mixed in JSX | Hard to read | Extract to custom hook | -| Business logic mixed in component | Responsibility violation | Separate to hooks/utils | -| Style calculation logic scattered | Hard to maintain | Extract to utility function | -| Same transformation in multiple places | DRY violation | Extract to common function | - -**Good abstraction examples:** -```tsx -// ❌ Conditional bloat -function UserBadge({ user }) { - if (user.role === 'admin') { - return Admin - } else if (user.role === 'moderator') { - return Moderator - } else if (user.role === 'premium') { - return Premium - } else { - return User - } -} - -// ✅ Abstracted with Map -const ROLE_CONFIG = { - admin: { label: 'Admin', className: 'bg-red-500' }, - moderator: { label: 'Moderator', className: 'bg-yellow-500' }, - premium: { label: 'Premium', className: 'bg-purple-500' }, - default: { label: 'User', className: 'bg-gray-500' }, -} - -function UserBadge({ user }) { - const config = ROLE_CONFIG[user.role] ?? ROLE_CONFIG.default - return {config.label} -} -``` - -```tsx -// ❌ Mixed abstraction levels -function OrderList() { - const [orders, setOrders] = useState([]) - useEffect(() => { - fetch('/api/orders') - .then(res => res.json()) - .then(data => setOrders(data)) - }, []) - - return orders.map(order => ( -
{order.total.toLocaleString()} USD
- )) -} - -// ✅ Aligned abstraction levels -function OrderList() { - const { data: orders } = useOrders() // Hide data fetching - - return orders.map(order => ( - - )) -} -``` - -### 6. Frontend and Backend Separation of Concerns - -#### 6.1 Display Format Responsibility - -**Principle: Backend returns "data", frontend converts to "display format".** - -```tsx -// ✅ Frontend: Convert to display format -export function formatPrice(amount: number): string { - return `$${amount.toLocaleString()}` -} - -export function formatDate(date: Date): string { - return format(date, 'MMM d, yyyy') -} -``` - -**Reasons:** -- Display format is a UI concern, not backend responsibility -- Easy to support internationalization -- Frontend can flexibly change display - -**Required Checks:** - -| Criteria | Judgment | -|----------|----------| -| Backend returns display strings | Suggest design review | -| Same format logic copy-pasted | Unify to utility function | -| Inline formatting in component | Extract to function | - -#### 6.2 Domain Logic Placement (Smart UI Elimination) - -**Principle: Domain logic (business rules) belongs in the backend. Frontend only displays and edits state.** - -**What is domain logic:** -- Aggregate business rules (stock validation, price calculation, status transitions) -- Business constraint validation -- Invariant enforcement - -**Frontend responsibilities:** -- Display state received from server -- Collect user input and send commands to backend -- Manage UI-only temporary state (focus, hover, modal open/close) -- Display format conversion (formatting, sorting, filtering) - -**Required Checks:** - -| Criteria | Judgment | -|----------|----------| -| Price calculation/stock validation in frontend | Move to backend → **REJECT** | -| Status transition rules in frontend | Move to backend → **REJECT** | -| Business validation in frontend | Move to backend → **REJECT** | -| Recalculating server-computable values in frontend | Redundant → **REJECT** | - -**Good vs Bad Examples:** - -```tsx -// ❌ BAD - Business rules in frontend -function OrderForm({ order }: { order: Order }) { - const totalPrice = order.items.reduce((sum, item) => - sum + item.price * item.quantity, 0 - ) - const canCheckout = totalPrice >= 100 && order.items.every(i => i.stock > 0) - - return -} - -// ✅ GOOD - Display state received from server -function OrderForm({ order }: { order: Order }) { - // totalPrice, canCheckout are received from server - return ( - <> -
{formatPrice(order.totalPrice)}
- - - ) -} -``` - -```tsx -// ❌ BAD - Status transition logic in frontend -function TaskCard({ task }: { task: Task }) { - const canStart = task.status === 'pending' && task.assignee !== null - const canComplete = task.status === 'in_progress' && /* complex conditions... */ - - return ( - <> - - - - ) -} - -// ✅ GOOD - Server returns allowed actions -function TaskCard({ task }: { task: Task }) { - // task.allowedActions = ['start', 'cancel'], etc., calculated by server - const canStart = task.allowedActions.includes('start') - const canComplete = task.allowedActions.includes('complete') - - return ( - <> - - - - ) -} -``` - -**Exceptions (OK to have logic in frontend):** - -| Case | Reason | -|------|--------| -| UI-only validation | UX feedback like "required field", "max length" (must also validate on server) | -| Client-side filter/sort | Changing display order of lists received from server | -| Display condition branching | UI control like "show details if logged in" | -| Real-time feedback | Preview display during input | - -**Decision criteria: "Would the business break if this calculation differs from the server?"** -- YES → Place in backend (domain logic) -- NO → OK in frontend (display logic) - -### 7. Performance - -**Required Checks:** - -| Criteria | Judgment | -|----------|----------| -| Unnecessary re-renders | Needs optimization | -| Large lists without virtualization | Warning | -| Unoptimized images | Warning | -| Unused code in bundle | Check tree-shaking | -| Excessive memoization | Verify necessity | - -**Optimization Checklist:** -- [ ] Are `React.memo` / `useMemo` / `useCallback` appropriate? -- [ ] Are large lists using virtual scroll? -- [ ] Is Code Splitting appropriate? -- [ ] Are images lazy loaded? - -**Anti-patterns:** - -```tsx -// ❌ New object every render - - -// ✅ Constant or useMemo -const style = useMemo(() => ({ color: 'red' }), []); - -``` - -### 8. Accessibility - -**Required Checks:** - -| Criteria | Judgment | -|----------|----------| -| Interactive elements without keyboard support | REJECT | -| Images without alt attribute | REJECT | -| Form elements without labels | REJECT | -| Information conveyed by color only | REJECT | -| Missing focus management (modals, etc.) | REJECT | - -**Checklist:** -- [ ] Using semantic HTML? -- [ ] Are ARIA attributes appropriate (not excessive)? -- [ ] Is keyboard navigation possible? -- [ ] Does it make sense with a screen reader? -- [ ] Is color contrast sufficient? - -### 9. TypeScript/Type Safety - -**Required Checks:** - -| Criteria | Judgment | -|----------|----------| -| Use of `any` type | REJECT | -| Excessive type assertions (as) | Needs review | -| No Props type definition | REJECT | -| Inappropriate event handler types | Needs fix | - -### 10. Frontend Security - -**Required Checks:** - -| Criteria | Judgment | -|----------|----------| -| dangerouslySetInnerHTML usage | Check XSS risk | -| Unsanitized user input | REJECT | -| Sensitive data stored in frontend | REJECT | -| CSRF token not used | Needs verification | - -### 11. Testability - -**Required Checks:** - -| Criteria | Judgment | -|----------|----------| -| No data-testid, etc. | Warning | -| Structure difficult to test | Consider separation | -| Business logic embedded in UI | REJECT | - -### 12. Anti-pattern Detection - -**REJECT** if found: - -| Anti-pattern | Problem | -|--------------|---------| -| God Component | All features concentrated in one component | -| Prop Drilling | Deep props bucket brigade | -| Inline Styles abuse | Maintainability degradation | -| useEffect hell | Dependencies too complex | -| Premature Optimization | Unnecessary memoization | -| Magic Strings | Hardcoded strings | -| Hidden Dependencies | Child components with hidden API calls | -| Over-generalization | Components forced to be generic | - ## Important - **Prioritize user experience**: UX over technical correctness diff --git a/resources/global/en/personas/security-reviewer.md b/resources/global/en/personas/security-reviewer.md index 26ee249..1c475cf 100644 --- a/resources/global/en/personas/security-reviewer.md +++ b/resources/global/en/personas/security-reviewer.md @@ -30,164 +30,6 @@ Security cannot be retrofitted. It must be built in from the design stage; "we'l - Write code yourself (only provide feedback and fix suggestions) - Review design or code quality (that's Architect's role) -## AI-Generated Code: Special Attention - -AI-generated code has unique vulnerability patterns. - -**Common security issues in AI-generated code:** - -| Pattern | Risk | Example | -|---------|------|---------| -| Plausible but dangerous defaults | High | `cors: { origin: '*' }` looks fine but is dangerous | -| Outdated security practices | Medium | Using deprecated encryption, old auth patterns | -| Incomplete validation | High | Validates format but not business rules | -| Over-trusting inputs | Critical | Assumes internal APIs are always safe | -| Copy-paste vulnerabilities | High | Same dangerous pattern repeated in multiple files | - -**Require extra scrutiny:** -- Auth/authorization logic (AI tends to miss edge cases) -- Input validation (AI may check syntax but miss semantics) -- Error messages (AI may expose internal details) -- Config files (AI may use dangerous defaults from training data) - -## Review Perspectives - -### 1. Injection Attacks - -**SQL Injection:** -- SQL construction via string concatenation → **REJECT** -- Not using parameterized queries → **REJECT** -- Unsanitized input in ORM raw queries → **REJECT** - -```typescript -// NG -db.query(`SELECT * FROM users WHERE id = ${userId}`) - -// OK -db.query('SELECT * FROM users WHERE id = ?', [userId]) -``` - -**Command Injection:** -- Unvalidated input in `exec()`, `spawn()` → **REJECT** -- Insufficient escaping in shell command construction → **REJECT** - -```typescript -// NG -exec(`ls ${userInput}`) - -// OK -execFile('ls', [sanitizedInput]) -``` - -**XSS (Cross-Site Scripting):** -- Unescaped output to HTML/JS → **REJECT** -- Improper use of `innerHTML`, `dangerouslySetInnerHTML` → **REJECT** -- Direct embedding of URL parameters → **REJECT** - -### 2. Authentication & Authorization - -**Authentication issues:** -- Hardcoded credentials → **Immediate REJECT** -- Plaintext password storage → **Immediate REJECT** -- Weak hash algorithms (MD5, SHA1) → **REJECT** -- Improper session token management → **REJECT** - -**Authorization issues:** -- Missing permission checks → **REJECT** -- IDOR (Insecure Direct Object Reference) → **REJECT** -- Privilege escalation possibility → **REJECT** - -```typescript -// NG - No permission check -app.get('/user/:id', (req, res) => { - return db.getUser(req.params.id) -}) - -// OK -app.get('/user/:id', authorize('read:user'), (req, res) => { - if (req.user.id !== req.params.id && !req.user.isAdmin) { - return res.status(403).send('Forbidden') - } - return db.getUser(req.params.id) -}) -``` - -### 3. Data Protection - -**Sensitive information exposure:** -- Hardcoded API keys, secrets → **Immediate REJECT** -- Sensitive info in logs → **REJECT** -- Internal info exposure in error messages → **REJECT** -- Committed `.env` files → **REJECT** - -**Data validation:** -- Unvalidated input values → **REJECT** -- Missing type checks → **REJECT** -- No size limits set → **REJECT** - -### 4. Cryptography - -- Use of weak crypto algorithms → **REJECT** -- Fixed IV/Nonce usage → **REJECT** -- Hardcoded encryption keys → **Immediate REJECT** -- No HTTPS (production) → **REJECT** - -### 5. File Operations - -**Path Traversal:** -- File paths containing user input → **REJECT** -- Insufficient `../` sanitization → **REJECT** - -```typescript -// NG -const filePath = path.join(baseDir, userInput) -fs.readFile(filePath) - -// OK -const safePath = path.resolve(baseDir, userInput) -if (!safePath.startsWith(path.resolve(baseDir))) { - throw new Error('Invalid path') -} -``` - -**File Upload:** -- No file type validation → **REJECT** -- No file size limits → **REJECT** -- Allowing executable file uploads → **REJECT** - -### 6. Dependencies - -- Packages with known vulnerabilities → **REJECT** -- Unmaintained packages → Warning -- Unnecessary dependencies → Warning - -### 7. Error Handling - -- Stack trace exposure in production → **REJECT** -- Detailed error message exposure → **REJECT** -- Swallowing security events → **REJECT** - -### 8. Rate Limiting & DoS Protection - -- No rate limiting (auth endpoints) → Warning -- Resource exhaustion attack possibility → Warning -- Infinite loop possibility → **REJECT** - -### 9. OWASP Top 10 Checklist - -| Category | Check Items | -|----------|-------------| -| A01 Broken Access Control | Authorization checks, CORS config | -| A02 Cryptographic Failures | Encryption, sensitive data protection | -| A03 Injection | SQL, Command, XSS | -| A04 Insecure Design | Security design patterns | -| A05 Security Misconfiguration | Default settings, unnecessary features | -| A06 Vulnerable Components | Dependency vulnerabilities | -| A07 Auth Failures | Authentication mechanisms | -| A08 Software Integrity | Code signing, CI/CD | -| A09 Logging Failures | Security logging | -| A10 SSRF | Server-side requests | - ## Important **Don't miss anything**: Security vulnerabilities get exploited in production. One oversight can lead to a critical incident. diff --git a/resources/global/en/pieces/default.yaml b/resources/global/en/pieces/default.yaml index ec2d547..60b7985 100644 --- a/resources/global/en/pieces/default.yaml +++ b/resources/global/en/pieces/default.yaml @@ -20,6 +20,9 @@ stances: review: ../stances/review.md testing: ../stances/testing.md +knowledge: + architecture: ../knowledge/architecture.md + personas: planner: ../personas/planner.md architect-planner: ../personas/architect-planner.md @@ -224,6 +227,7 @@ movements: edit: false persona: architecture-reviewer stance: review + knowledge: architecture report: name: 05-architect-review.md format: architecture-review diff --git a/resources/global/en/pieces/expert-cqrs.yaml b/resources/global/en/pieces/expert-cqrs.yaml index 72f92c8..e79ae21 100644 --- a/resources/global/en/pieces/expert-cqrs.yaml +++ b/resources/global/en/pieces/expert-cqrs.yaml @@ -28,6 +28,11 @@ stances: review: ../stances/review.md testing: ../stances/testing.md +knowledge: + frontend: ../knowledge/frontend.md + cqrs-es: ../knowledge/cqrs-es.md + security: ../knowledge/security.md + personas: planner: ../personas/planner.md coder: ../personas/coder.md @@ -196,6 +201,7 @@ movements: edit: false persona: cqrs-es-reviewer stance: review + knowledge: cqrs-es report: name: 04-cqrs-es-review.md format: cqrs-es-review @@ -215,6 +221,7 @@ movements: edit: false persona: frontend-reviewer stance: review + knowledge: frontend report: name: 05-frontend-review.md format: frontend-review @@ -234,6 +241,7 @@ movements: edit: false persona: security-reviewer stance: review + knowledge: security report: name: 06-security-review.md format: security-review diff --git a/resources/global/en/pieces/expert.yaml b/resources/global/en/pieces/expert.yaml index fed3e2e..37c73a8 100644 --- a/resources/global/en/pieces/expert.yaml +++ b/resources/global/en/pieces/expert.yaml @@ -28,6 +28,11 @@ stances: review: ../stances/review.md testing: ../stances/testing.md +knowledge: + frontend: ../knowledge/frontend.md + security: ../knowledge/security.md + architecture: ../knowledge/architecture.md + personas: planner: ../personas/planner.md coder: ../personas/coder.md @@ -195,6 +200,7 @@ movements: edit: false persona: architecture-reviewer stance: review + knowledge: architecture report: name: 04-architect-review.md format: architecture-review @@ -214,6 +220,7 @@ movements: edit: false persona: frontend-reviewer stance: review + knowledge: frontend report: name: 05-frontend-review.md format: frontend-review @@ -233,6 +240,7 @@ movements: edit: false persona: security-reviewer stance: review + knowledge: security report: name: 06-security-review.md format: security-review diff --git a/resources/global/ja/knowledge/.gitkeep b/resources/global/ja/knowledge/.gitkeep deleted file mode 100644 index e69de29..0000000 diff --git a/resources/global/ja/knowledge/architecture.md b/resources/global/ja/knowledge/architecture.md new file mode 100644 index 0000000..7527100 --- /dev/null +++ b/resources/global/ja/knowledge/architecture.md @@ -0,0 +1,427 @@ +# アーキテクチャ知識 + +## 構造・設計 + +**ファイル分割** + +| 基準 | 判定 | +|--------------|------| +| 1ファイル200行超 | 分割を検討 | +| 1ファイル300行超 | REJECT | +| 1ファイルに複数の責務 | REJECT | +| 関連性の低いコードが同居 | REJECT | + +**モジュール構成** + +- 高凝集: 関連する機能がまとまっているか +- 低結合: モジュール間の依存が最小限か +- 循環依存がないか +- 適切なディレクトリ階層か + +**関数設計** + +- 1関数1責務になっているか +- 30行を超える関数は分割を検討 +- 副作用が明確か + +**レイヤー設計** + +- 依存の方向: 上位層 → 下位層（逆方向禁止） +- Controller → Service → Repository の流れが守られているか +- 1インターフェース = 1責務（巨大なServiceクラス禁止） + +**ディレクトリ構造** + +構造パターンの選択: + +| パターン | 適用場面 | 例 | +|---------|---------|-----| +| レイヤード | 小規模、CRUD中心 | `controllers/`, `services/`, `repositories/` | +| Vertical Slice | 中〜大規模、機能独立性が高い | `features/auth/`, `features/order/` | +| ハイブリッド | 共通基盤 + 機能モジュール | `core/` + `features/` | + +Vertical Slice Architecture（機能単位でコードをまとめる構造）: + +``` +src/ +├── features/ +│ ├── auth/ +│ │ ├── LoginCommand.ts +│ │ ├── LoginHandler.ts +│ │ ├── AuthRepository.ts +│ │ └── auth.test.ts +│ └── order/ +│ ├── CreateOrderCommand.ts +│ ├── CreateOrderHandler.ts +│ └── ... +└── shared/ # 複数featureで共有 + ├── database/ + └── middleware/ +``` + +Vertical Slice の判定基準: + +| 基準 | 判定 | +|------|------| +| 1機能が3ファイル以上のレイヤーに跨る | Slice化を検討 | +| 機能間の依存がほぼない | Slice化推奨 | +| 共通処理が50%以上 | レイヤード維持 | +| チームが機能別に分かれている | Slice化必須 | + +禁止パターン: + +| パターン | 問題 | +|---------|------| +| `utils/` の肥大化 | 責務不明の墓場になる | +| `common/` への安易な配置 | 依存関係が不明確になる | +| 深すぎるネスト（4階層超） | ナビゲーション困難 | +| 機能とレイヤーの混在 | `features/services/` は禁止 | + +**責務の分離** + +- 読み取りと書き込みの責務が分かれているか +- データ取得はルート（View/Controller）で行い、子に渡しているか +- エラーハンドリングが一元化されているか（各所でtry-catch禁止） +- ビジネスロジックがController/Viewに漏れていないか + +## コード品質の検出手法 + +**説明コメント（What/How）の検出基準** + +コードの動作をそのまま言い換えているコメントを検出する。 + +| 判定 | 基準 | +|------|------| +| REJECT | コードの動作をそのまま自然言語で言い換えている | +| REJECT | 関数名・変数名から明らかなことを繰り返している | +| REJECT | JSDocが関数名の言い換えだけで情報を追加していない | +| OK | なぜその実装を選んだかの設計判断を説明している | +| OK | 一見不自然に見える挙動の理由を説明している | +| 最良 | コメントなしでコード自体が意図を語っている | + +```typescript +// REJECT - コードの言い換え（What） +// If interrupted, abort immediately +if (status === 'interrupted') { + return ABORT_STEP; +} + +// REJECT - ループの存在を言い換えただけ +// Check transitions in order +for (const transition of step.transitions) { + +// REJECT - 関数名の繰り返し +/** Check if status matches transition condition. */ +export function matchesCondition(status: Status, condition: TransitionCondition): boolean { + +// OK - 設計判断の理由（Why） +// ユーザー中断はピース定義のトランジションより優先する +if (status === 'interrupted') { + return ABORT_STEP; +} + +// OK - 一見不自然な挙動の理由 +// stay はループを引き起こす可能性があるが、ユーザーが明示的に指定した場合のみ使われる +return step.name; +``` + +**状態の直接変更の検出基準** + +配列やオブジェクトの直接変更（ミューテーション）を検出する。 + +```typescript +// REJECT - 配列の直接変更 +const steps: Step[] = getSteps(); +steps.push(newStep); // 元の配列を破壊 +steps.splice(index, 1); // 元の配列を破壊 +steps[0].status = 'done'; // ネストされたオブジェクトも直接変更 + +// OK - イミュータブルな操作 +const withNew = [...steps, newStep]; +const without = steps.filter((_, i) => i !== index); +const updated = steps.map((s, i) => + i === 0 ? { ...s, status: 'done' } : s +); + +// REJECT - オブジェクトの直接変更 +function updateConfig(config: Config) { + config.logLevel = 'debug'; // 引数を直接変更 + config.steps.push(newStep); // ネストも直接変更 + return config; +} + +// OK - 新しいオブジェクトを返す +function updateConfig(config: Config): Config { + return { + ...config, + logLevel: 'debug', + steps: [...config.steps, newStep], + }; +} +``` + +## セキュリティ（基本チェック） + +- インジェクション対策（SQL, コマンド, XSS） +- ユーザー入力の検証 +- 機密情報のハードコーディング + +## テスタビリティ + +- 依存性注入が可能な設計か +- モック可能か +- テストが書かれているか + +## アンチパターン検出 + +以下のパターンを見つけたら REJECT: + +| アンチパターン | 問題 | +|---------------|------| +| God Class/Component | 1つのクラスが多くの責務を持っている | +| Feature Envy | 他モジュールのデータを頻繁に参照している | +| Shotgun Surgery | 1つの変更が複数ファイルに波及する構造 | +| 過度な汎用化 | 今使わないバリアントや拡張ポイント | +| 隠れた依存 | 子コンポーネントが暗黙的にAPIを呼ぶ等 | +| 非イディオマティック | 言語・FWの作法を無視した独自実装 | + +## 抽象化レベルの評価 + +**条件分岐の肥大化検出** + +| パターン | 判定 | +|---------|------| +| 同じif-elseパターンが3箇所以上 | ポリモーフィズムで抽象化 → REJECT | +| switch/caseが5分岐以上 | Strategy/Mapパターンを検討 | +| フラグ引数で挙動を変える | 別関数に分割 → REJECT | +| 型による分岐（instanceof/typeof） | ポリモーフィズムに置換 → REJECT | +| ネストした条件分岐（3段以上） | 早期リターンまたは抽出 → REJECT | + +**抽象度の不一致検出** + +| パターン | 問題 | 修正案 | +|---------|------|--------| +| 高レベル処理の中に低レベル詳細 | 読みにくい | 詳細を関数に抽出 | +| 1関数内で抽象度が混在 | 認知負荷 | 同じ粒度に揃える | +| ビジネスロジックにDB操作が混在 | 責務違反 | Repository層に分離 | +| 設定値と処理ロジックが混在 | 変更困難 | 設定を外部化 | + +**良い抽象化の例** + +```typescript +// 条件分岐の肥大化 +function process(type: string) { + if (type === 'A') { /* 処理A */ } + else if (type === 'B') { /* 処理B */ } + else if (type === 'C') { /* 処理C */ } + // ...続く +} + +// Mapパターンで抽象化 +const processors: Record void> = { + A: processA, + B: processB, + C: processC, +}; +function process(type: string) { + processors[type]?.(); +} +``` + +```typescript +// 抽象度の混在 +function createUser(data: UserData) { + // 高レベル: ビジネスロジック + validateUser(data); + // 低レベル: DB操作の詳細 + const conn = await pool.getConnection(); + await conn.query('INSERT INTO users...'); + conn.release(); +} + +// 抽象度を揃える +function createUser(data: UserData) { + validateUser(data); + await userRepository.save(data); // 詳細は隠蔽 +} +``` + +## その場しのぎの検出 + +「とりあえず動かす」ための妥協を見逃さない。 + +| パターン | 例 | +|---------|-----| +| 不要なパッケージ追加 | 動かすためだけに入れた謎のライブラリ | +| テストの削除・スキップ | `@Disabled`、`.skip()`、コメントアウト | +| 空実装・スタブ放置 | `return null`、`// TODO: implement`、`pass` | +| モックデータの本番混入 | ハードコードされたダミーデータ | +| エラー握りつぶし | 空の `catch {}`、`rescue nil` | +| マジックナンバー | 説明なしの `if (status == 3)` | + +## TODOコメントの厳格な禁止 + +「将来やる」は決してやらない。今やらないことは永遠にやらない。 + +TODOコメントは即REJECT。 + +```kotlin +// REJECT - 将来を見越したTODO +// TODO: 施設IDによる認可チェックを追加 +fun deleteCustomHoliday(@PathVariable id: String) { + deleteCustomHolidayInputPort.execute(input) +} + +// APPROVE - 今実装する +fun deleteCustomHoliday(@PathVariable id: String) { + val currentUserFacilityId = getCurrentUserFacilityId() + val holiday = findHolidayById(id) + require(holiday.facilityId == currentUserFacilityId) { + "Cannot delete holiday from another facility" + } + deleteCustomHolidayInputPort.execute(input) +} +``` + +TODOが許容される唯一のケース: + +| 条件 | 例 | 判定 | +|------|-----|------| +| 外部依存で今は実装不可 + Issue化済み | `// TODO(#123): APIキー取得後に実装` | 許容 | +| 技術的制約で回避不可 + Issue化済み | `// TODO(#456): ライブラリバグ修正待ち` | 許容 | +| 「将来実装」「後で追加」 | `// TODO: バリデーション追加` | REJECT | +| 「時間がないので」 | `// TODO: リファクタリング` | REJECT | + +正しい対処: +- 今必要 → 今実装する +- 今不要 → コードを削除する +- 外部要因で不可 → Issue化してチケット番号をコメントに入れる + +## DRY違反の検出 + +重複コードを検出する。 + +| パターン | 判定 | +|---------|------| +| 同じロジックが3箇所以上 | 即REJECT - 関数/メソッドに抽出 | +| 同じバリデーションが2箇所以上 | 即REJECT - バリデーター関数に抽出 | +| 似たようなコンポーネントが3個以上 | 即REJECT - 共通コンポーネント化 | +| コピペで派生したコード | 即REJECT - パラメータ化または抽象化 | + +AHA原則（Avoid Hasty Abstractions）とのバランス: +- 2回の重複 → 様子見 +- 3回の重複 → 即抽出 +- ドメインが異なる重複 → 抽象化しない（例: 顧客用バリデーションと管理者用バリデーションは別物） + +## 仕様準拠の検証 + +変更が、プロジェクトの文書化された仕様に準拠しているか検証する。 + +検証対象: + +| 対象 | 確認内容 | +|------|---------| +| CLAUDE.md / README.md | スキーマ定義、設計原則、制約に従っているか | +| 型定義・Zodスキーマ | 新しいフィールドがスキーマに反映されているか | +| YAML/JSON設定ファイル | 文書化されたフォーマットに従っているか | + +具体的なチェック: + +1. 設定ファイル（YAML等）を変更・追加した場合: + - CLAUDE.md等に記載されたスキーマ定義と突合する + - 無視されるフィールドや無効なフィールドが含まれていないか + - 必須フィールドが欠落していないか + +2. 型定義やインターフェースを変更した場合: + - ドキュメントのスキーマ説明が更新されているか + - 既存の設定ファイルが新しいスキーマと整合するか + +このパターンを見つけたら REJECT: + +| パターン | 問題 | +|---------|------| +| 仕様に存在しないフィールドの使用 | 無視されるか予期しない動作 | +| 仕様上無効な値の設定 | 実行時エラーまたは無視される | +| 文書化された制約への違反 | 設計意図に反する | + +## 呼び出しチェーン検証 + +新しいパラメータ・フィールドが追加された場合、変更ファイル内だけでなく呼び出し元も検証する。 + +検証手順: +1. 新しいオプショナルパラメータや interface フィールドを見つけたら、`Grep` で全呼び出し元を検索 +2. 全呼び出し元が新しいパラメータを渡しているか確認 +3. フォールバック値（`?? default`）がある場合、フォールバックが使われるケースが意図通りか確認 + +危険パターン: + +| パターン | 問題 | 検出方法 | +|---------|------|---------| +| `options.xxx ?? fallback` で全呼び出し元が `xxx` を省略 | 機能が実装されているのに常にフォールバック | grep で呼び出し元を確認 | +| テストがモックで直接値をセット | 実際の呼び出しチェーンを経由しない | テストの構築方法を確認 | +| `executeXxx()` が内部で使う `options` を引数で受け取らない | 上位から値を渡す口がない | 関数シグネチャを確認 | + +```typescript +// 配線漏れ: projectCwd を受け取る口がない +export async function executePiece(config, cwd, task) { + const engine = new PieceEngine(config, cwd, task); // options なし +} + +// 配線済み: projectCwd を渡せる +export async function executePiece(config, cwd, task, options?) { + const engine = new PieceEngine(config, cwd, task, options); +} +``` + +呼び出し元の制約による論理的デッドコード: + +呼び出しチェーンの検証は「配線漏れ」だけでなく、逆方向——呼び出し元が既に保証している条件に対する不要な防御コード——にも適用する。 + +| パターン | 問題 | 検出方法 | +|---------|------|---------| +| 呼び出し元がTTY必須なのに関数内でTTYチェック | 到達しない分岐が残る | grep で全呼び出し元の前提条件を確認 | +| 呼び出し元がnullチェック済みなのに再度nullガード | 冗長な防御 | 呼び出し元の制約を追跡 | +| 呼び出し元が型で制約しているのにランタイムチェック | 型安全を信頼していない | TypeScriptの型制約を確認 | + +検証手順: +1. 防御的な条件分岐（TTYチェック、nullガード等）を見つけたら、grep で全呼び出し元を確認 +2. 全呼び出し元がその条件を既に保証しているなら、防御は不要 → REJECT +3. 一部の呼び出し元が保証していない場合は、防御を残す + +## 品質特性 + +| 特性 | 確認観点 | +|------|---------| +| Scalability | 負荷増加に対応できる設計か | +| Maintainability | 変更・修正が容易か | +| Observability | ログ・監視が可能な設計か | + +## 大局観 + +細かい「クリーンコード」の指摘に終始しない。 + +確認すべきこと: +- このコードは将来どう変化するか +- スケーリングの必要性は考慮されているか +- 技術的負債を生んでいないか +- ビジネス要件と整合しているか +- 命名がドメインと一貫しているか + +## 変更スコープの評価 + +変更スコープを確認し、レポートに記載する（ブロッキングではない）。 + +| スコープサイズ | 変更行数 | 対応 | +|---------------|---------|------| +| Small | 〜200行 | そのままレビュー | +| Medium | 200-500行 | そのままレビュー | +| Large | 500行以上 | レビューは継続。分割可能か提案を付記 | + +大きな変更が必要なタスクもある。行数だけでREJECTしない。 + +確認すること: +- 変更が論理的にまとまっているか（無関係な変更が混在していないか） +- Coderのスコープ宣言と実際の変更が一致しているか + +提案として記載すること（ブロッキングではない）: +- 分割可能な場合は分割案を提示 diff --git a/resources/global/ja/knowledge/cqrs-es.md b/resources/global/ja/knowledge/cqrs-es.md new file mode 100644 index 0000000..2579666 --- /dev/null +++ b/resources/global/ja/knowledge/cqrs-es.md @@ -0,0 +1,417 @@ +# CQRS+ES知識 + +## Aggregate設計 + +Aggregateは判断に必要なフィールドのみ保持する。 + +Command Model（Aggregate）の役割は「コマンドを受けて判断し、イベントを発行する」こと。クエリ用データはRead Model（Projection）が担当する。 + +「判断に必要」とは: +- `if`/`require`の条件分岐に使う +- インスタンスメソッドでイベント発行時にフィールド値を参照する + +| 基準 | 判定 | +|------|------| +| Aggregateが複数のトランザクション境界を跨ぐ | REJECT | +| Aggregate間の直接参照（ID参照でない） | REJECT | +| Aggregateが100行を超える | 分割を検討 | +| ビジネス不変条件がAggregate外にある | REJECT | +| 判断に使わないフィールドを保持 | REJECT | + +良いAggregate: +```kotlin +// 判断に必要なフィールドのみ +data class Order( + val orderId: String, // イベント発行時に使用 + val status: OrderStatus // 状態チェックに使用 +) { + fun confirm(confirmedBy: String): OrderConfirmedEvent { + require(status == OrderStatus.PENDING) { "確定できる状態ではありません" } + return OrderConfirmedEvent( + orderId = orderId, + confirmedBy = confirmedBy, + confirmedAt = LocalDateTime.now() + ) + } +} + +// 判断に使わないフィールドを保持（NG） +data class Order( + val orderId: String, + val customerId: String, // 判断に未使用 + val shippingAddress: Address, // 判断に未使用 + val status: OrderStatus +) +``` + +追加操作がないAggregateはIDのみ: +```kotlin +// 作成のみで追加操作がない場合 +data class Notification(val notificationId: String) { + companion object { + fun create(customerId: String, message: String): NotificationCreatedEvent { + return NotificationCreatedEvent( + notificationId = UUID.randomUUID().toString(), + customerId = customerId, + message = message + ) + } + } +} +``` + +## イベント設計 + +| 基準 | 判定 | +|------|------| +| イベントが過去形でない（Created → Create） | REJECT | +| イベントにロジックが含まれる | REJECT | +| イベントが他Aggregateの内部状態を含む | REJECT | +| イベントのスキーマがバージョン管理されていない | 警告 | +| CRUDスタイルのイベント（Updated, Deleted） | 要検討 | + +良いイベント: +```kotlin +// Good: ドメインの意図が明確 +OrderPlaced, PaymentReceived, ItemShipped + +// Bad: CRUDスタイル +OrderUpdated, OrderDeleted +``` + +イベント粒度: +- 細かすぎ: `OrderFieldChanged` → ドメインの意図が不明 +- 適切: `ShippingAddressChanged` → 意図が明確 +- 粗すぎ: `OrderModified` → 何が変わったか不明 + +## コマンドハンドラ + +| 基準 | 判定 | +|------|------| +| ハンドラがDBを直接操作 | REJECT | +| ハンドラが複数Aggregateを変更 | REJECT | +| コマンドのバリデーションがない | REJECT | +| ハンドラがクエリを実行して判断 | 要検討 | + +良いコマンドハンドラ: +``` +1. コマンドを受け取る +2. Aggregateをイベントストアから復元 +3. Aggregateにコマンドを適用 +4. 発行されたイベントを保存 +``` + +## プロジェクション設計 + +| 基準 | 判定 | +|------|------| +| プロジェクションがコマンドを発行 | REJECT | +| プロジェクションがWriteモデルを参照 | REJECT | +| 複数のユースケースを1つのプロジェクションで賄う | 要検討 | +| リビルド不可能な設計 | REJECT | + +良いプロジェクション: +- 特定の読み取りユースケースに最適化 +- イベントから冪等に再構築可能 +- Writeモデルから完全に独立 + +## Query側の設計 + +ControllerはQueryGatewayを使う。Repositoryを直接使わない。 + +レイヤー間の型: +- `application/query/` - Query結果の型（例: `OrderDetail`） +- `adapter/protocol/` - RESTレスポンスの型（例: `OrderDetailResponse`） +- QueryHandlerはapplication層の型を返し、Controllerがadapter層の型に変換 + +```kotlin +// application/query/OrderDetail.kt +data class OrderDetail( + val orderId: String, + val customerName: String, + val totalAmount: Money +) + +// adapter/protocol/OrderDetailResponse.kt +data class OrderDetailResponse(...) { + companion object { + fun from(detail: OrderDetail) = OrderDetailResponse(...) + } +} + +// QueryHandler - application層の型を返す +@QueryHandler +fun handle(query: GetOrderDetailQuery): OrderDetail? { + val entity = repository.findById(query.id) ?: return null + return OrderDetail(...) +} + +// Controller - adapter層の型に変換 +@GetMapping("/{id}") +fun getById(@PathVariable id: String): ResponseEntity { + val detail = queryGateway.query( + GetOrderDetailQuery(id), + OrderDetail::class.java + ).join() ?: throw NotFoundException("...") + + return ResponseEntity.ok(OrderDetailResponse.from(detail)) +} +``` + +構成: +``` +Controller (adapter) → QueryGateway → QueryHandler (application) → Repository + ↓ ↓ +Response.from(detail) OrderDetail +``` + +## 結果整合性 + +| 状況 | 対応 | +|------|------| +| UIが即座に更新を期待している | 設計見直し or ポーリング/WebSocket | +| 整合性遅延が許容範囲を超える | アーキテクチャ再検討 | +| 補償トランザクションが未定義 | 障害シナリオの検討を要求 | + +## Saga vs EventHandler + +Sagaは「競合が発生する複数アグリゲート間の操作」にのみ使用する。 + +Sagaが必要なケース: +``` +複数のアクターが同じリソースを取り合う場合 +例: 在庫確保（10人が同時に同じ商品を注文） + +OrderPlacedEvent + ↓ InventoryReservationSaga +ReserveInventoryCommand → Inventory集約（同時実行を直列化） + ↓ +InventoryReservedEvent → ConfirmOrderCommand +InventoryReservationFailedEvent → CancelOrderCommand +``` + +Sagaが不要なケース: +``` +競合が発生しない操作 +例: 注文キャンセル時の在庫解放 + +OrderCancelledEvent + ↓ InventoryReleaseHandler（単純なEventHandler） +ReleaseInventoryCommand + ↓ +InventoryReleasedEvent +``` + +判断基準: + +| 状況 | Saga | EventHandler | +|------|------|--------------| +| リソースの取り合いがある | 使う | - | +| 補償トランザクションが必要 | 使う | - | +| 競合しない単純な連携 | - | 使う | +| 失敗時は再試行で十分 | - | 使う | + +アンチパターン: +```kotlin +// NG - ライフサイクル管理のためにSagaを使う +@Saga +class OrderLifecycleSaga { + // 注文の全状態遷移をSagaで追跡 + // PLACED → CONFIRMED → SHIPPED → DELIVERED +} + +// OK - 結果整合性が必要な操作だけをSagaで処理 +@Saga +class InventoryReservationSaga { + // 在庫確保の同時実行制御のみ +} +``` + +Sagaはライフサイクル管理ツールではない。結果整合性が必要な「操作」単位で作成する。 + +## 例外 vs イベント（失敗時の選択） + +監査不要な失敗は例外、監査が必要な失敗はイベント。 + +例外アプローチ（推奨: ほとんどのケース）: +```kotlin +// ドメインモデル: バリデーション失敗時に例外をスロー +fun reserveInventory(orderId: String, quantity: Int): InventoryReservedEvent { + if (availableQuantity < quantity) { + throw InsufficientInventoryException("在庫が不足しています") + } + return InventoryReservedEvent(productId, orderId, quantity) +} + +// Saga: exceptionally でキャッチして補償アクション +commandGateway.send(command) + .exceptionally { ex -> + commandGateway.send(CancelOrderCommand( + orderId = orderId, + reason = ex.cause?.message ?: "在庫確保に失敗しました" + )) + null + } +``` + +イベントアプローチ（稀なケース）: +```kotlin +// 監査が必要な場合のみ +data class PaymentFailedEvent( + val paymentId: String, + val reason: String, + val attemptedAmount: Money +) : PaymentEvent +``` + +判断基準: + +| 質問 | 例外 | イベント | +|------|------|----------| +| この失敗を後で確認する必要があるか? | No | Yes | +| 規制やコンプライアンスで記録が必要か? | No | Yes | +| Sagaだけが失敗を気にするか? | Yes | No | +| Event Storeに残すと価値があるか? | No | Yes | + +デフォルトは例外アプローチ。監査要件がある場合のみイベントを検討する。 + +## 抽象化レベルの評価 + +**条件分岐の肥大化検出** + +| パターン | 判定 | +|---------|------| +| 同じif-elseパターンが3箇所以上 | ポリモーフィズムで抽象化 → REJECT | +| switch/caseが5分岐以上 | Strategy/Mapパターンを検討 | +| イベント種別による分岐が増殖 | イベントハンドラを分離 → REJECT | +| Aggregate内の状態分岐が複雑 | State Patternを検討 | + +**抽象度の不一致検出** + +| パターン | 問題 | 修正案 | +|---------|------|--------| +| CommandHandlerにDB操作詳細 | 責務違反 | Repository層に分離 | +| EventHandlerにビジネスロジック | 責務違反 | ドメインサービスに抽出 | +| Aggregateに永続化処理 | レイヤー違反 | EventStore経由に変更 | +| Projectionに計算ロジック | 保守困難 | 専用サービスに抽出 | + +良い抽象化の例: + +```kotlin +// イベント種別による分岐の増殖（NG） +@EventHandler +fun on(event: DomainEvent) { + when (event) { + is OrderPlacedEvent -> handleOrderPlaced(event) + is OrderConfirmedEvent -> handleOrderConfirmed(event) + is OrderShippedEvent -> handleOrderShipped(event) + // ...どんどん増える + } +} + +// イベントごとにハンドラを分離（OK） +@EventHandler +fun on(event: OrderPlacedEvent) { ... } + +@EventHandler +fun on(event: OrderConfirmedEvent) { ... } + +@EventHandler +fun on(event: OrderShippedEvent) { ... } +``` + +```kotlin +// 状態による分岐が複雑（NG） +fun process(command: ProcessCommand) { + when (status) { + PENDING -> if (command.type == "approve") { ... } else if (command.type == "reject") { ... } + APPROVED -> if (command.type == "ship") { ... } + // ...複雑化 + } +} + +// State Patternで抽象化（OK） +sealed class OrderState { + abstract fun handle(command: ProcessCommand): List +} +class PendingState : OrderState() { + override fun handle(command: ProcessCommand) = when (command) { + is ApproveCommand -> listOf(OrderApprovedEvent(...)) + is RejectCommand -> listOf(OrderRejectedEvent(...)) + else -> throw InvalidCommandException() + } +} +``` + +## アンチパターン検出 + +以下を見つけたら REJECT: + +| アンチパターン | 問題 | +|---------------|------| +| CRUD偽装 | CQRSの形だけ真似てCRUD実装 | +| Anemic Domain Model | Aggregateが単なるデータ構造 | +| Event Soup | 意味のないイベントが乱発される | +| Temporal Coupling | イベント順序に暗黙の依存 | +| Missing Events | 重要なドメインイベントが欠落 | +| God Aggregate | 1つのAggregateに全責務が集中 | + +## テスト戦略 + +レイヤーごとにテスト方針を分ける。 + +テストピラミッド: +``` + ┌─────────────┐ + │ E2E Test │ ← 少数: 全体フロー確認 + ├─────────────┤ + │ Integration │ ← Command→Event→Projection→Query の連携確認 + ├─────────────┤ + │ Unit Test │ ← 多数: 各レイヤー独立テスト + └─────────────┘ +``` + +Command側（Aggregate）: +```kotlin +// AggregateTestFixture使用 +@Test +fun `確定コマンドでイベントが発行される`() { + fixture + .given(OrderPlacedEvent(...)) + .`when`(ConfirmOrderCommand(orderId, confirmedBy)) + .expectSuccessfulHandlerExecution() + .expectEvents(OrderConfirmedEvent(...)) +} +``` + +Query側: +```kotlin +// Read Model直接セットアップ + QueryGateway +@Test +fun `注文詳細が取得できる`() { + // Given: Read Modelを直接セットアップ + orderRepository.save(OrderEntity(...)) + + // When: QueryGateway経由でクエリ実行 + val detail = queryGateway.query(GetOrderDetailQuery(orderId), ...).join() + + // Then + assertEquals(expectedDetail, detail) +} +``` + +チェック項目: + +| 観点 | 判定 | +|------|------| +| Aggregateテストが状態ではなくイベントを検証している | 必須 | +| Query側テストがCommand経由でデータを作っていない | 推奨 | +| 統合テストでAxonの非同期処理を考慮している | 必須 | + +## インフラ層 + +確認事項: +- イベントストアの選択は適切か +- メッセージング基盤は要件を満たすか +- スナップショット戦略は定義されているか +- イベントのシリアライズ形式は適切か diff --git a/resources/global/ja/knowledge/frontend.md b/resources/global/ja/knowledge/frontend.md new file mode 100644 index 0000000..f4f6958 --- /dev/null +++ b/resources/global/ja/knowledge/frontend.md @@ -0,0 +1,497 @@ +# フロントエンド専門知識 + +## コンポーネント設計 + +1ファイルにベタ書きしない。必ずコンポーネント分割する。 + +分離が必須なケース: +- 独自のstateを持つ → 必ず分離 +- 50行超のJSX → 分離 +- 再利用可能 → 分離 +- 責務が複数 → 分離 +- ページ内の独立したセクション → 分離 + +| 基準 | 判定 | +|------|------| +| 1コンポーネント200行超 | 分割を検討 | +| 1コンポーネント300行超 | REJECT | +| 表示とロジックが混在 | 分離を検討 | +| Props drilling（3階層以上） | 状態管理の導入を検討 | +| 複数の責務を持つコンポーネント | REJECT | + +良いコンポーネント: +- 単一責務: 1つのことをうまくやる +- 自己完結: 必要な依存が明確 +- テスト可能: 副作用が分離されている + +コンポーネント分類: + +| 種類 | 責務 | 例 | +|------|------|-----| +| Container | データ取得・状態管理 | `UserListContainer` | +| Presentational | 表示のみ | `UserCard` | +| Layout | 配置・構造 | `PageLayout`, `Grid` | +| Utility | 共通機能 | `ErrorBoundary`, `Portal` | + +ディレクトリ構成: +``` +features/{feature-name}/ +├── components/ +│ ├── {feature}-view.tsx # メインビュー（子を組み合わせる） +│ ├── {sub-component}.tsx # サブコンポーネント +│ └── index.ts +├── hooks/ +├── types.ts +└── index.ts +``` + +## 状態管理 + +子コンポーネントは自身で状態を変更しない。イベントを親にバブリングし、親が状態を操作する。 + +```tsx +// 子が自分で状態を変更（NG） +const ChildBad = ({ initialValue }: { initialValue: string }) => { + const [value, setValue] = useState(initialValue) + return setValue(e.target.value)} /> +} + +// 親が状態を管理、子はコールバックで通知（OK） +const ChildGood = ({ value, onChange }: { value: string; onChange: (v: string) => void }) => { + return onChange(e.target.value)} /> +} + +const Parent = () => { + const [value, setValue] = useState('') + return +} +``` + +例外（子がローカルstate持ってOK）: +- UI専用の一時状態（ホバー、フォーカス、アニメーション） +- 親に伝える必要がない完全にローカルな状態 + +| 基準 | 判定 | +|------|------| +| 不要なグローバル状態 | ローカル化を検討 | +| 同じ状態が複数箇所で管理 | 正規化が必要 | +| 子から親への状態変更（逆方向データフロー） | REJECT | +| APIレスポンスをそのまま状態に | 正規化を検討 | +| useEffectの依存配列が不適切 | REJECT | + +状態配置の判断基準: + +| 状態の性質 | 推奨配置 | +|-----------|---------| +| UIの一時的な状態（モーダル開閉等） | ローカル（useState） | +| フォームの入力値 | ローカル or フォームライブラリ | +| 複数コンポーネントで共有 | Context or 状態管理ライブラリ | +| サーバーデータのキャッシュ | TanStack Query等のデータフェッチライブラリ | + +## データ取得 + +API呼び出しはルート（View）コンポーネントで行い、子コンポーネントにはpropsで渡す。 + +```tsx +// CORRECT - ルートでデータ取得、子に渡す +const OrderDetailView = () => { + const { data: order, isLoading, error } = useGetOrder(orderId) + const { data: items } = useListOrderItems(orderId) + + if (isLoading) return + if (error) return + + return ( + + ) +} + +// WRONG - 子コンポーネントが自分でデータ取得 +const OrderSummary = ({ orderId }) => { + const { data: order } = useGetOrder(orderId) + // ... +} +``` + +UIの状態変更でパラメータが変わる場合（週切り替え、フィルタ等）: + +状態もViewレベルで管理し、コンポーネントにはコールバックを渡す。 + +```tsx +// CORRECT - 状態もViewで管理 +const ScheduleView = () => { + const [currentWeek, setCurrentWeek] = useState(startOfWeek(new Date())) + const { data } = useListSchedules({ + from: format(currentWeek, 'yyyy-MM-dd'), + to: format(endOfWeek(currentWeek), 'yyyy-MM-dd'), + }) + + return ( + + ) +} + +// WRONG - コンポーネント内で状態管理+データ取得 +const WeeklyCalendar = ({ facilityId }) => { + const [currentWeek, setCurrentWeek] = useState(...) + const { data } = useListSchedules({ facilityId, from, to }) + // ... +} +``` + +例外（コンポーネント内フェッチが許容されるケース）: + +| ケース | 理由 | +|--------|------| +| 無限スクロール | スクロール位置というUI内部状態に依存 | +| 検索オートコンプリート | 入力値に依存したリアルタイム検索 | +| 独立したウィジェット | 通知バッジ、天気等。親のデータと完全に無関係 | +| リアルタイム更新 | WebSocket/Pollingでの自動更新 | +| モーダル内の詳細取得 | 開いたときだけ追加データを取得 | + +判断基準: 「親が管理する意味がない / 親に影響を与えない」ケースのみ許容。 + +| 基準 | 判定 | +|------|------| +| コンポーネント内で直接fetch | Container層に分離 | +| エラーハンドリングなし | REJECT | +| ローディング状態の未処理 | REJECT | +| キャンセル処理なし | 警告 | +| N+1クエリ的なフェッチ | REJECT | + +## 共有コンポーネントと抽象化 + +同じパターンのUIは共有コンポーネント化する。インラインスタイルのコピペは禁止。 + +```tsx +// WRONG - インラインスタイルのコピペ + + +// CORRECT - 共有コンポーネント使用 + + + +``` + +共有コンポーネント化すべきパターン: +- アイコンボタン（閉じる、編集、削除等） +- ローディング/エラー表示 +- ステータスバッジ +- タブ切り替え +- ラベル+値の表示（詳細画面） +- 検索入力 +- カラー凡例 + +過度な汎用化を避ける: + +```tsx +// WRONG - IconButtonに無理やりステッパー用バリアントを追加 +export const iconButtonVariants = cva('...', { + variants: { + variant: { + default: '...', + outlined: '...', // ステッパー専用、他で使わない + }, + size: { + medium: 'p-2', + stepper: 'w-8 h-8', // outlinedとセットでしか使わない + }, + }, +}) + +// CORRECT - 用途別に専用コンポーネント +export function StepperButton(props) { + return ( + + ) +} +``` + +別コンポーネントにすべきサイン: +- 「このvariantはこのsizeとセット」のような暗黙の制約がある +- 追加したvariantが元のコンポーネントの用途と明らかに違う +- 使う側のprops指定が複雑になる + +## 抽象化レベルの評価 + +### 条件分岐の肥大化検出 + +| パターン | 判定 | +|---------|------| +| 同じ条件分岐が3箇所以上 | 共通コンポーネントに抽出 → REJECT | +| propsによる分岐が5種類以上 | コンポーネント分割を検討 | +| render内の三項演算子のネスト | 早期リターンまたはコンポーネント分離 → REJECT | +| 型による分岐レンダリング | ポリモーフィックコンポーネントを検討 | + +### 抽象度の不一致検出 + +| パターン | 問題 | 修正案 | +|---------|------|--------| +| データ取得ロジックがJSXに混在 | 読みにくい | カスタムフックに抽出 | +| ビジネスロジックがコンポーネントに混在 | 責務違反 | hooks/utilsに分離 | +| スタイル計算ロジックが散在 | 保守困難 | ユーティリティ関数に抽出 | +| 同じ変換処理が複数箇所に | DRY違反 | 共通関数に抽出 | + +良い抽象化の例: + +```tsx +// 条件分岐が肥大化 +function UserBadge({ user }) { + if (user.role === 'admin') { + return 管理者 + } else if (user.role === 'moderator') { + return モデレーター + } else if (user.role === 'premium') { + return プレミアム + } else { + return 一般 + } +} + +// Mapで抽象化 +const ROLE_CONFIG = { + admin: { label: '管理者', className: 'bg-red-500' }, + moderator: { label: 'モデレーター', className: 'bg-yellow-500' }, + premium: { label: 'プレミアム', className: 'bg-purple-500' }, + default: { label: '一般', className: 'bg-gray-500' }, +} + +function UserBadge({ user }) { + const config = ROLE_CONFIG[user.role] ?? ROLE_CONFIG.default + return {config.label} +} +``` + +```tsx +// 抽象度が混在 +function OrderList() { + const [orders, setOrders] = useState([]) + useEffect(() => { + fetch('/api/orders') + .then(res => res.json()) + .then(data => setOrders(data)) + }, []) + + return orders.map(order => ( +
{order.total.toLocaleString()}円
+ )) +} + +// 抽象度を揃える +function OrderList() { + const { data: orders } = useOrders() // データ取得を隠蔽 + + return orders.map(order => ( + + )) +} +``` + +## フロントエンドとバックエンドの責務分離 + +### 表示形式の責務 + +バックエンドは「データ」を返し、フロントエンドが「表示形式」に変換する。 + +```tsx +// フロントエンド: 表示形式に変換 +export function formatPrice(amount: number): string { + return `¥${amount.toLocaleString()}` +} + +export function formatDate(date: Date): string { + return format(date, 'yyyy年M月d日') +} +``` + +| 基準 | 判定 | +|------|------| +| バックエンドが表示用文字列を返している | 設計見直しを提案 | +| 同じフォーマット処理が複数箇所にコピペ | ユーティリティ関数に統一 | +| コンポーネント内でインラインフォーマット | 関数に抽出 | + +### ドメインロジックの配置（SmartUI排除） + +ドメインロジック（ビジネスルール）はバックエンドに配置。フロントエンドは状態の表示・編集のみ。 + +ドメインロジックとは: +- 集約のビジネスルール（在庫判定、価格計算、ステータス遷移） +- バリデーション（業務制約の検証） +- 不変条件の保証 + +フロントエンドの責務: +- サーバーから受け取った状態を表示 +- ユーザー入力を収集し、コマンドとしてバックエンドに送信 +- UI専用の一時状態管理（フォーカス、ホバー、モーダル開閉） +- 表示形式の変換（フォーマット、ソート、フィルタ） + +| 基準 | 判定 | +|------|------| +| フロントエンドで価格計算・在庫判定 | バックエンドに移動 → REJECT | +| フロントエンドでステータス遷移ルール | バックエンドに移動 → REJECT | +| フロントエンドでビジネスバリデーション | バックエンドに移動 → REJECT | +| サーバー側で計算可能な値をフロントで再計算 | 冗長 → REJECT | + +良い例 vs 悪い例: + +```tsx +// BAD - フロントエンドでビジネスルール +function OrderForm({ order }: { order: Order }) { + const totalPrice = order.items.reduce((sum, item) => + sum + item.price * item.quantity, 0 + ) + const canCheckout = totalPrice >= 1000 && order.items.every(i => i.stock > 0) + + return +} + +// GOOD - バックエンドから受け取った状態を表示 +function OrderForm({ order }: { order: Order }) { + // totalPrice, canCheckout はサーバーから受け取る + return ( + <> +
{formatPrice(order.totalPrice)}
+ + + ) +} +``` + +```tsx +// BAD - フロントエンドでステータス遷移判定 +function TaskCard({ task }: { task: Task }) { + const canStart = task.status === 'pending' && task.assignee !== null + const canComplete = task.status === 'in_progress' && /* 複雑な条件... */ + + return ( + <> + + + + ) +} + +// GOOD - サーバーが許可するアクションを返す +function TaskCard({ task }: { task: Task }) { + // task.allowedActions = ['start', 'cancel'] など、サーバーが計算 + const canStart = task.allowedActions.includes('start') + const canComplete = task.allowedActions.includes('complete') + + return ( + <> + + + + ) +} +``` + +例外（フロントエンドにロジックを置いてもOK）: + +| ケース | 理由 | +|--------|------| +| UI専用バリデーション | 「必須入力」「文字数制限」等のUXフィードバック（サーバー側でも検証必須） | +| クライアント側フィルタ/ソート | サーバーから受け取ったリストの表示順序変更 | +| 表示条件の分岐 | 「ログイン済みなら詳細表示」等のUI制御 | +| リアルタイムフィードバック | 入力中のプレビュー表示 | + +判断基準: 「この計算結果がサーバーとズレたら業務が壊れるか?」 +- YES → バックエンドに配置（ドメインロジック） +- NO → フロントエンドでもOK（表示ロジック） + +## パフォーマンス + +| 基準 | 判定 | +|------|------| +| 不要な再レンダリング | 最適化が必要 | +| 大きなリストの仮想化なし | 警告 | +| 画像の最適化なし | 警告 | +| バンドルに未使用コード | tree-shakingを確認 | +| メモ化の過剰使用 | 本当に必要か確認 | + +最適化チェックリスト: +- `React.memo` / `useMemo` / `useCallback` は適切か +- 大きなリストは仮想スクロール対応か +- Code Splittingは適切か +- 画像はlazy loadingされているか + +アンチパターン: + +```tsx +// レンダリングごとに新しいオブジェクト + + +// 定数化 or useMemo +const style = useMemo(() => ({ color: 'red' }), []); + +``` + +## アクセシビリティ + +| 基準 | 判定 | +|------|------| +| インタラクティブ要素にキーボード対応なし | REJECT | +| 画像にalt属性なし | REJECT | +| フォーム要素にlabelなし | REJECT | +| 色だけで情報を伝達 | REJECT | +| フォーカス管理の欠如（モーダル等） | REJECT | + +チェックリスト: +- セマンティックHTMLを使用しているか +- ARIA属性は適切か（過剰でないか） +- キーボードナビゲーション可能か +- スクリーンリーダーで意味が通じるか +- カラーコントラストは十分か + +## TypeScript/型安全性 + +| 基準 | 判定 | +|------|------| +| `any` 型の使用 | REJECT | +| 型アサーション（as）の乱用 | 要検討 | +| Props型定義なし | REJECT | +| イベントハンドラの型が不適切 | 修正が必要 | + +## フロントエンドセキュリティ + +| 基準 | 判定 | +|------|------| +| dangerouslySetInnerHTML使用 | XSSリスクを確認 | +| ユーザー入力の未サニタイズ | REJECT | +| 機密情報のフロントエンド保存 | REJECT | +| CSRFトークンの未使用 | 要確認 | + +## テスタビリティ + +| 基準 | 判定 | +|------|------| +| data-testid等の未付与 | 警告 | +| テスト困難な構造 | 分離を検討 | +| ビジネスロジックのUIへの埋め込み | REJECT | + +## アンチパターン検出 + +以下を見つけたら REJECT: + +| アンチパターン | 問題 | +|---------------|------| +| God Component | 1コンポーネントに全機能が集中 | +| Prop Drilling | 深いPropsバケツリレー | +| Inline Styles乱用 | 保守性低下 | +| useEffect地獄 | 依存関係が複雑すぎる | +| Premature Optimization | 不要なメモ化 | +| Magic Strings | ハードコードされた文字列 | +| Hidden Dependencies | 子コンポーネントの隠れたAPI呼び出し | +| Over-generalization | 無理やり汎用化したコンポーネント | diff --git a/resources/global/ja/knowledge/security.md b/resources/global/ja/knowledge/security.md new file mode 100644 index 0000000..14ab513 --- /dev/null +++ b/resources/global/ja/knowledge/security.md @@ -0,0 +1,164 @@ +# セキュリティ知識 + +## AI生成コードのセキュリティ問題 + +AI生成コードには特有の脆弱性パターンがある。 + +| パターン | リスク | 例 | +|---------|--------|-----| +| もっともらしいが危険なデフォルト | 高 | `cors: { origin: '*' }` は問題なく見えるが危険 | +| 古いセキュリティプラクティス | 中 | 非推奨の暗号化、古い認証パターンの使用 | +| 不完全なバリデーション | 高 | 形式は検証するがビジネスルールを検証しない | +| 入力を過度に信頼 | 重大 | 内部APIは常に安全と仮定 | +| コピペによる脆弱性 | 高 | 同じ危険なパターンが複数ファイルで繰り返される | + +特に厳しく審査が必要: +- 認証・認可ロジック（AIはエッジケースを見落としがち） +- 入力バリデーション（AIは構文を検証しても意味を見落とす可能性） +- エラーメッセージ（AIは内部詳細を露出する可能性） +- 設定ファイル（AIは学習データから危険なデフォルトを使う可能性） + +## インジェクション攻撃 + +**SQLインジェクション** + +- 文字列連結によるSQL構築 → REJECT +- パラメータ化クエリの不使用 → REJECT +- ORMの raw query での未サニタイズ入力 → REJECT + +```typescript +// NG +db.query(`SELECT * FROM users WHERE id = ${userId}`) + +// OK +db.query('SELECT * FROM users WHERE id = ?', [userId]) +``` + +**コマンドインジェクション** + +- `exec()`, `spawn()` での未検証入力 → REJECT +- シェルコマンド構築時のエスケープ不足 → REJECT + +```typescript +// NG +exec(`ls ${userInput}`) + +// OK +execFile('ls', [sanitizedInput]) +``` + +**XSS (Cross-Site Scripting)** + +- HTML/JSへの未エスケープ出力 → REJECT +- `innerHTML`, `dangerouslySetInnerHTML` の不適切な使用 → REJECT +- URLパラメータの直接埋め込み → REJECT + +## 認証・認可 + +**認証の問題** + +- ハードコードされたクレデンシャル → 即REJECT +- 平文パスワードの保存 → 即REJECT +- 弱いハッシュアルゴリズム (MD5, SHA1) → REJECT +- セッショントークンの不適切な管理 → REJECT + +**認可の問題** + +- 権限チェックの欠如 → REJECT +- IDOR (Insecure Direct Object Reference) → REJECT +- 権限昇格の可能性 → REJECT + +```typescript +// NG - 権限チェックなし +app.get('/user/:id', (req, res) => { + return db.getUser(req.params.id) +}) + +// OK +app.get('/user/:id', authorize('read:user'), (req, res) => { + if (req.user.id !== req.params.id && !req.user.isAdmin) { + return res.status(403).send('Forbidden') + } + return db.getUser(req.params.id) +}) +``` + +## データ保護 + +**機密情報の露出** + +- APIキー、シークレットのハードコーディング → 即REJECT +- ログへの機密情報出力 → REJECT +- エラーメッセージでの内部情報露出 → REJECT +- `.env` ファイルのコミット → REJECT + +**データ検証** + +- 入力値の未検証 → REJECT +- 型チェックの欠如 → REJECT +- サイズ制限の未設定 → REJECT + +## 暗号化 + +- 弱い暗号アルゴリズムの使用 → REJECT +- 固定IV/Nonceの使用 → REJECT +- 暗号化キーのハードコーディング → 即REJECT +- HTTPSの未使用（本番環境） → REJECT + +## ファイル操作 + +**パストラバーサル** + +- ユーザー入力を含むファイルパス → REJECT +- `../` のサニタイズ不足 → REJECT + +```typescript +// NG +const filePath = path.join(baseDir, userInput) +fs.readFile(filePath) + +// OK +const safePath = path.resolve(baseDir, userInput) +if (!safePath.startsWith(path.resolve(baseDir))) { + throw new Error('Invalid path') +} +``` + +**ファイルアップロード** + +- ファイルタイプの未検証 → REJECT +- ファイルサイズ制限なし → REJECT +- 実行可能ファイルのアップロード許可 → REJECT + +## 依存関係 + +- 既知の脆弱性を持つパッケージ → REJECT +- メンテナンスされていないパッケージ → 警告 +- 不必要な依存関係 → 警告 + +## エラーハンドリング + +- スタックトレースの本番露出 → REJECT +- 詳細なエラーメッセージの露出 → REJECT +- エラーの握りつぶし（セキュリティイベント） → REJECT + +## レート制限・DoS対策 + +- レート制限の欠如（認証エンドポイント） → 警告 +- リソース枯渇攻撃の可能性 → 警告 +- 無限ループの可能性 → REJECT + +## OWASP Top 10 チェックリスト + +| カテゴリ | 確認事項 | +|---------|---------| +| A01 Broken Access Control | 認可チェック、CORS設定 | +| A02 Cryptographic Failures | 暗号化、機密データ保護 | +| A03 Injection | SQL, コマンド, XSS | +| A04 Insecure Design | セキュリティ設計パターン | +| A05 Security Misconfiguration | デフォルト設定、不要な機能 | +| A06 Vulnerable Components | 依存関係の脆弱性 | +| A07 Auth Failures | 認証メカニズム | +| A08 Software Integrity | コード署名、CI/CD | +| A09 Logging Failures | セキュリティログ | +| A10 SSRF | サーバーサイドリクエスト | diff --git a/resources/global/ja/personas/architecture-reviewer.md b/resources/global/ja/personas/architecture-reviewer.md index 8dc5a01..d02e534 100644 --- a/resources/global/ja/personas/architecture-reviewer.md +++ b/resources/global/ja/personas/architecture-reviewer.md @@ -24,431 +24,3 @@ - 軽微な問題でも後に持ち越さない。今修正できる問題は今修正させる - 「条件付き承認」はしない。問題があれば差し戻す - 既存コードの踏襲を理由にした問題の放置は認めない - -## ドメイン知識 - -### 構造・設計 - -**ファイル分割** - -| 基準 | 判定 | -|--------------|------| -| 1ファイル200行超 | 分割を検討 | -| 1ファイル300行超 | REJECT | -| 1ファイルに複数の責務 | REJECT | -| 関連性の低いコードが同居 | REJECT | - -**モジュール構成** - -- 高凝集: 関連する機能がまとまっているか -- 低結合: モジュール間の依存が最小限か -- 循環依存がないか -- 適切なディレクトリ階層か - -**関数設計** - -- 1関数1責務になっているか -- 30行を超える関数は分割を検討 -- 副作用が明確か - -**レイヤー設計** - -- 依存の方向: 上位層 → 下位層（逆方向禁止） -- Controller → Service → Repository の流れが守られているか -- 1インターフェース = 1責務（巨大なServiceクラス禁止） - -**ディレクトリ構造** - -構造パターンの選択: - -| パターン | 適用場面 | 例 | -|---------|---------|-----| -| レイヤード | 小規模、CRUD中心 | `controllers/`, `services/`, `repositories/` | -| Vertical Slice | 中〜大規模、機能独立性が高い | `features/auth/`, `features/order/` | -| ハイブリッド | 共通基盤 + 機能モジュール | `core/` + `features/` | - -Vertical Slice Architecture（機能単位でコードをまとめる構造）: - -``` -src/ -├── features/ -│ ├── auth/ -│ │ ├── LoginCommand.ts -│ │ ├── LoginHandler.ts -│ │ ├── AuthRepository.ts -│ │ └── auth.test.ts -│ └── order/ -│ ├── CreateOrderCommand.ts -│ ├── CreateOrderHandler.ts -│ └── ... -└── shared/ # 複数featureで共有 - ├── database/ - └── middleware/ -``` - -Vertical Slice の判定基準: - -| 基準 | 判定 | -|------|------| -| 1機能が3ファイル以上のレイヤーに跨る | Slice化を検討 | -| 機能間の依存がほぼない | Slice化推奨 | -| 共通処理が50%以上 | レイヤード維持 | -| チームが機能別に分かれている | Slice化必須 | - -禁止パターン: - -| パターン | 問題 | -|---------|------| -| `utils/` の肥大化 | 責務不明の墓場になる | -| `common/` への安易な配置 | 依存関係が不明確になる | -| 深すぎるネスト（4階層超） | ナビゲーション困難 | -| 機能とレイヤーの混在 | `features/services/` は禁止 | - -**責務の分離** - -- 読み取りと書き込みの責務が分かれているか -- データ取得はルート（View/Controller）で行い、子に渡しているか -- エラーハンドリングが一元化されているか（各所でtry-catch禁止） -- ビジネスロジックがController/Viewに漏れていないか - -### コード品質の検出手法 - -**説明コメント（What/How）の検出基準** - -コードの動作をそのまま言い換えているコメントを検出する。 - -| 判定 | 基準 | -|------|------| -| REJECT | コードの動作をそのまま自然言語で言い換えている | -| REJECT | 関数名・変数名から明らかなことを繰り返している | -| REJECT | JSDocが関数名の言い換えだけで情報を追加していない | -| OK | なぜその実装を選んだかの設計判断を説明している | -| OK | 一見不自然に見える挙動の理由を説明している | -| 最良 | コメントなしでコード自体が意図を語っている | - -```typescript -// REJECT - コードの言い換え（What） -// If interrupted, abort immediately -if (status === 'interrupted') { - return ABORT_STEP; -} - -// REJECT - ループの存在を言い換えただけ -// Check transitions in order -for (const transition of step.transitions) { - -// REJECT - 関数名の繰り返し -/** Check if status matches transition condition. */ -export function matchesCondition(status: Status, condition: TransitionCondition): boolean { - -// OK - 設計判断の理由（Why） -// ユーザー中断はピース定義のトランジションより優先する -if (status === 'interrupted') { - return ABORT_STEP; -} - -// OK - 一見不自然な挙動の理由 -// stay はループを引き起こす可能性があるが、ユーザーが明示的に指定した場合のみ使われる -return step.name; -``` - -**状態の直接変更の検出基準** - -配列やオブジェクトの直接変更（ミューテーション）を検出する。 - -```typescript -// REJECT - 配列の直接変更 -const steps: Step[] = getSteps(); -steps.push(newStep); // 元の配列を破壊 -steps.splice(index, 1); // 元の配列を破壊 -steps[0].status = 'done'; // ネストされたオブジェクトも直接変更 - -// OK - イミュータブルな操作 -const withNew = [...steps, newStep]; -const without = steps.filter((_, i) => i !== index); -const updated = steps.map((s, i) => - i === 0 ? { ...s, status: 'done' } : s -); - -// REJECT - オブジェクトの直接変更 -function updateConfig(config: Config) { - config.logLevel = 'debug'; // 引数を直接変更 - config.steps.push(newStep); // ネストも直接変更 - return config; -} - -// OK - 新しいオブジェクトを返す -function updateConfig(config: Config): Config { - return { - ...config, - logLevel: 'debug', - steps: [...config.steps, newStep], - }; -} -``` - -### セキュリティ（基本チェック） - -- インジェクション対策（SQL, コマンド, XSS） -- ユーザー入力の検証 -- 機密情報のハードコーディング - -### テスタビリティ - -- 依存性注入が可能な設計か -- モック可能か -- テストが書かれているか - -### アンチパターン検出 - -以下のパターンを見つけたら REJECT: - -| アンチパターン | 問題 | -|---------------|------| -| God Class/Component | 1つのクラスが多くの責務を持っている | -| Feature Envy | 他モジュールのデータを頻繁に参照している | -| Shotgun Surgery | 1つの変更が複数ファイルに波及する構造 | -| 過度な汎用化 | 今使わないバリアントや拡張ポイント | -| 隠れた依存 | 子コンポーネントが暗黙的にAPIを呼ぶ等 | -| 非イディオマティック | 言語・FWの作法を無視した独自実装 | - -### 抽象化レベルの評価 - -**条件分岐の肥大化検出** - -| パターン | 判定 | -|---------|------| -| 同じif-elseパターンが3箇所以上 | ポリモーフィズムで抽象化 → REJECT | -| switch/caseが5分岐以上 | Strategy/Mapパターンを検討 | -| フラグ引数で挙動を変える | 別関数に分割 → REJECT | -| 型による分岐（instanceof/typeof） | ポリモーフィズムに置換 → REJECT | -| ネストした条件分岐（3段以上） | 早期リターンまたは抽出 → REJECT | - -**抽象度の不一致検出** - -| パターン | 問題 | 修正案 | -|---------|------|--------| -| 高レベル処理の中に低レベル詳細 | 読みにくい | 詳細を関数に抽出 | -| 1関数内で抽象度が混在 | 認知負荷 | 同じ粒度に揃える | -| ビジネスロジックにDB操作が混在 | 責務違反 | Repository層に分離 | -| 設定値と処理ロジックが混在 | 変更困難 | 設定を外部化 | - -**良い抽象化の例** - -```typescript -// 条件分岐の肥大化 -function process(type: string) { - if (type === 'A') { /* 処理A */ } - else if (type === 'B') { /* 処理B */ } - else if (type === 'C') { /* 処理C */ } - // ...続く -} - -// Mapパターンで抽象化 -const processors: Record void> = { - A: processA, - B: processB, - C: processC, -}; -function process(type: string) { - processors[type]?.(); -} -``` - -```typescript -// 抽象度の混在 -function createUser(data: UserData) { - // 高レベル: ビジネスロジック - validateUser(data); - // 低レベル: DB操作の詳細 - const conn = await pool.getConnection(); - await conn.query('INSERT INTO users...'); - conn.release(); -} - -// 抽象度を揃える -function createUser(data: UserData) { - validateUser(data); - await userRepository.save(data); // 詳細は隠蔽 -} -``` - -### その場しのぎの検出 - -「とりあえず動かす」ための妥協を見逃さない。 - -| パターン | 例 | -|---------|-----| -| 不要なパッケージ追加 | 動かすためだけに入れた謎のライブラリ | -| テストの削除・スキップ | `@Disabled`、`.skip()`、コメントアウト | -| 空実装・スタブ放置 | `return null`、`// TODO: implement`、`pass` | -| モックデータの本番混入 | ハードコードされたダミーデータ | -| エラー握りつぶし | 空の `catch {}`、`rescue nil` | -| マジックナンバー | 説明なしの `if (status == 3)` | - -### TODOコメントの厳格な禁止 - -「将来やる」は決してやらない。今やらないことは永遠にやらない。 - -TODOコメントは即REJECT。 - -```kotlin -// REJECT - 将来を見越したTODO -// TODO: 施設IDによる認可チェックを追加 -fun deleteCustomHoliday(@PathVariable id: String) { - deleteCustomHolidayInputPort.execute(input) -} - -// APPROVE - 今実装する -fun deleteCustomHoliday(@PathVariable id: String) { - val currentUserFacilityId = getCurrentUserFacilityId() - val holiday = findHolidayById(id) - require(holiday.facilityId == currentUserFacilityId) { - "Cannot delete holiday from another facility" - } - deleteCustomHolidayInputPort.execute(input) -} -``` - -TODOが許容される唯一のケース: - -| 条件 | 例 | 判定 | -|------|-----|------| -| 外部依存で今は実装不可 + Issue化済み | `// TODO(#123): APIキー取得後に実装` | 許容 | -| 技術的制約で回避不可 + Issue化済み | `// TODO(#456): ライブラリバグ修正待ち` | 許容 | -| 「将来実装」「後で追加」 | `// TODO: バリデーション追加` | REJECT | -| 「時間がないので」 | `// TODO: リファクタリング` | REJECT | - -正しい対処: -- 今必要 → 今実装する -- 今不要 → コードを削除する -- 外部要因で不可 → Issue化してチケット番号をコメントに入れる - -### DRY違反の検出 - -重複コードを検出する。 - -| パターン | 判定 | -|---------|------| -| 同じロジックが3箇所以上 | 即REJECT - 関数/メソッドに抽出 | -| 同じバリデーションが2箇所以上 | 即REJECT - バリデーター関数に抽出 | -| 似たようなコンポーネントが3個以上 | 即REJECT - 共通コンポーネント化 | -| コピペで派生したコード | 即REJECT - パラメータ化または抽象化 | - -AHA原則（Avoid Hasty Abstractions）とのバランス: -- 2回の重複 → 様子見 -- 3回の重複 → 即抽出 -- ドメインが異なる重複 → 抽象化しない（例: 顧客用バリデーションと管理者用バリデーションは別物） - -### 仕様準拠の検証 - -変更が、プロジェクトの文書化された仕様に準拠しているか検証する。 - -検証対象: - -| 対象 | 確認内容 | -|------|---------| -| CLAUDE.md / README.md | スキーマ定義、設計原則、制約に従っているか | -| 型定義・Zodスキーマ | 新しいフィールドがスキーマに反映されているか | -| YAML/JSON設定ファイル | 文書化されたフォーマットに従っているか | - -具体的なチェック: - -1. 設定ファイル（YAML等）を変更・追加した場合: - - CLAUDE.md等に記載されたスキーマ定義と突合する - - 無視されるフィールドや無効なフィールドが含まれていないか - - 必須フィールドが欠落していないか - -2. 型定義やインターフェースを変更した場合: - - ドキュメントのスキーマ説明が更新されているか - - 既存の設定ファイルが新しいスキーマと整合するか - -このパターンを見つけたら REJECT: - -| パターン | 問題 | -|---------|------| -| 仕様に存在しないフィールドの使用 | 無視されるか予期しない動作 | -| 仕様上無効な値の設定 | 実行時エラーまたは無視される | -| 文書化された制約への違反 | 設計意図に反する | - -### 呼び出しチェーン検証 - -新しいパラメータ・フィールドが追加された場合、変更ファイル内だけでなく呼び出し元も検証する。 - -検証手順: -1. 新しいオプショナルパラメータや interface フィールドを見つけたら、`Grep` で全呼び出し元を検索 -2. 全呼び出し元が新しいパラメータを渡しているか確認 -3. フォールバック値（`?? default`）がある場合、フォールバックが使われるケースが意図通りか確認 - -危険パターン: - -| パターン | 問題 | 検出方法 | -|---------|------|---------| -| `options.xxx ?? fallback` で全呼び出し元が `xxx` を省略 | 機能が実装されているのに常にフォールバック | grep で呼び出し元を確認 | -| テストがモックで直接値をセット | 実際の呼び出しチェーンを経由しない | テストの構築方法を確認 | -| `executeXxx()` が内部で使う `options` を引数で受け取らない | 上位から値を渡す口がない | 関数シグネチャを確認 | - -```typescript -// 配線漏れ: projectCwd を受け取る口がない -export async function executePiece(config, cwd, task) { - const engine = new PieceEngine(config, cwd, task); // options なし -} - -// 配線済み: projectCwd を渡せる -export async function executePiece(config, cwd, task, options?) { - const engine = new PieceEngine(config, cwd, task, options); -} -``` - -呼び出し元の制約による論理的デッドコード: - -呼び出しチェーンの検証は「配線漏れ」だけでなく、逆方向——呼び出し元が既に保証している条件に対する不要な防御コード——にも適用する。 - -| パターン | 問題 | 検出方法 | -|---------|------|---------| -| 呼び出し元がTTY必須なのに関数内でTTYチェック | 到達しない分岐が残る | grep で全呼び出し元の前提条件を確認 | -| 呼び出し元がnullチェック済みなのに再度nullガード | 冗長な防御 | 呼び出し元の制約を追跡 | -| 呼び出し元が型で制約しているのにランタイムチェック | 型安全を信頼していない | TypeScriptの型制約を確認 | - -検証手順: -1. 防御的な条件分岐（TTYチェック、nullガード等）を見つけたら、grep で全呼び出し元を確認 -2. 全呼び出し元がその条件を既に保証しているなら、防御は不要 → REJECT -3. 一部の呼び出し元が保証していない場合は、防御を残す - -### 品質特性 - -| 特性 | 確認観点 | -|------|---------| -| Scalability | 負荷増加に対応できる設計か | -| Maintainability | 変更・修正が容易か | -| Observability | ログ・監視が可能な設計か | - -### 大局観 - -細かい「クリーンコード」の指摘に終始しない。 - -確認すべきこと: -- このコードは将来どう変化するか -- スケーリングの必要性は考慮されているか -- 技術的負債を生んでいないか -- ビジネス要件と整合しているか -- 命名がドメインと一貫しているか - -### 変更スコープの評価 - -変更スコープを確認し、レポートに記載する（ブロッキングではない）。 - -| スコープサイズ | 変更行数 | 対応 | -|---------------|---------|------| -| Small | 〜200行 | そのままレビュー | -| Medium | 200-500行 | そのままレビュー | -| Large | 500行以上 | レビューは継続。分割可能か提案を付記 | - -大きな変更が必要なタスクもある。行数だけでREJECTしない。 - -確認すること: -- 変更が論理的にまとまっているか（無関係な変更が混在していないか） -- Coderのスコープ宣言と実際の変更が一致しているか - -提案として記載すること（ブロッキングではない）: -- 分割可能な場合は分割案を提示 diff --git a/resources/global/ja/personas/cqrs-es-reviewer.md b/resources/global/ja/personas/cqrs-es-reviewer.md index 06edbd1..4b42776 100644 --- a/resources/global/ja/personas/cqrs-es-reviewer.md +++ b/resources/global/ja/personas/cqrs-es-reviewer.md @@ -26,421 +26,3 @@ - 読み取りと書き込みは本質的に異なる関心事であり、無理に統合しない - 形だけのCQRSを見逃さない。CRUDをCommand/Queryに分けただけでは意味がない - シンプルなCRUDで十分なケースにCQRS+ESを強制しない - -## ドメイン知識 - -### Aggregate設計 - -Aggregateは判断に必要なフィールドのみ保持する。 - -Command Model（Aggregate）の役割は「コマンドを受けて判断し、イベントを発行する」こと。クエリ用データはRead Model（Projection）が担当する。 - -「判断に必要」とは: -- `if`/`require`の条件分岐に使う -- インスタンスメソッドでイベント発行時にフィールド値を参照する - -| 基準 | 判定 | -|------|------| -| Aggregateが複数のトランザクション境界を跨ぐ | REJECT | -| Aggregate間の直接参照（ID参照でない） | REJECT | -| Aggregateが100行を超える | 分割を検討 | -| ビジネス不変条件がAggregate外にある | REJECT | -| 判断に使わないフィールドを保持 | REJECT | - -良いAggregate: -```kotlin -// 判断に必要なフィールドのみ -data class Order( - val orderId: String, // イベント発行時に使用 - val status: OrderStatus // 状態チェックに使用 -) { - fun confirm(confirmedBy: String): OrderConfirmedEvent { - require(status == OrderStatus.PENDING) { "確定できる状態ではありません" } - return OrderConfirmedEvent( - orderId = orderId, - confirmedBy = confirmedBy, - confirmedAt = LocalDateTime.now() - ) - } -} - -// 判断に使わないフィールドを保持（NG） -data class Order( - val orderId: String, - val customerId: String, // 判断に未使用 - val shippingAddress: Address, // 判断に未使用 - val status: OrderStatus -) -``` - -追加操作がないAggregateはIDのみ: -```kotlin -// 作成のみで追加操作がない場合 -data class Notification(val notificationId: String) { - companion object { - fun create(customerId: String, message: String): NotificationCreatedEvent { - return NotificationCreatedEvent( - notificationId = UUID.randomUUID().toString(), - customerId = customerId, - message = message - ) - } - } -} -``` - -### イベント設計 - -| 基準 | 判定 | -|------|------| -| イベントが過去形でない（Created → Create） | REJECT | -| イベントにロジックが含まれる | REJECT | -| イベントが他Aggregateの内部状態を含む | REJECT | -| イベントのスキーマがバージョン管理されていない | 警告 | -| CRUDスタイルのイベント（Updated, Deleted） | 要検討 | - -良いイベント: -```kotlin -// Good: ドメインの意図が明確 -OrderPlaced, PaymentReceived, ItemShipped - -// Bad: CRUDスタイル -OrderUpdated, OrderDeleted -``` - -イベント粒度: -- 細かすぎ: `OrderFieldChanged` → ドメインの意図が不明 -- 適切: `ShippingAddressChanged` → 意図が明確 -- 粗すぎ: `OrderModified` → 何が変わったか不明 - -### コマンドハンドラ - -| 基準 | 判定 | -|------|------| -| ハンドラがDBを直接操作 | REJECT | -| ハンドラが複数Aggregateを変更 | REJECT | -| コマンドのバリデーションがない | REJECT | -| ハンドラがクエリを実行して判断 | 要検討 | - -良いコマンドハンドラ: -``` -1. コマンドを受け取る -2. Aggregateをイベントストアから復元 -3. Aggregateにコマンドを適用 -4. 発行されたイベントを保存 -``` - -### プロジェクション設計 - -| 基準 | 判定 | -|------|------| -| プロジェクションがコマンドを発行 | REJECT | -| プロジェクションがWriteモデルを参照 | REJECT | -| 複数のユースケースを1つのプロジェクションで賄う | 要検討 | -| リビルド不可能な設計 | REJECT | - -良いプロジェクション: -- 特定の読み取りユースケースに最適化 -- イベントから冪等に再構築可能 -- Writeモデルから完全に独立 - -### Query側の設計 - -ControllerはQueryGatewayを使う。Repositoryを直接使わない。 - -レイヤー間の型: -- `application/query/` - Query結果の型（例: `OrderDetail`） -- `adapter/protocol/` - RESTレスポンスの型（例: `OrderDetailResponse`） -- QueryHandlerはapplication層の型を返し、Controllerがadapter層の型に変換 - -```kotlin -// application/query/OrderDetail.kt -data class OrderDetail( - val orderId: String, - val customerName: String, - val totalAmount: Money -) - -// adapter/protocol/OrderDetailResponse.kt -data class OrderDetailResponse(...) { - companion object { - fun from(detail: OrderDetail) = OrderDetailResponse(...) - } -} - -// QueryHandler - application層の型を返す -@QueryHandler -fun handle(query: GetOrderDetailQuery): OrderDetail? { - val entity = repository.findById(query.id) ?: return null - return OrderDetail(...) -} - -// Controller - adapter層の型に変換 -@GetMapping("/{id}") -fun getById(@PathVariable id: String): ResponseEntity { - val detail = queryGateway.query( - GetOrderDetailQuery(id), - OrderDetail::class.java - ).join() ?: throw NotFoundException("...") - - return ResponseEntity.ok(OrderDetailResponse.from(detail)) -} -``` - -構成: -``` -Controller (adapter) → QueryGateway → QueryHandler (application) → Repository - ↓ ↓ -Response.from(detail) OrderDetail -``` - -### 結果整合性 - -| 状況 | 対応 | -|------|------| -| UIが即座に更新を期待している | 設計見直し or ポーリング/WebSocket | -| 整合性遅延が許容範囲を超える | アーキテクチャ再検討 | -| 補償トランザクションが未定義 | 障害シナリオの検討を要求 | - -### Saga vs EventHandler - -Sagaは「競合が発生する複数アグリゲート間の操作」にのみ使用する。 - -Sagaが必要なケース: -``` -複数のアクターが同じリソースを取り合う場合 -例: 在庫確保（10人が同時に同じ商品を注文） - -OrderPlacedEvent - ↓ InventoryReservationSaga -ReserveInventoryCommand → Inventory集約（同時実行を直列化） - ↓ -InventoryReservedEvent → ConfirmOrderCommand -InventoryReservationFailedEvent → CancelOrderCommand -``` - -Sagaが不要なケース: -``` -競合が発生しない操作 -例: 注文キャンセル時の在庫解放 - -OrderCancelledEvent - ↓ InventoryReleaseHandler（単純なEventHandler） -ReleaseInventoryCommand - ↓ -InventoryReleasedEvent -``` - -判断基準: - -| 状況 | Saga | EventHandler | -|------|------|--------------| -| リソースの取り合いがある | 使う | - | -| 補償トランザクションが必要 | 使う | - | -| 競合しない単純な連携 | - | 使う | -| 失敗時は再試行で十分 | - | 使う | - -アンチパターン: -```kotlin -// NG - ライフサイクル管理のためにSagaを使う -@Saga -class OrderLifecycleSaga { - // 注文の全状態遷移をSagaで追跡 - // PLACED → CONFIRMED → SHIPPED → DELIVERED -} - -// OK - 結果整合性が必要な操作だけをSagaで処理 -@Saga -class InventoryReservationSaga { - // 在庫確保の同時実行制御のみ -} -``` - -Sagaはライフサイクル管理ツールではない。結果整合性が必要な「操作」単位で作成する。 - -### 例外 vs イベント（失敗時の選択） - -監査不要な失敗は例外、監査が必要な失敗はイベント。 - -例外アプローチ（推奨: ほとんどのケース）: -```kotlin -// ドメインモデル: バリデーション失敗時に例外をスロー -fun reserveInventory(orderId: String, quantity: Int): InventoryReservedEvent { - if (availableQuantity < quantity) { - throw InsufficientInventoryException("在庫が不足しています") - } - return InventoryReservedEvent(productId, orderId, quantity) -} - -// Saga: exceptionally でキャッチして補償アクション -commandGateway.send(command) - .exceptionally { ex -> - commandGateway.send(CancelOrderCommand( - orderId = orderId, - reason = ex.cause?.message ?: "在庫確保に失敗しました" - )) - null - } -``` - -イベントアプローチ（稀なケース）: -```kotlin -// 監査が必要な場合のみ -data class PaymentFailedEvent( - val paymentId: String, - val reason: String, - val attemptedAmount: Money -) : PaymentEvent -``` - -判断基準: - -| 質問 | 例外 | イベント | -|------|------|----------| -| この失敗を後で確認する必要があるか? | No | Yes | -| 規制やコンプライアンスで記録が必要か? | No | Yes | -| Sagaだけが失敗を気にするか? | Yes | No | -| Event Storeに残すと価値があるか? | No | Yes | - -デフォルトは例外アプローチ。監査要件がある場合のみイベントを検討する。 - -### 抽象化レベルの評価 - -**条件分岐の肥大化検出** - -| パターン | 判定 | -|---------|------| -| 同じif-elseパターンが3箇所以上 | ポリモーフィズムで抽象化 → REJECT | -| switch/caseが5分岐以上 | Strategy/Mapパターンを検討 | -| イベント種別による分岐が増殖 | イベントハンドラを分離 → REJECT | -| Aggregate内の状態分岐が複雑 | State Patternを検討 | - -**抽象度の不一致検出** - -| パターン | 問題 | 修正案 | -|---------|------|--------| -| CommandHandlerにDB操作詳細 | 責務違反 | Repository層に分離 | -| EventHandlerにビジネスロジック | 責務違反 | ドメインサービスに抽出 | -| Aggregateに永続化処理 | レイヤー違反 | EventStore経由に変更 | -| Projectionに計算ロジック | 保守困難 | 専用サービスに抽出 | - -良い抽象化の例: - -```kotlin -// イベント種別による分岐の増殖（NG） -@EventHandler -fun on(event: DomainEvent) { - when (event) { - is OrderPlacedEvent -> handleOrderPlaced(event) - is OrderConfirmedEvent -> handleOrderConfirmed(event) - is OrderShippedEvent -> handleOrderShipped(event) - // ...どんどん増える - } -} - -// イベントごとにハンドラを分離（OK） -@EventHandler -fun on(event: OrderPlacedEvent) { ... } - -@EventHandler -fun on(event: OrderConfirmedEvent) { ... } - -@EventHandler -fun on(event: OrderShippedEvent) { ... } -``` - -```kotlin -// 状態による分岐が複雑（NG） -fun process(command: ProcessCommand) { - when (status) { - PENDING -> if (command.type == "approve") { ... } else if (command.type == "reject") { ... } - APPROVED -> if (command.type == "ship") { ... } - // ...複雑化 - } -} - -// State Patternで抽象化（OK） -sealed class OrderState { - abstract fun handle(command: ProcessCommand): List -} -class PendingState : OrderState() { - override fun handle(command: ProcessCommand) = when (command) { - is ApproveCommand -> listOf(OrderApprovedEvent(...)) - is RejectCommand -> listOf(OrderRejectedEvent(...)) - else -> throw InvalidCommandException() - } -} -``` - -### アンチパターン検出 - -以下を見つけたら REJECT: - -| アンチパターン | 問題 | -|---------------|------| -| CRUD偽装 | CQRSの形だけ真似てCRUD実装 | -| Anemic Domain Model | Aggregateが単なるデータ構造 | -| Event Soup | 意味のないイベントが乱発される | -| Temporal Coupling | イベント順序に暗黙の依存 | -| Missing Events | 重要なドメインイベントが欠落 | -| God Aggregate | 1つのAggregateに全責務が集中 | - -### テスト戦略 - -レイヤーごとにテスト方針を分ける。 - -テストピラミッド: -``` - ┌─────────────┐ - │ E2E Test │ ← 少数: 全体フロー確認 - ├─────────────┤ - │ Integration │ ← Command→Event→Projection→Query の連携確認 - ├─────────────┤ - │ Unit Test │ ← 多数: 各レイヤー独立テスト - └─────────────┘ -``` - -Command側（Aggregate）: -```kotlin -// AggregateTestFixture使用 -@Test -fun `確定コマンドでイベントが発行される`() { - fixture - .given(OrderPlacedEvent(...)) - .`when`(ConfirmOrderCommand(orderId, confirmedBy)) - .expectSuccessfulHandlerExecution() - .expectEvents(OrderConfirmedEvent(...)) -} -``` - -Query側: -```kotlin -// Read Model直接セットアップ + QueryGateway -@Test -fun `注文詳細が取得できる`() { - // Given: Read Modelを直接セットアップ - orderRepository.save(OrderEntity(...)) - - // When: QueryGateway経由でクエリ実行 - val detail = queryGateway.query(GetOrderDetailQuery(orderId), ...).join() - - // Then - assertEquals(expectedDetail, detail) -} -``` - -チェック項目: - -| 観点 | 判定 | -|------|------| -| Aggregateテストが状態ではなくイベントを検証している | 必須 | -| Query側テストがCommand経由でデータを作っていない | 推奨 | -| 統合テストでAxonの非同期処理を考慮している | 必須 | - -### インフラ層 - -確認事項: -- イベントストアの選択は適切か -- メッセージング基盤は要件を満たすか -- スナップショット戦略は定義されているか -- イベントのシリアライズ形式は適切か diff --git a/resources/global/ja/personas/frontend-reviewer.md b/resources/global/ja/personas/frontend-reviewer.md index d1ed85f..5ae2334 100644 --- a/resources/global/ja/personas/frontend-reviewer.md +++ b/resources/global/ja/personas/frontend-reviewer.md @@ -27,501 +27,3 @@ - アクセシビリティは後付け困難。最初から組み込む - 過度な抽象化を警戒。シンプルに保つ - フレームワークの作法に従う。独自パターンより標準的なアプローチ - -## ドメイン知識 - -### コンポーネント設計 - -1ファイルにベタ書きしない。必ずコンポーネント分割する。 - -分離が必須なケース: -- 独自のstateを持つ → 必ず分離 -- 50行超のJSX → 分離 -- 再利用可能 → 分離 -- 責務が複数 → 分離 -- ページ内の独立したセクション → 分離 - -| 基準 | 判定 | -|------|------| -| 1コンポーネント200行超 | 分割を検討 | -| 1コンポーネント300行超 | REJECT | -| 表示とロジックが混在 | 分離を検討 | -| Props drilling（3階層以上） | 状態管理の導入を検討 | -| 複数の責務を持つコンポーネント | REJECT | - -良いコンポーネント: -- 単一責務: 1つのことをうまくやる -- 自己完結: 必要な依存が明確 -- テスト可能: 副作用が分離されている - -コンポーネント分類: - -| 種類 | 責務 | 例 | -|------|------|-----| -| Container | データ取得・状態管理 | `UserListContainer` | -| Presentational | 表示のみ | `UserCard` | -| Layout | 配置・構造 | `PageLayout`, `Grid` | -| Utility | 共通機能 | `ErrorBoundary`, `Portal` | - -ディレクトリ構成: -``` -features/{feature-name}/ -├── components/ -│ ├── {feature}-view.tsx # メインビュー（子を組み合わせる） -│ ├── {sub-component}.tsx # サブコンポーネント -│ └── index.ts -├── hooks/ -├── types.ts -└── index.ts -``` - -### 状態管理 - -子コンポーネントは自身で状態を変更しない。イベントを親にバブリングし、親が状態を操作する。 - -```tsx -// 子が自分で状態を変更（NG） -const ChildBad = ({ initialValue }: { initialValue: string }) => { - const [value, setValue] = useState(initialValue) - return setValue(e.target.value)} /> -} - -// 親が状態を管理、子はコールバックで通知（OK） -const ChildGood = ({ value, onChange }: { value: string; onChange: (v: string) => void }) => { - return onChange(e.target.value)} /> -} - -const Parent = () => { - const [value, setValue] = useState('') - return -} -``` - -例外（子がローカルstate持ってOK）: -- UI専用の一時状態（ホバー、フォーカス、アニメーション） -- 親に伝える必要がない完全にローカルな状態 - -| 基準 | 判定 | -|------|------| -| 不要なグローバル状態 | ローカル化を検討 | -| 同じ状態が複数箇所で管理 | 正規化が必要 | -| 子から親への状態変更（逆方向データフロー） | REJECT | -| APIレスポンスをそのまま状態に | 正規化を検討 | -| useEffectの依存配列が不適切 | REJECT | - -状態配置の判断基準: - -| 状態の性質 | 推奨配置 | -|-----------|---------| -| UIの一時的な状態（モーダル開閉等） | ローカル（useState） | -| フォームの入力値 | ローカル or フォームライブラリ | -| 複数コンポーネントで共有 | Context or 状態管理ライブラリ | -| サーバーデータのキャッシュ | TanStack Query等のデータフェッチライブラリ | - -### データ取得 - -API呼び出しはルート（View）コンポーネントで行い、子コンポーネントにはpropsで渡す。 - -```tsx -// CORRECT - ルートでデータ取得、子に渡す -const OrderDetailView = () => { - const { data: order, isLoading, error } = useGetOrder(orderId) - const { data: items } = useListOrderItems(orderId) - - if (isLoading) return - if (error) return - - return ( - - ) -} - -// WRONG - 子コンポーネントが自分でデータ取得 -const OrderSummary = ({ orderId }) => { - const { data: order } = useGetOrder(orderId) - // ... -} -``` - -UIの状態変更でパラメータが変わる場合（週切り替え、フィルタ等）: - -状態もViewレベルで管理し、コンポーネントにはコールバックを渡す。 - -```tsx -// CORRECT - 状態もViewで管理 -const ScheduleView = () => { - const [currentWeek, setCurrentWeek] = useState(startOfWeek(new Date())) - const { data } = useListSchedules({ - from: format(currentWeek, 'yyyy-MM-dd'), - to: format(endOfWeek(currentWeek), 'yyyy-MM-dd'), - }) - - return ( - - ) -} - -// WRONG - コンポーネント内で状態管理+データ取得 -const WeeklyCalendar = ({ facilityId }) => { - const [currentWeek, setCurrentWeek] = useState(...) - const { data } = useListSchedules({ facilityId, from, to }) - // ... -} -``` - -例外（コンポーネント内フェッチが許容されるケース）: - -| ケース | 理由 | -|--------|------| -| 無限スクロール | スクロール位置というUI内部状態に依存 | -| 検索オートコンプリート | 入力値に依存したリアルタイム検索 | -| 独立したウィジェット | 通知バッジ、天気等。親のデータと完全に無関係 | -| リアルタイム更新 | WebSocket/Pollingでの自動更新 | -| モーダル内の詳細取得 | 開いたときだけ追加データを取得 | - -判断基準: 「親が管理する意味がない / 親に影響を与えない」ケースのみ許容。 - -| 基準 | 判定 | -|------|------| -| コンポーネント内で直接fetch | Container層に分離 | -| エラーハンドリングなし | REJECT | -| ローディング状態の未処理 | REJECT | -| キャンセル処理なし | 警告 | -| N+1クエリ的なフェッチ | REJECT | - -### 共有コンポーネントと抽象化 - -同じパターンのUIは共有コンポーネント化する。インラインスタイルのコピペは禁止。 - -```tsx -// WRONG - インラインスタイルのコピペ - - -// CORRECT - 共有コンポーネント使用 - - - -``` - -共有コンポーネント化すべきパターン: -- アイコンボタン（閉じる、編集、削除等） -- ローディング/エラー表示 -- ステータスバッジ -- タブ切り替え -- ラベル+値の表示（詳細画面） -- 検索入力 -- カラー凡例 - -過度な汎用化を避ける: - -```tsx -// WRONG - IconButtonに無理やりステッパー用バリアントを追加 -export const iconButtonVariants = cva('...', { - variants: { - variant: { - default: '...', - outlined: '...', // ステッパー専用、他で使わない - }, - size: { - medium: 'p-2', - stepper: 'w-8 h-8', // outlinedとセットでしか使わない - }, - }, -}) - -// CORRECT - 用途別に専用コンポーネント -export function StepperButton(props) { - return ( - - ) -} -``` - -別コンポーネントにすべきサイン: -- 「このvariantはこのsizeとセット」のような暗黙の制約がある -- 追加したvariantが元のコンポーネントの用途と明らかに違う -- 使う側のprops指定が複雑になる - -### 抽象化レベルの評価 - -**条件分岐の肥大化検出** - -| パターン | 判定 | -|---------|------| -| 同じ条件分岐が3箇所以上 | 共通コンポーネントに抽出 → REJECT | -| propsによる分岐が5種類以上 | コンポーネント分割を検討 | -| render内の三項演算子のネスト | 早期リターンまたはコンポーネント分離 → REJECT | -| 型による分岐レンダリング | ポリモーフィックコンポーネントを検討 | - -**抽象度の不一致検出** - -| パターン | 問題 | 修正案 | -|---------|------|--------| -| データ取得ロジックがJSXに混在 | 読みにくい | カスタムフックに抽出 | -| ビジネスロジックがコンポーネントに混在 | 責務違反 | hooks/utilsに分離 | -| スタイル計算ロジックが散在 | 保守困難 | ユーティリティ関数に抽出 | -| 同じ変換処理が複数箇所に | DRY違反 | 共通関数に抽出 | - -良い抽象化の例: - -```tsx -// 条件分岐が肥大化 -function UserBadge({ user }) { - if (user.role === 'admin') { - return 管理者 - } else if (user.role === 'moderator') { - return モデレーター - } else if (user.role === 'premium') { - return プレミアム - } else { - return 一般 - } -} - -// Mapで抽象化 -const ROLE_CONFIG = { - admin: { label: '管理者', className: 'bg-red-500' }, - moderator: { label: 'モデレーター', className: 'bg-yellow-500' }, - premium: { label: 'プレミアム', className: 'bg-purple-500' }, - default: { label: '一般', className: 'bg-gray-500' }, -} - -function UserBadge({ user }) { - const config = ROLE_CONFIG[user.role] ?? ROLE_CONFIG.default - return {config.label} -} -``` - -```tsx -// 抽象度が混在 -function OrderList() { - const [orders, setOrders] = useState([]) - useEffect(() => { - fetch('/api/orders') - .then(res => res.json()) - .then(data => setOrders(data)) - }, []) - - return orders.map(order => ( -
{order.total.toLocaleString()}円
- )) -} - -// 抽象度を揃える -function OrderList() { - const { data: orders } = useOrders() // データ取得を隠蔽 - - return orders.map(order => ( - - )) -} -``` - -### フロントエンドとバックエンドの責務分離 - -**表示形式の責務** - -バックエンドは「データ」を返し、フロントエンドが「表示形式」に変換する。 - -```tsx -// フロントエンド: 表示形式に変換 -export function formatPrice(amount: number): string { - return `¥${amount.toLocaleString()}` -} - -export function formatDate(date: Date): string { - return format(date, 'yyyy年M月d日') -} -``` - -| 基準 | 判定 | -|------|------| -| バックエンドが表示用文字列を返している | 設計見直しを提案 | -| 同じフォーマット処理が複数箇所にコピペ | ユーティリティ関数に統一 | -| コンポーネント内でインラインフォーマット | 関数に抽出 | - -**ドメインロジックの配置（SmartUI排除）** - -ドメインロジック（ビジネスルール）はバックエンドに配置。フロントエンドは状態の表示・編集のみ。 - -ドメインロジックとは: -- 集約のビジネスルール（在庫判定、価格計算、ステータス遷移） -- バリデーション（業務制約の検証） -- 不変条件の保証 - -フロントエンドの責務: -- サーバーから受け取った状態を表示 -- ユーザー入力を収集し、コマンドとしてバックエンドに送信 -- UI専用の一時状態管理（フォーカス、ホバー、モーダル開閉） -- 表示形式の変換（フォーマット、ソート、フィルタ） - -| 基準 | 判定 | -|------|------| -| フロントエンドで価格計算・在庫判定 | バックエンドに移動 → REJECT | -| フロントエンドでステータス遷移ルール | バックエンドに移動 → REJECT | -| フロントエンドでビジネスバリデーション | バックエンドに移動 → REJECT | -| サーバー側で計算可能な値をフロントで再計算 | 冗長 → REJECT | - -良い例 vs 悪い例: - -```tsx -// BAD - フロントエンドでビジネスルール -function OrderForm({ order }: { order: Order }) { - const totalPrice = order.items.reduce((sum, item) => - sum + item.price * item.quantity, 0 - ) - const canCheckout = totalPrice >= 1000 && order.items.every(i => i.stock > 0) - - return -} - -// GOOD - バックエンドから受け取った状態を表示 -function OrderForm({ order }: { order: Order }) { - // totalPrice, canCheckout はサーバーから受け取る - return ( - <> -
{formatPrice(order.totalPrice)}
- - - ) -} -``` - -```tsx -// BAD - フロントエンドでステータス遷移判定 -function TaskCard({ task }: { task: Task }) { - const canStart = task.status === 'pending' && task.assignee !== null - const canComplete = task.status === 'in_progress' && /* 複雑な条件... */ - - return ( - <> - - - - ) -} - -// GOOD - サーバーが許可するアクションを返す -function TaskCard({ task }: { task: Task }) { - // task.allowedActions = ['start', 'cancel'] など、サーバーが計算 - const canStart = task.allowedActions.includes('start') - const canComplete = task.allowedActions.includes('complete') - - return ( - <> - - - - ) -} -``` - -例外（フロントエンドにロジックを置いてもOK）: - -| ケース | 理由 | -|--------|------| -| UI専用バリデーション | 「必須入力」「文字数制限」等のUXフィードバック（サーバー側でも検証必須） | -| クライアント側フィルタ/ソート | サーバーから受け取ったリストの表示順序変更 | -| 表示条件の分岐 | 「ログイン済みなら詳細表示」等のUI制御 | -| リアルタイムフィードバック | 入力中のプレビュー表示 | - -判断基準: 「この計算結果がサーバーとズレたら業務が壊れるか?」 -- YES → バックエンドに配置（ドメインロジック） -- NO → フロントエンドでもOK（表示ロジック） - -### パフォーマンス - -| 基準 | 判定 | -|------|------| -| 不要な再レンダリング | 最適化が必要 | -| 大きなリストの仮想化なし | 警告 | -| 画像の最適化なし | 警告 | -| バンドルに未使用コード | tree-shakingを確認 | -| メモ化の過剰使用 | 本当に必要か確認 | - -最適化チェックリスト: -- `React.memo` / `useMemo` / `useCallback` は適切か -- 大きなリストは仮想スクロール対応か -- Code Splittingは適切か -- 画像はlazy loadingされているか - -アンチパターン: - -```tsx -// レンダリングごとに新しいオブジェクト - - -// 定数化 or useMemo -const style = useMemo(() => ({ color: 'red' }), []); - -``` - -### アクセシビリティ - -| 基準 | 判定 | -|------|------| -| インタラクティブ要素にキーボード対応なし | REJECT | -| 画像にalt属性なし | REJECT | -| フォーム要素にlabelなし | REJECT | -| 色だけで情報を伝達 | REJECT | -| フォーカス管理の欠如（モーダル等） | REJECT | - -チェックリスト: -- セマンティックHTMLを使用しているか -- ARIA属性は適切か（過剰でないか） -- キーボードナビゲーション可能か -- スクリーンリーダーで意味が通じるか -- カラーコントラストは十分か - -### TypeScript/型安全性 - -| 基準 | 判定 | -|------|------| -| `any` 型の使用 | REJECT | -| 型アサーション（as）の乱用 | 要検討 | -| Props型定義なし | REJECT | -| イベントハンドラの型が不適切 | 修正が必要 | - -### フロントエンドセキュリティ - -| 基準 | 判定 | -|------|------| -| dangerouslySetInnerHTML使用 | XSSリスクを確認 | -| ユーザー入力の未サニタイズ | REJECT | -| 機密情報のフロントエンド保存 | REJECT | -| CSRFトークンの未使用 | 要確認 | - -### テスタビリティ - -| 基準 | 判定 | -|------|------| -| data-testid等の未付与 | 警告 | -| テスト困難な構造 | 分離を検討 | -| ビジネスロジックのUIへの埋め込み | REJECT | - -### アンチパターン検出 - -以下を見つけたら REJECT: - -| アンチパターン | 問題 | -|---------------|------| -| God Component | 1コンポーネントに全機能が集中 | -| Prop Drilling | 深いPropsバケツリレー | -| Inline Styles乱用 | 保守性低下 | -| useEffect地獄 | 依存関係が複雑すぎる | -| Premature Optimization | 不要なメモ化 | -| Magic Strings | ハードコードされた文字列 | -| Hidden Dependencies | 子コンポーネントの隠れたAPI呼び出し | -| Over-generalization | 無理やり汎用化したコンポーネント | diff --git a/resources/global/ja/personas/security-reviewer.md b/resources/global/ja/personas/security-reviewer.md index daa9d41..5efb41c 100644 --- a/resources/global/ja/personas/security-reviewer.md +++ b/resources/global/ja/personas/security-reviewer.md @@ -24,168 +24,3 @@ - 「信頼しない、検証する」が基本原則 - 1つの脆弱性がシステム全体を危険にさらす。見逃しは許されない - AI生成コードには特有の脆弱性パターンがある。特に厳しく審査する - -## ドメイン知識 - -### AI生成コードのセキュリティ問題 - -AI生成コードには特有の脆弱性パターンがある。 - -| パターン | リスク | 例 | -|---------|--------|-----| -| もっともらしいが危険なデフォルト | 高 | `cors: { origin: '*' }` は問題なく見えるが危険 | -| 古いセキュリティプラクティス | 中 | 非推奨の暗号化、古い認証パターンの使用 | -| 不完全なバリデーション | 高 | 形式は検証するがビジネスルールを検証しない | -| 入力を過度に信頼 | 重大 | 内部APIは常に安全と仮定 | -| コピペによる脆弱性 | 高 | 同じ危険なパターンが複数ファイルで繰り返される | - -特に厳しく審査が必要: -- 認証・認可ロジック（AIはエッジケースを見落としがち） -- 入力バリデーション（AIは構文を検証しても意味を見落とす可能性） -- エラーメッセージ（AIは内部詳細を露出する可能性） -- 設定ファイル（AIは学習データから危険なデフォルトを使う可能性） - -### インジェクション攻撃 - -**SQLインジェクション** - -- 文字列連結によるSQL構築 → REJECT -- パラメータ化クエリの不使用 → REJECT -- ORMの raw query での未サニタイズ入力 → REJECT - -```typescript -// NG -db.query(`SELECT * FROM users WHERE id = ${userId}`) - -// OK -db.query('SELECT * FROM users WHERE id = ?', [userId]) -``` - -**コマンドインジェクション** - -- `exec()`, `spawn()` での未検証入力 → REJECT -- シェルコマンド構築時のエスケープ不足 → REJECT - -```typescript -// NG -exec(`ls ${userInput}`) - -// OK -execFile('ls', [sanitizedInput]) -``` - -**XSS (Cross-Site Scripting)** - -- HTML/JSへの未エスケープ出力 → REJECT -- `innerHTML`, `dangerouslySetInnerHTML` の不適切な使用 → REJECT -- URLパラメータの直接埋め込み → REJECT - -### 認証・認可 - -**認証の問題** - -- ハードコードされたクレデンシャル → 即REJECT -- 平文パスワードの保存 → 即REJECT -- 弱いハッシュアルゴリズム (MD5, SHA1) → REJECT -- セッショントークンの不適切な管理 → REJECT - -**認可の問題** - -- 権限チェックの欠如 → REJECT -- IDOR (Insecure Direct Object Reference) → REJECT -- 権限昇格の可能性 → REJECT - -```typescript -// NG - 権限チェックなし -app.get('/user/:id', (req, res) => { - return db.getUser(req.params.id) -}) - -// OK -app.get('/user/:id', authorize('read:user'), (req, res) => { - if (req.user.id !== req.params.id && !req.user.isAdmin) { - return res.status(403).send('Forbidden') - } - return db.getUser(req.params.id) -}) -``` - -### データ保護 - -**機密情報の露出** - -- APIキー、シークレットのハードコーディング → 即REJECT -- ログへの機密情報出力 → REJECT -- エラーメッセージでの内部情報露出 → REJECT -- `.env` ファイルのコミット → REJECT - -**データ検証** - -- 入力値の未検証 → REJECT -- 型チェックの欠如 → REJECT -- サイズ制限の未設定 → REJECT - -### 暗号化 - -- 弱い暗号アルゴリズムの使用 → REJECT -- 固定IV/Nonceの使用 → REJECT -- 暗号化キーのハードコーディング → 即REJECT -- HTTPSの未使用（本番環境） → REJECT - -### ファイル操作 - -**パストラバーサル** - -- ユーザー入力を含むファイルパス → REJECT -- `../` のサニタイズ不足 → REJECT - -```typescript -// NG -const filePath = path.join(baseDir, userInput) -fs.readFile(filePath) - -// OK -const safePath = path.resolve(baseDir, userInput) -if (!safePath.startsWith(path.resolve(baseDir))) { - throw new Error('Invalid path') -} -``` - -**ファイルアップロード** - -- ファイルタイプの未検証 → REJECT -- ファイルサイズ制限なし → REJECT -- 実行可能ファイルのアップロード許可 → REJECT - -### 依存関係 - -- 既知の脆弱性を持つパッケージ → REJECT -- メンテナンスされていないパッケージ → 警告 -- 不必要な依存関係 → 警告 - -### エラーハンドリング - -- スタックトレースの本番露出 → REJECT -- 詳細なエラーメッセージの露出 → REJECT -- エラーの握りつぶし（セキュリティイベント） → REJECT - -### レート制限・DoS対策 - -- レート制限の欠如（認証エンドポイント） → 警告 -- リソース枯渇攻撃の可能性 → 警告 -- 無限ループの可能性 → REJECT - -### OWASP Top 10 チェックリスト - -| カテゴリ | 確認事項 | -|---------|---------| -| A01 Broken Access Control | 認可チェック、CORS設定 | -| A02 Cryptographic Failures | 暗号化、機密データ保護 | -| A03 Injection | SQL, コマンド, XSS | -| A04 Insecure Design | セキュリティ設計パターン | -| A05 Security Misconfiguration | デフォルト設定、不要な機能 | -| A06 Vulnerable Components | 依存関係の脆弱性 | -| A07 Auth Failures | 認証メカニズム | -| A08 Software Integrity | コード署名、CI/CD | -| A09 Logging Failures | セキュリティログ | -| A10 SSRF | サーバーサイドリクエスト | diff --git a/resources/global/ja/pieces/default.yaml b/resources/global/ja/pieces/default.yaml index 1ae11b5..b3efe6a 100644 --- a/resources/global/ja/pieces/default.yaml +++ b/resources/global/ja/pieces/default.yaml @@ -20,6 +20,9 @@ stances: review: ../stances/review.md testing: ../stances/testing.md +knowledge: + architecture: ../knowledge/architecture.md + personas: planner: ../personas/planner.md architect-planner: ../personas/architect-planner.md @@ -224,6 +227,7 @@ movements: edit: false persona: architecture-reviewer stance: review + knowledge: architecture report: name: 05-architect-review.md format: architecture-review diff --git a/resources/global/ja/pieces/expert-cqrs.yaml b/resources/global/ja/pieces/expert-cqrs.yaml index bc6bf7f..3b39625 100644 --- a/resources/global/ja/pieces/expert-cqrs.yaml +++ b/resources/global/ja/pieces/expert-cqrs.yaml @@ -37,6 +37,11 @@ stances: review: ../stances/review.md testing: ../stances/testing.md +knowledge: + frontend: ../knowledge/frontend.md + cqrs-es: ../knowledge/cqrs-es.md + security: ../knowledge/security.md + personas: planner: ../personas/planner.md coder: ../personas/coder.md @@ -205,6 +210,7 @@ movements: edit: false persona: cqrs-es-reviewer stance: review + knowledge: cqrs-es report: name: 04-cqrs-es-review.md format: cqrs-es-review @@ -224,6 +230,7 @@ movements: edit: false persona: frontend-reviewer stance: review + knowledge: frontend report: name: 05-frontend-review.md format: frontend-review @@ -243,6 +250,7 @@ movements: edit: false persona: security-reviewer stance: review + knowledge: security report: name: 06-security-review.md format: security-review diff --git a/resources/global/ja/pieces/expert.yaml b/resources/global/ja/pieces/expert.yaml index 8671bc1..145d3e5 100644 --- a/resources/global/ja/pieces/expert.yaml +++ b/resources/global/ja/pieces/expert.yaml @@ -28,6 +28,11 @@ stances: review: ../stances/review.md testing: ../stances/testing.md +knowledge: + frontend: ../knowledge/frontend.md + security: ../knowledge/security.md + architecture: ../knowledge/architecture.md + personas: planner: ../personas/planner.md coder: ../personas/coder.md @@ -195,6 +200,7 @@ movements: edit: false persona: architecture-reviewer stance: review + knowledge: architecture report: name: 04-architect-review.md format: architecture-review @@ -214,6 +220,7 @@ movements: edit: false persona: frontend-reviewer stance: review + knowledge: frontend report: name: 05-frontend-review.md format: frontend-review @@ -233,6 +240,7 @@ movements: edit: false persona: security-reviewer stance: review + knowledge: security report: name: 06-security-review.md format: security-review