#1 feat(mr): notes with threads, line ranges, edits and verdicts

state: merged · 86ac875 · fast-forward
branch: feat/mr-notes → main
opened: bnema · 3d ago
updated: 3d ago

Merge request notes (ADR 0032): one uniform review object, published at once, no draft review.

Threads (one level, resolution on the root), line ranges on one side, author edit and delete.
Notes and descriptions rendered as safe Markdown (no raw HTML, link policy), ^N links to note N.
Optional verdict on a root note (approve, changes requested), needs the new mr:review scope; informative only; latest per author; approvals stale after a push; changes requested stay until the author's next verdict; immutable.
Events comment_edited, comment_deleted, comment_resolved.
RPC, CLI (mr comment --reply/--line A-B/--approve/--request-changes, mr edit-comment, mr delete-comment, show and watch), web (threads, quote, Shift+click ranges with a no-JS fallback, verdict list), docs and the ermite skill.

Depends on the ermite-api MR (feat/mr-notes). Until ermite-api v0.8.0 is tagged this branch builds only with a go.work that uses that branch; a last commit will require v0.8.0.

Upgrade: mr:review is a new scope, so a token that held every scope loses the owner's rights until it is recreated (--preset cli) with the root token or from an owner web session.

Checks (with the API branch in a go.work): build, go test -race, go test ./internal/app, golangci-lint --new-from-rev=main, hexcheck, templ drift, buf lint/breaking/generate, mocks drift, node tests (145). Reviewed by three read-only reviewers plus a targeted re-review; all findings fixed. Not verified yet: manual browser check of the merge request page.

Commit	Subject	Author	Date	Signature
86ac875	Merge remote-tracking branch 'origin/main' into feat/mr-notes	brice	3d ago	GPG Verified
77b3426	build(deps): ermite-api v0.8.0	brice	3d ago	GPG Verified
c064166	fix(web): split-only note links and disjoint heading ids of split note copies	brice	3d ago	GPG Verified
06682af	fix(mr): deleted verdict notes, split-layout note links and verdict labels	brice	3d ago	GPG Verified
3872b20	test(mr): stronger note retry and range tests	brice	3d ago	GPG Verified
31c9416	docs(mr): notes, threads, ranges and verdicts	brice	3d ago	GPG Verified
f8d5320	merge: web for merge request notes	brice	3d ago	GPG Verified
aec37bf	feat(web): threads, line ranges, Markdown notes, edits and verdicts on merge requests	brice	3d ago	GPG Verified
945925c	merge: RPC and CLI for merge request notes	brice	3d ago	GPG Verified
8def6de	feat(cli): replies, ranges, verdicts, edit and delete for merge request notes	brice	3d ago	GPG Verified
aa64b62	fix(mr): recheck the reply target when a note save is retried	brice	3d ago	GPG Verified
b31a354	feat(mr): replies, line ranges, edits, deletes and verdicts in the merge request use case	brice	3d ago	GPG Verified
3a784d4	feat(mr): note model with threads, ranges, verdicts and the mr:review scope	brice	3d ago	GPG Verified

Checks 8

CI · pull_request
Image success
Lint success
Protobuf and generated code success
Test success
CI · push
Image success
Lint success
Protobuf and generated code success
Test failure

62 files · +3086 -650

7/8

M docs/cli.md +2 -1


@@ -59,13 +59,13 @@ func (c *CI) OnPush(ctx context.Context, repo domain.RepoName, updates []domain.
59	59		c.markSchedulesDirty(repo, updates)
60	60		// The merge requests follow the push whoever made it (their refs); only a
61	61		// push that is not a job's starts pull_request runs.
62		-	defer c.onBranchesMoved(ctx, repo, updates, authorOf(pusher), domain.PusherOf(pusher), domain.TrustedActor(pusher), !pusher.ViaJobToken())
	62	+	defer c.onBranchesMoved(ctx, repo, updates, domain.AuthorName(pusher), domain.PusherOf(pusher), domain.TrustedActor(pusher), !pusher.ViaJobToken())
63	63		if pusher.ViaJobToken() {
64	64		log := zerowrap.FromCtx(ctx)
65	65		log.Debug().Str("repo", repo.String()).Int("refs", len(updates)).Msg("push by a job token starts no run")
66	66		return
67	67		}
68		-	c.enqueue(ctx, repo, updates, authorOf(pusher), domain.PusherOf(pusher), domain.TrustedActor(pusher))
	68	+	c.enqueue(ctx, repo, updates, domain.AuthorName(pusher), domain.PusherOf(pusher), domain.TrustedActor(pusher))
69	69		}
70	70
71	71		// OnMerge queues the move of a target branch by a merge. The actor of the run is

@@ -79,13 +79,13 @@ func (c *CI) OnMerge(ctx context.Context, repo domain.RepoName, updates []domain
79	79		// The merge moved a target branch: the test merges of the other requests
80	80		// into it are computed again (a merge starts no synchronize run), after the
81	81		// push workflows are queued.
82		-	defer c.onBranchesMoved(ctx, repo, updates, authorOf(domain.Owner()), domain.PusherOf(merger), domain.TrustedActor(merger), false)
	82	+	defer c.onBranchesMoved(ctx, repo, updates, domain.AuthorName(domain.Owner()), domain.PusherOf(merger), domain.TrustedActor(merger), false)
83	83		if merger.ViaJobToken() {
84	84		log := zerowrap.FromCtx(ctx)
85	85		log.Debug().Str("repo", repo.String()).Int("refs", len(updates)).Msg("merge by a job token starts no run")
86	86		return
87	87		}
88		-	c.enqueue(ctx, repo, updates, authorOf(domain.Owner()), domain.PusherOf(merger), domain.TrustedActor(merger))
	88	+	c.enqueue(ctx, repo, updates, domain.AuthorName(domain.Owner()), domain.PusherOf(merger), domain.TrustedActor(merger))
89	89		}
90	90
91	91		func (c *CI) enqueue(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, actor string, pusher domain.CIPusher, trusted bool) {


@@ -178,6 +178,12 @@ func (e *Events) snapshot(ctx context.Context, ev domain.Event) bufferedEvent {
178	178		return repoSnapshot(ev.Visibility, ev.CreatedAt)
179	179		case domain.CommentAdded:
180	180		return repoSnapshot(ev.Visibility, ev.CreatedAt)
	181	+	case domain.CommentEdited:
	182	+	return repoSnapshot(ev.Visibility, ev.CreatedAt)
	183	+	case domain.CommentDeleted:
	184	+	return repoSnapshot(ev.Visibility, ev.CreatedAt)
	185	+	case domain.CommentResolved:
	186	+	return repoSnapshot(ev.Visibility, ev.CreatedAt)
181	187		case domain.RunUpdated:
182	188		return repoSnapshot(ev.Visibility, ev.CreatedAt)
183	189		case domain.JobUpdated:

@@ -487,6 +493,12 @@ func (e Events) deliver(ctx context.Context, w watch, be bufferedEvent, send f
487	493		return e.deliverRepo(ctx, w, ev.MergeRequest.Repo, false, be, out, send)
488	494		case domain.CommentAdded:
489	495		return e.deliverRepo(ctx, w, ev.Repo, false, be, out, send)
	496	+	case domain.CommentEdited:
	497	+	return e.deliverRepo(ctx, w, ev.Repo, false, be, out, send)
	498	+	case domain.CommentDeleted:
	499	+	return e.deliverRepo(ctx, w, ev.Repo, false, be, out, send)
	500	+	case domain.CommentResolved:
	501	+	return e.deliverRepo(ctx, w, ev.Repo, false, be, out, send)
490	502		case domain.RunUpdated:
491	503		// Home shows the CI state of each repository.
492	504		return e.deliverRepo(ctx, w, ev.Run.Repo, false, be, out, send)


@@ -922,3 +922,35 @@ func TestWatchHomeGetsRunUpdates(t *testing.T) {
922	922		root.quiet(t, r, pub)
923	923		})
924	924		}
	925	+
	926	+	func TestNoteMutationEventVisibility(t *testing.T) {
	927	+	synctest.Test(t, func(t *testing.T) {
	928	+	r := newEventRig(t, 0)
	929	+	pub := r.repo("pub", domain.VisibilityPublic)
	930	+	priv := r.repo("priv", domain.VisibilityPrivate)
	931	+	ctx, cancel := context.WithCancel(context.Background())
	932	+	defer cancel()
	933	+	anon := r.watch(ctx, domain.Anonymous(), ports.WatchParams{})
	934	+	owner := r.watch(ctx, domain.Root(), ports.WatchParams{Topics: []string{"repo:priv"}})
	935	+	synctest.Wait()
	936	+	for _, kind := range []string{"edited", "deleted", "resolved"} {
	937	+	for _, n := range []domain.RepoName{priv, pub} {
	938	+	vis := domain.VisibilityPublic
	939	+	if n == priv {
	940	+	vis = domain.VisibilityPrivate
	941	+	}
	942	+	switch kind {
	943	+	case "edited":
	944	+	r.feed <- domain.CommentEdited{Repo: n, Visibility: vis, CreatedAt: now, At: now}
	945	+	case "deleted":
	946	+	r.feed <- domain.CommentDeleted{Repo: n, Visibility: vis, CreatedAt: now, At: now}
	947	+	case "resolved":
	948	+	r.feed <- domain.CommentResolved{Repo: n, Visibility: vis, CreatedAt: now, At: now}
	949	+	}
	950	+	}
	951	+	assert.Equal(t, "mr.comment_"+kind, anon.next(t).Event.EventKind())
	952	+	assert.Equal(t, "mr.comment_"+kind, owner.next(t).Event.EventKind())
	953	+	anon.quiet(t, r, pub)
	954	+	}
	955	+	})
	956	+	}


@@ -15,9 +15,6 @@ import (
15	15		"bnema.dev/ermite/internal/ports"
16	16		)
17	17
18		-	// AuthorOf exposes authorOf to the tests.
19		-	func AuthorOf(p domain.Principal) string { return authorOf(p) }
20		-
21	18		// Sweep exposes the daily sweep to the tests.
22	19		func (c *CI) Sweep(ctx context.Context) { c.sweep(ctx) }
23	20


@@ -241,7 +241,7 @@ func (m *MergeRequests) Create(ctx context.Context, p domain.Principal, name dom
241	241		now := m.clock.Now().UTC()
242	242		mr, err := m.store.Create(ctx, domain.MergeRequest{
243	243		Repo: name, Title: title, Description: desc, Source: source, Target: target,
244		-	State: domain.MRStateOpen, Author: authorOf(p), CreatedAt: now, UpdatedAt: now,
	244	+	State: domain.MRStateOpen, Author: domain.AuthorName(p), CreatedAt: now, UpdatedAt: now,
245	245		})
246	246		if err != nil {
247	247		return domain.MergeRequest{}, fmt.Errorf("create merge request of %q: %w", name, err)

@@ -468,6 +468,7 @@ func (m *MergeRequests) Get(ctx context.Context, p domain.Principal, name domain
468	468		if d.Comments, err = m.store.ListComments(ctx, name, number); err != nil {
469	469		return ports.MergeRequestDetail{}, fmt.Errorf("list comments of !%d in %q: %w", number, name, err)
470	470		}
	471	+	d.Verdicts = domain.CurrentVerdicts(d.Comments, mr.HeadID)
471	472		if d.LastReadID, err = m.store.GetRead(ctx, name, number); err != nil {
472	473		return ports.MergeRequestDetail{}, err
473	474		}

@@ -547,7 +548,7 @@ func (m *MergeRequests) rangeOf(ctx context.Context, mr domain.MergeRequest) (ex
547	548		}
548	549
549	550		// AddComment adds a comment and publishes CommentAdded. The principal needs
550		-	// mr:write. The text must follow the rules of ADR 0017 and not be blank; a
	551	+	// mr:write. The text must follow the rules of ADR 0032 and not be blank unless carrying a verdict; a
551	552		// comment on a line must name a line of the current diff of the request (see
552	553		// domain.Diff.HasLine), else the call fails with domain.ErrInvalid. A request
553	554		// that is not open gives domain.ErrPreconditionFailed, and one that already

@@ -566,21 +567,44 @@ func (m *MergeRequests) AddComment(ctx context.Context, p domain.Principal, name
566	567		switch {
567	568		case err != nil:
568	569		v.AddErr("text", err)
569		-	case text.IsBlank():
	570	+	case text.IsBlank() && params.Verdict == domain.VerdictNone:
570	571		v.Add("text", "is required")
571	572		}
572	573		general := params.Path == ""
573	574		switch {
574		-	case general && (params.Line != 0 \|\| params.Side != domain.SideNone):
	575	+	case general && (params.Line != 0 \|\| params.Side != domain.SideNone \|\| params.StartLine != 0):
575	576		v.Add("path", "is required with line and side")
576	577		case !general && params.Line < 1:
577	578		v.Add("line", "must be 1 or more with a path")
578	579		case !general && params.Side != domain.SideOld && params.Side != domain.SideNew:
579	580		v.Add("side", "is required with a path")
580	581		}
	582	+	if params.StartLine != 0 && (params.StartLine < 1 \|\| params.StartLine >= params.Line) {
	583	+	v.Add("start_line", "must be 1 or more and less than line")
	584	+	}
	585	+	if params.Verdict > domain.VerdictChangesRequested {
	586	+	v.Add("verdict", "is unknown")
	587	+	}
	588	+	if params.ReplyTo != 0 {
	589	+	if params.Path != "" \|\| params.Line != 0 \|\| params.StartLine != 0 \|\| params.Side != domain.SideNone \|\| params.Verdict != domain.VerdictNone {
	590	+	v.Add("reply_to", "a reply cannot specify an anchor or verdict")
	591	+	}
	592	+	}
581	593		if err := v.Err(); err != nil {
582	594		return domain.Comment{}, err
583	595		}
	596	+	if params.ReplyTo != 0 {
	597	+	root, err := m.replyRoot(ctx, name, number, params.ReplyTo)
	598	+	if err != nil {
	599	+	return domain.Comment{}, err
	600	+	}
	601	+	params.Path, params.Line, params.StartLine, params.Side = root.Path, root.Line, root.StartLine, root.Side
	602	+	}
	603	+	if params.Verdict != domain.VerdictNone {
	604	+	if err := scopeAccess(p, repo, domain.ScopeMRReview); err != nil {
	605	+	return domain.Comment{}, err
	606	+	}
	607	+	}
584	608		if !mr.IsOpen() {
585	609		return domain.Comment{}, fmt.Errorf("%w: !%d is %s", domain.ErrPreconditionFailed, number, mr.State)
586	610		}

@@ -588,7 +612,7 @@ func (m *MergeRequests) AddComment(ctx context.Context, p domain.Principal, name
588	612		if mr, err = m.withHead(ctx, mr); err != nil {
589	613		return domain.Comment{}, err
590	614		}
591		-	if !general {
	615	+	if !general && params.ReplyTo == 0 {
592	616		if err := m.checkLine(ctx, mr, params); err != nil {
593	617		return domain.Comment{}, err
594	618		}

@@ -600,7 +624,8 @@ func (m *MergeRequests) AddComment(ctx context.Context, p domain.Principal, name
600	624		return domain.Comment{}, fmt.Errorf("%w: !%d already has %d comments", domain.ErrResourceExhausted, number, MaxCommentsPerMergeRequest)
601	625		}
602	626		c = domain.Comment{
603		-	ID: mr.CommentSeq + 1, Author: authorOf(p), Path: params.Path, Line: params.Line, Side: params.Side,
	627	+	ID: mr.CommentSeq + 1, Author: domain.AuthorName(p), Path: params.Path, Line: params.Line, Side: params.Side,
	628	+	ReplyTo: params.ReplyTo, StartLine: params.StartLine, Verdict: params.Verdict, Resolved: params.Verdict == domain.VerdictApprove,
604	629		CommitID: mr.HeadID, Text: text, CreatedAt: m.clock.Now().UTC(),
605	630		}
606	631		mr.CommentSeq, mr.UpdatedAt = c.ID, c.CreatedAt

@@ -616,6 +641,12 @@ func (m *MergeRequests) AddComment(ctx context.Context, p domain.Principal, name
616	641		if !fresh.IsOpen() {
617	642		return domain.Comment{}, fmt.Errorf("%w: !%d is %s", domain.ErrPreconditionFailed, number, fresh.State)
618	643		}
	644	+	if params.ReplyTo != 0 {
	645	+	// The root may have been deleted by the write that got in between.
	646	+	if _, rerr := m.replyRoot(ctx, name, number, params.ReplyTo); rerr != nil {
	647	+	return domain.Comment{}, rerr
	648	+	}
	649	+	}
619	650		fresh.HeadID = mr.HeadID
620	651		mr = fresh
621	652		}

@@ -624,7 +655,7 @@ func (m *MergeRequests) AddComment(ctx context.Context, p domain.Principal, name
624	655		}
625	656		log := zerowrap.FromCtx(ctx)
626	657		log.Info().Str("repo", name.String()).Int64("number", number).Int64("comment", c.ID).
627		-	Bool("on_line", !general).Str("principal", p.Label()).Str("token_id", p.TokenID()).Msg("comment added")
	658	+	Int64("reply_to", c.ReplyTo).Str("verdict", c.Verdict.String()).Bool("on_line", !general).Str("principal", p.Label()).Str("token_id", p.TokenID()).Msg("comment added")
628	659		m.bus.Publish(ctx, domain.CommentAdded{
629	660		Repo: name, Number: number, Comment: c, At: c.CreatedAt, Visibility: repo.Visibility, CreatedAt: repo.CreatedAt,
630	661		})

@@ -667,6 +698,9 @@ func (m *MergeRequests) checkLine(ctx context.Context, mr domain.MergeRequest, p
667	698		if idx < 0 \|\| !diff.HasLine(params.Path, params.Side, params.Line) {
668	699		v.Add("path", fmt.Sprintf("%s:%d on the %s side is not a line of the diff", params.Path, params.Line, params.Side))
669	700		}
	701	+	if params.StartLine != 0 && !diff.HasLine(params.Path, params.Side, params.StartLine) {
	702	+	v.Add("start_line", "is not a line of the diff")
	703	+	}
670	704		return v.Err()
671	705		}
672	706

@@ -683,7 +717,8 @@ func diffFileName(f domain.FileDiff) string {
683	717		// not open domain.ErrPreconditionFailed. Setting the state the comment already
684	718		// has succeeds and writes nothing.
685	719		func (m *MergeRequests) ResolveComment(ctx context.Context, p domain.Principal, name domain.RepoName, number, commentID int64, resolved bool) (domain.Comment, error) {
686		-	if _, err := m.writable(ctx, p, name, domain.ScopeMRWrite); err != nil {
	720	+	repo, err := m.writable(ctx, p, name, domain.ScopeMRWrite)
	721	+	if err != nil {
687	722		return domain.Comment{}, err
688	723		}
689	724		for range maxWriteRetries {

@@ -698,6 +733,11 @@ func (m *MergeRequests) ResolveComment(ctx context.Context, p domain.Principal,
698	733		if !mr.IsOpen() {
699	734		return domain.Comment{}, fmt.Errorf("%w: !%d is %s", domain.ErrPreconditionFailed, number, mr.State)
700	735		}
	736	+	if c.IsReply() {
	737	+	var v domain.Violations
	738	+	v.Add("comment_id", fmt.Sprintf("resolve the thread on its root ^%d", c.ReplyTo))
	739	+	return domain.Comment{}, v.Err()
	740	+	}
701	741		if c.Resolved == resolved {
702	742		return c, nil
703	743		}

@@ -710,6 +750,7 @@ func (m *MergeRequests) ResolveComment(ctx context.Context, p domain.Principal,
710	750		if err != nil {
711	751		return domain.Comment{}, fmt.Errorf("resolve comment %d of !%d in %q: %w", commentID, number, name, err)
712	752		}
	753	+	m.bus.Publish(ctx, domain.CommentResolved{Repo: name, Number: number, Comment: c, At: mr.UpdatedAt, Visibility: repo.Visibility, CreatedAt: repo.CreatedAt})
713	754		return c, nil
714	755		}
715	756		return domain.Comment{}, fmt.Errorf("resolve comment %d of !%d in %q: %w", commentID, number, name, domain.ErrConflict)

@@ -1090,7 +1131,7 @@ func (m *MergeRequests) moveTarget(ctx context.Context, mr domain.MergeRequest,
1090	1131		// does, under the merge lock (domain.CIRefTip). The run shows the owner, as a
1091	1132		// merge's push does on GitHub.
1092	1133		if err := recordRefTips(ctx, m.tips, mr.Repo, []domain.RefUpdate{{Old: tip.String(), New: newTip.String(), Ref: "refs/heads/" + mr.Target}},
1093		-	authorOf(domain.Owner()), domain.PusherOf(p), domain.TrustedActor(p)); err != nil {
	1134	+	domain.AuthorName(domain.Owner()), domain.PusherOf(p), domain.TrustedActor(p)); err != nil {
1094	1135		return domain.ObjectID{}, err
1095	1136		}
1096	1137		err := m.git.UpdateBranch(ctx, mr.Repo, mr.Target, tip, newTip)

@@ -1129,3 +1170,90 @@ func (m *MergeRequests) markMerged(ctx context.Context, name domain.RepoName, nu
1129	1170		Msg("the target branch moved but the merge request could not be marked merged")
1130	1171		return domain.MergeRequest{}, fmt.Errorf("record the merge of !%d in %q: %w", number, name, domain.ErrConflict)
1131	1172		}
	1173	+
	1174	+	// EditComment changes only the author's text, preserving the immutable verdict.
	1175	+	func (m *MergeRequests) EditComment(ctx context.Context, p domain.Principal, name domain.RepoName, number, commentID int64, text string) (domain.Comment, error) {
	1176	+	return m.changeComment(ctx, p, name, number, commentID, text, false)
	1177	+	}
	1178	+
	1179	+	// DeleteComment removes text but retains the anchor, ID and verdict. Repeated deletion is a no-op.
	1180	+	func (m *MergeRequests) DeleteComment(ctx context.Context, p domain.Principal, name domain.RepoName, number, commentID int64) (domain.Comment, error) {
	1181	+	return m.changeComment(ctx, p, name, number, commentID, "", true)
	1182	+	}
	1183	+
	1184	+	func (m *MergeRequests) changeComment(ctx context.Context, p domain.Principal, name domain.RepoName, number, commentID int64, text string, remove bool) (domain.Comment, error) {
	1185	+	repo, err := m.writable(ctx, p, name, domain.ScopeMRWrite)
	1186	+	if err != nil {
	1187	+	return domain.Comment{}, err
	1188	+	}
	1189	+	for range maxWriteRetries {
	1190	+	mr, err := m.store.Get(ctx, name, number)
	1191	+	if err != nil {
	1192	+	return domain.Comment{}, err
	1193	+	}
	1194	+	c, err := m.store.GetComment(ctx, name, number, commentID)
	1195	+	if err != nil {
	1196	+	return domain.Comment{}, err
	1197	+	}
	1198	+	if !mr.IsOpen() {
	1199	+	return domain.Comment{}, fmt.Errorf("%w: !%d is %s", domain.ErrPreconditionFailed, number, mr.State)
	1200	+	}
	1201	+	if domain.AuthorName(p) != c.Author {
	1202	+	return domain.Comment{}, fmt.Errorf("%w: only the author of a note can edit or delete it", domain.ErrForbidden)
	1203	+	}
	1204	+	if c.Deleted {
	1205	+	if remove {
	1206	+	return c, nil
	1207	+	}
	1208	+	return domain.Comment{}, fmt.Errorf("%w: the note was deleted", domain.ErrPreconditionFailed)
	1209	+	}
	1210	+	parsed, err := domain.ParsePlainText(text)
	1211	+	var v domain.Violations
	1212	+	if err != nil {
	1213	+	v.AddErr("text", err)
	1214	+	} else if !remove && parsed.IsBlank() && c.Verdict == domain.VerdictNone {
	1215	+	v.Add("text", "is required")
	1216	+	}
	1217	+	if err := v.Err(); err != nil {
	1218	+	return domain.Comment{}, err
	1219	+	}
	1220	+	if !remove && parsed == c.Text {
	1221	+	return c, nil
	1222	+	}
	1223	+	c.Text, c.Deleted, c.EditedAt = parsed, remove, m.clock.Now().UTC()
	1224	+	mr.UpdatedAt = c.EditedAt
	1225	+	if err := m.store.SaveComment(ctx, mr, c); errors.Is(err, domain.ErrConflict) {
	1226	+	continue
	1227	+	} else if err != nil {
	1228	+	return domain.Comment{}, fmt.Errorf("save note %d of !%d in %q: %w", commentID, number, name, err)
	1229	+	}
	1230	+	log := zerowrap.FromCtx(ctx)
	1231	+	log.Info().Str("repo", name.String()).Int64("number", number).Int64("comment", c.ID).Bool("deleted", remove).Str("principal", p.Label()).Str("token_id", p.TokenID()).Msg("comment changed")
	1232	+	if remove {
	1233	+	m.bus.Publish(ctx, domain.CommentDeleted{Repo: name, Number: number, Comment: c, At: c.EditedAt, Visibility: repo.Visibility, CreatedAt: repo.CreatedAt})
	1234	+	} else {
	1235	+	m.bus.Publish(ctx, domain.CommentEdited{Repo: name, Number: number, Comment: c, At: c.EditedAt, Visibility: repo.Visibility, CreatedAt: repo.CreatedAt})
	1236	+	}
	1237	+	return c, nil
	1238	+	}
	1239	+	return domain.Comment{}, fmt.Errorf("save note %d of !%d in %q: %w", commentID, number, name, domain.ErrConflict)
	1240	+	}
	1241	+
	1242	+	// replyRoot returns the note a reply targets, or a violation when it is
	1243	+	// missing, itself a reply, or deleted.
	1244	+	func (m *MergeRequests) replyRoot(ctx context.Context, name domain.RepoName, number, id int64) (domain.Comment, error) {
	1245	+	root, err := m.store.GetComment(ctx, name, number, id)
	1246	+	var v domain.Violations
	1247	+	switch {
	1248	+	case errors.Is(err, domain.ErrNotFound):
	1249	+	v.Add("reply_to", fmt.Sprintf("no note ^%d", id))
	1250	+	case err != nil:
	1251	+	return domain.Comment{}, err
	1252	+	case root.IsReply() \|\| root.Deleted:
	1253	+	v.Add("reply_to", "must name a non-deleted root note")
	1254	+	}
	1255	+	if err := v.Err(); err != nil {
	1256	+	return domain.Comment{}, err
	1257	+	}
	1258	+	return root, nil
	1259	+	}


@@ -689,7 +689,10 @@ func TestResolveComment(t *testing.T) {
689	689		_, err = r.mrs.ResolveComment(r.ctx, mrW, r.repo, mr.Number, 0, true)
690	690		assert.ErrorIs(t, err, domain.ErrNotFound)
691	691
692		-	assert.Empty(t, r.drain(), "a resolved comment has no event")
	692	+	events := r.drain()
	693	+	require.Len(t, events, 2, "only state changes publish resolution events")
	694	+	assert.True(t, events[0].(domain.CommentResolved).Comment.Resolved)
	695	+	assert.False(t, events[1].(domain.CommentResolved).Comment.Resolved)
693	696		}
694	697
695	698		func TestEventsOfAMergeRequest(t *testing.T) {

@@ -1342,3 +1345,106 @@ func TestCommentsPerMergeRequestAreBounded(t *testing.T) {
1342	1345		_, err = r.mrs.AddComment(r.ctx, mrW, r.repo, other.Number, ports.AddCommentParams{Text: "fine"})
1343	1346		require.NoError(t, err)
1344	1347		}
	1348	+
	1349	+	func TestNoteThreadsRangesAndVerdicts(t *testing.T) {
	1350	+	t.Run("start outside diff", func(t *testing.T) {
	1351	+	r := newMRRig(t)
	1352	+	r.seed()
	1353	+	before := strings.Repeat("unchanged\n", 30)
	1354	+	r.commit(author, "long.txt", before, "long base")
	1355	+	r.push("main")
	1356	+	r.sh("checkout", "-q", "-B", "range", "main")
	1357	+	r.commit(author, "long.txt", before+"changed\n", "late hunk")
	1358	+	r.push("range")
	1359	+	mr := r.open("range")
	1360	+	_, err := r.mrs.AddComment(r.ctx, root, r.repo, mr.Number, ports.AddCommentParams{Path: "long.txt", Line: 31, Side: domain.SideNew, Text: "valid"})
	1361	+	require.NoError(t, err)
	1362	+	_, err = r.mrs.AddComment(r.ctx, root, r.repo, mr.Number, ports.AddCommentParams{Path: "long.txt", StartLine: 1, Line: 31, Side: domain.SideNew, Text: "invalid range"})
	1363	+	require.ErrorIs(t, err, domain.ErrInvalid)
	1364	+	assert.Contains(t, fieldsOf(t, err), "start_line")
	1365	+	})
	1366	+	r := newMRRig(t)
	1367	+	r.seed()
	1368	+	head := r.feature("feat")
	1369	+	mr := r.open("feat")
	1370	+	add := func(p domain.Principal, params ports.AddCommentParams) (domain.Comment, error) {
	1371	+	return r.mrs.AddComment(r.ctx, p, r.repo, mr.Number, params)
	1372	+	}
	1373	+	rootNote, err := add(root, ports.AddCommentParams{Path: "a.txt", StartLine: 1, Line: 3, Side: domain.SideNew, Text: "range"})
	1374	+	require.NoError(t, err)
	1375	+	reply, err := add(mrW, ports.AddCommentParams{ReplyTo: rootNote.ID, Text: "reply"})
	1376	+	require.NoError(t, err)
	1377	+	assert.Equal(t, rootNote.Path, reply.Path)
	1378	+	assert.Equal(t, rootNote.StartLine, reply.StartLine)
	1379	+	assert.Equal(t, rootNote.Line, reply.Line)
	1380	+	assert.Equal(t, rootNote.Side, reply.Side)
	1381	+	assert.Equal(t, head, reply.CommitID)
	1382	+	assert.False(t, reply.Resolved)
	1383	+	for _, params := range []ports.AddCommentParams{
	1384	+	{ReplyTo: reply.ID, Text: "x"}, {ReplyTo: 999, Text: "x"}, {ReplyTo: rootNote.ID, Path: "a.txt", Text: "x"}, {ReplyTo: rootNote.ID, Line: 1, Text: "x"}, {ReplyTo: rootNote.ID, Side: domain.SideNew, Text: "x"}, {ReplyTo: rootNote.ID, StartLine: 1, Text: "x"}, {ReplyTo: rootNote.ID, Verdict: domain.VerdictApprove},
	1385	+	{StartLine: 1, Text: "x"}, {Path: "a.txt", StartLine: 3, Line: 3, Side: domain.SideNew, Text: "x"}, {Path: "a.txt", StartLine: -1, Line: 3, Side: domain.SideNew, Text: "x"}, {Path: "a.txt", StartLine: 1, Line: 99, Side: domain.SideNew, Text: "x"},
	1386	+	} {
	1387	+	_, err := add(root, params)
	1388	+	assert.ErrorIs(t, err, domain.ErrInvalid, "%+v", params)
	1389	+	}
	1390	+	_, err = r.mrs.ResolveComment(r.ctx, root, r.repo, mr.Number, reply.ID, true)
	1391	+	assert.ErrorIs(t, err, domain.ErrInvalid)
	1392	+	assert.ErrorContains(t, err, fmt.Sprintf("root ^%d", rootNote.ID))
	1393	+	_, err = add(mrW, ports.AddCommentParams{Verdict: domain.VerdictApprove})
	1394	+	assert.ErrorIs(t, err, domain.ErrForbidden)
	1395	+	assert.ErrorContains(t, err, "this needs the mr:review scope")
	1396	+	reviewer := domain.TokenPrincipal(domain.Token{Label: "reviewer", Scopes: []domain.Scope{domain.ScopeMRWrite, domain.ScopeMRReview}})
	1397	+	approve, err := add(reviewer, ports.AddCommentParams{Verdict: domain.VerdictApprove})
	1398	+	require.NoError(t, err)
	1399	+	assert.True(t, approve.Resolved)
	1400	+	changes, err := add(root, ports.AddCommentParams{Verdict: domain.VerdictChangesRequested})
	1401	+	require.NoError(t, err)
	1402	+	assert.False(t, changes.Resolved)
	1403	+	_, err = r.mrs.EditComment(r.ctx, root, r.repo, mr.Number, approve.ID, "no")
	1404	+	assert.ErrorIs(t, err, domain.ErrForbidden)
	1405	+	_, err = r.mrs.DeleteComment(r.ctx, root, r.repo, mr.Number, approve.ID)
	1406	+	assert.ErrorIs(t, err, domain.ErrForbidden)
	1407	+	r.drain()
	1408	+	edited, err := r.mrs.EditComment(r.ctx, reviewer, r.repo, mr.Number, approve.ID, "updated")
	1409	+	require.NoError(t, err)
	1410	+	assert.False(t, edited.EditedAt.IsZero())
	1411	+	assert.Equal(t, approve.Verdict, edited.Verdict)
	1412	+	_, err = r.mrs.EditComment(r.ctx, reviewer, r.repo, mr.Number, approve.ID, "updated")
	1413	+	require.NoError(t, err)
	1414	+	events := r.drain()
	1415	+	require.Len(t, events, 1)
	1416	+	assert.IsType(t, domain.CommentEdited{}, events[0])
	1417	+	deleted, err := r.mrs.DeleteComment(r.ctx, reviewer, r.repo, mr.Number, approve.ID)
	1418	+	require.NoError(t, err)
	1419	+	assert.True(t, deleted.Deleted)
	1420	+	assert.Equal(t, "", deleted.Text.String())
	1421	+	assert.Equal(t, approve.Verdict, deleted.Verdict)
	1422	+	again, err := r.mrs.DeleteComment(r.ctx, reviewer, r.repo, mr.Number, approve.ID)
	1423	+	require.NoError(t, err)
	1424	+	deleted.Version = again.Version // SaveComment assigns the new version on the next read.
	1425	+	assert.Equal(t, deleted, again)
	1426	+	events = r.drain()
	1427	+	require.Len(t, events, 1)
	1428	+	assert.IsType(t, domain.CommentDeleted{}, events[0])
	1429	+	_, err = r.mrs.EditComment(r.ctx, reviewer, r.repo, mr.Number, approve.ID, "x")
	1430	+	assert.ErrorIs(t, err, domain.ErrPreconditionFailed)
	1431	+	_, err = r.mrs.EditComment(r.ctx, root, r.repo, mr.Number, rootNote.ID, "")
	1432	+	assert.ErrorIs(t, err, domain.ErrInvalid)
	1433	+	_, err = r.mrs.DeleteComment(r.ctx, root, r.repo, mr.Number, rootNote.ID)
	1434	+	require.NoError(t, err)
	1435	+	_, err = add(root, ports.AddCommentParams{ReplyTo: rootNote.ID, Text: "x"})
	1436	+	assert.ErrorIs(t, err, domain.ErrInvalid)
	1437	+	r.commit(author, "a.txt", "changed\n2\nthree\n", "advance")
	1438	+	r.push("feat")
	1439	+	detail, err := r.mrs.Get(r.ctx, root, r.repo, mr.Number, ports.GetMergeRequestOptions{})
	1440	+	require.NoError(t, err)
	1441	+	require.Len(t, detail.Verdicts, 2)
	1442	+	assert.True(t, detail.Verdicts[0].Stale)
	1443	+	assert.False(t, detail.Verdicts[1].Stale)
	1444	+	_, err = r.mrs.Close(r.ctx, root, r.repo, mr.Number)
	1445	+	require.NoError(t, err)
	1446	+	_, err = r.mrs.EditComment(r.ctx, root, r.repo, mr.Number, changes.ID, "x")
	1447	+	assert.ErrorIs(t, err, domain.ErrPreconditionFailed)
	1448	+	_, err = r.mrs.DeleteComment(r.ctx, root, r.repo, mr.Number, changes.ID)
	1449	+	assert.ErrorIs(t, err, domain.ErrPreconditionFailed)
	1450	+	}


@@ -6,6 +6,7 @@ import (
6	6		"context"
7	7		"errors"
8	8		"testing"
	9	+	"time"
9	10
10	11		"github.com/stretchr/testify/assert"
11	12		"github.com/stretchr/testify/mock"

@@ -117,3 +118,118 @@ func TestMergeReturnsTheFailureToReadTheSettings(t *testing.T) {
117	118		})
118	119		}
119	120		}
	121	+
	122	+	func TestNoteMutationRetries(t *testing.T) {
	123	+	for _, action := range []string{"edit", "delete", "resolve"} {
	124	+	t.Run(action, func(t *testing.T) {
	125	+	u := newMRUnit(t)
	126	+	u.meta.EXPECT().Get(mock.Anything, u.repo.Name).Return(u.repo, nil)
	127	+	mr := domain.MergeRequest{Repo: u.repo.Name, Number: 1, State: domain.MRStateOpen}
	128	+	text, err := domain.ParsePlainText("before")
	129	+	require.NoError(t, err)
	130	+	c := domain.Comment{ID: 1, Author: "root", Text: text}
	131	+	freshMR := mr
	132	+	freshMR.Version = 2
	133	+	fresh := c
	134	+	fresh.Version = 3
	135	+	fresh.CreatedAt = now.Add(-time.Hour)
	136	+	if action != "resolve" {
	137	+	fresh.Resolved = true
	138	+	}
	139	+	want := fresh
	140	+	switch action {
	141	+	case "edit":
	142	+	want.Text, err = domain.ParsePlainText("after")
	143	+	require.NoError(t, err)
	144	+	want.EditedAt = now.UTC()
	145	+	case "delete":
	146	+	want.Text, err = domain.ParsePlainText("")
	147	+	require.NoError(t, err)
	148	+	want.Deleted = true
	149	+	want.EditedAt = now.UTC()
	150	+	case "resolve":
	151	+	want.Resolved = true
	152	+	}
	153	+	u.store.EXPECT().Get(mock.Anything, u.repo.Name, int64(1)).Return(mr, nil).Once()
	154	+	u.store.EXPECT().Get(mock.Anything, u.repo.Name, int64(1)).Return(freshMR, nil).Once()
	155	+	u.store.EXPECT().GetComment(mock.Anything, u.repo.Name, int64(1), int64(1)).Return(c, nil).Once()
	156	+	u.store.EXPECT().GetComment(mock.Anything, u.repo.Name, int64(1), int64(1)).Return(fresh, nil).Once()
	157	+	u.store.EXPECT().SaveComment(mock.Anything, mock.Anything, mock.Anything).Return(domain.ErrConflict).Once()
	158	+	u.store.EXPECT().SaveComment(mock.Anything, mock.MatchedBy(func(saved domain.MergeRequest) bool {
	159	+	return saved.Version == freshMR.Version
	160	+	}), mock.MatchedBy(func(saved domain.Comment) bool { return saved == want })).Return(nil).Once()
	161	+	bus := mocks.NewEventBus(t)
	162	+	bus.EXPECT().Publish(mock.Anything, mock.MatchedBy(func(e domain.Event) bool {
	163	+	switch event := e.(type) {
	164	+	case domain.CommentEdited:
	165	+	return action == "edit" && event.Comment == want
	166	+	case domain.CommentDeleted:
	167	+	return action == "delete" && event.Comment == want
	168	+	case domain.CommentResolved:
	169	+	return action == "resolve" && event.Comment == want
	170	+	}
	171	+	return false
	172	+	})).Return().Once()
	173	+	clock := mocks.NewClock(t)
	174	+	clock.EXPECT().Now().Return(now)
	175	+	svc := usecase.NewMergeRequests(usecase.MergeRequestsConfig{Meta: u.meta, Store: u.store, Bus: bus, Clock: clock})
	176	+	var got domain.Comment
	177	+	switch action {
	178	+	case "edit":
	179	+	got, err = svc.EditComment(context.Background(), domain.Root(), u.repo.Name, 1, 1, "after")
	180	+	case "delete":
	181	+	got, err = svc.DeleteComment(context.Background(), domain.Root(), u.repo.Name, 1, 1)
	182	+	case "resolve":
	183	+	got, err = svc.ResolveComment(context.Background(), domain.Root(), u.repo.Name, 1, 1, true)
	184	+	}
	185	+	require.NoError(t, err)
	186	+	assert.Equal(t, want, got)
	187	+	})
	188	+	}
	189	+	}
	190	+
	191	+	// A reply whose root is deleted while the save is retried is rejected.
	192	+	func TestAddNoteRechecksReplyRootOnConflict(t *testing.T) {
	193	+	u := newMRUnit(t)
	194	+	u.meta.EXPECT().Get(mock.Anything, u.repo.Name).Return(u.repo, nil)
	195	+	u.git.EXPECT().BranchHead(mock.Anything, mock.Anything, mock.Anything).Return(domain.ObjectID{}, domain.ErrNotFound)
	196	+	u.store.EXPECT().Get(mock.Anything, u.repo.Name, int64(1)).Return(domain.MergeRequest{State: domain.MRStateOpen}, nil).Twice()
	197	+	u.store.EXPECT().GetComment(mock.Anything, u.repo.Name, int64(1), int64(1)).Return(domain.Comment{ID: 1}, nil).Once()
	198	+	u.store.EXPECT().GetComment(mock.Anything, u.repo.Name, int64(1), int64(1)).Return(domain.Comment{ID: 1, Deleted: true}, nil).Once()
	199	+	u.store.EXPECT().SaveComment(mock.Anything, mock.Anything, mock.Anything).Return(domain.ErrConflict).Once()
	200	+	_, err := u.mrs.AddComment(context.Background(), domain.Root(), u.repo.Name, 1, ports.AddCommentParams{ReplyTo: 1, Text: "x"})
	201	+	require.ErrorIs(t, err, domain.ErrInvalid)
	202	+	}
	203	+
	204	+	func TestAddNoteValidation(t *testing.T) {
	205	+	for _, tc := range []struct {
	206	+	name string
	207	+	params ports.AddCommentParams
	208	+	target domain.Comment
	209	+	missing bool
	210	+	}{
	211	+	{name: "range without path", params: ports.AddCommentParams{StartLine: 1, Text: "x"}},
	212	+	{name: "equal ends", params: ports.AddCommentParams{Path: "a", StartLine: 3, Line: 3, Side: domain.SideNew, Text: "x"}},
	213	+	{name: "reversed ends", params: ports.AddCommentParams{Path: "a", StartLine: 4, Line: 3, Side: domain.SideNew, Text: "x"}},
	214	+	{name: "reply anchor", params: ports.AddCommentParams{ReplyTo: 1, Path: "a", Line: 1, Side: domain.SideNew, Text: "x"}},
	215	+	{name: "reply verdict", params: ports.AddCommentParams{ReplyTo: 1, Verdict: domain.VerdictApprove}},
	216	+	{name: "reply to reply", params: ports.AddCommentParams{ReplyTo: 1, Text: "x"}, target: domain.Comment{ReplyTo: 2}},
	217	+	{name: "deleted root", params: ports.AddCommentParams{ReplyTo: 1, Text: "x"}, target: domain.Comment{Deleted: true}},
	218	+	{name: "missing root", params: ports.AddCommentParams{ReplyTo: 1, Text: "x"}, missing: true},
	219	+	} {
	220	+	t.Run(tc.name, func(t *testing.T) {
	221	+	u := newMRUnit(t)
	222	+	u.meta.EXPECT().Get(mock.Anything, u.repo.Name).Return(u.repo, nil)
	223	+	u.store.EXPECT().Get(mock.Anything, u.repo.Name, int64(1)).Return(domain.MergeRequest{State: domain.MRStateOpen}, nil)
	224	+	if tc.name == "reply to reply" \|\| tc.name == "deleted root" \|\| tc.missing {
	225	+	var err error
	226	+	if tc.missing {
	227	+	err = domain.ErrNotFound
	228	+	}
	229	+	u.store.EXPECT().GetComment(mock.Anything, u.repo.Name, int64(1), int64(1)).Return(tc.target, err)
	230	+	}
	231	+	_, err := u.mrs.AddComment(context.Background(), domain.Root(), u.repo.Name, 1, tc.params)
	232	+	require.ErrorIs(t, err, domain.ErrInvalid)
	233	+	})
	234	+	}
	235	+	}


@@ -179,7 +179,7 @@ func (u *Pushes) Check(ctx context.Context, p domain.Principal, name domain.Repo
179	179		// recorded before Git moves them, so that a tip never has the record of an
180	180		// earlier pusher at the same commit (domain.CIRefTip). A record that cannot
181	181		// be written refuses the push, as it cannot be run later with a known trust.
182		-	if err := recordRefTips(ctx, u.tips, name, updates, authorOf(p), domain.PusherOf(p), domain.TrustedActor(p)); err != nil {
	182	+	if err := recordRefTips(ctx, u.tips, name, updates, domain.AuthorName(p), domain.PusherOf(p), domain.TrustedActor(p)); err != nil {
183	183		return err
184	184		}
185	185		log := zerowrap.FromCtx(ctx)


@@ -334,12 +334,3 @@ func TestSignOutEverywhereEndsSessions(t *testing.T) {
334	334		_, err = f.sessions.Verify(ctx, c)
335	335		assert.NoError(t, err, "a new session works")
336	336		}
337		-
338		-	// The owner signed in on the web signs what it writes as "owner", where root
339		-	// signs as "root" although both hold every right.
340		-	func TestAuthorOfOwnerSession(t *testing.T) {
341		-	t.Parallel()
342		-	assert.Equal(t, "owner", usecase.AuthorOf(domain.Owner()))
343		-	assert.Equal(t, "root", usecase.AuthorOf(domain.Root()))
344		-	assert.Equal(t, "ci", usecase.AuthorOf(domain.TokenPrincipal(domain.Token{Label: "ci", Scopes: domain.AllScopes()})))
345		-	}


@@ -64,6 +64,13 @@ message MergeRequest {
64	64		int64 comment_seq = 14;
65	65		}
66	66
	67	+	// Verdict is the immutable review decision of a note.
	68	+	enum Verdict {
	69	+	VERDICT_UNSPECIFIED = 0;
	70	+	VERDICT_APPROVE = 1;
	71	+	VERDICT_CHANGES_REQUESTED = 2;
	72	+	}
	73	+
67	74		// Comment is stored under the key "mrc/<repo>/<number:010d>/<id:010d>".
68	75		message Comment {
69	76		int64 id = 1;

@@ -75,6 +82,11 @@ message Comment {
75	82		string text = 7;
76	83		bool resolved = 8;
77	84		google.protobuf.Timestamp created_at = 9;
	85	+	int64 reply_to = 10;
	86	+	int32 start_line = 11;
	87	+	Verdict verdict = 12;
	88	+	google.protobuf.Timestamp edited_at = 13;
	89	+	bool deleted = 14;
78	90		}
79	91
80	92		// Sequence is stored under the key "mrseq/<repo>": the last merge request


@@ -28,7 +28,7 @@ Presets add up with `--scope` (repeatable or comma-separated):
28	28
29	29		\| Preset \| Scopes \|
30	30		\|---\|---\|
31		-	\| `cli` \| every scope, for the owner's daily use \|
	31	+	\| `cli` \| every scope (including `mr:review`), for the owner's daily use; a token created before `mr:review` existed lacks it and loses owner rights: recreate it with `--preset cli` using the root token or from a signed-in owner web session (an old token lacking `mr:review` cannot grant it), then replace the stored CLI secret and revoke the old token \|
32	32		\| `agent` \| `repo:write`, `mr:write` \|
33	33		\| `ci` \| `repo:read` \|
34	34

@@ -39,7 +39,8 @@ Scopes:
39	39		\| `repo:read` \| read private repositories \|
40	40		\| `repo:write` \| push, create a repository \|
41	41		\| `repo:admin` \| delete a repository, change visibility, description, default branch, merge settings \|
42		-	\| `mr:write` \| open, comment on, resolve, close, mark read \|
	42	+	\| `mr:write` \| open, comment on, reply, edit and delete own notes, resolve, close, mark read \|
	43	+	\| `mr:review` \| give a verdict (`--approve`, `--request-changes`) \|
43	44		\| `mr:merge` \| merge, and delete the merged source branch with `--delete-branch` \|
44	45		\| `token:admin` \| create, list and revoke tokens \|
45	46		\| `ci:write`, `secrets:write`, `runner:register` \| reserved for CI \|

@@ -55,8 +56,10 @@ A token can never grant more than the token that creates it. `--repo` (repeatabl
55	56		\| `mr create <repo> --source B --title T [--target B] [--description TEXT\\|-]` \| `mr:write` \| target defaults to the default branch; title of 120 characters max; `-` reads standard input \|
56	57		\| `mr list <repo> [--state open\\|merged\\|closed\\|all] [--json]` \| none \| default `open` \|
57	58		\| `mr show <repo> <n> [--diff] [--since-last-read] [--mark-read] [--json]` \| `mr:write` for `--mark-read` \| commits, changed files and comments; `--diff` adds hunks with line numbers \|
58		-	\| `mr comment <repo> <n> -m TEXT\\|- [--path P --line N --side old\\|new]` \| `mr:write` \| the line must be in the current diff; a deleted file uses its old path \|
59		-	\| `mr resolve <repo> <n> <comment-id> [--undo]` \| `mr:write` \| doing it twice is not an error \|
	59	+	\| `mr comment <repo> <n> -m TEXT\\|- [--path P --line N\\|A-B --side old\\|new] [--reply ID] [--approve\\|--request-changes]` \| `mr:write`; verdicts `mr:review` \| Markdown note (8000 characters max); the lines must be in the current diff on one side; a deleted file uses its old path; `--reply` takes a root note (`12`, `#12` or `^12`), inherits its anchor and cannot carry a verdict; a verdict note may have no message \|
	60	+	\| `mr edit-comment <repo> <n> <comment-id> -m TEXT\\|-` \| `mr:write` \| author only, open merge request; the verdict is unchanged \|
	61	+	\| `mr delete-comment <repo> <n> <comment-id>` \| `mr:write` \| author only; the text is lost, the verdict is kept \|
	62	+	\| `mr resolve <repo> <n> <comment-id> [--undo]` \| `mr:write` \| on the thread root, covers its replies; doing it twice is not an error \|
60	63		\| `mr merge <repo> <n> --head <full-id> [--squash] [--delete-branch]` \| `mr:merge` \| refused if the branch moved or the target is not an ancestor of the head; `--delete-branch` deletes the source after the merge (refused for the default branch or a branch another open request uses; JSON adds `source_branch_deleted`) \|
61	64		\| `mr close <repo> <n>` \| `mr:write` \| a closed merge request cannot be reopened \|
62	65

@@ -64,7 +67,9 @@ A token can never grant more than the token that creates it. `--repo` (repeatabl
64	67		merge request web URL on its own line. With `--json`, merge request objects
65	68		(create, show, list, close and merge) include `url` alongside the other fields.
66	69


@@ -35,11 +35,20 @@ git push https://git.example.com/<repo>.git main # password = token,
35	35
36	36		1. List: `ermite mr list <repo> --json`.
37	37		2. Read: `ermite mr show <repo> <n> --diff --json`. Keep `head_id`; take line numbers from the hunks.
38		-	3. Comment on a line: `ermite mr comment <repo> <n> -m "text" --path <file> --line <n> --side new`. Without `--path` the comment is general. The line must appear in the diff.
39		-	4. Merge only when asked: `ermite mr merge <repo> <n> --head <head_id>`, with `--squash` for one commit signed by the server and `--delete-branch` to delete the source branch afterwards.
	38	+	3. Comment on a line: `ermite mr comment <repo> <n> -m "text" --path <file> --line <n> --side new`; `--line 10-14` covers a range on one side. Without `--path` the comment is general. The line must appear in the diff. Notes are Markdown; `^N` refers to note N.
	39	+	4. End a review with a verdict (needs `mr:review`): `--approve` or `--request-changes`, alone or with a message. Verdicts are informative and cannot be edited; a new verdict replaces the previous one.
	40	+	5. Merge only when asked: `ermite mr merge <repo> <n> --head <head_id>`, with `--squash` for one commit signed by the server and `--delete-branch` to delete the source branch afterwards.
40	41
41	42		The merge is refused when the branch moved since `head_id` (review again) or when the target is not an ancestor of the head (rebase and push again). The repository may allow only one merge mode.
42	43
	44	+	### Act on a review
	45	+
	46	+	Notes are visible as soon as they are written, so a review is not finished until its verdict. Act on a verdict, not on each note (`ermite watch --json` shows `verdict` on the `comment_added` event).
	47	+
	48	+	On `changes_requested`: run `ermite mr show <repo> <n> --json`, read the open threads (roots with `resolved` false and their replies), fix the code, push, then answer each thread with `ermite mr comment <repo> <n> --reply <id> -m "Fixed in ^N"`. Leave resolving to the reviewer: do not run `mr resolve` on their threads. Edit or delete only your own notes (`mr edit-comment`, `mr delete-comment`).
	49	+
	50	+	An approval is stale after a push; changes requested stay until the reviewer gives a new verdict.
	51	+
43	52		`ermite mr create` and `ermite mr close` print a confirmation followed by the full
44	53		merge request web URL on its own line. With `--json`, merge request objects
45	54		(create, show, list, close and merge) include `url` alongside the other fields.

@@ -52,11 +61,11 @@ Open one: `ermite mr create <repo> --source <branch> --title "..."`. After new p
52	61		ermite token create my-agent --preset agent --repo <repo> --expires 30d
53	62		```
54	63
55		-	The secret is printed once. Presets: `agent` (push, open and comment on merge requests), `ci` (read), `cli` (everything). Add `--scope mr:merge` to let an agent merge. A token can never grant more than the token that creates it. Full scope list: [CLI.md](CLI.md).
	64	+	The secret is printed once. Presets: `agent` (push, open and comment on merge requests; no verdicts), `ci` (read), `cli` (everything). Add `--scope mr:review` to let an agent give verdicts, `--scope mr:merge` to let it merge. A token can never grant more than the token that creates it. Full scope list: [CLI.md](CLI.md).
56	65
57	66		## Follow events
58	67
59		-	`ermite watch [repo] --json` prints one JSON object per event (pushes, repositories, merge requests, comments). It reconnects and resumes on its own; a `gap` event means some events were lost, so reload the state.
	68	+	`ermite watch [repo] --json` prints one JSON object per event (pushes, repositories, merge requests, notes: added, edited, deleted, resolved). It reconnects and resumes on its own; a `gap` event means some events were lost, so reload the state.
60	69
61	70		## Run a server
62	71

 | Preset | Scopes |
 |---|---|
-| `cli` | every scope, for the owner's daily use |
+| `cli` | every scope (including `mr:review`), for the owner's daily use; a token created before `mr:review` existed lacks it and loses owner rights: recreate it with `--preset cli` using the root token or from a signed-in owner web session (an old token lacking `mr:review` cannot grant it), then replace the stored CLI secret and revoke the old token |
 | `agent` | `repo:write`, `mr:write` |
 | `ci` | `repo:read` |
 | `repo:read` | read private repositories |
 | `repo:write` | push, create a repository |
 | `repo:admin` | delete a repository, change visibility, description, default branch, merge settings |
-| `mr:write` | open, comment on, resolve, close, mark read |
+| `mr:write` | open, comment on, reply, edit and delete own notes, resolve, close, mark read |
+| `mr:review` | give a verdict (`--approve`, `--request-changes`) |
 | `mr:merge` | merge, and delete the merged source branch with `--delete-branch` |
 | `token:admin` | create, list and revoke tokens |
 | `ci:write`, `secrets:write`, `runner:register` | reserved for CI |
 | `mr create <repo> --source B --title T [--target B] [--description TEXT\|-]` | `mr:write` | target defaults to the default branch; title of 120 characters max; `-` reads standard input |
 | `mr list <repo> [--state open\|merged\|closed\|all] [--json]` | none | default `open` |
 | `mr show <repo> <n> [--diff] [--since-last-read] [--mark-read] [--json]` | `mr:write` for `--mark-read` | commits, changed files and comments; `--diff` adds hunks with line numbers |
-| `mr comment <repo> <n> -m TEXT\|- [--path P --line N --side old\|new]` | `mr:write` | the line must be in the current diff; a deleted file uses its old path |
+| `mr comment <repo> <n> -m TEXT\|- [--path P --line N\|A-B --side old\|new] [--reply ID] [--approve\|--request-changes]` | `mr:write`; verdicts `mr:review` | Markdown note (8000 characters max); the lines must be in the current diff on one side; a deleted file uses its old path; `--reply` takes a root note (`12`, `#12` or `^12`), inherits its anchor and cannot carry a verdict; a verdict note may have no message |
-| `mr resolve <repo> <n> <comment-id> [--undo]` | `mr:write` | doing it twice is not an error |
+| `mr edit-comment <repo> <n> <comment-id> -m TEXT\|-` | `mr:write` | author only, open merge request; the verdict is unchanged |
+| `mr delete-comment <repo> <n> <comment-id>` | `mr:write` | author only; the text is lost, the verdict is kept |
+| `mr resolve <repo> <n> <comment-id> [--undo]` | `mr:write` | on the thread root, covers its replies; doing it twice is not an error |
 | `mr merge <repo> <n> --head <full-id> [--squash] [--delete-branch]` | `mr:merge` | refused if the branch moved or the target is not an ancestor of the head; `--delete-branch` deletes the source after the merge (refused for the default branch or a branch another open request uses; JSON adds `source_branch_deleted`) |
 | `mr close <repo> <n>` | `mr:write` | a closed merge request cannot be reopened |
 merge request web URL on its own line. With `--json`, merge request objects
 (create, show, list, close and merge) include `url` alongside the other fields.
-Merge request JSON: `repo`, `url` (full web URL), `number`, `title`, `description`, `source_branch`, `target_branch`, `state`, `author` (token label), `created_at`, `updated_at`, `head_id`, `merged_commit_id`, `merge_mode`. `mr show --json` adds `commits`, `diff_base_id`, `stats`, `files` (with `hunks` under `--diff`: `kind`, `old_number`, `new_number`, `text`), `comments`, `last_read_id`, `last_read_lost`.
+Merge request JSON: `repo`, `url` (full web URL), `number`, `title`, `description`, `source_branch`, `target_branch`, `state`, `author` (token label), `created_at`, `updated_at`, `head_id`, `merged_commit_id`, `merge_mode`. `mr show --json` adds `commits`, `diff_base_id`, `stats`, `files` (with `hunks` under `--diff`: `kind`, `old_number`, `new_number`, `text`), `comments`, `verdicts`, `last_read_id`, `last_read_lost`.
+Comment JSON: `id`, `author`, `path`, `line`, `side`, `commit_id`, `text`, `resolved`, `created_at`, `reply_to` (empty for roots), `start_line` (0 for a single line), `verdict` (`""`, `approve`, `changes_requested`), `edited_at`, `deleted`. Verdict JSON (latest per author): `author`, `verdict`, `comment_id`, `commit_id`, `created_at`, `stale` (an approval given on another head; changes requested are never stale). Verdicts are informative and never block a merge. In notes, `^N` refers to note N.
 ## Workflows
 | `repo_created`, `repo_updated`, `repo_deleted` | `repo`, `repository` |
 | `pushed` | `repo`, `ref_updates` (`ref`, `old_id`, `new_id`; an empty id means created or deleted) |
 | `merge_request_opened`, `merge_request_merged`, `merge_request_closed` | `repo`, `merge_request` |
-| `comment_added` | `repo`, `number`, `comment` |
+| `comment_added`, `comment_edited`, `comment_deleted`, `comment_resolved` | `repo`, `number`, `comment` (a verdict note has `verdict` set) |
 | `gap` | none: events were lost, reload the state |
 ## Server and web

 1. List: `ermite mr list <repo> --json`.
 2. Read: `ermite mr show <repo> <n> --diff --json`. Keep `head_id`; take line numbers from the hunks.
-3. Comment on a line: `ermite mr comment <repo> <n> -m "text" --path <file> --line <n> --side new`. Without `--path` the comment is general. The line must appear in the diff.
+3. Comment on a line: `ermite mr comment <repo> <n> -m "text" --path <file> --line <n> --side new`; `--line 10-14` covers a range on one side. Without `--path` the comment is general. The line must appear in the diff. Notes are Markdown; `^N` refers to note N.
-4. Merge only when asked: `ermite mr merge <repo> <n> --head <head_id>`, with `--squash` for one commit signed by the server and `--delete-branch` to delete the source branch afterwards.
+4. End a review with a verdict (needs `mr:review`): `--approve` or `--request-changes`, alone or with a message. Verdicts are informative and cannot be edited; a new verdict replaces the previous one.
+5. Merge only when asked: `ermite mr merge <repo> <n> --head <head_id>`, with `--squash` for one commit signed by the server and `--delete-branch` to delete the source branch afterwards.
 The merge is refused when the branch moved since `head_id` (review again) or when the target is not an ancestor of the head (rebase and push again). The repository may allow only one merge mode.
+### Act on a review
+Notes are visible as soon as they are written, so a review is not finished until its verdict. Act on a verdict, not on each note (`ermite watch --json` shows `verdict` on the `comment_added` event).
+On `changes_requested`: run `ermite mr show <repo> <n> --json`, read the open threads (roots with `resolved` false and their replies), fix the code, push, then answer each thread with `ermite mr comment <repo> <n> --reply <id> -m "Fixed in ^N"`. Leave resolving to the reviewer: do not run `mr resolve` on their threads. Edit or delete only your own notes (`mr edit-comment`, `mr delete-comment`).
+An approval is stale after a push; changes requested stay until the reviewer gives a new verdict.
 `ermite mr create` and `ermite mr close` print a confirmation followed by the full
 merge request web URL on its own line. With `--json`, merge request objects
 (create, show, list, close and merge) include `url` alongside the other fields.
 ermite token create my-agent --preset agent --repo <repo> --expires 30d
 ```
-The secret is printed once. Presets: `agent` (push, open and comment on merge requests), `ci` (read), `cli` (everything). Add `--scope mr:merge` to let an agent merge. A token can never grant more than the token that creates it. Full scope list: [CLI.md](CLI.md).
+The secret is printed once. Presets: `agent` (push, open and comment on merge requests; no verdicts), `ci` (read), `cli` (everything). Add `--scope mr:review` to let an agent give verdicts, `--scope mr:merge` to let it merge. A token can never grant more than the token that creates it. Full scope list: [CLI.md](CLI.md).
 ## Follow events
-`ermite watch [repo] --json` prints one JSON object per event (pushes, repositories, merge requests, comments). It reconnects and resumes on its own; a `gap` event means some events were lost, so reload the state.
+`ermite watch [repo] --json` prints one JSON object per event (pushes, repositories, merge requests, notes: added, edited, deleted, resolved). It reconnects and resumes on its own; a `gap` event means some events were lost, so reload the state.
 ## Run a server

Merge settings

Comments0 open / 0