M mcpsrv/backend.go => mcpsrv/backend.go +1 -0
@@ 48,6 48,7 @@ type Searcher interface {
var (
_ Reader = (*service.Service)(nil)
_ Searcher = (*search.Index)(nil)
+ _ Writer = (*service.Service)(nil)
)
const (
A => +219 -0
@@ 0,0 1,219 @@
package mcpsrv
import (
"context"
"fmt"
"strings"
"time"
"sourcecraft.dev/bigbes/sr-ht-spec/authn"
"sourcecraft.dev/bigbes/sr-ht-spec/service"
)
// Commenter is everything spec_comment may reach, and the list is short on
// purpose: read the threads, read the revision they are anchored against, and
// append a reply.
//
// What it leaves out is the load-bearing part. *service.Service also has
// CommentOn and ResolveThread, and both are owner-only there; naming them here
// would be harmless today and a regression the first time somebody relaxed the
// service-side check. An unresolved thread suppresses policy auto-merge, so an
// agent that could open or resolve one would hold the gate that exists to hold
// its own output back. The handler is written against this interface rather
// than against Writer so that "the comment tool cannot resolve a thread" is a
// property of the types, not of the handler remembering not to.
type Commenter interface {
Threads(ctx context.Context, p authn.Principal, proposalID int) ([]service.Thread, error)
ReplyTo(ctx context.Context, p authn.Principal, threadID int, body string) (service.Comment, error)
GetProposal(ctx context.Context, id int) (service.Proposal, error)
ProposalDiff(ctx context.Context, p service.Proposal) ([]service.ProposalDoc, error)
}
type commentInput struct {
Proposal int `json:"proposal" jsonschema:"the proposal whose review threads to read, or whose thread to reply to — the id spec_propose returned"`
// Thread and Body together are the reply; omitting both lists.
Thread int `json:"thread,omitempty" jsonschema:"reply to this thread, given as the thread id from a listing. Omit both this and body to list the proposal's threads instead."`
Body string `json:"body,omitempty" jsonschema:"the reply text, required when thread is given"`
}
// commentAuthor is one message: a thread's root or a reply. Times are formatted
// strings rather than time.Time because the tool's schema is derived from this
// struct by reflection, and a time.Time would reach the agent as an object with
// no fields.
type commentAuthor struct {
ID int `json:"id"`
// Thread is the id of the thread this message belongs to. On a root it is
// its own id, which is the value to send back as the thread argument.
Thread int `json:"thread"`
Author string `json:"author"`
// Agent reports whether an agent wrote this, so a reply of one's own is
// distinguishable from the owner's critique without parsing the name.
Agent bool `json:"agent"`
Body string `json:"body"`
Created string `json:"created"`
}
type commentThread struct {
commentAuthor
// Document is the path of the document the thread is on, and DocID the
// archive key its anchor was written against — the anchor survives a rename,
// so the two can disagree.
Document string `json:"document"`
DocID string `json:"doc_id,omitempty"`
// Heading is the enclosing headings of the commented block, outermost
// first. With Document it is how to find the block being talked about.
Heading []string `json:"heading,omitempty"`
// Side is which revision of the block was commented on: "new" for the
// proposed text, "old" for a block the proposal deletes.
Side string `json:"side,omitempty"`
// State is how well the anchor still fits the CURRENT proposal branch:
// "anchored" (the block is there verbatim), "edited" (the block is at that
// position but its text has changed since the comment) or "outdated" (the
// anchor lost its block). Acting on an outdated critique is the failure this
// field exists to prevent.
State string `json:"state"`
// Block is the block's index in the document, or -1 when the anchor is
// outdated and points at nothing.
Block int `json:"block"`
// Open reports whether the thread still awaits the owner. Only the owner can
// close one; a reply never does.
Open bool `json:"open"`
Replies []commentAuthor `json:"replies,omitempty"`
}
type commentOutput struct {
Proposal int `json:"proposal"`
// Threads is set when listing, and empty when the proposal has no review
// threads at all — which is a proposal nobody has commented on, not an
// error.
Threads []commentThread `json:"threads,omitempty"`
// Reply is set when replying, and carries the stored reply as it was
// attributed: an agent's reply comes back under its own agent identity.
Reply *commentAuthor `json:"reply,omitempty"`
}
func commentHandler(ctx context.Context, c Commenter, in commentInput) (commentOutput, error) {
if in.Proposal <= 0 {
return commentOutput{}, fmt.Errorf("proposal must name a proposal id")
}
// The mode is chosen on whether body was sent at all, not on whether it has
// anything in it: a whitespace body is a caller that meant to reply and
// botched it, and quietly listing instead would look like the reply landed.
body := strings.TrimSpace(in.Body)
switch {
case in.Thread > 0 && body == "":
return commentOutput{}, fmt.Errorf("a reply to thread %d needs a body", in.Thread)
case in.Thread <= 0 && in.Body != "":
return commentOutput{}, fmt.Errorf("body needs the thread it answers; pass thread, " +
"or omit body to list this proposal's threads")
}
// The principal is the one the resolver middleware put on this request.
// service.Threads and service.ReplyTo apply the ACL — this layer forwards
// the caller rather than deciding anything, so the read plane has one policy.
principal := authn.PrincipalFromContext(ctx)
// Threads runs before anything touches git, in both modes. It is one query,
// it is where the ACL is enforced, and a proposal with no threads needs no
// revision read at all.
threads, err := c.Threads(ctx, principal, in.Proposal)
if err != nil {
return commentOutput{}, err
}
if in.Thread > 0 {
return replyToThread(ctx, c, principal, threads, in.Proposal, in.Thread, body)
}
return listThreads(ctx, c, threads, in.Proposal)
}
// listThreads resolves every anchor against the proposal branch as it stands
// now and reports the fit.
//
// The anchoring is not optional decoration. A thread's stored anchor says where
// the comment was written, and the branch has moved since — often because this
// very agent revised it. An agent asking "what should I fix" that is not told
// the critique no longer describes any block will go and fix the wrong
// paragraph, so the state travels with every thread.
func listThreads(ctx context.Context, c Commenter, threads []service.Thread, proposalID int) (commentOutput, error) {
out := commentOutput{Proposal: proposalID}
if len(threads) == 0 {
return out, nil
}
p, err := c.GetProposal(ctx, proposalID)
if err != nil {
return commentOutput{}, err
}
docs, err := c.ProposalDiff(ctx, p)
if err != nil {
return commentOutput{}, err
}
out.Threads = make([]commentThread, 0, len(threads))
for _, t := range service.AnchorThreads(threads, docs) {
out.Threads = append(out.Threads, threadEntry(t))
}
return out, nil
}
// replyToThread appends the reply, after checking the thread is one of this
// proposal's.
//
// A thread id is a global integer, so a mistyped one names a real thread on
// somebody else's proposal, and service.ReplyTo would accept it: the id is all
// it needs. Requiring the proposal and checking membership here turns that
// typo into an error instead of a reply that lands out of sight of the agent
// that wrote it. The threads were already read for the ACL check, so it costs
// nothing.
func replyToThread(ctx context.Context, c Commenter, p authn.Principal, threads []service.Thread, proposalID, threadID int, body string) (commentOutput, error) {
found := false
for _, t := range threads {
if t.Root.ID == threadID {
found = true
break
}
}
if !found {
return commentOutput{}, fmt.Errorf("proposal %d has no review thread %d; "+
"call spec_comment with only proposal to list its threads", proposalID, threadID)
}
reply, err := c.ReplyTo(ctx, p, threadID, body)
if err != nil {
return commentOutput{}, err
}
entry := authorEntry(reply)
return commentOutput{Proposal: proposalID, Reply: &entry}, nil
}
func threadEntry(t service.Thread) commentThread {
e := commentThread{
commentAuthor: authorEntry(t.Root),
Document: t.DocPath,
DocID: t.Anchor.DocID,
Heading: t.Anchor.HeadingPath,
Side: string(t.Anchor.Side),
State: string(t.State),
Block: t.Block,
Open: t.Open(),
}
// A root's thread id is its own id: that is the value spec_comment takes
// back as the thread argument.
e.Thread = t.Root.ID
for _, r := range t.Replies {
e.Replies = append(e.Replies, authorEntry(r))
}
return e
}
func authorEntry(c service.Comment) commentAuthor {
return commentAuthor{
ID: c.ID,
Thread: c.ParentID,
Author: c.Author,
Agent: c.Agent,
Body: c.Body,
Created: c.Created.UTC().Format(time.RFC3339),
}
}
A => +319 -0
@@ 0,0 1,319 @@
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)
}
if c.sawPrincipal != principal {
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)
}
}
A => +244 -0
@@ 0,0 1,244 @@
package mcpsrv_test
import (
"context"
"encoding/json"
"errors"
"reflect"
"sort"
"testing"
"time"
"github.com/modelcontextprotocol/go-sdk/mcp"
"github.com/stretchr/testify/require"
"sourcecraft.dev/bigbes/sr-ht-spec/authn"
"sourcecraft.dev/bigbes/sr-ht-spec/core"
"sourcecraft.dev/bigbes/sr-ht-spec/mcpsrv"
"sourcecraft.dev/bigbes/sr-ht-spec/service"
)
// stubWriter is a write backend that only has to exist: these tests are about
// which tools the server offers and what arguments they take, and every one of
// them fails before a call would reach it.
type stubWriter struct{ called bool }
func (s *stubWriter) Propose(context.Context, service.ProposeRequest) (service.ProposeResult, error) {
s.called = true
return service.ProposeResult{}, errors.New("not reached")
}
func (s *stubWriter) Threads(context.Context, authn.Principal, int) ([]service.Thread, error) {
s.called = true
return nil, errors.New("not reached")
}
func (s *stubWriter) ReplyTo(context.Context, authn.Principal, int, string) (service.Comment, error) {
s.called = true
return service.Comment{}, errors.New("not reached")
}
func (s *stubWriter) GetProposal(context.Context, int) (service.Proposal, error) {
s.called = true
return service.Proposal{}, errors.New("not reached")
}
func (s *stubWriter) ProposalDiff(context.Context, service.Proposal) ([]service.ProposalDoc, error) {
s.called = true
return nil, errors.New("not reached")
}
// answeringWriter answers instead of failing, so one call can be made end to
// end. It applies no ACL, unlike the service it stands in for: an in-memory
// session carries no principal, since the resolver middleware is HTTP's.
type answeringWriter struct{ stubWriter }
func (*answeringWriter) Threads(context.Context, authn.Principal, int) ([]service.Thread, error) {
return []service.Thread{{
Root: service.Comment{
ID: 3, Body: "this needs a rationale", Author: "bigbes",
Created: time.Date(2026, 7, 24, 9, 0, 0, 0, time.UTC),
},
DocPath: "specs/0007-storage.md",
Anchor: core.CommentAnchor{
DocID: "SPEC-0007", HeadingPath: []string{"Storage"}, BlockHash: "deadbeef", Side: core.SideNew,
},
Replies: []service.Comment{{
ID: 5, ParentID: 3, Body: "adding it", Author: "claude-code/spec-writer", Agent: true,
Created: time.Date(2026, 7, 24, 10, 0, 0, 0, time.UTC),
}},
Block: -1,
}}, nil
}
func (*answeringWriter) GetProposal(_ context.Context, id int) (service.Proposal, error) {
return service.Proposal{ID: id}, nil
}
func (*answeringWriter) ProposalDiff(context.Context, service.Proposal) ([]service.ProposalDoc, error) {
// The proposal no longer changes the commented document — the agent reverted
// it — so the thread anchors to nothing and must say so rather than vanish.
return nil, nil
}
// connectWriting is connect with a write backend, so the two write tools are
// registered.
func connectWriting(t *testing.T, w mcpsrv.Writer) *mcp.ClientSession {
t.Helper()
ctx := context.Background()
serverTransport, clientTransport := mcp.NewInMemoryTransports()
r, s := newFixture()
srv, err := mcpsrv.New(mcpsrv.Backend{Docs: r, Index: s, Write: w}, "test")
require.NoError(t, err)
serverConn, err := srv.Connect(ctx, serverTransport, nil)
require.NoError(t, err)
t.Cleanup(func() { _ = serverConn.Close() })
client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "test"}, nil)
session, err := client.Connect(ctx, clientTransport, nil)
require.NoError(t, err)
t.Cleanup(func() { _ = session.Close() })
return session
}
func toolsOf(t *testing.T, s *mcp.ClientSession) map[string]*mcp.Tool {
t.Helper()
out := map[string]*mcp.Tool{}
for tool, err := range s.Tools(context.Background(), nil) {
require.NoError(t, err)
out[tool.Name] = tool
}
return out
}
// The write tools appear only with a write backend, and spec_comment is one of
// them: listing threads and replying travel together, since a listing an agent
// cannot answer is half a loop.
func TestCommentToolNeedsAWriteBackend(t *testing.T) {
r, s := newFixture()
require.NotContains(t, toolsOf(t, connect(t, r, s)), "spec_comment")
tools := toolsOf(t, connectWriting(t, &stubWriter{}))
names := make([]string, 0, len(tools))
for n := range tools {
names = append(names, n)
}
sort.Strings(names)
require.Equal(t, []string{"spec_comment", "spec_list", "spec_propose", "spec_read", "spec_search"}, names)
// Neither write tool claims to be read-only: a client that retried one
// freely would post the same reply twice.
require.Nil(t, tools["spec_comment"].Annotations)
require.NotEmpty(t, tools["spec_comment"].Description)
}
// The tool's whole argument surface is proposal, thread and body. There is no
// argument that opens a thread and none that resolves one, and this test is the
// one that would notice if somebody added one: an unresolved thread suppresses
// policy auto-merge, so an agent able to resolve its own critique would control
// the gate that exists to hold its output back.
func TestCommentToolOffersNoWayToOpenOrResolveAThread(t *testing.T) {
tools := toolsOf(t, connectWriting(t, &stubWriter{}))
// The schema arrives as the client sees it — decoded JSON, not the Go value
// the server built — which is the form an agent actually reads.
raw, err := json.Marshal(tools["spec_comment"].InputSchema)
require.NoError(t, err)
var schema struct {
Properties map[string]json.RawMessage `json:"properties"`
}
require.NoError(t, json.Unmarshal(raw, &schema))
args := make([]string, 0, len(schema.Properties))
for name := range schema.Properties {
args = append(args, name)
}
sort.Strings(args)
require.Equal(t, []string{"body", "proposal", "thread"}, args)
// And the schema is closed, so a resolve-shaped argument is refused by
// validation rather than ignored — an ignored one would let a caller believe
// it had resolved the thread.
stub := &stubWriter{}
res := call(t, connectWriting(t, stub), "spec_comment", map[string]any{
"proposal": 7, "thread": 1, "resolve": true,
})
require.True(t, res.IsError, "an unknown argument was accepted")
require.Contains(t, errorText(res), `additional properties ["resolve"]`,
"the refusal must be the schema rejecting the argument, not a later validation")
require.False(t, stub.called, "a rejected call reached the service")
}
// The whole listing as an agent receives it: the tool's output schema and its
// JSON are derived from Go structs by reflection, and the root comment's fields
// are promoted from an embedded one — so the flattening is a library detail
// that only a real call over the transport can confirm.
func TestCommentListReachesTheClientFlat(t *testing.T) {
res := call(t, connectWriting(t, &answeringWriter{}), "spec_comment", map[string]any{"proposal": 7})
var out struct {
Proposal int `json:"proposal"`
Threads []struct {
Thread int `json:"thread"`
Author string `json:"author"`
Agent bool `json:"agent"`
Body string `json:"body"`
Created string `json:"created"`
Document string `json:"document"`
DocID string `json:"doc_id"`
Heading []string `json:"heading"`
Side string `json:"side"`
State string `json:"state"`
Block int `json:"block"`
Open bool `json:"open"`
Replies []struct {
ID int `json:"id"`
Thread int `json:"thread"`
Author string `json:"author"`
Agent bool `json:"agent"`
Body string `json:"body"`
} `json:"replies"`
} `json:"threads"`
}
decode(t, res, &out)
require.Equal(t, 7, out.Proposal)
require.Len(t, out.Threads, 1)
th := out.Threads[0]
require.Equal(t, 3, th.Thread)
require.Equal(t, "bigbes", th.Author)
require.False(t, th.Agent)
require.Equal(t, "this needs a rationale", th.Body)
require.Equal(t, "2026-07-24T09:00:00Z", th.Created)
require.Equal(t, "specs/0007-storage.md", th.Document)
require.Equal(t, "SPEC-0007", th.DocID)
require.Equal(t, []string{"Storage"}, th.Heading)
require.Equal(t, "new", th.Side)
require.True(t, th.Open)
// The document the comment was on is no longer part of the proposal, so the
// anchor is reported lost rather than dropped or pointed at block 0.
require.Equal(t, "outdated", th.State)
require.Equal(t, -1, th.Block)
require.Len(t, th.Replies, 1)
require.Equal(t, 3, th.Replies[0].Thread)
require.True(t, th.Replies[0].Agent, "an agent's own reply is marked as one")
require.Equal(t, "adding it", th.Replies[0].Body)
}
// The structural half of the same rule. *service.Service has CommentOn and
// ResolveThread — both owner-only there — and the comment handler is written
// against this interface precisely so it cannot name them. Widening it is the
// change this test exists to stop; the service-side check is the second lock,
// not the only one.
func TestCommenterCannotReachOwnerOnlyThreadOperations(t *testing.T) {
typ := reflect.TypeOf((*mcpsrv.Commenter)(nil)).Elem()
methods := make([]string, 0, typ.NumMethod())
for i := range typ.NumMethod() {
methods = append(methods, typ.Method(i).Name)
}
require.Equal(t, []string{"GetProposal", "ProposalDiff", "ReplyTo", "Threads"}, methods,
"spec_comment's backend must reach nothing but reading threads and replying")
}
M mcpsrv/mcpsrv.go => mcpsrv/mcpsrv.go +33 -6
@@ 36,8 36,14 @@
//
// Serving drafts by default would poison every downstream agent context with
// unreviewed text, which is the exact failure the service exists to prevent.
-// The write tools (spec_propose, spec_comment) are Phase 3 and are deliberately
-// absent; nothing here writes.
+//
+// # The write plane
+//
+// Two tools write, and both are registered only when a write backend is wired:
+// spec_propose opens or extends a proposal, and spec_comment reads a proposal's
+// review threads and replies to them. Neither can approve, merge, open a review
+// thread or resolve one — those are the owner's, in service/, and the
+// interfaces here do not name them.
//
// # Layering
//
@@ 151,10 157,10 @@ func New(b Backend, version string) (*mcp.Server, error) {
return nil, out, err
})
- // The one write tool, registered only when a write backend is wired. It is
- // not read-only or idempotent — proposing twice opens two proposals — so it
- // carries neither hint, which is how a client tells a tool it can retry
- // freely from one it cannot.
+ // The write tools, registered only when a write backend is wired. Neither is
+ // read-only or idempotent — proposing twice opens two proposals, replying
+ // twice says it twice — so neither carries a hint, which is how a client
+ // tells a tool it can retry freely from one it cannot.
if b.Write != nil {
mcp.AddTool(srv, &mcp.Tool{
Name: "spec_propose",
@@ 173,6 179,27 @@ func New(b Backend, version string) (*mcp.Server, error) {
out, err := proposeHandler(ctx, b.Write, in)
return nil, out, err
})
+
+ mcp.AddTool(srv, &mcp.Tool{
+ Name: "spec_comment",
+ Description: "Read the review threads on a proposal, and reply to one.\n\n" +
+ "Pass only `proposal` to list its threads: each carries the owner's critique, its " +
+ "replies, the document and heading path it is anchored to, and whether it is still " +
+ "open.\n\n" +
+ "Read `state` before acting on a thread. \"anchored\" means the block you were " +
+ "criticised for is still there verbatim; \"edited\" means the block is still in that " +
+ "position but its text changed after the comment was written, so the critique may " +
+ "already be addressed; \"outdated\" means the anchor lost its block entirely and the " +
+ "comment describes text that is no longer in the proposal. Fixing what an outdated " +
+ "comment asks for edits something else.\n\n" +
+ "Pass `thread` and `body` to reply to that thread. Replying does not close it — only " +
+ "the owner resolves a thread, and an open thread holds back auto-merge. So answer the " +
+ "critique and push the revision with spec_propose; do not expect the reply itself to " +
+ "unblock the proposal.",
+ }, func(ctx context.Context, _ *mcp.CallToolRequest, in commentInput) (*mcp.CallToolResult, commentOutput, error) {
+ out, err := commentHandler(ctx, b.Write, in)
+ return nil, out, err
+ })
}
return srv, nil
M mcpsrv/mcpsrv_test.go => mcpsrv/mcpsrv_test.go +2 -2
@@ 605,8 605,8 @@ func TestListRevWithoutSpace(t *testing.T) {
// --- wiring -----------------------------------------------------------------
-// Only read tools exist. The write tools are Phase 3, and a half-wired one is
-// worse than none: an agent that sees spec_propose will call it.
+// A backend with no write side registers only the read tools. A half-wired
+// write tool is worse than none: an agent that sees spec_propose will call it.
func TestOnlyReadToolsAreRegistered(t *testing.T) {
r, s := newFixture()
session := connect(t, r, s)
M mcpsrv/propose.go => mcpsrv/propose.go +16 -5
@@ 8,16 8,27 @@ import (
"sourcecraft.dev/bigbes/sr-ht-spec/service"
)
-// Writer is the write side of the orchestration layer the propose tool calls —
-// the same service.Propose that the REST PUT calls, so the two write surfaces
-// share one implementation of If-Match, provenance and auto-merge rather than
-// drifting apart. *service.Service satisfies it.
+// Writer is the write side of the orchestration layer the write tools call.
+// *service.Service satisfies it.
//
// It is a distinct interface from Reader, and optional on the Backend, because
// the read tools need no write path and a read-only deployment (or a test)
// should be able to register the three read tools without a service that can
// mutate anything.
+//
+// It is a union of one narrow interface per write tool rather than a flat
+// method list, and each handler takes only its own half. That is what stops
+// spec_comment from reaching Propose and — the reason it matters — what stops
+// it from reaching a resolve method however service/ grows; see [Commenter].
type Writer interface {
+ Proposer
+ Commenter
+}
+
+// Proposer is what spec_propose calls — the same service.Propose the REST PUT
+// calls, so the two write surfaces share one implementation of If-Match,
+// provenance and auto-merge rather than drifting apart.
+type Proposer interface {
Propose(ctx context.Context, req service.ProposeRequest) (service.ProposeResult, error)
}
@@ 62,7 73,7 @@ type proposeOutput struct {
BaseRev string `json:"base_rev"`
}
-func proposeHandler(ctx context.Context, w Writer, in proposeInput) (proposeOutput, error) {
+func proposeHandler(ctx context.Context, w Proposer, in proposeInput) (proposeOutput, error) {
ref, err := parseSpace(in.Space)
if err != nil {
return proposeOutput{}, err