feat(detector): surface detector-authored warnings in the result contract

[davidslater]

Created by GitHub Ace · View Session

Closes #954.

Adds an optional warnings: [] array to the detection result so partial-inspection failures are visible to hosts programmatically on the uploaded result — not only as ::warning:: annotations in the job log.

Problem

The loader records an ArtifactWarning when an artifact channel is present but cannot be inspected (prompt / agent_output / patch staged but empty or unreadable; comment-memory unreadable). It emits each as a ::warning:: annotation and then drops it. detection_result.json carries only the three booleans and reasons, so a partially-inspectable bundle is indistinguishable from a fully-inspected clean one to any programmatic consumer: the detector analyzed less than the full artifact set, reported clean, and exited 0.

Change

New optional field on Result:

"warnings": [
  { "field": "comment_memory", "code": "ERR_VALIDATION",
    "message": "Unable to read comment-memory directory at /tmp/…: permission denied" }
]

• Populated by writeResult from Artifacts.Warnings, on a copy of the sink Result.
• Written to both --output and --full-output, unlike reasons. Warnings carry no model-authored text (fixed detector strings + host-controlled paths), so publishing them is safe — and the published artifact is exactly where the signal was missing.
• Rendered by threat-detect conclude under a distinct ⚠️ block:

  ⚠️  Detector warnings (2) — artifact channels that could not be fully inspected. These do not affect the verdict.
       [1] field=comment_memory code=ERR_VALIDATION message=Unable to read comment-memory directory at /tmp/…
  

• Fields sanitized on the way out, the same protection reasons get — a host-controlled path with an embedded CR cannot forge a workflow command (regression-tested).

Invariants

• Warnings never affect the verdict or the exit code, including under GH_AW_DETECTION_CONTINUE_ON_ERROR=false. Conflating "could not inspect" with "found a threat" would reintroduce the false-positive mode feat(detector): enforce structural eligibility for threat verdicts #916 exists to reduce; gating is a host policy decision tracked separately in the CONTINUE_ON_WARNING issue.
• The verdict remains sourced solely from the sink (TD-06a). The warnings assignment is unconditional, which is a security boundary rather than plumbing: the reporting tool exposes no warnings flag, but the engine holds a file-writing tool and knows THREAT_DETECTION_RESULT_FILE, so overwriting is what stops a model that wrote the sink directly from smuggling text into a published file. Tested.
• Bounds enforced identically on write and read (≤20 entries; field/code non-empty ≤64 runes; message non-empty ≤2000 runes), so nothing accepted on write is rejected on read. Over-long values are clipped within the bound with an ellipsis — deliberately not conclude.go's truncateRunes, which appends "… (truncated)" past the requested length and would produce a file the detector rejects on read.
• Additive and backward compatible: optional on parse (older results read fine), always emitted as [] (newer consumers can index without a null check).

Compatibility — verified against real consumers

I checked all three things that actually read detection_result.json:

Consumer	Parsing	Safe?
threat-detect conclude	this repo's parser	✅ updated here
gh-aw conclude_threat_detection.sh (pinned v0.87.4)	none — execs threat-detect conclude	✅
scripts/collect-detection-stats.sh	jq has("prompt_injection") and … presence check	✅ tolerant of new fields

The detection artifact uploads detection_result.json by exact filename, not a glob, so detection_result_full.json still never leaves the runner.

Stale docs corrected

Checking the above turned up an inaccurate claim in README.md and CLAUDE.md: that the smokes "do not exercise threat-detect conclude" and so "show no reasons". At the pinned v0.87.4 SHA the script does no parsing — it delegates, passing no --full-result-file, so the sibling full result is derived and reasons (and now warnings) do render in smoke job logs. Corrected both, and noted that warnings — unlike reasons — are present in the published artifact.

Verification

Beyond unit tests, I exercised the built binary end to end:

• report-result → sink round-trips with the new field.
• conclude on a safe verdict + 2 warnings → ⚠️ block renders, conclusion=success, exit 0.
• conclude on a pre-change result with no warnings key → parses, no spurious block, exit 0.
• Strict mode (CONTINUE_ON_ERROR=false) + safe verdict + warnings → exit 0, proceeds. Warnings do not fail the run.
• Strict mode + threat + warnings → exit 1, conclusion=failure. Threat still blocks.

make fmt lint build test test-scripts all clean.

Files

• pkg/detector/result.go — ResultWarning, bounds, schema, Redacted preserves warnings.
• pkg/artifacts/artifacts.go — export ErrCodeValidation; Code field on ArtifactWarning; MessageBody().
• cmd/threat-detect/main.go — buildResultWarnings, clipRunes, non-mutating writeResult.
• cmd/threat-detect/conclude.go — reportWarnings.
• specs/threat-detection-spec.md — TD-08 updated; new TD-10g and TD-20j.
• README.md, CLAUDE.md — "Warnings vs reasons" section; stale smoke-conclude claim corrected.

[Copilot]

Pull request overview

Adds structured detector-authored warnings for partially inspected artifacts.

Changes:

• Extends the result contract with bounded, validated warnings.
• Publishes warnings in both result files and renders them during conclusion.
• Adds documentation and regression tests.

Show a summary per file

File	Description
specs/threat-detection-spec.md	Specifies warning behavior and bounds.
README.md	Documents warnings and consumers.
CLAUDE.md	Corrects smoke-workflow behavior.
pkg/artifacts/artifacts.go	Adds structured warning codes.
pkg/detector/result.go	Extends parsing and serialization.
pkg/detector/result_warnings_test.go	Tests warning validation.
pkg/detector/result_file_test.go	Updates round-trip fixtures.
cmd/threat-detect/main.go	Attaches loader warnings to results.
cmd/threat-detect/warnings_test.go	Tests warning conversion and output.
cmd/threat-detect/conclude.go	Renders warning diagnostics.
cmd/threat-detect/conclude_warnings_test.go	Tests conclusion behavior.

Review details

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

• Files reviewed: 11/11 changed files
• Comments generated: 2
• Review effort level: Balanced

[davidslater]

Thanks — both comments were correct, and the second one was a real bug rather than a docs mismatch. Fixed in 7ae5e2d.

Unreadable prompt/patch were never detected

Confirmed empirically before changing anything. Staging a non-empty prompt.txt and aw-test.patch at mode 000 and calling artifacts.Load:

warnings: 0
HasWarningForField(prompt)=false patch=false
PromptFileSize=25 PatchFileInfo="/tmp/permcheck/aw-test.patch (19 bytes, git-patch)"

Zero warnings, and the patch is described to the model as present and inspected. The failure chain is worse than the missing warning alone: BuildPromptAnalysis reads the prompt under if err == nil { rendered = string(data) }, so the read error is silently discarded and an unreadable prompt is analyzed as the empty string. And since HasWarningForField("patch") was false, #916's uninspectable was false too, so eligibility treated the channel as cleanly present rather than failing open.

This also contradicted the code's own stated contract — the patch loop's comment claims a patch counts as present only if it is a "readable, non-empty regular file", which stat cannot establish.

Fix: readableFile opens the file and reads one byte, tolerating io.EOF so an empty-but-readable file is not misreported. Prompt and patch are probed; agent_output already used os.ReadFile and needed no change. An unreadable patch is routed through the existing unreadable list so it reuses the warning, the uninspectableNotice, and eligibility fail-open for free. An unreadable prompt yields no content, so it now counts toward AllPrimaryInputsMissing alongside missing and empty. Same fixture after:

warnings: 2
  field=prompt required=true msg=... could not be read (permission denied) ...
  field=patch  required=true msg=Unable to inspect 1 patch/bundle file(s) ...
HasWarningForField(prompt)=true patch=true
PatchFileInfo="Every patch/bundle file staged for this run was NOT analyzed ..."

Six regression tests added in pkg/artifacts/readability_test.go, plus two end-to-end ones. I verified they are genuine regression tests by neutering readableFile to return nil and confirming four of them fail; the two negative tests (readable and zero-byte inputs must not warn) correctly keep passing. They skip under root, where 0000 is advisory and the scenario cannot be built.

Strict mode — you're right, the invariant was overstated

main.go's !warnMode && arts.HasRequiredInputWarnings() returns config_error/exit 2 before writeResult, so "warnings never affect the exit code" was simply false for required inputs.

I took the "scope the requirement" option rather than moving the early return. Writing a result on that path would mean emitting a verdict for analysis that never ran — a clean-looking detection_result.json for a refused run is the exact fail-open outcome the refusal exists to prevent. So TD-10h now states what actually holds:

Warnings MUST NOT contribute to the verdict. No warning may cause a threat category to be reported true, and no warning may by itself cause the detector to exit with the threat-detected status.

followed by an explicit carve-out for TD-18c promotion, noting that the two exit statuses are distinct, that the run is refused rather than concluded, and why no result is written. TD-10h also now requires that inspectability not be decided on file metadata alone — which is what ties the two comments together. README carried the same overstatement in two places; both corrected, and the "recorded when" list now includes the unreadable cases that actually work.

TestWriteResult_WarningsDoNotChangeExitCode was already scoped to the write path with a non-required field, so it stayed valid; I sharpened its comment to record the boundary. Two new tests pin the behavior in both directions: warn mode surfaces the unreadable prompt in the published result and still exits 0, and strict mode exits 2 (explicitly asserting not exitThreat), does not invoke the engine, and writes no result file.

make fmt lint build test test-scripts clean, plus go test -race ./....

Not addressed here

warnDegradedPromptAnalysis (missing/empty prompt-template.txt / prompt-import-tree.json) still writes its ::warning:: straight to stderr without recording into Artifacts.Warnings, so it remains absent from the result. Same class of dropped signal, but #954 scoped this to Artifacts.Warnings and my docs never claimed that case works — unlike the two above. Flagged for a follow-up rather than folded in.

[davidslater]

Folded in the follow-up I'd flagged, in 47f1608 — warnDegradedPromptAnalysis was the last annotation-only path, so with it closed every degraded-inspection condition the detector knows about now reaches the published result.

It now returns its findings instead of only annotating, and they're merged into the set passed to writeResult. The annotation itself is unchanged, so job-log behavior is preserved — the result entry is additional, not a replacement.

The load-bearing detail is that these stay advisory. prompt-template.txt and prompt-import-tree.json are optional parts of the artifact contract. Had I recorded them with RequiredInput: true to match the other warning fields, TD-18c would promote them in strict mode and threat-detect would begin refusing every run of any host that never staged those files — silently converting an additive reporting change into a breaking one. So RequiredInput is false, pinned two ways: a unit test asserting HasRequiredInputWarnings() stays false, and an end-to-end test running strict mode against artifacts missing both files and requiring exit 0 with the engine still invoked.

Verified with the built binary. Warn mode publishes it:

"warnings": [
  { "field": "prompt_analysis", "code": "ERR_VALIDATION",
    "message": "Missing or unusable prompt analysis artifacts: aw-prompts/prompt-template.txt, aw-prompts/prompt-import-tree.json. ..." }
]

strict mode exits 0 with no "refusing to run degraded" line, and conclude renders it under the ⚠️ block. As before I confirmed the new test is a real regression test by dropping the merge and watching it fail.

Two smaller things: the reported set is built as a fresh slice rather than appending onto arts.Warnings, whose backing array belongs to the loader; and these findings are deliberately not added to arts.Warnings, so eligibility is untouched — #916 already derives prompt-injection inspectability from the analysis fields directly, and feeding it a second time would double-count.

Spec and README updated: TD-10h now requires that any degraded-inspection condition reported as an annotation also be recorded in warnings, and that doing so must not change whether a finding counts as a required input.

make fmt lint build test test-scripts and go test -race ./... clean.