feat(detector): add --engine-timeout and --max-turns kill switches for runaway models

[davidslater]

Created by GitHub Ace · View Session

Summary

Adds two per-attempt kill switches so a detection model that goes off the rails is stopped by the detector itself, not by the enclosing GitHub Actions job timeout.

Prior to this change the detector had no wall-clock cap, no turn cap, and no credit ceiling — only sink-driven early cancel (which shortens successful runs) and the retry loop (which only fires on malformed output). A runaway model could burn the entire 15-minute job budget on a single attempt.

New flags

Flag	Default	Env	Purpose
--engine-timeout	5m	THREAT_DETECTION_ENGINE_TIMEOUT	Wall-clock timeout per detection attempt. On expiry the engine subprocess is killed. 0 disables.
--max-turns	20	THREAT_DETECTION_MAX_TURNS, falls back to GH_AW_MAX_TURNS	Agentic tool-use turn cap. 0 disables.

Default sizing

• 5m per attempt × retries+1 (default 2 attempts) = 10 minutes worst case, comfortably inside the smokes' timeout-minutes: 15 with headroom for artifact prep and result upload.
• 20 turns matches the existing gh-aw smoke workflow convention and is well above the ~5–15 turns a verdict-only detection run typically needs.

Propagation

• GH_AW_MAX_TURNS=<n> is exported to the engine subprocess — the universal gh-aw contract read by all three engine harnesses (Claude, Codex, Copilot).
• --max-turns N is additionally passed to the bare Claude CLI (documented flag).
• The bare Copilot CLI has no equivalent flag; the detector logs a diagnostic so a caller who set --max-turns isn't misled about which cap is actually active on that path.

Error surfacing (soft failure)

New status reason engine_timeout (exit 2), distinct from engine_error so the daily statistics workflow can separate runaway-model kills from other engine failures.

Per TD-21a, engine_timeout falls in the same "MAY surface as step failure" bucket as engine_error, config_error, cancelled — so:

• The detection step fails.
• conclude in warn mode still lets safe outputs proceed (soft failure workflow-wide, matching how gh-aw treats agent_failure).
• Strict mode (GH_AW_DETECTION_CONTINUE_ON_ERROR=false) blocks safe outputs, as intended.

Verdict-vs-deadline race

If the engine writes a valid verdict to the result sink just before the deadline fires, the verdict wins over the timeout — the run reports result_recorded normally. This prevents a healthy run near its budget from being reclassified as a runaway kill.

Per-attempt, not aggregate

Each retry gets a fresh budget. A timeout on attempt 1 still allows the correction retry.

Docs

• Spec: adds TD-21b covering both budgets, the verdict-race rule, the Copilot bare-CLI caveat, and the "MUST comfortably fit inside the job timeout" invariant.
• README: documents both flags in the CLI section.

Tests

• TestRunEmitsEngineTimeoutStatusWhenAllAttemptsExceedDeadline — hanging engine is killed on the per-attempt timeout, both attempts are killed, terminal reason is engine_timeout.
• TestRunHonorsVerdictWrittenBeforeDeadline — verdict written just before deadline still wins.
• TestRunExportsMaxTurnsToEngineEnv — GH_AW_MAX_TURNS=17 reaches the engine subprocess.
• TestRunOmitsMaxTurnsEnvWhenDisabled — --max-turns=0 does NOT export the env var (so a downstream harness's own default resolution isn't blocked).
• TestRunResolvesMaxTurnsFromGhAwEnv — GH_AW_MAX_TURNS=42 in env is honored when no flag/detector env is set.
• Existing claudeArgs / claudeHarnessArgs tests extended for the new maxTurns parameter, including --max-turns propagation to the bare Claude CLI.

Validation

make fmt lint build test test-scripts all clean. gosec finding count unchanged from main (36 pre-existing findings, none introduced).

[Copilot]

Pull request overview

Adds per-attempt timeout and turn limits to prevent runaway threat-detection engines.

Changes:

• Adds --engine-timeout, --max-turns, and environment propagation.
• Introduces engine_timeout status handling and retry logic.
• Adds documentation and tests for limits and deadline races.

Show a summary per file

File	Description
specs/threat-detection-spec.md	Defines timeout and turn-cap requirements.
README.md	Documents the new flags.
pkg/engine/engine.go	Propagates turn limits to engines.
pkg/engine/engine_test.go	Tests Claude argument generation.
cmd/threat-detect/main.go	Implements timeout configuration and status handling.
cmd/threat-detect/main_test.go	Tests timeout and environment behavior.

Review details

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Suppressed comments (1)

cmd/threat-detect/main.go:189

• Negative CLI values bypass both kill switches: --engine-timeout=-1s skips context.WithTimeout, and --max-turns=-1 is omitted by maxTurnsEnv. This contradicts the documented contract that only 0 disables a cap and differs from the env parsing, which rejects negatives. Validate both resolved flag values after parsing and return config_error for negatives.

	flag.IntVar(&maxTurns, "max-turns", envMaxTurns(defaultMaxTurns),
		"Maximum agentic tool-use turns per attempt; 0 disables the cap. Exported to the engine subprocess as GH_AW_MAX_TURNS and passed as --max-turns to engines whose CLI accepts it (env: THREAT_DETECTION_MAX_TURNS, GH_AW_MAX_TURNS)")
	flag.DurationVar(&engineTimeout, "engine-timeout", envDuration("THREAT_DETECTION_ENGINE_TIMEOUT", defaultEngineTimeout),
		"Wall-clock timeout per detection attempt (e.g. 5m, 300s); 0 disables. On expiry the engine subprocess is killed; if all attempts time out the run exits 2 with reason engine_timeout (env: THREAT_DETECTION_ENGINE_TIMEOUT)")

• Files reviewed: 6/6 changed files
• Comments generated: 5
• Review effort level: Balanced

[davidslater]

Follow-up commit 177d661 addresses all five review comments. Summary of responses:

1. GH_AW_MAX_TURNS inheritance leak (pkg/engine/engine.go:90) — fixed. runCLIEnvWithSink now filters GH_AW_MAX_TURNS out of os.Environ() before composing cmd.Env, via a new scrubEngineInheritedEnv list and filterEnv helper. So --max-turns=0 and THREAT_DETECTION_MAX_TURNS=0 now genuinely disable the cap even if an ambient parent-process variable is set. envMaxTurns still reads GH_AW_MAX_TURNS as a fallback before the scrub happens, so the "single turn budget for both harness and standalone" contract still holds.

2. engine_timeout not in the stats source (cmd/threat-detect/main.go:93) — the stats claim was overstated; corrected. conclude.go's detectionStatusReasonMap now maps engine_timeout → ReasonAgentFailure, so the gh-aw daily-stats collector (which only ever sees threat_detected/agent_failure/parse_error from the tracking issue) will correctly aggregate timeouts under agent_failure. The detector's own status line and per-attempt outcome logging still preserve the distinct engine_timeout reason for local debugging, but I've rewritten the spec (TD-21b) and README to say exactly that instead of claiming daily-stats separability that doesn't exist.

3. Mixed-failure classification (cmd/threat-detect/main.go:573) — moot in the new design, but the underlying concern is addressed. Retries default to 0 and --engine-timeout is now terminal: on the first timeout analyzeWithRetries returns errEngineTimeout immediately without consuming further attempts. There is no longer a "final attempt timed out after malformed earlier attempts" path — a timeout ends the run, full stop. A same-prompt retry of a runaway would almost certainly run away again and just double the credit spend.

4. Process-group kill (cmd/threat-detect/main.go:573) — fixed. runCLIEnvWithSink now sets SysProcAttr.Setpgid: true and installs cmd.Cancel to deliver SIGKILL to -pid (the whole process group), falling back to cmd.Process.Kill() on error. On harness paths where the direct child is node and the real engine CLI is a grandchild, the whole tree is now killed on timeout or cancellation.

5. Reversed envDuration comment (cmd/threat-detect/main.go:75, also line 186) — fixed at both sites. The comment now correctly says invalid/unparseable/negative values fall back to the default, and disablement is reserved for a parsed zero.

Also, per user feedback in the same review:

• Negative flag values (--retries, --max-turns, --engine-timeout) are now rejected as config_error (exit 2) instead of silently bypassing the kill switch. Table-driven test added.
• Retries default to 0. The from-scratch retry model rarely fixes anything: engine CLIs already retry transient provider errors internally, and the threat_detection_result tool wrapper's non-zero exit + stderr already lets the agent iterate in-session on a bad tool call without a subprocess restart. Flag is kept for callers who explicitly want it.

All tests pass (make fmt lint build test).