From 9e0f1e93e2261b3f409ba06f6a1c6077cc4028b6 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 15 Oct 2026 09:43:17 -0500 Subject: [PATCH 1/2] Treat orphan fixup commits as normal stacks A stray fixup!/amend!/reword! subject aborts the whole snapshot, so one misplaced commit blocks review of the entire branch. Fall back to a standalone stack when no single adjacent target exists, with regression coverage for orphan feedback commits. --- internal/review/git.go | 16 +++++++---- internal/review/git_test.go | 55 +++++++++++++++++++++++-------------- 2 files changed, 45 insertions(+), 26 deletions(-) diff --git a/internal/review/git.go b/internal/review/git.go index fc4f0a8..7f76b7e 100644 --- a/internal/review/git.go +++ b/internal/review/git.go @@ -244,8 +244,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,19 +264,19 @@ 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 ( diff --git a/internal/review/git_test.go b/internal/review/git_test.go index f1be648..19df8a3 100644 --- a/internal/review/git_test.go +++ b/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))) From 39348dfdfb14aebd90f21beca3494993673501da Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 15 Oct 2026 08:11:34 -0500 Subject: [PATCH 2/2] Preserve review status with whitespace-sensitive patch IDs Hashing raw diffs invalidates approval when an earlier change shifts a hunk or changes a touched file's blob identity, even if the reviewed change stays the same. Use Git's verbatim patch IDs to ignore those incidental differences without ignoring significant whitespace such as Python indentation. Version the new hashes separately from the previous SHA-256 format. Existing records remain readable and retain exact-scope status, but old hashes cannot transfer approval to rewritten stacks under the new rule. --- README.md | 12 ++++-- internal/review/git.go | 22 ++++++++++- internal/review/patch_status_test.go | 59 ++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 524f41f..0d796a5 100644 --- a/README.md +++ b/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. diff --git a/internal/review/git.go b/internal/review/git.go index 7f76b7e..60f2b88 100644 --- a/internal/review/git.go +++ b/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) { @@ -280,7 +298,7 @@ func tryAppendFeedback(stacks []Stack, targets map[string][]int, sha, subject st } 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}$`) ) diff --git a/internal/review/patch_status_test.go b/internal/review/patch_status_test.go index f3a26b5..3e8512c 100644 --- a/internal/review/patch_status_test.go +++ b/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 {