M db/proposal.go => db/proposal.go +31 -1
@@ 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
A service/merge.go => service/merge.go +201 -0
@@ 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)
+ }
+}
A service/merge_test.go => service/merge_test.go +195 -0
@@ 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)
+ }
+}
A service/proposals.go => service/proposals.go +125 -0
@@ 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 <origin>/~owner/space/p/<id>, 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
+}
A service/propose.go => service/propose.go +423 -0
@@ 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/<id>" 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
+}
A service/propose_test.go => service/propose_test.go +344 -0
@@ 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)
+ }
+}
M service/service.go => service/service.go +25 -0
@@ 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