Conversation
…r handling Agent-Logs-Url: https://github.com/github/gh-aw/sessions/804c490f-da87-4b17-8cbb-108ea36917dd Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent test quality
Test Classification Details
Analysis✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adjusts threat-detection workflow behavior so that, in warn mode (default), parse/infrastructure issues don’t fail the detection job and block downstream safe_outputs.
Changes:
- Add
continue-on-error: trueto the “Parse and conclude threat detection” step when threat detection is configured for warn mode. - Wrap the threat-detection parsing script’s main logic in a top-level try/catch to route unexpected runtime errors through the normal output-setting path.
- Add a table-driven Go test asserting warn-mode vs strict-mode
continue-on-errorbehavior, and recompile workflow lockfiles.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/threat_detection.go | Conditionally emits continue-on-error: true for the detection conclusion step in warn mode. |
| actions/setup/js/parse_threat_detection_results.cjs | Adds top-level try/catch around the parsing/conclusion flow to avoid uncaught exceptions bypassing outputs. |
| pkg/workflow/detection_success_test.go | Adds coverage asserting presence/absence of continue-on-error: true based on warn vs strict mode. |
| .github/workflows/*.lock.yml | Recompiled generated workflow YAML to include the new warn-mode continue-on-error behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 193/193 changed files
- Comments generated: 3
| // Top-level try/catch ensures outputs are always set and the step never throws | ||
| // unexpectedly. Any unanticipated runtime error (e.g. missing module, I/O error | ||
| // outside the guarded paths) is caught here and surfaced as a parse_error warning | ||
| // (in warn mode) or failure (in strict mode). This is a defence-in-depth measure | ||
| // complementing the continue-on-error: true that is set on the parse step in warn mode. | ||
| try { | ||
| await runMain(); | ||
| } catch (/** @type {any} */ unexpectedError) { | ||
| const errorMsg = getErrorMessage(unexpectedError); | ||
| core.error(`❌ Unexpected error in threat detection parse: ${errorMsg}`); | ||
| setDetectionFailure("unexpected_error", `${ERR_SYSTEM}: ❌ Unexpected error in threat detection parse: ${errorMsg}`); | ||
| } |
| // Top-level try/catch ensures outputs are always set and the step never throws | ||
| // unexpectedly. Any unanticipated runtime error (e.g. missing module, I/O error | ||
| // outside the guarded paths) is caught here and surfaced as a parse_error warning | ||
| // (in warn mode) or failure (in strict mode). This is a defence-in-depth measure | ||
| // complementing the continue-on-error: true that is set on the parse step in warn mode. | ||
| try { | ||
| await runMain(); | ||
| } catch (/** @type {any} */ unexpectedError) { | ||
| const errorMsg = getErrorMessage(unexpectedError); | ||
| core.error(`❌ Unexpected error in threat detection parse: ${errorMsg}`); | ||
| setDetectionFailure("unexpected_error", `${ERR_SYSTEM}: ❌ Unexpected error in threat detection parse: ${errorMsg}`); |
| if continueOnError { | ||
| steps = append(steps, " continue-on-error: true\n") | ||
| } | ||
| steps = append(steps, []string{ | ||
| fmt.Sprintf(" uses: %s\n", getCachedActionPin("actions/github-script", data)), | ||
| " env:\n", | ||
| " RUN_DETECTION: ${{ steps.detection_guard.outputs.run_detection }}\n", | ||
| fmt.Sprintf(" GH_AW_DETECTION_CONTINUE_ON_ERROR: %q\n", strconv.FormatBool(continueOnError)), | ||
| " with:\n", | ||
| " script: |\n", | ||
| } | ||
| }...) |
…ence in warn mode Generated by Design Decision Gate — records the decision to add continue-on-error: true and a top-level try/catch as defence-in-depth for the threat detection parse step when operating in warn mode. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (126 new lines across AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: The draft captures the key design decision: using a two-layer defence-in-depth (YAML What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References: §25090276793
|
|
@copilot review all comments |
… reason Agent-Logs-Url: https://github.com/github/gh-aw/sessions/664ef711-94da-4e97-8b40-5b4673e4ce0c Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed all three reviewer comments in ad02b9e:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
The
detectionjob was failing (conclusion=failure) when AWF's squid container was unhealthy or the model produced noTHREAT_DETECTION_RESULTtoken, blockingsafe_outputsvia theneeds.detection.result == 'success'gate and marking the entire workflow run as failed.Changes
pkg/workflow/threat_detection.go—buildDetectionConclusionStepnow emitscontinue-on-error: trueon the "Parse and conclude" step when the workflow is in warn mode (the default). In strict mode (continue-on-error: false) it is deliberately omitted so real threat detections still blocksafe_outputs.pkg/workflow/threat_detection.go(buildResultsParsingScriptRequire) — The generated github-script snippet now wraps therequire()/main()call in a try/catch. If the parse module fails to load entirely (e.g. missing file, brokensetup_globals.cjs), fallback outputs are set (reason=parse_error,conclusion=warning/failure,success=false) sodetection_*outputs are never left empty.actions/setup/js/parse_threat_detection_results.cjs— Added a top-leveltry/catchwrapping the innerrunMain()logic. Any unanticipated runtime exception routes throughsetDetectionFailure()withreason=parse_error(aligned with the documented reason contract inmessages_run_status.cjs) instead of crashing the step without setting outputs. This complements the YAML-levelcontinue-on-errorand the generated-script try/catch.Lock files — All 204 compiled
.lock.ymlfiles recompiled to apply all fixes.pkg/workflow/detection_success_test.go— NewTestDetectionConclusionStepContinueOnErrortable-driven test asserts the warn-mode and strict-mode behavior.