johno/beads
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge main into PR #160 - combine reverse mode with substring bugfix

Browse Source 
Amp-Thread-ID: https://ampcode.com/threads/T-b2413b0e-2720-45b1-9b3d-acaa7d4cf9b4
Co-authored-by: Amp <amp@ampcode.com>
This commit is contained in:
Steve Yegge
2025-10-27 13:05:41 -07:00
parent 2035e02a34 8c2679a80e
commit d2989dd0f3
16 changed files with 2842 additions and 488 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
internal/storage/sqlite/dependencies.go
View File

@@ -501,7 +501,10 @@ func (s *SQLiteStorage) GetDependencyTree(ctx context.Context, issueID string, m
JOIN dependencies d ON i.id = d.issue_id
JOIN tree t ON d.depends_on_id = t.id
WHERE t.depth < ?
AND t.path NOT LIKE '%' || i.id || '%'
AND t.path != i.id
AND t.path NOT LIKE i.id || '→%'
AND t.path NOT LIKE '%→' || i.id || '→%'
AND t.path NOT LIKE '%→' || i.id
)
SELECT id, title, status, priority, description, design,
acceptance_criteria, notes, issue_type, assignee,
@@ -539,7 +542,10 @@ func (s *SQLiteStorage) GetDependencyTree(ctx context.Context, issueID string, m
JOIN dependencies d ON i.id = d.depends_on_id
JOIN tree t ON d.issue_id = t.id
WHERE t.depth < ?
AND t.path NOT LIKE '%' || i.id || '%'
AND t.path != i.id
AND t.path NOT LIKE i.id || '→%'
AND t.path NOT LIKE '%→' || i.id || '→%'
AND t.path NOT LIKE '%→' || i.id
)
SELECT id, title, status, priority, description, design,
acceptance_criteria, notes, issue_type, assignee,

103
internal/storage/sqlite/dependencies_test.go
View File

@@ -800,3 +800,106 @@ func TestGetDependencyTree_Reverse(t *testing.T) {
t.Errorf("Expected depth 2 for %s in reverse tree, got %d", issue3.ID, depthMap[issue3.ID])
}
}
func TestGetDependencyTree_SubstringBug(t *testing.T) {
store, cleanup := setupTestDB(t)
defer cleanup()
ctx := context.Background()
// Create 10 issues so we have both bd-1 and bd-10 (substring issue)
// The bug: when traversing from bd-10, bd-1 gets incorrectly excluded
// because "bd-10" contains "bd-1" as a substring
issues := make([]*types.Issue, 10)
for i := 0; i < 10; i++ {
issues[i] = &types.Issue{
Title: fmt.Sprintf("Issue %d", i+1),
Status: types.StatusOpen,
Priority: 1,
IssueType: types.TypeTask,
}
err := store.CreateIssue(ctx, issues[i], "test-user")
if err != nil {
t.Fatalf("CreateIssue failed: %v", err)
}
}
// Create chain: bd-10 → bd-9 → bd-8 → bd-2 → bd-1
// This tests the substring bug where bd-1 should appear but won't due to substring matching
err := store.AddDependency(ctx, &types.Dependency{
IssueID: issues[9].ID, // bd-10
DependsOnID: issues[8].ID, // bd-9
Type: types.DepBlocks,
}, "test-user")
if err != nil {
t.Fatalf("AddDependency bd-10→bd-9 failed: %v", err)
}
err = store.AddDependency(ctx, &types.Dependency{
IssueID: issues[8].ID, // bd-9
DependsOnID: issues[7].ID, // bd-8
Type: types.DepBlocks,
}, "test-user")
if err != nil {
t.Fatalf("AddDependency bd-9→bd-8 failed: %v", err)
}
err = store.AddDependency(ctx, &types.Dependency{
IssueID: issues[7].ID, // bd-8
DependsOnID: issues[1].ID, // bd-2
Type: types.DepBlocks,
}, "test-user")
if err != nil {
t.Fatalf("AddDependency bd-8→bd-2 failed: %v", err)
}
err = store.AddDependency(ctx, &types.Dependency{
IssueID: issues[1].ID, // bd-2
DependsOnID: issues[0].ID, // bd-1
Type: types.DepBlocks,
}, "test-user")
if err != nil {
t.Fatalf("AddDependency bd-2→bd-1 failed: %v", err)
}
// Get tree starting from bd-10
tree, err := store.GetDependencyTree(ctx, issues[9].ID, 10, false, false)
if err != nil {
t.Fatalf("GetDependencyTree failed: %v", err)
}
// Create map of issue IDs in tree for easy checking
treeIDs := make(map[string]bool)
for _, node := range tree {
treeIDs[node.ID] = true
}
// Verify all issues in the chain appear in the tree
// This is the KEY test: bd-1 should be in the tree
// With the substring bug, bd-1 will be missing because "bd-10" contains "bd-1"
expectedIssues := []int{9, 8, 7, 1, 0} // bd-10, bd-9, bd-8, bd-2, bd-1
for _, idx := range expectedIssues {
if !treeIDs[issues[idx].ID] {
t.Errorf("Expected %s in dependency tree, but it was missing (substring bug)", issues[idx].ID)
}
}
// Verify we have the correct number of nodes
if len(tree) != 5 {
t.Errorf("Expected 5 nodes in tree, got %d. Missing nodes indicate substring bug.", len(tree))
}
// Verify depths are correct
depthMap := make(map[string]int)
for _, node := range tree {
depthMap[node.ID] = node.Depth
}
// Check depths: bd-10(0) → bd-9(1) → bd-8(2) → bd-2(3) → bd-1(4)
if depthMap[issues[9].ID] != 0 {
t.Errorf("Expected bd-10 at depth 0, got %d", depthMap[issues[9].ID])
}
if depthMap[issues[0].ID] != 4 {
t.Errorf("Expected bd-1 at depth 4, got %d", depthMap[issues[0].ID])
}
}

165
internal/storage/sqlite/prefix_validation_test.go Normal file
View File

@@ -0,0 +1,165 @@
package sqlite
import (
"context"
"os"
"path/filepath"
"testing"
"github.com/steveyegge/beads/internal/types"
)
func TestPrefixValidation(t *testing.T) {
tmpDir, err := os.MkdirTemp("", "beads-prefix-test-*")
if err != nil {
t.Fatalf("failed to create temp dir: %v", err)
}
defer os.RemoveAll(tmpDir)
dbPath := filepath.Join(tmpDir, "test.db")
store, err := New(dbPath)
if err != nil {
t.Fatalf("failed to create storage: %v", err)
}
defer store.Close()
ctx := context.Background()
// Set prefix to "test"
if err := store.SetConfig(ctx, "issue_prefix", "test"); err != nil {
t.Fatalf("failed to set prefix: %v", err)
}
tests := []struct {
name string
issueID string
wantErr bool
}{
{
name: "valid prefix - matches",
issueID: "test-123",
wantErr: false,
},
{
name: "invalid prefix - wrong prefix",
issueID: "bd-456",
wantErr: true,
},
{
name: "invalid prefix - no dash",
issueID: "test123",
wantErr: true,
},
{
name: "invalid prefix - empty",
issueID: "",
wantErr: false, // Empty ID triggers auto-generation
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
issue := &types.Issue{
ID: tt.issueID,
Title: "Test issue",
Status: types.StatusOpen,
Priority: 1,
IssueType: types.TypeTask,
}
err := store.CreateIssue(ctx, issue, "test-user")
if (err != nil) != tt.wantErr {
t.Errorf("CreateIssue() error = %v, wantErr %v", err, tt.wantErr)
}
// If we expected success and the ID was empty, verify it was generated with correct prefix
if err == nil && tt.issueID == "" {
if issue.ID == "" {
t.Error("ID should be generated")
}
if issue.ID[:5] != "test-" {
t.Errorf("Generated ID should have prefix 'test-', got %s", issue.ID)
}
}
})
}
}
func TestPrefixValidationBatch(t *testing.T) {
tmpDir, err := os.MkdirTemp("", "beads-prefix-batch-test-*")
if err != nil {
t.Fatalf("failed to create temp dir: %v", err)
}
defer os.RemoveAll(tmpDir)
dbPath := filepath.Join(tmpDir, "test.db")
store, err := New(dbPath)
if err != nil {
t.Fatalf("failed to create storage: %v", err)
}
defer store.Close()
ctx := context.Background()
// Set prefix to "batch"
if err := store.SetConfig(ctx, "issue_prefix", "batch"); err != nil {
t.Fatalf("failed to set prefix: %v", err)
}
tests := []struct {
name string
issues []*types.Issue
wantErr bool
}{
{
name: "all valid prefixes",
issues: []*types.Issue{
{ID: "batch-1", Title: "Test 1", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask},
{ID: "batch-2", Title: "Test 2", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask},
},
wantErr: false,
},
{
name: "one invalid prefix in batch",
issues: []*types.Issue{
{ID: "batch-10", Title: "Test 1", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask},
{ID: "wrong-20", Title: "Test 2", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask},
},
wantErr: true,
},
{
name: "mixed auto-generated and explicit",
issues: []*types.Issue{
{ID: "batch-100", Title: "Explicit ID", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask},
{ID: "", Title: "Auto ID", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask},
},
wantErr: false,
},
{
name: "mixed with invalid prefix",
issues: []*types.Issue{
{ID: "", Title: "Auto ID", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask},
{ID: "invalid-500", Title: "Wrong prefix", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask},
},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := store.CreateIssues(ctx, tt.issues, "test-user")
if (err != nil) != tt.wantErr {
t.Errorf("CreateIssues() error = %v, wantErr %v", err, tt.wantErr)
}
// For successful batches, verify all IDs have correct prefix
if err == nil {
for i, issue := range tt.issues {
if issue.ID[:6] != "batch-" {
t.Errorf("Issue %d: ID should have prefix 'batch-', got %s", i, issue.ID)
}
}
}
})
}
}

61
internal/storage/sqlite/sqlite.go
View File

@@ -617,19 +617,19 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act
}
}()
// Get prefix from config (needed for both ID generation and validation)
var prefix string
err = conn.QueryRowContext(ctx, `SELECT value FROM config WHERE key = ?`, "issue_prefix").Scan(&prefix)
if err == sql.ErrNoRows || prefix == "" {
// CRITICAL: Reject operation if issue_prefix config is missing (bd-166)
// This prevents duplicate issues with wrong prefix
return fmt.Errorf("database not initialized: issue_prefix config is missing (run 'bd init --prefix <prefix>' first)")
} else if err != nil {
return fmt.Errorf("failed to get config: %w", err)
}
// Generate ID if not set (inside transaction to prevent race conditions)
if issue.ID == "" {
// Get prefix from config
var prefix string
err := conn.QueryRowContext(ctx, `SELECT value FROM config WHERE key = ?`, "issue_prefix").Scan(&prefix)
if err == sql.ErrNoRows || prefix == "" {
// CRITICAL: Reject operation if issue_prefix config is missing (bd-166)
// This prevents duplicate issues with wrong prefix
return fmt.Errorf("database not initialized: issue_prefix config is missing (run 'bd init --prefix <prefix>' first)")
} else if err != nil {
return fmt.Errorf("failed to get config: %w", err)
}
// Atomically initialize counter (if needed) and get next ID (within transaction)
// This ensures the counter starts from the max existing ID, not 1
// CRITICAL: We rely on BEGIN IMMEDIATE above to serialize this operation across processes
@@ -665,6 +665,13 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act
}
issue.ID = fmt.Sprintf("%s-%d", prefix, nextID)
} else {
// Validate that explicitly provided ID matches the configured prefix (bd-177)
// This prevents wrong-prefix bugs when IDs are manually specified
expectedPrefix := prefix + "-"
if !strings.HasPrefix(issue.ID, expectedPrefix) {
return fmt.Errorf("issue ID '%s' does not match configured prefix '%s'", issue.ID, prefix)
}
}
// Insert issue
@@ -743,19 +750,7 @@ func validateBatchIssues(issues []*types.Issue) error {
// generateBatchIDs generates IDs for all issues that need them atomically
func generateBatchIDs(ctx context.Context, conn *sql.Conn, issues []*types.Issue, dbPath string) error {
// Count how many issues need IDs
needIDCount := 0
for _, issue := range issues {
if issue.ID == "" {
needIDCount++
}
}
if needIDCount == 0 {
return nil
}
// Get prefix from config
// Get prefix from config (needed for both generation and validation)
var prefix string
err := conn.QueryRowContext(ctx, `SELECT value FROM config WHERE key = ?`, "issue_prefix").Scan(&prefix)
if err == sql.ErrNoRows || prefix == "" {
@@ -765,6 +760,24 @@ func generateBatchIDs(ctx context.Context, conn *sql.Conn, issues []*types.Issue
return fmt.Errorf("failed to get config: %w", err)
}
// Count how many issues need IDs and validate explicitly provided IDs
needIDCount := 0
expectedPrefix := prefix + "-"
for _, issue := range issues {
if issue.ID == "" {
needIDCount++
} else {
// Validate that explicitly provided ID matches the configured prefix (bd-177)
if !strings.HasPrefix(issue.ID, expectedPrefix) {
return fmt.Errorf("issue ID '%s' does not match configured prefix '%s'", issue.ID, prefix)
}
}
}
if needIDCount == 0 {
return nil
}
// Atomically reserve ID range
var nextID int
err = conn.QueryRowContext(ctx, `