From 65eac3cf812b912b1cdc5b160de08b569ac54510 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Thu, 23 Jul 2026 08:16:56 +0300 Subject: [PATCH] =?UTF-8?q?feat(service):=20write=20plane=20=E2=80=94=20Pr?= =?UTF-8?q?opose,=20Merge,=20ListProposals=20(Phase=203)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The service-layer orchestration for spec-zqb: the plane where the merge model and the proposal state machine first run under real proposals rather than on paper. Primitives (db proposal CRUD/merge, gitx branch/commit/merge, authn provenance, core policy matcher) already existed and were unit-tested; this composes them. - Propose: the write plane, identical for REST and MCP. Row-first open (branch name derives from the serial id), branch cut, and a provenance-stamped commit — the agent authors, the owner commits, and the X-Agent-Session / X-Agent-Base trailers carry the rest into a plain git log. Adds to an existing proposal via ProposalID against its fixed base. Frontmatter/schema/id validation at propose time, mirroring the update hook the in-process agent write bypasses. Returns {proposal, url}. - Auto-merge policy: a proposal whose every changed path matches the space's .spec.yml auto_merge lands immediately with ApprovalPolicy, best-effort — a stale or mixed-path proposal falls back to human review rather than failing the write. - Merge / Reject: the state machine. Merge does the already-merged ancestry check before gitx's If-Match staleness (design: the two need different tests), the owner-signed merge commit, and the atomic row + document-registry flip. ErrStale / ErrAlreadyMerged / ErrForbidden map the 409/403 boundary. - ListProposals / GetProposal / ProposalURL: the space-scoped read the graph Proposals port declared and left nil, plus the stable /~owner/space/p/ link. - db.ListProposalsBySpace: the per-space, per-state listing. Tested end to end against Postgres: open, auto-merge, mixed-path fallback, stale-base 409, human merge, reject, add-to-existing, drifted base. Surfaces (graph wiring, spec_propose, REST api/) fan out next. --- db/proposal.go | 32 ++- service/merge.go | 201 +++++++++++++++++++ service/merge_test.go | 195 ++++++++++++++++++ service/proposals.go | 125 ++++++++++++ service/propose.go | 423 ++++++++++++++++++++++++++++++++++++++++ service/propose_test.go | 344 ++++++++++++++++++++++++++++++++ service/service.go | 25 +++ 7 files changed, 1344 insertions(+), 1 deletion(-) create mode 100644 service/merge.go create mode 100644 service/merge_test.go create mode 100644 service/proposals.go create mode 100644 service/propose.go create mode 100644 service/propose_test.go diff --git a/db/proposal.go b/db/proposal.go index 9a302901d1adfb89ea04036132e9a3aa5bdcfd86..03b9284652062b1b4399a08dfdcea39439bfc0e8 100644 --- a/db/proposal.go +++ b/db/proposal.go @@ -173,9 +173,39 @@ func (s *Store) ListProposalsByState(ctx context.Context, state core.ProposalSta q += ` LIMIT $2` args = append(args, limit) } + return s.queryProposals(ctx, q, args...) +} + +// ListProposalsBySpace lists one space's proposals in one state, newest first — +// the per-space proposal listing the read surfaces serve ("the open proposals +// on ~bigbes/rfcs"). ListProposalsByState is the instance-wide inbox; this is +// its space-scoped counterpart, which is the shape every surface above service/ +// asks for, because a proposal is only ever meaningful inside its space. +// +// The (state, created DESC) index still serves this: the extra space_id +// predicate is a filter over a set that is already tiny at this instance's +// volume. limit <= 0 means no limit. +func (s *Store) ListProposalsBySpace(ctx context.Context, spaceID int, state core.ProposalState, limit int) ([]*Proposal, error) { + if _, err := core.ParseProposalState(string(state)); err != nil { + return nil, err + } + q := proposalSelect + ` WHERE space_id = $1 AND state = $2 ORDER BY created DESC, id DESC` + args := []any{spaceID, string(state)} + if limit > 0 { + q += ` LIMIT $3` + args = append(args, limit) + } + return s.queryProposals(ctx, q, args...) +} + +// queryProposals runs a proposalSelect query and scans every row. The two +// listings above differ only in their WHERE and args, so the row loop — the +// part that is easy to get subtly wrong (a missing rows.Err, a leaked cursor) — +// lives in one place. +func (s *Store) queryProposals(ctx context.Context, q string, args ...any) ([]*Proposal, error) { rows, err := s.q.QueryContext(ctx, q, args...) if err != nil { - return nil, fmt.Errorf("list proposals state=%s: %w", state, err) + return nil, fmt.Errorf("list proposals: %w", err) } defer rows.Close() var out []*Proposal diff --git a/service/merge.go b/service/merge.go new file mode 100644 index 0000000000000000000000000000000000000000..a39488e31ba38e4bc8ff4fd770a1eea0f83872d8 --- /dev/null +++ b/service/merge.go @@ -0,0 +1,201 @@ +package service + +import ( + "context" + "errors" + "fmt" + + "sourcecraft.dev/bigbes/sr-ht-spec/core" + "sourcecraft.dev/bigbes/sr-ht-spec/db" + "sourcecraft.dev/bigbes/sr-ht-spec/gitx" +) + +// Merge lands an open proposal onto the space's approved head and records how it +// was authorized. +// +// This is the state machine the design calls "approve merges immediately": +// there is no separate approved-then-merged step, because with one reviewer a +// merge is the approval. Phase 4's browser button calls it with ApprovalHuman; +// auto-merge policy calls the same path with ApprovalPolicy (see Propose), so +// the merge model runs identically whether a human clicked or a pattern +// matched — the only difference is the approval kind recorded, which is what a +// reader needs to tell reviewed content from firehose. +// +// The failure modes are the design's: a base that moved under the proposal is +// ErrStale (type-assert the chain to *gitx.StaleError for which document), and a +// proposal already in the approved history is ErrAlreadyMerged rather than a +// confusing staleness 409. +func (s *Service) Merge(ctx context.Context, ref core.SpaceRef, proposalID int, approval core.Approval) (Proposal, error) { + sp, row, err := s.openProposalRow(ctx, ref, proposalID) + if err != nil { + return Proposal{}, err + } + return s.mergeProposal(ctx, sp, row, approval) +} + +// Reject resolves an open proposal to rejected. There is no request-changes +// cycle — with one reviewer, a proposal you dislike is rejected and the agent +// proposes again — so this is the whole of the "do not merge" path. The branch +// and row are kept: the proposal URL still resolves and shows the outcome. +func (s *Service) Reject(ctx context.Context, ref core.SpaceRef, proposalID int) (Proposal, error) { + _, row, err := s.openProposalRow(ctx, ref, proposalID) + if err != nil { + return Proposal{}, err + } + if err := s.store.RejectProposal(ctx, row.ID); err != nil { + return Proposal{}, resolveProposalErr(err, row.ID) + } + return s.GetProposal(ctx, row.ID) +} + +// mergeProposal is the merge itself, shared by the public Merge and by +// auto-merge. It assumes row is the proposal's current row and sp its open +// space. +func (s *Service) mergeProposal(ctx context.Context, sp *Space, row *db.Proposal, approval core.Approval) (Proposal, error) { + if _, err := core.ParseApproval(string(approval)); err != nil { + return Proposal{}, err + } + if row.State != core.StateOpen { + return Proposal{}, fmt.Errorf("%w: proposal %d is %s", ErrProposalNotOpen, row.ID, row.State) + } + + head, err := sp.Repo.ApprovedHead(ctx) + if err != nil { + return Proposal{}, readErr(err, "read approved head of %s", sp.Ref) + } + proposalHead, err := sp.Repo.BranchHead(ctx, row.Branch) + if err != nil { + return Proposal{}, readErr(err, "read head of %s in %s", row.Branch, sp.Ref) + } + base, err := sp.Repo.ResolveRev(ctx, row.BaseRev) + if err != nil { + return Proposal{}, readErr(err, "resolve base %s of %s in %s", row.BaseRev, row.Branch, sp.Ref) + } + + // A branch still sitting on its base carries no commits. It is neither + // mergeable nor "already merged": its tip is trivially an ancestor of the + // approved head (the branch was cut there), which the ancestry check below + // would misread as a completed merge. Say "nothing to merge" first — the + // same qualification PlanRepairs makes for exactly this reason. + if proposalHead == base { + return Proposal{}, fmt.Errorf("service: proposal %d has no changes to merge: %w", + row.ID, gitx.ErrUnsupportedChange) + } + + // Already-merged proposals need an ancestry check, not a staleness check + // (design): once the approved head carries the proposal's own blobs, the + // blob comparison is trivially "changed" and gitx.Merge would return a + // confusing 409. Testing IsAncestor(proposalHead, head) first is the only + // thing that tells "landed" from "conflicts". + already, err := sp.Repo.IsAncestor(ctx, proposalHead, head) + if err != nil { + return Proposal{}, readErr(err, "ancestry of %s in %s", row.Branch, sp.Ref) + } + if already { + return Proposal{}, fmt.Errorf("%w: proposal %d (%s)", ErrAlreadyMerged, row.ID, row.Branch) + } + + // The owner approves and the owner commits, so both identities are the + // instance owner. A merge carries no agent trailers: it is a human (or + // policy) act, and the agent provenance rides on the proposal's own commits, + // which stay visible as the merge commit's second parent. + sig := s.ownerSignature() + res, err := sp.Repo.Merge(ctx, gitx.MergeRequest{ + Branch: row.Branch, + Base: row.BaseRev, + Meta: gitx.CommitMeta{ + Message: fmt.Sprintf("Merge proposal %d: %s", row.ID, row.Title), + Author: sig, + Committer: sig, + }, + }) + if err != nil { + return Proposal{}, mergeErr(err, sp.Ref, row.ID) + } + + // Refs are already truth for the merge; this makes Postgres agree, and does + // the row flip and the document-registry move in one transaction so a reader + // never sees a merged proposal whose documents are still registered at their + // pre-merge paths. + docs := make([]db.DocRef, 0, len(res.Docs)) + for _, d := range res.Docs { + id, err := core.ParseDocID(d.DocID) + if err != nil { + return Proposal{}, fmt.Errorf("service: merged document %q at %q in %s carries an unusable id: %w", + d.DocID, d.Path, sp.Ref, err) + } + docs = append(docs, db.DocRef{ID: id, Path: d.Path}) + } + mergedRev := res.Commit.String() + if err := s.store.MergeProposal(ctx, db.Merge{ + ProposalID: row.ID, + SpaceID: sp.ID, + Approval: approval, + MergedRev: mergedRev, + Docs: docs, + }); err != nil { + // The ref moved but the row did not: exactly the crash state the + // reconciler repairs (RepairMarkMerged). Surface it rather than + // reporting a failed merge — the merge commit is on the approved branch + // and reads already see it. + return Proposal{}, fmt.Errorf("service: proposal %d merged to %s in %s but its row could not be updated "+ + "(the reconciler will repair it): %w", row.ID, short(mergedRev), sp.Ref, err) + } + + return s.GetProposal(ctx, row.ID) +} + +// openProposalRow resolves a proposal by id within a named space, opening the +// space's repository. A proposal id that belongs to another space is reported +// as not found rather than acted on across the space boundary. +func (s *Service) openProposalRow(ctx context.Context, ref core.SpaceRef, proposalID int) (*Space, *db.Proposal, error) { + sp, err := s.OpenSpace(ctx, ref) + if err != nil { + return nil, nil, err + } + row, err := s.store.GetProposal(ctx, proposalID) + if err != nil { + if errors.Is(err, db.ErrNotFound) { + return nil, nil, fmt.Errorf("%w: proposal %d", ErrNotFound, proposalID) + } + return nil, nil, fmt.Errorf("service: look up proposal %d: %w", proposalID, err) + } + if row.SpaceID != sp.ID { + return nil, nil, fmt.Errorf("%w: proposal %d is not in %s", ErrNotFound, proposalID, ref) + } + return sp, row, nil +} + +// ownerSignature is the git identity every merge commit carries: the instance +// owner, stamped with the reconciler-injectable clock so provenance stays +// testable without sleeping. +func (s *Service) ownerSignature() gitx.Signature { + o := s.cfg.Instance.OwnerSignature() + return gitx.Signature{Name: o.Name, Email: o.Email, When: s.now().UTC()} +} + +// mergeErr maps a gitx merge failure onto this package's sentinels. A staleness +// error becomes ErrStale while keeping the *gitx.StaleError in the chain, so a +// surface that wants to tell the agent which document went stale type-asserts to +// it and one that only needs the 409 matches ErrStale. +func mergeErr(err error, ref core.SpaceRef, proposalID int) error { + var stale *gitx.StaleError + if errors.As(err, &stale) { + return fmt.Errorf("%w: proposal %d against %s: %w", ErrStale, proposalID, ref, err) + } + return fmt.Errorf("service: merge proposal %d in %s: %w", proposalID, ref, err) +} + +// resolveProposalErr maps a db resolution failure (reject) onto this package's +// sentinels: a missing row is ErrNotFound, and a row that has already merged or +// been rejected is ErrProposalNotOpen. +func resolveProposalErr(err error, proposalID int) error { + switch { + case errors.Is(err, db.ErrNotFound): + return fmt.Errorf("%w: proposal %d", ErrNotFound, proposalID) + case errors.Is(err, db.ErrProposalNotOpen): + return fmt.Errorf("%w: proposal %d", ErrProposalNotOpen, proposalID) + default: + return fmt.Errorf("service: reject proposal %d: %w", proposalID, err) + } +} diff --git a/service/merge_test.go b/service/merge_test.go new file mode 100644 index 0000000000000000000000000000000000000000..8b74d39cc6c1c72c1939e1ffd4769c47e84ee23f --- /dev/null +++ b/service/merge_test.go @@ -0,0 +1,195 @@ +package service + +import ( + "context" + "errors" + "testing" + + "sourcecraft.dev/bigbes/sr-ht-spec/core" +) + +// openProposalFor opens a proposal against the current approved head and returns +// its result. It is the common setup for the merge tests, which then act on the +// proposal the design's review plane would. +func openProposalFor(t *testing.T, svc *Service, sp *Space, path, id string, content []byte) ProposeResult { + t.Helper() + ctx := context.Background() + base, err := sp.Repo.ApprovedHead(ctx) + if err != nil { + t.Fatalf("ApprovedHead: %v", err) + } + res, err := svc.Propose(ctx, ProposeRequest{ + Space: fxSpace, + Principal: agentPrincipal(), + Title: "proposal for " + path, + IfMatch: base.String(), + Message: "write " + path, + Writes: []DocumentWrite{{Path: path, Content: content}}, + }) + if err != nil { + t.Fatalf("Propose(%s): %v", path, err) + } + return res +} + +// TestMergeLandsProposal proves the human approve path: an open proposal that no +// policy auto-merges is merged on request, recorded as human-approved, and its +// document is then readable at the approved head. +func TestMergeLandsProposal(t *testing.T) { + svc, _ := newTestService(t) + ctx := context.Background() + sp, err := svc.CreateSpace(ctx, fxSpace) + if err != nil { + t.Fatalf("CreateSpace: %v", err) + } + + res := openProposalFor(t, svc, sp, "specs/a.md", "S-1", mdDoc("S-1", "A", "body")) + if res.Merged { + t.Fatalf("specs/ proposal auto-merged without a policy") + } + + merged, err := svc.Merge(ctx, fxSpace, res.Proposal.ID, core.ApprovalHuman) + if err != nil { + t.Fatalf("Merge: %v", err) + } + if merged.State != core.StateMerged || merged.Approval != core.ApprovalHuman { + t.Fatalf("merged = %+v, want state=merged approval=human", merged) + } + + // The document is now on the approved head. + doc, err := svc.ReadDocument(ctx, sp, ApprovedRev, "specs/a.md") + if err != nil { + t.Fatalf("ReadDocument after merge: %v", err) + } + if len(doc.Data) == 0 { + t.Fatalf("merged document reads empty") + } + + // A merged proposal is no longer open, so a second resolution is refused. + if _, err := svc.Merge(ctx, fxSpace, res.Proposal.ID, core.ApprovalHuman); !errors.Is(err, ErrProposalNotOpen) { + t.Fatalf("re-merge: err = %v, want ErrProposalNotOpen", err) + } +} + +// TestRejectResolvesProposal proves reject moves an open proposal to rejected +// and leaves it listable there, its URL still resolving. +func TestRejectResolvesProposal(t *testing.T) { + svc, _ := newTestService(t) + ctx := context.Background() + sp, err := svc.CreateSpace(ctx, fxSpace) + if err != nil { + t.Fatalf("CreateSpace: %v", err) + } + res := openProposalFor(t, svc, sp, "specs/a.md", "S-1", mdDoc("S-1", "A", "body")) + + rejected, err := svc.Reject(ctx, fxSpace, res.Proposal.ID) + if err != nil { + t.Fatalf("Reject: %v", err) + } + if rejected.State != core.StateRejected { + t.Fatalf("state = %s, want rejected", rejected.State) + } + got, err := svc.GetProposal(ctx, res.Proposal.ID) + if err != nil { + t.Fatalf("GetProposal after reject: %v", err) + } + if got.State != core.StateRejected { + t.Fatalf("GetProposal state = %s, want rejected", got.State) + } +} + +// TestMergeStaleWhenDocumentChangedUnderIt proves the 409: a document the +// proposal edits, changed on the approved branch since the proposal's base, +// cannot merge — the loser refetches and re-proposes. +func TestMergeStaleWhenDocumentChangedUnderIt(t *testing.T) { + svc, _ := newTestService(t) + ctx := context.Background() + sp, err := svc.CreateSpace(ctx, fxSpace) + if err != nil { + t.Fatalf("CreateSpace: %v", err) + } + // The approved head carries v1 of the document the proposal will edit. + commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{ + "specs/a.md": mdDoc("S-1", "A", "v1"), + }) + res := openProposalFor(t, svc, sp, "specs/a.md", "S-1", mdDoc("S-1", "A", "v2-from-agent")) + + // A human pushes v3 of the same document onto the approved head, after the + // proposal's base. + commitFiles(t, sp, sp.ApprovedBranch(), 2, map[string][]byte{ + "specs/a.md": mdDoc("S-1", "A", "v3-from-human"), + }) + + _, err = svc.Merge(ctx, fxSpace, res.Proposal.ID, core.ApprovalHuman) + if !errors.Is(err, ErrStale) { + t.Fatalf("Merge of a proposal whose document moved under it: err = %v, want ErrStale", err) + } +} + +// TestProposeAddToExistingThenMerge proves the X-Proposal path: a second write +// against a proposal's fixed base adds to it, and merging then lands both +// documents at once. +func TestProposeAddToExistingThenMerge(t *testing.T) { + svc, _ := newTestService(t) + ctx := context.Background() + sp, err := svc.CreateSpace(ctx, fxSpace) + if err != nil { + t.Fatalf("CreateSpace: %v", err) + } + first := openProposalFor(t, svc, sp, "specs/a.md", "S-1", mdDoc("S-1", "A", "one")) + + // Add a second document to the same proposal, sending the same base. + second, err := svc.Propose(ctx, ProposeRequest{ + Space: fxSpace, + Principal: agentPrincipal(), + ProposalID: first.Proposal.ID, + IfMatch: first.Proposal.BaseRev, + Message: "add specs/b.md", + Writes: []DocumentWrite{{Path: "specs/b.md", Content: mdDoc("S-2", "B", "two")}}, + }) + if err != nil { + t.Fatalf("Propose add-to-existing: %v", err) + } + if second.Proposal.ID != first.Proposal.ID { + t.Fatalf("add opened a new proposal %d, want %d", second.Proposal.ID, first.Proposal.ID) + } + + if _, err := svc.Merge(ctx, fxSpace, first.Proposal.ID, core.ApprovalHuman); err != nil { + t.Fatalf("Merge: %v", err) + } + for _, path := range []string{"specs/a.md", "specs/b.md"} { + if _, err := svc.ReadDocument(ctx, sp, ApprovedRev, path); err != nil { + t.Fatalf("ReadDocument(%s) after merge: %v", path, err) + } + } +} + +// TestAddToExistingRejectsDriftedBase proves an add whose If-Match no longer +// names the proposal's base is a 409 rather than a silent write against the old +// base. +func TestAddToExistingRejectsDriftedBase(t *testing.T) { + svc, _ := newTestService(t) + ctx := context.Background() + sp, err := svc.CreateSpace(ctx, fxSpace) + if err != nil { + t.Fatalf("CreateSpace: %v", err) + } + first := openProposalFor(t, svc, sp, "specs/a.md", "S-1", mdDoc("S-1", "A", "one")) + + // The approved head moves; the agent mistakenly sends the new head as its + // base for the add. + newHead := commitFiles(t, sp, sp.ApprovedBranch(), 2, map[string][]byte{ + "specs/c.md": mdDoc("S-3", "C", "unrelated"), + }) + _, err = svc.Propose(ctx, ProposeRequest{ + Space: fxSpace, + Principal: agentPrincipal(), + ProposalID: first.Proposal.ID, + IfMatch: newHead.String(), + Message: "add specs/b.md", + Writes: []DocumentWrite{{Path: "specs/b.md", Content: mdDoc("S-2", "B", "two")}}, + }) + if !errors.Is(err, ErrStale) { + t.Fatalf("add with a drifted base: err = %v, want ErrStale", err) + } +} diff --git a/service/proposals.go b/service/proposals.go new file mode 100644 index 0000000000000000000000000000000000000000..94bc8a8e12c19d6fcb4e122534a5368cbbcd68e3 --- /dev/null +++ b/service/proposals.go @@ -0,0 +1,125 @@ +package service + +import ( + "context" + "errors" + "fmt" + "time" + + "sourcecraft.dev/bigbes/sr-ht-spec/core" + "sourcecraft.dev/bigbes/sr-ht-spec/db" +) + +// Proposal is one proposal as the surfaces above this layer need it: core +// types, its space named by reference rather than by the row's opaque id, and +// nothing from db/ or gitx/ leaking through. +// +// It is the read shape shared by every surface — GraphQL's `proposals` field, +// the MCP and REST write responses, the review page — so that "what a proposal +// is" has one spelling above service/. db.Proposal is the storage shape and +// stays in db/; the mapping between them is proposalView, in this package, +// because the dependency rule keeps db/ types out of every caller. +type Proposal struct { + ID int + Space core.SpaceRef + Title string + Rationale string + BaseRev string + Branch string + State core.ProposalState + Approval core.Approval + MergedRev string + Agent string + AgentSession string + Created time.Time + Resolved *time.Time +} + +// ProposalURL is the stable, shareable link to a proposal: the value every +// write response hands back so an agent can surface it in its transcript. +// +// The form is /~owner/space/p/, and it is built here rather than in +// each surface because the origin is service/'s config and a second surface +// spelling the path would be a link that resolves on one door and 404s on +// another. It resolves after merge or rejection too — proposal URLs outlive the +// branch — so it is the same URL whatever the proposal's state. +func (s *Service) ProposalURL(ref core.SpaceRef, id int) string { + return fmt.Sprintf("%s/~%s/%s/p/%d", s.cfg.Origin, ref.Owner, ref.Name, id) +} + +// proposalView maps a stored proposal onto the surface shape, naming its space +// by the reference the caller already resolved rather than re-reading the row's +// space_id. +func proposalView(p *db.Proposal, ref core.SpaceRef) Proposal { + return Proposal{ + ID: p.ID, + Space: ref, + Title: p.Title, + Rationale: p.Rationale, + BaseRev: p.BaseRev, + Branch: p.Branch, + State: p.State, + Approval: p.Approval, + MergedRev: p.MergedRev, + Agent: p.Agent, + AgentSession: p.AgentSession, + Created: p.Created, + Resolved: p.Resolved, + } +} + +// ListProposals returns a space's proposals in one state, newest first. +// +// This is the read the GraphQL `proposals` field, the inbox and the review UI +// all call — the space-scoped listing the design puts in the read schema. It +// exists here, in service/, because nothing above this layer may query db/ +// directly: the port graph/ declared and left nil until Phase 3 is this +// method. +// +// The space is resolved by reference to its row so the listing filters by +// space_id, and a space that does not exist is ErrNotFound rather than an empty +// list — "no such space" and "this space has an empty queue" are different +// answers, and a surface that conflated them would tell a reviewer their queue +// is clear when the space name was simply wrong. +func (s *Service) ListProposals(ctx context.Context, ref core.SpaceRef, state core.ProposalState) ([]Proposal, error) { + if _, err := core.ParseProposalState(string(state)); err != nil { + return nil, err + } + row, err := s.store.GetSpace(ctx, ref) + if err != nil { + if errors.Is(err, db.ErrNotFound) { + return nil, fmt.Errorf("%w: space %s", ErrNotFound, ref) + } + return nil, fmt.Errorf("service: look up space %s: %w", ref, err) + } + rows, err := s.store.ListProposalsBySpace(ctx, row.ID, state, 0) + if err != nil { + return nil, fmt.Errorf("service: list %s proposals of %s: %w", state, ref, err) + } + out := make([]Proposal, 0, len(rows)) + for _, p := range rows { + out = append(out, proposalView(p, ref)) + } + return out, nil +} + +// GetProposal resolves one proposal by id, naming its space by reference. +// +// It is the lookup the stable proposal URL resolves through, so it works in +// every state: a link to a merged or rejected proposal still shows the outcome. +// The space is resolved from the row's space_id back to a reference so no +// caller above this layer has to hold the opaque id. +func (s *Service) GetProposal(ctx context.Context, id int) (Proposal, error) { + row, err := s.store.GetProposal(ctx, id) + if err != nil { + if errors.Is(err, db.ErrNotFound) { + return Proposal{}, fmt.Errorf("%w: proposal %d", ErrNotFound, id) + } + return Proposal{}, fmt.Errorf("service: look up proposal %d: %w", id, err) + } + space, err := s.store.GetSpaceByID(ctx, row.SpaceID) + if err != nil { + return Proposal{}, fmt.Errorf("service: resolve space %d of proposal %d: %w", row.SpaceID, id, err) + } + return proposalView(row, space.Ref), nil +} diff --git a/service/propose.go b/service/propose.go new file mode 100644 index 0000000000000000000000000000000000000000..a2805324ead8a895635470aa68b1b954d05d13cd --- /dev/null +++ b/service/propose.go @@ -0,0 +1,423 @@ +package service + +import ( + "context" + "errors" + "fmt" + + "github.com/go-git/go-git/v5/plumbing" + + "sourcecraft.dev/bigbes/sr-ht-spec/authn" + "sourcecraft.dev/bigbes/sr-ht-spec/core" + "sourcecraft.dev/bigbes/sr-ht-spec/db" + "sourcecraft.dev/bigbes/sr-ht-spec/gitx" +) + +// DocumentWrite is one whole-document upload: a path and its complete bytes. +// There is no patch form — the write plane takes whole documents because that +// is how agents work, and it is what makes the merge model pure plumbing. +type DocumentWrite struct { + Path string + Content []byte +} + +// ProposeRequest is one call to the write plane, identical across REST and MCP: +// an agent uploads whole documents against a base revision, either opening a new +// proposal or adding to one it already owns. +type ProposeRequest struct { + // Space is the space being written to. + Space core.SpaceRef + + // Principal is who is writing, as authn resolved the bearer token. It must + // be an agent: proposing is an agent-only act, and the human write path is + // native receive-pack. + Principal authn.Principal + + // ProposalID selects an existing open proposal to add to (the REST + // X-Proposal header). Zero opens a new one. + ProposalID int + + // Title and Rationale describe a new proposal. Title is required when + // opening and ignored when adding. + Title string + Rationale string + + // IfMatch is the approved-head sha the agent read the document at — the base + // B. One value, one meaning across REST and MCP: opening cuts the branch + // from it, adding is validated against the proposal's fixed B. It may be + // abbreviated; it is resolved to a full object name before anything is + // stored. + IfMatch string + + // Message is the commit subject and body for this write. Empty defaults to + // the title when opening; a write that is neither given a message nor a + // title is refused, because a commit whose only content is provenance + // records that something happened without saying what. + Message string + + // Writes are the whole documents this call uploads. At least one is + // required. + Writes []DocumentWrite +} + +// ProposeResult is what every write returns. The URL is the whole point of the +// review plane's entry contract: the agent hands a human a link, so a write +// that did not surface one would make the work invisible. +type ProposeResult struct { + // Proposal is the proposal as it now stands — merged already when + // auto-merge policy landed it, otherwise open. + Proposal Proposal + + // URL is the stable, shareable proposal link. + URL string + + // Merged reports whether auto-merge policy landed this write immediately. + // It lets a caller phrase its message ("merged" vs "proposed") without + // re-reading the state. + Merged bool +} + +// Propose is the write plane: an agent uploads whole documents and gets back a +// proposal and its URL. +// +// The shape is the design's, and the ordering is load-bearing: +// +// - The base is resolved and, when opening, checked to be an ancestor of the +// approved head — the open-time 409. Adding is validated against the +// proposal's fixed base instead, which does not move as edits accumulate. +// - A new proposal is row-first (the branch name derives from the row's serial +// id), then its branch is cut, then the documents are committed onto it with +// the agent's provenance in the commit trailers. +// - Auto-merge policy is evaluated against everything the proposal changes. If +// every changed path may skip review, the proposal is merged immediately +// with ApprovalPolicy; otherwise it stays open for a human. +// +// Auto-merge is best-effort: if the immediate merge cannot land (the approved +// branch moved under the proposal between the commit and the merge), the +// proposal is left open for review rather than failing the write — falling back +// to human review is the safe direction, and the policy-merged digest exists to +// surface it either way. +func (s *Service) Propose(ctx context.Context, req ProposeRequest) (ProposeResult, error) { + if !req.Principal.IsAgent() { + 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) + } + + sp, err := s.OpenSpace(ctx, req.Space) + if err != nil { + return ProposeResult{}, err + } + + // Resolve the agent's If-Match to a full object name up front: an + // abbreviated base that is unique today could be ambiguous later, and a + // canonical base is what the branch is cut from, what the provenance trailer + // records, and what every later add and the merge measure staleness against. + baseHash, err := sp.Repo.ResolveRev(ctx, req.IfMatch) + if err != nil { + return ProposeResult{}, readErr(err, "resolve If-Match %q in %s", req.IfMatch, req.Space) + } + base := baseHash.String() + + var row *db.Proposal + if req.ProposalID == 0 { + row, err = s.openNewProposal(ctx, sp, req, baseHash) + } else { + row, err = s.addToProposal(ctx, sp, req, base) + } + if err != nil { + return ProposeResult{}, err + } + + // Auto-merge: land immediately when every path the proposal changes may skip + // human review under the policy at the approved head. A stale or otherwise + // unlandable auto-merge leaves the proposal open — see the method doc. + merged, mergedRow := s.tryAutoMerge(ctx, sp, row) + if merged { + row = mergedRow + } + + return ProposeResult{ + Proposal: proposalView(row, sp.Ref), + URL: s.ProposalURL(sp.Ref, row.ID), + Merged: merged, + }, nil +} + +// openNewProposal opens a proposal: the open-time ancestry 409, then row-first +// 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) + } + base := baseHash.String() + + // The open-time 409: a base that is not an ancestor of the current approved + // head means the agent read a revision that the approved branch has moved + // off, so the proposal could never merge. Reject it now rather than let it + // sit open until a merge discovers it. + head, err := sp.Repo.ApprovedHead(ctx) + if err != nil { + return nil, readErr(err, "read approved head of %s", sp.Ref) + } + onBranch, err := sp.Repo.IsAncestor(ctx, baseHash, head) + if err != nil { + return nil, readErr(err, "ancestry of base %s in %s", short(base), sp.Ref) + } + if !onBranch { + return nil, fmt.Errorf("%w: base %s is not an ancestor of the approved head %s; refetch and re-propose", + ErrStale, short(base), short(head.String())) + } + + meta, err := s.agentCommit(sp, req, base) + if err != nil { + return nil, err + } + if err := s.validateWrites(ctx, sp, req.Writes, base); err != nil { + return nil, err + } + + // Row first: the branch name "proposals/" derives from the row's serial + // id, so the id must be allocated before the branch can be named. A crash + // between the insert and the branch write leaves an open row with no branch, + // which the reconciler deletes after its grace window — the agent still + // holds the document and re-proposes. + row, err := s.store.OpenProposal(ctx, &db.Proposal{ + SpaceID: sp.ID, + Title: req.Title, + Rationale: req.Rationale, + BaseRev: base, + // The raw agent identity, the way the read schema documents it + // ("claude-code/spec-writer"). The git-author annotation ("… (for + // bigbes)") is a commit-message concern and lives only in prov; storing + // it here would make the row and the design's field disagree. + Agent: req.Principal.Agent, + AgentSession: req.Principal.Session, + }) + if err != nil { + return nil, fmt.Errorf("service: open proposal in %s: %w", sp.Ref, err) + } + + if _, err := sp.Repo.CreateProposalBranch(ctx, row.Branch, base); err != nil { + return nil, fmt.Errorf("service: cut %s in %s: %w", row.Branch, sp.Ref, err) + } + if _, err := sp.Repo.CommitProposal(ctx, row.Branch, toGitxWrites(req.Writes), meta); err != nil { + return nil, fmt.Errorf("service: commit onto %s in %s: %w", row.Branch, sp.Ref, err) + } + return row, nil +} + +// addToProposal appends documents to an open proposal the agent already owns. +// The base is the proposal's fixed B, not the request's If-Match: an agent +// revising its own proposal keeps sending the same value, and a value that no +// longer matches B is a base that drifted, which is a 409. +func (s *Service) addToProposal(ctx context.Context, sp *Space, req ProposeRequest, base string) (*db.Proposal, error) { + row, err := s.store.GetProposal(ctx, req.ProposalID) + if err != nil { + if errors.Is(err, db.ErrNotFound) { + return nil, fmt.Errorf("%w: proposal %d", ErrNotFound, req.ProposalID) + } + return nil, fmt.Errorf("service: look up proposal %d: %w", req.ProposalID, err) + } + if row.SpaceID != sp.ID { + return nil, fmt.Errorf("%w: proposal %d is not in %s", ErrNotFound, req.ProposalID, sp.Ref) + } + if row.State != core.StateOpen { + return nil, fmt.Errorf("%w: proposal %d is %s", ErrProposalNotOpen, row.ID, row.State) + } + // The proposal's base does not move; the agent's If-Match must still name it. + // A different value means the agent's understanding of the base drifted, and + // silently writing against the old B anyway would let it merge a change it + // thought it made against a newer revision. + if base != row.BaseRev { + return nil, fmt.Errorf("%w: proposal %d is based on %s, not the %s you sent; adds keep the original base", + ErrStale, row.ID, short(row.BaseRev), short(base)) + } + + meta, err := s.agentCommit(sp, req, row.BaseRev) + if err != nil { + return nil, err + } + if err := s.validateWrites(ctx, sp, req.Writes, row.BaseRev); err != nil { + return nil, err + } + if _, err := sp.Repo.CommitProposal(ctx, row.Branch, toGitxWrites(req.Writes), meta); err != nil { + return nil, fmt.Errorf("service: commit onto %s in %s: %w", row.Branch, sp.Ref, err) + } + return row, nil +} + +// agentCommit builds the provenance and the gitx commit metadata for an agent +// write at base: the agent authors, the instance owner commits, and the two +// trailers carry the session and the base so the claim is auditable in a plain +// git log rather than a Postgres-only table. +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) + } + prov, err := s.cfg.Instance.Provenance(write) + if err != nil { + return gitx.CommitMeta{}, fmt.Errorf("%w: %v", ErrForbidden, err) + } + + message := req.Message + if message == "" { + message = req.Title + } + if message == "" { + return gitx.CommitMeta{}, + fmt.Errorf("%w: a write needs a commit message (or a title to borrow one from)", ErrForbidden) + } + + when := s.now().UTC() + return gitx.CommitMeta{ + Message: message, + Trailers: []gitx.Trailer{ + {Key: authn.TrailerAgentSession, Value: prov.Session}, + {Key: authn.TrailerAgentBase, Value: prov.Base}, + }, + Author: gitx.Signature{Name: prov.Author.Name, Email: prov.Author.Email, When: when}, + Committer: gitx.Signature{Name: prov.Committer.Name, Email: prov.Committer.Email, When: when}, + }, nil +} + +// validateWrites enforces at propose time what a native push has validated on +// the receive path: every uploaded document parses, satisfies the space's +// schema, and carries a well-formed id, with no two uploads claiming one id. +// An agent write reaches gitx in-process, bypassing the update hook, so this is +// the equivalent gate — without it a malformed document lands on a proposal +// branch and only fails later, at merge, with a worse message. +// +// The schema is read at the base the agent proposed against, which is what it +// read the document under. Cross-space id collisions are left to the merge's +// registry write: an open proposal that would collide is a reviewable state, not +// a reason to refuse the upload. +func (s *Service) validateWrites(ctx context.Context, sp *Space, writes []DocumentWrite, base string) error { + policy, err := s.Policy(ctx, sp, base) + if err != nil { + return err + } + 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) + } + fm, _, err := core.ParseDocument(w.Content) + if err != nil { + return fmt.Errorf("%w: %s: %v", ErrForbidden, w.Path, err) + } + if err := policy.Schema.ValidateFrontmatter(fm); err != nil { + return fmt.Errorf("%w: %s: %v", ErrForbidden, w.Path, err) + } + id, err := core.ParseDocID(fm.ID) + if err != nil { + return fmt.Errorf("%w: %s: %v", ErrForbidden, 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) + } + seen[id.String()] = w.Path + } + return nil +} + +// tryAutoMerge lands the proposal immediately when policy permits, reporting +// whether it did and the resulting row. It never returns an error: auto-merge is +// an optimization over human review, and any failure — a base that moved under +// the proposal, a policy that does not cover every changed path — leaves the +// proposal open, which is the safe fallback and where the digest picks it up. +func (s *Service) tryAutoMerge(ctx context.Context, sp *Space, row *db.Proposal) (bool, *db.Proposal) { + auto, err := s.autoMerges(ctx, sp, row) + if err != nil || !auto { + return false, nil + } + if _, err := s.mergeProposal(ctx, sp, row, core.ApprovalPolicy); err != nil { + return false, nil + } + // mergeProposal returns the surface view; re-read the row so the caller + // keeps working in the db shape it built the result from. + mergedRow, err := s.store.GetProposal(ctx, row.ID) + if err != nil { + return false, nil + } + return true, mergedRow +} + +// autoMerges reports whether every path the proposal changes may skip human +// review under the policy at the approved head. +// +// It is fail-closed in two directions. An empty changed set is not auto-merged +// (there is nothing to land), and any path that does not match is enough to +// require a human: a proposal that touches one reviewed document is reviewed as +// a whole, never split. The policy is read at the approved head because that is +// where the merge lands and whose auto_merge patterns therefore govern it. +func (s *Service) autoMerges(ctx context.Context, sp *Space, row *db.Proposal) (bool, error) { + head, err := sp.Repo.ApprovedHead(ctx) + if err != nil { + return false, err + } + policy, err := s.Policy(ctx, sp, head.String()) + if err != nil { + return false, err + } + if len(policy.Review.AutoMerge) == 0 { + return false, nil + } + proposalHead, err := sp.Repo.BranchHead(ctx, row.Branch) + if err != nil { + return false, err + } + changed, err := s.changedPaths(ctx, sp, row.BaseRev, proposalHead.String()) + if err != nil { + return false, err + } + if len(changed) == 0 { + return false, nil + } + for path := range changed { + if !policy.AutoMerges(path) { + return false, nil + } + } + return true, nil +} + +// changedPaths is the set of document paths whose blob differs between the base +// and the proposal head. It is the auto-merge decision's input, and path-keyed +// rather than id-keyed deliberately: auto_merge patterns are path patterns, and +// an agent cannot rename or delete (both are human-push-only), so a proposal's +// changes are only additions and modifications at stable paths. +func (s *Service) changedPaths(ctx context.Context, sp *Space, baseRev, proposalRev string) (map[string]bool, error) { + baseDocs, err := s.ListDocuments(ctx, sp, baseRev) + if err != nil { + return nil, err + } + headDocs, err := s.ListDocuments(ctx, sp, proposalRev) + if err != nil { + return nil, err + } + prior := make(map[string]string, len(baseDocs)) + for _, d := range baseDocs { + prior[d.Path] = d.Blob + } + changed := make(map[string]bool) + for _, d := range headDocs { + if prior[d.Path] != d.Blob { + changed[d.Path] = true + } + } + return changed, nil +} + +// toGitxWrites converts the surface write shape into gitx's. It is a straight +// field copy — the two types are kept separate only so the git layer's type +// does not leak into every surface's request struct. +func toGitxWrites(writes []DocumentWrite) []gitx.Write { + out := make([]gitx.Write, 0, len(writes)) + for _, w := range writes { + out = append(out, gitx.Write{Path: w.Path, Content: w.Content}) + } + return out +} diff --git a/service/propose_test.go b/service/propose_test.go new file mode 100644 index 0000000000000000000000000000000000000000..b7288c68bdf30a173818a021e16c1dbfb420b74b --- /dev/null +++ b/service/propose_test.go @@ -0,0 +1,344 @@ +package service + +import ( + "context" + "errors" + "strings" + "testing" + + "sourcecraft.dev/bigbes/sr-ht-spec/authn" + "sourcecraft.dev/bigbes/sr-ht-spec/core" +) + +// agentPrincipal is the resolved identity every write test proposes as: an +// agent acting for the instance owner, with the two provenance fields a write +// demands already present. +func agentPrincipal() authn.Principal { + return authn.Principal{ + Kind: authn.KindAgent, + Owner: "bigbes", + Agent: "claude-code/spec-writer", + Session: "8fb9c9a4-b078-4af1-89eb-d97c522f9921", + } +} + +func ownerPrincipal() authn.Principal { + return authn.Principal{Kind: authn.KindOwner, Owner: "bigbes"} +} + +// TestProposeRejectsNonAgent proves proposing is agent-only, and that the +// refusal happens before any space is opened — the guard is on the principal, +// not the request, so it holds even for a database that cannot be reached. +func TestProposeRejectsNonAgent(t *testing.T) { + svc, _ := newService(t) + _, err := svc.Propose(context.Background(), ProposeRequest{ + Space: fxSpace, + Principal: ownerPrincipal(), + Title: "t", + IfMatch: headRev, + Writes: []DocumentWrite{{Path: "notes/a.md", Content: mdDoc("N-1", "A", "b")}}, + }) + if !errors.Is(err, ErrForbidden) { + t.Fatalf("Propose as owner: err = %v, want ErrForbidden", err) + } +} + +// TestProposeRejectsEmptyWrites refuses a proposal that writes nothing before it +// touches the store, for the same reason. +func TestProposeRejectsEmptyWrites(t *testing.T) { + svc, _ := newService(t) + _, err := svc.Propose(context.Background(), ProposeRequest{ + Space: fxSpace, + Principal: agentPrincipal(), + Title: "t", + IfMatch: headRev, + }) + if !errors.Is(err, ErrForbidden) { + t.Fatalf("Propose with no writes: err = %v, want ErrForbidden", err) + } +} + +// TestProposalURL pins the one spelling of a proposal link every surface hands +// back. A second spelling in a surface would 404 where this resolves. +func TestProposalURL(t *testing.T) { + svc, _ := newService(t) + got := svc.ProposalURL(fxSpace, 42) + want := "https://spec.srht.bigb.es/~bigbes/rfcs/p/42" + if got != want { + t.Fatalf("ProposalURL = %q, want %q", got, want) + } +} + +// TestValidateWrites exercises the propose-time gate that a native push gets +// from the update hook: schema-valid frontmatter, a well-formed id, and no two +// uploads claiming one id. +func TestValidateWrites(t *testing.T) { + svc, root := newService(t) + sp := newSpace(t, root, 1) + ctx := context.Background() + base, err := sp.Repo.ApprovedHead(ctx) + if err != nil { + t.Fatalf("ApprovedHead: %v", err) + } + baseRev := base.String() + + tests := []struct { + name string + writes []DocumentWrite + wantErr bool + }{ + { + name: "valid document", + writes: []DocumentWrite{{Path: "notes/a.md", Content: mdDoc("N-1", "A", "body")}}, + }, + { + name: "unparseable frontmatter", + writes: []DocumentWrite{{Path: "notes/a.md", Content: []byte("no frontmatter here")}}, + wantErr: true, + }, + { + name: "missing required status key", + writes: []DocumentWrite{{Path: "notes/a.md", Content: []byte("---\nid: N-1\ntitle: A\n---\n\nbody\n")}}, + wantErr: true, + }, + { + name: "two uploads claiming one id", + writes: []DocumentWrite{ + {Path: "notes/a.md", Content: mdDoc("N-1", "A", "one")}, + {Path: "notes/b.md", Content: mdDoc("N-1", "B", "two")}, + }, + wantErr: true, + }, + { + name: "path escaping the tree", + writes: []DocumentWrite{{Path: "../secret.md", Content: mdDoc("N-1", "A", "body")}}, + wantErr: true, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := svc.validateWrites(ctx, sp, tc.writes, baseRev) + if tc.wantErr && err == nil { + t.Fatalf("validateWrites(%s) = nil, want error", tc.name) + } + 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) + } + }) + } +} + +// TestChangedPaths proves the auto-merge decision's input is the set of document +// paths whose blob moved between the base and the proposal head — an addition +// and a modification counted, an untouched document not. +func TestChangedPaths(t *testing.T) { + svc, root := newService(t) + sp := newSpace(t, root, 1) + ctx := context.Background() + + // The approved head carries two documents; the proposal modifies one and + // adds a third, leaving the second untouched. + base := commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{ + "notes/a.md": mdDoc("N-1", "A", "original"), + "specs/b.md": mdDoc("S-1", "B", "kept"), + }) + branch := "proposals/1" + cutBranch(t, sp, branch, base.String()) + commitFiles(t, sp, branch, 2, map[string][]byte{ + "notes/a.md": mdDoc("N-1", "A", "revised"), + "notes/c.md": mdDoc("N-2", "C", "new"), + }) + head, err := sp.Repo.BranchHead(ctx, branch) + if err != nil { + t.Fatalf("BranchHead: %v", err) + } + + changed, err := svc.changedPaths(ctx, sp, base.String(), head.String()) + if err != nil { + t.Fatalf("changedPaths: %v", err) + } + want := map[string]bool{"notes/a.md": true, "notes/c.md": true} + if len(changed) != len(want) { + t.Fatalf("changedPaths = %v, want %v", keysOf(changed), keysOf(want)) + } + for p := range want { + if !changed[p] { + t.Errorf("changedPaths missing %q; got %v", p, keysOf(changed)) + } + } + if changed["specs/b.md"] { + t.Errorf("changedPaths includes the untouched specs/b.md") + } +} + +func keysOf(m map[string]bool) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + return out +} + +// --- Postgres-backed integration tests (skip when SPECSRHT_TEST_PG is unset) --- + +// TestProposeOpensProposal walks the whole open path: an agent uploads a +// document, gets back a proposal and its URL, and the proposal is listable in +// the open state with the provenance the agent supplied. +func TestProposeOpensProposal(t *testing.T) { + svc, _ := newTestService(t) + ctx := context.Background() + sp, err := svc.CreateSpace(ctx, fxSpace) + if err != nil { + t.Fatalf("CreateSpace: %v", err) + } + base, err := sp.Repo.ApprovedHead(ctx) + if err != nil { + t.Fatalf("ApprovedHead: %v", err) + } + + res, err := svc.Propose(ctx, ProposeRequest{ + Space: fxSpace, + Principal: agentPrincipal(), + Title: "Add a note", + Rationale: "because", + IfMatch: base.String(), + Message: "add notes/a.md", + Writes: []DocumentWrite{{Path: "notes/a.md", Content: mdDoc("N-1", "A", "body")}}, + }) + if err != nil { + t.Fatalf("Propose: %v", err) + } + if res.Merged { + t.Errorf("Merged = true, want false without an auto_merge policy") + } + if res.Proposal.State != core.StateOpen { + t.Errorf("state = %s, want open", res.Proposal.State) + } + if res.Proposal.Agent != "claude-code/spec-writer" { + t.Errorf("agent = %q, want the raw agent identity", res.Proposal.Agent) + } + if !strings.HasSuffix(res.URL, "/p/1") { + t.Errorf("URL = %q, want it to end in /p/1", res.URL) + } + + open, err := svc.ListProposals(ctx, fxSpace, core.StateOpen) + if err != nil { + t.Fatalf("ListProposals: %v", err) + } + if len(open) != 1 || open[0].ID != res.Proposal.ID { + t.Fatalf("ListProposals(open) = %v, want the one just opened", open) + } +} + +// TestProposeAutoMerges proves a proposal whose every changed path matches the +// space's auto_merge policy lands immediately, recorded as policy-approved. +func TestProposeAutoMerges(t *testing.T) { + svc, _ := newTestService(t) + ctx := context.Background() + sp, err := svc.CreateSpace(ctx, fxSpace) + if err != nil { + t.Fatalf("CreateSpace: %v", err) + } + // Install a policy that auto-merges everything under notes/. + commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{ + ".spec.yml": []byte("review:\n auto_merge: [notes/**]\n"), + }) + base, err := sp.Repo.ApprovedHead(ctx) + if err != nil { + t.Fatalf("ApprovedHead: %v", err) + } + + res, err := svc.Propose(ctx, ProposeRequest{ + Space: fxSpace, + Principal: agentPrincipal(), + Title: "firehose note", + IfMatch: base.String(), + Message: "add notes/a.md", + Writes: []DocumentWrite{{Path: "notes/a.md", Content: mdDoc("N-1", "A", "body")}}, + }) + if err != nil { + t.Fatalf("Propose: %v", err) + } + if !res.Merged { + t.Fatalf("Merged = false, want the policy to have landed it") + } + if res.Proposal.State != core.StateMerged { + t.Errorf("state = %s, want merged", res.Proposal.State) + } + if res.Proposal.Approval != core.ApprovalPolicy { + t.Errorf("approval = %q, want policy", res.Proposal.Approval) + } + if res.Proposal.MergedRev == "" { + t.Errorf("MergedRev is empty on a merged proposal") + } +} + +// TestProposeDoesNotAutoMergeMixedPaths proves a proposal touching one reviewed +// document is reviewed as a whole, even when its other paths would auto-merge. +func TestProposeDoesNotAutoMergeMixedPaths(t *testing.T) { + svc, _ := newTestService(t) + ctx := context.Background() + sp, err := svc.CreateSpace(ctx, fxSpace) + if err != nil { + t.Fatalf("CreateSpace: %v", err) + } + commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{ + ".spec.yml": []byte("review:\n auto_merge: [notes/**]\n"), + }) + base, err := sp.Repo.ApprovedHead(ctx) + if err != nil { + t.Fatalf("ApprovedHead: %v", err) + } + + res, err := svc.Propose(ctx, ProposeRequest{ + Space: fxSpace, + Principal: agentPrincipal(), + IfMatch: base.String(), + Title: "note plus spec", + Message: "two docs", + Writes: []DocumentWrite{ + {Path: "notes/a.md", Content: mdDoc("N-1", "A", "auto")}, + {Path: "specs/b.md", Content: mdDoc("S-1", "B", "reviewed")}, + }, + }) + if err != nil { + t.Fatalf("Propose: %v", err) + } + if res.Merged { + t.Fatalf("Merged = true, want a proposal touching specs/ to wait for review") + } + if res.Proposal.State != core.StateOpen { + t.Errorf("state = %s, want open", res.Proposal.State) + } +} + +// TestProposeRejectsStaleBase proves the open-time 409: a base the approved +// branch has moved off cannot open a proposal. +func TestProposeRejectsStaleBase(t *testing.T) { + svc, _ := newTestService(t) + ctx := context.Background() + sp, err := svc.CreateSpace(ctx, fxSpace) + if err != nil { + t.Fatalf("CreateSpace: %v", err) + } + // A revision that is a valid object name but not on the approved branch: + // commit it onto an unrelated ref so it resolves yet is not an ancestor of + // the approved head. + detached := commitFiles(t, sp, "detached/1", 1, map[string][]byte{ + "notes/x.md": mdDoc("N-9", "X", "detached"), + }) + _, err = svc.Propose(ctx, ProposeRequest{ + Space: fxSpace, + Principal: agentPrincipal(), + Title: "stale", + IfMatch: detached.String(), + Message: "m", + Writes: []DocumentWrite{{Path: "notes/a.md", Content: mdDoc("N-1", "A", "body")}}, + }) + if !errors.Is(err, ErrStale) { + t.Fatalf("Propose against a detached base: err = %v, want ErrStale", err) + } +} diff --git a/service/service.go b/service/service.go index 4f438d3152f684c19033be744cd780978f69390a..cc08b5bb272d80ca70a3e1f8783f90a2026b9cdf 100644 --- a/service/service.go +++ b/service/service.go @@ -45,6 +45,31 @@ var ( // ErrPushRejected marks a push the update hook must refuse. Type-assert to // *PushRejection for the message to print to the pushing client. ErrPushRejected = errors.New("service: push rejected") + + // ErrForbidden marks a write attempted by a principal that may not make it: + // a non-agent trying to propose, the write plane's 403. Proposing is an + // agent-only act — the human write path is native receive-pack — so this is + // a refusal of the principal, not of the request. + ErrForbidden = errors.New("service: forbidden") + + // 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 + // only needs the status code matches this. Refetch the approved head and + // re-propose. + ErrStale = errors.New("service: proposal base is stale") + + // ErrAlreadyMerged marks a merge of a proposal whose commits are already an + // ancestor of the approved head — it merged, and this is a repeat. It is a + // distinct answer from a staleness 409 (see the design's "already-merged + // proposals need an ancestry check, not a staleness check"): the proposal + // succeeded, so the caller should read the outcome rather than re-propose. + ErrAlreadyMerged = errors.New("service: proposal already merged") + + // ErrProposalNotOpen marks a write to, or a resolution of, a proposal that + // has already merged or been rejected. The state machine is terminal in one + // direction, so this is never a retryable condition. + ErrProposalNotOpen = errors.New("service: proposal is not open") ) // Config is everything service/ needs from the instance config.ini. It is a