diff --git a/docs/src/content/docs/tools/cli.md b/docs/src/content/docs/tools/cli.md index cc9481cfc7a..d91cf2f2988 100644 --- a/docs/src/content/docs/tools/cli.md +++ b/docs/src/content/docs/tools/cli.md @@ -131,9 +131,12 @@ The update command intelligently determines how to update based on the current r The update process: 1. Parses the source field to extract repository, path, and current ref 2. Resolves the latest compatible version/commit based on the ref type -3. Downloads the updated workflow content from GitHub -4. Performs a 3-way merge, preserving the source field with the new ref -5. Automatically recompiles the updated workflow +3. Downloads the base version (original from source) and new version from GitHub +4. Performs a 3-way merge using `git merge-file` to intelligently combine changes: + - Preserves both local modifications and upstream improvements when possible + - Detects conflicts when both versions modify the same content + - Uses diff3-style conflict markers for manual resolution when needed +5. Automatically recompiles the updated workflow (skips compilation if conflicts exist) **Source Field Format:** @@ -147,6 +150,36 @@ Examples: - `githubnext/agentics/workflows/ci-doctor.md@main` (branch) - `githubnext/agentics/workflows/ci-doctor.md` (no ref, uses default branch) +**Merge Behavior and Conflict Resolution:** + +The update command uses a 3-way merge algorithm (via `git merge-file`) to intelligently combine changes: + +- **Clean Merge**: When local and upstream changes don't overlap, both are automatically preserved + - Example: Local adds markdown section, upstream adds frontmatter field → both included + +- **Conflicts**: When both versions modify the same content, conflict markers are added: + ```yaml + <<<<<<< current (local changes) + permissions: + issues: write + ||||||| base (original) + ======= + permissions: + pull-requests: write + >>>>>>> new (upstream) + ``` + + To resolve conflicts: + 1. Review the conflict markers in the updated workflow file + 2. Manually edit to keep desired changes from both sides + 3. Remove conflict markers (`<<<<<<<`, `|||||||`, `=======`, `>>>>>>>`) + 4. Run `gh aw compile` to recompile the resolved workflow + +- **Conflict Notification**: When conflicts occur, the update command displays a warning: + ``` + ⚠ Updated ci-doctor.md from v1.0.0 to v1.1.0 with CONFLICTS - please review and resolve manually + ``` + ## 🔧 Workflow Recompilation The `compile` command transforms natural language workflow markdown files into executable GitHub Actions YAML files. This is the core functionality that converts your agentic workflow descriptions into automated GitHub workflows. diff --git a/pkg/cli/update_command.go b/pkg/cli/update_command.go index e22ff9f81cb..ae7aea66641 100644 --- a/pkg/cli/update_command.go +++ b/pkg/cli/update_command.go @@ -373,6 +373,16 @@ func updateWorkflow(wf *workflowWithSource, allowMajor, force, verbose bool, eng return nil } + // Download the base version (current ref from source) + if verbose { + fmt.Fprintln(os.Stderr, console.FormatVerboseMessage(fmt.Sprintf("Downloading base version from %s/%s@%s", sourceSpec.Repo, sourceSpec.Path, currentRef))) + } + + baseContent, err := downloadWorkflowContent(sourceSpec.Repo, sourceSpec.Path, currentRef, verbose) + if err != nil { + return fmt.Errorf("failed to download base workflow: %w", err) + } + // Download the latest version if verbose { fmt.Fprintln(os.Stderr, console.FormatVerboseMessage(fmt.Sprintf("Downloading latest version from %s/%s@%s", sourceSpec.Repo, sourceSpec.Path, latestRef))) @@ -389,8 +399,8 @@ func updateWorkflow(wf *workflowWithSource, allowMajor, force, verbose bool, eng return fmt.Errorf("failed to read current workflow: %w", err) } - // Perform 3-way merge - mergedContent, err := mergeWorkflowContent(string(currentContent), string(newContent), wf.SourceSpec, latestRef, verbose) + // Perform 3-way merge using git merge-file + mergedContent, hasConflicts, err := mergeWorkflowContent(string(baseContent), string(currentContent), string(newContent), wf.SourceSpec, latestRef, verbose) if err != nil { return fmt.Errorf("failed to merge workflow content: %w", err) } @@ -400,6 +410,11 @@ func updateWorkflow(wf *workflowWithSource, allowMajor, force, verbose bool, eng return fmt.Errorf("failed to write updated workflow: %w", err) } + if hasConflicts { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Updated %s from %s to %s with CONFLICTS - please review and resolve manually", wf.Name, currentRef, latestRef))) + return nil // Not an error, but user needs to resolve conflicts + } + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Updated %s from %s to %s", wf.Name, currentRef, latestRef))) // Compile the updated workflow @@ -435,81 +450,141 @@ func downloadWorkflowContent(repo, path, ref string, verbose bool) ([]byte, erro return content, nil } -// mergeWorkflowContent performs a 3-way merge of workflow content -// It removes the source field from the new content and updates it with the new ref -func mergeWorkflowContent(current, new, oldSourceSpec, newRef string, verbose bool) (string, error) { +// mergeWorkflowContent performs a 3-way merge of workflow content using git merge-file +// It returns the merged content, whether conflicts exist, and any error +func mergeWorkflowContent(base, current, new, oldSourceSpec, newRef string, verbose bool) (string, bool, error) { if verbose { - fmt.Fprintln(os.Stderr, console.FormatVerboseMessage("Merging workflow content")) + fmt.Fprintln(os.Stderr, console.FormatVerboseMessage("Performing 3-way merge using git merge-file")) } - // Parse both contents - currentResult, err := parser.ExtractFrontmatterFromContent(current) + // First, update the source field in the new content before merging + // This ensures the source field is correct even if there are conflicts + newWithUpdatedSource, err := updateSourceFieldInContent(new, oldSourceSpec, newRef) if err != nil { - return "", fmt.Errorf("failed to parse current frontmatter: %w", err) + if verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to update source in new content: %v", err))) + } + // Continue with original new content + newWithUpdatedSource = new } - newResult, err := parser.ExtractFrontmatterFromContent(new) + // Create temporary directory for merge files + tmpDir, err := os.MkdirTemp("", "gh-aw-merge-*") if err != nil { - return "", fmt.Errorf("failed to parse new frontmatter: %w", err) + return "", false, fmt.Errorf("failed to create temp directory: %w", err) } + defer os.RemoveAll(tmpDir) + + // Write base, current, and new versions to temporary files + baseFile := filepath.Join(tmpDir, "base.md") + currentFile := filepath.Join(tmpDir, "current.md") + newFile := filepath.Join(tmpDir, "new.md") - // Merge strategy: Keep local changes, but allow new fields from upstream - // Start with the new frontmatter as base - if newResult.Frontmatter == nil { - newResult.Frontmatter = make(map[string]any) + if err := os.WriteFile(baseFile, []byte(base), 0644); err != nil { + return "", false, fmt.Errorf("failed to write base file: %w", err) } - if currentResult.Frontmatter == nil { - currentResult.Frontmatter = make(map[string]any) + if err := os.WriteFile(currentFile, []byte(current), 0644); err != nil { + return "", false, fmt.Errorf("failed to write current file: %w", err) } + if err := os.WriteFile(newFile, []byte(newWithUpdatedSource), 0644); err != nil { + return "", false, fmt.Errorf("failed to write new file: %w", err) + } + + // Execute git merge-file + // Format: git merge-file + cmd := exec.Command("git", "merge-file", + "-L", "current (local changes)", + "-L", "base (original)", + "-L", "new (upstream)", + "--diff3", // Use diff3 style conflict markers for better context + currentFile, baseFile, newFile) - // Merge: new fields from upstream, but preserve local modifications - // This means we overlay current frontmatter on top of new frontmatter - for key, value := range currentResult.Frontmatter { - // Skip the source field as we'll update it separately - if key != "source" { - newResult.Frontmatter[key] = value + output, err := cmd.CombinedOutput() + + // git merge-file returns: + // - 0 if merge was successful without conflicts + // - >0 if conflicts were found (appears to return number of conflicts, but file is still updated) + // The exit code can be >1 for multiple conflicts, not just errors + hasConflicts := false + if err != nil { + if exitErr, ok := err.(*exec.ExitError); ok { + exitCode := exitErr.ExitCode() + if exitCode > 0 && exitCode < 128 { + // Conflicts found (exit codes 1-127 indicate conflicts) + // Exit codes >= 128 typically indicate system errors + hasConflicts = true + if verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Merge conflicts detected (exit code: %d)", exitCode))) + } + } else { + // Real error (exit code >= 128) + return "", false, fmt.Errorf("git merge-file failed: %w\nOutput: %s", err, output) + } + } else { + return "", false, fmt.Errorf("failed to execute git merge-file: %w", err) } } - // Use the new markdown content (assume upstream knows best for documentation) - // But if you want to preserve local markdown changes, you could use currentResult.Markdown - - // Update source field with new ref - sourceSpec, err := parseSourceSpec(oldSourceSpec) + // Read the merged content from the current file (git merge-file updates it in-place) + mergedContent, err := os.ReadFile(currentFile) if err != nil { - return "", fmt.Errorf("failed to parse source spec: %w", err) + return "", false, fmt.Errorf("failed to read merged content: %w", err) + } + + mergedStr := string(mergedContent) + + // Process @include directives if present and no conflicts + // Skip include processing if there are conflicts to avoid errors + if !hasConflicts { + sourceSpec, err := parseSourceSpec(oldSourceSpec) + if err == nil { + workflow := &WorkflowSpec{ + RepoSpec: RepoSpec{ + Repo: sourceSpec.Repo, + Version: newRef, + }, + WorkflowPath: sourceSpec.Path, + } + + processedContent, err := processIncludesInContent(mergedStr, workflow, newRef, verbose) + if err != nil { + if verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to process includes: %v", err))) + } + // Return unprocessed content on error + } else { + mergedStr = processedContent + } + } } - newSourceSpec := fmt.Sprintf("%s/%s@%s", sourceSpec.Repo, sourceSpec.Path, newRef) - newResult.Frontmatter["source"] = newSourceSpec + return mergedStr, hasConflicts, nil +} - // Reconstruct the workflow file with updated source - content, err := reconstructWorkflowFile(newResult.Frontmatter, newResult.Markdown) +// updateSourceFieldInContent updates the source field in workflow content +func updateSourceFieldInContent(content, oldSourceSpec, newRef string) (string, error) { + // Parse the content + result, err := parser.ExtractFrontmatterFromContent(content) if err != nil { - return "", err + return "", fmt.Errorf("failed to parse frontmatter: %w", err) } - // Process @include directives in the new content and replace with workflowspec - // Build a WorkflowSpec from the sourceSpec to use for processing includes - workflow := &WorkflowSpec{ - RepoSpec: RepoSpec{ - Repo: sourceSpec.Repo, - Version: newRef, - }, - WorkflowPath: sourceSpec.Path, + if result.Frontmatter == nil { + result.Frontmatter = make(map[string]any) } - // We don't have access to the package path here, so we'll just process the markdown - // The compile step will handle downloading the includes when needed - processedContent, err := processIncludesInContent(content, workflow, newRef, verbose) + // Parse the old source spec to construct the new one + sourceSpec, err := parseSourceSpec(oldSourceSpec) if err != nil { - if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to process includes: %v", err))) - } - return content, nil // Return unprocessed content on error + return "", fmt.Errorf("failed to parse source spec: %w", err) } - return processedContent, nil + // Update with new ref + newSourceSpec := fmt.Sprintf("%s/%s@%s", sourceSpec.Repo, sourceSpec.Path, newRef) + result.Frontmatter["source"] = newSourceSpec + + // Reconstruct the workflow file + return reconstructWorkflowFile(result.Frontmatter, result.Markdown) } // reconstructWorkflowFile reconstructs a workflow file from frontmatter and markdown diff --git a/pkg/cli/update_command_test.go b/pkg/cli/update_command_test.go index 7f1e458cd3a..5a312bfa9b1 100644 --- a/pkg/cli/update_command_test.go +++ b/pkg/cli/update_command_test.go @@ -1 +1,356 @@ package cli + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestMergeWorkflowContent_CleanMerge tests a merge with truly non-overlapping changes +func TestMergeWorkflowContent_CleanMerge(t *testing.T) { + base := `--- +on: push +engine: claude +--- + +# Base Workflow + +This is the base content.` + + // Local adds to markdown only + current := `--- +on: push +engine: claude +--- + +# Base Workflow + +This is the base content. + +## Local Addition + +This section was added locally.` + + // Upstream adds to frontmatter only + new := `--- +on: push +engine: claude +tools: + bash: ["ls"] +source: test/repo/workflow.md@v1.1.0 +--- + +# Base Workflow + +This is the base content.` + + oldSourceSpec := "test/repo/workflow.md@v1.0.0" + newRef := "v1.1.0" + + merged, hasConflicts, err := mergeWorkflowContent(base, current, new, oldSourceSpec, newRef, false) + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + if hasConflicts { + t.Errorf("Expected no conflicts when changes are in different sections (frontmatter vs markdown), merged content:\n%s", merged) + } + + // Check that local markdown changes are preserved + if !strings.Contains(merged, "Local Addition") { + t.Errorf("Expected local markdown changes to be preserved, got:\n%s", merged) + } + + // Check that upstream frontmatter changes are included + if !strings.Contains(merged, "bash:") { + t.Errorf("Expected upstream frontmatter changes to be included, got:\n%s", merged) + } + + // Check that source field is updated + if !strings.Contains(merged, "source: test/repo/workflow.md@v1.1.0") { + t.Errorf("Expected source field to be updated to v1.1.0, got:\n%s", merged) + } +} + +// TestMergeWorkflowContent_WithConflicts tests a merge with conflicts +func TestMergeWorkflowContent_WithConflicts(t *testing.T) { + base := `--- +on: push +engine: claude +--- + +# Original Workflow + +This is the original content.` + + current := `--- +on: push +engine: claude +--- + +# Original Workflow + +This is the local modified content.` + + new := `--- +on: push +engine: claude +--- + +# Original Workflow + +This is the upstream modified content.` + + oldSourceSpec := "test/repo/workflow.md@v1.0.0" + newRef := "v1.1.0" + + merged, hasConflicts, err := mergeWorkflowContent(base, current, new, oldSourceSpec, newRef, false) + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + if !hasConflicts { + t.Error("Expected conflicts to be detected") + } + + // Check for conflict markers + if !strings.Contains(merged, "<<<<<<<") || !strings.Contains(merged, ">>>>>>>") { + t.Error("Expected conflict markers in merged content") + } + + // The merged content should contain both versions + if !strings.Contains(merged, "local modified") && !strings.Contains(merged, "upstream modified") { + t.Error("Expected both local and upstream changes in conflict markers") + } +} + +// TestMergeWorkflowContent_MarkdownOnly tests merging only markdown changes +func TestMergeWorkflowContent_MarkdownOnly(t *testing.T) { + base := `--- +on: push +engine: claude +--- + +# Original + +Original markdown content. + +## Base Section + +Base content here.` + + current := `--- +on: push +engine: claude +--- + +# Original + +Original markdown content. + +## Base Section + +Base content here. + +## Local Section + +Local addition at the end.` + + new := `--- +on: push +engine: claude +source: test/repo/workflow.md@v1.1.0 +--- + +# Original + +Original markdown content. + +## Upstream Section + +Upstream addition after original. + +## Base Section + +Base content here.` + + oldSourceSpec := "test/repo/workflow.md@v1.0.0" + newRef := "v1.1.0" + + merged, hasConflicts, err := mergeWorkflowContent(base, current, new, oldSourceSpec, newRef, false) + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + // Git merge-file should successfully merge these non-overlapping sections + if hasConflicts { + t.Logf("Note: Conflicts detected even for non-overlapping sections:\n%s", merged) + // This is fine - git is being conservative + } + + // At minimum, the merge should include some content + if !strings.Contains(merged, "## Base Section") { + t.Errorf("Expected base section to be preserved, got:\n%s", merged) + } +} + +// TestMergeWorkflowContent_FrontmatterOnly tests merging only frontmatter changes +func TestMergeWorkflowContent_FrontmatterOnly(t *testing.T) { + base := `--- +on: push +engine: claude +--- + +# Workflow + +Content remains the same.` + + // Local adds permissions field + current := `--- +on: push +engine: claude +permissions: + contents: read +--- + +# Workflow + +Content remains the same.` + + // Upstream adds tools field + new := `--- +on: push +engine: claude +tools: + bash: ["ls"] +--- + +# Workflow + +Content remains the same.` + + oldSourceSpec := "test/repo/workflow.md@v1.0.0" + newRef := "v1.1.0" + + merged, hasConflicts, err := mergeWorkflowContent(base, current, new, oldSourceSpec, newRef, false) + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + // Since both added different fields, should not conflict + if hasConflicts { + t.Logf("Note: Conflicts detected for non-overlapping frontmatter fields:\n%s", merged) + } + + // At minimum, the merge should complete + if merged == "" { + t.Error("Expected non-empty merged content") + } + + // Both fields should be present (if no conflicts) or at least one should be there + hasPermissions := strings.Contains(merged, "permissions:") + hasTools := strings.Contains(merged, "tools:") + + if !hasPermissions && !hasTools { + t.Errorf("Expected at least one of the frontmatter changes to be present, got:\n%s", merged) + } +} + +// TestUpdateSourceFieldInContent tests the source field update function +func TestUpdateSourceFieldInContent(t *testing.T) { + content := `--- +on: push +source: old/repo/workflow.md@v1.0.0 +--- + +# Test Workflow` + + oldSourceSpec := "old/repo/workflow.md@v1.0.0" + newRef := "v2.0.0" + + updated, err := updateSourceFieldInContent(content, oldSourceSpec, newRef) + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + if !strings.Contains(updated, "source: old/repo/workflow.md@v2.0.0") { + t.Errorf("Expected source field to be updated to v2.0.0, got:\n%s", updated) + } + + // Ensure other content is preserved + if !strings.Contains(updated, "on: push") { + t.Error("Expected other frontmatter fields to be preserved") + } + + if !strings.Contains(updated, "# Test Workflow") { + t.Error("Expected markdown content to be preserved") + } +} + +// TestMergeWorkflowContent_Integration tests the merge with temporary files +func TestMergeWorkflowContent_Integration(t *testing.T) { + // Create a temporary directory for test files + tmpDir := t.TempDir() + + base := `--- +on: push +permissions: + contents: read +--- + +# Test Workflow + +Base content.` + + // Local adds issues permission + current := `--- +on: push +permissions: + contents: read + issues: write +--- + +# Test Workflow + +Base content with local notes.` + + // Upstream adds pr permission + new := `--- +on: push +permissions: + contents: read + pull-requests: write +--- + +# Test Workflow + +Base content with upstream notes.` + + oldSourceSpec := "test/repo/workflow.md@v1.0.0" + newRef := "v1.1.0" + + merged, hasConflicts, err := mergeWorkflowContent(base, current, new, oldSourceSpec, newRef, true) + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + // Write merged content to verify it's valid + testFile := filepath.Join(tmpDir, "merged.md") + if err := os.WriteFile(testFile, []byte(merged), 0644); err != nil { + t.Fatalf("Failed to write merged file: %v", err) + } + + // Since permissions are on different lines, git should merge them + if hasConflicts { + t.Logf("Conflicts detected (may be expected):\n%s", merged) + // With conflicts, we can't check the merged result as reliably + return + } + + // Without conflicts, verify both permissions are merged + if !strings.Contains(merged, "issues: write") || !strings.Contains(merged, "pull-requests: write") { + t.Logf("Merged content:\n%s", merged) + t.Error("Expected both local and upstream permission changes to be merged") + } +}