Skip to content

Commit 0003e99

Browse files
davidslaterGitHub Ace
andauthored
feat(detector): enforce structural eligibility for threat verdicts (#916)
* feat(detector): enforce structural eligibility for threat verdicts Add three-part defense against the false-positive class where the detection model reports a structurally impossible verdict (e.g. malicious_patch=true with zero patch files, or prompt_injection=true with zero untrusted input in the workflow prompt): 1. Structural eligibility check in the threat_detection_result tool. pkg/detector/eligibility.go computes per-category eligibility from the loaded artifacts and prompt analysis; the detector transports it to the report-result subprocess via THREAT_DETECTION_ELIGIBLE_* env vars, and report-result rejects any threat=true claim against an ineligible category as a normal correctable error. Missing/unparseable transport defaults to permissive so pre-existing callers are not tightened. 2. Prompt taxonomy update. Adds a "Not a Threat" section to the default detection prompt clarifying that instruction non-compliance is a quality signal (not a security verdict), framework-rejected safe-output validation errors are guardrails working, prompt_injection requires an untrusted origin, and malicious_patch requires an actual patch. Includes a self-check the model runs before setting any flag true. 3. Retry budget raised from 1 to 3 (env: THREAT_DETECTION_RETRIES). With eligibility rejections now flowing through the correction loop, one retry burns the budget on taxonomy noise rather than genuine malformed output. Three keeps the safety net cheap. Spec: adds TD-10g normative statement of the eligibility invariants and the correction-loop enforcement contract. README documents the new Structural eligibility section and the retries=3 default. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com> * fix(detector): make eligibility enforcement authoritative and honest Addresses review feedback on the eligibility change: - Add comment memory as an eligible prompt_injection origin (the agent reads it back into its own prompt) and as a secret_leak channel. - Fail open when prompt provenance is degraded: an absent optional prompt-template.txt leaves UntrustedInputs empty for reasons unrelated to untrusted content, so prompt_injection stays eligible there. - Re-validate every sink result in the detector process. The THREAT_DETECTION_ELIGIBLE_* environment check runs in a subprocess whose command line the model composes, so it can be overridden or stripped; it is now documented as advisory, and the binding check runs against the eligibility the detector computed from artifacts itself. An ineligible result is discarded and self-corrected, never rewritten. - Scope the prompt self-check per category: only prompt_injection needs an untrusted cause, and only it maps to "attack". Secret leaks are often accidental and patches are malicious on their own merits. The contradiction check ("does my reason argue against my verdict") stays universal, since that is what run 32239956539 failed. - Drop the "prefer a false negative on marginal calls" tie-breaker, which contradicted two existing err-toward-reporting rules. - Document the mid-run blind spot: MCP tool results, fetched content and the engine transcript are never staged, so eligibility neither sees nor claims to see injections delivered only through them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com> * revert(detector): restore default retries to 1 Each retry is a full engine restart, not another turn in a warm session: the sink is cleared, a new subprocess is spawned, and the entire artifact set is re-analyzed with the original prompt plus a bounded correction line. A default of 3 therefore meant up to four complete detection passes per run, and a systematically confused model would fail slowly and expensively rather than fast. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com> * docs(detector): clarify that safe-output bodies are the agent-output channel "Non-empty agent output" reads as free-form prose, which invites the question of whether create_issue, add_comment and create_pull_request bodies are separately gated for secret_leak. They are not, because they are not separate artifacts: agent_output.json is gh-aw's safe-output file and those bodies are its contents. No behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com> * fix(detector): stop truncating detector-authored correction feedback The 512-byte cap on correction feedback exists to bound text originating outside the detector — parser errors and model-written previews. The eligibility explanation is composed from the detector's own fixed strings, and applying the cap to it had a real cost: rejecting two or three categories at once produces up to 999 bytes, so the trailing explanations were cut mid-sentence and the model was told its verdict was rejected without being told why for every category it had to re-answer. Add BuildTrustedCorrectionPrompt for detector-composed feedback, which is bounded by construction, and use it on the eligibility path. BuildCorrectionPrompt keeps truncating and now delegates to it. Also name safe outputs in the secret_leak rejection message, so a model that found a secret in an issue or comment body can tell that body would have appeared in agent_output.json and re-check where it actually read the value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com> * docs(detector): document detection as a downstream gate, not a session audit The in-session blind spots (MCP tool results and fetched pages never staged; secrets exfiltrated mid-run never entering the bundle) were being read as gaps to close. They are not: detection runs after the agent finishes and gates whether its requested outputs may be applied, so anything the agent already did is by construction outside what it can gate. Network egress restriction and MCP tool constraints are the controls that cover in-session risk. Records this in the spec (new section 1.3), the README, and the prompt itself, which now frames the model's task as "should this be allowed to proceed?" and tells it not to infer threats from evidence it was never given. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com> * fix(detector): fail open on indeterminate provenance in eligibility A GPT-5.6 review pass found three paths where a missing or unreadable artifact made a category ineligible, so the binding parent-side check would discard a genuine finding. Suppressing real threats is the worst outcome this gate can produce, and each was a fail-closed bug. - Untrusted-input extraction needs both the prompt template and the rendered prompt, and matches the template's static segments against the rendered text; unmatched segments are skipped silently. An empty result therefore had two indistinguishable causes, and only the absent- template one was treated as degraded. Record indeterminacy explicitly in PromptAnalysis and grant eligibility on it, so an unreadable prompt.txt or a template/rendered divergence can no longer rule out injection. - An unreadable comment-memory directory was collapsed to "no files", making it indistinguishable from an absent one and able to make both prompt_injection and secret_leak ineligible. An uninspectable channel is a channel: add Artifacts.HasWarningForField and count it. - The prompt told the model to report unattributable suspicious content, then told it a prompt_injection verdict is false unless it can name the untrusted region, while the reason format offers "ORIGIN: unknown". The conflict landed exactly where eligibility now fails open. Resolve it on the axis that matters: the check requires concrete suspicious content, not a resolved origin, and degraded provenance is reported with ORIGIN: unknown rather than suppressed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com> * refactor(detector): derive eligibility from declared channels Eligibility was three hand-rolled boolean expressions with the comment-memory condition duplicated across two of them, and rejection messages that restated the channel list in prose. Adding a source meant finding every scattered condition, remembering the fail-open pattern, and updating strings by hand — and the two fail-closed bugs already found came from exactly that. Declare each source as a channel with separate `present` and `uninspectable` signals, either of which grants eligibility, and derive both the verdict and the rejection text from the same declarations. Adding a source is now one struct literal in inputChannels, outputChannels or patchChannel. Tests enforce the contract: every channel must be eligible on either signal, and every channel must be named in its rejection message. Uniform treatment also closes a fourth fail-closed path the earlier review did not reach: gh-aw sets HAS_PATCH when the agent job reported a patch, so a patch the host failed to stage is uninspectable rather than absent, and must not make malicious_patch ineligible. Behavior is otherwise unchanged; the existing eligibility tests pass untouched apart from one assertion on reworded message text. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com> * fix(artifacts): stop describing uninspectable channels as empty An artifact channel that exists but could not be read was handed to the detection model as "No comment-memory files found" / "No patch or bundle file found" -- the same text as a channel that genuinely holds nothing. The model then reported clean about content nobody looked at and the run exited 0, a fail-open outcome one layer above the eligibility check added earlier in this PR. Uninspectable channels now carry an explicit notice that they were not analyzed, plus a statement that the inspection failure is not itself evidence of a threat. That second half matters: without it a staging fault becomes a false-positive source, which is the failure mode this work exists to reduce. Also warns on the two silent refusal paths in comment-memory loading: a comment-memory entry that is not a directory, and a .md entry that is not a regular file. Both refusals are deliberate security guards against resolving outside the artifacts tree, which makes them agent-reachable -- returning silently let a run hide its comment memory while still reading as fully inspected. Refs #954, #955 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com> --------- Co-authored-by: GitHub Ace <githubnext@users.noreply.github.com> Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com>
1 parent b298231 commit 0003e99

14 files changed

Lines changed: 1535 additions & 39 deletions

File tree

README.md

Lines changed: 128 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,35 @@ This project is designed to help reduce risk when running AI agent workflows by
4545

4646
Do not treat a "safe" result as a security guarantee. Use the output as one signal in a broader security review process.
4747

48+
### What this layer is for
49+
50+
Threat detection is a **gate on downstream actions**, not a post-hoc audit of
51+
the agent's session. It runs after the agent finishes and before the
52+
safe-output job, and it decides whether the effects the agent is asking for —
53+
issues, comments, pull requests, patches — may be applied. What it analyzes is
54+
the staged artifact bundle, which is the record of what the agent wants
55+
published.
56+
57+
So anything the agent already **did** during its session is outside what this
58+
can gate, because it happened before detection ran:
59+
60+
- **Mid-run injection sources are not staged.** MCP tool results, fetched web
61+
pages, and the engine transcript never become artifacts, so an injection that
62+
arrived only through one of those leaves no trace in the analyzed inputs.
63+
- **Mid-run exfiltration never enters the bundle.** A secret sent out over an
64+
outbound request or an MCP call is gone by the time detection runs; there is
65+
no artifact for it to appear in.
66+
67+
Neither is a gap this component intends to close. Those risks belong to the
68+
controls that are live *while* the agent runs — network egress restriction (the
69+
[agentic workflow firewall](https://github.com/github/gh-aw-firewall)) and limits on which
70+
MCP servers and tools are reachable. Threat detection complements those controls
71+
and does not substitute for them.
72+
73+
This is also why the [structural eligibility](#structural-eligibility) rules are
74+
written in terms of artifacts: the bundle is the evidence that exists at the
75+
moment the gate is applied.
76+
4877
## Usage
4978

5079
### CLI
@@ -63,7 +92,7 @@ threat-detect [flags] <artifacts-dir>
6392
- `--custom-prompt-file` — Path to a file with additional detection instructions. Takes precedence over `--custom-prompt` and `CUSTOM_PROMPT`
6493
- `--output` — Path to write the JSON result (defaults to stdout). Its `reasons` array is always empty; see [Where the reasons go](#where-the-reasons-go)
6594
- `--full-output` — Path to write the JSON result *including* reasons. Defaults to the `--output` path with `_full` inserted before the extension (`detection_result.json``detection_result_full.json`); pass an explicitly empty value to disable it. **Hosts must not upload this file**
66-
- `--retries` — Retries after a failed detection attempt. **Default: `0`** (env: `THREAT_DETECTION_RETRIES`). A from-scratch retry rarely fixes anything: the 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. `--engine-timeout` is **always terminal** regardless of this value — retrying a runaway is overwhelmingly likely to run away again
95+
- `--retries` — Retries after a failed detection attempt, including one rejected on structural [eligibility](#structural-eligibility). **Default: `0`** (env: `THREAT_DETECTION_RETRIES`). A from-scratch retry rarely fixes anything: the 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 — including an ineligible one — without a subprocess restart. `--engine-timeout` is **always terminal** regardless of this value — retrying a runaway is overwhelmingly likely to run away again
6796
- `--engine-timeout` — Wall-clock timeout per detection attempt (Go duration, e.g. `5m`, `300s`). On expiry the engine subprocess **and its harness descendants** are killed via a process-group `SIGKILL`, so the actual model CLI running as a grandchild under `node` cannot keep burning credits after the deadline. If the result sink already holds a valid verdict written just before the deadline, that verdict wins over the timeout. Timeouts are terminal — the run exits `2` with the status reason `engine_timeout` immediately, without consuming any `--retries`. `0` disables the cap. Default: `5m` (env: `THREAT_DETECTION_ENGINE_TIMEOUT`)
6897
- `--max-turns` — Maximum agentic tool-use turns per attempt. Exported to the engine subprocess as `GH_AW_MAX_TURNS` (which the Claude, Codex, and Copilot harnesses read) and additionally passed as `--max-turns` to the bare Claude CLI. The bare Copilot CLI has no equivalent flag, so on that path only `--engine-timeout` enforces the cap; the detector logs a diagnostic when `--max-turns` is set for that path. `0` disables the cap and scrubs any inherited `GH_AW_MAX_TURNS` from the engine subprocess's env. Default: `50` — the turn cap's real job is catching tool-loop pathology (model stuck calling Read in a loop), not being the primary credit bound; the wall-clock is the primary bound, and 50 gives comfortable headroom for legitimate wide exploration (e.g. a patch touching many files). (env: `THREAT_DETECTION_MAX_TURNS`; also honors `GH_AW_MAX_TURNS` as a fallback so a turn budget configured for the harness-driven path applies to the standalone detector too)
6998
- `--step-summary` — Deprecated and ignored. Accepted so hosts that still pass it (older `gh-aw` releases) do not fail; the detector no longer writes a GitHub Actions step summary
@@ -73,9 +102,11 @@ threat-detect [flags] <artifacts-dir>
73102
verdict in-session by invoking the `threat_detection_result` tool, which writes
74103
a strict JSON object matching the result contract to an out-of-band result sink;
75104
the detector cancels the engine subprocess as soon as a valid result is written.
76-
The verdict is read exclusively from that sink; if no sink result is produced, a
77-
self-correction prompt is retried (`--retries`, once by default), and retry
78-
exhaustion is treated as an infrastructure error.
105+
The verdict is read exclusively from that sink; if no sink result is produced,
106+
or the reported verdict is rejected on structural eligibility (see [Structural
107+
eligibility](#structural-eligibility)), a self-correction prompt is retried
108+
(`--retries`, once by default), and retry exhaustion is treated as an
109+
infrastructure error.
79110

80111
#### In-session result reporting (`threat_detection_result`)
81112

@@ -120,6 +151,98 @@ exclusively from the sink; it does not scrape the engine transcript.
120151
- `1` — Threat detected
121152
- `2` — Infrastructure/configuration error
122153

154+
#### Structural eligibility
155+
156+
Each threat category is only raisable against an artifact bundle that could
157+
plausibly exhibit it:
158+
159+
- `prompt_injection` requires untrusted content that could have reached this
160+
run's inputs — a non-empty untrusted region in the prompt analysis, or a
161+
comment-memory file (which the agent reads back into its prompt).
162+
- `malicious_patch` requires at least one `.patch` or `.bundle` file in the
163+
artifact bundle. Framework-rejected safe-output validation errors are the
164+
guardrails working; they are not patches.
165+
- `secret_leak` requires a channel a secret could have reached — a non-empty
166+
agent output, a patch or bundle, or a comment-memory file. Note that
167+
`agent_output.json` is gh-aw's safe-outputs file, so issue bodies, comment
168+
bodies and PR descriptions the agent asked the framework to publish are
169+
covered by the agent-output channel rather than being separate ones.
170+
171+
Eligibility fails open whenever the evidence for a precondition could not be
172+
established — a missing, unreadable, or malformed artifact must never be able to
173+
suppress a real finding. An empty set of untrusted regions only proves the prompt
174+
received nothing untrusted if the analysis actually ran: if `prompt-template.txt`
175+
or the rendered prompt was unavailable, or the two could not be reconciled so
176+
regions were skipped, `prompt_injection` stays eligible. Likewise a
177+
comment-memory directory that exists but could not be read counts as a channel
178+
rather than as an absence.
179+
180+
The check runs in two places, with different standing:
181+
182+
- **In the reporting tool**, from `THREAT_DETECTION_ELIGIBLE_*` environment
183+
variables, so an ineligible report is corrected in-session without another
184+
engine pass. The tool prints `THREAT_DETECTION_RESULT_ERROR:` naming the
185+
category and what would make it eligible. This check is **advisory** — the
186+
model composes the command line that carries those variables, so it can
187+
override or omit them.
188+
- **In the detector process**, against the eligibility it computed from the
189+
artifacts itself. Every result read from the sink is re-checked here before
190+
it is recorded, so a verdict that bypassed the tool (or overrode its
191+
environment) is still rejected. This is the binding check.
192+
193+
An ineligible result is treated exactly like a malformed one: it is discarded,
194+
never rewritten, so the sink stays the sole source of any recorded result. With
195+
the default `--retries 0` the advisory tier is what recovers the run — the tool
196+
rejects the call and the model re-answers in-session, without a restart. The
197+
binding tier is the backstop for a verdict that reached the sink anyway: with no
198+
retries left it ends the run at `invalid_report_exhausted` and exit 2, and where
199+
retries are configured it first feeds the rejection back as a self-correction.
200+
An attempt that also hit `--engine-timeout` stays terminal and is not retried.
201+
202+
#### Adding a channel
203+
204+
Eligibility is derived from *channels* — declared in `inputChannels`,
205+
`outputChannels` and `patchChannel` in `pkg/detector/eligibility.go` — rather
206+
than from conditions written per category. To make a new artifact source count,
207+
append a channel to the relevant list:
208+
209+
```go
210+
{
211+
name: "an MCP tool result recorded during the run",
212+
present: len(arts.MCPToolResults) > 0,
213+
uninspectable: arts.HasWarningForField("mcp_tool_results"),
214+
}
215+
```
216+
217+
Both signals matter. `present` means content was found; `uninspectable` means
218+
the channel may hold content the detector could not read. Either makes the
219+
category eligible, because an artifact the detector failed to inspect must never
220+
be mistaken for one that does not exist — collapsing the two is what caused the
221+
fail-closed bugs this shape prevents. Tests enforce both halves: every channel
222+
must be eligible on either signal, and every channel must be named in the
223+
rejection message, which is generated from the same declarations.
224+
225+
A source also has to be *staged* before it can be a channel. Adding one to the
226+
detector only matters once the host writes it into the artifacts directory and
227+
`pkg/artifacts` loads it; until then there is nothing to point at.
228+
229+
A channel that is `uninspectable` also has to *say so in the prompt*. Eligibility
230+
decides whether a verdict may be raised from a channel; it does not decide what
231+
the model is told about it. If the loader describes an unread channel as empty,
232+
the model reports clean about content nobody looked at and the run exits 0 —
233+
a fail-open outcome one layer above eligibility. Use
234+
`artifacts.uninspectableNotice` so the description says the channel was
235+
unexamined rather than empty, and states that the failure is not itself evidence
236+
of a threat. That second half is not decoration: without it a staging fault
237+
becomes a false-positive source, which is the failure mode eligibility exists to
238+
reduce.
239+
240+
Eligibility is scoped to the artifact bundle. MCP tool results, fetched web
241+
content, and the engine transcript are never staged as artifacts, so an
242+
injection delivered only through those channels leaves no evidence in the
243+
analyzed inputs and is outside what this detection pass can see. See spec
244+
TD-10g.
245+
123246
The detector also emits a single machine-readable status line to stderr at the end
124247
of every detection run: `THREAT_DETECTION_STATUS: reason=<reason> exit=<code>`.
125248
(Informational modes that exit before running detection — `--help` and `--version`
@@ -167,7 +290,7 @@ log consumers must ignore any `THREAT_DETECTION_*` marker that carries the
167290
`[engine] ` prefix.
168291

169292
```text
170-
[threat-detect] run start: version=1.2.3 engine=copilot model=(none; using engine default) retries=1
293+
[threat-detect] run start: version=1.2.3 engine=copilot model=(none; using engine default) retries=0 max_turns=50 engine_timeout=5m0s
171294
[threat-detect] artifacts loaded: dir=/tmp/gh-aw/threat-detection prompt_bytes=4096 agent_output_bytes=812 patch_files=1 all_primary_inputs_missing=false
172295
[threat-detect] artifact inventory (3 entries):
173296
[threat-detect] aw-prompts/prompt.txt bytes=4096 kind=file consumed=true

0 commit comments

Comments
 (0)