From 33e14b5c825ef1832f5c39a6fc95f06a54d47b31 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Fri, 7 Aug 2026 04:32:06 -0700 Subject: [PATCH] Move the namespace-neutral error vocabulary into its own leaf (#5395) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 16 +++++++ beadserrors/errors.go | 67 ++++++++++++++++++++++++++++++ engdocs/ADDING_AN_ISSUEOPS_ROLE.md | 55 +++++++++++++++--------- issueops/errors.go | 57 +++++++++++++------------ memoryops/doc.go | 24 +++++++---- memoryops/errors.go | 28 ++++++++----- 6 files changed, 179 insertions(+), 68 deletions(-) create mode 100644 beadserrors/errors.go diff --git a/.golangci.yml b/.golangci.yml index 53b5c6c19..6cae25a1f 100644 --- a/.golangci.yml +++ b/.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 diff --git a/beadserrors/errors.go b/beadserrors/errors.go new file mode 100644 index 000000000..513ecab9d --- /dev/null +++ b/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) +} diff --git a/engdocs/ADDING_AN_ISSUEOPS_ROLE.md b/engdocs/ADDING_AN_ISSUEOPS_ROLE.md index 2ed97d56b..f3cc6c207 100644 --- a/engdocs/ADDING_AN_ISSUEOPS_ROLE.md +++ b/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/.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 diff --git a/issueops/errors.go b/issueops/errors.go index 60f3885fa..aef0c36ec 100644 --- a/issueops/errors.go +++ b/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") diff --git a/memoryops/doc.go b/memoryops/doc.go index 6db1e1638..688c55464 100644 --- a/memoryops/doc.go +++ b/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 diff --git a/memoryops/errors.go b/memoryops/errors.go index d46d4c534..c63c53e15 100644 --- a/memoryops/errors.go +++ b/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