fix: imported safe-output fragments no longer override explicit threat-detection: false

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)

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")
+}

tmp-smoke-test-22377081592.txt

@@ -0,0 +1 @@
+Test file for PR push - smoke test run 22377081592