Skip to content

Normalize Pi threat-detection models before Copilot fallback #41545

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Jun 25, 2026
+33 −0
Merged

Normalize Pi threat-detection models before Copilot fallback #41545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
81daf32
Normalize Pi detection model for Copilot
Copilot Jun 25, 2026
042f501
Fix Pi threat detection model fallback
Copilot Jun 25, 2026
79256e6
Document Pi detection fallback precedence
Copilot Jun 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions pkg/workflow/threat_detection_inline_engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,17 @@ func (c *Compiler) buildDetectionEngineExecutionStep(data *WorkflowData) []strin
if hasThreatDetectionEngineConfig {
engineConfig = data.SafeOutputs.ThreatDetection.EngineConfig
}
// Preserve the original engine identity before Pi is normalized to Copilot for
// detection. Precedence matches runtime engine resolution: explicit
// threat-detection.engine.id overrides the main engine config, which overrides
// the legacy top-level AI field.
originalEngineID := data.AI
if data.EngineConfig != nil && data.EngineConfig.ID != "" {
originalEngineID = data.EngineConfig.ID

@github-actions github-actions Bot Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] The data.EngineConfig.ID override here lacks the originalEngineID == "" guard present in getThreatDetectionEngineID, so the two precedence rules diverge.

In getThreatDetectionEngineID (threat_detection_external.go:124), data.AI wins if non-empty, with data.EngineConfig.ID used only as a fallback. Here data.EngineConfig.ID always overrides data.AI — the inverse. The comment above claims precedence matches runtime resolution, but it doesn't.

For the reported bug both fields are "pi" so the inconsistency is invisible, but it becomes a latent correctness hazard if the two fields ever diverge.

💡 Suggested fix

Add the same empty-check guard used in getThreatDetectionEngineID:

originalEngineID := data.AI
if originalEngineID == "" && data.EngineConfig != nil && data.EngineConfig.ID != "" {
	originalEngineID = data.EngineConfig.ID
}

This brings the precedence in line with the comment and with the existing engine-resolution logic.

@github-actions github-actions Bot Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precedence mismatch vs getThreatDetectionEngineID: this assignment makes data.EngineConfig.ID override data.AI, but getThreatDetectionEngineID (in threat_detection_external.go) uses data.AI as primary and data.EngineConfig.ID only as a fallback when data.AI is empty — the inverse priority. If the two fields diverge, originalEngineID resolves incorrectly and the pi→copilot model normalization silently skips.

💡 Details and suggested fix

getThreatDetectionEngineID (non-TD-config path):

engineID = data.AI                          // primary
if engineID == "" && data.EngineConfig != nil && data.EngineConfig.ID != "" {
    engineID = data.EngineConfig.ID         // fallback only when data.AI is empty
}

But the new code at line 42–43 does:

originalEngineID := data.AI
if data.EngineConfig != nil && data.EngineConfig.ID != "" {
    originalEngineID = data.EngineConfig.ID // unconditional override — opposite priority
}

The comment on lines 37–40 claims this matches runtime resolution (EngineConfig.ID > data.AI), but that is not what getThreatDetectionEngineID implements. One of these is wrong, and the comment risks cargo-culting the incorrect precedence into future code.

Impact: if data.AI = "pi" and data.EngineConfig.ID is any non-"pi" value, getThreatDetectionEngineID returns "copilot" via pi normalization, but originalEngineID resolves to the non-"pi" EngineConfig value. The guard engineSetting == "copilot" && originalEngineID == "pi" never fires, so copilot/gpt-5.4 leaks to the Copilot CLI unchanged.

Fix: mirror getThreatDetectionEngineID's fallback-only semantics:

originalEngineID := data.AI
if originalEngineID == "" && data.EngineConfig != nil && data.EngineConfig.ID != "" {
    originalEngineID = data.EngineConfig.ID // fallback only when data.AI is empty
}

Also update the comment on line 37 to match actual behaviour.

}
if hasThreatDetectionEngineConfig && data.SafeOutputs.ThreatDetection.EngineConfig.ID != "" {
originalEngineID = data.SafeOutputs.ThreatDetection.EngineConfig.ID
}

// Get the engine instance
engine, err := c.getAgenticEngine(engineSetting)
Expand Down Expand Up @@ -87,6 +98,13 @@ func (c *Compiler) buildDetectionEngineExecutionStep(data *WorkflowData) []strin
if detectionEngineConfig.APITarget == "" && data.EngineConfig != nil && data.EngineConfig.APITarget != "" {
detectionEngineConfig.APITarget = data.EngineConfig.APITarget
}
if engineSetting == "copilot" && originalEngineID == "pi" {
// Pi requires provider/model syntax (for example "copilot/gpt-5.4"), but the
// Copilot CLI expects only the model ID. extractPiModelID preserves bare model
// names unchanged, so empty or already-normalized values keep their current
// fallback behavior while provider-scoped Pi models become Copilot-compatible.
detectionEngineConfig.Model = extractPiModelID(detectionEngineConfig.Model)
}

// Create minimal WorkflowData for threat detection.
// SandboxConfig with AWF enabled ensures the engine runs inside the firewall.
Expand Down
15 changes: 15 additions & 0 deletions pkg/workflow/threat_detection_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1269,6 +1269,21 @@ func TestCopilotDetectionDefaultModel(t *testing.T) {
shouldContainModel: true,
expectedModel: "gpt-4",
},
{
name: "pi engine threat detection normalizes provider-scoped model for copilot fallback",
data: &WorkflowData{
AI: "pi",
EngineConfig: &EngineConfig{
ID: "pi",
Model: "copilot/gpt-5.4",
},
SafeOutputs: &SafeOutputsConfig{
ThreatDetection: &ThreatDetectionConfig{},
},
},
shouldContainModel: true,
expectedModel: "gpt-5.4",

@github-actions github-actions Bot Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test only covers the aligned AI/EngineConfig.ID = "pi" case: the new case sets both data.AI and data.EngineConfig.ID to "pi", so neither the second nor third branch of originalEngineID (lines 45–46) is meaningfully exercised in isolation. A regression in the threat-detection-specific engine config path would be invisible.

💡 Suggested additional test case

A case where normalization is driven by a threat-detection-specific engine config (exercises the originalEngineID = data.SafeOutputs.ThreatDetection.EngineConfig.ID branch):

{
    name: "pi threat-detection engine config normalizes provider-scoped model for copilot fallback",
    data: &WorkflowData{
        AI: "copilot", // main engine is copilot; TD override is pi
        SafeOutputs: &SafeOutputsConfig{
            ThreatDetection: &ThreatDetectionConfig{
                EngineConfig: &EngineConfig{
                    ID:    "pi",
                    Model: "copilot/gpt-5.4",
                },
            },
        },
    },
    shouldContainModel: true,
    expectedModel:      "gpt-5.4",
},

This ensures the third branch of originalEngineID (line 46) and the normalization block both fire when pi is specified as a TD-specific engine override rather than the main engine.

},
{

@github-actions github-actions Bot Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The third branch of originalEngineID resolution — where ThreatDetection.EngineConfig.ID is set explicitly to "pi" — has no test coverage.

A user who writes threat-detection: { engine: { id: pi, model: "copilot/gpt-5.4" } } in frontmatter would exercise this path (hasThreatDetectionEngineConfig && data.SafeOutputs.ThreatDetection.EngineConfig.ID != ""), but no test currently validates it.

💡 Suggested test case to add after this one 
{
    name: "pi engine via explicit threat-detection engine config normalizes provider-scoped model",
    data: &WorkflowData{
        AI: "claude",
        SafeOutputs: &SafeOutputsConfig{
            ThreatDetection: &ThreatDetectionConfig{
                EngineConfig: &EngineConfig{
                    ID:    "pi",
                    Model: "copilot/gpt-5.4",
                },
            },
        },
    },
    shouldContainModel: true,
    expectedModel:      "gpt-5.4",
},

This ensures the explicit threat-detection.engine.id: pi path is as robust as the top-level engine path.

name: "copilot engine with threat detection engine config with custom model",
data: &WorkflowData{
Expand Down
Loading