Move engine output collection to separate file and simplify implement…

…ation (#17) * Initial plan * Implement engine-declared output artifact support - Add GetDeclaredOutputFiles() method to AgenticEngine interface - Claude engine declares output.txt as output file - Codex engine declares no output files (empty list) - Compiler conditionally generates output steps only for engines with declared outputs - Add OutputArtifactName constant for consistent artifact naming - Update prompt generation and environment variables to be conditional - Add comprehensive tests for engine capabilities and conditional compilation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Final cleanup and test improvements - Use OutputArtifactName constant in tests for consistency - Add fmt import to agentic_output_test.go - Ensure all tests pass with updated constant references Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Fix engine output collection to preserve GITHUB_AW_OUTPUT for all engines - GITHUB_AW_OUTPUT functionality is now preserved for ALL engines (Claude and Codex) - Engine-declared output files are collected separately as "agent_outputs" artifact - Only engines that declare output files (like Claude with "output.txt") get engine output collection steps - Codex engine continues to use GITHUB_AW_OUTPUT but doesn't get engine output collection since it declares no output files - The GITHUB_AW_OUTPUT artifact remains named "aw_output.txt" as before - Engine-declared files are collected in a separate "agent_outputs" artifact as requested Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Move engine output collection to separate file and simplify implementation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
pelikhan · Aug 23, 2025 · Aug 23, 2025 · Aug 23, 2025 · Aug 23, 2025 · Aug 23, 2025
commit 84bed2930acb196865a10028d85539a8b2286b05
diff --git a/.github/workflows/issue-triage.lock.yml b/.github/workflows/issue-triage.lock.yml
diff --git a/.github/workflows/test-claude.lock.yml b/.github/workflows/test-claude.lock.yml
diff --git a/.github/workflows/test-proxy.lock.yml b/.github/workflows/test-proxy.lock.yml
diff --git a/.github/workflows/weekly-research.lock.yml b/.github/workflows/weekly-research.lock.yml
diff --git a/pkg/workflow/agentic_engine.go b/pkg/workflow/agentic_engine.go
@@ -32,6 +32,10 @@ type AgenticEngine interface {
 	// SupportsMaxTurns returns true if this engine supports the max-turns feature
 	SupportsMaxTurns() bool
 
+	// GetDeclaredOutputFiles returns a list of output files that this engine may produce
+	// These files will be automatically uploaded as artifacts if they exist
+	GetDeclaredOutputFiles() []string
+
 	// GetInstallationSteps returns the GitHub Actions steps needed to install this engine
 	GetInstallationSteps(engineConfig *EngineConfig) []GitHubActionStep
 
@@ -102,6 +106,11 @@ func (e *BaseEngine) SupportsMaxTurns() bool {
 	return e.supportsMaxTurns
 }
 
+// GetDeclaredOutputFiles returns an empty list by default (engines can override)
+func (e *BaseEngine) GetDeclaredOutputFiles() []string {
+	return []string{}
+}
+
 // EngineRegistry manages available agentic engines
 type EngineRegistry struct {
 	engines map[string]AgenticEngine

diff --git a/pkg/workflow/agentic_output_test.go b/pkg/workflow/agentic_output_test.go
@@ -1,6 +1,7 @@
 package workflow
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
@@ -15,7 +16,7 @@ func TestAgenticOutputCollection(t *testing.T) {
 	}
 	defer os.RemoveAll(tmpDir)
 
-	// Test case with agentic output collection
+	// Test case with agentic output collection for Claude engine
 	testContent := `---
 on: push
 permissions:
@@ -54,127 +55,176 @@ This workflow tests the agentic output collection functionality.
 
 	lockContent := string(content)
 
-	// Verify pre-step: Setup agentic output file step exists
+	// Verify GITHUB_AW_OUTPUT functionality (should be present for all engines)
 	if !strings.Contains(lockContent, "- name: Setup agent output") {
 		t.Error("Expected 'Setup agent output' step to be in generated workflow")
 	}
 
-	// Verify the step uses github-script and sets up the output file
-	if !strings.Contains(lockContent, "uses: actions/github-script@v7") {
-		t.Error("Expected github-script action to be used for output file setup")
+	if !strings.Contains(lockContent, "- name: Collect agent output") {
+		t.Error("Expected 'Collect agent output' step to be in generated workflow")
+	}
+
+	if !strings.Contains(lockContent, "- name: Upload agentic output file") {
+		t.Error("Expected 'Upload agentic output file' step to be in generated workflow")
 	}
 
-	if !strings.Contains(lockContent, "const outputFile = `/tmp/aw_output_${randomId}.txt`;") {
-		t.Error("Expected output file creation in setup step")
+	// Verify job output declaration for GITHUB_AW_OUTPUT
+	if !strings.Contains(lockContent, "outputs:\n      output: ${{ steps.collect_output.outputs.output }}") {
+		t.Error("Expected job output declaration for 'output'")
 	}
 
-	if !strings.Contains(lockContent, "core.exportVariable('GITHUB_AW_OUTPUT', outputFile);") {
-		t.Error("Expected GITHUB_AW_OUTPUT environment variable to be set")
+	// Verify GITHUB_AW_OUTPUT is passed to Claude
+	if !strings.Contains(lockContent, "GITHUB_AW_OUTPUT: ${{ env.GITHUB_AW_OUTPUT }}") {
+		t.Error("Expected GITHUB_AW_OUTPUT environment variable to be passed to engine")
 	}
 
-	// Verify prompt injection: Check for output instructions in the prompt
+	// Verify prompt contains output instructions
 	if !strings.Contains(lockContent, "**IMPORTANT**: If you need to provide output that should be captured as a workflow output variable, write it to the file") {
 		t.Error("Expected output instructions to be injected into prompt")
 	}
 
-	if !strings.Contains(lockContent, "\"${{ env.GITHUB_AW_OUTPUT }}\"") {
-		t.Error("Expected GITHUB_AW_OUTPUT environment variable reference in prompt")
+	// Verify Claude engine declared outputs are uploaded separately
+	if !strings.Contains(lockContent, "- name: Upload engine output files") {
+		t.Error("Expected 'Upload engine output files' step for Claude engine")
 	}
 
-	// Verify environment variable is passed to agentic engine
-	if !strings.Contains(lockContent, "claude_env: |\n            GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}\n            GITHUB_AW_OUTPUT: ${{ env.GITHUB_AW_OUTPUT }}") {
-		t.Error("Expected GITHUB_AW_OUTPUT environment variable to be passed to Claude via claude_env")
+	if !strings.Contains(lockContent, "name: agent_outputs") {
+		t.Error("Expected engine output artifact to be named 'agent_outputs'")
 	}
 
-	// Verify post-step: Collect agentic output step exists
-	if !strings.Contains(lockContent, "- name: Collect agent output") {
-		t.Error("Expected 'Collect agent output' step to be in generated workflow")
+	// Verify that both artifacts are uploaded
+	if !strings.Contains(lockContent, fmt.Sprintf("name: %s", OutputArtifactName)) {
+		t.Errorf("Expected GITHUB_AW_OUTPUT artifact name to be '%s'", OutputArtifactName)
 	}
 
-	if !strings.Contains(lockContent, "id: collect_output") {
-		t.Error("Expected collect_output step ID")
+	t.Log("Claude workflow correctly includes both GITHUB_AW_OUTPUT and engine output collection")
+}
+
+func TestCodexEngineNoOutputSteps(t *testing.T) {
+	// Create temporary directory for test files
+	tmpDir, err := os.MkdirTemp("", "codex-no-output-test")
+	if err != nil {
+		t.Fatal(err)
 	}
+	defer os.RemoveAll(tmpDir)
+
+	// Test case with Codex engine (should have GITHUB_AW_OUTPUT but no engine output collection)
+	testContent := `---
+on: push
+permissions:
+  contents: read
+  issues: write
+tools:
+  github:
+    allowed: [list_issues]
+engine: codex
+---
+
+# Test Codex No Engine Output Collection
+
+This workflow tests that Codex engine gets GITHUB_AW_OUTPUT but not engine output collection.
+`
 
-	if !strings.Contains(lockContent, "const outputFile = process.env.GITHUB_AW_OUTPUT;") {
-		t.Error("Expected output file reading in collection step")
+	testFile := filepath.Join(tmpDir, "test-codex-no-output.md")
+	if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil {
+		t.Fatal(err)
 	}
 
-	if !strings.Contains(lockContent, "core.setOutput('output', sanitizedContent);") {
-		t.Error("Expected sanitized output to be set in collection step")
+	compiler := NewCompiler(false, "", "test")
+
+	// Compile the workflow
+	err = compiler.CompileWorkflow(testFile)
+	if err != nil {
+		t.Fatalf("Unexpected error compiling workflow with Codex: %v", err)
 	}
 
-	// Verify sanitization function is included
-	if !strings.Contains(lockContent, "function sanitizeContent(content) {") {
-		t.Error("Expected sanitization function to be in collection step")
+	// Read the generated lock file
+	lockFile := filepath.Join(tmpDir, "test-codex-no-output.lock.yml")
+	content, err := os.ReadFile(lockFile)
+	if err != nil {
+		t.Fatalf("Failed to read generated lock file: %v", err)
 	}
 
-	if !strings.Contains(lockContent, "const sanitizedContent = sanitizeContent(outputContent);") {
-		t.Error("Expected sanitization function to be called on output content")
+	lockContent := string(content)
+
+	// Verify that Codex workflow DOES have GITHUB_AW_OUTPUT functionality
+	if !strings.Contains(lockContent, "- name: Setup agent output") {
+		t.Error("Codex workflow should have 'Setup agent output' step (GITHUB_AW_OUTPUT functionality)")
 	}
 
-	// Verify job output declaration
-	if !strings.Contains(lockContent, "outputs:\n      output: ${{ steps.collect_output.outputs.output }}") {
-		t.Error("Expected job output declaration for 'output'")
+	if !strings.Contains(lockContent, "- name: Collect agent output") {
+		t.Error("Codex workflow should have 'Collect agent output' step (GITHUB_AW_OUTPUT functionality)")
 	}
 
-	// Verify artifact upload step: Upload agentic output file step exists
 	if !strings.Contains(lockContent, "- name: Upload agentic output file") {
-		t.Error("Expected 'Upload agentic output file' step to be in generated workflow")
+		t.Error("Codex workflow should have 'Upload agentic output file' step (GITHUB_AW_OUTPUT functionality)")
 	}
 
-	// Verify the upload step uses actions/upload-artifact@v4
-	if !strings.Contains(lockContent, "uses: actions/upload-artifact@v4") {
-		t.Error("Expected upload-artifact action to be used for artifact upload step")
+	if !strings.Contains(lockContent, "GITHUB_AW_OUTPUT") {
+		t.Error("Codex workflow should reference GITHUB_AW_OUTPUT environment variable")
 	}
 
-	// Verify the artifact upload configuration
-	if !strings.Contains(lockContent, "name: aw_output.txt") {
-		t.Error("Expected artifact name to be 'aw_output.txt'")
+	if !strings.Contains(lockContent, fmt.Sprintf("name: %s", OutputArtifactName)) {
+		t.Errorf("Codex workflow should reference %s artifact (GITHUB_AW_OUTPUT)", OutputArtifactName)
 	}
 
-	if !strings.Contains(lockContent, "path: ${{ env.GITHUB_AW_OUTPUT }}") {
-		t.Error("Expected artifact path to use GITHUB_AW_OUTPUT environment variable")
+	// Verify that job outputs section includes output for GITHUB_AW_OUTPUT
+	if !strings.Contains(lockContent, "outputs:\n      output: ${{ steps.collect_output.outputs.output }}") {
+		t.Error("Codex workflow should have job output declaration for 'output' (GITHUB_AW_OUTPUT)")
 	}
 
-	if !strings.Contains(lockContent, "if-no-files-found: warn") {
-		t.Error("Expected if-no-files-found: warn configuration for artifact upload")
+	// Verify that Codex workflow does NOT have engine output collection steps
+	if strings.Contains(lockContent, "- name: Collect engine output files") {
+		t.Error("Codex workflow should NOT have 'Collect engine output files' step")
 	}
 
-	// Verify the upload step condition checks for non-empty output
-	if !strings.Contains(lockContent, "if: always() && steps.collect_output.outputs.output != ''") {
-		t.Error("Expected upload step to check for non-empty output from collection step")
+	if strings.Contains(lockContent, "- name: Upload engine output files") {
+		t.Error("Codex workflow should NOT have 'Upload engine output files' step")
 	}
 
-	// Verify step order: setup should come before agentic execution, collection should come after
-	setupIndex := strings.Index(lockContent, "- name: Setup agent output")
-	executeIndex := strings.Index(lockContent, "- name: Execute Claude Code Action")
-	collectIndex := strings.Index(lockContent, "- name: Collect agent output")
-	uploadIndex := strings.Index(lockContent, "- name: Upload agentic output file")
-
-	// If "Execute Claude Code" isn't found, try alternative step names
-	if executeIndex == -1 {
-		executeIndex = strings.Index(lockContent, "- name: Execute Claude")
-	}
-	if executeIndex == -1 {
-		executeIndex = strings.Index(lockContent, "uses: githubnext/claude-action")
+	if strings.Contains(lockContent, "name: agent_outputs") {
+		t.Error("Codex workflow should NOT reference 'agent_outputs' artifact")
 	}
 
-	if setupIndex == -1 || executeIndex == -1 || collectIndex == -1 || uploadIndex == -1 {
-		t.Fatal("Could not find expected steps in generated workflow")
+	// Verify that the Codex execution step is still present
+	if !strings.Contains(lockContent, "- name: Run Codex") {
+		t.Error("Expected 'Run Codex' step to be in generated workflow")
 	}
 
-	if setupIndex >= executeIndex {
-		t.Error("Setup step should appear before agentic execution step")
+	t.Log("Codex workflow correctly includes GITHUB_AW_OUTPUT functionality but excludes engine output collection")
+}
+
+func TestEngineOutputFileDeclarations(t *testing.T) {
+	// Test Claude engine declares output files
+	claudeEngine := NewClaudeEngine()
+	claudeOutputFiles := claudeEngine.GetDeclaredOutputFiles()
+
+	if len(claudeOutputFiles) == 0 {
+		t.Error("Claude engine should declare at least one output file")
 	}
 
-	if collectIndex <= executeIndex {
-		t.Error("Collection step should appear after agentic execution step")
+	if !stringSliceContains(claudeOutputFiles, "output.txt") {
+		t.Errorf("Claude engine should declare 'output.txt' as an output file, got: %v", claudeOutputFiles)
 	}
 
-	if uploadIndex <= collectIndex {
-		t.Error("Upload step should appear after collection step")
+	// Test Codex engine declares no output files
+	codexEngine := NewCodexEngine()
+	codexOutputFiles := codexEngine.GetDeclaredOutputFiles()
+
+	if len(codexOutputFiles) != 0 {
+		t.Errorf("Codex engine should declare no output files, got: %v", codexOutputFiles)
 	}
 
-	t.Logf("Step order verified: Setup (%d) < Execute (%d) < Collect (%d) < Upload (%d)",
-		setupIndex, executeIndex, collectIndex, uploadIndex)
+	t.Logf("Claude engine declares: %v", claudeOutputFiles)
+	t.Logf("Codex engine declares: %v", codexOutputFiles)
+}
+
+// Helper function to check if a string slice contains a specific string
+func stringSliceContains(slice []string, item string) bool {
+	for _, s := range slice {
+		if s == item {
+			return true
+		}
+	}
+	return false
 }
diff --git a/pkg/workflow/claude_engine.go b/pkg/workflow/claude_engine.go
@@ -35,6 +35,11 @@ func (e *ClaudeEngine) GetInstallationSteps(engineConfig *EngineConfig) []GitHub
 	return []GitHubActionStep{}
 }
 
+// GetDeclaredOutputFiles returns the output files that Claude may produce
+func (e *ClaudeEngine) GetDeclaredOutputFiles() []string {
+	return []string{"output.txt"}
+}
+
 func (e *ClaudeEngine) GetExecutionConfig(workflowName string, logFile string, engineConfig *EngineConfig) ExecutionConfig {
 	// Determine the action version to use
 	actionVersion := DefaultClaudeActionVersion // Default version