8 files changed, 141 insertions(+), 20 deletions(-)

M core/errors.go
M core/proposal.go
M core/proposal_test.go
M db/proposal.go
M db/proposal_test.go
M db/unit_test.go
M gitx/gitx_test.go
M gitx/refsrule.go

@@ 72,4 72,9 @@ var (
 
 	// ErrInvalidApproval is returned for an approval kind outside human/policy.
 	ErrInvalidApproval = errors.New("invalid approval kind")
+
+	// ErrInvalidProposalID is returned for a proposal id that cannot name a
+	// proposal: ids are Postgres sequence values starting at 1, so anything
+	// else is an unwritten row or an unset field rather than a proposal.
+	ErrInvalidProposalID = errors.New("invalid proposal id")
 )

@@ 1,6 1,36 @@
 package core
 
-import "fmt"
+import (
+	"fmt"
+	"strconv"
+)
+
+// ProposalPrefix is the ref namespace a proposal branch lives under, and the
+// only namespace an agent credential may write. The refs rule — an agent token
+// can only update refs under this prefix, and only the owner can move the
+// approved branch — is the boundary that actually bounds the damage a confused
+// agent can do.
+//
+// It lives here rather than in gitx or db because both derive branch names from
+// it and a proposal whose row and whose ref disagree about its name is a break
+// with no cheap way to trace it.
+const ProposalPrefix = "proposals/"
+
+// ProposalBranch is the branch name for a proposal id: "proposals/42". The id
+// is the proposal row's primary key, which is what the branch, the row and the
+// stable proposal URL share — so this is the one derivation, and gitx and db
+// both call it rather than each spelling the concatenation out.
+//
+// A non-positive id is refused rather than formatted: ids come from a Postgres
+// sequence and start at 1, so a zero is an unwritten row or an unset field, and
+// "proposals/0" is a branch name that would go on to be created, pushed and
+// looked for.
+func ProposalBranch(id int64) (string, error) {
+	if id <= 0 {
+		return "", fmt.Errorf("%w: proposal id %d must be positive", ErrInvalidProposalID, id)
+	}
+	return ProposalPrefix + strconv.FormatInt(id, 10), nil
+}
 
 // ProposalState is the lifecycle of a proposal.
 //

@@ 5,6 5,37 @@ import (
 	"testing"
 )
 
+// The one derivation of a proposal's branch name. gitx cuts the ref from it and
+// db records it on the row; if the two ever computed it separately, a proposal
+// whose row and ref disagreed would be a break with no cheap way to trace it.
+func TestProposalBranch(t *testing.T) {
+	for _, tc := range []struct {
+		id   int64
+		want string
+	}{
+		{1, "proposals/1"},
+		{42, "proposals/42"},
+		{1000000, "proposals/1000000"},
+	} {
+		got, err := ProposalBranch(tc.id)
+		if err != nil {
+			t.Errorf("ProposalBranch(%d): %v", tc.id, err)
+			continue
+		}
+		if got != tc.want {
+			t.Errorf("ProposalBranch(%d) = %q, want %q", tc.id, got, tc.want)
+		}
+	}
+	// Ids come from a sequence starting at 1, so a non-positive one is an
+	// unwritten row, not a proposal — and "proposals/0" is a name that would go
+	// on to be created and looked for.
+	for _, id := range []int64{0, -1} {
+		if _, err := ProposalBranch(id); !errors.Is(err, ErrInvalidProposalID) {
+			t.Errorf("ProposalBranch(%d) err = %v, want ErrInvalidProposalID", id, err)
+		}
+	}
+}
+
 func TestParseProposalState(t *testing.T) {
 	tests := []struct {
 		in string

@@ 5,22 5,26 @@ import (
 	"database/sql"
 	"errors"
 	"fmt"
-	"strconv"
 	"time"
 
 	"sourcecraft.dev/bigbes/sr-ht-spec/core"
 )
 
-// BranchPrefix is the ref namespace agents may write. The refs rule — an agent
-// token can only update refs under this prefix, and only the owner can move the
-// approved branch — is the boundary that actually bounds the damage a confused
-// agent can do.
-const BranchPrefix = "proposals/"
+// BranchPrefix is the ref namespace agents may write, under this package's
+// name. It is core's constant: the prefix the INSERT below builds a branch from
+// and the prefix gitx enforces on a push are one value, because a row and a ref
+// that disagree about a proposal's branch name is a break that surfaces as a
+// proposal nobody can find.
+const BranchPrefix = core.ProposalPrefix
 
 // ProposalBranch is the branch name for a proposal id: "proposals/42". The row
 // stores it verbatim (proposal.branch) because the row, not this function, is
 // what the reconciler compares against the refs it finds.
-func ProposalBranch(id int) string { return BranchPrefix + strconv.Itoa(id) }
+//
+// The derivation is core.ProposalBranch, so this and gitx.ProposalBranch cannot
+// disagree — and it inherits core's refusal of a non-positive id, which here
+// means an unwritten row rather than a proposal.
+func ProposalBranch(id int) (string, error) { return core.ProposalBranch(int64(id)) }
 
 // Proposal is a bundle of document edits awaiting review: a branch under
 // BranchPrefix plus this row.

@@ 19,8 19,12 @@ func TestProposalOpenAndRead(t *testing.T) {
 	if p.ID == 0 {
 		t.Fatal("expected a non-zero proposal id")
 	}
-	if p.Branch != ProposalBranch(p.ID) {
-		t.Fatalf("branch = %q, want %q", p.Branch, ProposalBranch(p.ID))
+	branch, err := ProposalBranch(p.ID)
+	if err != nil {
+		t.Fatalf("ProposalBranch(%d): %v", p.ID, err)
+	}
+	if p.Branch != branch {
+		t.Fatalf("branch = %q, want %q", p.Branch, branch)
 	}
 	if p.State != core.StateOpen {
 		t.Fatalf("state = %q, want open", p.State)

@@ 30,13 30,41 @@ func TestProposalBranch(t *testing.T) {
 		{42, "proposals/42"},
 		{1000000, "proposals/1000000"},
 	} {
-		if got := ProposalBranch(tc.id); got != tc.want {
+		got, err := ProposalBranch(tc.id)
+		if err != nil {
+			t.Errorf("ProposalBranch(%d): %v", tc.id, err)
+			continue
+		}
+		if got != tc.want {
 			t.Errorf("ProposalBranch(%d) = %q, want %q", tc.id, got, tc.want)
 		}
 	}
-	if !strings.HasPrefix(ProposalBranch(7), BranchPrefix) {
+	seven, err := ProposalBranch(7)
+	if err != nil {
+		t.Fatalf("ProposalBranch(7): %v", err)
+	}
+	if !strings.HasPrefix(seven, BranchPrefix) {
 		t.Errorf("ProposalBranch must stay under the refs-rule prefix %q", BranchPrefix)
 	}
+	// An id no row can have names no branch. "proposals/0" would otherwise be
+	// created, pushed and looked for.
+	for _, id := range []int{0, -1} {
+		if _, err := ProposalBranch(id); !errors.Is(err, core.ErrInvalidProposalID) {
+			t.Errorf("ProposalBranch(%d) err = %v, want ErrInvalidProposalID", id, err)
+		}
+	}
+	// One derivation: db and gitx must not be able to disagree.
+	fromCore, err := core.ProposalBranch(42)
+	if err != nil {
+		t.Fatalf("core.ProposalBranch(42): %v", err)
+	}
+	mine, err := ProposalBranch(42)
+	if err != nil {
+		t.Fatalf("ProposalBranch(42): %v", err)
+	}
+	if mine != fromCore {
+		t.Errorf("db derives %q where core derives %q", mine, fromCore)
+	}
 }
 
 func TestHashTokenAndMatches(t *testing.T) {

@@ 280,6 280,15 @@ func TestProposalBranchNaming(t *testing.T) {
 	}
 	if _, err := ProposalBranch(0); err == nil {
 		t.Fatal("ProposalBranch(0) succeeded")
+	} else if !errors.Is(err, ErrBadRev) || !errors.Is(err, core.ErrInvalidProposalID) {
+		t.Fatalf("ProposalBranch(0) err = %v, want both ErrBadRev and core.ErrInvalidProposalID", err)
+	}
+	// One derivation, shared with db: this package only adds its failure class.
+	if fromCore, err := core.ProposalBranch(42); err != nil || fromCore != b {
+		t.Fatalf("core.ProposalBranch(42) = %q, %v; gitx derives %q", fromCore, err, b)
+	}
+	if ProposalPrefix != core.ProposalPrefix {
+		t.Fatalf("ProposalPrefix = %q, core says %q", ProposalPrefix, core.ProposalPrefix)
 	}
 	if id, ok := ParseProposalBranch("proposals/42"); !ok || id != 42 {
 		t.Fatalf("ParseProposalBranch = %d, %v", id, ok)

@@ 7,10 7,15 @@ import (
 	"unicode/utf8"
 
 	"github.com/go-git/go-git/v5/plumbing"
+
+	"sourcecraft.dev/bigbes/sr-ht-spec/core"
 )
 
-// ProposalPrefix is the namespace agents may write and nothing else.
-const ProposalPrefix = "proposals/"
+// ProposalPrefix is the namespace agents may write and nothing else. It is
+// core's constant under this package's name: the prefix a ref is checked
+// against here and the prefix a proposal row's branch is built from are one
+// value, or the two disagree the day one of them is edited.
+const ProposalPrefix = core.ProposalPrefix
 
 // branchRefPrefix is the only ref namespace a space repository uses. Tags and
 // notes are not part of the model, and a branch outside these two namespaces


@@ 147,14 152,19 @@ func IsProposalBranch(branch string) bool {
 	return strings.TrimPrefix(branch, ProposalPrefix) != ""
 }
 
-// ProposalBranch is the branch name for a proposal id, "proposals/42". The id
-// is the proposal row's primary key, which is what the branch and the stable
-// proposal URL share.
+// ProposalBranch is the branch name for a proposal id, "proposals/42".
+//
+// The derivation is core's, so a branch cut here and a branch recorded on the
+// proposal row cannot drift apart. What this wrapper adds is the failure class
+// gitx callers branch on: an id that names no proposal is a bad revision here,
+// exactly like a malformed ref, and core's ErrInvalidProposalID stays in the
+// chain for a caller that wants to tell the two apart.
 func ProposalBranch(id int64) (string, error) {
-	if id <= 0 {
-		return "", fmt.Errorf("%w: proposal id %d must be positive", ErrBadRev, id)
+	branch, err := core.ProposalBranch(id)
+	if err != nil {
+		return "", fmt.Errorf("%w: %w", ErrBadRev, err)
 	}
-	return ProposalPrefix + strconv.FormatInt(id, 10), nil
+	return branch, nil
 }
 
 // ParseProposalBranch recovers the proposal id from a branch name produced by