refactor(workflow): split threat_detection.go (1542 lines) into focused modules#41231
Conversation
Splits the 1542-line file into 6 focused files: - threat_detection_config.go (195 lines): ThreatDetectionConfig struct, methods, config parsers, extractRawExpression - threat_detection_helpers.go (117 lines): threatLog, IsDetectionJobEnabled, IsConditionalDetection, utility helpers, constants - threat_detection_steps.go (446 lines): inline step builders (guard, clear, prepare, conclude, analysis, upload) - threat_detection_inline_engine.go (181 lines): buildDetectionEngineExecutionStep for inline AWF path - threat_detection_external.go (488 lines): external-detector install, run, conclude, and engine resolution - threat_detection_job.go (153 lines): buildDetectionJob top-level assembler All tests pass, no breaking changes to public API. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the threat-detection workflow compiler implementation by splitting the former pkg/workflow/threat_detection.go (1542 lines) into smaller, focused modules under pkg/workflow/, with the intent of preserving behavior and public API while improving navigability.
Changes:
- Split threat-detection config parsing, helpers/predicates, job assembly, and step builders into separate files.
- Isolated inline-engine execution logic and external-detector logic into dedicated modules.
- Removed the monolithic
pkg/workflow/threat_detection.gosource file after migrating its contents.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/threat_detection.go | Removed the previous monolithic implementation after splitting into focused modules. |
| pkg/workflow/threat_detection_config.go | ThreatDetectionConfig model + parsing helpers (including extractRawExpression). |
| pkg/workflow/threat_detection_helpers.go | Shared constants (threatLog, detectionStepCondition, stepEnvIndent) and predicates/helpers. |
| pkg/workflow/threat_detection_steps.go | Detection job step assembly (inline + external-detector paths) and supporting step builders. |
| pkg/workflow/threat_detection_inline_engine.go | Inline engine execution step builder (buildDetectionEngineExecutionStep). |
| pkg/workflow/threat_detection_external.go | External-detector install/run/conclude and env extraction utilities. |
| pkg/workflow/threat_detection_job.go | Top-level detection job assembler (buildDetectionJob). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 2
| // buildDetectionEngineExecutionStep creates the engine execution step for inline threat detection. | ||
| // It uses the same agentic engine already installed in the agent job, but runs it through | ||
| // sandbox.agent (AWF) with no allowed domains (network fully blocked) and no MCP configured. |
| @@ -0,0 +1,446 @@ | |||
| // Package workflow - step builders for the standard (inline) threat detection flow. | |||
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. PR #41231 is a pure refactor that splits threat_detection.go (1542 lines) into 6 focused modules with no changes to any *_test.go, *.test.cjs, or *.test.js files. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1580 new lines in 📄 Draft ADR committed: 📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Even a pure structural refactor like this one captures a real decision (file-organization strategy, the 500-line target, why six files instead of five) that future contributors will want to understand. This repo already records many similar split/decompose decisions as ADRs. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out, /improve-codebase-architecture, and /tdd — commenting with improvement opportunities (no blocking issues).
📋 Key Themes & Highlights
Key Themes
- Module ownership drift (3 comments): Two functions in
helpers.goare exclusively called byexternal.goand should live there;getThreatDetectionEngineIDlives inexternal.gobut is called from three files and belongs inhelpers.go. - Vacuous wrapper (1 comment):
getExternalThreatDetectionEngineIDdelegates 100% togetThreatDetectionEngineID— the alias should be removed. - Missing unit tests for pure functions (3 comments):
engineCoreSecretVarNames,extractStepEnvLines,extractRawExpression, andcodexProxyWebsocketBaseURLare all pure/deterministic and untested; the first has security consequences.
Positive Highlights
- ✅ Mechanically clean split — addition/deletion delta is symmetric and all six files stay in
package workflowwith zero import path changes. - ✅ Public API (
IsDetectionJobEnabled,IsConditionalDetection,ThreatDetectionConfig) preserved exactly. - ✅ Every new file is under the 500-line target; no single file dominates navigation.
- ✅ Well-written PR description with a clear file-breakdown table — makes review straightforward.
- ✅ Existing test suite provides solid end-to-end coverage for the refactored logic.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 105.1 AIC · ⌖ 9.79 AIC · ⊞ 6.5K
| // threat-detect path. Threat-detection engine resolution is centralized in | ||
| // getThreatDetectionEngineID, including Pi -> Copilot normalization. | ||
| func (c *Compiler) getExternalThreatDetectionEngineID(data *WorkflowData) string { | ||
| return c.getThreatDetectionEngineID(data) |
There was a problem hiding this comment.
[/improve-codebase-architecture] getExternalThreatDetectionEngineID is a vacuous wrapper — it adds a second name with no semantic distinction from getThreatDetectionEngineID.\n\n
💡 Suggestion
\n\nReplace all three call sites (lines 13, 173, 255) with direct calls togetThreatDetectionEngineID, then delete this wrapper. The two functions are identical; keeping both creates a false impression that the external path uses different resolution logic.\n\n|
|
||
| // getThreatDetectionEngineID returns the effective engine ID for the detection job. | ||
| // It mirrors threat-detection engine resolution: threat-detection.engine overrides main engine. | ||
| func (c *Compiler) getThreatDetectionEngineID(data *WorkflowData) string { |
There was a problem hiding this comment.
[/zoom-out] getThreatDetectionEngineID lives in threat_detection_external.go but is called by three files: threat_detection_external.go, threat_detection_inline_engine.go, and threat_detection_steps.go. This is a cross-cutting concern, not an external-path concern.\n\n
💡 Suggestion
\n\nMovegetThreatDetectionEngineID (and its vanishing alias getExternalThreatDetectionEngineID) to threat_detection_helpers.go, alongside the other shared predicates IsDetectionJobEnabled and IsConditionalDetection. A reader navigating threat_detection_inline_engine.go will reach for helpers first.\n\n| return additional | ||
| } | ||
|
|
||
| func canReuseThreatDetectionEngineConfigForExternalDetector(data *WorkflowData, engineID string) bool { |
There was a problem hiding this comment.
[/zoom-out] canReuseThreatDetectionEngineConfigForExternalDetector and engineCoreSecretVarNames (lines 94–118) are only ever called by threat_detection_external.go, yet they live in helpers.go. This muddies the helpers file's purpose as the home for shared, cross-cutting predicates.\n\n
💡 Suggestion
\n\nMove both functions tothreat_detection_external.go next to their callers. The helpers file should remain the source of truth for shared predicates (IsDetectionJobEnabled, IsConditionalDetection, threatLog, constants) and cross-cutting utilities. External-only decision functions should live closer to their use.\n\n| // engineCoreSecretVarNames returns the secret-backed env var names for the given engine ID | ||
| // that must be excluded from the AWF container via --exclude-env. These are the credentials | ||
| // that AWF's API proxy intercepts, so the container itself does not need them. | ||
| func engineCoreSecretVarNames(engineID string) []string { |
There was a problem hiding this comment.
[/tdd] engineCoreSecretVarNames has no unit test. A wrong credential mapping here means the detection container gets access to secrets it should not — the security consequence is high enough to warrant direct test coverage.\n\n
💡 Suggested test
\n\ngo\nfunc TestEngineCoreSecretVarNames(t *testing.T) {\n cases := []struct{ engine string; want []string }{\n {"copilot", []string{"COPILOT_GITHUB_TOKEN"}},\n {"claude", []string{"ANTHROPIC_API_KEY"}},\n {"codex", []string{"OPENAI_API_KEY", "CODEX_API_KEY"}},\n {"gemini", []string{"GEMINI_API_KEY", "ANTIGRAVITY_API_KEY"}},\n {"antigravity", []string{"GEMINI_API_KEY", "ANTIGRAVITY_API_KEY"}},\n {"unknown", []string{}},\n }\n for _, tc := range cases {\n got := engineCoreSecretVarNames(tc.engine)\n if !reflect.DeepEqual(got, tc.want) {\n t.Errorf("%s: got %v, want %v", tc.engine, got, tc.want)\n }\n }\n}\n\n\n| // extractStepEnvLines copies the YAML env: block from a rendered engine execution step. | ||
| // It intentionally stops when a comment line appears because comments in step templates | ||
| // are section separators, and consuming past them may bleed into non-env content. | ||
| func extractStepEnvLines(step GitHubActionStep) []string { |
There was a problem hiding this comment.
[/tdd] extractStepEnvLines has no unit tests despite non-trivial edge cases: the comment-break condition, the indentation-prefix boundary, and an env: marker that appears before index zero. A silent mis-extraction would pass wrong env vars to the external detector.\n\n
💡 Key edge cases to test
\n\n- Step with noenv: block → returns nil\n- Step with env: followed by correctly indented vars → returns those lines only\n- Step where a # comment line immediately after env: terminates extraction\n- Line with stepEnvIndent-width indent followed by a non-env line stopping extraction\n- Empty-string lines within the env block (should be skipped per current logic)\n\n| // embedded directly into a YAML if: condition expression tree. | ||
| // Callers must ensure the input is a valid expression (verified by isExpression()) before | ||
| // calling this function; non-expression strings are returned with no modification. | ||
| func extractRawExpression(expr string) string { |
There was a problem hiding this comment.
[/tdd] extractRawExpression is a pure transformation function with no test. It handles three cases — proper ${{ ... }} wrappers, already-bare expressions, and malformed input — and the callers rely on correct trimming for downstream YAML expression trees.\n\n
💡 Suggested test cases
\n\ngo\nfunc TestExtractRawExpression(t *testing.T) {\n cases := []struct{ in, want string }{\n {"${{ inputs.flag }}", "inputs.flag"},\n {"${{inputs.flag}}", "inputs.flag"}, // no spaces\n {"inputs.flag", "inputs.flag"}, // already bare\n {"${{ inputs.a && inputs.b }}", "inputs.a && inputs.b"},\n }\n for _, tc := range cases {\n if got := extractRawExpression(tc.in); got != tc.want {\n t.Errorf("extractRawExpression(%q) = %q, want %q", tc.in, got, tc.want)\n }\n }\n}\n\n\n| }, "\n") | ||
| } | ||
|
|
||
| func codexProxyWebsocketBaseURL(apiBase string) string { |
There was a problem hiding this comment.
[/tdd] codexProxyWebsocketBaseURL has three branches (https→wss, http→ws, default passthrough) and no unit test. The default pass-through silently returns an unchanged URL that is neither a ws nor wss scheme — worth asserting this behaviour is intentional.\n\n
💡 Suggested test cases
\n\ngo\nfunc TestCodexProxyWebsocketBaseURL(t *testing.T) {\n cases := []struct{ in, want string }{\n {"(api.example.com/redacted), "(api.example.com/redacted) {"(localhost/redacted) "(localhost/redacted) {"(example.com/redacted) "(example.com/redacted) // passthrough\n {"", ""}, // empty\n }\n for _, tc := range cases {\n if got := codexProxyWebsocketBaseURL(tc.in); got != tc.want {\n t.Errorf("codexProxyWebsocketBaseURL(%q) = %q, want %q", tc.in, got, tc.want)\n }\n }\n}\n\n\n|
@copilot review all comments and address unresolved review feedback.
|
|
@copilot please refresh the branch and rerun checks after any follow-up edits.
|
|
@copilot run pr-finisher skill |
Ran a PR-finisher pass on #41231.
No code changes were made in this pass. |
pkg/workflow/threat_detection.gohad grown to 1542 lines, making it hard to navigate and review. Splits it into 6 focused files, all under 500 lines, with no logic or API changes.File breakdown
threat_detection_config.goThreatDetectionConfigstruct + methods, config parsers,extractRawExpressionthreat_detection_helpers.goIsDetectionJobEnabled,IsConditionalDetection, shared predicates + constants (detectionStepCondition,stepEnvIndent,threatLog)threat_detection_steps.gothreat_detection_inline_engine.gobuildDetectionEngineExecutionStep— inline AWF engine executionthreat_detection_external.goextractStepEnvLinesthreat_detection_job.gobuildDetectionJobtop-level assemblerNotes
package workflow— no import path changes for callers.steps.go), but that estimate excludedbuildDetectionEngineExecutionStep(~168 lines). Extracting it tothreat_detection_inline_engine.gokeeps every file under the 500-line target.IsDetectionJobEnabled,IsConditionalDetection,ThreatDetectionConfig) is unchanged.pr-sous-chef branch update requested for workflow run https://github.com/github/gh-aw/actions/runs/28109601576.