# Code Quality Review — tbdflow v0.25.1

**Date:** 2026-03-21
**Reviewer:** code-analyzer skill
**Scope:** Full source — `src/` (5,417 LOC), `tests/`, `Cargo.toml`, hooks
**Build:** ✅ Compiles — **22 clippy warnings** (17 lib + 5 bin)
**Tests:** ✅ 18 passed, 0 failed

---

## Findings

### 1. [High] `handle_info` has swapped `verbose` / `dry_run` parameters

- **File:** `src/misc.rs:182`
- **Smell:** Misleading function signature (Long Parameter List + Inconsistent Names)
- **Why it matters:** The signature is `(dry_run, verbose, edit)` but every other
  function in the codebase uses `(verbose, dry_run, …)`. The call-site in `main.rs:48`
  passes `(verbose, dry_run, edit)` — meaning verbose mode silently acts as dry-run
  and vice versa. All git operations inside `handle_info` use hardcoded `(false, false)`
  so the bug is currently masked, but any future use of the parameters will misbehave.
- **Suggested change:** Reorder signature to `(verbose: bool, dry_run: bool, edit: bool)`
  to match the project convention, or introduce a shared `RunContext` struct.

### 2. [High] `handle_commit` takes 13 positional parameters

- **File:** `src/commit.rs:192-206`
- **Smell:** Long Parameter List / Data Clump
- **Why it matters:** Clippy warns (`too_many_arguments`). 13 bool/Option params invite
  call-site ordering mistakes (see finding #1). Several params travel together everywhere
  (type, scope, message, body, breaking, breaking_description, tag, issue).
- **Suggested change:** Extract a `CommitParams` struct:
  ```rust
  pub struct CommitParams {
      pub r#type: String,
      pub scope: Option<String>,
      pub message: String,
      pub body: Option<String>,
      pub breaking: bool,
      pub breaking_description: Option<String>,
      pub tag: Option<String>,
      pub issue: Option<String>,
      pub include_projects: bool,
  }
  ```
  This also aligns with the existing `CommitWizardResult` in `wizard.rs`.

### 3. [High] Hand-rolled JSON parsing in `review.rs` is fragile

- **Files:** `src/review.rs:866` (`extract_body_from_json`), `src/review.rs:1236`
  (`extract_issue_number`), `src/review.rs:964` (`extract_repo_from_json`)
- **Smell:** Leaky Abstraction / Reinvented Wheel
- **Why it matters:** `extract_issue_number` fails on `"number": 42` (space after colon) —
  documented as a known limitation in the test at line 1298. `extract_body_from_json`
  only handles `\\`, `\"`, `\n` escapes, missing `\t`, `\r`, `\uXXXX`. GitHub CLI output
  format may vary across versions.
- **Suggested change:** Add `serde_json` as a dependency and parse properly. All three
  functions become trivial one-liners with `serde_json::Value`.

### 4. [High] `regex::Regex::new().unwrap()` panics on invalid user config

- **File:** `src/commit.rs:107`
- **Smell:** Unwrap Panic Risk
- **Why it matters:** The regex pattern comes from `.tbdflow.yml` (user-controlled). An
  invalid pattern like `[unclosed` causes a panic with no user-friendly message.
- **Suggested change:**
  ```rust
  let re = regex::Regex::new(issue_key_pattern)
      .map_err(|e| anyhow::anyhow!("Invalid issue_key pattern '{}': {}", issue_key_pattern, e))?;
  ```
  (Requires changing the return type from `bool` to `Result<bool>`.)

### 5. [High] Double `git push` in `handle_complete`

- **File:** `src/branch.rs:109,114`
- **Smell:** Duplicate Code
- **Why it matters:** `git::push()` is called at line 109 and again at line 114 with
  no intervening state change. The second push is redundant for all branch types and
  makes non-release completes push twice.
- **Suggested change:** Remove the push at line 114.

### 6. [Medium] `review.rs` is a Large Module (1,304 lines, 31 functions)

- **File:** `src/review.rs`
- **Smell:** Bloater / God Module
- **Why it matters:** The module mixes CLI handler dispatch, GitHub API integration,
  JSON parsing, label management, and commit status updates. This makes it hard to test
  the business logic in isolation and increases change surface for any review feature work.
- **Suggested change:** Split into sub-modules:
  - `review/mod.rs` — public handlers and orchestration
  - `review/github.rs` — all `gh` CLI interactions (issue CRUD, labels, workflow)
  - `review/parse.rs` — JSON extraction helpers (or remove if `serde_json` is adopted)

### 7. [Medium] `git.rs` is a Large Module (921 lines, 66 functions)

- **File:** `src/git.rs`
- **Smell:** Bloater
- **Why it matters:** This is a flat list of thin wrappers around `git` subcommands plus
  radar diff helpers, CI status checks, and unit tests. The file is navigable but growing.
- **Suggested change:** Consider grouping into sections with clear `// ──` separators
  (some already exist) or extracting `git/ci.rs` and `git/radar.rs` helpers.

### 8. [Medium] `misc.rs` is a catch-all module (676 lines, 10 functions)

- **File:** `src/misc.rs`
- **Smell:** Bloater / Lazy Module (misleading name)
- **Why it matters:** Contains `init`, `info`, `sync`, `undo`, `check_branches`,
  `manpage`, and utility functions. The name "misc" obscures intent.
- **Suggested change:** Rename to `commands.rs` or split into `init.rs`, `sync.rs`,
  `undo.rs` etc. — each command gets its own module.

### 9. [Medium] Dead code: `handle_info_command` is never called

- **File:** `src/misc.rs:156-179`
- **Smell:** Dispensable / Dead Code
- **Why it matters:** This is the old info handler, superseded by `handle_info` at
  line 182. It inflates the module and confuses readers.
- **Suggested change:** Delete `handle_info_command`.

### 10. [Medium] Duplicated monorepo-aware status pattern (3 locations)

- **Files:** `src/main.rs:161-194`, `src/misc.rs:451-476`, `src/commit.rs:307-326`
- **Smell:** Shotgun Surgery / Duplicate Code
- **Why it matters:** The logic to detect project root → scope git status → exclude
  project dirs is copy-pasted. A change to monorepo behaviour requires edits in three
  places.
- **Suggested change:** Extract a `get_scoped_status(config, verbose, dry_run)` helper.

### 11. [Medium] `Option::unwrap()` after `is_none()` check in `main.rs`

- **File:** `src/main.rs:105,107,152`
- **Smell:** Unwrap Panic Risk / Non-idiomatic
- **Why it matters:** Clippy warns. The `if type.is_none() || message.is_none()` guard
  doesn't prove both are `Some` in the else branch, so `unwrap()` is technically unsafe
  under refactoring.
- **Suggested change:** Use `if let (Some(t), Some(m)) = (r#type, message) { … }` or
  destructure with `match`.

### 12. [Medium] `serde_yaml` is deprecated

- **File:** `Cargo.toml:22`
- **Smell:** Deprecated Dependency
- **Why it matters:** The `serde_yaml` crate is [officially deprecated](https://github.com/dtolnay/serde-yaml).
  Cargo shows `serde_yaml v0.9.34+deprecated` during compilation.
- **Suggested change:** Migrate to `serde_yml` or `yaml-rust2` + `serde`.

### 13. [Medium] `get_branch_log` hardcodes `origin/main`

- **File:** `src/git.rs:428`
- **Smell:** Tight Coupling to convention
- **Why it matters:** The function ignores the configured `main_branch_name`. Repos using
  `trunk` or `master` get wrong results.
- **Suggested change:** Add a `main_branch: &str` parameter.

### 14. [Low] Unnecessary `.clone()` in `is_valid_scope`

- **File:** `src/commit.rs:263`
- **Smell:** Clone Abuse
- **Why it matters:** `is_valid_scope(&Some(s.clone()), config)` clones a `String` only
  to pass it as `&Option<String>`. The function should take `&Option<&str>` or just `&str`.
- **Suggested change:** Refactor `is_valid_scope` to take `Option<&str>`.

### 15. [Low] `to_option` helper is buried inside `run_commit_wizard`

- **File:** `src/wizard.rs:85-91`
- **Smell:** Poor discoverability
- **Why it matters:** The pattern of converting empty strings to `None` is common. A
  module-level helper or a trait extension would be cleaner.
- **Suggested change:** Move to module level or use `.filter(|s| !s.is_empty())`.

### 16. [Low] No unit tests for linting functions

- **File:** `src/commit.rs`
- **Smell:** Missing Error Context (test gap)
- **Why it matters:** `is_valid_commit_type`, `is_valid_scope`, `is_valid_subject_line`,
  `is_valid_body_lines`, and `is_valid_issue_key` are pure functions ideal for unit
  testing but have zero test coverage.
- **Suggested change:** Add a `#[cfg(test)] mod tests` in `commit.rs`.

### 17. [Low] `config::find_project_root` uses `parent().unwrap()`

- **File:** `src/config.rs:490`
- **Smell:** Unwrap Panic Risk
- **Why it matters:** The `None` case is guarded by `|| current_dir.parent().is_none()`
  in the loop condition (line 487), but the guard uses `||` so both conditions could be
  false while `parent()` is `None` in edge cases on some OS root paths.
- **Suggested change:** Use `if let Some(parent) = current_dir.parent()` for safety.

---

## Clippy / Compiler Warnings (22 total)

| Category | Count | Files |
|----------|-------|-------|
| `useless_format!` | 5 | misc.rs, review.rs |
| `needless_borrow` | 4 | branch.rs, commit.rs, git.rs |
| `unnecessary_unwrap` | 4 | main.rs |
| `useless_conversion` | 2 | git.rs |
| `collapsible_if` | 2 | main.rs, commit.rs |
| `too_many_arguments` | 1 | commit.rs |
| `derivable_impls` | 1 | config.rs |
| `cmp_owned` | 1 | misc.rs |
| `single_match` | 1 | radar.rs |
| `map_clone` | 1 | wizard.rs |

All are auto-fixable except the structural ones (`too_many_arguments`, `unnecessary_unwrap`).

---

## Positives

- **Clean module boundaries:** `cli`, `config`, `git`, `commit`, `branch`, `changelog`,
  `radar`, `review`, `wizard` each have a clear responsibility.
- **Consistent `verbose` / `dry_run` threading:** Every git operation supports both flags,
  providing excellent debuggability and safety.
- **Good use of `thiserror` for `GitError`:** Structured error variants with meaningful
  messages.
- **Radar system is well-tested:** 9 unit tests covering hunk overlap, ignore patterns,
  and formatting. Creative and genuinely useful feature.
- **Interactive wizard fallback:** When CLI flags are missing, guided prompts kick in —
  great UX for new users.
- **Pre-commit hook exists:** `hooks/pre-commit` enforces `cargo fmt`. Just needs
  auto-installation.
- **No CVEs:** All 12 direct dependencies are clean.
- **All 18 tests pass** consistently.

---

## Risks / Follow-ups

1. **Parameter swap bug (#1)** is live and silent — should be fixed before next release.
2. **`serde_yaml` deprecation (#12)** — plan migration before it stops receiving patches.
3. **`review.rs` complexity (#6)** — any new review feature will make this worse. Split
   before adding more strategies.
4. **JSON parsing fragility (#3)** — a `gh` CLI update changing whitespace in JSON output
   could silently break all review operations.
5. **Linting functions lack tests (#16)** — these are the commit quality gatekeepers; a
   regression here silently lets bad commits through.

