update: revise skill files
- Update skills/code-review/SKILL.md - Update skills/plan/SKILL.md
This commit is contained in:
toki
parent
182dcae5bf
commit
0703373a30
2 changed files with 51 additions and 35 deletions
|
|
@ -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 <old_name>` 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.
|
||||
|
|
|
|||
|
|
@ -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
|
||||
# <Title>
|
||||
> 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`.
|
||||
|
|
|
|||
Loading…
Reference in a new issue