Commit `2faceb4c41`

2faceb4c41e9bdcaddd7028d2311ae9cd6e2a478

parent: 65a1d7220c

Verified · cmc

cmc <hello@cleberg.net> · 2026-09-29 05:22 UTC

control: apply-suggestion resolves the thread only for those who may, and exits 0 once the commit lands

Ref #288

Layout: unified · split

internal/control/diffcomment.go +19 −8

 	if err != nil {
 		return c.fail(protocol.ExitUsage, "bad thread id %q", args[2])
 	}
-	// Thread author, MR author, or anyone with write may resolve.
+	ok, err := canResolveThread(c, repo, mr, threadID)
-	author, err := c.Store.DiffCommentAuthor(mr.ID, threadID)
 	if errors.Is(err, store.ErrNotFound) {
 		return c.fail(protocol.ExitNotFound, "no thread %d on %s!%d", threadID, repo.Path(), mr.Number)
 	}
 	if err != nil {
 		return c.fail(protocol.ExitFailure, "%v", err)
 	}
-	grant, err := c.Store.AccessRole(repo.ID, c.User.ID)
+	if !ok {
-	if err != nil {
+		return c.fail(protocol.ExitDenied, "%s", cannotResolve)
-		return c.fail(protocol.ExitFailure, "%v", err)
-	}
-	if author != c.User.ID && mr.Author != c.User.Username && !policy.CanWrite(c.User, repo, grant) {
-		return c.fail(protocol.ExitDenied, "only the thread author, the MR author, or users with write access can resolve threads")
 	}
 	if err := c.Store.SetThreadResolved(mr.ID, threadID, c.User.ID, resolved); err != nil {
 		if errors.Is(err, store.ErrNotFound) {
 	})
 }
+const cannotResolve = "only the thread author, the MR author, or users with write access can resolve threads"
+// canResolveThread reports whether the caller may resolve a thread: its
+// author, the MR author, or anyone with write on the target.
+func canResolveThread(c *Ctx, repo store.Repo, mr store.MR, threadID int64) (bool, error) {
+	author, err := c.Store.DiffCommentAuthor(mr.ID, threadID)
+	if err != nil {
+		return false, err
+	}
+	grant, err := c.Store.AccessRole(repo.ID, c.User.ID)
+	if err != nil {
+		return false, err
+	}
+	return author == c.User.ID || mr.Author == c.User.Username || policy.CanWrite(c.User, repo, grant), nil
+}
 func runMRResolve(c *Ctx, args []string) int   { return setThreadResolved(c, args, true) }
 func runMRUnresolve(c *Ctx, args []string) int { return setThreadResolved(c, args, false) }

internal/control/suggestion.go +27 −5

	@@ -332,11 +332,33 @@ func runMRApplySuggestion(c *Ctx, args []string) int {
332	return c.fail(protocol.ExitFailure, "the source branch moved; reload and retry")	332	return c.fail(protocol.ExitFailure, "the source branch moved; reload and retry")
333	}	333	}
334	RefsUpdated(c.Store, c.Cfg, src.ID, c.User.ID, c.Scope, updates)	334	RefsUpdated(c.Store, c.Cfg, src.ID, c.User.ID, c.Scope, updates)
335	if err := c.Store.SetThreadResolved(mr.ID, threadID, c.User.ID, true); err != nil {	335
336	return c.failErr(err)	336	// The commit has landed, so from here nothing fails the command: a
		337	// thread that cannot be resolved is left open and the output says so.
		338	resolved, warning := false, ""
		339	switch ok, err := canResolveThread(c, repo, mr, threadID); {
		340	case err != nil:
		341	warning = fmt.Sprintf("thread %d is still open: %v", threadID, err)
		342	case !ok:
		343	warning = fmt.Sprintf("thread %d is still open: %s", threadID, cannotResolve)
		344	default:
		345	if err := c.Store.SetThreadResolved(mr.ID, threadID, c.User.ID, true); err != nil {
		346	warning = fmt.Sprintf("thread %d is still open: %v", threadID, err)
		347	} else {
		348	resolved = true
		349	TryQueuedMerge(c.Store, c.Cfg, mr.ID)
		350	}
337	}	351	}
338	TryQueuedMerge(c.Store, c.Cfg, mr.ID)	352	d := map[string]any{"thread": threadID, "sha": sha, "source": source, "resolved": resolved}
339	return c.emit(map[string]any{"thread": threadID, "sha": sha, "source": source, "resolved": true}, func(w io.Writer) {	353	if warning != "" {
340	fmt.Fprintf(w, "applied thread %d to %s at %.10s; thread resolved\n", threadID, source, sha)	354	d["warning"] = warning
		355	}
		356	return c.emit(d, func(w io.Writer) {
		357	if resolved {
		358	fmt.Fprintf(w, "applied thread %d to %s at %.10s; thread resolved\n", threadID, source, sha)
		359	return
		360	}
		361	fmt.Fprintf(w, "applied thread %d to %s at %.10s\n", threadID, source, sha)
		362	fmt.Fprintln(c.Stderr, "warning:", warning)
341	})	363	})
342	}	364	}

internal/control/suggestion_test.go +24

	@@ -464,6 +464,30 @@ func TestApplySuggestionFork(t *testing.T) {
464	if !said {	464	if !said {
465	t.Fatalf("timeline does not say why the merge was dequeued: %+v", cs)	465	t.Fatalf("timeline does not say why the merge was dequeued: %+v", cs)
466	}	466	}
		467	// carol writes to the fork and is neither the thread's author, the
		468	// merge request's, nor a writer of the target: her apply lands and
		469	// leaves the thread open, saying so.
		470	carolID, _ := f.st.CreateUser("carol", false)
		471	carol, _ := f.st.UserByID(carolID)
		472	f.verified(carol)
		473	if err := f.st.GrantAccess(forkID, carolID, "write"); err != nil {
		474	t.Fatal(err)
		475	}
		476	out.Reset()
		477	errOut.Reset()
		478	c.Stdin = strings.NewReader("```suggestion\nTWO\n```\n")
		479	if code := Dispatch(c, []string{"mr", "diff-comment", f.repo.Path(), "2", "--path", "lib.txt", "--line", "2", "--file", "-"}); code != protocol.ExitOK {
		480	t.Fatalf("diff-comment: %s", errOut.String())
		481	}
		482	json.Unmarshal([]byte(out.String()), &env)
		483	second := strconv.Itoa(int(env.Data.Thread))
		484	stdout := f.mustWrite(carol, "mr", "apply-suggestion", f.repo.Path(), "2", second, "--json")
		485	if !strings.Contains(stdout, `"resolved":false`) \|\| !strings.Contains(stdout, "still open") {
		486	t.Fatalf("carol's apply = %s, want it applied and the thread left open", stdout)
		487	}
		488	if n, _ := f.st.UnresolvedThreadCount(mr.ID); n != 1 {
		489	t.Fatalf("unresolved threads = %d, want carol's left open", n)
		490	}
467	}	491	}
468		492
469	// Reading suggestions costs git processes per file and commit, not per	493	// Reading suggestions costs git processes per file and commit, not per

Commit 2faceb4c41

Commit `2faceb4c41`