diff --git a/pkg/cli/compile_force_refresh_action_pins_test.go b/pkg/cli/compile_force_refresh_action_pins_test.go index 31bd1fdc25..edc14ef1c9 100644 --- a/pkg/cli/compile_force_refresh_action_pins_test.go +++ b/pkg/cli/compile_force_refresh_action_pins_test.go @@ -9,55 +9,10 @@ import ( "testing" "github.com/github/gh-aw/pkg/testutil" - "github.com/github/gh-aw/pkg/workflow" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestForceRefreshActionPins_ClearCache(t *testing.T) { - // Create temporary directory for testing - tmpDir := testutil.TempDir(t, "test-*") - - // Change to temp directory to simulate running from repo root - oldCwd, err := os.Getwd() - require.NoError(t, err, "Failed to get current working directory") - defer func() { - _ = os.Chdir(oldCwd) - }() - - err = os.Chdir(tmpDir) - require.NoError(t, err, "Failed to change to temp directory") - - // Create a cache with some entries - cache := workflow.NewActionCache(tmpDir) - cache.Set("actions/checkout", "v5", "abc123") - cache.Set("actions/setup-node", "v4", "def456") - err = cache.Save() - require.NoError(t, err, "Failed to save initial cache") - - // Verify cache file exists and has entries - cachePath := filepath.Join(tmpDir, ".github", "aw", workflow.CacheFileName) - require.FileExists(t, cachePath, "Cache file should exist before test") - - // Load the cache to verify it has entries - testCache := workflow.NewActionCache(tmpDir) - err = testCache.Load() - require.NoError(t, err, "Failed to load cache") - assert.Len(t, testCache.Entries, 2, "Cache should have 2 entries before force refresh") - - // Create compiler with force refresh enabled - compiler := workflow.NewCompiler( - workflow.WithVersion("test"), - ) - compiler.SetForceRefreshActionPins(true) - - // Get the shared action resolver - this should skip loading the cache - actionCache, _ := compiler.GetSharedActionResolverForTest() - - // Verify cache is empty (not loaded from disk) - assert.Empty(t, actionCache.Entries, "Cache should be empty when force refresh is enabled") -} - func TestForceRefreshActionPins_ResetFile(t *testing.T) { // Create temporary directory for testing tmpDir := testutil.TempDir(t, "test-*") diff --git a/pkg/workflow/compiler_types.go b/pkg/workflow/compiler_types.go index c21b42d9b1..c5d4041d9a 100644 --- a/pkg/workflow/compiler_types.go +++ b/pkg/workflow/compiler_types.go @@ -292,12 +292,6 @@ func (c *Compiler) getSharedActionResolver() (*ActionCache, *ActionResolver) { return c.actionCache, c.actionResolver } -// GetSharedActionResolverForTest exposes the shared action resolver for testing purposes -// This should only be used in tests -func (c *Compiler) GetSharedActionResolverForTest() (*ActionCache, *ActionResolver) { - return c.getSharedActionResolver() -} - // getSharedImportCache returns the shared import cache, initializing it on first use // This ensures all workflows compiled by this compiler instance share the same import cache func (c *Compiler) getSharedImportCache() *parser.ImportCache { diff --git a/pkg/workflow/prompts_test.go b/pkg/workflow/prompts_test.go index 5723f32112..de4c974cc2 100644 --- a/pkg/workflow/prompts_test.go +++ b/pkg/workflow/prompts_test.go @@ -15,156 +15,12 @@ import ( // Safe Outputs Prompt Tests // ============================================================================ -func TestGenerateSafeOutputsPromptStep_IncludesWhenEnabled(t *testing.T) { - // Test that safe outputs are included in unified prompt step when enabled - compiler := &Compiler{} - var yaml strings.Builder - - // Create a config with create-issue enabled - safeOutputs := &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{}, - } - - data := &WorkflowData{ - ParsedTools: NewTools(map[string]any{}), - SafeOutputs: safeOutputs, - } - - compiler.generateUnifiedPromptStep(&yaml, data) - - output := yaml.String() - if !strings.Contains(output, "Create prompt with built-in context") { - t.Error("Expected unified prompt step to be generated when safe outputs enabled") - } - // Static intro is now in safe_outputs_prompt.md (referenced by file, not inline) - if !strings.Contains(output, "safe_outputs_prompt.md") { - t.Error("Expected reference to safe_outputs_prompt.md for static safe outputs intro") - } - // Per-tool instructions are still inline - if !strings.Contains(output, "create_issue") { - t.Error("Expected prompt to include create_issue tool name") - } -} - -func TestGenerateSafeOutputsPromptStep_SkippedWhenDisabled(t *testing.T) { - // Test that safe outputs are not included in unified prompt step when disabled - compiler := &Compiler{} - var yaml strings.Builder - - data := &WorkflowData{ - ParsedTools: NewTools(map[string]any{}), - SafeOutputs: nil, - } - - compiler.generateUnifiedPromptStep(&yaml, data) - - output := yaml.String() - // Should still have unified step (for temp folder), but not safe outputs - if strings.Contains(output, "safe_outputs_prompt.md") { - t.Error("Expected safe outputs section to NOT be in unified prompt when disabled") - } -} - func TestSafeOutputsPromptText_FollowsXMLFormat(t *testing.T) { // This test is for the embedded prompt text which is no longer used // Skip it as we now generate the prompt dynamically t.Skip("Safe outputs prompt is now generated dynamically based on enabled tools") } -func TestSafeOutputsPrompt_IncludesPerToolInstructions(t *testing.T) { - // Test that per-tool instructions are included in the safe outputs prompt - // for each enabled tool, helping the agent understand how to use them. - compiler := &Compiler{} - var yaml strings.Builder - - // Create a config with multiple safe outputs enabled - safeOutputs := &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{}, - AddComments: &AddCommentsConfig{}, - CreateDiscussions: &CreateDiscussionsConfig{}, - UpdateIssues: &UpdateIssuesConfig{}, - } - - data := &WorkflowData{ - ParsedTools: NewTools(map[string]any{}), - SafeOutputs: safeOutputs, - } - - compiler.generateUnifiedPromptStep(&yaml, data) - output := yaml.String() - - // Static intro is now in safe_outputs_prompt.md (file reference, not inline) - if !strings.Contains(output, "safe_outputs_prompt.md") { - t.Fatal("Expected safe_outputs_prompt.md file reference in generated prompt") - } - - // Per-tool instructions are wrapped in - if !strings.Contains(output, "") { - t.Fatal("Expected section in generated prompt") - } - - // Verify enabled tool names are present - for _, toolName := range []string{"create_issue", "add_comment", "create_discussion", "update_issue"} { - t.Run(toolName, func(t *testing.T) { - if !strings.Contains(output, toolName) { - t.Errorf("Expected per-tool instruction to include tool name %q", toolName) - } - }) - } -} - -func TestSafeOutputsPrompt_AlwaysIncludesNoop(t *testing.T) { - // noop should always appear in the Tools list for any - // workflow that has a safe-outputs section, regardless of whether noop was - // explicitly listed in the frontmatter (it is auto-injected). - tests := []struct { - name string - safeOutputs *SafeOutputsConfig - }{ - { - name: "noop only", - safeOutputs: &SafeOutputsConfig{ - NoOp: &NoOpConfig{}, - }, - }, - { - name: "noop with other tools", - safeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{}, - NoOp: &NoOpConfig{}, - }, - }, - { - name: "auto-injected noop (missing_tool and missing_data auto-enabled)", - safeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{}, - MissingTool: &MissingToolConfig{}, - MissingData: &MissingDataConfig{}, - NoOp: &NoOpConfig{}, - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - compiler := &Compiler{} - var yaml strings.Builder - - data := &WorkflowData{ - ParsedTools: NewTools(map[string]any{}), - SafeOutputs: tt.safeOutputs, - } - - compiler.generateUnifiedPromptStep(&yaml, data) - output := yaml.String() - - if !strings.Contains(output, "noop") { - t.Errorf("Expected 'noop' to be present in Tools list, got:\n%s", output) - } - }) - } -} - // ============================================================================ // Cache Memory Prompt Tests // ============================================================================ diff --git a/pkg/workflow/safe_outputs_default_create_issue_test.go b/pkg/workflow/safe_outputs_default_create_issue_test.go index b2c9ddf11b..ecbf1e119c 100644 --- a/pkg/workflow/safe_outputs_default_create_issue_test.go +++ b/pkg/workflow/safe_outputs_default_create_issue_test.go @@ -4,7 +4,6 @@ package workflow import ( "fmt" - "strings" "testing" "github.com/stretchr/testify/assert" @@ -255,67 +254,6 @@ func TestAutoInjectCreateIssue(t *testing.T) { } } -// TestAutoInjectedCreateIssuePrompt verifies that the auto-injected create-issue produces -// a specific prompt instruction to create an issue with results or call noop. -func TestAutoInjectedCreateIssuePrompt(t *testing.T) { - tests := []struct { - name string - safeOutputs *SafeOutputsConfig - expectSpecific bool // expect the auto_create_issue file reference - }{ - { - name: "auto-injected create-issue produces specific prompt", - safeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, - Labels: []string{"my-workflow"}, - TitlePrefix: "[my-workflow]", - }, - AutoInjectedCreateIssue: true, - }, - expectSpecific: true, - }, - { - name: "user-configured create-issue does NOT produce specific prompt", - safeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{ - TitlePrefix: "[custom]", - }, - AutoInjectedCreateIssue: false, - }, - expectSpecific: false, - }, - { - name: "no create-issue configured", - safeOutputs: &SafeOutputsConfig{ - AddComments: &AddCommentsConfig{}, - }, - expectSpecific: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - compiler := &Compiler{} - var yaml strings.Builder - data := &WorkflowData{ - ParsedTools: NewTools(map[string]any{}), - SafeOutputs: tt.safeOutputs, - } - compiler.generateUnifiedPromptStep(&yaml, data) - output := yaml.String() - - if tt.expectSpecific { - assert.Contains(t, output, safeOutputsAutoCreateIssueFile, - "Auto-injected create-issue should include the auto_create_issue file reference") - } else { - assert.NotContains(t, output, safeOutputsAutoCreateIssueFile, - "Non-auto-injected create-issue should not include the auto_create_issue file reference") - } - }) - } -} - // TestAutoInjectCreateIssueWithVariousWorkflowIDs verifies correct label/prefix generation func TestAutoInjectCreateIssueWithVariousWorkflowIDs(t *testing.T) { workflowIDs := []string{ diff --git a/pkg/workflow/unified_prompt_step.go b/pkg/workflow/unified_prompt_step.go index afabe94778..96a4ff50d3 100644 --- a/pkg/workflow/unified_prompt_step.go +++ b/pkg/workflow/unified_prompt_step.go @@ -24,125 +24,6 @@ type PromptSection struct { EnvVars map[string]string } -// generateUnifiedPromptStep generates a single workflow step that appends all prompt sections. -// This consolidates what used to be multiple separate steps (temp folder, playwright, safe outputs, -// GitHub context, PR context, cache memory, repo memory) into one step. -func (c *Compiler) generateUnifiedPromptStep(yaml *strings.Builder, data *WorkflowData) { - unifiedPromptLog.Print("Generating unified prompt step") - - // Get the heredoc delimiter for consistent usage - delimiter := GenerateHeredocDelimiter("PROMPT") - - // Collect all prompt sections in order - sections := c.collectPromptSections(data) - - if len(sections) == 0 { - unifiedPromptLog.Print("No prompt sections to append, skipping unified step") - return - } - - unifiedPromptLog.Printf("Collected %d prompt sections", len(sections)) - - // Collect all environment variables from all sections - // Only include GitHub Actions expressions in the prompt creation step - // Static values should only be in the substitution step - allEnvVars := make(map[string]string) - for _, section := range sections { - for key, value := range section.EnvVars { - // Only add GitHub Actions expressions to the prompt creation step - // Static values (not wrapped in ${{ }}) are for the substitution step only - if strings.HasPrefix(value, "${{ ") && strings.HasSuffix(value, " }}") { - allEnvVars[key] = value - } - } - } - - // Generate the step - yaml.WriteString(" - name: Create prompt with built-in context\n") - yaml.WriteString(" env:\n") - yaml.WriteString(" GH_AW_PROMPT: /tmp/gh-aw/aw-prompts/prompt.txt\n") - - // Add all environment variables in sorted order for consistency - var envKeys []string - for key := range allEnvVars { - envKeys = append(envKeys, key) - } - sort.Strings(envKeys) - for _, key := range envKeys { - fmt.Fprintf(yaml, " %s: %s\n", key, allEnvVars[key]) - } - - yaml.WriteString(" run: |\n") - - // Track if we're inside a heredoc - inHeredoc := false - - // Write each section's content - for i, section := range sections { - unifiedPromptLog.Printf("Writing section %d/%d: hasCondition=%v, isFile=%v", - i+1, len(sections), section.ShellCondition != "", section.IsFile) - - if section.ShellCondition != "" { - // Close heredoc if open, add conditional - if inHeredoc { - yaml.WriteString(" " + delimiter + "\n") - inHeredoc = false - } - fmt.Fprintf(yaml, " if %s; then\n", section.ShellCondition) - - if section.IsFile { - // File reference inside conditional - promptPath := fmt.Sprintf("%s/%s", promptsDir, section.Content) - yaml.WriteString(" " + fmt.Sprintf("cat \"%s\" >> \"$GH_AW_PROMPT\"\n", promptPath)) - } else { - // Inline content inside conditional - open heredoc, write content, close - yaml.WriteString(" cat << '" + delimiter + "' >> \"$GH_AW_PROMPT\"\n") - normalizedContent := normalizeLeadingWhitespace(section.Content) - cleanedContent := removeConsecutiveEmptyLines(normalizedContent) - contentLines := strings.SplitSeq(cleanedContent, "\n") - for line := range contentLines { - yaml.WriteString(" " + line + "\n") - } - yaml.WriteString(" " + delimiter + "\n") - } - - yaml.WriteString(" fi\n") - } else { - // Unconditional section - if section.IsFile { - // Close heredoc if open - if inHeredoc { - yaml.WriteString(" " + delimiter + "\n") - inHeredoc = false - } - // Cat the file - promptPath := fmt.Sprintf("%s/%s", promptsDir, section.Content) - yaml.WriteString(" " + fmt.Sprintf("cat \"%s\" >> \"$GH_AW_PROMPT\"\n", promptPath)) - } else { - // Inline content - open heredoc if not already open - if !inHeredoc { - yaml.WriteString(" cat << '" + delimiter + "' >> \"$GH_AW_PROMPT\"\n") - inHeredoc = true - } - // Write content directly to open heredoc - normalizedContent := normalizeLeadingWhitespace(section.Content) - cleanedContent := removeConsecutiveEmptyLines(normalizedContent) - contentLines := strings.SplitSeq(cleanedContent, "\n") - for line := range contentLines { - yaml.WriteString(" " + line + "\n") - } - } - } - } - - // Close heredoc if still open - if inHeredoc { - yaml.WriteString(" " + delimiter + "\n") - } - - unifiedPromptLog.Print("Unified prompt step generated successfully") -} - // normalizeLeadingWhitespace removes consistent leading whitespace from all lines // This handles content that was generated with indentation for heredocs func normalizeLeadingWhitespace(content string) string { diff --git a/pkg/workflow/unified_prompt_step_test.go b/pkg/workflow/unified_prompt_step_test.go index d90832aa63..7f86c5ad71 100644 --- a/pkg/workflow/unified_prompt_step_test.go +++ b/pkg/workflow/unified_prompt_step_test.go @@ -10,170 +10,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestGenerateUnifiedPromptStep_AllSections(t *testing.T) { - // Test that all prompt sections are included when all features are enabled - compiler := &Compiler{ - trialMode: false, - trialLogicalRepoSlug: "", - } - - data := &WorkflowData{ - ParsedTools: NewTools(map[string]any{ - "playwright": true, - "github": true, - }), - CacheMemoryConfig: &CacheMemoryConfig{ - Caches: []CacheMemoryEntry{ - {ID: "default"}, - }, - }, - RepoMemoryConfig: &RepoMemoryConfig{ - Memories: []RepoMemoryEntry{ - {ID: "default", BranchName: "memory"}, - }, - }, - SafeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{}, - }, - Permissions: "contents: read", - On: "issue_comment", - } - - var yaml strings.Builder - compiler.generateUnifiedPromptStep(&yaml, data) - - output := yaml.String() - - // Verify single step is created with correct name - assert.Contains(t, output, "- name: Create prompt with built-in context") - - // Verify all sections are included - assert.Contains(t, output, "temp_folder_prompt.md", "Should include temp folder instructions") - assert.Contains(t, output, "playwright_prompt.md", "Should include playwright instructions") - assert.Contains(t, output, "cache_memory_prompt.md", "Should include cache memory template file") - assert.Contains(t, output, "repo_memory_prompt.md", "Should include repo memory template file") - assert.Contains(t, output, "safe_outputs_prompt.md", "Should include safe outputs file reference") - assert.Contains(t, output, "", "Should include per-tool instructions") - assert.Contains(t, output, "", "Should include GitHub context") - - // Verify cache env vars are NOT in the prompt creation step - promptStepStart := strings.Index(output, "- name: Create prompt with built-in context") - promptStepEnd := strings.Index(output, "- name:") - if promptStepEnd <= promptStepStart { - promptStepEnd = len(output) - } - promptStep := output[promptStepStart:promptStepEnd] - assert.NotContains(t, promptStep, "GH_AW_CACHE_DIR:", "Cache env vars should not be in prompt creation step") - - // Verify environment variables are declared at the top - lines := strings.Split(output, "\n") - envSectionStarted := false - runSectionStarted := false - for _, line := range lines { - if strings.Contains(line, "env:") { - envSectionStarted = true - } - if strings.Contains(line, "run: |") { - runSectionStarted = true - } - // Check that environment variable declarations (key: ${{ ... }}) are in env section - // Skip lines that are just references to the variables (like __GH_AW_GITHUB_ACTOR__) - if strings.Contains(line, ": ${{") && runSectionStarted { - t.Errorf("Found environment variable declaration after run section started: %s", line) - } - } - assert.True(t, envSectionStarted, "Should have env section") - assert.True(t, runSectionStarted, "Should have run section") -} - -func TestGenerateUnifiedPromptStep_MinimalSections(t *testing.T) { - // Test that only temp folder is included when no other features are enabled - compiler := &Compiler{ - trialMode: false, - trialLogicalRepoSlug: "", - } - - data := &WorkflowData{ - ParsedTools: NewTools(map[string]any{}), - CacheMemoryConfig: nil, - RepoMemoryConfig: nil, - SafeOutputs: nil, - Permissions: "", - On: "push", - } - - var yaml strings.Builder - compiler.generateUnifiedPromptStep(&yaml, data) - - output := yaml.String() - - // Verify single step is created - assert.Contains(t, output, "- name: Create prompt with built-in context") - - // Verify only temp folder is included - assert.Contains(t, output, "temp_folder_prompt.md", "Should include temp folder instructions") - - // Verify other sections are NOT included - assert.NotContains(t, output, "playwright_prompt.md", "Should not include playwright without tool") - assert.NotContains(t, output, "cache_memory_prompt.md", "Should not include cache memory template without config") - assert.NotContains(t, output, "repo_memory_prompt.md", "Should not include repo memory without config") - assert.NotContains(t, output, "", "Should not include safe outputs without config") - assert.NotContains(t, output, "", "Should not include GitHub context without tool") -} - -func TestGenerateUnifiedPromptStep_TrialMode(t *testing.T) { - // Test that trial mode note is included - compiler := &Compiler{ - trialMode: true, - trialLogicalRepoSlug: "owner/repo", - } - - data := &WorkflowData{ - ParsedTools: NewTools(map[string]any{}), - CacheMemoryConfig: nil, - RepoMemoryConfig: nil, - SafeOutputs: nil, - Permissions: "", - On: "push", - } - - var yaml strings.Builder - compiler.generateUnifiedPromptStep(&yaml, data) - - output := yaml.String() - - // Verify trial mode note is included - assert.Contains(t, output, "## Note") - assert.Contains(t, output, "owner/repo") -} - -func TestGenerateUnifiedPromptStep_PRContext(t *testing.T) { - // Test that PR context is included with proper condition - compiler := &Compiler{ - trialMode: false, - trialLogicalRepoSlug: "", - } - - data := &WorkflowData{ - ParsedTools: NewTools(map[string]any{}), - CacheMemoryConfig: nil, - RepoMemoryConfig: nil, - SafeOutputs: nil, - Permissions: "contents: read", - On: "issue_comment", - } - - var yaml strings.Builder - compiler.generateUnifiedPromptStep(&yaml, data) - - output := yaml.String() - - // Verify PR context is included with condition - assert.Contains(t, output, "pr_context_prompt.md", "Should include PR context file") - assert.Contains(t, output, "if [", "Should have shell conditional for PR context") - assert.Contains(t, output, "GITHUB_EVENT_NAME", "Should check event name") -} - func TestCollectPromptSections_Order(t *testing.T) { // Test that sections are collected in the correct order compiler := &Compiler{ @@ -257,27 +93,6 @@ func TestCollectPromptSections_Order(t *testing.T) { } } -func TestGenerateUnifiedPromptStep_NoSections(t *testing.T) { - // This should never happen in practice, but test the edge case - compiler := &Compiler{ - trialMode: false, - } - - // Create minimal data that would result in at least temp folder - data := &WorkflowData{ - ParsedTools: NewTools(map[string]any{}), - } - - var yaml strings.Builder - compiler.generateUnifiedPromptStep(&yaml, data) - - output := yaml.String() - - // Should still generate step with at least temp folder - assert.Contains(t, output, "- name: Create prompt with built-in context") - assert.Contains(t, output, "temp_folder_prompt.md") -} - func TestNormalizeLeadingWhitespace(t *testing.T) { tests := []struct { name string @@ -399,71 +214,6 @@ Line 1`, } } -func TestGenerateUnifiedPromptStep_EnvVarsSorted(t *testing.T) { - // Test that environment variables are sorted alphabetically - compiler := &Compiler{ - trialMode: false, - trialLogicalRepoSlug: "", - } - - data := &WorkflowData{ - ParsedTools: NewTools(map[string]any{ - "github": true, - }), - CacheMemoryConfig: nil, - RepoMemoryConfig: nil, - SafeOutputs: nil, - Permissions: "", - On: "push", - } - - var yaml strings.Builder - compiler.generateUnifiedPromptStep(&yaml, data) - - output := yaml.String() - - // Verify environment variables are present and sorted - lines := strings.Split(output, "\n") - envSectionStarted := false - runSectionStarted := false - var envVarLines []string - - for _, line := range lines { - if strings.Contains(line, "env:") { - envSectionStarted = true - continue - } - if strings.Contains(line, "run: |") { - runSectionStarted = true - break - } - if envSectionStarted && strings.Contains(line, ": ${{") { - // Extract just the variable name (before the colon) - trimmed := strings.TrimSpace(line) - colonIndex := strings.Index(trimmed, ":") - if colonIndex > 0 { - varName := trimmed[:colonIndex] - envVarLines = append(envVarLines, varName) - } - } - } - - assert.True(t, runSectionStarted, "Should have found run section") - - // Verify that environment variables (excluding GH_AW_PROMPT which is always first) are sorted - // Skip the first entry which is GH_AW_PROMPT - if len(envVarLines) > 0 { - // Check that the remaining variables are in sorted order - for i := range len(envVarLines) - 1 { - current := envVarLines[i] - next := envVarLines[i+1] - if current > next { - t.Errorf("Environment variables are not sorted: %s comes before %s", current, next) - } - } - } -} - func TestCollectPromptSections_DisableXPIA(t *testing.T) { // Test that XPIA section is excluded when disable-xpia-prompt feature flag is set compiler := &Compiler{