diff --git a/pkg/cli/update_command.go b/pkg/cli/update_command.go index cb305d3da02..87584cb16d1 100644 --- a/pkg/cli/update_command.go +++ b/pkg/cli/update_command.go @@ -4,6 +4,9 @@ import ( "context" "fmt" "os" + "path/filepath" + "sort" + "strings" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" @@ -133,9 +136,14 @@ func RunUpdateWorkflows(ctx context.Context, workflowNames []string, allowMajor, // Resolve and store SHA-256 digest pins for container images referenced in lock files. updateLog.Print("Updating container image digest pins") - if err := UpdateContainerPins(ctx, workflowsDir, verbose); err != nil { + pinsUpdated, err := UpdateContainerPins(ctx, workflowsDir, verbose) + if err != nil { // Non-fatal: Docker may not be available in all environments. fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Warning: Failed to update container pins: %v", err))) + } else if pinsUpdated && !noCompile { + if err := recompileWorkflowsForContainerPins(workflowsDir, engineOverride, verbose); err != nil { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Warning: Failed to apply container pins to lock files: %v", err))) + } } // Update action references in user-provided steps within workflow .md files. @@ -149,3 +157,55 @@ func RunUpdateWorkflows(ctx context.Context, workflowNames []string, allowMajor, updateLog.Printf("Update process complete: had_error=%v", firstErr != nil) return firstErr } + +// recompileWorkflowsForContainerPins recompiles workflows with existing lock files +// so newly resolved container digest pins are applied in the same update run. +func recompileWorkflowsForContainerPins(workflowsDir, engineOverride string, verbose bool) error { + workflowFiles, err := workflowFilesForExistingLocks(workflowsDir) + if err != nil { + return err + } + for _, workflowFile := range workflowFiles { + quiet := false + refreshStopTime := false + if err := compileWorkflowWithRefresh(workflowFile, verbose, quiet, engineOverride, refreshStopTime); err != nil { + return fmt.Errorf("failed to recompile %s after updating container pins: %w", filepath.Base(workflowFile), err) + } + } + return nil +} + +// workflowFilesForExistingLocks returns sorted workflow markdown file paths that +// have a matching .lock.yml file in the same workflow directory. +func workflowFilesForExistingLocks(workflowsDir string) ([]string, error) { + if workflowsDir == "" { + workflowsDir = getWorkflowsDir() + } + + entries, err := os.ReadDir(workflowsDir) + if err != nil { + return nil, fmt.Errorf("failed to read workflows directory %s: %w", workflowsDir, err) + } + + lockFiles := make(map[string]struct{}) + for _, entry := range entries { + if entry.IsDir() || !strings.HasSuffix(entry.Name(), ".lock.yml") { + continue + } + base := strings.TrimSuffix(entry.Name(), ".lock.yml") + lockFiles[base] = struct{}{} + } + + workflowFiles := make([]string, 0, len(lockFiles)) + for _, entry := range entries { + if entry.IsDir() || !strings.HasSuffix(entry.Name(), ".md") { + continue + } + base := strings.TrimSuffix(entry.Name(), ".md") + if _, ok := lockFiles[base]; ok { + workflowFiles = append(workflowFiles, filepath.Join(workflowsDir, entry.Name())) + } + } + sort.Strings(workflowFiles) + return workflowFiles, nil +} diff --git a/pkg/cli/update_command_container_pins_test.go b/pkg/cli/update_command_container_pins_test.go new file mode 100644 index 00000000000..e80f84b0e8b --- /dev/null +++ b/pkg/cli/update_command_container_pins_test.go @@ -0,0 +1,49 @@ +//go:build !integration + +package cli + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestWorkflowFilesForExistingLocks(t *testing.T) { + t.Run("returns only workflow markdown files with matching lock files", func(t *testing.T) { + tmpDir := t.TempDir() + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + require.NoError(t, os.MkdirAll(workflowsDir, 0o755)) + + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "a.md"), []byte("---\non: issues\n---"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "a.lock.yml"), []byte("name: a"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "b.md"), []byte("---\non: issues\n---"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "b.lock.yml"), []byte("name: b"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "c.md"), []byte("---\non: issues\n---"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "notes.md"), []byte("# include"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "notes.lock.yml"), []byte("name: notes"), 0o644)) + + files, err := workflowFilesForExistingLocks(workflowsDir) + require.NoError(t, err, "workflow file discovery should succeed") + assert.Equal(t, []string{ + filepath.Join(workflowsDir, "a.md"), + filepath.Join(workflowsDir, "b.md"), + filepath.Join(workflowsDir, "notes.md"), + }, files, "only markdown files with lock counterparts should be returned in sorted order") + }) + + t.Run("returns empty slice when no matching pairs exist", func(t *testing.T) { + tmpDir := t.TempDir() + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + require.NoError(t, os.MkdirAll(workflowsDir, 0o755)) + + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "only-md.md"), []byte("---\non: issues\n---"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "only-lock.lock.yml"), []byte("name: only-lock"), 0o644)) + + files, err := workflowFilesForExistingLocks(workflowsDir) + require.NoError(t, err, "workflow file discovery should succeed") + assert.Empty(t, files, "no matching markdown/lock pairs should produce an empty result") + }) +} diff --git a/pkg/cli/update_container_pins.go b/pkg/cli/update_container_pins.go index 594172944a7..a6fea3df836 100644 --- a/pkg/cli/update_container_pins.go +++ b/pkg/cli/update_container_pins.go @@ -48,9 +48,10 @@ var buildxDigestPattern = regexp.MustCompile(`(?m)^Digest:\s+(sha256:[a-f0-9]{64 // as they are already pinned. Images without a cached pin are queried via the // Docker CLI ("docker buildx imagetools inspect"). // -// When Docker is unavailable the function logs a warning and returns nil so that -// the overall upgrade flow is not interrupted. -func UpdateContainerPins(ctx context.Context, workflowDir string, verbose bool) error { +// When Docker is unavailable the function logs a warning and returns (false, nil) +// so that the overall upgrade flow is not interrupted. +// The bool return value indicates whether any container pin entries were newly added. +func UpdateContainerPins(ctx context.Context, workflowDir string, verbose bool) (bool, error) { containerPinsLog.Print("Starting container pin update") if verbose { @@ -65,14 +66,14 @@ func UpdateContainerPins(ctx context.Context, workflowDir string, verbose bool) if verbose { fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Warning: Failed to collect container images: %v", err))) } - return nil + return false, nil } if len(images) == 0 { if verbose { fmt.Fprintln(os.Stderr, console.FormatVerboseMessage("No container images found in lock files")) } - return nil + return false, nil } containerPinsLog.Printf("Found %d unique container image(s) across lock files", len(images)) @@ -82,7 +83,7 @@ func UpdateContainerPins(ctx context.Context, workflowDir string, verbose bool) actionCache := workflow.NewActionCache(".") if _, statErr := os.Stat(actionsLockPath); statErr == nil { if loadErr := actionCache.Load(); loadErr != nil { - return fmt.Errorf("failed to load actions-lock.json: %w", loadErr) + return false, fmt.Errorf("failed to load actions-lock.json: %w", loadErr) } } @@ -154,12 +155,12 @@ func UpdateContainerPins(ctx context.Context, workflowDir string, verbose bool) if len(updatedImages) > 0 { if err := actionCache.Save(); err != nil { - return fmt.Errorf("failed to save actions-lock.json: %w", err) + return false, fmt.Errorf("failed to save actions-lock.json: %w", err) } fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Updated container pins in actions-lock.json")) } - return nil + return len(updatedImages) > 0, nil } // collectImagesFromLockFiles scans all .lock.yml files under workflowDir and returns diff --git a/pkg/cli/upgrade_command.go b/pkg/cli/upgrade_command.go index 643f3f6cb31..0dbdff6b91c 100644 --- a/pkg/cli/upgrade_command.go +++ b/pkg/cli/upgrade_command.go @@ -248,7 +248,7 @@ func runUpgradeCommand(ctx context.Context, verbose bool, workflowDir string, no // pinned @sha256: references in the generated lock files. if !noFix && !noActions { upgradeLog.Print("Updating container image digest pins") - if err := UpdateContainerPins(ctx, workflowDir, verbose); err != nil { + if _, err := UpdateContainerPins(ctx, workflowDir, verbose); err != nil { upgradeLog.Printf("Failed to update container pins: %v", err) // Non-critical — Docker may not be available in all environments. fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Warning: Failed to update container pins: %v", err))) diff --git a/pkg/parser/frontmatter_hash.go b/pkg/parser/frontmatter_hash.go index e821ee85466..f5277c28ef5 100644 --- a/pkg/parser/frontmatter_hash.go +++ b/pkg/parser/frontmatter_hash.go @@ -192,7 +192,7 @@ func computeFrontmatterHashFromContent(content string, parsedFrontmatter map[str } // Compute hash using text-based approach with custom file reader - return computeFrontmatterHashTextBasedWithReader(frontmatterText, fullBody, baseDir, cache, relevantExpressions, fileReader) + return computeFrontmatterHashTextBasedWithReader(frontmatterText, fullBody, baseDir, filePath, cache, relevantExpressions, fileReader) } // extractRelevantTemplateExpressions extracts template expressions from markdown @@ -421,7 +421,7 @@ func processImportsTextBased(frontmatterText, baseDir string, visited map[string // computeFrontmatterHashTextBasedWithReader computes the hash using text-based approach with custom file reader. // When markdown is non-empty, it is included as the full body text in the canonical data (used for // inlined-imports mode where the entire body is compiled into the lock file). -func computeFrontmatterHashTextBasedWithReader(frontmatterText, markdown, baseDir string, cache *ImportCache, expressions []string, fileReader FileReader) (string, error) { +func computeFrontmatterHashTextBasedWithReader(frontmatterText, markdown, baseDir, filePath string, cache *ImportCache, expressions []string, fileReader FileReader) (string, error) { frontmatterHashLog.Print("Computing frontmatter hash using text-based approach") // Process imports using text-based parsing with custom file reader @@ -462,6 +462,10 @@ func computeFrontmatterHashTextBasedWithReader(frontmatterText, markdown, baseDi canonical["template-expressions"] = expressions } + // Include relevant container digest pins in the canonical hash input. + // This ensures lock metadata changes when resolved container pins change. + addContainerPinsToCanonical(canonical, frontmatterText, filePath, fileReader) + // Serialize to canonical JSON canonicalJSON := marshalSorted(canonical) @@ -474,3 +478,163 @@ func computeFrontmatterHashTextBasedWithReader(frontmatterText, markdown, baseDi frontmatterHashLog.Printf("Computed hash: %s", hashHex) return hashHex, nil } + +// addContainerPinsToCanonical augments canonical hash data with digest pins relevant +// to container images declared in frontmatter tools. +func addContainerPinsToCanonical(canonical map[string]any, frontmatterText, workflowPath string, fileReader FileReader) { + frontmatter, err := ExtractFrontmatterFromContent("---\n" + frontmatterText + "\n---\n") + if err != nil { + frontmatterHashLog.Printf("Skipping container pins in hash for %s: failed to parse frontmatter for container pin extraction: %v", workflowPath, err) + return + } + + images := extractContainerImagesFromFrontmatter(frontmatter.Frontmatter) + if len(images) == 0 { + return + } + + lockData, ok := findActionsLockContent(workflowPath, fileReader) + if !ok { + return + } + + type containerPin struct { + Digest string `json:"digest"` + PinnedImage string `json:"pinned_image"` + } + var lock struct { + Containers map[string]containerPin `json:"containers"` + } + if err := json.Unmarshal(lockData, &lock); err != nil { + frontmatterHashLog.Printf("Skipping container pins in hash for %s: failed to parse actions-lock.json: %v", workflowPath, err) + return + } + if len(lock.Containers) == 0 { + return + } + + pins := make(map[string]any) + for _, image := range images { + if pin, ok := lock.Containers[image]; ok { + if pinnedRef := normalizePinnedContainerReference(image, pin); pinnedRef != "" { + pins[image] = pinnedRef + } + } + } + + if len(pins) > 0 { + canonical["container-pins"] = pins + } +} + +// normalizePinnedContainerReference returns a normalized container pin reference +// in the canonical image@sha256: syntax when available. +func normalizePinnedContainerReference(image string, pin struct { + Digest string `json:"digest"` + PinnedImage string `json:"pinned_image"` +}) string { + if strings.Contains(pin.PinnedImage, "@sha256:") { + return pin.PinnedImage + } + if strings.HasPrefix(pin.Digest, "sha256:") { + return image + "@" + pin.Digest + } + return "" +} + +// findActionsLockContent searches upward from workflowPath for .github/aw/actions-lock.json. +func findActionsLockContent(workflowPath string, fileReader FileReader) ([]byte, bool) { + dir := filepath.Dir(workflowPath) + for { + candidate := filepath.Join(dir, ".github", "aw", "actions-lock.json") + if content, err := fileReader(candidate); err == nil { + return content, true + } + + parent := filepath.Dir(dir) + if parent == dir { + break + } + dir = parent + } + return nil, false +} + +// extractContainerImagesFromFrontmatter returns sorted, deduplicated container images +// from tool definitions in frontmatter. +func extractContainerImagesFromFrontmatter(frontmatter map[string]any) []string { + if frontmatter == nil { + return nil + } + + tools, ok := frontmatter["tools"].(map[string]any) + if !ok || len(tools) == 0 { + return nil + } + + set := make(map[string]bool) + for _, toolDef := range tools { + toolMap, ok := toolDef.(map[string]any) + if !ok { + continue + } + + if container, ok := toolMap["container"].(string); ok && container != "" { + set[container] = true + continue + } + + command, ok := toolMap["command"].(string) + // Only extract container images from docker commands. Tool definitions + // using other commands are not expected to expose container images in + // args, so skip them. + if !ok || command != "docker" { + continue + } + image := extractImageFromDockerRunArgs(toolMap["args"]) + if image != "" { + set[image] = true + } + } + + if len(set) == 0 { + return nil + } + + images := make([]string, 0, len(set)) + for image := range set { + images = append(images, image) + } + sort.Strings(images) + return images +} + +// extractImageFromDockerRunArgs returns the last argument from docker run args, +// assuming it is the image reference for `docker run ... `. +// Returns empty string if args is empty, cannot be converted to []any/[]string, +// the last value is not a string, or the last argument starts with "-" (likely +// a flag rather than an image name). +// This heuristic is intentionally narrow for frontmatter tool definitions where +// args are expected to end at the image reference and not include a container +// command after the image. +func extractImageFromDockerRunArgs(args any) string { + var last string + switch v := args.(type) { + case []any: + if len(v) == 0 { + return "" + } + last, _ = v[len(v)-1].(string) + case []string: + if len(v) == 0 { + return "" + } + last = v[len(v)-1] + default: + return "" + } + if last == "" || strings.HasPrefix(last, "-") { + return "" + } + return last +} diff --git a/pkg/parser/frontmatter_hash_test.go b/pkg/parser/frontmatter_hash_test.go index 9af46ef3e93..80b446a89d9 100644 --- a/pkg/parser/frontmatter_hash_test.go +++ b/pkg/parser/frontmatter_hash_test.go @@ -254,3 +254,161 @@ tools: assert.Len(t, hash, 64, "Hash should be 64 characters") assert.Regexp(t, "^[a-f0-9]{64}$", hash, "Hash should be lowercase hex") } + +func TestComputeFrontmatterHash_IncludesRelevantContainerPins(t *testing.T) { + workflowPath := "/repo/.github/workflows/workflow.md" + workflowContent := `--- +engine: copilot +tools: + mcp-server: + container: ghcr.io/github/github-mcp-server:v0.32.0 +--- + +# Body +` + + lockV1 := `{ + "entries": {}, + "containers": { + "ghcr.io/github/github-mcp-server:v0.32.0": { + "image": "ghcr.io/github/github-mcp-server:v0.32.0", + "digest": "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "pinned_image": "ghcr.io/github/github-mcp-server:v0.32.0@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + } + } +}` + lockV2 := `{ + "entries": {}, + "containers": { + "ghcr.io/github/github-mcp-server:v0.32.0": { + "image": "ghcr.io/github/github-mcp-server:v0.32.0", + "digest": "sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "pinned_image": "ghcr.io/github/github-mcp-server:v0.32.0@sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + } + } +}` + + readWithLock := func(lockContent string) FileReader { + return func(filePath string) ([]byte, error) { + switch filePath { + case workflowPath: + return []byte(workflowContent), nil + case "/repo/.github/aw/actions-lock.json": + return []byte(lockContent), nil + default: + return nil, os.ErrNotExist + } + } + } + + hashV1, err := ComputeFrontmatterHashFromFileWithReader(workflowPath, nil, readWithLock(lockV1)) + require.NoError(t, err, "Should compute hash with first lock data") + hashV2, err := ComputeFrontmatterHashFromFileWithReader(workflowPath, nil, readWithLock(lockV2)) + require.NoError(t, err, "Should compute hash with second lock data") + assert.NotEqual(t, hashV1, hashV2, "Hash should change when relevant container pin changes") +} + +func TestComputeFrontmatterHash_IgnoresUnrelatedContainerPins(t *testing.T) { + workflowPath := "/repo/.github/workflows/workflow.md" + workflowContent := `--- +engine: copilot +tools: + mcp-server: + container: ghcr.io/github/github-mcp-server:v0.32.0 +--- + +# Body +` + + lockA := `{ + "entries": {}, + "containers": { + "ghcr.io/github/other-image:v1": { + "image": "ghcr.io/github/other-image:v1", + "digest": "sha256:1111111111111111111111111111111111111111111111111111111111111111", + "pinned_image": "ghcr.io/github/other-image:v1@sha256:1111111111111111111111111111111111111111111111111111111111111111" + } + } +}` + lockB := `{ + "entries": {}, + "containers": { + "ghcr.io/github/other-image:v1": { + "image": "ghcr.io/github/other-image:v1", + "digest": "sha256:2222222222222222222222222222222222222222222222222222222222222222", + "pinned_image": "ghcr.io/github/other-image:v1@sha256:2222222222222222222222222222222222222222222222222222222222222222" + } + } +}` + + readWithLock := func(lockContent string) FileReader { + return func(filePath string) ([]byte, error) { + switch filePath { + case workflowPath: + return []byte(workflowContent), nil + case "/repo/.github/aw/actions-lock.json": + return []byte(lockContent), nil + default: + return nil, os.ErrNotExist + } + } + } + + hashA, err := ComputeFrontmatterHashFromFileWithReader(workflowPath, nil, readWithLock(lockA)) + require.NoError(t, err, "Should compute hash with unrelated lock data A") + hashB, err := ComputeFrontmatterHashFromFileWithReader(workflowPath, nil, readWithLock(lockB)) + require.NoError(t, err, "Should compute hash with unrelated lock data B") + assert.Equal(t, hashA, hashB, "Hash should stay the same when only unrelated container pins change") +} + +func TestComputeFrontmatterHash_NormalizesContainerPinSyntax(t *testing.T) { + workflowPath := "/repo/.github/workflows/workflow.md" + workflowContent := `--- +engine: copilot +tools: + mcp-server: + container: ghcr.io/github/github-mcp-server:v0.32.0 +--- + +# Body +` + + lockDigestOnly := `{ + "entries": {}, + "containers": { + "ghcr.io/github/github-mcp-server:v0.32.0": { + "image": "ghcr.io/github/github-mcp-server:v0.32.0", + "digest": "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + } + } +}` + lockPinnedSyntax := `{ + "entries": {}, + "containers": { + "ghcr.io/github/github-mcp-server:v0.32.0": { + "image": "ghcr.io/github/github-mcp-server:v0.32.0", + "digest": "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "pinned_image": "ghcr.io/github/github-mcp-server:v0.32.0@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + } + } +}` + + readWithLock := func(lockContent string) FileReader { + return func(filePath string) ([]byte, error) { + switch filePath { + case workflowPath: + return []byte(workflowContent), nil + case "/repo/.github/aw/actions-lock.json": + return []byte(lockContent), nil + default: + return nil, os.ErrNotExist + } + } + } + + hashDigestOnly, err := ComputeFrontmatterHashFromFileWithReader(workflowPath, nil, readWithLock(lockDigestOnly)) + require.NoError(t, err, "Should compute hash when only digest is present") + hashPinnedSyntax, err := ComputeFrontmatterHashFromFileWithReader(workflowPath, nil, readWithLock(lockPinnedSyntax)) + require.NoError(t, err, "Should compute hash when pinned_image syntax is present") + assert.Equal(t, hashDigestOnly, hashPinnedSyntax, "Hash should normalize to image@sha256 syntax") +}