aide-family/code-review

Code Review

Objective

Review the given code for correctness and potential bugs. For each issue found, report the exact location (file and line) and provide a recommended fix.

Review Process

1. Read the code in scope (file(s) or diff the user provided).
2. Identify potential bugs, including:
   • Logic errors, off-by-one, wrong conditions
   • Nil/zero dereference, out-of-bounds access
   • Race conditions, concurrency misuse
   • Error handling gaps (ignored errors, wrong propagation)
   • Resource leaks (unclosed handles, connections)
   • Incorrect assumptions about types, APIs, or data
   • HTTP route conflicts: e.g. a new route GET /v1/resource/simple while GET /v1/resource/{id} exists causes the former URL to be matched as id=simple and can lead to parse errors (e.g. strconv.ParseInt "parsing "simple": invalid syntax"). Check that no new path is a prefix or literal segment that would be captured by an existing path parameter.
3. For each finding: state file path, line number(s), and a short recommended solution.

Output Format

Use this structure for the review:

# Code Review

## Summary
[1–2 sentence overview: scope and overall risk level]

## Findings

### [Severity] Brief title
- **Location**: `path/to/file.ext:LINE` (and optional `:END_LINE` if spanning multiple lines)
- **Issue**: What is wrong and why it can cause a bug.
- **Recommendation**: Concrete fix (code snippet or step-by-step change).

---
[Repeat for each finding]

Severity:

• Critical: Likely crash, data corruption, or security impact.
• High: Clear bug under common usage.
• Medium: Edge-case bug or maintainability/robustness concern.
• Low: Minor or speculative; worth fixing when touching the area.

Checklist (when reviewing API / proto changes)

• [ ] Route conflict: If the change adds or modifies HTTP routes (e.g. in proto google.api.http or generated *_http.pb.go), verify that no new path is matched by an existing parameterized route (e.g. /v1/foo/{id} matching /v1/foo/simple). Resolve by using a distinct path prefix (e.g. /v1/foos/simple) or extra segment.

Rules

• Always cite location: Use path:line (and line range if needed). Do not say "somewhere in this function" without the line.
• One finding per item: One bug/issue per subsection; do not merge unrelated issues.
• Recommendation must be actionable: Prefer a small code change or clear steps, not vague advice.
• Assume context: If the project has patterns (e.g. error handling, logging), align recommendations with them; do not invent new patterns without need.

Example

### [High] Possible nil pointer dereference
- **Location**: `app/service/user.go:42`
- **Issue**: `u` may be nil when `FindByID` returns no user; calling `u.Name` can panic.
- **Recommendation**: Check and handle nil before use:

  if u == nil {
      return nil, ErrUserNotFound
  }
  return u.Name, nil