diff --git a/internal/config/config.go b/internal/config/config.go index 8c800376..3bb72821 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -69,7 +69,7 @@ func Initialize() error { // Environment variables take precedence over config file // E.g., BD_JSON, BD_NO_DAEMON, BD_ACTOR, BD_DB v.SetEnvPrefix("BD") - + // Replace hyphens and dots with underscores for env var mapping // This allows BD_NO_DAEMON to map to "no-daemon" config key v.SetEnvKeyReplacer(strings.NewReplacer(".", "_", "-", "_")) @@ -85,20 +85,20 @@ func Initialize() error { v.SetDefault("actor", "") v.SetDefault("issue-prefix", "") v.SetDefault("lock-timeout", "30s") - + // Additional environment variables (not prefixed with BD_) // These are bound explicitly for backward compatibility _ = v.BindEnv("flush-debounce", "BEADS_FLUSH_DEBOUNCE") _ = v.BindEnv("auto-start-daemon", "BEADS_AUTO_START_DAEMON") _ = v.BindEnv("identity", "BEADS_IDENTITY") _ = v.BindEnv("remote-sync-interval", "BEADS_REMOTE_SYNC_INTERVAL") - + // Set defaults for additional settings v.SetDefault("flush-debounce", "30s") v.SetDefault("auto-start-daemon", true) v.SetDefault("identity", "") v.SetDefault("remote-sync-interval", "30s") - + // Routing configuration defaults v.SetDefault("routing.mode", "auto") v.SetDefault("routing.default", ".") @@ -122,8 +122,13 @@ func Initialize() error { v.SetDefault("validation.on-create", "none") v.SetDefault("validation.on-sync", "none") + // Hierarchy configuration defaults (GH#995) + // Maximum nesting depth for hierarchical IDs (e.g., bd-abc.1.2.3) + // Default matches types.MaxHierarchyDepth constant + v.SetDefault("hierarchy.max-depth", 3) + // Git configuration defaults (GH#600) - v.SetDefault("git.author", "") // Override commit author (e.g., "beads-bot ") + v.SetDefault("git.author", "") // Override commit author (e.g., "beads-bot ") v.SetDefault("git.no-gpg-sign", false) // Disable GPG signing for beads commits // Directory-aware label scoping (GH#541) @@ -200,7 +205,10 @@ func GetValueSource(key string) ConfigSource { // CheckOverrides checks for configuration overrides and returns a list of detected overrides. // This is useful for informing users when env vars or flags override config file values. // flagOverrides is a map of key -> (flagValue, flagWasSet) for flags that were explicitly set. -func CheckOverrides(flagOverrides map[string]struct{ Value interface{}; WasSet bool }) []ConfigOverride { +func CheckOverrides(flagOverrides map[string]struct { + Value interface{} + WasSet bool +}) []ConfigOverride { var overrides []ConfigOverride for key, flagInfo := range flagOverrides { diff --git a/internal/config/yaml_config.go b/internal/config/yaml_config.go index cdeec260..3d22f7f0 100644 --- a/internal/config/yaml_config.go +++ b/internal/config/yaml_config.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "regexp" + "strconv" "strings" ) @@ -18,16 +19,16 @@ import ( // at startup, not from SQLite). var YamlOnlyKeys = map[string]bool{ // Bootstrap flags (affect how bd starts) - "no-db": true, - "no-daemon": true, - "no-auto-flush": true, - "no-auto-import": true, - "json": true, + "no-db": true, + "no-daemon": true, + "no-auto-flush": true, + "no-auto-import": true, + "json": true, "auto-start-daemon": true, // Database and identity - "db": true, - "actor": true, + "db": true, + "actor": true, "identity": true, // Timing settings @@ -36,14 +37,14 @@ var YamlOnlyKeys = map[string]bool{ "remote-sync-interval": true, // Git settings - "git.author": true, - "git.no-gpg-sign": true, - "no-push": true, - "no-git-ops": true, // Disable git ops in bd prime session close protocol (GH#593) + "git.author": true, + "git.no-gpg-sign": true, + "no-push": true, + "no-git-ops": true, // Disable git ops in bd prime session close protocol (GH#593) // Sync settings - "sync-branch": true, - "sync.branch": true, + "sync-branch": true, + "sync.branch": true, "sync.require_confirmation_on_mass_delete": true, // Daemon settings (GH#871: team-wide auto-sync config) @@ -64,6 +65,9 @@ var YamlOnlyKeys = map[string]bool{ // Values: "warn" | "error" | "none" "validation.on-create": true, "validation.on-sync": true, + + // Hierarchy settings (GH#995) + "hierarchy.max-depth": true, } // IsYamlOnlyKey returns true if the given key should be stored in config.yaml @@ -75,7 +79,7 @@ func IsYamlOnlyKey(key string) bool { } // Check prefix matches for nested keys - prefixes := []string{"routing.", "sync.", "git.", "directory.", "repos.", "external_projects.", "validation.", "daemon."} + prefixes := []string{"routing.", "sync.", "git.", "directory.", "repos.", "external_projects.", "validation.", "daemon.", "hierarchy."} for _, prefix := range prefixes { if strings.HasPrefix(key, prefix) { return true @@ -105,6 +109,11 @@ func normalizeYamlKey(key string) string { // It handles both adding new keys and updating existing (possibly commented) keys. // Keys are normalized to their canonical yaml format (e.g., sync.branch -> sync-branch). func SetYamlConfig(key, value string) error { + // Validate specific keys (GH#995) + if err := validateYamlConfigValue(key, value); err != nil { + return err + } + configPath, err := findProjectConfigYaml() if err != nil { return err @@ -274,3 +283,20 @@ func needsQuoting(s string) bool { } return false } + +// validateYamlConfigValue validates a configuration value before setting. +// Returns an error if the value is invalid for the given key. +func validateYamlConfigValue(key, value string) error { + switch key { + case "hierarchy.max-depth": + // Must be a positive integer >= 1 (GH#995) + depth, err := strconv.Atoi(value) + if err != nil { + return fmt.Errorf("hierarchy.max-depth must be a positive integer, got %q", value) + } + if depth < 1 { + return fmt.Errorf("hierarchy.max-depth must be at least 1, got %d", depth) + } + } + return nil +} diff --git a/internal/config/yaml_config_test.go b/internal/config/yaml_config_test.go index 347cd597..c59c9447 100644 --- a/internal/config/yaml_config_test.go +++ b/internal/config/yaml_config_test.go @@ -37,6 +37,10 @@ func TestIsYamlOnlyKey(t *testing.T) { {"daemon.auto_pull", true}, {"daemon.custom_setting", true}, // prefix match + // Hierarchy settings (GH#995) + {"hierarchy.max-depth", true}, + {"hierarchy.custom_setting", true}, // prefix match + // SQLite keys (should return false) {"jira.url", false}, {"jira.project", false}, @@ -164,10 +168,10 @@ func TestNormalizeYamlKey(t *testing.T) { input string expected string }{ - {"sync.branch", "sync-branch"}, // alias should be normalized - {"sync-branch", "sync-branch"}, // already canonical - {"no-db", "no-db"}, // no alias, unchanged - {"json", "json"}, // no alias, unchanged + {"sync.branch", "sync-branch"}, // alias should be normalized + {"sync-branch", "sync-branch"}, // already canonical + {"no-db", "no-db"}, // no alias, unchanged + {"json", "json"}, // no alias, unchanged {"routing.mode", "routing.mode"}, // no alias for this one } @@ -328,3 +332,53 @@ other-setting: value t.Errorf("config.yaml should preserve other settings, got:\n%s", contentStr) } } + +// TestValidateYamlConfigValue_HierarchyMaxDepth tests validation of hierarchy.max-depth (GH#995) +func TestValidateYamlConfigValue_HierarchyMaxDepth(t *testing.T) { + tests := []struct { + name string + value string + expectErr bool + errMsg string + }{ + {"valid positive integer", "5", false, ""}, + {"valid minimum value", "1", false, ""}, + {"valid large value", "100", false, ""}, + {"invalid zero", "0", true, "hierarchy.max-depth must be at least 1, got 0"}, + {"invalid negative", "-1", true, "hierarchy.max-depth must be at least 1, got -1"}, + {"invalid non-integer", "abc", true, "hierarchy.max-depth must be a positive integer, got \"abc\""}, + {"invalid float", "3.5", true, "hierarchy.max-depth must be a positive integer, got \"3.5\""}, + {"invalid empty", "", true, "hierarchy.max-depth must be a positive integer, got \"\""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateYamlConfigValue("hierarchy.max-depth", tt.value) + if tt.expectErr { + if err == nil { + t.Errorf("expected error for value %q, got nil", tt.value) + } else if err.Error() != tt.errMsg { + t.Errorf("expected error %q, got %q", tt.errMsg, err.Error()) + } + } else { + if err != nil { + t.Errorf("unexpected error for value %q: %v", tt.value, err) + } + } + }) + } +} + +// TestValidateYamlConfigValue_OtherKeys tests that other keys are not validated +func TestValidateYamlConfigValue_OtherKeys(t *testing.T) { + // Other keys should pass validation regardless of value + err := validateYamlConfigValue("no-db", "invalid") + if err != nil { + t.Errorf("unexpected error for no-db: %v", err) + } + + err = validateYamlConfigValue("routing.mode", "anything") + if err != nil { + t.Errorf("unexpected error for routing.mode: %v", err) + } +} diff --git a/internal/storage/memory/memory.go b/internal/storage/memory/memory.go index a941dbbc..5eb68b4d 100644 --- a/internal/storage/memory/memory.go +++ b/internal/storage/memory/memory.go @@ -12,6 +12,7 @@ import ( "sync" "time" + "github.com/steveyegge/beads/internal/config" "github.com/steveyegge/beads/internal/storage" "github.com/steveyegge/beads/internal/types" ) @@ -21,14 +22,14 @@ type MemoryStorage struct { mu sync.RWMutex // Protects all maps // Core data - issues map[string]*types.Issue // ID -> Issue + issues map[string]*types.Issue // ID -> Issue dependencies map[string][]*types.Dependency // IssueID -> Dependencies - labels map[string][]string // IssueID -> Labels - events map[string][]*types.Event // IssueID -> Events - comments map[string][]*types.Comment // IssueID -> Comments - config map[string]string // Config key-value pairs - metadata map[string]string // Metadata key-value pairs - counters map[string]int // Prefix -> Last ID + labels map[string][]string // IssueID -> Labels + events map[string][]*types.Event // IssueID -> Events + comments map[string][]*types.Comment // IssueID -> Comments + config map[string]string // Config key-value pairs + metadata map[string]string // Metadata key-value pairs + counters map[string]int // Prefix -> Last ID // Indexes for O(1) lookups externalRefToID map[string]string // ExternalRef -> IssueID @@ -1598,10 +1599,9 @@ func (m *MemoryStorage) GetNextChildID(ctx context.Context, parentID string) (st return "", fmt.Errorf("parent issue %s does not exist", parentID) } - // Calculate depth (count dots) - depth := strings.Count(parentID, ".") - if depth >= 3 { - return "", fmt.Errorf("maximum hierarchy depth (3) exceeded for parent %s", parentID) + // Check hierarchy depth limit (GH#995) + if err := types.CheckHierarchyDepth(parentID, config.GetInt("hierarchy.max-depth")); err != nil { + return "", err } // Get or initialize counter for this parent diff --git a/internal/storage/memory/memory_test.go b/internal/storage/memory/memory_test.go index a4979500..73ae42f1 100644 --- a/internal/storage/memory/memory_test.go +++ b/internal/storage/memory/memory_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/steveyegge/beads/internal/config" "github.com/steveyegge/beads/internal/types" ) @@ -1276,3 +1277,76 @@ func TestGetIssueByExternalRefLoadFromIssues(t *testing.T) { t.Errorf("Expected to find bd-2 by external ref jira#200") } } + +// TestGetNextChildID_ConfigurableMaxDepth tests that hierarchy.max-depth config is respected (GH#995) +func TestGetNextChildID_ConfigurableMaxDepth(t *testing.T) { + // Initialize config for testing + if err := config.Initialize(); err != nil { + t.Fatalf("failed to initialize config: %v", err) + } + + // Ensure config is reset even if test fails or panics + t.Cleanup(func() { + config.Set("hierarchy.max-depth", 3) + }) + + store := setupTestMemory(t) + defer store.Close() + ctx := context.Background() + + // Create a chain of issues up to depth 3 + issues := []struct { + id string + title string + }{ + {"bd-depth", "Root"}, + {"bd-depth.1", "Level 1"}, + {"bd-depth.1.1", "Level 2"}, + {"bd-depth.1.1.1", "Level 3"}, + } + + for _, issue := range issues { + iss := &types.Issue{ + ID: issue.id, + Title: issue.title, + Description: "Test issue", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + } + if err := store.CreateIssue(ctx, iss, "test"); err != nil { + t.Fatalf("failed to create issue %s: %v", issue.id, err) + } + } + + // Test 1: With default max-depth (3), depth 4 should fail + config.Set("hierarchy.max-depth", 3) + _, err := store.GetNextChildID(ctx, "bd-depth.1.1.1") + if err == nil { + t.Errorf("expected error for depth 4 with max-depth=3, got nil") + } + if err != nil && err.Error() != "maximum hierarchy depth (3) exceeded for parent bd-depth.1.1.1" { + t.Errorf("unexpected error message: %v", err) + } + + // Test 2: With max-depth=5, depth 4 should succeed + config.Set("hierarchy.max-depth", 5) + childID, err := store.GetNextChildID(ctx, "bd-depth.1.1.1") + if err != nil { + t.Errorf("depth 4 should be allowed with max-depth=5, got error: %v", err) + } + expectedID := "bd-depth.1.1.1.1" + if childID != expectedID { + t.Errorf("expected %s, got %s", expectedID, childID) + } + + // Test 3: With max-depth=2, depth 3 should fail + config.Set("hierarchy.max-depth", 2) + _, err = store.GetNextChildID(ctx, "bd-depth.1.1") + if err == nil { + t.Errorf("expected error for depth 3 with max-depth=2, got nil") + } + if err != nil && err.Error() != "maximum hierarchy depth (2) exceeded for parent bd-depth.1.1" { + t.Errorf("unexpected error message: %v", err) + } +} diff --git a/internal/storage/sqlite/child_id_test.go b/internal/storage/sqlite/child_id_test.go index b8335099..03594201 100644 --- a/internal/storage/sqlite/child_id_test.go +++ b/internal/storage/sqlite/child_id_test.go @@ -5,6 +5,7 @@ import ( "os" "testing" + "github.com/steveyegge/beads/internal/config" "github.com/steveyegge/beads/internal/types" ) @@ -427,6 +428,81 @@ this is not json either } } +// TestGetNextChildID_ConfigurableMaxDepth tests that hierarchy.max-depth config is respected (GH#995) +func TestGetNextChildID_ConfigurableMaxDepth(t *testing.T) { + // Initialize config for testing + if err := config.Initialize(); err != nil { + t.Fatalf("failed to initialize config: %v", err) + } + + // Ensure config is reset even if test fails or panics + t.Cleanup(func() { + config.Set("hierarchy.max-depth", 3) + }) + + tmpFile := t.TempDir() + "/test.db" + defer os.Remove(tmpFile) + store := newTestStore(t, tmpFile) + defer store.Close() + ctx := context.Background() + + // Create a chain of issues up to depth 3 + issues := []struct { + id string + title string + }{ + {"bd-depth", "Root"}, + {"bd-depth.1", "Level 1"}, + {"bd-depth.1.1", "Level 2"}, + {"bd-depth.1.1.1", "Level 3"}, + } + + for _, issue := range issues { + iss := &types.Issue{ + ID: issue.id, + Title: issue.title, + Description: "Test issue", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + } + if err := store.CreateIssue(ctx, iss, "test"); err != nil { + t.Fatalf("failed to create issue %s: %v", issue.id, err) + } + } + + // Test 1: With default max-depth (3), depth 4 should fail + config.Set("hierarchy.max-depth", 3) + _, err := store.GetNextChildID(ctx, "bd-depth.1.1.1") + if err == nil { + t.Errorf("expected error for depth 4 with max-depth=3, got nil") + } + if err != nil && err.Error() != "maximum hierarchy depth (3) exceeded for parent bd-depth.1.1.1" { + t.Errorf("unexpected error message: %v", err) + } + + // Test 2: With max-depth=5, depth 4 should succeed + config.Set("hierarchy.max-depth", 5) + childID, err := store.GetNextChildID(ctx, "bd-depth.1.1.1") + if err != nil { + t.Errorf("depth 4 should be allowed with max-depth=5, got error: %v", err) + } + expectedID := "bd-depth.1.1.1.1" + if childID != expectedID { + t.Errorf("expected %s, got %s", expectedID, childID) + } + + // Test 3: With max-depth=2, depth 3 should fail + config.Set("hierarchy.max-depth", 2) + _, err = store.GetNextChildID(ctx, "bd-depth.1.1") + if err == nil { + t.Errorf("expected error for depth 3 with max-depth=2, got nil") + } + if err != nil && err.Error() != "maximum hierarchy depth (2) exceeded for parent bd-depth.1.1" { + t.Errorf("unexpected error message: %v", err) + } +} + // TestGetNextChildID_ResurrectParentChain tests resurrection of deeply nested missing parents (bd-ar2.7) func TestGetNextChildID_ResurrectParentChain(t *testing.T) { tmpDir := t.TempDir() diff --git a/internal/storage/sqlite/hash_ids.go b/internal/storage/sqlite/hash_ids.go index e5890c63..97092465 100644 --- a/internal/storage/sqlite/hash_ids.go +++ b/internal/storage/sqlite/hash_ids.go @@ -4,7 +4,9 @@ import ( "context" "database/sql" "fmt" - "strings" + + "github.com/steveyegge/beads/internal/config" + "github.com/steveyegge/beads/internal/types" ) // getNextChildNumber atomically increments and returns the next child counter for a parent issue. @@ -48,19 +50,18 @@ func (s *SQLiteStorage) GetNextChildID(ctx context.Context, parentID string) (st return "", fmt.Errorf("parent issue %s does not exist and could not be resurrected from JSONL history", parentID) } } - - // Calculate current depth by counting dots - depth := strings.Count(parentID, ".") - if depth >= 3 { - return "", fmt.Errorf("maximum hierarchy depth (3) exceeded for parent %s", parentID) + + // Check hierarchy depth limit (GH#995) + if err := types.CheckHierarchyDepth(parentID, config.GetInt("hierarchy.max-depth")); err != nil { + return "", err } - + // Get next child number atomically nextNum, err := s.getNextChildNumber(ctx, parentID) if err != nil { return "", err } - + // Format as parentID.counter childID := fmt.Sprintf("%s.%d", parentID, nextNum) return childID, nil diff --git a/internal/types/id_generator.go b/internal/types/id_generator.go index 43729cae..bf2f46fb 100644 --- a/internal/types/id_generator.go +++ b/internal/types/id_generator.go @@ -90,3 +90,25 @@ func ParseHierarchicalID(id string) (rootID, parentID string, depth int) { // MaxHierarchyDepth is the maximum nesting level for hierarchical IDs. // Prevents over-decomposition and keeps IDs manageable. const MaxHierarchyDepth = 3 + +// CheckHierarchyDepth validates that adding a child to parentID won't exceed maxDepth. +// Returns an error if the depth would be exceeded. +// If maxDepth < 1, it defaults to MaxHierarchyDepth. +func CheckHierarchyDepth(parentID string, maxDepth int) error { + if maxDepth < 1 { + maxDepth = MaxHierarchyDepth + } + + // Count dots to determine current depth + depth := 0 + for _, ch := range parentID { + if ch == '.' { + depth++ + } + } + + if depth >= maxDepth { + return fmt.Errorf("maximum hierarchy depth (%d) exceeded for parent %s", maxDepth, parentID) + } + return nil +} diff --git a/internal/types/id_generator_test.go b/internal/types/id_generator_test.go index ac5a4901..839e5adb 100644 --- a/internal/types/id_generator_test.go +++ b/internal/types/id_generator_test.go @@ -209,3 +209,46 @@ func BenchmarkGenerateChildID(b *testing.B) { GenerateChildID("bd-af78e9a2", 42) } } + +func TestCheckHierarchyDepth(t *testing.T) { + tests := []struct { + name string + parentID string + maxDepth int + wantErr bool + errMsg string + }{ + // Default maxDepth (uses MaxHierarchyDepth = 3) + {"root parent with default depth", "bd-abc123", 0, false, ""}, + {"depth 1 parent with default depth", "bd-abc123.1", 0, false, ""}, + {"depth 2 parent with default depth", "bd-abc123.1.2", 0, false, ""}, + {"depth 3 parent with default depth - exceeds", "bd-abc123.1.2.3", 0, true, "maximum hierarchy depth (3) exceeded for parent bd-abc123.1.2.3"}, + + // Custom maxDepth + {"root parent with max=1", "bd-abc123", 1, false, ""}, + {"depth 1 parent with max=1 - exceeds", "bd-abc123.1", 1, true, "maximum hierarchy depth (1) exceeded for parent bd-abc123.1"}, + {"depth 3 parent with max=5", "bd-abc123.1.2.3", 5, false, ""}, + {"depth 4 parent with max=5", "bd-abc123.1.2.3.4", 5, false, ""}, + {"depth 5 parent with max=5 - exceeds", "bd-abc123.1.2.3.4.5", 5, true, "maximum hierarchy depth (5) exceeded for parent bd-abc123.1.2.3.4.5"}, + + // Negative maxDepth falls back to default + {"negative maxDepth uses default", "bd-abc123.1.2.3", -1, true, "maximum hierarchy depth (3) exceeded for parent bd-abc123.1.2.3"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := CheckHierarchyDepth(tt.parentID, tt.maxDepth) + if tt.wantErr { + if err == nil { + t.Errorf("expected error, got nil") + } else if err.Error() != tt.errMsg { + t.Errorf("expected error %q, got %q", tt.errMsg, err.Error()) + } + } else { + if err != nil { + t.Errorf("unexpected error: %v", err) + } + } + }) + } +}