From 643a162f0e0c68c11414d391e298fbeb4b89b771 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Tue, 30 Dec 2025 15:58:27 -0800 Subject: [PATCH] Centralize validation patterns with composable validators (bd-jbqx) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add composable issue validators to internal/validation package: - IssueValidator type with Chain() composition function - Exists(), NotTemplate(), NotPinned(), NotClosed(), NotHooked() validators - HasStatus(), HasType() for checking allowed values - ForUpdate(), ForClose(), ForDelete(), ForReopen() convenience chains Update cmd/bd/show_unit_helpers.go to use centralized validators instead of duplicated inline validation logic. This enables consistent validation across all commands with a single source of truth. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- cmd/bd/show_unit_helpers.go | 32 ++- internal/validation/issue.go | 156 +++++++++++++ internal/validation/issue_test.go | 367 ++++++++++++++++++++++++++++++ 3 files changed, 537 insertions(+), 18 deletions(-) create mode 100644 internal/validation/issue.go create mode 100644 internal/validation/issue_test.go diff --git a/cmd/bd/show_unit_helpers.go b/cmd/bd/show_unit_helpers.go index 23c80a1e..df5d99cb 100644 --- a/cmd/bd/show_unit_helpers.go +++ b/cmd/bd/show_unit_helpers.go @@ -2,33 +2,29 @@ package main import ( "context" - "fmt" "github.com/steveyegge/beads/internal/storage" "github.com/steveyegge/beads/internal/types" + "github.com/steveyegge/beads/internal/validation" ) +// validateIssueUpdatable checks if an issue can be updated. +// Uses the centralized validation package for consistency. func validateIssueUpdatable(id string, issue *types.Issue) error { - if issue == nil { - return nil - } - if issue.IsTemplate { - return fmt.Errorf("Error: cannot update template %s: templates are read-only; use 'bd molecule instantiate' to create a work item", id) - } - return nil + // Note: We use NotTemplate() directly instead of ForUpdate() to maintain + // backward compatibility - the original didn't check for nil issues. + return validation.NotTemplate()(id, issue) } +// validateIssueClosable checks if an issue can be closed. +// Uses the centralized validation package for consistency. func validateIssueClosable(id string, issue *types.Issue, force bool) error { - if issue == nil { - return nil - } - if issue.IsTemplate { - return fmt.Errorf("Error: cannot close template %s: templates are read-only", id) - } - if !force && issue.Status == types.StatusPinned { - return fmt.Errorf("Error: cannot close pinned issue %s (use --force to override)", id) - } - return nil + // Note: We use individual validators instead of ForClose() to maintain + // backward compatibility - the original didn't check for nil issues. + return validation.Chain( + validation.NotTemplate(), + validation.NotPinned(force), + )(id, issue) } func applyLabelUpdates(ctx context.Context, st storage.Storage, issueID, actor string, setLabels, addLabels, removeLabels []string) error { diff --git a/internal/validation/issue.go b/internal/validation/issue.go new file mode 100644 index 00000000..e1cac049 --- /dev/null +++ b/internal/validation/issue.go @@ -0,0 +1,156 @@ +package validation + +import ( + "fmt" + + "github.com/steveyegge/beads/internal/types" +) + +// IssueValidator validates an issue and returns an error if validation fails. +// Validators can be composed using Chain() for complex validation logic. +type IssueValidator func(id string, issue *types.Issue) error + +// Chain composes multiple validators into a single validator. +// Validators are executed in order and the first error stops the chain. +func Chain(validators ...IssueValidator) IssueValidator { + return func(id string, issue *types.Issue) error { + for _, v := range validators { + if err := v(id, issue); err != nil { + return err + } + } + return nil + } +} + +// Exists validates that an issue is not nil. +func Exists() IssueValidator { + return func(id string, issue *types.Issue) error { + if issue == nil { + return fmt.Errorf("issue %s not found", id) + } + return nil + } +} + +// NotTemplate validates that an issue is not a template. +// Templates are read-only and cannot be modified. +func NotTemplate() IssueValidator { + return func(id string, issue *types.Issue) error { + if issue == nil { + return nil // Let Exists() handle nil check if needed + } + if issue.IsTemplate { + return fmt.Errorf("cannot modify template %s: templates are read-only; use 'bd mol pour' to create a work item", id) + } + return nil + } +} + +// NotPinned validates that an issue is not pinned. +// Returns an error if the issue is pinned, unless force is true. +func NotPinned(force bool) IssueValidator { + return func(id string, issue *types.Issue) error { + if issue == nil { + return nil // Let Exists() handle nil check if needed + } + if !force && issue.Status == types.StatusPinned { + return fmt.Errorf("cannot modify pinned issue %s (use --force to override)", id) + } + return nil + } +} + +// NotClosed validates that an issue is not already closed. +func NotClosed() IssueValidator { + return func(id string, issue *types.Issue) error { + if issue == nil { + return nil + } + if issue.Status == types.StatusClosed { + return fmt.Errorf("issue %s is already closed", id) + } + return nil + } +} + +// NotHooked validates that an issue is not in hooked status. +func NotHooked(force bool) IssueValidator { + return func(id string, issue *types.Issue) error { + if issue == nil { + return nil + } + if !force && issue.Status == types.StatusHooked { + return fmt.Errorf("cannot modify hooked issue %s (use --force to override)", id) + } + return nil + } +} + +// HasStatus validates that an issue has one of the allowed statuses. +func HasStatus(allowed ...types.Status) IssueValidator { + return func(id string, issue *types.Issue) error { + if issue == nil { + return nil + } + for _, status := range allowed { + if issue.Status == status { + return nil + } + } + return fmt.Errorf("issue %s has status %s, expected one of: %v", id, issue.Status, allowed) + } +} + +// HasType validates that an issue has one of the allowed types. +func HasType(allowed ...types.IssueType) IssueValidator { + return func(id string, issue *types.Issue) error { + if issue == nil { + return nil + } + for _, t := range allowed { + if issue.IssueType == t { + return nil + } + } + return fmt.Errorf("issue %s has type %s, expected one of: %v", id, issue.IssueType, allowed) + } +} + +// ForUpdate returns a validator chain for update operations. +// Validates: issue exists and is not a template. +func ForUpdate() IssueValidator { + return Chain( + Exists(), + NotTemplate(), + ) +} + +// ForClose returns a validator chain for close operations. +// Validates: issue exists, is not a template, and is not pinned (unless force). +func ForClose(force bool) IssueValidator { + return Chain( + Exists(), + NotTemplate(), + NotPinned(force), + ) +} + +// ForDelete returns a validator chain for delete operations. +// Validates: issue exists and is not a template. +func ForDelete() IssueValidator { + return Chain( + Exists(), + NotTemplate(), + ) +} + +// ForReopen returns a validator chain for reopen operations. +// Validates: issue exists, is not a template, and is closed. +func ForReopen() IssueValidator { + return Chain( + Exists(), + NotTemplate(), + HasStatus(types.StatusClosed), + ) +} diff --git a/internal/validation/issue_test.go b/internal/validation/issue_test.go new file mode 100644 index 00000000..928d0cda --- /dev/null +++ b/internal/validation/issue_test.go @@ -0,0 +1,367 @@ +package validation + +import ( + "testing" + + "github.com/steveyegge/beads/internal/types" +) + +func TestExists(t *testing.T) { + tests := []struct { + name string + issue *types.Issue + wantErr bool + }{ + { + name: "nil issue returns error", + issue: nil, + wantErr: true, + }, + { + name: "non-nil issue passes", + issue: &types.Issue{ID: "bd-test"}, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := Exists()("bd-test", tt.issue) + if (err != nil) != tt.wantErr { + t.Errorf("Exists() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func TestNotTemplate(t *testing.T) { + tests := []struct { + name string + issue *types.Issue + wantErr bool + }{ + { + name: "nil issue passes (delegated check)", + issue: nil, + wantErr: false, + }, + { + name: "non-template passes", + issue: &types.Issue{ID: "bd-test", IsTemplate: false}, + wantErr: false, + }, + { + name: "template returns error", + issue: &types.Issue{ID: "bd-test", IsTemplate: true}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := NotTemplate()("bd-test", tt.issue) + if (err != nil) != tt.wantErr { + t.Errorf("NotTemplate() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func TestNotPinned(t *testing.T) { + tests := []struct { + name string + issue *types.Issue + force bool + wantErr bool + }{ + { + name: "nil issue passes", + issue: nil, + force: false, + wantErr: false, + }, + { + name: "open issue passes", + issue: &types.Issue{ID: "bd-test", Status: types.StatusOpen}, + force: false, + wantErr: false, + }, + { + name: "pinned issue without force fails", + issue: &types.Issue{ID: "bd-test", Status: types.StatusPinned}, + force: false, + wantErr: true, + }, + { + name: "pinned issue with force passes", + issue: &types.Issue{ID: "bd-test", Status: types.StatusPinned}, + force: true, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := NotPinned(tt.force)("bd-test", tt.issue) + if (err != nil) != tt.wantErr { + t.Errorf("NotPinned() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func TestNotClosed(t *testing.T) { + tests := []struct { + name string + issue *types.Issue + wantErr bool + }{ + { + name: "nil issue passes", + issue: nil, + wantErr: false, + }, + { + name: "open issue passes", + issue: &types.Issue{ID: "bd-test", Status: types.StatusOpen}, + wantErr: false, + }, + { + name: "closed issue fails", + issue: &types.Issue{ID: "bd-test", Status: types.StatusClosed}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := NotClosed()("bd-test", tt.issue) + if (err != nil) != tt.wantErr { + t.Errorf("NotClosed() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func TestChain(t *testing.T) { + tests := []struct { + name string + issue *types.Issue + validators []IssueValidator + wantErr bool + }{ + { + name: "empty chain passes", + issue: &types.Issue{ID: "bd-test"}, + validators: []IssueValidator{}, + wantErr: false, + }, + { + name: "all validators pass", + issue: &types.Issue{ID: "bd-test", Status: types.StatusOpen}, + validators: []IssueValidator{ + Exists(), + NotTemplate(), + NotPinned(false), + }, + wantErr: false, + }, + { + name: "first validator fails stops chain", + issue: nil, + validators: []IssueValidator{ + Exists(), + NotTemplate(), + }, + wantErr: true, + }, + { + name: "middle validator fails", + issue: &types.Issue{ID: "bd-test", IsTemplate: true}, + validators: []IssueValidator{ + Exists(), + NotTemplate(), + NotPinned(false), + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + chain := Chain(tt.validators...) + err := chain("bd-test", tt.issue) + if (err != nil) != tt.wantErr { + t.Errorf("Chain() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func TestHasStatus(t *testing.T) { + tests := []struct { + name string + issue *types.Issue + allowed []types.Status + wantErr bool + }{ + { + name: "nil issue passes", + issue: nil, + allowed: []types.Status{types.StatusOpen}, + wantErr: false, + }, + { + name: "matching status passes", + issue: &types.Issue{ID: "bd-test", Status: types.StatusOpen}, + allowed: []types.Status{types.StatusOpen}, + wantErr: false, + }, + { + name: "one of multiple allowed passes", + issue: &types.Issue{ID: "bd-test", Status: types.StatusClosed}, + allowed: []types.Status{types.StatusOpen, types.StatusClosed}, + wantErr: false, + }, + { + name: "non-matching status fails", + issue: &types.Issue{ID: "bd-test", Status: types.StatusPinned}, + allowed: []types.Status{types.StatusOpen, types.StatusClosed}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := HasStatus(tt.allowed...)("bd-test", tt.issue) + if (err != nil) != tt.wantErr { + t.Errorf("HasStatus() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func TestHasType(t *testing.T) { + tests := []struct { + name string + issue *types.Issue + allowed []types.IssueType + wantErr bool + }{ + { + name: "nil issue passes", + issue: nil, + allowed: []types.IssueType{types.TypeTask}, + wantErr: false, + }, + { + name: "matching type passes", + issue: &types.Issue{ID: "bd-test", IssueType: types.TypeTask}, + allowed: []types.IssueType{types.TypeTask}, + wantErr: false, + }, + { + name: "one of multiple allowed passes", + issue: &types.Issue{ID: "bd-test", IssueType: types.TypeBug}, + allowed: []types.IssueType{types.TypeTask, types.TypeBug}, + wantErr: false, + }, + { + name: "non-matching type fails", + issue: &types.Issue{ID: "bd-test", IssueType: types.TypeEpic}, + allowed: []types.IssueType{types.TypeTask, types.TypeBug}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := HasType(tt.allowed...)("bd-test", tt.issue) + if (err != nil) != tt.wantErr { + t.Errorf("HasType() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func TestForUpdate(t *testing.T) { + tests := []struct { + name string + issue *types.Issue + wantErr bool + }{ + { + name: "nil issue fails", + issue: nil, + wantErr: true, + }, + { + name: "template fails", + issue: &types.Issue{ID: "bd-test", IsTemplate: true}, + wantErr: true, + }, + { + name: "regular issue passes", + issue: &types.Issue{ID: "bd-test", IsTemplate: false}, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ForUpdate()("bd-test", tt.issue) + if (err != nil) != tt.wantErr { + t.Errorf("ForUpdate() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func TestForClose(t *testing.T) { + tests := []struct { + name string + issue *types.Issue + force bool + wantErr bool + }{ + { + name: "nil issue fails", + issue: nil, + force: false, + wantErr: true, + }, + { + name: "template fails", + issue: &types.Issue{ID: "bd-test", IsTemplate: true}, + force: false, + wantErr: true, + }, + { + name: "pinned without force fails", + issue: &types.Issue{ID: "bd-test", Status: types.StatusPinned}, + force: false, + wantErr: true, + }, + { + name: "pinned with force passes", + issue: &types.Issue{ID: "bd-test", Status: types.StatusPinned}, + force: true, + wantErr: false, + }, + { + name: "regular open issue passes", + issue: &types.Issue{ID: "bd-test", Status: types.StatusOpen}, + force: false, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ForClose(tt.force)("bd-test", tt.issue) + if (err != nil) != tt.wantErr { + t.Errorf("ForClose() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +}