diff --git a/pkg/workflow/imports.go b/pkg/workflow/imports.go index d6a2ef8b00..dc7f4af430 100644 --- a/pkg/workflow/imports.go +++ b/pkg/workflow/imports.go @@ -630,8 +630,12 @@ func mergeSafeOutputConfig(result *SafeOutputsConfig, config map[string]any, c * if result.NoOp == nil && importedConfig.NoOp != nil { result.NoOp = importedConfig.NoOp } - if result.ThreatDetection == nil && importedConfig.ThreatDetection != nil { - result.ThreatDetection = importedConfig.ThreatDetection + // ThreatDetection is a workflow-level concern; only merge from an import that + // explicitly carries a threat-detection key (not just an auto-enabled default). + if result.ThreatDetection == nil { + if _, hasTD := config["threat-detection"]; hasTD && importedConfig.ThreatDetection != nil { + result.ThreatDetection = importedConfig.ThreatDetection + } } // Merge meta-configuration fields (only set if empty/zero in result) diff --git a/pkg/workflow/safe_outputs_import_test.go b/pkg/workflow/safe_outputs_import_test.go index 228d030d23..a8e0b00283 100644 --- a/pkg/workflow/safe_outputs_import_test.go +++ b/pkg/workflow/safe_outputs_import_test.go @@ -1823,3 +1823,107 @@ safe-outputs: assert.Equal(t, "Shared started", workflowData.SafeOutputs.Messages.RunStarted, "RunStarted should come from shared") assert.Equal(t, "Shared failure", workflowData.SafeOutputs.Messages.RunFailure, "RunFailure should come from shared") } + +// TestMergeSafeOutputsThreatDetectionExplicitDisableNotOverridden tests that when the main workflow +// explicitly disables threat-detection, imported fragments with no threat-detection key do not +// re-enable it. +func TestMergeSafeOutputsThreatDetectionExplicitDisableNotOverridden(t *testing.T) { + compiler := NewCompilerWithVersion("1.0.0") + + // Simulate main workflow that explicitly disabled threat-detection: + // threat-detection: false → parseThreatDetectionConfig returns nil. + topConfig := &SafeOutputsConfig{ + ThreatDetection: nil, + AddComments: &AddCommentsConfig{}, + } + + // Import fragment with safe-outputs but no threat-detection key. + importedJSON := []string{ + `{"add-comment":{"max":1}}`, + } + + result, err := compiler.MergeSafeOutputs(topConfig, importedJSON) + require.NoError(t, err, "MergeSafeOutputs should not error") + require.NotNil(t, result, "Result should not be nil") + + // The explicit disable must survive the merge: threat detection must remain nil. + assert.Nil(t, result.ThreatDetection, "ThreatDetection must remain nil when explicitly disabled by main workflow") +} + +// TestMergeSafeOutputsThreatDetectionImportedWhenExplicit tests that an import that explicitly +// carries a threat-detection key can set it when the main workflow has not configured it. +func TestMergeSafeOutputsThreatDetectionImportedWhenExplicit(t *testing.T) { + compiler := NewCompilerWithVersion("1.0.0") + + // Import fragment that explicitly enables threat-detection. + importedJSON := []string{ + `{"add-comment":{"max":1},"threat-detection":{"enabled":true}}`, + } + + result, err := compiler.MergeSafeOutputs(nil, importedJSON) + require.NoError(t, err, "MergeSafeOutputs should not error") + require.NotNil(t, result, "Result should not be nil") + + // Import explicitly set threat-detection, so it should be present. + assert.NotNil(t, result.ThreatDetection, "ThreatDetection should be set when explicitly configured in import") +} + +// TestSafeOutputsImportDoesNotReenableThreatDetection is an integration test that reproduces +// the bug where an imported fragment re-enables threat-detection that was explicitly disabled +// in the main workflow. This caused a compilation error when sandbox.agent was also false. +func TestSafeOutputsImportDoesNotReenableThreatDetection(t *testing.T) { + compiler := NewCompilerWithVersion("1.0.0") + + tmpDir := t.TempDir() + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + err := os.MkdirAll(workflowsDir, 0755) + require.NoError(t, err, "Failed to create workflows directory") + + // Fragment with safe-outputs but no threat-detection key (mimics safe-output-add-comment.md) + sharedWorkflow := `--- +safe-outputs: + add-comment: + max: 1 +--- + +# Shared Add Comment Fragment +` + + sharedFile := filepath.Join(workflowsDir, "safe-output-add-comment.md") + err = os.WriteFile(sharedFile, []byte(sharedWorkflow), 0644) + require.NoError(t, err, "Failed to write shared file") + + // Main workflow: sandbox.agent disabled + threat-detection explicitly disabled + mainWorkflow := `--- +on: issues +engine: copilot +strict: false +sandbox: + agent: false +imports: + - ./safe-output-add-comment.md +safe-outputs: + activation-comments: false + threat-detection: false +--- + +# Main Workflow +` + + mainFile := filepath.Join(workflowsDir, "main.md") + err = os.WriteFile(mainFile, []byte(mainWorkflow), 0644) + require.NoError(t, err, "Failed to write main file") + + oldDir, err := os.Getwd() + require.NoError(t, err, "Failed to get current directory") + err = os.Chdir(workflowsDir) + require.NoError(t, err, "Failed to change directory") + defer func() { _ = os.Chdir(oldDir) }() + + workflowData, err := compiler.ParseWorkflowFile("main.md") + require.NoError(t, err, "ParseWorkflowFile should not error when threat-detection is explicitly disabled") + require.NotNil(t, workflowData.SafeOutputs, "SafeOutputs should not be nil") + + // The explicit disable must survive the import merge. + assert.Nil(t, workflowData.SafeOutputs.ThreatDetection, "ThreatDetection must remain nil when explicitly disabled by main workflow") +} diff --git a/tmp-smoke-test-22377081592.txt b/tmp-smoke-test-22377081592.txt new file mode 100644 index 0000000000..a715ec58f7 --- /dev/null +++ b/tmp-smoke-test-22377081592.txt @@ -0,0 +1 @@ +Test file for PR push - smoke test run 22377081592