64e65092
wip: fleet phase 7 -- design artifacts complete
a73x 2026-03-21 18:32
Spec, plan, tasks, checklist, analysis, and review for patch revisions feature. Ready for implementation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
diff --git a/specs/015-gerrit-style-patchsets/.analyze-done b/specs/015-gerrit-style-patchsets/.analyze-done new file mode 100644 index 0000000..d073c13 --- /dev/null +++ b/specs/015-gerrit-style-patchsets/.analyze-done @@ -0,0 +1 @@ 2026-03-21T18:26:19+00:00 - Analysis complete. 0 critical, 2 medium, 4 low findings. diff --git a/specs/015-gerrit-style-patchsets/checklists/revision-model.md b/specs/015-gerrit-style-patchsets/checklists/revision-model.md new file mode 100644 index 0000000..a207bb3 --- /dev/null +++ b/specs/015-gerrit-style-patchsets/checklists/revision-model.md @@ -0,0 +1,65 @@ # Revision Model Requirements Quality Checklist: Patch Revisions **Purpose**: Validate completeness, clarity, and consistency of requirements for the patch revision system **Created**: 2026-03-21 **Feature**: [spec.md](../spec.md) ## Requirement Completeness - [ ] CHK001 - Are requirements defined for what happens when `patch revise` is run on a closed or merged patch? [Completeness, Gap] - [ ] CHK002 - Is the behavior of `patch merge` with `require_approval_on_latest=true` specified when the patch has zero revisions (legacy patch)? [Completeness, Spec §FR-014] - [ ] CHK003 - Are requirements defined for how `patch log` handles legacy patches with no recorded revisions? [Completeness, Spec §FR-010] - [ ] CHK004 - Is the behavior specified for `patch comment --revision N` when N is higher than the current latest revision? [Completeness, Spec §FR-004] - [ ] CHK005 - Are requirements defined for what `patch diff --between N M` shows when N > M (reversed order)? [Completeness, Spec §FR-006] - [ ] CHK006 - Is the `refs/collab/config` schema for merge policy specified (format, how to read/write, default when absent)? [Completeness, Spec §FR-014] - [ ] CHK007 - Are requirements defined for how `patch show --json` includes revision data in its output? [Completeness, Spec §FR-008] ## Requirement Clarity - [ ] CHK008 - Is "topological DAG order" precisely defined for determining revision numbering? Does it mean oldest-first or newest-first? [Clarity, Spec §FR-003a] - [ ] CHK009 - Is the scope of "write operations" that trigger auto-detection exhaustively listed, or could `patch close` also trigger it? [Clarity, Spec §FR-002] - [ ] CHK010 - Is it clear whether `--revision N` on `patch comment` (with `--file`/`--line`) targets revision N's tree for validation, or just records the number without validation? [Clarity, Spec §FR-004] - [ ] CHK011 - Is the meaning of "latest revision" precisely defined when the branch has moved but no write operation has recorded it yet? [Clarity, Spec §Assumptions] - [ ] CHK012 - Is it specified whether the `body` field on auto-detected `PatchRevision` events is always `null`, or could it be populated from some default? [Clarity, Spec §Event Schema] ## Requirement Consistency - [ ] CHK013 - Is the relationship between the existing `PatchRevise` action and the new `PatchRevision` action consistent? The plan says "replaced" but spec says "can be kept as an alias" [Consistency, Spec §Assumptions vs Plan §6] - [ ] CHK014 - Are the `commit`/`tree` field names on `PatchCreate` consistent with the `branch` field already storing the branch name? Could `commit` conflict with the existing `branch` alias for `head_commit`? [Consistency, Spec §Event Schema] - [ ] CHK015 - Is `--revision` flag behavior consistent between `patch comment` (rejects for thread comments) and `patch review` (always allowed)? [Consistency, Contracts §CLI] - [ ] CHK016 - Does the spec's description of `patch show --revision N` filtering align with the contract's definition? Spec says "inline comments and reviews," contract says the same — are thread comments explicitly included or excluded from the filter? [Consistency, Spec §FR-009 vs Contracts §CLI] ## Acceptance Criteria Quality - [ ] CHK017 - Can SC-001 ("patch revise correctly records revision snapshots") be objectively measured without implementation knowledge? [Measurability, Spec §SC-001] - [ ] CHK018 - Is SC-007 ("TUI dashboard displays revision count") testable independently, or does it require visual inspection? [Measurability, Spec §SC-007] - [ ] CHK019 - Are acceptance scenarios for User Story 1 testable for the auto-detection mechanism without race conditions in the test setup? [Measurability, Spec §US-1] ## Scenario Coverage - [ ] CHK020 - Are requirements defined for the interaction between `patch revise` (explicit) and auto-detection when both happen concurrently from different collaborators? [Coverage, Gap] - [ ] CHK021 - Are requirements defined for `patch diff --revision N` when the base branch no longer exists or has been renamed? [Coverage, Edge Case] - [ ] CHK022 - Are requirements specified for how sync handles `PatchRevision` events from a collaborator who has a different branch tip? [Coverage, Spec §FR-012] - [ ] CHK023 - Is the behavior defined for `patch create` when the branch has uncommitted changes (dirty working tree)? [Coverage, Gap] - [ ] CHK024 - Are requirements defined for how the TUI displays patches with many revisions (e.g., 50+)? Truncation? Scrolling? [Coverage, Spec §SC-007] ## Edge Case Coverage - [ ] CHK025 - Is behavior defined when a `PatchRevision` event references a commit OID that no longer exists in the repository (e.g., after garbage collection)? [Edge Case, Gap] - [ ] CHK026 - Are requirements defined for `patch diff --between 1` (single arg shorthand) when there is only one revision? [Edge Case, Spec §FR-006] - [ ] CHK027 - Is behavior specified for `patch comment --revision 0` or `patch comment --revision -1` (invalid revision numbers)? [Edge Case, Gap] - [ ] CHK028 - Are requirements defined for what happens if the tree OID stored in a revision doesn't match the actual tree of the stored commit OID (data corruption)? [Edge Case, Gap] ## Backward Compatibility Requirements - [ ] CHK029 - Is the deserialization behavior for old `PatchRevise` events explicitly specified alongside the new `PatchRevision` events? [Completeness, Spec §Assumptions] - [ ] CHK030 - Are requirements defined for what revision number is assigned to legacy inline comments that have `revision: null`? Is "unknown" a display-only concept or does it affect filtering? [Clarity, Spec §FR-011] - [ ] CHK031 - Is it specified whether `patch show --revision 1` on a legacy patch (no revisions) returns an error or shows all comments? [Coverage, Spec §FR-011] - [ ] CHK032 - Are requirements defined for the bootstrap behavior when auto-detection creates revision 1 on a legacy patch that already has inline comments with `revision: null`? Do those comments remain "unknown" or get retroactively assigned to revision 1? [Completeness, Gap] ## Notes - Check items off as completed: `[x]` - Add comments or findings inline - Items are numbered sequentially for easy reference - Focus areas: revision data model, CLI consistency, backward compatibility, auto-detection edge cases diff --git a/specs/015-gerrit-style-patchsets/contracts/cli.md b/specs/015-gerrit-style-patchsets/contracts/cli.md new file mode 100644 index 0000000..53076db --- /dev/null +++ b/specs/015-gerrit-style-patchsets/contracts/cli.md @@ -0,0 +1,88 @@ # CLI Contract: Patch Revisions ## Modified Commands ### `patch create` No CLI changes. Internally records revision 1 (commit + tree OID) in the PatchCreate event. ### `patch revise <id> [--body <text>]` Repurposed: now appends a `PatchRevision` event instead of `PatchRevise`. - Resolves current branch tip, compares to last recorded revision - If changed: appends `PatchRevision` with commit OID, tree OID, and optional body - If unchanged: exits with error "no changes since revision N" - Exit code: 0 on success, 1 on error ### `patch comment <id> --body <text> [--file <path> --line <n>] [--revision <n>]` New optional flag: `--revision <n>` - Without `--revision`: anchors to the latest revision - With `--revision N`: anchors to revision N (error if revision N doesn't exist) - Thread comments (no `--file`/`--line`): `--revision` is rejected ("thread comments are not revision-scoped") ### `patch review <id> --verdict <v> --body <text> [--revision <n>]` New optional flag: `--revision <n>` - Without `--revision`: anchors to the latest revision - With `--revision N`: anchors to revision N (error if revision N doesn't exist) ### `patch show <id> [--json] [--revision <n>]` New optional flag: `--revision <n>` - Without `--revision`: shows all inline comments/reviews grouped by revision - With `--revision N`: filters to show only inline comments and reviews from revision N - Thread comments always shown regardless of `--revision` filter - JSON output includes `revisions` array and `revision` field on inline comments/reviews ### `patch diff <id> [--revision <n>] [--between <n> [<m>]]` New flags: `--revision <n>` and `--between <n> [<m>]` - No flags: existing behavior (merge-base diff against current branch tip) - `--revision N`: diff between merge-base and revision N's tree (historical full diff) - `--between N M`: interdiff between revision N and revision M trees - `--between N`: shorthand for "revision N to latest revision" - `--revision` and `--between` are mutually exclusive (error if both provided) - Error if referenced revision doesn't exist ## New Command ### `patch log <id> [--json]` Lists all revisions of a patch. **Default output** (human-readable): ``` Revision 1 abc1234 2026-03-21 10:00 (initial) Revision 2 def5678 2026-03-21 14:30 +2 -1 files changed "addressed review feedback" Revision 3 ghi9012 2026-03-21 16:45 +1 files changed ``` **JSON output** (`--json`): ```json [ { "number": 1, "commit": "abc1234...", "tree": "...", "timestamp": "2026-03-21T10:00:00Z", "body": null, "files_changed": null }, { "number": 2, "commit": "def5678...", "tree": "...", "timestamp": "2026-03-21T14:30:00Z", "body": "addressed review feedback", "files_changed": { "added": 2, "removed": 1 } } ] ``` Exit code: 0 on success, 1 if patch not found. diff --git a/specs/015-gerrit-style-patchsets/data-model.md b/specs/015-gerrit-style-patchsets/data-model.md new file mode 100644 index 0000000..c54a895 --- /dev/null +++ b/specs/015-gerrit-style-patchsets/data-model.md @@ -0,0 +1,136 @@ # Data Model: Patch Revisions ## Entities ### Revision A numbered snapshot of the branch state at a point in time. | Field | Type | Description | |-------|------|-------------| | number | u32 | 1-indexed, derived from topological DAG order during materialization | | commit | String | Branch tip commit OID at time of recording | | tree | String | Tree OID of the commit (for fast interdiff without re-walking history) | | body | Option\<String\> | Optional author note (e.g., "addressed review feedback") | | timestamp | String | ISO 8601 timestamp of the PatchRevision event | **Identity**: Revision number is derived, not stored. The commit OID is the true identifier. **Uniqueness**: Within a patch, no two revisions share the same commit OID (dedup during materialization). **Lifecycle**: Append-only. Revisions are never deleted or modified. ### PatchState (modified) Existing entity, gains a `revisions` field. | New Field | Type | Description | |-----------|------|-------------| | revisions | Vec\<Revision\> | Ordered list of all revisions, oldest first | **Derivation**: Revision 1 comes from `PatchCreate` event (if `commit`/`tree` fields present). Subsequent revisions come from `PatchRevision` events in topological order. Duplicates (same commit OID) are skipped. ### InlineComment (modified) Existing entity, gains a `revision` field. | New Field | Type | Description | |-----------|------|-------------| | revision | Option\<u32\> | Revision number this comment was made on. None for legacy comments. | ### Review (modified) Existing entity, gains a `revision` field. | New Field | Type | Description | |-----------|------|-------------| | revision | Option\<u32\> | Revision number this review was made on. None for legacy reviews. | ## Event Schema Changes ### New: `patch.revision` ```rust Action::PatchRevision { commit: String, // branch tip commit OID tree: String, // tree OID of the commit body: Option<String>, // optional revision note } ``` Serialized as: ```json { "type": "patch.revision", "commit": "abc123...", "tree": "def456...", "body": "addressed review feedback" } ``` ### Modified: `patch.create` Add optional fields (backward-compatible via `skip_serializing_if`): ```rust Action::PatchCreate { title: String, body: String, base_ref: String, branch: String, fixes: Option<String>, commit: Option<String>, // NEW: branch tip at creation tree: Option<String>, // NEW: tree OID at creation } ``` ### Modified: `patch.inline_comment` Add optional field: ```rust Action::PatchInlineComment { file: String, line: u32, body: String, revision: Option<u32>, // NEW: revision number at time of commenting } ``` ### Modified: `patch.review` Add optional field: ```rust Action::PatchReview { verdict: ReviewVerdict, body: String, revision: Option<u32>, // NEW: revision number at time of review } ``` ## State Transitions Revisions are append-only. The materialization flow: ``` PatchCreate (with commit/tree) → revision 1 PatchRevision (commit A) → revision 2 PatchRevision (commit A) → SKIPPED (dedup: same commit OID) PatchRevision (commit B) → revision 3 ``` ## Relationships ``` Patch 1──* Revision (a patch has zero or more revisions) Revision 1──* InlineComment (a revision has zero or more inline comments) Revision 1──* Review (a revision has zero or more reviews) Patch 1──* Comment (thread comments belong to the patch, not a revision) ``` ## Config Model ### Merge Policy (in `refs/collab/config`) | Key | Type | Default | Description | |-----|------|---------|-------------| | merge.require_approval_on_latest | bool | false | When true, patch merge requires approval on the latest revision | diff --git a/specs/015-gerrit-style-patchsets/plan.md b/specs/015-gerrit-style-patchsets/plan.md new file mode 100644 index 0000000..56a595d --- /dev/null +++ b/specs/015-gerrit-style-patchsets/plan.md @@ -0,0 +1,150 @@ # Implementation Plan: Patch Revisions **Branch**: `015-gerrit-style-patchsets` | **Date**: 2026-03-21 | **Spec**: [spec.md](spec.md) ## Summary Add numbered revisions to git-collab patches, following the Radicle model (patch + revisions). Each revision snapshots the branch tip (commit OID + tree OID) in the review DAG. Inline comments and reviews are anchored to specific revisions. Interdiff between any two revisions shows exactly what changed. Revision numbers are derived from topological DAG order during materialization, making concurrent detection safe. ## Technical Context **Language/Version**: Rust 2021 edition **Primary Dependencies**: git2 0.19, clap 4 (derive), serde/serde_json 1, chrono 0.4, ed25519-dalek 2 **Storage**: Git object database (collab DAG refs under `.git/refs/collab/`) **Testing**: cargo test (integration tests in `tests/`, unit tests inline) **Target Platform**: Linux/macOS/Windows CLI **Project Type**: CLI tool with TUI dashboard **Performance Goals**: N/A (CLI tool, operations are fast by nature of git2) **Constraints**: All data in git objects — no external state, no network dependencies **Scale/Scope**: Single-repo, multi-collaborator via git remotes ## Constitution Check *No constitution configured for this project. No gates to check.* ## Design Decisions ### 1. New Event Type: `patch.revision` Add `PatchRevision` variant to `Action` enum in `src/event.rs`: - Fields: `commit: String`, `tree: String`, `body: Option<String>` - Serialized as `"type": "patch.revision"` - No revision number stored — derived from DAG order during materialization ### 2. Modified `PatchCreate` Event Add required `commit` and `tree` fields to `Action::PatchCreate` in `src/event.rs`: - These capture revision 1's snapshot at creation time - Required fields (not optional) — no backward compat needed ### 3. Revision-Anchored Comments and Reviews Add required `revision: u32` field to `Action::PatchInlineComment` and `Action::PatchReview`: - Set at event creation time from the current derived revision number - Not added to `Action::PatchComment` (thread comments are patch-level) ### 4. PatchState Gains Revision Tracking In `src/state.rs`, `PatchState` gains: - `revisions: Vec<Revision>` — ordered list of revision snapshots - `Revision` struct: `{ number: u32, commit: String, tree: String, body: Option<String>, timestamp: String }` - During materialization: PatchCreate contributes revision 1 (if `commit`/`tree` present), each `PatchRevision` event contributes subsequent revisions - Dedup: skip `PatchRevision` events whose `commit` matches an already-seen commit OID ### 5. Auto-Detection on Write Operations In `src/patch.rs`, before appending events in `comment()`, `review()`, and `merge()`: 1. Materialize current `PatchState` to get the last recorded revision's commit OID 2. Resolve current branch tip via `resolve_head()` 3. If they differ, append a `PatchRevision` event first (with current tip's commit + tree OID) 4. Then append the actual event with the (possibly updated) revision number Extract this as a helper: `fn auto_detect_revision(repo, ref_name, state, sk) -> Result<u32>` ### 6. `patch revise` Command Repurposed The `PatchRevise` action is removed and replaced by `PatchRevision`. The CLI command `patch revise`: - Resolves current branch tip - Compares to last recorded revision's commit OID - If different: appends `PatchRevision` event with optional `--body` - If same: rejects with "no changes since revision N" ### 7. CLI Changes **Modified commands**: - `patch comment` — add `--revision N` flag (optional, defaults to latest) - `patch review` — add `--revision N` flag (optional, defaults to latest) - `patch show` — add `--revision N` flag (filters inline comments/reviews to that revision) - `patch diff` — add `--revision N` flag (historical diff: revision N vs merge-base) and `--between N [M]` flag (interdiff; single arg = N to latest) - `patch revise` — now appends `PatchRevision` event instead of `PatchRevise`; keeps `--body` **New command**: - `patch log <id>` — lists all revisions with number, timestamp, commit short OID, file-change summary; supports `--json` ### 8. Interdiff Implementation In `src/patch.rs`, new function `interdiff(repo, patch, from_rev, to_rev) -> Result<String>`: - Look up tree OIDs for the two revision numbers from `PatchState.revisions` - `repo.find_tree(from_tree)` and `repo.find_tree(to_tree)` - `repo.diff_tree_to_tree(from, to, None)` - Format as unified diff (same as existing `generate_diff`) ### 9. Historical Diff In `src/patch.rs`, modify `generate_diff` to accept an optional revision number: - If provided, use that revision's tree OID instead of the current branch tip - Merge-base is still computed against the base branch ### 10. Merge Policy Config In `refs/collab/config` (project-level, synced): - New key: `merge.require_approval_on_latest` (boolean, default false) - `patch merge` reads this config before proceeding - If true, checks that at least one `Approve` review exists with `revision == latest_revision_number` - Config read/write helpers in a new section of `src/patch.rs` or extracted to a config module ### 11. TUI Updates In `src/tui/`: - Patch list view: show revision count (e.g., "r3") alongside status - Patch detail view: show revision list, annotate inline comments with revision number - No new TUI interactions for this feature (revision browsing is CLI-only for now) ### 12. No Backward Compatibility Needed The project is pre-release with no existing patches. The old `PatchRevise` event type is removed entirely. All new fields (`commit`/`tree` on PatchCreate, `revision` on inline comments/reviews) are required, not optional. ## Project Structure ### Documentation (this feature) ```text specs/015-gerrit-style-patchsets/ ├── plan.md # This file ├── spec.md # Feature specification ├── research.md # Phase 0 output ├── data-model.md # Phase 1 output ├── contracts/ # Phase 1 output (CLI contract) └── tasks.md # Phase 2 output ``` ### Source Code (repository root) ```text src/ ├── event.rs # Add PatchRevision variant, modify PatchCreate/PatchInlineComment/PatchReview ├── state.rs # Add Revision struct, revisions vec to PatchState, dedup logic ├── patch.rs # Auto-detect helper, interdiff, historical diff, patch log, merge policy ├── cli.rs # Add --revision, --between flags; add patch log subcommand ├── lib.rs # Dispatch for new/modified CLI commands └── tui/ └── widgets.rs # Show revision count in patch list, revision annotations in detail tests/ ├── revision_test.rs # New: revision recording, dedup, interdiff, anchored comments └── cli_test.rs # Extended: new flags and commands ``` ## Migration Strategy No migration needed — pre-release project with no existing patches. Breaking changes are acceptable. diff --git a/specs/015-gerrit-style-patchsets/research.md b/specs/015-gerrit-style-patchsets/research.md new file mode 100644 index 0000000..6c58fdd --- /dev/null +++ b/specs/015-gerrit-style-patchsets/research.md @@ -0,0 +1,39 @@ # Research: Patch Revisions ## Decision: Revision Number Derivation **Decision**: Derive revision numbers from topological DAG order during materialization, not store them in events. **Rationale**: Two collaborators may independently detect the same branch change and create `PatchRevision` events concurrently. If numbers were stored, they'd conflict after DAG merge. Deriving from topo order + dedup by commit OID ensures consistent numbering post-sync. **Alternatives considered**: - Store revision number in event: Simpler materialization, but breaks on concurrent detection. Rejected. - Use commit OID as revision identifier instead of numbers: Correct but poor UX — users prefer `--between 1 3` over `--between abc123 def456`. Rejected for CLI, but commit OID is the underlying identifier. ## Decision: Auto-Detection Trigger **Decision**: Auto-detect revision changes only on write operations (comment, review, merge), never on reads (show, list, diff). **Rationale**: Reads having side effects (appending to DAG) is surprising and requires a signing key. Write operations already require a signing key and already modify the DAG, so the incremental cost is zero. **Alternatives considered**: - Auto-detect on all operations including reads: Ensures revisions are always up-to-date, but side-effecting reads is an anti-pattern. Rejected. - Explicit-only (`patch revise`): Clean but requires developer discipline. Kept as escape hatch alongside auto-detection. ## Decision: Terminology (Radicle Model) **Decision**: Use "patch" + "revision" terminology (Radicle model), not "change" + "patch set" (Gerrit model). **Rationale**: git-collab already uses "patch" as the top-level entity. Radicle — the closest prior art for git-native forges — also uses "patch" + "revision". Renaming to "change" would break existing CLI, events, and refs for no functional benefit. **Alternatives considered**: - Gerrit model ("change" + "patch set"): Would require renaming all patch commands, events, and refs. Confusing terminology ("patch set" is misleading). Rejected. ## Decision: Thread Comments Not Revision-Anchored **Decision**: `patch.comment` events (thread comments) do not get a `revision` field. Only `patch.inline_comment` and `patch.review` are revision-anchored. **Rationale**: Thread comments are general conversation about the patch as a whole ("looks good overall", "what's the motivation for this?"). They don't reference specific code and would gain nothing from revision anchoring. Inline comments reference file:line in a specific tree, so revision context is essential. **Alternatives considered**: - Anchor all events to revisions for consistency: Adds complexity without value for thread comments. Rejected. diff --git a/specs/015-gerrit-style-patchsets/review.md b/specs/015-gerrit-style-patchsets/review.md new file mode 100644 index 0000000..5fb7553 --- /dev/null +++ b/specs/015-gerrit-style-patchsets/review.md @@ -0,0 +1,33 @@ # Pre-Implementation Review **Feature**: Patch Revisions **Artifacts reviewed**: spec.md, plan.md, tasks.md, checklists/revision-model.md **Review model**: Claude Opus 4.6 **Date**: 2026-03-21 ## Summary | Dimension | Verdict | Issues | |-----------|---------|--------| | Spec-Plan Alignment | PASS | All user stories and FRs addressed | | Plan-Tasks Completeness | PASS | Dispatch wiring now per-phase (fixed) | | Dependency Ordering | PASS | Phases correctly ordered | | Parallelization Correctness | PASS | [P] markers accurate | | Feasibility & Risk | PASS | Simplified by removing backward compat | | Standards Compliance | PASS | No constitution configured | | Implementation Readiness | PASS | All tasks have exact file paths | **Overall**: READY ## Findings No critical or warning items remain after simplification (backward compat removed, dispatch wiring moved per-phase). ## Post-Review Changes Applied 1. Removed backward compat requirements (FR-011, FR-013, SC-005) 2. Made `commit`/`tree` on PatchCreate and `revision` on inline comments/reviews required (not Optional) 3. Removed `PatchRevise` event type entirely 4. Split dispatch wiring into per-phase tasks (T010a, T015a, T019, T022) 5. Added sync round-trip test case to T029 6. Clarified auto-detect runs for thread comments too (T005) diff --git a/specs/015-gerrit-style-patchsets/spec.md b/specs/015-gerrit-style-patchsets/spec.md index d41a0bb..fd63227 100644 --- a/specs/015-gerrit-style-patchsets/spec.md +++ b/specs/015-gerrit-style-patchsets/spec.md @@ -1,162 +1,164 @@ # Feature Specification: Gerrit-Style Patchsets # Feature Specification: Patch Revisions **Feature Branch**: `015-gerrit-style-patchsets` **Created**: 2026-03-21 **Status**: Draft **Input**: Patches-as-branches are redundant when users share a git host — branches already exist. The differentiator is Gerrit-style change tracking: numbered patchsets, comments anchored to specific revisions, and interdiffs between revisions. This is the thing GitHub PRs are actually bad at. **Input**: Patches-as-branches are redundant when users share a git host — branches already exist. The differentiator is revision-aware review (à la Radicle/Gerrit): numbered revisions, comments anchored to specific revisions, and interdiffs between revisions. This is the thing GitHub PRs are actually bad at. ## Motivation The current patch model wraps a git branch with a review conversation. When collaborators share a remote, the branch already exists — the collab layer adds review metadata but doesn't solve any problem that GitHub/GitLab PRs don't already handle better. What Gerrit gets right (and GitHub gets wrong) is **revision-aware review**: What Radicle and Gerrit get right (and GitHub gets wrong) is **revision-aware review**: 1. A reviewer leaves inline comments on version 1 of the code 2. The author amends/rebases and pushes version 2 3. The reviewer can see: what changed between v1 and v2 (interdiff), which comments are on stale code, and which lines are untouched 4. Comments stay anchored to the patchset they were made on — they don't float to wrong line numbers 1. A reviewer leaves inline comments on revision 1 of the code 2. The author amends/rebases and pushes revision 2 3. The reviewer can see: what changed between r1 and r2 (interdiff), which comments are on older code, and which lines are untouched 4. Comments stay anchored to the revision they were made on — they don't float to wrong line numbers This spec transforms git-collab patches into Gerrit-style changes with numbered patchsets. This spec adds numbered revisions to git-collab patches, following the Radicle model (patch + revisions) rather than the Gerrit model (change + patch sets). ## User Scenarios & Testing *(mandatory)* ### User Story 1 - Automatic Patchset Recording on Write Operations (Priority: P1) ### User Story 1 - Automatic Revision Recording on Write Operations (Priority: P1) The system automatically detects when the branch tip has changed since the last recorded patchset. This detection happens only during **write operations** that touch the patch DAG — `patch comment`, `patch review`, `patch merge`, and `patch create`. Before appending their own event, these commands check the branch tip against the last recorded patchset's commit OID. If they differ, a `PatchUpdate` event is appended first, then the actual event. Read operations (`patch show`, `patch list`, `patch diff`, TUI) never modify the DAG. The system automatically detects when the branch tip has changed since the last recorded revision. This detection happens only during **write operations** that touch the patch DAG — `patch comment`, `patch review`, `patch merge`, and `patch create`. Before appending their own event, these commands check the branch tip against the last recorded revision's commit OID. If they differ, a `PatchRevision` event is appended first, then the actual event. Read operations (`patch show`, `patch list`, `patch diff`, TUI) never modify the DAG. Patchset numbers are **not stored in events** — they are derived from DAG order during materialization. The Nth `PatchUpdate` event (in topological order) is patchset N+1 (patchset 1 comes from `PatchCreate`). This avoids conflicts when two collaborators independently detect the same branch change. Revision numbers are **not stored in events** — they are derived from DAG order during materialization. The Nth `PatchRevision` event (in topological order) is revision N+1 (revision 1 comes from `PatchCreate`). This avoids conflicts when two collaborators independently detect the same branch change. **Why this priority**: Without patchset recording, nothing else (anchored comments, interdiff) works. Automatic recording on writes means developers don't need to learn a new command — patchsets appear naturally as the review conversation progresses. **Why this priority**: Without revision recording, nothing else (anchored comments, interdiff) works. Automatic recording on writes means developers don't need to learn a new command — revisions appear naturally as the review conversation progresses. **Independent Test**: Create a patch (patchset 1 from create), push a commit, run `patch comment`, verify a PatchUpdate event was auto-inserted before the comment event. **Independent Test**: Create a patch (revision 1 from create), push a commit, run `patch comment`, verify a PatchRevision event was auto-inserted before the comment event. **Acceptance Scenarios**: 1. **Given** a patch exists with patchset 1, **When** the developer pushes new commits and runs `patch comment <id>`, **Then** a PatchUpdate event is auto-inserted before the comment, and the comment is anchored to patchset 2. 2. **Given** the branch hasn't changed since the last patchset, **When** the developer runs `patch comment`, **Then** no PatchUpdate is inserted — the comment is anchored to the current patchset. 3. **Given** a patch is created with `patch create`, **Then** patchset 1 is recorded in the PatchCreate event with the branch tip commit OID and tree OID. 4. **Given** a patch has 3 patchsets, **When** viewing the patch, **Then** all 3 patchsets are listed with their number (derived from DAG order), commit OID, and timestamp. 5. **Given** a developer rebases their branch (force-push), **When** the next write operation touches the patch, **Then** a PatchUpdate event is auto-inserted capturing the post-rebase state. 6. **Given** the branch has been deleted, **When** a write operation attempts auto-detection, **Then** no PatchUpdate is inserted — the branch is simply unavailable. 7. **Given** two collaborators independently detect the same branch change and create PatchUpdate events, **When** their DAGs are synced and merged, **Then** patchset numbers are derived from topological order — both events contribute but the numbering remains consistent after materialization. 1. **Given** a patch exists with revision 1, **When** the developer pushes new commits and runs `patch comment <id>`, **Then** a PatchRevision event is auto-inserted before the comment, and the comment is anchored to revision 2. 2. **Given** the branch hasn't changed since the last revision, **When** the developer runs `patch comment`, **Then** no PatchRevision is inserted — the comment is anchored to the current revision. 3. **Given** a patch is created with `patch create`, **Then** revision 1 is recorded in the PatchCreate event with the branch tip commit OID and tree OID. 4. **Given** a patch has 3 revisions, **When** viewing the patch, **Then** all 3 revisions are listed with their number (derived from DAG order), commit OID, and timestamp. 5. **Given** a developer rebases their branch (force-push), **When** the next write operation touches the patch, **Then** a PatchRevision event is auto-inserted capturing the post-rebase state. 6. **Given** the branch has been deleted, **When** a write operation attempts auto-detection, **Then** no PatchRevision is inserted — the branch is simply unavailable. 7. **Given** two collaborators independently detect the same branch change and create PatchRevision events, **When** their DAGs are synced and merged, **Then** revision numbers are derived from topological order — both events contribute but the numbering remains consistent after materialization. --- ### User Story 2 - Inline Comments Anchored to Patchsets (Priority: P1) ### User Story 2 - Inline Comments Anchored to Revisions (Priority: P1) A reviewer leaves an inline comment on a specific file and line. The comment is anchored to the current patchset number. Comments belong to the patchset they were made on — period. There is no attempt to track or map comments across patchsets. When viewing a specific patchset, you see its comments. When viewing a different patchset, you see that patchset's comments. This is the Gerrit model: each patchset is its own self-contained review surface. A reviewer leaves an inline comment on a specific file and line. The comment is anchored to the current revision number. Comments belong to the revision they were made on — period. There is no attempt to track or map comments across revisions. When viewing a specific revision, you see its comments. When viewing a different revision, you see that revision's comments. This is the Radicle model: each revision is its own self-contained review surface. **Why this priority**: This is the core value proposition — without patchset-anchored comments, we're just a worse GitHub PR. **Why this priority**: This is the core value proposition — without revision-anchored comments, we're just a worse GitHub PR. **Independent Test**: Create a patch, leave an inline comment on patchset 1, publish patchset 2, leave a different comment on patchset 2, verify each patchset shows only its own comments. **Independent Test**: Create a patch, leave an inline comment on revision 1, publish revision 2, leave a different comment on revision 2, verify each revision shows only its own comments. **Acceptance Scenarios**: 1. **Given** a patch is on patchset 3, **When** a reviewer adds an inline comment, **Then** the comment is recorded with `patchset: 3` in the event. 2. **Given** comments exist on patchsets 1 and 3, **When** viewing with `patch show <id> --patchset 1`, **Then** only patchset 1's comments are shown. 3. **Given** `patch show <id>` is run without `--patchset`, **Then** comments from all patchsets are shown, grouped by patchset number. 4. **Given** an inline comment was made without a patchset number (legacy), **Then** it is displayed with "(patchset: unknown)" and grouped separately. 5. **Given** a reviewer is viewing patchset 2, **When** they add an inline comment, **Then** the comment is anchored to patchset 2 regardless of what the latest patchset is. 1. **Given** a patch is on revision 3, **When** a reviewer adds an inline comment, **Then** the comment is recorded with `revision: 3` in the event. 2. **Given** comments exist on revisions 1 and 3, **When** viewing with `patch show <id> --revision 1`, **Then** only revision 1's comments are shown. 3. **Given** `patch show <id>` is run without `--revision`, **Then** comments from all revisions are shown, grouped by revision number. 4. **Given** a reviewer is viewing revision 2, **When** they add an inline comment, **Then** the comment is anchored to revision 2 regardless of what the latest revision is. --- ### User Story 3 - View Interdiff Between Patchsets (Priority: P1) ### User Story 3 - View Interdiff Between Revisions (Priority: P1) A reviewer wants to see only what changed between two revisions of the patch, not the full diff against the base. They run `patch diff <id> --between 1 2` and see the diff between patchset 1's tree and patchset 2's tree. This is the critical "what did the author actually change in response to my feedback" view. A reviewer wants to see only what changed between two revisions of the patch, not the full diff against the base. They run `patch diff <id> --between 1 2` and see the diff between revision 1's tree and revision 2's tree. This is the critical "what did the author actually change in response to my feedback" view. **Why this priority**: This is the killer feature that makes patchsets useful. Without interdiff, numbered patchsets are just bookmarks. **Why this priority**: This is the killer feature that makes revisions useful. Without interdiff, numbered revisions are just bookmarks. **Independent Test**: Create a patch with 3 patchsets where each modifies different files, run `patch diff --between 1 3`, verify the diff shows only the changes between those two snapshots. **Independent Test**: Create a patch with 3 revisions where each modifies different files, run `patch diff --between 1 3`, verify the diff shows only the changes between those two snapshots. **Acceptance Scenarios**: 1. **Given** a patch has patchsets 1 and 2, **When** running `patch diff <id> --between 1 2`, **Then** the output is the unified diff between patchset 1's tree and patchset 2's tree. 2. **Given** a patch has patchsets 1, 2, and 3, **When** running `patch diff <id> --between 1 3`, **Then** the diff spans from patchset 1 to 3, skipping 2. 3. **Given** a patch has only 1 patchset, **When** running `patch diff <id> --between 1 2`, **Then** the system errors with "patchset 2 not found". 1. **Given** a patch has revisions 1 and 2, **When** running `patch diff <id> --between 1 2`, **Then** the output is the unified diff between revision 1's tree and revision 2's tree. 2. **Given** a patch has revisions 1, 2, and 3, **When** running `patch diff <id> --between 1 3`, **Then** the diff spans from revision 1 to 3, skipping 2. 3. **Given** a patch has only 1 revision, **When** running `patch diff <id> --between 1 2`, **Then** the system errors with "revision 2 not found". 4. **Given** `patch diff <id>` is run without `--between`, **Then** behavior is unchanged — shows the diff between merge-base and current branch tip (the latest code). 5. **Given** patchsets 1 and 2 have identical trees (edge case: metadata-only revision), **Then** interdiff is empty. 5. **Given** revisions 1 and 2 have identical trees (edge case: metadata-only revision), **Then** interdiff is empty. --- ### User Story 4 - View Patchset History (Priority: P2) ### User Story 4 - View Revision Log (Priority: P2) A reviewer or author wants to see the revision history of a patch — all patchsets with their numbers, timestamps, commit OIDs, and which files changed between each consecutive pair. This gives a timeline of how the patch evolved. A reviewer or author wants to see the revision history of a patch — all revisions with their numbers, timestamps, commit OIDs, and which files changed between each consecutive pair. This gives a timeline of how the patch evolved. **Why this priority**: Useful context for reviewers picking up a patch mid-conversation, but not blocking for the core review workflow. **Independent Test**: Create a patch with 3 patchsets, run `patch history <id>`, verify all 3 are listed with timestamps and file-change summaries. **Independent Test**: Create a patch with 3 revisions, run `patch log <id>`, verify all 3 are listed with timestamps and file-change summaries. **Acceptance Scenarios**: 1. **Given** a patch with 3 patchsets, **When** running `patch history <id>`, **Then** the output lists each patchset with number, timestamp, commit short OID, and a summary like "+2 -1 files changed" relative to the previous patchset. 2. **Given** `--json` flag, **Then** the output is a JSON array of patchset objects. 3. **Given** a patch with only 1 patchset, **Then** the history shows just patchset 1 with "(initial)" label. 1. **Given** a patch with 3 revisions, **When** running `patch log <id>`, **Then** the output lists each revision with number, timestamp, commit short OID, and a summary like "+2 -1 files changed" relative to the previous revision. 2. **Given** `--json` flag, **Then** the output is a JSON array of revision objects. 3. **Given** a patch with only 1 revision, **Then** the log shows just revision 1 with "(initial)" label. --- ### User Story 5 - Review Anchored to Patchsets (Priority: P2) ### User Story 5 - Review Anchored to Revisions (Priority: P2) A reviewer submits a review (approve/request-changes). The review is anchored to the current patchset. Reviews belong to their patchset, same as comments. `patch show` displays the latest review per reviewer with its patchset number, so it's clear whether the approval is on the current code or an older version. A reviewer submits a review (approve/request-changes). The review is anchored to the current revision. Reviews belong to their revision, same as comments. `patch show` displays the latest review per reviewer with its revision number, so it's clear whether the approval is on the current code or an older version. **Why this priority**: Important for understanding review state, but the core approve/request-changes mechanics already exist. **Acceptance Scenarios**: 1. **Given** a reviewer approves on patchset 3 and patchset 4 is recorded, **When** viewing the patch, **Then** the approval is shown as "approved (patchset 3)". 2. **Given** a reviewer requests changes on patchset 2, **When** viewing on patchset 4, **Then** the request-changes review is shown with its patchset number. 3. **Given** `patch show`, **Then** the latest review per reviewer is highlighted, with patchset context. 4. **Given** a configurable merge policy exists, **When** merging, **Then** the policy determines whether approval must be on the latest patchset. See FR-014. 1. **Given** a reviewer approves on revision 3 and revision 4 is recorded, **When** viewing the patch, **Then** the approval is shown as "approved (revision 3)". 2. **Given** a reviewer requests changes on revision 2, **When** viewing on revision 4, **Then** the request-changes review is shown with its revision number. 3. **Given** `patch show`, **Then** the latest review per reviewer is highlighted, with revision context. 4. **Given** a configurable merge policy exists, **When** merging, **Then** the policy determines whether approval must be on the latest revision. See FR-014. --- ### Edge Cases - What if the branch is deleted when auto-detection runs? No new patchset is created. The last recorded patchset remains current. - What if the base branch has moved significantly between patchsets? The interdiff still works — it compares the two patchset trees directly, independent of the base. - What about merge commits in the branch? The patchset snapshots the tree at the branch tip, regardless of history shape. - What if a reviewer comments on a patchset that doesn't exist yet (race condition in distributed sync)? The comment is stored with its patchset number; if the patchset doesn't exist locally, it's displayed with "(patchset N — not yet synced)". - What if the tree hasn't changed but commits were amended (message-only change)? Auto-detection compares commit OID, not tree OID, so a new patchset is recorded. The interdiff will show no code changes. - What about backward compat with existing patches that have no patchsets? They work as today. Inline comments without a patchset field are treated as "patchset: unknown". The `--between` flag is unavailable. The first read that detects a branch tip different from the stored one bootstraps patchset tracking. - What about auto-recording requiring a signing key? The auto-detection must have access to the signing key to append a DAG event. If the key is unavailable (e.g., read-only clone), patchset detection is skipped silently — the patch still works, just without new patchset tracking. - What if the branch is deleted when auto-detection runs? No new revision is created. The last recorded revision remains current. - What if the base branch has moved significantly between revisions? The interdiff still works — it compares the two revision trees directly, independent of the base. - What about merge commits in the branch? The revision snapshots the tree at the branch tip, regardless of history shape. - What if a reviewer comments on a revision that doesn't exist yet (race condition in distributed sync)? The comment is stored with its revision number; if the revision doesn't exist locally, it's displayed with "(revision N — not yet synced)". - What if the tree hasn't changed but commits were amended (message-only change)? Auto-detection compares commit OID, not tree OID, so a new revision is recorded. The interdiff will show no code changes. - What about auto-recording requiring a signing key? The auto-detection must have access to the signing key to append a DAG event. If the key is unavailable (e.g., read-only clone), revision detection is skipped silently — the patch still works, just without new revision tracking. - Thread comments (`patch.comment`) are NOT revision-anchored — they are patch-level conversation that spans the whole lifecycle. ## Requirements *(mandatory)* ### Functional Requirements - **FR-001**: `patch create` MUST record patchset 1 in the PatchCreate event, capturing the branch tip commit OID and tree OID. - **FR-002**: Write operations (`patch comment`, `patch review`, `patch merge`) MUST automatically detect when the branch tip differs from the last recorded patchset and prepend a `PatchUpdate` event before their own event. - **FR-003**: Auto-detection MUST compare commit OIDs (not tree OIDs) to decide whether a new patchset is needed. - **FR-003a**: Patchset numbers MUST be derived from topological DAG order during materialization, NOT stored in events. The Nth `PatchUpdate` in topological order is patchset N+1. - **FR-004**: Inline comments (`patch.inline_comment` events) MUST include a `patchset` field recording the current patchset number at the time of commenting. - **FR-005**: Reviews (`patch.review` events) MUST include a `patchset` field recording the current patchset number at the time of review. - **FR-006**: `patch diff <id> --between N M` MUST compute the diff between patchset N's tree and patchset M's tree. - **FR-007**: `patch diff <id>` without `--between` MUST continue to show the merge-base diff against the current branch tip (existing behavior). - **FR-008**: `patch show` MUST display the current patchset number, list all patchsets, and annotate comments/reviews with their patchset context. - **FR-009**: `patch show --patchset N` MUST filter to show only comments and reviews from patchset N. - **FR-010**: `patch history <id>` MUST list all patchsets with number, timestamp, commit OID, and file-change summary. - **FR-011**: System MUST handle existing patches without patchsets gracefully — they continue to work as today, with comments shown as "patchset: unknown". - **FR-012**: Patchset data MUST be stored in the collab DAG (not external state), so it syncs via the existing sync mechanism. - **FR-013**: Auto-detection on a legacy patch (no patchsets) MUST bootstrap by treating the current branch tip as patchset 1. - **FR-014**: Merge policy MUST be configurable via collab config: `merge.require_approval_on_latest` (boolean, default false). When true, `patch merge` requires at least one approval on the latest patchset. When false, any prior approval suffices (current behavior). - **FR-001**: `patch create` MUST record revision 1 in the PatchCreate event, capturing the branch tip commit OID and tree OID. - **FR-002**: Write operations (`patch comment`, `patch review`, `patch merge`) MUST automatically detect when the branch tip differs from the last recorded revision and prepend a `PatchRevision` event before their own event. - **FR-003**: Auto-detection MUST compare commit OIDs (not tree OIDs) to decide whether a new revision is needed. - **FR-003a**: Revision numbers MUST be derived from topological DAG order during materialization, NOT stored in events. The Nth `PatchRevision` in topological order is revision N+1. - **FR-003b**: During materialization, `PatchRevision` events with the same commit OID MUST be deduplicated — only the first (in topological order) contributes a revision boundary. This prevents duplicate revisions when collaborators independently detect the same branch change. - **FR-004**: Inline comments (`patch.inline_comment` events) MUST include a `revision` field recording the current revision number at the time of commenting. The `--revision N` flag on `patch comment` allows targeting a specific revision; without it, the latest revision is used. - **FR-005**: Reviews (`patch.review` events) MUST include a `revision` field recording the current revision number at the time of review. The `--revision N` flag on `patch review` allows targeting a specific revision; without it, the latest revision is used. - **FR-006**: `patch diff <id> --between N M` MUST compute the diff between revision N's tree and revision M's tree. `--between N` (single argument) is shorthand for "revision N to latest revision". - **FR-007**: `patch diff <id>` without `--between` or `--revision` MUST continue to show the merge-base diff against the current branch tip (existing behavior). - **FR-007a**: `patch diff <id> --revision N` MUST show the diff between the merge-base and revision N's tree (historical full diff). - **FR-008**: `patch show` MUST display the current revision number, list all revisions, and annotate inline comments/reviews with their revision context. - **FR-009**: `patch show --revision N` MUST filter to show only inline comments and reviews from revision N. - **FR-010**: `patch log <id>` MUST list all revisions with number, timestamp, commit OID, and file-change summary. - **FR-011**: Revision data MUST be stored in the collab DAG (not external state), so it syncs via the existing sync mechanism. - **FR-012**: Merge policy MUST be configurable via `refs/collab/config` (project-level, synced between collaborators): `merge.require_approval_on_latest` (boolean, default false). When true, `patch merge` requires at least one approval on the latest revision. When false, any prior approval suffices (current behavior). ### Key Entities - **Patchset**: A numbered snapshot of the branch state at a point in time. Contains: patchset number (1-indexed, monotonically increasing), commit OID (branch tip), tree OID (for fast interdiff), and timestamp. Stored as `PatchUpdate` events in the review DAG. Patchset 1 is embedded in the `PatchCreate` event. - **Anchored Comment**: An inline comment or review that references a specific patchset number. The patchset field is optional for backward compatibility with legacy comments. - **Interdiff**: The diff between two patchset trees. Computed on-the-fly from stored tree OIDs, not pre-computed. - **Revision**: A numbered snapshot of the branch state at a point in time. Contains: revision number (1-indexed, monotonically increasing), commit OID (branch tip), tree OID (for fast interdiff), and timestamp. Stored as `PatchRevision` events in the review DAG. Revision 1 is embedded in the `PatchCreate` event. - **Anchored Comment**: An inline comment or review that references a specific revision number. - **Interdiff**: The diff between two revision trees. Computed on-the-fly from stored tree OIDs, not pre-computed. ### Event Schema Changes New event type (patchset number omitted — derived from DAG order): New event type (revision number omitted — derived from DAG order): ```json { "type": "patch.update", "type": "patch.revision", "commit": "abc123...", "tree": "def456..." "tree": "def456...", "body": "addressed review feedback on error handling" } ``` Modified `patch.create` — add optional snapshot fields: The `body` field is optional — omitted when auto-detected on write operations, provided when the author runs `patch revise --body "..."` explicitly. Modified `patch.create` — add required snapshot fields: ```json { "type": "patch.create", @@ -169,52 +171,70 @@ Modified `patch.create` — add optional snapshot fields: } ``` Modified `patch.inline_comment` — add patchset field (derived number at time of event creation): Modified `patch.inline_comment` — add revision field (derived number at time of event creation): ```json { "type": "patch.inline_comment", "file": "src/lib.rs", "line": 42, "body": "...", "patchset": 3 "revision": 3 } ``` Modified `patch.review` — add patchset field (derived number at time of event creation): Modified `patch.review` — add revision field (derived number at time of event creation): ```json { "type": "patch.review", "verdict": "approve", "body": "...", "patchset": 3 "revision": 3 } ``` Note: The `patchset` field on comments and reviews stores the derived patchset number at the time the event was created. This is a convenience for display — the authoritative patchset boundaries are always the PatchCreate + PatchUpdate events in topological order. Note: The `revision` field on inline comments and reviews stores the derived revision number at the time the event was created. This is a convenience for display — the authoritative revision boundaries are always the PatchCreate + PatchRevision events in topological order. Thread comments (`patch.comment`) do not get a revision field. ## Success Criteria *(mandatory)* ### Measurable Outcomes - **SC-001**: `patch update` correctly records patchset snapshots; `patch history` shows all patchsets in order. - **SC-002**: Inline comments reference their patchset number; `patch show` displays patchset context for every comment. - **SC-003**: `patch diff --between N M` produces the correct interdiff between any two patchsets. - **SC-004**: Staleness detection correctly identifies when a comment's file has changed in later patchsets. - **SC-005**: Existing patches without patchsets continue to function identically to current behavior. - **SC-006**: All patchset data survives sync (push/fetch) between collaborators. - **SC-007**: The TUI dashboard displays patchset count and current patchset number for each patch. - **SC-001**: `patch revise` correctly records revision snapshots; `patch log` shows all revisions in order. - **SC-002**: Inline comments reference their revision number; `patch show` displays revision context for every inline comment. - **SC-003**: `patch diff --between N M` produces the correct interdiff between any two revisions. - **SC-004**: Comments and reviews display their revision number; interdiff between revisions is the mechanism for reviewing what changed (no cross-revision staleness tracking). - **SC-005**: All revision data survives sync (push/fetch) between collaborators. - **SC-007**: The TUI dashboard displays revision count and current revision number for each patch. ## Assumptions - Patchset recording is automatic on write operations only. Reads never modify the DAG. This means a patchset boundary is only recorded when someone actually interacts with the patch (comments, reviews, merges), not on passive viewing. - Revision recording is automatic on write operations only. Reads never modify the DAG. This means a revision boundary is only recorded when someone actually interacts with the patch (comments, reviews, merges), not on passive viewing. - Tree OIDs are stored alongside commit OIDs for fast interdiff computation (avoids re-walking commit history to find the tree). - Comments and reviews belong to their patchset. No cross-patchset tracking, no line-mapping, no staleness detection. Each patchset is a self-contained review surface. The interdiff is how you see what changed. - Patchset numbers are derived from topological DAG order during materialization, not stored in events. This makes concurrent auto-detection safe — duplicate PatchUpdate events for the same branch change result in consistent numbering after sync. - `PatchRevise` is superseded by `PatchUpdate` — the revise command can be kept as an alias or deprecated. - Patchset 1 data in `PatchCreate` is optional for backward compatibility — old patches without it simply don't have patchset tracking until auto-detection bootstraps it. - Auto-detection requires a signing key to append events. If unavailable, the write operation proceeds without inserting a PatchUpdate (the comment/review is anchored to the last known patchset). - Inline comments and reviews belong to their revision. Thread comments are patch-level. No cross-revision tracking, no line-mapping, no staleness detection. Each revision is a self-contained review surface. The interdiff is how you see what changed. - Revision numbers are derived from topological DAG order during materialization, not stored in events. This makes concurrent auto-detection safe — duplicate PatchRevision events for the same branch change are deduplicated by commit OID. - The existing `patch revise` command is repurposed: it now appends a `PatchRevision` event (with commit + tree OID) if the branch has changed. It serves as the explicit "I've pushed new commits, please re-review" signal. - No backward compatibility needed — the project is pre-release with no existing patches to migrate. The old `PatchRevise` event type is removed entirely. `commit`/`tree` on `PatchCreate` and `revision` on inline comments/reviews are required fields, not optional. - Auto-detection requires a signing key to append events. If unavailable, the write operation proceeds without inserting a PatchRevision (the comment/review is anchored to the last known revision). - Merge policy is configurable, not hardcoded. Default is permissive (any approval suffices). ## Clarifications ### Session 2026-03-21 - Q: Should duplicate PatchRevision events (same commit OID from concurrent detection) produce one or two revisions? → A: Deduplicate by commit OID during materialization. - Q: Should thread comments (`patch.comment`) be revision-anchored? → A: No — thread comments are patch-level conversation, not revision-scoped. - Q: How does a reviewer target an older revision for inline comments? → A: `patch comment <id> --revision N` flag to explicitly target a revision. - Q: Should `patch revise` allow creating a revision when branch is unchanged? → A: No — reject with "no changes since revision N". - Q: Should `--between` require both endpoints or allow single-arg shorthand? → A: Single arg = "N to latest" shorthand; two args = explicit range. ### Session 2026-03-21 (round 2) - Q: Should `patch review` support `--revision N` to target a specific revision? → A: Yes, symmetric with `patch comment --revision N`. - Q: Should `patch revise` support an optional `--body` message? → A: Yes, stored in PatchRevision event alongside commit/tree. Omitted on auto-detected revisions. - Q: Should `patch diff --revision N` show a historical full diff against the base? → A: Yes, completes the trifecta: no flags (latest vs base), `--between` (interdiff), `--revision N` (historical diff). - Q: Where does merge policy config live? → A: `refs/collab/config` — project-level, synced between collaborators. ## Resolved Questions 1. **Gap between push and first write**: `patch update <id>` (replacing `patch revise`) serves as the explicit "I've pushed new commits, please re-review" signal. It appends a `PatchUpdate` event if the branch has changed. Auto-detection on write operations (`comment`, `review`, `merge`) catches cases where the developer forgets. Both paths produce the same event. 1. **Gap between push and first write**: `patch revise <id>` serves as the explicit "I've pushed new commits, please re-review" signal. It appends a `PatchRevision` event if the branch has changed. If the branch hasn't changed, it rejects with "no changes since revision N". Auto-detection on write operations (`comment`, `review`, `merge`) catches cases where the developer forgets. Both paths produce the same event. 2. **Duplicate revision deduplication**: `PatchRevision` events with the same commit OID are deduplicated during materialization — only the first in topological order contributes a revision boundary. 3. **Thread comments**: `patch.comment` (thread comments) are NOT revision-anchored. They are patch-level conversation. Only inline comments and reviews get a `revision` field. diff --git a/specs/015-gerrit-style-patchsets/tasks.md b/specs/015-gerrit-style-patchsets/tasks.md new file mode 100644 index 0000000..ea535b7 --- /dev/null +++ b/specs/015-gerrit-style-patchsets/tasks.md @@ -0,0 +1,184 @@ # Tasks: Patch Revisions **Input**: Design documents from `/specs/015-gerrit-style-patchsets/` **Prerequisites**: plan.md, spec.md, data-model.md, contracts/cli.md **Organization**: Tasks are grouped by user story to enable independent implementation and testing of each story. ## Format: `[ID] [P?] [Story] Description` - **[P]**: Can run in parallel (different files, no dependencies) - **[Story]**: Which user story this task belongs to (e.g., US1, US2, US3) - Include exact file paths in descriptions --- ## Phase 1: Foundational — Event Schema & State Materialization **Purpose**: Core data model changes that ALL user stories depend on. Must complete before any story work begins. **⚠️ CRITICAL**: No user story work can begin until this phase is complete. <!-- parallel-group: 1 (max 3 concurrent) --> - [ ] T001 [P] Add `PatchRevision` variant to `Action` enum in `src/event.rs`: fields `commit: String`, `tree: String`, `body: Option<String>`, serialized as `"patch.revision"`. Add required `commit: String` and `tree: String` fields to `Action::PatchCreate`. Add required `revision: u32` field to `Action::PatchInlineComment` and `Action::PatchReview`. Remove the `PatchRevise` variant entirely. - [ ] T002 [P] Add `Revision` struct to `src/state.rs`: `{ number: u32, commit: String, tree: String, body: Option<String>, timestamp: String }` with Serialize/Deserialize. Add `revisions: Vec<Revision>` field to `PatchState`. - [ ] T003 [P] Add `revision: u32` field to `InlineComment` struct and `Review` struct in `src/state.rs`. <!-- sequential --> - [ ] T004 Update `PatchState::from_ref_uncached()` materialization in `src/state.rs`: handle `Action::PatchCreate` to extract `commit`/`tree` into revision 1. Handle `Action::PatchRevision` to append revisions with dedup by commit OID (skip if commit already seen). Derive revision numbers from insertion order (1-indexed). Handle `Action::PatchInlineComment` and `Action::PatchReview` to store the `revision` field on `InlineComment`/`Review` structs. Remove the `Action::PatchRevise` match arm. **Checkpoint**: Event schema and state materialization support revisions. Existing tests updated for new required fields. --- ## Phase 2: User Story 1 — Automatic Revision Recording (Priority: P1) 🎯 MVP **Goal**: Write operations auto-detect branch changes and insert PatchRevision events before their own event. **Independent Test**: Create a patch, push a commit, run `patch comment`, verify a PatchRevision event was auto-inserted before the comment. <!-- sequential --> - [ ] T005 [US1] Implement `auto_detect_revision()` helper in `src/patch.rs`: takes `repo`, `ref_name`, `&PatchState`, and `&SigningKey`. Resolves current branch tip via `resolve_head()`. Compares to last revision's commit OID in `state.revisions`. If different, appends a `PatchRevision` event (commit OID + tree OID from `repo.find_commit(tip)?.tree()?.id()`). Returns the current revision number (u32). If branch is deleted or resolve fails, returns last known revision number without inserting. Note: auto-detect runs for ALL write ops including thread comments (records revision boundary), even though thread comments don't get a revision field. - [ ] T006 [US1] Modify `comment()` in `src/patch.rs` to call `auto_detect_revision()` before appending the comment event. Set the `revision` field on `PatchInlineComment` to the returned revision number. Thread comments (`PatchComment`) do not get a revision field — no change needed there. - [ ] T007 [US1] Modify `review()` in `src/patch.rs` to call `auto_detect_revision()` before appending the review event. Set the `revision` field on `PatchReview` to the returned revision number. - [ ] T008 [US1] Modify `merge()` in `src/patch.rs` to call `auto_detect_revision()` before appending the merge event (no revision field on PatchMerge, but the revision should be recorded if branch changed). - [ ] T009 [US1] Modify `create()` in `src/patch.rs` to populate `commit` and `tree` fields on `PatchCreate` event: resolve the branch tip commit OID and its tree OID. - [ ] T010 [US1] Rewrite `revise()` in `src/patch.rs`: create a `PatchRevision` event with commit OID, tree OID, and optional body. Compare branch tip to last recorded revision — reject with error "no changes since revision N" if unchanged. - [ ] T010a [US1] Wire Phase 2 dispatch in `src/lib.rs`: update `PatchCmd::Revise` dispatch to call the rewritten `revise()`. Ensure `PatchCmd::Comment`, `PatchCmd::Review`, and `PatchCmd::Merge` dispatch passes through to the modified functions that now call `auto_detect_revision()`. **Checkpoint**: `patch create` records revision 1. `patch comment`, `patch review`, `patch merge` auto-detect branch changes. `patch revise` creates explicit revisions. --- ## Phase 3: User Story 2 — Comments Anchored to Revisions (Priority: P1) **Goal**: Inline comments are anchored to specific revisions. `patch show --revision N` filters by revision. **Independent Test**: Create a patch, comment on revision 1, push a commit, comment on revision 2, verify `patch show --revision 1` shows only revision 1's comments. <!-- sequential --> - [ ] T011 [US2] Add `--revision` optional arg (type `Option<u32>`) to `PatchCmd::Comment` in `src/cli.rs`. Validate: if `--revision` is provided with `--file`/`--line` (inline comment), accept it. If `--revision` is provided without `--file`/`--line` (thread comment), reject with error "thread comments are not revision-scoped". - [ ] T012 [US2] Modify `comment()` in `src/patch.rs` to accept an optional `target_revision: Option<u32>` parameter. If provided, validate it exists in `state.revisions` (error if not). Use it instead of the auto-detected current revision for the `revision` field on the inline comment event. - [ ] T013 [US2] Add `--revision` optional arg (type `Option<u32>`) to `PatchCmd::Show` in `src/cli.rs`. - [ ] T014 [US2] Modify patch show output in `src/lib.rs` (or wherever show is dispatched): when `--revision N` is provided, filter `inline_comments` and `reviews` to only those with `revision == Some(N)`. Thread comments are always shown. When no `--revision`, group inline comments/reviews by revision number in output. Display "(revision: unknown)" for comments with `revision: None`. - [ ] T015 [US2] Update `show_json()` in `src/patch.rs` to include `revisions` array in JSON output. Ensure `InlineComment` and `Review` structs serialize their `revision` field. - [ ] T015a [US2] Wire Phase 3 dispatch in `src/lib.rs`: pass `--revision` arg through to `comment()` and show output. List revisions in `patch show` output. **Checkpoint**: Inline comments anchored to revisions. `patch show --revision N` filters correctly. --- ## Phase 4: User Story 3 — Interdiff Between Revisions (Priority: P1) **Goal**: `patch diff --between N M` shows interdiff. `patch diff --revision N` shows historical diff. **Independent Test**: Create a patch with 3 revisions modifying different files, run `patch diff --between 1 3`, verify diff shows only changes between those snapshots. <!-- sequential --> - [ ] T016 [US3] Implement `interdiff()` function in `src/patch.rs`: takes `repo`, `&PatchState`, `from_rev: u32`, `to_rev: u32`. Looks up tree OIDs from `state.revisions` for both revision numbers (error if not found). Computes `repo.diff_tree_to_tree(from_tree, to_tree, None)` and formats as unified diff (reuse the existing diff formatting logic from `generate_diff`). - [ ] T017 [US3] Modify `generate_diff()` in `src/patch.rs` to accept an optional `revision: Option<u32>` parameter. If provided, use that revision's tree OID (from `state.revisions`) instead of resolving the current branch tip. Merge-base still computed against the base branch. - [ ] T018 [US3] Add `--revision` optional arg (type `Option<u32>`) and `--between` arg (type `Option<Vec<u32>>`, 1-2 values) to `PatchCmd::Diff` in `src/cli.rs`. Validate: `--revision` and `--between` are mutually exclusive (error if both). `--between` with single value N means "N to latest". - [ ] T019 [US3] Wire Phase 4 dispatch in `src/lib.rs`: if `--between` provided, call `interdiff()`. If `--revision` provided, call `generate_diff()` with the revision parameter. If neither, call `generate_diff()` without revision (existing behavior). **Checkpoint**: Interdiff works between any two revisions. Historical diff shows a specific revision against the base. --- ## Phase 5: User Story 4 — Revision Log (Priority: P2) **Goal**: `patch log <id>` lists all revisions with timestamps and file-change summaries. **Independent Test**: Create a patch with 3 revisions, run `patch log`, verify all 3 listed with correct data. <!-- sequential --> - [ ] T020 [US4] Implement `patch_log()` function in `src/patch.rs`: takes `repo` and `id_prefix`. Materializes PatchState, iterates `state.revisions`. For each consecutive pair, computes file-change summary by diffing their tree OIDs (`repo.diff_tree_to_tree`). Returns structured data (Vec of revision info with file stats). - [ ] T021 [US4] Implement `patch_log_to_writer()` in `src/patch.rs` for human-readable output: format each revision as `Revision N <short_oid> <timestamp> <files_summary> "<body>"`. First revision gets "(initial)" label. Implement `patch_log_json()` for JSON output. - [ ] T022 [US4] Add `PatchCmd::Log` variant to `src/cli.rs` with `id: String` and `--json` flag. Wire Phase 5 dispatch in `src/lib.rs` to call `patch_log_to_writer()` or `patch_log_json()`. **Checkpoint**: `patch log` shows revision history with file-change summaries. --- ## Phase 6: User Story 5 — Reviews Anchored to Revisions (Priority: P2) **Goal**: Reviews are anchored to revisions via `--revision N`. Configurable merge policy checks approval on latest revision. **Independent Test**: Approve on revision 1, push new code (revision 2), verify `patch show` displays "approved (revision 1)". <!-- sequential --> - [ ] T023 [US5] Add `--revision` optional arg (type `Option<u32>`) to `PatchCmd::Review` in `src/cli.rs`. - [ ] T024 [US5] Modify `review()` in `src/patch.rs` to accept an optional `target_revision: Option<u32>` parameter. If provided, validate it exists in `state.revisions` (error if not). Use it instead of the auto-detected revision for the `revision` field on the review event. - [ ] T025 [US5] Update review display in patch show output: show review verdict with revision context, e.g., "approved (revision 3)". Highlight latest review per reviewer. - [ ] T026 [US5] Implement merge policy config reader: read `merge.require_approval_on_latest` from `refs/collab/config` (JSON blob). Default to `false` if config ref doesn't exist or key is absent. - [ ] T027 [US5] Modify `merge()` in `src/patch.rs`: after existing checks, if `merge.require_approval_on_latest` is true, verify at least one `Approve` review has `revision == latest_revision_number`. Error if not: "merge requires approval on the latest revision (revision N)". **Checkpoint**: Reviews show revision context. Merge policy enforces latest-revision approval when configured. --- ## Phase 7: Polish & Cross-Cutting Concerns **Purpose**: TUI updates, backward compat verification, dispatch wiring <!-- parallel-group: 1 (max 2 concurrent) --> - [ ] T028 [P] Update `src/tui/widgets.rs`: show revision count (e.g., "r3") alongside patch status in the patch list view. In patch detail view, show revision list and annotate inline comments with their revision number. - [ ] T029 [P] Create `tests/revision_test.rs`: integration tests for revision recording (auto-detect on comment/review/merge), dedup by commit OID, revision numbering from DAG order, interdiff between revisions, anchored comments/reviews, `patch revise` rejection when unchanged, `--between N` single-arg shorthand, sync round-trip for PatchRevision events. --- ## Dependencies & Execution Order ### Phase Dependencies - **Phase 1 (Foundational)**: No dependencies — start immediately. BLOCKS all user stories. - **Phase 2 (US1)**: Depends on Phase 1. BLOCKS US2, US5 (they need auto-detect and revision numbers). - **Phase 3 (US2)**: Depends on Phase 2 (needs revision numbers on comments). - **Phase 4 (US3)**: Depends on Phase 1 only (needs revision data in state, not auto-detect). - **Phase 5 (US4)**: Depends on Phase 1 only (needs revision data in state). - **Phase 6 (US5)**: Depends on Phase 2 (needs revision numbers on reviews). - **Phase 7 (Polish)**: Depends on all user story phases. ### User Story Dependencies - **US1 (P1)**: Can start after Phase 1 — no dependencies on other stories - **US2 (P1)**: Depends on US1 (needs auto-detect for revision numbers) - **US3 (P1)**: Can start after Phase 1 — independent (only needs revision data in state) - **US4 (P2)**: Can start after Phase 1 — independent (only needs revision data in state) - **US5 (P2)**: Depends on US1 (needs auto-detect for revision numbers on reviews) ### Parallel Opportunities After Phase 1 completes: - US1 can start immediately - US3 and US4 can start in parallel with US1 (they don't need auto-detect) After US1 completes: - US2 and US5 can start in parallel --- ## Implementation Strategy ### MVP First (US1 Only) 1. Complete Phase 1: Foundational (event schema + state) 2. Complete Phase 2: US1 (auto-detect + revise) 3. **STOP and VALIDATE**: Create a patch, push commits, verify revisions are recorded ### Incremental Delivery 1. Phase 1 → Foundation ready 2. US1 → Revisions are recorded (MVP!) 3. US3 + US4 (parallel) → Interdiff + revision log 4. US2 → Comments anchored to revisions 5. US5 → Reviews anchored + merge policy 6. Phase 7 → TUI + tests + polish --- ## Notes - [P] tasks = different files, no dependencies - [Story] label maps task to specific user story for traceability - Most tasks touch `src/patch.rs` and `src/cli.rs` so cross-story parallelism is limited - US3 and US4 are the best candidates for parallel execution (independent of US1)