From e79fb4ccdfeab937043e1ae31ddf46ce338014cd Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Mon, 7 Sep 2026 09:56:59 -0700 Subject: [PATCH] review: Remap drafts to remote change head Review comment drafts need a stable revision for their file and line anchors. Record the local branch head before opening the comment editor so concurrent branch movement cannot pair an old anchor with a newer commit. At publication, retrieve the forge's current change head and map each root anchor from its recorded commit to that remote revision. Use the same remote head for review-patch validation so submitted coordinates and local checks describe one revision. Reuse one lazily loaded source-to-head patch for drafts that share a source commit. Mapping follows ordinary edits and Git-detected renames for file, line, and range comments. A source or diff-loading failure warns and falls back to the saved coordinate; a target deleted without replacement remains an error. No refs are retained solely for draft comments. --- .agents/docs/cli.md | 55 ++ .agents/docs/style.md | 2 + AGENTS.md | 2 +- internal/git/diff_wt.go | 36 +- internal/git/diff_wt_test.go | 27 + internal/handler/review/comment.go | 35 +- internal/handler/review/handler.go | 9 +- internal/handler/review/handler_test.go | 649 +++++++++++++++++- internal/handler/review/mocks_test.go | 79 +++ internal/handler/review/publish.go | 155 ++++- internal/jsonmut/jsonmut.go | 13 + internal/jsonmut/jsonmut_test.go | 24 + internal/review/draft.go | 11 +- internal/reviewdiff/map.go | 145 ++++ internal/reviewdiff/map_test.go | 186 +++++ internal/reviewdiff/patch.go | 6 + internal/spice/state/review_draft.go | 47 +- .../spice/state/review_draft_publish_test.go | 6 +- internal/spice/state/review_draft_test.go | 7 +- review.go | 8 +- .../script/review_publish_remaps_drafts.txt | 125 ++++ 21 files changed, 1549 insertions(+), 78 deletions(-) create mode 100644 internal/reviewdiff/map.go create mode 100644 internal/reviewdiff/map_test.go create mode 100644 testdata/script/review_publish_remaps_drafts.txt diff --git a/.agents/docs/cli.md b/.agents/docs/cli.md index f0c6e58db..85be3fdef 100644 --- a/.agents/docs/cli.md +++ b/.agents/docs/cli.md @@ -74,6 +74,61 @@ Rendering belongs at the command or handler boundary. Domain operations should not need to know whether a result will be printed as text, JSON, or another representation. +## User-Facing Logs + +Log messages are part of the standard-error interface. +Write them for a user following the operation, +not as an internal trace of the implementation. + +We use two forms of logging based on purpose: +structured logs and printf-style logs. + +### Structured Logs + +Structured logs use full sentences (capitalized, punctuated, and complete) +with structured attributes for dynamic values. +Use these when reporting a single event, or for debug-level logs. + +```go +log.Warn( + "Could not load remote change data. Using local data.", + "changeID", changeID, + "error", err, +) +``` + +Structured log attributes get user-friendly camelCase keys. +Errors use the `"error"` key. +When an error is recoverable, +state the fallback or consequence in the message. + +### printf-style Logs + +printf-style logs use a lowercase prefix for the subject of the message, +then a colon and a lowercase statement of the action or result. +Use these when repeated messages report progress for individual items. + +```go +log.Infof("%v: restacked onto %v", branch, onto) +log.Infof("%v: anchor moved to %v", draft.ID, anchor) +``` + +Exception: Debug level logs always use structured logging. + +#### printf formatting verbs + +Use `%v` when Go's default formatting is the intended presentation, +including for strings and integers. +Use another formatting verb only when its distinct presentation +is part of the output contract, +such as quoting, a non-decimal base, padding, or precision. +Use `%q` for quoted strings. + +Use `WithPrefix` for stable subsystem context, +such as a forge or a long-running operation. +Keep dynamic branch, change, draft, and other item identities +in the message or structured attributes. + ## Generated Files Run `mise run generate` diff --git a/.agents/docs/style.md b/.agents/docs/style.md index bf0e7d82e..092fdc116 100644 --- a/.agents/docs/style.md +++ b/.agents/docs/style.md @@ -116,6 +116,8 @@ Use `make` for slices only when specifying length or capacity. ## Logging Use `internal/silog.Logger`. +For user-facing message content and formatting, +follow the logging conventions in `.agents/docs/cli.md`. In tests, use `silogtest.New(t)` by default so log output is attached to the test. Use `silog.Nop()` only when the test needs to suppress logging entirely. diff --git a/AGENTS.md b/AGENTS.md index 9a7e0eb0b..2245c9ecb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,7 +9,7 @@ Read the relevant guide before editing that kind of code or prose: | Package boundaries, dependencies, constructors, APIs | `.agents/docs/design.md` | | Go implementation style and symbol ordering | `.agents/docs/style.md` | | Code comments and symbol documentation | `.agents/docs/comments.md` | -| Command behavior, flags, output, generated CLI docs | `.agents/docs/cli.md` | +| Command behavior, flags, user-facing logs and output, generated CLI docs | `.agents/docs/cli.md` | | Unit tests, test scripts, mocks, regression tests | `.agents/docs/testing.md` | | Documentation, changelog, release-facing prose | `.agents/docs/docs-and-release.md` | | Branches, commits, stacks, PR publishing boundaries | `.agents/docs/git-workflow.md` | diff --git a/internal/git/diff_wt.go b/internal/git/diff_wt.go index 06173440a..77a24a53e 100644 --- a/internal/git/diff_wt.go +++ b/internal/git/diff_wt.go @@ -31,7 +31,33 @@ func (w *Worktree) OpenBranchDiff( ctx context.Context, base, head string, ) (io.ReadCloser, error) { - cmd := w.gitCmd(ctx, "diff", base+"..."+head) + return w.openDiff(ctx, base+"..."+head) +} + +// OpenCommitDiff starts a unified diff that maps the first commit's tree to +// the second commit's tree. Rename detection is enabled and hunk context is +// omitted because callers use the result to map changed lines. +// +// The caller must close the returned reader to wait for Git and receive its +// exit status. +func (w *Worktree) OpenCommitDiff( + ctx context.Context, + from, to string, +) (io.ReadCloser, error) { + return w.openDiff( + ctx, + "--find-renames", + "--unified=0", + from, + to, + ) +} + +func (w *Worktree) openDiff( + ctx context.Context, + args ...string, +) (io.ReadCloser, error) { + cmd := w.gitCmd(ctx, "diff", args...) stdout, err := cmd.StdoutPipe() if err != nil { return nil, fmt.Errorf("pipe stdout: %w", err) @@ -43,20 +69,20 @@ func (w *Worktree) OpenBranchDiff( ) } - return &branchDiffReader{ + return &diffReader{ ReadCloser: stdout, cmd: cmd, }, nil } -// branchDiffReader waits for the Git process after closing its stdout pipe. -type branchDiffReader struct { +// diffReader waits for the Git process after closing its stdout pipe. +type diffReader struct { io.ReadCloser cmd *gitCmd } // Close releases the pipe and reports the Git process exit status. -func (r *branchDiffReader) Close() error { +func (r *diffReader) Close() error { closeErr := r.ReadCloser.Close() waitErr := r.cmd.Wait() if waitErr != nil { diff --git a/internal/git/diff_wt_test.go b/internal/git/diff_wt_test.go index 9f7c64ed1..21f4acb71 100644 --- a/internal/git/diff_wt_test.go +++ b/internal/git/diff_wt_test.go @@ -72,3 +72,30 @@ func TestWorktree_OpenBranchDiff(t *testing.T) { assert.ErrorContains(t, diff.Close(), "diff: git command failed") }) } + +func TestWorktree_OpenCommitDiff(t *testing.T) { + t.Parallel() + + mockExecer := git.NewMockExecer(gomock.NewController(t)) + _, wt := git.NewFakeRepository(t, "", mockExecer) + + mockExecer.EXPECT(). + Start(gomock.Any()). + DoAndReturn(func(cmd *exec.Cmd) error { + assert.Equal(t, []string{ + "git", "diff", "--find-renames", "--unified=0", "old", "new", + }, cmd.Args) + _, err := io.WriteString(cmd.Stdout, "diff output\n") + return errors.Join(err, cmd.Stdout.(io.Closer).Close()) + }) + mockExecer.EXPECT(). + Wait(gomock.Any()). + Return(nil) + + diff, err := wt.OpenCommitDiff(t.Context(), "old", "new") + require.NoError(t, err) + got, err := io.ReadAll(diff) + require.NoError(t, err) + require.NoError(t, diff.Close()) + assert.Equal(t, "diff output\n", string(got)) +} diff --git a/internal/handler/review/comment.go b/internal/handler/review/comment.go index c6a5fb3ca..8bb35bdc0 100644 --- a/internal/handler/review/comment.go +++ b/internal/handler/review/comment.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "io" "strings" "go.abhg.dev/gs/internal/forge" @@ -29,6 +30,15 @@ func (h *DraftHandler) SaveCommentDraft( ctx context.Context, req *CommentRequest, ) error { + // Capture the anchor's source revision before the editor can leave this + // process waiting while another process advances the branch. + branch, err := h.Service.LookupBranch(ctx, req.Branch) + if err != nil { + if errors.Is(err, state.ErrNotExist) { + return fmt.Errorf("branch not tracked: %s", req.Branch) + } + return fmt.Errorf("get branch: %w", err) + } body, err := h.commentBody(ctx, req.Message) if err != nil { return err @@ -36,7 +46,12 @@ func (h *DraftHandler) SaveCommentDraft( draft, err := h.Store.AddReviewDraft( ctx, req.Branch, - review.Draft{ID: 0, Body: body, Anchor: req.Anchor}, + review.Draft{ + ID: 0, + Body: body, + Anchor: req.Anchor, + CommitHash: branch.Head, + }, ) if err != nil { return fmt.Errorf("save draft comment: %w", err) @@ -239,20 +254,30 @@ func (h *DraftHandler) commentBody( return body, nil } -// loadPatch parses the selected branch's review diff. +// loadPatch parses the review diff ending at head. // Closing the diff reader also reports failures from the Git process. func (h *Handler) loadPatch( ctx context.Context, - base, branch string, + base, head string, ) (*reviewdiff.Patch, error) { - diff, err := h.Worktree.OpenBranchDiff(ctx, base, branch) + diff, err := h.Worktree.OpenBranchDiff(ctx, base, head) if err != nil { return nil, fmt.Errorf("open diff: %w", err) } + patch, err := parsePatch(diff) + if err != nil { + return nil, fmt.Errorf("parse diff: %w", err) + } + return patch, nil +} + +// parsePatch consumes and closes a Git diff. Closing the reader waits for the +// Git process, so a command failure is reported together with any parse error. +func parsePatch(diff io.ReadCloser) (*reviewdiff.Patch, error) { patch, err := reviewdiff.Parse(diff) err = errors.Join(err, diff.Close()) if err != nil { - return nil, fmt.Errorf("parse diff: %w", err) + return nil, err } return patch, nil } diff --git a/internal/handler/review/handler.go b/internal/handler/review/handler.go index 71daa5960..0d48d4fe5 100644 --- a/internal/handler/review/handler.go +++ b/internal/handler/review/handler.go @@ -34,9 +34,10 @@ type Handler struct { // DraftHandler coordinates workflows that only access local drafts. type DraftHandler struct { - Log *silog.Logger // required - Store Store // required - Editor CommentEditor // required + Log *silog.Logger // required + Service Service // required + Store Store // required + Editor CommentEditor // required } // ThreadHandler coordinates review-thread resolution changes. @@ -50,6 +51,8 @@ type ThreadHandler struct { // Worktree provides the Git operations used by review workflows. type Worktree interface { OpenBranchDiff(context.Context, string, string) (io.ReadCloser, error) + OpenCommitDiff(context.Context, string, string) (io.ReadCloser, error) + PeelToCommit(context.Context, string) (git.Hash, error) } var _ Worktree = (*git.Worktree)(nil) diff --git a/internal/handler/review/handler_test.go b/internal/handler/review/handler_test.go index be572fee4..4cb1b7790 100644 --- a/internal/handler/review/handler_test.go +++ b/internal/handler/review/handler_test.go @@ -1,7 +1,9 @@ package review import ( + "bytes" "context" + "errors" "io" "iter" "strings" @@ -10,6 +12,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.abhg.dev/gs/internal/forge" + "go.abhg.dev/gs/internal/git" reviewmodel "go.abhg.dev/gs/internal/review" "go.abhg.dev/gs/internal/silog" "go.abhg.dev/gs/internal/spice" @@ -18,20 +21,32 @@ import ( func TestDraftHandler_SaveCommentDraft(t *testing.T) { ctrl := gomock.NewController(t) + service := NewMockService(ctrl) store := NewMockStore(ctrl) handler := &DraftHandler{ - Log: silog.Nop(), - Store: store, + Log: silog.Nop(), + Service: service, + Store: store, Editor: func(context.Context, string) (string, error) { t.Fatal("editor should not open when a message is supplied") return "", nil }, } anchor := reviewmodel.Anchor{Path: "review.go", StartLine: 3, EndLine: 3} - wantDraft := reviewmodel.Draft{ID: 0, Body: "Use a constant.", Anchor: anchor} + head := git.Hash("1111111111111111111111111111111111111111") + wantDraft := reviewmodel.Draft{ + ID: 0, + Body: "Use a constant.", + Anchor: anchor, + CommitHash: head, + } wantSavedDraft := wantDraft wantSavedDraft.ID = 1 + service. + EXPECT(). + LookupBranch(gomock.Any(), "feature"). + Return(&spice.LookupBranchResponse{Head: head}, nil) store. EXPECT(). AddReviewDraft(gomock.Any(), "feature", wantDraft). @@ -45,12 +60,63 @@ func TestDraftHandler_SaveCommentDraft(t *testing.T) { require.NoError(t, err) } +func TestDraftHandler_SaveCommentDraft_capturesHeadBeforeEditor(t *testing.T) { + ctrl := gomock.NewController(t) + service := NewMockService(ctrl) + store := NewMockStore(ctrl) + sourceHead := git.Hash("1111111111111111111111111111111111111111") + updatedHead := git.Hash("2222222222222222222222222222222222222222") + currentHead := sourceHead + anchor := reviewmodel.Anchor{Path: "review.go", StartLine: 3, EndLine: 3} + handler := &DraftHandler{ + Log: silog.Nop(), + Service: service, + Store: store, + Editor: func(context.Context, string) (string, error) { + currentHead = updatedHead + return "Use a constant.", nil + }, + } + + service. + EXPECT(). + LookupBranch(gomock.Any(), "feature"). + DoAndReturn(func(context.Context, string) (*spice.LookupBranchResponse, error) { + return &spice.LookupBranchResponse{Head: currentHead}, nil + }) + store. + EXPECT(). + AddReviewDraft( + gomock.Any(), + "feature", + reviewmodel.Draft{ + ID: 0, + Body: "Use a constant.", + Anchor: anchor, + CommitHash: sourceHead, + }, + ). + Return(reviewmodel.Draft{ + ID: 1, + Body: "Use a constant.", + Anchor: anchor, + CommitHash: sourceHead, + }, nil) + + err := handler.SaveCommentDraft(t.Context(), &CommentRequest{ + Branch: "feature", + Anchor: anchor, + }) + require.NoError(t, err) +} + func TestDraftHandler_SaveReplyDraft(t *testing.T) { ctrl := gomock.NewController(t) store := NewMockStore(ctrl) handler := &DraftHandler{ - Log: silog.Nop(), - Store: store, + Log: silog.Nop(), + Service: nil, + Store: store, Editor: func(context.Context, string) (string, error) { return "That makes sense.", nil }, @@ -195,8 +261,9 @@ func TestDraftHandler_ReplaceDraftBody(t *testing.T) { ctrl := gomock.NewController(t) store := NewMockStore(ctrl) handler := &DraftHandler{ - Log: silog.Nop(), - Store: store, + Log: silog.Nop(), + Service: nil, + Store: store, Editor: func(context.Context, string) (string, error) { t.Fatal("editor should not open when a message is supplied") return "", nil @@ -230,9 +297,10 @@ func TestDraftHandler_DeleteDrafts(t *testing.T) { ctrl := gomock.NewController(t) store := NewMockStore(ctrl) handler := &DraftHandler{ - Log: silog.Nop(), - Store: store, - Editor: nil, + Log: silog.Nop(), + Service: nil, + Store: store, + Editor: nil, } store. @@ -327,8 +395,9 @@ func TestHandler_PublishDrafts(t *testing.T) { Editor: nil, } anchor := reviewmodel.Anchor{Path: "review.go", StartLine: 3, EndLine: 3} + head := git.Hash("2222222222222222222222222222222222222222") drafts := []Draft{ - {ID: 1, Body: "Use a constant.", Anchor: anchor}, + {ID: 1, Body: "Use a constant.", Anchor: anchor, CommitHash: head}, {ID: 2, Body: "Updated.", ReplyTo: "thread-1"}, } threadID := testThreadID("thread-1") @@ -342,11 +411,20 @@ func TestHandler_PublishDrafts(t *testing.T) { LookupBranch(gomock.Any(), "feature"). Return(&spice.LookupBranchResponse{ Base: "main", + Head: head, Change: &testChangeMetadata{id: testChangeID("42")}, }, nil) + repository. + EXPECT(). + ChangeStatuses(gomock.Any(), []forge.ChangeID{testChangeID("42")}). + Return([]forge.ChangeStatus{{HeadHash: head}}, nil) worktree. EXPECT(). - OpenBranchDiff(gomock.Any(), "main", "feature"). + PeelToCommit(gomock.Any(), head.String()). + Return(head, nil) + worktree. + EXPECT(). + OpenBranchDiff(gomock.Any(), "main", head.String()). Return(io.NopCloser(strings.NewReader(`diff --git a/review.go b/review.go --- a/review.go +++ b/review.go @@ -392,6 +470,553 @@ func TestHandler_PublishDrafts(t *testing.T) { require.NoError(t, err) } +func TestHandler_PublishDrafts_remapsAnchorsOncePerSource(t *testing.T) { + ctrl := gomock.NewController(t) + worktree := NewMockWorktree(ctrl) + store := NewMockStore(ctrl) + service := NewMockService(ctrl) + repository := NewMockReviewRepository(ctrl) + var logBuffer bytes.Buffer + handler := &Handler{ + Log: silog.New(&logBuffer, nil), + Worktree: worktree, + Service: service, + Store: store, + Repository: repository, + Editor: nil, + } + source := git.Hash("1111111111111111111111111111111111111111") + target := git.Hash("2222222222222222222222222222222222222222") + localHead := git.Hash("3333333333333333333333333333333333333333") + drafts := []Draft{ + { + ID: 1, + Body: "Use a constant.", + Anchor: reviewmodel.Anchor{Path: "old.go", StartLine: 2, EndLine: 2}, + CommitHash: source, + }, + { + ID: 2, + Body: "Keep this name.", + Anchor: reviewmodel.Anchor{Path: "other.go", StartLine: 1, EndLine: 1}, + CommitHash: source, + }, + } + store. + EXPECT(). + LoadReviewDrafts(gomock.Any(), "feature"). + Return(drafts, nil) + service. + EXPECT(). + LookupBranch(gomock.Any(), "feature"). + Return(&spice.LookupBranchResponse{ + Base: "main", + Head: localHead, + Change: &testChangeMetadata{id: testChangeID("42")}, + }, nil) + repository. + EXPECT(). + ChangeStatuses(gomock.Any(), []forge.ChangeID{testChangeID("42")}). + Return([]forge.ChangeStatus{{HeadHash: target}}, nil) + worktree. + EXPECT(). + PeelToCommit(gomock.Any(), target.String()). + Return(target, nil) + worktree. + EXPECT(). + OpenBranchDiff(gomock.Any(), "main", target.String()). + Return(io.NopCloser(strings.NewReader(`diff --git a/new.go b/new.go +new file mode 100644 +--- /dev/null ++++ b/new.go +@@ -0,0 +1,3 @@ ++zero ++one ++two +diff --git a/other.go b/other.go +new file mode 100644 +--- /dev/null ++++ b/other.go +@@ -0,0 +1 @@ ++package other +`)), nil) + worktree. + EXPECT(). + PeelToCommit(gomock.Any(), source.String()). + Return(source, nil) + worktree. + EXPECT(). + OpenCommitDiff(gomock.Any(), source.String(), target.String()). + Return(io.NopCloser(strings.NewReader(`diff --git a/old.go b/new.go +similarity index 66% +rename from old.go +rename to new.go +--- a/old.go ++++ b/new.go +@@ -1,2 +1,3 @@ ++zero + one + two +`)), nil) + repository. + EXPECT(). + SubmitReview( + gomock.Any(), + testChangeID("42"), + forge.SubmitReviewRequest{ + Comments: []forge.SubmitReviewCommentRequest{ + { + Path: "new.go", + Range: forge.ReviewThreadLine(3), + Body: "Use a constant.", + Side: forge.ReviewThreadSideRight, + }, + { + Path: "other.go", + Range: forge.ReviewThreadLine(1), + Body: "Keep this name.", + Side: forge.ReviewThreadSideRight, + }, + }, + }, + ). + Return(forge.SubmitReviewResult{}, nil) + store. + EXPECT(). + RemovePublishedReviewDrafts(gomock.Any(), "feature", drafts). + Return(nil) + + err := handler.PublishDrafts(t.Context(), &PublishDraftsRequest{ + Branch: "feature", + }) + require.NoError(t, err) + assert.Contains( + t, + logBuffer.String(), + "Draft 1: anchor moved to new.go:3", + ) + assert.NotContains(t, logBuffer.String(), "Draft 2: anchor moved") +} + +func TestHandler_PublishDrafts_statusFailureUsesLocalHead(t *testing.T) { + ctrl := gomock.NewController(t) + worktree := NewMockWorktree(ctrl) + store := NewMockStore(ctrl) + service := NewMockService(ctrl) + repository := NewMockReviewRepository(ctrl) + var logBuffer bytes.Buffer + handler := &Handler{ + Log: silog.New(&logBuffer, nil), + Worktree: worktree, + Service: service, + Store: store, + Repository: repository, + Editor: nil, + } + localHead := git.Hash("1111111111111111111111111111111111111111") + drafts := []Draft{{ + ID: 1, + Body: "Use a constant.", + Anchor: reviewmodel.Anchor{Path: "main.go", StartLine: 2, EndLine: 2}, + CommitHash: localHead, + }} + + store. + EXPECT(). + LoadReviewDrafts(gomock.Any(), "feature"). + Return(drafts, nil) + service. + EXPECT(). + LookupBranch(gomock.Any(), "feature"). + Return(&spice.LookupBranchResponse{ + Base: "main", + Head: localHead, + Change: &testChangeMetadata{id: testChangeID("42")}, + }, nil) + repository. + EXPECT(). + ChangeStatuses(gomock.Any(), []forge.ChangeID{testChangeID("42")}). + Return(nil, errors.New("status unavailable")) + worktree. + EXPECT(). + PeelToCommit(gomock.Any(), localHead.String()). + Return(localHead, nil) + worktree. + EXPECT(). + OpenBranchDiff(gomock.Any(), "main", localHead.String()). + Return(io.NopCloser(strings.NewReader(`diff --git a/main.go b/main.go +new file mode 100644 +--- /dev/null ++++ b/main.go +@@ -0,0 +1,2 @@ ++package main ++const answer = 42 +`)), nil) + repository. + EXPECT(). + SubmitReview( + gomock.Any(), + testChangeID("42"), + forge.SubmitReviewRequest{ + Comments: []forge.SubmitReviewCommentRequest{{ + Path: "main.go", + Range: forge.ReviewThreadLine(2), + Body: "Use a constant.", + Side: forge.ReviewThreadSideRight, + }}, + }, + ). + Return(forge.SubmitReviewResult{}, nil) + store. + EXPECT(). + RemovePublishedReviewDrafts(gomock.Any(), "feature", drafts). + Return(nil) + + err := handler.PublishDrafts(t.Context(), &PublishDraftsRequest{ + Branch: "feature", + }) + require.NoError(t, err) + assert.Contains(t, logBuffer.String(), "using local branch head") + assert.Contains(t, logBuffer.String(), "status unavailable") +} + +func TestHandler_PublishDrafts_missingRemoteHeadUsesLocalHead(t *testing.T) { + ctrl := gomock.NewController(t) + worktree := NewMockWorktree(ctrl) + store := NewMockStore(ctrl) + service := NewMockService(ctrl) + repository := NewMockReviewRepository(ctrl) + var logBuffer bytes.Buffer + handler := &Handler{ + Log: silog.New(&logBuffer, nil), + Worktree: worktree, + Service: service, + Store: store, + Repository: repository, + Editor: nil, + } + localHead := git.Hash("1111111111111111111111111111111111111111") + drafts := []Draft{{ + ID: 1, + Body: "Use a constant.", + Anchor: reviewmodel.Anchor{Path: "main.go", StartLine: 2, EndLine: 2}, + CommitHash: localHead, + }} + + store. + EXPECT(). + LoadReviewDrafts(gomock.Any(), "feature"). + Return(drafts, nil) + service. + EXPECT(). + LookupBranch(gomock.Any(), "feature"). + Return(&spice.LookupBranchResponse{ + Base: "main", + Head: localHead, + Change: &testChangeMetadata{id: testChangeID("42")}, + }, nil) + repository. + EXPECT(). + ChangeStatuses(gomock.Any(), []forge.ChangeID{testChangeID("42")}). + Return([]forge.ChangeStatus{{}}, nil) + worktree. + EXPECT(). + PeelToCommit(gomock.Any(), localHead.String()). + Return(localHead, nil) + worktree. + EXPECT(). + OpenBranchDiff(gomock.Any(), "main", localHead.String()). + Return(io.NopCloser(strings.NewReader(`diff --git a/main.go b/main.go +new file mode 100644 +--- /dev/null ++++ b/main.go +@@ -0,0 +1,2 @@ ++package main ++const answer = 42 +`)), nil) + repository. + EXPECT(). + SubmitReview( + gomock.Any(), + testChangeID("42"), + forge.SubmitReviewRequest{ + Comments: []forge.SubmitReviewCommentRequest{{ + Path: "main.go", + Range: forge.ReviewThreadLine(2), + Body: "Use a constant.", + Side: forge.ReviewThreadSideRight, + }}, + }, + ). + Return(forge.SubmitReviewResult{}, nil) + store. + EXPECT(). + RemovePublishedReviewDrafts(gomock.Any(), "feature", drafts). + Return(nil) + + err := handler.PublishDrafts(t.Context(), &PublishDraftsRequest{ + Branch: "feature", + }) + require.NoError(t, err) + assert.Contains(t, logBuffer.String(), "using local branch head") +} + +func TestHandler_PublishDrafts_unavailableHeadUsesSavedAnchor(t *testing.T) { + ctrl := gomock.NewController(t) + worktree := NewMockWorktree(ctrl) + store := NewMockStore(ctrl) + service := NewMockService(ctrl) + repository := NewMockReviewRepository(ctrl) + var logBuffer bytes.Buffer + handler := &Handler{ + Log: silog.New(&logBuffer, nil), + Worktree: worktree, + Service: service, + Store: store, + Repository: repository, + Editor: nil, + } + sourceHead := git.Hash("1111111111111111111111111111111111111111") + remoteHead := git.Hash("2222222222222222222222222222222222222222") + drafts := []Draft{{ + ID: 1, + Body: "Use a constant.", + Anchor: reviewmodel.Anchor{Path: "main.go", StartLine: 2, EndLine: 2}, + CommitHash: sourceHead, + }} + + store. + EXPECT(). + LoadReviewDrafts(gomock.Any(), "feature"). + Return(drafts, nil) + service. + EXPECT(). + LookupBranch(gomock.Any(), "feature"). + Return(&spice.LookupBranchResponse{ + Base: "main", + Head: sourceHead, + Change: &testChangeMetadata{id: testChangeID("42")}, + }, nil) + repository. + EXPECT(). + ChangeStatuses(gomock.Any(), []forge.ChangeID{testChangeID("42")}). + Return([]forge.ChangeStatus{{HeadHash: remoteHead}}, nil) + worktree. + EXPECT(). + PeelToCommit(gomock.Any(), remoteHead.String()). + Return("", git.ErrNotExist) + worktree. + EXPECT(). + OpenBranchDiff(gomock.Any(), "main", "feature"). + Return(io.NopCloser(strings.NewReader(`diff --git a/main.go b/main.go +new file mode 100644 +--- /dev/null ++++ b/main.go +@@ -0,0 +1,2 @@ ++package main ++const answer = 42 +`)), nil) + repository. + EXPECT(). + SubmitReview( + gomock.Any(), + testChangeID("42"), + forge.SubmitReviewRequest{ + Comments: []forge.SubmitReviewCommentRequest{{ + Path: "main.go", + Range: forge.ReviewThreadLine(2), + Body: "Use a constant.", + Side: forge.ReviewThreadSideRight, + }}, + }, + ). + Return(forge.SubmitReviewResult{}, nil) + store. + EXPECT(). + RemovePublishedReviewDrafts(gomock.Any(), "feature", drafts). + Return(nil) + + err := handler.PublishDrafts(t.Context(), &PublishDraftsRequest{ + Branch: "feature", + }) + require.NoError(t, err) + assert.Contains(t, logBuffer.String(), "using saved anchors") + assert.Contains(t, logBuffer.String(), git.ErrNotExist.Error()) +} + +func TestHandler_remapDraftAnchors_stopsAfterDeletedAnchor(t *testing.T) { + ctrl := gomock.NewController(t) + worktree := NewMockWorktree(ctrl) + handler := &Handler{ + Log: silog.Nop(), + Worktree: worktree, + Service: nil, + Store: nil, + Repository: nil, + Editor: nil, + } + firstSource := git.Hash("1111111111111111111111111111111111111111") + secondSource := git.Hash("2222222222222222222222222222222222222222") + head := git.Hash("3333333333333333333333333333333333333333") + drafts := []Draft{ + { + ID: 1, + Body: "Deleted target.", + Anchor: reviewmodel.Anchor{Path: "deleted.go", StartLine: 1, EndLine: 1}, + CommitHash: firstSource, + }, + { + ID: 2, + Body: "Later target.", + Anchor: reviewmodel.Anchor{Path: "later.go", StartLine: 1, EndLine: 1}, + CommitHash: secondSource, + }, + } + + worktree. + EXPECT(). + PeelToCommit(gomock.Any(), firstSource.String()). + Return(firstSource, nil) + worktree. + EXPECT(). + OpenCommitDiff(gomock.Any(), firstSource.String(), head.String()). + Return(io.NopCloser(strings.NewReader(`diff --git a/deleted.go b/deleted.go +deleted file mode 100644 +--- a/deleted.go ++++ /dev/null +@@ -1 +0,0 @@ +-package deleted +`)), nil) + + err := handler.remapDraftAnchors(t.Context(), head, drafts) + assert.ErrorContains(t, err, "comment target deleted.go:1 no longer exists") +} + +func TestHandler_remapDraftAnchors_sourceDiffFailureUsesSavedAnchor( + t *testing.T, +) { + ctrl := gomock.NewController(t) + worktree := NewMockWorktree(ctrl) + var logBuffer bytes.Buffer + handler := &Handler{ + Log: silog.New(&logBuffer, nil), + Worktree: worktree, + Service: nil, + Store: nil, + Repository: nil, + Editor: nil, + } + source := git.Hash("1111111111111111111111111111111111111111") + head := git.Hash("2222222222222222222222222222222222222222") + draft := Draft{ + ID: 1, + Body: "Use a constant.", + Anchor: reviewmodel.Anchor{Path: "main.go", StartLine: 2, EndLine: 2}, + CommitHash: source, + } + + worktree. + EXPECT(). + PeelToCommit(gomock.Any(), source.String()). + Return(source, nil) + worktree. + EXPECT(). + OpenCommitDiff(gomock.Any(), source.String(), head.String()). + Return(nil, errors.New("diff unavailable")) + + err := handler.remapDraftAnchors(t.Context(), head, []Draft{draft}) + require.NoError(t, err) + assert.Contains(t, logBuffer.String(), "Using saved anchor") + assert.Contains(t, logBuffer.String(), "diff unavailable") +} + +func TestHandler_PublishDrafts_unavailableSourceUsesSavedAnchor(t *testing.T) { + ctrl := gomock.NewController(t) + worktree := NewMockWorktree(ctrl) + store := NewMockStore(ctrl) + service := NewMockService(ctrl) + repository := NewMockReviewRepository(ctrl) + var logBuffer bytes.Buffer + handler := &Handler{ + Log: silog.New(&logBuffer, nil), + Worktree: worktree, + Service: service, + Store: store, + Repository: repository, + Editor: nil, + } + source := git.Hash("1111111111111111111111111111111111111111") + target := git.Hash("2222222222222222222222222222222222222222") + drafts := []Draft{{ + ID: 1, + Body: "Use a constant.", + Anchor: reviewmodel.Anchor{Path: "main.go", StartLine: 2, EndLine: 2}, + CommitHash: source, + }} + + store. + EXPECT(). + LoadReviewDrafts(gomock.Any(), "feature"). + Return(drafts, nil) + service. + EXPECT(). + LookupBranch(gomock.Any(), "feature"). + Return(&spice.LookupBranchResponse{ + Base: "main", + Head: target, + Change: &testChangeMetadata{id: testChangeID("42")}, + }, nil) + repository. + EXPECT(). + ChangeStatuses(gomock.Any(), []forge.ChangeID{testChangeID("42")}). + Return([]forge.ChangeStatus{{HeadHash: target}}, nil) + worktree. + EXPECT(). + PeelToCommit(gomock.Any(), target.String()). + Return(target, nil) + worktree. + EXPECT(). + OpenBranchDiff(gomock.Any(), "main", target.String()). + Return(io.NopCloser(strings.NewReader(`diff --git a/main.go b/main.go +new file mode 100644 +--- /dev/null ++++ b/main.go +@@ -0,0 +1,2 @@ ++package main ++const answer = 42 +`)), nil) + worktree. + EXPECT(). + PeelToCommit(gomock.Any(), source.String()). + Return("", git.ErrNotExist) + repository. + EXPECT(). + SubmitReview( + gomock.Any(), + testChangeID("42"), + forge.SubmitReviewRequest{ + Comments: []forge.SubmitReviewCommentRequest{{ + Path: "main.go", + Range: forge.ReviewThreadLine(2), + Body: "Use a constant.", + Side: forge.ReviewThreadSideRight, + }}, + }, + ). + Return(forge.SubmitReviewResult{}, nil) + store. + EXPECT(). + RemovePublishedReviewDrafts(gomock.Any(), "feature", drafts). + Return(nil) + + err := handler.PublishDrafts(t.Context(), &PublishDraftsRequest{ + Branch: "feature", + }) + require.NoError(t, err) + assert.Contains(t, logBuffer.String(), "Using saved anchor") + assert.Contains(t, logBuffer.String(), "source commit 1111111") +} + func TestThreadHandler_SetThreadResolution(t *testing.T) { ctrl := gomock.NewController(t) service := NewMockService(ctrl) diff --git a/internal/handler/review/mocks_test.go b/internal/handler/review/mocks_test.go index 662ba1afc..e62fd1240 100644 --- a/internal/handler/review/mocks_test.go +++ b/internal/handler/review/mocks_test.go @@ -14,6 +14,7 @@ import ( io "io" reflect "reflect" + git "go.abhg.dev/gs/internal/git" review "go.abhg.dev/gs/internal/review" spice "go.abhg.dev/gs/internal/spice" gomock "go.uber.org/mock/gomock" @@ -82,6 +83,84 @@ func (c *MockWorktreeOpenBranchDiffCall) DoAndReturn(f func(context.Context, str return c } +// OpenCommitDiff mocks base method. +func (m *MockWorktree) OpenCommitDiff(arg0 context.Context, arg1, arg2 string) (io.ReadCloser, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "OpenCommitDiff", arg0, arg1, arg2) + ret0, _ := ret[0].(io.ReadCloser) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// OpenCommitDiff indicates an expected call of OpenCommitDiff. +func (mr *MockWorktreeMockRecorder) OpenCommitDiff(arg0, arg1, arg2 any) *MockWorktreeOpenCommitDiffCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "OpenCommitDiff", reflect.TypeOf((*MockWorktree)(nil).OpenCommitDiff), arg0, arg1, arg2) + return &MockWorktreeOpenCommitDiffCall{Call: call} +} + +// MockWorktreeOpenCommitDiffCall wrap *gomock.Call +type MockWorktreeOpenCommitDiffCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockWorktreeOpenCommitDiffCall) Return(arg0 io.ReadCloser, arg1 error) *MockWorktreeOpenCommitDiffCall { + c.Call = c.Call.Return(arg0, arg1) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockWorktreeOpenCommitDiffCall) Do(f func(context.Context, string, string) (io.ReadCloser, error)) *MockWorktreeOpenCommitDiffCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockWorktreeOpenCommitDiffCall) DoAndReturn(f func(context.Context, string, string) (io.ReadCloser, error)) *MockWorktreeOpenCommitDiffCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + +// PeelToCommit mocks base method. +func (m *MockWorktree) PeelToCommit(arg0 context.Context, arg1 string) (git.Hash, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "PeelToCommit", arg0, arg1) + ret0, _ := ret[0].(git.Hash) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// PeelToCommit indicates an expected call of PeelToCommit. +func (mr *MockWorktreeMockRecorder) PeelToCommit(arg0, arg1 any) *MockWorktreePeelToCommitCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "PeelToCommit", reflect.TypeOf((*MockWorktree)(nil).PeelToCommit), arg0, arg1) + return &MockWorktreePeelToCommitCall{Call: call} +} + +// MockWorktreePeelToCommitCall wrap *gomock.Call +type MockWorktreePeelToCommitCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockWorktreePeelToCommitCall) Return(arg0 git.Hash, arg1 error) *MockWorktreePeelToCommitCall { + c.Call = c.Call.Return(arg0, arg1) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockWorktreePeelToCommitCall) Do(f func(context.Context, string) (git.Hash, error)) *MockWorktreePeelToCommitCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockWorktreePeelToCommitCall) DoAndReturn(f func(context.Context, string) (git.Hash, error)) *MockWorktreePeelToCommitCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + // MockService is a mock of Service interface. type MockService struct { ctrl *gomock.Controller diff --git a/internal/handler/review/publish.go b/internal/handler/review/publish.go index fc1b389f7..fe7df36e1 100644 --- a/internal/handler/review/publish.go +++ b/internal/handler/review/publish.go @@ -6,6 +6,10 @@ import ( "fmt" "go.abhg.dev/gs/internal/forge" + "go.abhg.dev/gs/internal/git" + "go.abhg.dev/gs/internal/must" + "go.abhg.dev/gs/internal/review" + "go.abhg.dev/gs/internal/reviewdiff" "go.abhg.dev/gs/internal/spice/state" ) @@ -50,20 +54,47 @@ func (h *Handler) PublishDrafts( req.Branch, ) } - patch, err := h.loadPatch(ctx, change.Base, req.Branch) + changeID := change.Change.ChangeID() + reviewHead := change.Head + // Prefer the remote change head because forges interpret review coordinates + // against it. Status lookup is best-effort so review publication retains its + // pre-remapping behavior when the forge cannot report that revision. + statuses, err := h.Repository.ChangeStatuses(ctx, []forge.ChangeID{changeID}) + if err != nil { + h.Log.Warnf("%v: remote head unavailable, using local branch head (%v): %v", changeID, reviewHead.Short(), err) + } else { + must.BeEqualf(len(statuses), 1, "expected one status for %v, got %d", changeID, len(statuses)) + if statuses[0].HeadHash.IsZero() { + h.Log.Warnf("%v: remote head unavailable, using local branch head (%v)", changeID, reviewHead.Short()) + } else { + reviewHead = statuses[0].HeadHash + } + } + + patchHead := req.Branch + resolvedHead, err := h.Worktree.PeelToCommit(ctx, reviewHead.String()) + if err != nil { + h.Log.Warnf("%v: head (%v) unavailable locally, using saved anchors: %v", changeID, reviewHead.Short(), err) + } else { + patchHead = resolvedHead.String() + if err := h.remapDraftAnchors(ctx, resolvedHead, drafts); err != nil { + return err + } + } + + patch, err := h.loadPatch(ctx, change.Base, patchHead) if err != nil { return err } threadIDs, err := resolveDraftThreadIDs( ctx, h.Repository, - change.Change.ChangeID(), + changeID, drafts, ) if err != nil { return err } - comments := make([]forge.SubmitReviewCommentRequest, 0, len(drafts)) for _, draft := range drafts { if draft.ReplyTo != "" { @@ -74,29 +105,26 @@ func (h *Handler) PublishDrafts( continue } - if draft.Anchor.IsFile() && !patch.ContainsFile(draft.Anchor.Path) { + anchor := draft.Anchor + if anchor.IsFile() && !patch.ContainsFile(anchor.Path) { return fmt.Errorf( "draft %s: review diff does not contain file %q", draft.ID, - draft.Anchor.Path, + anchor.Path, ) } - if !draft.Anchor.IsFile() && !patch.ContainsLineRange( - draft.Anchor.Path, - draft.Anchor.StartLine, - draft.Anchor.EndLine, - ) { + if !anchor.IsFile() && !patch.ContainsLineRange(anchor.Path, anchor.StartLine, anchor.EndLine) { return fmt.Errorf( "draft %s: review diff does not contain %s", draft.ID, - draft.Anchor, + anchor, ) } comments = append(comments, forge.SubmitReviewCommentRequest{ - Path: draft.Anchor.Path, + Path: anchor.Path, Range: forge.ReviewThreadRange{ - StartLine: draft.Anchor.StartLine, - EndLine: draft.Anchor.EndLine, + StartLine: anchor.StartLine, + EndLine: anchor.EndLine, }, Body: draft.Body, Side: forge.ReviewThreadSideRight, @@ -105,7 +133,7 @@ func (h *Handler) PublishDrafts( if _, err := h.Repository.SubmitReview( ctx, - change.Change.ChangeID(), + changeID, forge.SubmitReviewRequest{ Body: req.Body, Disposition: req.Disposition, @@ -125,11 +153,106 @@ func (h *Handler) PublishDrafts( h.Log.Infof( "Published %d comment(s) as review on %s.", len(comments), - change.Change.ChangeID(), + changeID, ) return nil } +// remapDraftAnchors follows root drafts from their recorded branch revisions +// to head. Failures loading an individual source patch preserve saved +// coordinates; a target known to have been deleted returns an error. +func (h *Handler) remapDraftAnchors( + ctx context.Context, + head git.Hash, + drafts []review.Draft, +) error { + // draftAnchorSource memoizes one source-to-head patch or its load failure. + type draftAnchorSource struct { + patch *reviewdiff.Patch + err error + } + + // Key entries by the commit hash recorded with each draft. Drafts created + // at the same branch revision then share a patch or its load failure. + sources := make(map[git.Hash]draftAnchorSource) + for idx, draft := range drafts { + if draft.ReplyTo != "" { + continue + } + + if draft.CommitHash.IsZero() || draft.CommitHash == head { + // Unlikely that the commit hash wasn't recorded + // but easy enough to handle. + continue + } + + source, ok := sources[draft.CommitHash] + if !ok { + source.patch, source.err = h.loadDraftAnchorPatch(ctx, draft.CommitHash, head) + sources[draft.CommitHash] = source + } + if source.err != nil { + h.Log.Warn( + "Draft head moved but patch was unavailable. Using saved anchor.", + "draftID", draft.ID, + "anchor", draft.Anchor, + "oldHead", draft.CommitHash.Short(), + "newHead", head.Short(), + "error", source.err, + ) + continue + } + + anchor, ok := source.patch.MapAnchor(draft.Anchor) + if !ok { + return fmt.Errorf( + "draft %s: comment target %s no longer exists after branch changes", + draft.ID, + draft.Anchor, + ) + } + + if anchor != draft.Anchor { + h.Log.Infof("Draft %d: anchor moved to %v", draft.ID, anchor) + } + + draft.Anchor = anchor + drafts[idx] = draft + } + return nil +} + +// loadDraftAnchorPatch parses the change from the commit hash recorded with a +// draft to the branch's current head. +func (h *Handler) loadDraftAnchorPatch( + ctx context.Context, + draftCommit git.Hash, + head git.Hash, +) (*reviewdiff.Patch, error) { + source, err := h.Worktree.PeelToCommit(ctx, draftCommit.String()) + if err != nil { + return nil, fmt.Errorf( + "resolve source commit %s: %w", + draftCommit.Short(), + err, + ) + } + + diff, err := h.Worktree.OpenCommitDiff( + ctx, + source.String(), + head.String(), + ) + if err != nil { + return nil, fmt.Errorf("open commit diff: %w", err) + } + patch, err := parsePatch(diff) + if err != nil { + return nil, fmt.Errorf("parse commit diff: %w", err) + } + return patch, nil +} + // resolveDraftThreadIDs recovers opaque forge IDs for every drafted reply. // One traversal resolves all targets before the review is submitted. func resolveDraftThreadIDs( diff --git a/internal/jsonmut/jsonmut.go b/internal/jsonmut/jsonmut.go index 78684925b..c53578985 100644 --- a/internal/jsonmut/jsonmut.go +++ b/internal/jsonmut/jsonmut.go @@ -214,6 +214,19 @@ func Delete(path jsontext.Pointer) Statement { return newMutationStatement(path, nil, mutationDelete) } +// DeleteIfPresent returns a statement that removes the object member at path. +// It leaves the document unchanged when path does not exist. +func DeleteIfPresent(path jsontext.Pointer) Statement { + must.Bef(path.IsValid(), "invalid JSON pointer %q", path) + must.Bef(path != "", "delete path must not be the document root") + return Lookup(path).Then(func(value jsontext.Value) Statement { + if len(value) == 0 { + return Block() + } + return Delete(path) + }) +} + type mutationMode uint8 const ( diff --git a/internal/jsonmut/jsonmut_test.go b/internal/jsonmut/jsonmut_test.go index f3f35e12d..8dfab4264 100644 --- a/internal/jsonmut/jsonmut_test.go +++ b/internal/jsonmut/jsonmut_test.go @@ -376,6 +376,30 @@ func TestDelete_missing(t *testing.T) { assert.ErrorIs(t, err, jsonmut.ErrNotExist) } +func TestDeleteIfPresent(t *testing.T) { + t.Parallel() + + updated, _, err := jsonmut.Apply( + jsontext.Value(`{ + "drafts": { + "7": {"body": "remove"}, + "8": {"body": "also remove"}, + "9": {"body": "keep"} + } + }`), + jsonmut.Block( + jsonmut.DeleteIfPresent("/drafts/7"), + jsonmut.DeleteIfPresent("/drafts/missing"), + jsonmut.DeleteIfPresent("/missing/8"), + jsonmut.DeleteIfPresent("/drafts/8"), + ), + ) + require.NoError(t, err) + assert.JSONEq(t, `{ + "drafts": {"9": {"body": "keep"}} + }`, updated.String()) +} + func TestSet_missingParent(t *testing.T) { t.Parallel() diff --git a/internal/review/draft.go b/internal/review/draft.go index fa4b31910..db6d05415 100644 --- a/internal/review/draft.go +++ b/internal/review/draft.go @@ -1,6 +1,10 @@ package review -import "strconv" +import ( + "strconv" + + "go.abhg.dev/gs/internal/git" +) // DraftID identifies a local review draft within one branch. type DraftID int @@ -15,6 +19,11 @@ type Draft struct { ID DraftID // required Body string // required + // CommitHash identifies the branch revision containing Anchor. + // It is zero for replies and drafts created before source revisions were + // recorded. + CommitHash git.Hash + // Anchor identifies the location of a root comment. // It is zero for a reply. Anchor Anchor diff --git a/internal/reviewdiff/map.go b/internal/reviewdiff/map.go new file mode 100644 index 000000000..bfcbd4664 --- /dev/null +++ b/internal/reviewdiff/map.go @@ -0,0 +1,145 @@ +package reviewdiff + +import ( + "github.com/bluekeyes/go-gitdiff/gitdiff" + "go.abhg.dev/gs/internal/review" +) + +// MapAnchor maps an anchor in the patch preimage to its corresponding +// postimage region. It reports false when the attached file or region is +// deleted without replacement. +// +// An anchor for a file absent from the patch is unchanged. Insertions within a +// line range expand the range, deletions shrink it, and replacements map to the +// replacement region. +func (p *Patch) MapAnchor(anchor review.Anchor) (review.Anchor, bool) { + var zero review.Anchor + + mapping, ok := p.mappings[anchor.Path] + if !ok { + return anchor, true + } + if mapping.newPath == "" { + return zero, false + } + + anchor.Path = mapping.newPath + if anchor.IsFile() { + return anchor, true + } + + start, end, ok := mapping.mapRange(anchor.StartLine, anchor.EndLine) + if !ok { + return zero, false + } + anchor.StartLine = start + anchor.EndLine = end + return anchor, true +} + +// fileMapping describes how one preimage file becomes its postimage file. +// An empty newPath means that the file was deleted. +type fileMapping struct { + newPath string + edits []lineEdit +} + +// newFileMapping translates parsed diff fragments into ordered replacement +// operations against preimage line numbers. +func newFileMapping(file *gitdiff.File) fileMapping { + mapping := fileMapping{newPath: file.NewName} + for _, fragment := range file.TextFragments { + oldLine := int(fragment.OldPosition) + if fragment.OldLines == 0 { + // A zero-length hunk identifies the line before an insertion. + oldLine++ + } + + // Adjacent deletions and additions form one replacement. Context lines + // end the current replacement and advance both sides of the diff. + var edit lineEdit + appendEdit := func() { + if edit.deleted == 0 && edit.added == 0 { + return + } + mapping.edits = append(mapping.edits, edit) + edit = lineEdit{} + } + for _, line := range fragment.Lines { + switch line.Op { + case gitdiff.OpContext: + appendEdit() + oldLine++ + case gitdiff.OpDelete: + if edit.deleted == 0 && edit.added == 0 { + edit.oldStart = oldLine + } + edit.deleted++ + oldLine++ + case gitdiff.OpAdd: + if edit.deleted == 0 && edit.added == 0 { + edit.oldStart = oldLine + } + edit.added++ + } + } + appendEdit() + } + return mapping +} + +// mapRange applies each replacement in source order. It reports false only +// when deletions consume the complete selected range without replacing it. +func (m fileMapping) mapRange(start, end int) (int, int, bool) { + var offset int + for _, edit := range m.edits { + // Earlier edits move later preimage coordinates by their net line count. + position := edit.oldStart + offset + if edit.deleted == 0 { + // An insertion before the range shifts it. An insertion strictly + // inside the range becomes part of the selected region. + switch { + case position <= start: + start += edit.added + end += edit.added + case position <= end: + end += edit.added + } + offset += edit.added + continue + } + + deletedEnd := position + edit.deleted - 1 + switch { + case deletedEnd < start: + start += edit.added - edit.deleted + end += edit.added - edit.deleted + case position > end: + default: + // Preserve surviving selected lines around the edit and include any + // replacement lines when the edit overlaps the selected region. + keepsLeft := start < position + keepsRight := end > deletedEnd + if edit.added == 0 && !keepsLeft && !keepsRight { + return 0, 0, false + } + if !keepsLeft { + start = position + } + if keepsRight { + end += edit.added - edit.deleted + } else { + end = position + edit.added - 1 + } + } + offset += edit.added - edit.deleted + } + return start, end, true +} + +// lineEdit replaces deleted preimage lines with added postimage lines. +type lineEdit struct { + oldStart int + deleted int + added int +} diff --git a/internal/reviewdiff/map_test.go b/internal/reviewdiff/map_test.go new file mode 100644 index 000000000..9f169bf5f --- /dev/null +++ b/internal/reviewdiff/map_test.go @@ -0,0 +1,186 @@ +package reviewdiff_test + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.abhg.dev/gs/internal/review" + "go.abhg.dev/gs/internal/reviewdiff" +) + +func TestPatchMapAnchor(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + patch string + anchor review.Anchor + want review.Anchor + wantOK bool + }{ + { + name: "EarlierInsertionShiftsRange", + patch: `diff --git a/main.go b/main.go +--- a/main.go ++++ b/main.go +@@ -1,2 +1,3 @@ ++zero + one + two +`, + anchor: review.Anchor{Path: "main.go", StartLine: 2, EndLine: 2}, + want: review.Anchor{Path: "main.go", StartLine: 3, EndLine: 3}, + wantOK: true, + }, + { + name: "InternalInsertionExpandsRange", + patch: `diff --git a/main.go b/main.go +--- a/main.go ++++ b/main.go +@@ -1,3 +1,4 @@ + one ++inserted + two + three +`, + anchor: review.Anchor{Path: "main.go", StartLine: 1, EndLine: 2}, + want: review.Anchor{Path: "main.go", StartLine: 1, EndLine: 3}, + wantOK: true, + }, + { + name: "InsertionAtStartShiftsRange", + patch: `diff --git a/main.go b/main.go +--- a/main.go ++++ b/main.go +@@ -1,0 +2 @@ ++inserted +`, + anchor: review.Anchor{Path: "main.go", StartLine: 2, EndLine: 3}, + want: review.Anchor{Path: "main.go", StartLine: 3, EndLine: 4}, + wantOK: true, + }, + { + name: "InsertionAfterEndKeepsRange", + patch: `diff --git a/main.go b/main.go +--- a/main.go ++++ b/main.go +@@ -3,0 +4 @@ ++inserted +`, + anchor: review.Anchor{Path: "main.go", StartLine: 2, EndLine: 3}, + want: review.Anchor{Path: "main.go", StartLine: 2, EndLine: 3}, + wantOK: true, + }, + { + name: "InternalDeletionShrinksRange", + patch: `diff --git a/main.go b/main.go +--- a/main.go ++++ b/main.go +@@ -1,4 +1,3 @@ + one +-two + three + four +`, + anchor: review.Anchor{Path: "main.go", StartLine: 1, EndLine: 3}, + want: review.Anchor{Path: "main.go", StartLine: 1, EndLine: 2}, + wantOK: true, + }, + { + name: "ReplacementMapsToReplacementRange", + patch: `diff --git a/main.go b/main.go +--- a/main.go ++++ b/main.go +@@ -1,3 +1,4 @@ + one +-two ++replacement one ++replacement two + three +`, + anchor: review.Anchor{Path: "main.go", StartLine: 2, EndLine: 2}, + want: review.Anchor{Path: "main.go", StartLine: 2, EndLine: 3}, + wantOK: true, + }, + { + name: "RenameMapsPath", + patch: `diff --git a/old.go b/new.go +similarity index 100% +rename from old.go +rename to new.go +`, + anchor: review.Anchor{Path: "old.go", StartLine: 4, EndLine: 6}, + want: review.Anchor{Path: "new.go", StartLine: 4, EndLine: 6}, + wantOK: true, + }, + { + name: "RenameMapsFile", + patch: `diff --git a/old.go b/new.go +similarity index 100% +rename from old.go +rename to new.go +`, + anchor: review.Anchor{Path: "old.go"}, + want: review.Anchor{Path: "new.go"}, + wantOK: true, + }, + { + name: "CopyKeepsOriginalPath", + patch: `diff --git a/original.go b/copy.go +similarity index 100% +copy from original.go +copy to copy.go +`, + anchor: review.Anchor{Path: "original.go", StartLine: 4, EndLine: 6}, + want: review.Anchor{Path: "original.go", StartLine: 4, EndLine: 6}, + wantOK: true, + }, + { + name: "DeletedRangeDisappears", + patch: `diff --git a/main.go b/main.go +--- a/main.go ++++ b/main.go +@@ -1,3 +1,2 @@ + one +-two + three +`, + anchor: review.Anchor{Path: "main.go", StartLine: 2, EndLine: 2}, + wantOK: false, + }, + { + name: "DeletedFileDisappears", + patch: `diff --git a/main.go b/main.go +deleted file mode 100644 +--- a/main.go ++++ /dev/null +@@ -1 +0,0 @@ +-package main +`, + anchor: review.Anchor{Path: "main.go"}, + wantOK: false, + }, + { + name: "UnchangedFileKeepsAnchor", + patch: "", + anchor: review.Anchor{Path: "main.go", StartLine: 4, EndLine: 6}, + want: review.Anchor{Path: "main.go", StartLine: 4, EndLine: 6}, + wantOK: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + patch, err := reviewdiff.Parse(strings.NewReader(tt.patch)) + require.NoError(t, err) + + got, ok := patch.MapAnchor(tt.anchor) + assert.Equal(t, tt.wantOK, ok) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/internal/reviewdiff/patch.go b/internal/reviewdiff/patch.go index 48fcaf485..171dad376 100644 --- a/internal/reviewdiff/patch.go +++ b/internal/reviewdiff/patch.go @@ -16,6 +16,7 @@ import ( type Patch struct { files map[string][]lineRange deletions map[string][]lineRange + mappings map[string]fileMapping } // Parse parses a Git patch for review-comment queries. @@ -28,8 +29,13 @@ func Parse(src io.Reader) (*Patch, error) { patch := &Patch{ files: make(map[string][]lineRange), deletions: make(map[string][]lineRange), + mappings: make(map[string]fileMapping), } for _, file := range files { + if file.OldName != "" && !file.IsCopy { + patch.mappings[file.OldName] = newFileMapping(file) + } + // A destination path is enough to make a file commentable, including // binary, rename-only, and mode-only changes without text fragments. if file.NewName != "" { diff --git a/internal/spice/state/review_draft.go b/internal/spice/state/review_draft.go index 1c96669d9..7316278c5 100644 --- a/internal/spice/state/review_draft.go +++ b/internal/spice/state/review_draft.go @@ -10,6 +10,7 @@ import ( "path" "slices" + "go.abhg.dev/gs/internal/git" "go.abhg.dev/gs/internal/jsonmut" "go.abhg.dev/gs/internal/review" "go.abhg.dev/gs/internal/spice/state/storage" @@ -23,11 +24,12 @@ type reviewDraftState struct { } type storedReviewDraft struct { - File string `json:"file"` - Line int `json:"line"` - EndLine int `json:"endLine,omitempty"` - Body string `json:"body"` - ThreadID string `json:"threadID,omitempty"` + File string `json:"file"` + Line int `json:"line"` + EndLine int `json:"endLine,omitempty"` + Body string `json:"body"` + ThreadID string `json:"threadID,omitempty"` + CommitHash git.Hash `json:"commitHash,omitempty"` } // AddReviewDraft atomically assigns a branch-local ID and saves a draft. @@ -152,7 +154,7 @@ func (s *Store) UpdateReviewDraftBody( return nil } -// RemovePublishedReviewDrafts removes unchanged drafts after publication. +// RemovePublishedReviewDrafts removes drafts by ID after publication. func (s *Store) RemovePublishedReviewDrafts( ctx context.Context, branch string, @@ -160,23 +162,13 @@ func (s *Store) RemovePublishedReviewDrafts( ) error { statements := make([]jsonmut.Statement, 0, len(published)) for _, draft := range published { - stored := storeReviewDraft(draft) path := jsontext.Pointer("/drafts").AppendToken(draft.ID.String()) - statements = append(statements, - jsonmut.Decode[*storedReviewDraft](path).Then( - func(current *storedReviewDraft) jsonmut.Statement { - if current == nil || *current != stored { - return jsonmut.Block() - } - return jsonmut.Delete(path) - }, - ), - ) + statements = append(statements, jsonmut.DeleteIfPresent(path)) } - // Forge submission happens before this mutation starts. - // Replaying the program removes only values that still match the request, - // leaving drafts added or edited while submission was in flight intact. + // Forge submission happens before this mutation starts. Replaying the + // program removes the published IDs from the latest state, including drafts + // edited while submission was in flight. err := storage.UpdateJSON( ctx, s.db, @@ -214,9 +206,10 @@ func (s *Store) LoadReviewDrafts( stored := state.Drafts[id] if stored.ThreadID != "" { drafts[i] = review.Draft{ - ID: id, - Body: stored.Body, - ReplyTo: stored.ThreadID, + ID: id, + Body: stored.Body, + ReplyTo: stored.ThreadID, + CommitHash: stored.CommitHash, } continue } @@ -226,8 +219,9 @@ func (s *Store) LoadReviewDrafts( endLine = stored.Line } drafts[i] = review.Draft{ - ID: id, - Body: stored.Body, + ID: id, + Body: stored.Body, + CommitHash: stored.CommitHash, Anchor: review.Anchor{ Path: stored.File, StartLine: stored.Line, @@ -254,7 +248,8 @@ func (s *Store) loadReviewDraftState( func storeReviewDraft(draft review.Draft) storedReviewDraft { stored := storedReviewDraft{ - Body: draft.Body, + Body: draft.Body, + CommitHash: draft.CommitHash, } if draft.ReplyTo != "" { stored.ThreadID = draft.ReplyTo diff --git a/internal/spice/state/review_draft_publish_test.go b/internal/spice/state/review_draft_publish_test.go index 089b2cfdf..f240a3d43 100644 --- a/internal/spice/state/review_draft_publish_test.go +++ b/internal/spice/state/review_draft_publish_test.go @@ -66,7 +66,7 @@ func TestReviewDraftsPublishPreservesAddedDraft(t *testing.T) { assert.Equal(t, []review.Draft{added}, drafts) } -func TestReviewDraftsPublishPreservesEditedDraft(t *testing.T) { +func TestReviewDraftsPublishRemovesEditedDraft(t *testing.T) { ctx := t.Context() db := storage.NewDB(make(storage.MapBackend)) store, err := state.InitStore(ctx, state.InitStoreRequest{ @@ -108,9 +108,7 @@ func TestReviewDraftsPublishPreservesEditedDraft(t *testing.T) { drafts, err := store.LoadReviewDrafts(ctx, "feat") require.NoError(t, err) require.NotNil(t, drafts) - edited := added - edited.Body = "First edited" - assert.Equal(t, []review.Draft{edited}, drafts) + assert.Empty(t, drafts) } func TestReviewDraftsPublishAfterBranchDeletion(t *testing.T) { diff --git a/internal/spice/state/review_draft_test.go b/internal/spice/state/review_draft_test.go index 349e88041..6ee40b468 100644 --- a/internal/spice/state/review_draft_test.go +++ b/internal/spice/state/review_draft_test.go @@ -5,6 +5,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.abhg.dev/gs/internal/git" "go.abhg.dev/gs/internal/review" "go.abhg.dev/gs/internal/spice/state" "go.abhg.dev/gs/internal/spice/state/storage" @@ -31,8 +32,9 @@ func TestReviewDrafts(t *testing.T) { ctx, "feature", review.Draft{ - ID: 0, - Body: "comment body", + ID: 0, + Body: "comment body", + CommitHash: git.Hash("1111111111111111111111111111111111111111"), Anchor: review.Anchor{ Path: "main.go", StartLine: 42, @@ -61,6 +63,7 @@ func TestReviewDrafts(t *testing.T) { require.NoError(t, err) require.Len(t, drafts, 2) assert.Equal(t, "updated body", drafts[0].Body) + assert.Equal(t, comment.CommitHash, drafts[0].CommitHash) assert.Equal(t, reply, drafts[1]) require.NoError(t, store.RemovePublishedReviewDrafts( diff --git a/review.go b/review.go index 804b76fb1..f42f39577 100644 --- a/review.go +++ b/review.go @@ -50,13 +50,15 @@ func (*reviewCmd) AfterApply(kctx *kong.Context) error { }), kctx.BindToProvider(func( log *silog.Logger, + svc *spice.Service, store *state.Store, gitRepo *git.Repository, ) (ReviewDraftHandler, error) { return &review.DraftHandler{ - Log: log, - Store: store, - Editor: newReviewCommentEditor(gitRepo), + Log: log, + Service: svc, + Store: store, + Editor: newReviewCommentEditor(gitRepo), }, nil }), kctx.BindToProvider(func( diff --git a/testdata/script/review_publish_remaps_drafts.txt b/testdata/script/review_publish_remaps_drafts.txt new file mode 100644 index 000000000..22a54a78f --- /dev/null +++ b/testdata/script/review_publish_remaps_drafts.txt @@ -0,0 +1,125 @@ +# Draft anchors follow their source lines when the branch changes before +# publication. + +as 'Test ' +at '2026-09-06T13:57:13Z' + +cd repo +git init +git commit --allow-empty -m 'Initial commit' +gs repo init + +shamhub-setup +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +git add feature.go +gs bc -m 'Add feature' feature1 +gs branch submit --fill +stderr 'Created #' + +# Record line, range, and file anchors against the original branch head. +gs review comment feature.go:4 -m 'Line draft.' +stderr 'Drafted comment 1 on feature.go:4' +gs review comment feature.go:5-6 -m 'Range draft.' +stderr 'Drafted comment 2 on feature.go:5-6' +gs review comment feature.go -m 'File draft.' +stderr 'Drafted comment 3 on feature.go' + +# Rename the file, insert before both line anchors, and replace the final line +# in the range with three lines. Publishing should follow those edits. +mv feature.go reviewed.go +cp $WORK/extra/reviewed.go reviewed.go +git add -A +gs ca --no-edit +gs branch submit --force + +gs review publish +stderr 'Published 3 comment' +shamhub dump reviews 1 +cmp stdout $WORK/golden/reviews.yaml + +-- repo/feature.go -- +package main + +func work() { + prepare() + first() + second() + finish() +} + +func helper() { + one() + two() + three() + four() + five() +} +-- extra/reviewed.go -- +package main + +const enabled = true +func work() { + prepare() + first() + inserted() + replacementOne() + replacementTwo() + finish() +} + +func helper() { + one() + two() + three() + four() + five() +} +-- golden/reviews.yaml -- +changes: + - change: 1 + submissions: + - submitter: alice + disposition: comment + commentIDs: + - 2 + - 3 + - 4 + threads: + - id: thread-2 + path: reviewed.go + range: + start: 5 + end: 5 + side: right + resolved: false + outdated: false + comments: + - id: 2 + author: alice + body: Line draft. + - id: thread-3 + path: reviewed.go + range: + start: 6 + end: 9 + side: right + resolved: false + outdated: false + comments: + - id: 3 + author: alice + body: Range draft. + - id: thread-4 + path: reviewed.go + resolved: false + outdated: false + comments: + - id: 4 + author: alice + body: File draft.