From 6716395853e764ec0f1577e19ebebd13e6ddeee7 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Wed, 17 Dec 2025 22:44:23 -0800 Subject: [PATCH] Revert "fix: Address workflow template issues from code review" This reverts commit aae8407aa4880bd6b8969a5c30fe7d46a5970b27. --- cmd/bd/show.go | 46 +----- cmd/bd/templates/workflows/version-bump.yaml | 2 +- cmd/bd/workflow.go | 146 +++---------------- internal/rpc/protocol.go | 3 +- internal/rpc/server_issues_epics.go | 40 +---- 5 files changed, 27 insertions(+), 210 deletions(-) diff --git a/cmd/bd/show.go b/cmd/bd/show.go index 89baaf1b..b98312dc 100644 --- a/cmd/bd/show.go +++ b/cmd/bd/show.go @@ -554,12 +554,6 @@ var updateCmd = &cobra.Command{ } updates["issue_type"] = issueType } - parentChanged := cmd.Flags().Changed("parent") - var newParent string - if parentChanged { - newParent, _ = cmd.Flags().GetString("parent") - updates["parent"] = newParent - } if len(updates) == 0 { fmt.Println("No updates specified") @@ -646,9 +640,6 @@ var updateCmd = &cobra.Command{ if issueType, ok := updates["issue_type"].(string); ok { updateArgs.IssueType = &issueType } - if parent, ok := updates["parent"].(string); ok { - updateArgs.Parent = &parent - } resp, err := daemonClient.Update(updateArgs) if err != nil { @@ -684,7 +675,7 @@ var updateCmd = &cobra.Command{ // Apply regular field updates if any regularUpdates := make(map[string]interface{}) for k, v := range updates { - if k != "add_labels" && k != "remove_labels" && k != "set_labels" && k != "parent" { + if k != "add_labels" && k != "remove_labels" && k != "set_labels" { regularUpdates[k] = v } } @@ -740,40 +731,6 @@ var updateCmd = &cobra.Command{ } } - // Handle parent change - if newParent, ok := updates["parent"].(string); ok { - // Remove existing parent-child dependencies - deps, err := store.GetDependencyRecords(ctx, id) - if err != nil { - fmt.Fprintf(os.Stderr, "Error getting dependencies for %s: %v\n", id, err) - continue - } - for _, dep := range deps { - if dep.Type == types.DepParentChild { - if err := store.RemoveDependency(ctx, id, dep.DependsOnID, actor); err != nil { - fmt.Fprintf(os.Stderr, "Error removing old parent from %s: %v\n", id, err) - } - } - } - // Add new parent if specified - if newParent != "" { - // Resolve partial ID for new parent - resolvedParent, err := utils.ResolvePartialID(ctx, store, newParent) - if err != nil { - fmt.Fprintf(os.Stderr, "Error resolving parent ID %s: %v\n", newParent, err) - continue - } - dep := &types.Dependency{ - IssueID: id, - DependsOnID: resolvedParent, - Type: types.DepParentChild, - } - if err := store.AddDependency(ctx, dep, actor); err != nil { - fmt.Fprintf(os.Stderr, "Error setting new parent for %s: %v\n", id, err) - } - } - } - // Run update hook (bd-kwro.8) issue, _ := store.GetIssue(ctx, id) if issue != nil && hookRunner != nil { @@ -1296,7 +1253,6 @@ func init() { updateCmd.Flags().StringSlice("add-label", nil, "Add labels (repeatable)") updateCmd.Flags().StringSlice("remove-label", nil, "Remove labels (repeatable)") updateCmd.Flags().StringSlice("set-labels", nil, "Set labels, replacing all existing (repeatable)") - updateCmd.Flags().String("parent", "", "New parent issue ID (use empty string to remove parent)") updateCmd.Flags().Bool("json", false, "Output JSON format") rootCmd.AddCommand(updateCmd) diff --git a/cmd/bd/templates/workflows/version-bump.yaml b/cmd/bd/templates/workflows/version-bump.yaml index 33eb7e79..e18ffd45 100644 --- a/cmd/bd/templates/workflows/version-bump.yaml +++ b/cmd/bd/templates/workflows/version-bump.yaml @@ -355,7 +355,7 @@ tasks: depends_on: - local-go-install verification: - command: "grep -q 'bd-hooks-version: {{version}}' .git/hooks/pre-commit" + command: "grep -q 'bd-hooks-version: {{version}}' .git/hooks/pre-commit 2>/dev/null || echo 'Hooks may not be installed - verify manually'" - id: final-verification title: "Final release verification" diff --git a/cmd/bd/workflow.go b/cmd/bd/workflow.go index 23ca12bb..5b96d72c 100644 --- a/cmd/bd/workflow.go +++ b/cmd/bd/workflow.go @@ -1,7 +1,6 @@ package main import ( - "context" "embed" "encoding/json" "fmt" @@ -15,9 +14,7 @@ import ( "github.com/fatih/color" "github.com/spf13/cobra" "github.com/steveyegge/beads/internal/rpc" - "github.com/steveyegge/beads/internal/storage" "github.com/steveyegge/beads/internal/types" - "github.com/steveyegge/beads/internal/utils" "gopkg.in/yaml.v3" ) @@ -203,11 +200,6 @@ Use 'bd ready' to see which tasks are ready to work on.`, FatalError("%v", err) } - // Validate workflow has at least one task - if len(wf.Tasks) == 0 { - FatalError("workflow template '%s' has no tasks defined", templateName) - } - // Parse variable flags varFlags, _ := cmd.Flags().GetStringSlice("var") vars := make(map[string]string) @@ -536,81 +528,36 @@ status is not automatically changed - use 'bd close' to mark complete.`, dim := color.New(color.Faint).SprintFunc() // Look for verification in task description - // Format: ```verify\ncommand\nexpect_exit: N\nexpect_stdout: pattern\n``` - verify := extractVerification(task.Description) - if verify == nil || verify.Command == "" { + // Format: ```verify\ncommand\n``` + verifyCmd := extractVerifyCommand(task.Description) + if verifyCmd == "" { fmt.Printf("No verification command found for task: %s\n", blue(taskID)) fmt.Printf("Add a verification block to the task description:\n") fmt.Printf(" %s\n", dim("```verify")) fmt.Printf(" %s\n", dim("./scripts/check-versions.sh")) - fmt.Printf(" %s\n", dim("expect_exit: 0")) - fmt.Printf(" %s\n", dim("expect_stdout: success")) fmt.Printf(" %s\n", dim("```")) return } fmt.Printf("Running verification for: %s \"%s\"\n", blue(taskID), task.Title) - fmt.Printf("Command: %s\n", dim(verify.Command)) - if verify.ExpectExit != nil { - fmt.Printf("Expected exit: %d\n", *verify.ExpectExit) - } - if verify.ExpectStdout != "" { - fmt.Printf("Expected stdout: %s\n", verify.ExpectStdout) - } - fmt.Println() + fmt.Printf("Command: %s\n\n", dim(verifyCmd)) - // Run the command, capturing stdout if we need to check it - execCmd := exec.Command("sh", "-c", verify.Command) - var stdout strings.Builder - if verify.ExpectStdout != "" { - // Capture stdout for pattern matching, but also display it - execCmd.Stdout = &stdout - } else { - execCmd.Stdout = os.Stdout - } + // Run the command + execCmd := exec.Command("sh", "-c", verifyCmd) + execCmd.Stdout = os.Stdout execCmd.Stderr = os.Stderr err := execCmd.Run() - // Display captured stdout - if verify.ExpectStdout != "" { - fmt.Print(stdout.String()) - } - fmt.Println() - - // Check exit code - actualExit := 0 if err != nil { if exitErr, ok := err.(*exec.ExitError); ok { - actualExit = exitErr.ExitCode() + fmt.Printf("%s Verification failed (exit code: %d)\n", red("✗"), exitErr.ExitCode()) } else { fmt.Printf("%s Verification failed: %v\n", red("✗"), err) - os.Exit(1) } - } - - // Check expected exit code - if verify.ExpectExit != nil { - if actualExit != *verify.ExpectExit { - fmt.Printf("%s Verification failed: expected exit code %d, got %d\n", - red("✗"), *verify.ExpectExit, actualExit) - os.Exit(1) - } - } else if actualExit != 0 { - // Default: expect exit code 0 - fmt.Printf("%s Verification failed (exit code: %d)\n", red("✗"), actualExit) os.Exit(1) } - // Check expected stdout pattern - if verify.ExpectStdout != "" { - if !strings.Contains(stdout.String(), verify.ExpectStdout) { - fmt.Printf("%s Verification failed: stdout does not contain %q\n", - red("✗"), verify.ExpectStdout) - os.Exit(1) - } - } - fmt.Printf("%s Verification passed\n", green("✓")) fmt.Printf("\nTo close this task: %s\n", blue("bd close "+taskID)) }, @@ -796,8 +743,9 @@ func createWorkflowInstance(wf *types.WorkflowTemplate, vars map[string]string) } // Add verification block to description if present - if verifyBlock := buildVerificationBlock(task.Verification, vars); verifyBlock != "" { - taskDesc += "\n\n" + verifyBlock + if task.Verification != nil && task.Verification.Command != "" { + verifyCmd := substituteVars(task.Verification.Command, vars) + taskDesc += "\n\n```verify\n" + verifyCmd + "\n```" } taskLabels := []string{"workflow"} @@ -830,7 +778,6 @@ func createWorkflowInstance(wf *types.WorkflowTemplate, vars map[string]string) for _, depName := range task.DependsOn { depID := instance.TaskMap[depName] if depID == "" { - fmt.Fprintf(os.Stderr, "Warning: task '%s' depends_on references unknown task ID '%s'\n", task.ID, depName) continue } depArgs := &rpc.DepAddArgs{ @@ -887,8 +834,9 @@ func createWorkflowInstance(wf *types.WorkflowTemplate, vars map[string]string) } // Add verification block to description if present - if verifyBlock := buildVerificationBlock(task.Verification, vars); verifyBlock != "" { - taskDesc += "\n\n" + verifyBlock + if task.Verification != nil && task.Verification.Command != "" { + verifyCmd := substituteVars(task.Verification.Command, vars) + taskDesc += "\n\n```verify\n" + verifyCmd + "\n```" } // Get next child ID @@ -935,7 +883,6 @@ func createWorkflowInstance(wf *types.WorkflowTemplate, vars map[string]string) for _, depName := range task.DependsOn { depID := instance.TaskMap[depName] if depID == "" { - fmt.Fprintf(os.Stderr, "Warning: task '%s' depends_on references unknown task ID '%s'\n", task.ID, depName) continue } dep := &types.Dependency{ @@ -955,71 +902,24 @@ func createWorkflowInstance(wf *types.WorkflowTemplate, vars map[string]string) return instance, nil } -// parsedVerification holds verification data extracted from task description -type parsedVerification struct { - Command string - ExpectExit *int - ExpectStdout string -} - -// extractVerification extracts verification data from task description -// Looks for ```verify\ncommand\nexpect_exit: N\nexpect_stdout: pattern\n``` block -func extractVerification(description string) *parsedVerification { +// extractVerifyCommand extracts a verify command from task description +// Looks for ```verify\ncommand\n``` block +func extractVerifyCommand(description string) string { start := strings.Index(description, "```verify\n") if start == -1 { - return nil + return "" } start += len("```verify\n") end := strings.Index(description[start:], "\n```") if end == -1 { - return nil - } - content := description[start : start+end] - - result := &parsedVerification{} - lines := strings.Split(content, "\n") - - // First line is always the command - if len(lines) > 0 { - result.Command = strings.TrimSpace(lines[0]) - } - - // Remaining lines are optional metadata - for _, line := range lines[1:] { - line = strings.TrimSpace(line) - if strings.HasPrefix(line, "expect_exit:") { - valStr := strings.TrimSpace(strings.TrimPrefix(line, "expect_exit:")) - if val, err := fmt.Sscanf(valStr, "%d", new(int)); err == nil && val == 1 { - exitCode := 0 - fmt.Sscanf(valStr, "%d", &exitCode) - result.ExpectExit = &exitCode - } - } else if strings.HasPrefix(line, "expect_stdout:") { - result.ExpectStdout = strings.TrimSpace(strings.TrimPrefix(line, "expect_stdout:")) - } - } - - return result -} - -// buildVerificationBlock creates a verify block string from Verification data -func buildVerificationBlock(v *types.Verification, vars map[string]string) string { - if v == nil || v.Command == "" { return "" } - verifyCmd := substituteVars(v.Command, vars) - block := "```verify\n" + verifyCmd - if v.ExpectExit != nil { - block += fmt.Sprintf("\nexpect_exit: %d", *v.ExpectExit) - } - if v.ExpectStdout != "" { - block += "\nexpect_stdout: " + substituteVars(v.ExpectStdout, vars) - } - block += "\n```" - return block + return strings.TrimSpace(description[start : start+end]) } // resolvePartialID resolves a partial ID to a full ID (for direct mode) -func resolvePartialID(ctx context.Context, s storage.Storage, id string) (string, error) { - return utils.ResolvePartialID(ctx, s, id) +func resolvePartialID(ctx interface{}, s interface{}, id string) (string, error) { + // This is a simplified version - the real implementation is in utils package + // For now, just return the ID as-is if it looks complete + return id, nil } diff --git a/internal/rpc/protocol.go b/internal/rpc/protocol.go index 6f496a9e..55591ed2 100644 --- a/internal/rpc/protocol.go +++ b/internal/rpc/protocol.go @@ -89,10 +89,9 @@ type UpdateArgs struct { AcceptanceCriteria *string `json:"acceptance_criteria,omitempty"` Notes *string `json:"notes,omitempty"` Assignee *string `json:"assignee,omitempty"` - ExternalRef *string `json:"external_ref,omitempty"` // Link to external issue trackers + ExternalRef *string `json:"external_ref,omitempty"` // Link to external issue trackers EstimatedMinutes *int `json:"estimated_minutes,omitempty"` // Time estimate in minutes IssueType *string `json:"issue_type,omitempty"` // Issue type (bug|feature|task|epic|chore) - Parent *string `json:"parent,omitempty"` // New parent issue ID (empty string to remove parent) AddLabels []string `json:"add_labels,omitempty"` RemoveLabels []string `json:"remove_labels,omitempty"` SetLabels []string `json:"set_labels,omitempty"` diff --git a/internal/rpc/server_issues_epics.go b/internal/rpc/server_issues_epics.go index 107acc5d..3bcbca3b 100644 --- a/internal/rpc/server_issues_epics.go +++ b/internal/rpc/server_issues_epics.go @@ -371,46 +371,8 @@ func (s *Server) handleUpdate(req *Request) Response { } } - // Handle parent change - parentChanged := false - if updateArgs.Parent != nil { - parentChanged = true - // First, remove any existing parent-child dependency - deps, err := store.GetDependencyRecords(ctx, updateArgs.ID) - if err != nil { - return Response{ - Success: false, - Error: fmt.Sprintf("failed to get dependencies: %v", err), - } - } - for _, dep := range deps { - if dep.Type == types.DepParentChild { - if err := store.RemoveDependency(ctx, updateArgs.ID, dep.DependsOnID, actor); err != nil { - return Response{ - Success: false, - Error: fmt.Sprintf("failed to remove old parent: %v", err), - } - } - } - } - // Add new parent if specified (empty string means just remove parent) - if *updateArgs.Parent != "" { - newDep := &types.Dependency{ - IssueID: updateArgs.ID, - DependsOnID: *updateArgs.Parent, - Type: types.DepParentChild, - } - if err := store.AddDependency(ctx, newDep, actor); err != nil { - return Response{ - Success: false, - Error: fmt.Sprintf("failed to set new parent: %v", err), - } - } - } - } - // Emit mutation event for event-driven daemon (only if any updates or label operations were performed) - if len(updates) > 0 || len(updateArgs.SetLabels) > 0 || len(updateArgs.AddLabels) > 0 || len(updateArgs.RemoveLabels) > 0 || parentChanged { + if len(updates) > 0 || len(updateArgs.SetLabels) > 0 || len(updateArgs.AddLabels) > 0 || len(updateArgs.RemoveLabels) > 0 { s.emitMutation(MutationUpdate, updateArgs.ID) }