Skip to content

Skip safe output jobs when threat detection fails#3840

Merged
pelikhan merged 3 commits into
mainfrom
copilot/update-compiler-safe-output-jobs
Nov 13, 2025
Merged

Skip safe output jobs when threat detection fails#3840
pelikhan merged 3 commits into
mainfrom
copilot/update-compiler-safe-output-jobs

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Nov 13, 2025

Update compiler to skip safe output jobs when detection job fails ✅

Problem

When the detection job fails (due to threat detection), safe output jobs still run because they only check if the agent job completed, not if detection succeeded.

Reference: https://github.com/githubnext/gh-aw/actions/runs/19301864310

Solution Implemented

1. Detection Job Success Output ✅

File: pkg/workflow/threat_detection.go

  • Added id: parse_results to the "Parse threat detection results" step
  • Added core.setOutput('success', 'true') when no threats detected
  • Added core.setOutput('success', 'false') before core.setFailed() when threats detected
  • Added Outputs field to detection job: success: ${{ steps.parse_results.outputs.success }}

2. Safe Output Job Conditions ✅

Files: pkg/workflow/compiler_jobs.go, pkg/workflow/expressions.go

  • Created helper function AddDetectionSuccessCheck() to add detection success check to job conditions
  • Updated all safe output job builders to check needs.detection.outputs.success == 'true'
  • Jobs affected:
    • create_issue
    • create_discussion
    • create_pull_request
    • add_comment
    • create_pr_review_comment
    • create_code_scanning_alert
    • add_labels
    • update_issue
    • push_to_pull_request_branch
    • missing_tool
    • upload_assets
    • create_agent_task

3. Update_reaction Job ✅

File: pkg/workflow/notify_comment.go

  • No changes needed - already uses always() condition
  • Does not check detection success (intentional)
  • Will run even when detection fails to update reaction emoji

4. Tests Added ✅

File: pkg/workflow/detection_success_test.go

  • TestDetectionJobHasSuccessOutput - Verifies detection job sets success output
  • TestSafeOutputJobsCheckDetectionSuccess - Verifies safe output jobs check detection success

Verification

Compiled Workflow Example

detection:
  needs: agent
  if: needs.agent.outputs.output_types != '' || needs.agent.outputs.has_patch == 'true'
  runs-on: ubuntu-latest
  outputs:
    success: ${{ steps.parse_results.outputs.success }}
  steps:
    # ... other steps ...
    - name: Parse threat detection results
      id: parse_results
      uses: actions/github-script@...
      with:
        script: |
          # ... parsing logic ...
          if (threats_detected) {
            core.setOutput('success', 'false');
            core.setFailed('❌ Security threats detected...');
          } else {
            core.info('✅ No security threats detected...');
            core.setOutput('success', 'true');
          }

create_issue:
  needs:
    - agent
    - detection
  if: >
    (((!cancelled()) && (needs.agent.result != 'skipped')) && 
    (contains(needs.agent.outputs.output_types, 'create_issue'))) &&
    (needs.detection.outputs.success == 'true')
  # ... rest of job ...

Test Results

✅ All 340+ unit tests pass
✅ New tests verify detection success output
✅ Existing workflows recompile successfully
✅ Detection job has success output
✅ Safe output jobs check detection success
✅ Code formatted and linted successfully

Behavior

When detection succeeds:

  • Detection job: ✅ SUCCESS (sets success='true')
  • Safe output jobs: ✅ RUN (condition met)
  • Update_reaction: ⏭️ SKIPPED (no update needed)

When detection fails:

  • Detection job: ❌ FAILED (sets success='false')
  • Safe output jobs: ⏭️ SKIPPED (condition not met)
  • Update_reaction: ✅ RUNS (always() condition)
Original prompt

Update the compiler such that safe output jobs are skipped when the detection job has a failure.

detection should have a final step that sets a success output and this output should be checked in the "if" of safe outputs.

See run where detection failed and create issue was run https://github.com/githubnext/gh-aw/actions/runs/19301864310

update_reaction should still run


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

- Detection job now sets a success output (true/false) in parse_results step
- Safe output jobs check needs.detection.outputs.success == 'true' before running
- Update_reaction job still runs with always() condition (not affected by detection failure)
- Added tests to verify the new behavior

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Update compiler to skip safe outputs on detection failure Skip safe output jobs when threat detection fails Nov 13, 2025
Copilot AI requested a review from pelikhan November 13, 2025 11:17
@pelikhan pelikhan marked this pull request as ready for review November 13, 2025 11:18
Copilot AI review requested due to automatic review settings November 13, 2025 11:18
@pelikhan
Copy link
Copy Markdown
Collaborator

@copilot lint

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a critical security vulnerability where safe output jobs were executing even when the threat detection job failed, allowing potentially malicious outputs to be created. The fix ensures that safe output jobs only run when threat detection explicitly passes by checking a success output from the detection job.

Key changes:

  • Detection job now outputs a success field that is set to 'true' or 'false' based on threat verdict
  • All safe output jobs check needs.detection.outputs.success == 'true' before executing
  • Helper function AddDetectionSuccessCheck() consistently applies the check across all safe output types
  • The update_reaction job preserves its always() behavior to update emoji regardless of detection status

Reviewed Changes

Copilot reviewed 71 out of 71 changed files in this pull request and generated no comments.

Show a summary per file
File Description
pkg/workflow/threat_detection.go Adds success output to detection job and sets it in the parsing step based on threat verdict
pkg/workflow/expressions.go Implements AddDetectionSuccessCheck() helper to consistently add detection success checks to job conditions
pkg/workflow/compiler_jobs.go Applies detection success check to all 12 safe output job types when threat detection is enabled
pkg/workflow/detection_success_test.go Adds test coverage verifying detection job has success output and safe output jobs check it
.github/workflows/*.lock.yml Auto-generated workflow files reflecting the compiler changes (consistent application across all workflows)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Nov 13, 2025

@copilot lint

Linting completed successfully. Fixed code formatting issues in commit 603d5bc.

@pelikhan pelikhan merged commit 24db3fb into main Nov 13, 2025
41 checks passed
@pelikhan pelikhan deleted the copilot/update-compiler-safe-output-jobs branch November 13, 2025 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants