fix: propagate model config into external threat detection path - #49454
Conversation
…ent.md Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Propagates model aliases, overrides, and pricing into external threat detection execution to prevent AWF pricing failures.
Changes:
- Resolves and propagates detection model configuration.
- Adds regression tests for model and pricing propagation.
- Regenerates affected workflows and reformats detection comments.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/threat_detection_external.go |
Propagates detection model configuration. |
pkg/workflow/threat_detection_test.go |
Adds propagation tests. |
actions/setup/md/detection_runs_comment.md |
Uses a table for detection comments. |
.github/workflows/pr-description-caveman.lock.yml |
Regenerates model aliases. |
.github/workflows/pr-code-quality-reviewer.lock.yml |
Regenerates model aliases. |
.github/workflows/duplicate-code-detector.lock.yml |
Regenerates model aliases. |
.github/workflows/daily-repo-chronicle.lock.yml |
Regenerates model aliases. |
.github/workflows/daily-performance-summary.lock.yml |
Regenerates model aliases. |
.github/workflows/daily-mcp-concurrency-analysis.lock.yml |
Regenerates model aliases. |
.github/workflows/daily-max-ai-credits-test.lock.yml |
Regenerates model aliases. |
.github/workflows/daily-graft-intelligence.lock.yml |
Regenerates model aliases. |
.github/workflows/daily-geo-optimizer.lock.yml |
Regenerates model aliases. |
.github/workflows/daily-formal-spec-verifier.lock.yml |
Regenerates model aliases. |
.github/workflows/daily-file-diet.lock.yml |
Regenerates model aliases. |
.github/workflows/daily-experiment-report.lock.yml |
Regenerates model aliases. |
.github/workflows/daily-credit-limit-test.lock.yml |
Regenerates model aliases. |
.github/workflows/daily-compiler-quality.lock.yml |
Regenerates model aliases. |
.github/workflows/daily-cli-tools-tester.lock.yml |
Regenerates model aliases. |
.github/workflows/daily-cli-performance.lock.yml |
Regenerates model aliases. |
.github/workflows/daily-architecture-diagram.lock.yml |
Regenerates model aliases. |
.github/workflows/daily-ambient-context-optimizer.lock.yml |
Regenerates model aliases. |
.github/workflows/craft.lock.yml |
Regenerates model aliases. |
.github/workflows/copilot-pr-prompt-analysis.lock.yml |
Regenerates model aliases. |
.github/workflows/copilot-pr-nlp-analysis.lock.yml |
Regenerates model aliases. |
.github/workflows/copilot-pr-merged-report.lock.yml |
Regenerates model aliases. |
.github/workflows/copilot-opt.lock.yml |
Regenerates model aliases. |
.github/workflows/copilot-cli-deep-research.lock.yml |
Regenerates model aliases. |
.github/workflows/contribution-check.lock.yml |
Regenerates model aliases. |
.github/workflows/code-scanning-fixer.lock.yml |
Regenerates model aliases. |
.github/workflows/cli-consistency-checker.lock.yml |
Regenerates model aliases. |
.github/workflows/ci-coach.lock.yml |
Regenerates model aliases. |
.github/workflows/breaking-change-checker.lock.yml |
Regenerates model aliases. |
.github/workflows/artifacts-summary.lock.yml |
Regenerates model aliases. |
.github/workflows/architecture-guardian.lock.yml |
Regenerates model aliases. |
.github/workflows/archie.lock.yml |
Regenerates model aliases. |
.github/workflows/agent-performance-analyzer.lock.yml |
Regenerates aliases and pricing. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 99/99 changed files
- Comments generated: 2
- Review effort level: Balanced
| originalEngineID := data.AI | ||
| if data.EngineConfig != nil && data.EngineConfig.ID != "" { | ||
| originalEngineID = data.EngineConfig.ID | ||
| } |
There was a problem hiding this comment.
Fixed in commit 36b4974. originalEngineID now checks safe-outputs.threat-detection.engine.id as the highest-priority source, mirroring the three-tier precedence in the inline path (data.AI → data.EngineConfig.ID → data.SafeOutputs.ThreatDetection.EngineConfig.ID). Two regression subtests were added to cover both edge cases.
🧪 Test Quality Sentinel Report
📊 Metrics (4 tests)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix correctly mirrors the inline detection path. buildExternalDetectorExecutionStep now propagates Model, ModelMappings, and DefaultAiCreditsPricing from the main WorkflowData into threatDetectionData, preventing the COPILOT_MODEL=auto HTTP 400 regression.
- Model resolution logic matches
buildDetectionEngineExecutionStepexactly (SafeOutputs override → env default → engine default → Pi normalization) APITargetwas already propagated viabuildExternalDetectorWorkflowData; no gap there- Test coverage is solid: four sub-tests cover model inheritance, mappings, detection-specific override, and pricing propagation
No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 34 AIC · ⌖ 9.89 AIC · ⊞ 5.3K
…al threat detection
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (136 new lines in business logic directories, exceeding the 100-line threshold) but does not have a linked Architecture Decision Record (ADR). 📄 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 Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 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.
Request changes: A high-severity correctness bug in the originalEngineID precedence for the external threat-detection path — it silently diverges from the inline path for workflows using a per-workflow threat-detection.engine.id override combined with the Pi engine, producing incorrect Pi model-string normalization.
💡 Themes and highlights
- The core fix (propagating
Model/ModelMappings/DefaultAiCreditsPricinginto the external detector'sWorkflowData) is correct and well targeted — it directly addresses the reported COPILOT_MODEL=auto/HTTP 400 failure. - However, the new
originalEngineIDcomputation copied intobuildExternalDetectorExecutionSteponly reimplements two of the three precedence tiers used by the equivalent inline-path logic (and bygetThreatDetectionEngineIDitself), omitting thesafe-outputs.threat-detection.engine.idoverride. This causesextractPiModelIDto fire or skip incorrectly for workflows that override the detection engine while using Pi on either side. - This is exactly the class of bug this PR sets out to fix (drift between the inline and external paths), which reinforces that this logic should be extracted into one shared helper rather than duplicated a second time.
- The new
TestExternalDetectorPropagatesModeltest suite is a solid addition for the primary fix but does not cover the Pi + engine-override interaction, so the regression above would not be caught by CI. - The
detection_runs_comment.mdreformat and lockfile regeneration are mechanical and uncontroversial.
🔎 Code quality review by PR Code Quality Reviewer · auto · 73.1 AIC · ⌖ 15.2 AIC · ⊞ 7.8K
Comment /review to run again
| } | ||
| // Pi workflows normalise to Copilot; strip the provider prefix so the Copilot CLI | ||
| // receives a bare model ID rather than a "pi/model-name" string. | ||
| originalEngineID := data.AI |
There was a problem hiding this comment.
originalEngineID here does not account for safe-outputs.threat-detection.engine.id, so Pi normalization can diverge from the inline path in workflows that override the detection engine.
💡 Details and fix
The inline path (buildDetectionEngineExecutionStep in threat_detection_inline_engine.go, lines ~40-47) computes originalEngineID with three-tier precedence: data.AI → data.EngineConfig.ID → data.SafeOutputs.ThreatDetection.EngineConfig.ID (highest priority). This mirrors getThreatDetectionEngineID's own precedence for engineID itself.
Here in the external path, the equivalent block only checks data.AI and data.EngineConfig.ID:
originalEngineID := data.AI
if data.EngineConfig != nil && data.EngineConfig.ID != "" {
originalEngineID = data.EngineConfig.ID
}
if engineID == "copilot" && originalEngineID == "pi" {
resolvedDetectionModel = extractPiModelID(resolvedDetectionModel)
}Concrete failure case: a workflow with main engine: pi but safe-outputs.threat-detection.engine.id: copilot (explicitly opting out of Pi for detection) — engineID (via getThreatDetectionEngineID) correctly resolves to "copilot", but originalEngineID here incorrectly stays "pi" (since it never checks the threat-detection override), so extractPiModelID fires when it should not, potentially mangling a valid non-Pi-prefixed model string. Conversely, a workflow overriding threat-detection.engine.id: pi while main engine is something else would fail to strip the Pi prefix at all, since originalEngineID here never reflects that override.
Fix: mirror the inline path's full three-tier precedence, including the hasThreatDetectionEngineConfig check:
originalEngineID := data.AI
if data.EngineConfig != nil && data.EngineConfig.ID != "" {
originalEngineID = data.EngineConfig.ID
}
if data.SafeOutputs != nil && data.SafeOutputs.ThreatDetection != nil &&
data.SafeOutputs.ThreatDetection.EngineConfig != nil &&
data.SafeOutputs.ThreatDetection.EngineConfig.ID != "" {
originalEngineID = data.SafeOutputs.ThreatDetection.EngineConfig.ID
}Better yet, extract a single shared helper used by both the inline and external paths — this PR exists precisely because the two paths drifted out of sync once already.
There was a problem hiding this comment.
Fixed in commit 36b4974. Added the safe-outputs.threat-detection.engine.id tier to originalEngineID resolution in buildExternalDetectorExecutionStep, matching the three-tier precedence in buildDetectionEngineExecutionStep. The shared-helper suggestion is noted; deferred as a follow-up refactor to keep this PR focused on the correctness fix.
| // TestExternalDetectorPropagatesModel verifies that buildExternalDetectorExecutionStep | ||
| // inherits the main workflow model and model mappings, preventing the COPILOT_MODEL env | ||
| // var from falling back to 'auto' when no org variable is configured. | ||
| func TestExternalDetectorPropagatesModel(t *testing.T) { |
There was a problem hiding this comment.
The new test suite never exercises Pi normalization combined with a threat-detection.engine.id override — exactly the scenario where the external path's originalEngineID bug diverges from the inline path.
💡 Details
All four subtests set AI: "copilot" and never combine AI: "pi" with a SafeOutputs.ThreatDetection.EngineConfig.ID override (or vice versa). This is precisely the gap flagged in the other comment on threat_detection_external.go — a targeted subtest such as:
t.Run("pi normalization respects threat-detection engine override", func(t *testing.T) {
data := &WorkflowData{
AI: "pi",
Model: "pi/claude-haiku-4.5",
SafeOutputs: &SafeOutputsConfig{
ThreatDetection: &ThreatDetectionConfig{
EngineConfig: &EngineConfig{ID: "copilot"},
},
},
}
// ... assert extractPiModelID behavior matches inline path
})would have caught the originalEngineID precedence bug immediately, and should be added alongside a fix for it.
There was a problem hiding this comment.
Added in commit 36b4974. Two subtests cover the Pi+override scenarios: "Pi detection engine override on Copilot main workflow strips pi/ prefix" (AI:copilot, detection engine pi) and "Pi main workflow with explicit Copilot detection engine does not strip model" (AI:pi, detection engine copilot).
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting with two targeted improvements.
📋 Key Themes & Highlights
Key Themes
- Root cause correctly addressed:
buildExternalDetectorExecutionStepnow mirrorsbuildDetectionEngineExecutionStep— propagatingModel,ModelMappings, andDefaultAiCreditsPricing. The fix is precise and well-scoped. - Test coverage is solid: four subtests cover the main model inheritance, alias map, override, and pricing paths.
- Two gaps worth addressing: the Pi→Copilot normalisation branch is untested, and one assertion is fragile (see inline comments).
Positive Highlights
- ✅ Excellent inline comments explaining why each field is propagated — directly references the failure mode (HTTP 400 /
autofallback). - ✅ Model resolution precedence (override → main → env default → engine default) is consistent with the inline path and clearly documented.
- ✅
buildExternalDetectorWorkflowDataalready handlesAPITargetpropagation, so no duplication needed there.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 72.4 AIC · ⌖ 7.81 AIC · ⊞ 7K
Comment /matt to run again
| originalEngineID = data.EngineConfig.ID | ||
| } | ||
| if engineID == "copilot" && originalEngineID == "pi" { | ||
| resolvedDetectionModel = extractPiModelID(resolvedDetectionModel) |
There was a problem hiding this comment.
[/tdd] Missing test: the Pi→Copilot normalisation branch (engineID == "copilot" && originalEngineID == "pi") has no subtest — a regression would go undetected.
💡 Suggested subtest
t.Run("strips provider prefix for pi→copilot workflows", func(t *testing.T) {
data := &WorkflowData{
AI: "pi",
Model: "pi/claude-haiku-4.5",
SafeOutputs: &SafeOutputsConfig{
ThreatDetection: &ThreatDetectionConfig{},
},
}
steps := compiler.buildExternalDetectorExecutionStep(data)
allSteps := strings.Join(steps, "")
if strings.Contains(allSteps, "pi/") {
t.Errorf("expected provider prefix stripped; got:
%s", allSteps)
}
})@copilot please address this.
There was a problem hiding this comment.
Added in commit bbc5d19. New subtest "strips pi/ provider prefix for Pi-engine main workflow with no detection override" covers the basic Pi→Copilot normalization path (AI:pi, no detection engine override) and asserts COPILOT_MODEL: claude-haiku-4.5 (no pi/ prefix) in the generated YAML.
| AI: "copilot", | ||
| Model: "haiku", | ||
| ModelMappings: map[string][]string{ | ||
| "haiku": {"copilot/claude-haiku-4.5"}, |
There was a problem hiding this comment.
[/tdd] The || 'auto' check tests the compiled YAML output string directly. This couples the test to the emitted template syntax rather than the resolved value — if the template changes format, this check could silently pass even if auto is still reachable at runtime.
💡 Why this matters
The guard strings.Contains(allSteps, "|| 'auto'") is checking the template expression that appears when no model is set, not whether the resolved model is correct. If the template wording changes (e.g., to || \"auto\"), this assertion becomes a no-op. Consider instead asserting that the YAML contains a hard-coded model value and does not contain the fallback variable expression at all, or extracting the resolved model via a struct rather than scanning compiled YAML.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit bbc5d19. Replaced the || 'auto' string check with a ${{ template-expression check: the assertion now iterates lines looking for any COPILOT_MODEL: entry containing ${{, which is robust to fallback string format changes (e.g. single-quote vs double-quote, or a different default model value).
|
@copilot run pr-reviewer skill |
…tor Pi check - Add safe-outputs.threat-detection.engine.id as the highest-precedence source for originalEngineID in buildExternalDetectorExecutionStep, matching the inline path in buildDetectionEngineExecutionStep. Without this, a Copilot main workflow with an explicit pi detection engine override would not strip the pi/ prefix from the model, and a Pi main workflow with an explicit copilot detection engine override would incorrectly strip the prefix. - Add two regression subtests covering these two scenarios. - Use static 'View run' link label in detection_runs_comment.md table to prevent workflow names containing '|' from breaking the layout. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed both in-scope review comments (commit
|
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /tdd, and /domain-modeling — requesting changes on two correctness issues and one housekeeping item.
📋 Key Themes & Highlights
Key Issues
-
DefaultAiCreditsPricingasymmetry (correctness risk): The external path now propagatesDefaultAiCreditsPricing, but the inline path (threat_detection_inline_engine.go) never does. Inline-path workflows withmodels.default-ai-credits-pricingconfigured will silently drop that config from the detection AWF, potentially reproducing the same HTTP 400 errors this PR is fixing. -
Missing regression test for zero-model scenario (
/tdd): All new subtests supply an explicit model. The actual failure path —data.Model == ""with no env default and no engine default — has no test. This is the root-cause scenario the PR description calls out. -
originalEngineIDplacement (readability): Computed afterresolvedDetectionModelis set, diverging from the inline path's ordering. No correctness bug, but makes the code harder to audit for future parity checks. -
Duplication debt not tracked (
/domain-modeling): The ADR notes that model-resolution logic is now duplicated across paths, but no TODO or follow-up issue anchors that debt.
Positive Highlights
- ✅ Excellent root-cause description in the PR body and in source comments — future readers will understand exactly why this change exists.
- ✅ Six subtests covering the happy path, model override, Pi normalisation, and Pi→Copilot cross-engine cases.
- ✅ ADR is well-structured and the alternatives section is thorough.
- ✅
compilerenv.ResolveDefaultDetectionModel/engine.GetDefaultDetectionModelfallback chain mirrors the inline path correctly.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 76.7 AIC · ⌖ 9.12 AIC · ⊞ 7K
Comment /matt to run again
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please address the outstanding blocking review feedback, refresh the branch if GitHub allows it, and run the Current maintainer-facing blocker:
|
…k assertion
- Add basic Pi->Copilot normalization subtest (AI:pi, no detection override)
to cover the scenario requested in review thread PRRT_kwDOPc1QR86VlRvr.
- Replace `|| 'auto'` negative assertion with a '${{' template-expression
check, which is robust to changes in fallback string formatting (e.g.
double-quote vs single-quote variants) as flagged in PRRT_kwDOPc1QR86VlRvv.
- Use strings.SplitSeq for range iteration (modernize lint).
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All five unresolved review threads have been replied to. Summary:
Actions taken: added basic Pi→Copilot normalization subtest; replaced fragile Hand-off: a maintainer should resolve the now-addressed review threads, then re-trigger CI (close/reopen or |
|
🎉 This pull request is included in a new release. Release: |
Workflows using
features: gh-aw-detection: true(external detection path) were failing with HTTP 400 becauseCOPILOT_MODELfell back toauto— the AWF API proxy has no pricing forcopilot/autoand no default pricing configured.Root cause
buildExternalDetectorExecutionStepdid not copyModel,ModelMappings, orDefaultAiCreditsPricingfrom the mainWorkflowDataintothreatDetectionData. The inline path (buildDetectionEngineExecutionStep) already handled this correctly. Without a model,addCopilotModelEnvemits:When neither org var is set,
COPILOT_MODEL=auto→ AWF API proxy returns 400 on every inference attempt.Changes
threat_detection_external.go— resolve detection model (with per-workflow override → main model → env default → engine default) and propagateModel,ModelMappings,DefaultAiCreditsPricingintothreatDetectionData, mirroring the inline path. Pi→Copilot normalization (extractPiModelID) applied where needed.threat_detection_test.go—TestExternalDetectorPropagatesModel: 4 subtests covering main model inheritance, model mappings in AWF config, detection-specific model override, and default AI credits pricing propagation.detection_runs_comment.md— reformatted as a Markdown table.Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
run_url: https://github.com/github/gh-aw/actions/runs/30683195966