diff --git a/modules/actions/jobparser/interpeter.go b/modules/actions/jobparser/interpeter.go index 512b6f02ab..bdff039f91 100644 --- a/modules/actions/jobparser/interpeter.go +++ b/modules/actions/jobparser/interpeter.go @@ -33,6 +33,10 @@ func NewInterpeter( }, JobID: jobID, } + + // Add the current job to the workflow so run.Job() doesn't return nil + run.Workflow.Jobs[jobID] = job + for id, result := range results { need := yaml.Node{} _ = need.Encode(result.Needs) diff --git a/modules/actions/jobparser/jobparser.go b/modules/actions/jobparser/jobparser.go index 1d4c4c1756..f49b0057a6 100644 --- a/modules/actions/jobparser/jobparser.go +++ b/modules/actions/jobparser/jobparser.go @@ -14,6 +14,23 @@ import ( "go.yaml.in/yaml/v4" ) +// deepCopyYamlNode creates a deep copy of a yaml.Node to prevent mutations +// from affecting the original. This is important because yaml.Node.Content +// is a slice of pointers, and a shallow copy would share the same child nodes. +func deepCopyYamlNode(node *yaml.Node) *yaml.Node { + if node == nil { + return nil + } + nodeCopy := *node + if node.Content != nil { + nodeCopy.Content = make([]*yaml.Node, len(node.Content)) + for i, child := range node.Content { + nodeCopy.Content[i] = deepCopyYamlNode(child) + } + } + return &nodeCopy +} + func Parse(content []byte, options ...ParseOption) ([]*SingleWorkflow, error) { origin, err := model.ReadWorkflow(bytes.NewReader(content)) if err != nil { @@ -31,10 +48,11 @@ func Parse(content []byte, options ...ParseOption) ([]*SingleWorkflow, error) { } results := map[string]*JobResult{} for id, job := range origin.Jobs { + outputs := pc.jobOutputs[id] results[id] = &JobResult{ Needs: job.Needs(), Result: pc.jobResults[id], - Outputs: nil, // not supported yet + Outputs: outputs, } } @@ -49,7 +67,32 @@ func Parse(content []byte, options ...ParseOption) ([]*SingleWorkflow, error) { for i, id := range ids { job := jobs[i] - matricxes, err := getMatrixes(origin.GetJob(id)) + originJob := origin.GetJob(id) + + if originJob == nil { + return nil, fmt.Errorf("job %s not found in origin workflow", id) + } + + // Clone the origin job to avoid modifying the shared object + evaluatedJob := *originJob + if originJob.Strategy != nil { + stratCopy := *originJob.Strategy + // Deep copy the RawMatrix yaml.Node to prevent mutations from affecting the original + stratCopy.RawMatrix = *deepCopyYamlNode(&originJob.Strategy.RawMatrix) + evaluatedJob.Strategy = &stratCopy + } + + // Create an evaluator with access to needs/outputs for matrix evaluation + matrixEvaluator := NewExpressionEvaluator(NewInterpeter(id, &evaluatedJob, nil, pc.gitContext, results, pc.vars, pc.inputs)) + + // Evaluate the matrix before expanding it + if evaluatedJob.Strategy != nil && evaluatedJob.Strategy.RawMatrix.Kind != 0 { + if err := matrixEvaluator.EvaluateYamlNode(&evaluatedJob.Strategy.RawMatrix); err != nil { + return nil, fmt.Errorf("error evaluating matrix for job %s: %w", id, err) + } + } + + matricxes, err := getMatrixes(&evaluatedJob) if err != nil { return nil, fmt.Errorf("getMatrixes: %w", err) } @@ -59,9 +102,9 @@ func Parse(content []byte, options ...ParseOption) ([]*SingleWorkflow, error) { job.Name = id } job.Strategy.RawMatrix = encodeMatrix(matrix) - evaluator := NewExpressionEvaluator(NewInterpeter(id, origin.GetJob(id), matrix, pc.gitContext, results, pc.vars, pc.inputs)) + evaluator := NewExpressionEvaluator(NewInterpeter(id, &evaluatedJob, matrix, pc.gitContext, results, pc.vars, pc.inputs)) job.Name = nameWithMatrix(job.Name, matrix, evaluator) - runsOn := origin.GetJob(id).RunsOn() + runsOn := evaluatedJob.RunsOn() for i, v := range runsOn { runsOn[i] = evaluator.Interpolate(v) } @@ -89,6 +132,12 @@ func WithJobResults(results map[string]string) ParseOption { } } +func WithJobOutputs(outputs map[string]map[string]string) ParseOption { + return func(c *parseContext) { + c.jobOutputs = outputs + } +} + func WithGitContext(context *model.GithubContext) ParseOption { return func(c *parseContext) { c.gitContext = context @@ -109,6 +158,7 @@ func WithInputs(inputs map[string]any) ParseOption { type parseContext struct { jobResults map[string]string + jobOutputs map[string]map[string]string gitContext *model.GithubContext vars map[string]string inputs map[string]any diff --git a/modules/actions/jobparser/jobparser_test.go b/modules/actions/jobparser/jobparser_test.go index 51ba70fc2a..4dcdd78581 100644 --- a/modules/actions/jobparser/jobparser_test.go +++ b/modules/actions/jobparser/jobparser_test.go @@ -7,8 +7,11 @@ import ( "strings" "testing" + "github.com/nektos/act/pkg/exprparser" + "github.com/nektos/act/pkg/model" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.yaml.in/yaml/v4" ) @@ -53,6 +56,20 @@ func TestParse(t *testing.T) { options: nil, wantErr: false, }, + { + name: "job_name_with_matrix_dynamic", + options: []ParseOption{ + WithJobResults(map[string]string{ + "job1": "success", + }), + WithJobOutputs(map[string]map[string]string{ + "job1": { + "versions": "[1.17, 1.18, 1.19]", + }, + }), + }, + wantErr: false, + }, { name: "prefixed_newline", options: nil, @@ -85,3 +102,418 @@ func TestParse(t *testing.T) { }) } } + +func TestDeepCopyYamlNode(t *testing.T) { + t.Run("deep_copy_preserves_isolation", func(t *testing.T) { + // Create original node with nested content + original := &yaml.Node{ + Kind: yaml.MappingNode, + Tag: "!!map", + Value: "", + Content: []*yaml.Node{ + {Kind: yaml.ScalarNode, Tag: "!!str", Value: "key1"}, + {Kind: yaml.ScalarNode, Tag: "!!str", Value: "value1"}, + {Kind: yaml.ScalarNode, Tag: "!!str", Value: "key2"}, + {Kind: yaml.ScalarNode, Tag: "!!str", Value: "value2"}, + }, + } + + // Create deep copy + copied := deepCopyYamlNode(original) + + // Verify copy is not nil + require.NotNil(t, copied) + + // Verify values are equal + assert.Equal(t, original.Kind, copied.Kind) + assert.Equal(t, original.Tag, copied.Tag) + assert.Equal(t, len(original.Content), len(copied.Content)) + + // Verify content pointers are different (isolation) + for i, node := range original.Content { + assert.NotSame(t, node, copied.Content[i], "Content[%d] should be different pointers", i) + assert.Equal(t, node.Value, copied.Content[i].Value, "Content[%d] values should be equal", i) + } + + // Modify the copy and verify original is unaffected + copied.Content[0].Value = "modified" + assert.NotEqual(t, original.Content[0].Value, copied.Content[0].Value) + }) + + t.Run("deep_copy_handles_nil", func(t *testing.T) { + copied := deepCopyYamlNode(nil) + assert.Nil(t, copied) + }) + + t.Run("deep_copy_handles_recursive", func(t *testing.T) { + // Create a nested structure + original := &yaml.Node{ + Kind: yaml.MappingNode, + Tag: "!!map", + Value: "", + Content: []*yaml.Node{ + {Kind: yaml.ScalarNode, Tag: "!!str", Value: "nested"}, + { + Kind: yaml.MappingNode, + Tag: "!!map", + Value: "", + Content: []*yaml.Node{ + {Kind: yaml.ScalarNode, Tag: "!!str", Value: "inner"}, + {Kind: yaml.ScalarNode, Tag: "!!str", Value: "data"}, + }, + }, + }, + } + + copied := deepCopyYamlNode(original) + + // Verify deep isolation at all levels + require.NotNil(t, copied) + assert.NotSame(t, original.Content[1], copied.Content[1]) + assert.NotSame(t, original.Content[1].Content[0], copied.Content[1].Content[0]) + + // Modify nested copy and verify original is unaffected + copied.Content[1].Content[0].Value = "modified" + assert.NotEqual(t, original.Content[1].Content[0].Value, copied.Content[1].Content[0].Value) + }) +} + +func TestStrategyIsolationAfterEvaluation(t *testing.T) { + // This test verifies that EvaluateYamlNode mutations on a copied Strategy + // do not affect the original Strategy. This was the root cause of the issue. + t.Run("evaluation_does_not_mutate_original", func(t *testing.T) { + // Create an original job with a matrix + originalJob := &model.Job{ + Strategy: &model.Strategy{ + RawMatrix: yaml.Node{ + Kind: yaml.MappingNode, + Tag: "!!map", + Value: "", + Content: []*yaml.Node{ + {Kind: yaml.ScalarNode, Tag: "!!str", Value: "version"}, + { + Kind: yaml.SequenceNode, + Tag: "!!seq", + Value: "", + Content: []*yaml.Node{ + {Kind: yaml.ScalarNode, Tag: "!!str", Value: "${{ fromJson(needs.setup.outputs.versions) }}"}, + }, + }, + }, + }, + }, + } + + // Save the original Content pointer for verification + originalContentPtr := originalJob.Strategy.RawMatrix.Content[1].Content[0] + originalValue := originalContentPtr.Value + + // Simulate what happens in Parse(): shallow copy followed by evaluation + evaluatedJob := *originalJob + if originalJob.Strategy != nil { + stratCopy := *originalJob.Strategy + // This is the fix: deep copy the RawMatrix + stratCopy.RawMatrix = *deepCopyYamlNode(&originalJob.Strategy.RawMatrix) + evaluatedJob.Strategy = &stratCopy + } + + // Create an evaluator and evaluate the matrix + // (In real usage, this would have job outputs and other context) + evaluator := NewExpressionEvaluator(exprparser.NewInterpeter( + &exprparser.EvaluationEnvironment{ + Github: &model.GithubContext{}, + Vars: map[string]string{}, + Inputs: map[string]any{}, + }, + exprparser.Config{}, + )) + + // Evaluate the copied node + _ = evaluator.EvaluateYamlNode(&evaluatedJob.Strategy.RawMatrix) + + // Verify that the original job's matrix is unchanged + assert.Equal(t, originalValue, originalJob.Strategy.RawMatrix.Content[1].Content[0].Value, + "Original job's matrix should not be mutated by evaluation") + + // Verify that they are now different pointers (isolation) + assert.NotSame(t, originalJob.Strategy.RawMatrix.Content[1].Content[0], + evaluatedJob.Strategy.RawMatrix.Content[1].Content[0], + "Evaluated job should have different node pointers") + }) +} + +func TestParseWithMissingJobOutputs(t *testing.T) { + // Test graceful degradation when job outputs are missing + t.Run("missing_job_outputs_degrades_gracefully", func(t *testing.T) { + workflowYAML := ` +name: test-missing-outputs +on: push + +jobs: + setup: + runs-on: ubuntu-latest + strategy: + matrix: + version: [1.0, 2.0] + steps: + - run: echo setup + + build: + runs-on: ubuntu-latest + strategy: + matrix: + os: [ubuntu-latest, windows-latest] + steps: + - run: echo build +` + // Parse without providing job outputs - should gracefully handle + result, err := Parse([]byte(workflowYAML)) + + // Should not error on parse + assert.NoError(t, err) + assert.NotNil(t, result) + assert.Greater(t, len(result), 0) + }) + + t.Run("empty_job_outputs_map", func(t *testing.T) { + workflowYAML := ` +name: test-empty-outputs +on: push + +jobs: + setup: + runs-on: ubuntu-latest + steps: + - run: echo setup + + build: + needs: setup + runs-on: ubuntu-latest + strategy: + matrix: + version: [1.0, 2.0] + steps: + - run: echo build +` + // Parse with empty job outputs + result, err := Parse([]byte(workflowYAML), + WithJobOutputs(map[string]map[string]string{})) + + assert.NoError(t, err) + assert.NotNil(t, result) + + // Should still parse successfully + assert.Greater(t, len(result), 0) + }) +} + +func TestParseWithNeedsReferenceNoOutputs(t *testing.T) { + // Test references to jobs that have no outputs provided + t.Run("needs_reference_without_outputs", func(t *testing.T) { + workflowYAML := ` +name: test-needs-no-outputs +on: push + +jobs: + setup: + runs-on: ubuntu-latest + steps: + - run: echo setup + + build: + needs: setup + runs-on: ubuntu-latest + strategy: + matrix: + os: [ubuntu-latest, windows-latest] + steps: + - run: echo build +` + // Parse with a needs reference but static matrix only + result, err := Parse([]byte(workflowYAML), + WithJobResults(map[string]string{ + "setup": "success", + })) + + // Should not error on parse + assert.NoError(t, err) + assert.NotNil(t, result) + assert.Greater(t, len(result), 0) + }) + + t.Run("needs_reference_with_partial_outputs", func(t *testing.T) { + workflowYAML := ` +name: test-partial-outputs +on: push + +jobs: + setup: + runs-on: ubuntu-latest + outputs: + versions: "[1.0, 2.0]" + steps: + - run: echo setup + + build: + needs: setup + runs-on: ubuntu-latest + strategy: + matrix: + version: ${{ fromJson(needs.setup.outputs.versions) }} + os: [ubuntu-latest, windows-latest] + steps: + - run: echo build +` + // Parse with partial outputs provided + result, err := Parse([]byte(workflowYAML), + WithJobOutputs(map[string]map[string]string{ + "setup": { + "versions": "[1.0, 2.0]", + }, + })) + + assert.NoError(t, err) + assert.NotNil(t, result) + + // Should parse successfully + assert.Greater(t, len(result), 0) + }) +} + +func TestParseWithMixedMatrixValues(t *testing.T) { + // Test matrix with both static arrays and dynamic template expressions + t.Run("static_and_dynamic_matrix_values", func(t *testing.T) { + workflowYAML := ` +name: test-mixed-matrix +on: push + +jobs: + setup: + runs-on: ubuntu-latest + outputs: + versions: "[1.0, 2.0]" + steps: + - run: echo setup + + build: + needs: setup + runs-on: ubuntu-latest + strategy: + matrix: + os: [ubuntu-latest, windows-latest, macos-latest] + version: ${{ fromJson(needs.setup.outputs.versions) }} + node: [14, 16, 18] + steps: + - run: echo build +` + // Parse with dynamic matrix values + result, err := Parse([]byte(workflowYAML), + WithJobOutputs(map[string]map[string]string{ + "setup": { + "versions": "[1.0, 2.0]", + }, + })) + + assert.NoError(t, err) + assert.NotNil(t, result) + + // Verify we have workflows + assert.Greater(t, len(result), 0) + + // Check that all three matrix dimensions are present + hasAllDimensions := false + for _, workflow := range result { + id, swfJob := workflow.Job() + if id == "build" { + // In jobparser, we just verify the job was parsed successfully + if swfJob != nil { + // Check strategy has matrix + if swfJob.Strategy.RawMatrix.Kind != 0 { + // All three dimensions should be defined + hasAllDimensions = true + } + } + break + } + } + + assert.True(t, hasAllDimensions, "should have all matrix dimensions") + }) + + t.Run("multiple_dynamic_matrix_values", func(t *testing.T) { + workflowYAML := ` +name: test-multiple-dynamic +on: push + +jobs: + setup: + runs-on: ubuntu-latest + outputs: + versions: "[1.0, 2.0]" + platforms: "[\"linux\", \"darwin\"]" + steps: + - run: echo setup + + build: + needs: setup + runs-on: ubuntu-latest + strategy: + matrix: + version: ${{ fromJson(needs.setup.outputs.versions) }} + platform: ${{ fromJson(needs.setup.outputs.platforms) }} + static: [a, b] + steps: + - run: echo build +` + // Parse with multiple dynamic values + result, err := Parse([]byte(workflowYAML), + WithJobOutputs(map[string]map[string]string{ + "setup": { + "versions": "[1.0, 2.0]", + "platforms": "[\"linux\", \"darwin\"]", + }, + })) + + assert.NoError(t, err) + assert.NotNil(t, result) + assert.Greater(t, len(result), 0) + }) + + t.Run("all_static_arrays_no_dynamic", func(t *testing.T) { + workflowYAML := ` +name: test-all-static +on: push + +jobs: + build: + runs-on: ubuntu-latest + strategy: + matrix: + os: [ubuntu-latest, windows-latest] + version: [1.18, 1.19, 1.20] + node: [14, 16] + steps: + - run: echo build +` + // Parse with all static arrays, no dynamic values + result, err := Parse([]byte(workflowYAML)) + + assert.NoError(t, err) + assert.NotNil(t, result) + + // Should expand correctly + // 2 os * 3 versions * 2 node = 12 combinations + assert.Greater(t, len(result), 0) + + // Verify matrix structure + for _, workflow := range result { + id, swfJob := workflow.Job() + if id == "build" { + // Verify the job was parsed with a matrix strategy + assert.NotNil(t, swfJob) + assert.NotEqual(t, 0, swfJob.Strategy.RawMatrix.Kind) + break + } + } + }) +} diff --git a/modules/actions/jobparser/testdata/job_name_with_matrix_dynamic.in.yaml b/modules/actions/jobparser/testdata/job_name_with_matrix_dynamic.in.yaml new file mode 100644 index 0000000000..8d9efb9c59 --- /dev/null +++ b/modules/actions/jobparser/testdata/job_name_with_matrix_dynamic.in.yaml @@ -0,0 +1,24 @@ +name: test +jobs: + job1: + runs-on: ubuntu-latest + outputs: + versions: ${{ steps.version-map.outputs.versions }} + steps: + - name: Generate the version map + id: version-map + run: | + echo "versions=[1.17, 1.18, 1.19]" >> $GITHUB_OUTPUT + job2: + needs: job1 + strategy: + matrix: + version: ${{ fromJSON(needs.job1.outputs.versions) }} + os: ubuntu-24.04 + runs-on: ${{ matrix.os }} + name: test_version_${{ matrix.version }}_on_${{ matrix.os }} + steps: + - uses: actions/setup-go@v3 + with: + go-version: ${{ matrix.version }} + - run: uname -a && go version diff --git a/modules/actions/jobparser/testdata/job_name_with_matrix_dynamic.out.yaml b/modules/actions/jobparser/testdata/job_name_with_matrix_dynamic.out.yaml new file mode 100644 index 0000000000..58ea9535f5 --- /dev/null +++ b/modules/actions/jobparser/testdata/job_name_with_matrix_dynamic.out.yaml @@ -0,0 +1,66 @@ +name: test +jobs: + job1: + name: job1 + runs-on: ubuntu-latest + steps: + - id: version-map + name: Generate the version map + run: | + echo "versions=[1.17, 1.18, 1.19]" >> $GITHUB_OUTPUT + outputs: + versions: ${{ steps.version-map.outputs.versions }} +--- +name: test +jobs: + job2: + name: test_version_1.17_on_ubuntu-24.04 + needs: job1 + runs-on: ubuntu-24.04 + steps: + - uses: actions/setup-go@v3 + with: + go-version: ${{ matrix.version }} + - run: uname -a && go version + strategy: + matrix: + os: + - ubuntu-24.04 + version: + - 1.17 +--- +name: test +jobs: + job2: + name: test_version_1.18_on_ubuntu-24.04 + needs: job1 + runs-on: ubuntu-24.04 + steps: + - uses: actions/setup-go@v3 + with: + go-version: ${{ matrix.version }} + - run: uname -a && go version + strategy: + matrix: + os: + - ubuntu-24.04 + version: + - 1.18 +--- +name: test +jobs: + job2: + name: test_version_1.19_on_ubuntu-24.04 + needs: job1 + runs-on: ubuntu-24.04 + steps: + - uses: actions/setup-go@v3 + with: + go-version: ${{ matrix.version }} + - run: uname -a && go version + strategy: + matrix: + os: + - ubuntu-24.04 + version: + - 1.19 diff --git a/services/actions/matrix.go b/services/actions/matrix.go index ef9577dec6..448b51c972 100644 --- a/services/actions/matrix.go +++ b/services/actions/matrix.go @@ -14,7 +14,7 @@ import ( actions_model "code.gitea.io/gitea/models/actions" "code.gitea.io/gitea/modules/log" - "github.com/nektos/act/pkg/jobparser" + "code.gitea.io/gitea/modules/actions/jobparser" "go.yaml.in/yaml/v4" ) diff --git a/services/actions/matrix_metrics.go b/services/actions/matrix_metrics.go index fab39f8bd7..7c9c120e56 100644 --- a/services/actions/matrix_metrics.go +++ b/services/actions/matrix_metrics.go @@ -34,6 +34,7 @@ var ( matrixMetricsInstance *MatrixMetrics metricsOnce sync.Once ) + // GetMatrixMetrics returns the global matrix metrics instance func GetMatrixMetrics() *MatrixMetrics { metricsOnce.Do(func() { diff --git a/services/actions/run.go b/services/actions/run.go index 70b4c425cd..b2c8770951 100644 --- a/services/actions/run.go +++ b/services/actions/run.go @@ -9,8 +9,8 @@ import ( actions_model "code.gitea.io/gitea/models/actions" "code.gitea.io/gitea/models/db" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/actions/jobparser" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/util" notify_service "code.gitea.io/gitea/services/notify"