diff --git a/skills/code-review/SKILL.md b/skills/code-review/SKILL.md index 8964a81..e208cec 100644 --- a/skills/code-review/SKILL.md +++ b/skills/code-review/SKILL.md @@ -1,6 +1,6 @@ --- name: code-review -description: Review completed implementation work for toki_socket. Reads PLAN.md and CODE_REVIEW.md from the active task, reviews the actual changed source files, appends a verdict to CODE_REVIEW.md, then archives both files as .log. If Required issues are found, writes a new PLAN.md and CODE_REVIEW.md stub so the loop continues immediately. +description: Review completed implementation work for toki_socket. Reads PLAN.md and CODE_REVIEW.md from the active task, reviews the actual changed source files, appends a verdict to CODE_REVIEW.md, then archives both files as .log. If any issues are found (WARN or FAIL), writes a new PLAN.md and CODE_REVIEW.md stub so the loop continues immediately. --- # Code Review @@ -29,7 +29,7 @@ Note: `PLAN.md` and `CODE_REVIEW.md` are always present together. The implementi ## Step 1 — Find the Active Task 1. Glob `tasks/*/CODE_REVIEW.md`. -2. If exactly one exists: that is the active task. Record `task_name`. Confirm `PLAN.md` is also present in the same directory. +2. If exactly one exists: that is the active task. Record `task_name`. `PLAN.md` is always present alongside `CODE_REVIEW.md` — this is an invariant guaranteed by the plan skill and code-review skill. No existence check needed. 3. If no `CODE_REVIEW.md` found: nothing to review. Stop and report. 4. If multiple: list them and ask the user which to review. Stop. @@ -53,39 +53,48 @@ No git narrowing. Read everything relevant to the plan: 2. `tasks/{task_name}/PLAN.md` 3. Every source file listed in the plan's "수정 파일 및 체크리스트" — read the whole file. 4. Every test file that exercises the changed code. -5. Every file that imports or is imported by the changed files. +5. Files that import or are imported by the changed files — **up to 2 levels deep**. ### 2-2B. Follow-up review — diff-anchored + expansion -Start with git to identify what changed, then expand outward to connected code. +Start with git unstaged changes to identify what changed, then expand outward to connected code. + +The implementing agent does not commit — changes are always in the working tree (unstaged or staged). **Collect the diff:** ```bash -git rev-parse HEAD 2>/dev/null && git diff HEAD || (git diff && git diff --cached) -git diff main..HEAD # if on a feature branch -git log --oneline -10 +git diff # unstaged changes +git diff --cached # staged changes +git log --oneline -5 ``` +Use the union of both as the change set. + **Expand beyond the diff** — for each changed file or symbol, also read: -- Files that **import** the changed file (callers may be broken by signature changes) +- Files that **import** the changed file — **up to 2 levels deep** (callers may be broken by signature changes) - Files that **implement or extend** a changed interface or abstract class - Test files that **exercise** the changed logic, even if not modified themselves - Any file the plan's "수정 파일 및 체크리스트" lists that does not appear in the diff (planned but possibly missing) The diff scopes where to start; it does not limit where to stop. Follow the connections. +> Note: import expansion boundary follows the project's structural rules (e.g., DDD layer rules). If a separate rules file defines allowed dependency directions, use it as the ceiling for how far to trace. + --- ## Step 3 — Pre-Review Checklist -Complete every item before writing the verdict. Use the git diff as the primary reference — only reach outside it when a specific check demands it. +Complete every item before writing the verdict. -- [ ] Compare git diff against the plan's "수정 파일 및 체크리스트" — are all planned changes present? Are there unplanned changes? -- [ ] For every symbol renamed or removed in the diff: `git grep ` on the pre-change tree (or Grep on current tree) to confirm no stale references remain outside the diff. -- [ ] For every new test added in the diff: confirm the test name matches the plan, and the assertion is non-trivial (not just "no exception thrown"). -- [ ] Scan the diff for code quality issues: debug prints, dead code, leftover TODOs, commented-out blocks. -- [ ] Check for unintended diff noise: formatting-only changes, unrelated file edits, accidentally committed debug code. -- [ ] Cross-check the claimed `dart analyze` / `go test` output in CODE_REVIEW.md against what the diff actually introduces. +**Applies to all reviews:** +- [ ] Compare actual source files against the plan's "수정 파일 및 체크리스트" — are all planned changes present in the code? +- [ ] For every symbol renamed or removed: Grep on the current working tree to confirm no stale references remain. +- [ ] For every new test listed in the plan: confirm it exists, name matches, assertion is non-trivial. +- [ ] Cross-check the claimed `dart analyze` / `go test` output in CODE_REVIEW.md against the actual code. + +**Follow-up review only** (count ≥ 1, diff available): +- [ ] Compare `git diff` + `git diff --cached` against the plan's checklist — are there unplanned changes or missing changes? +- [ ] Scan the diff for noise: debug prints, dead code, leftover TODOs, formatting-only changes, unrelated edits. --- @@ -125,7 +134,8 @@ Append the following section to the existing `CODE_REVIEW.md`. Every field is re **[PASS]** 추가 작업 불필요. 아카이브 완료. -**[FIX]** Required 문제 N건. 새 PLAN.md 작성 완료. 구현 에이전트는 PLAN.md를 읽고 작업 시작. +**[WARN]** Suggested/Nit N건. 새 PLAN.md 작성 완료. 구현 에이전트는 PLAN.md를 읽고 작업 시작. +**[FAIL]** Required N건. 새 PLAN.md 작성 완료. 구현 에이전트는 PLAN.md를 읽고 작업 시작. ``` --- @@ -148,14 +158,16 @@ After this step neither `PLAN.md` nor `CODE_REVIEW.md` exists. ## Step 6 — Post-Review Actions -### If verdict is PASS (zero Required issues) +### If verdict is PASS (zero issues) No further files needed. Report to the user: - Verdict: PASS - Archives: `code_review_N.log`, `plan_M.log` - Task status: complete (only `.log` files remain in `tasks/{task_name}/`) -### If verdict is WARN or FAIL (one or more Required issues) +### If verdict is WARN or FAIL + +WARN = Suggested/Nit issues only, zero Required. FAIL = one or more Required issues. Both unconditionally write new PLAN.md and CODE_REVIEW.md stub. Write new files for the fix round: @@ -169,7 +181,7 @@ Write new files for the fix round: - **수정 파일 및 체크리스트**: exhaustive checkbox list. - **테스트 작성**: decision + justification. - **중간 검증**: runnable command. -- Suggested/Nit issues may be grouped into one optional item. +- If verdict is WARN (Suggested/Nit issues, no Required): group all Suggested and Nit issues into one plan item. Include it in the plan — it is not optional. - Include 수정 파일 요약 table. - Include 최종 검증 section. @@ -179,7 +191,7 @@ Write new files for the fix round: - Empty sections: 계획 대비 변경 사항, 주요 설계 결정, 리뷰어 체크포인트, 검증 결과. Report to the user: -- Verdict: WARN/FAIL, N Required issues +- Verdict: FAIL → Required N건 / WARN → Suggested/Nit N건 - Archives: `code_review_N.log`, `plan_M.log` - New plan: `tasks/{task_name}/PLAN.md` ready for implementing agent @@ -220,5 +232,5 @@ Report to the user: 2. `plan_M.log` exists (was PLAN.md). 3. Neither `CODE_REVIEW.md` nor `PLAN.md` exists as `.md` in the task directory. 4. If PASS: only `.log` files remain in `tasks/{task_name}/`. -5. If FAIL: new `PLAN.md` exists, header `plan=N` matches count of `plan_*.log` after archiving, covers every Required issue with full sub-sections. -6. If FAIL: new `CODE_REVIEW.md` stub exists, header `plan=N` matches new PLAN.md, completion table lists every item. +5. If WARN or FAIL: new `PLAN.md` exists, header `plan=N` matches count of `plan_*.log` after archiving. FAIL: every Required issue has its own plan item with full sub-sections. WARN: grouped Suggested/Nit plan item exists with full sub-sections. +6. If WARN or FAIL: new `CODE_REVIEW.md` stub exists, header `plan=N` matches new PLAN.md, completion table lists every item. diff --git a/skills/plan/SKILL.md b/skills/plan/SKILL.md index 8877890..19ddd58 100644 --- a/skills/plan/SKILL.md +++ b/skills/plan/SKILL.md @@ -10,7 +10,7 @@ description: Analyze the codebase and write a detailed PLAN.md for toki_socket i This skill produces two files that drive the implementation loop: ``` -implement skill (this) → PLAN.md + CODE_REVIEW.md stub +plan skill (this) → PLAN.md + CODE_REVIEW.md stub ↓ Separate implementing agent → reads PLAN.md, codes, fills CODE_REVIEW.md ↓ @@ -21,7 +21,7 @@ All files live under `tasks/{task_name}/`. Directory state at any point: | State | Meaning | |-------|---------| -| `PLAN.md` only (no `CODE_REVIEW.md`) | Should not occur — implement skill always writes both | +| `PLAN.md` only (no `CODE_REVIEW.md`) | Should not occur — plan skill always writes both | | `PLAN.md` + `CODE_REVIEW.md` (stub, unfilled) | Implementing agent is working | | `PLAN.md` + `CODE_REVIEW.md` (filled) | Ready for code-review skill | | Only `*.log` files | Task complete | @@ -30,16 +30,17 @@ All files live under `tasks/{task_name}/`. Directory state at any point: ## Step 1 — Determine Task -### New task +If the user names the task explicitly, use that name and skip auto-detection. -1. Derive `task_name`: short snake_case from the user request (e.g., `dart_api_refactor`, `go_base_client`). -2. Create `tasks/{task_name}/` if it does not exist. +Otherwise, glob `tasks/*/PLAN.md` and branch: -### Continuing an existing task (re-plan triggered by code-review) +| Result | Action | +|--------|--------| +| Exactly one found | Continuing task — use that directory. `task_name` = directory name. | +| None found | New task — derive `task_name` from the user request (short snake_case, e.g. `dart_api_refactor`). Create `tasks/{task_name}/`. | +| Multiple found | Cannot auto-detect — list the found paths and ask the user which task to work on. Stop. | -1. Glob `tasks/*/PLAN.md` — if exactly one exists, that is the active task. Use its directory. -2. If none but `tasks/*/CODE_REVIEW.md` exists: the task is waiting for `code-review`, not this skill. Stop and report. -3. If the user names the task explicitly, use that name regardless. +> PLAN.md is the entry point of the loop. Without it the loop has not started. --- @@ -73,12 +74,15 @@ Before writing new files, archive any active files from a previous round: ## Step 4 — Write PLAN.md +**N for the header** = count of `tasks/{task_name}/plan_*.log` files after Step 3 (i.e., including the one just archived, if any). + Write `tasks/{task_name}/PLAN.md`: -```markdown -# +> Note: Include the "의존 관계 및 구현 순서" section only when plan items must be done in a specific order. Omit it entirely when items are independent. +```markdown <!-- task={task_name} plan=N tag=<TAG> --> +# <Title> ## 이 파일을 읽는 구현 에이전트에게 @@ -148,7 +152,7 @@ Expected: <N개 통과> --- -## 의존 관계 및 구현 순서 ← 순서가 있을 때만 포함 +## 의존 관계 및 구현 순서 \``` [TAG-1] → [TAG-2] → [TAG-3] @@ -244,7 +248,7 @@ task={task_name}, plan=N, tag=TAG After writing both files: -1. `tasks/{task_name}/PLAN.md` exists and line 1 is `<!-- task={task_name} plan=N tag=TAG -->`. +1. `tasks/{task_name}/PLAN.md` exists and **line 1** is `<!-- task={task_name} plan=N tag=TAG -->`. 2. `tasks/{task_name}/CODE_REVIEW.md` exists and line 1 matches `<!-- task={task_name} plan=N tag=TAG -->`. 3. Previous `PLAN.md` (if existed) was archived as `plan_N.log` with N = count of pre-existing logs. 4. Previous `CODE_REVIEW.md` (if existed) was archived as `code_review_M.log`.