# Code Review

## 1. Determine Diff Against origin/main

Always review changes against `origin/main` directly. Do not use local `main`, `git merge-base`, a PR base branch, or a user-specified alternate base.

Get the CURRENT diff — never use cached results from earlier in the conversation. Include both committed and uncommitted changes (staged + unstaged):

```
BASE=origin/main
git diff $BASE --stat
```

Note: `git diff origin/main` (not `origin/main...HEAD`) diffs `origin/main` against the working tree, capturing committed changes, staged changes, and unstaged changes in one pass.

Show the user the file list and total count. If there are 0 changed files, report that to the user and stop — there is nothing to review. Otherwise, immediately proceed to the review agents. Do NOT wait for confirmation.

Before launching agents, build a concise review context and pass it to every agent. Show the context as a table before launching agents so assumptions are visible:

| Context | Source | Value |
| --- | --- | --- |
| User goal | Conversation | One-line summary, or `not provided` |
| Implementation plan | Conversation / docs | One-line summary, or `not provided` |
| PR context | PR title/body | One-line summary, or `no PR found` |
| Commits | `git log --oneline origin/main..HEAD` | One-line summary of commit intent |
| Changed surface | diff file list | Main packages/files touched |
| Inferred behavior | commits/tests/docs/user text | Intended behavior change, or `diff-only inference` |

## 2. Spawn Parallel Review Agents

### Review Philosophy

Pass these rules to every agent:

- **It is OK to find nothing.** A clean review is a valid outcome. Do NOT manufacture findings to justify the review. Only flag issues you are confident are real problems.
- **Be opinionated and consistent.** If a pattern is acceptable, don't flag it. If you flag something, commit to that position — don't suggest the opposite approach on a re-review.
- **Don't flag trade-offs with no clear winner.** If there are two reasonable approaches and neither is clearly better, don't flag it. The author already made a choice.
- **High confidence only.** Every finding must pass the bar: "I am confident this is a problem, and I can explain specifically what goes wrong if it's not fixed." Vague unease is not a finding.
- **Permission-friendly reads.** Avoid shell pipelines, command separators, subshells, and output filters for read-only investigation because they create extra permission prompts and block background review agents.
- **Intent-aware review.** Review changed code against the review context, not against the old behavior alone.

## 3. Synthesizing Report

### Findings

**Severity: MEDIUM** | **Category: Clean-Go / Error Handling Style**

### No-op `fmt.Errorf("%w", err)` wraps around formatted errors

**Problem:** The code wraps `unsupportedVersionError` (which is a fully-formatted error with its own `Error()` method) with `fmt.Errorf("%w", err)` without adding context. This is redundant wrapping that doesn't improve the error message.

**Locations (6 instances):**
1. `cmd/entire/cli/explain.go:760`
2. `cmd/entire/cli/explain_export.go:270`
3. `cmd/entire/cli/explain_export.go:366`
4. `cmd/entire/cli/resume.go:371`
5. `cmd/entire/cli/rewind.go:737`
6. `cmd/entire/cli/strategy/manual_commit_rewind.go:655`

**Suggested Fix:** Return the error directly:
```go
if err := checkpointpolicy.EnsureCanReadVersion(cpID.String(), summary.CheckpointVersion); err != nil {
    return nil, nil, err  // Direct return; unsupportedVersionError is already formatted
}
```

This is a style issue (not a bug), but modern Go practice favors minimal error wrapping.