Allow opting out of "[aw] Detection Runs" tracking issue independently of threat detection - #61428
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🔬 Test Quality Sentinel is analyzing test quality on this pull request... |
|
✅ Ponytail Reviewer completed successfully! 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.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ 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
- "github.com"See Network Configuration for more information.
|
|
|
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.
Ponytail review: one place to trim abstraction without changing behavior.
net: -4 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #61428 · codex · gpt53codex · 3.67 AIC · ⌖ 4.02 AIC · ⊞ 12.4K
Comment /ponytail to run again
Comments that could not be inline-anchored
pkg/workflow/notify_comment_conclusion_helpers.go:106
L106: yagni: IsReportAsIssueEnabled() helper wraps one nil/default bool check used once. Inline detectionConfig == nil || detectionConfig.ReportAsIssue == nil || *detectionConfig.ReportAsIssue at call site until a second caller exists.
🏗️ ADR required — draft added for PR #61428I enforced the ADR gate for this PR because it adds 128 new lines in business-logic directories ( Evidence reviewed
OutcomeI generated and committed a draft ADR here:
Next actionPlease review and refine the draft ADR, then keep it with this implementation so the decision remains explicit before merge.
|
There was a problem hiding this comment.
🟡 Changes recommended
The generated frontmatter reference is stale, and the documentation overstates run-summary availability.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds an independent opt-out for threat-detection tracking issues while preserving detection and enforcement.
Changes:
- Adds and parses
report-as-issue. - Omits tracking-issue reporting when disabled.
- Updates schema, tests, documentation, and release notes.
File summaries
| File | Description |
|---|---|
pkg/workflow/threat_detection_config.go |
Defines and parses the option. |
pkg/workflow/notify_comment_conclusion_helpers.go |
Skips tracking-issue logging when disabled. |
pkg/workflow/detection_success_test.go |
Tests opt-out behavior. |
pkg/parser/schemas/main_workflow_schema.json |
Exposes the option in the schema. |
docs/src/content/docs/reference/threat-detection.md |
Documents configuration and behavior. |
.changeset/threat-detection-report-as-issue-option.md |
Records the feature for release. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
| "report-as-issue": { | ||
| "type": "boolean", | ||
| "default": true, | ||
| "description": "When true (default), detection warnings/failures create or update the '[aw] Detection Runs' tracking issue and post a comment to it. When false, detection still runs and enforces its configured continue-on-error behavior, but no tracking issue is created or updated; results remain available in the GitHub Actions run diagnostics." |
There was a problem hiding this comment.
Regenerated frontmatter-full.md from the schema, including the new report-as-issue option. Addressed in 03afa9f.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — solid feature implementation with good end-to-end (compile-level) test coverage; a couple of unit-level test gaps worth closing.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps at unit level: The new
IsReportAsIssueEnabled()helper and thereport-as-issueparsing branch mirror the existingIsContinueOnError()/continue-on-errorpair, both of which have direct table-driven unit tests (TestIsContinueOnError, cases inTestParseThreatDetectionConfig). The new field only has an end-to-end compile test (TestDetectionRunsStepOmittedWhenReportAsIssueDisabled), which is good but doesn't substitute for fast, focused unit coverage of the parsing/default logic.
Positive Highlights
- ✅ Clean, minimal implementation:
ReportAsIssue *bool+IsReportAsIssueEnabled()mirrors the existingContinueOnErrorpattern exactly, keeping the codebase consistent (/codebase-design). - ✅ Correct scoping: detection job, its outputs, and
continue-on-errorenforcement are left untouched — only the "Log detection run" step is skipped. - ✅ Docs and JSON schema updated together with the code change, including a concrete opt-out example.
- ✅ The parser refactor (
parseThreatDetectionScalarFields/parseThreatDetectionReportingFields/parseThreatDetectionEngineField) is a reasonable, low-risk decomposition that keepsparseThreatDetectionObjectConfigreadable.
@copilot please address the review comments above.
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 Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 85 AIC · ⌖ 15.2 AIC · ⊞ 10.4K
Comment /matt to run again
| _, engineConfig, model := c.ExtractEngineConfig(map[string]any{"engine": engineObj}) | ||
| threatConfig.EngineConfig = engineConfig | ||
| threatConfig.Model = model | ||
| // Parse report-as-issue field (default: true). |
There was a problem hiding this comment.
[/tdd] parseThreatDetectionObjectConfig's table-driven test (TestParseThreatDetectionConfig) covers every other reporting/kill-switch field but has no case for report-as-issue, so parsing regressions here (e.g. accidentally dropping the field, or breaking it when combined with continue-on-error) wouldn't be caught at the parser-unit level — only by the heavier compile-level test in detection_success_test.go.
💡 Suggested addition to TestParseThreatDetectionConfig
{
name: "object with report-as-issue false",
outputMap: map[string]any{
"threat-detection": map[string]any{
"report-as-issue": false,
},
},
expectedConfig: &ThreatDetectionConfig{
ReportAsIssue: boolPtr(false),
},
},This keeps parser-level coverage symmetric with continue-on-error and catches regressions closer to the source than the full-compile test.
@copilot please address this.
There was a problem hiding this comment.
Added parser-level coverage for report-as-issue: false combined with continue-on-error: false. Addressed in 03afa9f.
🧪 Test Quality Sentinel AnalysisSummaryTest Quality Score: 100/100 ✅ Excellent
Test Coverage Breakdown✅ Feature Validation:
✅ Table-Driven Coverage: 5 test scenarios across configuration modes ✅ No Forbidden Patterns: Zero mock library usage, all tests verify actual compiled workflow output Recommendation✅ APPROVE — Excellent test coverage. Score: 100/100. All 9 tests are design-focused behavioral tests with comprehensive workflow output validation. The opt-out feature has explicit test coverage. 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
- "github.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100. All 9 behavioral tests verify design invariants with comprehensive workflow output validation. Feature opt-out test (TestDetectionRunsStepOmittedWhenReportAsIssueDisabled) validates the PR requirement correctly. 0% implementation tests. No forbidden patterns (0 mock violations). Ready to approve.
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.
🧪 Test quality analysis by Test Quality Sentinel · copilot · haiku45 · 141.5 AIC · ⌖ 8.56 AIC · ⊞ 8.4K
Comment /review to run again
|
Please address the unresolved review feedback:
If the branch is behind or stale, refresh it, regenerate any drifted generated docs if needed, and then continue with the 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
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
PR #61428 added safe-outputs.threat-detection.report-as-issue to the schema but safe-outputs-runtime.md never listed it alongside the other threat-detection sub-fields. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
🎉 This pull request is included in a new release. Release: |
Threat detection's automatic reporting to the
[aw] Detection Runstracking issue had no independent opt-out — it could only be disabled by turning off threat detection entirely, which also drops enforcement (continue-on-errorbehavior, safe-output blocking, etc.). Repositories using gh-aw for issue triage ended up with framework diagnostics (e.g.parse_errorreports) polluting user-facing issue queues, with no way to keep detection active while suppressing the tracking issue.Config
report-as-issue(boolean, defaulttrue) tosafe-outputs.threat-detection.falseskips creating/updating the tracking issue and posting comments to it; detection still runs and enforcement (continue-on-error, blocking behavior) is unaffected.Implementation
ThreatDetectionConfiggains aReportAsIssue *boolfield andIsReportAsIssueEnabled()helper (nil → enabled, preserving current default behavior).threat-detectionconfig extractsreport-as-issue; the parser function was split into smaller helpers (scalar fields vs. engine field) to keep it within line-length limits.report-as-issue: false, while the detection job, its outputs, and downstream enforcement remain untouched.Schema & docs
report-as-issueproperty underthreat-detection.threat-detection.mdreference doc updated with the field description and a dedicated opt-out example.Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.laiyagushi.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.