6 files changed, 206 insertions(+), 52 deletions(-)

M db/proposal.go
M db/proposal_test.go
M db/store.go
M gitx/write.go
M gitx/write_test.go
M service/reconcile.go

@@ 258,6 258,55 @@ WHERE id = $1 AND state = $6`
 	return fmt.Errorf("proposal %d: guarded update matched no row while state is %s", id, from)
 }
 
+// DeleteOpenProposal removes an open proposal row outright.
+//
+// It is the only delete in this package and it is deliberately narrow: the
+// reconciler's repair for a row whose branch never appeared, where the daemon
+// died between the row insert and the branch write. Such a row holds no content
+// — the agent still has the document it wanted to write and re-proposes — so
+// deleting it loses nothing. A resolved proposal is history and is never
+// deleted, which is why this is not a general-purpose delete.
+//
+// The `state = 'open'` guard is in the statement, not in Go: a check-then-write
+// would let a merge land in between and delete the row of a proposal that had
+// just succeeded. When the guard bites, the current state is read back only to
+// name it in the error — ErrNotFound when the row is gone, ErrProposalNotOpen
+// when it has been resolved — exactly as resolveProposal does.
+func (s *Store) DeleteOpenProposal(ctx context.Context, id int) error {
+	const q = `DELETE FROM proposal WHERE id = $1 AND state = $2`
+	res, err := s.q.ExecContext(ctx, q, id, string(core.StateOpen))
+	if err != nil {
+		return fmt.Errorf("delete proposal %d: %w", id, err)
+	}
+	n, err := res.RowsAffected()
+	if err != nil {
+		return fmt.Errorf("delete proposal %d: rows affected: %w", id, err)
+	}
+	if n == 1 {
+		return nil
+	}
+
+	// Nothing was deleted: either the proposal is gone, or it is no longer open.
+	var current string
+	err = s.q.QueryRowContext(ctx, `SELECT state FROM proposal WHERE id = $1`, id).Scan(&current)
+	if errors.Is(err, sql.ErrNoRows) {
+		return ErrNotFound
+	}
+	if err != nil {
+		return fmt.Errorf("delete proposal %d: read current state: %w", id, err)
+	}
+	from, err := core.ParseProposalState(current)
+	if err != nil {
+		return fmt.Errorf("proposal %d: %w", id, err)
+	}
+	if from == core.StateOpen {
+		// The row is open, yet the guarded DELETE matched nothing. That cannot
+		// happen; refuse rather than report success.
+		return fmt.Errorf("proposal %d: guarded delete matched no row while state is %s", id, from)
+	}
+	return fmt.Errorf("%w: proposal %d is %s", ErrProposalNotOpen, id, from)
+}
+
 // MergeProposal records a merge: the proposal's transition to merged and the
 // new registry location of every document it landed, atomically.
 //

@@ 176,6 176,61 @@ func TestProposalTransitionsAreGuarded(t *testing.T) {
 	}
 }
 
+// TestDeleteOpenProposal covers the reconciler's one destructive repair: the
+// row of a proposal whose branch never appeared. The guard is the whole point —
+// a merge landing between the reconciler's decision and its write must not let
+// it delete a proposal that had just succeeded.
+func TestDeleteOpenProposal(t *testing.T) {
+	s, _, cleanup := newTestStore(t)
+	defer cleanup()
+	ctx := context.Background()
+
+	space := mkSpace(t, s, ctx, "bigbes", "rfcs")
+
+	p := mkProposal(t, s, ctx, space.ID, "never got a branch")
+	if err := s.DeleteOpenProposal(ctx, p.ID); err != nil {
+		t.Fatalf("delete open proposal: %v", err)
+	}
+	if _, err := s.GetProposal(ctx, p.ID); !errors.Is(err, ErrNotFound) {
+		t.Fatalf("get after delete = %v, want ErrNotFound", err)
+	}
+	// A row that is already gone is ErrNotFound, distinct from a row that moved
+	// on: the reconciler has to tell "nothing to repair" from "the repair no
+	// longer applies".
+	if err := s.DeleteOpenProposal(ctx, p.ID); !errors.Is(err, ErrNotFound) {
+		t.Fatalf("second delete = %v, want ErrNotFound", err)
+	}
+	if err := s.DeleteOpenProposal(ctx, 99999); !errors.Is(err, ErrNotFound) {
+		t.Fatalf("delete of a missing proposal = %v, want ErrNotFound", err)
+	}
+
+	merged := mkProposal(t, s, ctx, space.ID, "merged under the reconciler")
+	if err := s.MarkProposalMerged(ctx, merged.ID, core.ApprovalHuman, "deadbeef"); err != nil {
+		t.Fatalf("merge: %v", err)
+	}
+	if err := s.DeleteOpenProposal(ctx, merged.ID); !errors.Is(err, ErrProposalNotOpen) {
+		t.Fatalf("delete of a merged proposal = %v, want ErrProposalNotOpen", err)
+	}
+	got, err := s.GetProposal(ctx, merged.ID)
+	if err != nil {
+		t.Fatalf("get merged: %v", err)
+	}
+	if got.State != core.StateMerged || got.MergedRev != "deadbeef" {
+		t.Fatalf("the guarded delete modified the row: %+v", got)
+	}
+
+	rejected := mkProposal(t, s, ctx, space.ID, "rejected")
+	if err := s.RejectProposal(ctx, rejected.ID); err != nil {
+		t.Fatalf("reject: %v", err)
+	}
+	if err := s.DeleteOpenProposal(ctx, rejected.ID); !errors.Is(err, ErrProposalNotOpen) {
+		t.Fatalf("delete of a rejected proposal = %v, want ErrProposalNotOpen", err)
+	}
+	if _, err := s.GetProposal(ctx, rejected.ID); err != nil {
+		t.Fatalf("the guarded delete removed a rejected proposal: %v", err)
+	}
+}
+
 // TestMergeProposalIsAtomic is the reason the transaction exists: a merge that
 // cannot re-point the registry must not leave the proposal reading merged.
 func TestMergeProposalIsAtomic(t *testing.T) {

@@ 144,6 144,13 @@ var (
 	// log can say which happened; both map to 401 at the API boundary.
 	ErrTokenRevoked = errors.New("db: agent token revoked")
 
+	// ErrProposalNotOpen is returned by DeleteOpenProposal for a proposal that
+	// exists but has already been resolved. Kept distinct from ErrNotFound so
+	// the reconciler can tell "the row is gone", which is the state it wanted,
+	// from "the proposal merged under us", which means the repair it planned no
+	// longer applies.
+	ErrProposalNotOpen = errors.New("db: proposal is not open")
+
 	// ErrNoTransaction is returned by InTx when the Store is not bound to
 	// something that can begin one (i.e. it is already inside a transaction).
 	ErrNoTransaction = errors.New("db: store cannot begin a transaction")

@@ 342,6 342,47 @@ func (r *Repo) CreateProposalBranch(ctx context.Context, branch, base string) (p
 	return head, nil
 }
 
+// DeleteProposalBranch removes a proposal branch, under the space write lock so
+// it excludes the merge path and any other in-process write.
+//
+// It refuses anything outside proposals/*. That check is not defence in depth,
+// it is the only thing standing between a caller bug and a deleted approved
+// branch — the approved branch is deleted by nobody, ever, which is also what
+// CheckRefUpdate tells receive-pack.
+//
+// An already-absent branch is success rather than ErrNotFound. The caller is
+// the reconciler, whose repair is a postcondition ("this unreferenced ref does
+// not exist") and not an action, and the ref can legitimately vanish between
+// the listing that found it and this call — a native receive-pack push deleting
+// it, or an earlier pass that raced this one. Reporting that as a failure would
+// fill the reconcile report with failures for repairs that in fact hold.
+//
+// There is no compare-and-swap here and no lost-race retry: unlike a ref move,
+// a delete does not depend on the value it is replacing, so a concurrent writer
+// cannot make it do the wrong thing. It can only make it redundant, which is
+// the case above.
+func (r *Repo) DeleteProposalBranch(ctx context.Context, branch string) error {
+	ctx, cancel := r.withTimeout(ctx)
+	defer cancel()
+
+	if !IsProposalBranch(branch) {
+		return fmt.Errorf("%w: %q is not a %s* branch; only a proposal branch may be deleted",
+			ErrBadRev, branch, ProposalPrefix)
+	}
+
+	unlock, err := r.lock(ctx)
+	if err != nil {
+		return err
+	}
+	defer unlock()
+
+	name := plumbing.NewBranchReferenceName(branch)
+	if err := r.repo.Storer.RemoveReference(name); err != nil {
+		return fmt.Errorf("gitx: delete %s in %s: %w", name, r.ref, err)
+	}
+	return nil
+}
+
 // CommitProposal commits whole-document blobs onto a proposal branch.
 //
 // It refuses any branch outside proposals/*: the approved branch moves in

@@ 47,6 47,52 @@ func TestCreateProposalBranch(t *testing.T) {
 	}
 }
 
+func TestDeleteProposalBranch(t *testing.T) {
+	repo, _ := newSpace(t)
+	ctx := context.Background()
+
+	base := pushApproved(t, repo, ownerMeta("seed", 1),
+		Write{Path: "specs/0007.md", Content: doc("SPEC-0007", "Storage", "v1")})
+	openProposal(t, repo, "proposals/1", base.String(), meta("revise 0007", 2),
+		Write{Path: "specs/0007.md", Content: doc("SPEC-0007", "Storage", "v2")})
+
+	if err := repo.DeleteProposalBranch(ctx, "proposals/1"); err != nil {
+		t.Fatalf("DeleteProposalBranch: %v", err)
+	}
+	branches, err := repo.ListProposalBranches(ctx)
+	if err != nil {
+		t.Fatalf("ListProposalBranches: %v", err)
+	}
+	if len(branches) != 0 {
+		t.Fatalf("branches = %+v, want none", branches)
+	}
+	if _, err := repo.BranchHead(ctx, "proposals/1"); !errors.Is(err, ErrNotFound) {
+		t.Fatalf("BranchHead after the delete = %v, want ErrNotFound", err)
+	}
+
+	// An already-absent branch is success, not ErrNotFound: the reconciler's
+	// repair is a postcondition, and the ref may vanish between the listing
+	// that found it and the delete. Asserted here rather than inherited from
+	// go-git, so the contract is this package's.
+	if err := repo.DeleteProposalBranch(ctx, "proposals/1"); err != nil {
+		t.Fatalf("second DeleteProposalBranch = %v, want success", err)
+	}
+	if err := repo.DeleteProposalBranch(ctx, "proposals/404"); err != nil {
+		t.Fatalf("DeleteProposalBranch of a branch that never existed = %v, want success", err)
+	}
+
+	// The namespace check is the only thing standing between a caller bug and a
+	// deleted approved branch.
+	for _, b := range []string{repo.ApprovedBranch(), "scratch", "proposals", "", "refs/heads/proposals/1"} {
+		if err := repo.DeleteProposalBranch(ctx, b); !errors.Is(err, ErrBadRev) {
+			t.Fatalf("DeleteProposalBranch(%q) = %v, want ErrBadRev", b, err)
+		}
+	}
+	if _, err := repo.ApprovedHead(ctx); err != nil {
+		t.Fatalf("approved branch is gone: %v", err)
+	}
+}
+
 func TestCommitProposalSplicesWholeDocuments(t *testing.T) {
 	repo, _ := newSpace(t)
 	ctx := context.Background()

@@ 6,7 6,6 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/go-git/go-git/v5"
 	"github.com/go-git/go-git/v5/plumbing"
 
 	"sourcecraft.dev/bigbes/sr-ht-spec/core"


@@ 473,7 472,7 @@ func (s *Service) applyRepair(ctx context.Context, sp *Space, r Repair) error {
 	case RepairMarkMerged:
 		return s.store.MarkProposalMerged(ctx, r.ProposalID, r.Approval, r.Rev)
 	case RepairDeleteRow:
-		return s.deleteOpenProposalRow(ctx, r.ProposalID)
+		return s.store.DeleteOpenProposal(ctx, r.ProposalID)
 	case RepairDeleteRef:
 		return s.deleteProposalRef(ctx, sp, r.Branch)
 	default:


@@ 481,60 480,17 @@ func (s *Service) applyRepair(ctx context.Context, sp *Space, r Repair) error {
 	}
 }
 
-// deleteOpenProposalRow removes a proposal row that never got a branch.
-//
-// The SQL is written here rather than called on db.Store because db/ exposes no
-// delete — the corrected repair table needs one and the persistence layer was
-// built before the table was corrected. It belongs in db/ as DeleteOpenProposal
-// and should move there; it lives in this file so that the repair the design
-// specifies actually happens rather than silently not happening.
-//
-// The `state = 'open'` guard is in the statement, not in Go: a check-then-write
-// would let a merge land in between and delete the row of a proposal that had
-// just succeeded.
-func (s *Service) deleteOpenProposalRow(ctx context.Context, id int) error {
-	const q = `DELETE FROM proposal WHERE id = $1 AND state = 'open'`
-	res, err := s.q.ExecContext(ctx, q, id)
-	if err != nil {
-		return fmt.Errorf("service: delete proposal %d: %w", id, err)
-	}
-	n, err := res.RowsAffected()
-	if err != nil {
-		return fmt.Errorf("service: delete proposal %d: rows affected: %w", id, err)
-	}
-	if n == 0 {
-		return fmt.Errorf("service: proposal %d is no longer open; not deleted", id)
-	}
-	return nil
-}
-
 // deleteProposalRef removes an unreferenced proposals/* branch.
 //
-// Like the row delete above, this primitive is missing from the layer that
-// should own it: gitx exposes branch creation and the merge but no delete. It
-// is written here against go-git directly, under gitx's own per-space write
-// lock, so it still excludes the merge path and a concurrent in-process write —
-// but it should move into gitx, where the compare-and-swap retry logic for the
-// two-writer problem already lives.
-//
-// The namespace check is not defence in depth, it is the only thing standing
-// between a caller bug and a deleted approved branch.
+// The namespace check, the per-space write lock and the choice to treat an
+// already-absent branch as success all live in gitx.DeleteProposalBranch, which
+// owns refs. What is left here is naming the space the failure belongs to, so
+// the reconcile report says which repository could not be repaired.
 func (s *Service) deleteProposalRef(ctx context.Context, sp *Space, branch string) error {
-	if !gitx.IsProposalBranch(branch) {
-		return fmt.Errorf("service: refusing to delete %q: only %s* branches are deletable",
-			branch, gitx.ProposalPrefix)
+	if err := sp.Repo.DeleteProposalBranch(ctx, branch); err != nil {
+		return fmt.Errorf("service: delete branch %q in %s: %w", branch, sp.Ref, err)
 	}
-	return sp.Repo.WithLock(ctx, func(ctx context.Context) error {
-		repo, err := git.PlainOpen(sp.Repo.Dir())
-		if err != nil {
-			return fmt.Errorf("service: open %s: %w", sp.Repo.Dir(), err)
-		}
-		name := plumbing.NewBranchReferenceName(branch)
-		if err := repo.Storer.RemoveReference(name); err != nil {
-			return fmt.Errorf("service: delete %s in %s: %w", name, sp.Ref, err)
-		}
-		return nil
-	})
+	return nil
 }
 
 // RunReconciler runs the reconciler at startup and then on a ticker, until ctx