From 3848a772dae77cda48bd17859ef78d0a2fe9ea7d Mon Sep 17 00:00:00 2001 From: Shinsuke Kagawa Date: Sun, 19 Jul 2026 15:50:57 +0900 Subject: [PATCH] refactor: improve skill execution guidance --- .agents/skills/ai-development-guide/SKILL.md | 120 ++-------- .../references/frontend.md | 50 ---- .agents/skills/coding-rules/SKILL.md | 4 +- .../coding-rules/references/typescript.md | 85 +------ .../references/design-template.md | 2 +- .../skills/implementation-approach/SKILL.md | 24 +- .../skills/integration-e2e-testing/SKILL.md | 7 + .../subagents-orchestration-guide/SKILL.md | 10 +- .../references/skills-index.yaml | 11 +- .agents/skills/testing/SKILL.md | 134 ++--------- .../skills/testing/references/typescript.md | 217 +++--------------- .codex/agents/acceptance-test-generator.toml | 8 +- .codex/agents/work-planner.toml | 8 +- package.json | 2 +- 14 files changed, 110 insertions(+), 572 deletions(-) diff --git a/.agents/skills/ai-development-guide/SKILL.md b/.agents/skills/ai-development-guide/SKILL.md index 1800385..de71aa7 100644 --- a/.agents/skills/ai-development-guide/SKILL.md +++ b/.agents/skills/ai-development-guide/SKILL.md @@ -1,6 +1,6 @@ --- name: ai-development-guide -description: "Anti-pattern detection, debugging techniques, quality check workflow, and implementation completeness assurance. Use when: fixing bugs, reviewing code quality, refactoring, making technical decisions, or performing quality assurance." +description: "Anti-pattern detection, root-cause discipline, quality check workflow, and implementation completeness assurance. Use when: fixing bugs, reviewing code quality, refactoring, making technical decisions, or performing quality assurance." --- # AI Developer Guide - Technical Decision Criteria and Anti-pattern Collection @@ -24,7 +24,7 @@ Explore broadly, then converge on the lowest-lifecycle-cost solution that delive ### Code Quality Anti-patterns 1. **Writing similar code 3 or more times** - Violates Rule of Three 2. **Multiple responsibilities mixed in a single file** - Violates Single Responsibility Principle (SRP) -3. **Defining same content in multiple files** - Violates DRY principle +3. **Maintaining the same runtime rule or data in multiple implementation sources when one source can safely serve all consumers** - Creates synchronization risk 4. **Making changes without checking dependencies** - Potential for unexpected impacts 5. **Disabling code with comments** - Should use version control 6. **Error suppression** - Hiding problems creates technical debt @@ -76,19 +76,6 @@ Make errors explicit with full context. Prioritize primary code reliability over **ENFORCEMENT**: Fallbacks without Design Doc approval are PROHIBITED -### Implementation Pattern - -``` -AVOID: Silent fallback that hides errors - : - return DEFAULT_VALUE // Error hidden, debugging impossible - -PREFERRED: Explicit failure with context - : - log_error('Operation failed', context, error) - // Re-throw, return Error type, return error tuple -``` - ## Rule of Three - Criteria for Code Duplication How to handle duplicate code based on Martin Fowler's "Refactoring": @@ -113,78 +100,15 @@ How to handle duplicate code based on Martin Fowler's "Refactoring": - Significant readability decrease from commonalization - Simple helpers in test code -## Common Failure Patterns and Avoidance Methods - -### Pattern 1: Error Fix Chain -**Symptom**: Fixing one error causes new errors -**Cause**: Surface-level fixes without understanding root cause -**Avoidance**: Identify root cause with 5 Whys before fixing - -### Pattern 2: Circumventing Correctness Guarantees -**Symptom**: Bypassing safety mechanisms (type systems, validation, contracts) -**Cause**: Impulse to avoid correctness errors -**Avoidance**: Use language-appropriate safety mechanisms - -### Pattern 3: Implementation Without Sufficient Testing -**Symptom**: Many bugs after implementation -**Cause**: Ignoring Red-Green-Refactor process -**Avoidance**: Always start with failing tests - -### Pattern 4: Ignoring Technical Uncertainty -**Symptom**: Frequent unexpected errors when introducing new technology -**Cause**: Assuming things work without prior investigation -**Avoidance**: -- Record certainty evaluation at the beginning of task files -- For low certainty cases, create minimal verification code first - -### Pattern 5: Insufficient Existing Code Investigation -**Symptom**: Duplicate implementations, architecture inconsistency, integration failures, adopting outdated patterns -**Cause**: Insufficient understanding of existing code before implementation; referencing only nearby files without checking representativeness -**Avoidance**: -- Before implementation, always search for similar functionality -- Similar functionality found: Use that implementation (do not create new) +## Pattern 5: Existing Code Investigation [MANDATORY] + +Before implementation: +- Search for similar functionality +- Similar functionality found: Verify that it satisfies the current requirement and is representative of the repository; reuse or extend it when both checks pass - Similar functionality is technical debt: Repair it when it blocks the current outcome, was caused by the current change, or lies in confirmed scope; otherwise report it separately. When the repair requires an architectural decision, record the decision in an ADR -- No similar functionality: Implement following existing design philosophy +- No suitable similar functionality: Implement using representative repository patterns - When adopting a pattern or dependency from nearby code, verify it is representative across the repository before adopting it -## Debugging Techniques - -### 1. Error Analysis Procedure -1. Read error message (first line) accurately -2. Focus on first and last of stack trace -3. Identify first line where your code appears - -### 2. 5 Whys - Root Cause Analysis -``` -Example: -Symptom: Build error -Why1: Contract definitions don't match -Why2: Interface was updated -Why3: Dependency change -Why4: Package update impact -Why5: Major version upgrade with breaking changes -Root cause: Inappropriate version specification in dependency manifest -``` - -### 3. Minimal Reproduction Code -To isolate problems, attempt reproduction with minimal code: -- Remove unrelated parts -- Replace external dependencies with mocks -- Create minimal configuration that reproduces problem - -### 4. Debug Log Output (temporary) -Add structured debug logs to isolate the issue, then remove them before commit. - -``` -Pattern: Structured logging with context -{ - context: 'operation-name', - input: { relevant, input, data }, - state: currentState, - timestamp: current_time_ISO8601 -} -``` - ## Quality Assurance Mechanism Awareness Before executing quality checks, discover applicable quality tools and constraints by inspecting the affected files' types, project manifests, CI pipelines, and configuration: @@ -225,21 +149,6 @@ All checks MUST pass before proceeding: **ENFORCEMENT**: Cannot proceed with ANY quality check failures — fix ALL errors before marking task complete -## Situations Requiring Technical Decisions - -### Timing of Abstraction -- Extract patterns after writing concrete implementation 3 times (Rule of Three) - -### Performance vs Readability -- Prioritize readability unless clear bottleneck exists -- **Measure first** — profile before optimizing (no guessing) -- Document reason with comments when optimizing - -### Granularity of Contracts and Interfaces -- Overly detailed contracts reduce maintainability -- Design interfaces that appropriately express domain -- Use abstraction mechanisms to reduce duplication - ## Implementation Completeness Assurance ### Impact Analysis: Mandatory 3-Stage Process [MANDATORY] @@ -286,15 +195,20 @@ Complete these stages sequentially before any implementation: ### Unused Code Deletion When unused code is detected: -- Will it be used in this work? Yes: Implement now | No: Delete now (Git preserves) +- Within confirmed scope or dependencies required for the current outcome: Will it be used in this work? Yes: Implement now | No: Delete now (Git preserves) +- Outside that boundary: Report it separately for a scope decision - Applies to: Code, tests, docs, configs, assets ### Existing Code Modification ``` -In use? No -> Delete - Yes -> Working? No -> Delete + Reimplement - Yes -> Fix/Extend +Within confirmed scope or required dependency? + No -> Report separately + Yes -> In use? + No -> Delete + Yes -> Working? + No -> Delete + Reimplement + Yes -> Fix/Extend ``` **Principle**: Prefer clean implementation over patching broken code diff --git a/.agents/skills/ai-development-guide/references/frontend.md b/.agents/skills/ai-development-guide/references/frontend.md index 338179a..63bd645 100644 --- a/.agents/skills/ai-development-guide/references/frontend.md +++ b/.agents/skills/ai-development-guide/references/frontend.md @@ -16,16 +16,6 @@ In addition to the general anti-patterns in SKILL.md, detect these frontend-spec - Custom hooks with shared logic - Validation rules for form inputs -**Implementation Example**: -```typescript -// Bad: Immediate commonalization on 1st duplication -function UserEmailInput() { /* ... */ } -function ContactEmailInput() { /* ... */ } - -// Good: Commonalize on 3rd occurrence -function EmailInput({ context }: { context: 'user' | 'contact' | 'admin' }) { /* ... */ } -``` - ## Frontend Fallback Design ### Layer Responsibilities (React-specific) @@ -38,41 +28,6 @@ function EmailInput({ context }: { context: 'user' | 'contact' | 'admin' }) { /* - Verify Design Doc definition before implementing fallbacks - Log errors explicitly and make failures visible -## Frontend Debugging Techniques - -### Error Analysis (React-specific) -1. Read error message (first line) accurately -2. Focus on first and last of stack trace -3. Identify first line where your code appears -4. **Check React DevTools for component hierarchy** - -### 5 Whys Example (Frontend) -``` -Symptom: Component not rendering -Why1: Props are undefined -Why2: Parent component didn't pass props -Why3: Parent using old prop names -Why4: Component interface was updated -Why5: No update to parent after refactoring -Root cause: Incomplete refactoring, missing call-site updates -``` - -### Minimal Reproduction (React-specific) -- Remove unrelated components -- Replace API calls with mocks -- Create minimal configuration that reproduces the problem -- Use React DevTools to inspect component tree - -### Debug Log Pattern (Frontend) -```typescript -console.log('DEBUG:', { - context: 'user-form-submission', - props: { email, name }, - state: currentState, - timestamp: new Date().toISOString() -}) -``` - ## Frontend Quality Check Workflow Read `package.json` scripts and run them with the project's package manager from the `packageManager` field. Map the phases below using the script names declared in `package.json`. @@ -84,11 +39,6 @@ Read `package.json` scripts and run them with the project's package manager from 4. **Test** - unit and integration tests 5. **Coverage** - coverage run when configured or when the task added or changed behavior -### Troubleshooting -- **Port already in use**: stop the stale dev, preview, or test process holding the port -- **Stale cache**: re-run with the project's fresh or clean-cache option -- **Dependency errors**: clean reinstall dependencies - ## Frontend Technical Decisions ### Component/Type Granularity diff --git a/.agents/skills/coding-rules/SKILL.md b/.agents/skills/coding-rules/SKILL.md index dbf4243..d1ab3c7 100644 --- a/.agents/skills/coding-rules/SKILL.md +++ b/.agents/skills/coding-rules/SKILL.md @@ -14,7 +14,7 @@ For language-specific rules, also read: 1. **Maintainability over Speed**: Prioritize long-term code health 2. **Simplicity First**: YAGNI principle — simplest solution that meets requirements -3. **Design Convergence**: Deliver the current required outcome with the least new design surface. Use "Design Surface Terms" below for classification and implementation-approach for design selection. +3. **Design Convergence**: Deliver the current required outcome with the least new design surface. Add surface only to satisfy a current requirement, verified constraint, observed problem, or evidence-backed material risk; among sufficient options, choose the lowest-lifecycle-cost option. 4. **Explicit over Implicit**: Clear intentions through code structure and naming 5. **Delete over Comment**: Remove unused code instead of commenting it out @@ -27,7 +27,7 @@ Use these definitions when classifying Design Convergence additions or code-revi - **Maintenance-surface-bearing elements**: persistent state; public-contract or cross-boundary fields/props; behavioral modes, flags, or variants; reusable abstractions; extracted services; shared utilities; component splits. - **Non-surface elements**: private local variables, internal helper functions with no external observers, test fixtures or mocks, temporary migration scaffolding removed before completion, and private implementation details confined to one function or file. - **Classification precedence**: When an element matches both surface-bearing and non-surface conditions, classify it as surface-bearing. -- **Selection rule**: Follow implementation-approach Design Convergence for design selection. These terms classify design surface; they do not define a second selection process. +- **Selection rule**: Add a surface-bearing element only when it is required by a current requirement, verified constraint, observed problem, or evidence-backed material risk. Prefer fewer new elements only when the remaining candidates are otherwise sufficient and equivalent. - **Relation to YAGNI**: YAGNI decides present vs. future need over time; Design Convergence minimizes surface area for the current accepted scope. ## Code Quality [MANDATORY] diff --git a/.agents/skills/coding-rules/references/typescript.md b/.agents/skills/coding-rules/references/typescript.md index 5ed3efc..c31294b 100644 --- a/.agents/skills/coding-rules/references/typescript.md +++ b/.agents/skills/coding-rules/references/typescript.md @@ -22,19 +22,6 @@ 3. **Union Types / Intersection Types**: Combinations of multiple types 4. **Type Assertions (Last Resort)**: Only when type is certain -**Type Guard Implementation Pattern** -```typescript -function isUser(value: unknown): value is User { - return typeof value === 'object' && value !== null && 'id' in value && 'name' in value -} -``` - -**Modern Type Features** -- **satisfies Operator**: `const config = { apiUrl: '/api' } satisfies Config` - Preserves inference -- **const Assertion**: `const ROUTES = { HOME: '/' } as const satisfies Routes` - Immutable and type-safe -- **Branded Types**: `type UserId = string & { __brand: 'UserId' }` - Distinguish meaning -- **Template Literal Types**: `type EventName = \`on\${Capitalize}\`` - Express string patterns with types - **Type Safety in Frontend Implementation** - **React Props/State**: TypeScript manages types, unknown unnecessary - **External API Responses**: Always receive as `unknown`, validate with type guards @@ -57,8 +44,8 @@ function isUser(value: unknown): value is User { ## Coding Conventions **Component Design Criteria** -- **Function Components (Mandatory)**: Official React recommendation, optimizable by modern tooling -- **Classes Prohibited**: Class components completely deprecated (Exception: Error Boundary) +- **Function Components (Mandatory)**: Use function components for new and modified React components +- **Class Component Exception**: Retain or introduce a class component only when an existing or required Error Boundary needs it - **Custom Hooks**: Standard pattern for logic reuse and dependency injection - **Component Hierarchy**: Follow the project's existing component architecture. Use Atoms > Molecules > Organisms > Templates > Pages only when the project adopts Atomic Design. - **Co-location**: Place tests, styles, and related files alongside components @@ -79,21 +66,8 @@ function isUser(value: unknown): value is User { - **Unidirectional Flow**: Data flows top-down via props - **Immutable Updates**: Use immutable patterns for state updates -```typescript -// Good: Immutable state update -setUsers(prev => [...prev, newUser]) - -// Bad: Mutable state update -users.push(newUser) -setUsers(users) -``` - **Function Design** - **0-2 parameters maximum**: Use object for 3+ parameters - ```typescript - // Object parameter - function createUser({ name, email, role }: CreateUserParams) {} - ``` **Props Design (Props-driven Approach)** - Props are the interface: Define all necessary information as props @@ -103,17 +77,8 @@ setUsers(users) **Environment Variables** - **Use the build tool's env accessor**: read client-side env through the bundler's exposed accessor, such as Vite `import.meta.env` or Next.js/CRA prefixed `process.env`. - **Only prefixed vars reach the client**: build tools expose only vars carrying their public prefix. Match the project's bundler, such as Vite `VITE_`, Next.js `NEXT_PUBLIC_`, or CRA `REACT_APP_`. -- Centrally manage environment variables through a typed configuration layer with defaults. - -```typescript -// Client-exposed env must carry the bundler's public prefix, or it is undefined in the browser. -// Vite: import.meta.env.VITE_API_URL -// Next.js: process.env.NEXT_PUBLIC_API_URL -const config = { - apiUrl: import.meta.env.VITE_API_URL || 'http://localhost:3000', - appName: import.meta.env.VITE_APP_NAME || 'My App' -} -``` +- Centrally manage environment variables through a typed configuration layer. +- Treat a missing required variable as a configuration error. Use a default only when requirements or a Design Doc explicitly define that fallback. **Security (Client-side Constraints)** - **CRITICAL**: All frontend code is public and visible in browser @@ -121,15 +86,6 @@ const config = { - Do not include `.env` files in Git - Do not include sensitive information in error messages -```typescript -// Bad: API key exposed in browser -// const apiKey = import.meta.env.VITE_API_KEY -// const response = await fetch(`https://api.example.com/data?key=${apiKey}`) - -// Good: Backend manages secrets, frontend accesses via proxy -const response = await fetch('/api/data') // Backend handles API key authentication -``` - **Dependency Injection** - **Custom Hooks for dependency injection**: Ensure testability and modularity @@ -155,39 +111,6 @@ const response = await fetch('/api/data') // Backend handles API key authenticat **Absolute Rule**: Handle every error explicitly with log output, recovery logic, or escalation appropriate to the failure mode. **Fail-Fast Principle**: Fail quickly on errors to prevent continued processing in invalid states -```typescript -// Bad: Unconditional fallback -catch (error) { - return defaultValue // Hides error -} - -// Good: Explicit failure -catch (error) { - logger.error('Processing failed', error) - throw error // Handle with Error Boundary or higher layer -} -``` - -**Result Type Pattern**: Express errors with types for explicit handling -```typescript -type Result = { ok: true; value: T } | { ok: false; error: E } - -function parseUser(data: unknown): Result { - if (!isValid(data)) return { ok: false, error: new ValidationError() } - return { ok: true, value: data as User } -} -``` - -**Custom Error Classes** -```typescript -export class AppError extends Error { - constructor(message: string, public readonly code: string, public readonly statusCode = 500) { - super(message) - this.name = this.constructor.name - } -} -// Purpose-specific: ValidationError(400), ApiError(502), NotFoundError(404) -``` **Layer-Specific Error Handling (React)** - Error Boundary: Catch React component errors, display fallback UI diff --git a/.agents/skills/documentation-criteria/references/design-template.md b/.agents/skills/documentation-criteria/references/design-template.md index 26a4b35..aeefe85 100644 --- a/.agents/skills/documentation-criteria/references/design-template.md +++ b/.agents/skills/documentation-criteria/references/design-template.md @@ -266,7 +266,7 @@ For serialized boundaries -- a value encoded on one side and parsed on the other ## Verification Strategy -Define correctness and how to prove it at design time. L1/L2/L3 from implementation-approach define task-level verification depth. +Define correctness and how to prove it at design time. Use task-level verification depth matching the exercised boundary: L1 for unit or local in-process behavior, L2 for interaction across a named component, persistence, process, or contract boundary, and L3 for a complete user, browser, process, or service journey. Use the minimal form only for low-risk changes (1-2 files, no external contract/integration/data-flow change) or self-evident internal refactors with identical observable inputs/outputs. ### Correctness Proof Method diff --git a/.agents/skills/implementation-approach/SKILL.md b/.agents/skills/implementation-approach/SKILL.md index ff18d4c..4fba943 100644 --- a/.agents/skills/implementation-approach/SKILL.md +++ b/.agents/skills/implementation-approach/SKILL.md @@ -38,7 +38,7 @@ Test the Direct MVP against every current requirement, verified constraint, obse ### Phase 4: Targeted Expansion -For each Failed Item, choose the sufficient candidate with the lowest lifecycle cost, treating maintenance-surface-bearing elements defined by coding-rules as cost factors and preferring fewer new elements only when candidates are otherwise equivalent. Record it as `Adopted Additions`; record `None` when no expansion is needed. An addition without a Failed Item is excluded. +For each Failed Item, choose the sufficient candidate with the lowest lifecycle cost. Treat persistent state, public-contract or cross-boundary fields and props, behavioral modes, flags or variants, reusable abstractions, extracted services, shared utilities, and component splits as maintenance-surface-bearing cost factors. Prefer fewer new elements only when candidates are otherwise sufficient and equivalent. Record the choice as `Adopted Additions`; record `None` when no expansion is needed. An addition without a Failed Item is excluded. ### Phase 5: Subtraction Check [MANDATORY] @@ -61,7 +61,7 @@ After the design converges, select its implementation slicing approach: #### Hybrid **Characteristics**: Combination of vertical and horizontal slices **Application Conditions**: Verified dependencies require foundation work before some, but not all, user-value slices -**Verification Method**: Verify at appropriate L1/L2/L3 levels according to each phase's goals +**Verification Method**: Use the task verification level required by each phase's proof obligation ### Phase 7: Decision Rationale Documentation @@ -69,13 +69,23 @@ After the design converges, select its implementation slicing approach: ## Verification Level Definitions -Priority for completion verification of each task: +Task verification levels describe the boundary exercised by the task's runnable check: -- **L1: Functional Operation Verification** - Operates as end-user feature (e.g., search executable) -- **L2: Test Operation Verification** - New tests added and passing -- **L3: Build Success Verification** - Code builds/runs without errors +- **L1: Unit or Local Verification** - Exercises one unit or an in-process behavior without crossing an integration boundary +- **L2: Integration Verification** - Exercises interaction across components, persistence, processes, or another named integration boundary +- **L3: End-to-End Verification** - Exercises the complete user, browser, process, or service journey required by the proof obligation -**Priority**: L1 > L2 > L3 in order of verifiability importance +Select the level that exercises the boundary named by the task's proof obligation. A broader level does not replace a required narrower check, and a narrower level does not prove a required integration or end-to-end boundary. + +### Completion Evidence Priority + +When judging whether implementation is complete, prefer evidence in this order: + +1. **Functional operation evidence** - The required end-user or externally observable behavior operates +2. **Test operation evidence** - The relevant tests execute and pass +3. **Build evidence** - The code builds or runs without errors + +Build success alone does not prove required behavior when functional or test evidence is applicable. ## Integration Point Definitions diff --git a/.agents/skills/integration-e2e-testing/SKILL.md b/.agents/skills/integration-e2e-testing/SKILL.md index 9cc1945..236b64f 100644 --- a/.agents/skills/integration-e2e-testing/SKILL.md +++ b/.agents/skills/integration-e2e-testing/SKILL.md @@ -41,6 +41,13 @@ description: "Integration and E2E test design principles, value-based selection, Value Score = (Business Value x User Frequency) + (Legal Requirement x 10) + Defect Detection ``` +Score each factor as follows: + +- **Business Value**: `0` for no material outcome impact, `5` for a material secondary outcome, and `10` for a critical or core outcome; interpolate `1-4` and `6-9` +- **User Frequency**: `0` below 1% of eligible journeys, `1-9` for the corresponding 10%-90% band, and `10` for virtually every eligible journey +- **Legal Requirement**: `1` when legally required, otherwise `0` +- **Defect Detection**: `0` when existing tests already prove the same failure mode, `5` for a material coverage gap, and `10` when this is the primary detector of a critical regression; interpolate `1-4` and `6-9` + Use `Value Score` for ranking candidates of the same test type. Handle E2E cost through budget limits and reserved-slot rules instead of cost-division scoring. ### E2E Lane Thresholds diff --git a/.agents/skills/subagents-orchestration-guide/SKILL.md b/.agents/skills/subagents-orchestration-guide/SKILL.md index 6dd40a5..d02aab8 100644 --- a/.agents/skills/subagents-orchestration-guide/SKILL.md +++ b/.agents/skills/subagents-orchestration-guide/SKILL.md @@ -143,23 +143,23 @@ Autonomous execution MUST stop and wait for user input at these points. ### Approval Status Vocabulary [MANDATORY] -All agents MUST use this vocabulary consistently: +These values standardize review and approval decisions. Review and approval agents MUST use this vocabulary consistently. Executor and quality-control operational statuses such as `completed`, `escalation_needed`, and `stub_detected` remain governed by their agent schemas and the Structured Response Specification. | Status | Scope | Meaning | Next Action | |--------|-------|---------|-------------| -| `approved` | All agents | All criteria met | Proceed to next phase | +| `approved` | Review/approval agents | All criteria met | Proceed to next phase | | `approved_with_conditions` | Document agents | Criteria met with minor open items | Proceed — carry conditions as input to next phase | | `approved_with_notes` | security-reviewer | Only hardening/policy findings | Proceed — include notes in completion report (no resolution required) | -| `needs_revision` | All agents | Significant issues found | Return to author agent for revision (max 2 iterations) | +| `needs_revision` | Review/approval agents | Significant issues found | Return to author agent for revision (max 2 iterations) | | `rejected` | Document agents | Fundamental problems | Halt workflow, escalate to user | | `blocked` | security-reviewer | Committed secrets or high-confidence exploitable risk | Halt workflow immediately, escalate to user (requires human intervention) | -| `skipped` | All agents | Preconditions not met for this step | Report reason, proceed | +| `skipped` | Review/approval agents whose schema permits skipping | Preconditions not met for this step | Report reason, proceed | Handling rules: - `approved_with_conditions`: append the listed conditions to the document's open-items section, carry them into the next phase, and resolve them before implementation - `approved_with_notes`: include the notes in the completion report for awareness -**ENFORCEMENT**: Using any status value outside this vocabulary is a VIOLATION. +**ENFORCEMENT**: Using any status value outside this vocabulary for a review or approval decision is a VIOLATION. ### WorkPlan Review State [MANDATORY] diff --git a/.agents/skills/task-analyzer/references/skills-index.yaml b/.agents/skills/task-analyzer/references/skills-index.yaml index c211823..48789b7 100644 --- a/.agents/skills/task-analyzer/references/skills-index.yaml +++ b/.agents/skills/task-analyzer/references/skills-index.yaml @@ -41,13 +41,12 @@ skills: - "Test-Driven Development - Kent Beck" - "Red-Green-Refactor Cycle - Kent Beck" - "AAA Pattern - Arrange-Act-Assert" - - "Test Pyramid - Mike Cohn" sections: - "Language-Specific References" - "Core Testing Philosophy" - "TDD Process [MANDATORY for all code changes]" - "Quality Requirements [MANDATORY]" - - "Test Types" + - "Test Level Selection" - "Test Design Principles" - "Test Independence" - "Mocking and Test Doubles" @@ -56,8 +55,6 @@ skills: - "What to Test" - "Test Quality Criteria [MANDATORY]" - "Verification Requirements [MANDATORY for VERIFY phase]" - - "Test Organization" - - "Performance Considerations" - "Common Anti-Patterns" - "Regression Testing" references: @@ -66,7 +63,7 @@ skills: ai-development-guide: skill: "ai-development-guide" tags: [anti-patterns, technical-judgment, debugging, quality-commands, rule-of-three, implementation, refactoring, code-reading, best-practices, fail-fast, error-handling, impact-analysis] - typical-use: "Technical decision criteria, anti-pattern detection, debugging techniques, quality check workflows, impact analysis procedures" + typical-use: "Technical decision criteria, anti-pattern detection, root-cause discipline, quality check workflows, impact analysis procedures" size: large key-references: - "Rule of Three - Martin Fowler" @@ -78,11 +75,9 @@ skills: - "Technical Anti-patterns (Red Flag Patterns) [MANDATORY]" - "Fail-Fast Fallback Design Principles" - "Rule of Three - Criteria for Code Duplication" - - "Common Failure Patterns and Avoidance Methods" - - "Debugging Techniques" + - "Pattern 5: Existing Code Investigation [MANDATORY]" - "Quality Assurance Mechanism Awareness" - "Quality Check Workflow [MANDATORY]" - - "Situations Requiring Technical Decisions" - "Implementation Completeness Assurance" - "Impact Analysis" references: diff --git a/.agents/skills/testing/SKILL.md b/.agents/skills/testing/SKILL.md index db26d8b..1abf63a 100644 --- a/.agents/skills/testing/SKILL.md +++ b/.agents/skills/testing/SKILL.md @@ -24,7 +24,7 @@ For language-specific testing patterns, also read: ### RED Phase **STEP 1**: Write test that defines expected behavior **STEP 2**: Run test -**STEP 3**: Confirm test FAILS (if it passes, the test is wrong) +**STEP 3**: Confirm the test fails for the targeted missing behavior or regression. If it passes before implementation, verify that it can fail for the targeted defect; then determine whether the behavior is already implemented or the test does not exercise the intended failure mode. ### GREEN Phase **STEP 1**: Write MINIMAL code to make test pass @@ -70,69 +70,18 @@ All tests MUST be: **ENFORCEMENT**: Tests failing ANY characteristic MUST be fixed immediately -## Test Types +## Test Level Selection -### Unit Tests - -**Purpose**: Test individual components in isolation - -**Characteristics**: -- Test single function, method, or class -- Fast execution (milliseconds) -- No external dependencies -- Mock external services -- Majority of your test suite - -### Integration Tests - -**Purpose**: Test interactions between components - -**Characteristics**: -- Test multiple components together -- May include database, file system, or APIs -- Slower than unit tests -- Verify contracts between modules -- Smaller portion of test suite - -### End-to-End (E2E) Tests - -**Purpose**: Test complete workflows from user perspective - -**Characteristics**: -- Test entire application stack -- Simulate real user interactions -- Slowest test type -- Fewest in number -- Highest confidence level - -### Test Pyramid - -Follow the test pyramid structure: -``` - /\ <- Few E2E Tests (High confidence, slow) - / \ - / \ <- Some Integration Tests (Medium confidence, medium speed) - / \ -/________\ <- Many Unit Tests (Fast, foundational) -``` +- **Unit/local**: Exercise one unit or in-process behavior and isolate external I/O +- **Integration**: Exercise the real component, persistence, process, or contract boundary named by the proof obligation +- **End-to-end**: Exercise the complete user, browser, process, or service journey named by the proof obligation +- Select the level that proves the required boundary; a broader test does not replace a required focused check, and a focused test does not prove an integration boundary ## Test Design Principles ### AAA Pattern (Arrange-Act-Assert) -Structure every test in three clear phases: - -``` -// Arrange: Setup test data and conditions -user = createTestUser() -validator = createValidator() - -// Act: Execute the code under test -result = validator.validate(user) - -// Assert: Verify expected outcome -assert(result.isValid == true) -``` +Structure every test in clear Arrange, Act, and Assert phases. ### One Assertion Per Concept @@ -167,12 +116,12 @@ Test names should clearly describe: ## Mocking and Test Doubles -### When to Use Mocks +### Boundary Selection -- **Mock external dependencies**: APIs, databases, file systems -- **Mock slow operations**: Network calls, heavy computations -- **Mock unpredictable behavior**: Random values, current time -- **Mock unavailable services**: Third-party services +- In unit/local tests, isolate external I/O such as APIs, databases, file systems, network calls, time, and randomness +- In integration and data-layer tests, keep the dependency or boundary named by the proof obligation real or production-like +- Isolate unavailable third-party services unless their contract payload or failure behavior is the boundary under test +- Follow the Design Doc `Test Boundaries` decision when one exists ### Mocking Principles [MANDATORY] @@ -181,14 +130,6 @@ Test names should clearly describe: - Verify mock expectations when relevant - Use adapters for external libraries/frameworks you do not control -### Types of Test Doubles - -- **Stub**: Returns predetermined values -- **Mock**: Verifies it was called correctly -- **Spy**: Records information about calls -- **Fake**: Simplified working implementation -- **Dummy**: Passed but never used - ## Data Layer Testing ### Mock Limitations for Data Access @@ -245,29 +186,16 @@ When a Design Doc includes `Test Boundaries`, follow it as the baseline for deci ## What to Test -### Focus on Behavior - -**Test observable behavior, not implementation**: - -- Good: Test that function returns expected output -- Good: Test that correct API endpoint is called -- Bad: Test that internal variable was set -- Bad: Test order of private method calls - -### Test Edge Cases - -Always test: -- **Boundary conditions**: Min/max values, empty collections -- **Error cases**: Invalid input, null values, missing data -- **Edge cases**: Special characters, extreme values -- **Happy path**: Normal, expected usage +- Test accepted behavior through the public or externally observable boundary +- Cover the happy path and the boundary, error, and regression cases required by the proof obligation +- Do not assert private state or private call order unless that internal contract is itself the named proof target ## Test Quality Criteria [MANDATORY] 1. **Literal expectations**: Use hardcoded literal values in assertions — expected value ≠ mock return value (implementation processes data) 2. **Result verification**: Assert return values and state, not call order 3. **Meaningful assertions**: Every test MUST have at least one assertion — a test without assertions provides zero value -4. **Mock external I/O only**: Mock DB/API/filesystem, use real internal utilities +4. **Boundary fidelity**: Mock only boundaries intentionally isolated at the selected test level; keep every dependency named as real by the proof obligation or Design Doc boundary decision 5. **Boundary coverage**: Include empty/zero/max/error cases with happy paths **ENFORCEMENT**: Tests violating ANY criterion MUST be rewritten @@ -287,38 +215,10 @@ Always test: - **Zero failing tests**: Fix immediately - **Zero skipped tests**: Delete or fix - **Zero flaky tests**: Make deterministic -- **Zero slow tests**: Optimize or split +- **Zero unreviewed slow tests**: When a test or suite exceeds the project's accepted feedback window, optimize it, split it, move it to the appropriate test level, or record the accepted exception in the governing task or Design Doc **ENFORCEMENT**: Cannot proceed with task completion if ANY quality check fails -## Test Organization - -### File Structure - -- **Mirror production structure**: Tests follow code organization -- **Clear naming conventions**: Follow project's test file patterns -- **Logical grouping**: Group related tests together -- **Separate test types**: Unit, integration, e2e in separate directories - -### Test Suite Organization - -``` -tests/ -├── unit/ # Fast, isolated unit tests -├── integration/ # Integration tests -├── e2e/ # End-to-end tests -├── fixtures/ # Test data and fixtures -└── helpers/ # Shared test utilities -``` - -## Performance Considerations - -### Test Speed - -- **Unit tests**: < 100ms each -- **Integration tests**: < 1s each -- **Full suite**: Should run frequently (< 10 minutes) - ## Common Anti-Patterns Detect and eliminate these patterns immediately: diff --git a/.agents/skills/testing/references/typescript.md b/.agents/skills/testing/references/typescript.md index 1844b92..1b3a84d 100644 --- a/.agents/skills/testing/references/typescript.md +++ b/.agents/skills/testing/references/typescript.md @@ -1,220 +1,59 @@ # TypeScript Testing Reference (Vitest + RTL + MSW + Playwright) -## Unit & Integration Tests (Vitest + React Testing Library + MSW) - -### Test Framework Setup - -```typescript -import { describe, it, expect, beforeEach, vi } from 'vitest' -import { render, screen } from '@testing-library/react' -import userEvent from '@testing-library/user-event' -``` +## Unit and Integration Tests ### Where to Concentrate Test Rigor -Test foundational, high-reuse units the hardest: shared components, custom hooks, utilities, and business rules reused across features carry the widest blast radius. Higher-composition surfaces such as pages and organisms lean more on integration or E2E coverage. Any numeric threshold is the project's CI, task file, work plan, or Design Doc config. - -### Test Types +Apply the strongest focused coverage to shared components, custom hooks, utilities, and business rules reused across features. Use integration or E2E coverage for higher-composition surfaces whose proof obligation crosses component or browser boundaries. Numeric thresholds come from the project's CI, task file, work plan, or Design Doc. -1. **Unit Tests (RTL)**: Verify individual components/functions, mock all external dependencies -2. **Integration Tests (RTL + MSW)**: Verify component coordination, mock APIs with MSW +### Test Level and Boundary Rules -### Directory Structure (Co-location) - -``` -src/ -└── components/ - └── Button/ - ├── Button.tsx - ├── Button.test.tsx # Co-located with component - └── index.ts -``` +- **Unit/local with RTL or Vitest**: Exercise one component, hook, function, or in-process behavior; isolate external I/O +- **Integration with RTL and MSW**: Exercise component coordination while controlling only the API boundary named for isolation +- Keep internal validators, formatters, and other project logic real unless the governing test-boundary decision says otherwise +- Keep mocks type-safe and limited to the behavior the test controls or observes ### Naming Conventions - Test files: `{ComponentName}.test.tsx` - Integration test files: `{FeatureName}.integration.test.tsx` -### Test Granularity: User-Observable Behavior Only - -**MUST Test**: Rendered output, user interactions, accessibility, error states -**MUST NOT Test**: Component internal state, implementation details, CSS class names - -```typescript -// Good: Test user-observable behavior -expect(screen.getByRole('button', { name: 'Submit' })).toBeInTheDocument() - -// Bad: Test implementation details -expect(component.state.count).toBe(0) -``` - -### RTL Test Example - -```typescript -import { describe, it, expect, vi } from 'vitest' -import { render, screen } from '@testing-library/react' -import userEvent from '@testing-library/user-event' -import { Button } from './Button' - -describe('Button', () => { - it('should call onClick when clicked', async () => { - const user = userEvent.setup() - const onClick = vi.fn() - render(