diff --git a/core/errors.go b/core/errors.go index 05f20060d64269a0e82cbd7c4e9abe4a84107071..ab8b6daedd56d564f3fbda08569da36c6e08cf0b 100644 --- a/core/errors.go +++ b/core/errors.go @@ -72,4 +72,9 @@ ErrInvalidTransition = errors.New("invalid proposal state transition") // 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") ) diff --git a/core/proposal.go b/core/proposal.go index 22df5e0cbeabc31b22a3e868dc59391b78718c7e..1efdf02f55ab89d2dcc1f2b05aa954f0e76f9b71 100644 --- a/core/proposal.go +++ b/core/proposal.go @@ -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. // diff --git a/core/proposal_test.go b/core/proposal_test.go index 368bce7548c0465dc6b621f9ccaba0def3d74608..4c9b579cc8c1da4502d764553976c231d02e2b30 100644 --- a/core/proposal_test.go +++ b/core/proposal_test.go @@ -5,6 +5,37 @@ "errors" "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 diff --git a/db/proposal.go b/db/proposal.go index ff88767e5676d168178727880bcf20a396f70736..9a302901d1adfb89ea04036132e9a3aa5bdcfd86 100644 --- a/db/proposal.go +++ b/db/proposal.go @@ -5,22 +5,26 @@ "context" "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. diff --git a/db/proposal_test.go b/db/proposal_test.go index 7c37ed0b1ff3ca64e6f469ae762e6755b8797ebc..436ad87b6dd597a59e749facfc27550ac9d1eb55 100644 --- a/db/proposal_test.go +++ b/db/proposal_test.go @@ -19,8 +19,12 @@ 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) diff --git a/db/unit_test.go b/db/unit_test.go index 319e034a71661177af6e27557beb3cc877f98df6..804b398acff5682b175dfeee212768282f8eef03 100644 --- a/db/unit_test.go +++ b/db/unit_test.go @@ -30,12 +30,40 @@ {1, "proposals/1"}, {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) } } diff --git a/gitx/gitx_test.go b/gitx/gitx_test.go index 0befec5ef471be8c3e7a1d6febf075efda0d104d..b9be92c3bf032c2122ed99e47d552f0a194f3047 100644 --- a/gitx/gitx_test.go +++ b/gitx/gitx_test.go @@ -280,6 +280,15 @@ t.Fatalf("ProposalBranch(42) = %q", b) } 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) diff --git a/gitx/refsrule.go b/gitx/refsrule.go index 84553952a0a57d3fcc3fcbd659da8fc2ceae5a61..0c061a487fb4bb8ad7b616cf466af3e484d81097 100644 --- a/gitx/refsrule.go +++ b/gitx/refsrule.go @@ -7,10 +7,15 @@ "strings" "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 @@ } 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