Implement cycle detection and prevention improvements
- Add diagnostic warnings when cycles detected after dep add (bd-309) - Add semantic validation for parent-child dependency direction (bd-308) - Document cycle handling behavior in code, README, and DESIGN (bd-310) Changes: - cmd/bd/dep.go: Add DetectCycles() call and warning after dep add - internal/storage/sqlite/dependencies.go: Add parent-child direction validation and comprehensive cycle prevention comments - internal/storage/sqlite/dependencies_test.go: Add TestParentChildValidation - README.md: Add dependency types and cycle prevention section with examples - DESIGN.md: Add detailed cycle prevention design rationale and trade-offs
This commit is contained in:
File diff suppressed because one or more lines are too long
50
DESIGN.md
50
DESIGN.md
@@ -366,12 +366,56 @@ type Dependency struct {
|
||||
|
||||
type DependencyType string
|
||||
const (
|
||||
DepBlocks DependencyType = "blocks" // hard blocker
|
||||
DepRelated DependencyType = "related" // soft relationship
|
||||
DepParentChild DependencyType = "parent-child" // epic/subtask
|
||||
DepBlocks DependencyType = "blocks" // hard blocker
|
||||
DepRelated DependencyType = "related" // soft relationship
|
||||
DepParentChild DependencyType = "parent-child" // epic/subtask
|
||||
DepDiscoveredFrom DependencyType = "discovered-from" // discovered during work
|
||||
)
|
||||
```
|
||||
|
||||
#### Cycle Prevention Design
|
||||
|
||||
**Goal**: Maintain a directed acyclic graph (DAG) across all dependency types.
|
||||
|
||||
**Rationale**: Cycles create three major problems:
|
||||
|
||||
1. **Ready Work Calculation**: Issues in a cycle appear blocked by each other, hiding them from `bd ready` even though there's no clear blocking reason. Example: A depends on B, B depends on A → neither appears as ready work.
|
||||
|
||||
2. **Semantic Confusion**: Circular dependencies are conceptually problematic. If A depends on B and B depends on A (directly or through other issues), which should be done first? The answer is ambiguous.
|
||||
|
||||
3. **Traversal Complexity**: Operations like `bd dep tree`, blocking propagation, and hierarchy display rely on DAG structure. Cycles require special handling (cycle detection, path marking) or risk infinite loops.
|
||||
|
||||
**Implementation Strategy**:
|
||||
|
||||
- **Prevention over Detection**: We prevent cycles at insertion time in `AddDependency`, so cycles can never exist in the database.
|
||||
- **Cross-Type Checking**: We check ALL dependency types, not just `blocks`. Cross-type cycles (e.g., A blocks B, B parent-child A) are just as problematic as single-type cycles.
|
||||
- **Recursive CTE**: SQLite recursive common table expression traverses from `DependsOnID` to check if `IssueID` is reachable. If yes, adding the edge would complete a cycle.
|
||||
- **Depth Limit**: Traversal limited to 100 levels to prevent excessive query cost and handle edge cases.
|
||||
- **Transaction Safety**: Cycle check happens in transaction before INSERT, so no partial state on rejection.
|
||||
|
||||
**Performance Considerations**:
|
||||
|
||||
- Cycle check runs on every `AddDependency` call (not skippable)
|
||||
- Indexed on `dependencies.issue_id` for efficient traversal
|
||||
- Cost grows with dependency graph depth, not total issue count
|
||||
- Typical case (small trees): <10ms overhead
|
||||
- Pathological case (deep chains): O(depth × branching) but limited by depth=100
|
||||
|
||||
**User Experience**:
|
||||
|
||||
- Clear error messages: "cannot add dependency: would create a cycle (bd-3 → bd-1 → bd-2 → bd-3)"
|
||||
- After successful addition, we run `DetectCycles()` and warn if any cycles exist elsewhere
|
||||
- `bd dep cycles` command for manual cycle detection and diagnosis
|
||||
|
||||
**Trade-offs**:
|
||||
|
||||
- ✅ Prevents semantic confusion and broken ready work calculation
|
||||
- ✅ Keeps code simple (no cycle handling in traversals)
|
||||
- ⚠️ Small performance overhead on every dependency addition
|
||||
- ⚠️ Cannot represent certain real-world patterns (mutual blockers must be modeled differently)
|
||||
|
||||
**Alternative Considered**: Allow cycles and handle during traversal with cycle detection and path tracking. Rejected because it adds complexity everywhere dependencies are used and doesn't solve the semantic ambiguity problem.
|
||||
|
||||
### Labels
|
||||
|
||||
```go
|
||||
|
||||
32
README.md
32
README.md
@@ -408,6 +408,38 @@ bd dep tree bd-2
|
||||
bd dep cycles
|
||||
```
|
||||
|
||||
#### Dependency Types
|
||||
|
||||
- **blocks**: Hard blocker (default) - issue cannot start until blocker is resolved
|
||||
- **related**: Soft relationship - issues are connected but not blocking
|
||||
- **parent-child**: Hierarchical relationship (child depends on parent)
|
||||
- Correct: `bd dep add bd-task bd-epic --type parent-child` (task → epic)
|
||||
- Wrong: `bd dep add bd-epic bd-task --type parent-child` (reversed!)
|
||||
- **discovered-from**: Issue discovered during work on another issue
|
||||
|
||||
#### Cycle Prevention
|
||||
|
||||
beads maintains a directed acyclic graph (DAG) of dependencies and prevents cycles across **all** dependency types. This ensures:
|
||||
|
||||
- **Ready work is accurate**: Cycles can hide issues from `bd ready` by making them appear blocked when they're actually part of a circular dependency
|
||||
- **Dependencies are clear**: Circular dependencies are semantically confusing (if A depends on B and B depends on A, which should be done first?)
|
||||
- **Traversals work correctly**: Commands like `bd dep tree` rely on DAG structure
|
||||
|
||||
**Example - Prevented Cycle:**
|
||||
```bash
|
||||
bd dep add bd-1 bd-2 # bd-1 blocks on bd-2 ✓
|
||||
bd dep add bd-2 bd-3 # bd-2 blocks on bd-3 ✓
|
||||
bd dep add bd-3 bd-1 # ERROR: would create cycle bd-3 → bd-1 → bd-2 → bd-3 ✗
|
||||
```
|
||||
|
||||
Cross-type cycles are also prevented:
|
||||
```bash
|
||||
bd dep add bd-1 bd-2 --type blocks # bd-1 blocks on bd-2 ✓
|
||||
bd dep add bd-2 bd-1 --type parent-child # ERROR: would create cycle ✗
|
||||
```
|
||||
|
||||
If you try to add a dependency that creates a cycle, you'll get a clear error message. After successfully adding dependencies, beads will warn you if any cycles are detected elsewhere in the graph.
|
||||
|
||||
### Finding Work
|
||||
|
||||
```bash
|
||||
|
||||
@@ -31,26 +31,51 @@ var depAddCmd = &cobra.Command{
|
||||
|
||||
ctx := context.Background()
|
||||
if err := store.AddDependency(ctx, dep, actor); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
|
||||
// Schedule auto-flush
|
||||
markDirtyAndScheduleFlush()
|
||||
|
||||
if jsonOutput {
|
||||
outputJSON(map[string]interface{}{
|
||||
"status": "added",
|
||||
"issue_id": args[0],
|
||||
"depends_on_id": args[1],
|
||||
"type": depType,
|
||||
})
|
||||
return
|
||||
// Check for cycles after adding dependency
|
||||
cycles, err := store.DetectCycles(ctx)
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: Failed to check for cycles: %v\n", err)
|
||||
} else if len(cycles) > 0 {
|
||||
yellow := color.New(color.FgYellow).SprintFunc()
|
||||
fmt.Fprintf(os.Stderr, "\n%s Warning: Dependency cycle detected!\n", yellow("⚠"))
|
||||
fmt.Fprintf(os.Stderr, "This can hide issues from the ready work list and cause confusion.\n\n")
|
||||
fmt.Fprintf(os.Stderr, "Cycle path:\n")
|
||||
for _, cycle := range cycles {
|
||||
for j, issue := range cycle {
|
||||
if j == 0 {
|
||||
fmt.Fprintf(os.Stderr, " %s", issue.ID)
|
||||
} else {
|
||||
fmt.Fprintf(os.Stderr, " → %s", issue.ID)
|
||||
}
|
||||
}
|
||||
if len(cycle) > 0 {
|
||||
fmt.Fprintf(os.Stderr, " → %s", cycle[0].ID)
|
||||
}
|
||||
fmt.Fprintf(os.Stderr, "\n")
|
||||
}
|
||||
fmt.Fprintf(os.Stderr, "\nRun 'bd dep cycles' for detailed analysis.\n\n")
|
||||
}
|
||||
|
||||
green := color.New(color.FgGreen).SprintFunc()
|
||||
fmt.Printf("%s Added dependency: %s depends on %s (%s)\n",
|
||||
green("✓"), args[0], args[1], depType)
|
||||
if jsonOutput {
|
||||
outputJSON(map[string]interface{}{
|
||||
"status": "added",
|
||||
"issue_id": args[0],
|
||||
"depends_on_id": args[1],
|
||||
"type": depType,
|
||||
})
|
||||
return
|
||||
}
|
||||
|
||||
green := color.New(color.FgGreen).SprintFunc()
|
||||
fmt.Printf("%s Added dependency: %s depends on %s (%s)\n",
|
||||
green("✓"), args[0], args[1], depType)
|
||||
},
|
||||
}
|
||||
|
||||
|
||||
@@ -46,6 +46,21 @@ func (s *SQLiteStorage) AddDependency(ctx context.Context, dep *types.Dependency
|
||||
return fmt.Errorf("issue cannot depend on itself")
|
||||
}
|
||||
|
||||
// Validate parent-child dependency direction
|
||||
// In parent-child relationships: child depends on parent (child is part of parent)
|
||||
// Parent should NOT depend on child (semantically backwards)
|
||||
// Consistent with dependency semantics: IssueID depends on DependsOnID
|
||||
if dep.Type == types.DepParentChild {
|
||||
// issueExists is the dependent (the one that depends on something)
|
||||
// dependsOnExists is what it depends on
|
||||
// Correct: Task (child) depends on Epic (parent) - child belongs to parent
|
||||
// Incorrect: Epic (parent) depends on Task (child) - backwards
|
||||
if issueExists.IssueType == types.TypeEpic && dependsOnExists.IssueType != types.TypeEpic {
|
||||
return fmt.Errorf("invalid parent-child dependency: parent (%s) cannot depend on child (%s). Use: bd dep add %s %s --type parent-child",
|
||||
dep.IssueID, dep.DependsOnID, dep.DependsOnID, dep.IssueID)
|
||||
}
|
||||
}
|
||||
|
||||
dep.CreatedAt = time.Now()
|
||||
dep.CreatedBy = actor
|
||||
|
||||
@@ -55,12 +70,27 @@ func (s *SQLiteStorage) AddDependency(ctx context.Context, dep *types.Dependency
|
||||
}
|
||||
defer tx.Rollback()
|
||||
|
||||
// Check if this would create a cycle across ALL dependency types
|
||||
// We check before inserting to avoid unnecessary write on failure
|
||||
// We traverse all dependency types to detect cross-type cycles
|
||||
// (e.g., A blocks B, B parent-child A would create a cycle)
|
||||
// We need to check if we can reach IssueID from DependsOnID
|
||||
// If yes, adding "IssueID depends on DependsOnID" would create a cycle
|
||||
// Cycle Detection and Prevention
|
||||
//
|
||||
// We prevent cycles across ALL dependency types (blocks, related, parent-child, discovered-from)
|
||||
// to maintain a directed acyclic graph (DAG). This is critical for:
|
||||
//
|
||||
// 1. Ready Work Calculation: Cycles can hide issues from the ready list by making them
|
||||
// appear blocked when they're actually part of a circular dependency.
|
||||
//
|
||||
// 2. Dependency Traversal: Operations like dep tree and blocking propagation rely on
|
||||
// DAG structure. Cycles would require special handling and could cause confusion.
|
||||
//
|
||||
// 3. Semantic Clarity: Circular dependencies are conceptually problematic - if A depends
|
||||
// on B and B depends on A (directly or through other issues), which should be done first?
|
||||
//
|
||||
// Implementation: We use a recursive CTE to traverse from DependsOnID to see if we can
|
||||
// reach IssueID. If yes, adding "IssueID depends on DependsOnID" would complete a cycle.
|
||||
// We check ALL dependency types because cross-type cycles (e.g., A blocks B, B parent-child A)
|
||||
// are just as problematic as single-type cycles.
|
||||
//
|
||||
// The traversal is depth-limited to maxDependencyDepth (100) to prevent infinite loops
|
||||
// and excessive query cost. We check before inserting to avoid unnecessary write on failure.
|
||||
var cycleExists bool
|
||||
err = tx.QueryRowContext(ctx, `
|
||||
WITH RECURSIVE paths AS (
|
||||
|
||||
@@ -59,6 +59,58 @@ func TestAddDependencyDiscoveredFrom(t *testing.T) {
|
||||
testAddDependencyWithType(t, types.DepDiscoveredFrom, "Parent task", "Bug found during work")
|
||||
}
|
||||
|
||||
func TestParentChildValidation(t *testing.T) {
|
||||
store, cleanup := setupTestDB(t)
|
||||
defer cleanup()
|
||||
|
||||
ctx := context.Background()
|
||||
|
||||
// Create an epic (parent) and a task (child)
|
||||
epic := &types.Issue{Title: "Epic Feature", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeEpic}
|
||||
task := &types.Issue{Title: "Subtask", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask}
|
||||
|
||||
store.CreateIssue(ctx, epic, "test-user")
|
||||
store.CreateIssue(ctx, task, "test-user")
|
||||
|
||||
// Test 1: Valid direction - Task depends on Epic (child belongs to parent)
|
||||
err := store.AddDependency(ctx, &types.Dependency{
|
||||
IssueID: task.ID,
|
||||
DependsOnID: epic.ID,
|
||||
Type: types.DepParentChild,
|
||||
}, "test-user")
|
||||
if err != nil {
|
||||
t.Fatalf("Valid parent-child dependency failed: %v", err)
|
||||
}
|
||||
|
||||
// Verify it was added
|
||||
deps, err := store.GetDependencies(ctx, task.ID)
|
||||
if err != nil {
|
||||
t.Fatalf("GetDependencies failed: %v", err)
|
||||
}
|
||||
if len(deps) != 1 {
|
||||
t.Fatalf("Expected 1 dependency, got %d", len(deps))
|
||||
}
|
||||
|
||||
// Remove the dependency for next test
|
||||
err = store.RemoveDependency(ctx, task.ID, epic.ID, "test-user")
|
||||
if err != nil {
|
||||
t.Fatalf("RemoveDependency failed: %v", err)
|
||||
}
|
||||
|
||||
// Test 2: Invalid direction - Epic depends on Task (parent depends on child - backwards!)
|
||||
err = store.AddDependency(ctx, &types.Dependency{
|
||||
IssueID: epic.ID,
|
||||
DependsOnID: task.ID,
|
||||
Type: types.DepParentChild,
|
||||
}, "test-user")
|
||||
if err == nil {
|
||||
t.Fatal("Expected error when parent depends on child, but got none")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "child") || !strings.Contains(err.Error(), "parent") {
|
||||
t.Errorf("Expected error message to mention child/parent relationship, got: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRemoveDependency(t *testing.T) {
|
||||
store, cleanup := setupTestDB(t)
|
||||
defer cleanup()
|
||||
|
||||
Reference in New Issue
Block a user