feat(detector): enforce structural eligibility for threat verdicts

[davidslater]

Created by GitHub Ace · View Session

Addresses the false-positive class seen in recent runs (PR Description Updater 32201512271 / 32203115530 / 32239956539, PR Sous Chef 32305540872) where the detection model reported structurally impossible verdicts — malicious_patch=true with zero patch files, prompt_injection=true citing only the workflow author's own trusted template — and, in one case, wrote "this is a workflow execution error, not a security vulnerability" in its own reason text while setting the flag to true anyway.

The fix is three layers, ordered by how much they depend on the model behaving:

1. Structural eligibility (deterministic, in code)

New pkg/detector/eligibility.go computes per-category eligibility from the loaded artifacts and the static prompt analysis:

• prompt_injection requires ≥1 non-empty untrusted region in PromptAnalysis (an interpolated placeholder, an import-tree entry, or the activation-context summary). Without any untrusted content in the instruction channel there is nothing an injection could have arrived through.
• malicious_patch requires ≥1 .patch or .bundle file. A framework-rejected safe-output validation error (run 32305540872's review_id type mismatch) is the guardrail working, not a patch.
• secret_leak requires a non-empty agent output or ≥1 patch/bundle — a channel a secret could have leaked through.

The detector transports eligibility to the report-result subprocess via THREAT_DETECTION_ELIGIBLE_* env vars. The threat_detection_result tool rejects any threat=true claim against an ineligible category as a normal correctable error (same path as a malformed report today), and the model re-answers within the same engine session. A missing or unparseable transport variable defaults to permissive so pre-existing callers and tests aren't tightened.

This is the safety net: no prompt wording alone can ship malicious_patch=true with zero patch files anymore.

2. Prompt taxonomy (reduces the rate at which the safety net has to fire)

Adds a "Not a Threat" section to the default detection prompt naming the recurring false-positive patterns:

• Instruction non-compliance is a quality signal, not a security verdict — unless untrusted content caused it.
• Framework-rejected safe-output validation errors are the guardrails working, not attacks.
• Workflow execution errors are tooling failures, not findings — if the reason text would say "this is a workflow execution error", the verdict MUST be false.
• prompt_injection requires an untrusted origin; the trusted template and framework scaffolding are what the injection is trying to subvert, not injection sites themselves.
• malicious_patch requires a patch; secret_leak requires a channel.

Followed by a self-check the model runs before setting any flag true: name the untrusted content, confirm the reason doesn't contradict the verdict, confirm a maintainer reading only the reason would understand it as an attack.

The section explicitly says the tool enforces the last three as structural eligibility, so the model doesn't have to guess at the contract.

Spec & docs

• Spec: adds normative TD-10g stating the eligibility invariants, the correction-loop enforcement contract, and the requirement that the default prompt documents the taxonomic distinctions.
• README: new "Structural eligibility" section, updated retries default and example log line.

Tests

• pkg/detector/eligibility_test.go — computation, env round-trip, unset/unparseable permissiveness, Validate message content.
• cmd/threat-detect/report_test.go — ineligible malicious_patch/prompt_injection claims rejected with no sink write; eligible claims still recorded; safe verdicts unaffected regardless of eligibility.
• Updated existing TestRunReportsArtifactInventoryOnStderr for the new retries=3 default (attempts=4).

make fmt lint build test test-scripts all clean.

What this does not try to do

• No post-hoc override of the model's verdict based on reason text — the sink stays the sole source of the recorded verdict; eligibility rejects before write and feeds the correction back, so the model itself re-answers.
• No enforcement that reason text contains a specific substring from an untrusted segment — that's fragile and left to prompt taxonomy.
• No new persisted files or reason echoes — TD-10f is preserved; correction messages travel through the existing THREAT_DETECTION_RESULT_ERROR channel, which is framed and inert on stderr.

[Copilot]

Pull request overview

Adds structural eligibility checks to prevent impossible threat verdicts.

Changes:

• Computes and enforces per-category eligibility through the result tool.
• Expands prompt taxonomy and documents eligibility rules.
• Raises default retries from one to three and adds tests.

Show a summary per file

File	Description
specs/threat-detection-spec.md	Defines eligibility requirements.
README.md	Documents eligibility and retry behavior.
pkg/engine/engine.go	Transports eligibility to engines.
pkg/detector/prompts/threat_detection.md	Adds false-positive guidance.
pkg/detector/eligibility.go	Implements eligibility computation and validation.
pkg/detector/eligibility_test.go	Tests eligibility behavior.
cmd/threat-detect/report.go	Rejects ineligible reports.
cmd/threat-detect/report_test.go	Tests report rejection and acceptance.
cmd/threat-detect/main.go	Integrates eligibility and increases retries.
cmd/threat-detect/main_test.go	Updates retry expectations.

Review details

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

Suppressed comments (4)

pkg/detector/eligibility.go:61

• Comment memory is another persisted output channel that can contain leaked secrets. TD-18a explicitly requires it to be analyzed for secret leakage, but this condition rejects secret_leak=true when comment memory is the only non-empty output. Include discovered comment-memory files in this eligibility check.

	if arts != nil && (arts.AgentOutputFileSize > 0 || len(arts.PatchFiles) > 0) {

specs/threat-detection-spec.md:198

• TD-18a defines comment memory as input that must be analyzed for secret leakage, but this exhaustive channel list omits it. As written, the new rule makes a secret found only in comment memory structurally unreportable. Add non-empty comment-memory output as an eligible channel.

- `secret_leak` is eligible only when the artifact bundle contains at least
  one channel a secret could have leaked through — a non-empty agent output
  file or at least one patch or bundle file.

README.md:138

• Comment memory is a persisted channel written by the agent and TD-18a requires checking it for secret leakage. This documented eligibility would incorrectly reject a leak found only there; include comment-memory files in the channel list.

- `secret_leak` requires either a non-empty agent output or at least one patch
  or bundle — a channel a secret could have leaked through.

pkg/detector/prompts/threat_detection.md:166

• This claim overlooks comment memory, which the template later calls an attacker-influenced persisted channel to analyze for secret leakage. A secret can be present there even when agent output and patches are empty, so this instruction would suppress a valid finding. Add comment memory to the listed channels.

6. **`secret_leak` requires a channel.** If the agent produced no output and
   no patch, there is nowhere a secret could have leaked to.

• Files reviewed: 10/10 changed files
• Comments generated: 8
• Review effort level: Balanced