~bigbes/sr-ht-spec

ref: 3d811988f9960cf9057e09f30b59ceb71a7623c2 sr-ht-spec/mcpsrv/comment_internal_test.go -rw-r--r-- 12.6 KiB
3d811988 — Eugene Blikh service: wrap the merge and reject failures with culpa 10 days ago
                                                                                
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
package mcpsrv

import (
	"context"
	"fmt"
	"strings"
	"testing"
	"time"

	"sourcecraft.dev/bigbes/sr-ht-spec/authn"
	"sourcecraft.dev/bigbes/sr-ht-spec/core"
	"sourcecraft.dev/bigbes/sr-ht-spec/prosediff"
	"sourcecraft.dev/bigbes/sr-ht-spec/service"
)

// The two revisions of the document under review. The comments were written
// against fxCommentBase; fxCommentHead is what the agent has since pushed, and
// every anchor is resolved against it — which is the whole point of resolving
// at read time rather than storing a state.
const (
	fxCommentBase = "# Storage\n\nThe first paragraph.\n\nThe second paragraph, as it was.\n\n" +
		"# Gone\n\nA section that was removed.\n"
	fxCommentHead = "# Storage\n\nThe first paragraph.\n\nThe second paragraph, rewritten.\n"
)

// fakeCommenter stands in for *service.Service. It holds no database, no git
// and no repository, which is what lets these tests run against real anchor
// resolution — service.AnchorThreads is called for real, on real segmented
// documents, because that is the part with something to get wrong.
//
// Its Threads mirrors service.Threads' ACL rather than owning one: the check
// lives in service/ and this only stands in for it, so that "the tool surfaces
// the refusal instead of swallowing it" can be tested at all.
type fakeCommenter struct {
	threads []service.Thread
	docs    []service.ProposalDoc

	sawPrincipal authn.Principal
	sawThreadID  int
	sawBody      string
	diffCalls    int
}

func (f *fakeCommenter) Threads(_ context.Context, p authn.Principal, _ int) ([]service.Thread, error) {
	f.sawPrincipal = p
	if !p.CanRead() {
		return nil, fmt.Errorf("%w: %s may not read review threads", service.ErrForbidden, p)
	}
	return f.threads, nil
}

func (f *fakeCommenter) ReplyTo(_ context.Context, p authn.Principal, threadID int, body string) (service.Comment, error) {
	f.sawPrincipal, f.sawThreadID, f.sawBody = p, threadID, body
	// The attribution rule itself is service.ReplyTo's; reproduced here only so
	// the mapping back out has something shaped like a stored reply to map.
	author, agent := p.Owner, false
	if p.IsAgent() {
		if p.Agent == "" || p.Session == "" {
			return service.Comment{}, fmt.Errorf("%w: an agent reply must carry its identity and session", service.ErrInvalid)
		}
		author, agent = p.Agent, true
	}
	return service.Comment{
		ID: 99, ParentID: threadID, Body: body, Author: author, Agent: agent,
		Created: time.Date(2026, 7, 24, 12, 0, 0, 0, time.UTC),
	}, nil
}

func (f *fakeCommenter) GetProposal(_ context.Context, id int) (service.Proposal, error) {
	return service.Proposal{ID: id, Branch: fmt.Sprintf("proposals/%d", id)}, nil
}

func (f *fakeCommenter) ProposalDiff(context.Context, service.Proposal) ([]service.ProposalDoc, error) {
	f.diffCalls++
	return f.docs, nil
}

// anchorTo builds the anchor a comment on the named block of src would have
// been stored with, using service.AnchorOf — the same conversion the web form
// makes, so the fixtures cannot anchor to something no surface could produce.
func anchorTo(t *testing.T, src, text string) core.CommentAnchor {
	t.Helper()
	for i, b := range prosediff.Segment([]byte(src)) {
		if strings.TrimSpace(b.Text) != text {
			continue
		}
		a, err := service.AnchorOf("SPEC-0007", []byte(src), i, core.SideNew)
		if err != nil {
			t.Fatalf("AnchorOf(%q): %v", text, err)
		}
		return a
	}
	t.Fatalf("no block %q in the fixture", text)
	return core.CommentAnchor{}
}

func commentFixture(t *testing.T) *fakeCommenter {
	t.Helper()
	resolved := time.Date(2026, 7, 20, 9, 0, 0, 0, time.UTC)
	thread := func(id int, body, block string, resolvedAt *time.Time, replies ...service.Comment) service.Thread {
		return service.Thread{
			Root: service.Comment{
				ID: id, Body: body, Author: "bigbes",
				Created: time.Date(2026, 7, 19, 8, 0, 0, 0, time.UTC),
			},
			DocPath:  "specs/0007-storage.md",
			Anchor:   anchorTo(t, fxCommentBase, block),
			Replies:  replies,
			Resolved: resolvedAt,
			// Block is -1 out of service.Threads and stays so until anchoring
			// runs, which is exactly what the tool must not report as 0.
			Block: -1,
		}
	}
	return &fakeCommenter{
		threads: []service.Thread{
			thread(1, "this paragraph is still wrong", "The first paragraph.", nil,
				service.Comment{
					ID: 4, ParentID: 1, Body: "on it", Author: "claude-code/spec-writer", Agent: true,
					Created: time.Date(2026, 7, 19, 8, 30, 0, 0, time.UTC),
				}),
			thread(2, "reword this", "The second paragraph, as it was.", nil),
			thread(3, "and this section", "A section that was removed.", &resolved),
		},
		docs: []service.ProposalDoc{{
			Path:     "specs/0007-storage.md",
			Base:     []byte(fxCommentBase),
			Proposed: []byte(fxCommentHead),
		}},
	}
}

func agentPrincipal() authn.Principal {
	return authn.Principal{
		Kind:    authn.KindAgent,
		Owner:   "bigbes",
		Agent:   "claude-code/spec-writer",
		Session: "sess-1",
	}
}

// Listing reports each thread's fit against the revision the branch is at NOW,
// not against the one the comment was written on. All three states appear here
// because an agent that cannot tell them apart will go and edit the wrong
// paragraph: "anchored" is a live critique, "edited" may already be answered,
// and "outdated" describes text that is no longer in the proposal at all.
func TestCommentListReportsAnchorStateAtTheCurrentRevision(t *testing.T) {
	c := commentFixture(t)
	ctx := authn.WithPrincipal(context.Background(), agentPrincipal())

	out, err := commentHandler(ctx, c, commentInput{Proposal: 7})
	if err != nil {
		t.Fatalf("commentHandler: %v", err)
	}
	if out.Proposal != 7 || len(out.Threads) != 3 {
		t.Fatalf("output = %+v, want proposal 7 and 3 threads", out)
	}

	// The untouched paragraph: the commented text is still there verbatim.
	if got := out.Threads[0]; got.State != string(core.AnchorExact) || got.Block < 0 {
		t.Errorf("thread 1 = state %q block %d, want anchored at a real block", got.State, got.Block)
	}
	// The rewritten paragraph: same position under the same heading, new text.
	if got := out.Threads[1]; got.State != string(core.AnchorEdited) || got.Block < 0 {
		t.Errorf("thread 2 = state %q block %d, want edited at a real block", got.State, got.Block)
	}
	// The deleted section: no block to point at, and -1 rather than 0, which
	// would name the first block of the document.
	if got := out.Threads[2]; got.State != string(core.AnchorOutdated) || got.Block != -1 {
		t.Errorf("thread 3 = state %q block %d, want outdated at no block", got.State, got.Block)
	}

	// Everything the agent needs to find the block, and to know who is waiting.
	first := out.Threads[0]
	if first.Thread != 1 || first.Document != "specs/0007-storage.md" || first.DocID != "SPEC-0007" {
		t.Errorf("thread identity = %+v, want thread 1 on SPEC-0007", first)
	}
	if len(first.Heading) != 1 || first.Heading[0] != "Storage" || first.Side != string(core.SideNew) {
		t.Errorf("anchor = heading %v side %q, want [Storage]/new", first.Heading, first.Side)
	}
	if !first.Open || first.Author != "bigbes" || first.Agent {
		t.Errorf("root = %+v, want an open thread authored by the owner", first)
	}
	if len(first.Replies) != 1 || first.Replies[0].Thread != 1 || !first.Replies[0].Agent {
		t.Errorf("replies = %+v, want the agent's one reply, attributed to it", first.Replies)
	}
	if out.Threads[2].Open {
		t.Error("a resolved thread must not report itself open; the owner closed it")
	}
	if out.Reply != nil {
		t.Error("listing returned a reply it never wrote")
	}
}

// A reply carries the agent's provenance to service.ReplyTo unaltered. It
// matters because ReplyTo refuses an agent whose identity or session is empty:
// a tool that forwarded a stripped principal would turn every agent reply into
// an invalid-argument error, and one that substituted the owner would forge
// attribution.
func TestCommentReplyCarriesTheAgentIdentityAndSession(t *testing.T) {
	c := commentFixture(t)
	principal := agentPrincipal()
	ctx := authn.WithPrincipal(context.Background(), principal)

	out, err := commentHandler(ctx, c, commentInput{Proposal: 7, Thread: 2, Body: "reworded in the next push"})
	if err != nil {
		t.Fatalf("commentHandler: %v", err)
	}
	// Principal is no longer comparable with == — it carries a grant set — so
	// its rendering stands in: that names the kind, the agent, the session and
	// the owner, which is everything this assertion is about.
	if c.sawPrincipal.String() != principal.String() {
		t.Errorf("principal = %+v, want the one on the context %+v", c.sawPrincipal, principal)
	}
	if c.sawThreadID != 2 || c.sawBody != "reworded in the next push" {
		t.Errorf("service saw thread %d body %q, want 2 and the reply text", c.sawThreadID, c.sawBody)
	}
	if out.Reply == nil {
		t.Fatal("reply missing from the output")
	}
	if out.Reply.Author != "claude-code/spec-writer" || !out.Reply.Agent || out.Reply.Thread != 2 {
		t.Errorf("reply = %+v, want it attributed to the agent on thread 2", out.Reply)
	}
	if out.Reply.Created != "2026-07-24T12:00:00Z" {
		t.Errorf("created = %q, want an RFC3339 timestamp", out.Reply.Created)
	}
	if out.Threads != nil {
		t.Error("a reply must not claim to have listed threads")
	}
	// Replying reads no revision: anchor fit is a listing question.
	if c.diffCalls != 0 {
		t.Errorf("ProposalDiff called %d times while replying", c.diffCalls)
	}
}

// A thread id is a global integer, so a mistyped one names a real thread on
// some other proposal and service.ReplyTo — which takes only the id — would
// accept it. The reply would land where the agent cannot see it.
func TestCommentReplyRefusesAThreadThatIsNotOnThisProposal(t *testing.T) {
	c := commentFixture(t)
	ctx := authn.WithPrincipal(context.Background(), agentPrincipal())

	_, err := commentHandler(ctx, c, commentInput{Proposal: 7, Thread: 404, Body: "answering"})
	if err == nil {
		t.Fatal("a thread from another proposal was accepted")
	}
	if !strings.Contains(err.Error(), "no review thread 404") {
		t.Errorf("error = %v, want it to name the thread", err)
	}
	if c.sawThreadID != 0 {
		t.Errorf("the reply reached the service anyway, on thread %d", c.sawThreadID)
	}
}

// thread and body are one argument in two halves. Half of them is a caller that
// meant to reply, and answering the other question — listing — would look like
// the reply had been posted.
func TestCommentRequiresThreadAndBodyTogether(t *testing.T) {
	c := commentFixture(t)
	ctx := authn.WithPrincipal(context.Background(), agentPrincipal())

	if _, err := commentHandler(ctx, c, commentInput{Proposal: 7, Thread: 1}); err == nil {
		t.Error("a reply with no body was accepted")
	}
	if _, err := commentHandler(ctx, c, commentInput{Proposal: 7, Body: "   "}); err == nil {
		t.Error("a body with no thread was accepted")
	}
	if _, err := commentHandler(ctx, c, commentInput{Body: "x", Thread: 1}); err == nil {
		t.Error("a call naming no proposal was accepted")
	}
	if c.sawThreadID != 0 || c.diffCalls != 0 {
		t.Error("a malformed call reached the service")
	}
}

// The ACL is service.Threads'. What this pins is that the tool forwards the
// caller and surfaces the refusal, rather than reporting an empty thread list —
// which would read as "nobody has commented" to an agent asking what to fix.
func TestCommentRefusesAPrincipalThatMayNotRead(t *testing.T) {
	c := commentFixture(t)
	ctx := authn.WithPrincipal(context.Background(), authn.Anonymous())

	_, err := commentHandler(ctx, c, commentInput{Proposal: 7})
	if err == nil {
		t.Fatal("an anonymous caller read the review threads")
	}
	if !strings.Contains(err.Error(), "forbidden") {
		t.Errorf("error = %v, want the service's refusal", err)
	}
	// The refusal lands before anything reads git, because the ACL check is the
	// first call the handler makes in either mode.
	if c.diffCalls != 0 {
		t.Errorf("ProposalDiff ran for a caller that may not read")
	}

	// And it cannot write either: the same refusal covers the reply path.
	if _, err := commentHandler(ctx, c, commentInput{Proposal: 7, Thread: 1, Body: "hi"}); err == nil {
		t.Fatal("an anonymous caller replied to a review thread")
	}
	if c.sawThreadID != 0 {
		t.Error("the reply reached the service despite the refusal")
	}
}

// A proposal nobody has commented on is not an error, and it is not worth a
// revision read: anchoring resolves nothing when there is nothing to anchor.
func TestCommentListOfAnUncommentedProposalReadsNoRevision(t *testing.T) {
	c := commentFixture(t)
	c.threads = nil
	ctx := authn.WithPrincipal(context.Background(), agentPrincipal())

	out, err := commentHandler(ctx, c, commentInput{Proposal: 7})
	if err != nil {
		t.Fatalf("commentHandler: %v", err)
	}
	if len(out.Threads) != 0 || out.Proposal != 7 {
		t.Errorf("output = %+v, want proposal 7 with no threads", out)
	}
	if c.diffCalls != 0 {
		t.Errorf("ProposalDiff ran %d times for a proposal with no threads", c.diffCalls)
	}
}