From 72242451815737865228e6b82d3194afa9b0c9f8 Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 24 Mar 2026 11:16:03 -0700 Subject: [PATCH 01/18] Add retry logic to Jira client and jitter to all tracker HTTP clients - Add retry with exponential backoff to Jira doRequest (was single-attempt) - Add MaxRetries, RetryDelay, MaxResponseSize constants to jira/types.go - Add response body size limit via io.LimitReader to Jira client - Handle permanent errors (400/401/403/404) without retry - Parse Retry-After header for rate-limited responses - Add jitter (0-50% of delay) to backoff in all 5 tracker clients: GitHub, GitLab, Linear, ADO, and Jira - Guard rand.Int64N against zero delay to prevent panics - Add retry tests: 429, 503, no-retry-on-400, max-retries-exceeded Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/ado/client.go | 7 +++ internal/github/client.go | 5 ++ internal/gitlab/client.go | 4 ++ internal/jira/client.go | 99 ++++++++++++++++++++++-------- internal/jira/retry_test.go | 117 ++++++++++++++++++++++++++++++++++++ internal/jira/types.go | 14 +++++ internal/linear/client.go | 4 ++ 7 files changed, 225 insertions(+), 25 deletions(-) create mode 100644 internal/jira/retry_test.go diff --git a/internal/ado/client.go b/internal/ado/client.go index e0f2decd2d..d2fbd171f5 100644 --- a/internal/ado/client.go +++ b/internal/ado/client.go @@ -7,6 +7,7 @@ import ( "encoding/json" "fmt" "io" + "math/rand/v2" "net/http" "net/url" "regexp" @@ -207,6 +208,9 @@ func (c *Client) doRequest(ctx context.Context, method, urlStr, contentType stri lastErr = fmt.Errorf("request failed (attempt %d/%d): %w", attempt+1, maxAttempts+1, err) if attempt < maxAttempts { delay := RetryDelay * time.Duration(1< 0 { + delay += time.Duration(rand.Int64N(half)) + } select { case <-ctx.Done(): return nil, ctx.Err() @@ -244,6 +248,9 @@ func (c *Client) doRequest(ctx context.Context, method, urlStr, contentType stri delay = time.Duration(seconds) * time.Second } } + if half := int64(delay / 2); half > 0 { + delay += time.Duration(rand.Int64N(half)) + } lastErr = fmt.Errorf("transient error %d (attempt %d/%d)", resp.StatusCode, attempt+1, maxAttempts+1) select { case <-ctx.Done(): diff --git a/internal/github/client.go b/internal/github/client.go index eee391d782..ff3218dbe9 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -7,6 +7,7 @@ import ( "encoding/json" "fmt" "io" + "math/rand/v2" "net/http" "regexp" "strconv" @@ -121,6 +122,10 @@ func (c *Client) doRequest(ctx context.Context, method, urlStr string, body inte } } + if half := int64(delay / 2); half > 0 { + delay += time.Duration(rand.Int64N(half)) + } + lastErr = fmt.Errorf("transient error %d (attempt %d/%d)", resp.StatusCode, attempt+1, MaxRetries+1) select { case <-ctx.Done(): diff --git a/internal/gitlab/client.go b/internal/gitlab/client.go index 91bf727caa..d587622396 100644 --- a/internal/gitlab/client.go +++ b/internal/gitlab/client.go @@ -7,6 +7,7 @@ import ( "encoding/json" "fmt" "io" + "math/rand/v2" "net/http" "net/url" "strconv" @@ -117,6 +118,9 @@ func (c *Client) doRequest(ctx context.Context, method, urlStr string, body inte if retriable { delay := RetryDelay * time.Duration(1< 0 { + delay += time.Duration(rand.Int64N(half)) + } lastErr = fmt.Errorf("transient error %d (attempt %d/%d)", resp.StatusCode, attempt+1, MaxRetries+1) select { case <-ctx.Done(): diff --git a/internal/jira/client.go b/internal/jira/client.go index 6845b06f78..fb8b5379fd 100644 --- a/internal/jira/client.go +++ b/internal/jira/client.go @@ -7,8 +7,10 @@ import ( "encoding/json" "fmt" "io" + "math/rand/v2" "net/http" "net/url" + "strconv" "strings" "time" @@ -317,39 +319,86 @@ func (c *Client) doRequest(ctx context.Context, method, apiURL string, body []by bodyReader = bytes.NewReader(body) } - req, err := http.NewRequestWithContext(ctx, method, apiURL, bodyReader) - if err != nil { - return nil, fmt.Errorf("create request: %w", err) - } + var lastErr error + for attempt := 0; attempt <= MaxRetries; attempt++ { + req, err := http.NewRequestWithContext(ctx, method, apiURL, bodyReader) + if err != nil { + return nil, fmt.Errorf("create request: %w", err) + } - c.setAuth(req) - req.Header.Set("Accept", "application/json") - req.Header.Set("User-Agent", "bd-jira-sync/1.0") - if body != nil { - req.Header.Set("Content-Type", "application/json") - } + c.setAuth(req) + req.Header.Set("Accept", "application/json") + req.Header.Set("User-Agent", "bd-jira-sync/1.0") + if body != nil { + req.Header.Set("Content-Type", "application/json") + } - resp, err := c.HTTPClient.Do(req) - if err != nil { - return nil, err - } - defer func() { _ = resp.Body.Close() }() + resp, err := c.HTTPClient.Do(req) + if err != nil { + lastErr = fmt.Errorf("request failed (attempt %d/%d): %w", attempt+1, MaxRetries+1, err) + continue + } - respBody, err := io.ReadAll(resp.Body) - if err != nil { - return nil, fmt.Errorf("read response: %w", err) - } + respBody, err := io.ReadAll(io.LimitReader(resp.Body, MaxResponseSize)) + _ = resp.Body.Close() + if err != nil { + lastErr = fmt.Errorf("failed to read response (attempt %d/%d): %w", attempt+1, MaxRetries+1, err) + continue + } - // PUT returns 204 No Content on success - if resp.StatusCode == http.StatusNoContent { - return nil, nil - } + // PUT returns 204 No Content on success + if resp.StatusCode == http.StatusNoContent { + return nil, nil + } + + if resp.StatusCode >= 200 && resp.StatusCode < 300 { + return respBody, nil + } + + // Permanent failures — no retry. + switch resp.StatusCode { + case http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden, http.StatusNotFound: + return nil, fmt.Errorf("jira API returned %d: %s", resp.StatusCode, string(respBody)) + } + + // Retry on rate-limiting and server errors with exponential backoff. + retriable := resp.StatusCode == http.StatusTooManyRequests || + resp.StatusCode == http.StatusInternalServerError || + resp.StatusCode == http.StatusBadGateway || + resp.StatusCode == http.StatusServiceUnavailable || + resp.StatusCode == http.StatusGatewayTimeout + + if retriable { + delay := RetryDelay * time.Duration(1< 0 { + delay += time.Duration(rand.Int64N(half)) + } + + lastErr = fmt.Errorf("transient error %d (attempt %d/%d)", resp.StatusCode, attempt+1, MaxRetries+1) + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(delay): + // Reset body reader for retry + if body != nil { + bodyReader = bytes.NewReader(body) + } + continue + } + } - if resp.StatusCode < 200 || resp.StatusCode >= 300 { return nil, fmt.Errorf("jira API returned %d: %s", resp.StatusCode, string(respBody)) } - return respBody, nil + return nil, fmt.Errorf("max retries (%d) exceeded: %w", MaxRetries+1, lastErr) } // setAuth sets the appropriate authentication header on the request. diff --git a/internal/jira/retry_test.go b/internal/jira/retry_test.go new file mode 100644 index 0000000000..853e48f7f9 --- /dev/null +++ b/internal/jira/retry_test.go @@ -0,0 +1,117 @@ +package jira + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" +) + +func TestDoRequest_RetryOn429(t *testing.T) { + var attempts int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := atomic.AddInt32(&attempts, 1) + if n <= 2 { + w.Header().Set("Retry-After", "0") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = w.Write([]byte(`{"message":"rate limited"}`)) + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(Issue{ID: "1", Key: "PROJ-1"}) + })) + defer srv.Close() + + c := newTestClient(srv.URL, "3") + body, err := c.doRequest(context.Background(), "GET", srv.URL+"/rest/api/3/issue/PROJ-1", nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if atomic.LoadInt32(&attempts) < 3 { + t.Errorf("expected at least 3 attempts for 429 retry, got %d", atomic.LoadInt32(&attempts)) + } + if !strings.Contains(string(body), "PROJ-1") { + t.Errorf("unexpected body: %s", body) + } +} + +func TestDoRequest_RetryOn503(t *testing.T) { + var attempts int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := atomic.AddInt32(&attempts, 1) + if n == 1 { + w.WriteHeader(http.StatusServiceUnavailable) + _, _ = w.Write([]byte(`{"message":"service unavailable"}`)) + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(Issue{ID: "1", Key: "PROJ-1"}) + })) + defer srv.Close() + + c := newTestClient(srv.URL, "3") + body, err := c.doRequest(context.Background(), "GET", srv.URL+"/rest/api/3/issue/PROJ-1", nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if atomic.LoadInt32(&attempts) < 2 { + t.Errorf("expected at least 2 attempts for 503 retry, got %d", atomic.LoadInt32(&attempts)) + } + if !strings.Contains(string(body), "PROJ-1") { + t.Errorf("unexpected body: %s", body) + } +} + +func TestDoRequest_NoRetryOn400(t *testing.T) { + var attempts int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt32(&attempts, 1) + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"message":"bad request"}`)) + })) + defer srv.Close() + + c := newTestClient(srv.URL, "3") + _, err := c.doRequest(context.Background(), "GET", srv.URL+"/rest/api/3/issue/PROJ-1", nil) + if err == nil { + t.Fatal("expected error for 400") + } + if !strings.Contains(err.Error(), "400") { + t.Errorf("error should mention 400: %v", err) + } + if !strings.Contains(err.Error(), "bad request") { + t.Errorf("error should include response body: %v", err) + } + if atomic.LoadInt32(&attempts) != 1 { + t.Errorf("expected exactly 1 attempt for 400, got %d", atomic.LoadInt32(&attempts)) + } +} + +func TestDoRequest_MaxRetriesExceeded(t *testing.T) { + var attempts int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt32(&attempts, 1) + w.Header().Set("Retry-After", "0") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = w.Write([]byte(`{"message":"rate limited"}`)) + })) + defer srv.Close() + + c := newTestClient(srv.URL, "3") + _, err := c.doRequest(context.Background(), "GET", srv.URL+"/rest/api/3/issue/PROJ-1", nil) + if err == nil { + t.Fatal("expected error after max retries") + } + if !strings.Contains(err.Error(), "max retries") { + t.Errorf("error should mention max retries: %v", err) + } + expectedAttempts := int32(MaxRetries + 1) + if atomic.LoadInt32(&attempts) != expectedAttempts { + t.Errorf("expected %d attempts, got %d", expectedAttempts, atomic.LoadInt32(&attempts)) + } +} diff --git a/internal/jira/types.go b/internal/jira/types.go index fedb042ae8..d41b784d73 100644 --- a/internal/jira/types.go +++ b/internal/jira/types.go @@ -1,2 +1,16 @@ // Package jira provides client, types, and utilities for Jira integration. package jira + +import "time" + +// API configuration constants. +const ( + // MaxRetries is the maximum number of retries for transient failures. + MaxRetries = 3 + + // RetryDelay is the base delay between retries (exponential backoff). + RetryDelay = time.Second + + // MaxResponseSize is the maximum response body size (50 MB). + MaxResponseSize = 50 * 1024 * 1024 +) diff --git a/internal/linear/client.go b/internal/linear/client.go index 34fede83e3..bf440dc004 100644 --- a/internal/linear/client.go +++ b/internal/linear/client.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "io" + "math/rand/v2" "net/http" "net/url" "strconv" @@ -183,6 +184,9 @@ func (c *Client) Execute(ctx context.Context, req *GraphQLRequest) (json.RawMess if resp.StatusCode == http.StatusTooManyRequests { delay := RetryDelay * time.Duration(1< 0 { + delay += time.Duration(rand.Int64N(half)) + } lastErr = fmt.Errorf("rate limited (attempt %d/%d), retrying after %v", attempt+1, MaxRetries+1, delay) select { case <-ctx.Done(): From 0d3daf59649f364c83c10de84c7398336bad5841 Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 24 Mar 2026 11:20:02 -0700 Subject: [PATCH 02/18] Add pagination guards to Linear and context checks to Jira client - Linear FetchIssues/FetchIssuesSince: add MaxPages=1000 guard to prevent infinite loops from malformed API responses with hasNextPage=true - Linear: add ctx.Done() checks at start of each pagination iteration - Jira SearchIssues: add ctx.Done() check at start of each pagination iteration - Add MaxPages constant to internal/linear/types.go Fixes: bd-6fy Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/jira/client.go | 6 ++++++ internal/linear/client.go | 22 ++++++++++++++++++++++ internal/linear/types.go | 4 ++++ 3 files changed, 32 insertions(+) diff --git a/internal/jira/client.go b/internal/jira/client.go index fb8b5379fd..4558d02c01 100644 --- a/internal/jira/client.go +++ b/internal/jira/client.go @@ -167,6 +167,12 @@ func (c *Client) SearchIssues(ctx context.Context, jql string) ([]Issue, error) maxResults := 100 for { + select { + case <-ctx.Done(): + return allIssues, ctx.Err() + default: + } + params := url.Values{ "jql": {jql}, "fields": {searchFields}, diff --git a/internal/linear/client.go b/internal/linear/client.go index bf440dc004..23e473254a 100644 --- a/internal/linear/client.go +++ b/internal/linear/client.go @@ -261,7 +261,14 @@ func (c *Client) FetchIssues(ctx context.Context, state string) ([]Issue, error) } } + page := 0 for { + select { + case <-ctx.Done(): + return allIssues, ctx.Err() + default: + } + variables := map[string]interface{}{ "filter": filter, "first": MaxPageSize, @@ -291,6 +298,10 @@ func (c *Client) FetchIssues(ctx context.Context, state string) ([]Issue, error) break } cursor = issuesResp.Issues.PageInfo.EndCursor + page++ + if page >= MaxPages { + return nil, fmt.Errorf("pagination limit exceeded: stopped after %d pages", MaxPages) + } } return allIssues, nil @@ -343,7 +354,14 @@ func (c *Client) FetchIssuesSince(ctx context.Context, state string, since time. } } + page := 0 for { + select { + case <-ctx.Done(): + return allIssues, ctx.Err() + default: + } + variables := map[string]interface{}{ "filter": filter, "first": MaxPageSize, @@ -373,6 +391,10 @@ func (c *Client) FetchIssuesSince(ctx context.Context, state string, since time. break } cursor = issuesResp.Issues.PageInfo.EndCursor + page++ + if page >= MaxPages { + return nil, fmt.Errorf("pagination limit exceeded: stopped after %d pages", MaxPages) + } } return allIssues, nil diff --git a/internal/linear/types.go b/internal/linear/types.go index e0693f5c50..fc03023d1a 100644 --- a/internal/linear/types.go +++ b/internal/linear/types.go @@ -26,6 +26,10 @@ const ( // MaxPageSize is the maximum number of issues to fetch per page. MaxPageSize = 100 + + // MaxPages is the maximum number of pages to fetch before stopping. + // This prevents infinite loops from malformed API responses. + MaxPages = 1000 ) // Client provides methods to interact with the Linear GraphQL API. From 100463cb6f32b94ec8cdf50c7a8e356bc2a76270 Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 24 Mar 2026 11:24:06 -0700 Subject: [PATCH 03/18] Sanitize external tracker content for terminal display and add response size limits - Add ui.SanitizeForTerminal() to strip ANSI escape sequences, OSC sequences, and C0/C1 control characters from external content before terminal display. Preserves printable UTF-8, newlines, and tabs. - Apply sanitization in tracker engine dry-run output (3 call sites) - Add MaxResponseSize (50MB) and io.LimitReader to Linear client to prevent OOM from malformed API responses (was the only tracker without this guard) - Add comprehensive test suite for sanitization (17 test cases including ANSI injection, screen clearing, OSC title change, control characters) Fixes: bd-u9u Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/linear/client.go | 2 +- internal/linear/types.go | 3 + internal/tracker/engine.go | 7 ++- internal/ui/sanitize.go | 114 +++++++++++++++++++++++++++++++++++ internal/ui/sanitize_test.go | 106 ++++++++++++++++++++++++++++++++ 5 files changed, 228 insertions(+), 4 deletions(-) create mode 100644 internal/ui/sanitize.go create mode 100644 internal/ui/sanitize_test.go diff --git a/internal/linear/client.go b/internal/linear/client.go index 23e473254a..5b5440f022 100644 --- a/internal/linear/client.go +++ b/internal/linear/client.go @@ -175,7 +175,7 @@ func (c *Client) Execute(ctx context.Context, req *GraphQLRequest) (json.RawMess continue } - respBody, err := io.ReadAll(resp.Body) + respBody, err := io.ReadAll(io.LimitReader(resp.Body, MaxResponseSize)) _ = resp.Body.Close() // Best effort: HTTP body close; connection may be reused regardless if err != nil { lastErr = fmt.Errorf("failed to read response (attempt %d/%d): %w", attempt+1, MaxRetries+1, err) diff --git a/internal/linear/types.go b/internal/linear/types.go index fc03023d1a..c9f211a364 100644 --- a/internal/linear/types.go +++ b/internal/linear/types.go @@ -30,6 +30,9 @@ const ( // MaxPages is the maximum number of pages to fetch before stopping. // This prevents infinite loops from malformed API responses. MaxPages = 1000 + + // MaxResponseSize is the maximum allowed HTTP response body size (50MB). + MaxResponseSize = 50 * 1024 * 1024 ) // Client provides methods to interact with the Linear GraphQL API. diff --git a/internal/tracker/engine.go b/internal/tracker/engine.go index 5897d270e4..882433adf3 100644 --- a/internal/tracker/engine.go +++ b/internal/tracker/engine.go @@ -13,6 +13,7 @@ import ( "github.com/steveyegge/beads/internal/storage" "github.com/steveyegge/beads/internal/types" + "github.com/steveyegge/beads/internal/ui" ) // syncTracer is the OTel tracer for tracker sync spans. @@ -297,7 +298,7 @@ func (e *Engine) doPull(ctx context.Context, opts SyncOptions) (*PullStats, erro } if opts.DryRun { - e.msg("[dry-run] Would import: %s - %s", extIssue.Identifier, extIssue.Title) + e.msg("[dry-run] Would import: %s - %s", extIssue.Identifier, ui.SanitizeForTerminal(extIssue.Title)) stats.Created++ continue } @@ -440,10 +441,10 @@ func (e *Engine) doPush(ctx context.Context, opts SyncOptions, skipIDs, forceIDs if opts.DryRun { if extRef == "" { - e.msg("[dry-run] Would create in %s: %s", e.Tracker.DisplayName(), issue.Title) + e.msg("[dry-run] Would create in %s: %s", e.Tracker.DisplayName(), ui.SanitizeForTerminal(issue.Title)) stats.Created++ } else { - e.msg("[dry-run] Would update in %s: %s", e.Tracker.DisplayName(), issue.Title) + e.msg("[dry-run] Would update in %s: %s", e.Tracker.DisplayName(), ui.SanitizeForTerminal(issue.Title)) stats.Updated++ } continue diff --git a/internal/ui/sanitize.go b/internal/ui/sanitize.go new file mode 100644 index 0000000000..f040d2a178 --- /dev/null +++ b/internal/ui/sanitize.go @@ -0,0 +1,114 @@ +package ui + +import ( + "strings" + "unicode" +) + +// SanitizeForTerminal removes ANSI escape sequences and control characters +// from a string to prevent terminal injection attacks. External tracker content +// (issue titles, descriptions) should be sanitized before terminal display. +// +// Preserves printable UTF-8 characters including common punctuation and emoji. +// Strips: ANSI CSI sequences (\x1b[...), OSC sequences (\x1b]...\x07), +// other escape sequences, and C0/C1 control characters (except \n and \t). +func SanitizeForTerminal(s string) string { + var b strings.Builder + b.Grow(len(s)) + + i := 0 + for i < len(s) { + ch := s[i] + + // Strip ANSI escape sequences: ESC [ ... final_byte + if ch == '\x1b' && i+1 < len(s) { + next := s[i+1] + if next == '[' { + // CSI sequence: skip until final byte (0x40-0x7E) + j := i + 2 + for j < len(s) && s[j] >= 0x20 && s[j] <= 0x3F { + j++ // skip parameter bytes + } + if j < len(s) && s[j] >= 0x40 && s[j] <= 0x7E { + j++ // skip final byte + } + i = j + continue + } + if next == ']' { + // OSC sequence: skip until BEL (\x07) or ST (\x1b\x5c) + j := i + 2 + for j < len(s) { + if s[j] == '\x07' { + j++ + break + } + if s[j] == '\x1b' && j+1 < len(s) && s[j+1] == '\\' { + j += 2 + break + } + j++ + } + i = j + continue + } + // Other escape: skip ESC + one byte + i += 2 + continue + } + + // Allow newlines and tabs + if ch == '\n' || ch == '\t' { + b.WriteByte(ch) + i++ + continue + } + + // Strip C0 control characters (0x00-0x1F except \n, \t handled above) + if ch < 0x20 { + i++ + continue + } + + // Strip DEL + if ch == 0x7F { + i++ + continue + } + + // For multi-byte UTF-8, decode and check + r := rune(ch) + size := 1 + if ch >= 0x80 { + // Decode UTF-8 rune + r, size = decodeRune(s[i:]) + if r == unicode.ReplacementChar && size == 1 { + // Invalid UTF-8 byte, skip + i++ + continue + } + // Strip C1 control characters (U+0080-U+009F) + if r >= 0x80 && r <= 0x9F { + i += size + continue + } + } + + b.WriteString(s[i : i+size]) + i += size + } + + return b.String() +} + +// decodeRune decodes a single UTF-8 rune from the string. +func decodeRune(s string) (rune, int) { + if len(s) == 0 { + return unicode.ReplacementChar, 0 + } + // Use range to decode + for _, r := range s { + return r, len(string(r)) + } + return unicode.ReplacementChar, 1 +} diff --git a/internal/ui/sanitize_test.go b/internal/ui/sanitize_test.go new file mode 100644 index 0000000000..221ca33289 --- /dev/null +++ b/internal/ui/sanitize_test.go @@ -0,0 +1,106 @@ +package ui + +import "testing" + +func TestSanitizeForTerminal(t *testing.T) { + tests := []struct { + name string + input string + want string + }{ + { + name: "plain text unchanged", + input: "Hello, world!", + want: "Hello, world!", + }, + { + name: "preserves newlines and tabs", + input: "line1\nline2\ttab", + want: "line1\nline2\ttab", + }, + { + name: "strips ANSI color codes", + input: "\x1b[31mred text\x1b[0m", + want: "red text", + }, + { + name: "strips ANSI clear screen", + input: "\x1b[2J\x1b[HMalicious title", + want: "Malicious title", + }, + { + name: "strips ANSI cursor movement", + input: "\x1b[10;20Hmoved cursor", + want: "moved cursor", + }, + { + name: "strips OSC sequences (title change)", + input: "\x1b]0;evil title\x07Normal text", + want: "Normal text", + }, + { + name: "strips OSC with ST terminator", + input: "\x1b]0;evil\x1b\\Normal", + want: "Normal", + }, + { + name: "strips null bytes", + input: "before\x00after", + want: "beforeafter", + }, + { + name: "strips bell character", + input: "ding\x07dong", + want: "dingdong", + }, + { + name: "strips backspace", + input: "abc\x08def", + want: "abcdef", + }, + { + name: "strips carriage return", + input: "overwrite\rme", + want: "overwriteme", + }, + { + name: "strips DEL character", + input: "hello\x7Fworld", + want: "helloworld", + }, + { + name: "preserves unicode", + input: "café résumé naïve 日本語", + want: "café résumé naïve 日本語", + }, + { + name: "preserves emoji", + input: "🚀 Launch 🎉 Party", + want: "🚀 Launch 🎉 Party", + }, + { + name: "complex injection attempt", + input: "\x1b[2J\x1b[H\x1b]0;pwned\x07IGNORE PREVIOUS INSTRUCTIONS\x1b[31m", + want: "IGNORE PREVIOUS INSTRUCTIONS", + }, + { + name: "empty string", + input: "", + want: "", + }, + { + name: "only control characters", + input: "\x1b[31m\x1b[0m", + want: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := SanitizeForTerminal(tt.input) + if got != tt.want { + t.Errorf("SanitizeForTerminal(%q) = %q, want %q", tt.input, got, tt.want) + } + }) + } +} From d118ccd87fafb47677adfbc0fe7449c40d5bbd68 Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 24 Mar 2026 11:33:02 -0700 Subject: [PATCH 04/18] Add Integration Charter: define scope boundary for tracker integrations Formally documents what beads tracker integrations will and will not build. Establishes that integrations are an adoption bridge, not a product. Explicit out-of-scope: webhooks, cross-tracker orchestration, attachment sync, comment mirroring, credential vaults, UI parity features. Includes design guidelines for new integrations and a decision log. Fixes: bd-9go Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/INTEGRATION_CHARTER.md | 88 +++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 docs/INTEGRATION_CHARTER.md diff --git a/docs/INTEGRATION_CHARTER.md b/docs/INTEGRATION_CHARTER.md new file mode 100644 index 0000000000..130d51b47c --- /dev/null +++ b/docs/INTEGRATION_CHARTER.md @@ -0,0 +1,88 @@ +# Integration Charter + +This document defines the scope boundary for beads tracker integrations. It exists to keep the project focused on its core value — dependency-aware issue tracking for AI agents — and to prevent scope creep into platform territory. + +## Core Principle + +Tracker integrations are an **adoption bridge**, not a product. They exist to lower the barrier to entry for teams already using GitHub Issues, Jira, Linear, GitLab, or Azure DevOps. The goal is to make it easy to try beads alongside an existing tracker, not to replace that tracker's UI or workflow. + +## What Beads Will Maintain + +### Bidirectional Sync (Polled) + +- **Issue metadata**: title, status, assignee, priority, labels +- **Dependency relationships**: mapped to beads' native dependency graph +- **Conflict resolution**: deterministic strategies (last-write-wins, beads-wins, tracker-wins) +- **Configurable sync intervals**: polled on user-initiated or scheduled cadence + +### One-Way Import + +- Bulk import from any supported tracker to populate the dependency graph +- Useful for initial adoption or one-time migration + +### Reliable Error Handling + +- Retry with exponential backoff and jitter for transient failures +- Response size limits to prevent OOM from malformed API responses +- Context cancellation support for graceful shutdown +- Pagination guards to prevent infinite loops +- Terminal sanitization for external content display + +## What Beads Will NOT Build + +These are explicitly out of scope. If a feature falls into this category, it should be rejected or redirected to an external tool. + +### Webhook Gateways / Real-Time Event Systems + +Beads does not need sub-second sync. Polled sync on a reasonable interval (minutes to hours) is sufficient for its use case. Webhooks add operational complexity (public endpoints, authentication, retry queues) that is disproportionate to the value they provide. + +### Cross-Tracker Orchestration + +Routing issues between trackers (e.g., "when a Jira issue is labeled X, create a GitHub issue") is workflow automation, not issue tracking. Tools like Zapier, n8n, or GitHub Actions are better suited for this. + +### Attachment / Binary Content Sync + +Syncing file attachments across platforms introduces storage management, content-type handling, and size limit mismatches. This is a CDN problem, not an issue tracker problem. + +### Full Comment / Thread Mirroring + +Comment threads are tightly coupled to each platform's UI and notification systems. Mirroring them creates confusing duplicate notifications and attribution problems. Beads syncs issue metadata, not conversation history. + +### Credential Vault / Multi-Platform Token Aggregation + +Each tracker integration uses a single API token configured by the user. Beads does not aggregate, rotate, or vault credentials across platforms. Users manage their own tokens through their platform's standard mechanisms. + +### UI Parity Features + +Beads will not replicate a tracker's web UI features (dashboards, burndown charts, sprint boards). The CLI and JSON output are the interface. If a team needs rich visualization, they should use their tracker's native UI alongside beads. + +## Design Guidelines for New Integrations + +When adding support for a new tracker or extending an existing one: + +1. **Follow the existing pattern** — See `internal/github/`, `internal/jira/`, etc. Each tracker implements the same interface with consistent retry, pagination, and error handling. + +2. **Map to beads concepts** — Translate the tracker's data model into beads' core types (Issue, Dependency, Status). Don't import tracker-specific concepts that don't map cleanly. + +3. **Fail loudly** — Surface warnings and errors in `SyncStats`. Never silently swallow a failure that could lead to data inconsistency. + +4. **Respect rate limits** — Honor `Retry-After` headers, implement backoff with jitter, and document the tracker's rate limit policy. + +5. **Test with mocks** — Integration tests should use mock HTTP responses, not live API calls. This keeps CI fast and avoids token management in CI environments. + +6. **Document the mapping** — Each integration should document how tracker statuses, priorities, and fields map to beads equivalents. + +## Decision Log + +| Date | Decision | Rationale | +|------|----------|-----------| +| 2026-03-24 | Integrations are adoption bridge, not product | Prevent scope creep; focus on dependency graph as differentiator | +| 2026-03-24 | No webhooks, ever | Operational complexity disproportionate to value for polled use case | +| 2026-03-24 | No cross-tracker orchestration | Workflow automation is a different problem domain | +| 2026-03-24 | Fail loudly on sync errors | Silent failures cause data inconsistency and erode trust | + +## Related Documents + +- [CLI Reference](CLI_REFERENCE.md) — Full command documentation +- [SECURITY.md](../SECURITY.md) — Security considerations for tracker tokens +- [CONTRIBUTING.md](../CONTRIBUTING.md) — How to contribute new integrations From fe6aea19293f0ee1aea7afdbd8c7d51f2fe8dc30 Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 24 Mar 2026 11:36:54 -0700 Subject: [PATCH 05/18] Surface sync engine warnings in SyncResult and fix silent failure paths - Collect warnings in Engine.warnings during Sync() and populate result.Warnings (field existed but was never used) - Fix createDependencies: return error count, stop silently discarding GetIssueByExternalRef errors (previously used _ discard) - Fix external_ref update failure: now increments stats.Errors when the local link-back fails after creating an external issue - Add Errors field to PullStats and propagate to SyncResult - Dependency creation failures now counted as Skipped in pull stats Fixes: bd-5cx Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/tracker/engine.go | 42 ++++++++++++++++++++++++++++++-------- internal/tracker/types.go | 1 + 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/internal/tracker/engine.go b/internal/tracker/engine.go index 882433adf3..0de2c5c0cd 100644 --- a/internal/tracker/engine.go +++ b/internal/tracker/engine.go @@ -82,6 +82,9 @@ type Engine struct { // stateCache holds the opaque value from PushHooks.BuildStateCache during a push. // Tracker adapters access it via ResolveState(). stateCache interface{} + + // warnings collects warning messages during a Sync() call for inclusion in SyncResult. + warnings []string } // NewEngine creates a new sync engine for the given tracker and storage. @@ -106,6 +109,7 @@ func (e *Engine) Sync(ctx context.Context, opts SyncOptions) (*SyncResult, error defer span.End() result := &SyncResult{Success: true} + e.warnings = nil now := time.Now().UTC() // Default to bidirectional if neither specified @@ -132,6 +136,7 @@ func (e *Engine) Sync(ctx context.Context, opts SyncOptions) (*SyncResult, error result.Stats.Created += pullStats.Created result.Stats.Updated += pullStats.Updated result.Stats.Skipped += pullStats.Skipped + result.Stats.Errors += pullStats.Errors } // Phase 2: Detect conflicts (only for bidirectional sync) @@ -183,6 +188,7 @@ func (e *Engine) Sync(ctx context.Context, opts SyncOptions) (*SyncResult, error result.LastSync = lastSync } + result.Warnings = e.warnings return result, nil } @@ -376,7 +382,8 @@ func (e *Engine) doPull(ctx context.Context, opts SyncOptions) (*PullStats, erro } // Create dependencies after all issues are imported - e.createDependencies(ctx, pendingDeps) + depErrors := e.createDependencies(ctx, pendingDeps) + stats.Skipped += depErrors span.SetAttributes( attribute.Int("sync.created", stats.Created), @@ -472,6 +479,9 @@ func (e *Engine) doPush(ctx context.Context, opts SyncOptions, skipIDs, forceIDs updates := map[string]interface{}{"external_ref": ref} if err := e.Store.UpdateIssue(ctx, issue.ID, updates, e.Actor); err != nil { e.warn("Failed to update external_ref for %s: %v", issue.ID, err) + stats.Errors++ + // Note: issue WAS created externally, so we still count Created + // but also flag the error so the user knows the link is broken } stats.Created++ } else if !opts.CreateOnly || forceIDs[issue.ID] { @@ -576,18 +586,30 @@ func (e *Engine) reimportIssue(ctx context.Context, c Conflict) { } // createDependencies creates dependencies from the pending list, matching -// external IDs to local issue IDs. -func (e *Engine) createDependencies(ctx context.Context, deps []DependencyInfo) { +// external IDs to local issue IDs. Returns the number of dependencies that +// failed to resolve or create. +func (e *Engine) createDependencies(ctx context.Context, deps []DependencyInfo) int { if len(deps) == 0 { - return + return 0 } + errCount := 0 for _, dep := range deps { - fromIssue, _ := e.Store.GetIssueByExternalRef(ctx, dep.FromExternalID) - toIssue, _ := e.Store.GetIssueByExternalRef(ctx, dep.ToExternalID) + fromIssue, err := e.Store.GetIssueByExternalRef(ctx, dep.FromExternalID) + if err != nil { + e.warn("Failed to resolve dependency source %s: %v", dep.FromExternalID, err) + errCount++ + continue + } + toIssue, err := e.Store.GetIssueByExternalRef(ctx, dep.ToExternalID) + if err != nil { + e.warn("Failed to resolve dependency target %s: %v", dep.ToExternalID, err) + errCount++ + continue + } if fromIssue == nil || toIssue == nil { - continue + continue // Not found (no error) — expected if issue wasn't imported } d := &types.Dependency{ @@ -597,8 +619,10 @@ func (e *Engine) createDependencies(ctx context.Context, deps []DependencyInfo) } if err := e.Store.AddDependency(ctx, d, e.Actor); err != nil { e.warn("Failed to create dependency %s -> %s: %v", fromIssue.ID, toIssue.ID, err) + errCount++ } } + return errCount } // shouldPushIssue checks if an issue should be included in push based on filters. @@ -661,7 +685,9 @@ func (e *Engine) msg(format string, args ...interface{}) { } func (e *Engine) warn(format string, args ...interface{}) { + msg := fmt.Sprintf(format, args...) + e.warnings = append(e.warnings, msg) if e.OnWarning != nil { - e.OnWarning(fmt.Sprintf(format, args...)) + e.OnWarning(msg) } } diff --git a/internal/tracker/types.go b/internal/tracker/types.go index a465b242b1..54c26b95ba 100644 --- a/internal/tracker/types.go +++ b/internal/tracker/types.go @@ -111,6 +111,7 @@ type PullStats struct { Created int Updated int Skipped int + Errors int Incremental bool SyncedSince string } From ca0e6bd90310f2f2b4902daeac3a0f3db986190c Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 24 Mar 2026 11:40:41 -0700 Subject: [PATCH 06/18] Add --explain flag to bd ready for dependency-aware reasoning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bd ready --explain provides a comprehensive view of the dependency graph: - Ready items with reasoning (resolved blockers, dependency counts) - Blocked items with detailed blocker info (ID, title, status, priority) - Cycle detection results - Aggregate summary stats Works in both JSON (--json) and human-readable text modes. JSON output gives agents structured data for graph-aware work prioritization. No storage layer changes — all data comes from existing GetReadyWork, GetBlockedIssues, GetDependencyRecordsForIssues, and DetectCycles APIs. New types: ReadyExplanation, ReadyItem, BlockedItem, BlockerInfo, ExplainSummary (all in internal/types/types.go). Fixes: bd-7zt Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cmd/bd/ready.go | 223 ++++++++++++++++++++++++++++++++++++++++ internal/types/types.go | 40 +++++++ 2 files changed, 263 insertions(+) diff --git a/cmd/bd/ready.go b/cmd/bd/ready.go index 2c3399658f..4b2773afab 100644 --- a/cmd/bd/ready.go +++ b/cmd/bd/ready.go @@ -46,6 +46,13 @@ This is useful for agents executing molecules to see which steps can run next.`, return } + // Handle --explain flag (dependency-aware reasoning) + explain, _ := cmd.Flags().GetBool("explain") + if explain { + runReadyExplain(cmd) + return + } + limit, _ := cmd.Flags().GetInt("limit") assignee, _ := cmd.Flags().GetString("assignee") unassigned, _ := cmd.Flags().GetBool("unassigned") @@ -409,6 +416,221 @@ func displayReadyList(issues []*types.Issue, parentEpicMap map[string]string) { fmt.Println("Status: ○ open ◐ in_progress ● blocked ✓ closed ❄ deferred") } +// runReadyExplain shows dependency-aware reasoning for why issues are ready or blocked. +func runReadyExplain(cmd *cobra.Command) { + ctx := rootCtx + + // Get the active store (respect --rig flag) + activeStore := store + rigOverride, _ := cmd.Flags().GetString("rig") + if rigOverride != "" { + rigStore, err := openStoreForRig(ctx, rigOverride) + if err != nil { + FatalError("%v", err) + } + defer func() { _ = rigStore.Close() }() + activeStore = rigStore + } else { + routedStore, routed, err := openRoutedReadStore(ctx, activeStore) + if err != nil { + FatalError("%v", err) + } + if routed { + defer func() { _ = routedStore.Close() }() + activeStore = routedStore + } + } + + // Get ready issues (no limit for explain mode — show everything) + filter := types.WorkFilter{ + Status: types.StatusOpen, + SortPolicy: types.SortPolicyPriority, + } + readyIssues, err := activeStore.GetReadyWork(ctx, filter) + if err != nil { + FatalErrorRespectJSON("%v", err) + } + + // Get blocked issues + blockedIssues, err := activeStore.GetBlockedIssues(ctx, types.WorkFilter{}) + if err != nil { + FatalErrorRespectJSON("%v", err) + } + + // Get dependency records for ready issues to find resolved blockers + readyIDs := make([]string, len(readyIssues)) + for i, issue := range readyIssues { + readyIDs[i] = issue.ID + } + depCounts, _ := activeStore.GetDependencyCounts(ctx, readyIDs) + allDeps, _ := activeStore.GetDependencyRecordsForIssues(ctx, readyIDs) + + // Detect cycles + cycles, _ := activeStore.DetectCycles(ctx) + + // Build ready items with explanations + readyItems := make([]types.ReadyItem, 0, len(readyIssues)) + for _, issue := range readyIssues { + counts := depCounts[issue.ID] + if counts == nil { + counts = &types.DependencyCounts{} + } + + // Find resolved blockers (closed issues that this depended on) + var resolvedBlockers []string + reason := "no blocking dependencies" + deps := allDeps[issue.ID] + for _, dep := range deps { + if dep.Type == types.DepBlocks || dep.Type == types.DepConditionalBlocks || dep.Type == types.DepWaitsFor { + resolvedBlockers = append(resolvedBlockers, dep.DependsOnID) + } + } + if len(resolvedBlockers) > 0 { + reason = fmt.Sprintf("%d blocker(s) resolved", len(resolvedBlockers)) + } + + // Compute parent + var parent *string + for _, dep := range deps { + if dep.Type == types.DepParentChild { + parent = &dep.DependsOnID + break + } + } + + readyItems = append(readyItems, types.ReadyItem{ + Issue: issue, + Reason: reason, + ResolvedBlockers: resolvedBlockers, + DependencyCount: counts.DependencyCount, + DependentCount: counts.DependentCount, + Parent: parent, + }) + } + + // Build blocked items with blocker details + blockedItems := make([]types.BlockedItem, 0, len(blockedIssues)) + // Collect all blocker IDs to batch-fetch + allBlockerIDs := make(map[string]bool) + for _, bi := range blockedIssues { + for _, blockerID := range bi.BlockedBy { + allBlockerIDs[blockerID] = true + } + } + blockerIDList := make([]string, 0, len(allBlockerIDs)) + for id := range allBlockerIDs { + blockerIDList = append(blockerIDList, id) + } + blockerIssues, _ := activeStore.GetIssuesByIDs(ctx, blockerIDList) + blockerMap := make(map[string]*types.Issue, len(blockerIssues)) + for _, issue := range blockerIssues { + blockerMap[issue.ID] = issue + } + + for _, bi := range blockedIssues { + blockers := make([]types.BlockerInfo, 0, len(bi.BlockedBy)) + for _, blockerID := range bi.BlockedBy { + info := types.BlockerInfo{ID: blockerID} + if blocker, ok := blockerMap[blockerID]; ok { + info.Title = blocker.Title + info.Status = blocker.Status + info.Priority = blocker.Priority + } + blockers = append(blockers, info) + } + blockedItems = append(blockedItems, types.BlockedItem{ + Issue: bi.Issue, + BlockedBy: blockers, + BlockedByCount: bi.BlockedByCount, + }) + } + + // Build cycle info + var cycleIDs [][]string + for _, cycle := range cycles { + ids := make([]string, len(cycle)) + for i, issue := range cycle { + ids[i] = issue.ID + } + cycleIDs = append(cycleIDs, ids) + } + + explanation := types.ReadyExplanation{ + Ready: readyItems, + Blocked: blockedItems, + Cycles: cycleIDs, + Summary: types.ExplainSummary{ + TotalReady: len(readyItems), + TotalBlocked: len(blockedItems), + CycleCount: len(cycleIDs), + }, + } + + if jsonOutput { + outputJSON(explanation) + return + } + + // Human-readable output + fmt.Printf("\n%s Ready Work Explanation\n\n", ui.RenderAccent("📊")) + + // Ready section + if len(explanation.Ready) > 0 { + fmt.Printf("%s Ready (%d issues):\n\n", ui.RenderPass("●"), len(explanation.Ready)) + for _, item := range explanation.Ready { + fmt.Printf(" %s [%s] %s\n", + ui.RenderID(item.ID), + ui.RenderPriority(item.Priority), + item.Title) + fmt.Printf(" Reason: %s\n", item.Reason) + if len(item.ResolvedBlockers) > 0 { + fmt.Printf(" Resolved blockers: %s\n", strings.Join(item.ResolvedBlockers, ", ")) + } + if item.DependentCount > 0 { + fmt.Printf(" Unblocks: %d issue(s)\n", item.DependentCount) + } + fmt.Println() + } + } else { + fmt.Printf("%s No ready work\n\n", ui.RenderWarn("○")) + } + + // Blocked section + if len(explanation.Blocked) > 0 { + fmt.Printf("%s Blocked (%d issues):\n\n", ui.RenderFail("●"), len(explanation.Blocked)) + for _, item := range explanation.Blocked { + fmt.Printf(" %s [%s] %s\n", + ui.RenderID(item.ID), + ui.RenderPriority(item.Priority), + item.Title) + for _, blocker := range item.BlockedBy { + fmt.Printf(" ← blocked by %s: %s [%s]\n", + ui.RenderID(blocker.ID), blocker.Title, blocker.Status) + } + fmt.Println() + } + } + + // Cycles section + if len(explanation.Cycles) > 0 { + fmt.Printf("%s Cycles detected (%d):\n\n", ui.RenderFail("⚠"), len(explanation.Cycles)) + for _, cycle := range explanation.Cycles { + fmt.Printf(" %s → %s\n", strings.Join(cycle, " → "), cycle[0]) + } + fmt.Println() + } + + // Summary + fmt.Printf("%s Summary: %d ready, %d blocked", + ui.RenderMuted("─"), + explanation.Summary.TotalReady, + explanation.Summary.TotalBlocked) + if explanation.Summary.CycleCount > 0 { + fmt.Printf(", %d cycle(s)", explanation.Summary.CycleCount) + } + fmt.Printf("\n\n") +} + // runMoleculeReady shows ready steps within a specific molecule func runMoleculeReady(_ *cobra.Command, molIDArg string) { ctx := rootCtx @@ -553,6 +775,7 @@ func init() { readyCmd.Flags().Bool("gated", false, "Find molecules ready for gate-resume dispatch") readyCmd.Flags().StringSlice("exclude-type", nil, "Exclude issue types from results (comma-separated or repeatable, e.g., --exclude-type=convoy,epic)") readyCmd.Flags().String("rig", "", "Query a different rig's database (e.g., --rig my-project, --rig gt-, --rig gt)") + readyCmd.Flags().Bool("explain", false, "Show dependency-aware reasoning for why issues are ready or blocked") // Metadata filtering (GH#1406) readyCmd.Flags().StringArray("metadata-field", nil, "Filter by metadata field (key=value, repeatable)") readyCmd.Flags().String("has-metadata-key", "", "Filter issues that have this metadata key set") diff --git a/internal/types/types.go b/internal/types/types.go index b3b05a86c4..4871f0c013 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -945,6 +945,46 @@ type BlockedIssue struct { BlockedBy []string `json:"blocked_by"` } +// ReadyExplanation provides reasoning for why issues are ready or blocked. +type ReadyExplanation struct { + Ready []ReadyItem `json:"ready"` + Blocked []BlockedItem `json:"blocked"` + Cycles [][]string `json:"cycles,omitempty"` + Summary ExplainSummary `json:"summary"` +} + +// ReadyItem explains why a specific issue is ready for work. +type ReadyItem struct { + *Issue + Reason string `json:"reason"` + ResolvedBlockers []string `json:"resolved_blockers"` + DependencyCount int `json:"dependency_count"` + DependentCount int `json:"dependent_count"` + Parent *string `json:"parent,omitempty"` +} + +// BlockedItem explains why a specific issue is blocked. +type BlockedItem struct { + Issue + BlockedBy []BlockerInfo `json:"blocked_by"` + BlockedByCount int `json:"blocked_by_count"` +} + +// BlockerInfo provides details about a single blocker. +type BlockerInfo struct { + ID string `json:"id"` + Title string `json:"title"` + Status Status `json:"status"` + Priority int `json:"priority"` +} + +// ExplainSummary provides aggregate statistics. +type ExplainSummary struct { + TotalReady int `json:"total_ready"` + TotalBlocked int `json:"total_blocked"` + CycleCount int `json:"cycle_count"` +} + // TreeNode represents a node in a dependency tree type TreeNode struct { Issue From 17744d7a38d82b8952f715178b2fc5453b6f9f60 Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 24 Mar 2026 11:47:32 -0700 Subject: [PATCH 07/18] Expand SECURITY.md with tracker integration trust model Add documentation for: - Trust boundary: all external tracker data treated as untrusted - Content sanitization (ANSI stripping, response size limits) - Credential handling: recommend platform-native auth over plaintext tokens - Sync security model: user-initiated only, no daemons or webhooks - AI agent content safety: prompt injection awareness - Dependency pinning via go.sum Fixes: bd-68o Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- SECURITY.md | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/SECURITY.md b/SECURITY.md index 7e7dd6a9ef..d180fa13ac 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -55,6 +55,33 @@ export DOLT_DISABLE_EVENT_FLUSH=1 To verify, block `doltremoteapi.dolthub.com` in your firewall or DNS — beads continues working normally with no degradation. +### Tracker Integration Trust Model + +When syncing with external trackers (GitHub Issues, Jira, Linear, GitLab, Azure DevOps), all data crossing the integration boundary is treated as **untrusted input**. + +**Trust boundaries:** +- Issue titles and descriptions from external trackers may contain arbitrary content, including ANSI escape sequences, control characters, or prompt injection payloads targeting AI agents +- External content is sanitized before terminal display (ANSI stripping, control character removal) +- API responses are size-limited to prevent out-of-memory conditions from malformed responses +- External issue identifiers are validated before use in SQL queries + +**Credential handling:** +- Tracker API tokens stored in beads config (`bd config set`) are **plaintext** in the Dolt database +- Prefer platform-native authentication when available (`gh auth`, `glab auth`, Azure CLI) — these use the platform's secure credential store +- Never store tokens in environment variables in shared environments +- Tokens are scoped to the permissions you grant — use minimal required scopes + +**Sync security model:** +- Sync is always **user-initiated** — no background daemons, no inbound webhooks, no listening ports +- No data is sent to external trackers unless the user explicitly runs `bd push` or `bd sync` +- Conflict resolution strategies are deterministic and auditable via Dolt history + +**Content safety for AI agents:** +- Issue descriptions imported from external trackers may contain prompt injection payloads +- Consuming agents should treat all issue content as untrusted input +- The `--json` output flag provides structured data that separates metadata from free-text content +- beads does not execute or interpret issue content — it is stored and displayed only + ### Command Injection Protection bd uses parameterized SQL queries to prevent SQL injection. However: @@ -69,7 +96,7 @@ bd has minimal dependencies: - Dolt (version-controlled SQL database) - Cobra CLI framework -All dependencies are regularly updated. Run `go mod verify` to check integrity. +All dependencies are pinned via `go.sum` and verified with `go mod verify`. Renovate (or Dependabot) monitors for known vulnerabilities. Run `go mod verify` locally to check integrity. ## Supported Versions From 66a6c94e6ce80d098db0482d9e224cab23915cb2 Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 24 Mar 2026 11:51:06 -0700 Subject: [PATCH 08/18] Enforce graph integrity: extend cycle detection and add bd graph check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extend write-path CTE and DFS cycle detection to include conditional-blocks (also creates blocking relationships, same as blocks) - Add explicit self-dependency check in AddDependencyInTx - Add bd graph check subcommand: runs DetectCycles, reports results in JSON or human-readable format, exits with code 1 if issues found waits-for is intentionally excluded from cycle detection — it uses gate semantics (fan-in) rather than direct blocking, so A waits-for B does not mean B blocks A in the traditional sense. Fixes: bd-2qr Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cmd/bd/graph.go | 73 +++++++++++++++++++++++ internal/storage/issueops/cycles.go | 4 +- internal/storage/issueops/dependencies.go | 9 ++- 3 files changed, 82 insertions(+), 4 deletions(-) diff --git a/cmd/bd/graph.go b/cmd/bd/graph.go index 9f70e73f4f..67086369bd 100644 --- a/cmd/bd/graph.go +++ b/cmd/bd/graph.go @@ -3,6 +3,7 @@ package main import ( "context" "fmt" + "os" "sort" "strings" @@ -162,6 +163,77 @@ Examples: }, } +var graphCheckCmd = &cobra.Command{ + Use: "check", + Short: "Check dependency graph integrity", + Long: `Check the dependency graph for cycles, orphans, and other integrity issues. + +Returns exit code 0 if the graph is clean, 1 if issues are found.`, + Run: func(cmd *cobra.Command, args []string) { + ctx := rootCtx + + type GraphCheckResult struct { + Clean bool `json:"clean"` + Cycles [][]string `json:"cycles"` + Summary struct { + CycleCount int `json:"cycle_count"` + } `json:"summary"` + } + + result := GraphCheckResult{Clean: true} + + // Detect cycles + cycles, err := store.DetectCycles(ctx) + if err != nil { + FatalErrorRespectJSON("cycle detection failed: %v", err) + } + + for _, cycle := range cycles { + ids := make([]string, len(cycle)) + for i, issue := range cycle { + ids[i] = issue.ID + } + result.Cycles = append(result.Cycles, ids) + } + result.Summary.CycleCount = len(cycles) + + if len(cycles) > 0 { + result.Clean = false + } + + if jsonOutput { + outputJSON(result) + if !result.Clean { + os.Exit(1) + } + return + } + + // Human-readable output + if result.Clean { + fmt.Printf("\n%s Graph integrity check passed\n\n", ui.RenderPass("✓")) + } else { + fmt.Printf("\n%s Graph integrity issues found\n\n", ui.RenderFail("✗")) + } + + if len(result.Cycles) > 0 { + fmt.Printf("%s Cycles (%d):\n\n", ui.RenderFail("⚠"), len(result.Cycles)) + for _, cycle := range result.Cycles { + fmt.Printf(" %s → %s\n", strings.Join(cycle, " → "), cycle[0]) + } + fmt.Println() + } else { + fmt.Printf(" %s No dependency cycles\n", ui.RenderPass("✓")) + } + + fmt.Println() + + if !result.Clean { + os.Exit(1) + } + }, +} + func init() { graphCmd.Flags().BoolVar(&graphAll, "all", false, "Show graph for all open issues") graphCmd.Flags().BoolVar(&graphCompact, "compact", false, "Tree format, one line per issue, more scannable") @@ -170,6 +242,7 @@ func init() { graphCmd.Flags().BoolVar(&graphHTML, "html", false, "Output self-contained interactive HTML (redirect to file)") graphCmd.ValidArgsFunction = issueIDCompletion rootCmd.AddCommand(graphCmd) + graphCmd.AddCommand(graphCheckCmd) } // loadGraphSubgraph loads an issue and its subgraph for visualization diff --git a/internal/storage/issueops/cycles.go b/internal/storage/issueops/cycles.go index 93b6f1d9cd..5aa05493a3 100644 --- a/internal/storage/issueops/cycles.go +++ b/internal/storage/issueops/cycles.go @@ -10,7 +10,7 @@ import ( // DetectCyclesInTx finds dependency cycles across both the dependencies and // wisp_dependencies tables. Returns slices of issues forming each cycle. -// Only considers "blocks" dependencies for cycle detection. +// Only considers "blocks" and "conditional-blocks" dependencies for cycle detection. // //nolint:gosec // G201: depTable is hardcoded to "dependencies" or "wisp_dependencies" func DetectCyclesInTx(ctx context.Context, tx *sql.Tx) ([][]*types.Issue, error) { @@ -31,7 +31,7 @@ func DetectCyclesInTx(ctx context.Context, tx *sql.Tx) ([][]*types.Issue, error) _ = rows.Close() return nil, fmt.Errorf("detect cycles: scan %s: %w", depTable, err) } - if types.DependencyType(depType) == types.DepBlocks { + if types.DependencyType(depType) == types.DepBlocks || types.DependencyType(depType) == types.DepConditionalBlocks { graph[issueID] = append(graph[issueID], dependsOnID) } } diff --git a/internal/storage/issueops/dependencies.go b/internal/storage/issueops/dependencies.go index 6734a13a81..084316db51 100644 --- a/internal/storage/issueops/dependencies.go +++ b/internal/storage/issueops/dependencies.go @@ -114,13 +114,18 @@ func AddDependencyInTx(ctx context.Context, tx *sql.Tx, dep *types.Dependency, a } } + // Self-dependency check + if dep.IssueID == dep.DependsOnID { + return fmt.Errorf("cannot add self-dependency: %s cannot depend on itself", dep.IssueID) + } + // Cycle detection for blocking deps via recursive CTE. - if dep.Type == types.DepBlocks { + if dep.Type == types.DepBlocks || dep.Type == types.DepConditionalBlocks { // Build UNION ALL across all dep tables for the CTE. var unions []string for _, t := range depTables { //nolint:gosec // G201: depTables are caller-controlled constants - unions = append(unions, fmt.Sprintf("SELECT issue_id, depends_on_id FROM %s WHERE type = 'blocks'", t)) + unions = append(unions, fmt.Sprintf("SELECT issue_id, depends_on_id FROM %s WHERE type IN ('blocks', 'conditional-blocks')", t)) } unionQuery := strings.Join(unions, " UNION ALL ") From df43a576fda06b63c137d773ae2c9357160303a0 Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 24 Mar 2026 11:51:15 -0700 Subject: [PATCH 09/18] Improve quickstart docs with Why Beads section and --explain examples - Add 'Why Beads?' section contrasting flat trackers vs dependency-aware ready queue - Add bd ready --explain example showing full graph reasoning output - Document bd ready vs bd list --status open distinction - Add graph check and explain commands to Next Steps section Fixes: bd-47m Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/QUICKSTART.md | 58 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/docs/QUICKSTART.md b/docs/QUICKSTART.md index 3cce8ab8e4..1511d736be 100644 --- a/docs/QUICKSTART.md +++ b/docs/QUICKSTART.md @@ -2,6 +2,33 @@ Get up and running with Beads in 2 minutes. +## Why Beads? + +Flat issue trackers (GitHub Issues, Jira, etc.) show you a list of open items. You pick one. But if that item depends on something else that isn't done yet, you've wasted time. Multiply this across a team of AI agents and humans, and you get thrashing. + +Beads tracks **dependencies between issues** and computes a **ready queue** — only items with no active blockers appear. Here's the difference: + +**Flat tracker (GitHub Issues):** +``` +Open issues: Set up database, Create API, Add authentication +→ An agent picks "Add authentication" and gets stuck immediately +``` + +**Beads:** +```bash +$ bd ready +1. [P1] [task] bd-1: Set up database + +$ bd ready --explain --json | jq '.blocked[0]' +{ + "id": "bd-3", + "title": "Add authentication", + "blocked_by": [{"id": "bd-2", "title": "Create API", "status": "open"}] +} +``` + +The agent picks the right task every time. No wasted cycles. + ## Installation ```bash @@ -153,6 +180,35 @@ Output: Only bd-1 is ready because bd-2 and bd-3 are blocked! +**Understanding why:** Use `--explain` to see the full graph reasoning: + +```bash +./bd ready --explain +``` + +Output: +``` +📊 Ready Work Explanation + +● Ready (1 issues): + + bd-1 [P1] Set up database + Reason: no blocking dependencies + Unblocks: 1 issue(s) + +● Blocked (2 issues): + + bd-2 [P2] Create API + ← blocked by bd-1: Set up database [open] + + bd-3 [P2] Add authentication + ← blocked by bd-2: Create API [open] + +─ Summary: 1 ready, 2 blocked +``` + +**Note:** `bd ready` is not the same as `bd list --status open`. The `list` command shows all open issues regardless of blockers. The `ready` command computes the dependency graph and only shows truly unblocked work. + ## Work the Queue ```bash @@ -253,6 +309,8 @@ bd admin cleanup --force - Add labels: `./bd create "Task" -l "backend,urgent"` - Filter ready work: `./bd ready --priority 1` +- Explain the graph: `./bd ready --explain` +- Check graph integrity: `./bd graph check` - Search issues: `./bd list --status open` - Detect cycles: `./bd dep cycles` - Use gates for PR/CI sync: See [DEPENDENCIES.md](DEPENDENCIES.md) From 0736b057f531ea4a907d6e5d03b8babb18350f4b Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 24 Mar 2026 12:08:57 -0700 Subject: [PATCH 10/18] Fix ADO Init tests leaking environment variables TestTracker_InitMissingPAT, InitMissingOrg, and InitMissingProject were not isolating from environment variables. When AZURE_DEVOPS_PAT is set in the environment (common in CI and dev), getConfig falls through to os.Getenv and the PAT check passes, causing the test to hit the org validation instead and fail. Fix: Use t.Setenv to clear all ADO env vars in tests that assert specific missing-config errors. t.Setenv automatically restores the original values after the test. Fixes: bd-czk Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/ado/tracker_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/ado/tracker_test.go b/internal/ado/tracker_test.go index aab4886607..8df62779cf 100644 --- a/internal/ado/tracker_test.go +++ b/internal/ado/tracker_test.go @@ -127,6 +127,11 @@ func TestTracker_InitWithCustomURL(t *testing.T) { } func TestTracker_InitMissingPAT(t *testing.T) { + // Clear env vars that getConfig falls back to, so mock store controls all config. + t.Setenv("AZURE_DEVOPS_PAT", "") + t.Setenv("AZURE_DEVOPS_ORG", "") + t.Setenv("AZURE_DEVOPS_PROJECT", "") + t.Setenv("AZURE_DEVOPS_URL", "") tr := &Tracker{} err := tr.Init(context.Background(), newMockStore(nil)) if err == nil { @@ -138,6 +143,8 @@ func TestTracker_InitMissingPAT(t *testing.T) { } func TestTracker_InitMissingOrg(t *testing.T) { + t.Setenv("AZURE_DEVOPS_ORG", "") + t.Setenv("AZURE_DEVOPS_URL", "") tr := &Tracker{} store := newMockStore(map[string]string{ "ado.pat": "some-pat", @@ -153,6 +160,7 @@ func TestTracker_InitMissingOrg(t *testing.T) { } func TestTracker_InitMissingProject(t *testing.T) { + t.Setenv("AZURE_DEVOPS_PROJECT", "") tr := &Tracker{} store := newMockStore(map[string]string{ "ado.pat": "some-pat", From 1f0cf1e724dceef7d21a714a8f51263581197af5 Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 24 Mar 2026 12:14:06 -0700 Subject: [PATCH 11/18] Improve compaction dry-run and analyze output with per-issue details All three compact paths (single, all, analyze) now show consistent structured output: - Per-candidate details: ID, title, closed date, age in days, content size - Summary footer: total candidates and aggregate content bytes - JSON output uses {candidates: [...], summary: {...}} structure Text output uses a tabular format for easy scanning. No changes to actual compaction behavior. Fixes: bd-oxs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cmd/bd/compact.go | 117 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 93 insertions(+), 24 deletions(-) diff --git a/cmd/bd/compact.go b/cmd/bd/compact.go index e8120a9467..33b38c7e27 100644 --- a/cmd/bd/compact.go +++ b/cmd/bd/compact.go @@ -221,22 +221,43 @@ func runCompactSingle(ctx context.Context, compactor *compact.Compactor, store s originalSize := len(issue.Description) + len(issue.Design) + len(issue.Notes) + len(issue.AcceptanceCriteria) if compactDryRun { + ageDays := 0 + var closedAtStr string + if issue.ClosedAt != nil { + ageDays = int(time.Since(*issue.ClosedAt).Hours() / 24) + closedAtStr = issue.ClosedAt.Format(time.RFC3339) + } + + candidate := map[string]interface{}{ + "id": issueID, + "title": issue.Title, + "closed_at": closedAtStr, + "age_days": ageDays, + "content_size": originalSize, + } + if jsonOutput { output := map[string]interface{}{ - "dry_run": true, - "tier": compactTier, - "issue_id": issueID, - "original_size": originalSize, - "estimated_reduction": "70-80%", + "dry_run": true, + "tier": compactTier, + "candidates": []interface{}{candidate}, + "summary": map[string]interface{}{ + "total_candidates": 1, + "total_content_bytes": originalSize, + }, } outputJSON(output) return } fmt.Printf("DRY RUN - Tier %d compaction\n\n", compactTier) - fmt.Printf("Issue: %s\n", issueID) - fmt.Printf("Original size: %d bytes\n", originalSize) - fmt.Printf("Estimated reduction: 70-80%%\n") + fmt.Printf(" %-12s %-40s %8s %10s\n", "ID", "TITLE", "AGE", "SIZE") + title := issue.Title + if len(title) > 40 { + title = title[:37] + "..." + } + fmt.Printf(" %-12s %-40s %5dd %10d B\n", issueID, title, ageDays, originalSize) + fmt.Printf("\nSummary: 1 candidate, %d bytes total content\n", originalSize) return } @@ -323,31 +344,64 @@ func runCompactAll(ctx context.Context, compactor *compact.Compactor, store stor } if compactDryRun { + type dryRunCandidate struct { + ID string `json:"id"` + Title string `json:"title"` + ClosedAt string `json:"closed_at"` + AgeDays int `json:"age_days"` + ContentSize int `json:"content_size"` + } + + var dryCandidates []dryRunCandidate totalSize := 0 for _, id := range candidates { issue, err := store.GetIssue(ctx, id) if err != nil { continue } - totalSize += len(issue.Description) + len(issue.Design) + len(issue.Notes) + len(issue.AcceptanceCriteria) + contentSize := len(issue.Description) + len(issue.Design) + len(issue.Notes) + len(issue.AcceptanceCriteria) + totalSize += contentSize + + ageDays := 0 + var closedAtStr string + if issue.ClosedAt != nil { + ageDays = int(time.Since(*issue.ClosedAt).Hours() / 24) + closedAtStr = issue.ClosedAt.Format(time.RFC3339) + } + + dryCandidates = append(dryCandidates, dryRunCandidate{ + ID: issue.ID, + Title: issue.Title, + ClosedAt: closedAtStr, + AgeDays: ageDays, + ContentSize: contentSize, + }) } if jsonOutput { output := map[string]interface{}{ - "dry_run": true, - "tier": compactTier, - "candidate_count": len(candidates), - "total_size_bytes": totalSize, - "estimated_reduction": "70-80%", + "dry_run": true, + "tier": compactTier, + "candidates": dryCandidates, + "summary": map[string]interface{}{ + "total_candidates": len(dryCandidates), + "total_content_bytes": totalSize, + }, } outputJSON(output) return } fmt.Printf("DRY RUN - Tier %d compaction\n\n", compactTier) - fmt.Printf("Candidates: %d issues\n", len(candidates)) - fmt.Printf("Total size: %d bytes\n", totalSize) - fmt.Printf("Estimated reduction: 70-80%%\n") + fmt.Printf(" %-12s %-40s %8s %10s\n", "ID", "TITLE", "AGE", "SIZE") + for _, c := range dryCandidates { + title := c.Title + if len(title) > 40 { + title = title[:37] + "..." + } + fmt.Printf(" %-12s %-40s %5dd %10d B\n", c.ID, title, c.AgeDays, c.ContentSize) + } + fmt.Printf("\nSummary: %d candidates, %d bytes total content\n", len(dryCandidates), totalSize) return } @@ -546,23 +600,38 @@ func runCompactAnalyze(ctx context.Context, store storage.DoltStorage) { } if jsonOutput { - outputJSON(candidates) + totalSize := 0 + for _, c := range candidates { + totalSize += c.SizeBytes + } + output := map[string]interface{}{ + "candidates": candidates, + "summary": map[string]interface{}{ + "total_candidates": len(candidates), + "total_content_bytes": totalSize, + }, + } + outputJSON(output) return } // Human-readable output fmt.Printf("Compaction Candidates (Tier %d)\n\n", compactTier) + fmt.Printf(" %-12s %-40s %8s %10s\n", "ID", "TITLE", "AGE", "SIZE") + totalSize := 0 for _, c := range candidates { compactStatus := "" if c.Compacted { - compactStatus = " (already compacted)" + compactStatus = " *" + } + title := c.Title + if len(title) > 40 { + title = title[:37] + "..." } - fmt.Printf("ID: %s%s\n", c.ID, compactStatus) - fmt.Printf(" Title: %s\n", c.Title) - fmt.Printf(" Size: %d bytes\n", c.SizeBytes) - fmt.Printf(" Age: %d days\n\n", c.AgeDays) + fmt.Printf(" %-12s %-40s %5dd %10d B%s\n", c.ID, title, c.AgeDays, c.SizeBytes, compactStatus) + totalSize += c.SizeBytes } - fmt.Printf("Total: %d candidates\n", len(candidates)) + fmt.Printf("\nSummary: %d candidates, %d bytes total content\n", len(candidates), totalSize) } func runCompactApply(ctx context.Context, store storage.DoltStorage) { From ca2715b4621ce55fe6fde41f3672520d4d92e864 Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 24 Mar 2026 12:15:58 -0700 Subject: [PATCH 12/18] Add integration test coverage for partial failures, warnings, and self-deps - Enhance mockTracker with createFailAfter/createCount for partial failure simulation (previously only supported all-or-nothing errors) - TestEnginePushPartialFailure: 3 issues, fail after 2 creates, asserts Created=2, Errors=1, warnings collected - TestEngineSyncCollectsWarnings: verifies result.Warnings is populated from push failures (tests the bd-5cx warning collection fix) - TestAddDependency_SelfDependencyRejected: verifies self-dependency check returns error (tests the bd-2qr self-dep guard) Fixes: bd-3jd Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../dolt/dependencies_extended_test.go | 37 +++++ internal/tracker/engine_test.go | 140 ++++++++++++++++-- 2 files changed, 168 insertions(+), 9 deletions(-) diff --git a/internal/storage/dolt/dependencies_extended_test.go b/internal/storage/dolt/dependencies_extended_test.go index 51a3077362..6e04f08036 100644 --- a/internal/storage/dolt/dependencies_extended_test.go +++ b/internal/storage/dolt/dependencies_extended_test.go @@ -1108,4 +1108,41 @@ func TestAddDependency_ParentChild_CrossType_Allowed(t *testing.T) { } } +// ============================================================================= +// Self-Dependency Rejection Tests (bd-2qr) +// ============================================================================= + +func TestAddDependency_SelfDependencyRejected(t *testing.T) { + store, cleanup := setupTestStore(t) + defer cleanup() + + ctx, cancel := testContext(t) + defer cancel() + + issue := &types.Issue{ + ID: "self-dep-issue", + Title: "Self Dependency Test", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + if err := store.CreateIssue(ctx, issue, "tester"); err != nil { + t.Fatalf("failed to create issue: %v", err) + } + + // Try to add a dependency from the issue to itself + dep := &types.Dependency{ + IssueID: issue.ID, + DependsOnID: issue.ID, + Type: types.DepBlocks, + } + err := store.AddDependency(ctx, dep, "tester") + if err == nil { + t.Fatal("expected AddDependency to reject self-dependency, but it succeeded") + } + if !strings.Contains(err.Error(), "self-dependency") { + t.Errorf("expected error containing 'self-dependency', got: %v", err) + } +} + // Note: testContext is already defined in dolt_test.go for this package diff --git a/internal/tracker/engine_test.go b/internal/tracker/engine_test.go index 941e99b883..b54e0a1ddb 100644 --- a/internal/tracker/engine_test.go +++ b/internal/tracker/engine_test.go @@ -56,14 +56,16 @@ func newTestStore(t *testing.T) *dolt.DoltStore { // mockTracker implements IssueTracker for testing. type mockTracker struct { - name string - issues []TrackerIssue - created []*types.Issue - updated map[string]*types.Issue - fetchErr error - createErr error - updateErr error - fieldMapper FieldMapper + name string + issues []TrackerIssue + created []*types.Issue + updated map[string]*types.Issue + fetchErr error + createErr error + updateErr error + fieldMapper FieldMapper + createFailAfter int // If > 0, fail CreateIssue after this many successful calls + createCount int // Counter for CreateIssue calls } func newMockTracker(name string) *mockTracker { @@ -114,7 +116,14 @@ func (m *mockTracker) FetchIssue(_ context.Context, identifier string) (*Tracker func (m *mockTracker) CreateIssue(_ context.Context, issue *types.Issue) (*TrackerIssue, error) { if m.createErr != nil { - return nil, m.createErr + if m.createFailAfter > 0 { + m.createCount++ + if m.createCount > m.createFailAfter { + return nil, m.createErr + } + } else { + return nil, m.createErr + } } m.created = append(m.created, issue) return &TrackerIssue{ @@ -750,3 +759,116 @@ func TestEnginePushWithStateCache(t *testing.T) { t.Errorf("ResolveState(Closed) = (%q, %v), want (%q, true)", stateID, ok, "state-closed-id") } } + +func TestEnginePushPartialFailure(t *testing.T) { + ctx := context.Background() + store := newTestStore(t) + defer store.Close() + + // Create 3 local issues + for i, id := range []string{"bd-pf1", "bd-pf2", "bd-pf3"} { + issue := &types.Issue{ + ID: id, + Title: fmt.Sprintf("Partial failure issue %d", i+1), + Status: types.StatusOpen, + IssueType: types.TypeTask, + Priority: 2, + } + if err := store.CreateIssue(ctx, issue, "test-actor"); err != nil { + t.Fatalf("CreateIssue(%s) error: %v", id, err) + } + } + + tracker := newMockTracker("test") + tracker.createFailAfter = 2 + tracker.createErr = fmt.Errorf("rate limit exceeded") + + engine := NewEngine(tracker, store, "test-actor") + + result, err := engine.Sync(ctx, SyncOptions{Push: true}) + if err != nil { + t.Fatalf("Sync() error: %v", err) + } + + // Partial failure doesn't abort the sync + if !result.Success { + t.Errorf("Sync() should succeed on partial failure, got Success=false: %s", result.Error) + } + if result.Stats.Created != 2 { + t.Errorf("Stats.Created = %d, want 2", result.Stats.Created) + } + if result.Stats.Errors != 1 { + t.Errorf("Stats.Errors = %d, want 1", result.Stats.Errors) + } + + // Warnings should contain the failure message + if len(result.Warnings) == 0 { + t.Fatal("expected warnings for partial failure, got none") + } + found := false + for _, w := range result.Warnings { + if strings.Contains(w, "rate limit exceeded") { + found = true + break + } + } + if !found { + t.Errorf("expected warning containing 'rate limit exceeded', got: %v", result.Warnings) + } +} + +func TestEngineSyncCollectsWarnings(t *testing.T) { + ctx := context.Background() + store := newTestStore(t) + defer store.Close() + + // Create 2 local issues — one will succeed, one will fail + for _, tc := range []struct { + id string + title string + }{ + {"bd-warn1", "Succeeds"}, + {"bd-warn2", "Fails"}, + } { + issue := &types.Issue{ + ID: tc.id, + Title: tc.title, + Status: types.StatusOpen, + IssueType: types.TypeTask, + Priority: 2, + } + if err := store.CreateIssue(ctx, issue, "test-actor"); err != nil { + t.Fatalf("CreateIssue(%s) error: %v", tc.id, err) + } + } + + tracker := newMockTracker("test") + tracker.createFailAfter = 1 + tracker.createErr = fmt.Errorf("API timeout") + + engine := NewEngine(tracker, store, "test-actor") + + result, err := engine.Sync(ctx, SyncOptions{Push: true}) + if err != nil { + t.Fatalf("Sync() error: %v", err) + } + + // result.Warnings should be non-nil and contain the failure message + if result.Warnings == nil { + t.Fatal("result.Warnings is nil, expected warning messages") + } + if len(result.Warnings) == 0 { + t.Fatal("result.Warnings is empty, expected at least one warning") + } + + found := false + for _, w := range result.Warnings { + if strings.Contains(w, "API timeout") { + found = true + break + } + } + if !found { + t.Errorf("expected warning containing 'API timeout', got: %v", result.Warnings) + } +} From 420af96f67357cbdb005db42eac58927397bda16 Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 24 Mar 2026 12:17:56 -0700 Subject: [PATCH 13/18] Enforce .beads/ directory permissions at runtime - Add config.BeadsDirPerm (0700) and config.BeadsFilePerm (0600) constants - Add config.CheckBeadsDirPermissions() for startup warning if permissions are too permissive (non-fatal, checks group/world bits) - Add config.EnsureBeadsDir() helper for consistent directory creation - Tighten all MkdirAll calls for .beads/ paths from 0750/0755 to 0700: init.go, doltserver.go, credentials.go, audit.go, recipes.go, embeddeddolt store.go and flock.go - Add startup check in loadEnvironment() (cmd/bd/main.go) - Windows build tag with no-op implementation (ACL-based permissions) - 6 tests: constants, dir creation perms, warning on permissive, no warning on secure, no error on nonexistent Fixes: bd-kl6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cmd/bd/init.go | 6 +- cmd/bd/main.go | 2 + internal/audit/audit.go | 2 +- internal/config/permissions.go | 34 ++++++++ internal/config/permissions_test.go | 114 +++++++++++++++++++++++++ internal/config/permissions_windows.go | 24 ++++++ internal/doltserver/doltserver.go | 6 +- internal/recipes/recipes.go | 2 +- internal/storage/dolt/credentials.go | 2 +- internal/storage/embeddeddolt/flock.go | 2 +- internal/storage/embeddeddolt/store.go | 2 +- 11 files changed, 185 insertions(+), 11 deletions(-) create mode 100644 internal/config/permissions.go create mode 100644 internal/config/permissions_test.go create mode 100644 internal/config/permissions_windows.go diff --git a/cmd/bd/init.go b/cmd/bd/init.go index d3bc1f39da..85205db077 100644 --- a/cmd/bd/init.go +++ b/cmd/bd/init.go @@ -276,8 +276,8 @@ environment variable.`, useLocalBeads := !hasExplicitBeadsDir || filepath.Clean(initDBDirAbs) == filepath.Clean(beadsDirAbs) if useLocalBeads { - // Create .beads directory - if err := os.MkdirAll(beadsDir, 0750); err != nil { + // Create .beads directory with owner-only permissions (0700). + if err := os.MkdirAll(beadsDir, config.BeadsDirPerm); err != nil { FatalError("failed to create .beads directory: %v", err) } @@ -335,7 +335,7 @@ environment variable.`, // Ensure storage directory exists (.beads/dolt). // In server mode, dolt.New() connects via TCP and doesn't create local directories, // so we create the marker directory explicitly. - if err := os.MkdirAll(initDBPath, 0750); err != nil { + if err := os.MkdirAll(initDBPath, config.BeadsDirPerm); err != nil { FatalError("failed to create storage directory %s: %v", initDBPath, err) } diff --git a/cmd/bd/main.go b/cmd/bd/main.go index 447906f993..f404ece529 100644 --- a/cmd/bd/main.go +++ b/cmd/bd/main.go @@ -146,6 +146,8 @@ func loadEnvironment() { // and resolves BEADS_DIR, redirects, and worktree paths. if beadsDir := beads.FindBeadsDir(); beadsDir != "" { loadBeadsEnvFile(beadsDir) + // Non-fatal warning if .beads/ directory has overly permissive access. + config.CheckBeadsDirPermissions(beadsDir) } } diff --git a/internal/audit/audit.go b/internal/audit/audit.go index 053562452f..69ebf7882f 100644 --- a/internal/audit/audit.go +++ b/internal/audit/audit.go @@ -62,7 +62,7 @@ func EnsureFile() (string, error) { if err != nil { return "", err } - if err := os.MkdirAll(filepath.Dir(p), 0750); err != nil { + if err := os.MkdirAll(filepath.Dir(p), 0700); err != nil { return "", fmt.Errorf("failed to create .beads directory: %w", err) } _, statErr := os.Stat(p) diff --git a/internal/config/permissions.go b/internal/config/permissions.go new file mode 100644 index 0000000000..f91a4ee5e5 --- /dev/null +++ b/internal/config/permissions.go @@ -0,0 +1,34 @@ +//go:build !windows + +package config + +import ( + "fmt" + "io/fs" + "os" +) + +const ( + // BeadsDirPerm is the permission mode for .beads/ directories (owner-only). + BeadsDirPerm fs.FileMode = 0700 + // BeadsFilePerm is the permission mode for state files inside .beads/ (owner-only). + BeadsFilePerm fs.FileMode = 0600 +) + +// EnsureBeadsDir creates the .beads directory with secure permissions. +func EnsureBeadsDir(path string) error { + return os.MkdirAll(path, BeadsDirPerm) +} + +// CheckBeadsDirPermissions warns to stderr if the .beads directory has +// group or world-accessible permissions. The check is non-fatal. +func CheckBeadsDirPermissions(path string) { + info, err := os.Stat(path) + if err != nil { + return // directory doesn't exist yet + } + perm := info.Mode().Perm() + if perm&0077 != 0 { + fmt.Fprintf(os.Stderr, "Warning: %s has permissions %04o (recommended: 0700). Run: chmod 700 %s\n", path, perm, path) + } +} diff --git a/internal/config/permissions_test.go b/internal/config/permissions_test.go new file mode 100644 index 0000000000..223d09fee8 --- /dev/null +++ b/internal/config/permissions_test.go @@ -0,0 +1,114 @@ +//go:build !windows + +package config + +import ( + "bytes" + "fmt" + "os" + "path/filepath" + "testing" +) + +func TestBeadsDirPermConstants(t *testing.T) { + if BeadsDirPerm != 0700 { + t.Errorf("BeadsDirPerm = %04o, want 0700", BeadsDirPerm) + } + if BeadsFilePerm != 0600 { + t.Errorf("BeadsFilePerm = %04o, want 0600", BeadsFilePerm) + } +} + +func TestEnsureBeadsDir(t *testing.T) { + dir := filepath.Join(t.TempDir(), ".beads") + if err := EnsureBeadsDir(dir); err != nil { + t.Fatalf("EnsureBeadsDir(%q) = %v", dir, err) + } + info, err := os.Stat(dir) + if err != nil { + t.Fatalf("Stat(%q) = %v", dir, err) + } + perm := info.Mode().Perm() + if perm != BeadsDirPerm { + t.Errorf("directory permissions = %04o, want %04o", perm, BeadsDirPerm) + } +} + +func TestEnsureBeadsDirNested(t *testing.T) { + dir := filepath.Join(t.TempDir(), ".beads", "dolt") + if err := EnsureBeadsDir(dir); err != nil { + t.Fatalf("EnsureBeadsDir(%q) = %v", dir, err) + } + // Both parent and child should exist + for _, d := range []string{filepath.Dir(dir), dir} { + info, err := os.Stat(d) + if err != nil { + t.Fatalf("Stat(%q) = %v", d, err) + } + perm := info.Mode().Perm() + if perm != BeadsDirPerm { + t.Errorf("%s permissions = %04o, want %04o", d, perm, BeadsDirPerm) + } + } +} + +func TestCheckBeadsDirPermissions_Secure(t *testing.T) { + dir := filepath.Join(t.TempDir(), ".beads") + if err := os.MkdirAll(dir, 0700); err != nil { + t.Fatal(err) + } + // Capture stderr + old := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + CheckBeadsDirPermissions(dir) + + w.Close() + os.Stderr = old + var buf bytes.Buffer + buf.ReadFrom(r) + if buf.Len() != 0 { + t.Errorf("expected no warning for 0700 dir, got: %s", buf.String()) + } +} + +func TestCheckBeadsDirPermissions_Permissive(t *testing.T) { + dir := filepath.Join(t.TempDir(), ".beads") + if err := os.MkdirAll(dir, 0755); err != nil { + t.Fatal(err) + } + // Capture stderr + old := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + CheckBeadsDirPermissions(dir) + + w.Close() + os.Stderr = old + var buf bytes.Buffer + buf.ReadFrom(r) + want := fmt.Sprintf("Warning: %s has permissions 0755 (recommended: 0700). Run: chmod 700 %s\n", dir, dir) + if buf.String() != want { + t.Errorf("warning = %q, want %q", buf.String(), want) + } +} + +func TestCheckBeadsDirPermissions_Nonexistent(t *testing.T) { + dir := filepath.Join(t.TempDir(), "no-such-dir") + // Capture stderr — should produce no output + old := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + CheckBeadsDirPermissions(dir) + + w.Close() + os.Stderr = old + var buf bytes.Buffer + buf.ReadFrom(r) + if buf.Len() != 0 { + t.Errorf("expected no output for nonexistent dir, got: %s", buf.String()) + } +} diff --git a/internal/config/permissions_windows.go b/internal/config/permissions_windows.go new file mode 100644 index 0000000000..264f186977 --- /dev/null +++ b/internal/config/permissions_windows.go @@ -0,0 +1,24 @@ +//go:build windows + +package config + +import ( + "io/fs" + "os" +) + +const ( + // BeadsDirPerm is the permission mode for .beads/ directories (owner-only). + BeadsDirPerm fs.FileMode = 0700 + // BeadsFilePerm is the permission mode for state files inside .beads/ (owner-only). + BeadsFilePerm fs.FileMode = 0600 +) + +// EnsureBeadsDir creates the .beads directory with secure permissions. +func EnsureBeadsDir(path string) error { + return os.MkdirAll(path, BeadsDirPerm) +} + +// CheckBeadsDirPermissions is a no-op on Windows where filesystem +// permissions use ACLs rather than Unix permission bits. +func CheckBeadsDirPermissions(path string) {} diff --git a/internal/doltserver/doltserver.go b/internal/doltserver/doltserver.go index ea5cc441f7..8f67a6a50a 100644 --- a/internal/doltserver/doltserver.go +++ b/internal/doltserver/doltserver.go @@ -129,7 +129,7 @@ func SharedServerDir() (string, error) { return "", fmt.Errorf("cannot determine home directory: %w", err) } dir := filepath.Join(home, ".beads", "shared-server") - if err := os.MkdirAll(dir, 0750); err != nil { + if err := os.MkdirAll(dir, config.BeadsDirPerm); err != nil { return "", fmt.Errorf("cannot create shared server directory %s: %w", dir, err) } return dir, nil @@ -143,7 +143,7 @@ func SharedDoltDir() (string, error) { return "", err } dir := filepath.Join(serverDir, "dolt") - if err := os.MkdirAll(dir, 0750); err != nil { + if err := os.MkdirAll(dir, config.BeadsDirPerm); err != nil { return "", fmt.Errorf("cannot create shared dolt directory %s: %w", dir, err) } return dir, nil @@ -1032,7 +1032,7 @@ const bdDoltMarker = ".bd-dolt-ok" // If .dolt/ exists, seeds the .bd-dolt-ok marker for existing working databases. // See GH#2137 for background on pre-0.56 database compatibility. func ensureDoltInit(doltDir string) error { - if err := os.MkdirAll(doltDir, 0750); err != nil { + if err := os.MkdirAll(doltDir, config.BeadsDirPerm); err != nil { return fmt.Errorf("creating dolt directory: %w", err) } diff --git a/internal/recipes/recipes.go b/internal/recipes/recipes.go index 24232a2690..549dd30c7f 100644 --- a/internal/recipes/recipes.go +++ b/internal/recipes/recipes.go @@ -218,7 +218,7 @@ func SaveUserRecipe(beadsDir, name, path string) error { } // Ensure directory exists - if err := os.MkdirAll(beadsDir, 0o755); err != nil { + if err := os.MkdirAll(beadsDir, 0o700); err != nil { return fmt.Errorf("create beads dir: %w", err) } diff --git a/internal/storage/dolt/credentials.go b/internal/storage/dolt/credentials.go index 43abe80d16..438adeaed9 100644 --- a/internal/storage/dolt/credentials.go +++ b/internal/storage/dolt/credentials.go @@ -93,7 +93,7 @@ func (s *DoltStore) initCredentialKey(ctx context.Context) error { // Write key file with owner-only permissions (0600). // Ensure the directory exists first — when connecting to an external // server without having run `bd init`, .beads/ may not exist yet (GH#2641). - if err := os.MkdirAll(s.beadsDir, 0750); err != nil { + if err := os.MkdirAll(s.beadsDir, 0700); err != nil { return fmt.Errorf("failed to create beads directory %s: %w", s.beadsDir, err) } if err := os.WriteFile(keyPath, key, 0600); err != nil { diff --git a/internal/storage/embeddeddolt/flock.go b/internal/storage/embeddeddolt/flock.go index ecbc324963..4ab546087a 100644 --- a/internal/storage/embeddeddolt/flock.go +++ b/internal/storage/embeddeddolt/flock.go @@ -21,7 +21,7 @@ type Lock struct { // If another process holds the lock, returns an error directing the user to // the dolt server backend for concurrent access. func TryLock(dataDir string) (*Lock, error) { - if err := os.MkdirAll(dataDir, 0750); err != nil { + if err := os.MkdirAll(dataDir, 0700); err != nil { return nil, fmt.Errorf("embeddeddolt: creating data directory for lock: %w", err) } diff --git a/internal/storage/embeddeddolt/store.go b/internal/storage/embeddeddolt/store.go index b77117fa5c..acf37d5547 100644 --- a/internal/storage/embeddeddolt/store.go +++ b/internal/storage/embeddeddolt/store.go @@ -51,7 +51,7 @@ func New(ctx context.Context, beadsDir, database, branch string) (*EmbeddedDoltS return nil, fmt.Errorf("embeddeddolt: resolving beads dir: %w", err) } dataDir := filepath.Join(absBeadsDir, "embeddeddolt") - if err := os.MkdirAll(dataDir, 0750); err != nil { + if err := os.MkdirAll(dataDir, config.BeadsDirPerm); err != nil { return nil, fmt.Errorf("embeddeddolt: creating data directory: %w", err) } From 05aabdc697bf0553b020b7c34c5300b1a03c15b0 Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 24 Mar 2026 12:32:03 -0700 Subject: [PATCH 14/18] jira: add MaxPages pagination guard and fix Retry-After jitter Add MaxPages=1000 pagination guard to SearchIssues() to match the pattern already implemented in Linear, GitHub, and GitLab clients. Without this, a malicious or buggy Jira instance returning inflated Total values could cause an infinite pagination loop. Also fix jitter application: only add jitter to our own exponential backoff delays, not to server-mandated Retry-After values. Adding jitter to Retry-After could violate rate limit contracts. Additionally guard against zero-length pages which could also cause infinite loops even within the MaxPages limit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/jira/client.go | 19 +++++++++++++++---- internal/jira/types.go | 3 +++ 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/internal/jira/client.go b/internal/jira/client.go index 4558d02c01..d170e38a3b 100644 --- a/internal/jira/client.go +++ b/internal/jira/client.go @@ -165,6 +165,7 @@ func (c *Client) SearchIssues(ctx context.Context, jql string) ([]Issue, error) var allIssues []Issue startAt := 0 maxResults := 100 + page := 0 for { select { @@ -173,6 +174,11 @@ func (c *Client) SearchIssues(ctx context.Context, jql string) ([]Issue, error) default: } + page++ + if page > MaxPages { + return nil, fmt.Errorf("pagination limit exceeded: stopped after %d pages", MaxPages) + } + params := url.Values{ "jql": {jql}, "fields": {searchFields}, @@ -199,7 +205,7 @@ func (c *Client) SearchIssues(ctx context.Context, jql string) ([]Issue, error) allIssues = append(allIssues, result.Issues...) - if startAt+len(result.Issues) >= result.Total { + if len(result.Issues) == 0 || startAt+len(result.Issues) >= result.Total { break } startAt += len(result.Issues) @@ -376,16 +382,21 @@ func (c *Client) doRequest(ctx context.Context, method, apiURL string, body []by if retriable { delay := RetryDelay * time.Duration(1< 0 { - delay += time.Duration(rand.Int64N(half)) + // Only add jitter to our own exponential backoff, not server-mandated delays + if !useServerDelay { + if half := int64(delay / 2); half > 0 { + delay += time.Duration(rand.Int64N(half)) + } } lastErr = fmt.Errorf("transient error %d (attempt %d/%d)", resp.StatusCode, attempt+1, MaxRetries+1) diff --git a/internal/jira/types.go b/internal/jira/types.go index d41b784d73..f4f9eb44af 100644 --- a/internal/jira/types.go +++ b/internal/jira/types.go @@ -13,4 +13,7 @@ const ( // MaxResponseSize is the maximum response body size (50 MB). MaxResponseSize = 50 * 1024 * 1024 + + // MaxPages is the maximum number of pagination requests to prevent infinite loops. + MaxPages = 1000 ) From bb089983fe9ae850f662c3280d2fbbeb96d3deef Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 24 Mar 2026 12:33:12 -0700 Subject: [PATCH 15/18] github,ado: fix Retry-After jitter in retry loops Apply the same fix as the Jira client: only add jitter to our own exponential backoff delays, not to server-mandated Retry-After values. Adding jitter to Retry-After could violate rate limit contracts by waiting less than the server-specified duration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/ado/client.go | 9 +++++++-- internal/github/client.go | 11 ++++++++--- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/internal/ado/client.go b/internal/ado/client.go index d2fbd171f5..d07355c4ff 100644 --- a/internal/ado/client.go +++ b/internal/ado/client.go @@ -243,13 +243,18 @@ func (c *Client) doRequest(ctx context.Context, method, urlStr, contentType stri if retriable && attempt < maxAttempts { delay := RetryDelay * time.Duration(1< 0 { - delay += time.Duration(rand.Int64N(half)) + // Only add jitter to our own exponential backoff, not server-mandated delays + if !useServerDelay { + if half := int64(delay / 2); half > 0 { + delay += time.Duration(rand.Int64N(half)) + } } lastErr = fmt.Errorf("transient error %d (attempt %d/%d)", resp.StatusCode, attempt+1, maxAttempts+1) select { diff --git a/internal/github/client.go b/internal/github/client.go index ff3218dbe9..793e2baa01 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -114,16 +114,21 @@ func (c *Client) doRequest(ctx context.Context, method, urlStr string, body inte if retriable { delay := RetryDelay * time.Duration(1< 0 { - delay += time.Duration(rand.Int64N(half)) + // Only add jitter to our own exponential backoff, not server-mandated delays + if !useServerDelay { + if half := int64(delay / 2); half > 0 { + delay += time.Duration(rand.Int64N(half)) + } } lastErr = fmt.Errorf("transient error %d (attempt %d/%d)", resp.StatusCode, attempt+1, MaxRetries+1) From 02b778c8ac879a6e51640d69463be8b39f20b103 Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 24 Mar 2026 14:12:09 -0700 Subject: [PATCH 16/18] tests: add coverage for explain logic, cycle detection, and engine warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract BuildReadyExplanation into types package as a pure function, enabling unit tests without Dolt dependency. Add 8 test cases covering: empty input, ready with/without deps, resolved blockers, parent links, blocked issues with missing blockers, cycles, and full scenario. Add 7 Dolt integration tests for cycle detection: - conditional-blocks cycles (prevented by AddDependency) - mixed blocks + conditional-blocks cycles - waits-for intentionally excluded from cycle detection - self-dependency rejected for all dependency types - ready work respects conditional-blocks - blocked issues include blocker details Add 4 engine tests: - createDependencies with valid deps - createDependencies with unresolvable external refs (warns) - createDependencies with empty input - warn() collects messages and triggers OnWarning callback Types package coverage: 89.6% → 91.0% Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cmd/bd/ready.go | 85 +----- .../dolt/dependencies_extended_test.go | 247 ++++++++++++++++++ internal/tracker/engine_test.go | 143 ++++++++++ internal/types/explain_test.go | 241 +++++++++++++++++ internal/types/types.go | 92 +++++++ 5 files changed, 727 insertions(+), 81 deletions(-) create mode 100644 internal/types/explain_test.go diff --git a/cmd/bd/ready.go b/cmd/bd/ready.go index 4b2773afab..7b5a7e078e 100644 --- a/cmd/bd/ready.go +++ b/cmd/bd/ready.go @@ -468,49 +468,7 @@ func runReadyExplain(cmd *cobra.Command) { // Detect cycles cycles, _ := activeStore.DetectCycles(ctx) - // Build ready items with explanations - readyItems := make([]types.ReadyItem, 0, len(readyIssues)) - for _, issue := range readyIssues { - counts := depCounts[issue.ID] - if counts == nil { - counts = &types.DependencyCounts{} - } - - // Find resolved blockers (closed issues that this depended on) - var resolvedBlockers []string - reason := "no blocking dependencies" - deps := allDeps[issue.ID] - for _, dep := range deps { - if dep.Type == types.DepBlocks || dep.Type == types.DepConditionalBlocks || dep.Type == types.DepWaitsFor { - resolvedBlockers = append(resolvedBlockers, dep.DependsOnID) - } - } - if len(resolvedBlockers) > 0 { - reason = fmt.Sprintf("%d blocker(s) resolved", len(resolvedBlockers)) - } - - // Compute parent - var parent *string - for _, dep := range deps { - if dep.Type == types.DepParentChild { - parent = &dep.DependsOnID - break - } - } - - readyItems = append(readyItems, types.ReadyItem{ - Issue: issue, - Reason: reason, - ResolvedBlockers: resolvedBlockers, - DependencyCount: counts.DependencyCount, - DependentCount: counts.DependentCount, - Parent: parent, - }) - } - - // Build blocked items with blocker details - blockedItems := make([]types.BlockedItem, 0, len(blockedIssues)) - // Collect all blocker IDs to batch-fetch + // Collect all blocker IDs to batch-fetch blocker details allBlockerIDs := make(map[string]bool) for _, bi := range blockedIssues { for _, blockerID := range bi.BlockedBy { @@ -521,50 +479,15 @@ func runReadyExplain(cmd *cobra.Command) { for id := range allBlockerIDs { blockerIDList = append(blockerIDList, id) } + + // Build ready items with explanations blockerIssues, _ := activeStore.GetIssuesByIDs(ctx, blockerIDList) blockerMap := make(map[string]*types.Issue, len(blockerIssues)) for _, issue := range blockerIssues { blockerMap[issue.ID] = issue } - for _, bi := range blockedIssues { - blockers := make([]types.BlockerInfo, 0, len(bi.BlockedBy)) - for _, blockerID := range bi.BlockedBy { - info := types.BlockerInfo{ID: blockerID} - if blocker, ok := blockerMap[blockerID]; ok { - info.Title = blocker.Title - info.Status = blocker.Status - info.Priority = blocker.Priority - } - blockers = append(blockers, info) - } - blockedItems = append(blockedItems, types.BlockedItem{ - Issue: bi.Issue, - BlockedBy: blockers, - BlockedByCount: bi.BlockedByCount, - }) - } - - // Build cycle info - var cycleIDs [][]string - for _, cycle := range cycles { - ids := make([]string, len(cycle)) - for i, issue := range cycle { - ids[i] = issue.ID - } - cycleIDs = append(cycleIDs, ids) - } - - explanation := types.ReadyExplanation{ - Ready: readyItems, - Blocked: blockedItems, - Cycles: cycleIDs, - Summary: types.ExplainSummary{ - TotalReady: len(readyItems), - TotalBlocked: len(blockedItems), - CycleCount: len(cycleIDs), - }, - } + explanation := types.BuildReadyExplanation(readyIssues, blockedIssues, depCounts, allDeps, blockerMap, cycles) if jsonOutput { outputJSON(explanation) diff --git a/internal/storage/dolt/dependencies_extended_test.go b/internal/storage/dolt/dependencies_extended_test.go index 6e04f08036..7f7fc68839 100644 --- a/internal/storage/dolt/dependencies_extended_test.go +++ b/internal/storage/dolt/dependencies_extended_test.go @@ -1145,4 +1145,251 @@ func TestAddDependency_SelfDependencyRejected(t *testing.T) { } } +// ============================================================================= +// Conditional-Blocks Cycle Detection Tests +// ============================================================================= + +func TestDetectCycles_ConditionalBlocksCycle(t *testing.T) { + store, cleanup := setupTestStore(t) + defer cleanup() + + ctx, cancel := testContext(t) + defer cancel() + + // Conditional-blocks should also be detected as cycles + issueA := &types.Issue{ID: "cb-cycle-a", Title: "A", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask} + issueB := &types.Issue{ID: "cb-cycle-b", Title: "B", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask} + + for _, issue := range []*types.Issue{issueA, issueB} { + if err := store.CreateIssue(ctx, issue, "tester"); err != nil { + t.Fatalf("failed to create issue: %v", err) + } + } + + // A conditional-blocks B + dep1 := &types.Dependency{IssueID: issueA.ID, DependsOnID: issueB.ID, Type: types.DepConditionalBlocks} + if err := store.AddDependency(ctx, dep1, "tester"); err != nil { + t.Fatalf("failed to add dependency A->B: %v", err) + } + + // B conditional-blocks A would create cycle — should be rejected + dep2 := &types.Dependency{IssueID: issueB.ID, DependsOnID: issueA.ID, Type: types.DepConditionalBlocks} + err := store.AddDependency(ctx, dep2, "tester") + if err == nil { + t.Fatal("expected AddDependency to reject conditional-blocks cycle, but it succeeded") + } + if !strings.Contains(err.Error(), "cycle") { + t.Errorf("expected error containing 'cycle', got: %v", err) + } +} + +func TestDetectCycles_MixedBlocksAndConditionalBlocks(t *testing.T) { + store, cleanup := setupTestStore(t) + defer cleanup() + + ctx, cancel := testContext(t) + defer cancel() + + // Mixed: A blocks B, B conditional-blocks C, C blocks A = cycle + issueA := &types.Issue{ID: "mixed-cycle-a", Title: "A", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask} + issueB := &types.Issue{ID: "mixed-cycle-b", Title: "B", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask} + issueC := &types.Issue{ID: "mixed-cycle-c", Title: "C", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask} + + for _, issue := range []*types.Issue{issueA, issueB, issueC} { + if err := store.CreateIssue(ctx, issue, "tester"); err != nil { + t.Fatalf("failed to create issue: %v", err) + } + } + + dep1 := &types.Dependency{IssueID: issueA.ID, DependsOnID: issueB.ID, Type: types.DepBlocks} + if err := store.AddDependency(ctx, dep1, "tester"); err != nil { + t.Fatalf("failed to add A->B blocks: %v", err) + } + + dep2 := &types.Dependency{IssueID: issueB.ID, DependsOnID: issueC.ID, Type: types.DepConditionalBlocks} + if err := store.AddDependency(ctx, dep2, "tester"); err != nil { + t.Fatalf("failed to add B->C conditional-blocks: %v", err) + } + + // C->A would close the cycle through mixed types + dep3 := &types.Dependency{IssueID: issueC.ID, DependsOnID: issueA.ID, Type: types.DepBlocks} + err := store.AddDependency(ctx, dep3, "tester") + if err == nil { + t.Fatal("expected AddDependency to reject mixed blocks/conditional-blocks cycle") + } + if !strings.Contains(err.Error(), "cycle") { + t.Errorf("expected error containing 'cycle', got: %v", err) + } +} + +func TestDetectCycles_WaitsForDoesNotCreateCycle(t *testing.T) { + store, cleanup := setupTestStore(t) + defer cleanup() + + ctx, cancel := testContext(t) + defer cancel() + + // waits-for should NOT trigger cycle detection (gate semantics) + issueA := &types.Issue{ID: "wf-nocycle-a", Title: "A", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask} + issueB := &types.Issue{ID: "wf-nocycle-b", Title: "B", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask} + + for _, issue := range []*types.Issue{issueA, issueB} { + if err := store.CreateIssue(ctx, issue, "tester"); err != nil { + t.Fatalf("failed to create issue: %v", err) + } + } + + // A waits-for B + dep1 := &types.Dependency{IssueID: issueA.ID, DependsOnID: issueB.ID, Type: types.DepWaitsFor} + if err := store.AddDependency(ctx, dep1, "tester"); err != nil { + t.Fatalf("failed to add A waits-for B: %v", err) + } + + // B waits-for A should succeed (waits-for doesn't create blocking cycles) + dep2 := &types.Dependency{IssueID: issueB.ID, DependsOnID: issueA.ID, Type: types.DepWaitsFor} + if err := store.AddDependency(ctx, dep2, "tester"); err != nil { + t.Fatalf("expected waits-for cycle to be allowed, got error: %v", err) + } + + // DetectCycles should find nothing (waits-for excluded from cycle detection) + cycles, err := store.DetectCycles(ctx) + if err != nil { + t.Fatalf("DetectCycles failed: %v", err) + } + if len(cycles) != 0 { + t.Errorf("expected no cycles (waits-for excluded), found %d", len(cycles)) + } +} + +func TestAddDependency_SelfDependencyAllTypes(t *testing.T) { + store, cleanup := setupTestStore(t) + defer cleanup() + + ctx, cancel := testContext(t) + defer cancel() + + issue := &types.Issue{ + ID: "self-dep-all", + Title: "Self Dep All Types", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + if err := store.CreateIssue(ctx, issue, "tester"); err != nil { + t.Fatalf("failed to create issue: %v", err) + } + + // Self-dependency should be rejected for all dependency types + for _, depType := range []types.DependencyType{ + types.DepBlocks, + types.DepConditionalBlocks, + types.DepWaitsFor, + } { + dep := &types.Dependency{ + IssueID: issue.ID, + DependsOnID: issue.ID, + Type: depType, + } + err := store.AddDependency(ctx, dep, "tester") + if err == nil { + t.Errorf("expected self-dependency to be rejected for type %q", depType) + } + if !strings.Contains(err.Error(), "self-dependency") { + t.Errorf("expected 'self-dependency' in error for type %q, got: %v", depType, err) + } + } +} + +// ============================================================================= +// Ready Work + Blocked Issues Integration Tests +// ============================================================================= + +func TestGetReadyWork_WithConditionalBlocks(t *testing.T) { + store, cleanup := setupTestStore(t) + defer cleanup() + + ctx, cancel := testContext(t) + defer cancel() + + // A conditional-blocks B: B should be blocked, A should be ready + issueA := &types.Issue{ID: "cb-ready-a", Title: "Blocker", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask} + issueB := &types.Issue{ID: "cb-ready-b", Title: "Blocked", Status: types.StatusOpen, Priority: 2, IssueType: types.TypeTask} + + for _, issue := range []*types.Issue{issueA, issueB} { + if err := store.CreateIssue(ctx, issue, "tester"); err != nil { + t.Fatalf("failed to create issue: %v", err) + } + } + + dep := &types.Dependency{IssueID: issueB.ID, DependsOnID: issueA.ID, Type: types.DepConditionalBlocks} + if err := store.AddDependency(ctx, dep, "tester"); err != nil { + t.Fatalf("failed to add dependency: %v", err) + } + + readyIssues, err := store.GetReadyWork(ctx, types.WorkFilter{Status: types.StatusOpen}) + if err != nil { + t.Fatalf("GetReadyWork failed: %v", err) + } + + // A should be ready, B should not + readyIDs := make(map[string]bool) + for _, issue := range readyIssues { + readyIDs[issue.ID] = true + } + + if !readyIDs["cb-ready-a"] { + t.Error("expected issue A to be ready") + } + if readyIDs["cb-ready-b"] { + t.Error("expected issue B to be blocked, but it appeared in ready work") + } +} + +func TestGetBlockedIssues_WithBlockerDetails(t *testing.T) { + store, cleanup := setupTestStore(t) + defer cleanup() + + ctx, cancel := testContext(t) + defer cancel() + + blocker := &types.Issue{ID: "blk-detail-a", Title: "The Blocker", Status: types.StatusOpen, Priority: 0, IssueType: types.TypeBug} + blocked := &types.Issue{ID: "blk-detail-b", Title: "Waiting on fix", Status: types.StatusOpen, Priority: 2, IssueType: types.TypeTask} + + for _, issue := range []*types.Issue{blocker, blocked} { + if err := store.CreateIssue(ctx, issue, "tester"); err != nil { + t.Fatalf("failed to create issue: %v", err) + } + } + + dep := &types.Dependency{IssueID: blocked.ID, DependsOnID: blocker.ID, Type: types.DepBlocks} + if err := store.AddDependency(ctx, dep, "tester"); err != nil { + t.Fatalf("failed to add dependency: %v", err) + } + + blockedIssues, err := store.GetBlockedIssues(ctx, types.WorkFilter{}) + if err != nil { + t.Fatalf("GetBlockedIssues failed: %v", err) + } + + var found *types.BlockedIssue + for _, bi := range blockedIssues { + if bi.ID == "blk-detail-b" { + found = bi + break + } + } + if found == nil { + t.Fatal("expected blocked issue 'blk-detail-b' not found") + } + if found.BlockedByCount < 1 { + t.Errorf("expected BlockedByCount >= 1, got %d", found.BlockedByCount) + } + if len(found.BlockedBy) == 0 { + t.Fatal("expected BlockedBy to contain blocker ID") + } + if found.BlockedBy[0] != "blk-detail-a" { + t.Errorf("expected BlockedBy[0] = 'blk-detail-a', got %q", found.BlockedBy[0]) + } +} + // Note: testContext is already defined in dolt_test.go for this package diff --git a/internal/tracker/engine_test.go b/internal/tracker/engine_test.go index b54e0a1ddb..5d56d9da55 100644 --- a/internal/tracker/engine_test.go +++ b/internal/tracker/engine_test.go @@ -872,3 +872,146 @@ func TestEngineSyncCollectsWarnings(t *testing.T) { t.Errorf("expected warning containing 'API timeout', got: %v", result.Warnings) } } + +func TestEngineCreateDependencies(t *testing.T) { + ctx := context.Background() + store := newTestStore(t) + defer store.Close() + + // Create two issues that will be the dependency endpoints + issue1 := &types.Issue{ + ID: "bd-dep1", + Title: "Dependency source", + Status: types.StatusOpen, + IssueType: types.TypeTask, + Priority: 2, + } + issue2 := &types.Issue{ + ID: "bd-dep2", + Title: "Dependency target", + Status: types.StatusOpen, + IssueType: types.TypeTask, + Priority: 2, + } + for _, issue := range []*types.Issue{issue1, issue2} { + if err := store.CreateIssue(ctx, issue, "test-actor"); err != nil { + t.Fatalf("CreateIssue error: %v", err) + } + } + + // Set external refs so GetIssueByExternalRef can find them + store.UpdateIssue(ctx, issue1.ID, map[string]interface{}{"external_ref": "https://test.test/EXT-1"}, "test-actor") + store.UpdateIssue(ctx, issue2.ID, map[string]interface{}{"external_ref": "https://test.test/EXT-2"}, "test-actor") + + tracker := newMockTracker("test") + engine := NewEngine(tracker, store, "test-actor") + + // Valid dependency + deps := []DependencyInfo{ + {FromExternalID: "https://test.test/EXT-1", ToExternalID: "https://test.test/EXT-2", Type: string(types.DepBlocks)}, + } + errCount := engine.createDependencies(ctx, deps) + if errCount != 0 { + t.Errorf("createDependencies returned errCount=%d, want 0", errCount) + } + + // Verify dependency was actually created + depRecords, err := store.GetDependencyRecords(ctx, issue1.ID) + if err != nil { + t.Fatalf("GetDependencyRecords error: %v", err) + } + if len(depRecords) == 0 { + t.Fatal("expected at least one dependency record, got none") + } +} + +func TestEngineCreateDependencies_UnresolvableRef(t *testing.T) { + ctx := context.Background() + store := newTestStore(t) + defer store.Close() + + tracker := newMockTracker("test") + engine := NewEngine(tracker, store, "test-actor") + + // Both external refs don't exist — should generate warnings + deps := []DependencyInfo{ + {FromExternalID: "https://test.test/MISSING-1", ToExternalID: "https://test.test/MISSING-2", Type: string(types.DepBlocks)}, + } + errCount := engine.createDependencies(ctx, deps) + if errCount == 0 { + t.Error("expected errCount > 0 for unresolvable external refs") + } + + // Warnings should be collected + if len(engine.warnings) == 0 { + t.Error("expected warnings for unresolvable refs, got none") + } + found := false + for _, w := range engine.warnings { + if strings.Contains(w, "MISSING-1") || strings.Contains(w, "resolve") { + found = true + break + } + } + if !found { + t.Errorf("expected warning about unresolvable ref, got: %v", engine.warnings) + } +} + +func TestEngineCreateDependencies_Empty(t *testing.T) { + ctx := context.Background() + store := newTestStore(t) + defer store.Close() + + tracker := newMockTracker("test") + engine := NewEngine(tracker, store, "test-actor") + + // Empty deps list should return 0 + errCount := engine.createDependencies(ctx, nil) + if errCount != 0 { + t.Errorf("createDependencies(nil) returned %d, want 0", errCount) + } + errCount = engine.createDependencies(ctx, []DependencyInfo{}) + if errCount != 0 { + t.Errorf("createDependencies([]) returned %d, want 0", errCount) + } +} + +func TestEngineWarnCollectsMessages(t *testing.T) { + tracker := newMockTracker("test") + engine := &Engine{ + Tracker: tracker, + Actor: "test-actor", + } + + // Verify warn() collects messages + engine.warn("warning %d", 1) + engine.warn("warning %d", 2) + + if len(engine.warnings) != 2 { + t.Fatalf("expected 2 warnings, got %d", len(engine.warnings)) + } + if engine.warnings[0] != "warning 1" { + t.Errorf("warnings[0] = %q, want %q", engine.warnings[0], "warning 1") + } + if engine.warnings[1] != "warning 2" { + t.Errorf("warnings[1] = %q, want %q", engine.warnings[1], "warning 2") + } + + // Verify OnWarning callback is also called + var callbackMsgs []string + engine.OnWarning = func(msg string) { + callbackMsgs = append(callbackMsgs, msg) + } + engine.warn("warning %d", 3) + + if len(callbackMsgs) != 1 { + t.Fatalf("expected 1 callback message, got %d", len(callbackMsgs)) + } + if callbackMsgs[0] != "warning 3" { + t.Errorf("callback got %q, want %q", callbackMsgs[0], "warning 3") + } + if len(engine.warnings) != 3 { + t.Errorf("expected 3 total warnings, got %d", len(engine.warnings)) + } +} diff --git a/internal/types/explain_test.go b/internal/types/explain_test.go new file mode 100644 index 0000000000..bd8c2b0ee7 --- /dev/null +++ b/internal/types/explain_test.go @@ -0,0 +1,241 @@ +package types + +import ( + "testing" +) + +func TestBuildReadyExplanation_NoIssues(t *testing.T) { + result := BuildReadyExplanation(nil, nil, nil, nil, nil, nil) + + if len(result.Ready) != 0 { + t.Errorf("expected 0 ready items, got %d", len(result.Ready)) + } + if len(result.Blocked) != 0 { + t.Errorf("expected 0 blocked items, got %d", len(result.Blocked)) + } + if result.Summary.TotalReady != 0 { + t.Errorf("expected TotalReady=0, got %d", result.Summary.TotalReady) + } + if result.Summary.TotalBlocked != 0 { + t.Errorf("expected TotalBlocked=0, got %d", result.Summary.TotalBlocked) + } + if result.Summary.CycleCount != 0 { + t.Errorf("expected CycleCount=0, got %d", result.Summary.CycleCount) + } +} + +func TestBuildReadyExplanation_ReadyWithNoDeps(t *testing.T) { + issues := []*Issue{ + {ID: "bd-1", Title: "First", Priority: 1, Status: StatusOpen}, + {ID: "bd-2", Title: "Second", Priority: 2, Status: StatusOpen}, + } + + result := BuildReadyExplanation(issues, nil, nil, nil, nil, nil) + + if len(result.Ready) != 2 { + t.Fatalf("expected 2 ready items, got %d", len(result.Ready)) + } + if result.Ready[0].Reason != "no blocking dependencies" { + t.Errorf("expected reason 'no blocking dependencies', got %q", result.Ready[0].Reason) + } + if result.Ready[0].DependencyCount != 0 { + t.Errorf("expected DependencyCount=0, got %d", result.Ready[0].DependencyCount) + } + if result.Summary.TotalReady != 2 { + t.Errorf("expected TotalReady=2, got %d", result.Summary.TotalReady) + } +} + +func TestBuildReadyExplanation_ReadyWithResolvedBlockers(t *testing.T) { + issues := []*Issue{ + {ID: "bd-1", Title: "Unblocked", Priority: 1, Status: StatusOpen}, + } + + depCounts := map[string]*DependencyCounts{ + "bd-1": {DependencyCount: 2, DependentCount: 1}, + } + + allDeps := map[string][]*Dependency{ + "bd-1": { + {IssueID: "bd-1", DependsOnID: "bd-blocker-1", Type: DepBlocks}, + {IssueID: "bd-1", DependsOnID: "bd-blocker-2", Type: DepConditionalBlocks}, + }, + } + + result := BuildReadyExplanation(issues, nil, depCounts, allDeps, nil, nil) + + if len(result.Ready) != 1 { + t.Fatalf("expected 1 ready item, got %d", len(result.Ready)) + } + + item := result.Ready[0] + if item.Reason != "2 blocker(s) resolved" { + t.Errorf("expected reason '2 blocker(s) resolved', got %q", item.Reason) + } + if len(item.ResolvedBlockers) != 2 { + t.Errorf("expected 2 resolved blockers, got %d", len(item.ResolvedBlockers)) + } + if item.DependencyCount != 2 { + t.Errorf("expected DependencyCount=2, got %d", item.DependencyCount) + } + if item.DependentCount != 1 { + t.Errorf("expected DependentCount=1, got %d", item.DependentCount) + } +} + +func TestBuildReadyExplanation_ReadyWithParent(t *testing.T) { + issues := []*Issue{ + {ID: "bd-child", Title: "Child task", Priority: 2, Status: StatusOpen}, + } + + allDeps := map[string][]*Dependency{ + "bd-child": { + {IssueID: "bd-child", DependsOnID: "bd-epic", Type: DepParentChild}, + }, + } + + result := BuildReadyExplanation(issues, nil, nil, allDeps, nil, nil) + + if len(result.Ready) != 1 { + t.Fatalf("expected 1 ready item, got %d", len(result.Ready)) + } + if result.Ready[0].Parent == nil { + t.Fatal("expected Parent to be set") + } + if *result.Ready[0].Parent != "bd-epic" { + t.Errorf("expected Parent='bd-epic', got %q", *result.Ready[0].Parent) + } +} + +func TestBuildReadyExplanation_BlockedIssues(t *testing.T) { + blockedIssues := []*BlockedIssue{ + { + Issue: Issue{ID: "bd-blocked", Title: "Stuck", Priority: 2, Status: StatusOpen}, + BlockedByCount: 2, + BlockedBy: []string{"bd-blocker-1", "bd-blocker-2"}, + }, + } + + blockerMap := map[string]*Issue{ + "bd-blocker-1": {ID: "bd-blocker-1", Title: "Critical bug", Status: StatusOpen, Priority: 0}, + "bd-blocker-2": {ID: "bd-blocker-2", Title: "Design review", Status: StatusInProgress, Priority: 1}, + } + + result := BuildReadyExplanation(nil, blockedIssues, nil, nil, blockerMap, nil) + + if len(result.Blocked) != 1 { + t.Fatalf("expected 1 blocked item, got %d", len(result.Blocked)) + } + + item := result.Blocked[0] + if item.BlockedByCount != 2 { + t.Errorf("expected BlockedByCount=2, got %d", item.BlockedByCount) + } + if len(item.BlockedBy) != 2 { + t.Fatalf("expected 2 blockers, got %d", len(item.BlockedBy)) + } + + // Check blocker details were populated + if item.BlockedBy[0].Title != "Critical bug" { + t.Errorf("expected blocker title 'Critical bug', got %q", item.BlockedBy[0].Title) + } + if item.BlockedBy[0].Priority != 0 { + t.Errorf("expected blocker priority 0, got %d", item.BlockedBy[0].Priority) + } + if item.BlockedBy[1].Status != StatusInProgress { + t.Errorf("expected blocker status 'in_progress', got %q", item.BlockedBy[1].Status) + } + + if result.Summary.TotalBlocked != 1 { + t.Errorf("expected TotalBlocked=1, got %d", result.Summary.TotalBlocked) + } +} + +func TestBuildReadyExplanation_BlockedWithMissingBlocker(t *testing.T) { + blockedIssues := []*BlockedIssue{ + { + Issue: Issue{ID: "bd-blocked", Title: "Stuck", Priority: 2, Status: StatusOpen}, + BlockedByCount: 1, + BlockedBy: []string{"bd-unknown"}, + }, + } + + // Empty blocker map — blocker not found + result := BuildReadyExplanation(nil, blockedIssues, nil, nil, nil, nil) + + if len(result.Blocked) != 1 { + t.Fatalf("expected 1 blocked item, got %d", len(result.Blocked)) + } + // Blocker info should have ID but no enriched fields + if result.Blocked[0].BlockedBy[0].ID != "bd-unknown" { + t.Errorf("expected blocker ID 'bd-unknown', got %q", result.Blocked[0].BlockedBy[0].ID) + } + if result.Blocked[0].BlockedBy[0].Title != "" { + t.Errorf("expected empty title for missing blocker, got %q", result.Blocked[0].BlockedBy[0].Title) + } +} + +func TestBuildReadyExplanation_Cycles(t *testing.T) { + cycles := [][]*Issue{ + { + {ID: "bd-a", Title: "A"}, + {ID: "bd-b", Title: "B"}, + {ID: "bd-c", Title: "C"}, + }, + } + + result := BuildReadyExplanation(nil, nil, nil, nil, nil, cycles) + + if len(result.Cycles) != 1 { + t.Fatalf("expected 1 cycle, got %d", len(result.Cycles)) + } + if len(result.Cycles[0]) != 3 { + t.Fatalf("expected cycle of length 3, got %d", len(result.Cycles[0])) + } + if result.Cycles[0][0] != "bd-a" || result.Cycles[0][1] != "bd-b" || result.Cycles[0][2] != "bd-c" { + t.Errorf("unexpected cycle IDs: %v", result.Cycles[0]) + } + if result.Summary.CycleCount != 1 { + t.Errorf("expected CycleCount=1, got %d", result.Summary.CycleCount) + } +} + +func TestBuildReadyExplanation_FullScenario(t *testing.T) { + readyIssues := []*Issue{ + {ID: "bd-ready1", Title: "Ready task", Priority: 1, Status: StatusOpen}, + } + blockedIssues := []*BlockedIssue{ + { + Issue: Issue{ID: "bd-stuck", Title: "Blocked task", Priority: 2, Status: StatusOpen}, + BlockedByCount: 1, + BlockedBy: []string{"bd-ready1"}, + }, + } + depCounts := map[string]*DependencyCounts{ + "bd-ready1": {DependencyCount: 0, DependentCount: 1}, + } + blockerMap := map[string]*Issue{ + "bd-ready1": readyIssues[0], + } + cycles := [][]*Issue{ + {{ID: "bd-x"}, {ID: "bd-y"}}, + } + + result := BuildReadyExplanation(readyIssues, blockedIssues, depCounts, nil, blockerMap, cycles) + + if result.Summary.TotalReady != 1 { + t.Errorf("TotalReady=%d, want 1", result.Summary.TotalReady) + } + if result.Summary.TotalBlocked != 1 { + t.Errorf("TotalBlocked=%d, want 1", result.Summary.TotalBlocked) + } + if result.Summary.CycleCount != 1 { + t.Errorf("CycleCount=%d, want 1", result.Summary.CycleCount) + } + if result.Ready[0].DependentCount != 1 { + t.Errorf("Ready item DependentCount=%d, want 1", result.Ready[0].DependentCount) + } + if result.Blocked[0].BlockedBy[0].Title != "Ready task" { + t.Errorf("Blocked item blocker title=%q, want 'Ready task'", result.Blocked[0].BlockedBy[0].Title) + } +} diff --git a/internal/types/types.go b/internal/types/types.go index 4871f0c013..20beea5742 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -985,6 +985,98 @@ type ExplainSummary struct { CycleCount int `json:"cycle_count"` } +// BuildReadyExplanation constructs a ReadyExplanation from pre-fetched data. +// This pure function is separated from CLI concerns for testability. +func BuildReadyExplanation( + readyIssues []*Issue, + blockedIssues []*BlockedIssue, + depCounts map[string]*DependencyCounts, + allDeps map[string][]*Dependency, + blockerMap map[string]*Issue, + cycles [][]*Issue, +) ReadyExplanation { + // Build ready items with explanations + readyItems := make([]ReadyItem, 0, len(readyIssues)) + for _, issue := range readyIssues { + counts := depCounts[issue.ID] + if counts == nil { + counts = &DependencyCounts{} + } + + // Find resolved blockers (closed issues that this depended on) + var resolvedBlockers []string + reason := "no blocking dependencies" + deps := allDeps[issue.ID] + for _, dep := range deps { + if dep.Type == DepBlocks || dep.Type == DepConditionalBlocks || dep.Type == DepWaitsFor { + resolvedBlockers = append(resolvedBlockers, dep.DependsOnID) + } + } + if len(resolvedBlockers) > 0 { + reason = fmt.Sprintf("%d blocker(s) resolved", len(resolvedBlockers)) + } + + // Compute parent + var parent *string + for _, dep := range deps { + if dep.Type == DepParentChild { + parent = &dep.DependsOnID + break + } + } + + readyItems = append(readyItems, ReadyItem{ + Issue: issue, + Reason: reason, + ResolvedBlockers: resolvedBlockers, + DependencyCount: counts.DependencyCount, + DependentCount: counts.DependentCount, + Parent: parent, + }) + } + + // Build blocked items with blocker details + blockedItems := make([]BlockedItem, 0, len(blockedIssues)) + for _, bi := range blockedIssues { + blockers := make([]BlockerInfo, 0, len(bi.BlockedBy)) + for _, blockerID := range bi.BlockedBy { + info := BlockerInfo{ID: blockerID} + if blocker, ok := blockerMap[blockerID]; ok { + info.Title = blocker.Title + info.Status = blocker.Status + info.Priority = blocker.Priority + } + blockers = append(blockers, info) + } + blockedItems = append(blockedItems, BlockedItem{ + Issue: bi.Issue, + BlockedBy: blockers, + BlockedByCount: bi.BlockedByCount, + }) + } + + // Build cycle info + var cycleIDs [][]string + for _, cycle := range cycles { + ids := make([]string, len(cycle)) + for i, issue := range cycle { + ids[i] = issue.ID + } + cycleIDs = append(cycleIDs, ids) + } + + return ReadyExplanation{ + Ready: readyItems, + Blocked: blockedItems, + Cycles: cycleIDs, + Summary: ExplainSummary{ + TotalReady: len(readyItems), + TotalBlocked: len(blockedItems), + CycleCount: len(cycleIDs), + }, + } +} + // TreeNode represents a node in a dependency tree type TreeNode struct { Issue From 23867cec753b2695ac392b688342f117a6f8d468 Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 24 Mar 2026 15:22:03 -0700 Subject: [PATCH 17/18] lint: suppress gosec G404 for retry jitter (math/rand is fine for backoff) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/ado/client.go | 4 ++-- internal/github/client.go | 2 +- internal/gitlab/client.go | 2 +- internal/jira/client.go | 2 +- internal/linear/client.go | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/ado/client.go b/internal/ado/client.go index d07355c4ff..7dade4d2e1 100644 --- a/internal/ado/client.go +++ b/internal/ado/client.go @@ -209,7 +209,7 @@ func (c *Client) doRequest(ctx context.Context, method, urlStr, contentType stri if attempt < maxAttempts { delay := RetryDelay * time.Duration(1< 0 { - delay += time.Duration(rand.Int64N(half)) + delay += time.Duration(rand.Int64N(half)) //nolint:gosec // G404: jitter for retry backoff does not need crypto rand } select { case <-ctx.Done(): @@ -253,7 +253,7 @@ func (c *Client) doRequest(ctx context.Context, method, urlStr, contentType stri // Only add jitter to our own exponential backoff, not server-mandated delays if !useServerDelay { if half := int64(delay / 2); half > 0 { - delay += time.Duration(rand.Int64N(half)) + delay += time.Duration(rand.Int64N(half)) //nolint:gosec // G404: jitter for retry backoff does not need crypto rand } } lastErr = fmt.Errorf("transient error %d (attempt %d/%d)", resp.StatusCode, attempt+1, maxAttempts+1) diff --git a/internal/github/client.go b/internal/github/client.go index 793e2baa01..a528e1c76d 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -127,7 +127,7 @@ func (c *Client) doRequest(ctx context.Context, method, urlStr string, body inte // Only add jitter to our own exponential backoff, not server-mandated delays if !useServerDelay { if half := int64(delay / 2); half > 0 { - delay += time.Duration(rand.Int64N(half)) + delay += time.Duration(rand.Int64N(half)) //nolint:gosec // G404: jitter for retry backoff does not need crypto rand } } diff --git a/internal/gitlab/client.go b/internal/gitlab/client.go index d587622396..bfcd2d801e 100644 --- a/internal/gitlab/client.go +++ b/internal/gitlab/client.go @@ -119,7 +119,7 @@ func (c *Client) doRequest(ctx context.Context, method, urlStr string, body inte if retriable { delay := RetryDelay * time.Duration(1< 0 { - delay += time.Duration(rand.Int64N(half)) + delay += time.Duration(rand.Int64N(half)) //nolint:gosec // G404: jitter for retry backoff does not need crypto rand } lastErr = fmt.Errorf("transient error %d (attempt %d/%d)", resp.StatusCode, attempt+1, MaxRetries+1) select { diff --git a/internal/jira/client.go b/internal/jira/client.go index d170e38a3b..9e444d697e 100644 --- a/internal/jira/client.go +++ b/internal/jira/client.go @@ -395,7 +395,7 @@ func (c *Client) doRequest(ctx context.Context, method, apiURL string, body []by // Only add jitter to our own exponential backoff, not server-mandated delays if !useServerDelay { if half := int64(delay / 2); half > 0 { - delay += time.Duration(rand.Int64N(half)) + delay += time.Duration(rand.Int64N(half)) //nolint:gosec // G404: jitter for retry backoff does not need crypto rand } } diff --git a/internal/linear/client.go b/internal/linear/client.go index 5b5440f022..6874228e83 100644 --- a/internal/linear/client.go +++ b/internal/linear/client.go @@ -185,7 +185,7 @@ func (c *Client) Execute(ctx context.Context, req *GraphQLRequest) (json.RawMess if resp.StatusCode == http.StatusTooManyRequests { delay := RetryDelay * time.Duration(1< 0 { - delay += time.Duration(rand.Int64N(half)) + delay += time.Duration(rand.Int64N(half)) //nolint:gosec // G404: jitter for retry backoff does not need crypto rand } lastErr = fmt.Errorf("rate limited (attempt %d/%d), retrying after %v", attempt+1, MaxRetries+1, delay) select { From 5fb2622a75db59b6ab2bc7fd964220d02565e1c1 Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 24 Mar 2026 15:46:49 -0700 Subject: [PATCH 18/18] ci: retrigger after flaky Dolt lock contention in TestEmbeddedInit Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>