specs/010-email-patch-import/review.md

Ref: main   Size: 5.6 KiB

# Pre-Implementation Review

**Feature**: Email/Format-Patch Import
**Artifacts reviewed**: spec.md, plan.md, tasks.md, checklists/requirements.md
**Review model**: Claude Opus 4.6 (1M context)
**Generating model**: Unknown (spec/plan generated in earlier session)

## Summary

| Dimension | Verdict | Issues |
|-----------|---------|--------|
| Spec-Plan Alignment | PASS | All user stories and requirements addressed |
| Plan-Tasks Completeness | PASS | Every plan component has corresponding tasks |
| Dependency Ordering | PASS | Correct phase ordering, no circular dependencies |
| Parallelization Correctness | PASS | [P] markers accurate, no same-file conflicts in parallel groups |
| Feasibility & Risk | WARN | Two API concerns worth noting (see findings) |
| Standards Compliance | PASS | Constitution is a template (not customized), no violations possible |
| Implementation Readiness | WARN | One task needs more specificity (see findings) |

**Overall**: READY WITH WARNINGS

## Findings

### Critical (FAIL -- must fix before implementing)

None.

### Warnings (WARN -- recommend fixing, can proceed)

1. **`Diff::from_buffer` lifetime mismatch**: `git2::Diff::from_buffer()` returns `Diff<'static>` but `repo.apply_to_tree()` accepts `&Diff<'_>`. This will work, but the diff must be parsed from the raw diff portion of the patch file only (from `diff --git` onward), not the full mbox content. The plan states this correctly ("everything from `diff --git` onward"), but if the parser includes mbox headers in the buffer passed to `from_buffer()`, it will fail silently or produce wrong results. **Recommendation**: T011 should explicitly note that only the diff portion (starting at `diff --git`) is passed to `Diff::from_buffer()`.

2. **CLI `files` field design for T022**: The plan describes changing `file: PathBuf` to accept multiple files for series mode, but clap's `Vec<PathBuf>` with `num_args = 1..` would make the single-file case also use a `Vec`. T022 and T023 should clarify that `import()` receives `&[PathBuf]` with the first element used for single mode and the full slice for series mode, or use separate positional args. This is a minor design choice but should be decided before implementation to avoid rework.

3. **Author signature extraction from patch file**: T011 describes parsing `From:` / `Date:` headers to preserve the original contributor's author identity. The `git2::Signature` requires name, email, and a `Time` struct. Parsing RFC 2822 dates (as used in `git format-patch` output) into `git2::Time` requires manual conversion -- there is no built-in parser in git2. The `chrono` crate (already a dependency) can parse RFC 2822 via `DateTime::parse_from_rfc2822()` and convert to a Unix timestamp for `git2::Time::new()`. This is feasible but non-trivial and should be explicitly called out in T011.

4. **`apply_to_tree` failure modes**: When `apply_to_tree()` fails due to conflicts, git2 returns a generic `Error` with class `Apply`. The error message may not be descriptive enough for the user. T026 mentions binary diffs but the plan should also consider that `apply_to_tree` does not produce merge conflicts in the traditional sense -- it simply fails. The error handling in T013 should wrap the git2 error with a user-friendly message like "patch does not apply cleanly to base branch '<name>'".

### Observations (informational)

1. **No new crates needed**: Confirmed that `git2::Diff::from_buffer()` and `Repository::apply_to_tree()` both exist in git2 0.19.0. Signatures verified against the vendored source.

2. **Existing `Error` enum already has `Io` variant**: The `std::io::Error` variant in `src/error.rs` already covers file-not-found cases. The proposed `PatchFileNotFound(PathBuf)` variant in T002 is more descriptive but overlaps. Either approach works; the explicit variant gives better error messages.

3. **US2 is pure verification**: Correctly identified as requiring no new code. The 4 test tasks (T015-T018) validate that `patch::create()` produces DAG entries fully compatible with existing `show/review/comment/merge` operations. This is the right approach.

4. **Temp branch naming**: The plan uses `refs/heads/collab/imported/<short-oid>`. This creates branches visible in `git branch` output. Consider whether these should be cleaned up after merge, or documented as expected behavior. The spec does not address cleanup.

5. **Duplicate import (EC-004)**: The spec explicitly states "two separate DAG entries are created." This is correct and matches the existing `patch::create()` behavior -- each call creates a new orphan commit with a new OID.

6. **Working tree safety (FR-007)**: Using `apply_to_tree()` operates entirely in-memory on tree objects, never touching the working directory or index. This is the correct approach and satisfies FR-007 by design.

7. **Task count is proportional**: 29 tasks for a medium-complexity feature (new subcommand, file parsing, git operations, series mode) is appropriate. The breakdown avoids tasks that are too large or too granular.

## Recommended Actions

- [ ] Clarify in T011 that `Diff::from_buffer()` receives only the diff portion (from first `diff --git` line), not the full mbox content
- [ ] Clarify in T011 that RFC 2822 date parsing uses `chrono::DateTime::parse_from_rfc2822()` converted to `git2::Time::new(unix_timestamp, offset_minutes)`
- [ ] Decide in T022 whether `Import` uses `files: Vec<PathBuf>` from the start (with single-file being `files.len() == 1`) or keeps `file: PathBuf` until the series phase
- [ ] Ensure T013 wraps `apply_to_tree()` errors with user-friendly context about which base branch the patch failed to apply against