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

Fix code review issues bd-19 through bd-23

Browse Source 
- bd-19: Fix import zero-value field handling using JSON presence detection
- bd-20: Add --strict flag for dependency import failures
- bd-21: Simplify getNextID SQL query (reduce params from 4 to 2)
- bd-22: Add validation/warning for malformed issue IDs
- bd-23: Optimize export dependency queries (fix N+1 problem)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Steve Yegge
2025-10-12 15:30:11 -07:00
parent f048602d9f
commit 3440899850
6 changed files with 111 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
.beads/issues.jsonl
View File

@@ -8,12 +8,12 @@
{"id":"bd-16","title":"Write comprehensive collision resolution tests","description":"Test cases: simple collision, multiple collisions, dependency updates, text reference updates, chain dependencies, edge cases (partial ID matches, case sensitivity, triple merges). Add to import_test.go and collision_test.go.","status":"open","priority":1,"issue_type":"task","created_at":"2025-10-12T14:40:56.702127-07:00","updated_at":"2025-10-12T15:07:26.333559-07:00","dependencies":[{"issue_id":"bd-16","depends_on_id":"bd-9","type":"parent-child","created_at":"2025-10-12T14:41:07.965816-07:00","created_by":"stevey"}]}
{"id":"bd-17","title":"Update documentation for collision resolution","description":"Update README.md with collision resolution section. Update CLAUDE.md with new workflow. Document --resolve-collisions and --dry-run flags. Add example scenarios showing branch merge workflows.","status":"open","priority":1,"issue_type":"task","created_at":"2025-10-12T14:40:56.866649-07:00","updated_at":"2025-10-12T15:07:26.333636-07:00","dependencies":[{"issue_id":"bd-17","depends_on_id":"bd-9","type":"parent-child","created_at":"2025-10-12T14:41:07.970302-07:00","created_by":"stevey"}]}
{"id":"bd-18","title":"Add design/notes/acceptance_criteria fields to update command","description":"Currently bd update only supports status, priority, title, assignee. Add support for --design, --notes, --acceptance-criteria flags. This makes it easier to add detailed designs to issues after creation.","status":"open","priority":2,"issue_type":"feature","created_at":"2025-10-12T14:40:57.032395-07:00","updated_at":"2025-10-12T15:07:26.33372-07:00"}
{"id":"bd-19","title":"Fix import zero-value field handling","description":"Import uses zero-value checks (Priority != 0) to determine field updates. This prevents setting priority to 0 or clearing string fields. Export/import round-trip not fully idempotent for zero values. Consider JSON presence detection or explicit preserve-existing semantics. Location: cmd/bd/import.go:95-106","status":"open","priority":2,"issue_type":"bug","created_at":"2025-10-12T15:13:17.895083-07:00","updated_at":"2025-10-12T15:13:17.895083-07:00"}
{"id":"bd-19","title":"Fix import zero-value field handling","description":"Import uses zero-value checks (Priority != 0) to determine field updates. This prevents setting priority to 0 or clearing string fields. Export/import round-trip not fully idempotent for zero values. Consider JSON presence detection or explicit preserve-existing semantics. Location: cmd/bd/import.go:95-106","status":"closed","priority":2,"issue_type":"bug","created_at":"2025-10-12T15:13:17.895083-07:00","updated_at":"2025-10-12T15:21:23.259692-07:00"}
{"id":"bd-2","title":"Add PostgreSQL backend","description":"Implement PostgreSQL storage backend as alternative to SQLite for larger teams","status":"closed","priority":3,"issue_type":"feature","created_at":"2025-10-12T00:43:03.457453-07:00","updated_at":"2025-10-12T15:07:26.333816-07:00","closed_at":"2025-10-12T14:15:04.00695-07:00"}
{"id":"bd-20","title":"Add --strict flag for dependency import failures","description":"Currently dependency import errors are warnings (logged to stderr, execution continues). Missing targets or cycles may indicate JSONL corruption. Add --strict flag to fail on any dependency errors for data integrity validation. Location: cmd/bd/import.go:159-164","status":"open","priority":2,"issue_type":"feature","created_at":"2025-10-12T15:13:18.954834-07:00","updated_at":"2025-10-12T15:13:18.954834-07:00"}
{"id":"bd-21","title":"Simplify getNextID SQL query parameters","description":"Query passes prefix four times to same SQL query. Works but fragile if query changes. Consider simplifying SQL to require fewer parameters. Location: internal/storage/sqlite/sqlite.go:73-78","status":"open","priority":3,"issue_type":"task","created_at":"2025-10-12T15:13:20.114733-07:00","updated_at":"2025-10-12T15:13:20.114733-07:00"}
{"id":"bd-22","title":"Add validation/warning for malformed issue IDs","description":"getNextID silently ignores non-numeric ID suffixes (e.g., bd-foo). CAST returns NULL for invalid strings. Consider detecting and warning about malformed IDs in database. Location: internal/storage/sqlite/sqlite.go:79-82","status":"open","priority":3,"issue_type":"task","created_at":"2025-10-12T15:13:21.195975-07:00","updated_at":"2025-10-12T15:13:21.195975-07:00"}
{"id":"bd-23","title":"Optimize export dependency queries (N+1 problem)","description":"Export triggers separate GetDependencyRecords() per issue. For large DBs (1000+ issues), this is N+1 queries. Add GetAllDependencyRecords() to fetch all dependencies in one query. Location: cmd/bd/export.go:52-59, import.go:138-142","status":"open","priority":3,"issue_type":"task","created_at":"2025-10-12T15:13:22.325113-07:00","updated_at":"2025-10-12T15:13:22.325113-07:00"}
{"id":"bd-20","title":"Add --strict flag for dependency import failures","description":"Currently dependency import errors are warnings (logged to stderr, execution continues). Missing targets or cycles may indicate JSONL corruption. Add --strict flag to fail on any dependency errors for data integrity validation. Location: cmd/bd/import.go:159-164","status":"closed","priority":2,"issue_type":"feature","created_at":"2025-10-12T15:13:18.954834-07:00","updated_at":"2025-10-12T15:22:25.81646-07:00"}
{"id":"bd-21","title":"Simplify getNextID SQL query parameters","description":"Query passes prefix four times to same SQL query. Works but fragile if query changes. Consider simplifying SQL to require fewer parameters. Location: internal/storage/sqlite/sqlite.go:73-78","status":"closed","priority":3,"issue_type":"task","created_at":"2025-10-12T15:13:20.114733-07:00","updated_at":"2025-10-12T15:23:08.014893-07:00"}
{"id":"bd-22","title":"Add validation/warning for malformed issue IDs","description":"getNextID silently ignores non-numeric ID suffixes (e.g., bd-foo). CAST returns NULL for invalid strings. Consider detecting and warning about malformed IDs in database. Location: internal/storage/sqlite/sqlite.go:79-82","status":"closed","priority":3,"issue_type":"task","created_at":"2025-10-12T15:13:21.195975-07:00","updated_at":"2025-10-12T15:23:57.434894-07:00"}
{"id":"bd-23","title":"Optimize export dependency queries (N+1 problem)","description":"Export triggers separate GetDependencyRecords() per issue. For large DBs (1000+ issues), this is N+1 queries. Add GetAllDependencyRecords() to fetch all dependencies in one query. Location: cmd/bd/export.go:52-59, import.go:138-142","status":"closed","priority":3,"issue_type":"task","created_at":"2025-10-12T15:13:22.325113-07:00","updated_at":"2025-10-12T15:25:26.278357-07:00"}
{"id":"bd-3","title":"Document git workflow in README","description":"Add Git Workflow section to README explaining binary vs text approaches","status":"closed","priority":1,"issue_type":"chore","created_at":"2025-10-12T00:43:03.461615-07:00","updated_at":"2025-10-12T15:07:26.333908-07:00","closed_at":"2025-10-12T00:43:30.283178-07:00"}
{"id":"bd-4","title":"Add demo GIF/video showing bd quickstart in action","description":"Record asciinema or create animated GIF showing the full workflow","status":"open","priority":2,"issue_type":"feature","created_at":"2025-10-12T10:50:49.500051-07:00","updated_at":"2025-10-12T15:07:26.334-07:00","dependencies":[{"issue_id":"bd-4","depends_on_id":"bd-8","type":"parent-child","created_at":"2025-10-12T10:51:08.399915-07:00","created_by":"stevey"}]}
{"id":"bd-5","title":"Implement MCP server for Claude Desktop","description":"Complete the claude-desktop-mcp example with working TypeScript implementation","status":"open","priority":1,"issue_type":"feature","created_at":"2025-10-12T10:50:50.942964-07:00","updated_at":"2025-10-12T15:07:26.334092-07:00","dependencies":[{"issue_id":"bd-5","depends_on_id":"bd-8","type":"parent-child","created_at":"2025-10-12T10:51:08.404381-07:00","created_by":"stevey"}]}

10
cmd/bd/export.go
View File

@@ -48,14 +48,14 @@ Output to stdout by default, or use -o flag for file output.`,
return issues[i].ID < issues[j].ID
})
// Populate dependencies for each issue
for _, issue := range issues {
deps, err := store.GetDependencyRecords(ctx, issue.ID)
// Populate dependencies for all issues in one query (avoids N+1 problem)
allDeps, err := store.GetAllDependencyRecords(ctx)
if err != nil {
fmt.Fprintf(os.Stderr, "Error getting dependencies for %s: %v\n", issue.ID, err)
fmt.Fprintf(os.Stderr, "Error getting dependencies: %v\n", err)
os.Exit(1)
}
issue.Dependencies = deps
for _, issue := range issues {
issue.Dependencies = allDeps[issue.ID]
}
// Open output

47
cmd/bd/import.go
View File

@@ -25,6 +25,7 @@ Behavior:
Run: func(cmd *cobra.Command, args []string) {
input, _ := cmd.Flags().GetString("input")
skipUpdate, _ := cmd.Flags().GetBool("skip-existing")
strict, _ := cmd.Flags().GetBool("strict")
// Open input
in := os.Stdin
@@ -59,7 +60,14 @@ Behavior:
continue
}
// Parse JSON
// Parse JSON - first into a map to detect which fields are present
var rawData map[string]interface{}
if err := json.Unmarshal([]byte(line), &rawData); err != nil {
fmt.Fprintf(os.Stderr, "Error parsing line %d: %v\n", lineNum, err)
os.Exit(1)
}
// Then parse into the Issue struct
var issue types.Issue
if err := json.Unmarshal([]byte(line), &issue); err != nil {
fmt.Fprintf(os.Stderr, "Error parsing line %d: %v\n", lineNum, err)
@@ -81,28 +89,41 @@ Behavior:
skipped++
continue
}
// Update existing issue - convert to updates map
// Update existing issue - only update fields that are present in JSON
updates := make(map[string]interface{})
if issue.Title != "" {
if _, ok := rawData["title"]; ok {
updates["title"] = issue.Title
}
if issue.Description != "" {
if _, ok := rawData["description"]; ok {
updates["description"] = issue.Description
}
if issue.Status != "" {
if _, ok := rawData["design"]; ok {
updates["design"] = issue.Design
}
if _, ok := rawData["acceptance_criteria"]; ok {
updates["acceptance_criteria"] = issue.AcceptanceCriteria
}
if _, ok := rawData["notes"]; ok {
updates["notes"] = issue.Notes
}
if _, ok := rawData["status"]; ok {
updates["status"] = issue.Status
}
if issue.Priority != 0 {
if _, ok := rawData["priority"]; ok {
updates["priority"] = issue.Priority
}
if issue.IssueType != "" {
if _, ok := rawData["issue_type"]; ok {
updates["issue_type"] = issue.IssueType
}
if issue.Assignee != "" {
if _, ok := rawData["assignee"]; ok {
updates["assignee"] = issue.Assignee
}
if _, ok := rawData["estimated_minutes"]; ok {
if issue.EstimatedMinutes != nil {
updates["estimated_minutes"] = *issue.EstimatedMinutes
} else {
updates["estimated_minutes"] = nil
}
}
if err := store.UpdateIssue(ctx, issue.ID, updates, "import"); err != nil {
@@ -157,7 +178,14 @@ Behavior:
// Add dependency
if err := store.AddDependency(ctx, dep, "import"); err != nil {
// Ignore errors for missing target issues or cycles
if strict {
// In strict mode, fail on any dependency error
fmt.Fprintf(os.Stderr, "Error: could not add dependency %s → %s: %v\n",
dep.IssueID, dep.DependsOnID, err)
fmt.Fprintf(os.Stderr, "Use --strict=false to treat dependency errors as warnings\n")
os.Exit(1)
}
// In non-strict mode, ignore errors for missing target issues or cycles
// This can happen if dependencies reference issues not in the import
fmt.Fprintf(os.Stderr, "Warning: could not add dependency %s → %s: %v\n",
dep.IssueID, dep.DependsOnID, err)
@@ -185,5 +213,6 @@ Behavior:
func init() {
importCmd.Flags().StringP("input", "i", "", "Input file (default: stdin)")
importCmd.Flags().BoolP("skip-existing", "s", false, "Skip existing issues instead of updating them")
importCmd.Flags().Bool("strict", false, "Fail on dependency errors instead of treating them as warnings")
rootCmd.AddCommand(importCmd)
}

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

@@ -210,6 +210,39 @@ func (s *SQLiteStorage) GetDependencyRecords(ctx context.Context, issueID string
return deps, nil
}
// GetAllDependencyRecords returns all dependency records grouped by issue ID
// This is optimized for bulk export operations to avoid N+1 queries
func (s *SQLiteStorage) GetAllDependencyRecords(ctx context.Context) (map[string][]*types.Dependency, error) {
rows, err := s.db.QueryContext(ctx, `
SELECT issue_id, depends_on_id, type, created_at, created_by
FROM dependencies
ORDER BY issue_id, created_at ASC
`)
if err != nil {
return nil, fmt.Errorf("failed to get all dependency records: %w", err)
}
defer rows.Close()
// Group dependencies by issue ID
depsMap := make(map[string][]*types.Dependency)
for rows.Next() {
var dep types.Dependency
err := rows.Scan(
&dep.IssueID,
&dep.DependsOnID,
&dep.Type,
&dep.CreatedAt,
&dep.CreatedBy,
)
if err != nil {
return nil, fmt.Errorf("failed to scan dependency: %w", err)
}
depsMap[dep.IssueID] = append(depsMap[dep.IssueID], &dep)
}
return depsMap, nil
}
// GetDependencyTree returns the full dependency tree
func (s *SQLiteStorage) GetDependencyTree(ctx context.Context, issueID string, maxDepth int) ([]*types.TreeNode, error) {
if maxDepth <= 0 {

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

@@ -74,13 +74,36 @@ func getNextID(db *sql.DB) int {
SELECT MAX(CAST(SUBSTR(id, LENGTH(?) + 2) AS INTEGER))
FROM issues
WHERE id LIKE ? || '-%'
AND SUBSTR(id, 1, LENGTH(?)) = ?
`
err = db.QueryRow(query, prefix, prefix, prefix, prefix).Scan(&maxNum)
err = db.QueryRow(query, prefix, prefix).Scan(&maxNum)
if err != nil || !maxNum.Valid {
return 1 // Start from 1 if table is empty or no matching IDs
}
// Check for malformed IDs (non-numeric suffixes) and warn
// These are silently ignored by CAST but indicate data quality issues
malformedQuery := `
SELECT id FROM issues
WHERE id LIKE ? || '-%'
AND CAST(SUBSTR(id, LENGTH(?) + 2) AS INTEGER) IS NULL
`
rows, err := db.Query(malformedQuery, prefix, prefix)
if err == nil {
defer rows.Close()
var malformedIDs []string
for rows.Next() {
var id string
if err := rows.Scan(&id); err == nil {
malformedIDs = append(malformedIDs, id)
}
}
if len(malformedIDs) > 0 {
fmt.Fprintf(os.Stderr, "Warning: Found %d malformed issue IDs with non-numeric suffixes: %v\n",
len(malformedIDs), malformedIDs)
fmt.Fprintf(os.Stderr, "These IDs are being ignored for ID generation. Consider fixing them.\n")
}
}
return int(maxNum.Int64) + 1
}

1
internal/storage/storage.go
View File

@@ -22,6 +22,7 @@ type Storage interface {
GetDependencies(ctx context.Context, issueID string) ([]*types.Issue, error)
GetDependents(ctx context.Context, issueID string) ([]*types.Issue, error)
GetDependencyRecords(ctx context.Context, issueID string) ([]*types.Dependency, error)
GetAllDependencyRecords(ctx context.Context) (map[string][]*types.Dependency, error)
GetDependencyTree(ctx context.Context, issueID string, maxDepth int) ([]*types.TreeNode, error)
DetectCycles(ctx context.Context) ([][]*types.Issue, error)