diff --git a/mcpsrv/backend.go b/mcpsrv/backend.go new file mode 100644 index 0000000000000000000000000000000000000000..57d2b2e8e0ff732ec0cdeff62247f1d8cd50651b --- /dev/null +++ b/mcpsrv/backend.go @@ -0,0 +1,211 @@ +package mcpsrv + +import ( + "context" + "errors" + "fmt" + "strings" + + "github.com/go-git/go-git/v5/plumbing" + + "sourcecraft.dev/bigbes/sr-ht-spec/core" + "sourcecraft.dev/bigbes/sr-ht-spec/doc" + "sourcecraft.dev/bigbes/sr-ht-spec/gitx" + "sourcecraft.dev/bigbes/sr-ht-spec/search" + "sourcecraft.dev/bigbes/sr-ht-spec/service" +) + +// Reader is the part of the orchestration layer the read tools call. +// *service.Service satisfies it. +// +// It is deliberately four methods. Every one of them is the same function the +// REST handlers, the GraphQL resolvers and the web UI call, which is what the +// design means by "the MCP tools call the same resolver layer, not a parallel +// implementation" — a fifth method here that service/ does not have would be +// the beginning of a second implementation. +// +// ReadDocument is absent on purpose. Addressing a document by id needs the +// space's whole document set anyway (see resolvePage), and reading the body +// from that set rather than issuing a second, path-only read keeps one code +// path instead of two that must agree about what an id names. +type Reader interface { + ListSpaces(ctx context.Context) ([]*service.Space, error) + OpenSpace(ctx context.Context, ref core.SpaceRef) (*service.Space, error) + ResolveRev(ctx context.Context, sp *service.Space, rev string) (string, error) + ListDocuments(ctx context.Context, sp *service.Space, rev string) ([]service.Document, error) +} + +// Searcher is the query side of the one global index. *search.Index satisfies +// it. +type Searcher interface { + Search(ctx context.Context, q search.Query) (search.Results, error) +} + +// Compile-time assertions that the production types satisfy the interfaces +// this package is written against. They are here rather than in a test so that +// a signature change in service/ or search/ breaks the build of the package +// that depends on them, not of a test somebody may not run. +var ( + _ Reader = (*service.Service)(nil) + _ Searcher = (*search.Index)(nil) +) + +const ( + // minRevLen and maxRevLen bound a git object name. They are the design's + // X-Agent-Base grammar — 7-64 lowercase hex — reused here so that "a + // revision an agent may name" has one spelling across every surface. + minRevLen = 7 + maxRevLen = 64 +) + +// parseSpace resolves the tool's space argument, which is written the way the +// design writes a space everywhere else: "~owner/name". +func parseSpace(s string) (core.SpaceRef, error) { + trimmed := strings.TrimSpace(s) + if trimmed == "" { + return core.SpaceRef{}, errors.New("space must not be empty; call spec_list with no arguments to list spaces") + } + ref, err := core.ParseSpaceRef(trimmed) + if err != nil { + return core.SpaceRef{}, fmt.Errorf("space %q: %w", trimmed, err) + } + return ref, nil +} + +// parseRev turns the tool's optional rev argument into a revision string for +// service/. An empty argument becomes service.ApprovedRev, which is the read +// contract's default: the approved head. +// +// Anything else must be a git object name. Ref names are refused even though +// service/ would happily resolve them, and that refusal is the whole point: +// "proposals/42" is a legal revision one layer down, so accepting it here +// would make unreviewed proposal content reachable by a plausible-looking +// argument. Reading it requires naming a commit sha, which no read tool ever +// hands back, so it cannot be reached by accident. +func parseRev(s string) (string, error) { + rev := strings.TrimSpace(s) + if rev == "" { + return service.ApprovedRev, nil + } + if len(rev) < minRevLen || len(rev) > maxRevLen { + return "", fmt.Errorf("rev %q must be a git object name of %d-%d hex characters; "+ + "omit rev to read the approved head", rev, minRevLen, maxRevLen) + } + for i := 0; i < len(rev); i++ { + c := rev[i] + if (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') { + continue + } + return "", fmt.Errorf("rev %q must be a git object name (lowercase hex), not a branch or ref name; "+ + "omit rev to read the approved head", rev) + } + return rev, nil +} + +// archiveAt reads a space at a revision and builds the addressable document +// set, returning it together with the bodies keyed by path and the resolved +// commit. +// +// The revision is resolved first and everything below is read at the resolved +// sha, never at the caller's string. That is what makes an unpinned read +// pinnable: the rev reported back names the exact bytes returned, so a merge +// landing between the resolve and the read cannot make one answer describe two +// revisions. +// +// The whole space is read for one document. At the confirmed volume — tens of +// documents a day — that is cheap, and the alternative is worse: doc.Archive is +// where the design's addressing rule (id when valid and unique, else path) +// actually lives, and it is built from a revision's whole document set. A +// path-only fast path would be a second addressing implementation. +func archiveAt(ctx context.Context, b Backend, sp *service.Space, rev string) (*doc.Archive, map[string][]byte, string, error) { + resolved, err := b.Docs.ResolveRev(ctx, sp, rev) + if err != nil { + return nil, nil, "", err + } + docs, err := b.Docs.ListDocuments(ctx, sp, resolved) + if err != nil { + return nil, nil, "", err + } + converted, err := toGitDocuments(docs) + if err != nil { + return nil, nil, "", err + } + arc := doc.FromDocuments(sp.Ref, resolved, converted) + bodies := make(map[string][]byte, len(docs)) + for _, d := range docs { + bodies[d.Path] = d.Data + } + return arc, bodies, resolved, nil +} + +// toGitDocuments converts service/'s hex-object-name documents back into the +// shape doc.FromDocuments takes. +// +// This conversion should not exist, and it is the one place this package +// reaches below service/. service.Document carries Blob and Rev as hex strings +// precisely so that api/, mcpsrv/, graph/ and web/ need not import gitx — but +// doc.FromDocuments, which owns the addressing rule those surfaces have to +// apply, takes []gitx.Document. Until service/ exposes the archive itself, +// something above it has to bridge the two, and doing it here in four lines is +// better than restating the addressing rule in Go. A malformed sha is an +// error, not a zero hash: plumbing.NewHash silently yields the zero value for +// anything it cannot parse, and a document whose render-cache key is zero is a +// cache collision waiting to happen. +func toGitDocuments(docs []service.Document) ([]gitx.Document, error) { + out := make([]gitx.Document, 0, len(docs)) + for _, d := range docs { + if !plumbing.IsHash(d.Blob) { + return nil, fmt.Errorf("document %q carries a malformed blob id %q", d.Path, d.Blob) + } + out = append(out, gitx.Document{Path: d.Path, Blob: plumbing.NewHash(d.Blob), Data: d.Data}) + } + return out, nil +} + +// resolvePage applies the design's addressing rule to one caller-supplied +// token, using the archive's own lookups rather than reimplementing them: +// +// - a document with a well-formed id that no other document in its space +// claims is addressed by that id; +// - a document whose id is absent, malformed or duplicated is addressed by +// its path — with or without the ".md" extension, since the read plane's +// URL grammar carries no extension and an agent copying a path out of a +// search hit has one. +// +// A token naming an id that two documents claim resolves to neither, and says +// so. Silently picking one would point an agent at a document its author did +// not mean, and the ambiguity would be invisible. +func resolvePage(arc *doc.Archive, token string) (*doc.Page, error) { + name := strings.TrimSpace(token) + if name == "" { + return nil, errors.New("document must not be empty") + } + if p, ok := arc.Page(name); ok { + return p, nil + } + if p, ok := arc.ByPath(name); ok { + return p, nil + } + if p, ok := arc.ByPath(name + ".md"); ok { + return p, nil + } + if dup := duplicateIDPaths(arc, name); len(dup) > 1 { + return nil, fmt.Errorf("document id %q is claimed by %d documents (%s) and resolves to none of them; "+ + "address one of them by path instead", name, len(dup), strings.Join(dup, ", ")) + } + return nil, fmt.Errorf("no document %q in %s at %s", name, arc.Space, arc.Rev) +} + +// duplicateIDPaths lists the paths of every document claiming id. doc/ excludes +// a duplicated id from the archive's lookup, which is the correct resolution +// but leaves nothing to report; this walk exists only to turn "not found" into +// a message that names the collision. +func duplicateIDPaths(arc *doc.Archive, id string) []string { + var paths []string + for _, p := range arc.All() { + if p.DocID == id { + paths = append(paths, p.Path) + } + } + return paths +} diff --git a/mcpsrv/hostguard_test.go b/mcpsrv/hostguard_test.go new file mode 100644 index 0000000000000000000000000000000000000000..f1ab9733422c723e6a737d9bb3ebf2317b41db6c --- /dev/null +++ b/mcpsrv/hostguard_test.go @@ -0,0 +1,83 @@ +package mcpsrv_test + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "sourcecraft.dev/bigbes/sr-ht-spec/mcpsrv" +) + +// The SDK's own DNS-rebinding guard is disabled (it cannot tell nginx from an +// attacker), so this replacement is the only thing protecting /mcp. These cases +// are the reason the bypass is defensible: a hostile Host is still refused, and +// the legitimate proxied Host that the SDK guard would have rejected is let +// through. +func TestHostGuard(t *testing.T) { + const origin = "https://spec.srht.bigb.es" + + for _, tc := range []struct { + name string + host string + want int + }{ + {"proxied real hostname", "spec.srht.bigb.es", http.StatusOK}, + {"proxied with port", "spec.srht.bigb.es:443", http.StatusOK}, + {"case-insensitive", "SPEC.SRHT.BIGB.ES", http.StatusOK}, + {"loopback for dev", "127.0.0.1:5091", http.StatusOK}, + {"localhost for dev", "localhost:5091", http.StatusOK}, + {"rebinding attacker domain", "evil.example.com", http.StatusForbidden}, + {"attacker subdomain of us", "spec.srht.bigb.es.evil.com", http.StatusForbidden}, + {"another srht service", "git.srht.bigb.es", http.StatusForbidden}, + } { + t.Run(tc.name, func(t *testing.T) { + r, s := newFixture() + h, err := mcpsrv.Handler(mcpsrv.Backend{Docs: r, Index: s}, "test", origin) + if err != nil { + t.Fatalf("Handler: %v", err) + } + + req := httptest.NewRequest(http.MethodPost, "/mcp", + strings.NewReader(`{"jsonrpc":"2.0","id":1,"method":"initialize"}`)) + req.Host = tc.host + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + + if tc.want == http.StatusForbidden && w.Code != http.StatusForbidden { + t.Fatalf("Host %q got %d, want 403 — the guard did not block it", tc.host, w.Code) + } + if tc.want == http.StatusOK && w.Code == http.StatusForbidden { + t.Fatalf("Host %q got 403 — the guard blocked a legitimate request", tc.host) + } + }) + } +} + +// An unusable origin must leave a loud trail rather than silently unguarding the +// endpoint. It still serves (refusing to start over a config typo is worse), but +// Handler logs a warning; this pins that it does not instead start refusing +// every request, which would look like the guard "working". +func TestHostGuardWithoutOriginStillServes(t *testing.T) { + r, s := newFixture() + h, err := mcpsrv.Handler(mcpsrv.Backend{Docs: r, Index: s}, "test", "") + if err != nil { + t.Fatalf("Handler: %v", err) + } + + req := httptest.NewRequest(http.MethodPost, "/mcp", + strings.NewReader(`{"jsonrpc":"2.0","id":1,"method":"initialize"}`)) + req.Host = "anything.example.com" + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + + if w.Code == http.StatusForbidden { + t.Fatalf("unconfigured origin should not 403 every request; got %d", w.Code) + } +} diff --git a/mcpsrv/http_test.go b/mcpsrv/http_test.go new file mode 100644 index 0000000000000000000000000000000000000000..03c55d5b7c90f1d75056e83a07e87cceadf1f709 --- /dev/null +++ b/mcpsrv/http_test.go @@ -0,0 +1,85 @@ +package mcpsrv_test + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/go-chi/chi/v5" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/require" + + "sourcecraft.dev/bigbes/sr-ht-spec/mcpsrv" +) + +// The transport the design settled on: streamable HTTP on the same chi router +// at /mcp, so that one listener and one nginx block cover the whole service. +// This is the exact mounting call cmd/specsrht makes. +func TestHandlerMountsOnChiAtMCP(t *testing.T) { + r, s := newFixture() + + h, err := mcpsrv.Handler(mcpsrv.Backend{Docs: r, Index: s}, "test", "https://spec.srht.bigb.es") + require.NoError(t, err) + + router := chi.NewRouter() + router.Handle("/mcp", h) + + srv := httptest.NewServer(router) + t.Cleanup(srv.Close) + + client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "test"}, nil) + session, err := client.Connect(context.Background(), + &mcp.StreamableClientTransport{Endpoint: srv.URL + "/mcp"}, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = session.Close() }) + + require.Equal(t, mcpsrv.ServerName, session.InitializeResult().ServerInfo.Name) + + res, err := session.CallTool(context.Background(), &mcp.CallToolParams{ + Name: "spec_read", + Arguments: map[string]any{"space": "~bigbes/rfcs", "document": "SPEC-0007"}, + }) + require.NoError(t, err) + var out readResult + decode(t, res, &out) + require.Contains(t, out.Markdown, "the approved body") + require.Equal(t, approvedRev, out.Rev) +} + +// The deployment shape, pinned by a test because it is invisible otherwise: +// the daemon listens on loopback and nginx forwards with the public Host +// (`proxy_set_header Host $host`). The SDK's DNS-rebinding guard rejects +// exactly that combination with a 403, so Handler disables it. Without this +// test the whole MCP endpoint would 403 in production and pass every local +// check, since a local client sends a loopback Host. +func TestHandlerAcceptsProxiedHostHeader(t *testing.T) { + r, s := newFixture() + + h, err := mcpsrv.Handler(mcpsrv.Backend{Docs: r, Index: s}, "test", "https://spec.srht.bigb.es") + require.NoError(t, err) + + router := chi.NewRouter() + router.Handle("/mcp", h) + srv := httptest.NewServer(router) + t.Cleanup(srv.Close) + + body := `{"jsonrpc":"2.0","id":1,"method":"initialize","params":{` + + `"protocolVersion":"2025-06-18","capabilities":{},` + + `"clientInfo":{"name":"test-client","version":"test"}}}` + + req, err := http.NewRequest(http.MethodPost, srv.URL+"/mcp", io.Reader(strings.NewReader(body))) + require.NoError(t, err) + req.Host = "spec.srht.bigb.es" + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + + resp, err := srv.Client().Do(req) + require.NoError(t, err) + t.Cleanup(func() { _ = resp.Body.Close() }) + + require.Equal(t, http.StatusOK, resp.StatusCode, + "a loopback listener behind a proxy that sets the public Host must not be refused") +} diff --git a/mcpsrv/list.go b/mcpsrv/list.go new file mode 100644 index 0000000000000000000000000000000000000000..fc3b2129e5d55bf995ed12446d9f7411df67642d --- /dev/null +++ b/mcpsrv/list.go @@ -0,0 +1,111 @@ +package mcpsrv + +import ( + "context" + "errors" + "strings" +) + +type listInput struct { + Space string `json:"space,omitempty" jsonschema:"the space to list documents from, written \"~owner/name\". Omit it to list the spaces on this instance instead."` + Rev string `json:"rev,omitempty" jsonschema:"pin the listing to one immutable revision, given as a git object name (lowercase hex). Omit to list the space's approved head. Requires space."` +} + +type spaceEntry struct { + // Space is the name every other tool takes: "~owner/name". + Space string `json:"space"` + Owner string `json:"owner"` + Name string `json:"name"` +} + +type documentEntry struct { + // ID is how spec_read addresses this document. + ID string `json:"id"` + DocID string `json:"doc_id,omitempty"` + Path string `json:"path"` + Blob string `json:"blob,omitempty"` + Title string `json:"title,omitempty"` + // Section is the top-level directory the document lives under, and is what + // spec_search's sections filter takes. + Section string `json:"section,omitempty"` + // Status is the authored lifecycle marker, not approval state. + Status string `json:"status,omitempty"` + Summary string `json:"summary,omitempty"` + Tags []string `json:"tags,omitempty"` +} + +// listOutput carries whichever of the two listings was asked for. One tool +// rather than two because the answer to "what can I read" is one question with +// a drill-down, and an agent that has just been handed a space name should not +// have to find a second tool to use it. +type listOutput struct { + // Spaces is set when no space was named. + Spaces []spaceEntry `json:"spaces,omitempty"` + // Space, Rev and Documents are set when one was. + Space string `json:"space,omitempty"` + // Rev is the revision listed, resolved to an immutable commit. Pass it to + // spec_read to read any of these documents at exactly this revision. + Rev string `json:"rev,omitempty"` + Documents []documentEntry `json:"documents,omitempty"` +} + +func listHandler(ctx context.Context, b Backend, in listInput) (listOutput, error) { + if strings.TrimSpace(in.Space) == "" { + // A rev with no space is a caller that meant to name one. Ignoring it + // would answer a different question than was asked and look like it + // had worked. + if strings.TrimSpace(in.Rev) != "" { + return listOutput{}, errors.New("rev names a revision of a space; pass space as well, or omit rev to list spaces") + } + return listSpaces(ctx, b) + } + + ref, err := parseSpace(in.Space) + if err != nil { + return listOutput{}, err + } + rev, err := parseRev(in.Rev) + if err != nil { + return listOutput{}, err + } + sp, err := b.Docs.OpenSpace(ctx, ref) + if err != nil { + return listOutput{}, err + } + arc, _, resolved, err := archiveAt(ctx, b, sp, rev) + if err != nil { + return listOutput{}, err + } + + out := listOutput{Space: ref.String(), Rev: resolved, Documents: make([]documentEntry, 0, len(arc.All()))} + for _, p := range arc.All() { + out.Documents = append(out.Documents, documentEntry{ + ID: p.ID, + DocID: p.DocID, + Path: p.Path, + Blob: p.Blob, + Title: p.Title, + Section: p.Section, + Status: string(p.Status), + Summary: p.Summary, + Tags: p.Tags, + }) + } + return out, nil +} + +func listSpaces(ctx context.Context, b Backend) (listOutput, error) { + spaces, err := b.Docs.ListSpaces(ctx) + if err != nil { + return listOutput{}, err + } + out := listOutput{Spaces: make([]spaceEntry, 0, len(spaces))} + for _, sp := range spaces { + out.Spaces = append(out.Spaces, spaceEntry{ + Space: sp.Ref.String(), + Owner: sp.Ref.Owner, + Name: sp.Ref.Name, + }) + } + return out, nil +} diff --git a/mcpsrv/mcpsrv.go b/mcpsrv/mcpsrv.go new file mode 100644 index 0000000000000000000000000000000000000000..f8a565c6503ef375e3bb7299038d6015b8273198 --- /dev/null +++ b/mcpsrv/mcpsrv.go @@ -0,0 +1,254 @@ +// Package mcpsrv is spec.sr.ht's Model Context Protocol surface: the tools an +// agent calls to find and read approved documents. +// +// It is warren's mcpsrv/ absorbed, and it keeps warren's two structural +// choices — a narrow backend interface the tools are written against, and the +// SDK's generic AddTool deriving every schema from a Go struct — while +// discarding everything that assumed a local vault: the vault-wide id space, +// the parent/child tools, and the hybrid keyword+semantic mode argument (vector +// search is Phase 5 here). +// +// # A surface, not a wrapper +// +// The design's rule is that MCP is a first-class surface and that its tools +// call the same resolver layer as REST, GraphQL and the web UI rather than a +// parallel implementation. That is what this package is: every lookup goes +// through service/, every addressing decision through doc.Archive, and every +// query through search.Index. Nothing here re-derives which document an id +// names or which spaces a project covers, because a second implementation of +// those rules is how two surfaces start answering the same question +// differently — silently, and months later. +// +// # The read contract is the reason this exists +// +// Bots need "the approved text of SPEC-0007", not "whatever a branch points at +// while another agent rewrites it". So: +// +// - Omitting rev reads the space's approved head. That is the default, +// and it is service.ApprovedRev — the same default every other surface has. +// - Passing rev pins the read to one immutable revision, and rev must be an +// object name: 7-64 lowercase hex, the same grammar the design pins for +// X-Agent-Base. Ref names are refused outright, which is what makes +// "read a proposal branch" impossible to reach by accident or by +// mistyping — an agent can only reach unapproved content by naming a +// commit sha it had to obtain deliberately, since neither spec_search nor +// 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. +// +// # Layering +// +// Reader and Searcher are declared here rather than imported as concrete types +// so the tools can be tested without a repository, a Postgres instance or a +// bleve index. *service.Service satisfies Reader and *search.Index satisfies +// Searcher, structurally, with no adapter. +package mcpsrv + +import ( + "context" + "errors" + "log/slog" + "net" + "net/http" + "net/url" + "strings" + + "github.com/modelcontextprotocol/go-sdk/mcp" + + "sourcecraft.dev/bigbes/sr-ht-spec/search" +) + +// ServerName is the implementation name reported in the MCP handshake. It is +// the service's config-section name, so a client listing several SourceHut MCP +// endpoints sees which one it is talking to. +const ServerName = "spec.sr.ht" + +// maxSearchLimit caps how many hits one call may ask for. +const maxSearchLimit = 100 + +// Backend is everything the tools read through. Both halves are required: a +// nil one is a wiring mistake, and New reports it at startup rather than +// letting the first tool call panic inside a request. +type Backend struct { + // Docs is the orchestration layer — in production *service.Service. + Docs Reader + // Index is the one global bleve index — in production *search.Index. + Index Searcher +} + +// New builds the MCP server with the Phase 2 read tools registered. version is +// reported as the implementation version in the handshake. +// +// The returned server is not connected to a transport; Handler wires it to +// streamable HTTP, and a caller that wants stdio can call Run itself. +func New(b Backend, version string) (*mcp.Server, error) { + if b.Docs == nil { + return nil, errors.New("mcpsrv: backend has no document reader") + } + if b.Index == nil { + return nil, errors.New("mcpsrv: backend has no search index") + } + + srv := mcp.NewServer(&mcp.Implementation{Name: ServerName, Version: version}, nil) + readOnly := &mcp.ToolAnnotations{ReadOnlyHint: true, IdempotentHint: true} + + mcp.AddTool(srv, &mcp.Tool{ + Name: "spec_search", + Annotations: readOnly, + Description: "Search every approved document on this instance, ranked. Each hit carries " + + "the space, the document id, its path, the revision it was indexed at, its title " + + "and a plain-text snippet — enough to fetch it with spec_read without a second " + + "lookup.\n\n" + + "Pass `spaces` to restrict the search to a set of spaces: that set is what a " + + "project is on this service — a saved filter over one global index, not a " + + "container. Omitting it searches everything, which is the meta-project.\n\n" + + "Results come from the approved revision of each space. Proposal branches are " + + "not indexed and never appear here.", + }, func(ctx context.Context, _ *mcp.CallToolRequest, in searchInput) (*mcp.CallToolResult, searchOutput, error) { + out, err := searchHandler(ctx, b, in) + return nil, out, err + }) + + mcp.AddTool(srv, &mcp.Tool{ + Name: "spec_read", + Annotations: readOnly, + Description: "Read one document's markdown, frontmatter included, exactly as it is stored.\n\n" + + "By default this returns the space's APPROVED text — the reviewed, canonical " + + "revision — and reports the revision it resolved to in `rev`. Pass that value " + + "back as the `rev` argument later to re-read the identical bytes; a revision, " + + "once named, is immutable.\n\n" + + "Address the document by its frontmatter id (\"SPEC-0007\") when it has a " + + "well-formed one that no other document in the space claims, and otherwise by " + + "its path, with or without the \".md\" extension. A document whose id is " + + "duplicated within its space resolves to neither document and is reported as " + + "ambiguous rather than guessed at.", + }, func(ctx context.Context, _ *mcp.CallToolRequest, in readInput) (*mcp.CallToolResult, readOutput, error) { + out, err := readHandler(ctx, b, in) + return nil, out, err + }) + + mcp.AddTool(srv, &mcp.Tool{ + Name: "spec_list", + Annotations: readOnly, + Description: "List spaces, or list the documents in one space.\n\n" + + "Omit `space` to get every space on the instance — those names are what " + + "spec_search's `spaces` filter takes. Pass `space` to get that space's " + + "documents at its approved head, each with the id spec_read addresses it by, " + + "its path, title, section and authored status. Pass `rev` as well to list a " + + "pinned revision instead.", + }, func(ctx context.Context, _ *mcp.CallToolRequest, in listInput) (*mcp.CallToolResult, listOutput, error) { + out, err := listHandler(ctx, b, in) + return nil, out, err + }) + + return srv, nil +} + +// Handler mounts the server on streamable HTTP. cmd/specsrht hangs it off the +// chi router at /mcp, which is what keeps MCP to one listener and one nginx +// block rather than a second port. +// +// The SDK's DNS-rebinding guard is disabled deliberately, and the reason is +// worth stating because disabling a security default usually is not. +// +// The guard rejects any request arriving on a loopback address that carries a +// non-loopback Host header. That is precisely our deployment: the daemon binds +// 127.0.0.1:5091 and nginx forwards with `proxy_set_header Host $host`, so +// every genuine request would 403 — and it would 403 only in production, +// because a local client sends a loopback Host and passes. +// +// It is not that the guard has nothing to catch. A browser running on the +// daemon's own host could reach 127.0.0.1:5091 directly with an attacker's +// Host header, which is the attack the guard is for. The problem is that the +// guard cannot tell that request from nginx's: both arrive from loopback +// carrying a Host that is not loopback, and the SDK exposes no allowlist to +// separate them. +// +// So the guard is disabled and REPLACED, in the same constructor, by allowHosts +// below — a stricter check than the one removed. The SDK asks only "is Host +// loopback?"; we require Host to equal this instance's configured origin. A +// rebinding attack carries the attacker's name in Host and fails that; nginx +// forwards our real hostname and passes. Disabling the SDK guard without this +// replacement would be a genuine regression, not a formality. +func Handler(b Backend, version, origin string) (http.Handler, error) { + srv, err := New(b, version) + if err != nil { + return nil, err + } + h := mcp.NewStreamableHTTPHandler( + func(*http.Request) *mcp.Server { return srv }, + &mcp.StreamableHTTPOptions{DisableLocalhostProtection: true}, + ) + return allowHosts(h, origin), nil +} + +// allowHosts is this endpoint's DNS-rebinding protection, in the form the +// deployment actually needs: Host must be the service's own origin hostname, or +// a loopback name for local development. +// +// An empty or unparseable origin leaves the endpoint unguarded, so it says so +// loudly. A misconfigured origin must not quietly become the difference between +// protected and open — that is the class of failure nobody discovers. +func allowHosts(next http.Handler, origin string) http.Handler { + want := originHost(origin) + if want == "" { + slog.Warn("mcpsrv: no usable origin configured; Host validation on /mcp is DISABLED") + return next + } + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !hostAllowed(r.Host, want) { + http.Error(w, "Forbidden: unexpected Host header", http.StatusForbidden) + return + } + next.ServeHTTP(w, r) + }) +} + +// hostAllowed compares a request's Host against the expected hostname, ignoring +// any port and IPv6 brackets. Loopback names stay allowed so `make run-dev` and +// a local MCP client keep working. +func hostAllowed(reqHost, want string) bool { + h := reqHost + if stripped, _, err := net.SplitHostPort(h); err == nil { + h = stripped + } + h = strings.TrimSuffix(strings.TrimPrefix(h, "["), "]") + switch { + case strings.EqualFold(h, want): + return true + case h == "localhost", h == "127.0.0.1", h == "::1": + return true + default: + return false + } +} + +// originHost extracts the hostname from a configured origin URL. +func originHost(origin string) string { + if origin == "" { + return "" + } + u, err := url.Parse(origin) + if err != nil { + return "" + } + return u.Hostname() +} + +// clampLimit applies the hit-count policy: unset defers to search's own +// default rather than restating it, and anything larger than maxSearchLimit is +// clamped. A tool result is a context window, so an agent asking for a thousand +// hits is asking for something it cannot use. +func clampLimit(n int) int { + switch { + case n <= 0: + return search.DefaultLimit + case n > maxSearchLimit: + return maxSearchLimit + default: + return n + } +} diff --git a/mcpsrv/mcpsrv_test.go b/mcpsrv/mcpsrv_test.go new file mode 100644 index 0000000000000000000000000000000000000000..75c06163c8a9983f9d5a96d6514995b68b8869f7 --- /dev/null +++ b/mcpsrv/mcpsrv_test.go @@ -0,0 +1,627 @@ +package mcpsrv_test + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "sort" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/require" + + "sourcecraft.dev/bigbes/sr-ht-spec/core" + "sourcecraft.dev/bigbes/sr-ht-spec/mcpsrv" + "sourcecraft.dev/bigbes/sr-ht-spec/search" + "sourcecraft.dev/bigbes/sr-ht-spec/service" +) + +// --- fixtures --------------------------------------------------------------- + +// The two revisions every read test is written against: an approved head whose +// SPEC-0007 says "approved", and an older pinned revision whose SPEC-0007 says +// something else. Nothing distinguishes them but the revision, which is the +// point — one storage tier, one code path, a different ref. +const ( + approvedRev = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + olderRev = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + // proposalRev is a real commit sha that happens to be a proposal branch + // tip. It is readable only because the caller named it. + proposalRev = "cccccccccccccccccccccccccccccccccccccccc" +) + +var fxSpace = core.SpaceRef{Owner: "bigbes", Name: "rfcs"} + +func blob(n int) string { return fmt.Sprintf("%040x", n) } + +func md(id, title, status, body string) []byte { + return []byte(fmt.Sprintf("---\nid: %s\ntitle: %s\nstatus: %s\n---\n\n%s\n", id, title, status, body)) +} + +// fakeReader stands in for *service.Service: a document set per revision, and +// nothing else. It holds no repository, which is the property that makes these +// tests run without git. +type fakeReader struct { + spaces []*service.Space + revs map[string][]service.Document + approved string + + openErr error + listErr error + lastRevs []string +} + +func (f *fakeReader) ListSpaces(context.Context) ([]*service.Space, error) { + return f.spaces, nil +} + +func (f *fakeReader) OpenSpace(_ context.Context, ref core.SpaceRef) (*service.Space, error) { + if f.openErr != nil { + return nil, f.openErr + } + for _, sp := range f.spaces { + if sp.Ref == ref { + return sp, nil + } + } + return nil, fmt.Errorf("%w: space %s", service.ErrNotFound, ref) +} + +func (f *fakeReader) ResolveRev(_ context.Context, _ *service.Space, rev string) (string, error) { + if rev == service.ApprovedRev { + return f.approved, nil + } + if _, ok := f.revs[rev]; !ok { + return "", fmt.Errorf("%w: revision %q", service.ErrNotFound, rev) + } + return rev, nil +} + +func (f *fakeReader) ListDocuments(_ context.Context, _ *service.Space, rev string) ([]service.Document, error) { + if f.listErr != nil { + return nil, f.listErr + } + f.lastRevs = append(f.lastRevs, rev) + docs, ok := f.revs[rev] + if !ok { + return nil, fmt.Errorf("%w: revision %q", service.ErrNotFound, rev) + } + return docs, nil +} + +type fakeSearcher struct { + last search.Query + calls int + results search.Results + err error +} + +func (f *fakeSearcher) Search(_ context.Context, q search.Query) (search.Results, error) { + f.last = q + f.calls++ + return f.results, f.err +} + +// newFixture builds the standard two-revision, two-space backend. +func newFixture() (*fakeReader, *fakeSearcher) { + doc := func(path string, data []byte, rev string, n int) service.Document { + return service.Document{Path: path, Blob: blob(n), Rev: rev, Data: data} + } + r := &fakeReader{ + spaces: []*service.Space{ + {Ref: fxSpace, ID: 1}, + {Ref: core.SpaceRef{Owner: "bigbes", Name: "notes"}, ID: 2}, + }, + approved: approvedRev, + revs: map[string][]service.Document{ + approvedRev: { + doc("specs/0007-storage.md", md("SPEC-0007", "Storage model", "review", "the approved body"), approvedRev, 1), + doc("notes/untitled.md", []byte("# Loose note\n\nno frontmatter here\n"), approvedRev, 2), + }, + olderRev: { + doc("specs/0007-storage.md", md("SPEC-0007", "Storage model", "draft", "the older body"), olderRev, 3), + }, + proposalRev: { + doc("specs/0007-storage.md", md("SPEC-0007", "Storage model", "draft", "unreviewed proposal body"), proposalRev, 4), + }, + }, + } + return r, &fakeSearcher{} +} + +// --- plumbing --------------------------------------------------------------- + +func connect(t *testing.T, r mcpsrv.Reader, s mcpsrv.Searcher) *mcp.ClientSession { + t.Helper() + ctx := context.Background() + serverTransport, clientTransport := mcp.NewInMemoryTransports() + + srv, err := mcpsrv.New(mcpsrv.Backend{Docs: r, Index: s}, "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 call(t *testing.T, s *mcp.ClientSession, name string, args map[string]any) *mcp.CallToolResult { + t.Helper() + res, err := s.CallTool(context.Background(), &mcp.CallToolParams{Name: name, Arguments: args}) + require.NoError(t, err, "protocol-level failure calling %s", name) + return res +} + +func decode(t *testing.T, res *mcp.CallToolResult, out any) { + t.Helper() + require.False(t, res.IsError, "unexpected tool error: %s", errorText(res)) + require.NotNil(t, res.StructuredContent, "no structured output") + raw, err := json.Marshal(res.StructuredContent) + require.NoError(t, err) + require.NoError(t, json.Unmarshal(raw, out)) +} + +func errorText(res *mcp.CallToolResult) string { + var s string + for _, c := range res.Content { + if tc, ok := c.(*mcp.TextContent); ok { + s += tc.Text + } + } + return s +} + +type readResult struct { + Space string `json:"space"` + ID string `json:"id"` + DocID string `json:"doc_id"` + Path string `json:"path"` + Rev string `json:"rev"` + Blob string `json:"blob"` + Pinned bool `json:"pinned"` + Title string `json:"title"` + Section string `json:"section"` + Status string `json:"status"` + Tags []string `json:"tags"` + Markdown string `json:"markdown"` +} + +// --- the read contract ------------------------------------------------------ + +// The default read is the approved head. This is the property the whole service +// exists for: an agent that names no revision must never be handed unreviewed +// text. +func TestReadDefaultsToApprovedHead(t *testing.T) { + r, s := newFixture() + session := connect(t, r, s) + + res := call(t, session, "spec_read", map[string]any{ + "space": "~bigbes/rfcs", "document": "SPEC-0007", + }) + var out readResult + decode(t, res, &out) + + require.Contains(t, out.Markdown, "the approved body") + require.NotContains(t, out.Markdown, "older body") + require.Equal(t, approvedRev, out.Rev, "the approved head is reported so the caller can pin it") + require.False(t, out.Pinned) + require.Equal(t, "SPEC-0007", out.ID) + require.Equal(t, "SPEC-0007", out.DocID) + require.Equal(t, "specs/0007-storage.md", out.Path) + require.Equal(t, blob(1), out.Blob) + require.Equal(t, "review", out.Status, "status is authored metadata, not approval state") + + // Everything below the tool read the resolved sha, never the empty string: + // a merge landing mid-read cannot make one answer describe two revisions. + require.Equal(t, []string{approvedRev}, r.lastRevs) +} + +// A pinned rev returns that revision, not the head. +func TestReadPinnedRevReturnsThatRevision(t *testing.T) { + r, s := newFixture() + session := connect(t, r, s) + + res := call(t, session, "spec_read", map[string]any{ + "space": "~bigbes/rfcs", "document": "SPEC-0007", "rev": olderRev, + }) + var out readResult + decode(t, res, &out) + + require.Contains(t, out.Markdown, "the older body") + require.Equal(t, olderRev, out.Rev) + require.True(t, out.Pinned) + require.Equal(t, "draft", out.Status) + require.Equal(t, []string{olderRev}, r.lastRevs) +} + +// Proposal content is reachable only by naming its commit — never by naming a +// branch. This is the guard that keeps unreviewed text out of an agent's +// context by accident. +func TestReadRefusesRefNames(t *testing.T) { + r, s := newFixture() + session := connect(t, r, s) + + for _, rev := range []string{"proposals/42", "main", "HEAD", "cafe"} { + res := call(t, session, "spec_read", map[string]any{ + "space": "~bigbes/rfcs", "document": "SPEC-0007", "rev": rev, + }) + require.True(t, res.IsError, "rev %q was accepted", rev) + require.Contains(t, errorText(res), "object name") + require.Contains(t, errorText(res), "approved head") + } + require.Empty(t, r.lastRevs, "a rejected rev never reaches the service layer") + + // Naming the commit itself is deliberate, and works. + res := call(t, session, "spec_read", map[string]any{ + "space": "~bigbes/rfcs", "document": "SPEC-0007", "rev": proposalRev, + }) + var out readResult + decode(t, res, &out) + require.Contains(t, out.Markdown, "unreviewed proposal body") + require.True(t, out.Pinned) +} + +// The design's addressing rule, both halves: id when it is well-formed and +// unique, path when there is no usable id. +func TestReadAddressing(t *testing.T) { + r, s := newFixture() + session := connect(t, r, s) + + cases := []struct{ name, document, wantID, wantPath string }{ + {"by id", "SPEC-0007", "SPEC-0007", "specs/0007-storage.md"}, + {"by path", "specs/0007-storage.md", "SPEC-0007", "specs/0007-storage.md"}, + {"by extensionless path", "specs/0007-storage", "SPEC-0007", "specs/0007-storage.md"}, + {"id-less document by path", "notes/untitled", "notes/untitled", "notes/untitled.md"}, + {"id-less document by path with extension", "notes/untitled.md", "notes/untitled", "notes/untitled.md"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + res := call(t, session, "spec_read", map[string]any{ + "space": "~bigbes/rfcs", "document": c.document, + }) + var out readResult + decode(t, res, &out) + require.Equal(t, c.wantID, out.ID) + require.Equal(t, c.wantPath, out.Path) + }) + } + + // A document with no frontmatter id reports none rather than inventing one + // from its path. + res := call(t, session, "spec_read", map[string]any{ + "space": "~bigbes/rfcs", "document": "notes/untitled", + }) + var out readResult + decode(t, res, &out) + require.Empty(t, out.DocID) +} + +// A duplicated id resolves to neither document, and says which two claim it. +func TestReadDuplicateIDIsAmbiguous(t *testing.T) { + r, s := newFixture() + r.revs[approvedRev] = []service.Document{ + {Path: "specs/a.md", Blob: blob(5), Rev: approvedRev, Data: md("SPEC-0007", "A", "draft", "a")}, + {Path: "specs/b.md", Blob: blob(6), Rev: approvedRev, Data: md("SPEC-0007", "B", "draft", "b")}, + } + session := connect(t, r, s) + + res := call(t, session, "spec_read", map[string]any{ + "space": "~bigbes/rfcs", "document": "SPEC-0007", + }) + require.True(t, res.IsError) + require.Contains(t, errorText(res), "claimed by 2 documents") + require.Contains(t, errorText(res), "specs/a.md") + require.Contains(t, errorText(res), "specs/b.md") + + // Both are still readable by path: a duplicated id is excluded from id + // resolution, not from the archive. + var out readResult + decode(t, call(t, session, "spec_read", map[string]any{ + "space": "~bigbes/rfcs", "document": "specs/a.md", + }), &out) + require.Equal(t, "A", out.Title) +} + +// A missing document, space or revision is a tool error with a usable message, +// never a panic and never an empty document. +func TestReadMissing(t *testing.T) { + r, s := newFixture() + session := connect(t, r, s) + + res := call(t, session, "spec_read", map[string]any{ + "space": "~bigbes/rfcs", "document": "SPEC-9999", + }) + require.True(t, res.IsError) + require.Contains(t, errorText(res), `no document "SPEC-9999"`) + require.Contains(t, errorText(res), "~bigbes/rfcs") + + res = call(t, session, "spec_read", map[string]any{ + "space": "~bigbes/nope", "document": "SPEC-0007", + }) + require.True(t, res.IsError) + require.Contains(t, errorText(res), "not found") + + res = call(t, session, "spec_read", map[string]any{ + "space": "~bigbes/rfcs", "document": "SPEC-0007", "rev": blob(99), + }) + require.True(t, res.IsError) + require.Contains(t, errorText(res), "not found") + + res = call(t, session, "spec_read", map[string]any{"space": "", "document": "SPEC-0007"}) + require.True(t, res.IsError) + require.Contains(t, errorText(res), "space must not be empty") + + res = call(t, session, "spec_read", map[string]any{"space": "~bigbes/rfcs", "document": " "}) + require.True(t, res.IsError) + require.Contains(t, errorText(res), "document must not be empty") +} + +// A failure below service/ surfaces as a tool error rather than taking the +// session down. +func TestReadBackendFailure(t *testing.T) { + r, s := newFixture() + r.listErr = errors.New("git object store is on fire") + session := connect(t, r, s) + + res := call(t, session, "spec_read", map[string]any{ + "space": "~bigbes/rfcs", "document": "SPEC-0007", + }) + require.True(t, res.IsError) + require.Contains(t, errorText(res), "on fire") +} + +// --- search ----------------------------------------------------------------- + +type searchResult struct { + Hits []struct { + Space string `json:"space"` + ID string `json:"id"` + Path string `json:"path"` + Rev string `json:"rev"` + Anchor string `json:"anchor"` + Title string `json:"title"` + Section string `json:"section"` + Score float64 `json:"score"` + Snippet string `json:"snippet"` + } `json:"hits"` + Total uint64 `json:"total"` +} + +// The spaces argument is the project filter, and it reaches the index as one. +func TestSearchSpaceFilter(t *testing.T) { + r, s := newFixture() + session := connect(t, r, s) + + call(t, session, "spec_search", map[string]any{ + "query": "storage", + "spaces": []string{"~bigbes/rfcs", "bigbes/notes"}, + }) + require.Equal(t, []core.SpaceRef{ + {Owner: "bigbes", Name: "rfcs"}, + {Owner: "bigbes", Name: "notes"}, + }, s.last.Spaces) + + // Omitting it is the meta-project: a filter that excludes nothing. + call(t, session, "spec_search", map[string]any{"query": "storage"}) + require.Nil(t, s.last.Spaces) + + // Sections pass through the same way. + call(t, session, "spec_search", map[string]any{"query": "storage", "sections": []string{"specs"}}) + require.Equal(t, []string{"specs"}, s.last.Sections) +} + +// An unparseable space is refused rather than dropped: a dropped filter term +// silently widens the search past what was asked for. +func TestSearchRejectsBadSpace(t *testing.T) { + r, s := newFixture() + session := connect(t, r, s) + + res := call(t, session, "spec_search", map[string]any{ + "query": "storage", "spaces": []string{"~bigbes/rfcs", "not a space ref"}, + }) + require.True(t, res.IsError) + require.Contains(t, errorText(res), "not a space ref") + require.Zero(t, s.calls, "nothing was searched") + + res = call(t, session, "spec_search", map[string]any{"query": " "}) + require.True(t, res.IsError) + require.Contains(t, errorText(res), "query must not be empty") + require.Zero(t, s.calls) +} + +func TestSearchLimit(t *testing.T) { + r, s := newFixture() + session := connect(t, r, s) + + call(t, session, "spec_search", map[string]any{"query": "q"}) + require.Equal(t, search.DefaultLimit, s.last.Limit) + + call(t, session, "spec_search", map[string]any{"query": "q", "limit": 5, "offset": 10}) + require.Equal(t, 5, s.last.Limit) + require.Equal(t, 10, s.last.Offset) + + call(t, session, "spec_search", map[string]any{"query": "q", "limit": 100000}) + require.Equal(t, 100, s.last.Limit) +} + +// Hits carry everything spec_read needs, the snippet is plain text, and a log +// entry's id is one spec_read accepts. +func TestSearchHitShape(t *testing.T) { + r, s := newFixture() + s.results = search.Results{ + Total: 2, + Hits: []search.Hit{ + { + Space: fxSpace, ID: "SPEC-0007", Rev: approvedRev, + Path: "specs/0007-storage.md", Title: "Storage model", Section: "specs", + Score: 1.5, Snippet: "the approved body & nothing else", + }, + { + Space: fxSpace, ID: "notes/dev-log#2026-05-31-1", Rev: approvedRev, + Path: "notes/dev-log.md", Anchor: "2026-05-31-shipped", Section: "log", + Title: "2026-05-31 shipped", Score: 0.9, + }, + }, + } + session := connect(t, r, s) + + var out searchResult + decode(t, call(t, session, "spec_search", map[string]any{"query": "approved"}), &out) + + require.Equal(t, uint64(2), out.Total) + require.Len(t, out.Hits, 2) + + h := out.Hits[0] + require.Equal(t, "~bigbes/rfcs", h.Space) + require.Equal(t, "SPEC-0007", h.ID) + require.Equal(t, "specs/0007-storage.md", h.Path) + require.Equal(t, approvedRev, h.Rev) + require.Equal(t, "the approved body & nothing else", h.Snippet, + "the snippet is plain text: no , no HTML entities") + + // The indexed id of a log entry carries the entry suffix; the id reported + // is the document one, and the entry's position is the anchor. + require.Equal(t, "notes/dev-log", out.Hits[1].ID) + require.Equal(t, "2026-05-31-shipped", out.Hits[1].Anchor) + + // And that id round-trips through spec_read. + r.revs[approvedRev] = append(r.revs[approvedRev], service.Document{ + Path: "notes/dev-log.md", Blob: blob(7), Rev: approvedRev, + Data: []byte("# Dev log\n\n## 2026-05-31 shipped\n\ndone\n"), + }) + var doc readResult + decode(t, call(t, session, "spec_read", map[string]any{ + "space": out.Hits[1].Space, "document": out.Hits[1].ID, + }), &doc) + require.Equal(t, "notes/dev-log.md", doc.Path) +} + +func TestSearchBackendFailure(t *testing.T) { + r, s := newFixture() + s.err = errors.New("index is closed") + session := connect(t, r, s) + + res := call(t, session, "spec_search", map[string]any{"query": "q"}) + require.True(t, res.IsError) + require.Contains(t, errorText(res), "index is closed") +} + +// --- list ------------------------------------------------------------------- + +type listResult struct { + Spaces []struct { + Space string `json:"space"` + Owner string `json:"owner"` + Name string `json:"name"` + } `json:"spaces"` + Space string `json:"space"` + Rev string `json:"rev"` + Documents []struct { + ID string `json:"id"` + DocID string `json:"doc_id"` + Path string `json:"path"` + Blob string `json:"blob"` + Title string `json:"title"` + Section string `json:"section"` + Status string `json:"status"` + } `json:"documents"` +} + +func TestListSpaces(t *testing.T) { + r, s := newFixture() + session := connect(t, r, s) + + var out listResult + decode(t, call(t, session, "spec_list", map[string]any{}), &out) + + require.Len(t, out.Spaces, 2) + require.Equal(t, "~bigbes/rfcs", out.Spaces[0].Space) + require.Equal(t, "bigbes", out.Spaces[0].Owner) + require.Equal(t, "rfcs", out.Spaces[0].Name) + require.Empty(t, out.Documents) +} + +func TestListDocuments(t *testing.T) { + r, s := newFixture() + session := connect(t, r, s) + + var out listResult + decode(t, call(t, session, "spec_list", map[string]any{"space": "~bigbes/rfcs"}), &out) + + require.Equal(t, "~bigbes/rfcs", out.Space) + require.Equal(t, approvedRev, out.Rev, "listing defaults to the approved head, like every other read") + require.Len(t, out.Documents, 2) + require.Empty(t, out.Spaces) + + byID := map[string]string{} + for _, d := range out.Documents { + byID[d.ID] = d.Path + } + ids := make([]string, 0, len(byID)) + for id := range byID { + ids = append(ids, id) + } + sort.Strings(ids) + require.Equal(t, []string{"SPEC-0007", "notes/untitled"}, ids) + require.Equal(t, "specs/0007-storage.md", byID["SPEC-0007"]) + + // A pinned revision lists that revision. + decode(t, call(t, session, "spec_list", map[string]any{ + "space": "~bigbes/rfcs", "rev": olderRev, + }), &out) + require.Equal(t, olderRev, out.Rev) + require.Len(t, out.Documents, 1) + require.Equal(t, "draft", out.Documents[0].Status) +} + +// A rev with no space is a caller that meant to name one. Answering the other +// question would look like it had worked. +func TestListRevWithoutSpace(t *testing.T) { + r, s := newFixture() + session := connect(t, r, s) + + res := call(t, session, "spec_list", map[string]any{"rev": approvedRev}) + require.True(t, res.IsError) + require.Contains(t, errorText(res), "pass space as well") + + res = call(t, session, "spec_list", map[string]any{"space": "~bigbes/rfcs", "rev": "proposals/42"}) + require.True(t, res.IsError) + require.Contains(t, errorText(res), "object name") +} + +// --- 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. +func TestOnlyReadToolsAreRegistered(t *testing.T) { + r, s := newFixture() + session := connect(t, r, s) + + var names []string + for tool, err := range session.Tools(context.Background(), nil) { + require.NoError(t, err) + names = append(names, tool.Name) + require.True(t, tool.Annotations.ReadOnlyHint, "%s is not annotated read-only", tool.Name) + require.NotEmpty(t, tool.Description) + } + sort.Strings(names) + require.Equal(t, []string{"spec_list", "spec_read", "spec_search"}, names) +} + +func TestNewRefusesAnIncompleteBackend(t *testing.T) { + r, s := newFixture() + + _, err := mcpsrv.New(mcpsrv.Backend{Index: s}, "test") + require.ErrorContains(t, err, "no document reader") + + _, err = mcpsrv.New(mcpsrv.Backend{Docs: r}, "test") + require.ErrorContains(t, err, "no search index") + + _, err = mcpsrv.Handler(mcpsrv.Backend{}, "test", "https://spec.srht.bigb.es") + require.Error(t, err) +} diff --git a/mcpsrv/read.go b/mcpsrv/read.go new file mode 100644 index 0000000000000000000000000000000000000000..c20941082d2e9147b351dcbec00f4b8bdec98f71 --- /dev/null +++ b/mcpsrv/read.go @@ -0,0 +1,97 @@ +package mcpsrv + +import ( + "context" + "fmt" + + "sourcecraft.dev/bigbes/sr-ht-spec/service" +) + +type readInput struct { + Space string `json:"space" jsonschema:"the space to read from, written \"~owner/name\" as spec_list and spec_search report it"` + Document string `json:"document" jsonschema:"which document: its frontmatter id (\"SPEC-0007\"), or its path with or without the \".md\" extension (\"specs/0007-storage\")"` + Rev string `json:"rev,omitempty" jsonschema:"pin the read to one immutable revision, given as a git object name (lowercase hex, as returned in the rev field of any result). Omit this to read the space's approved head, which is what an agent almost always wants: the reviewed text. Branch names are not accepted."` +} + +type readOutput struct { + Space string `json:"space"` + // ID is how this document is addressed: its frontmatter id when that is + // well-formed and unique in the space, otherwise its path without the + // extension. + ID string `json:"id"` + // DocID is the frontmatter id when the document has a well-formed one, and + // empty otherwise. It differs from ID exactly when the document has no + // usable id, which is the case an agent proposing an edit needs to see. + DocID string `json:"doc_id,omitempty"` + Path string `json:"path"` + // Rev is the commit this content was read at. It is immutable: pass it back + // as the rev argument to re-read these exact bytes. It is also the value + // the Phase 3 write plane takes as If-Match when it is an approved head. + Rev string `json:"rev"` + // Blob is the sha of this document's content, and changes only when the + // content does. + Blob string `json:"blob"` + // Pinned reports how Rev was chosen. False means the caller named no + // revision and this is the space's approved head as of this call — the + // reviewed, canonical text. True means the caller pinned a revision, and + // whether that revision is on the approved branch is the caller's business: + // this service does not claim it either way. + Pinned bool `json:"pinned"` + Title string `json:"title,omitempty"` + Section string `json:"section,omitempty"` + // Status is the document's authored lifecycle marker (draft, review, + // superseded). It is not approval state: a document is approved by being + // reachable from the approved ref, never by its frontmatter. + Status string `json:"status,omitempty"` + Summary string `json:"summary,omitempty"` + Tags []string `json:"tags,omitempty"` + // Markdown is the whole document, frontmatter included, exactly as stored. + // It is the text to edit and send back when proposing a change. + Markdown string `json:"markdown"` +} + +func readHandler(ctx context.Context, b Backend, in readInput) (readOutput, error) { + ref, err := parseSpace(in.Space) + if err != nil { + return readOutput{}, err + } + rev, err := parseRev(in.Rev) + if err != nil { + return readOutput{}, err + } + sp, err := b.Docs.OpenSpace(ctx, ref) + if err != nil { + return readOutput{}, err + } + arc, bodies, resolved, err := archiveAt(ctx, b, sp, rev) + if err != nil { + return readOutput{}, err + } + page, err := resolvePage(arc, in.Document) + if err != nil { + return readOutput{}, err + } + body, ok := bodies[page.Path] + if !ok { + // The archive is built from these very bodies, so a page without one + // is a broken invariant rather than a missing document. Returning an + // empty markdown field would be indistinguishable from an empty + // document. + return readOutput{}, fmt.Errorf("document %s at %s in %s has no body", page.Path, resolved, ref) + } + return readOutput{ + Space: ref.String(), + ID: page.ID, + DocID: page.DocID, + Path: page.Path, + Rev: resolved, + Blob: page.Blob, + Pinned: rev != service.ApprovedRev, + Title: page.Title, + Section: page.Section, + Status: string(page.Status), + Summary: page.Summary, + Tags: page.Tags, + Markdown: string(body), + }, nil +} diff --git a/mcpsrv/search.go b/mcpsrv/search.go new file mode 100644 index 0000000000000000000000000000000000000000..428a261d4140ec0efffacac1010ffd89ebed16b2 --- /dev/null +++ b/mcpsrv/search.go @@ -0,0 +1,169 @@ +package mcpsrv + +import ( + "context" + "errors" + "html" + "strings" + + "sourcecraft.dev/bigbes/sr-ht-spec/core" + "sourcecraft.dev/bigbes/sr-ht-spec/search" +) + +type searchInput struct { + Query string `json:"query" jsonschema:"what to search for; matched over document titles and body text in both English and Russian"` + // Spaces is the project filter. The design's "a project is a saved filter + // over one global index, not a container" is this argument and nothing + // else, which is why it is a space list rather than a project name: a + // project's membership is resolved by whoever holds the project row, and + // what search takes is the set it resolves to. + Spaces []string `json:"spaces,omitempty" jsonschema:"restrict the search to these spaces, each written \"~owner/name\" as spec_list reports them. This is the project filter: a project on this service is a named set of spaces. Omit to search every space."` + Sections []string `json:"sections,omitempty" jsonschema:"restrict the search to these top-level sections (\"specs\", \"notes\", \"reports\"). Omit to search every section except the dated activity log; name \"log\" to search that."` + Limit int `json:"limit,omitempty" jsonschema:"maximum number of hits to return, clamped to 1..100"` + Offset int `json:"offset,omitempty" jsonschema:"how many of the top-ranked hits to skip, for paging through a large result set"` +} + +// searchHit is one result as an agent sees it. +// +// It is search.Hit reshaped for a tool caller rather than the type itself: +// Snippet becomes plain text, and ID is split from Anchor so that every id +// reported here is one spec_read accepts. +type searchHit struct { + Space string `json:"space"` + // ID addresses the document in spec_read. + ID string `json:"id"` + // Path is the document's path in the space's git tree. + Path string `json:"path,omitempty"` + // Rev is the revision the document was indexed at — the approved head of + // its space at index time. Pass it to spec_read to pin the read to exactly + // what was searched. + Rev string `json:"rev,omitempty"` + // Anchor is the heading fragment a hit inside a dated activity log lands + // on. Empty for an ordinary document. + Anchor string `json:"anchor,omitempty"` + Title string `json:"title,omitempty"` + Section string `json:"section,omitempty"` + Score float64 `json:"score"` + // Snippet is the matching fragment as plain text. + Snippet string `json:"snippet,omitempty"` +} + +type searchOutput struct { + Hits []searchHit `json:"hits"` + // Total is how many documents matched, not how many are in Hits. + Total uint64 `json:"total"` +} + +func searchHandler(ctx context.Context, b Backend, in searchInput) (searchOutput, error) { + text := strings.TrimSpace(in.Query) + if text == "" { + return searchOutput{}, errors.New("query must not be empty") + } + spaces, err := parseSpaceFilter(in.Spaces) + if err != nil { + return searchOutput{}, err + } + sections, err := trimAll("section", in.Sections) + if err != nil { + return searchOutput{}, err + } + + res, err := b.Index.Search(ctx, search.Query{ + Text: text, + Spaces: spaces, + Sections: sections, + Limit: clampLimit(in.Limit), + Offset: in.Offset, + }) + if err != nil { + return searchOutput{}, err + } + + out := searchOutput{Hits: make([]searchHit, 0, len(res.Hits)), Total: res.Total} + for _, h := range res.Hits { + out.Hits = append(out.Hits, searchHit{ + Space: h.Space.String(), + ID: documentID(h.ID), + Path: h.Path, + Rev: h.Rev, + Anchor: h.Anchor, + Title: h.Title, + Section: h.Section, + Score: h.Score, + Snippet: plainSnippet(h.Snippet), + }) + } + return out, nil +} + +// parseSpaceFilter validates the project filter. An unparseable space is an +// error rather than a dropped filter term: dropping one would silently widen +// the search past the set the caller asked for, and a wider answer than +// requested is indistinguishable from a correct one. +func parseSpaceFilter(in []string) ([]core.SpaceRef, error) { + if len(in) == 0 { + return nil, nil + } + out := make([]core.SpaceRef, 0, len(in)) + for _, s := range in { + ref, err := parseSpace(s) + if err != nil { + return nil, err + } + out = append(out, ref) + } + return out, nil +} + +// trimAll trims each element and refuses an empty one. search/ rejects an empty +// filter term outright; catching it here names the argument that carried it. +func trimAll(what string, in []string) ([]string, error) { + if len(in) == 0 { + return nil, nil + } + out := make([]string, 0, len(in)) + for _, s := range in { + t := strings.TrimSpace(s) + if t == "" { + return nil, errors.New(what + " must not be empty") + } + out = append(out, t) + } + return out, nil +} + +// documentID strips the entry suffix an activity-log hit carries. +// +// search/ indexes each dated entry of a log as its own document under +// "#-", so that a hit lands on the entry rather than on the +// whole log. That id is not a document id: spec_read would not resolve it. The +// entry's position is already reported separately as Anchor, so the split loses +// nothing and makes every id in a search result one an agent can hand straight +// to spec_read. +func documentID(id string) string { + if i := strings.IndexByte(id, '#'); i >= 0 { + return id[:i] + } + return id +} + +// plainSnippet converts bleve's highlighted fragment to plain text. +// +// search.Hit.Snippet is HTML: the matched terms are wrapped in and +// everything around them is HTML-escaped, because the web UI renders it. A tool +// result is not rendered, so leaving it would show an agent literal "&" and +// "" and invite it to copy them into prose. Both are undone exactly +// rather than by a general tag stripper: the only markup bleve's formatter +// emits is that one tag pair, so removing it and unescaping restores the +// document's own text byte for byte. +// +// This is coupled to search/'s choice of highlighter. If that ever stops being +// bleve's default HTML formatter, this must change with it. +func plainSnippet(s string) string { + if s == "" { + return "" + } + s = strings.ReplaceAll(s, "", "") + s = strings.ReplaceAll(s, "", "") + return html.UnescapeString(s) +} diff --git a/service/read.go b/service/read.go index 9bdb593f4e92a5f5c501efaa8087a4e98a6263f5..bd4db419e2800bcf3804c43540d28849c0cc79b7 100644 --- a/service/read.go +++ b/service/read.go @@ -150,6 +150,9 @@ func (s *Service) resolveRev(ctx context.Context, sp *Space, rev string) (plumbing.Hash, string, error) { if sp == nil || sp.Repo == nil { return plumbing.ZeroHash, "", errors.New("service: space has no open repository") } + if err := ValidateReadRev(rev); err != nil { + return plumbing.ZeroHash, "", err + } resolved := rev if resolved == ApprovedRev { resolved = sp.Repo.ApprovedBranch() @@ -159,6 +162,34 @@ if err != nil { return plumbing.ZeroHash, "", readErr(err, "resolve revision %q in %s", resolved, sp.Ref) } return commit, resolved, nil +} + +// ValidateReadRev enforces the read contract: the read plane serves the approved +// head by default, and otherwise only an immutable object name. +// +// gitx.ResolveRev happily resolves ref names, so without this guard a caller +// could pass rev="proposals/42" and have the READ plane hand back unreviewed +// proposal content — the single failure this service exists to prevent, since +// that text would then flow into agent context as though it were approved. +// Reading a proposal branch is a deliberate act belonging to the review path, +// not something any read surface can be talked into. +// +// Object names are required to be full: an abbreviation that is unique today +// can become ambiguous later, so a pinned revision would silently stop meaning +// one thing. +func ValidateReadRev(rev string) error { + if rev == ApprovedRev { + return nil + } + if len(rev) != 40 { + return fmt.Errorf("%w: revision %q must be a full 40-character object name", ErrBadReadRev, rev) + } + for _, c := range rev { + if (c < '0' || c > '9') && (c < 'a' || c > 'f') { + return fmt.Errorf("%w: revision %q must be a full 40-character object name", ErrBadReadRev, rev) + } + } + return nil } // readErr maps a gitx failure onto this package's sentinels so callers above diff --git a/service/readrev_test.go b/service/readrev_test.go new file mode 100644 index 0000000000000000000000000000000000000000..a23f7df78192e20ece3fae2f51220b88232f8e94 --- /dev/null +++ b/service/readrev_test.go @@ -0,0 +1,46 @@ +package service + +import ( + "errors" + "strings" + "testing" +) + +// The read plane must never be talkable into serving proposal content. gitx +// resolves ref names happily, so this guard is the only thing standing between +// a crafted rev and unreviewed text entering an agent's context as approved. +func TestValidateReadRevRefusesRefNames(t *testing.T) { + sha := strings.Repeat("a1b2c3d4", 5) // 40 hex chars + + for _, tc := range []struct { + name string + rev string + ok bool + }{ + {"approved head sentinel", ApprovedRev, true}, + {"full object name", sha, true}, + {"proposal branch", "proposals/42", false}, + {"approved branch by name", "main", false}, + {"HEAD", "HEAD", false}, + {"tag", "refs/tags/v1", false}, + {"abbreviated sha", sha[:12], false}, + {"uppercase hex", strings.ToUpper(sha), false}, + {"sha with trailing path", sha + "/x", false}, + {"almost hex", strings.Repeat("g", 40), false}, + } { + t.Run(tc.name, func(t *testing.T) { + err := ValidateReadRev(tc.rev) + if tc.ok && err != nil { + t.Fatalf("ValidateReadRev(%q) = %v, want nil", tc.rev, err) + } + if !tc.ok { + if err == nil { + t.Fatalf("ValidateReadRev(%q) = nil, want rejection", tc.rev) + } + if !errors.Is(err, ErrBadReadRev) { + t.Fatalf("ValidateReadRev(%q) error = %v, want ErrBadReadRev", tc.rev, err) + } + } + }) + } +} diff --git a/service/service.go b/service/service.go index ab16cfbda2097da0489da10b7271237e7dca81d9..4f438d3152f684c19033be744cd780978f69390a 100644 --- a/service/service.go +++ b/service/service.go @@ -32,6 +32,8 @@ ErrIncompleteConfig = errors.New("service: incomplete configuration") // ErrNotFound marks a missing space, revision, document or row. ErrNotFound = errors.New("service: not found") + // ErrBadReadRev rejects a revision that is not an immutable object name. + ErrBadReadRev = errors.New("service: revision must be an object name") // ErrSpaceExists marks a create that would clobber an existing space, // either its repository on disk or its row.