Move the namespace-neutral error vocabulary into its own leaf (#5395)

memoryops needed one sentinel — ErrValidation — and the value the whole repo
matches lived in issueops. So the memory leaf imported the issue package for
one errors.New and pulled internal/types in behind it, claiming memory sits
downstream of issues. It does not: the two are sibling planes over one config
table.

beadserrors now declares the vocabulary that belongs to no plane in
particular — ErrValidation, ErrNotFound, ErrNotInitialized, the ErrUnsupported
type — and issueops re-exports it by alias, exactly as internal/storage and
backend already re-export from issueops. 141 qualified ErrValidation
references across the tree, zero edited: a Go alias preserves identity.
memoryops now depends on beadserrors and context, nothing more.

What belongs down there is what a leaf for a plane nobody has written yet
would need. ErrCloseBlocked and ErrDependencyCycle name issue concepts and
stay in issueops.

beadserrors imports stdlib and nothing else, enforced by a strict depguard
rule rather than asked for in a comment, because it sits beneath every leaf
and whatever it imports is imported by all of them.

Answers Q4 from #5387.

.golangci.yml

@@ -171,6 +171,22 @@ linters:
               desc: "a command reaches the ready-count role through store.ReadyCounter(), never through the constructor: the accessor is where each storage decorator adds its layer - see internal/workapi/storereadycounter/counter.go"
             - pkg: github.com/steveyegge/beads/internal/workapi/storequerier
               desc: "a command reaches the boolean-query role through store.Querier(), never through the constructor: the accessor is where each storage decorator adds its layer - see internal/workapi/storequerier/querier.go"
+        # beadserrors sits BENEATH every role leaf, so whatever it imports is
+        # imported by all of them - which is the reason it exists. The second
+        # leaf reached into issueops for one sentinel and pulled internal/types
+        # along behind it, and that import claimed the memory plane sits
+        # downstream of the issue plane. Stdlib only, and enforced rather than
+        # requested, because the next tempting import is always one shared
+        # constant that "obviously belongs with the errors".
+        #
+        # strict, not lax: this is an allowlist of the whole world, so a rule
+        # that named packages to deny would have to predict them.
+        beadserrors-leaf:
+          list-mode: strict
+          files:
+            - "**/beadserrors/**"
+          allow:
+            - "$gostd"
     # The CONSTRUCTION half of that same boundary, and the half that actually
     # matches the failure. The depguard rule above denies a handler IMPORTING
     # the builders; it does not deny a handler naming the filter types, which

beadserrors/errors.go

@@ -0,0 +1,67 @@
+// Package beadserrors holds the error vocabulary that is shared by every role
+// leaf rather than owned by any one of them.
+//
+// It exists because the second leaf proved the first one could not keep owning
+// it. memoryops needs to classify a validation refusal, and the value the whole
+// repo matches lived in issueops — so the memory plane imported the issue
+// package for one errors.New, and dragged internal/types in behind it. That
+// import claimed memory is downstream of issues. It is not: the two are sibling
+// planes over one config table.
+//
+// WHAT BELONGS HERE is the vocabulary any role needs whatever it operates on: a
+// request was invalid, a thing was not there, the substrate was not ready, this
+// backend does not implement the capability. WHAT DOES NOT is every refusal
+// that names a domain concept — an issue cannot be claimed, a close is blocked,
+// a dependency would cycle. Those stay in the leaf that defines the concept.
+// The test is whether a leaf for some plane nobody has written yet would need
+// it; if the answer requires knowing what the plane holds, it is not shared.
+//
+// Leaves RE-EXPORT from here rather than making callers import it, so code
+// holding one role interface can classify a refusal without discovering a
+// second package. Those re-exports are Go aliases, so every value is identical
+// and one errors.Is arm matches it under any of its names — which is the whole
+// reason to alias instead of minting per-package twins. issueops.ErrValidation,
+// storage.ErrValidation, backend.ErrValidation and memoryops.ErrValidation are
+// four doorplates on the one value declared below.
+//
+// The package imports stdlib and nothing else, and it must stay that way: it
+// sits beneath every leaf, so anything it imports is imported by all of them.
+package beadserrors
+
+import (
+	"errors"
+	"fmt"
+)
+
+// ErrValidation classifies deterministic request-validation failures.
+var ErrValidation = errors.New("validation failed")
+
+// ErrNotFound is returned when a requested entity does not exist in the database.
+var ErrNotFound = errors.New("not found")
+
+// ErrNotInitialized is returned when the database has not been initialized
+// (e.g., issue_prefix config is missing).
+var ErrNotInitialized = errors.New("database not initialized")
+
+// ErrUnsupported reports something the caller asked for that this backend
+// cannot serve — a capability accessor it does not implement, or a request
+// field it will not honor. It is a TYPE rather than a sentinel because the
+// two facts a caller needs are which operation refused and which backend
+// refused it, and neither survives a formatted string.
+//
+// A backend returns it instead of quietly doing something narrower. The case
+// that made that rule explicit is Reader's Offset: the store-backed body
+// rendered LIMIT without OFFSET, so a caller that paged with it received the
+// first page over and over with no error to notice.
+//
+// It lives here so a caller holding only a role interface can classify the
+// refusal with errors.As without importing internal/storage — or, since the
+// capability shell is not an issue concept, without importing issueops either.
+type ErrUnsupported struct {
+	Op      string // method name, e.g. "AddLabel" or "Transaction.CreateIssues"
+	Backend string // e.g. "dolt-server"
+}
+
+func (e *ErrUnsupported) Error() string {
+	return fmt.Sprintf("operation %q not supported by the %s backend", e.Op, e.Backend)
+}

engdocs/ADDING_AN_ISSUEOPS_ROLE.md

@@ -62,7 +62,10 @@ in; 13 is the HTTP surface, which lands with the command rather than after it.
 1. **The leaf contract.** `issueops/<role>.go`. Request and result types plus
    the interface, with the doc comment written as a SPECIFICATION — every
    promise a conformance case will cite by line. The package imports
-   `internal/types` and stdlib only, and exports no constructors.
+   `internal/types`, `beadserrors` and stdlib only, and exports no
+   constructors. A leaf in some OTHER namespace imports `beadserrors` and
+   stdlib, and reaches for `internal/types` only if its plane actually holds
+   issue-shaped things — `memoryops` does not, and says so.
    Template: `issueops/readyclaimer.go`, `issueops/commenter.go`.
 
 2. **The shared request→filter builder**, if the role takes a filter-shaped
@@ -293,31 +296,45 @@ because that is the list a third one inherits, and each of these reads like a
 convention until you notice it was situational.
 
 **Alias the sentinels; do not mint a second vocabulary.** `memoryops/errors.go`
-is one line of declaration — `var ErrValidation = issueops.ErrValidation`
-(:16) — and the IDENTITY is the entire point. `errors.Is` against that one
+is one line of declaration — `var ErrValidation = beadserrors.ErrValidation`
+— and the IDENTITY is the entire point. `errors.Is` against that one
 value is what the HTTP problem classifier, `cmd/bd`'s error handling and every
 conformance contract already do. A `memoryops`-flavored twin would be a
 different value meaning the same thing, so all of those sites would have to
 match both forever; and the cost is not the double-match, it is the site that
 adds only the first arm and classifies a validation refusal as an internal
-error. Re-exporting rather than telling callers to reach through `issueops` is
-the other half: code holding only `Memories` can classify a refusal without
-knowing the issue package exists.
+error. Re-exporting rather than telling callers to reach through the declaring
+package is the other half: code holding only `Memories` can classify a refusal
+without discovering a second import.
 
-That import is the leaf's only non-stdlib one, and it makes step 1's rule
-NARROWER here rather than wider — a memory is a string under a string key, so
-nothing in `internal/types` is needed and importing it would invite
-issue-shaped types into a plane that has none. Say that in the package doc,
-along with the fact that `internal/types` still arrives transitively through
-`issueops` (`memoryops/doc.go:27-35`): it looks like a dependency, it is an
-artifact of the alias, and the obvious "fix" for it is to copy the sentinel.
+**Alias DOWNWARD, to `beadserrors`, not sideways into `issueops`.** This is the
+part the second namespace got wrong first and then fixed. `memoryops` originally
+aliased `issueops.ErrValidation`, which kept identity but put the issue package
+in a memory leaf's dependency graph with `internal/types` behind it — a claim
+that the memory plane sits downstream of the issue plane, when the two are
+siblings over one config table. `beadserrors` now declares the namespace-neutral
+vocabulary (`ErrValidation`, `ErrNotFound`, `ErrNotInitialized`, the
+`ErrUnsupported` type) and `issueops` re-exports it by alias like everyone else.
+`memoryops` depends on `beadserrors` and `context`, nothing more.
 
-The open question is a THIRD namespace's to answer, not to inherit. Two
-namespaces sharing one vocabulary by alias is right. At three, the shared
-sentinels probably want their own tiny leaf that all of them re-export from —
-and that move is CHEAP BEFORE the third namespace exists and expensive after,
-because by then every `errors.Is` site in the tree points at the `issueops`
-value and either the new leaf aliases backwards or every site moves at once.
+The test for what belongs down there: **would a leaf for a plane nobody has
+written yet need it?** A request can be invalid, a row can be missing, a
+substrate can be uninitialized and a backend can not implement a capability, on
+any plane. `ErrCloseBlocked` and `ErrDependencyCycle` name issue concepts and
+stay put. `beadserrors` imports stdlib and nothing else — enforced by the
+`beadserrors-leaf` depguard rule, not just asked for, because the next tempting
+import is always one shared constant that "obviously belongs with the errors".
+
+**One earlier claim here was wrong and is worth keeping visible**: this section
+used to say moving the sentinels later would be expensive, "because by then
+every `errors.Is` site in the tree points at the `issueops` value and either the
+new leaf aliases backwards or every site moves at once." That is a false
+dichotomy. A Go alias preserves identity in both directions, so the new leaf
+declares canonically and the old home aliases FORWARD to it — 141 `ErrValidation`
+references across the tree, and not one of them moved. What actually grows with
+each namespace is not migration cost but the number of leaves that have baked in
+the wrong dependency direction. Judge that timing on the direction, not on churn
+that never materializes.
 
 **No `.golangci.yml` entry, and that is not an omission.** `memoryops` adds
 nothing to the `cmd-bd-role-constructors` deny list because there is nothing to

issueops/errors.go

@@ -3,6 +3,8 @@ package issueops
 import (
 	"errors"
 	"fmt"
+
+	"github.com/steveyegge/beads/beadserrors"
 )
 
 // ErrAlreadyClaimed is returned when attempting to claim an issue that is already
@@ -41,27 +43,16 @@ func (e *ClaimConflictError) Error() string { return e.Err.Error() }
 // Unwrap makes ClaimConflictError match the refusal it carries.
 func (e *ClaimConflictError) Unwrap() error { return e.Err }
 
-// ErrUnsupported reports something the caller asked for that this backend
-// cannot serve — a capability accessor it does not implement, or a request
-// field it will not honor. It is a TYPE rather than a sentinel because the
-// two facts a caller needs are which operation refused and which backend
-// refused it, and neither survives a formatted string.
+// ErrUnsupported reports a capability this backend does not serve. It is an
+// alias of beadserrors.ErrUnsupported — the same type, so one errors.As arm
+// matches it under either name — and it is re-exported here because a caller
+// holding an issueops role should not have to discover a second package to
+// classify the refusal.
 //
-// A backend returns it instead of quietly doing something narrower. The case
-// that made that rule explicit is Reader's Offset: the store-backed body
-// rendered LIMIT without OFFSET, so a caller that paged with it received the
-// first page over and over with no error to notice.
-//
-// It lives here so a caller holding only a role interface can classify the
-// refusal with errors.As without importing internal/storage.
-type ErrUnsupported struct {
-	Op      string // method name, e.g. "AddLabel" or "Transaction.CreateIssues"
-	Backend string // e.g. "dolt-server"
-}
-
-func (e *ErrUnsupported) Error() string {
-	return fmt.Sprintf("operation %q not supported by the %s backend", e.Op, e.Backend)
-}
+// It is declared there rather than here because the capability shell is not an
+// issue concept: a memory role can go unimplemented by a backend exactly as a
+// Reader can.
+type ErrUnsupported = beadserrors.ErrUnsupported
 
 // ErrAssigneeMismatch is returned by UnclaimIssueIfAssignee when the issue's
 // current assignee does not match the expected assignee (including when the
@@ -69,15 +60,23 @@ func (e *ErrUnsupported) Error() string {
 // stale; the issue is left untouched.
 var ErrAssigneeMismatch = errors.New("assignee mismatch")
 
-// ErrNotFound is returned when a requested entity does not exist in the database.
-var ErrNotFound = errors.New("not found")
-
-// ErrValidation classifies deterministic request-validation failures.
-var ErrValidation = errors.New("validation failed")
-
-// ErrNotInitialized is returned when the database has not been initialized
-// (e.g., issue_prefix config is missing).
-var ErrNotInitialized = errors.New("database not initialized")
+// The namespace-neutral part of this vocabulary is declared by beadserrors and
+// re-exported here. These are ALIASES, so they are the same values: every
+// existing issueops.ErrX reference and every errors.Is site keeps matching the
+// identical error, and a leaf that never imports issueops still matches it too.
+//
+// They live down there because none of them names an issue: a request can be
+// invalid, a row can be missing and a database can be uninitialized on any
+// plane. The refusals BELOW that name issue concepts stay here.
+var (
+	// ErrNotFound is returned when a requested entity does not exist in the database.
+	ErrNotFound = beadserrors.ErrNotFound
+	// ErrValidation classifies deterministic request-validation failures.
+	ErrValidation = beadserrors.ErrValidation
+	// ErrNotInitialized is returned when the database has not been initialized
+	// (e.g., issue_prefix config is missing).
+	ErrNotInitialized = beadserrors.ErrNotInitialized
+)
 
 // ErrPrefixMismatch is returned when an issue ID does not match the configured prefix.
 var ErrPrefixMismatch = errors.New("prefix mismatch")

memoryops/doc.go

@@ -24,15 +24,21 @@
 // it is part of what the contract MEANS — it is why a memory converges on pull
 // and a setting does not — not because a caller has to construct it.
 //
-// WHAT THIS PACKAGE IMPORTS: stdlib, plus github.com/steveyegge/beads/issueops
-// for the error sentinels and nothing else. That is narrower than the issueops
-// leaf rule ("internal/types and stdlib"): a memory is a string under a string
-// key, so nothing in internal/types is needed here and importing it would only
-// invite issue-shaped types into a plane that has none. The one sentinel is
-// ALIASED rather than redeclared — see errors.go — and the transitive pull of
-// internal/types through issueops is an artifact of that alias, not a
-// dependency anything here uses. Nobody should "fix" it by copying the
-// sentinel.
+// WHAT THIS PACKAGE IMPORTS: stdlib, plus
+// github.com/steveyegge/beads/beadserrors for the error sentinel and nothing
+// else. That is narrower than the issueops leaf rule ("internal/types and
+// stdlib"): a memory is a string under a string key, so nothing in
+// internal/types is needed here and importing it would only invite issue-shaped
+// types into a plane that has none. The sentinel is ALIASED rather than
+// redeclared — see errors.go.
+//
+// It aliases through beadserrors rather than through issueops, which is where
+// that value used to live. Reaching into the issue package for it worked and
+// kept errors.Is identity, but it put issueops in this leaf's dependency graph
+// and internal/types behind it — a claim that the memory plane sits downstream
+// of the issue plane, when the two are siblings over one config table. The
+// shared vocabulary moved down instead. beadserrors imports stdlib only, and a
+// depguard rule keeps it that way.
 //
 // THERE IS NO PAGE HERE. Memories are a keyed namespace a workspace holds tens
 // of, exactly like settings, so List answers with a map: no order, no limit, no

memoryops/errors.go

@@ -1,28 +1,34 @@
 package memoryops
 
-import "github.com/steveyegge/beads/issueops"
+import "github.com/steveyegge/beads/beadserrors"
 
 // ErrValidation classifies this role's deterministic request-validation
-// failures. It is an ALIAS of issueops.ErrValidation, not a second sentinel,
+// failures. It is an ALIAS of beadserrors.ErrValidation, not a second sentinel,
 // and the identity is the point: the HTTP problem classifier, cmd/bd's error
 // handling and every conformance suite already errors.Is against that one
 // value, so a memoryops-flavored twin would make each of them double-match
 // forever — one vocabulary with two doorplates instead of two vocabularies.
 //
 // It is re-exported here rather than left for callers to reach through
-// issueops so that code holding only the Memories interface can classify a
-// refusal without knowing the issue package exists, which is the courtesy
+// beadserrors so that code holding only the Memories interface can classify a
+// refusal without knowing a second package exists, which is the courtesy
 // issueops.ErrUnsupported's doc extends for the same reason.
-var ErrValidation = issueops.ErrValidation
+//
+// The alias points at beadserrors rather than at issueops, where this value
+// used to be declared. Reaching through the issue package said the memory plane
+// is downstream of the issue plane — it is not, they are siblings over one
+// config table — and the import dragged internal/types along to say it. This
+// leaf's whole dependency set is now beadserrors and context.
+var ErrValidation = beadserrors.ErrValidation
 
 // THERE IS DELIBERATELY NO ErrNotFound ON THIS ROLE.
 //
-// The storage seam beneath it cannot tell an absent config row from a row
-// stored as the empty string (issueops/workspaceconfig.go:41-52 states the same
-// conflation for settings, and it is the same table). A role that answered a
-// Recall of an unknown key with ErrNotFound would be minting an error out of a
-// distinction it cannot actually see, and the first out-of-band empty write
-// would make it a lie.
+// beadserrors declares one; this leaf does not re-export it. The storage seam
+// beneath it cannot tell an absent config row from a row stored as the empty
+// string (issueops/workspaceconfig.go states the same conflation for settings,
+// and it is the same table). A role that answered a Recall of an unknown key
+// with ErrNotFound would be minting an error out of a distinction it cannot
+// actually see, and the first out-of-band empty write would make it a lie.
 //
 // Misses are RESULT-CARRIED instead — RecallResult.Found, ForgetResult.Found —
 // and the front doors translate: the CLI to its SilentExit contract, an HTTP