cs-feat-review

cs-feat-review

Required Reading at Startup

Before making any judgment or taking any action, first read .codestable/attention.md; if missing, treat the skeleton as incomplete, prompt to fill it in first or run cs-onboard, do not fall back to an external AI entry file.

This phase is an independent code review gate after implementation is complete and before QA. It only reads code and artifacts, and only writes {slug}-review.md, does not directly modify code, does not update the checklist, does not change the design, and does not replace QA or acceptance.

The goal of the review is not to pursue perfect code, but to confirm that this change does not reduce the system's code health and does indeed move toward the goal of the design. Issues that can be automatically formatted or linted should not be manually blocked; issues that affect correctness, maintainability, security, performance, testability, requirement satisfaction, or the credibility of subsequent acceptance must be pointed out.

For shared paths and naming conventions see .codestable/reference/shared-conventions.md Section 0.

---

Inputs

Before entering review, you must read:

• .codestable/attention.md
• {slug}-design.md
• {slug}-checklist.yaml
• Implementation completion report / most recent implementation record (if in the conversation, cite according to conversation facts; if already saved to a file, read the file)
• git status --short
• git diff(when there is a staged diff, also read git diff --cached）
• human-written code files involved in the diff and adjacent key call sites
• architecture / requirement / roadmap related documents pointed to by design Section 4 (read-only, to judge whether the change will affect consolidation)
• independent reviewer output (if Paseo or another external reviewer was enabled this round)

If the workspace has pre-existing dirty files outside the feature, first record them as baseline/unrelated changes; the review conclusion only targets changes attributable to this feature. When attribution cannot be distinguished, write it as residual-risk, do not treat uncertainty as a pass.

---

Startup Check

1. {slug}-design.md exists, frontmatter doc_type=feature-design、status=approved、feature is consistent with the directory.
2. {slug}-checklist.yaml exists,steps all done，checks are still in the pre-acceptance state (usually pending）。
3. The current diff shows implementation changes for this feature; if there are no code or artifact changes at all, fall back to cs-feat-impl。
4. If there is already {slug}-review.md：
   • status: passed and the diff has not changed: prompt that you can proceed to cs-feat-qa。
   • status: changes-requested / blocked: read the old findings and confirm whether this is a re-review after review-fix.
   • The diff has changed: review again, and record in the report which round this is.

---

Independent Reviewer Enhancement

By default, this phase is completed as a local review by the current agent; the independent reviewer is an enhancement, not a hard dependency. When no external agent can be detected, Paseo is unavailable, the provider is not configured, or the user explicitly requests quick completion, you may continue with the local review, and record in the report Independent reviewer: local-only / skipped-by-user。

But once the independent reviewer has been started this round, it becomes an input to this round's review gate. The main agent may first make a local review draft, but must not finalize before the independent reviewer returns {slug}-review.md, must not give passed, and must not enter cs-feat-qa. If the reviewer is stuck, fails, is blocked by permissions, or takes too long, you can only mark this round as blocked / independent-review-pending, and then let the user decide: continue waiting, retry the reviewer, or explicitly downgrade to a local-only review.

First run the detection script bundled with this skill. According to the loaded SKILL.md Parse the script path from its directory; do not guess the path from the business repository root:

python3 scripts/detect-review-agent.py --pretty

The script only outputs capability JSON; it does not start an agent or modify files.

Priority:

1. Paseo available: prefer using Paseo to start audit -type subagents for read-only independent review. The detection script can only indicate that Paseo candidate capability exists on this machine; before actually starting, you must also confirm that the current agent runtime exposes the Paseo tool, or that the Paseo CLI from the detection result can be invoked. Before starting, first load / read the current instructions of the paseo skill and follow its rules: read ~/.paseo/orchestration-preferences.json, use providers.audit, do not hardcode Claude or Codex; when files are missing, use reasonable defaults and explain them in the report. If no tool / CLI is available, do not fake a start; record the local-only or blocked reason. Do not poll a running agent indefinitely; if the reviewer has started but the result has not returned, stop at the review gate, record pending/blocked, and wait for notification or a user decision.
2. Only CLIs such as claude / gemini / aider are visible: do not invoke them automatically. Review locally, unless the user explicitly requests using a certain CLI.
3. No external reviewer: review locally.

The Paseo subagent prompt must provide only raw materials and boundaries, and must not reveal local review conclusions:

你是 CodeStable feature 的独立代码审查 agent。只读，不修改文件，不更新 checklist/design。

请读取：
- .codestable/attention.md
- {design_path}
- {checklist_path}
- 当前 git status / git diff / staged diff
- diff 涉及的人写代码和相邻关键调用点

按 cs-feat-review 的严重度语义输出：blocking / important / nit / suggestion / learning / praise / residual-risk。
每条 finding 必须有 file:line 或仓库事实证据、影响、建议修复边界。
额外输出 Test And QA Focus：QA 必须重点复核的场景、建议新增或加强的测试、review 无法确认的点。
不要写 {slug}-review.md；只把审查结果回传给主 agent。

The main agent is still the party ultimately responsible for the review: it must verify the independent reviewer's findings one by one, deduplicate, assign severity, and merge them into {slug}-review.md. External conclusions that have not been verified against local repository facts may only be written as residual-risk or ignored, and must not be directly escalated to blocking。

Review process

1. Context and scope

• Use sections 1/2/3 of the design to confirm the goal, explicit non-goals, key decisions, and acceptance scenarios.
• Use the checklist steps to confirm the scope that the implementation claims to have completed.
• Use git status / git diff to list the actual changed files, marking added, modified, deleted, untracked, and staged.
• Assess diff size and risk: cross-module, cross-boundary, data migration, permissions/security, concurrency/asynchrony, user-visible UI, public API, test gaps.

2. Independent review merge

• Record the detection result:paseo-subagent / local-review-with-agent-cli-available / local-review。
• If no independent reviewer was started: record not-available / skipped-by-user / local-only, and the local review may be finalized.
• If a Paseo subagent was started: first do a local review draft, but the final verdict must wait for the independent reviewer to return. Do not poll a running notify-on-finish agent; wait for notification, for the user to bring back the result, or to continue in the next round.
• If the independent reviewer returns: perform local fact verification item by item, and merge only what can be reproduced / has evidence; if evidence is insufficient, only write it as residual risk or do not adopt it.
• If the independent reviewer fails, is blocked by permissions, times out, or is still running: do not silently downgrade; report status: blocked, write pending|failed|blocked and the agent id / reason in Independent Review, and leave the next step to the user to decide whether to keep waiting, retry, or downgrade to local-only.
• Keep the sources in the report: which findings came from the independent reviewer and which were found by the local review.

3. Overall review

Look at the whole first, then at line-level details:

• design fit: whether the implementation satisfies the design without secretly expanding scope.
• architecture fit: whether the new code is placed at the correct layer, and whether it bypasses existing abstractions, introduces reverse dependencies, or causes excessive coupling.
• Complexity: Whether over-generalization, patch branches, parameter bloat, or continued growth of large functions/classes is introduced for the current problem.
• Test strategy: Whether existing tests and newly added tests can prove key scenarios; whether tests will genuinely fail when the code breaks.
• Risk surface: Error handling, data validation, security boundaries, permissions, concurrency, idempotency, performance, observability, rollback/uninstall.
• Documentation/merge impact: Whether there are architecture / requirement / roadmap changes that acceptance must write back.

4. Line-level review

Review human-written code file by file, covering at least:

• Logical correctness: Boundary values, null values, exception paths, state transitions, timing issues, off-by-one.
• Error handling: Whether error semantics are clear, whether errors are swallowed, whether recovery logic and business logic are mixed together.
• Data and security: Input validation, injection risk, sensitive information, permission checks, cross-tenant/cross-user isolation.
• Performance and resources: Repeated IO, N+1, unbounded loops/caches, memory leaks, unreleased resources.
• Concurrency/async: Races, deadlocks, cancellation, reentrancy, duplicate submissions, idempotency.
• Maintainability: Whether naming follows design terminology, whether existing helpers are reused, whether duplicate logic is added.
• Cleanliness: Debug output, temporary TODO/FIXME, commented-out code, unused imports, files outside the plan.

Generated code, lockfiles, and large data files may be sampled, but the sampling scope must be stated in the report. Human-written business code cannot be skipped.

5. Conclusion

Classify findings by severity and give a clear verdict:

• passed: No blocking; important issues have been fixed, there are no important items, or the user has explicitly accepted deferral.
• changes-requested: There are blocking issues, or there are so many important issues that they affect acceptance credibility.
• blocked: Key inputs are missing, diff attribution cannot be determined, the design/implementation state does not meet the review preconditions, or an independent reviewer has been started this round but the result is still pending / failed / blocked and the user has not confirmed a downgrade.

---

Severity

• blocking: Must be fixed first. It will cause functionality to fail to meet the design, data/security/permission risks, obvious bugs, acceptance cannot be credibly executed, serious architectural regression, or tests completely fail to cover key risks.
• important: Should be fixed; if the user decides to defer, it must be clearly recorded in the review report and acceptance residual risk.
• nit: Minor clarity or consistency suggestion, non-blocking.
• suggestion: Alternative implementation approach, not required to be adopted this time.
• learning: Informational note, no action required.
• praise: Record good practices worth keeping; a small number is enough.
• residual-risk: Uncertainty that review cannot fully eliminate and that QA / acceptance needs to focus on rechecking.

Do not escalate personal preferences into blocking. Blocking must be supported by repository facts, design contracts, reliable engineering principles, or reproducible examples.

---

Report template

Report path:.codestable/features/{feature}/{slug}-review.md。

---
doc_type: feature-review
feature: YYYY-MM-DD-slug
status: passed|changes-requested|blocked
reviewed: YYYY-MM-DD
round: 1
---

# {slug} 代码审查报告

## 1. Scope And Inputs

- Design: {path}
- Checklist: {path}
- Implementation evidence: {实现汇报 / 对话 / 文件}
- Diff basis: {git status / git diff 摘要}
- Baseline dirty files: {none / 列表 + 归因}

### Independent Review

- Status: not-available|skipped-by-user|local-only|pending|completed|failed|blocked
- Detection: paseo-subagent|local-review-with-agent-cli-available|local-review|skipped
- Provider / agent: {providers.audit / agent id / none}
- Raw output: {摘要 / 路径 / none}
- Merge policy: {已逐条核验 / 未启用原因 / pending 时不得定稿}
- Gate effect: {none | blocks final verdict until completed / user-approved downgrade}

## 2. Diff Summary

- 新增：{文件列表}
- 修改：{文件列表}
- 删除：{文件列表}
- 未跟踪 / staged：{文件列表}
- 风险热点：{跨模块 / 权限 / 数据 / 并发 / UI / API / none}

## 3. Findings

### blocking

- [ ] REV-001 `{file:line}` {问题}
  - Evidence: {仓库事实 / design 契约 / 失败路径}
  - Impact: {为什么阻塞 QA / acceptance}
  - Expected fix scope: {只描述问题边界，不替实现写方案}

### important

- [ ] REV-00N `{file:line}` {问题}
  - Evidence: {证据}
  - Impact: {影响}

### nit

- [ ] REV-00N `{file:line}` {建议}

### suggestion

- [ ] REV-00N {建议}

### learning

- {可复用经验或注意点}

### praise

- {值得保留的做法}

## 4. Test And QA Focus

- QA 必须重点复核：{场景 / 命令 / 手工验证}
- 建议新增或加强的测试：{unit / integration / e2e / function / none}
- 不能靠 review 完全确认的点：{列表}

## 5. Residual Risk

- {风险 + QA / acceptance 如何处理；没有写 none}

## 6. Verdict

- Status: passed|changes-requested|blocked
- Next: `cs-feat-qa` | `cs-feat-impl` review-fix | 等 independent reviewer 完成 / 用户确认降级后重跑 `cs-feat-review` | 补齐输入后重跑 `cs-feat-review`

When there is no certain type of finding, write none, do not delete the section; the next review round must be able to compare.

---

review-fix handoff

If there is a blocking：

1. report status: changes-requested。
2. tell the user that the next step is to trigger cs-feat-impl in review-fix mode.
3. review-fix only fixes blocking findings; whether important issues are fixed is decided by the user or implementer, but scope must not be expanded casually.
4. After review-fix is complete, you must rerun cs-feat-review, and cannot go directly to cs-feat-qa or cs-feat-accept。

If there is only important：

• By default, it is recommended to fix first; if the user explicitly accepts deferring it, move it into residual risk in the report and allow proceeding to cs-feat-qa。

If there is no blocking, and important items have been handled or explicitly accepted:

• Report status: passed。
• Tell the user the next step is cs-feat-qa。

---

Exit conditions

• attention, design, checklist, implementation evidence, git status, git diff, and related code have been read.
• It has been confirmed that all checklist steps are done; otherwise fall back to cs-feat-impl。
• An independent reviewer check has been run, or the reason for skipping it is recorded.
• If the independent reviewer was not started, the reason not-available / skipped-by-user / local-only is recorded.
• If the independent reviewer was started, it has been waited on until completed and the findings have been locally verified one by one for merging / rejection; otherwise report status: blocked, and do not enter QA.
• An overall review and line-level review have been performed.
• blocking / important / nit / suggestion / learning / praise / residual-risk have been clearly distinguished.
• Written .codestable/features/{feature}/{slug}-review.md。
• When there is blocking, QA / acceptance was not entered, and instead it points to cs-feat-impl review-fix。
• When there is no blocking, clearly tell the user the next step is cs-feat-qa。

---

Common pitfalls

• Fixing code while reviewing, turning a read-only gate into an implementation phase.
• Only looking at the implementation report, not the real git diff。
• Only looking at whether tests pass, without judging whether the tests are effective.
• Escalating formatting, naming preferences, and personal style into blocking.
• Discovering out-of-design implementation but not going back to the design contract to judge it.
• Copying an external reviewer's conclusion directly as blocking without local factual verification.
• After starting the independent reviewer, finalizing the local review as passed before the results come back.
• When the independent reviewer is stuck, silently downgrading to local-only without asking the user.
• After fixing blocking issues, skipping re-review and going straight to acceptance.
• The review report was not saved to disk, so acceptance has no traceable input.