diff --git a/builtins/en/facets/instructions/gather-pr.md b/builtins/en/facets/instructions/gather-pr.md new file mode 100644 index 0000000..43af33e --- /dev/null +++ b/builtins/en/facets/instructions/gather-pr.md @@ -0,0 +1,14 @@ +Gather PR information and produce a report for reviewers to reference. + +**Do:** +1. Extract the PR number from the task ("PR #42", "#42", "pull/42", etc.) +2. Run `gh pr view {number}` to retrieve the PR title, description, and labels +3. Run `gh pr diff {number}` to retrieve the diff +4. Compile the list of changed files +5. Extract the purpose and requirements from the PR description +6. If linked Issues exist, retrieve them with `gh issue view {number}` + - Extract Issue numbers from "Closes #N", "Fixes #N", "Resolves #N" in the PR description + - Collect the Issue title, description, labels, and comments + +**If no PR number is found:** +- Inspect the branch diff and identify the code under review diff --git a/builtins/en/facets/instructions/review-requirements.md b/builtins/en/facets/instructions/review-requirements.md new file mode 100644 index 0000000..6c7f8f5 --- /dev/null +++ b/builtins/en/facets/instructions/review-requirements.md @@ -0,0 +1,21 @@ +Review the changes from a requirements fulfillment perspective. + +**Review criteria:** +- Whether each requested requirement has been implemented +- Whether implicit requirements (naturally expected behaviors) are satisfied +- Whether changes outside the scope (scope creep) have crept in +- Whether there are any partial or missing implementations + +**Previous finding tracking (required):** +- First, extract open findings from "Previous Response" +- Assign `finding_id` to each finding and classify current status as `new / persists / resolved` +- If status is `persists`, provide concrete unresolved evidence (file/line) + +## Judgment Procedure + +1. Extract requirements one by one from the PR description and task +2. For each requirement, identify the implementing code (file:line) +3. Confirm that the code satisfies the requirement +4. Check for any changes not covered by the requirements +5. For each detected issue, classify as blocking/non-blocking based on Policy's scope determination table and judgment rules +6. If there is even one blocking issue (`new` or `persists`), judge as REJECT diff --git a/builtins/en/facets/output-contracts/pr-gather.md b/builtins/en/facets/output-contracts/pr-gather.md new file mode 100644 index 0000000..f32892c --- /dev/null +++ b/builtins/en/facets/output-contracts/pr-gather.md @@ -0,0 +1,29 @@ +```markdown +# PR Information + +## PR Overview +| Field | Details | +|-------|---------| +| PR Number | #{number} | +| Title | {PR title} | +| Labels | {label list} | + +## Purpose & Requirements +{Purpose and requirements extracted from the PR description} + +## Linked Issues +{State "None" if no issues are linked} + +### Issue #{number}: {Issue title} +- Labels: {label list} +- Description: {Summary of Issue body} +- Key comments: {Summary of relevant comments} + +## Changed Files +| File | Type | Lines Changed | +|------|------|---------------| +| `{file path}` | Added/Modified/Deleted | +{added} -{removed} | + +## Diff +{Output of gh pr diff} +``` diff --git a/builtins/en/facets/output-contracts/requirements-review.md b/builtins/en/facets/output-contracts/requirements-review.md new file mode 100644 index 0000000..c16467d --- /dev/null +++ b/builtins/en/facets/output-contracts/requirements-review.md @@ -0,0 +1,44 @@ +```markdown +# Requirements Review + +## Result: APPROVE / REJECT + +## Summary +{Summarize the result in 1-2 sentences} + +## Requirements Cross-Reference +| # | Requirement (from PR) | Satisfied | Evidence (file:line) | +|---|----------------------|-----------|----------------------| +| 1 | {requirement 1} | ✅/❌ | `src/file.ts:42` | + +- If even one ❌ exists, REJECT is mandatory +- A ✅ without evidence is invalid (must be verified in actual code) + +## Scope Check +| # | Out-of-scope Change | File | Justification | +|---|---------------------|------|---------------| +| 1 | {change not in requirements} | `src/file.ts` | Justified/Unnecessary | + +## Current Iteration Findings (new) +| # | finding_id | Category | Location | Issue | Fix Suggestion | +|---|------------|----------|----------|-------|----------------| +| 1 | REQ-NEW-src-file-L42 | Unimplemented | `src/file.ts:42` | Issue description | Fix suggestion | + +## Carry-over Findings (persists) +| # | finding_id | Previous Evidence | Current Evidence | Issue | Fix Suggestion | +|---|------------|-------------------|------------------|-------|----------------| +| 1 | REQ-PERSIST-src-file-L77 | `file:line` | `file:line` | Unresolved | Fix suggestion | + +## Resolved Findings (resolved) +| finding_id | Resolution Evidence | +|------------|---------------------| +| REQ-RESOLVED-src-file-L10 | `file:line` now satisfies the requirement | + +## Rejection Gate +- REJECT is valid only when at least one finding exists in `new` or `persists` +- Findings without `finding_id` are invalid +``` + +**Cognitive load reduction rules:** +- APPROVE: Summary only (5 lines or fewer) +- REJECT: Only relevant findings in tables (30 lines or fewer) diff --git a/builtins/en/facets/output-contracts/testing-review.md b/builtins/en/facets/output-contracts/testing-review.md new file mode 100644 index 0000000..6bba886 --- /dev/null +++ b/builtins/en/facets/output-contracts/testing-review.md @@ -0,0 +1,41 @@ +```markdown +# Testing Review + +## Result: APPROVE / REJECT + +## Summary +{Summarize the result in 1-2 sentences} + +## Reviewed Aspects +| Aspect | Result | Notes | +|--------|--------|-------| +| Test coverage | ✅ | - | +| Test structure (Given-When-Then) | ✅ | - | +| Test naming | ✅ | - | +| Test independence & reproducibility | ✅ | - | +| Mocks & fixtures | ✅ | - | +| Test strategy (unit/integration/E2E) | ✅ | - | + +## Current Iteration Findings (new) +| # | finding_id | Category | Location | Issue | Fix Suggestion | +|---|------------|----------|----------|-------|----------------| +| 1 | TEST-NEW-src-test-L42 | Coverage | `src/test.ts:42` | Issue description | Fix suggestion | + +## Carry-over Findings (persists) +| # | finding_id | Previous Evidence | Current Evidence | Issue | Fix Suggestion | +|---|------------|-------------------|------------------|-------|----------------| +| 1 | TEST-PERSIST-src-test-L77 | `src/test.ts:77` | `src/test.ts:77` | Unresolved | Fix suggestion | + +## Resolved Findings (resolved) +| finding_id | Resolution Evidence | +|------------|---------------------| +| TEST-RESOLVED-src-test-L10 | `src/test.ts:10` now has sufficient coverage | + +## Rejection Gate +- REJECT is valid only when at least one finding exists in `new` or `persists` +- Findings without `finding_id` are invalid +``` + +**Cognitive load reduction rules:** +- APPROVE: Summary only (5 lines or fewer) +- REJECT: Only relevant findings in tables (30 lines or fewer) diff --git a/builtins/en/facets/personas/requirements-reviewer.md b/builtins/en/facets/personas/requirements-reviewer.md new file mode 100644 index 0000000..6517719 --- /dev/null +++ b/builtins/en/facets/personas/requirements-reviewer.md @@ -0,0 +1,26 @@ +# Requirements Reviewer + +You are a requirements fulfillment verifier. You verify that changes satisfy the original requirements and specifications, and flag any gaps or excess. + +## Role Boundaries + +**Do:** +- Cross-reference requirements against implementation (whether each requirement is realized in actual code) +- Detect implicit requirements (whether naturally expected behaviors are satisfied) +- Detect scope creep (whether changes unrelated to requirements have crept in) +- Identify unimplemented or partially implemented items +- Flag ambiguity in specifications + +**Don't:** +- Review code quality (Architecture Reviewer's job) +- Review test coverage (Testing Reviewer's job) +- Review security concerns (Security Reviewer's job) +- Write code yourself + +## Behavioral Principles + +- Verify requirements one by one. Never say "broadly satisfied" in aggregate +- Verify in actual code. Do not take "implemented" claims at face value +- Guard the scope. Question any change not covered by the requirements +- Do not tolerate ambiguity. Flag unclear or underspecified requirements +- Pay attention to deletions. Confirm that file or code removals are justified by the requirements diff --git a/builtins/en/facets/personas/testing-reviewer.md b/builtins/en/facets/personas/testing-reviewer.md new file mode 100644 index 0000000..30dcbdc --- /dev/null +++ b/builtins/en/facets/personas/testing-reviewer.md @@ -0,0 +1,27 @@ +# Testing Reviewer + +You are a test code quality specialist. You evaluate test structure, naming, coverage, independence, and verify the reliability of the test suite. + +## Role Boundaries + +**Do:** +- Evaluate test structure (Given-When-Then / Arrange-Act-Assert) +- Verify test naming conventions +- Assess test coverage (whether new behaviors and bug fixes have tests) +- Verify test independence and reproducibility +- Check appropriateness of mocks and fixtures +- Evaluate test strategy (unit/integration/E2E selection) + +**Don't:** +- Review error handling or logging (QA Reviewer's job) +- Review security concerns (Security Reviewer's job) +- Review architecture decisions (Architecture Reviewer's job) +- Write code yourself + +## Behavioral Principles + +- Untested code is not trustworthy. New behaviors must have tests +- Structure matters. Demand improvements for tests that lack clear Given-When-Then +- Ensure independence. Flag tests that depend on execution order or external state +- Names convey intent. Verify that test names clearly describe the behavior under test +- Balance coverage. Suggest both removing unnecessary tests and adding missing cases diff --git a/builtins/en/piece-categories.yaml b/builtins/en/piece-categories.yaml index a90428b..06ff99e 100644 --- a/builtins/en/piece-categories.yaml +++ b/builtins/en/piece-categories.yaml @@ -36,8 +36,7 @@ piece_categories: - structural-reform 🔍 Review: pieces: - - review-fix-minimal - - review-only + - pr-review 🧪 Testing: pieces: - unit-test diff --git a/builtins/en/pieces/review-only.yaml b/builtins/en/pieces/pr-review.yaml similarity index 57% rename from builtins/en/pieces/review-only.yaml rename to builtins/en/pieces/pr-review.yaml index 01ff36c..f3c110c 100644 --- a/builtins/en/pieces/review-only.yaml +++ b/builtins/en/pieces/pr-review.yaml @@ -1,5 +1,5 @@ -name: review-only -description: Review-only piece - reviews code without making edits +name: pr-review +description: Multi-perspective PR Review - reviews a PR in parallel from 5 perspectives and outputs consolidated results piece_config: provider_options: codex: @@ -7,42 +7,34 @@ piece_config: opencode: network_access: true max_movements: 10 -initial_movement: plan +initial_movement: gather + movements: - - name: plan + - name: gather edit: false persona: planner allowed_tools: - Read - Glob - Grep + - Bash - WebSearch - WebFetch + instruction: gather-pr + output_contracts: + report: + - name: 00-pr-info.md + format: pr-gather rules: - - condition: Review scope is clear + - condition: PR information gathered next: reviewers - - condition: User is asking a question (not a review task) - next: COMPLETE - - condition: Requirements unclear, insufficient info + - condition: PR number unidentifiable, insufficient info next: ABORT appendix: | Clarifications needed: - {Question 1} - {Question 2} - instruction_template: | - ## Previous Response (when returned from supervise) - {previous_response} - Analyze the review request and create a review plan. - - **This is a review-only piece.** No code edits will be made. - Focus on: - 1. Identify which files/modules to review - 2. Determine review focus areas (architecture, security, AI patterns, etc.) - 3. Note any specific concerns mentioned in the request - - **If a PR number is mentioned** (e.g., "PR #42"), include it in your plan - so reviewers can focus on the PR's changed files. - name: reviewers parallel: - name: arch-review @@ -54,16 +46,18 @@ movements: - Read - Glob - Grep + - Bash - WebSearch - WebFetch - rules: - - condition: approved - - condition: needs_fix instruction: review-arch output_contracts: report: - - name: 01-architect-review.md + - name: 01-architecture-review.md format: architecture-review + rules: + - condition: approved + - condition: needs_fix + - name: security-review edit: false persona: security-reviewer @@ -73,41 +67,88 @@ movements: - Read - Glob - Grep + - Bash - WebSearch - WebFetch - rules: - - condition: approved - - condition: needs_fix instruction: review-security output_contracts: report: - name: 02-security-review.md format: security-review - - name: ai-review + rules: + - condition: approved + - condition: needs_fix + + - name: qa-review edit: false - persona: ai-antipattern-reviewer + persona: qa-reviewer policy: - review - - ai-antipattern + - qa allowed_tools: - Read - Glob - Grep + - Bash - WebSearch - WebFetch + instruction: review-qa + output_contracts: + report: + - name: 03-qa-review.md + format: qa-review rules: - condition: approved - condition: needs_fix - instruction: review-ai + + - name: testing-review + edit: false + persona: testing-reviewer + policy: + - review + - testing + allowed_tools: + - Read + - Glob + - Grep + - Bash + - WebSearch + - WebFetch + instruction: review-test output_contracts: report: - - name: 03-ai-review.md - format: ai-review + - name: 04-testing-review.md + format: testing-review + rules: + - condition: approved + - condition: needs_fix + + - name: requirements-review + edit: false + persona: requirements-reviewer + policy: review + allowed_tools: + - Read + - Glob + - Grep + - Bash + - WebSearch + - WebFetch + instruction: review-requirements + output_contracts: + report: + - name: 05-requirements-review.md + format: requirements-review + rules: + - condition: approved + - condition: needs_fix + rules: - condition: all("approved") next: supervise - condition: any("needs_fix") next: supervise + - name: supervise edit: false persona: supervisor @@ -120,32 +161,29 @@ movements: - WebFetch pass_previous_response: false rules: - - condition: approved, PR comment requested - next: pr-comment - - condition: approved + - condition: Review synthesis complete next: COMPLETE - - condition: rejected - next: ABORT instruction_template: | ## Review Results {previous_response} - **This is a review-only piece.** Do NOT run tests or builds. + **This is a PR review-only piece.** Do NOT run tests or builds. Your role is to synthesize the review results and produce a final summary. **Tasks:** 1. Read all review reports in the Report Directory - 2. Synthesize findings from architecture, security, and AI reviews + - `00-pr-info.md` (PR information) + - `01-architecture-review.md` (Architecture review) + - `02-security-review.md` (Security review) + - `03-qa-review.md` (QA review) + - `04-testing-review.md` (Testing review) + - `05-requirements-review.md` (Requirements review) + 2. Synthesize the 5 review results 3. Produce a consolidated review summary with overall verdict - 4. Determine routing: - - If the task mentions posting to a PR (e.g., "post comments to PR", "comment on PR"), - route to `pr-comment` movement (condition: "approved, PR comment requested") - - If local review only, route to COMPLETE (condition: "approved") - - If critical issues found, route to ABORT (condition: "rejected") **Review Summary output contract:** ```markdown - # Review Summary + # PR Review Summary ## Overall Verdict: APPROVE / REJECT @@ -157,7 +195,9 @@ movements: |--------|--------|--------------| | Architecture | APPROVE/REJECT | {Brief finding} | | Security | APPROVE/REJECT | {Brief finding} | - | AI Antipattern | APPROVE/REJECT | {Brief finding} | + | QA | APPROVE/REJECT | {Brief finding} | + | Testing | APPROVE/REJECT | {Brief finding} | + | Requirements | APPROVE/REJECT | {Brief finding} | ## Issues Requiring Attention | # | Severity | Source | Location | Issue | @@ -172,19 +212,21 @@ movements: - name: review-summary.md format: | ```markdown - # Review Summary + # PR Review Summary ## Overall Verdict: APPROVE / REJECT ## Summary - {Integrate all review results in 2-3 sentences} + {2-3 sentences consolidating all review results} ## Review Results | Review | Result | Key Findings | - |--------|--------|-------------| + |--------|--------|--------------| | Architecture | APPROVE/REJECT | {Overview} | | Security | APPROVE/REJECT | {Overview} | - | AI Anti-pattern | APPROVE/REJECT | {Overview} | + | QA | APPROVE/REJECT | {Overview} | + | Testing | APPROVE/REJECT | {Overview} | + | Requirements | APPROVE/REJECT | {Overview} | ## Current Iteration Findings (new) | # | finding_id | Severity | Source | Location | Issue | Fix Suggestion | @@ -208,53 +250,3 @@ movements: - REJECT is valid only when at least one finding exists in `new` or `persists` - Findings without `finding_id` are invalid ``` - - name: pr-comment - edit: false - persona: pr-commenter - allowed_tools: - - Read - - Glob - - Grep - - Bash - rules: - - condition: Comments posted - next: COMPLETE - - condition: Failed to post comments - next: COMPLETE - instruction_template: | - ## Review Summary - {previous_response} - - Post the review results to the PR as comments. - - **Procedure:** - 1. Extract the PR number from the task description - 2. Read all review reports in the Report Directory: - - `01-architect-review.md` (Architecture review) - - `02-security-review.md` (Security review) - - `03-ai-review.md` (AI antipattern review) - - `review-summary.md` (Consolidated summary) - 3. Filter findings by severity and post inline comments for Critical/High/Medium - 4. Post a summary comment with the following format: - - ``` - ## Automated Review Summary - - {Overall verdict and summary from review-summary.md} - - ### Review Results - | Review | Result | - |--------|--------| - | Architecture | {result} | - | Security | {result} | - | AI Antipattern | {result} | - - ### Key Findings - {Bulleted list of important findings} - - ### Improvement Suggestions - {Consolidated suggestions} - - --- - *Generated by [takt](https://github.com/toruticas/takt) review-only piece* - ``` diff --git a/builtins/en/pieces/review-fix-minimal.yaml b/builtins/en/pieces/review-fix-minimal.yaml deleted file mode 100644 index 309638f..0000000 --- a/builtins/en/pieces/review-fix-minimal.yaml +++ /dev/null @@ -1,190 +0,0 @@ -name: review-fix-minimal -description: Review and fix piece for existing code (starts with review, no implementation) -piece_config: - provider_options: - codex: - network_access: true - opencode: - network_access: true -max_movements: 20 -initial_movement: reviewers -movements: - - name: implement - edit: true - persona: coder - policy: - - coding - - testing - allowed_tools: - - Read - - Glob - - Grep - - Edit - - Write - - Bash - - WebSearch - - WebFetch - required_permission_mode: edit - instruction: implement - rules: - - condition: Implementation complete - next: reviewers - - condition: Cannot proceed, insufficient info - next: ABORT - - condition: User input required because there are items to confirm with the user - next: implement - requires_user_input: true - interactive_only: true - output_contracts: - report: - - name: coder-scope.md - format: coder-scope - - name: coder-decisions.md - format: coder-decisions - - name: reviewers - parallel: - - name: ai_review - edit: false - persona: ai-antipattern-reviewer - policy: - - review - - ai-antipattern - allowed_tools: - - Read - - Glob - - Grep - - WebSearch - - WebFetch - instruction: review-ai - rules: - - condition: No AI-specific issues - - condition: AI-specific issues found - output_contracts: - report: - - name: 03-ai-review.md - format: ai-review - - name: supervise - edit: false - persona: supervisor - policy: review - allowed_tools: - - Read - - Glob - - Grep - - Bash - - WebSearch - - WebFetch - instruction: supervise - rules: - - condition: All checks passed - - condition: Requirements unmet, tests failing - output_contracts: - report: - - name: supervisor-validation.md - format: supervisor-validation - - name: summary.md - format: summary - use_judge: false - rules: - - condition: all("No AI-specific issues", "All checks passed") - next: COMPLETE - - condition: all("AI-specific issues found", "Requirements unmet, tests failing") - next: fix_both - - condition: any("AI-specific issues found") - next: ai_fix - - condition: any("Requirements unmet, tests failing") - next: supervise_fix - - name: fix_both - parallel: - - name: ai_fix_parallel - edit: true - persona: coder - policy: - - coding - - testing - allowed_tools: - - Read - - Glob - - Grep - - Edit - - Bash - - WebSearch - - WebFetch - required_permission_mode: edit - rules: - - condition: AI Reviewer's issues fixed - - condition: No fix needed (verified target files/spec) - - condition: Cannot proceed, insufficient info - instruction: ai-fix - - name: supervise_fix_parallel - edit: true - persona: coder - policy: - - coding - - testing - allowed_tools: - - Read - - Glob - - Grep - - Edit - - Bash - - WebSearch - - WebFetch - required_permission_mode: edit - rules: - - condition: Supervisor's issues fixed - - condition: Cannot proceed, insufficient info - instruction: fix-supervisor - rules: - - condition: all("AI Reviewer's issues fixed", "Supervisor's issues fixed") - next: reviewers - - condition: any("No fix needed (verified target files/spec)", "Cannot proceed, insufficient info") - next: implement - - name: ai_fix - edit: true - persona: coder - policy: - - coding - - testing - allowed_tools: - - Read - - Glob - - Grep - - Edit - - Write - - Bash - - WebSearch - - WebFetch - required_permission_mode: edit - pass_previous_response: false - rules: - - condition: AI Reviewer's issues fixed - next: reviewers - - condition: No fix needed (verified target files/spec) - next: implement - - condition: Cannot proceed, insufficient info - next: implement - instruction: ai-fix - - name: supervise_fix - edit: true - persona: coder - policy: - - coding - - testing - allowed_tools: - - Read - - Glob - - Grep - - Edit - - Write - - Bash - - WebSearch - - WebFetch - required_permission_mode: edit - pass_previous_response: false - rules: - - condition: Supervisor's issues fixed - next: reviewers - - condition: Cannot proceed, insufficient info - next: implement - instruction: fix-supervisor diff --git a/builtins/ja/facets/instructions/gather-pr.md b/builtins/ja/facets/instructions/gather-pr.md new file mode 100644 index 0000000..8d353fb --- /dev/null +++ b/builtins/ja/facets/instructions/gather-pr.md @@ -0,0 +1,14 @@ +PR の情報を収集し、レビュアーが参照するレポートを作成してください。 + +**やること:** +1. タスクから PR 番号を抽出する("PR #42"、"#42"、"pull/42" 等) +2. `gh pr view {番号}` で PR のタイトル・説明・ラベルを取得 +3. `gh pr diff {番号}` で差分を取得 +4. 変更ファイル一覧をまとめる +5. PR の説明から要件・目的を抽出する +6. リンクされた Issue があれば `gh issue view {番号}` で取得する + - PR 説明内の "Closes #N"、"Fixes #N"、"Resolves #N" 等からIssue番号を抽出 + - Issue のタイトル・説明・ラベル・コメントを収集する + +**PR番号が見つからない場合:** +- ブランチの差分を確認し、レビュー対象のコードを特定する diff --git a/builtins/ja/facets/instructions/review-requirements.md b/builtins/ja/facets/instructions/review-requirements.md new file mode 100644 index 0000000..3a2c6bb --- /dev/null +++ b/builtins/ja/facets/instructions/review-requirements.md @@ -0,0 +1,21 @@ +要件充足の観点から変更をレビューしてください。 + +**レビュー観点:** +- 要求された各要件が実装されているか +- 暗黙の要求(当然期待される動作)が満たされているか +- 要求にない変更(スコープクリープ)が紛れていないか +- 部分実装や未実装がないか + +**前回指摘の追跡(必須):** +- まず「Previous Response」から前回の open findings を抽出する +- 各 finding に `finding_id` を付け、今回の状態を `new / persists / resolved` で判定する +- `persists` と判定する場合は、未解決である根拠(ファイル/行)を必ず示す + +## 判定手順 + +1. PR の説明・タスクから要件を1つずつ抽出する +2. 各要件について、実装されたコード(ファイル:行)を特定する +3. コードが要件を満たしていることを確認する +4. 要求にない変更がないかチェックする +5. 検出した問題ごとに、Policy のスコープ判定表と判定ルールに基づいてブロッキング/非ブロッキングを分類する +6. ブロッキング問題(`new` または `persists`)が1件でもあれば REJECT と判定する diff --git a/builtins/ja/facets/output-contracts/pr-gather.md b/builtins/ja/facets/output-contracts/pr-gather.md new file mode 100644 index 0000000..f9fe22f --- /dev/null +++ b/builtins/ja/facets/output-contracts/pr-gather.md @@ -0,0 +1,29 @@ +```markdown +# PR 情報 + +## PR 概要 +| 項目 | 内容 | +|------|------| +| PR番号 | #{番号} | +| タイトル | {PRタイトル} | +| ラベル | {ラベル一覧} | + +## 目的・要件 +{PRの説明から抽出した目的と要件} + +## リンクされた Issue +{Issue がない場合は「なし」と記載} + +### Issue #{番号}: {Issueタイトル} +- ラベル: {ラベル一覧} +- 説明: {Issue本文の要約} +- 主要コメント: {関連するコメントの要約} + +## 変更ファイル一覧 +| ファイル | 種別 | 変更行数 | +|---------|------|---------| +| `{ファイルパス}` | 追加/変更/削除 | +{追加} -{削除} | + +## 差分 +{gh pr diff の出力} +``` diff --git a/builtins/ja/facets/output-contracts/requirements-review.md b/builtins/ja/facets/output-contracts/requirements-review.md new file mode 100644 index 0000000..70f9039 --- /dev/null +++ b/builtins/ja/facets/output-contracts/requirements-review.md @@ -0,0 +1,44 @@ +```markdown +# 要件充足レビュー + +## 結果: APPROVE / REJECT + +## サマリー +{1-2文で結果を要約} + +## 要件照合 +| # | 要件(PRから抽出) | 充足 | 根拠(ファイル:行) | +|---|-------------------|------|-------------------| +| 1 | {要件1} | ✅/❌ | `src/file.ts:42` | + +- ❌ が1件でもある場合は REJECT 必須 +- 根拠なしの ✅ は無効(実コードで確認すること) + +## スコープチェック +| # | 要求外の変更 | ファイル | 妥当性 | +|---|-------------|---------|--------| +| 1 | {要求にない変更} | `src/file.ts` | 妥当/不要 | + +## 今回の指摘(new) +| # | finding_id | カテゴリ | 場所 | 問題 | 修正案 | +|---|------------|---------|------|------|--------| +| 1 | REQ-NEW-src-file-L42 | 未実装 | `src/file.ts:42` | 問題の説明 | 修正方法 | + +## 継続指摘(persists) +| # | finding_id | 前回根拠 | 今回根拠 | 問題 | 修正案 | +|---|------------|----------|----------|------|--------| +| 1 | REQ-PERSIST-src-file-L77 | `file:line` | `file:line` | 未解消 | 修正方法 | + +## 解消済み(resolved) +| finding_id | 解消根拠 | +|------------|----------| +| REQ-RESOLVED-src-file-L10 | `file:line` は要件を充足 | + +## REJECT判定条件 +- `new` または `persists` が1件以上ある場合のみ REJECT 可 +- `finding_id` なしの指摘は無効 +``` + +**認知負荷軽減ルール:** +- APPROVE → サマリーのみ(5行以内) +- REJECT → 該当指摘のみ表で記載(30行以内) diff --git a/builtins/ja/facets/output-contracts/testing-review.md b/builtins/ja/facets/output-contracts/testing-review.md new file mode 100644 index 0000000..fdf4511 --- /dev/null +++ b/builtins/ja/facets/output-contracts/testing-review.md @@ -0,0 +1,41 @@ +```markdown +# テストレビュー + +## 結果: APPROVE / REJECT + +## サマリー +{1-2文で結果を要約} + +## 確認した観点 +| 観点 | 結果 | 備考 | +|------|------|------| +| テストカバレッジ | ✅ | - | +| テスト構造(Given-When-Then) | ✅ | - | +| テスト命名 | ✅ | - | +| テスト独立性・再現性 | ✅ | - | +| モック・フィクスチャ | ✅ | - | +| テスト戦略(ユニット/統合/E2E) | ✅ | - | + +## 今回の指摘(new) +| # | finding_id | カテゴリ | 場所 | 問題 | 修正案 | +|---|------------|---------|------|------|--------| +| 1 | TEST-NEW-src-test-L42 | カバレッジ | `src/test.ts:42` | 問題の説明 | 修正方法 | + +## 継続指摘(persists) +| # | finding_id | 前回根拠 | 今回根拠 | 問題 | 修正案 | +|---|------------|----------|----------|------|--------| +| 1 | TEST-PERSIST-src-test-L77 | `src/test.ts:77` | `src/test.ts:77` | 未解消 | 修正方法 | + +## 解消済み(resolved) +| finding_id | 解消根拠 | +|------------|----------| +| TEST-RESOLVED-src-test-L10 | `src/test.ts:10` でカバレッジ充足 | + +## REJECT判定条件 +- `new` または `persists` が1件以上ある場合のみ REJECT 可 +- `finding_id` なしの指摘は無効 +``` + +**認知負荷軽減ルール:** +- APPROVE → サマリーのみ(5行以内) +- REJECT → 該当指摘のみ表で記載(30行以内) diff --git a/builtins/ja/facets/personas/requirements-reviewer.md b/builtins/ja/facets/personas/requirements-reviewer.md new file mode 100644 index 0000000..0c1d67e --- /dev/null +++ b/builtins/ja/facets/personas/requirements-reviewer.md @@ -0,0 +1,26 @@ +# Requirements Reviewer + +あなたは要件充足の検証者です。変更が元の要求・仕様を満たしているかを検証し、過不足を指摘します。 + +## 役割の境界 + +**やること:** +- 要求と実装の照合(各要件が実コードで実現されているか) +- 暗黙の要求の検出(当然期待される動作が満たされているか) +- スコープクリープの検出(要求にない変更が紛れていないか) +- 未実装・部分実装の特定 +- 仕様の曖昧さの指摘 + +**やらないこと:** +- コード品質のレビュー(Architecture Reviewer が担当) +- テストカバレッジの確認(Testing Reviewer が担当) +- セキュリティの懸念(Security Reviewer が担当) +- 自分でコードを書く + +## 行動姿勢 + +- 要求を1つずつ照合する。まとめて「概ね充足」とは言わない +- 実コードで確認する。「実装しました」を鵜呑みにしない +- スコープを守る。要求にない変更は理由を問う +- 曖昧さを放置しない。仕様が不明確なら指摘する +- 削除に注目する。ファイルやコードの削除が要求に基づくか確認する diff --git a/builtins/ja/facets/personas/testing-reviewer.md b/builtins/ja/facets/personas/testing-reviewer.md new file mode 100644 index 0000000..6188d76 --- /dev/null +++ b/builtins/ja/facets/personas/testing-reviewer.md @@ -0,0 +1,27 @@ +# Testing Reviewer + +あなたはテストコード品質の専門家です。テストの構造、命名、カバレッジ、独立性を評価し、テストスイートの信頼性を検証します。 + +## 役割の境界 + +**やること:** +- テストの構造評価(Given-When-Then / Arrange-Act-Assert) +- テスト命名規約の確認 +- テストカバレッジの評価(新しい振る舞い・バグ修正に対するテスト有無) +- テスト独立性・再現性の検証 +- モック・フィクスチャの適切さの確認 +- テスト戦略の妥当性(ユニット/インテグレーション/E2Eの選択) + +**やらないこと:** +- エラーハンドリング・ログの確認(QA Reviewer が担当) +- セキュリティの懸念(Security Reviewer が担当) +- アーキテクチャの判断(Architecture Reviewer が担当) +- 自分でコードを書く + +## 行動姿勢 + +- テストがないコードは信用しない。新しい振る舞いにはテストが必須 +- 構造を重視する。Given-When-Then が明確でないテストは改善を求める +- 独立性を確保する。実行順序や外部状態に依存するテストは指摘する +- 命名で意図を伝える。テスト名から振る舞いが読み取れるか確認する +- 過不足を見極める。不要なテストの削除も、足りないケースの追加も提案する diff --git a/builtins/ja/piece-categories.yaml b/builtins/ja/piece-categories.yaml index 21323ea..8e5ebae 100644 --- a/builtins/ja/piece-categories.yaml +++ b/builtins/ja/piece-categories.yaml @@ -36,8 +36,7 @@ piece_categories: - structural-reform 🔍 レビュー: pieces: - - review-fix-minimal - - review-only + - pr-review 🧪 テスト: pieces: - unit-test diff --git a/builtins/ja/pieces/pr-review.yaml b/builtins/ja/pieces/pr-review.yaml new file mode 100644 index 0000000..c0c945a --- /dev/null +++ b/builtins/ja/pieces/pr-review.yaml @@ -0,0 +1,225 @@ +name: pr-review +description: PR多角レビュー - 5つの観点から並列にPRをレビューし、統合結果を出力する +piece_config: + provider_options: + codex: + network_access: true + opencode: + network_access: true +max_movements: 10 +initial_movement: gather + +movements: + - name: gather + edit: false + persona: planner + allowed_tools: + - Read + - Glob + - Grep + - Bash + - WebSearch + - WebFetch + instruction: gather-pr + output_contracts: + report: + - name: 00-pr-info.md + format: pr-gather + rules: + - condition: PR情報の収集完了 + next: reviewers + - condition: PR番号が特定できない、情報不足 + next: ABORT + appendix: | + 確認事項: + - {質問1} + - {質問2} + + - name: reviewers + parallel: + - name: arch-review + edit: false + persona: architecture-reviewer + policy: review + knowledge: architecture + allowed_tools: + - Read + - Glob + - Grep + - Bash + - WebSearch + - WebFetch + instruction: review-arch + output_contracts: + report: + - name: 01-architecture-review.md + format: architecture-review + rules: + - condition: approved + - condition: needs_fix + + - name: security-review + edit: false + persona: security-reviewer + policy: review + knowledge: security + allowed_tools: + - Read + - Glob + - Grep + - Bash + - WebSearch + - WebFetch + instruction: review-security + output_contracts: + report: + - name: 02-security-review.md + format: security-review + rules: + - condition: approved + - condition: needs_fix + + - name: qa-review + edit: false + persona: qa-reviewer + policy: + - review + - qa + allowed_tools: + - Read + - Glob + - Grep + - Bash + - WebSearch + - WebFetch + instruction: review-qa + output_contracts: + report: + - name: 03-qa-review.md + format: qa-review + rules: + - condition: approved + - condition: needs_fix + + - name: testing-review + edit: false + persona: testing-reviewer + policy: + - review + - testing + allowed_tools: + - Read + - Glob + - Grep + - Bash + - WebSearch + - WebFetch + instruction: review-test + output_contracts: + report: + - name: 04-testing-review.md + format: testing-review + rules: + - condition: approved + - condition: needs_fix + + - name: requirements-review + edit: false + persona: requirements-reviewer + policy: review + allowed_tools: + - Read + - Glob + - Grep + - Bash + - WebSearch + - WebFetch + instruction: review-requirements + output_contracts: + report: + - name: 05-requirements-review.md + format: requirements-review + rules: + - condition: approved + - condition: needs_fix + + rules: + - condition: all("approved") + next: supervise + - condition: any("needs_fix") + next: supervise + + - name: supervise + edit: false + persona: supervisor + policy: review + allowed_tools: + - Read + - Glob + - Grep + - WebSearch + - WebFetch + pass_previous_response: false + rules: + - condition: レビュー統合完了 + next: COMPLETE + instruction_template: | + ## レビュー結果 + {previous_response} + + **これはPRレビュー専用ピースです。** テスト実行やビルドは行わないでください。 + レビュー結果を統合し、最終サマリーを作成する役割です。 + + **やること:** + 1. Report Directory内の全レビューレポートを読む + - `00-pr-info.md`(PR情報) + - `01-architecture-review.md`(アーキテクチャレビュー) + - `02-security-review.md`(セキュリティレビュー) + - `03-qa-review.md`(QAレビュー) + - `04-testing-review.md`(テストレビュー) + - `05-requirements-review.md`(要件充足レビュー) + 2. 5つのレビュー結果を統合 + 3. 統合レビューサマリーと総合判定を作成 + output_contracts: + report: + - name: review-summary.md + format: | + ```markdown + # PRレビューサマリー + + ## 総合判定: APPROVE / REJECT + + ## サマリー + {2-3文で全レビュー結果を統合} + + ## レビュー結果 + | レビュー | 結果 | 主要な発見 | + |---------|------|-----------| + | アーキテクチャ | APPROVE/REJECT | {概要} | + | セキュリティ | APPROVE/REJECT | {概要} | + | QA | APPROVE/REJECT | {概要} | + | テスト | APPROVE/REJECT | {概要} | + | 要件充足 | APPROVE/REJECT | {概要} | + + ## 今回の指摘(new) + | # | finding_id | 重大度 | ソース | 場所 | 問題 | 修正案 | + |---|------------|--------|--------|------|------|--------| + | 1 | SUM-NEW-src-file-L42 | High | セキュリティ | `file:line` | 説明 | 提案 | + + ## 継続指摘(persists) + | # | finding_id | ソース | 前回根拠 | 今回根拠 | 問題 | + |---|------------|--------|----------|----------|------| + | 1 | SUM-PERSIST-src-file-L77 | アーキテクチャ | `file:line` | `file:line` | 説明 | + + ## 解消済み(resolved) + | finding_id | ソース | 解消根拠 | + |------------|--------|----------| + | SUM-RESOLVED-src-file-L10 | QA | `file:line` | + + ## 改善提案 + - {全レビューからの統合提案} + + ## REJECT判定条件 + - `new` または `persists` が1件以上ある場合のみ REJECT 可 + - `finding_id` なしの指摘は無効 + ``` diff --git a/builtins/ja/pieces/review-fix-minimal.yaml b/builtins/ja/pieces/review-fix-minimal.yaml deleted file mode 100644 index 9803ae7..0000000 --- a/builtins/ja/pieces/review-fix-minimal.yaml +++ /dev/null @@ -1,190 +0,0 @@ -name: review-fix-minimal -description: 既存コードのレビューと修正ピース(レビュー開始、実装なし) -piece_config: - provider_options: - codex: - network_access: true - opencode: - network_access: true -max_movements: 20 -initial_movement: reviewers -movements: - - name: implement - edit: true - persona: coder - policy: - - coding - - testing - allowed_tools: - - Read - - Glob - - Grep - - Edit - - Write - - Bash - - WebSearch - - WebFetch - required_permission_mode: edit - instruction: implement - rules: - - condition: 実装が完了した - next: reviewers - - condition: 実装を進行できない - next: ABORT - - condition: ユーザーへの確認事項があるためユーザー入力が必要 - next: implement - requires_user_input: true - interactive_only: true - output_contracts: - report: - - name: coder-scope.md - format: coder-scope - - name: coder-decisions.md - format: coder-decisions - - name: reviewers - parallel: - - name: ai_review - edit: false - persona: ai-antipattern-reviewer - policy: - - review - - ai-antipattern - allowed_tools: - - Read - - Glob - - Grep - - WebSearch - - WebFetch - instruction: review-ai - rules: - - condition: AI特有の問題なし - - condition: AI特有の問題あり - output_contracts: - report: - - name: 03-ai-review.md - format: ai-review - - name: supervise - edit: false - persona: supervisor - policy: review - allowed_tools: - - Read - - Glob - - Grep - - Bash - - WebSearch - - WebFetch - instruction: supervise - rules: - - condition: すべて問題なし - - condition: 要求未達成、テスト失敗、ビルドエラー - output_contracts: - report: - - name: supervisor-validation.md - format: supervisor-validation - - name: summary.md - format: summary - use_judge: false - rules: - - condition: all("AI特有の問題なし", "すべて問題なし") - next: COMPLETE - - condition: all("AI特有の問題あり", "要求未達成、テスト失敗、ビルドエラー") - next: fix_both - - condition: any("AI特有の問題あり") - next: ai_fix - - condition: any("要求未達成、テスト失敗、ビルドエラー") - next: supervise_fix - - name: fix_both - parallel: - - name: ai_fix_parallel - edit: true - persona: coder - policy: - - coding - - testing - allowed_tools: - - Read - - Glob - - Grep - - Edit - - Bash - - WebSearch - - WebFetch - required_permission_mode: edit - rules: - - condition: AI問題の修正完了 - - condition: 修正不要(指摘対象ファイル/仕様の確認済み) - - condition: 判断できない、情報不足 - instruction: ai-fix - - name: supervise_fix_parallel - edit: true - persona: coder - policy: - - coding - - testing - allowed_tools: - - Read - - Glob - - Grep - - Edit - - Bash - - WebSearch - - WebFetch - required_permission_mode: edit - rules: - - condition: 監督者の指摘に対する修正が完了した - - condition: 修正を進行できない - instruction: fix-supervisor - rules: - - condition: all("AI問題の修正完了", "監督者の指摘に対する修正が完了した") - next: reviewers - - condition: any("修正不要(指摘対象ファイル/仕様の確認済み)", "判断できない、情報不足", "修正を進行できない") - next: implement - - name: ai_fix - edit: true - persona: coder - policy: - - coding - - testing - allowed_tools: - - Read - - Glob - - Grep - - Edit - - Write - - Bash - - WebSearch - - WebFetch - required_permission_mode: edit - pass_previous_response: false - rules: - - condition: AI問題の修正完了 - next: reviewers - - condition: 修正不要(指摘対象ファイル/仕様の確認済み) - next: implement - - condition: 判断できない、情報不足 - next: implement - instruction: ai-fix - - name: supervise_fix - edit: true - persona: coder - policy: - - coding - - testing - allowed_tools: - - Read - - Glob - - Grep - - Edit - - Write - - Bash - - WebSearch - - WebFetch - required_permission_mode: edit - pass_previous_response: false - rules: - - condition: 監督者の指摘に対する修正が完了した - next: reviewers - - condition: 修正を進行できない - next: implement - instruction: fix-supervisor diff --git a/builtins/ja/pieces/review-only.yaml b/builtins/ja/pieces/review-only.yaml deleted file mode 100644 index cdd77be..0000000 --- a/builtins/ja/pieces/review-only.yaml +++ /dev/null @@ -1,261 +0,0 @@ -name: review-only -description: レビュー専用ピース - コードをレビューするだけで編集は行わない -piece_config: - provider_options: - codex: - network_access: true - opencode: - network_access: true -max_movements: 10 -initial_movement: plan -movements: - - name: plan - edit: false - persona: planner - allowed_tools: - - Read - - Glob - - Grep - - WebSearch - - WebFetch - rules: - - condition: レビュー対象が明確 - next: reviewers - - condition: ユーザーが質問をしている(レビュータスクではない) - next: COMPLETE - - condition: 要件が不明確、情報不足 - next: ABORT - appendix: | - 確認事項: - - {質問1} - - {質問2} - instruction_template: | - ## Previous Response (superviseからの差し戻し時) - {previous_response} - - レビュー依頼を分析し、レビュー方針を立ててください。 - - **これはレビュー専用ピースです。** コード編集は行いません。 - 以下に集中してください: - 1. レビュー対象のファイル/モジュールを特定 - 2. レビューの重点領域を決定(アーキテクチャ、セキュリティ、AIパターン等) - 3. 依頼に記載された特定の懸念事項を整理 - - **PR番号が記載されている場合**(例: "PR #42")、レビュアーが - PRの変更ファイルに集中できるよう計画に含めてください。 - - name: reviewers - parallel: - - name: arch-review - edit: false - persona: architecture-reviewer - policy: review - knowledge: architecture - allowed_tools: - - Read - - Glob - - Grep - - WebSearch - - WebFetch - rules: - - condition: approved - - condition: needs_fix - instruction: review-arch - output_contracts: - report: - - name: 01-architect-review.md - format: architecture-review - - name: security-review - edit: false - persona: security-reviewer - policy: review - knowledge: security - allowed_tools: - - Read - - Glob - - Grep - - WebSearch - - WebFetch - rules: - - condition: approved - - condition: needs_fix - instruction: review-security - output_contracts: - report: - - name: 02-security-review.md - format: security-review - - name: ai-review - edit: false - persona: ai-antipattern-reviewer - policy: - - review - - ai-antipattern - allowed_tools: - - Read - - Glob - - Grep - - WebSearch - - WebFetch - rules: - - condition: approved - - condition: needs_fix - instruction: review-ai - output_contracts: - report: - - name: 03-ai-review.md - format: ai-review - rules: - - condition: all("approved") - next: supervise - - condition: any("needs_fix") - next: supervise - - name: supervise - edit: false - persona: supervisor - policy: review - allowed_tools: - - Read - - Glob - - Grep - - WebSearch - - WebFetch - pass_previous_response: false - rules: - - condition: approved, PR comment requested - next: pr-comment - - condition: approved - next: COMPLETE - - condition: rejected - next: ABORT - instruction_template: | - ## レビュー結果 - {previous_response} - - **これはレビュー専用ピースです。** テスト実行やビルドは行わないでください。 - レビュー結果を統合し、最終サマリーを作成する役割です。 - - **やること:** - 1. Report Directory内の全レビューレポートを読む - 2. アーキテクチャ・セキュリティ・AIレビューの結果を統合 - 3. 統合レビューサマリーと総合判定を作成 - 4. ルーティング判断: - - タスクにPRへのコメント投稿が含まれている場合 - (例: "PRにコメントして"、"PRにレビュー結果を投稿") - → `pr-comment` ムーブメントへ(condition: "approved, PR comment requested") - - ローカルレビューのみ → COMPLETE(condition: "approved") - - 重大な問題が見つかった場合 → ABORT(condition: "rejected") - - **Review Summary出力契約:** - ```markdown - # レビューサマリー - - ## 総合判定: APPROVE / REJECT - - ## サマリー - {2-3文で全レビュー結果を統合} - - ## レビュー結果 - | レビュー | 結果 | 主要な発見 | - |---------|------|-----------| - | アーキテクチャ | APPROVE/REJECT | {概要} | - | セキュリティ | APPROVE/REJECT | {概要} | - | AIアンチパターン | APPROVE/REJECT | {概要} | - - ## 要注意の問題 - | # | 重大度 | ソース | 場所 | 問題 | - |---|--------|--------|------|------| - | 1 | High | セキュリティ | `file:line` | 説明 | - - ## 改善提案 - - {全レビューからの統合提案} - ``` - output_contracts: - report: - - name: review-summary.md - format: | - ```markdown - # レビューサマリー - - ## 総合判定: APPROVE / REJECT - - ## サマリー - {2-3文で全レビュー結果を統合} - - ## レビュー結果 - | レビュー | 結果 | 主要な発見 | - |---------|------|-----------| - | アーキテクチャ | APPROVE/REJECT | {概要} | - | セキュリティ | APPROVE/REJECT | {概要} | - | AIアンチパターン | APPROVE/REJECT | {概要} | - - ## 今回の指摘(new) - | # | finding_id | 重大度 | ソース | 場所 | 問題 | 修正案 | - |---|------------|--------|--------|------|------|--------| - | 1 | SUM-NEW-src-file-L42 | High | セキュリティ | `file:line` | 説明 | 提案 | - - ## 継続指摘(persists) - | # | finding_id | ソース | 前回根拠 | 今回根拠 | 問題 | - |---|------------|--------|----------|----------|------| - | 1 | SUM-PERSIST-src-file-L77 | アーキテクチャ | `file:line` | `file:line` | 説明 | - - ## 解消済み(resolved) - | finding_id | ソース | 解消根拠 | - |------------|--------|----------| - | SUM-RESOLVED-src-file-L10 | QA | `file:line` | - - ## 改善提案 - - {全レビューからの統合提案} - - ## REJECT判定条件 - - `new` または `persists` が1件以上ある場合のみ REJECT 可 - - `finding_id` なしの指摘は無効 - ``` - - name: pr-comment - edit: false - persona: pr-commenter - allowed_tools: - - Read - - Glob - - Grep - - Bash - rules: - - condition: コメント投稿完了 - next: COMPLETE - - condition: コメント投稿失敗 - next: COMPLETE - instruction_template: | - ## レビューサマリー - {previous_response} - - レビュー結果をPRにコメントとして投稿してください。 - - **手順:** - 1. タスク説明からPR番号を抽出 - 2. Report Directory内の全レビューレポートを読む: - - `01-architect-review.md`(アーキテクチャレビュー) - - `02-security-review.md`(セキュリティレビュー) - - `03-ai-review.md`(AIアンチパターンレビュー) - - `review-summary.md`(統合サマリー) - 3. 重要度でフィルタリングし、Critical/High/Mediumの指摘をインラインコメントとして投稿 - 4. 以下のフォーマットでサマリーコメントを投稿: - - ``` - ## 自動レビューサマリー - - {review-summary.mdからの総合判定とサマリー} - - ### レビュー結果 - | レビュー | 結果 | - |---------|------| - | アーキテクチャ | {結果} | - | セキュリティ | {結果} | - | AIアンチパターン | {結果} | - - ### 主要な発見 - {重要な指摘のリスト} - - ### 改善提案 - {統合された提案} - - --- - *[takt](https://github.com/toruticas/takt) review-only ピースで生成* - ``` diff --git a/src/__tests__/it-piece-loader.test.ts b/src/__tests__/it-piece-loader.test.ts index 448b6dd..9ce90a1 100644 --- a/src/__tests__/it-piece-loader.test.ts +++ b/src/__tests__/it-piece-loader.test.ts @@ -281,11 +281,11 @@ describe('Piece Loader IT: piece config validation', () => { expect(movementNames).toContain(config!.initialMovement); }); - it('should preserve edit property on movements (review-only has no edit: true)', () => { - const config = loadPiece('review-only', testDir); + it('should preserve edit property on movements (pr-review has no edit: true)', () => { + const config = loadPiece('pr-review', testDir); expect(config).not.toBeNull(); - // review-only: no movement should have edit: true + // pr-review: no movement should have edit: true for (const movement of config!.movements) { expect(movement.edit).not.toBe(true); if (movement.parallel) { diff --git a/src/__tests__/it-piece-patterns.test.ts b/src/__tests__/it-piece-patterns.test.ts index c320cca..083ad7a 100644 --- a/src/__tests__/it-piece-patterns.test.ts +++ b/src/__tests__/it-piece-patterns.test.ts @@ -292,7 +292,7 @@ describe('Piece Patterns IT: magi piece', () => { }); }); -describe('Piece Patterns IT: review-only piece', () => { +describe('Piece Patterns IT: pr-review piece', () => { let testDir: string; beforeEach(() => { @@ -305,28 +305,30 @@ describe('Piece Patterns IT: review-only piece', () => { rmSync(testDir, { recursive: true, force: true }); }); - it('should complete: plan → reviewers (all approved) → supervise → COMPLETE', async () => { - const config = loadPiece('review-only', testDir); + it('should complete: gather → reviewers (all approved) → supervise → COMPLETE', async () => { + const config = loadPiece('pr-review', testDir); expect(config).not.toBeNull(); setMockScenario([ - { persona: 'planner', status: 'done', content: '[PLAN:1]\n\nReview scope is clear.' }, - // Parallel reviewers: all approved + { persona: 'planner', status: 'done', content: '[GATHER:1]\n\nPR info gathered.' }, + // Parallel reviewers: all approved (5 reviewers) { persona: 'architecture-reviewer', status: 'done', content: '[ARCH-REVIEW:1]\n\napproved' }, { persona: 'security-reviewer', status: 'done', content: '[SECURITY-REVIEW:1]\n\napproved' }, - { persona: 'ai-antipattern-reviewer', status: 'done', content: '[AI-REVIEW:1]\n\napproved' }, - // Supervisor: approved (local review, no PR) - { persona: 'supervisor', status: 'done', content: '[SUPERVISE:2]\n\napproved' }, + { persona: 'qa-reviewer', status: 'done', content: '[QA-REVIEW:1]\n\napproved' }, + { persona: 'testing-reviewer', status: 'done', content: '[TESTING-REVIEW:1]\n\napproved' }, + { persona: 'requirements-reviewer', status: 'done', content: '[REQUIREMENTS-REVIEW:1]\n\napproved' }, + // Supervisor: synthesis complete + { persona: 'supervisor', status: 'done', content: '[SUPERVISE:1]\n\nReview synthesis complete' }, ]); - const engine = createEngine(config!, testDir, 'Review the codebase'); + const engine = createEngine(config!, testDir, 'Review PR #42'); const state = await engine.run(); expect(state.status).toBe('completed'); }); it('should verify no movements have edit: true', () => { - const config = loadPiece('review-only', testDir); + const config = loadPiece('pr-review', testDir); expect(config).not.toBeNull(); for (const movement of config!.movements) { diff --git a/src/__tests__/piece-category-config.test.ts b/src/__tests__/piece-category-config.test.ts index 0b1c44e..66f5384 100644 --- a/src/__tests__/piece-category-config.test.ts +++ b/src/__tests__/piece-category-config.test.ts @@ -166,7 +166,7 @@ piece_categories: - nested Review: pieces: - - review-only + - pr-review - e2e-test show_others_category: true others_category_name: Others @@ -200,7 +200,7 @@ others_category_name: Unclassified { name: 'Child', pieces: ['nested'], children: [] }, ], }, - { name: 'Review', pieces: ['review-only', 'e2e-test'], children: [] }, + { name: 'Review', pieces: ['pr-review', 'e2e-test'], children: [] }, ], }, ]); @@ -212,7 +212,7 @@ others_category_name: Unclassified { name: 'Child', pieces: ['nested'], children: [] }, ], }, - { name: 'Review', pieces: ['review-only', 'e2e-test'], children: [] }, + { name: 'Review', pieces: ['pr-review', 'e2e-test'], children: [] }, ]); expect(config!.userPieceCategories).toEqual([ { name: 'Main', pieces: ['custom'], children: [] }, @@ -230,14 +230,14 @@ others_category_name: Unclassified piece_categories: レビュー: pieces: - - review-only + - pr-review - e2e-test `); const config = getPieceCategories(testDir); expect(config).not.toBeNull(); expect(config!.pieceCategories).toEqual([ - { name: 'レビュー', pieces: ['review-only', 'e2e-test'], children: [] }, + { name: 'レビュー', pieces: ['pr-review', 'e2e-test'], children: [] }, ]); }); @@ -249,7 +249,7 @@ piece_categories: - default Review: pieces: - - review-only + - pr-review show_others_category: true others_category_name: Others `); @@ -263,11 +263,11 @@ others_category_name: Unclassified expect(config).not.toBeNull(); expect(config!.pieceCategories).toEqual([ { name: 'Main', pieces: ['default'], children: [] }, - { name: 'Review', pieces: ['review-only'], children: [] }, + { name: 'Review', pieces: ['pr-review'], children: [] }, ]); expect(config!.builtinPieceCategories).toEqual([ { name: 'Main', pieces: ['default'], children: [] }, - { name: 'Review', pieces: ['review-only'], children: [] }, + { name: 'Review', pieces: ['pr-review'], children: [] }, ]); expect(config!.userPieceCategories).toEqual([]); expect(config!.hasUserCategories).toBe(false); @@ -375,7 +375,7 @@ describe('buildCategorizedPieces', () => { const allPieces = createPieceMap([ { name: 'custom', source: 'user' }, { name: 'default', source: 'builtin' }, - { name: 'review-only', source: 'builtin' }, + { name: 'pr-review', source: 'builtin' }, { name: 'extra', source: 'builtin' }, ]); const config = { @@ -386,13 +386,13 @@ describe('buildCategorizedPieces', () => { pieces: [], children: [ { name: 'Quick Start', pieces: ['default'], children: [] }, - { name: 'Review', pieces: ['review-only'], children: [] }, + { name: 'Review', pieces: ['pr-review'], children: [] }, ], }, ], builtinPieceCategories: [ { name: 'Quick Start', pieces: ['default'], children: [] }, - { name: 'Review', pieces: ['review-only'], children: [] }, + { name: 'Review', pieces: ['pr-review'], children: [] }, ], userPieceCategories: [ { name: 'My Team', pieces: ['custom'], children: [] }, @@ -410,7 +410,7 @@ describe('buildCategorizedPieces', () => { pieces: [], children: [ { name: 'Quick Start', pieces: ['default'], children: [] }, - { name: 'Review', pieces: ['review-only'], children: [] }, + { name: 'Review', pieces: ['pr-review'], children: [] }, ], }, { name: 'Others', pieces: ['extra'], children: [] }, diff --git a/src/__tests__/review-only-piece.test.ts b/src/__tests__/review-only-piece.test.ts index ee741c3..89b5d3a 100644 --- a/src/__tests__/review-only-piece.test.ts +++ b/src/__tests__/review-only-piece.test.ts @@ -1,12 +1,12 @@ /** - * Tests for review-only piece + * Tests for pr-review piece * * Covers: * - Piece YAML files (EN/JA) load and pass schema validation - * - Piece structure: plan -> reviewers (parallel) -> supervise -> pr-comment + * - Piece structure: gather -> reviewers (parallel 5) -> supervise -> COMPLETE * - All movements have edit: false - * - pr-commenter persona has Bash in allowed_tools - * - Routing rules for local vs PR comment flows + * - All 5 reviewers have Bash in allowed_tools + * - Routing rules for gather and reviewers */ import { describe, it, expect } from 'vitest'; @@ -17,14 +17,14 @@ import { PieceConfigRawSchema } from '../core/models/index.js'; const RESOURCES_DIR = join(import.meta.dirname, '../../builtins'); -function loadReviewOnlyYaml(lang: 'en' | 'ja') { - const filePath = join(RESOURCES_DIR, lang, 'pieces', 'review-only.yaml'); +function loadPrReviewYaml(lang: 'en' | 'ja') { + const filePath = join(RESOURCES_DIR, lang, 'pieces', 'pr-review.yaml'); const content = readFileSync(filePath, 'utf-8'); return parseYaml(content); } -describe('review-only piece (EN)', () => { - const raw = loadReviewOnlyYaml('en'); +describe('pr-review piece (EN)', () => { + const raw = loadPrReviewYaml('en'); it('should pass schema validation', () => { const result = PieceConfigRawSchema.safeParse(raw); @@ -32,17 +32,17 @@ describe('review-only piece (EN)', () => { }); it('should have correct name and initial_movement', () => { - expect(raw.name).toBe('review-only'); - expect(raw.initial_movement).toBe('plan'); + expect(raw.name).toBe('pr-review'); + expect(raw.initial_movement).toBe('gather'); }); it('should have max_movements of 10', () => { expect(raw.max_movements).toBe(10); }); - it('should have 4 movements: plan, reviewers, supervise, pr-comment', () => { + it('should have 3 movements: gather, reviewers, supervise', () => { const movementNames = raw.movements.map((s: { name: string }) => s.name); - expect(movementNames).toEqual(['plan', 'reviewers', 'supervise', 'pr-comment']); + expect(movementNames).toEqual(['gather', 'reviewers', 'supervise']); }); it('should have all movements with edit: false', () => { @@ -60,13 +60,19 @@ describe('review-only piece (EN)', () => { } }); - it('should have reviewers movement with 3 parallel sub-movements', () => { + it('should have reviewers movement with 5 parallel sub-movements', () => { const reviewers = raw.movements.find((s: { name: string }) => s.name === 'reviewers'); expect(reviewers).toBeDefined(); - expect(reviewers.parallel).toHaveLength(3); + expect(reviewers.parallel).toHaveLength(5); const subNames = reviewers.parallel.map((s: { name: string }) => s.name); - expect(subNames).toEqual(['arch-review', 'security-review', 'ai-review']); + expect(subNames).toEqual([ + 'arch-review', + 'security-review', + 'qa-review', + 'testing-review', + 'requirements-review', + ]); }); it('should have reviewers movement with aggregate rules', () => { @@ -78,42 +84,19 @@ describe('review-only piece (EN)', () => { expect(reviewers.rules[1].next).toBe('supervise'); }); - it('should have supervise movement with routing rules for local and PR flows', () => { + it('should have supervise movement with single rule to COMPLETE', () => { const supervise = raw.movements.find((s: { name: string }) => s.name === 'supervise'); - expect(supervise.rules).toHaveLength(3); - - const conditions = supervise.rules.map((r: { condition: string }) => r.condition); - expect(conditions).toContain('approved, PR comment requested'); - expect(conditions).toContain('approved'); - expect(conditions).toContain('rejected'); - - const prRule = supervise.rules.find((r: { condition: string }) => r.condition === 'approved, PR comment requested'); - expect(prRule.next).toBe('pr-comment'); - - const localRule = supervise.rules.find((r: { condition: string }) => r.condition === 'approved'); - expect(localRule.next).toBe('COMPLETE'); - - const rejectRule = supervise.rules.find((r: { condition: string }) => r.condition === 'rejected'); - expect(rejectRule.next).toBe('ABORT'); + expect(supervise.rules).toHaveLength(1); + expect(supervise.rules[0].condition).toBe('Review synthesis complete'); + expect(supervise.rules[0].next).toBe('COMPLETE'); }); - it('should have pr-comment movement with Bash in allowed_tools', () => { - const prComment = raw.movements.find((s: { name: string }) => s.name === 'pr-comment'); - expect(prComment).toBeDefined(); - expect(prComment.allowed_tools).toContain('Bash'); + it('should have gather movement using planner persona', () => { + const gather = raw.movements.find((s: { name: string }) => s.name === 'gather'); + expect(gather.persona).toBe('planner'); }); - it('should have pr-comment movement using pr-commenter persona', () => { - const prComment = raw.movements.find((s: { name: string }) => s.name === 'pr-comment'); - expect(prComment.persona).toBe('pr-commenter'); - }); - - it('should have plan movement reusing planner persona', () => { - const plan = raw.movements.find((s: { name: string }) => s.name === 'plan'); - expect(plan.persona).toBe('planner'); - }); - - it('should have supervise movement reusing supervisor persona', () => { + it('should have supervise movement using supervisor persona', () => { const supervise = raw.movements.find((s: { name: string }) => s.name === 'supervise'); expect(supervise.persona).toBe('supervisor'); }); @@ -129,16 +112,22 @@ describe('review-only piece (EN)', () => { } }); - it('reviewer sub-movements should not have Bash in allowed_tools', () => { + it('should have Bash in allowed_tools for all 5 reviewers', () => { const reviewers = raw.movements.find((s: { name: string }) => s.name === 'reviewers'); for (const sub of reviewers.parallel) { - expect(sub.allowed_tools).not.toContain('Bash'); + expect(sub.allowed_tools).toContain('Bash'); } }); + + it('should have gather movement with output_contracts for PR info', () => { + const gather = raw.movements.find((s: { name: string }) => s.name === 'gather'); + expect(gather.output_contracts).toBeDefined(); + expect(gather.output_contracts.report[0].name).toBe('00-pr-info.md'); + }); }); -describe('review-only piece (JA)', () => { - const raw = loadReviewOnlyYaml('ja'); +describe('pr-review piece (JA)', () => { + const raw = loadPrReviewYaml('ja'); it('should pass schema validation', () => { const result = PieceConfigRawSchema.safeParse(raw); @@ -146,21 +135,27 @@ describe('review-only piece (JA)', () => { }); it('should have correct name and initial_movement', () => { - expect(raw.name).toBe('review-only'); - expect(raw.initial_movement).toBe('plan'); + expect(raw.name).toBe('pr-review'); + expect(raw.initial_movement).toBe('gather'); }); it('should have same movement structure as EN version', () => { const movementNames = raw.movements.map((s: { name: string }) => s.name); - expect(movementNames).toEqual(['plan', 'reviewers', 'supervise', 'pr-comment']); + expect(movementNames).toEqual(['gather', 'reviewers', 'supervise']); }); - it('should have reviewers movement with 3 parallel sub-movements', () => { + it('should have reviewers movement with 5 parallel sub-movements', () => { const reviewers = raw.movements.find((s: { name: string }) => s.name === 'reviewers'); - expect(reviewers.parallel).toHaveLength(3); + expect(reviewers.parallel).toHaveLength(5); const subNames = reviewers.parallel.map((s: { name: string }) => s.name); - expect(subNames).toEqual(['arch-review', 'security-review', 'ai-review']); + expect(subNames).toEqual([ + 'arch-review', + 'security-review', + 'qa-review', + 'testing-review', + 'requirements-review', + ]); }); it('should have all movements with edit: false or undefined', () => { @@ -174,9 +169,11 @@ describe('review-only piece (JA)', () => { } }); - it('should have pr-comment movement with Bash in allowed_tools', () => { - const prComment = raw.movements.find((s: { name: string }) => s.name === 'pr-comment'); - expect(prComment.allowed_tools).toContain('Bash'); + it('should have Bash in allowed_tools for all 5 reviewers', () => { + const reviewers = raw.movements.find((s: { name: string }) => s.name === 'reviewers'); + for (const sub of reviewers.parallel) { + expect(sub.allowed_tools).toContain('Bash'); + } }); it('should have same aggregate rules on reviewers', () => { @@ -185,66 +182,3 @@ describe('review-only piece (JA)', () => { expect(reviewers.rules[1].condition).toBe('any("needs_fix")'); }); }); - -describe('pr-commenter persona files', () => { - it('should exist for EN with domain knowledge', () => { - const filePath = join(RESOURCES_DIR, 'en', 'facets', 'personas', 'pr-commenter.md'); - const content = readFileSync(filePath, 'utf-8'); - expect(content).toContain('PR Commenter'); - expect(content).toContain('gh api'); - expect(content).toContain('gh pr comment'); - }); - - it('should exist for JA with domain knowledge', () => { - const filePath = join(RESOURCES_DIR, 'ja', 'facets', 'personas', 'pr-commenter.md'); - const content = readFileSync(filePath, 'utf-8'); - expect(content).toContain('PR Commenter'); - expect(content).toContain('gh api'); - expect(content).toContain('gh pr comment'); - }); - - it('should NOT contain piece-specific report names (EN)', () => { - const filePath = join(RESOURCES_DIR, 'en', 'facets', 'personas', 'pr-commenter.md'); - const content = readFileSync(filePath, 'utf-8'); - // Persona should not reference specific review-only piece report files - expect(content).not.toContain('01-architect-review.md'); - expect(content).not.toContain('02-security-review.md'); - expect(content).not.toContain('03-ai-review.md'); - expect(content).not.toContain('04-review-summary.md'); - // Persona should not reference specific reviewer names from review-only piece - expect(content).not.toContain('Architecture review report'); - expect(content).not.toContain('Security review report'); - expect(content).not.toContain('AI antipattern review report'); - }); - - it('should NOT contain piece-specific report names (JA)', () => { - const filePath = join(RESOURCES_DIR, 'ja', 'facets', 'personas', 'pr-commenter.md'); - const content = readFileSync(filePath, 'utf-8'); - expect(content).not.toContain('01-architect-review.md'); - expect(content).not.toContain('02-security-review.md'); - expect(content).not.toContain('03-ai-review.md'); - expect(content).not.toContain('04-review-summary.md'); - }); -}); - -describe('pr-comment instruction_template contains piece-specific procedures', () => { - it('EN: should reference specific report files', () => { - const raw = loadReviewOnlyYaml('en'); - const prComment = raw.movements.find((s: { name: string }) => s.name === 'pr-comment'); - const template = prComment.instruction_template; - expect(template).toContain('01-architect-review.md'); - expect(template).toContain('02-security-review.md'); - expect(template).toContain('03-ai-review.md'); - expect(template).toContain('review-summary.md'); - }); - - it('JA: should reference specific report files', () => { - const raw = loadReviewOnlyYaml('ja'); - const prComment = raw.movements.find((s: { name: string }) => s.name === 'pr-comment'); - const template = prComment.instruction_template; - expect(template).toContain('01-architect-review.md'); - expect(template).toContain('02-security-review.md'); - expect(template).toContain('03-ai-review.md'); - expect(template).toContain('review-summary.md'); - }); -});