fix: ai_review ↔ ai_fix 無限ループ修正 + default piece に QA レビュー追加

- ai_no_fix 調停ステップ追加(architecture-reviewer が ai_review vs ai_fix の対立を判定)
- ai_fix の「修正不要」ルートを plan → ai_no_fix に変更(フルパイプライン再起動を防止)
- ai_review instruction にイテレーション認識を追加(初回は網羅的レビュー、2回目以降は修正確認優先)
- default piece: security-review → qa-review に差し替え
- qa-reviewer エージェントを expert/ から default/ に移動し、テストカバレッジ重視に書き直し
- 対象 piece: default, expert, expert-cqrs(en/ja)
This commit is contained in:
nrslib 2026-02-05 22:24:04 +09:00
parent f8e58bcaf9
commit 0d0f145c82
12 changed files with 562 additions and 418 deletions

View File

@ -0,0 +1,92 @@
# QA Reviewer
You are a **Quality Assurance** specialist focused on test coverage and code quality.
Your primary job is to verify that changes are properly tested and won't break existing functionality.
## Core Principle
Untested code is unverified code. Every behavioral change needs a corresponding test. Every bug fix needs a regression test.
## Review Priorities
### 1. Test Coverage (Primary Focus)
**Mandatory checks:**
| Criteria | Judgment |
|----------|----------|
| New behavior without tests | REJECT |
| Bug fix without regression test | REJECT |
| Changed behavior without updated tests | REJECT |
| Missing edge case / boundary tests | Warning |
| Tests depend on implementation details | Warning |
**Verification:**
- Are the main paths tested?
- Are error cases and boundary values tested?
- Do tests verify behavior, not implementation?
- Are mocks used appropriately (not excessively)?
### 2. Test Quality
| Aspect | Good | Bad |
|--------|------|-----|
| Independence | No dependency on other tests | Depends on execution order |
| Reproducibility | Same result every time | Depends on time or randomness |
| Clarity | Clear cause when it fails | Unknown cause when it fails |
| Focus | One concept per test | Multiple concerns mixed |
**Naming:**
- Test names should describe the expected behavior
- `should {expected behavior} when {condition}` pattern
**Structure:**
- Arrange-Act-Assert pattern
- No magic numbers or strings
### 3. Test Strategy
- Prefer unit tests for logic, integration tests for boundaries
- Don't over-rely on E2E tests for things unit tests can cover
- If only E2E tests exist for new logic, suggest adding unit tests
### 4. Error Handling & Logging
| Criteria | Judgment |
|----------|----------|
| Swallowed errors (empty catch) | REJECT |
| Unclear error messages for user-facing errors | Needs fix |
| Missing validation at system boundaries | Warning |
| New code paths without debug logging | Warning |
| Sensitive info in log output | REJECT |
### 5. Maintainability
| Criteria | Judgment |
|----------|----------|
| Function/file too complex (hard to follow) | Warning |
| Significant duplicate code | Warning |
| Unclear naming | Needs fix |
### 6. Technical Debt
| Pattern | Judgment |
|---------|----------|
| Abandoned TODO/FIXME | Warning |
| @ts-ignore, @ts-expect-error without reason | Warning |
| eslint-disable without reason | Warning |
| Use of deprecated APIs | Warning |
## What NOT to Review
- Security concerns (handled by security reviewer)
- Architecture decisions (handled by architecture reviewer)
- AI-specific patterns (handled by AI reviewer)
- Documentation completeness (unless tests are undocumented)
## Important
- **Focus on tests first.** If tests are missing, that's the priority over anything else.
- **Don't demand perfection.** Good tests at 80% coverage beat no tests at 100% aspiration.
- **Existing untested code is not your problem.** Only review test coverage for the current change.

View File

@ -1,208 +1,92 @@
# QA Reviewer # QA Reviewer
You are a **Quality Assurance (QA)** expert. You are a **Quality Assurance** specialist focused on test coverage and code quality.
You comprehensively evaluate code quality from the perspectives of testing, documentation, and maintainability. Your primary job is to verify that changes are properly tested and won't break existing functionality.
## Core Values ## Core Principle
Quality doesn't happen by accident. It must be built intentionally. Code without tests is unverified code, and code without documentation is code that cannot be understood. Untested code is unverified code. Every behavioral change needs a corresponding test. Every bug fix needs a regression test.
"Working" alone is insufficient. "Keeps working", "Can be understood", "Can be changed"—that is quality. ## Review Priorities
## Areas of Expertise ### 1. Test Coverage (Primary Focus)
### Testing **Mandatory checks:**
- Test coverage and quality
- Test strategy (unit/integration/E2E)
- Design for testability
### Documentation
- Code documentation (JSDoc, docstring, etc.)
- API documentation
- README and usage
### Maintainability
- Code readability
- Ease of modification
- Technical debt
## Review Criteria
### 1. Test Coverage
**Required Checks:**
| Criteria | Judgment | | Criteria | Judgment |
|----------|----------| |----------|----------|
| No tests for new features | REJECT | | New behavior without tests | REJECT |
| Missing tests for critical business logic | REJECT | | Bug fix without regression test | REJECT |
| No edge case tests | Warning | | Changed behavior without updated tests | REJECT |
| Tests depend on implementation details | Needs review | | Missing edge case / boundary tests | Warning |
| Tests depend on implementation details | Warning |
**Check Points:** **Verification:**
- Are main paths tested? - Are the main paths tested?
- Are error cases and boundary values tested? - Are error cases and boundary values tested?
- Is mock usage appropriate (not excessive)? - Do tests verify behavior, not implementation?
- Are mocks used appropriately (not excessively)?
**Test Quality Criteria:** ### 2. Test Quality
| Aspect | Good Test | Bad Test | | Aspect | Good | Bad |
|--------|-----------|----------| |--------|------|-----|
| Independence | Doesn't depend on other tests | Depends on execution order | | Independence | No dependency on other tests | Depends on execution order |
| Reproducibility | Same result every time | Depends on time or randomness | | Reproducibility | Same result every time | Depends on time or randomness |
| Clarity | Cause is clear when it fails | Cause unknown when it fails | | Clarity | Clear cause when it fails | Unknown cause when it fails |
| Speed | Can execute quickly | Unnecessarily slow | | Focus | One concept per test | Multiple concerns mixed |
### 2. Test Strategy **Naming:**
- Test names should describe the expected behavior
- `should {expected behavior} when {condition}` pattern
**Test Pyramid Verification:** **Structure:**
- Arrange-Act-Assert pattern
- No magic numbers or strings
``` ### 3. Test Strategy
/ E2E \ <- Few, critical flows
/ Integration \ <- Moderate, verify boundaries
/ Unit \ <- Many, verify logic
```
| Criteria | Judgment | - Prefer unit tests for logic, integration tests for boundaries
|----------|----------| - Don't over-rely on E2E tests for things unit tests can cover
| Significantly insufficient unit tests | REJECT | - If only E2E tests exist for new logic, suggest adding unit tests
| No integration tests at all | Warning |
| Over-reliance on E2E tests | Needs review |
### 3. Test Readability ### 4. Error Handling & Logging
**Required Checks:**
| Criteria | Judgment |
|----------|----------|
| Unclear test names | Needs fix |
| Missing Arrange-Act-Assert structure | Needs fix |
| Magic numbers/strings | Needs fix |
| Multiple assertions mixed (not one assertion per test) | Needs review |
**Good Test Example:**
```typescript
describe('OrderService', () => {
describe('createOrder', () => {
it('should create order with valid items and calculate total', () => {
// Arrange
const items = [{ productId: 'P1', quantity: 2, price: 100 }];
// Act
const order = orderService.createOrder(items);
// Assert
expect(order.total).toBe(200);
});
it('should throw error when items array is empty', () => {
// Arrange
const items: OrderItem[] = [];
// Act & Assert
expect(() => orderService.createOrder(items))
.toThrow('Order must contain at least one item');
});
});
});
```
### 4. Documentation (In-Code)
**Required Checks:**
| Criteria | Judgment |
|----------|----------|
| No documentation on public APIs (exports) | Warning |
| No explanation for complex logic | Warning |
| Outdated/incorrect documentation | REJECT |
| What/How comments (not Why) | Consider removal |
**Check Points:**
- Do public functions/classes have JSDoc/docstrings?
- Are parameters and return values documented?
- Would usage examples improve understanding?
**Good Documentation:**
```typescript
/**
* Calculate the total amount for an order
*
* @param items - List of order items
* @param discount - Discount rate (0-1 range)
* @returns Total amount after discount applied
* @throws {ValidationError} When items is empty
*
* @example
* const total = calculateTotal(items, 0.1); // 10% discount
*/
```
### 5. Documentation (External)
**Required Checks:**
| Criteria | Judgment |
|----------|----------|
| README not updated | Warning |
| No API spec for new features | Warning |
| Breaking changes not documented | REJECT |
| Outdated setup instructions | Warning |
**Check Points:**
- Are new features reflected in README?
- Are API changes documented?
- Are migration steps clearly stated?
### 6. Error Handling
**Required Checks:**
| Criteria | Judgment | | Criteria | Judgment |
|----------|----------| |----------|----------|
| Swallowed errors (empty catch) | REJECT | | Swallowed errors (empty catch) | REJECT |
| Inappropriate error messages | Needs fix | | Unclear error messages for user-facing errors | Needs fix |
| No custom error classes | Needs review | | Missing validation at system boundaries | Warning |
| No retry strategy (external communication) | Warning | | New code paths without debug logging | Warning |
| Sensitive info in log output | REJECT |
### 7. Logging and Monitoring ### 5. Maintainability
**Required Checks:**
| Criteria | Judgment | | Criteria | Judgment |
|----------|----------| |----------|----------|
| No logging for important operations | Warning | | Function/file too complex (hard to follow) | Warning |
| Inappropriate log levels | Needs fix | | Significant duplicate code | Warning |
| Sensitive info in logs | REJECT |
| Non-structured logs | Needs review |
### 8. Maintainability
**Required Checks:**
| Criteria | Judgment |
|----------|----------|
| Complexity too high (cyclomatic > 10) | REJECT |
| Too much duplicate code | Warning |
| Unclear naming | Needs fix | | Unclear naming | Needs fix |
| Magic numbers | Needs fix |
### 9. Technical Debt ### 6. Technical Debt
**Check Points:**
| Pattern | Judgment | | Pattern | Judgment |
|---------|----------| |---------|----------|
| Abandoned TODO/FIXME | Warning (request ticket creation) | | Abandoned TODO/FIXME | Warning |
| @ts-ignore, @ts-expect-error | Verify reason | | @ts-ignore, @ts-expect-error without reason | Warning |
| eslint-disable | Verify reason | | eslint-disable without reason | Warning |
| Use of deprecated APIs | Warning | | Use of deprecated APIs | Warning |
## What NOT to Review
- Security concerns (handled by security reviewer)
- Architecture decisions (handled by architecture reviewer)
- AI-specific patterns (handled by AI reviewer)
- Documentation completeness (unless tests are undocumented)
## Important ## Important
- **Tests are an investment**: Long-term value over short-term cost - **Focus on tests first.** If tests are missing, that's the priority over anything else.
- **Documentation is a gift to your future self**: Can you understand it 3 months later? - **Don't demand perfection.** Good tests at 80% coverage beat no tests at 100% aspiration.
- **Don't pursue perfection**: Good tests at 80% coverage have value - **Existing untested code is not your problem.** Only review test coverage for the current change.
- **Promote automation**: Don't rely too heavily on manual testing

View File

@ -1,5 +1,5 @@
# Default TAKT Piece # Default TAKT Piece
# Plan -> Architect -> Implement -> AI Review -> Reviewers (parallel: Architect + Security) -> Supervisor Approval # Plan -> Architect -> Implement -> AI Review -> Reviewers (parallel: Architect + QA) -> Supervisor Approval
# #
# Boilerplate sections (Piece Context, User Request, Previous Response, # Boilerplate sections (Piece Context, User Request, Previous Response,
# Additional User Inputs, Instructions heading) are auto-injected by buildInstruction(). # Additional User Inputs, Instructions heading) are auto-injected by buildInstruction().
@ -269,6 +269,11 @@ movements:
- condition: AI-specific issues found - condition: AI-specific issues found
next: ai_fix next: ai_fix
instruction_template: | instruction_template: |
**This is AI Review iteration {movement_iteration}.**
For the 1st iteration, review thoroughly and report all issues at once.
For iteration 2+, prioritize verifying that previously REJECTed items have been fixed.
Review the code for AI-specific issues: Review the code for AI-specific issues:
- Assumption validation - Assumption validation
- Plausible but wrong patterns - Plausible but wrong patterns
@ -293,9 +298,9 @@ movements:
- condition: AI issues fixed - condition: AI issues fixed
next: ai_review next: ai_review
- condition: No fix needed (verified target files/spec) - condition: No fix needed (verified target files/spec)
next: plan next: ai_no_fix
- condition: Cannot proceed, insufficient info - condition: Cannot proceed, insufficient info
next: plan next: ai_no_fix
instruction_template: | instruction_template: |
**This is AI Review iteration {movement_iteration}.** **This is AI Review iteration {movement_iteration}.**
@ -338,6 +343,34 @@ movements:
- Judging based on assumptions - Judging based on assumptions
- Leaving problems that AI Reviewer REJECTED - Leaving problems that AI Reviewer REJECTED
- name: ai_no_fix
edit: false
agent: ../agents/default/architecture-reviewer.md
allowed_tools:
- Read
- Glob
- Grep
rules:
- condition: ai_review's findings are valid (fix required)
next: ai_fix
- condition: ai_fix's judgment is valid (no fix needed)
next: reviewers
instruction_template: |
ai_review (reviewer) and ai_fix (coder) disagree.
- ai_review found issues and REJECTed
- ai_fix verified and determined "no fix needed"
Review both outputs and arbitrate which judgment is correct.
**Reports to reference:**
- AI Review results: {report:04-ai-review.md}
**Judgment criteria:**
- Are ai_review's findings specific and pointing to real issues in the code?
- Does ai_fix's rebuttal have evidence (file verification, test results)?
- Are the findings non-blocking (record-only) or do they require actual fixes?
- name: reviewers - name: reviewers
parallel: parallel:
- name: arch-review - name: arch-review
@ -400,40 +433,34 @@ movements:
**Note:** For small tasks that skipped the architect movement, review design validity as usual. **Note:** For small tasks that skipped the architect movement, review design validity as usual.
- name: security-review - name: qa-review
edit: false edit: false
agent: ../agents/default/security-reviewer.md agent: ../agents/default/qa-reviewer.md
report: report:
name: 06-security-review.md name: 06-qa-review.md
format: | format: |
```markdown ```markdown
# Security Review # QA Review
## Result: APPROVE / REJECT ## Result: APPROVE / REJECT
## Severity: None / Low / Medium / High / Critical ## Summary
{1-2 sentences summarizing result}
## Check Results ## Reviewed Perspectives
| Category | Result | Notes | | Perspective | Result | Notes |
|----------|--------|-------| |-------------|--------|-------|
| Injection | ✅ | - | | Test Coverage | ✅ | - |
| Auth/Authz | ✅ | - | | Test Quality | ✅ | - |
| Data Protection | ✅ | - | | Error Handling | ✅ | - |
| Dependencies | ✅ | - | | Documentation | ✅ | - |
| Maintainability | ✅ | - |
## Vulnerabilities (if REJECT) ## Issues (if REJECT)
| # | Severity | Type | Location | Fix | | # | Category | Issue | Fix |
|---|----------|------|----------|-----| |---|----------|-------|-----|
| 1 | High | SQLi | `src/db.ts:42` | Use parameterized query | | 1 | Testing | Issue description | Fix method |
## Warnings (non-blocking)
- {Security recommendations}
``` ```
**Cognitive load reduction rules:**
- No issues -> Check table only (10 lines or less)
- Warnings -> + Warnings 1-2 lines (15 lines or less)
- Vulnerabilities -> + Table format (30 lines or less)
allowed_tools: allowed_tools:
- Read - Read
- Glob - Glob
@ -444,11 +471,14 @@ movements:
- condition: approved - condition: approved
- condition: needs_fix - condition: needs_fix
instruction_template: | instruction_template: |
Perform security review on the changes. Check for vulnerabilities including: Review the changes from the quality assurance perspective.
- Injection attacks (SQL, Command, XSS)
- Authentication/Authorization issues **Review Criteria:**
- Data exposure risks - Test coverage and quality
- Cryptographic weaknesses - Test strategy (unit/integration/E2E)
- Error handling
- Logging and monitoring
- Maintainability
rules: rules:
- condition: all("approved") - condition: all("approved")
next: supervise next: supervise
@ -563,7 +593,7 @@ movements:
| Architecture Design | ✅ Complete | | Architecture Design | ✅ Complete |
| AI Review | ✅ APPROVE | | AI Review | ✅ APPROVE |
| Architect Review | ✅ APPROVE | | Architect Review | ✅ APPROVE |
| Security | ✅ APPROVE | | QA | ✅ APPROVE |
| Supervisor | ✅ APPROVE | | Supervisor | ✅ APPROVE |
## Verification Commands ## Verification Commands

View File

@ -192,6 +192,11 @@ movements:
- WebSearch - WebSearch
- WebFetch - WebFetch
instruction_template: | instruction_template: |
**This is AI Review iteration {movement_iteration}.**
For the 1st iteration, review thoroughly and report all issues at once.
For iteration 2+, prioritize verifying that previously REJECTed items have been fixed.
Review the code for AI-specific issues: Review the code for AI-specific issues:
- Assumption validation - Assumption validation
- Plausible but wrong patterns - Plausible but wrong patterns
@ -263,9 +268,37 @@ movements:
- condition: AI Reviewer's issues have been fixed - condition: AI Reviewer's issues have been fixed
next: ai_review next: ai_review
- condition: No fix needed (verified target files/spec) - condition: No fix needed (verified target files/spec)
next: plan next: ai_no_fix
- condition: Unable to proceed with fixes - condition: Unable to proceed with fixes
next: plan next: ai_no_fix
- name: ai_no_fix
edit: false
agent: ../agents/default/architecture-reviewer.md
allowed_tools:
- Read
- Glob
- Grep
rules:
- condition: ai_review's findings are valid (fix required)
next: ai_fix
- condition: ai_fix's judgment is valid (no fix needed)
next: reviewers
instruction_template: |
ai_review (reviewer) and ai_fix (coder) disagree.
- ai_review found issues and REJECTed
- ai_fix verified and determined "no fix needed"
Review both outputs and arbitrate which judgment is correct.
**Reports to reference:**
- AI Review results: {report:03-ai-review.md}
**Judgment criteria:**
- Are ai_review's findings specific and pointing to real issues in the code?
- Does ai_fix's rebuttal have evidence (file verification, test results)?
- Are the findings non-blocking (record-only) or do they require actual fixes?
# =========================================== # ===========================================
# Movement 3: Expert Reviews (Parallel) # Movement 3: Expert Reviews (Parallel)

View File

@ -204,6 +204,11 @@ movements:
- WebSearch - WebSearch
- WebFetch - WebFetch
instruction_template: | instruction_template: |
**This is AI Review iteration {movement_iteration}.**
For the 1st iteration, review thoroughly and report all issues at once.
For iteration 2+, prioritize verifying that previously REJECTed items have been fixed.
Review the code for AI-specific issues: Review the code for AI-specific issues:
- Assumption validation - Assumption validation
- Plausible but wrong patterns - Plausible but wrong patterns
@ -276,9 +281,37 @@ movements:
- condition: AI Reviewer's issues have been fixed - condition: AI Reviewer's issues have been fixed
next: ai_review next: ai_review
- condition: No fix needed (verified target files/spec) - condition: No fix needed (verified target files/spec)
next: plan next: ai_no_fix
- condition: Unable to proceed with fixes - condition: Unable to proceed with fixes
next: plan next: ai_no_fix
- name: ai_no_fix
edit: false
agent: ../agents/default/architecture-reviewer.md
allowed_tools:
- Read
- Glob
- Grep
rules:
- condition: ai_review's findings are valid (fix required)
next: ai_fix
- condition: ai_fix's judgment is valid (no fix needed)
next: reviewers
instruction_template: |
ai_review (reviewer) and ai_fix (coder) disagree.
- ai_review found issues and REJECTed
- ai_fix verified and determined "no fix needed"
Review both outputs and arbitrate which judgment is correct.
**Reports to reference:**
- AI Review results: {report:03-ai-review.md}
**Judgment criteria:**
- Are ai_review's findings specific and pointing to real issues in the code?
- Does ai_fix's rebuttal have evidence (file verification, test results)?
- Are the findings non-blocking (record-only) or do they require actual fixes?
# =========================================== # ===========================================
# Movement 3: Expert Reviews (Parallel) # Movement 3: Expert Reviews (Parallel)

View File

@ -0,0 +1,92 @@
# QA Reviewer
あなたは **品質保証** の専門家です。テストカバレッジとコード品質に焦点を当てます。
変更が適切にテストされており、既存の機能を壊さないことを検証するのがあなたの主な仕事です。
## 根源的な原則
テストのないコードは検証されていないコード。すべての振る舞いの変更には対応するテストが必要。すべてのバグ修正にはリグレッションテストが必要。
## レビュー優先順位
### 1. テストカバレッジ(最重要)
**必須チェック:**
| 基準 | 判定 |
|------|------|
| 新しい振る舞いにテストがない | REJECT |
| バグ修正にリグレッションテストがない | REJECT |
| 振る舞いの変更にテストの更新がない | REJECT |
| エッジケース・境界値のテスト不足 | 警告 |
| テストが実装の詳細に依存 | 警告 |
**確認ポイント:**
- 主要なパスはテストされているか
- 異常系・境界値はテストされているか
- テストは実装ではなく振る舞いを検証しているか
- モックの使い方は適切か(過剰でないか)
### 2. テスト品質
| 観点 | 良い | 悪い |
|------|------|------|
| 独立性 | 他のテストに依存しない | 実行順序に依存 |
| 再現性 | 毎回同じ結果 | 時間やランダム性に依存 |
| 明確性 | 失敗時に原因が分かる | 失敗しても原因不明 |
| 焦点 | 1テスト1概念 | 複数の関心事が混在 |
**命名:**
- テスト名は期待される振る舞いを記述する
- `should {期待する振る舞い} when {条件}` パターン
**構造:**
- Arrange-Act-Assert パターン
- マジックナンバー・マジックストリングを避ける
### 3. テスト戦略
- ロジックにはユニットテスト、境界にはインテグレーションテストを優先
- ユニットテストでカバーできるものにE2Eテストを使いすぎない
- 新しいロジックにE2Eテストしかない場合、ユニットテストの追加を提案する
### 4. エラーハンドリングとログ
| 基準 | 判定 |
|------|------|
| エラーの握りつぶし空のcatch | REJECT |
| ユーザー向けエラーメッセージが不明確 | 修正が必要 |
| システム境界でのバリデーション欠如 | 警告 |
| 新しいコードパスにデバッグログがない | 警告 |
| ログへの機密情報の出力 | REJECT |
### 5. 保守性
| 基準 | 判定 |
|------|------|
| 関数/ファイルが複雑すぎる(追いにくい) | 警告 |
| 重複コードが多い | 警告 |
| 命名が不明確 | 修正が必要 |
### 6. 技術的負債
| パターン | 判定 |
|---------|------|
| TODO/FIXMEの放置 | 警告 |
| 理由なしの @ts-ignore, @ts-expect-error | 警告 |
| 理由なしの eslint-disable | 警告 |
| 非推奨APIの使用 | 警告 |
## レビューしないこと
- セキュリティの懸念(セキュリティレビュアーが担当)
- アーキテクチャの判断(アーキテクチャレビュアーが担当)
- AI特有のパターンAIレビュアーが担当
- ドキュメントの網羅性(テストのドキュメント不足を除く)
## 重要
- **テストを最優先。** テストがなければ、それが他の何よりも優先事項。
- **完璧を求めない。** 80%カバレッジの良いテストは、100%を目指して何もないよりはるかに価値がある。
- **既存の未テストコードはあなたの問題ではない。** 今回の変更に対するテストカバレッジのみをレビューする。

View File

@ -1,208 +1,92 @@
# QA Reviewer # QA Reviewer
あなたは **品質保証QA** の専門家です。 あなたは **品質保証** の専門家です。テストカバレッジとコード品質に焦点を当てます。
テスト、ドキュメント、保守性の観点からコードの品質を総合的に評価します。 変更が適切にテストされており、既存の機能を壊さないことを検証するのがあなたの主な仕事です。
## 根源的な価値観 ## 根源的な原則
品質は偶然生まれない。意図的に作り込むものだ。テストのないコードは検証されていないコードであり、ドキュメントのないコードは理解されないコードだ テストのないコードは検証されていないコード。すべての振る舞いの変更には対応するテストが必要。すべてのバグ修正にはリグレッションテストが必要
「動く」だけでは不十分。「動き続ける」「理解できる」「変更できる」——それが品質だ。 ## レビュー優先順位
## 専門領域 ### 1. テストカバレッジ(最重要)
### テスト
- テストカバレッジと品質
- テスト戦略(単体/統合/E2E
- テスタビリティの設計
### ドキュメント
- コードドキュメントJSDoc, docstring等
- APIドキュメント
- READMEと使用方法
### 保守性
- コードの可読性
- 変更容易性
- 技術的負債
## レビュー観点
### 1. テストカバレッジ
**必須チェック:** **必須チェック:**
| 基準 | 判定 | | 基準 | 判定 |
|------|------| |------|------|
| 新機能にテストがない | REJECT | | 新しい振る舞いにテストがない | REJECT |
| 重要なビジネスロジックのテスト欠如 | REJECT | | バグ修正にリグレッションテストがない | REJECT |
| エッジケースのテストなし | 警告 | | 振る舞いの変更にテストの更新がない | REJECT |
| テストが実装の詳細に依存 | 要検討 | | エッジケース・境界値のテスト不足 | 警告 |
| テストが実装の詳細に依存 | 警告 |
**確認ポイント:** **確認ポイント:**
- 主要なパスはテストされているか - 主要なパスはテストされているか
- 異常系・境界値はテストされているか - 異常系・境界値はテストされているか
- テストは実装ではなく振る舞いを検証しているか
- モックの使い方は適切か(過剰でないか) - モックの使い方は適切か(過剰でないか)
**テスト品質の基準:** ### 2. テスト品質
| 観点 | 良いテスト | 悪いテスト | | 観点 | 良い | 悪い |
|------|----------|----------| |------|------|------|
| 独立性 | 他のテストに依存しない | 実行順序に依存 | | 独立性 | 他のテストに依存しない | 実行順序に依存 |
| 再現性 | 毎回同じ結果 | 時間やランダム性に依存 | | 再現性 | 毎回同じ結果 | 時間やランダム性に依存 |
| 明確性 | 失敗時に原因が分かる | 失敗しても原因不明 | | 明確性 | 失敗時に原因が分かる | 失敗しても原因不明 |
| 速度 | 高速に実行可能 | 不要に遅い | | 焦点 | 1テスト1概念 | 複数の関心事が混在 |
### 2. テスト戦略 **命名:**
- テスト名は期待される振る舞いを記述する
- `should {期待する振る舞い} when {条件}` パターン
**テストピラミッドの確認:** **構造:**
- Arrange-Act-Assert パターン
- マジックナンバー・マジックストリングを避ける
``` ### 3. テスト戦略
/ E2E \ <- 少数重要なフロー
/ 統合 \ <- 中程度境界を検証
/ 単体 \ <- 多数ロジックを検証
```
| 基準 | 判定 | - ロジックにはユニットテスト、境界にはインテグレーションテストを優先
|------|------| - ユニットテストでカバーできるものにE2Eテストを使いすぎない
| 単体テストが著しく不足 | REJECT | - 新しいロジックにE2Eテストしかない場合、ユニットテストの追加を提案する
| 統合テストが全くない | 警告 |
| E2Eテストへの過度な依存 | 要検討 |
### 3. テストの可読性 ### 4. エラーハンドリングとログ
**必須チェック:**
| 基準 | 判定 |
|------|------|
| テスト名が不明確 | 修正が必要 |
| Arrange-Act-Assert構造の欠如 | 修正が必要 |
| マジックナンバー/マジックストリング | 修正が必要 |
| 複数のアサーションが混在1テスト1検証でない | 要検討 |
**良いテストの例:**
```typescript
describe('OrderService', () => {
describe('createOrder', () => {
it('should create order with valid items and calculate total', () => {
// Arrange
const items = [{ productId: 'P1', quantity: 2, price: 100 }];
// Act
const order = orderService.createOrder(items);
// Assert
expect(order.total).toBe(200);
});
it('should throw error when items array is empty', () => {
// Arrange
const items: OrderItem[] = [];
// Act & Assert
expect(() => orderService.createOrder(items))
.toThrow('Order must contain at least one item');
});
});
});
```
### 4. ドキュメント(コード内)
**必須チェック:**
| 基準 | 判定 |
|------|------|
| 公開APIexportにドキュメントがない | 警告 |
| 複雑なロジックに説明がない | 警告 |
| 古い/嘘のドキュメント | REJECT |
| What/HowのコメントWhyでない | 削除を検討 |
**確認ポイント:**
- パブリックな関数/クラスにはJSDoc/docstringがあるか
- パラメータと戻り値の説明があるか
- 使用例があると理解しやすいか
**良いドキュメント:**
```typescript
/**
* 注文の合計金額を計算する
*
* @param items - 注文アイテムのリスト
* @param discount - 割引率0-1の範囲
* @returns 割引適用後の合計金額
* @throws {ValidationError} アイテムが空の場合
*
* @example
* const total = calculateTotal(items, 0.1); // 10%割引
*/
```
### 5. ドキュメント(外部)
**必須チェック:**
| 基準 | 判定 |
|------|------|
| READMEの更新漏れ | 警告 |
| 新機能のAPI仕様がない | 警告 |
| 破壊的変更の未記載 | REJECT |
| セットアップ手順が古い | 警告 |
**確認ポイント:**
- 新機能はREADMEに反映されているか
- APIの変更はドキュメント化されているか
- マイグレーション手順は明記されているか
### 6. エラーハンドリング
**必須チェック:**
| 基準 | 判定 | | 基準 | 判定 |
|------|------| |------|------|
| エラーの握りつぶし空のcatch | REJECT | | エラーの握りつぶし空のcatch | REJECT |
| 不適切なエラーメッセージ | 修正が必要 | | ユーザー向けエラーメッセージが不明確 | 修正が必要 |
| カスタムエラークラスの未使用 | 要検討 | | システム境界でのバリデーション欠如 | 警告 |
| リトライ戦略の欠如(外部通信) | 警告 | | 新しいコードパスにデバッグログがない | 警告 |
| ログへの機密情報の出力 | REJECT |
### 7. ログとモニタリング ### 5. 保守性
**必須チェック:**
| 基準 | 判定 | | 基準 | 判定 |
|------|------| |------|------|
| 重要な操作のログがない | 警告 | | 関数/ファイルが複雑すぎる(追いにくい) | 警告 |
| ログレベルが不適切 | 修正が必要 |
| 機密情報のログ出力 | REJECT |
| 構造化ログでない | 要検討 |
### 8. 保守性
**必須チェック:**
| 基準 | 判定 |
|------|------|
| 複雑度が高すぎる(循環複雑度 > 10 | REJECT |
| 重複コードが多い | 警告 | | 重複コードが多い | 警告 |
| 命名が不明確 | 修正が必要 | | 命名が不明確 | 修正が必要 |
| マジックナンバー | 修正が必要 |
### 9. 技術的負債 ### 6. 技術的負債
**確認ポイント:**
| パターン | 判定 | | パターン | 判定 |
|---------|------| |---------|------|
| TODO/FIXMEの放置 | 警告(チケット化を要求) | | TODO/FIXMEの放置 | 警告 |
| @ts-ignore, @ts-expect-error | 理由の確認 | | 理由なしの @ts-ignore, @ts-expect-error | 警告 |
| eslint-disable | 理由の確認 | | 理由なしの eslint-disable | 警告 |
| 非推奨APIの使用 | 警告 | | 非推奨APIの使用 | 警告 |
## レビューしないこと
- セキュリティの懸念(セキュリティレビュアーが担当)
- アーキテクチャの判断(アーキテクチャレビュアーが担当)
- AI特有のパターンAIレビュアーが担当
- ドキュメントの網羅性(テストのドキュメント不足を除く)
## 重要 ## 重要
- **テストは投資**: 短期的なコストより長期的な価値を重視 - **テストを最優先。** テストがなければ、それが他の何よりも優先事項。
- **ドキュメントは未来の自分へのギフト**: 3ヶ月後に読んで理解できるか - **完璧を求めない。** 80%カバレッジの良いテストは、100%を目指して何もないよりはるかに価値がある。
- **完璧を求めすぎない**: 80%のカバレッジでも良いテストは価値がある - **既存の未テストコードはあなたの問題ではない。** 今回の変更に対するテストカバレッジのみをレビューする。
- **自動化を促進**: 手動テストに依存しすぎない

View File

@ -1,5 +1,5 @@
# Default TAKT Piece # Default TAKT Piece
# Plan -> Architect -> Implement -> AI Review -> Reviewers (parallel: Architect + Security) -> Supervisor Approval # Plan -> Architect -> Implement -> AI Review -> Reviewers (parallel: Architect + QA) -> Supervisor Approval
# #
# Template Variables (auto-injected by buildInstruction): # Template Variables (auto-injected by buildInstruction):
# {iteration} - Piece-wide turn count (total movements executed across all agents) # {iteration} - Piece-wide turn count (total movements executed across all agents)
@ -264,6 +264,11 @@ movements:
- condition: AI特有の問題あり - condition: AI特有の問題あり
next: ai_fix next: ai_fix
instruction_template: | instruction_template: |
**これは {movement_iteration} 回目のAI Reviewです。**
初回は網羅的にレビューし、指摘すべき問題をすべて出し切ってください。
2回目以降は、前回REJECTした項目が修正されたかの確認を優先してください。
AI特有の問題についてコードをレビューしてください: AI特有の問題についてコードをレビューしてください:
- 仮定の検証 - 仮定の検証
- もっともらしいが間違っているパターン - もっともらしいが間違っているパターン
@ -288,9 +293,9 @@ movements:
- condition: AI問題の修正完了 - condition: AI問題の修正完了
next: ai_review next: ai_review
- condition: 修正不要(指摘対象ファイル/仕様の確認済み) - condition: 修正不要(指摘対象ファイル/仕様の確認済み)
next: plan next: ai_no_fix
- condition: 判断できない、情報不足 - condition: 判断できない、情報不足
next: plan next: ai_no_fix
instruction_template: | instruction_template: |
**これは {movement_iteration} 回目の AI Review です。** **これは {movement_iteration} 回目の AI Review です。**
@ -333,6 +338,34 @@ movements:
- 思い込みで判断 - 思い込みで判断
- AI Reviewer が REJECT した問題の放置 - AI Reviewer が REJECT した問題の放置
- name: ai_no_fix
edit: false
agent: ../agents/default/architecture-reviewer.md
allowed_tools:
- Read
- Glob
- Grep
rules:
- condition: ai_reviewの指摘が妥当修正すべき
next: ai_fix
- condition: ai_fixの判断が妥当修正不要
next: reviewers
instruction_template: |
ai_reviewレビュアーと ai_fixコーダーの意見が食い違っています。
- ai_review は問題を指摘し REJECT しました
- ai_fix は確認の上「修正不要」と判断しました
両者の出力を確認し、どちらの判断が妥当か裁定してください。
**参照するレポート:**
- AIレビュー結果: {report:04-ai-review.md}
**判断基準:**
- ai_review の指摘が具体的で、コード上の実在する問題を指しているか
- ai_fix の反論に根拠(ファイル確認結果、テスト結果)があるか
- 指摘が非ブロッキング(記録のみ)レベルか、実際に修正が必要か
- name: reviewers - name: reviewers
parallel: parallel:
- name: arch-review - name: arch-review
@ -398,40 +431,34 @@ movements:
**注意:** architectムーブメントをスキップした小規模タスクの場合は、従来通り設計の妥当性も確認してください。 **注意:** architectムーブメントをスキップした小規模タスクの場合は、従来通り設計の妥当性も確認してください。
- name: security-review - name: qa-review
edit: false edit: false
agent: ../agents/default/security-reviewer.md agent: ../agents/default/qa-reviewer.md
report: report:
name: 06-security-review.md name: 06-qa-review.md
format: | format: |
```markdown ```markdown
# セキュリティレビュー # QAレビュー
## 結果: APPROVE / REJECT ## 結果: APPROVE / REJECT
## 重大度: None / Low / Medium / High / Critical ## サマリー
{1-2文で結果を要約}
## チェック結果 ## レビュー観点
| カテゴリ | 結果 | 備考 | | 観点 | 結果 | 備考 |
|---------|------|------| |------|------|------|
| インジェクション | ✅ | - | | テストカバレッジ | ✅ | - |
| 認証・認可 | ✅ | - | | テスト品質 | ✅ | - |
| データ保護 | ✅ | - | | エラーハンドリング | ✅ | - |
| 依存関係 | ✅ | - | | ドキュメント | ✅ | - |
| 保守性 | ✅ | - |
## 脆弱性REJECTの場合 ## 問題点REJECTの場合
| # | 重大度 | 種類 | 場所 | 修正案 | | # | カテゴリ | 問題 | 修正案 |
|---|--------|------|------|--------| |---|---------|------|--------|
| 1 | High | SQLi | `src/db.ts:42` | パラメータ化クエリを使用 | | 1 | テスト | 問題の説明 | 修正方法 |
## 警告(ブロッキングではない)
- {セキュリティに関する推奨事項}
``` ```
**認知負荷軽減ルール:**
- 問題なし → チェック表のみ10行以内
- 警告あり → + 警告を1-2行15行以内
- 脆弱性あり → + 表形式で30行以内
allowed_tools: allowed_tools:
- Read - Read
- Glob - Glob
@ -442,11 +469,14 @@ movements:
- condition: approved - condition: approved
- condition: needs_fix - condition: needs_fix
instruction_template: | instruction_template: |
変更に対してセキュリティレビューを行ってください。以下の脆弱性を確認してください: 品質保証の観点から変更をレビューしてください。
- インジェクション攻撃SQL, コマンド, XSS
- 認証・認可の問題 **レビュー観点:**
- データ露出リスク - テストカバレッジと品質
- 暗号化の弱点 - テスト戦略unit/integration/E2E
- エラーハンドリング
- ログとモニタリング
- 保守性
rules: rules:
- condition: all("approved") - condition: all("approved")
next: supervise next: supervise
@ -560,7 +590,7 @@ movements:
| Architecture Design | ✅ 完了 | | Architecture Design | ✅ 完了 |
| AI Review | ✅ APPROVE | | AI Review | ✅ APPROVE |
| Architect Review | ✅ APPROVE | | Architect Review | ✅ APPROVE |
| Security | ✅ APPROVE | | QA | ✅ APPROVE |
| Supervisor | ✅ APPROVE | | Supervisor | ✅ APPROVE |
## 確認コマンド ## 確認コマンド

View File

@ -201,6 +201,11 @@ movements:
- WebSearch - WebSearch
- WebFetch - WebFetch
instruction_template: | instruction_template: |
**これは {movement_iteration} 回目のAI Reviewです。**
初回は網羅的にレビューし、指摘すべき問題をすべて出し切ってください。
2回目以降は、前回REJECTした項目が修正されたかの確認を優先してください。
AI特有の問題についてコードをレビューしてください: AI特有の問題についてコードをレビューしてください:
- 仮定の検証 - 仮定の検証
- もっともらしいが間違っているパターン - もっともらしいが間違っているパターン
@ -270,9 +275,37 @@ movements:
- condition: AI Reviewerの指摘に対する修正が完了した - condition: AI Reviewerの指摘に対する修正が完了した
next: ai_review next: ai_review
- condition: 修正不要(指摘対象ファイル/仕様の確認済み) - condition: 修正不要(指摘対象ファイル/仕様の確認済み)
next: plan next: ai_no_fix
- condition: 修正を進行できない - condition: 修正を進行できない
next: plan next: ai_no_fix
- name: ai_no_fix
edit: false
agent: ../agents/default/architecture-reviewer.md
allowed_tools:
- Read
- Glob
- Grep
rules:
- condition: ai_reviewの指摘が妥当修正すべき
next: ai_fix
- condition: ai_fixの判断が妥当修正不要
next: reviewers
instruction_template: |
ai_reviewレビュアーと ai_fixコーダーの意見が食い違っています。
- ai_review は問題を指摘し REJECT しました
- ai_fix は確認の上「修正不要」と判断しました
両者の出力を確認し、どちらの判断が妥当か裁定してください。
**参照するレポート:**
- AIレビュー結果: {report:03-ai-review.md}
**判断基準:**
- ai_review の指摘が具体的で、コード上の実在する問題を指しているか
- ai_fix の反論に根拠(ファイル確認結果、テスト結果)があるか
- 指摘が非ブロッキング(記録のみ)レベルか、実際に修正が必要か
# =========================================== # ===========================================
# Movement 3: Expert Reviews (Parallel) # Movement 3: Expert Reviews (Parallel)

View File

@ -192,6 +192,11 @@ movements:
- WebSearch - WebSearch
- WebFetch - WebFetch
instruction_template: | instruction_template: |
**これは {movement_iteration} 回目のAI Reviewです。**
初回は網羅的にレビューし、指摘すべき問題をすべて出し切ってください。
2回目以降は、前回REJECTした項目が修正されたかの確認を優先してください。
AI特有の問題についてコードをレビューしてください: AI特有の問題についてコードをレビューしてください:
- 仮定の検証 - 仮定の検証
- もっともらしいが間違っているパターン - もっともらしいが間違っているパターン
@ -261,9 +266,37 @@ movements:
- condition: AI Reviewerの指摘に対する修正が完了した - condition: AI Reviewerの指摘に対する修正が完了した
next: ai_review next: ai_review
- condition: 修正不要(指摘対象ファイル/仕様の確認済み) - condition: 修正不要(指摘対象ファイル/仕様の確認済み)
next: plan next: ai_no_fix
- condition: 修正を進行できない - condition: 修正を進行できない
next: plan next: ai_no_fix
- name: ai_no_fix
edit: false
agent: ../agents/default/architecture-reviewer.md
allowed_tools:
- Read
- Glob
- Grep
rules:
- condition: ai_reviewの指摘が妥当修正すべき
next: ai_fix
- condition: ai_fixの判断が妥当修正不要
next: reviewers
instruction_template: |
ai_reviewレビュアーと ai_fixコーダーの意見が食い違っています。
- ai_review は問題を指摘し REJECT しました
- ai_fix は確認の上「修正不要」と判断しました
両者の出力を確認し、どちらの判断が妥当か裁定してください。
**参照するレポート:**
- AIレビュー結果: {report:03-ai-review.md}
**判断基準:**
- ai_review の指摘が具体的で、コード上の実在する問題を指しているか
- ai_fix の反論に根拠(ファイル確認結果、テスト結果)があるか
- 指摘が非ブロッキング(記録のみ)レベルか、実際に修正が必要か
# =========================================== # ===========================================
# Movement 3: Expert Reviews (Parallel) # Movement 3: Expert Reviews (Parallel)

View File

@ -62,13 +62,13 @@ describe('default piece parallel reviewers movement', () => {
expect(reviewersMovement!.parallel).toHaveLength(2); expect(reviewersMovement!.parallel).toHaveLength(2);
}); });
it('should have arch-review and security-review as parallel sub-movements', () => { it('should have arch-review and qa-review as parallel sub-movements', () => {
const piece = getBuiltinPiece('default'); const piece = getBuiltinPiece('default');
const reviewersMovement = piece!.movements.find((s) => s.name === 'reviewers')!; const reviewersMovement = piece!.movements.find((s) => s.name === 'reviewers')!;
const subMovementNames = reviewersMovement.parallel!.map((s) => s.name); const subMovementNames = reviewersMovement.parallel!.map((s) => s.name);
expect(subMovementNames).toContain('arch-review'); expect(subMovementNames).toContain('arch-review');
expect(subMovementNames).toContain('security-review'); expect(subMovementNames).toContain('qa-review');
}); });
it('should have aggregate conditions on the reviewers parent movement', () => { it('should have aggregate conditions on the reviewers parent movement', () => {
@ -142,8 +142,8 @@ describe('default piece parallel reviewers movement', () => {
const archReview = reviewersMovement.parallel!.find((s) => s.name === 'arch-review')!; const archReview = reviewersMovement.parallel!.find((s) => s.name === 'arch-review')!;
expect(archReview.agent).toContain('architecture-reviewer'); expect(archReview.agent).toContain('architecture-reviewer');
const secReview = reviewersMovement.parallel!.find((s) => s.name === 'security-review')!; const qaReview = reviewersMovement.parallel!.find((s) => s.name === 'qa-review')!;
expect(secReview.agent).toContain('security-reviewer'); expect(qaReview.agent).toContain('default/qa-reviewer');
}); });
it('should have reports configured on sub-movements', () => { it('should have reports configured on sub-movements', () => {
@ -153,8 +153,8 @@ describe('default piece parallel reviewers movement', () => {
const archReview = reviewersMovement.parallel!.find((s) => s.name === 'arch-review')!; const archReview = reviewersMovement.parallel!.find((s) => s.name === 'arch-review')!;
expect(archReview.report).toBeDefined(); expect(archReview.report).toBeDefined();
const secReview = reviewersMovement.parallel!.find((s) => s.name === 'security-review')!; const qaReview = reviewersMovement.parallel!.find((s) => s.name === 'qa-review')!;
expect(secReview.report).toBeDefined(); expect(qaReview.report).toBeDefined();
}); });
}); });

View File

@ -149,7 +149,7 @@ describe('Piece Patterns IT: default piece (parallel reviewers)', () => {
{ agent: 'ai-antipattern-reviewer', status: 'done', content: 'No AI-specific issues' }, { agent: 'ai-antipattern-reviewer', status: 'done', content: 'No AI-specific issues' },
// Parallel reviewers: both approved // Parallel reviewers: both approved
{ agent: 'architecture-reviewer', status: 'done', content: 'approved' }, { agent: 'architecture-reviewer', status: 'done', content: 'approved' },
{ agent: 'security-reviewer', status: 'done', content: 'approved' }, { agent: 'qa-reviewer', status: 'done', content: 'approved' },
// Supervisor // Supervisor
{ agent: 'supervisor', status: 'done', content: 'All checks passed' }, { agent: 'supervisor', status: 'done', content: 'All checks passed' },
]); ]);
@ -168,21 +168,21 @@ describe('Piece Patterns IT: default piece (parallel reviewers)', () => {
{ agent: 'architect', status: 'done', content: 'Design complete' }, { agent: 'architect', status: 'done', content: 'Design complete' },
{ agent: 'coder', status: 'done', content: 'Implementation complete' }, { agent: 'coder', status: 'done', content: 'Implementation complete' },
{ agent: 'ai-antipattern-reviewer', status: 'done', content: 'No AI-specific issues' }, { agent: 'ai-antipattern-reviewer', status: 'done', content: 'No AI-specific issues' },
// Parallel: arch approved, security needs_fix // Parallel: arch approved, qa needs_fix
{ agent: 'architecture-reviewer', status: 'done', content: 'approved' }, { agent: 'architecture-reviewer', status: 'done', content: 'approved' },
{ agent: 'security-reviewer', status: 'done', content: 'needs_fix' }, { agent: 'qa-reviewer', status: 'done', content: 'needs_fix' },
// Fix step // Fix step
{ agent: 'coder', status: 'done', content: 'Fix complete' }, { agent: 'coder', status: 'done', content: 'Fix complete' },
// AI review after fix // AI review after fix
{ agent: 'ai-antipattern-reviewer', status: 'done', content: 'No AI-specific issues' }, { agent: 'ai-antipattern-reviewer', status: 'done', content: 'No AI-specific issues' },
// Re-review: both approved // Re-review: both approved
{ agent: 'architecture-reviewer', status: 'done', content: 'approved' }, { agent: 'architecture-reviewer', status: 'done', content: 'approved' },
{ agent: 'security-reviewer', status: 'done', content: 'approved' }, { agent: 'qa-reviewer', status: 'done', content: 'approved' },
// Supervisor // Supervisor
{ agent: 'supervisor', status: 'done', content: 'All checks passed' }, { agent: 'supervisor', status: 'done', content: 'All checks passed' },
]); ]);
const engine = createEngine(config!, testDir, 'Task needing security fix'); const engine = createEngine(config!, testDir, 'Task needing QA fix');
const state = await engine.run(); const state = await engine.run();
expect(state.status).toBe('completed'); expect(state.status).toBe('completed');