diff --git a/pkg/cli/imports.go b/pkg/cli/imports.go index 63d55aea51c..cebb6cd3a85 100644 --- a/pkg/cli/imports.go +++ b/pkg/cli/imports.go @@ -3,6 +3,7 @@ package cli import ( "bufio" "fmt" + "maps" "os" "path/filepath" "strings" @@ -56,54 +57,74 @@ func processImportsWithWorkflowSpec(content string, workflow *WorkflowSpec, comm return content, nil // No imports field, return original content } - // processImportPaths converts a list of raw import paths to workflowspec format. + // resolveOneImportPath converts a single raw import path to workflowspec format. // Paths that already use the workflowspec format (contain "@") are left unchanged. // When localWorkflowDir is set, relative paths whose files exist locally are also // preserved as-is so that consumers who have copied shared files into their own repo // are not forced onto cross-repo references after every `gh aw update`. + resolveOneImportPath := func(importPath string) string { + if isWorkflowSpecFormat(importPath) { + importsLog.Printf("Import already in workflowspec format: %s", importPath) + return importPath + } + // Preserve relative paths whose files exist in the local workflow directory. + // Absolute paths (starting with "/") are not checked — they are always resolved + // relative to the repo root and cannot be reliably tested here. + if localWorkflowDir != "" && !strings.HasPrefix(importPath, "/") { + if isLocalFileForUpdate(localWorkflowDir, importPath) { + importsLog.Printf("Import path exists locally, preserving relative path: %s", importPath) + return importPath + } + } + resolvedPath := resolveImportPath(importPath, filepath.Dir(workflow.WorkflowPath), importPathImportsOpts) + importsLog.Printf("Resolved import path: %s -> %s (workflow: %s)", importPath, resolvedPath, workflow.WorkflowPath) + workflowSpec := buildWorkflowSpecRef(workflow.RepoSlug, resolvedPath, commitSHA, workflow.Version) + importsLog.Printf("Converted import: %s -> %s", importPath, workflowSpec) + return workflowSpec + } + + // processImportPaths converts a list of raw string import paths to workflowspec format. processImportPaths := func(imports []string) []string { processed := make([]string, 0, len(imports)) for _, importPath := range imports { - if isWorkflowSpecFormat(importPath) { - importsLog.Printf("Import already in workflowspec format: %s", importPath) - processed = append(processed, importPath) - continue - } - // Preserve relative paths whose files exist in the local workflow directory. - // Absolute paths (starting with "/") are not checked — they are always resolved - // relative to the repo root and cannot be reliably tested here. - if localWorkflowDir != "" && !strings.HasPrefix(importPath, "/") { - if isLocalFileForUpdate(localWorkflowDir, importPath) { - importsLog.Printf("Import path exists locally, preserving relative path: %s", importPath) - processed = append(processed, importPath) - continue - } - } - resolvedPath := resolveImportPath(importPath, filepath.Dir(workflow.WorkflowPath), importPathImportsOpts) - importsLog.Printf("Resolved import path: %s -> %s (workflow: %s)", importPath, resolvedPath, workflow.WorkflowPath) - workflowSpec := buildWorkflowSpecRef(workflow.RepoSlug, resolvedPath, commitSHA, workflow.Version) - importsLog.Printf("Converted import: %s -> %s", importPath, workflowSpec) - processed = append(processed, workflowSpec) + processed = append(processed, resolveOneImportPath(importPath)) } return processed } - // collectStringImports extracts string paths from a []any slice. - collectStringImports := func(items []any) []string { - var paths []string + // processImportItems converts a list of import entries to workflowspec format. + // Each entry may be a plain string path, or an object with a "path"/"uses" key + // (and optionally an "inputs"/"with" key). Object-form entries are preserved as + // objects — only their path/uses value is rewritten — so that any accompanying + // inputs/with data is not silently dropped from the imports collection. + processImportItems := func(items []any) []any { + processed := make([]any, 0, len(items)) for _, item := range items { - if str, ok := item.(string); ok { - paths = append(paths, str) + switch v := item.(type) { + case string: + processed = append(processed, resolveOneImportPath(v)) + case map[string]any: + updated := make(map[string]any, len(v)) + maps.Copy(updated, v) + if pathVal, ok := updated["path"].(string); ok { + updated["path"] = resolveOneImportPath(pathVal) + } else if usesVal, ok := updated["uses"].(string); ok { + updated["uses"] = resolveOneImportPath(usesVal) + } + processed = append(processed, updated) + default: + importsLog.Printf("Preserving import entry of unsupported type: %T", item) + processed = append(processed, item) } } - return paths + return processed } switch v := importsField.(type) { case []any: - imports := collectStringImports(v) - importsLog.Printf("Found %d imports (array form) to process", len(imports)) - result.Frontmatter["imports"] = processImportPaths(imports) + processedItems := processImportItems(v) + importsLog.Printf("Found %d imports (array form) to process", len(processedItems)) + result.Frontmatter["imports"] = processedItems case []string: importsLog.Printf("Found %d imports ([]string form) to process", len(v)) result.Frontmatter["imports"] = processImportPaths(v) @@ -112,9 +133,9 @@ func processImportsWithWorkflowSpec(content string, workflow *WorkflowSpec, comm if awAny, hasAW := v["aw"]; hasAW { switch aw := awAny.(type) { case []any: - awImports := collectStringImports(aw) - importsLog.Printf("Found %d imports (object form, aw subfield) to process", len(awImports)) - v["aw"] = processImportPaths(awImports) + processedItems := processImportItems(aw) + importsLog.Printf("Found %d imports (object form, aw subfield) to process", len(processedItems)) + v["aw"] = processedItems case []string: importsLog.Printf("Found %d imports (object form, aw []string) to process", len(aw)) v["aw"] = processImportPaths(aw) diff --git a/pkg/cli/imports_test.go b/pkg/cli/imports_test.go index d709917fac3..fed47a690a2 100644 --- a/pkg/cli/imports_test.go +++ b/pkg/cli/imports_test.go @@ -794,6 +794,61 @@ Test content. } } +// TestProcessImportsWithWorkflowSpec_ObjectFormPreserved tests that object-form +// import entries (using "uses"/"with" or "path"/"inputs") are preserved rather +// than silently dropped when the imports field is rewritten to workflowspec +// format. This is a regression test for: gh aw update replaces valid workflow +// imports with `imports: []` when the only entries are object-form. +func TestProcessImportsWithWorkflowSpec_ObjectFormPreserved(t *testing.T) { + content := `--- +engine: copilot +imports: + - uses: shared/control.md + with: + role: orchestrator + rollout_mode: ${{ vars.CENTRAL_AGENTIC_OPS_DEPENDABOT_MODE || 'preview' }} + review_repo: ${{ vars.CENTRAL_AGENTIC_OPS_DEPENDABOT_REVIEW_REPO || '' }} +--- + +# Test Workflow + +Test content. +` + + workflow := &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: "github/gh-aw", + Version: "main", + }, + WorkflowPath: ".github/workflows/dependabot.md", + } + + commitSHA := "abc123def456" + + result, err := processImportsWithWorkflowSpec(content, workflow, commitSHA, "", false) + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + // The imports field must NOT be replaced with an empty array. + if strings.Contains(result, "imports: []") { + t.Fatalf("Expected imports to be preserved, but got empty imports array:\n%s", result) + } + + // The uses path should be rewritten to workflowspec format. + expectedUses := "uses: github/gh-aw/.github/workflows/shared/control.md@abc123def456" + if !strings.Contains(result, expectedUses) { + t.Errorf("Expected result to contain '%s'\nGot:\n%s", expectedUses, result) + } + + // The "with" subfields must be preserved. + for _, expected := range []string{"role: orchestrator", "rollout_mode:", "review_repo:"} { + if !strings.Contains(result, expected) { + t.Errorf("Expected result to contain '%s'\nGot:\n%s", expected, result) + } + } +} + // TestProcessImportsWithWorkflowSpec_PreservesLocalRelativePaths tests that when // localWorkflowDir is provided and import files exist on disk, the relative paths // are kept as-is and NOT rewritten to cross-repo workflowspec references. diff --git a/pkg/cli/update_command_test.go b/pkg/cli/update_command_test.go index 8dacfef0503..5d82152839b 100644 --- a/pkg/cli/update_command_test.go +++ b/pkg/cli/update_command_test.go @@ -82,6 +82,82 @@ This is the base content.` } } +// TestMergeWorkflowContent_PreservesUnchangedObjectFormImports is a regression test +// for: gh aw update replacing a valid, unchanged upstream object-form import +// (uses/with) with `imports: []` when only the prompt body changed upstream. +func TestMergeWorkflowContent_PreservesUnchangedObjectFormImports(t *testing.T) { + base := `--- +on: push +engine: claude +imports: + - uses: shared/control.md + with: + role: orchestrator +--- + +# Dependabot Workflow + +Base prompt body.` + + // Local has no modifications relative to base. + current := `--- +on: push +engine: claude +imports: + - uses: shared/control.md + with: + role: orchestrator +source: test/repo/dependabot.md@v1.0.0 +--- + +# Dependabot Workflow + +Base prompt body.` + + // Upstream only changes the prompt body; the object-form import is unchanged. + new := `--- +on: push +engine: claude +imports: + - uses: shared/control.md + with: + role: orchestrator +source: test/repo/dependabot.md@v1.1.0 +--- + +# Dependabot Workflow + +Updated prompt body.` + + oldSourceSpec := "test/repo/dependabot.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, merged content:\n%s", merged) + } + + if strings.Contains(merged, "imports: []") { + t.Fatalf("Expected the unchanged upstream import to be preserved, but imports were emptied:\n%s", merged) + } + + if !strings.Contains(merged, "uses: shared/control.md") && !strings.Contains(merged, "shared/control.md") { + t.Errorf("Expected the shared/control.md import to be preserved, got:\n%s", merged) + } + + if !strings.Contains(merged, "role: orchestrator") { + t.Errorf("Expected the import 'with' fields to be preserved, got:\n%s", merged) + } + + if !strings.Contains(merged, "Updated prompt body.") { + t.Errorf("Expected upstream prompt body change to be applied, got:\n%s", merged) + } +} + func TestNewUpdateCommand_CoolDownFlagUsage(t *testing.T) { cmd := NewUpdateCommand(func(string) error { return nil }) require.NotNil(t, cmd)