fix: prevent closing issues with open blockers (GH#962)
Added IsBlocked method to Storage interface that checks if an issue is in the blocked_issues_cache and returns the blocking issue IDs. The close command now checks for blockers before allowing an issue to be closed: - If an issue has open blockers, closing is blocked with an error message - The --force flag overrides this check - Works in both daemon mode (RPC) and direct storage mode - Also handles cross-rig routed IDs This addresses the bug where agents could close a bead even when it depends on an open bug/issue. Closes #962 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -122,6 +122,7 @@ create, update, show, or close operation).`,
|
|||||||
Reason: reason,
|
Reason: reason,
|
||||||
Session: session,
|
Session: session,
|
||||||
SuggestNext: suggestNext,
|
SuggestNext: suggestNext,
|
||||||
|
Force: force,
|
||||||
}
|
}
|
||||||
resp, err := daemonClient.CloseIssue(closeArgs)
|
resp, err := daemonClient.CloseIssue(closeArgs)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
@@ -191,6 +192,21 @@ create, update, show, or close operation).`,
|
|||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Check if issue has open blockers (GH#962)
|
||||||
|
if !force {
|
||||||
|
blocked, blockers, err := result.Store.IsBlocked(ctx, result.ResolvedID)
|
||||||
|
if err != nil {
|
||||||
|
result.Close()
|
||||||
|
fmt.Fprintf(os.Stderr, "Error checking blockers for %s: %v\n", id, err)
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
if blocked && len(blockers) > 0 {
|
||||||
|
result.Close()
|
||||||
|
fmt.Fprintf(os.Stderr, "cannot close %s: blocked by open issues %v (use --force to override)\n", id, blockers)
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
if err := result.Store.CloseIssue(ctx, result.ResolvedID, reason, actor, session); err != nil {
|
if err := result.Store.CloseIssue(ctx, result.ResolvedID, reason, actor, session); err != nil {
|
||||||
result.Close()
|
result.Close()
|
||||||
fmt.Fprintf(os.Stderr, "Error closing %s: %v\n", id, err)
|
fmt.Fprintf(os.Stderr, "Error closing %s: %v\n", id, err)
|
||||||
@@ -240,6 +256,19 @@ create, update, show, or close operation).`,
|
|||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Check if issue has open blockers (GH#962)
|
||||||
|
if !force {
|
||||||
|
blocked, blockers, err := store.IsBlocked(ctx, id)
|
||||||
|
if err != nil {
|
||||||
|
fmt.Fprintf(os.Stderr, "Error checking blockers for %s: %v\n", id, err)
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
if blocked && len(blockers) > 0 {
|
||||||
|
fmt.Fprintf(os.Stderr, "cannot close %s: blocked by open issues %v (use --force to override)\n", id, blockers)
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
if err := store.CloseIssue(ctx, id, reason, actor, session); err != nil {
|
if err := store.CloseIssue(ctx, id, reason, actor, session); err != nil {
|
||||||
fmt.Fprintf(os.Stderr, "Error closing %s: %v\n", id, err)
|
fmt.Fprintf(os.Stderr, "Error closing %s: %v\n", id, err)
|
||||||
continue
|
continue
|
||||||
@@ -283,6 +312,21 @@ create, update, show, or close operation).`,
|
|||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Check if issue has open blockers (GH#962)
|
||||||
|
if !force {
|
||||||
|
blocked, blockers, err := result.Store.IsBlocked(ctx, result.ResolvedID)
|
||||||
|
if err != nil {
|
||||||
|
result.Close()
|
||||||
|
fmt.Fprintf(os.Stderr, "Error checking blockers for %s: %v\n", id, err)
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
if blocked && len(blockers) > 0 {
|
||||||
|
result.Close()
|
||||||
|
fmt.Fprintf(os.Stderr, "cannot close %s: blocked by open issues %v (use --force to override)\n", id, blockers)
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
if err := result.Store.CloseIssue(ctx, result.ResolvedID, reason, actor, session); err != nil {
|
if err := result.Store.CloseIssue(ctx, result.ResolvedID, reason, actor, session); err != nil {
|
||||||
result.Close()
|
result.Close()
|
||||||
fmt.Fprintf(os.Stderr, "Error closing %s: %v\n", id, err)
|
fmt.Fprintf(os.Stderr, "Error closing %s: %v\n", id, err)
|
||||||
|
|||||||
@@ -174,6 +174,7 @@ type CloseArgs struct {
|
|||||||
Reason string `json:"reason,omitempty"`
|
Reason string `json:"reason,omitempty"`
|
||||||
Session string `json:"session,omitempty"` // Claude Code session ID that closed this issue
|
Session string `json:"session,omitempty"` // Claude Code session ID that closed this issue
|
||||||
SuggestNext bool `json:"suggest_next,omitempty"` // Return newly unblocked issues (GH#679)
|
SuggestNext bool `json:"suggest_next,omitempty"` // Return newly unblocked issues (GH#679)
|
||||||
|
Force bool `json:"force,omitempty"` // Force close even with open blockers (GH#962)
|
||||||
}
|
}
|
||||||
|
|
||||||
// CloseResult is returned when SuggestNext is true (GH#679)
|
// CloseResult is returned when SuggestNext is true (GH#679)
|
||||||
|
|||||||
@@ -831,6 +831,23 @@ func (s *Server) handleClose(req *Request) Response {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Check if issue has open blockers (GH#962)
|
||||||
|
if !closeArgs.Force {
|
||||||
|
blocked, blockers, err := store.IsBlocked(ctx, closeArgs.ID)
|
||||||
|
if err != nil {
|
||||||
|
return Response{
|
||||||
|
Success: false,
|
||||||
|
Error: fmt.Sprintf("failed to check blockers: %v", err),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if blocked && len(blockers) > 0 {
|
||||||
|
return Response{
|
||||||
|
Success: false,
|
||||||
|
Error: fmt.Sprintf("cannot close %s: blocked by open issues %v (use --force to override)", closeArgs.ID, blockers),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// Capture old status for rich mutation event
|
// Capture old status for rich mutation event
|
||||||
oldStatus := ""
|
oldStatus := ""
|
||||||
if issue != nil {
|
if issue != nil {
|
||||||
|
|||||||
@@ -1281,6 +1281,19 @@ func (m *MemoryStorage) GetBlockedIssues(ctx context.Context, filter types.WorkF
|
|||||||
return results, nil
|
return results, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// IsBlocked checks if an issue is blocked by open dependencies (GH#962).
|
||||||
|
// Returns true if the issue has open blockers, along with the list of blocker IDs.
|
||||||
|
func (m *MemoryStorage) IsBlocked(ctx context.Context, issueID string) (bool, []string, error) {
|
||||||
|
m.mu.RLock()
|
||||||
|
defer m.mu.RUnlock()
|
||||||
|
|
||||||
|
blockers := m.getOpenBlockers(issueID)
|
||||||
|
if len(blockers) == 0 {
|
||||||
|
return false, nil, nil
|
||||||
|
}
|
||||||
|
return true, blockers, nil
|
||||||
|
}
|
||||||
|
|
||||||
// getAllDescendants returns all descendant IDs of a parent issue recursively
|
// getAllDescendants returns all descendant IDs of a parent issue recursively
|
||||||
func (m *MemoryStorage) getAllDescendants(parentID string) map[string]bool {
|
func (m *MemoryStorage) getAllDescendants(parentID string) map[string]bool {
|
||||||
descendants := make(map[string]bool)
|
descendants := make(map[string]bool)
|
||||||
|
|||||||
@@ -680,6 +680,52 @@ func filterBlockedByExternalDeps(ctx context.Context, blocked []*types.BlockedIs
|
|||||||
return result
|
return result
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// IsBlocked checks if an issue is blocked by open dependencies (GH#962).
|
||||||
|
// Returns true if the issue is in the blocked_issues_cache, along with a list
|
||||||
|
// of issue IDs that are blocking it.
|
||||||
|
// This is used to prevent closing issues that still have open blockers.
|
||||||
|
func (s *SQLiteStorage) IsBlocked(ctx context.Context, issueID string) (bool, []string, error) {
|
||||||
|
// First check if the issue is in the blocked cache
|
||||||
|
var inCache bool
|
||||||
|
err := s.db.QueryRowContext(ctx, `
|
||||||
|
SELECT EXISTS(SELECT 1 FROM blocked_issues_cache WHERE issue_id = ?)
|
||||||
|
`, issueID).Scan(&inCache)
|
||||||
|
if err != nil {
|
||||||
|
return false, nil, fmt.Errorf("failed to check blocked status: %w", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
if !inCache {
|
||||||
|
return false, nil, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// Get the blocking issue IDs
|
||||||
|
// We query dependencies for 'blocks' type where the blocker is still open
|
||||||
|
rows, err := s.db.QueryContext(ctx, `
|
||||||
|
SELECT d.depends_on_id
|
||||||
|
FROM dependencies d
|
||||||
|
JOIN issues blocker ON d.depends_on_id = blocker.id
|
||||||
|
WHERE d.issue_id = ?
|
||||||
|
AND d.type = 'blocks'
|
||||||
|
AND blocker.status IN ('open', 'in_progress', 'blocked', 'deferred', 'hooked')
|
||||||
|
ORDER BY blocker.priority ASC
|
||||||
|
`, issueID)
|
||||||
|
if err != nil {
|
||||||
|
return true, nil, fmt.Errorf("failed to get blockers: %w", err)
|
||||||
|
}
|
||||||
|
defer func() { _ = rows.Close() }()
|
||||||
|
|
||||||
|
var blockers []string
|
||||||
|
for rows.Next() {
|
||||||
|
var blockerID string
|
||||||
|
if err := rows.Scan(&blockerID); err != nil {
|
||||||
|
return true, nil, fmt.Errorf("failed to scan blocker ID: %w", err)
|
||||||
|
}
|
||||||
|
blockers = append(blockers, blockerID)
|
||||||
|
}
|
||||||
|
|
||||||
|
return true, blockers, rows.Err()
|
||||||
|
}
|
||||||
|
|
||||||
// GetNewlyUnblockedByClose returns issues that became unblocked when the given issue was closed.
|
// GetNewlyUnblockedByClose returns issues that became unblocked when the given issue was closed.
|
||||||
// This is used by the --suggest-next flag on bd close to show what work is now available.
|
// This is used by the --suggest-next flag on bd close to show what work is now available.
|
||||||
// An issue is "newly unblocked" if:
|
// An issue is "newly unblocked" if:
|
||||||
|
|||||||
@@ -1855,3 +1855,53 @@ func TestParentIDEmptyParent(t *testing.T) {
|
|||||||
t.Fatalf("Expected 0 ready issues for empty parent, got %d", len(ready))
|
t.Fatalf("Expected 0 ready issues for empty parent, got %d", len(ready))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestIsBlocked tests the IsBlocked method (GH#962)
|
||||||
|
func TestIsBlocked(t *testing.T) {
|
||||||
|
env := newTestEnv(t)
|
||||||
|
ctx := context.Background()
|
||||||
|
|
||||||
|
// Create issues:
|
||||||
|
// issue1: open, no dependencies → NOT BLOCKED
|
||||||
|
// issue2: open, depends on issue1 (open) → BLOCKED by issue1
|
||||||
|
// issue3: open, depends on issue4 (closed) → NOT BLOCKED (blocker is closed)
|
||||||
|
issue1 := env.CreateIssue("Open No Deps")
|
||||||
|
issue2 := env.CreateIssue("Blocked by open")
|
||||||
|
issue3 := env.CreateIssue("Blocked by closed")
|
||||||
|
issue4 := env.CreateIssue("Will be closed")
|
||||||
|
|
||||||
|
env.AddDep(issue2, issue1) // issue2 depends on issue1 (open)
|
||||||
|
env.AddDep(issue3, issue4) // issue3 depends on issue4
|
||||||
|
|
||||||
|
env.Close(issue4, "Done") // Close issue4
|
||||||
|
|
||||||
|
// Test issue1: not blocked
|
||||||
|
blocked, blockers, err := env.Store.IsBlocked(ctx, issue1.ID)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("IsBlocked failed: %v", err)
|
||||||
|
}
|
||||||
|
if blocked {
|
||||||
|
t.Errorf("Expected issue1 to NOT be blocked, got blocked=true with blockers=%v", blockers)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Test issue2: blocked by issue1
|
||||||
|
blocked, blockers, err = env.Store.IsBlocked(ctx, issue2.ID)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("IsBlocked failed: %v", err)
|
||||||
|
}
|
||||||
|
if !blocked {
|
||||||
|
t.Error("Expected issue2 to be blocked")
|
||||||
|
}
|
||||||
|
if len(blockers) != 1 || blockers[0] != issue1.ID {
|
||||||
|
t.Errorf("Expected blockers=[%s], got %v", issue1.ID, blockers)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Test issue3: not blocked (issue4 is closed)
|
||||||
|
blocked, blockers, err = env.Store.IsBlocked(ctx, issue3.ID)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("IsBlocked failed: %v", err)
|
||||||
|
}
|
||||||
|
if blocked {
|
||||||
|
t.Errorf("Expected issue3 to NOT be blocked (blocker is closed), got blocked=true with blockers=%v", blockers)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -110,6 +110,7 @@ type Storage interface {
|
|||||||
// Ready Work & Blocking
|
// Ready Work & Blocking
|
||||||
GetReadyWork(ctx context.Context, filter types.WorkFilter) ([]*types.Issue, error)
|
GetReadyWork(ctx context.Context, filter types.WorkFilter) ([]*types.Issue, error)
|
||||||
GetBlockedIssues(ctx context.Context, filter types.WorkFilter) ([]*types.BlockedIssue, error)
|
GetBlockedIssues(ctx context.Context, filter types.WorkFilter) ([]*types.BlockedIssue, error)
|
||||||
|
IsBlocked(ctx context.Context, issueID string) (bool, []string, error) // GH#962: Check if issue has open blockers
|
||||||
GetEpicsEligibleForClosure(ctx context.Context) ([]*types.EpicStatus, error)
|
GetEpicsEligibleForClosure(ctx context.Context) ([]*types.EpicStatus, error)
|
||||||
GetStaleIssues(ctx context.Context, filter types.StaleFilter) ([]*types.Issue, error)
|
GetStaleIssues(ctx context.Context, filter types.StaleFilter) ([]*types.Issue, error)
|
||||||
GetNewlyUnblockedByClose(ctx context.Context, closedIssueID string) ([]*types.Issue, error) // GH#679
|
GetNewlyUnblockedByClose(ctx context.Context, closedIssueID string) ([]*types.Issue, error) // GH#679
|
||||||
|
|||||||
@@ -98,6 +98,9 @@ func (m *mockStorage) GetReadyWork(ctx context.Context, filter types.WorkFilter)
|
|||||||
func (m *mockStorage) GetBlockedIssues(ctx context.Context, filter types.WorkFilter) ([]*types.BlockedIssue, error) {
|
func (m *mockStorage) GetBlockedIssues(ctx context.Context, filter types.WorkFilter) ([]*types.BlockedIssue, error) {
|
||||||
return nil, nil
|
return nil, nil
|
||||||
}
|
}
|
||||||
|
func (m *mockStorage) IsBlocked(ctx context.Context, issueID string) (bool, []string, error) {
|
||||||
|
return false, nil, nil
|
||||||
|
}
|
||||||
func (m *mockStorage) GetEpicsEligibleForClosure(ctx context.Context) ([]*types.EpicStatus, error) {
|
func (m *mockStorage) GetEpicsEligibleForClosure(ctx context.Context) ([]*types.EpicStatus, error) {
|
||||||
return nil, nil
|
return nil, nil
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user