gitgrip 0.19.0

# Gitgrip Architectural Assessment

*February 2026 — Codebase: ~30,222 lines across 77 source files*

This report identifies concrete modifications to improve quality, extensibility, and maintainability, with particular focus on unsafe operations and non-atomic concurrency. Three parallel audits were conducted: safety/concurrency, architecture quality, and CLI/API design.

---

## 1. Safety & Concurrency (CRITICAL)

### 1.1 Unsafe Code

**4 instances total — ALL in test code (no unsafe in production `src/`)**

| File | Line(s) | Usage | Status |
|------|---------|-------|--------|
| `tests/common/mock_platform.rs` | 18-20 | `Once::call_once(\|\| unsafe { set_var(...) })` | OK — guarded by `Once` |
| `tests/common/mock_platform.rs` | 626-628 | `unsafe { set_var("BITBUCKET_TOKEN", ...) }` | **FIX** — missing `Once` guard |
| `tests/test_platform_github_auth.rs` | 14-16 | `unsafe { set_var("GITHUB_TOKEN", ...) }` | OK — test guard cleanup |
| `tests/test_platform_github_auth.rs` | 24-27 | `unsafe { remove_var(...) }` | OK — test isolation |

**Action:** Add `Once` guard to `setup_bitbucket_mock()` at `tests/common/mock_platform.rs:626` to match the pattern already used in `setup_github_mock()`.

### 1.2 Thread Panic Handling (CRITICAL)

`handle.join().unwrap()` will propagate panics from worker threads, crashing the entire CLI:

| File | Line | Context |
|------|------|---------|
| `src/cli/commands/grep.rs` | 150 | Parallel grep across repos |
| `src/cli/commands/forall.rs` | 1011 | Parallel forall across repos |

**Fix:** Replace `.unwrap()` with proper error handling:
```rust
// Before:
handle.join().unwrap();

// After:
handle.join().map_err(|_| anyhow::anyhow!("Worker thread panicked"))?;
```

### 1.3 Mutex Poison Vulnerability (HIGH)

All `.lock().unwrap()` calls will panic if any thread panics while holding the lock, causing cascading failures:

| File | Lines | Count |
|------|-------|-------|
| `src/cli/commands/forall.rs` | 1002, 1015 | 2 |
| `src/cli/commands/grep.rs` | 141, 153 | 2 |
| `src/git/cache.rs` | 46, 67, 79, 85 | 4 |
| `src/telemetry/metrics.rs` | 32, 46, 53, 66-68, 79-81 | 6 |
| `src/cli/commands/sync.rs` | 411 | 1 |
| `src/cli/commands/pull.rs` | 137 | 1 |

**Fix options (pick one):**
1. Replace all `.lock().unwrap()` with `.lock().expect("mutex poisoned")` (minimal)
2. Switch to `parking_lot::Mutex` which never poisons (recommended — also slightly faster)

### 1.4 Non-Atomic Multi-Repo Operations (MODERATE)

| Issue | File | Impact |
|-------|------|--------|
| Git clone fallback not atomic — partial dir left on interruption | `src/git/mod.rs:73-128` | Corrupted workspace state |
| Parallel sync: if repo 5/10 fails, repos 1-4 already synced | `src/cli/commands/sync.rs:376-413` | Inconsistent workspace |
| No git lock handling (`.git/index.lock`) | `src/git/` (missing) | Concurrent ops fail mysteriously |

**Actions:**
- Add cleanup of partial clone directory on fallback failure in `clone_repo()`
- Add git lock detection with retry/wait logic in `src/git/mod.rs`
- Results are already reported per-repo (mitigates sync inconsistency)

---

## 2. Architecture Quality

### 2.1 Monolithic `main.rs` (1,264 lines)

The single largest maintainability issue. Contains:
- `Commands` enum: 345 lines (~25+ variants)
- Sub-command enums: 223 lines
- `main()` match dispatch: 575 lines with repetitive patterns

Each match arm follows the same pattern: `load_gripspace()` -> extract args -> call handler. This is pure boilerplate.

**Recommendation:** Extract a `CommandHandler` trait:
```rust
trait CommandHandler {
    fn run(&self, ctx: &WorkspaceContext) -> anyhow::Result<()>;
}
```
Each command struct implements the trait. The match block becomes a single dispatch call. Enables future plugin/dynamic command registration.

### 2.2 Code Duplication: Repo Iteration (20+ instances)

Nearly identical `for repo in &repos { ... }` loops with similar error handling across: `branch.rs`, `sync.rs`, `pull.rs`, `gc.rs`, `rebase.rs`, `commit.rs`, `add.rs`, `cherry_pick.rs`, etc.

**Recommendation:** Extract a shared helper:
```rust
fn apply_to_repos<F>(repos: &[RepoInfo], op: F) -> Vec<RepoResult>
where F: Fn(&RepoInfo) -> anyhow::Result<String>
```
Reduces duplication by ~30% and standardizes error collection/reporting.

### 2.3 Output Inconsistency

- 342 direct `println!`/`eprintln!` calls
- ~80 `Output::` method calls
- Mixed usage within the same command files

**Recommendation:** Consolidate all output through `Output::` methods. Add `Output::debug()` for verbose-mode output. This also enables future features like `--quiet` mode and machine-readable output.

### 2.4 Large Functions

| Function | File | Lines | Recommendation |
|----------|------|-------|----------------|
| `main()` dispatch | `src/main.rs` | ~575 | CommandHandler trait (2.1) |
| `run_sync()` | `src/cli/commands/sync.rs` | ~200 | Extract `SyncStrategy` trait (parallel/sequential) |
| `run_release()` | `src/cli/commands/release.rs` | ~250 | Break into phases: Bump, PR, Release |
| `run_init_from_dirs()` | `src/cli/commands/init.rs` | ~180 | Extract initialization strategies |
| `run_forall()` | `src/cli/commands/forall.rs` | ~150 | Extract command interception logic |
| `load_gripspace()` | `src/main.rs` | ~88 | Split: `load_griptree()`, `load_gripspace()`, `load_legacy_repo()` |

### 2.5 Missing Trait Abstractions

| Area | Current State | Recommendation |
|------|--------------|----------------|
| Git operations | Free functions in `git/mod.rs` | `GitBackend` trait (enables git2 vs gitoxide swap) |
| CLI commands | Separate `pub fn run_*()` functions | `CommandHandler` trait (enables dynamic registration) |
| Output | Static methods on `Output` struct | `OutputSink` trait (enables testing, quiet mode) |

### 2.6 Global State (Acceptable)

Only 2 global singletons, both justified:
- `STATUS_CACHE` in `src/git/cache.rs:97` — 5s TTL, properly invalidated
- `GLOBAL_METRICS` in `src/telemetry/metrics.rs:11` — feature-gated, has `reset()` for tests

### 2.7 Dead Code (Minimal — 6 instances)

All in `telemetry/` (spans.rs, init.rs) and `forall.rs`/`pr/create.rs`. Suggests incomplete telemetry integration. Low priority — add comments explaining why `#[allow(dead_code)]` is needed.

---

## 3. CLI & API Design

### 3.1 JSON Output Gaps (HIGH)

`--json` flag exists globally but is NOT implemented in most commands:

| Command | JSON Support | Status |
|---------|-------------|--------|
| `status` | Yes | Working |
| `sync` | Yes | Working |
| `branch` | **Has param, no output code** | **BROKEN** |
| `push` | Yes (partial) | Working |
| `checkout`, `add`, `commit` | No | Missing |
| `rebase`, `forall`, `grep` | No | Missing |

**Action:** Implement JSON output for `branch` (it already accepts the flag) and progressively add to other commands.

### 3.2 Flag Inconsistencies

| Issue | Location | Fix |
|-------|----------|-----|
| `Status` has local `verbose` that shadows global | `main.rs:79` | Remove local, use global |
| `--json` has no `-j` shortcut | `main.rs:20` | Add `short = 'j'` |
| `--group` not available on all commands | `add`, `commit`, `push` | Add `--group` filter where missing |

### 3.3 Platform Adapter Gaps

**GitHub:**
- `enable_auto_merge()` uses `gh` CLI — doesn't work with GHE (TODO at line 379)
- `create_repository()`, `delete_repository()`, `update_branch()`, `create_release()` — all stubs

**GitLab:** `update_branch()`, `enable_auto_merge()`, `create_release()` — stubs

**Azure DevOps:** `update_branch()`, `enable_auto_merge()`, `create_release()` — stubs

**Bitbucket:** Most optional methods are stubs

**Action:** Document a platform capability matrix. Fix GHE auto-merge by passing `--hostname` to `gh`.

### 3.4 Error Message Quality

Generally good, but some gaps:
- `sync.rs:668`: "Repositories are on different branches: X vs Y" — doesn't say which repos
- Platform errors use generic `ApiError(String)` — could be more specific
- `pr/merge.rs`: merge failure errors lack repo/PR number context

### 3.5 Git Lock Handling (MISSING)

No `.git/index.lock` detection anywhere in `src/git/`. Concurrent operations (parallel sync, forall) can hit lock contention with no retry or helpful error message.

**Action:** Add lock detection with retry in `src/git/mod.rs`.

---

## 4. Test Coverage

### Overall: 38% test-to-source ratio (44 test files, 11,600 lines)

### Critical Gaps

| Module | Coverage | Gap |
|--------|----------|-----|
| `src/cli/commands/cherry_pick.rs` | **None** | No unit tests |
| `src/cli/commands/gc.rs` | **None** | No unit tests |
| `src/core/griptree.rs` | **None** | No unit tests despite complexity |
| `src/platform/azure.rs` | Sparse | Core only |
| `src/platform/bitbucket.rs` | Sparse | Core only |
| `src/git/cherry_pick.rs` | **None** | No tests |
| `src/git/gc.rs` | **None** | No tests |
| `src/git/remote.rs` | **None** | No unit tests |
| Error paths across commands | Sparse | Mostly happy-path testing |

---

## 5. Dependencies

- `tokio = { features = ["full"] }` — could trim to specific features for faster compile (~5-10%)
- `octocrab` with `rustls-webpki-tokio` — correct choice (fixed earlier this session)
- Regex patterns compiled per-call in `release.rs:139,170` — should use `once_cell::Lazy`
- 720 `String::from()`/`.to_string()` calls — consider `Cow<'_, str>` for hot paths

---

## 6. Prioritized Action Items

### P0 — Must Fix (Safety)
1. **Thread panic handling** — Replace `.join().unwrap()` in `grep.rs:150`, `forall.rs:1011`
2. **Mutex poison risk** — Switch to `parking_lot::Mutex` or add `.expect()` messages
3. **Bitbucket mock race** — Add `Once` guard to `setup_bitbucket_mock()` in tests

### P1 — Should Fix (Quality)
4. **Fix broken JSON output** for `branch` command (has param, no implementation)
5. **Add git lock detection** with retry logic
6. **Fix GHE auto-merge** — pass `--hostname` to `gh` CLI call
7. **Add tests** for `cherry_pick`, `gc`, `griptree` modules

### P2 — Improve (Maintainability)
8. **Refactor main.rs** — Extract `CommandHandler` trait, reduce 575-line match block
9. **Extract repo iteration helper** — Eliminate 20+ duplicate loops
10. **Break up large functions** — `run_sync()`, `run_release()`, `load_gripspace()`
11. **Consolidate output** — Route all output through `Output::` methods

### P3 — Nice to Have (Extensibility)
12. **Add `GitBackend` trait** — Abstract over git2/gitoxide
13. **Add `OutputSink` trait** — Enable testing and quiet mode
14. **Document platform capability matrix**
15. **Trim tokio features** for faster compilation
16. **Add multi-repo rollback/transaction semantics**