# Code Quality Review — 2026-06-12

**Scope:** Full codebase (`src/`) — 6,367 lines across 14 files  
**Skills applied:** `code-analyzer`, `rust-developer`

---

## Findings

### 1. [High] God Module — `review.rs` (1,196 lines)

- **File:** `src/review.rs`
- **Why it matters:** At nearly 1,200 lines this is the largest file by far. It handles GitHub issue creation, label management, workflow triggering, concern tracking, dismiss logic, approval, commit status, and JSON parsing — too many responsibilities for one module. This is a textbook **Large Module** and **Divergent Change** smell.
- **Suggested change:** Split into sub-modules:
  - `review/mod.rs` — public API (`handle_review_*` functions)
  - `review/github.rs` — GitHub CLI interactions (issue CRUD, labels, workflow dispatch)
  - `review/status.rs` — commit status management

### 2. [High] God Function — `main()` at 299 lines

- **File:** `src/main.rs:14-312`
- **Why it matters:** The entire CLI dispatch lives in a single `main()`. This violates the rust-developer skill guidance: *"Keep `main.rs` thin: parse args, load config, dispatch to handlers."* Adding a new command requires editing this massive match block.
- **Suggested change:** Extract each `Commands::*` arm into a dedicated dispatch function, or refactor into a `dispatch(command, config, opts)` function in `commands.rs`. Each match arm becomes a one-liner.

### 3. [High] Long Function — `handle_commit()` at 193 lines

- **File:** `src/commit.rs:193-385`
- **Why it matters:** This function handles validation, message construction, DoD checklist, intent log, staging, radar check, branch detection, commit, push, intent cleanup, review trigger, and tagging — all in one function. Classic **Long Function** and **Shotgun Surgery** smell.
- **Suggested change:** Extract into:
  - `validate_commit_params()` — lint checks
  - `build_commit_message()` — message assembly
  - `commit_to_trunk()` / `commit_to_branch()` — the two branch paths
  - `post_commit_housekeeping()` — intent cleanup + review trigger

### 4. [Medium] Long Function — `handle_sync()` at 119 lines

- **File:** `src/commands.rs:360-478`
- **Why it matters:** Mixes pre-flight checks, CI status prompts, pull/rebase, status display, log, radar scan, and stale branch warning. Hard to test individual steps.
- **Suggested change:** Extract `check_ci_before_sync()` and `display_post_sync_status()` helpers.

### 5. [Medium] Long Function — `handle_init_command()` at 121 lines

- **File:** `src/commands.rs:31-151`
- **Why it matters:** Contains two completely different flows (monorepo sub-project init vs root init) interleaved with remote setup. Moderate complexity.
- **Suggested change:** Split into `init_root()` and `init_project()`.

### 6. [Medium] Long Function — `handle_undo()` at 109 lines

- **File:** `src/commands.rs:538-646`
- **Why it matters:** Similar to sync — mixes safety snapshot, validation, sync, revert, push, and display. Moderate complexity.

### 7. [Medium] Long Function — `handle_changelog()` at 102 lines

- **File:** `src/changelog.rs:19-120`
- **Why it matters:** Parsing, grouping, and formatting all in one function. Could benefit from separating parsing from rendering.

### 8. [Medium] `&PathBuf` parameter instead of `&Path`

- **File:** `src/commands.rs:188`
- **Why it matters:** Clippy warns about this — accepting `&PathBuf` is less flexible than `&Path`. Callers with `Path` or `PathBuf` should both work without conversion.
- **Suggested change:** Change `root_config_path: &PathBuf` → `root_config_path: &Path` and add `use std::path::Path;`.

### 9. [Medium] Redundant closure

- **File:** `src/commit.rs:279`
- **Why it matters:** `.and_then(|log| intent::format_for_commit(log))` can be simplified to `.and_then(intent::format_for_commit)`. Clippy auto-fix available.

### 10. [Low] Duplicate GitHub CLI availability check pattern

- **Files:** `src/review.rs:435`, `src/review.rs:265`, `src/review.rs:651`, `src/review.rs:889`, `src/review.rs:1014`, `src/git.rs:730`
- **Why it matters:** `is_gh_cli_available()` is called at the top of almost every review function with identical early-return + warning patterns. This is **Duplicate Code**.
- **Suggested change:** Use a guard macro or a wrapper that returns `Result` so the check is done once at the entry point.

### 11. [Low] Duplicate issue search pattern

- **Files:** `src/review.rs:660`, `src/review.rs:898`, `src/review.rs:1024`
- **Why it matters:** The `[Review] in:title {hash} in:title is:open` search + `gh issue list` call is copy-pasted three times with minor variations. **Duplicate Code** smell.
- **Suggested change:** Extract `find_review_issue(short_hash, opts) -> Result<Option<i64>>`.

### 12. [Low] Unnecessary `sort_by` instead of `sort_by_key`

- **File:** `src/git.rs:504`
- **Why it matters:** Clippy suggests `sort_by_key(|b| std::cmp::Reverse(b.1))` which is more idiomatic.

### 13. [Low] `unwrap_or_else` with non-lazy value

- **File:** `src/radar.rs:184-185`
- **Why it matters:** `unwrap_or_else(|| OverlapKind::SameFile)` should be `unwrap_or(OverlapKind::SameFile)` since the fallback is a simple value, not a computation.

### 14. [Low] Print literal in format string

- **File:** `src/recover.rs:71`
- **Why it matters:** `"Hash"` is passed as a format argument but has no formatting specifier — it can be inlined directly into the format string.

### 15. [Low] Collapsible `if` into `match` guard

- **File:** `src/review.rs:397`
- **Why it matters:** The `if opts.verbose` inside the `Ok(output) if output.status.success()` arm can be collapsed into the match guard.

---

## Clippy / Compiler Warnings

6 standard warnings, all auto-fixable:

| Warning | File | Fix |
|---------|------|-----|
| `ptr_arg` | `commands.rs:188` | `&PathBuf` → `&Path` |
| `redundant_closure` | `commit.rs:279` | Use function pointer |
| `unnecessary_sort_by` | `git.rs:504` | Use `sort_by_key` |
| `unnecessary_lazy_evaluations` | `radar.rs:184` | `unwrap_or_else` → `unwrap_or` |
| `print_literal` | `recover.rs:71` | Inline `"Hash"` |
| `collapsible_match` | `review.rs:397` | Collapse `if` into match guard |

Pedantic clippy reveals ~60 additional warnings, mostly:
- Missing `# Errors` / `# Panics` doc sections on public functions
- `format!` variables that could be inlined (`format!("{}", x)` → direct interpolation)
- Several `unnecessary_boolean_not` patterns
- A few `format!(..)` appended to `String` (should use `write!` or `push_str`)

---

## Positives

- **Clean error types:** `GitError` uses `thiserror` correctly with meaningful variants — exactly what the rust-developer skill recommends.
- **`RunOpts` pattern:** Threading verbose/dry_run via a `Copy` struct is clean and consistent across the codebase. The recent refactor from individual parameters was a great improvement.
- **Dry-run support:** Every git operation respects `dry_run` — solid safety-first design.
- **Good test coverage on validators:** `commit.rs` has thorough unit tests for all lint rules with edge cases covered.
- **`review.rs` unit tests:** Small pure functions (`short_hash`, `extract_issue_number`) are well-tested with boundary cases.
- **Intent log system:** Clean separation between recording, formatting, and consuming breadcrumbs.
- **Config design:** `Config` uses `#[derive(Default, Serialize, Deserialize)]` idiomatically with `serde(default)` for optional fields.

---

## Risks / Follow-ups

1. **review.rs is the #1 refactoring target.** At 1,196 lines with 8 functions over 50 lines, it's the highest-risk module for maintenance issues. A split into sub-modules would be the single highest-impact improvement.

2. **main.rs dispatch** should be extracted before adding any new commands — each new command makes the 299-line `main()` worse.

3. **`handle_commit()` at 193 lines** is a close second for refactoring priority. The function does too many things and is hard to test in isolation.

4. **No `.unwrap()` regression risk.** The recent fix removed dangerous unwraps from production code. The remaining `.unwrap()` calls are confined to tests and safe `unwrap_or`/`unwrap_or_default` patterns. ✅

5. **Pedantic clippy debt** (~60 warnings) is manageable but should be addressed incrementally. The `format!` variable inlining and `# Errors` doc sections are low-effort improvements.

