Repository navigation
test(conclude): expand conformance matrix for gh-aw-threat-detection#694 - #738
Merged
Merged
Conversation
Add test cases covering the full {result state} x {warn/strict} x
{execution outcome} matrix for the `conclude` subcommand: success is
unaffected by warn mode/executionFailed, threat_detected always warns
in warn mode (mustFail excludes it), malformed/missing/unreadable
result files are exercised across all warn-mode/executionFailed
combinations to confirm mustFail scoping.
conclude.go itself already implements the fixes called for in the
issue (writes reason/conclusion on every fail path including strict
missing-file, exports GH_AW_DETECTION_* consistently, and documents
the mustFail divergence in specs/threat-detection-spec.md TD-20b/21a).
The remaining divergence is entirely in gh-aw's
actions/setup/sh/conclude_threat_detection.sh, which lives in the
github/gh-aw repo and is out of scope for this repo.
Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Expands conclude conformance coverage for issue #694 without changing production behavior.
Changes:
- Adds safe and threat-verdict execution-outcome cases.
- Expands malformed, missing, and unreadable-result coverage.
Show a summary per file
| File | Description |
|---|---|
cmd/threat-detect/conclude_test.go |
Extends conclusion contract tests across warning, strict, and execution outcomes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (1)
cmd/threat-detect/conclude_test.go:287
- This table contains only three of the four
{warn/strict} x {execution outcome}combinations: strict mode withexecutionFailed=trueis absent, despite the updated doc comment saying the unreadable-file matrix is complete.
{
name: "strict mode fails closed",
warnMode: false,
wantExit: concludeExitFail,
wantConclusion: "failure",
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Balanced
Comment on lines
+194
to
+204
| { | ||
| name: "malformed file warns when execution succeeded", | ||
| runDetection: "true", | ||
| warnMode: true, | ||
| executionFailed: false, | ||
| resultContent: "{not json", | ||
| wantExit: concludeExitProceed, | ||
| wantConclusion: "warning", | ||
| wantSuccess: "false", | ||
| wantReason: "parse_error", | ||
| }, |
Comment on lines
+327
to
+337
| outputs := parseKV(t, filepath.Join(dir, "out")) | ||
| if got := outputs["reason"]; got != "agent_failure" { | ||
| t.Errorf("reason output = %q, want %q", got, "agent_failure") | ||
| } | ||
| if got := outputs["conclusion"]; got != tt.wantConclusion { | ||
| t.Errorf("conclusion output = %q, want %q", got, tt.wantConclusion) | ||
| } | ||
| env := parseKV(t, filepath.Join(dir, "env")) | ||
| if got := env["GH_AW_DETECTION_REASON"]; got != "agent_failure" { | ||
| t.Errorf("GH_AW_DETECTION_REASON = %q, want %q", got, "agent_failure") | ||
| } |
Add strict-mode + executionFailed=true cases for malformed and unreadable result files, and assert success/GH_AW_DETECTION_CONCLUSION on the unreadable-file matrix (previously only reason/conclusion were checked), per copilot-pull-request-reviewer feedback on #738. Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com>
davidslater
added a commit
that referenced
this pull request
Aug 4, 2026
…-log) Merges origin/main (PR #742 workflow-context flags/--custom-prompt-file, PR #737 --detection-log-based failure-reason refinement, PR #743 comment-memory analysis, PR #738 conclude test matrix) into the step-summary branch. Conflicts were in cmd/threat-detect/main.go and conclude.go, both from overlapping var/flag blocks and the two independent conclude.go refactors (fail() gaining a *detector.Result param for step-summary vs. detectionFailureReason()/--detection-log for status-aware failure reasons). Resolved by keeping both flag sets and having the fs.ErrNotExist/PathError branches call detectionFailureReason() and then fail(nil, reason, message) with the new three-arg signature. Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com>
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.
Closes #694
Context
Issue #694 describes a divergence between two implementations of the gh-aw job-output contract:
actions/setup/sh/conclude_threat_detection.sh— lives ingithub/gh-aw, not this repo.cmd/threat-detect/conclude.go— lives in this repo.Findings
I verified the current state of both sides:
conclude.go+specs/threat-detection-spec.md) already fixes divergences (2), (3), and (4) from the issue:reasonandconclusionto$GITHUB_OUTPUT, so a strict-mode infrastructure failure no longer renders as a generic/confusing threat message.GH_AW_DETECTION_CONCLUSION/GH_AW_DETECTION_REASONare exported to$GITHUB_ENVon every path, including skip.mustFailfail-closed rule (execution failure +agent_failure/parse_error) is explicitly documented inspecs/threat-detection-spec.mdTD-20b and TD-21a, including the acknowledged divergence from gh-aw's inline path.github/gh-aw'sactions/setup/sh/conclude_threat_detection.sh, which I fetched and confirmed still has the exact bugs from the issue (strict-mode missing-file branch exits 1 without writingreason/conclusion; noGH_AW_DETECTION_*env export; short-circuits before ever callingthreat-detect concludefor the missing-file case). Fixing that requires a change in thegithub/gh-awrepo, which is out of scope for this repo/PR.Change in this PR
Expanded
cmd/threat-detect/conclude_test.goto close the remaining gaps in proposed work item (4) — conformance tests covering the full matrix:executionFailed.threat_detectedalways warns in warn mode, even when execution failed (mustFailis scoped toagent_failure/parse_erroronly, neverthreat_detected).{warn/strict} x {execution outcome}combinations (previously only spot-checked in strict mode).No behavior change to
conclude.go— this is test-only.Follow-up needed (not in this repo)
File/track a corresponding fix in
github/gh-awforactions/setup/sh/conclude_threat_detection.shto either delegate its missing-file handling tothreat-detect conclude(simplest: always runthreat-detect conclude, since it already handles missing/unreadable files correctly) or replicate the output/env contract fixes inline.