Normalize Pi threat-detection models before Copilot fallback#41545
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
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. No ADR enforcement needed: PR #41545 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (33 additions across 2 files). |
|
❌ Test Quality Sentinel failed during test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This pull request fixes a model-identifier mismatch in threat detection when workflows configure the Pi engine but threat detection normalizes Pi → Copilot. In that fallback case, Pi-style provider/model strings (e.g. copilot/gpt-5.4) are now converted to the bare model ID (gpt-5.4) before being passed to the Copilot CLI, preventing “unsupported/retired model” errors.
Changes:
- Preserve the original configured engine identity (before Pi normalization) so the compiler can detect when the Copilot detector is being used as a Pi fallback.
- When the effective detection engine is Copilot but the original engine was Pi, strip the provider prefix from the configured model via
extractPiModelID. - Add a regression test ensuring
copilot/gpt-5.4becomesgpt-5.4in the detection job’sCOPILOT_MODEL.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/threat_detection_inline_engine.go | Tracks original engine ID and normalizes Pi-style provider/model to a Copilot-compatible model ID in the Pi→Copilot detection fallback path. |
| pkg/workflow/threat_detection_test.go | Adds regression coverage asserting the detection steps emit COPILOT_MODEL: gpt-5.4 for a Pi workflow configured with copilot/gpt-5.4. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (1 test analyzed)
Go: 1 (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — two targeted observations, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Precedence divergence:
originalEngineIDresolution doesn't quite mirrorgetThreatDetectionEngineID—data.EngineConfig.IDalways overridesdata.AIin the new code, while the existing helper only uses it as a fallback whendata.AIis empty. Harmless for the current bug (both fields are"pi"), but the comment claiming they match is inaccurate and could mislead a future reader. - Coverage gap on ThreatDetection.EngineConfig path: The third
originalEngineIDbranch (explicitthreat-detection.engine.id: pi) has no regression test. The explicit-override path should be as hardened as the top-level path.
Positive Highlights
- ✅ Clean reuse of
extractPiModelID— no new logic for stripping the prefix - ✅ Backward-compatible: bare/empty model IDs pass through unchanged
- ✅ Good PR description with root-cause analysis and a concrete before/after example
- ✅ Regression test added for the exact failing scenario
- ✅ Normalization is correctly placed after APITarget inheritance and after default-model resolution, so ordering is sound
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 53.6 AIC · ⌖ 7.07 AIC · ⊞ 6.5K
| // the legacy top-level AI field. | ||
| originalEngineID := data.AI | ||
| if data.EngineConfig != nil && data.EngineConfig.ID != "" { | ||
| originalEngineID = data.EngineConfig.ID |
There was a problem hiding this comment.
[/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.
| shouldContainModel: true, | ||
| expectedModel: "gpt-5.4", | ||
| }, | ||
| { |
There was a problem hiding this comment.
[/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.
There was a problem hiding this comment.
Two non-blocking observations on the new originalEngineID logic. The primary fix (stripping the provider/ prefix before handing the model to Copilot CLI) is correct for all realistic Pi workflows.
### Findings
Precedence inversion vs getThreatDetectionEngineID (line 42–43)
The new code unconditionally overrides data.AI with data.EngineConfig.ID. getThreatDetectionEngineID does the opposite: data.AI wins and data.EngineConfig.ID is a fallback only when data.AI is empty. In the edge case where the two fields disagree (e.g. a workflow authored with both ai: pi and a conflicting engine.id), originalEngineID resolves to the wrong value and the normalization guard never fires. The inline comment also documents the wrong precedence, which risks cargo-culting it deeper.
Test gap: TD-specific engine config path not covered (test line 1285)
The new test sets both data.AI and data.EngineConfig.ID to "pi". The third branch of originalEngineID — where SafeOutputs.ThreatDetection.EngineConfig.ID = "pi" triggers the normalization — has no dedicated test case.
🔎 Code quality review by PR Code Quality Reviewer · 88.4 AIC · ⌖ 9.37 AIC · ⊞ 5.2K
| // the legacy top-level AI field. | ||
| originalEngineID := data.AI | ||
| if data.EngineConfig != nil && data.EngineConfig.ID != "" { | ||
| originalEngineID = data.EngineConfig.ID |
There was a problem hiding this comment.
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.
| }, | ||
| }, | ||
| shouldContainModel: true, | ||
| expectedModel: "gpt-5.4", |
There was a problem hiding this comment.
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.
pelikhan
deleted the
copilot/investigate-copilot-cli-detection-job-failure
branch
Issue Monster’s detection job was passing the Pi-style model identifier
copilot/gpt-5.4into the Copilot CLI fallback path. That string is valid for Pi engine config, but Copilot detection expects the bare model id so the API proxy can resolve it correctly.Root cause
pito thecopilotdetector, but it preserved the original Piprovider/modelstring inCOPILOT_MODEL.copilot/gpt-5.4instead ofgpt-5.4, which surfaced as an unsupported/retired model error in the failing job.Change
pitocopilot, strip the provider prefix from the configured model before passing it to the Copilot detector.Regression coverage
copilot/gpt-5.4so the detection path now emits a Copilot-compatible model id.Example of the normalization applied in the fallback path:
This turns:
into an effective Copilot detection model of: