specs/012-patch-branch-refactor/review.md
Ref: Size: 8.2 KiB
# Pre-Implementation Review: 012-patch-branch-refactor
**Reviewer**: Cross-model review (Claude Opus 4.6)
**Date**: 2026-03-21
**Verdict**: APPROVED with minor findings
## Summary Table
| Check | Status | Notes |
|-------------------------------|--------|--------------------------------------------------------------|
| Spec-Plan Alignment | PASS | All 5 user stories addressed in plan |
| Plan-Tasks Completeness | PASS | 38 tasks cover all plan items |
| Dependency Ordering | PASS | Phases correctly ordered; parallel opportunities valid |
| `graph_ahead_behind()` exists | PASS | Confirmed in git2 0.19 Repository docs (2 references found) |
| `refname_to_id()` works | PASS | Already used 11+ times across codebase |
| Serde optional field compat | PASS | Pattern already used for `fixes: Option<String>` on PatchCreate |
| CLI `--head` to `--branch` | PASS | clap derive supports hidden deprecated aliases |
| Breaking changes risk | WARN | See finding F3 below |
| Migration issues | PASS | No migration needed; `Option` deserialization handles it |
| TDD coverage | WARN | See finding F5 below |
## Detailed Findings
### F1: Serde Backward Compat Pattern Already Proven (PASS)
The plan proposes adding `branch: Option<String>` with `#[serde(default, skip_serializing_if = "Option::is_none")]`. This exact pattern is already used in the codebase for the `fixes` field on `Action::PatchCreate` (event.rs:51). Deserialization of old events without the field will work correctly, producing `None`.
### F2: git2 API Feasibility Confirmed (PASS)
- **`repo.graph_ahead_behind(oid1, oid2)`**: Present in git2 0.19. Returns `(usize, usize)` for (ahead, behind). Confirmed in generated docs.
- **`repo.refname_to_id()`**: Already used extensively in `patch.rs`, `dag.rs`, `sync.rs`, `signing.rs`, and `issue.rs`. Well-tested pattern.
- **`repo.merge_base()`**: Already used in `dag.rs` for sync reconciliation.
### F3: CLI Breaking Change Risk (WARN - Low)
The plan changes `--head` to `--branch` with `--head` kept as a hidden alias. Current code in `cli.rs:169` shows `--head` is `Option<String>` defaulting to HEAD when `None`. The plan's new `--branch` flag has the same default behavior (current branch from `repo.head()`).
**Risk**: The `--head` flag currently accepts a commit OID (e.g., `--head abc123`). The new `--branch` flag accepts a branch name. If users pass OIDs to `--branch`, the auto-create-branch-from-detached-HEAD logic in T017 must handle this case. The plan mentions this (spec line 23: "detached HEAD or commit OID"), but T017's description is vague on the OID-to-branch path.
**Recommendation**: T017 should explicitly state: "if `--branch` value is not a valid ref but IS a valid OID, create branch `collab/patch/{short-oid}` pointing at that OID."
### F4: `patch revise` Interaction with Branch Model (PASS with note)
The plan says `patch revise` becomes optional for branch-based patches (T028). Current `revise()` in `patch.rs:112-131` stores a new `head_commit` in a `PatchRevise` event, and `PatchState::from_ref()` (state.rs:216-223) updates `self.head_commit` from it.
For branch-based patches, `resolve_head()` (T004) resolves from the branch ref, so `PatchRevise` events updating `head_commit` would be ignored (branch takes precedence). This is correct behavior, but worth documenting for developers: **revise events on branch-based patches only serve as notes, not as head updates**.
### F5: TDD Coverage Gaps (WARN - Medium)
The task list has 14 test tasks (T007-T012, T019-T021, T024-T025, T027, T029-T031). Missing test coverage:
1. **No test for branch deletion edge case** (spec line 95-96, FR-009). The TUI task T035 handles display, but no test verifies `resolve_head()` returns a clear error when the branch is deleted.
2. **No test for `patch import` with branch model** (T036 implementation but no test task). Import currently creates a detached commit and passes the OID to `patch::create()`. After refactor, it needs to create a branch first. This is a behavior change that should be tested.
3. **No test for duplicate patch detection across branch renames**. If branch "foo" has a patch and the user renames it to "bar", then creates a patch for "bar", the duplicate check (T016) passes but the old patch for "foo" now has a dangling branch reference.
**Recommendation**: Add T039 (test for deleted branch error), T040 (test for import creating a branch), and consider whether branch rename is an edge case worth covering.
### F6: `generate_diff()` Currently Does Two-Dot Diff (PASS with note)
Current `generate_diff()` in `patch.rs:222-266` diffs the base branch tip tree against the head commit tree. This is two-dot semantics. The plan (T022) correctly identifies this needs to change to three-dot (merge-base to branch tip) for branch-based patches, while keeping old behavior for OID-based patches.
The current code at `patch.rs:232-237` resolves the base via `refname_to_id`. The refactored version needs to first find the merge-base between base tip and branch tip, then diff merge-base tree against branch tip tree. This is straightforward with the existing git2 APIs.
### F7: Spec-Plan-Tasks Alignment Verified (PASS)
| User Story | Spec Section | Plan Section | Tasks | Count |
|-----------|-------------|-------------|-------|-------|
| US1: Create from branch | Lines 10-24 | "CLI Changes" + "Duplicate Prevention" | T009-T018 | 10 |
| US2: Staleness and diff | Lines 27-41 | "Diff via Merge-Base" | T019-T023 | 5 |
| US3: Merge as git merge | Lines 44-58 | "Merge via Branch" | T024-T026 | 3 |
| US4: Implicit revision | Lines 61-74 | "Revise Becomes Optional" | T027-T028 | 2 |
| US5: Backward compat | Lines 77-90 | "Migration Strategy" | T029-T032 | 4 |
| Cross-cutting | Edge cases | Plan sections 7-8 | T033-T038 | 6 |
All functional requirements (FR-001 through FR-011) map to at least one task. All success criteria (SC-001 through SC-006) are covered by the test tasks.
### F8: Phase Dependency Analysis (PASS)
The dependency graph is correct:
```
Phase 1 (schema) --> Phase 2 (resolution helpers)
|
+----------+----------+----------+
| | | |
Phase 3 Phase 5 Phase 7 Phase 8
(US1) (US3) (US5) (TUI)
|
Phase 4 Phase 6
(US2) (US4)
|
Phase 9 (polish)
```
Phase 5 (US3/merge) is listed as depending on Phase 2, not Phase 3. This is correct because merge only needs `resolve_head()` from Phase 2, and the existing `merge()` function already works with OID-based patches.
Phase 6 (US4) depends on Phase 3 because implicit revision requires branch-based patches to exist. This is correct.
### F9: Existing Merge Function Already Handles Non-FF (PASS)
The existing `merge()` in `patch.rs:133-212` already handles fast-forward, same-commit, and non-fast-forward (merge commit) cases, including conflict detection. The refactor (T026) only needs to change how `head_commit` is resolved (branch ref vs stored OID). The merge logic itself remains unchanged.
## Recommendations
1. **Add missing test tasks** for branch deletion error handling (F5.1) and patch import with branches (F5.2)
2. **Clarify T017** to handle OID arguments passed to `--branch` (F3)
3. **Document in code** that `PatchRevise` events on branch-based patches are informational only (F4)
4. **Consider** adding a `patch checkout` command that checks out the branch associated with a patch -- natural UX complement to branch-based patches (not blocking, future enhancement)
## Conclusion
The feature is well-designed and feasible. The existing codebase already uses all required git2 APIs, the serde backward-compat pattern is proven, and the migration strategy (no migration needed) is sound. The 38 tasks provide good coverage with 2-3 test gaps that should be addressed before implementation. The phased approach with MVP at Phase 3 is a good risk management strategy.