336 lines
22 KiB
Markdown
336 lines
22 KiB
Markdown
---
|
|
name: code-review
|
|
description: Use for active task review requests such as 리뷰 진행해, 리뷰해줘, 코드 리뷰해줘, code review, CODE_REVIEW.md, or 리뷰 루프. Review the active PLAN/CODE_REVIEW pair, append PASS/WARN/FAIL, archive both active files, and create the required next state: PASS writes complete.log and moves the task to archive; WARN/FAIL immediately writes follow-up PLAN/CODE_REVIEW files. Never stop after verdict append.
|
|
---
|
|
|
|
# Code Review
|
|
|
|
## Purpose
|
|
|
|
Review the implementation phase of the plan-code-review loop:
|
|
|
|
```text
|
|
plan skill -> implementation -> code-review skill
|
|
^ |
|
|
+----- issues found: new routed plan/review files
|
|
```
|
|
|
|
## Core Loop Rules
|
|
|
|
- Trigger: Korean or English active-task review requests, including `리뷰 진행해` and `리뷰해줘`, must use this skill when an active `CODE_REVIEW-*-G??.md` exists under `agent-task/*/` or `agent-task/*/*/`, excluding `agent-task/archive/**`.
|
|
- Finalize every selected active review: append one verdict, archive the active review and plan files, then create exactly one next state before reporting.
|
|
- Next state: `PASS` writes `complete.log` and moves the task under `agent-task/archive/YYYY/MM/`; `WARN` or `FAIL` writes the next active `PLAN-{build_lane}-GNN.md` and `CODE_REVIEW-{review_lane}-GNN.md`.
|
|
- Do not ask for confirmation before WARN/FAIL follow-up files. If details are uncertain, write the smallest concrete follow-up plan with file references and verification commands.
|
|
- Recovery: if a prior turn appended a verdict without archive or next-state files, resume at Step 5 and finish finalization first.
|
|
|
|
## Workflow Contract
|
|
|
|
Active work must live under an active task directory using routed filenames. This is the state protocol shared with the plan skill.
|
|
|
|
Task path terms:
|
|
|
|
- `{task_group}` is the top-level work category under `agent-task/`, using a short snake_case name such as `refactoring`.
|
|
- `{subtask_dir}` is used only for split work and follows the indexed directory naming contract, such as `01_core` or `02+01_db`.
|
|
- `{subtask_name}` is the short snake_case name after the index or dependency prefix inside `{subtask_dir}`.
|
|
- `{task_name}` in headers and templates means the active task path relative to `agent-task/`: either `{task_group}` for a single-plan task or `{task_group}/{subtask_dir}` for a split subtask.
|
|
- A single-plan task stores active files directly under `agent-task/{task_group}/`.
|
|
- Split work stores active files under `agent-task/{task_group}/{subtask_dir}/`; the parent `agent-task/{task_group}/` is only the grouping folder and must not contain active plan/review files.
|
|
|
|
Filename rules:
|
|
|
|
- Plan file: `PLAN-{build_lane}-GNN.md`
|
|
- Review file: `CODE_REVIEW-{review_lane}-GNN.md`
|
|
- `{lane}` is only `local` or `cloud`; never put model names in filenames.
|
|
- `GNN` is a two-digit capability grade from `G01` to `G10`; runtime maps lane+grade to current models externally.
|
|
|
|
Multi-plan runtime contract:
|
|
|
|
- Multi-plan work is represented as multiple subtask directories under one shared `{task_group}`. Each subtask directory owns exactly one normal active plan file and one normal active review file.
|
|
- Multi-plan subtask directory names encode runtime scheduling metadata:
|
|
- `NN_{subtask_name}` has no runtime dependencies.
|
|
- `NN+PP[,QQ...]_{subtask_name}` depends on the listed earlier task indices.
|
|
- Subtask directory names are the runtime dependency source of truth. Preserve them verbatim; do not normalize, reinterpret, infer extra dependencies from numeric order, or choose execution order by agent judgment.
|
|
- If the user/runtime names a task group, task path, or subtask directory that identifies exactly one active review file, review that directory even when other active review files exist.
|
|
|
|
Review routing rules:
|
|
|
|
- `local`: narrow, low-risk, or first-pass review where tests and scope are clear.
|
|
- `cloud`: multi-file, API/call-site impact, meaningful test judgment, plan deviation, weak verification, security/auth, storage/migration, concurrency, protocol/schema, cross-domain, or repeated Required issues.
|
|
- `cloud-G07` or higher is mandatory for terminal-agent follow-up work: shell/CLI workflow implementation, bin script orchestration, process control, stdout/stderr parsing, exit-status contracts, long-running command diagnosis, or terminal benchmark-style tasks. Merely running deterministic tests such as `go test` does not make a task terminal-agent work.
|
|
- `cloud-G07` or higher is mandatory for follow-up plans when a local implementation failed a real bin/smoke/integration command after unit tests passed, when the success condition depends on an interactive TUI/PTY/browser/external CLI or screen repaint/cursor stream, or when verification trust is Fail because recorded stdout/stderr does not match a rerun.
|
|
|
|
Directory states:
|
|
|
|
| State | Meaning |
|
|
|-------|---------|
|
|
| `PLAN-*-G??.md` + `CODE_REVIEW-*-G??.md` stub or placeholders | Implementation is pending/incomplete; review should fail completeness if invoked |
|
|
| `PLAN-*-G??.md` + filled `CODE_REVIEW-*-G??.md` | Ready for code-review skill |
|
|
| `complete.log` + `*.log` files | Task complete (PASS), before final task-directory archive move |
|
|
| `agent-task/archive/YYYY/MM/{task_name}/complete.log` + `*.log` files | Archived completed task path (PASS); not active |
|
|
| Only `*.log` files (no `complete.log`) | Task terminated mid-loop or abandoned |
|
|
|
|
The implementing agent never archives or deletes active files; archiving is this skill's responsibility.
|
|
|
|
## Step 1 - Find Active Task
|
|
|
|
Find active review files with both globs, excluding `agent-task/archive/**`:
|
|
|
|
- `agent-task/*/CODE_REVIEW-*-G??.md`
|
|
- `agent-task/*/*/CODE_REVIEW-*-G??.md`
|
|
|
|
| Result | Action |
|
|
|--------|--------|
|
|
| Exactly one | Review that task; exactly one `PLAN-*-G??.md` is expected beside it |
|
|
| None | Nothing to review; stop and report |
|
|
| Multiple | If the user/runtime named a task group, task path, or subtask directory that identifies exactly one active review file, review that directory. Otherwise list paths and ask which task to review; do not choose by agent judgment. |
|
|
|
|
## Step 2 - Load Context
|
|
|
|
Count `agent-task/{task_name}/code_review_*.log` in the selected active task directory:
|
|
|
|
- `0`: first review. Read the active review file, active plan file, every planned source file, related tests, and files importing/imported by changed files up to 2 levels deep.
|
|
- `>=1`: follow-up review. Start with `git diff`, `git diff --cached`, and `git log --oneline -5`, then expand to related callers, implementers, tests, and any planned files missing from the diff.
|
|
|
|
The diff is the starting point, not the boundary. Follow behavior and API connections far enough to judge correctness.
|
|
|
|
## Step 3 - Pre-Review Checklist
|
|
|
|
Before writing the verdict:
|
|
|
|
- Compare actual source files against every planned checklist item.
|
|
- Compare the plan `구현 체크리스트` and review stub `구현 체크리스트`; item text/order must match.
|
|
- Confirm the implementation marked the matching checklist items in the active review file, including the final mandatory `CODE_REVIEW-*-G??.md` completion item.
|
|
- Treat blank placeholder sections, missing actual implementation notes, missing checklist completion, or missing actual stdout/stderr in the active review file as a completeness or verification-trust failure.
|
|
- Grep renamed/removed symbols for stale references.
|
|
- Confirm every required test exists, name matches, and assertions are meaningful.
|
|
- Cross-check claimed verification output in the active review file against actual code and project commands.
|
|
- For follow-up reviews, compare diff against the plan and scan for unplanned changes, debug prints, dead code, TODOs, formatting-only noise, and unrelated edits.
|
|
|
|
## Step 4 - Append Verdict
|
|
|
|
Append `코드리뷰 결과` to the active `CODE_REVIEW-*-G??.md`.
|
|
|
|
Required fields:
|
|
|
|
- `종합 판정`: exactly `PASS`, `WARN`, or `FAIL`.
|
|
- `차원별 평가`: Pass/Warn/Fail for correctness, completeness, test coverage, API contract, code quality, plan deviation, verification trust.
|
|
- `발견된 문제`: `없음`, or bullets using `Required`, `Suggested`, or `Nit` with `file:line` and a concrete fix.
|
|
- `다음 단계`: keep only the matching PASS/WARN/FAIL line.
|
|
|
|
Do not check archive/next-state items in `코드리뷰 전용 체크리스트` during Step 4. Complete the applicable dedicated checklist items in the archived `code_review_*.log` during Step 7, after archive, next-state writes, and PASS task-directory moves are done.
|
|
|
|
Severity semantics:
|
|
|
|
| Verdict | Meaning | Follow-up plan |
|
|
|---------|---------|----------------|
|
|
| `PASS` | No Required/Suggested issues. Nit-only findings may still PASS. | No |
|
|
| `WARN` | One or more Suggested issues, zero Required. | Yes |
|
|
| `FAIL` | One or more Required issues. | Yes |
|
|
|
|
Issue severity:
|
|
|
|
- `Required`: correctness, API contract, missing required test, plan-completeness issue, or incomplete/placeholder `CODE_REVIEW-*-G??.md` content required from the implementing agent.
|
|
- `Suggested`: useful improvement that should enter the loop but does not block correctness.
|
|
- `Nit`: tiny cleanup; may be recorded without forcing WARN.
|
|
|
|
Verdict consistency:
|
|
|
|
- `PASS` requires all dimensions to be Pass and no Required/Suggested issues. Nit-only findings may still PASS only when every dimension remains Pass.
|
|
- Any Fail dimension or any Required issue forces `FAIL`.
|
|
- Any Warn dimension or any Suggested issue forces `WARN`, unless the only findings are explicitly Nit and every dimension remains Pass.
|
|
|
|
## Step 5 - Archive Active Files
|
|
|
|
Archive is mandatory for `PASS`, `WARN`, and `FAIL`. Archive order is fixed:
|
|
|
|
1. Count existing `code_review_*.log` as `N`; rename `CODE_REVIEW-{review_lane}-GNN.md` to `code_review_{review_lane}_GNN_N.log`.
|
|
2. Count existing `plan_*.log` as `M`; rename `PLAN-{build_lane}-GNN.md` to `plan_{build_lane}_GNN_M.log`.
|
|
|
|
After archiving, neither active `.md` file remains unless Step 6 writes a follow-up.
|
|
|
|
## Step 6 - Post-Review Actions
|
|
|
|
For `PASS`, write `agent-task/{task_name}/complete.log` before reporting.
|
|
|
|
Complete log template:
|
|
|
|
- Template path: `agent-ops/skills/common/code-review/templates/complete-log-template.md`
|
|
- Copy the template's section order and fill every placeholder from the archived plan/review logs and final verdict.
|
|
- Do not leave placeholders in `complete.log`.
|
|
- Use `없음` for empty `잔여 Nit` or `후속 작업`.
|
|
- A PASS `complete.log` must not contain unresolved Required or Suggested issues. Nit-only leftovers may be recorded under `잔여 Nit`.
|
|
|
|
For `WARN` or `FAIL`, write new routed plan/review files using the plan skill format:
|
|
|
|
- New plan number is the count of `plan_*.log` after archive.
|
|
- Header tag is `REVIEW_<PARENT_TAG>`.
|
|
- Base the follow-up scope directly on the archived review findings. Keep it narrow and actionable.
|
|
- Choose lane/grade again; preserve the prior route only when it was adequate, otherwise raise `GNN` and/or move `local -> cloud`.
|
|
- Before choosing the follow-up route, apply this escalation gate:
|
|
- If the archived plan was `local-*` and the verdict is `FAIL` for correctness, completeness, test coverage, or verification trust, move the follow-up build lane to `cloud` unless the issue is trivially deterministic and review-detectable without live environment behavior.
|
|
- If the follow-up work is terminal-agent work (shell/CLI workflow implementation, bin script orchestration, process control, stdout/stderr parsing, exit-status contracts, long-running command diagnosis, or terminal benchmark-style tasks), use `cloud-G07` or higher.
|
|
- If unit tests passed but a real bin/smoke/integration command failed, use `cloud-G07` or higher.
|
|
- If the task depends on interactive TUI/PTY/browser/external CLI behavior, screen repaint/cursor stream parsing, or live command-palette state, use `cloud-G07` or higher.
|
|
- If recorded verification output was reconstructed, stale, or mismatched on rerun, use `cloud-G07` or higher and make verification trust recovery a plan item.
|
|
- `FAIL`: one plan item per Required issue.
|
|
- `WARN`: one grouped plan item for Suggested issues, plus related Nit issues if useful.
|
|
- Each plan item needs problem, solution with before/after when non-trivial, checklist, test decision, intermediate verification.
|
|
- The follow-up plan and review stub must contain matching `구현 체크리스트` item text/order, including the final mandatory `CODE_REVIEW-*-G??.md` completion item.
|
|
|
|
Routed review stub template (fill `{…}` placeholders; everything else is fixed and must not be changed by the implementing agent):
|
|
|
|
```markdown
|
|
<!-- task={task_name} plan={N} tag={TAG} -->
|
|
|
|
# Code Review Reference - {TAG}
|
|
|
|
> **[IMPLEMENTING AGENT — READ FIRST] Filling in this file is the mandatory final step of implementation.**
|
|
> The task is NOT complete until every implementation-owned section below is filled in.
|
|
> Complete the `구현 체크리스트`; the final checklist item is mandatory before saving.
|
|
> Fill implementation-owned sections, then stop with active files in place and report ready for review.
|
|
> Finalization (`코드리뷰 결과`, log rename, `complete.log`, archive moves, `코드리뷰 전용 체크리스트`) is review-agent-only, even after compaction/resume.
|
|
> Follow the ownership table at the bottom of this file for which sections you own.
|
|
|
|
## 개요
|
|
|
|
date={YYYY-MM-DD}
|
|
task={task_name}, plan={N}, tag={TAG}
|
|
|
|
## 이 파일을 읽는 리뷰 에이전트에게
|
|
|
|
> **[REVIEW AGENT ONLY]** 아래 종결 절차는 코드리뷰 에이전트 전용이다. 구현 에이전트는 이 섹션을 실행하지 않는다.
|
|
|
|
각 항목의 구현을 실제 소스 파일과 대조하고, `검증 결과` 섹션의 출력이 코드와 일치하는지 확인하세요.
|
|
리뷰 완료는 아래 순서까지 끝난 상태를 의미합니다.
|
|
|
|
1. 판정을 append한다.
|
|
2. `CODE_REVIEW-{review_lane}-GNN.md` → `code_review_{review_lane}_GNN_N.log`, `PLAN-{build_lane}-GNN.md` → `plan_{build_lane}_GNN_M.log`로 아카이브한다.
|
|
3. PASS이면 `complete.log` 작성 후 active task 디렉터리를 `agent-task/archive/YYYY/MM/{task_name}/`로 이동한다. WARN/FAIL이면 다음 active plan/review 파일을 즉시 작성한다.
|
|
4. 적용 가능한 `코드리뷰 전용 체크리스트` 항목을 최종 `.log` 위치에서 체크한 뒤 보고한다.
|
|
|
|
---
|
|
|
|
## 구현 항목별 완료 여부
|
|
|
|
| 항목 | 완료 여부 |
|
|
|------|---------|
|
|
| [{TAG}-1] {item description} | [ ] |
|
|
| [{TAG}-2] {item description} | [ ] |
|
|
|
|
## 구현 체크리스트
|
|
|
|
{copy the follow-up plan's 구현 체크리스트 items exactly, preserving order and checkbox text}
|
|
|
|
## 코드리뷰 전용 체크리스트
|
|
|
|
> **[REVIEW AGENT ONLY]** 이 체크리스트는 코드리뷰 에이전트만 사용한다.
|
|
> 구현 에이전트는 이 섹션을 수정하거나 체크하지 않는다.
|
|
|
|
- [ ] `코드리뷰 결과`에 `PASS`, `WARN`, `FAIL` 중 하나의 판정을 append한다.
|
|
- [ ] 판정과 `차원별 평가`, Required/Suggested/Nit 분류가 서로 일치한다.
|
|
- [ ] active `CODE_REVIEW-*-G??.md`를 `code_review_{review_lane}_GNN_N.log`로 아카이브한다.
|
|
- [ ] active `PLAN-*-G??.md`를 `plan_{build_lane}_GNN_M.log`로 아카이브한다.
|
|
- [ ] PASS이면 `agent-ops/skills/common/code-review/templates/complete-log-template.md` 기준으로 `complete.log`를 작성하고 active `.md` 파일을 남기지 않는다.
|
|
- [ ] PASS이면 active task 디렉터리 `agent-task/{task_name}/`를 `agent-task/archive/YYYY/MM/{task_name}/`로 이동하고 최종 archive 경로에서 이 체크리스트를 갱신한다.
|
|
- [ ] PASS split 작업이면 이동 후 빈 active parent `agent-task/{task_group}/`를 제거하거나, 남은 sibling/file이 있어 유지했다고 확인한다.
|
|
- [ ] WARN/FAIL이면 다음 active `PLAN-{build_lane}-GNN.md`와 `CODE_REVIEW-{review_lane}-GNN.md`를 작성하고 `complete.log`를 작성하지 않는다.
|
|
|
|
## 계획 대비 변경 사항
|
|
|
|
_구현 에이전트가 계획과 다르게 구현한 부분을 이유와 함께 기록한다._
|
|
|
|
## 주요 설계 결정
|
|
|
|
_구현 에이전트가 주요 설계 결정 사항을 기록한다._
|
|
|
|
## 리뷰어를 위한 체크포인트
|
|
|
|
{pre-filled from plan — one bullet per review focus area}
|
|
|
|
## 검증 결과
|
|
|
|
_구현 에이전트가 각 중간 검증 및 최종 검증 명령 실행 후 출력을 여기에 붙여 넣는다._
|
|
|
|
필수 규칙:
|
|
- 검증 명령은 고정된 계약이다. 임의로 대체하지 않는다.
|
|
- 대체가 필요하면 `계획 대비 변경 사항`에 이유와 대체 명령을 기록한다.
|
|
- `검증 결과`에는 실제 stdout/stderr를 붙여 넣는다.
|
|
|
|
### {TAG}-1 중간 검증
|
|
```
|
|
$ {verification command from plan}
|
|
(output)
|
|
```
|
|
|
|
### 최종 검증
|
|
```
|
|
$ {final verification command from plan}
|
|
(output)
|
|
```
|
|
|
|
---
|
|
|
|
> **[IMPLEMENTING AGENT — BEFORE SAVING] Have you filled in every implementation-owned section: completion table, implementation checklist, changes from plan, design decisions, and verification output?**
|
|
> If anything is blank, go back and fill it in before saving this file.
|
|
> Leave review-agent-only sections unchanged.
|
|
```
|
|
|
|
Sections and their ownership:
|
|
|
|
| 섹션 | 소유자 | 설명 |
|
|
|------|--------|------|
|
|
| 헤더 주석, 개요(date/task/plan/tag), 리뷰 에이전트 지시 | 스텁 생성 시 고정 | 구현 에이전트가 수정하거나 실행하지 않음 |
|
|
| 구현 항목별 완료 여부 (항목명) | 스텁 생성 시 고정 | `[ ]` → `[x]` 체크만 구현 에이전트가 수행 |
|
|
| 구현 체크리스트 (항목 텍스트/순서) | follow-up plan에서 복사해 스텁 생성 시 고정 | 구현 에이전트가 `[ ]` → `[x]` 체크만 수행; 마지막 체크박스는 저장 전 필수 |
|
|
| 코드리뷰 전용 체크리스트 | Review agent only | Implementing agent must not modify or check this section |
|
|
| 계획 대비 변경 사항, 주요 설계 결정 | 구현 에이전트가 채움 | placeholder 텍스트를 실제 내용으로 교체 |
|
|
| 리뷰어를 위한 체크포인트 | 스텁 생성 시 고정 | 계획에서 추출한 리뷰 포인트 |
|
|
| 검증 결과 (섹션 제목 + 명령) | 스텁 생성 시 고정 | 실행 출력만 구현 에이전트가 채움; 명령 변경은 `계획 대비 변경 사항`에 기록 |
|
|
| 코드리뷰 결과 | 리뷰 에이전트가 append | 스텁에 포함하지 않음 |
|
|
|
|
## Step 7 - Complete Review-Only Checklist, Move PASS Task, And Report
|
|
|
|
After Step 6:
|
|
|
|
- If verdict is `PASS`, determine archive month from the current completion date as `YYYY/MM`, create the needed archive parent directories, then move the selected active task directory `agent-task/{task_name}/` to `agent-task/archive/YYYY/MM/{task_name}/`. For split work, the selected active task directory is the subtask directory, and the archive must preserve the task group path, e.g. `agent-task/refactoring/01_core/` moves to `agent-task/archive/YYYY/MM/refactoring/01_core/`.
|
|
- Do not overwrite an existing archive directory. If `agent-task/archive/YYYY/MM/{task_name}/` already exists, append the next numeric suffix to the final path segment: single-plan `agent-task/archive/YYYY/MM/{task_group}_1/`, split-plan `agent-task/archive/YYYY/MM/{task_group}/{subtask_dir}_1/`, and so on.
|
|
- After moving a split subtask, remove the active parent `agent-task/{task_group}/` only if it is empty. If sibling subtask directories or other files remain, leave the parent task group in place.
|
|
- For `PASS`, open the moved `agent-task/archive/YYYY/MM/{final_task_name}/code_review_{review_lane}_GNN_N.log`, where `{final_task_name}` is the archived task path, including `{task_group}/` for split work.
|
|
- For `WARN` or `FAIL`, open `agent-task/{task_name}/code_review_{review_lane}_GNN_N.log`.
|
|
- Check every applicable item in `코드리뷰 전용 체크리스트`; leave mutually exclusive verdict items unchecked.
|
|
- If any applicable item cannot be checked, finish the missing archive, `complete.log`, task-directory move, or follow-up plan/review write first.
|
|
- Do not recreate an active review file just to update this checklist; update the archived `code_review_*.log`.
|
|
- Only report after the archived review log has the verdict, applicable checked review-only checklist, required next-state files, and for `PASS` the final task archive move.
|
|
|
|
Report Required/Suggested counts, archive names, and the final task archive path for `PASS` or new plan path for `WARN`/`FAIL`.
|
|
|
|
## Review Dimensions
|
|
|
|
| Dimension | Check |
|
|
|-----------|-------|
|
|
| Correctness | Logic, edge cases, concurrency, errors |
|
|
| Completeness | All planned checklist items done, including matching plan/review `구현 체크리스트` completion |
|
|
| Test coverage | Required tests present and meaningful |
|
|
| API contract | Call sites, compatibility, docs |
|
|
| Code quality | No debug prints, dead code, leftover TODOs |
|
|
| Plan deviation | Deviations justified, no unrelated risk |
|
|
| Verification trust | Reported output matches actual code |
|
|
|
|
## Quality Rules
|
|
|
|
- Lead with findings; use specific `file:line`.
|
|
- Provide a concrete fix for every Required issue.
|
|
- Name exact stale symbols or missing tests.
|
|
- Do not write vague praise or style opinions without a rule.
|
|
- Every dimension gets Pass/Warn/Fail.
|
|
- For follow-up plans about verification trust, specify deterministic commands, for example `rg --sort path`, and forbid repo-local tool artifacts.
|
|
|
|
## Final Checklist
|
|
|
|
- `code_review_{review_lane}_GNN_N.log` exists with verdict appended.
|
|
- `plan_{build_lane}_GNN_M.log` exists.
|
|
- No active `.md` files remain after PASS.
|
|
- PASS: `complete.log` written from `agent-ops/skills/common/code-review/templates/complete-log-template.md`, then task directory moved under `agent-task/archive/YYYY/MM/` with task-group path preserved for split work.
|
|
- PASS split: empty active parent `agent-task/{task_group}/` removed after the subtask move; non-empty parent left in place.
|
|
- WARN/FAIL: new active `PLAN-{build_lane}-GNN.md` and `CODE_REVIEW-{review_lane}-GNN.md` created with matching headers and matching `구현 체크리스트`; no `complete.log`.
|
|
- The applicable review-agent-only finalization checklist was completed before reporting.
|