From d1c4526e6eaf54c70ce5ac5274212268096bc1e3 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Wed, 24 Dec 2025 13:50:19 -0800 Subject: [PATCH] fix(formula): address code review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes from self-review of formula parser and bd cook: 1. Atomicity: Add cleanup on failure in cookFormula - if labels/deps transaction fails, delete the already-created issues 2. Validation: Add recursive depends_on validation for child steps and validate priority ranges for children 3. Documentation: Mark unimplemented fields (Expand, ExpandVars, Condition, Gate) with TODO(future) comments 4. Thread safety: Add note that Parser is NOT thread-safe 5. Error messages: Track first definition location for duplicate ID errors (e.g., "duplicate id at steps[1], first defined at steps[0]") 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- cmd/bd/cook.go | 15 +++++++ internal/formula/parser.go | 4 ++ internal/formula/parser_test.go | 46 +++++++++++++++++++++ internal/formula/types.go | 73 ++++++++++++++++++++++++--------- 4 files changed, 118 insertions(+), 20 deletions(-) diff --git a/cmd/bd/cook.go b/cmd/bd/cook.go index 9a7de2fb..25bd0342 100644 --- a/cmd/bd/cook.go +++ b/cmd/bd/cook.go @@ -243,6 +243,9 @@ func cookFormula(ctx context.Context, s storage.Storage, f *formula.Formula) (*c return nil, fmt.Errorf("failed to create issues: %w", err) } + // Track if we need cleanup on failure + issuesCreated := true + // Add labels and dependencies in a transaction err := s.RunInTransaction(ctx, func(tx storage.Transaction) error { // Add labels @@ -263,6 +266,18 @@ func cookFormula(ctx context.Context, s storage.Storage, f *formula.Formula) (*c }) if err != nil { + // Clean up: delete the issues we created since labels/deps failed + if issuesCreated { + cleanupErr := s.RunInTransaction(ctx, func(tx storage.Transaction) error { + for i := len(issues) - 1; i >= 0; i-- { + _ = tx.DeleteIssue(ctx, issues[i].ID) // Best effort cleanup + } + return nil + }) + if cleanupErr != nil { + return nil, fmt.Errorf("%w (cleanup also failed: %v)", err, cleanupErr) + } + } return nil, err } diff --git a/internal/formula/parser.go b/internal/formula/parser.go index e94b990a..a1381017 100644 --- a/internal/formula/parser.go +++ b/internal/formula/parser.go @@ -14,6 +14,10 @@ import ( const FormulaExt = ".formula.yaml" // Parser handles loading and resolving formulas. +// +// NOTE: Parser is NOT thread-safe. Create a new Parser per goroutine or +// synchronize access externally. The cache and resolving maps have no +// internal synchronization. type Parser struct { // searchPaths are directories to search for formulas (in order). searchPaths []string diff --git a/internal/formula/parser_test.go b/internal/formula/parser_test.go index 92a8378d..4a9d170c 100644 --- a/internal/formula/parser_test.go +++ b/internal/formula/parser_test.go @@ -198,6 +198,52 @@ func TestValidate_ChildSteps(t *testing.T) { } } +func TestValidate_ChildStepsInvalidDependsOn(t *testing.T) { + formula := &Formula{ + Formula: "mol-bad-child-dep", + Version: 1, + Type: TypeWorkflow, + Steps: []*Step{ + { + ID: "epic1", + Title: "Epic 1", + Children: []*Step{ + {ID: "child1", Title: "Child 1"}, + {ID: "child2", Title: "Child 2", DependsOn: []string{"nonexistent"}}, + }, + }, + }, + } + + err := formula.Validate() + if err == nil { + t.Error("Validate should fail for child depends_on referencing unknown step") + } +} + +func TestValidate_ChildStepsInvalidPriority(t *testing.T) { + p := 10 // invalid + formula := &Formula{ + Formula: "mol-bad-child-priority", + Version: 1, + Type: TypeWorkflow, + Steps: []*Step{ + { + ID: "epic1", + Title: "Epic 1", + Children: []*Step{ + {ID: "child1", Title: "Child 1", Priority: &p}, + }, + }, + }, + } + + err := formula.Validate() + if err == nil { + t.Error("Validate should fail for child with invalid priority") + } +} + func TestValidate_BondPoints(t *testing.T) { formula := &Formula{ Formula: "mol-compose", diff --git a/internal/formula/types.go b/internal/formula/types.go index b918d5b5..8591f0cc 100644 --- a/internal/formula/types.go +++ b/internal/formula/types.go @@ -139,23 +139,28 @@ type Step struct { // Expand references an expansion formula to inline here. // When set, this step is replaced by the expansion's steps. + // TODO(future): Not yet implemented in bd cook. Filed as future work. Expand string `yaml:"expand,omitempty" json:"expand,omitempty"` // ExpandVars are variable overrides for the expansion. + // TODO(future): Not yet implemented in bd cook. Filed as future work. ExpandVars map[string]string `yaml:"expand_vars,omitempty" json:"expand_vars,omitempty"` // Condition makes this step optional based on a variable. // Format: "{{var}}" (truthy) or "{{var}} == value". + // TODO(future): Not yet implemented in bd cook. Filed as future work. Condition string `yaml:"condition,omitempty" json:"condition,omitempty"` // Children are nested steps (for creating epic hierarchies). Children []*Step `yaml:"children,omitempty" json:"children,omitempty"` // Gate defines an async wait condition for this step. + // TODO(future): Not yet implemented in bd cook. Will integrate with bd-udsi gates. Gate *Gate `yaml:"gate,omitempty" json:"gate,omitempty"` } // Gate defines an async wait condition (integrates with bd-udsi). +// TODO(future): Not yet implemented in bd cook. Schema defined for future use. type Gate struct { // Type is the condition type: gh:run, gh:pr, timer, human, mail. Type string `yaml:"type" json:"type"` @@ -239,38 +244,42 @@ func (f *Formula) Validate() error { } } - // Validate steps - stepIDs := make(map[string]bool) + // Validate steps - track where each ID was first defined for better error messages + stepIDLocations := make(map[string]string) // ID -> location where first defined for i, step := range f.Steps { + prefix := fmt.Sprintf("steps[%d]", i) if step.ID == "" { - errs = append(errs, fmt.Sprintf("steps[%d]: id is required", i)) + errs = append(errs, fmt.Sprintf("%s: id is required", prefix)) continue } - if stepIDs[step.ID] { - errs = append(errs, fmt.Sprintf("steps[%d]: duplicate id %q", i, step.ID)) + if firstLoc, exists := stepIDLocations[step.ID]; exists { + errs = append(errs, fmt.Sprintf("%s: duplicate id %q (first defined at %s)", prefix, step.ID, firstLoc)) + } else { + stepIDLocations[step.ID] = prefix } - stepIDs[step.ID] = true if step.Title == "" && step.Expand == "" { - errs = append(errs, fmt.Sprintf("steps[%d] (%s): title is required (unless using expand)", i, step.ID)) + errs = append(errs, fmt.Sprintf("%s (%s): title is required (unless using expand)", prefix, step.ID)) } // Validate priority range if step.Priority != nil && (*step.Priority < 0 || *step.Priority > 4) { - errs = append(errs, fmt.Sprintf("steps[%d] (%s): priority must be 0-4", i, step.ID)) + errs = append(errs, fmt.Sprintf("%s (%s): priority must be 0-4", prefix, step.ID)) } // Collect child IDs (for dependency validation) - collectChildIDs(step.Children, stepIDs, &errs, fmt.Sprintf("steps[%d]", i)) + collectChildIDs(step.Children, stepIDLocations, &errs, prefix) } - // Validate step dependencies reference valid IDs + // Validate step dependencies reference valid IDs (including children) for i, step := range f.Steps { for _, dep := range step.DependsOn { - if !stepIDs[dep] { + if _, exists := stepIDLocations[dep]; !exists { errs = append(errs, fmt.Sprintf("steps[%d] (%s): depends_on references unknown step %q", i, step.ID, dep)) } } + // Validate children's depends_on recursively + validateChildDependsOn(step.Children, stepIDLocations, &errs, fmt.Sprintf("steps[%d]", i)) } // Validate compose rules @@ -282,11 +291,15 @@ func (f *Formula) Validate() error { if bp.AfterStep != "" && bp.BeforeStep != "" { errs = append(errs, fmt.Sprintf("compose.bond_points[%d] (%s): cannot have both after_step and before_step", i, bp.ID)) } - if bp.AfterStep != "" && !stepIDs[bp.AfterStep] { - errs = append(errs, fmt.Sprintf("compose.bond_points[%d] (%s): after_step references unknown step %q", i, bp.ID, bp.AfterStep)) + if bp.AfterStep != "" { + if _, exists := stepIDLocations[bp.AfterStep]; !exists { + errs = append(errs, fmt.Sprintf("compose.bond_points[%d] (%s): after_step references unknown step %q", i, bp.ID, bp.AfterStep)) + } } - if bp.BeforeStep != "" && !stepIDs[bp.BeforeStep] { - errs = append(errs, fmt.Sprintf("compose.bond_points[%d] (%s): before_step references unknown step %q", i, bp.ID, bp.BeforeStep)) + if bp.BeforeStep != "" { + if _, exists := stepIDLocations[bp.BeforeStep]; !exists { + errs = append(errs, fmt.Sprintf("compose.bond_points[%d] (%s): before_step references unknown step %q", i, bp.ID, bp.BeforeStep)) + } } } @@ -308,23 +321,43 @@ func (f *Formula) Validate() error { } // collectChildIDs recursively collects step IDs from children. -func collectChildIDs(children []*Step, ids map[string]bool, errs *[]string, prefix string) { +// idLocations maps ID -> location where first defined (for better duplicate error messages). +func collectChildIDs(children []*Step, idLocations map[string]string, errs *[]string, prefix string) { for i, child := range children { childPrefix := fmt.Sprintf("%s.children[%d]", prefix, i) if child.ID == "" { *errs = append(*errs, fmt.Sprintf("%s: id is required", childPrefix)) continue } - if ids[child.ID] { - *errs = append(*errs, fmt.Sprintf("%s: duplicate id %q", childPrefix, child.ID)) + if firstLoc, exists := idLocations[child.ID]; exists { + *errs = append(*errs, fmt.Sprintf("%s: duplicate id %q (first defined at %s)", childPrefix, child.ID, firstLoc)) + } else { + idLocations[child.ID] = childPrefix } - ids[child.ID] = true if child.Title == "" && child.Expand == "" { *errs = append(*errs, fmt.Sprintf("%s (%s): title is required", childPrefix, child.ID)) } - collectChildIDs(child.Children, ids, errs, childPrefix) + // Validate priority range for children + if child.Priority != nil && (*child.Priority < 0 || *child.Priority > 4) { + *errs = append(*errs, fmt.Sprintf("%s (%s): priority must be 0-4", childPrefix, child.ID)) + } + + collectChildIDs(child.Children, idLocations, errs, childPrefix) + } +} + +// validateChildDependsOn recursively validates depends_on references for children. +func validateChildDependsOn(children []*Step, idLocations map[string]string, errs *[]string, prefix string) { + for i, child := range children { + childPrefix := fmt.Sprintf("%s.children[%d]", prefix, i) + for _, dep := range child.DependsOn { + if _, exists := idLocations[dep]; !exists { + *errs = append(*errs, fmt.Sprintf("%s (%s): depends_on references unknown step %q", childPrefix, child.ID, dep)) + } + } + validateChildDependsOn(child.Children, idLocations, errs, childPrefix) } }