fix: propagate max-ai-credits to external detector AWF config - #50721
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
max-ai-credits to external detector AWF config
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No ADR enforcement needed: PR #50721 does not have the 'implementation' label and has only 76 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
🟡 Changes recommended
Linked issue acceptance criteria for AIC output validation and proxy-only scope documentation remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Propagates detection-specific AI credit limits to external detector AWF configuration.
Changes:
- Copies
ThreatDetection.MaxAICreditsinto external detector engine configuration. - Tests explicit, default, and non-inherited budget behavior.
File summaries
| File | Description |
|---|---|
pkg/workflow/threat_detection_helpers.go |
Propagates the external detector budget. |
pkg/workflow/threat_detection_test.go |
Verifies external-path budget semantics. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| } | ||
| } | ||
|
|
||
| func TestBuildExternalDetectorWorkflowDataMaxAICredits(t *testing.T) { |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The fix is correct and well-tested. The three-case test coverage (runtime default when unset, explicit value, non-inheritance from main agent) mirrors the existing inline-path tests and validates the expected behavior precisely. No issues found.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 16.5 AIC · ⊞ 5.3K
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality SentinelPR #50721 — "fix: propagate Quality Score: 90/100 ✅ Excellent
Test Coverage2 new test functions added
Behavioral Contracts Verified
Test Inflation Analysis
Observations✅ All assertions include descriptive failure messages Recommendation: Approve. Implementation tests: 0% (threshold: ≤30%).
|
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — one minor comment on test coverage completeness, otherwise this is a clean fix.
📋 Key Themes & Highlights
Key Themes
- Root cause addressed:
MaxAICreditswas silently dropped on the external detector path while correctly propagated on the inline path. The 3-line fix mirrors the existingAPITargetinheritance pattern — appropriate and minimal. - Test isolation is correct: The inheritance-blocking test (
NotInheritedFromMainAgent) correctly validates the budget isolation boundary. - One gap: No test covers the interaction when both
ThreatDetection.MaxAICreditsandThreatDetection.EngineConfig.MaxAICreditsare set simultaneously (thecanReusebranch). The inline comment explains the priority rule, but a test would pin it.
Positive Highlights
- ✅ Fix mirrors the existing
APITargetpropagation pattern exactly — consistent style - ✅ Three targeted tests cover unset/explicit/no-inheritance scenarios well
- ✅ PR description clearly explains the root cause and maps changes to tests
- ✅ Zero deletions — purely additive change, low regression risk
| if d.EngineConfig.APITarget == "" && data.EngineConfig != nil { | ||
| d.EngineConfig.APITarget = data.EngineConfig.APITarget | ||
| } | ||
| if data.SafeOutputs != nil && data.SafeOutputs.ThreatDetection != nil && data.SafeOutputs.ThreatDetection.MaxAICredits != 0 { |
There was a problem hiding this comment.
[/tdd] Missing interaction test: when canReuseThreatDetectionEngineConfigForExternalDetector returns true (i.e. ThreatDetection.EngineConfig != nil), the cloned engine config may already carry its own MaxAICredits. The new code unconditionally overwrites it with ThreatDetection.MaxAICredits, which is the correct priority rule, but there is no test covering both fields being set simultaneously.
💡 Suggested additional sub-test
t.Run("ThreatDetection.MaxAICredits overrides EngineConfig.MaxAICredits when both set", func(t *testing.T) {
data := &WorkflowData{
AI: "copilot",
SafeOutputs: &SafeOutputsConfig{
ThreatDetection: &ThreatDetectionConfig{
MaxAICredits: 777,
EngineConfig: &EngineConfig{MaxAICredits: 999},
},
},
}
steps := compiler.buildExternalDetectorExecutionStep(data)
allSteps := strings.Join(steps, "")
if !strings.Contains(allSteps, `"maxAiCredits":777`) {
t.Fatalf("expected top-level MaxAICredits 777 to win, got:\n%s", allSteps)
}
})This pins the priority rule: ThreatDetection.MaxAICredits wins over any value in ThreatDetection.EngineConfig.MaxAICredits.
@copilot please address this.
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: Approve with a minor hardening suggestion
Small, correct fix that mirrors the existing inline-path behavior and is covered by solid new regression tests, including the important "must not inherit from main agent" case.
💡 Review notes
- The 3-line fix correctly propagates
safe-outputs.threat-detection.max-ai-creditsto the external detector'sEngineConfig.MaxAICredits, matching the pattern already used on the inline detection path. - New tests (
TestBuildExternalDetectorWorkflowDataMaxAICredits,TestBuildExternalDetectorWorkflowDataMaxAICreditsNotInheritedFromMainAgent) verify both the explicit-value case and the no-inheritance-from-main-agent-budget case — good coverage, and I confirmed they pass locally. - Left one non-blocking comment: the new guard relies on an implicit (currently true, but unenforced) invariant that
ThreatDetection.EngineConfig.MaxAICreditsis always zero when cloned viacloneThreatDetectionEngineConfig, whereas the sibling inline path explicitly resets/reconstructsEngineConfigto guarantee this. Suggest dropping the!= 0guard so the field is always set fromThreatDetection.MaxAICredits(defaults to 0 when unset), for defense-in-depth and symmetry with the inline path's documented approach. - No correctness, concurrency, or security-adjacent issues found in the changed lines.
| if d.EngineConfig.APITarget == "" && data.EngineConfig != nil { | ||
| d.EngineConfig.APITarget = data.EngineConfig.APITarget | ||
| } | ||
| if data.SafeOutputs != nil && data.SafeOutputs.ThreatDetection != nil && data.SafeOutputs.ThreatDetection.MaxAICredits != 0 { |
There was a problem hiding this comment.
Nit: this relies on an implicit, unenforced invariant that ThreatDetection.EngineConfig.MaxAICredits is always zero for the external path — unlike the inline path, which explicitly rebuilds EngineConfig field-by-field to guarantee this.
💡 Details
buildExternalDetectorWorkflowData can populate d.EngineConfig via cloneThreatDetectionEngineConfig, which does a shallow *source copy of data.SafeOutputs.ThreatDetection.EngineConfig. Today that source is built through ExtractEngineConfig(map[string]any{"engine": engineObj}) — an isolated map with no top-level max-ai-credits key — so its MaxAICredits is always 0 in practice, and this new guarded assignment is safe.
But the sibling inline path (threat_detection_inline_engine.go) doesn't rely on that implicit invariant: it explicitly reconstructs a fresh EngineConfig{...} listing only the fields it wants to inherit and documents in a comment that MaxAICredits is intentionally omitted so it always starts at zero before applying the override. This diff's new block just adds a conditional overwrite to whatever d.EngineConfig.MaxAICredits shallow-copy already contains, without an explicit reset when ThreatDetection.MaxAICredits == 0.
Suggested hardening, mirroring the inline path's defensiveness:
if data.SafeOutputs != nil && data.SafeOutputs.ThreatDetection != nil {
d.EngineConfig.MaxAICredits = data.SafeOutputs.ThreatDetection.MaxAICredits // always resets to 0 when unset
}This removes the dependency on the current parsing behavior never populating that nested field, and a corresponding test (nonzero ThreatDetection.EngineConfig.MaxAICredits + zero ThreatDetection.MaxAICredits) would catch any future regression in ExtractEngineConfig's isolated-map parsing.
Not currently exploitable — no path today sets that nested field nonzero — so this is a defense-in-depth suggestion, not a blocker.
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please address the latest review feedback, refresh this branch if it is behind
Run: https://github.com/github/gh-aw/actions/runs/31060975271
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 🎯 Great work on closing the threat detection budget gap! This PR is well-structured and ready for review. What you fixed: The external detector path now correctly propagates Quality highlights:
This change directly addresses all the requirements outlined in issue #50654 and should safely close the budget enforcement gap without side effects.
|
|
🎉 This pull request is included in a new release. Release: |
safe-outputs.threat-detection.max-ai-creditswas applied on the inline detection path but silently dropped on the external detector path, causing the AWF API proxy to fall back to the generic runtime expression instead of the detection-specific budget.Changes
threat_detection_helpers.go—buildExternalDetectorWorkflowDatanow setsd.EngineConfig.MaxAICreditsfromThreatDetection.MaxAICredits, mirroring the inline path:threat_detection_test.go— Two new tests mirroring the existing inline-path coverage:TestBuildExternalDetectorWorkflowDataMaxAICredits— explicit value used when set; runtime default expression used when unset.TestBuildExternalDetectorWorkflowDataMaxAICreditsNotInheritedFromMainAgent— detection budget must not inherit from the main agent'sMaxAICredits.max-ai-creditsis not propagated to the external detector's AWF config #50654Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Run: https://github.com/github/gh-aw/actions/runs/31060975271> Generated by 👨🍳 PR Sous Chef · gpt54 · 10.7 AIC · ⊞ 5.9K · ◷