From 42d4ee137ee6f4c70dd2ce4097c21606151f4a29 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Thu, 23 Jul 2026 08:22:18 +0300 Subject: [PATCH] feat(mcpsrv): spec_propose write tool (Phase 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The agent-facing half of the write plane over MCP. spec_propose uploads whole documents and returns {proposal, url, merged}, calling the same service.Propose the REST PUT will — one implementation of If-Match, provenance and auto-merge behind both surfaces, never two that drift. - Writer is an optional Backend field: nil keeps the server read-only (the three read tools, unchanged), so a read-only deploy or a test needs no mutable backend. Set, it registers spec_propose with neither the read-only nor the idempotent hint — proposing twice opens two proposals. - The acting agent is resolved from the bearer token on the tool call by the resolver middleware now installed on /mcp; the read tools never needed it. The ACL stays in service/: service.Propose refuses a non-agent, so an anonymous or owner caller is rejected there. --- cmd/specsrht/main.go | 7 ++- mcpsrv/mcpsrv.go | 30 +++++++++ mcpsrv/propose.go | 105 ++++++++++++++++++++++++++++++++ mcpsrv/propose_internal_test.go | 95 +++++++++++++++++++++++++++++ 4 files changed, 236 insertions(+), 1 deletion(-) create mode 100644 mcpsrv/propose.go create mode 100644 mcpsrv/propose_internal_test.go diff --git a/cmd/specsrht/main.go b/cmd/specsrht/main.go index 681125dbb0d8aa150506aef2ae265ae14d3003a7..80a18b02143b668ff52bec6f934ad6d023c47f1b 100644 --- a/cmd/specsrht/main.go +++ b/cmd/specsrht/main.go @@ -471,11 +471,16 @@ func newSurfaces(conf ini.File, cfg service.Config, svc *service.Service, versio // guard cannot tell a reverse proxy from an attacker (both reach a loopback // listener with a non-loopback Host), so it is replaced by a check against // this value. Traefik must pass the Host header through or every call 403s. - mcp, err := mcpsrv.Handler(mcpsrv.Backend{Docs: svc, Index: index}, version, cfg.Origin) + mcp, err := mcpsrv.Handler(mcpsrv.Backend{Docs: svc, Index: index, Write: svc}, version, cfg.Origin) if err != nil { index.Close() return nil, fmt.Errorf("assemble the MCP surface: %w", err) } + // spec_propose resolves the acting agent from the bearer token on the tool + // call, so /mcp needs the principal middleware the read tools never did. + // It sets an anonymous principal when there is no token, which service.Propose + // refuses — the ACL stays in service/, this only populates the identity. + mcp = svc.Resolver().Middleware()(mcp) gql, err := graph.New(graph.Options{ Reader: svc, diff --git a/mcpsrv/mcpsrv.go b/mcpsrv/mcpsrv.go index f8a565c6503ef375e3bb7299038d6015b8273198..e547f77fb817ba24ef6d83336aa41137202566b4 100644 --- a/mcpsrv/mcpsrv.go +++ b/mcpsrv/mcpsrv.go @@ -77,6 +77,12 @@ type Backend struct { Docs Reader // Index is the one global bleve index — in production *search.Index. Index Searcher + // Write is the write side — in production *service.Service. It is optional: + // when nil the server registers only the read tools, so a read-only + // deployment or a test needs no mutable backend. When set, spec_propose is + // registered and every write goes through the same service.Propose the REST + // PUT calls. + Write Writer } // New builds the MCP server with the Phase 2 read tools registered. version is @@ -144,6 +150,30 @@ func New(b Backend, version string) (*mcp.Server, error) { return nil, out, err }) + // The one write tool, registered only when a write backend is wired. It is + // not read-only or idempotent — proposing twice opens two proposals — so it + // carries neither hint, which is how a client tells a tool it can retry + // freely from one it cannot. + if b.Write != nil { + mcp.AddTool(srv, &mcp.Tool{ + Name: "spec_propose", + Description: "Propose a change to a space: upload whole documents and get back a proposal " + + "and a URL to hand a human for review.\n\n" + + "Pass `if_match` as the `rev` you read the approved head at (from spec_read) — it becomes " + + "the proposal's base, and a base the approved branch has moved off is rejected so you " + + "refetch and re-propose. Each document in `documents` is the WHOLE markdown, frontmatter " + + "included; there are no patches.\n\n" + + "Omit `proposal` to open a new one (give it a `title`); pass an existing proposal id to add " + + "more documents to it, sending the same `if_match` you opened it with.\n\n" + + "The response always carries the proposal `url`. Surface it: the human reviews there, and a " + + "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 + }) + } + return srv, nil } diff --git a/mcpsrv/propose.go b/mcpsrv/propose.go new file mode 100644 index 0000000000000000000000000000000000000000..4a6722801af18c2f07a309366b8dc8ea958df824 --- /dev/null +++ b/mcpsrv/propose.go @@ -0,0 +1,105 @@ +package mcpsrv + +import ( + "context" + "fmt" + + "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. +// +// 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. +type Writer interface { + Propose(ctx context.Context, req service.ProposeRequest) (service.ProposeResult, error) +} + +// proposeDoc is one whole-document upload. The write plane takes whole +// documents, not patches — that is how agents work and what makes the merge +// model plumbing — so an agent sends the full markdown it wants the document to +// have, frontmatter included. +type proposeDoc struct { + Path string `json:"path" jsonschema:"the document's path in the space, e.g. \"specs/0007-storage.md\""` + Content string `json:"content" jsonschema:"the whole document, frontmatter included, exactly as it should be stored"` +} + +type proposeInput struct { + Space string `json:"space" jsonschema:"the space to propose against, written \"~owner/name\""` + // IfMatch is the base: the approved-head sha the agent read the document at, + // which is the rev field spec_read returns for an approved read. It pins the + // proposal's base and is what staleness is measured against. + IfMatch string `json:"if_match" jsonschema:"the approved-head revision you read at — the rev value from spec_read of the approved head. It becomes the proposal's base; a base the approved branch has moved off is rejected."` + Proposal int `json:"proposal,omitempty" jsonschema:"add these documents to an existing open proposal with this id, rather than opening a new one. Omit to open a new proposal."` + Title string `json:"title,omitempty" jsonschema:"a short title for a new proposal (required when opening one, ignored when adding)"` + Rationale string `json:"rationale,omitempty" jsonschema:"why the change is proposed, for the reviewer"` + Message string `json:"message,omitempty" jsonschema:"the commit message for this write; defaults to the title when opening"` + Documents []proposeDoc `json:"documents" jsonschema:"the whole documents to write, at least one"` +} + +type proposeOutput struct { + // Proposal is the proposal id, and Url the stable link to hand a human. An + // agent that proposes without surfacing the url makes the work invisible. + Proposal int `json:"proposal"` + URL string `json:"url"` + // Merged reports whether auto-merge policy landed this immediately. When + // true the change is already on the approved head; when false it is open and + // waiting for a human, and the url is where they review it. + Merged bool `json:"merged"` + // State is the proposal's lifecycle state after this write: "open" or, when + // policy auto-merged, "merged". + State string `json:"state"` + // Branch is the proposal branch, "proposals/". BaseRev is the base the + // proposal is measured against — the value to keep sending as if_match when + // adding to this proposal. + Branch string `json:"branch"` + BaseRev string `json:"base_rev"` +} + +func proposeHandler(ctx context.Context, w Writer, in proposeInput) (proposeOutput, error) { + ref, err := parseSpace(in.Space) + if err != nil { + return proposeOutput{}, err + } + if len(in.Documents) == 0 { + return proposeOutput{}, fmt.Errorf("documents must not be empty; a proposal writes at least one whole document") + } + writes := make([]service.DocumentWrite, 0, len(in.Documents)) + for _, d := range in.Documents { + writes = append(writes, service.DocumentWrite{Path: d.Path, Content: []byte(d.Content)}) + } + + // The principal is resolved by the resolver middleware on /mcp from the + // bearer token on this very request. service.Propose refuses a non-agent, so + // an anonymous or owner caller is rejected there rather than here — the ACL + // has one home, in service/. + principal := authn.PrincipalFromContext(ctx) + + res, err := w.Propose(ctx, service.ProposeRequest{ + Space: ref, + Principal: principal, + ProposalID: in.Proposal, + Title: in.Title, + Rationale: in.Rationale, + IfMatch: in.IfMatch, + Message: in.Message, + Writes: writes, + }) + if err != nil { + return proposeOutput{}, err + } + return proposeOutput{ + Proposal: res.Proposal.ID, + URL: res.URL, + Merged: res.Merged, + State: string(res.Proposal.State), + Branch: res.Proposal.Branch, + BaseRev: res.Proposal.BaseRev, + }, nil +} diff --git a/mcpsrv/propose_internal_test.go b/mcpsrv/propose_internal_test.go new file mode 100644 index 0000000000000000000000000000000000000000..60382686c7d8e53ad46cb8bfce7cbe4985061fa2 --- /dev/null +++ b/mcpsrv/propose_internal_test.go @@ -0,0 +1,95 @@ +package mcpsrv + +import ( + "context" + "testing" + + "sourcecraft.dev/bigbes/sr-ht-spec/authn" + "sourcecraft.dev/bigbes/sr-ht-spec/core" + "sourcecraft.dev/bigbes/sr-ht-spec/service" +) + +// fakeWriter captures the request the tool builds and returns a canned result, +// so the handler's mapping — arguments in, principal from context, result out — +// is checked without a service, a repository or a database. +type fakeWriter struct { + got service.ProposeRequest + res service.ProposeResult + err error +} + +func (f *fakeWriter) Propose(_ context.Context, req service.ProposeRequest) (service.ProposeResult, error) { + f.got = req + return f.res, f.err +} + +// TestProposeHandlerForwardsPrincipalAndArgs proves the tool passes the acting +// agent (resolved onto the request context by the /mcp middleware) and every +// argument through to service.Propose, and maps the result — id, url, merged — +// back out. +func TestProposeHandlerForwardsPrincipalAndArgs(t *testing.T) { + principal := authn.Principal{ + Kind: authn.KindAgent, + Owner: "bigbes", + Agent: "claude-code/spec-writer", + Session: "sess-1", + } + ctx := authn.WithPrincipal(context.Background(), principal) + + w := &fakeWriter{res: service.ProposeResult{ + Proposal: service.Proposal{ + ID: 7, + Branch: "proposals/7", + BaseRev: "1f0c1d1a1e2b3c4d5e6f708192a3b4c5d6e7f809", + State: core.StateOpen, + }, + URL: "https://spec.srht.bigb.es/~bigbes/rfcs/p/7", + Merged: false, + }} + + out, err := proposeHandler(ctx, w, proposeInput{ + Space: "~bigbes/rfcs", + IfMatch: "1f0c1d1a1e2b3c4d5e6f708192a3b4c5d6e7f809", + Title: "Add a note", + Rationale: "because", + Message: "write it", + Documents: []proposeDoc{{Path: "notes/a.md", Content: "---\nid: N-1\n---\n"}}, + }) + if err != nil { + t.Fatalf("proposeHandler: %v", err) + } + + // The request the service saw. + if w.got.Principal != principal { + t.Errorf("principal = %+v, want the one on the context %+v", w.got.Principal, principal) + } + if w.got.Space != (core.SpaceRef{Owner: "bigbes", Name: "rfcs"}) { + t.Errorf("space = %v, want ~bigbes/rfcs", w.got.Space) + } + if w.got.IfMatch != "1f0c1d1a1e2b3c4d5e6f708192a3b4c5d6e7f809" || w.got.Title != "Add a note" { + t.Errorf("request args not forwarded: %+v", w.got) + } + if len(w.got.Writes) != 1 || w.got.Writes[0].Path != "notes/a.md" { + t.Errorf("writes = %+v, want the one document", w.got.Writes) + } + + // The result mapped out. + if out.Proposal != 7 || out.URL != "https://spec.srht.bigb.es/~bigbes/rfcs/p/7" || out.Merged { + t.Errorf("output = %+v, want id 7, the url, merged=false", out) + } + if out.State != "open" || out.Branch != "proposals/7" { + t.Errorf("output state/branch = %q/%q, want open/proposals/7", out.State, out.Branch) + } +} + +// TestProposeHandlerRejectsEmptyDocuments refuses a call with no documents +// before it reaches the service. +func TestProposeHandlerRejectsEmptyDocuments(t *testing.T) { + w := &fakeWriter{} + if _, err := proposeHandler(context.Background(), w, proposeInput{Space: "~bigbes/rfcs"}); err == nil { + t.Fatal("proposeHandler with no documents = nil, want error") + } + if w.got.Writes != nil || w.got.Space != (core.SpaceRef{}) { + t.Fatal("service was called despite empty documents") + } +}