From 1178bb8e90cd0674bb1de4ea2c21758c0e8dfcef Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Fri, 16 Oct 2026 12:23:32 -0500 Subject: [PATCH 1/3] Count amend and reword commits separately from fixup commits Count attached amend! and reword! commits because their messages can need a new review. Fixup! commits carry no message. --- internal/review/git.go | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/internal/review/git.go b/internal/review/git.go index 60f2b88..ba88435 100644 --- a/internal/review/git.go +++ b/internal/review/git.go @@ -15,6 +15,9 @@ import ( // Stack identifies an original commit and its adjacent feedback commits. // Field names retain the Pi state format. ChangeID is optional lineage metadata. +// AmendCount counts attached amend! and reword! commits, whose messages carry +// reviewable content. Attached fixup! commits carry no message, so patch +// identity alone decides their review status. type Stack struct { Repo string `json:"repo"` Commits []string `json:"commits"` @@ -24,6 +27,7 @@ type Stack struct { Key string `json:"key"` ChangeID string `json:"changeId,omitempty"` PatchHash string `json:"patchHash,omitempty"` + AmendCount int `json:"amendCount,omitzero"` } func (s Stack) validate() error { @@ -61,6 +65,12 @@ func (s Stack) validateMetadata() error { return fmt.Errorf("invalid stack Change-Id") } } + if s.AmendCount < 0 { + return fmt.Errorf("invalid stack amend count") + } + if s.AmendCount > len(s.Commits) { + return fmt.Errorf("invalid stack amend count") + } return nil } @@ -262,7 +272,7 @@ 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 !tryAppendFeedback(stacks, targets, sha, match[1]) { + if !tryAppendFeedback(stacks, targets, sha, match[1], match[2]) { id, err := r.changeID(ctx, sha, ids) if err != nil { return nil, err @@ -282,7 +292,7 @@ func (r Repository) stacks(ctx context.Context, repo, base, history string) ([]S return stacks, nil } -func tryAppendFeedback(stacks []Stack, targets map[string][]int, sha, subject string) bool { +func tryAppendFeedback(stacks []Stack, targets map[string][]int, sha, kind, subject string) bool { matches := targets[subject] if len(matches) != 1 { return false @@ -294,11 +304,14 @@ func tryAppendFeedback(stacks []Stack, targets map[string][]int, sha, subject st last.Commits = append(last.Commits, sha) last.Tip = sha last.Key = last.Commits[0] + ".." + sha + if kind == "amend!" || kind == "reword!" { + last.AmendCount++ + } return true } var ( patchHashPattern = regexp.MustCompile(`^(?:v1:[a-f0-9]{64}|v2:(?:[a-f0-9]{40}|[a-f0-9]{64}))$`) - feedbackSubject = regexp.MustCompile(`^(?:fixup!|reword!|amend!) (.+)$`) + feedbackSubject = regexp.MustCompile(`^(fixup!|reword!|amend!) (.+)$`) changeIDPattern = regexp.MustCompile(`^I[0-9a-fA-F]{40}$`) ) From 1031c652ddf71f02ef39bf5250f5960aa1d035e1 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Fri, 16 Oct 2026 12:23:57 -0500 Subject: [PATCH 2/3] Require re-review for empty amend and reword commits Treat appended `amend!` and `reword!` commits as new versions because they add feedback that needs review even when the patch is unchanged. --- internal/review/patch_status_test.go | 45 ++++++++++++++++++++++++++++ internal/review/state.go | 25 +++++++++++++++- 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/internal/review/patch_status_test.go b/internal/review/patch_status_test.go index 3e8512c..e3ee209 100644 --- a/internal/review/patch_status_test.go +++ b/internal/review/patch_status_test.go @@ -10,6 +10,51 @@ import ( "github.com/stretchr/testify/require" ) +func TestAppendedFeedbackWithUnchangedPatchRequiresReview(t *testing.T) { + for _, tc := range []struct { + subject string + want string + }{ + {subject: "amend! Feature", want: "needs_review"}, + {subject: "reword! Feature", want: "needs_review"}, + // A fixup! commit carries no message, so patch identity alone decides it. + {subject: "fixup! Feature", want: "approved"}, + } { + t.Run(tc.subject, func(t *testing.T) { + r := testRepo(t) + file := filepath.Join(r.Dir, "file") + require.NoError(t, os.WriteFile(file, []byte("one\n"), 0o600)) + gitTest(t, r.Dir, "add", ".") + gitTest(t, r.Dir, "commit", "-m", "Feature") + service := Service{Repository: r} + _, err := service.Approve(t.Context(), "HEAD", "") + require.NoError(t, err) + before, err := r.Snapshot(t.Context()) + require.NoError(t, err) + require.NotEmpty(t, before.Stacks[0].PatchHash) + gitTest(t, r.Dir, "commit", "--allow-empty", "-m", tc.subject) + after, err := r.Snapshot(t.Context()) + require.NoError(t, err) + require.Len(t, after.Stacks, 1) + require.Equal(t, before.Stacks[0].PatchHash, after.Stacks[0].PatchHash) + if tc.want == "approved" { + require.Equal(t, before.Stacks[0].AmendCount, after.Stacks[0].AmendCount) + result, err := service.Status(t.Context(), true) + require.NoError(t, err) + require.Equal(t, "idle", result.Status) + require.Empty(t, result.Entries) + return + } + require.Equal(t, before.Stacks[0].AmendCount+1, after.Stacks[0].AmendCount) + result, err := service.Status(t.Context(), true) + require.NoError(t, err) + require.Len(t, result.Entries, 1) + require.Equal(t, "needs_review", result.Entries[0].Status) + require.Equal(t, "approved", result.Entries[0].Previous.Status) + }) + } +} + func TestReviewStatusAfterStackRewrite(t *testing.T) { for _, tc := range []struct { name, status, rewrite, want string diff --git a/internal/review/state.go b/internal/review/state.go index a95bf8b..96ab8df 100644 --- a/internal/review/state.go +++ b/internal/review/state.go @@ -223,7 +223,30 @@ func samePatch(a, b Stack) bool { if a.PatchHash == "" { return false } - return a.PatchHash == b.PatchHash + if a.PatchHash != b.PatchHash { + return false + } + // An appended amend! or reword! commit is a new version even when the + // aggregate diff is unchanged (for example an empty commit). An appended + // fixup! commit carries no message, so patch identity alone decides it. + if appendedAmends(a, b) || appendedAmends(b, a) { + return false + } + return true +} + +func appendedAmends(old, current Stack) bool { + if !extendsStack(old, current) { + return false + } + return current.AmendCount > old.AmendCount +} + +func extendsStack(old, current Stack) bool { + if len(current.Commits) <= len(old.Commits) { + return false + } + return slices.Equal(old.Commits, current.Commits[:len(old.Commits)]) } func sameStack(a, b Stack) bool { From ba8808d6bc94de014efb94d3ad937a077ea5673e Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Fri, 16 Oct 2026 12:24:46 -0500 Subject: [PATCH 3/3] Document review status for fixup, amend, and reword commits --- CONTEXT.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CONTEXT.md b/CONTEXT.md index 0fd4324..6f56706 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -11,6 +11,8 @@ Approval applies to the reviewed stack content, even when its commit identities **Stack content**: The combined change from the original commit's parent to the final commit in the stack. Commit metadata and intermediate commit boundaries are not part of this content. +A fixup! commit carries no message, so patch identity alone decides its review status. +Appending an amend! or reword! commit still creates a new stack version that requires review, even when the combined change is unchanged. **Attempt**: One recorded review or explicit approval of an exact stack.