diff --git a/mcpsrv/backend.go b/mcpsrv/backend.go index b3a90a45462f8e1654beb8a9513dca31d0d9bb7b..73288bad2ae0ab2edc4c7f756720e9a19adeec83 100644 --- a/mcpsrv/backend.go +++ b/mcpsrv/backend.go @@ -48,6 +48,7 @@ // that depends on them, not of a test somebody may not run. var ( _ Reader = (*service.Service)(nil) _ Searcher = (*search.Index)(nil) + _ Writer = (*service.Service)(nil) ) const ( diff --git a/mcpsrv/comment.go b/mcpsrv/comment.go new file mode 100644 index 0000000000000000000000000000000000000000..65ee92726bb84d9dc8b26df169ac0543f461cd8b --- /dev/null +++ b/mcpsrv/comment.go @@ -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), + } +} diff --git a/mcpsrv/comment_internal_test.go b/mcpsrv/comment_internal_test.go new file mode 100644 index 0000000000000000000000000000000000000000..126fd3024681dd6d293405c16000c4011b1c08a7 --- /dev/null +++ b/mcpsrv/comment_internal_test.go @@ -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) + } +} diff --git a/mcpsrv/comment_test.go b/mcpsrv/comment_test.go new file mode 100644 index 0000000000000000000000000000000000000000..b9274e440f75124a9ba7ebbefd3b097e30ddcb37 --- /dev/null +++ b/mcpsrv/comment_test.go @@ -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") +} diff --git a/mcpsrv/mcpsrv.go b/mcpsrv/mcpsrv.go index 6b9f942385bcd937d357cab1a26777aa6d03268a..6a74c1e28650bb9b062714c5aecdc820f7009bda 100644 --- a/mcpsrv/mcpsrv.go +++ b/mcpsrv/mcpsrv.go @@ -36,8 +36,14 @@ // spec_list ever reports one. // // 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 @@ out, err := listHandler(ctx, b, in) 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", @@ -171,6 +177,27 @@ "proposal whose link you never mention is invisible. When `merged` is true the space's " + "auto_merge policy landed the change immediately; otherwise it is open and waiting.", }, func(ctx context.Context, _ *mcp.CallToolRequest, in proposeInput) (*mcp.CallToolResult, proposeOutput, 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 }) } diff --git a/mcpsrv/mcpsrv_test.go b/mcpsrv/mcpsrv_test.go index 2c9cab7b8de6579268dda7cc453c33da7930b426..c8d92c24b1a866390371c59bec0dafb48dc8150f 100644 --- a/mcpsrv/mcpsrv_test.go +++ b/mcpsrv/mcpsrv_test.go @@ -605,8 +605,8 @@ } // --- 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) diff --git a/mcpsrv/propose.go b/mcpsrv/propose.go index 4a6722801af18c2f07a309366b8dc8ea958df824..22d75d2cff79af4c843ff194cb4b7fefe3877a96 100644 --- a/mcpsrv/propose.go +++ b/mcpsrv/propose.go @@ -8,16 +8,27 @@ "sourcecraft.dev/bigbes/sr-ht-spec/authn" "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 @@ Branch string `json:"branch"` 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