fix(artifacts): emit ERR_VALIDATION warnings for degraded/missing inputs - #750
Merged
Conversation
Previously pkg/artifacts.Load silently substituted sentinel strings for missing prompt.txt, agent_output.json, and patch/bundle files, so a staging bug (wrong artifact name, failed download, empty file) degraded detection to a silent no-op that returned a clean verdict — a fail-open failure mode in a security control. - Load now emits an ERR_VALIDATION warning (matching gh-aw's error-code prefix) for: missing/empty prompt.txt, missing/empty/invalid-JSON agent_output.json, and HAS_PATCH=true with no aw-*.patch/.bundle found. - Adds Artifacts.Warnings ([]ArtifactWarning) and PromptFileSize / AgentOutputFileSize fields for size-aware diagnostics. - Adds Artifacts.AllPrimaryInputsMissing: when prompt, agent output, and patch are all missing/empty simultaneously, main.go now hard-fails with config_error (exit 2) instead of proceeding to a clean pass. - main.go surfaces each warning as a ::warning:: annotation and mirrors it (plus the hard-fail) into the JSONL run log. - Adds tests for each degraded-input case in pkg/artifacts. Fixes #701 Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com>
- Treat zero-length/unreadable patch or bundle files as absent for both the HAS_PATCH cross-check and AllPrimaryInputsMissing, closing the loophole where an empty patch file bypassed the fail-closed check. - Escape warning messages with escapeWorkflowData before emitting them as ::warning:: annotations, since they embed the caller-supplied artifacts directory path and could otherwise forge additional workflow commands. - Add run() tests covering the empty-artifacts-directory fail-closed path (with and without --log-file), and artifacts tests for zero-length patch files. Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com>
Rebasing onto main's #744 (prompt-analysis degradation warnings) surfaced two tests that built empty artifact directories, which now trip the all-primary-inputs-missing hard-fail path added by this PR. Give them minimal valid prompt.txt/agent_output.json via writeMinimalArtifacts so they exercise only the prompt-template/import-tree degradation they're actually testing. Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds visibility and fail-closed handling for missing or degraded threat-detection artifacts.
Changes:
- Adds structured
ERR_VALIDATIONwarnings and artifact size metadata. - Rejects runs where all primary inputs are unavailable.
- Expands CLI and artifact-loading tests for degraded inputs.
Show a summary per file
| File | Description |
|---|---|
pkg/artifacts/artifacts.go |
Detects degraded or absent inputs. |
pkg/artifacts/artifacts_test.go |
Tests artifact validation behavior. |
cmd/threat-detect/main.go |
Emits warnings, logs events, and fails closed. |
cmd/threat-detect/main_test.go |
Tests fail-closed CLI behavior. |
cmd/threat-detect/promptcontext_test.go |
Seeds valid artifacts for context tests. |
cmd/threat-detect/logfile_test.go |
Updates run-log and inventory tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (2)
pkg/artifacts/artifacts.go:316
agentOutputInvalidis non-empty input and its path is still passed to the model, so treating it as absent makes a missing prompt plus malformed agent output fail withconfig_erroreven though there is content to analyze. This also contradictsAllPrimaryInputsMissing's documented “missing or empty” condition. Keep invalid JSON as a degraded-input warning, but reserve this hard fail for missing or zero-length output.
arts.AllPrimaryInputsMissing = (promptMissing || promptEmpty) &&
(agentOutputMissing || agentOutputEmpty || agentOutputInvalid) &&
!hasReadablePatch
cmd/threat-detect/main.go:247
- The new
config_errorcondition is an externally observable CLI behavior change, but the normative threat-detection and usage specs do not define the degraded-input warnings or when all primary inputs must hard-fail. The repository treats those specs as the source of truth, and the linked issue explicitly requires the hard-error decision to be documented. Add normative requirements for these warnings and the exact hard-fail condition alongside this implementation.
// All primary inputs missing simultaneously means detection would silently
// analyze nothing and return a clean verdict — a fail-open failure mode in
// a security control. Fail closed instead of proceeding.
if arts.AllPrimaryInputsMissing {
fmt.Fprintf(os.Stderr, "Error: prompt, agent output, and patch/bundle files are all missing or empty in %s; refusing to run detection on empty input.\n", artifactsDir)
logger.Error("artifacts_all_primary_inputs_missing", map[string]any{"artifacts_dir": artifactsDir})
reason = reasonConfigError
return exitError
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Balanced
| fmt.Fprintf(os.Stderr, "::warning::%s\n", escapeWorkflowData(w)) | ||
| logger.Info("artifacts_warning", map[string]any{"warning": w}) | ||
| fmt.Fprintf(os.Stderr, "::warning::%s\n", escapeWorkflowData(w.Message)) | ||
| logger.Error("artifact_degraded", map[string]any{"field": w.Field, "message": w.Message}) |
Comment on lines
+227
to
+230
| agentOutputData, agentOutputErr := os.ReadFile(agentOutputPath) | ||
| agentOutputMissing := agentOutputErr != nil | ||
| agentOutputEmpty := agentOutputErr == nil && len(agentOutputData) == 0 | ||
| agentOutputInvalid := agentOutputErr == nil && len(agentOutputData) > 0 && !json.Valid(agentOutputData) |
Comment on lines
+283
to
289
| info, statErr := os.Stat(p) | ||
| if statErr != nil { | ||
| continue | ||
| } | ||
| if info.Size() > 0 { | ||
| hasReadablePatch = true | ||
| } |
Comment on lines
+559
to
+563
| info, err := os.Stat(path) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| return info.Size(), nil |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebased continuation of #740 (its Ace session died with a merge conflict against
main). This re-applies the same three commits on top of currentmainand resolves the conflict with the new step-summary tests from #739.Fixes #701.
Problem
pkg/artifacts.Loadsilently substituted sentinel strings ("No prompt file found", etc.) for missing or degraded inputs, so a staging bug (wrong artifact name, failed download, empty file) degraded detection to a no-op that passes with a clean verdict — a fail-open failure mode in a security control.Changes
Loadnow emits anERR_VALIDATION-prefixed warning (matching gh-aw'serror_codes.cjsconvention) for:aw-prompts/prompt.txtagent_output.jsonHAS_PATCH=trueset in the environment but noaw-*.patch/aw-*.bundlefoundArtifactsfields:Warnings []ArtifactWarning,PromptFileSize,AgentOutputFileSize, andAllPrimaryInputsMissing.main.goreturns exit 2 (config_error) instead of proceeding to analyze nothing and returning a clean verdict.main.gosurfaces each warning as a::warning::GitHub Actions annotation on stderr and mirrors it (plus the hard-fail) into the JSONL run log (artifact_degraded/artifacts_all_primary_inputs_missingevents).pkg/artifacts/artifacts_test.gofor each degraded-input case;cmd/threat-detecttests using empty artifact directories now seed a minimal valid prompt/agent-output viawriteMinimalArtifacts.Rebase note
The only conflict was in
cmd/threat-detect/main_test.go, where #739 addedTestRunWritesPromptStepSummary/TestRunStepSummaryDefaultsToEnvin the same location the PR addedTestRunFailsClosedOnEmptyArtifactsDirectory{,WithLogFile}. Resolved by keeping all four tests and seeding the step-summary tests withwriteMinimalArtifactsso they don't hit the new all-inputs-missing hard-fail.Out of scope (tracked separately)
prompt-template.txt/prompt-import-tree.jsonstaging (Parity: prompt-template.txt / prompt-import-tree.json are never staged, so trusted-vs-untrusted prompt analysis is silently disabled #686) andcomment-memory/analysis (Parity: comment-memory files are not analyzed (missing {COMMENT_MEMORY_FILES} prompt section and artifact discovery) #689) — not yet implemented in this repo.Testing
make fmt lint build— cleango test ./...andgo test -race ./cmd/... ./pkg/artifacts/...— all pass