Fix threat detection config_error for workflows using custom engines - #59636
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"
- "proxy.golang.org"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer is reviewing this pull request using Matt Pocock's engineering skills... |
🏗️ ADR Required checkAn ADR was required for this PR because the prefetch summary reports 256 added lines in business-logic directories, which is above the default 100-line threshold. I did not find a complete existing ADR in the PR body, and the linked issue reference (
Evidence used:
Next action: review and refine the draft ADR, then keep it with the PR as the architectural record for this change. Note: I attempted the required local validation flow, but Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"
- "proxy.golang.org"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This still has one blocking correctness issue in the custom-engine threat-detection path.
Blocking theme
The new engine fallback normalizes custom engines to a built-in detector, but the Pi-only model normalization still keys off the original engine ID. That means any custom engine that falls back to Copilot and uses provider-scoped model names can still pass an incompatible model string into the Copilot detector unless the model inheritance path happens to zero it out first. The PR needs a guard that normalizes models based on the resolved detector/runtime pair, not just the legacy pi special case.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 47.2 AIC · ⌖ 7.4 AIC · ⊞ 21.8K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/threat_detection_inline_engine.go:127
This fallback still only strips provider prefixes when the original engine is pi, so a custom engine that normalizes to Copilot can still feed Copilot an incompatible provider-scoped model string and fail threat detection at runtime.
<details><summary>💡 Why this blocks</summary>
Both inline and external detection now choose a built-in detector through getThreatDetectionEngineID, but the model cleanup logic still checks originalEngineID == "pi" before calling extractPiModelID. That …
There was a problem hiding this comment.
🟡 Changes recommended
The new “threat-detect capable engine” check currently includes embedded engines like gemini, which would still produce config_error because threat-detect --engine only accepts copilot|claude|codex.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a long-standing threat detection failure mode for workflows that use custom engines with safe outputs by ensuring the external threat-detect --engine value is always a supported built-in engine, adding a detection-engine frontmatter escape hatch for engine definitions, and making the compiler surface a warning when fallback occurs.
Changes:
- Normalize threat-detect engine selection for non-supported workflow engines and allow custom engine definitions to declare a built-in
detection-engine. - Stop inheriting the main workflow’s model for detection runs when the detection engine is normalized, avoiding invalid model IDs for the built-in analyzer.
- Add compiler warnings, schema/docs updates, and tests to cover fallback + precedence behavior.
File summaries
| File | Description |
|---|---|
| pkg/workflow/threat_detection_external.go | Normalize detection engine IDs and add detection-engine resolution + defaults. |
| pkg/workflow/threat_detection_inline_engine.go | Use inheritedDetectionModel for inline detection model resolution. |
| pkg/workflow/threat_detection_helpers.go | Add inheritedDetectionModel helper (model inheritance gating). |
| pkg/workflow/compiler_validators.go | Emit compile-time warning when detection falls back for unsupported engines. |
| pkg/workflow/engine_definition.go | Add DetectionEngine field to engine definitions and related nolint annotations. |
| pkg/workflow/behavior_defined_engine.go | Treat detection-engine as a signal that YAML is an engine-definition form. |
| pkg/workflow/threat_detection_custom_engine_test.go | New tests covering fallback, frontmatter declaration, precedence, and warning text. |
| pkg/parser/schemas/main_workflow_schema.json | Add engine.detection-engine schema validation (enum). |
| docs/src/content/docs/reference/threat-detection.md | Document custom-engine detection fallback and detection-engine. |
| docs/src/content/docs/reference/third-party-agent.md | Document detection-engine for third-party engine definitions. |
| docs/src/content/docs/reference/frontmatter-full.md | Regenerate frontmatter reference including detection-engine. |
| .github/workflows/shared/aider.md | Declare detection-engine: copilot in shared engine definition. |
| .github/workflows/shared/crush.md | Declare detection-engine: copilot in shared engine definition. |
| .github/workflows/shared/cursor.md | Declare detection-engine: copilot in shared engine definition. |
| .github/workflows/shared/deepseek-harness.md | Declare detection-engine: copilot in shared engine definition. |
| .github/workflows/shared/goose.md | Declare detection-engine: copilot in shared engine definition. |
| .github/workflows/shared/kiro.md | Declare detection-engine: copilot in shared engine definition. |
| .github/workflows/shared/opencode.md | Declare detection-engine: copilot in shared engine definition. |
| .github/workflows/shared/pydantic.md | Declare detection-engine: copilot in shared engine definition. |
| .github/workflows/smoke-aider.lock.yml | Regenerated lockfile reflecting detection engine normalization. |
| .github/workflows/smoke-cursor.lock.yml | Regenerated lockfile reflecting detection engine normalization. |
| .github/workflows/smoke-deepseek-harness.lock.yml | Regenerated lockfile reflecting detection engine normalization. |
| .github/workflows/smoke-kiro.lock.yml | Regenerated lockfile reflecting detection engine normalization. |
| .github/workflows/smoke-opencode.lock.yml | Regenerated lockfile reflecting detection engine normalization. |
| .github/workflows/daily-token-consumption-report.lock.yml | Regenerated lockfile reflecting detection engine normalization. |
| .github/workflows/daily-team-evolution-insights.lock.yml | Regenerated lockfile reflecting detection engine normalization. |
| .github/workflows/daily-security-red-team.lock.yml | Regenerated lockfile reflecting detection engine normalization. |
| .github/workflows/daily-reliability-review.lock.yml | Regenerated lockfile reflecting detection engine normalization. |
| .github/workflows/daily-go-test-stubs-aider.lock.yml | Regenerated lockfile reflecting detection engine normalization. |
| .github/workflows/daily-code-debt-aider.lock.yml | Regenerated lockfile reflecting detection engine normalization. |
| .changeset/threat-detection-custom-engine-fallback.md | Changeset for the new fallback behavior and detection-engine support. |
Review details
- Files reviewed: 35/35 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if engineID == "" { | ||
| return false | ||
| } | ||
| return parser.BuiltinVirtualFileExists(builtinEnginePath(engineID)) | ||
| } |
There was a problem hiding this comment.
Reviewed the threat-detection custom-engine fallback fix. The core logic (getThreatDetectionEngineID, resolveCustomEngineDetectionEngineID, the new detection-engine frontmatter key, and the compiler warning) is sound, well-documented, and covered by targeted unit tests (threat_detection_custom_engine_test.go). go build ./... succeeds and all threat-detection/custom-engine tests pass locally. No blocking issues found; this is a backend fix with no UI surface, so the Impeccable UI-review modes (audit/critique/harden/etc.) do not apply here.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 73.1 AIC · ⌖ 13.2 AIC · ⊞ 8.3K
|
@copilot PR sous-chef triage for #59636. Please address the current forward-progress blockers and then run the Please review the latest feedback/checks and move this PR toward ready-for-maintainer-review.
|
|
@copilot Please review this PR for forward progress. If there are unresolved review comments or failing checks, address them and then run the pr-finisher skill. If the branch is stale, refresh it from the base branch. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot merge main and recompile |
Addressed the blockers in cda2d91: threat detection now accepts only copilot/claude/codex, Gemini falls back to Copilot with regression coverage, and Pi’s existing model normalization remains intact. Local lint, full Go tests, workflow recompilation, and final pre-push validation pass. |
|
🎉 This pull request is included in a new release. Release: |
Threat detection passed the workflow's own engine ID to
threat-detect --engine, which only accepts built-in engines. Every run of a custom-engine workflow with safe outputs ended withTHREAT_DETECTION_STATUS: reason=config_error, and because the detection job iscontinue-on-error, the run stayed green with safe outputs applied and no analysis performed.Engine resolution (
pkg/workflow/threat_detection_external.go)getThreatDetectionEngineIDnormalizes any non-built-in engine ID to a built-in detection engine, generalizing the existingpi→copilotspecial case. Applies to both the externalthreat-detectpath and the inline path.safe-outputs.threat-detection.engine> engine definitiondetection-engine>copilot.New
detection-enginefrontmatter key (pkg/workflow/engine_definition.go, schema)An engine definition can now ship a working detection default, so its users get no failure and no warning:
Compile-time visibility (
pkg/workflow/compiler_validators.go)When a custom engine falls back to the default, the compiler now names both escape hatches instead of letting the run fail silently:
The warning is suppressed when the definition declares
detection-engineor the workflow setssafe-outputs.threat-detection.engine.Model inheritance (
inheritedDetectionModel)Detection no longer inherits the main workflow's model when the engine was normalized — a custom provider model (e.g.
openai/gpt-5) is not valid for the built-in detection engine. The detection engine's own default model is used instead.Other
detection-engine: copilotdeclared in this repo's custom engine definitions (aider, crush, cursor, deepseek-harness, goose, kiro, opencode, pydantic); the only lock-file deltas are frontmatter hashes, confirming behavior is unchanged for them.reference/threat-detection.md,reference/third-party-agent.md, generated frontmatter reference), changeset, and tests covering fallback, declaration, override precedence, and the warning.Three
//nolintdirectives were added for pre-existing custom-linter diagnostics in touched files (large function, manual path concat, package-level map assignment) rather than restructuring unrelated code.config_errorfor every custom engine and the run still passes #59622Automated branch refresh attempt from PR Sous Chef. Run: https://github.com/github/gh-aw/actions/runs/34341665126
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.