fix: wire detection gate for safe-outputs assembled entirely from imports#24155
fix: wire detection gate for safe-outputs assembled entirely from imports#24155
Conversation
…curity gap) Agent-Logs-Url: https://github.com/github/gh-aw/sessions/04d5841d-b6e2-4420-84a8-f1645236c51d Co-authored-by: szabta89 <1330202+szabta89@users.noreply.github.com>
…ledInConfigs Agent-Logs-Url: https://github.com/github/gh-aw/sessions/04d5841d-b6e2-4420-84a8-f1645236c51d Co-authored-by: szabta89 <1330202+szabta89@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a compiler gap where workflows that inherit safe-outputs exclusively via imports:/@include did not get the threat-detection gate (missing detection job and needs.detection.result == 'success' condition), violating the “auto-enabled when safe outputs are configured” guarantee.
Changes:
- Apply default
ThreatDetectionafterMergeSafeOutputswhen the main workflow has nosafe-outputs:block but merged safe-outputs exists (unless explicitly disabled). - Add a helper to detect explicit threat-detection disablement in imported safe-outputs configs.
- Add tests covering imports-only safe-outputs defaulting and compiled output gating; regenerate affected
.lock.ymlworkflows.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/threat_detection.go | Adds helper to detect explicit threat-detection disablement in imported safe-outputs configs. |
| pkg/workflow/compiler_orchestrator_workflow.go | Applies default threat detection when safe-outputs are assembled solely from imports/includes. |
| pkg/workflow/safe_jobs_threat_detection_test.go | Adds unit/integration-style tests for imports-only safe-outputs and compiled detection gating. |
| .github/workflows/weekly-issue-summary.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/typist.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/terminal-stylist.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/schema-consistency-checker.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/repository-quality-improver.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/notion-issue-summary.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/mcp-inspector.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/github-remote-mcp-auth-test.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/example-workflow-analyzer.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/daily-team-evolution-insights.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/daily-secrets-analysis.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/daily-regulatory.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/daily-observability-report.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/daily-issues-report.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/daily-integrity-analysis.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/daily-copilot-token-report.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/daily-compiler-quality.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/copilot-pr-prompt-analysis.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/copilot-pr-nlp-analysis.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/copilot-pr-merged-report.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
| .github/workflows/blog-auditor.lock.yml | Regenerated compiled workflow now includes detection job and gates safe_outputs on it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // isThreatDetectionExplicitlyDisabledInConfigs checks whether any of the provided | ||
| // safe-outputs config JSON strings has threat-detection explicitly set to false. | ||
| // This is used to determine whether the default detection should be applied when | ||
| // safe-outputs comes from imports/includes (i.e. no safe-outputs: section in the | ||
| // main workflow frontmatter). | ||
| func isThreatDetectionExplicitlyDisabledInConfigs(configs []string) bool { | ||
| for _, configJSON := range configs { | ||
| if configJSON == "" || configJSON == "{}" { | ||
| continue | ||
| } | ||
| var config map[string]any | ||
| if err := json.Unmarshal([]byte(configJSON), &config); err != nil { | ||
| continue | ||
| } | ||
| if tdVal, exists := config["threat-detection"]; exists { | ||
| // tdBool is the raw bool value of the threat-detection key. | ||
| // When threat-detection: false, tdBool == false, so !tdBool == true → disabled. | ||
| if tdBool, ok := tdVal.(bool); ok && !tdBool { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
isThreatDetectionExplicitlyDisabledInConfigs only treats threat-detection: false (boolean) as an explicit disable. However parseThreatDetectionConfig also supports object syntax with enabled: false (and will return nil in that case). If an imported config uses { "threat-detection": { "enabled": false } }, the merge leaves ThreatDetection nil, but this helper returns false, so the orchestrator applies the default and unintentionally re-enables detection. Update the helper to also detect the object form (e.g., threat-detection.enabled == false) so explicit disables aren’t overridden.
| // TestIsThreatDetectionExplicitlyDisabledInConfigs verifies the helper function | ||
| // that checks whether any imported safe-outputs config explicitly disables detection. | ||
| func TestIsThreatDetectionExplicitlyDisabledInConfigs(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| configs []string | ||
| expected bool | ||
| }{ | ||
| { | ||
| name: "empty configs", | ||
| configs: []string{}, | ||
| expected: false, | ||
| }, | ||
| { | ||
| name: "empty JSON objects", | ||
| configs: []string{"{}", ""}, | ||
| expected: false, | ||
| }, | ||
| { | ||
| name: "config without threat-detection key", | ||
| configs: []string{`{"create-issue": {"max": 1}}`}, | ||
| expected: false, | ||
| }, | ||
| { | ||
| name: "config with threat-detection false", | ||
| configs: []string{`{"create-issue": {"max": 1}, "threat-detection": false}`}, | ||
| expected: true, | ||
| }, | ||
| { | ||
| name: "config with threat-detection true", | ||
| configs: []string{`{"create-issue": {"max": 1}, "threat-detection": true}`}, | ||
| expected: false, | ||
| }, | ||
| { | ||
| name: "config with threat-detection as object", | ||
| configs: []string{`{"create-issue": {"max": 1}, "threat-detection": {"prompt": "check for injection"}}`}, | ||
| expected: false, | ||
| }, | ||
| { | ||
| name: "multiple configs, one has false", | ||
| configs: []string{`{"create-issue": {"max": 1}}`, `{"create-discussion": {"max": 1}, "threat-detection": false}`}, | ||
| expected: true, |
There was a problem hiding this comment.
The new tests cover threat-detection: false but not the supported object form threat-detection: { enabled: false }. Adding a case for that form would catch the current regression where imports with enabled: false can be overridden by the new defaulting logic.
|
@copilot review comments |
…disable check Agent-Logs-Url: https://github.com/github/gh-aw/sessions/cd8593c7-a28a-4244-a8f2-832f0ecec847 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed both review comments in commit 695d62e:
|
Workflows that have no
safe-outputs:in their own frontmatter but pull it in viaimports:compiled to asafe_outputsjob withneeds: agentonly — nodetectionjob, nodetection.result == 'success'gate. The "automatically enabled when safe outputs are configured" guarantee did not hold for this path.Root cause
extractSafeOutputsConfigapplies the defaultThreatDetection = &ThreatDetectionConfig{}only when parsing the main frontmatter'ssafe-outputs:block. WhenSafeOutputsis instead assembled byMergeSafeOutputsfrom imported configs,ThreatDetectionstayednil, soIsDetectionJobEnabledreturnedfalse.Changes
pkg/workflow/compiler_orchestrator_workflow.go— afterMergeSafeOutputs, when the originalsafeOutputsfrom frontmatter wasnil(nosafe-outputs:in main workflow) but the merged result is non-nil, apply the same default:ThreatDetection = &ThreatDetectionConfig{}. Skipped when any imported config explicitly disabled detection.pkg/workflow/threat_detection.go— addedisThreatDetectionExplicitlyDisabledInConfigs([]string) boolhelper that scans raw imported config JSON for an explicit disable, supporting both the boolean form (threat-detection: false) and the object form (threat-detection: { enabled: false }), mirroringparseThreatDetectionConfig's syntax handling. This ensures the default is not applied when the import intentionally disables detection.31 compiled
.lock.ymlfiles regenerated — these are the real-world workflows (e.g.blog-auditor,copilot-session-insights) that imported a sharedsafe-outputsconfig without their own frontmatter block and now get adetectionjob and the properneeds.detection.result == 'success'condition onsafe_outputs.pkg/workflow/safe_jobs_threat_detection_test.go— five new tests:TestIsThreatDetectionExplicitlyDisabledInConfigs— unit tests for the helper, including both boolean and object-form disable casesTestDefaultThreatDetectionAppliedWhenSafeOutputsFromImportsOnly— detection enabled by default when imports provide safe-outputsTestDefaultThreatDetectionNotAppliedWhenImportedConfigExplicitlyDisables—threat-detection: falsein an imported config is respectedTestDefaultThreatDetectionNotAppliedWhenImportedConfigObjectFormDisables—threat-detection: { enabled: false }object form in an imported config is respectedTestImportedSafeOutputsCompiledWithDetectionJob— end-to-end: compiled lock file contains adetection:job andsafe_outputsgates on it