10 files changed, 700 insertions(+), 65 deletions(-)

A mcpsrv/cache.go
A mcpsrv/cache_internal_test.go
A mcpsrv/cache_test.go
A mcpsrv/errors.go
M mcpsrv/hostguard_test.go
M mcpsrv/list.go
M mcpsrv/mcpsrv.go
M mcpsrv/mcpsrv_test.go
M mcpsrv/read.go
M mcpsrv/search.go

@@ 0,0 1,133 @@
+package mcpsrv
+
+import "net/http"
+
+// What this endpoint tells a cache, and why it is said here rather than by the
+// router's middleware.
+//
+// # The shared helper this is a copy of
+//
+// sr-ht-ecore/mcphttp.PrivateCache is this function, one package up, written for
+// the services on the instance that mount an MCP endpoint. It is not imported
+// because it is not resolvable: the ecore commit that adds mcphttp is not
+// published, and this module pins an ecore from before it. So the code is here,
+// deliberately identical in behaviour and in header values, and the swap when
+// mcphttp is reachable is a delete and an import with no observable change to
+// any response — which is the reason the constants below are byte for byte what
+// mcphttp emits rather than a spelling of this package's own.
+const (
+	// cacheControl is what every response of this endpoint carries.
+	//
+	// private and no-store are the instance's pair for an answer that depends
+	// entirely on the credential the request carried and says nothing about it in
+	// its URL — which is every answer here. no-cache would not do: it still
+	// permits a shared cache to *store* the body and merely revalidate, which is
+	// the one thing no-store forbids.
+	//
+	// no-transform is the SDK's own, kept. Its streamable transport writes
+	// `no-cache, no-transform` on every response it produces; no-transform
+	// protects the SSE framing from an intermediary that would recompress or
+	// rechunk it, and there is no reason to drop it. Only no-cache is replaced.
+	cacheControl = "private, no-store, no-transform"
+
+	// cacheVary names what an answer here depends on, and it names both planes
+	// because on this service both planes genuinely reach /mcp.
+	//
+	// That is worth stating, because the sibling that mounts the same surface
+	// decided the other way: dolt.sr.ht varies on Authorization alone, arguing
+	// that its /mcp is bearer-only and that naming Cookie would promise a cache
+	// a dependency the surface never reads. The argument is right and does not
+	// apply here. authn.Resolver.Resolve prefers a bearer token when one is
+	// present, but falls through to login.UsernameFromRequest when none is —
+	// and an owner cookie resolves to KindOwner, which is exactly what Gate's
+	// CanRead admits. So on this service the cookie is not an unread header: it
+	// is the difference between the whole corpus and a 401, which is the
+	// strongest reason a response can have to vary on something.
+	cacheVary = "Cookie, Authorization"
+)
+
+// privateCache marks every response this endpoint writes as one no cache may
+// keep, and states what it depends on.
+//
+// It cannot be a middleware that sets the headers before the handler runs, which
+// is how the rest of this service does it. The SDK's streamable transport sets
+// Cache-Control itself, with Set, from inside the handler — so a value written
+// on the way in is overwritten on the way out, and the response leaves with
+// `no-cache, no-transform` and no Vary at all. The headers are therefore written
+// at the last moment they still can be: when the status line is committed and
+// every Set the handler was going to make has been made.
+//
+// It wraps everything Handler owns, the Host allowlist's 403 included. Gate's
+// 401 is written outside this chain — it has to be, since Gate runs inside the
+// resolver middleware and Handler runs inside Gate — so Gate sets the same two
+// headers itself.
+func privateCache(next http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		next.ServeHTTP(&cacheWriter{ResponseWriter: w}, r)
+	})
+}
+
+// cacheWriter is the http.ResponseWriter privateCache hands down: it sets the
+// two headers when the response is committed, by whichever of the three routes
+// the handler takes.
+type cacheWriter struct {
+	http.ResponseWriter
+	committed bool
+}
+
+func (w *cacheWriter) WriteHeader(status int) {
+	w.commit()
+	w.ResponseWriter.WriteHeader(status)
+}
+
+// Write commits first: net/http commits the response on the first Write and
+// drops every header set after that, silently. A handler that never calls
+// WriteHeader is the ordinary case and not an exotic one.
+func (w *cacheWriter) Write(b []byte) (int, error) {
+	w.commit()
+	return w.ResponseWriter.Write(b)
+}
+
+// Flush is the third commit and the one an event stream takes. It has to be a
+// method on this type: http.ResponseController prefers a Flush on the writer it
+// was handed over one reached through Unwrap, so without this the flush would
+// commit the response at the writer below and the two headers would never be
+// written — on exactly the answers that stay open longest, and with every other
+// test in this package still green (TestHeadersLandOnAFlush).
+func (w *cacheWriter) Flush() {
+	w.commit()
+	// http.Flusher.Flush reports nothing, and the controller's error can only be
+	// "this writer does not support flushing" — which, if it happens, is a writer
+	// that could not have streamed through any wrapper.
+	_ = http.NewResponseController(w.ResponseWriter).Flush()
+}
+
+func (w *cacheWriter) commit() {
+	if w.committed {
+		return
+	}
+	w.committed = true
+	w.Header().Set("Cache-Control", cacheControl)
+	w.Header().Set("Vary", cacheVary)
+}
+
+// Unwrap is what http.ResponseController follows to reach the real writer for
+// everything this type does not implement itself: the deadlines, Hijack,
+// EnableFullDuplex. Without it a controller handed this writer answers
+// ErrNotSupported to all of them, because cacheWriter embeds the
+// http.ResponseWriter *interface* and so promotes nothing of the writer below.
+//
+// It is worth being exact about what it does *not* do, because the sentence it
+// is usually given — "without Unwrap the wrapper hides the flusher and SSE
+// streaming breaks" — is not true of this type and is easy to keep repeating.
+// It is true of a wrapper whose only method is WriteHeader; here Flush above is
+// a method on cacheWriter, so a controller finds that one and never needs to
+// unwrap to flush. Deleting Unwrap leaves every flush, every header and every
+// MCP session in this package's tests working, which is measured rather than
+// asserted: TestUnwrapReachesTheWriterBelow pins the deadline call, which is the
+// thing that actually stops working, and it is the test that goes red.
+//
+// The SDK asks for none of those today — its streamable transport calls Flush
+// and nothing else — so this method is here for the wrapper to be a wrapper
+// rather than to keep a feature alive.
+func (w *cacheWriter) Unwrap() http.ResponseWriter { return w.ResponseWriter }

@@ 0,0 1,168 @@
+package mcpsrv
+
+import (
+	"bufio"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// The wrapper privateCache hands down, tested where it can be reached. The
+// endpoint-level statement — that every answer of /mcp carries these two headers
+// — is in cache_test.go; what is here is the three ways a handler can commit a
+// response, because a wrapper that catches only one of them passes every header
+// test while setting no header at all on the path the SDK actually takes.
+
+// TestHeadersLandOnAnExplicitWriteHeader is the ordinary path.
+func TestHeadersLandOnAnExplicitWriteHeader(t *testing.T) {
+	h := privateCache(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusAccepted)
+	}))
+
+	rec := httptest.NewRecorder()
+	h.ServeHTTP(rec, httptest.NewRequest(http.MethodPost, "/mcp", nil))
+
+	assert.Equal(t, http.StatusAccepted, rec.Code)
+	assert.Equal(t, cacheControl, rec.Header().Get("Cache-Control"))
+	assert.Equal(t, cacheVary, rec.Header().Get("Vary"))
+}
+
+// TestHeadersLandOnAnImplicitCommit covers the handler that never calls
+// WriteHeader at all. net/http commits on the first Write and drops every header
+// set after that point, silently.
+func TestHeadersLandOnAnImplicitCommit(t *testing.T) {
+	h := privateCache(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		_, _ = w.Write([]byte(`{"jsonrpc":"2.0"}`))
+	}))
+
+	rec := httptest.NewRecorder()
+	h.ServeHTTP(rec, httptest.NewRequest(http.MethodPost, "/mcp", nil))
+
+	assert.Equal(t, http.StatusOK, rec.Code)
+	assert.Equal(t, cacheControl, rec.Header().Get("Cache-Control"))
+	assert.Equal(t, cacheVary, rec.Header().Get("Vary"))
+	assert.Equal(t, `{"jsonrpc":"2.0"}`, rec.Body.String())
+}
+
+// TestHeadersLandOnAFlush is the third commit, and the one a stream takes: an
+// SSE handler writes nothing before its first flush, so a wrapper that hooks
+// Write and WriteHeader alone leaves the response uncached-marked exactly on the
+// answers that stay open longest.
+func TestHeadersLandOnAFlush(t *testing.T) {
+	h := privateCache(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		require.NoError(t, http.NewResponseController(w).Flush())
+	}))
+
+	rec := httptest.NewRecorder()
+	h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/mcp", nil))
+
+	assert.True(t, rec.Flushed)
+	assert.Equal(t, cacheControl, rec.Header().Get("Cache-Control"))
+	assert.Equal(t, cacheVary, rec.Header().Get("Vary"))
+}
+
+// TestTheSDKsOwnDirectivesAreOverridden is why this is a wrapper and not a
+// middleware that sets the headers on the way in. The streamable transport sets
+// Cache-Control itself, with Set, from inside the handler, so anything written
+// before it runs loses. The handler here does exactly what the SDK does.
+func TestTheSDKsOwnDirectivesAreOverridden(t *testing.T) {
+	h := privateCache(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Cache-Control", "no-cache, no-transform")
+		w.WriteHeader(http.StatusOK)
+	}))
+
+	rec := httptest.NewRecorder()
+	h.ServeHTTP(rec, httptest.NewRequest(http.MethodPost, "/mcp", nil))
+
+	assert.Equal(t, cacheControl, rec.Header().Get("Cache-Control"),
+		"no-cache lets a cache store the body and revalidate, which is the thing no-store forbids")
+	assert.Equal(t, cacheVary, rec.Header().Get("Vary"))
+}
+
+// TestOnlyTheFirstCommitWrites checks the committed flag: a handler that writes
+// after committing must not have its headers re-Set on every Write.
+func TestOnlyTheFirstCommitWrites(t *testing.T) {
+	h := privateCache(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		_, _ = w.Write([]byte("a"))
+		_, _ = w.Write([]byte("b"))
+	}))
+
+	rec := httptest.NewRecorder()
+	h.ServeHTTP(rec, httptest.NewRequest(http.MethodPost, "/mcp", nil))
+
+	assert.Equal(t, []string{cacheControl}, rec.Header().Values("Cache-Control"))
+	assert.Equal(t, []string{cacheVary}, rec.Header().Values("Vary"))
+	assert.Equal(t, "ab", rec.Body.String())
+}
+
+// TestUnwrapReachesTheWriterBelow is the test the Unwrap method exists for, and
+// it deliberately does not measure flushing.
+//
+// The sentence usually attached to Unwrap on a writer like this — that without
+// it the flusher is hidden and SSE breaks — is not true of this type: Flush is a
+// method on cacheWriter, so a controller finds that one first and never unwraps.
+// (Measured, not assumed: with Unwrap deleted every other test in this file, and
+// every MCP session the rest of this package opens, stays green.) What stops
+// working is everything cacheWriter does not implement itself, and the deadlines
+// are the reachable half of that — so this is written against a deadline, which
+// is the call that actually goes ErrNotSupported.
+//
+// It runs over a real server because httptest.ResponseRecorder supports no
+// deadline at all, and a recorder therefore cannot tell the two cases apart.
+func TestUnwrapReachesTheWriterBelow(t *testing.T) {
+	var deadlineErr error
+	srv := httptest.NewServer(privateCache(http.HandlerFunc(
+		func(w http.ResponseWriter, _ *http.Request) {
+			deadlineErr = http.NewResponseController(w).SetWriteDeadline(time.Now().Add(time.Minute))
+		})))
+	t.Cleanup(srv.Close)
+
+	req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL, nil)
+	require.NoError(t, err)
+	resp, err := (&http.Client{Timeout: 5 * time.Second}).Do(req)
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = resp.Body.Close() })
+
+	require.NoError(t, deadlineErr, "a controller must reach the writer this one wraps")
+}
+
+// TestAStreamReachesTheClientBeforeTheHandlerReturns is the same property
+// measured rather than asserted: over a real connection, with a real client,
+// bytes flushed inside the handler have to arrive while the handler is still
+// running. A recorder cannot tell a flush that worked from one that was buffered
+// until the end — and a broken flush here is what would turn every MCP session
+// on this endpoint into a hang rather than an error.
+func TestAStreamReachesTheClientBeforeTheHandlerReturns(t *testing.T) {
+	release := make(chan struct{})
+	srv := httptest.NewServer(privateCache(http.HandlerFunc(
+		func(w http.ResponseWriter, _ *http.Request) {
+			w.Header().Set("Content-Type", "text/event-stream")
+			_, _ = w.Write([]byte("data: first\n\n"))
+			if err := http.NewResponseController(w).Flush(); err != nil {
+				t.Errorf("flush: %v", err)
+			}
+			<-release
+		})))
+	t.Cleanup(func() {
+		close(release)
+		srv.Close()
+	})
+
+	req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL, nil)
+	require.NoError(t, err)
+	resp, err := (&http.Client{Timeout: 5 * time.Second}).Do(req)
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = resp.Body.Close() })
+
+	assert.Equal(t, cacheControl, resp.Header.Get("Cache-Control"))
+	assert.Equal(t, cacheVary, resp.Header.Get("Vary"))
+
+	line, err := bufio.NewReader(resp.Body).ReadString('\n')
+	require.NoError(t, err, "the first event must arrive while the handler is still blocked")
+	assert.Equal(t, "data: first\n", line)
+}

@@ 0,0 1,152 @@
+package mcpsrv_test
+
+import (
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/go-chi/chi/v5"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"sourcecraft.dev/bigbes/sr-ht-spec/authn"
+	"sourcecraft.dev/bigbes/sr-ht-spec/mcpsrv"
+)
+
+// What a cache is told about /mcp, stated at the endpoint rather than at the
+// wrapper (cache_internal_test.go has the wrapper).
+//
+// Every answer here depends entirely on the credential the request carried and
+// says nothing about it in its URL, and some of those answers are the whole
+// approved corpus. A shared cache that stored one and replayed it to the next
+// caller would be handing one principal's read to another.
+
+const (
+	wantCacheControl = "private, no-store, no-transform"
+	wantVary         = "Cookie, Authorization"
+)
+
+// mount builds the chain cmd/specsrht builds, minus the resolver middleware:
+// Gate outside, Handler inside, with a principal planted directly so the test
+// does not need a tokens.sr.ht plane.
+func mount(t *testing.T, p authn.Principal) *httptest.Server {
+	t.Helper()
+	r, s := newFixture()
+	h, err := mcpsrv.Handler(mcpsrv.Backend{Docs: r, Index: s}, "test", "https://spec.srht.bigb.es")
+	require.NoError(t, err)
+
+	withPrincipal := func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+			next.ServeHTTP(w, req.WithContext(authn.WithPrincipal(req.Context(), p)))
+		})
+	}
+
+	router := chi.NewRouter()
+	router.Handle("/mcp", withPrincipal(mcpsrv.Gate(h)))
+	srv := httptest.NewServer(router)
+	t.Cleanup(srv.Close)
+	return srv
+}
+
+func post(t *testing.T, srv *httptest.Server, host, accept, body string) *http.Response {
+	t.Helper()
+	req, err := http.NewRequestWithContext(t.Context(), http.MethodPost, srv.URL+"/mcp", strings.NewReader(body))
+	require.NoError(t, err)
+	req.Host = host
+	req.Header.Set("Content-Type", "application/json")
+	req.Header.Set("Accept", accept)
+
+	resp, err := srv.Client().Do(req)
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = resp.Body.Close() })
+	return resp
+}
+
+const initialize = `{"jsonrpc":"2.0","id":1,"method":"initialize","params":{` +
+	`"protocolVersion":"2025-06-18","capabilities":{},` +
+	`"clientInfo":{"name":"test-client","version":"test"}}}`
+
+// The answer an agent actually gets, on the transport it actually uses: the SDK
+// replies to a POST with an event stream, which is the response that never calls
+// WriteHeader and commits through Flush instead. This is the case a wrapper
+// hooking WriteHeader alone would miss while passing every other test here.
+func TestTheHeadersLandOnAStreamedAnswer(t *testing.T) {
+	srv := mount(t, authn.Principal{Kind: authn.KindOwner})
+
+	resp := post(t, srv, "spec.srht.bigb.es", "application/json, text/event-stream", initialize)
+
+	require.Equal(t, http.StatusOK, resp.StatusCode)
+	require.Equal(t, "text/event-stream", strings.Split(resp.Header.Get("Content-Type"), ";")[0],
+		"this test is worth nothing unless the answer really was a stream")
+
+	assert.Equal(t, wantCacheControl, resp.Header.Get("Cache-Control"))
+	assert.Equal(t, wantVary, resp.Header.Get("Vary"))
+
+	body, err := io.ReadAll(resp.Body)
+	require.NoError(t, err)
+	require.Contains(t, string(body), mcpsrv.ServerName, "and it really was this server answering")
+}
+
+// The SDK writes `no-cache, no-transform` on its own responses. no-cache still
+// permits a shared cache to store the body and merely revalidate, which is the
+// one thing no-store forbids — so the wrapper has to win, and it can only do
+// that by writing at commit time rather than on the way in.
+func TestTheSDKsCacheControlDoesNotSurvive(t *testing.T) {
+	srv := mount(t, authn.Principal{Kind: authn.KindOwner})
+
+	resp := post(t, srv, "spec.srht.bigb.es", "application/json, text/event-stream", initialize)
+
+	require.Equal(t, []string{wantCacheControl}, resp.Header.Values("Cache-Control"),
+		"exactly one directive set, and it is ours")
+	assert.NotContains(t, resp.Header.Get("Cache-Control"), "no-cache")
+}
+
+// A refusal is an answer too. The Host allowlist writes this one before the SDK
+// is reached at all, and it is inside the wrapper for that reason.
+func TestTheHeadersLandOnAHostRefusal(t *testing.T) {
+	srv := mount(t, authn.Principal{Kind: authn.KindOwner})
+
+	resp := post(t, srv, "evil.example.com", "application/json, text/event-stream", initialize)
+
+	require.Equal(t, http.StatusForbidden, resp.StatusCode)
+	assert.Equal(t, wantCacheControl, resp.Header.Get("Cache-Control"))
+	assert.Equal(t, wantVary, resp.Header.Get("Vary"))
+}
+
+// Gate's 401 is written outside the wrapper — Gate runs inside the resolver
+// middleware and Handler runs inside Gate — so Gate sets the two headers itself.
+// A cache free to keep this 401 would refuse a credential this service never
+// saw.
+func TestTheHeadersLandOnAGateRefusal(t *testing.T) {
+	srv := mount(t, authn.Anonymous())
+
+	resp := post(t, srv, "spec.srht.bigb.es", "application/json, text/event-stream", initialize)
+
+	require.Equal(t, http.StatusUnauthorized, resp.StatusCode)
+	assert.Equal(t, wantCacheControl, resp.Header.Get("Cache-Control"))
+	assert.Equal(t, wantVary, resp.Header.Get("Vary"))
+}
+
+// Vary names the cookie because on this service the cookie genuinely decides the
+// answer, which is the one thing that separates this endpoint from dolt.sr.ht's.
+//
+// dolt varies on Authorization alone and is right to: its /mcp is bearer-only,
+// so naming Cookie there would promise a cache a dependency the surface never
+// reads. Here authn.Resolver.Resolve falls through to the unified-login cookie
+// whenever no bearer token is present, and an owner cookie resolves to KindOwner
+// — which is exactly what Gate admits. The two requests below differ in nothing
+// but the principal the cookie plane would have produced, and they get different
+// statuses; that difference is what Vary: Cookie is for.
+func TestTheCookiePlaneReallyDoesDecideTheAnswer(t *testing.T) {
+	asOwner := post(t, mount(t, authn.Principal{Kind: authn.KindOwner}),
+		"spec.srht.bigb.es", "application/json, text/event-stream", initialize)
+	asAnonymous := post(t, mount(t, authn.Anonymous()),
+		"spec.srht.bigb.es", "application/json, text/event-stream", initialize)
+
+	require.Equal(t, http.StatusOK, asOwner.StatusCode)
+	require.Equal(t, http.StatusUnauthorized, asAnonymous.StatusCode)
+	require.NotEqual(t, asOwner.StatusCode, asAnonymous.StatusCode,
+		"if these ever agree, Vary: Cookie has stopped being a statement about this endpoint")
+}

@@ 0,0 1,90 @@
+package mcpsrv
+
+import (
+	"errors"
+	"log/slog"
+
+	"github.com/modelcontextprotocol/go-sdk/jsonrpc"
+	"go.bigb.es/auxilia/scribe"
+
+	"sourcecraft.dev/bigbes/sr-ht-spec/core"
+	"sourcecraft.dev/bigbes/sr-ht-spec/service"
+)
+
+// The one place this surface decides what a failure *is*, which on MCP is a
+// question with two answers rather than a status code.
+//
+//   - A tool result error (CallToolResult.IsError) is an answer to the agent:
+//     the call was understood, executed, and the thing asked for is not there.
+//     The SDK produces one out of any ordinary error a handler returns, and the
+//     agent reads it as text and decides what to ask next.
+//   - A protocol error (a *jsonrpc.Error returned by a handler, which the SDK
+//     passes through as the JSON-RPC error of the response) says the call did
+//     not produce an answer at all. The client's CallTool returns an error
+//     rather than a result, which is exactly right for a git object store that
+//     is down: an agent must not read "the store could not answer" as "that
+//     document does not exist" and go rewrite a specification around a document
+//     that is perfectly real.
+//
+// Before this file the two were one. Every error from below travelled to the
+// agent as a tool result carrying its own text, so `git object store is on
+// fire` and `no document "SPEC-0007"` were the same kind of answer, told apart
+// only by prose the agent would have had to parse. The tests pinned it: a
+// backend failure asserted that the agent was shown the words "on fire".
+//
+// The whole table is missingOrDenied plus a default, and the default is the
+// protocol arm on purpose: an unmapped error is a bug in a layer below, and
+// rendering it as a tool result would report that bug to the agent as a fact
+// about the corpus.
+
+// internalMessage is the message of every protocol error this surface returns.
+// The detail is logged, never sent: the errors below name spaces, revisions,
+// paths and git internals, and an agent holding a working token is not the
+// audience for any of it.
+const internalMessage = "internal server error"
+
+// missingOrDenied is the answer to a read that resolved to nothing.
+//
+// missing is the sentence the agent sees, and every caller builds it from the
+// arguments of the call being answered ("no space ~alice/rfcs"). That is
+// deliberate: the sentence is written from what the caller passed, so it
+// discloses nothing the caller did not already know, and it does not carry the
+// wrapped text of the error it is replacing — service/ wraps its misses with
+// what it looked up, and echoing that is how a surface eventually publishes the
+// difference between "no such space" and "not yours".
+//
+// where is the operator's half — the tool name — and appears only in the log
+// line of the protocol arm.
+func missingOrDenied(err error, where, missing string) error {
+	if errors.Is(err, service.ErrNotFound) {
+		// A tool result error: the SDK packs an ordinary error into
+		// CallToolResult with IsError set.
+		return errors.New(missing)
+	}
+	return internalError(err, where)
+}
+
+// internalError logs the cause and returns the protocol error the client sees.
+//
+// The error goes through scribe.Err, which expands a culpa chain into err.msg,
+// err.code and err.hint instead of flattening it with %v.
+func internalError(err error, where string) error {
+	slog.Error("a tool call failed", "tool", where, scribe.Err(err))
+	return &jsonrpc.Error{Code: jsonrpc.CodeInternalError, Message: internalMessage}
+}
+
+// noSpace and noRevision are the two "missing" sentences the read tools pass to
+// missingOrDenied, built from the call's own arguments and nothing else.
+//
+// There is no masked-versus-absent distinction to preserve here, unlike the
+// sibling services: Gate has already established that the caller is the owner
+// or one of its agents, and this is a single-user instance whose spaces all
+// belong to that owner. A space this caller cannot see does not exist.
+func noSpace(ref core.SpaceRef) string { return "no space " + ref.String() }
+
+func noRevision(ref core.SpaceRef, rev string) string {
+	if rev == service.ApprovedRev {
+		return "space " + ref.String() + " has no approved revision to read"
+	}
+	return "no revision " + rev + " in " + ref.String()
+}

@@ 6,6 6,8 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/stretchr/testify/require"
+
 	"sourcecraft.dev/bigbes/sr-ht-spec/mcpsrv"
 )
 


@@ 57,27 59,40 @@ func TestHostGuard(t *testing.T) {
 	}
 }
 
-// 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)
+// An origin with no host in it is a construction error, not a warning.
+//
+// This used to be the opposite: Handler logged "Host validation on /mcp is
+// DISABLED" and returned the bare handler, on the reasoning that refusing to
+// start over a config typo is worse than running unguarded. It is not. The
+// guard is the only thing protecting /mcp once the SDK's own is disabled, so
+// that path turned one unparseable value into a silently open endpoint that
+// passes every functional test — the class of failure nobody discovers. The
+// daemon cannot reach it in any case: service.Config.Validate already refuses
+// to start unless the origin parses and carries a host.
+func TestHandlerRefusesAnOriginItCannotGuardWith(t *testing.T) {
+	for _, origin := range []string{
+		"",
+		"   ",
+		"not a url at all",
+		"https://",   // parses, but carries no host
+		"/just/path", // relative, no host
+	} {
+		t.Run(origin, func(t *testing.T) {
+			r, s := newFixture()
+			h, err := mcpsrv.Handler(mcpsrv.Backend{Docs: r, Index: s}, "test", origin)
 
-	if w.Code == http.StatusForbidden {
-		t.Fatalf("unconfigured origin should not 403 every request; got %d", w.Code)
+			require.Error(t, err, "an origin with no host must not yield a handler")
+			require.Nil(t, h, "an unguarded handler must never escape the constructor")
+			require.Contains(t, err.Error(), "no host to guard /mcp with")
+		})
 	}
 }
+
+// A usable origin still builds, which is what keeps the test above from passing
+// for the wrong reason.
+func TestHandlerAcceptsAUsableOrigin(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)
+	require.NotNil(t, h)
+}

@@ 73,11 73,11 @@ func listHandler(ctx context.Context, b Backend, in listInput) (listOutput, erro
 	}
 	sp, err := b.Docs.OpenSpace(ctx, ref)
 	if err != nil {
-		return listOutput{}, err
+		return listOutput{}, missingOrDenied(err, "spec_list", noSpace(ref))
 	}
 	arc, _, resolved, err := archiveAt(ctx, b, sp, rev)
 	if err != nil {
-		return listOutput{}, err
+		return listOutput{}, missingOrDenied(err, "spec_list", noRevision(ref, rev))
 	}
 
 	out := listOutput{Space: ref.String(), Rev: resolved, Documents: make([]documentEntry, 0, len(arc.All()))}


@@ 100,7 100,9 @@ func listHandler(ctx context.Context, b Backend, in listInput) (listOutput, erro
 func listSpaces(ctx context.Context, b Backend) (listOutput, error) {
 	spaces, err := b.Docs.ListSpaces(ctx)
 	if err != nil {
-		return listOutput{}, err
+		// No space was named, so there is no "that one does not exist" to
+		// answer: whatever went wrong here is this service's.
+		return listOutput{}, internalError(err, "spec_list")
 	}
 	out := listOutput{Spaces: make([]spaceEntry, 0, len(spaces))}
 	for _, sp := range spaces {

@@ 56,7 56,7 @@ package mcpsrv
 import (
 	"context"
 	"errors"
-	"log/slog"
+	"fmt"
 	"net"
 	"net/http"
 	"strings"


@@ 231,37 231,53 @@ func New(b Backend, version string) (*mcp.Server, error) {
 // 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.
+//
+// origin is therefore required, and an origin with no host in it is refused
+// here rather than warned about and then served unguarded. This used to warn:
+// it logged that Host validation was disabled and returned the bare handler,
+// on the reasoning that refusing to start over a config typo is worse than
+// running without the guard. That trade is the wrong way round. The guard is
+// the *only* thing protecting /mcp once the SDK's own is disabled, so the warn
+// path turned one unparseable config value into a silently open endpoint —
+// discoverable, in principle, from a log line nobody reads, and indistinguishable
+// in every functional test from a correctly guarded one. A daemon cannot reach
+// this call with such an origin anyway: service.Config.Validate already refuses
+// to start unless [spec.sr.ht] origin parses and carries a host. A caller that
+// got here without one is not an operator to be warned, it is a bug.
 func Handler(b Backend, version, origin string) (http.Handler, error) {
 	srv, err := New(b, version)
 	if err != nil {
 		return nil, err
 	}
+	// instconf.OriginHost and not a local parse: this is the reading of an
+	// origin, and it is the same one authn's mailbox derivation and service's
+	// config validation make. An origin nobody can extract a host from answers
+	// "" here — never a guess such as "localhost", which would silently make
+	// every malformed origin agree with a local client on the one code path
+	// where that decides an allowlist.
+	want := instconf.OriginHost(origin)
+	if want == "" {
+		return nil, fmt.Errorf("mcpsrv: origin %q has no host to guard /mcp with", origin)
+	}
 	h := mcp.NewStreamableHTTPHandler(
 		func(*http.Request) *mcp.Server { return srv },
 		&mcp.StreamableHTTPOptions{DisableLocalhostProtection: true},
 	)
-	return allowHosts(h, origin), nil
+	// The cache directives wrap the Host allowlist rather than the other way
+	// round, so that the 403 carries them too: a refusal by hostname is as
+	// unstorable as an answer, and it is written before the SDK is reached at all.
+	return privateCache(allowHosts(h, want)), 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 {
-	// instconf.OriginHost and not a local parse: this is the reading of an
-	// origin, and it is the same one authn's mailbox derivation and service's
-	// config validation make. An origin nobody can extract a host from answers
-	// "" here — never a guess such as "localhost", which would silently make
-	// every malformed origin agree with a local client — and "" is what turns
-	// the guard off, loudly, below.
-	want := instconf.OriginHost(origin)
-	if want == "" {
-		slog.Warn("mcpsrv: no usable origin configured; Host validation on /mcp is DISABLED")
-		return next
-	}
+// want is the hostname already extracted from the origin by Handler, which is
+// also where an origin that yields none is refused. It is a resolved host and
+// never an origin, so there is no path through this function that leaves the
+// guard off.
+func allowHosts(next http.Handler, want string) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if !hostAllowed(r.Host, want) {
 			http.Error(w, "Forbidden: unexpected Host header", http.StatusForbidden)


@@ 293,11 309,19 @@ func allowHosts(next http.Handler, origin string) http.Handler {
 // tokens.sr.ht token minted for spec:propose alone at `initialize`, before it
 // ever named a tool. The grant is therefore checked per tool, by requireRead and
 // by service.Propose, each of which knows what is being attempted.
+//
+// The refusal carries the cache directives itself, which privateCache would
+// otherwise have written for it. It has to: Gate runs inside the resolver
+// middleware and Handler runs inside Gate, so a 401 written here never reaches
+// the wrapper Handler installs. A shared cache free to keep this 401 and replay
+// it to the next caller would refuse a credential this service never saw.
 func Gate(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if !authn.PrincipalFromContext(r.Context()).CanRead() {
 			w.Header().Set("Content-Type", "text/plain; charset=utf-8")
 			w.Header().Set("WWW-Authenticate", authn.Challenge())
+			w.Header().Set("Cache-Control", cacheControl)
+			w.Header().Set("Vary", cacheVary)
 			http.Error(w, "authentication required", http.StatusUnauthorized)
 			return
 		}

@@ 345,17 345,25 @@ func TestReadMissing(t *testing.T) {
 	require.Contains(t, errorText(res), `no document "SPEC-9999"`)
 	require.Contains(t, errorText(res), "~bigbes/rfcs")
 
+	// The sentence is built from the argument the caller passed, not from the
+	// error service/ wrapped its miss with. It used to be the latter, so what
+	// reached the agent was the sentinel's own text — "service: not found:
+	// space ~bigbes/nope" — which names a layer the agent cannot see and is one
+	// refactor away from saying something different about a private space than
+	// about an absent one.
 	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")
+	require.Contains(t, errorText(res), "no space ~bigbes/nope")
+	require.NotContains(t, errorText(res), "service:")
 
 	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")
+	require.Contains(t, errorText(res), "no revision "+blob(99))
+	require.NotContains(t, errorText(res), "service:")
 
 	res = call(t, session, "spec_read", map[string]any{"space": "", "document": "SPEC-0007"})
 	require.True(t, res.IsError)


@@ 366,18 374,62 @@ func TestReadMissing(t *testing.T) {
 	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) {
+// A store that could not answer is a protocol error, not a tool result — the
+// distinction errors.go exists for.
+//
+// This test used to assert the opposite: that the agent was handed a tool
+// result carrying the words "on fire". That is the failure mode bench shipped
+// and cov corrected. A tool result means "the call was understood and the thing
+// you asked for is not there", so an agent reading one for a dead object store
+// concludes the document does not exist and rewrites its plan around a document
+// that is perfectly real. It also published the store's own words to the agent.
+func TestAStoreThatCouldNotAnswerIsAProtocolError(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",
+	res, err := session.CallTool(context.Background(), &mcp.CallToolParams{
+		Name:      "spec_read",
+		Arguments: map[string]any{"space": "~bigbes/rfcs", "document": "SPEC-0007"},
+	})
+	require.Error(t, err, "a store outage must fail the call, not answer it")
+	require.Nil(t, res, "a protocol error carries no result for an agent to read as an answer")
+
+	// And it says nothing about what failed. The detail is logged, never sent.
+	require.Contains(t, err.Error(), "internal server error")
+	require.NotContains(t, err.Error(), "on fire")
+	require.NotContains(t, err.Error(), "object store")
+}
+
+// The same for the search index, which is the other store this surface reads.
+func TestASearchIndexOutageIsAProtocolError(t *testing.T) {
+	r, s := newFixture()
+	s.err = errors.New("index is closed")
+	session := connect(t, r, s)
+
+	res, err := session.CallTool(context.Background(), &mcp.CallToolParams{
+		Name:      "spec_search",
+		Arguments: map[string]any{"query": "storage"},
 	})
+	require.Error(t, err)
+	require.Nil(t, res)
+	require.Contains(t, err.Error(), "internal server error")
+	require.NotContains(t, err.Error(), "index is closed")
+}
+
+// A miss stays a tool result, which is the other half of the split: the agent
+// is meant to read this one and ask something else.
+func TestAMissStaysAToolResult(t *testing.T) {
+	r, s := newFixture()
+	session := connect(t, r, s)
+
+	res, err := session.CallTool(context.Background(), &mcp.CallToolParams{
+		Name:      "spec_read",
+		Arguments: map[string]any{"space": "~bigbes/nope", "document": "SPEC-0007"},
+	})
+	require.NoError(t, err, "a miss is an answer, not a failed call")
 	require.True(t, res.IsError)
-	require.Contains(t, errorText(res), "on fire")
+	require.Contains(t, errorText(res), "no space ~bigbes/nope")
 }
 
 // --- search -----------------------------------------------------------------


@@ 510,15 562,8 @@ func TestSearchHitShape(t *testing.T) {
 	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")
-}
+// The index-outage case lives with the other half of the split, in
+// TestASearchIndexOutageIsAProtocolError.
 
 // --- list -------------------------------------------------------------------
 

@@ 64,11 64,11 @@ func readHandler(ctx context.Context, b Backend, in readInput) (readOutput, erro
 	}
 	sp, err := b.Docs.OpenSpace(ctx, ref)
 	if err != nil {
-		return readOutput{}, err
+		return readOutput{}, missingOrDenied(err, "spec_read", noSpace(ref))
 	}
 	arc, bodies, resolved, err := archiveAt(ctx, b, sp, rev)
 	if err != nil {
-		return readOutput{}, err
+		return readOutput{}, missingOrDenied(err, "spec_read", noRevision(ref, rev))
 	}
 	page, err := resolvePage(arc, in.Document)
 	if err != nil {


@@ 77,10 77,12 @@ func readHandler(ctx context.Context, b Backend, in readInput) (readOutput, erro
 	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)
+		// is a broken invariant rather than a missing document — this service's
+		// fault and not an answer about the corpus, so it goes to the agent as a
+		// protocol error and the detail goes to the log. Returning an empty
+		// markdown field would be indistinguishable from an empty document.
+		return readOutput{}, internalError(
+			fmt.Errorf("document %s at %s in %s has no body", page.Path, resolved, ref), "spec_read")
 	}
 	return readOutput{
 		Space:    ref.String(),

@@ 79,7 79,11 @@ func searchHandler(ctx context.Context, b Backend, in searchInput) (searchOutput
 		Offset:   in.Offset,
 	})
 	if err != nil {
-		return searchOutput{}, err
+		// A query that matched nothing is an empty result set and not an error,
+		// so every error the index can return here is the index failing. An
+		// agent told "no hits" by a closed index would conclude the corpus does
+		// not cover what it asked about.
+		return searchOutput{}, internalError(err, "spec_search")
 	}
 
 	out := searchOutput{Hits: make([]searchHit, 0, len(res.Hits)), Total: res.Total}