Propagate max-turn-cache-misses into the threat-detection job - #62489
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
max-turn-cache-misses into the threat-detection job
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The default-value subtest is environment-dependent and fails when the supported compiler override is set.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
What changed in this PR
Propagates the workflow-level cache-miss guardrail to inline and external threat-detection jobs.
Changes:
- Adds shared cache-miss inheritance logic.
- Covers inheritance, defaults, and turn-cap separation.
- Updates documentation, changeset, and generated workflow output.
| File | Description |
|---|---|
pkg/workflow/threat_detection_inline_engine.go |
Applies inheritance to inline detection. |
pkg/workflow/threat_detection_helpers.go |
Adds shared inheritance logic. |
pkg/workflow/threat_detection_external_detector_config_test.go |
Tests external detection inheritance. |
pkg/workflow/threat_detection_engine_test.go |
Tests inline inheritance and defaults. |
docs/src/content/docs/reference/frontmatter.md |
Documents detection behavior. |
.github/workflows/smoke-crush.lock.yml |
Updates generated cache-miss limit. |
.changeset/detection-inherits-max-turn-cache-misses.md |
Records the patch change. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| } | ||
| }) | ||
|
|
||
| t.Run("falls back to the compile default when the workflow does not configure it", func(t *testing.T) { |
There was a problem hiding this comment.
Addressed in f87c5c6 by clearing compilerenv.DefaultMaxTurnCacheMisses in the default-cache-miss subtest before asserting the compile default.
|
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.
|
|
✅ 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.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
🏗️ ADR Required — draft added for PR #62489I added a draft ADR at Evidence used
Draft decision captured
Next actionPlease review and update the draft ADR if needed, then keep it with the PR as the explicit design record for this architectural behavior change.
|
|
@copilot Please address the open Copilot review finding on this PR, then run the pr-finisher skill and update the branch. Open review item: Clear compiler environment override before testing default cache misses (#discussion_r4067417444). Also refresh the branch if needed after resolving review feedback.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Impeccable Skills Review
Modes applied: mixed_unclear → critique, audit (small internal compiler fix; no user-facing UI, so scored on correctness/consistency instead).
Summary
The change correctly propagates max-turn-cache-misses into the threat-detection job, mirroring the existing MaxAICredits-style inheritance pattern:
inheritDetectionTurnCacheMissesis nil-safe (data,data.EngineConfig,detectionConfigall guarded) and only fills in when the detection config doesn't already have a positive value — consistent with howmax-turn-cache-missesis documented as top-level-only (noengine-config-level override exists in the schema), so there's no realistic clobbering risk.- Both call sites (
buildDetectionEngineExecutionStepinline path,buildExternalDetectorWorkflowDataexternal path) are covered, and thedoes not inherit max-turnsregression test guards against accidentally coupling this to the unrelatedmax-turnsbudget precedent. - Docs and the single affected lock file (
smoke-crush.lock.yml,maxCacheMisses5→15) are consistent with the frontmatter change. - Build (
go build ./...) and targeted tests (go test ./pkg/workflow/...threat-detection/engine-config suites) pass.
Notes
- There's already an open review comment on
threat_detection_engine_test.go:186(from a prior pass) flagging that the "falls back to default" subtest doesn't guard againstGH_AW_DEFAULT_MAX_TURN_CACHE_MISSESbeing set in the test environment. That's a valid, non-blocking robustness nit — not duplicating it here.
No additional blocking issues found in the changed lines.
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 · 129 AIC · ⌖ 13.1 AIC · ⊞ 8.1K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd to this bug fix. The core change (inheritDetectionTurnCacheMisses in pkg/workflow/threat_detection_helpers.go:226-243) is small, well-reasoned, and matches the pattern of the existing max-ai-credits handling — inherit only when the detection engine config has no explicit value of its own (<= 0), so overrides are respected. Verified by building and running go test ./pkg/workflow/... for the touched suites: all pass.
📋 Key Themes & Findings
Findings
- A reviewer (Copilot) already flagged the one real issue I found: the new subtest
"falls back to the compile default when the workflow does not configure it"(pkg/workflow/threat_detection_engine_test.go:186) asserts against the rawconstants.DefaultMaxTurnCacheMissesconstant, but the actual code path resolves throughGetMaxTurnCacheMisses(), which honorsGH_AW_DEFAULT_MAX_TURN_CACHE_MISSES. I reproduced this: running the suite withGH_AW_DEFAULT_MAX_TURN_CACHE_MISSES=9set in the environment makes this specific subtest fail, while the sibling subtests pass. This is a real, reproducible test fragility (not just theoretical) in an enterprise deployment that sets this env var, or in CI that happens to inherit it. I'd support requesting a fix (e.g. wrap the expected value withcompilerenv.ResolveDefaultMaxTurnCacheMisses(constants.DefaultMaxTurnCacheMisses)as done elsewhere in the codebase) before merge, per the existing comment.
Positive Highlights
- ✅ Root cause is correctly identified and addressed at the source (
buildAWFConfig/GetMaxTurnCacheMisses), not patched at the symptom. - ✅ Good regression-test coverage per
/tdd: separate subtests for inheritance, default fallback, and explicit non-inheritance ofmax-runs, plus a matching test for the external-detector path. - ✅ The doc comment justifying why
max-turns/max-runsis intentionally not inherited (unlikemax-turn-cache-misses) is a nice touch — it preempts an obvious "why not do the same for max-runs" question and is consistent with the existingmax-ai-creditsprecedent. - ✅ Lock-file diff is minimal and expected (only
smoke-crush, the one workflow setting this field).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 175.7 AIC · ⌖ 14.5 AIC · ⊞ 10.1K
Comment /matt to run again
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in f87c5c6. The default-cache-miss subtest now clears |
|
🎉 This pull request is included in a new release. Release: |
The threat-detection job always compiled with the compile-time default
maxCacheMisses(5): the detectionEngineConfigis rebuilt from a fixed field list that omitsMaxTurnCacheMisses, sobuildAWFConfigresolved it through the nil/default path. Under BYOK providers that reportcached_tokensonly intermittently, detection then trips the AWF proxy'smax_cache_misses_exceededguard (HTTP 403) and — withthreat-detection.continue-on-error: false— fails closed on every run carrying a real patch. The only existing lever was theGH_AW_DEFAULT_MAX_TURN_CACHE_MISSEScompile env var.Changes
pkg/workflow/threat_detection_helpers.go: newinheritDetectionTurnCacheMissescopies the workflow's top-levelmax-turn-cache-missesinto the detection engine config when detection has no value of its own.buildDetectionEngineExecutionStep(inline) andbuildExternalDetectorWorkflowData(external detector).smoke-crush(the sole workflow setting the field), whose detection job now emitsmaxCacheMisses: 15instead of5.max-turns, and the external-detector path; frontmatter reference notes the field also applies to detection.Note on
max-turnsThe issue suggested propagating
max-runs"for consistency"; this PR deliberately does not.max-turnsis a budget, and likemax-ai-credits(which is explicitly not inherited today) detection keeps its own invocation cap so a tightly capped agent job cannot starve detection. The cache-miss guardrail is different in kind: it describes a property of the configured LLM provider, so it must apply to every job talking to that provider. Happy to extend tomax-runsif reviewers prefer the symmetric behaviour.max-turn-cache-misses; detection always runs at the compile-time default (5) #61489