# Review Policy

Define the shared judgment criteria and behavioral principles for all reviewers.

## Principles

| Principle | Criteria |
|-----------|----------|
| Fix immediately | Never defer minor issues to "the next task." Fix now what can be fixed now |
| Eliminate ambiguity | Vague feedback like "clean this up a bit" is prohibited. Specify file, line, and proposed fix |
| Fact-check | Verify against actual code before raising issues. Do not speculate |
| Practical fixes | Propose implementable solutions, not theoretical ideals |
| Boy Scout | If a changed file has problems, have them fixed within the task scope |

## Scope Determination

| Situation | Verdict | Action |
|-----------|---------|--------|
| Problem introduced by this change | Blocking | REJECT |
| Code made unused by this change (arguments, imports, variables, functions) | Blocking | REJECT (change-induced problem) |
| Existing problem in a changed file | Blocking | REJECT (Boy Scout rule) |
| Structural problem in the changed module | Blocking | REJECT if within scope |
| Problem in an unchanged file | Non-blocking | Record only (informational) |
| Refactoring that greatly exceeds task scope | Non-blocking | Note as a suggestion |

## Judgment Criteria

### REJECT (Request Changes)

REJECT without exception if any of the following apply.

- New behavior without tests
- Bug fix without a regression test
- Use of `any` type
- Fallback value abuse (`?? 'unknown'`)
- Explanatory comments (What/How comments)
- Unused code ("just in case" code)
- Direct mutation of objects/arrays
- Swallowed errors (empty catch blocks)
- TODO/FIXME without an issue number, external blocker, and removal condition
- Essentially identical logic duplicated (DRY violation)
- Method proliferation doing the same thing (should be absorbed by configuration differences)
- Specific implementation leaking into generic layers (imports and branching for specific implementations in generic layers)
- Internal implementation exported from public API (infrastructure functions or internal classes exposed publicly)
- Replaced code/exports surviving after refactoring
- Missing cross-validation of related fields (invariants of semantically coupled config values left unverified)
- Missing caller, producer, or test data updates after a contract change
- Sensitive data exposed in logs, error responses, or test output

A DRY finding is not complete unless the proposed consolidation target is also sound. A consolidation proposal is invalid unless all of the following hold.

- The consolidation target matches existing responsibility boundaries and dependency direction
- Any new public API, wrapper, or helper does not expand the existing contract unnaturally
- If the proposal introduces abstraction not required by the task or plan, its necessity is explained with evidence

### Warning

Not blocking, but improvement is recommended.

- Insufficient edge case / boundary value tests
- Tests coupled to implementation details
- Overly complex functions/files
- Unclear naming
- TODO/FIXME with issue number, external blocker, and removal condition
- `@ts-ignore` or `eslint-disable` without justification

### APPROVE

Approve when all REJECT criteria are cleared and quality standards are met. Never give conditional approval. If there are problems, reject.

## Fact-Checking

Always verify facts before raising an issue.

| Do | Do Not |
|----|--------|
| Open the file and check actual code | Assume "it should be fixed already" |
| Search for call sites and usages | Raise issues based on memory |
| Cross-reference type definitions and schemas | Guess that code is dead |
| Distinguish generated files (reports, etc.) from source | Review generated files as if they were source code |
| Verify tool output is readable and uncorrupted | Raise issues based on garbled or abnormal output |
| When claiming code is absent, read the target lines directly | Conclude "code doesn't exist" based on search results alone |

### Tool Output Reliability

If tool output is unreadable, re-read using a reliable method before making any judgment.

| Situation | Action |
|-----------|--------|
| Output contains garbled text or encoding anomalies | Recognize the corruption, then re-read using an alternative method (open the file directly, specify line numbers for the target section) before judging |
| Search command did not find the target code | Read the specific lines of the file directly to confirm absence before raising an issue. Search failure does not equal code absence |
| Re-raising a prior finding without re-checking actual code | Must read current code before marking as persists. Do not re-raise from memory of the prior review |

## Writing Specific Feedback

Every issue raised must include the following.

- **Which file and line number**
- **What the problem is**
- **How to fix it**
- **If requesting abstraction or consolidation, why that placement is the natural one**

```
❌ "Review the structure"
❌ "Clean this up a bit"
❌ "Refactoring is needed"

✅ "src/auth/service.ts:45 — validateUser() is duplicated in 3 places.
     Extract into a shared function."
```

## Finding ID Tracking (`finding_id`)

To prevent circular rejections, track findings by ID.

- Every issue raised in a REJECT must include a `finding_id`
- If the same issue is raised again, reuse the same `finding_id`
- For repeated issues, set status to `persists` and include concrete evidence (file/line) that it remains unresolved
- New issues must use status `new`
- Resolved issues must be listed with status `resolved`
- Issues without `finding_id` are invalid (cannot be used as rejection grounds)
- REJECT is allowed only when there is at least one `new` or `persists` issue
- Before treating a prior finding as resolved, verify that the fix did not introduce a different structural or contract problem

## Reopen Conditions (`resolved` -> open)

Reopening a resolved finding requires reproducible evidence.

- To reopen a previously `resolved` finding, all of the following are required  
  1. Reproduction steps (command/input)  
  2. Expected result vs. actual result  
  3. Failing file/line evidence
- If any of the three is missing, the reopen attempt is invalid (cannot be used as REJECT grounds)
- If reproduction conditions changed, treat it as a different problem and issue a new `finding_id`

## Immutable Meaning of `finding_id`

Do not mix different problems under the same ID.

- A `finding_id` must refer to one and only one problem
- If problem meaning, evidence files, or reproduction conditions change, issue a new `finding_id`
- Rewriting an existing `finding_id` to represent a different problem is prohibited

## Handling Test File Size and Duplication

Test file length and duplication are warning-level maintainability concerns by default.

- Excessive test file length and duplicated test setup are `Warning` by default
- They may be `REJECT` only when reproducible harm is shown  
  - flaky behavior  
  - false positives/false negatives  
  - inability to detect regressions
- "Too long" or "duplicated" alone is not sufficient for `REJECT`

## Handling Changelog and History Files

Files or sections that record point-in-time facts (e.g., `CHANGELOG.md`, `RELEASE_NOTES.md`, `MIGRATION.md`) are history, not specifications of the current code. Judge them by their correctness as history.

| Target | Judgment |
|--------|----------|
| Past entry's config keys, API names, or behaviors do not match current code | REJECT prohibited |
| Records that were correct at the time of the relevant release | Modification requests prohibited |
| Factual errors in newly added entries (relative to the target release) | REJECT allowed |
| Markdown formatting issues, duplication, broken links, obvious typos | REJECT or Warning allowed |

### Judgment Criteria

- History records "what changed at that point in time," not "how the system currently works"
- Even if names or behaviors have been changed in current code, that is not grounds to rewrite past entries
- To request modification of a past entry, demonstrate that it was incorrect even at the relevant release point
- Identify history files/sections by file name (`CHANGELOG.md`, etc.) or conventional headings (`### Changed`, `### Added`, dated release headings)
- Do not REJECT a history file or section based solely on disagreement with current schema or current config keys

## Boy Scout Rule

Leave it better than you found it.

### In Scope

- Existing problems in changed files (unused code, poor naming, broken abstractions)
- Structural problems in changed modules (mixed responsibilities, unnecessary dependencies)

### Out of Scope

- Unchanged files (record existing issues only)
- Refactoring that greatly exceeds task scope (note as a suggestion, non-blocking)

### Judgment

| Situation | Verdict |
|-----------|---------|
| Changed file has an obvious problem | REJECT — have it fixed together |
| Redundant expression (a shorter equivalent exists) | REJECT |
| Unnecessary branch/condition (unreachable or always the same result) | REJECT |
| Fixable in seconds to minutes | REJECT (do not mark as "non-blocking") |
| Code made unused as a result of the change (arguments, imports, etc.) | REJECT — change-induced, not an "existing problem" |
| Fix requires refactoring (large scope) | Record only (technical debt) |

Do not tolerate problems just because existing code does the same. If existing code is bad, improve it rather than match it.

## Judgment Rules

- All issues detected in changed files are blocking (REJECT targets), even if the code existed before the change
- Only issues in files NOT targeted by the change may be classified as "existing problems" or "non-blocking"
- "The code itself existed before" is not a valid reason for non-blocking. As long as it is in a changed file, the Boy Scout rule applies
- If even one issue exists, REJECT. "APPROVE with warnings" or "APPROVE with suggestions" is prohibited

## Basic Review Procedure

Common procedure that every reviewer must follow. Do not duplicate this in individual instructions.

### Referring to Primary Sources

- Use `order.md`, `plan.md`, and the actual code as primary sources
- Treat decisions from earlier steps (prior review results, planning decisions) as supplementary
- When information conflicts, prioritize `order.md` / `plan.md` / actual code

### Referring to Design Decisions

- If the implementation step has emitted `coder-decisions.md`, read it and understand the recorded design decisions
- Do not dismiss intentional decisions as false positives just because they were recorded. Evaluate validity against `order.md` / `plan.md` / actual code
- If the design decision itself is flawed, raise it

### Tracking Findings from Previous Reviews

- Look in the Report Directory for review reports this step has previously produced, along with their timestamped history
- Treat the unsuffixed file as the latest result and the most recent `{report-name}.{timestamp}` as the previous result
- `Previous Response` may be used as supplementary information, but finding state determinations must prioritize the report history
- Do not drop open findings from the previous report when producing the new report
- Apply the `finding_id` management rules when classifying each finding as `new` / `persists` / `resolved` / `reopened`

### Final Decision Steps

1. Classify each detected issue as blocking / non-blocking according to the scope rules and decision rules above
2. When citing test, build, or behavior verification as evidence, record the target, the check, and the result in the report
3. REJECT if there is at least one blocking issue (`new`, `persists`, or `reopened`)

## Detecting Circular Arguments

When the same kind of issue keeps recurring, reconsider the approach itself rather than repeating the same fix instructions.

### When the Same Problem Recurs

1. Check if the same kind of issue is being repeated
2. If so, propose an alternative approach instead of granular fix instructions
3. Even when rejecting, include the perspective of "a different approach should be considered"

Rather than repeating "fix this again," stop and suggest a different path.
