From 6f1dd31458fed6305e8987bc9bcfe6eecc7724bd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 16 Mar 2026 04:15:48 +0000 Subject: [PATCH 1/2] Initial plan From 6ac26bf60f6d11d9e25d1f2d838e42c02f4a7360 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 16 Mar 2026 04:27:21 +0000 Subject: [PATCH 2/2] refactor: extract shared flattenArtifactTree helper to eliminate duplicate flattening logic Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/logs_download.go | 232 ++++++++--------------------------- pkg/cli/logs_flatten_test.go | 110 ++++++++++++++++- 2 files changed, 159 insertions(+), 183 deletions(-) diff --git a/pkg/cli/logs_download.go b/pkg/cli/logs_download.go index 95708bab223..962582ea337 100644 --- a/pkg/cli/logs_download.go +++ b/pkg/cli/logs_download.go @@ -145,55 +145,23 @@ func findArtifactDir(outputDir, baseName string, legacyName string) string { return "" } -// flattenUnifiedArtifact flattens the unified agent artifact directory structure. -// The artifact is uploaded with all paths under /tmp/gh-aw/, so the action strips the -// common prefix and files land directly inside the artifact directory (new structure). -// For backward compatibility, it also handles the old structure where the full -// tmp/gh-aw/ path was preserved inside the artifact directory. -// New artifact name: "agent" (preferred) -// Legacy artifact name: "agent-artifacts" (backward compat for older workflow runs) -// In workflow_call context, the artifact may be prefixed: "-agent" -func flattenUnifiedArtifact(outputDir string, verbose bool) error { - agentArtifactsDir := findArtifactDir(outputDir, "agent", "agent-artifacts") - if agentArtifactsDir == "" { - // No unified artifact, nothing to flatten - return nil - } - - logsDownloadLog.Printf("Flattening unified agent artifact directory: %s", agentArtifactsDir) - - // Check for old nested structure (agent-artifacts/tmp/gh-aw/) - tmpGhAwPath := filepath.Join(agentArtifactsDir, "tmp", "gh-aw") - hasOldStructure := false - if _, err := os.Stat(tmpGhAwPath); err == nil { - hasOldStructure = true - logsDownloadLog.Printf("Found old artifact structure with tmp/gh-aw prefix") - } - - // Determine the source path for flattening - var sourcePath string - if hasOldStructure { - // Old structure: flatten from agent-artifacts/tmp/gh-aw/ - sourcePath = tmpGhAwPath - } else { - // New structure: flatten from artifact directory directly - sourcePath = agentArtifactsDir - logsDownloadLog.Printf("Found new artifact structure without tmp/gh-aw prefix") - } - - // Walk through source path and move all files to root output directory - err := filepath.Walk(sourcePath, func(path string, info os.FileInfo, err error) error { +// flattenArtifactTree moves all files from sourceDir into outputDir, preserving relative paths, +// then removes artifactDir (which may equal sourceDir, or be a parent of it in the old-structure +// case). label is used in log and user-facing messages. +// Cleanup failures are non-fatal: they are logged (and optionally printed) but do not return an error. +func flattenArtifactTree(sourceDir, artifactDir, outputDir, label string, verbose bool) error { + err := filepath.Walk(sourceDir, func(path string, info os.FileInfo, err error) error { if err != nil { return err } // Skip the source directory itself - if path == sourcePath { + if path == sourceDir { return nil } // Calculate relative path from source - relPath, err := filepath.Rel(sourcePath, path) + relPath, err := filepath.Rel(sourceDir, path) if err != nil { return fmt.Errorf("failed to get relative path for %s: %w", path, err) } @@ -207,7 +175,6 @@ func flattenUnifiedArtifact(outputDir string, verbose bool) error { } logsDownloadLog.Printf("Created directory: %s", destPath) } else { - // Move file to destination // Ensure parent directory exists with owner+group permissions only (0750) if err := os.MkdirAll(filepath.Dir(destPath), 0750); err != nil { return fmt.Errorf("failed to create parent directory for %s: %w", destPath, err) @@ -226,34 +193,62 @@ func flattenUnifiedArtifact(outputDir string, verbose bool) error { }) if err != nil { - return fmt.Errorf("failed to flatten unified artifact: %w", err) + return fmt.Errorf("failed to flatten %s: %w", label, err) } - // Remove the now-empty artifact directory structure - if err := os.RemoveAll(agentArtifactsDir); err != nil { - logsDownloadLog.Printf("Failed to remove agent artifact directory %s: %v", agentArtifactsDir, err) + // Remove the now-empty artifact directory structure. + // Don't fail the entire operation if cleanup fails. + if err := os.RemoveAll(artifactDir); err != nil { + logsDownloadLog.Printf("Failed to remove %s directory %s: %v", label, artifactDir, err) if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to remove agent artifact directory: %v", err))) + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to remove %s directory: %v", label, err))) } - // Don't fail the entire operation if cleanup fails } else { - logsDownloadLog.Printf("Removed agent artifact directory: %s", agentArtifactsDir) + logsDownloadLog.Printf("Removed %s directory: %s", label, artifactDir) if verbose { - fmt.Fprintln(os.Stderr, console.FormatVerboseMessage("Flattened unified agent artifact and removed nested structure")) + fmt.Fprintln(os.Stderr, console.FormatVerboseMessage(fmt.Sprintf("Flattened %s and removed nested structure", label))) } } return nil } -// flattenActivationArtifact flattens the activation artifact directory structure -// The activation artifact contains aw_info.json and aw-prompts/prompt.txt +// flattenUnifiedArtifact flattens the unified agent artifact directory structure. +// The artifact is uploaded with all paths under /tmp/gh-aw/, so the action strips the +// common prefix and files land directly inside the artifact directory (new structure). +// For backward compatibility, it also handles the old structure where the full +// tmp/gh-aw/ path was preserved inside the artifact directory. +// New artifact name: "agent" (preferred) +// Legacy artifact name: "agent-artifacts" (backward compat for older workflow runs) +// In workflow_call context, the artifact may be prefixed: "-agent" +func flattenUnifiedArtifact(outputDir string, verbose bool) error { + agentArtifactsDir := findArtifactDir(outputDir, "agent", "agent-artifacts") + if agentArtifactsDir == "" { + // No unified artifact, nothing to flatten + return nil + } + + logsDownloadLog.Printf("Flattening unified agent artifact directory: %s", agentArtifactsDir) + + // Determine the source path: old structure preserves the tmp/gh-aw/ prefix inside the artifact + sourceDir := agentArtifactsDir + tmpGhAwPath := filepath.Join(agentArtifactsDir, "tmp", "gh-aw") + if _, err := os.Stat(tmpGhAwPath); err == nil { + logsDownloadLog.Printf("Found old artifact structure with tmp/gh-aw prefix") + sourceDir = tmpGhAwPath + } else { + logsDownloadLog.Printf("Found new artifact structure without tmp/gh-aw prefix") + } + + return flattenArtifactTree(sourceDir, agentArtifactsDir, outputDir, "unified agent artifact", verbose) +} + +// flattenActivationArtifact flattens the activation artifact directory structure. +// The activation artifact contains aw_info.json and aw-prompts/prompt.txt. // This function moves those files to the root output directory and removes the nested structure. // In workflow_call context, the artifact may be prefixed: "-activation" func flattenActivationArtifact(outputDir string, verbose bool) error { activationDir := findArtifactDir(outputDir, "activation", "") - - // Check if activation directory exists if activationDir == "" { // No activation artifact, nothing to flatten return nil @@ -261,73 +256,12 @@ func flattenActivationArtifact(outputDir string, verbose bool) error { logsDownloadLog.Printf("Flattening activation artifact directory: %s", activationDir) - // Walk through activation directory and move all files to root output directory - err := filepath.Walk(activationDir, func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - - // Skip the source directory itself - if path == activationDir { - return nil - } - - // Calculate relative path from source - relPath, err := filepath.Rel(activationDir, path) - if err != nil { - return fmt.Errorf("failed to get relative path for %s: %w", path, err) - } - - destPath := filepath.Join(outputDir, relPath) - - if info.IsDir() { - // Create directory in destination with owner+group permissions only (0750) - if err := os.MkdirAll(destPath, 0750); err != nil { - return fmt.Errorf("failed to create directory %s: %w", destPath, err) - } - logsDownloadLog.Printf("Created directory: %s", destPath) - } else { - // Move file to destination - // Ensure parent directory exists with owner+group permissions only (0750) - if err := os.MkdirAll(filepath.Dir(destPath), 0750); err != nil { - return fmt.Errorf("failed to create parent directory for %s: %w", destPath, err) - } - - if err := os.Rename(path, destPath); err != nil { - return fmt.Errorf("failed to move file %s to %s: %w", path, destPath, err) - } - logsDownloadLog.Printf("Moved file: %s → %s", path, destPath) - if verbose { - fmt.Fprintln(os.Stderr, console.FormatVerboseMessage(fmt.Sprintf("Flattened: %s → %s", relPath, relPath))) - } - } - - return nil - }) - - if err != nil { - return fmt.Errorf("failed to flatten activation artifact: %w", err) - } - - // Remove the now-empty activation directory structure - if err := os.RemoveAll(activationDir); err != nil { - logsDownloadLog.Printf("Failed to remove activation directory %s: %v", activationDir, err) - if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to remove activation directory: %v", err))) - } - // Don't fail the entire operation if cleanup fails - } else { - logsDownloadLog.Printf("Removed activation directory: %s", activationDir) - if verbose { - fmt.Fprintln(os.Stderr, console.FormatVerboseMessage("Flattened activation artifact and removed nested structure")) - } - } - - return nil + return flattenArtifactTree(activationDir, activationDir, outputDir, "activation artifact", verbose) } +// flattenAgentOutputsArtifact flattens the agent_outputs artifact directory structure. // The agent_outputs artifact contains session logs with detailed token usage data -// that are critical for accurate token count parsing +// that are critical for accurate token count parsing. func flattenAgentOutputsArtifact(outputDir string, verbose bool) error { agentOutputsDir := filepath.Join(outputDir, "agent_outputs") @@ -340,69 +274,7 @@ func flattenAgentOutputsArtifact(outputDir string, verbose bool) error { logsDownloadLog.Printf("Flattening agent_outputs directory: %s", agentOutputsDir) - // Walk through agent_outputs and move all files to root output directory - err := filepath.Walk(agentOutputsDir, func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - - // Skip the root directory itself - if path == agentOutputsDir { - return nil - } - - // Calculate relative path from agent_outputs - relPath, err := filepath.Rel(agentOutputsDir, path) - if err != nil { - return fmt.Errorf("failed to get relative path for %s: %w", path, err) - } - - destPath := filepath.Join(outputDir, relPath) - - if info.IsDir() { - // Create directory in destination - if err := os.MkdirAll(destPath, 0750); err != nil { - return fmt.Errorf("failed to create directory %s: %w", destPath, err) - } - logsDownloadLog.Printf("Created directory: %s", destPath) - } else { - // Move file to destination - // Ensure parent directory exists - if err := os.MkdirAll(filepath.Dir(destPath), 0750); err != nil { - return fmt.Errorf("failed to create parent directory for %s: %w", destPath, err) - } - - if err := os.Rename(path, destPath); err != nil { - return fmt.Errorf("failed to move file %s to %s: %w", path, destPath, err) - } - logsDownloadLog.Printf("Moved file: %s → %s", path, destPath) - if verbose { - fmt.Fprintln(os.Stderr, console.FormatVerboseMessage(fmt.Sprintf("Flattened: %s → %s", relPath, relPath))) - } - } - - return nil - }) - - if err != nil { - return fmt.Errorf("failed to flatten agent_outputs artifact: %w", err) - } - - // Remove the now-empty agent_outputs directory - if err := os.RemoveAll(agentOutputsDir); err != nil { - logsDownloadLog.Printf("Failed to remove agent_outputs directory %s: %v", agentOutputsDir, err) - if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to remove agent_outputs directory: %v", err))) - } - // Don't fail the entire operation if cleanup fails - } else { - logsDownloadLog.Printf("Removed agent_outputs directory: %s", agentOutputsDir) - if verbose { - fmt.Fprintln(os.Stderr, console.FormatVerboseMessage("Flattened agent_outputs artifact and removed nested structure")) - } - } - - return nil + return flattenArtifactTree(agentOutputsDir, agentOutputsDir, outputDir, "agent_outputs artifact", verbose) } // downloadWorkflowRunLogs downloads and unzips workflow run logs using GitHub API diff --git a/pkg/cli/logs_flatten_test.go b/pkg/cli/logs_flatten_test.go index 95d8f659e5e..8d0c36fcc52 100644 --- a/pkg/cli/logs_flatten_test.go +++ b/pkg/cli/logs_flatten_test.go @@ -10,11 +10,10 @@ import ( "testing" "github.com/github/gh-aw/pkg/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -// Tests for artifact unfold rule implementation -// Unfold rule: If an artifact download folder contains a single file, move the file to root and delete the folder - func TestFlattenSingleFileArtifacts(t *testing.T) { tests := []struct { name string @@ -532,3 +531,108 @@ func TestFlattenUnifiedArtifact(t *testing.T) { }) } } + +// TestFlattenArtifactTree tests the shared flatten helper directly. + +func TestFlattenArtifactTreeBasic(t *testing.T) { + outputDir := t.TempDir() + artifactDir := filepath.Join(outputDir, "myartifact") + require.NoError(t, os.MkdirAll(artifactDir, 0750), "setup: create artifactDir") + require.NoError(t, os.WriteFile(filepath.Join(artifactDir, "file.txt"), []byte("hello"), 0600), "setup: write file") + + err := flattenArtifactTree(artifactDir, artifactDir, outputDir, "test artifact", false) + require.NoError(t, err, "flattenArtifactTree should succeed") + + content, err := os.ReadFile(filepath.Join(outputDir, "file.txt")) + require.NoError(t, err, "file should be moved to outputDir") + assert.Equal(t, "hello", string(content), "file content should be preserved") + + _, err = os.Stat(artifactDir) + assert.True(t, os.IsNotExist(err), "artifactDir should be removed after flattening") +} + +func TestFlattenArtifactTreeNestedDirs(t *testing.T) { + outputDir := t.TempDir() + artifactDir := filepath.Join(outputDir, "artifact") + nestedDir := filepath.Join(artifactDir, "subdir", "deeper") + require.NoError(t, os.MkdirAll(nestedDir, 0750), "setup: create nested dirs") + require.NoError(t, os.WriteFile(filepath.Join(nestedDir, "nested.txt"), []byte("nested"), 0600), "setup: write nested file") + + err := flattenArtifactTree(artifactDir, artifactDir, outputDir, "nested artifact", false) + require.NoError(t, err, "flattenArtifactTree should succeed with nested dirs") + + content, err := os.ReadFile(filepath.Join(outputDir, "subdir", "deeper", "nested.txt")) + require.NoError(t, err, "nested file should be moved preserving relative path") + assert.Equal(t, "nested", string(content), "nested file content should be preserved") + + _, err = os.Stat(artifactDir) + assert.True(t, os.IsNotExist(err), "artifactDir should be removed") +} + +func TestFlattenArtifactTreeDifferentSourceAndArtifactDir(t *testing.T) { + // Covers the old-structure unified artifact case where sourceDir is a subdirectory of artifactDir. + outputDir := t.TempDir() + artifactDir := filepath.Join(outputDir, "agent-artifacts") + sourceDir := filepath.Join(artifactDir, "tmp", "gh-aw") + require.NoError(t, os.MkdirAll(sourceDir, 0750), "setup: create old-structure sourceDir") + require.NoError(t, os.WriteFile(filepath.Join(sourceDir, "session.json"), []byte(`{}`), 0600), "setup: write file") + + err := flattenArtifactTree(sourceDir, artifactDir, outputDir, "unified agent artifact", false) + require.NoError(t, err, "flattenArtifactTree should succeed") + + content, err := os.ReadFile(filepath.Join(outputDir, "session.json")) + require.NoError(t, err, "file should be moved to outputDir relative to sourceDir") + assert.Equal(t, `{}`, string(content), "file content should be preserved") + + _, err = os.Stat(artifactDir) + assert.True(t, os.IsNotExist(err), "entire artifactDir should be removed, not just sourceDir") +} + +func TestFlattenArtifactTreeEmptySource(t *testing.T) { + outputDir := t.TempDir() + artifactDir := filepath.Join(outputDir, "empty-artifact") + require.NoError(t, os.MkdirAll(artifactDir, 0750), "setup: create empty artifactDir") + + err := flattenArtifactTree(artifactDir, artifactDir, outputDir, "empty artifact", false) + require.NoError(t, err, "flattenArtifactTree should succeed on empty dir") + + _, err = os.Stat(artifactDir) + assert.True(t, os.IsNotExist(err), "empty artifactDir should be removed") +} + +func TestFlattenArtifactTreeMultipleFiles(t *testing.T) { + outputDir := t.TempDir() + artifactDir := filepath.Join(outputDir, "multi") + require.NoError(t, os.MkdirAll(artifactDir, 0750), "setup: create artifactDir") + + files := map[string]string{ + "a.txt": "content-a", + "b.txt": "content-b", + "c.txt": "content-c", + } + for name, body := range files { + require.NoError(t, os.WriteFile(filepath.Join(artifactDir, name), []byte(body), 0600), "setup: write "+name) + } + + err := flattenArtifactTree(artifactDir, artifactDir, outputDir, "multi artifact", false) + require.NoError(t, err, "flattenArtifactTree should succeed") + + for name, expected := range files { + content, err := os.ReadFile(filepath.Join(outputDir, name)) + require.NoError(t, err, "file %s should exist in outputDir", name) + assert.Equal(t, expected, string(content), "content of %s should be preserved", name) + } +} + +func TestFlattenArtifactTreeVerboseMode(t *testing.T) { + outputDir := t.TempDir() + artifactDir := filepath.Join(outputDir, "verbose-artifact") + require.NoError(t, os.MkdirAll(artifactDir, 0750), "setup: create artifactDir") + require.NoError(t, os.WriteFile(filepath.Join(artifactDir, "log.txt"), []byte("log"), 0600), "setup: write file") + + err := flattenArtifactTree(artifactDir, artifactDir, outputDir, "verbose artifact", true) + require.NoError(t, err, "flattenArtifactTree should succeed in verbose mode") + + _, err = os.ReadFile(filepath.Join(outputDir, "log.txt")) + require.NoError(t, err, "file should be moved in verbose mode too") +}