Skip to content

Fix compiler to only generate task job when required for checks#803

Merged
pelikhan merged 3 commits intomainfrom
copilot/fix-39356bd3-56e5-4bd1-ae4f-c0e26b616db0
Sep 12, 2025
Merged

Fix compiler to only generate task job when required for checks#803
pelikhan merged 3 commits intomainfrom
copilot/fix-39356bd3-56e5-4bd1-ae4f-c0e26b616db0

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Sep 12, 2025

Problem

The compiler was generating task jobs redundantly due to duplicate permission checks in two different places:

  1. isTaskJobNeeded() called needsPermissionChecks() internally
  2. buildJobs() also called needsPermissionChecksWithFrontmatter() separately
  3. The task job was created if either condition returned true: if c.isTaskJobNeeded(data) || needsPermissionCheck

This meant permission checks were effectively being evaluated twice, and the task job might be generated when it shouldn't be according to the consolidated logic.

Solution

Consolidated permission check logic by modifying isTaskJobNeeded() to accept the pre-calculated needsPermissionCheck parameter instead of calculating it internally:

// Before: Called needsPermissionChecks() internally
func (c *Compiler) isTaskJobNeeded(data *WorkflowData) bool {
    return data.Command != "" || data.NeedsTextOutput || data.If != "" || c.needsPermissionChecks(data)
}

// After: Accepts pre-calculated permission check result
func (c *Compiler) isTaskJobNeeded(data *WorkflowData, needsPermissionCheck bool) bool {
    return data.Command != "" || data.NeedsTextOutput || data.If != "" || needsPermissionCheck
}

Simplified the condition in buildJobs() from:

if c.isTaskJobNeeded(data) || needsPermissionCheck {

To:

if c.isTaskJobNeeded(data, needsPermissionCheck) {

Impact

The task job is now only generated when actually required:

  • Safe workflows with workflow_dispatch + roles: allNo task job generated
  • Unsafe workflows with push events → Task job generated with permission checks
  • Conditional workflows with if conditions → Task job generated for runtime barriers

This reduces workflow complexity and execution time for safe workflows that don't need permission checks.

Testing

Added comprehensive test TestTaskJobGenerationFix that validates all scenarios, and verified all existing workflows still compile correctly with make recompile.

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:

  • api.contoso.com
    • Triggering command: /tmp/go-build406408965/b278/cli.test -test.testlogfile=/tmp/go-build406408965/b278/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true (dns block)
    • Triggering command: /tmp/go-build4022932314/b278/cli.test -test.testlogfile=/tmp/go-build4022932314/b278/testlog.txt -test.paniconexit0 -test.timeout=10m0s (dns block)
    • Triggering command: /tmp/go-build3493872628/b278/cli.test -test.testlogfile=/tmp/go-build3493872628/b278/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true (dns block)

If you need me to access, download, or install something from one of these locations, you can either:


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix compiler such that the "task" job is only generated if it is required (for checks). Otherwise do not generate code for it. Fix compiler to only generate task job when required for checks Sep 12, 2025
Copilot AI requested a review from pelikhan September 12, 2025 21:56
@pelikhan pelikhan marked this pull request as ready for review September 12, 2025 22:18
@pelikhan pelikhan merged commit 54758dc into main Sep 12, 2025
13 of 15 checks passed
@pelikhan pelikhan deleted the copilot/fix-39356bd3-56e5-4bd1-ae4f-c0e26b616db0 branch September 12, 2025 22:18
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.

2 participants