Treat orphan fixup commits as normal stacks and preserve review status with whitespace-sensitive patch IDs

README.md

@@ -372,15 +372,19 @@ Compatibility with Pi's branch state requires Pi to adopt the `tuicr-reviews` pa
 ## Changed stacks and Change-Ids
 
 git-review records an optional `patchHash` for each stack's combined diff, from the original commit's parent to the final fixup.
-The hash uses SHA-256 with a `v1:` prefix and includes paths, file modes, whitespace, binary content, and diff context.
-It excludes commit messages, authors, timestamps, and commit identities.
+New hashes use Git's `patch-id --verbatim` with a `v2:` prefix and preserve paths, file modes, whitespace, binary content, and diff context.
+They ignore text blob IDs, hunk line numbers, file ordering, commit messages, authors, timestamps, and commit identities.
+Whitespace remains significant for every file type, including Python indentation.
 
 Within the same repository and branch history, matching non-empty patch hashes preserve approval and pending feedback.
 Rewording, rebasing onto unrelated changes, or squashing attached fixups can therefore preserve review status.
 A changed diff requires another review.
-Matching is conservative: a rebase that changes diff context or a touched file's blob identity can require another review.
+A change outside the diff context can shift a hunk without invalidating approval.
+Matching remains conservative: changes to diff context still require another review.
+Matching patches do not guarantee unchanged behavior when surrounding code changes.
 Empty diffs omit the hash, so unrelated empty commits cannot share approval.
-Older records without a hash still require exact commits and parent matches.
+Older `v1:` hashes remain readable but do not match `v2:` hashes.
+Records with an older hash or no hash still retain their status for exact commits and parent matches, but rewritten stacks require another review.
 
 Stored attempts retain their original commit scope, including feedback locations and saved session identity.
 Active and incomplete attempts do not transfer status through a patch hash.

internal/review/git.go

@@ -220,7 +220,25 @@ func (r Repository) patchHash(ctx context.Context, base, tip string) (string, er
 	if patch == "" {
 		return "", nil
 	}
-	return fmt.Sprintf("v1:%x", sha256.Sum256([]byte(patch))), nil
+	// Verbatim patch IDs preserve whitespace and context, but ignore text blob IDs
+	// and hunk line numbers so an unchanged patch can survive relocation.
+	ctx, cancel := context.WithTimeout(ctx, 10*time.Second)
+	defer cancel()
+	cmd := exec.CommandContext(ctx, "git", "patch-id", "--verbatim")
+	cmd.Dir = r.Dir
+	cmd.Env = append(os.Environ(), "LC_ALL=C")
+	cmd.Stdin = strings.NewReader(patch)
+	var stderr strings.Builder
+	cmd.Stderr = &stderr
+	out, err := cmd.Output()
+	if err != nil {
+		return "", fmt.Errorf("git patch-id --verbatim: %w: %s", err, strings.TrimSpace(stderr.String()))
+	}
+	fields := strings.Fields(string(out))
+	if len(fields) != 2 || !fullSHA.MatchString(fields[0]) {
+		return "", fmt.Errorf("git patch-id --verbatim returned an invalid patch ID")
+	}
+	return "v2:" + fields[0], nil
 }
 
 func (r Repository) stacks(ctx context.Context, repo, base, history string) ([]Stack, error) {
@@ -244,8 +262,12 @@ func (r Repository) stacks(ctx context.Context, repo, base, history string) ([]S
 			return nil, fmt.Errorf("commit %s has an empty subject", sha)
 		}
 		if match := feedbackSubject.FindStringSubmatch(subject); match != nil {
-			if err := appendFeedback(stacks, targets, sha, match[1]); err != nil {
-				return nil, err
+			if !tryAppendFeedback(stacks, targets, sha, match[1]) {
+				id, err := r.changeID(ctx, sha, ids)
+				if err != nil {
+					return nil, err
+				}
+				stacks = append(stacks, Stack{Repo: repo, Commits: []string{sha}, Base: parent, Tip: sha, Subject: subject, Key: sha, ChangeID: id})
 			}
 		} else {
 			id, err := r.changeID(ctx, sha, ids)
@@ -260,23 +282,23 @@ func (r Repository) stacks(ctx context.Context, repo, base, history string) ([]S
 	return stacks, nil
 }
 
-func appendFeedback(stacks []Stack, targets map[string][]int, sha, subject string) error {
+func tryAppendFeedback(stacks []Stack, targets map[string][]int, sha, subject string) bool {
 	matches := targets[subject]
 	if len(matches) != 1 {
-		return fmt.Errorf("feedback commit %s has an ambiguous or non-adjacent target", sha)
+		return false
 	}
 	if matches[0] != len(stacks)-1 {
-		return fmt.Errorf("feedback commit %s has a non-adjacent target", sha)
+		return false
 	}
 	last := &stacks[len(stacks)-1]
 	last.Commits = append(last.Commits, sha)
 	last.Tip = sha
 	last.Key = last.Commits[0] + ".." + sha
-	return nil
+	return true
 }
 
 var (
-	patchHashPattern = regexp.MustCompile(`^v1:[a-f0-9]{64}$`)
+	patchHashPattern = regexp.MustCompile(`^(?:v1:[a-f0-9]{64}|v2:(?:[a-f0-9]{40}|[a-f0-9]{64}))$`)
 	feedbackSubject  = regexp.MustCompile(`^(?:fixup!|reword!|amend!) (.+)$`)
 	changeIDPattern  = regexp.MustCompile(`^I[0-9a-fA-F]{40}$`)
 )

internal/review/git_test.go

@@ -70,6 +70,41 @@ func TestSnapshotGroupsNativeFixupsTargetingStackTip(t *testing.T) {
 	require.Equal(t, commits[0]+".."+commits[len(commits)-1], view.Stacks[0].Key)
 }
 
+func TestSnapshotTreatsOrphanFeedbackAsNormalCommit(t *testing.T) {
+	cases := []struct {
+		name     string
+		subjects []string
+		want     []string
+	}{
+		{"non-adjacent fixup", []string{"One", "Two", "fixup! One"}, []string{"One", "Two", "fixup! One"}},
+		{"non-adjacent amend", []string{"One", "Two", "amend! One"}, []string{"One", "Two", "amend! One"}},
+		{"non-adjacent reword", []string{"One", "Two", "reword! One"}, []string{"One", "Two", "reword! One"}},
+		{"ambiguous subject", []string{"One", "One", "fixup! One"}, []string{"One", "One", "fixup! One"}},
+		{"missing target", []string{"One", "fixup! fixup! One"}, []string{"One", "fixup! fixup! One"}},
+		{"forward reference", []string{"One", "fixup! Two", "Three"}, []string{"One", "fixup! Two", "Three"}},
+		{"non-adjacent nested fixup", []string{"One", "fixup! One", "Two", "fixup! fixup! One"}, []string{"One", "Two", "fixup! fixup! One"}},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			r := testRepo(t)
+			for _, s := range tc.subjects {
+				gitTest(t, r.Dir, "commit", "--allow-empty", "-m", s)
+			}
+			view, err := r.Snapshot(t.Context())
+			require.NoError(t, err)
+			require.Len(t, view.Stacks, len(tc.want))
+			for i, subject := range tc.want {
+				require.Equal(t, subject, view.Stacks[i].Subject)
+				if i == 0 && tc.name == "non-adjacent nested fixup" {
+					require.Len(t, view.Stacks[i].Commits, 2)
+				} else {
+					require.Len(t, view.Stacks[i].Commits, 1)
+				}
+			}
+		})
+	}
+}
+
 func TestSnapshotRejectsUnsafeHistory(t *testing.T) {
 	cases := []struct {
 		name    string
@@ -78,26 +113,6 @@ func TestSnapshotRejectsUnsafeHistory(t *testing.T) {
 	}{
 		{"no upstream", func(t *testing.T, r Repository) { gitTest(t, r.Dir, "branch", "--unset-upstream") }, "requires a configured Git upstream"},
 		{"detached", func(t *testing.T, r Repository) { gitTest(t, r.Dir, "checkout", "--detach") }, "not detached HEAD"},
-		{"non-adjacent fixup", func(t *testing.T, r Repository) {
-			for _, s := range []string{"One", "Two", "fixup! One"} {
-				gitTest(t, r.Dir, "commit", "--allow-empty", "-m", s)
-			}
-		}, "non-adjacent"},
-		{"non-adjacent nested fixup", func(t *testing.T, r Repository) {
-			for _, s := range []string{"One", "fixup! One", "Two", "fixup! fixup! One"} {
-				gitTest(t, r.Dir, "commit", "--allow-empty", "-m", s)
-			}
-		}, "non-adjacent"},
-		{"missing nested target", func(t *testing.T, r Repository) {
-			for _, s := range []string{"One", "fixup! fixup! One"} {
-				gitTest(t, r.Dir, "commit", "--allow-empty", "-m", s)
-			}
-		}, "non-adjacent"},
-		{"ambiguous subject", func(t *testing.T, r Repository) {
-			for _, s := range []string{"One", "One", "fixup! One"} {
-				gitTest(t, r.Dir, "commit", "--allow-empty", "-m", s)
-			}
-		}, "ambiguous"},
 		{"duplicate change id", func(t *testing.T, r Repository) {
 			for i := range 2 {
 				gitTest(t, r.Dir, "commit", "--allow-empty", "-m", fmt.Sprintf("Feature %d\n\nChange-Id: I%s", i, strings.Repeat("b", 40)))

internal/review/patch_status_test.go

@@ -4,6 +4,7 @@ import (
 	"encoding/json/jsontext"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -13,6 +14,7 @@ func TestReviewStatusAfterStackRewrite(t *testing.T) {
 	for _, tc := range []struct {
 		name, status, rewrite, want string
 		legacy                      bool
+		v1                          bool
 	}{
 		{name: "approved reword", status: "approved", rewrite: "reword", want: "approved"},
 		{name: "approved rebase", status: "approved", rewrite: "rebase", want: "approved"},
@@ -22,6 +24,7 @@ func TestReviewStatusAfterStackRewrite(t *testing.T) {
 		{name: "active reword", status: "reviewing", rewrite: "reword", want: "needs_review"},
 		{name: "changed content", status: "approved", rewrite: "content", want: "needs_review"},
 		{name: "legacy reword", status: "approved", rewrite: "reword", want: "needs_review", legacy: true},
+		{name: "v1 reword", status: "approved", rewrite: "reword", want: "needs_review", v1: true},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			r := testRepo(t)
@@ -38,6 +41,9 @@ func TestReviewStatusAfterStackRewrite(t *testing.T) {
 			if tc.legacy {
 				attempt.Stack.PatchHash = ""
 			}
+			if tc.v1 {
+				attempt.Stack.PatchHash = "v1:" + strings.Repeat("a", 64)
+			}
 			if tc.status == "feedback" {
 				attempt.Review = jsontext.Value(`{"comments":[{"text":"Please fix this"}]}`)
 			}
@@ -100,6 +106,59 @@ func TestReviewStatusAfterStackRewrite(t *testing.T) {
 	}
 }
 
+// A rebase can change blob identities and hunk offsets without changing the
+// reviewed patch. Python indentation must still distinguish rewritten content.
+func TestApprovalAfterTouchedFileRebase(t *testing.T) {
+	for _, indentChanged := range []bool{false, true} {
+		name := "shifted hunk"
+		if indentChanged {
+			name = "changed Python indentation"
+		}
+		t.Run(name, func(t *testing.T) {
+			r := testRepo(t)
+			file := filepath.Join(r.Dir, "script.py")
+			prefix := strings.Repeat("# unchanged\n", 10)
+			base := prefix + "if True:\n    print('one')\n"
+			require.NoError(t, os.WriteFile(file, []byte(base), 0o600))
+			gitTest(t, r.Dir, "add", ".")
+			gitTest(t, r.Dir, "commit", "-m", "Add script")
+			gitTest(t, r.Dir, "checkout", "main")
+			gitTest(t, r.Dir, "merge", "--ff-only", "feature")
+			gitTest(t, r.Dir, "checkout", "feature")
+			changed := prefix + "if True:\n    print('two')\n"
+			require.NoError(t, os.WriteFile(file, []byte(changed), 0o600))
+			gitTest(t, r.Dir, "add", ".")
+			gitTest(t, r.Dir, "commit", "-m", "Change output")
+			service := Service{Repository: r}
+			approved, err := service.Approve(t.Context(), "HEAD", "")
+			require.NoError(t, err)
+
+			gitTest(t, r.Dir, "checkout", "main")
+			require.NoError(t, os.WriteFile(file, []byte("# header\n"+base), 0o600))
+			gitTest(t, r.Dir, "add", ".")
+			gitTest(t, r.Dir, "commit", "-m", "Add header")
+			gitTest(t, r.Dir, "checkout", "feature")
+			gitTest(t, r.Dir, "rebase", "main")
+			if indentChanged {
+				changed = strings.Replace(changed, "    print", "        print", 1)
+				require.NoError(t, os.WriteFile(file, []byte("# header\n"+changed), 0o600))
+				gitTest(t, r.Dir, "add", ".")
+				gitTest(t, r.Dir, "commit", "--amend", "--no-edit")
+			}
+			result, err := service.Status(t.Context(), true)
+			require.NoError(t, err)
+			require.NotEqual(t, approved.View.Head, result.View.Head)
+			if indentChanged {
+				require.Len(t, result.Entries, 1)
+				require.Equal(t, "needs_review", result.Entries[0].Status)
+			} else {
+				require.Empty(t, result.Entries)
+				require.Equal(t, "idle", result.Status)
+			}
+		})
+	}
+}
+
 func rewriteTestStack(t *testing.T, r Repository, view Snapshot, kind string) {
 	t.Helper()
 	switch kind {