fix: safe-outputs types from imports dropped when main workflow defines any safe-outputs

[Copilot]

When a main workflow defines any safe-outputs type (e.g. noop), extractSafeOutputsConfig auto-defaults threat-detection, noop, missing-tool, missing-data, and report-incomplete even for types not present in the raw frontmatter. This caused topDefinedTypes in MergeSafeOutputs to treat those auto-defaults as user-defined, silently deleting conflicting keys from imports before merging — so threat-detection steps from an import would disappear entirely.

Root cause (two parts)

1. MergeSafeOutputs — topDefinedTypes pollution
hasSafeOutputType(topSafeOutputs, key) can't distinguish auto-defaults from explicit user config, so imported types are incorrectly deleted.

2. mergeSafeOutputConfig — result.X == nil guard
Even when an import's key survives deletion, the result.ThreatDetection != nil (auto-default empty struct) condition blocks the import's explicit config from being applied.

Changes

• MergeSafeOutputs — adds topRawSafeOutputs map[string]any parameter. When provided, only keys explicitly present in the raw frontmatter map count as "defined" for conflict detection. All existing call sites pass nil to preserve current unit-test behavior.

• mergeSafeOutputConfig — for the 5 auto-defaultable types (threat-detection, noop, missing-tool, missing-data, report-incomplete), uses raw import config map key presence as the authoritative merge signal instead of result.X == nil:

  // before
  if result.NoOp == nil && importedConfig.NoOp != nil { ... }
  
  // after: import's explicit key wins over an auto-default in result
  _, hasNoop := config["noop"]
  if (hasNoop || result.NoOp == nil) && importedConfig.NoOp != nil { ... }

• compiler_orchestrator_workflow.go — passes frontmatter["safe-outputs"] as the raw map to MergeSafeOutputs.

• TestSafeOutputsDifferentTypesFromImportsMerged — integration test reproducing the exact issue scenario: main defines noop, import defines threat-detection with custom steps; verifies both survive the merge.

• TestSafeOutputsAutoDefaultableTypesImportedWhenMainHasNone — verifies all five auto-defaultable types are imported with custom configuration when the main workflow has no safe-outputs section.

• TestSafeOutputsMainExplicitAutoDefaultableTypeOverridesImport — verifies that when the main workflow explicitly configures an auto-defaultable type (e.g. noop), the import's value for the same type is overridden by main, while other types from the import still merge correctly.

• TestSafeOutputsMultipleImportsEachContributeAutoDefaultableType — verifies that multiple imports can each contribute a different auto-defaultable type without triggering false conflict errors, all merging alongside main's explicitly defined types.

[pelikhan]

@copilot Add more test scenarios

[Copilot]

Pull request overview

Fixes a safe-outputs merge regression where auto-defaulted safe-output types in the main workflow were incorrectly treated as user-defined, causing imported safe-output configurations (notably threat-detection) to be dropped during merge.

Changes:

• Updates MergeSafeOutputs to accept the raw safe-outputs frontmatter map and use it to determine which types were explicitly defined in the main workflow.
• Adjusts mergeSafeOutputConfig so imported configs for auto-defaultable types can override auto-default structs in the merge result.
• Adds an integration test reproducing the “main defines noop, import defines threat-detection” scenario.

Show a summary per file

File	Description
pkg/workflow/imports.go	Implements raw-frontmatter-aware conflict detection and refines merge behavior for auto-defaultable safe-output types.
pkg/workflow/compiler_orchestrator_workflow.go	Passes raw frontmatter["safe-outputs"] into MergeSafeOutputs to avoid treating auto-defaults as user-defined.
pkg/workflow/safe_outputs_import_test.go	Updates call sites for the new MergeSafeOutputs signature and adds an integration regression test for the reported merge bug.
pkg/workflow/safe_outputs_fix_test.go	Updates unit test call site for the new MergeSafeOutputs signature.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

pkg/workflow/imports.go:213

• topDefinedTypes is only populated when topSafeOutputs != nil. If a caller passes topRawSafeOutputs (raw frontmatter) but topSafeOutputs is nil, explicit main-workflow keys in topRawSafeOutputs won’t be treated as defined and imports can incorrectly override them. Consider populating topDefinedTypes from topRawSafeOutputs regardless of whether topSafeOutputs is nil (and only fall back to hasSafeOutputType when topRawSafeOutputs is nil).

	// Collect all safe output types defined in the top-level config.
	// When topRawSafeOutputs is provided (from raw frontmatter), use only keys that are
	// explicitly present in the raw map to avoid counting auto-defaults as user-defined types.
	// When nil, fall back to inspecting the processed config struct (legacy/test behaviour).
	topDefinedTypes := make(map[string]bool)
	if topSafeOutputs != nil {
		for _, key := range typeKeys {
			if topRawSafeOutputs != nil {
				if _, exists := topRawSafeOutputs[key]; exists {
					topDefinedTypes[key] = true
				}
			} else if hasSafeOutputType(topSafeOutputs, key) {
				topDefinedTypes[key] = true
			}
		}
	}
	importsLog.Printf("Top-level safe-outputs defines %d types", len(topDefinedTypes))

• Files reviewed: 4/4 changed files
• Comments generated: 2

[Copilot]

The doc comment says MergeSafeOutputs returns an error when a type is defined in both main and imported configs, but the implementation actually treats main workflow definitions as overriding imports (it deletes the imported key) and only errors when the same type is defined in multiple imports. Update the comment to match the real conflict/override behavior so callers aren’t misled.

Suggested change

	// Returns an error if a conflict is detected (same safe-output type defined in both main and imported).
	// Explicit safe-output types defined in the main workflow take precedence over imported definitions.
	// Returns an error only if the same safe-output type is defined by multiple imports.

[Copilot]

The test comment links to an issue URL with a <issue> placeholder. Replace it with the actual issue number/URL or remove the link to avoid leaving a misleading reference in the codebase.

Suggested change

	// TestSafeOutputsDifferentTypesFromImportsMerged reproduces the bug reported in
	// https://github.com/github/gh-aw/issues/<issue>:
	// When the main workflow defines one safe-outputs type (e.g. noop) and an imported
	// workflow defines a different type (e.g. threat-detection), the imported type should
	// be merged into the compiled output. Previously the auto-default applied by
	// extractSafeOutputsConfig (which enabled threat-detection by default whenever any
	// safe-outputs were present) caused threat-detection to appear as "already defined" in
	// topDefinedTypes, so the import's explicit threat-detection configuration was dropped.
	// TestSafeOutputsDifferentTypesFromImportsMerged reproduces a bug where, when the main
	// workflow defines one safe-outputs type (e.g. noop) and an imported workflow defines a
	// different type (e.g. threat-detection), the imported type should be merged into the
	// compiled output. Previously the auto-default applied by extractSafeOutputsConfig
	// (which enabled threat-detection by default whenever any safe-outputs were present)
	// caused threat-detection to appear as "already defined" in topDefinedTypes, so the
	// import's explicit threat-detection configuration was dropped.

[Copilot]

@copilot Add more test scenarios

Added three new integration tests in commit a62daa7:

1. TestSafeOutputsAutoDefaultableTypesImportedWhenMainHasNone — all five auto-defaultable types (noop, missing-tool, missing-data, report-incomplete, threat-detection) are imported with explicit custom configuration when the main workflow has no safe-outputs section
2. TestSafeOutputsMainExplicitAutoDefaultableTypeOverridesImport — when the main workflow explicitly configures noop, the import's noop is overridden by main (main wins), while the import's other types (missing-tool) still merge correctly
3. TestSafeOutputsMultipleImportsEachContributeAutoDefaultableType — three separate imports each contribute a different auto-defaultable type with no conflicts; all merge alongside main's create-issue

[github-actions]

Hey @Copilot 👋 — great work on tracking down this subtle safe-outputs merge bug! The root-cause analysis is thorough, the fix is well-scoped, and the PR comes with a solid suite of new integration tests covering all the edge cases. This looks ready for maintainer review. 🎉

Checklist summary:

Check	Result
On-topic	✅ Yes — bug fix directly in the safe-outputs merge pipeline
Follows process	✅ Yes — PR authored by Copilot coding agent, core-team reviewer assigned
Focused	✅ Yes — all changes address the single safe-outputs merge regression
New dependencies	✅ No new deps introduced
Tests included	✅ Yes — 4 new integration tests added in safe_outputs_import_test.go
Has description	✅ Yes — detailed root-cause writeup with before/after code snippets

Verdict: 🟢 Aligned — no blocking issues found.

Generated by Contribution Check · ● 2.6M · ◷