From 1476e065ac4b46a6c37df219b8a4c507c77231d7 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Thu, 23 Jul 2026 08:28:36 +0300 Subject: [PATCH] =?UTF-8?q?feat(api):=20REST=20write=20plane=20=E2=80=94?= =?UTF-8?q?=20PUT=20a=20document=20to=20propose=20(Phase=203)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The other agent-facing write surface, PUT /api/v1/spaces/~owner/name/ docs/ with If-Match and X-Proposal headers, returning {proposal, url}. Like spec_propose it holds no proposal logic: it parses the request into a service.ProposeRequest, calls the same service.Propose, and maps the result and the service sentinels onto status codes — 201 on open, 200 on add, 403 non-agent, 422 malformed document, 409 stale / already-merged, 404 missing. The body is the whole document; title, rationale and message ride in the query string so the body stays the document. The acting agent is resolved from the bearer token by the principal middleware the endpoint installs, and service.Propose is the one ACL — an anonymous caller is a 403 there. Split the service error taxonomy this surface exposed: ErrInvalid (422, a malformed document the agent must fix) is now distinct from ErrForbidden (403, a principal that may not propose at all). Conflating them answered "bad document" with "you are not allowed", which is exactly the distinction the retrying agent needs. --- api/api.go | 172 +++++++++++++++++++++++++++++++ api/api_test.go | 218 ++++++++++++++++++++++++++++++++++++++++ api/propose.go | 139 +++++++++++++++++++++++++ cmd/specsrht/main.go | 21 +++- service/propose.go | 20 ++-- service/propose_test.go | 8 +- service/service.go | 9 ++ 7 files changed, 568 insertions(+), 19 deletions(-) create mode 100644 api/api.go create mode 100644 api/api_test.go create mode 100644 api/propose.go diff --git a/api/api.go b/api/api.go new file mode 100644 index 0000000000000000000000000000000000000000..b1e6923fdb1903fc967dccb54cdfb2310af8b721 --- /dev/null +++ b/api/api.go @@ -0,0 +1,172 @@ +// Package api is spec.sr.ht's REST write plane: the HTTP surface an agent PUTs +// a whole document to in order to open or extend a proposal. +// +// It is one of the two agent-facing write surfaces — the other is mcpsrv's +// spec_propose — and the design's rule is that both call the same +// service.Propose rather than each implementing If-Match, provenance and +// auto-merge for themselves. This package therefore holds no proposal logic: it +// parses an HTTP request into a service.ProposeRequest, calls through, and maps +// the result and the service's error sentinels onto status codes. A rule decided +// here that service/ did not would be exactly the drift the shared layer exists +// to prevent. +// +// # The contract +// +// PUT /api/v1/spaces/~owner/name/docs/ +// If-Match: # required: the approved head you read at +// X-Proposal: # optional: add to this open proposal +// +// The body is the whole document. `If-Match` is the base B — the approved-head +// sha at the time the agent read — with one meaning shared across REST and MCP: +// opening cuts the branch from it, adding is validated against the proposal's +// fixed B. Title, rationale and the commit message ride in the query string, so +// the body stays the document and nothing else. Every response carries the +// proposal and its URL. +// +// # Who may write +// +// Proposing is agent-only; the human write path is native receive-pack. The +// endpoint installs authn's principal middleware so the bearer token resolves, +// and lets service.Propose refuse a non-agent — the ACL stays in service/, +// spelled once, rather than here and there. +package api + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + + "github.com/go-chi/chi/v5" + "github.com/go-chi/chi/v5/middleware" + + "sourcecraft.dev/bigbes/sr-ht-spec/authn" + "sourcecraft.dev/bigbes/sr-ht-spec/service" +) + +// maxBodyBytes caps the document a single PUT may carry. It is generous — a +// document is prose, not an upload — and exists only so a runaway or hostile +// client cannot make the daemon buffer an unbounded body into memory. gitx +// enforces its own per-blob limit on the commit; this is the earlier, cheaper +// refusal. +const maxBodyBytes = 5 << 20 // 5 MiB + +// Writer is the write side of the orchestration layer this surface calls. +// *service.Service satisfies it, and it is the same method mcpsrv's spec_propose +// calls, which is what keeps the two write surfaces one implementation. +type Writer interface { + Propose(ctx context.Context, req service.ProposeRequest) (service.ProposeResult, error) +} + +// Options is everything a Server needs. New reports which one is missing rather +// than failing later inside a handler. +type Options struct { + // Writer is the orchestration layer. *service.Service satisfies it. + Writer Writer + + // Resolver turns an agent bearer token into a principal. Handler installs + // its middleware; Register does not. + Resolver *authn.Resolver +} + +// Server is the REST write endpoint. It is built once at startup and is safe +// for concurrent use. +type Server struct { + writer Writer + resolver *authn.Resolver +} + +// New assembles the server over the seams in opts. +func New(opts Options) (*Server, error) { + if opts.Writer == nil { + return nil, fmt.Errorf("api: Writer is required") + } + if opts.Resolver == nil { + return nil, fmt.Errorf("api: authn Resolver is required") + } + return &Server{writer: opts.Writer, resolver: opts.Resolver}, nil +} + +// Handler returns the REST routes with panic recovery and authn's principal +// middleware installed, so it can be mounted on a router that has none: +// +// router.Mount("/api", api.Handler()) +// +// A caller whose router already resolves a principal uses Register instead; +// installing the middleware twice is harmless — it is idempotent. +func (s *Server) Handler() http.Handler { + r := chi.NewRouter() + r.Use(middleware.Recoverer) + r.Use(s.resolver.Middleware()) + s.Register(r) + return r +} + +// Register mounts the write routes onto r. It installs no middleware of its own; +// the router it is handed must already resolve a principal into the request +// context (authn.Resolver.Middleware), or every writer looks anonymous and is +// refused. +// +// The document path is a single trailing wildcard: the space is "~owner/name" +// and everything after "/docs/" is the document's path in the tree, decoded per +// segment so a percent-encoded separator inside a name is not mistaken for one. +func (s *Server) Register(r chi.Router) { + r.Put("/v1/spaces/~{owner}/{space}/docs/*", s.handlePut) +} + +// proposeResponse is the JSON a successful write returns. It is the REST spelling +// of mcpsrv's proposeOutput — the same fields, so an agent switching surfaces +// reads the same answer — and it always carries the url, the whole point of the +// review plane's entry contract. +type proposeResponse struct { + Proposal int `json:"proposal"` + URL string `json:"url"` + Merged bool `json:"merged"` + State string `json:"state"` + Branch string `json:"branch"` + BaseRev string `json:"base_rev"` +} + +// writeJSON writes v as the response body with the given status. A failure to +// encode is logged into the void here — the header is already sent — but cannot +// be helped, so it is deliberately not retried into a second WriteHeader. +func writeJSON(w http.ResponseWriter, status int, v any) { + w.Header().Set("Content-Type", "application/json; charset=utf-8") + w.WriteHeader(status) + _ = json.NewEncoder(w).Encode(v) +} + +// writeError maps a service error onto a status code and a JSON body an agent +// can act on. The 4xx cases carry the service's message — the agent has to fix +// and retry, and "which document failed the schema" is the whole point — while +// a 5xx is a generic line, because an infrastructure failure's detail belongs in +// the daemon's log, not a client's error field. +func writeError(w http.ResponseWriter, err error) { + status := statusFor(err) + msg := err.Error() + if status >= 500 { + msg = "internal error" + } + writeJSON(w, status, map[string]string{"error": msg}) +} + +// statusFor maps the service sentinels onto HTTP. The staleness and +// already-merged cases are the design's 409; a malformed document is 422, kept +// distinct from the 403 of a principal that may not propose at all. +func statusFor(err error) int { + switch { + case errors.Is(err, service.ErrForbidden): + return http.StatusForbidden + case errors.Is(err, service.ErrInvalid): + return http.StatusUnprocessableEntity + case errors.Is(err, service.ErrStale), + errors.Is(err, service.ErrAlreadyMerged), + errors.Is(err, service.ErrProposalNotOpen): + return http.StatusConflict + case errors.Is(err, service.ErrNotFound): + return http.StatusNotFound + default: + return http.StatusInternalServerError + } +} diff --git a/api/api_test.go b/api/api_test.go new file mode 100644 index 0000000000000000000000000000000000000000..40097395d86432e77cb67e74aa9a46ee690aafea --- /dev/null +++ b/api/api_test.go @@ -0,0 +1,218 @@ +package api_test + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/go-chi/chi/v5" + + "sourcecraft.dev/bigbes/sr-ht-spec/api" + "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 handler builds and returns a canned +// result or error, so routing, header parsing and status mapping are 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 +} + +// router builds the write routes with a principal injected into every request, +// bypassing token resolution — the handler reads the principal off the context, +// and what put it there is not this package's concern. +func router(t *testing.T, w api.Writer, p authn.Principal) http.Handler { + t.Helper() + srv, err := api.New(api.Options{Writer: w, Resolver: testResolver(t)}) + if err != nil { + t.Fatalf("api.New: %v", err) + } + r := chi.NewRouter() + r.Use(func(next http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + next.ServeHTTP(rw, req.WithContext(authn.WithPrincipal(req.Context(), p))) + }) + }) + srv.Register(r) + return r +} + +// testResolver is a resolver New requires but Register does not use. One token +// store with no tokens is enough to construct it. +func testResolver(t *testing.T) *authn.Resolver { + t.Helper() + res, err := authn.NewResolver("bigbes", stubTokens{}) + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + return res +} + +type stubTokens struct{} + +func (stubTokens) LookupAgentToken(context.Context, []byte) (authn.AgentToken, error) { + return authn.AgentToken{}, authn.ErrUnknownToken +} + +func agent() authn.Principal { + return authn.Principal{Kind: authn.KindAgent, Owner: "bigbes", Agent: "claude-code", Session: "s1"} +} + +const specPath = "/v1/spaces/~bigbes/rfcs/docs/specs/0007-storage.md" + +// TestPutOpensProposal proves the whole open path: a PUT with a body and an +// If-Match opens a proposal, returns 201 with the proposal and its url, and +// forwards every field to the service. +func TestPutOpensProposal(t *testing.T) { + w := &fakeWriter{res: service.ProposeResult{ + Proposal: service.Proposal{ID: 42, Branch: "proposals/42", BaseRev: "deadbeef", State: core.StateOpen}, + URL: "https://spec.srht.bigb.es/~bigbes/rfcs/p/42", + }} + h := router(t, w, agent()) + + req := httptest.NewRequest(http.MethodPut, specPath+"?title=Storage&message=add+it", strings.NewReader("the document")) + req.Header.Set("If-Match", "1f0c1d1a") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201; body %s", rec.Code, rec.Body) + } + var body struct { + Proposal int `json:"proposal"` + URL string `json:"url"` + State string `json:"state"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("decode body: %v", err) + } + if body.Proposal != 42 || !strings.HasSuffix(body.URL, "/p/42") || body.State != "open" { + t.Fatalf("body = %+v, want proposal 42 open with its url", body) + } + + // The request the service saw. + g := w.got + if g.Space != (core.SpaceRef{Owner: "bigbes", Name: "rfcs"}) { + t.Errorf("space = %v", g.Space) + } + if g.IfMatch != "1f0c1d1a" || g.Title != "Storage" || g.Message != "add it" { + t.Errorf("forwarded fields wrong: %+v", g) + } + if g.ProposalID != 0 { + t.Errorf("ProposalID = %d, want 0 for an open", g.ProposalID) + } + if len(g.Writes) != 1 || g.Writes[0].Path != "specs/0007-storage.md" || string(g.Writes[0].Content) != "the document" { + t.Errorf("writes = %+v", g.Writes) + } + if g.Principal != agent() { + t.Errorf("principal = %+v, want the agent on the context", g.Principal) + } +} + +// TestPutAddsToExistingProposal proves X-Proposal routes to an add and returns +// 200 rather than 201. +func TestPutAddsToExistingProposal(t *testing.T) { + w := &fakeWriter{res: service.ProposeResult{ + Proposal: service.Proposal{ID: 7, Branch: "proposals/7", State: core.StateOpen}, + URL: "https://spec.srht.bigb.es/~bigbes/rfcs/p/7", + }} + h := router(t, w, agent()) + + req := httptest.NewRequest(http.MethodPut, specPath, strings.NewReader("doc")) + req.Header.Set("If-Match", "1f0c1d1a") + req.Header.Set("X-Proposal", "7") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body %s", rec.Code, rec.Body) + } + if w.got.ProposalID != 7 { + t.Errorf("ProposalID = %d, want 7", w.got.ProposalID) + } +} + +// TestPutRequiresIfMatch refuses a write with no base. +func TestPutRequiresIfMatch(t *testing.T) { + w := &fakeWriter{} + h := router(t, w, agent()) + req := httptest.NewRequest(http.MethodPut, specPath, strings.NewReader("doc")) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400", rec.Code) + } + if w.got.IfMatch != "" || w.got.Space != (core.SpaceRef{}) { + t.Fatal("service was called despite a missing If-Match") + } +} + +// TestPutRejectsBadProposalHeader refuses a non-numeric X-Proposal. +func TestPutRejectsBadProposalHeader(t *testing.T) { + h := router(t, &fakeWriter{}, agent()) + req := httptest.NewRequest(http.MethodPut, specPath, strings.NewReader("doc")) + req.Header.Set("If-Match", "1f0c1d1a") + req.Header.Set("X-Proposal", "not-a-number") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400", rec.Code) + } +} + +// TestPutMapsServiceErrors proves each service sentinel reaches the right status. +func TestPutMapsServiceErrors(t *testing.T) { + tests := []struct { + name string + err error + want int + }{ + {"forbidden", service.ErrForbidden, http.StatusForbidden}, + {"invalid", service.ErrInvalid, http.StatusUnprocessableEntity}, + {"stale", service.ErrStale, http.StatusConflict}, + {"already merged", service.ErrAlreadyMerged, http.StatusConflict}, + {"not open", service.ErrProposalNotOpen, http.StatusConflict}, + {"not found", service.ErrNotFound, http.StatusNotFound}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + h := router(t, &fakeWriter{err: tc.err}, agent()) + req := httptest.NewRequest(http.MethodPut, specPath, strings.NewReader("doc")) + req.Header.Set("If-Match", "1f0c1d1a") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != tc.want { + t.Fatalf("status = %d, want %d", rec.Code, tc.want) + } + }) + } +} + +// TestPutDecodesEncodedPath proves a percent-encoded document path reaches the +// service decoded, so a name with a space or a Cyrillic letter is addressable. +func TestPutDecodesEncodedPath(t *testing.T) { + w := &fakeWriter{res: service.ProposeResult{Proposal: service.Proposal{ID: 1, State: core.StateOpen}}} + h := router(t, w, agent()) + req := httptest.NewRequest(http.MethodPut, "/v1/spaces/~bigbes/rfcs/docs/notes/hello%20world.md", strings.NewReader("doc")) + req.Header.Set("If-Match", "1f0c1d1a") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201; body %s", rec.Code, rec.Body) + } + if len(w.got.Writes) != 1 || w.got.Writes[0].Path != "notes/hello world.md" { + t.Fatalf("path = %q, want decoded", w.got.Writes[0].Path) + } +} diff --git a/api/propose.go b/api/propose.go new file mode 100644 index 0000000000000000000000000000000000000000..d28d15da863ca2c815c8c87bfc327a0c220ce0f7 --- /dev/null +++ b/api/propose.go @@ -0,0 +1,139 @@ +package api + +import ( + "io" + "net/http" + "net/url" + "strconv" + "strings" + + "github.com/go-chi/chi/v5" + + "sourcecraft.dev/bigbes/sr-ht-spec/authn" + "sourcecraft.dev/bigbes/sr-ht-spec/core" + "sourcecraft.dev/bigbes/sr-ht-spec/service" +) + +// handlePut is the write plane's one handler: PUT a whole document to open or +// extend a proposal. +// +// It parses the request into a service.ProposeRequest and calls through — the +// document path from the URL, the base from If-Match, the optional target +// proposal from X-Proposal, the title/rationale/message from the query string, +// and the document from the body. The status is 201 when a new proposal was +// opened and 200 when documents were added to an existing one; the merged field +// says whether policy landed it immediately. +func (s *Server) handlePut(w http.ResponseWriter, r *http.Request) { + ref := core.SpaceRef{Owner: chi.URLParam(r, "owner"), Name: chi.URLParam(r, "space")} + + docPath, ok := unescapePath(chi.URLParam(r, "*")) + if !ok || docPath == "" { + writeJSON(w, http.StatusBadRequest, map[string]string{ + "error": "the document path after /docs/ is missing or malformed", + }) + return + } + + base := strings.TrimSpace(r.Header.Get("If-Match")) + if base == "" { + writeJSON(w, http.StatusBadRequest, map[string]string{ + "error": "If-Match is required: send the approved-head revision you read at, so the " + + "proposal has a base and a concurrent change cannot be clobbered", + }) + return + } + + proposalID, ok := parseProposalHeader(r.Header.Get("X-Proposal")) + if !ok { + writeJSON(w, http.StatusBadRequest, map[string]string{ + "error": "X-Proposal must be a positive proposal id; omit it to open a new proposal", + }) + return + } + + body, err := io.ReadAll(http.MaxBytesReader(w, r.Body, maxBodyBytes)) + if err != nil { + // MaxBytesReader signals an over-limit body through a read error; there + // is no way to tell it from a truncated client here, so both are 413. + writeJSON(w, http.StatusRequestEntityTooLarge, map[string]string{ + "error": "the document body could not be read or exceeds the size limit", + }) + return + } + + // The acting agent is resolved from the bearer token by the middleware. + // service.Propose refuses a non-agent, so an anonymous caller is a 403 there + // rather than a check duplicated here. + principal := authn.PrincipalFromContext(r.Context()) + + res, err := s.writer.Propose(r.Context(), service.ProposeRequest{ + Space: ref, + Principal: principal, + ProposalID: proposalID, + Title: strings.TrimSpace(r.URL.Query().Get("title")), + Rationale: strings.TrimSpace(r.URL.Query().Get("rationale")), + IfMatch: base, + Message: strings.TrimSpace(r.URL.Query().Get("message")), + Writes: []service.DocumentWrite{{Path: docPath, Content: body}}, + }) + if err != nil { + writeError(w, err) + return + } + + status := http.StatusOK + if proposalID == 0 { + status = http.StatusCreated + } + writeJSON(w, status, proposeResponse{ + Proposal: res.Proposal.ID, + URL: res.URL, + Merged: res.Merged, + State: string(res.Proposal.State), + Branch: res.Proposal.Branch, + BaseRev: res.Proposal.BaseRev, + }) +} + +// parseProposalHeader reads the optional X-Proposal header. An empty header +// means "open a new proposal" and is valid; a present value must be a positive +// integer. It returns the id and whether the header was well-formed. +func parseProposalHeader(v string) (int, bool) { + v = strings.TrimSpace(v) + if v == "" { + return 0, true + } + id, err := strconv.Atoi(v) + if err != nil || id <= 0 { + return 0, false + } + return id, true +} + +// unescapePath decodes a chi trailing wildcard back into a document path, +// per segment. +// +// chi routes on the raw (percent-encoded) path when the request had one, so a +// document whose name carries a space or a Cyrillic letter arrives encoded. +// Decoding per segment is deliberate: a %2F inside a segment is a literal slash +// in a name, not a path separator, and joining decoded segments with "/" keeps +// it from becoming one. It mirrors web/'s unescapePath so the read and write +// surfaces address a document by exactly the same path grammar. +func unescapePath(raw string) (string, bool) { + if raw == "" { + return "", true + } + segs := strings.Split(raw, "/") + for i, seg := range segs { + dec, err := url.PathUnescape(seg) + if err != nil { + return "", false + } + segs[i] = dec + } + return strings.Join(segs, "/"), true +} + +// compile-time assertion that the production service satisfies the write this +// surface needs. +var _ Writer = (*service.Service)(nil) diff --git a/cmd/specsrht/main.go b/cmd/specsrht/main.go index 80a18b02143b668ff52bec6f934ad6d023c47f1b..301903495473f58cf62fc2481872d73feccdd522 100644 --- a/cmd/specsrht/main.go +++ b/cmd/specsrht/main.go @@ -68,6 +68,7 @@ import ( "sourcecraft.dev/bigbes/sr-ht-core/config" coreserver "sourcecraft.dev/bigbes/sr-ht-core/server" + "sourcecraft.dev/bigbes/sr-ht-spec/api" "sourcecraft.dev/bigbes/sr-ht-spec/core" "sourcecraft.dev/bigbes/sr-ht-spec/graph" "sourcecraft.dev/bigbes/sr-ht-spec/hooks" @@ -436,6 +437,7 @@ type surfaces struct { web *web.Server mcp http.Handler gql http.Handler + api http.Handler } // newSurfaces opens the index and builds the three read surfaces over it. @@ -493,7 +495,13 @@ func newSurfaces(conf ini.File, cfg service.Config, svc *service.Service, versio return nil, fmt.Errorf("assemble the GraphQL surface: %w", err) } - return &surfaces{index: index, web: site, mcp: mcp, gql: gql.Handler()}, nil + rest, err := api.New(api.Options{Writer: svc, Resolver: svc.Resolver()}) + if err != nil { + index.Close() + return nil, fmt.Errorf("assemble the REST write surface: %w", err) + } + + return &surfaces{index: index, web: site, mcp: mcp, gql: gql.Handler(), api: rest.Handler()}, nil } func (s *surfaces) Close() error { @@ -503,11 +511,13 @@ func (s *surfaces) Close() error { return s.index.Close() } -// mountWeb attaches the three read surfaces. +// mountWeb attaches the HTTP surfaces: the three read surfaces (web, MCP, +// GraphQL) and the REST write plane. // -// Order is load-bearing: /mcp and /query are registered before the web UI, -// which mounts at "/" and would otherwise swallow them as document paths — -// "spec" and "query" are legal space names as far as the router is concerned. +// Order is load-bearing: /mcp, /query and /api are registered before the web +// UI, which mounts at "/" and would otherwise swallow them as document paths — +// "spec", "query" and "api" are all legal space names as far as the router is +// concerned. // // Each surface installs its own authentication (they are fail-closed and agree // on one ACL), so core-go's WithDefaultMiddleware is deliberately not used: its @@ -519,6 +529,7 @@ func mountWeb(router chi.Router, _ ini.File, s *surfaces) { } router.Handle("/mcp", s.mcp) router.Handle("/query", s.gql) + router.Mount("/api", s.api) router.Mount("/", s.web.Handler()) } diff --git a/service/propose.go b/service/propose.go index a2805324ead8a895635470aa68b1b954d05d13cd..67b2613b320f4749c6c30d9721639d37c2e2daa2 100644 --- a/service/propose.go +++ b/service/propose.go @@ -102,7 +102,7 @@ func (s *Service) Propose(ctx context.Context, req ProposeRequest) (ProposeResul return ProposeResult{}, fmt.Errorf("%w: %s may not propose; proposing is agent-only", ErrForbidden, req.Principal) } if len(req.Writes) == 0 { - return ProposeResult{}, fmt.Errorf("%w: a proposal must write at least one document", ErrForbidden) + return ProposeResult{}, fmt.Errorf("%w: a proposal must write at least one document", ErrInvalid) } sp, err := s.OpenSpace(ctx, req.Space) @@ -149,7 +149,7 @@ func (s *Service) Propose(ctx context.Context, req ProposeRequest) (ProposeResul // insert, branch cut, and the provenance-stamped commit. func (s *Service) openNewProposal(ctx context.Context, sp *Space, req ProposeRequest, baseHash plumbing.Hash) (*db.Proposal, error) { if req.Title == "" { - return nil, fmt.Errorf("%w: opening a proposal requires a title", ErrForbidden) + return nil, fmt.Errorf("%w: opening a proposal requires a title", ErrInvalid) } base := baseHash.String() @@ -255,11 +255,11 @@ func (s *Service) addToProposal(ctx context.Context, sp *Space, req ProposeReque func (s *Service) agentCommit(sp *Space, req ProposeRequest, base string) (gitx.CommitMeta, error) { write, err := req.Principal.AgentWriteFor(base) if err != nil { - return gitx.CommitMeta{}, fmt.Errorf("%w: %v", ErrForbidden, err) + return gitx.CommitMeta{}, fmt.Errorf("%w: %v", ErrInvalid, err) } prov, err := s.cfg.Instance.Provenance(write) if err != nil { - return gitx.CommitMeta{}, fmt.Errorf("%w: %v", ErrForbidden, err) + return gitx.CommitMeta{}, fmt.Errorf("%w: %v", ErrInvalid, err) } message := req.Message @@ -268,7 +268,7 @@ func (s *Service) agentCommit(sp *Space, req ProposeRequest, base string) (gitx. } if message == "" { return gitx.CommitMeta{}, - fmt.Errorf("%w: a write needs a commit message (or a title to borrow one from)", ErrForbidden) + fmt.Errorf("%w: a write needs a commit message (or a title to borrow one from)", ErrInvalid) } when := s.now().UTC() @@ -302,21 +302,21 @@ func (s *Service) validateWrites(ctx context.Context, sp *Space, writes []Docume seen := make(map[string]string, len(writes)) for _, w := range writes { if err := core.ValidateDocPath(w.Path); err != nil { - return fmt.Errorf("%w: %v", ErrForbidden, err) + return fmt.Errorf("%w: %v", ErrInvalid, err) } fm, _, err := core.ParseDocument(w.Content) if err != nil { - return fmt.Errorf("%w: %s: %v", ErrForbidden, w.Path, err) + return fmt.Errorf("%w: %s: %v", ErrInvalid, w.Path, err) } if err := policy.Schema.ValidateFrontmatter(fm); err != nil { - return fmt.Errorf("%w: %s: %v", ErrForbidden, w.Path, err) + return fmt.Errorf("%w: %s: %v", ErrInvalid, w.Path, err) } id, err := core.ParseDocID(fm.ID) if err != nil { - return fmt.Errorf("%w: %s: %v", ErrForbidden, w.Path, err) + return fmt.Errorf("%w: %s: %v", ErrInvalid, w.Path, err) } if prev, dup := seen[id.String()]; dup { - return fmt.Errorf("%w: %s and %s both carry id %s", ErrForbidden, prev, w.Path, id) + return fmt.Errorf("%w: %s and %s both carry id %s", ErrInvalid, prev, w.Path, id) } seen[id.String()] = w.Path } diff --git a/service/propose_test.go b/service/propose_test.go index b7288c68bdf30a173818a021e16c1dbfb420b74b..40e9a2b79723394fc4699fff82e096e329281a85 100644 --- a/service/propose_test.go +++ b/service/propose_test.go @@ -53,8 +53,8 @@ func TestProposeRejectsEmptyWrites(t *testing.T) { Title: "t", IfMatch: headRev, }) - if !errors.Is(err, ErrForbidden) { - t.Fatalf("Propose with no writes: err = %v, want ErrForbidden", err) + if !errors.Is(err, ErrInvalid) { + t.Fatalf("Propose with no writes: err = %v, want ErrInvalid", err) } } @@ -124,8 +124,8 @@ func TestValidateWrites(t *testing.T) { if !tc.wantErr && err != nil { t.Fatalf("validateWrites(%s) = %v, want nil", tc.name, err) } - if tc.wantErr && err != nil && !errors.Is(err, ErrForbidden) { - t.Fatalf("validateWrites(%s) err = %v, want ErrForbidden", tc.name, err) + if tc.wantErr && err != nil && !errors.Is(err, ErrInvalid) { + t.Fatalf("validateWrites(%s) err = %v, want ErrInvalid", tc.name, err) } }) } diff --git a/service/service.go b/service/service.go index cc08b5bb272d80ca70a3e1f8783f90a2026b9cdf..a15e70fde585311cb4d55c95593a3e3084c87148 100644 --- a/service/service.go +++ b/service/service.go @@ -52,6 +52,15 @@ var ( // a refusal of the principal, not of the request. ErrForbidden = errors.New("service: forbidden") + // ErrInvalid marks a write the principal may make but the request itself is + // malformed: a document whose frontmatter does not parse or fails the + // schema, a path outside the tree, two uploads claiming one id, an open with + // no title, a write with no documents. It is the write plane's 400/422, kept + // apart from ErrForbidden so a surface does not answer "bad document" with + // "you are not allowed" — a distinction that matters most to the agent that + // has to fix and retry. + ErrInvalid = errors.New("service: invalid request") + // ErrStale marks a proposal whose base moved under it: the write plane's // 409. It wraps a gitx staleness reason, so a caller that wants to tell the // agent which document went stale type-asserts to *gitx.StaleError; one that