Pipe threat-detection kill-switch frontmatter into compiled threat-detect invocation - #55532
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
threat-detect invocation
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The refactor is clean and the implementation is correct.
buildThreatDetectCommandcorrectly handlesnilconfig and omits optional flags when unset — good defensive design.shellEscapeArgis applied to the duration string (user-supplied) but not to integer values fromstrconv.Itoa— consistent and correct.- Negative
engine-timeoutrejection delegates to the JSON schema regex; theTestThreatDetectionKillSwitchValidationtest validates this throughCompileWorkflow. - Test coverage is solid with both unit and integration-style compilation tests.
LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 23.5 AIC · ⌖ 11.8 AIC · ⊞ 6.2K
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Requesting changes
This refactor introduces a shell-safety regression in the generated threat-detect command: engineID is now concatenated into the run: line without escaping, so the workflow can break or execute the wrong command as soon as an engine name stops being a single shell-safe token.
Blocking theme
- The new helper correctly treats the newly added optional flags carefully, but it also centralizes command assembly and now inserts
engineIDunescaped beforestrings.Join(args, " "). - That makes the generated shell command depend on an implicit invariant about engine identifiers that is not enforced here.
- This is a correctness and security-adjacent bug in workflow generation, so it should be fixed before merge.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 8.02 AIC · ⌖ 8.27 AIC · ⊞ 7K
Comment /review to run again
| func buildThreatDetectCommand(npmPathSetup, engineID string, config *ThreatDetectionConfig) string { | ||
| args := []string{ | ||
| "threat-detect", | ||
| "--engine", engineID, |
There was a problem hiding this comment.
The new command builder stops shell-escaping engineID, so an engine name containing whitespace or shell metacharacters will compile into a broken or injectable run: command instead of a single CLI argument.
💡 Why this blocks merge
Before this refactor, the whole threat-detect invocation was assembled with fmt.Sprintf, and every user-derived positional value except engineID stayed localized in one place. This helper now appends engineID directly into args and joins with spaces:
args := []string{"threat-detect", "--engine", engineID}
return fmt.Sprintf("%s && %s", npmPathSetup, strings.Join(args, " "))That is only safe if engineID is permanently restricted to shell-safe tokens everywhere upstream. If an engine id ever becomes configurable as an arbitrary string (or even just contains a space), the generated workflow will execute the wrong command. This is exactly the sort of regression that hides until a new engine/provider is added.
Please shell-escape engineID at the point it is inserted, and add a test that covers an engine id with whitespace or quoting-sensitive characters.
args := []string{
"threat-detect",
"--engine", shellEscapeArg(engineID),
}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — approving with two minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Minor escaping inconsistency:
shellEscapeArgis used for--engine-timeoutbut not for the integer flags; worth clarifying the intent. - Test helper gap:
intPtris missing alongsidestrPtr, leading to verbose anonymous closures in tests.
Positive Highlights
- ✅ Clean pointer-based opt-in design — unset fields emit no flags, preserving detector defaults
- ✅ Solid type-switch parsing for
parseThreatDetectionEngineTimeoutcoveringint,int64, andfloat64edge cases - ✅ Good schema validation with the duration-pattern regex plus
const: 0special case - ✅ Tests cover both the "omit when unset" and "emit when set" contracts in
threat_detection_external_detector_execution_test.go - ✅ Validation tests in
TestThreatDetectionKillSwitchValidationexercise the compile-time schema path
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 32.6 AIC · ⌖ 10.9 AIC · ⊞ 7.6K
Comment /matt to run again
| if config.EngineTimeout != nil { | ||
| args = append(args, "--engine-timeout", shellEscapeArg(*config.EngineTimeout)) | ||
| } | ||
| if config.MaxTurns != nil { |
There was a problem hiding this comment.
[/codebase-design] shellEscapeArg is applied to EngineTimeout but not to the integer flags — a minor inconsistency that could confuse future contributors about when escaping is needed.
💡 Suggestion
The schema regex already validates the duration format, so shell-special characters should not appear. Either drop the escaping for consistency with the integer flags, or add a comment explaining the intent:
// shellEscapeArg guards against unexpected chars in user-supplied duration strings
args = append(args, "--engine-timeout", shellEscapeArg(*config.EngineTimeout))@copilot please address this.
| expectedConfig: &ThreatDetectionConfig{ | ||
| EngineTimeout: strPtr("10m"), | ||
| MaxTurns: func() *int { | ||
| v := 100 |
There was a problem hiding this comment.
[/tdd] The anonymous func() *int { v := 100; return &v }() pattern is repeated across both test files — an intPtr helper (matching the existing strPtr) would improve readability.
💡 Suggestion
Add alongside strPtr in the test helpers file:
func intPtr(v int) *int { return &v }Then tests become:
MaxTurns: intPtr(100),
Retries: intPtr(1),@copilot please address this.
There was a problem hiding this comment.
One simplification stands out in the new config parsing path: the dedicated helpers are carrying more branchy type handling than the schema and call sites need. net: -38 lines possible.
Generated by ✂️ Ponytail Reviewer for #55532 · codex · mai10 · 6.41 AIC · ⌖ 1.92 AIC · ⊞ 16.7K
Comment /ponytail to run again
| return strings.TrimSpace(s) | ||
| } | ||
|
|
||
| func parseThreatDetectionEngineTimeout(raw any) *string { |
There was a problem hiding this comment.
pkg/workflow/threat_detection_config.go:226-294: shrink: two bespoke parser helpers for three new fields and a handful of type branches. Inline the tiny conversions at the call sites and delete the helpers.
There was a problem hiding this comment.
Pull request overview
Adds workflow-level threat-detection kill-switch configuration and conditionally emits the corresponding detector flags.
Changes:
- Adds frontmatter schema and parsing for timeout, turn, and retry limits.
- Builds optional
threat-detectarguments while preserving defaults. - Adds parsing, validation, and command-generation tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/threat_detection_external.go |
Generates detector flags. |
pkg/workflow/threat_detection_external_detector_execution_test.go |
Tests command generation. |
pkg/workflow/threat_detection_config.go |
Parses new settings. |
pkg/workflow/threat_detection_config_test.go |
Tests parsing and validation. |
pkg/parser/schemas/main_workflow_schema.json |
Defines the frontmatter schema. |
Review details
Suppressed comments (1)
pkg/workflow/threat_detection_config.go:273
- Normal positive YAML integers are decoded as
uint64, but this helper has nouint64case. Consequently authored values such asmax-turns: 100andretries: 1validate successfully yet are silently discarded, so neither flag reaches the compiled command.
case int64:
if v < 0 {
return nil
}
value := int(v)
return &value
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Balanced
| if config.EngineTimeout != nil { | ||
| args = append(args, "--engine-timeout", shellEscapeArg(*config.EngineTimeout)) | ||
| } | ||
| if config.MaxTurns != nil { | ||
| args = append(args, "--max-turns", strconv.Itoa(*config.MaxTurns)) |
| case int64: | ||
| if v != 0 { | ||
| threatLog.Printf("Ignoring invalid numeric threat-detection.engine-timeout value %d; use a Go duration string such as '10m' or 0", v) | ||
| return nil | ||
| } | ||
| zero := "0" | ||
| return &zero |
| "oneOf": [ | ||
| { | ||
| "type": "string", | ||
| "pattern": "^(0|([0-9]+(\\.[0-9]+)?(ns|us|µs|ms|s|m|h))+)$" |
| ], | ||
| "description": "Per-attempt timeout for threat detection engine execution as a Go duration (for example '90s', '10m', '1h30m'). Set to 0 to disable timeout enforcement in threat-detect." | ||
| }, | ||
| "max-turns": { |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (298 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot upgrade the release and recompile |
|
Please do one focused follow-up pass:
Run: https://github.com/github/gh-aw/actions/runs/32787277683
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
/smoke-copilot |
|
📰 BREAKING: Smoke Copilot is now investigating this issue comment. Sources say the story is developing... |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in commit fd7617e: fixed shell escaping for |
|
/smoke-copilot |
|
📰 BREAKING: Smoke Copilot is now investigating this issue comment. Sources say the story is developing... |
|
Smoke Test: Copilot Engine PR: "Pipe threat-detection kill-switch frontmatter into compiled
Overall: FAIL (2 checks failed, see above) cc Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "www.google.com"
- "www.gstatic.com"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "www.google.com"
- "www.gstatic.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Caveman review done. Code good, small nits left. Ug.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comwww.google.comwww.gstatic.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "www.google.com"
- "www.gstatic.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · copilot · auto · 52.9 AIC · ⌖ 2.71 AIC · ⊞ 9.2K
Comment /smoke-copilot to run again
Add label smoke to run again
| args = append(args, "--engine-timeout", shellEscapeArg(*config.EngineTimeout)) | ||
| } | ||
| if config.MaxTurns != nil { | ||
| args = append(args, "--max-turns", strconv.Itoa(*config.MaxTurns)) |
There was a problem hiding this comment.
Ugga. New func good. Small nit: consider validating negative Retries/MaxTurns before appending to args, ugh.
| return strings.TrimSpace(s) | ||
| } | ||
|
|
||
| func parseThreatDetectionEngineTimeout(raw any) *string { |
There was a problem hiding this comment.
Ugh, config field look ok. Maybe add comment for future cavemen readers.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "www.google.com"
- "www.gstatic.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Smoke test review submission.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comwww.google.comwww.gstatic.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "www.google.com"
- "www.gstatic.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · copilot · auto · 31.6 AIC · ⌖ 2.35 AIC · ⊞ 9.1K
Comment /smoke-copilot to run again
Add label smoke to run again
| return strings.TrimSpace(s) | ||
| } | ||
|
|
||
| func parseThreatDetectionEngineTimeout(raw any) *string { |
There was a problem hiding this comment.
Smoke test: inline review comment.
gh-aw-threat-detectionadded per-attempt controls (--engine-timeout,--max-turns,--retries), butgh-awcould not configure them per workflow. This PR adds detector-specific frontmatter support and emits these flags only when explicitly set, preserving detector-default behavior for unset fields.Frontmatter surface for detector controls
safe-outputs.threat-detectionkeys:engine-timeout(Go duration string,0allowed)max-turns(non-negative integer)retries(non-negative integer)Schema-level validation
main_workflow_schema.jsonto validate the new fields at compile time.max-turnsandretries.engine-timeoutto valid duration-like input (plus0), preventing invalid numeric forms from being treated as valid config.Command generation behavior (key contract)
--engine-timeout <value>--max-turns <value>--retries <value>only when each field is explicitly configured.
Example emitted invocation
pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/32787277683