Address copilot review comments

Signed-off-by: Aravindhan Ayyanathan <aravindhan.a@est.tech>

Author: aravindtga

e2e/testdata/fn-render/exec-without-permissions/.expected/diff.patch

@@ -1,5 +1,5 @@
 diff --git a/Kptfile b/Kptfile
-index 0d98dbb..885a1d0 100644
+index 0d98dbb..a70d5e2 100644
 --- a/Kptfile
 +++ b/Kptfile
 @@ -4,4 +4,18 @@ metadata:
@@ -20,5 +20,5 @@ index 0d98dbb..885a1d0 100644
 +    mutationSteps:
 +      - exec: sed -e 's/foo/bar/'
 +        executionError: must run with `--allow-exec` option to allow running function binaries
-+        exitCode: 0
++        exitCode: 1
 +    errorSummary: 'sed -e ''s/foo/bar/'': must run with `--allow-exec` option to allow running function binaries'

internal/util/render/executor.go

@@ -246,7 +246,14 @@ func buildRenderStatus(hctx *hydrationContext, hydErr error) *kptfilev1.RenderSt
 	}
 	if hydErr != nil {
 		var errLines []string
-		for _, s := range append(hctx.mutationSteps, hctx.validationSteps...) {
+		for _, s := range hctx.mutationSteps {
+			if s.ExecutionError != "" {
+				errLines = append(errLines, fmt.Sprintf("%s: %s", stepName(s), s.ExecutionError))
+			} else if s.ExitCode != 0 {
+				errLines = append(errLines, fmt.Sprintf("%s: exit code %d", stepName(s), s.ExitCode))
+			}
+		}
+		for _, s := range hctx.validationSteps {
 			if s.ExecutionError != "" {
 				errLines = append(errLines, fmt.Sprintf("%s: %s", stepName(s), s.ExecutionError))
 			} else if s.ExitCode != 0 {
@@ -286,7 +293,7 @@ func setRenderStatus(fs filesys.FileSystem, pkgPath string, condition kptfilev1.
 	kf.Status.Conditions = append(kf.Status.Conditions, condition)
 	kf.Status.RenderStatus = renderStatus
 	if err := kptfileutil.WriteKptfileToFS(fs, pkgPath, kf); err != nil {
-		klog.V(3).Infof("failed to write render status condition to Kptfile at %s: %v", pkgPath, err)
+		klog.V(3).Infof("failed to write render status to Kptfile at %s: %v", pkgPath, err)
 	}
 }
 
@@ -743,10 +750,10 @@ func (pn *pkgNode) runMutators(ctx context.Context, hctx *hydrationContext, inpu
 		return input, nil
 	}
 
-	mutators, err := fnChain(ctx, hctx, pn.pkg.UniquePath, pl.Mutators)
+	mutators, failIdx, err := fnChain(ctx, hctx, pn.pkg.UniquePath, pl.Mutators)
 	if err != nil {
 		// Capture execution error (e.g. missing exec, image resolution failure)
-		hctx.mutationSteps = append(hctx.mutationSteps, executionErrorStep(pl.Mutators, err))
+		hctx.mutationSteps = append(hctx.mutationSteps, preExecFailureStep(pl.Mutators[failIdx], err))
 		return nil, err
 	}
 
@@ -802,11 +809,11 @@ func (pn *pkgNode) runMutators(ctx context.Context, hctx *hydrationContext, inpu
 		err = mutation.Execute()
 		if err != nil {
 			clearAnnotationsOnMutFailure(input)
-			hctx.mutationSteps = append(hctx.mutationSteps, captureStepResult(pl.Mutators[i], hctx.fnResults, prevLen))
+			hctx.mutationSteps = append(hctx.mutationSteps, captureStepResult(pl.Mutators[i], hctx.fnResults, prevLen, err))
 			return input, err
 		}
 		hctx.executedFunctionCnt++
-		hctx.mutationSteps = append(hctx.mutationSteps, captureStepResult(pl.Mutators[i], hctx.fnResults, prevLen))
+		hctx.mutationSteps = append(hctx.mutationSteps, captureStepResult(pl.Mutators[i], hctx.fnResults, prevLen, nil))
 
 		if len(selectors) > 0 || len(exclusions) > 0 {
 			// merge the output resources with input resources
@@ -852,23 +859,23 @@ func (pn *pkgNode) runValidators(ctx context.Context, hctx *hydrationContext, in
 			displayResourceCount = true
 		}
 		if function.Exec != "" && !hctx.runnerOptions.AllowExec {
-			hctx.validationSteps = append(hctx.validationSteps, executionErrorStep([]kptfilev1.Function{function}, errAllowedExecNotSpecified))
+			hctx.validationSteps = append(hctx.validationSteps, preExecFailureStep(function, errAllowedExecNotSpecified))
 			return errAllowedExecNotSpecified
 		}
 		opts := hctx.runnerOptions
 		opts.SetPkgPathAnnotation = true
 		opts.DisplayResourceCount = displayResourceCount
 		validator, err = fnruntime.NewRunner(ctx, hctx.fileSystem, &function, pn.pkg.UniquePath, hctx.fnResults, opts, hctx.runtime)
 		if err != nil {
-			hctx.validationSteps = append(hctx.validationSteps, executionErrorStep([]kptfilev1.Function{function}, err))
+			hctx.validationSteps = append(hctx.validationSteps, preExecFailureStep(function, err))
 			return err
 		}
 		if _, err = validator.Filter(cloneResources(selectedResources)); err != nil {
-			hctx.validationSteps = append(hctx.validationSteps, captureStepResult(function, hctx.fnResults, prevLen))
+			hctx.validationSteps = append(hctx.validationSteps, captureStepResult(function, hctx.fnResults, prevLen, err))
 			return err
 		}
 		hctx.executedFunctionCnt++
-		hctx.validationSteps = append(hctx.validationSteps, captureStepResult(function, hctx.fnResults, prevLen))
+		hctx.validationSteps = append(hctx.validationSteps, captureStepResult(function, hctx.fnResults, prevLen, nil))
 	}
 	return nil
 }
@@ -970,7 +977,7 @@ func pathRelToRoot(rootPkgPath, subPkgPath, resourcePath string) (relativePath s
 }
 
 // fnChain returns a slice of function runners given a list of functions defined in pipeline.
-func fnChain(ctx context.Context, hctx *hydrationContext, pkgPath types.UniquePath, fns []kptfilev1.Function) ([]*fnruntime.FunctionRunner, error) {
+func fnChain(ctx context.Context, hctx *hydrationContext, pkgPath types.UniquePath, fns []kptfilev1.Function) ([]*fnruntime.FunctionRunner, int, error) {
 	var runners []*fnruntime.FunctionRunner
 	for i := range fns {
 		var err error
@@ -981,18 +988,18 @@ func fnChain(ctx context.Context, hctx *hydrationContext, pkgPath types.UniquePa
 			displayResourceCount = true
 		}
 		if function.Exec != "" && !hctx.runnerOptions.AllowExec {
-			return nil, errAllowedExecNotSpecified
+			return nil, i, errAllowedExecNotSpecified
 		}
 		opts := hctx.runnerOptions
 		opts.SetPkgPathAnnotation = true
 		opts.DisplayResourceCount = displayResourceCount
 		runner, err = fnruntime.NewRunner(ctx, hctx.fileSystem, &function, pkgPath, hctx.fnResults, opts, hctx.runtime)
 		if err != nil {
-			return nil, err
+			return nil, i, err
 		}
 		runners = append(runners, runner)
 	}
-	return runners, nil
+	return runners, -1, nil
 }
 
 // trackInputFiles records file paths of input resources in the hydration context.
@@ -1041,7 +1048,7 @@ func pruneResources(fsys filesys.FileSystem, hctx *hydrationContext) error {
 
 // captureStepResult builds a PipelineStepResult from the fnresult.Result items
 // appended since prevLen.
-func captureStepResult(fn kptfilev1.Function, fnResults *fnresult.ResultList, prevLen int) kptfilev1.PipelineStepResult {
+func captureStepResult(fn kptfilev1.Function, fnResults *fnresult.ResultList, prevLen int, execErr error) kptfilev1.PipelineStepResult {
 	step := kptfilev1.PipelineStepResult{
 		Name:     fn.Name,
 		Image:    fn.Image,
@@ -1057,22 +1064,23 @@ func captureStepResult(fn kptfilev1.Function, fnResults *fnresult.ResultList, pr
 				step.ErrorResults = append(step.ErrorResults, ri)
 			}
 		}
+	} else if execErr != nil {
+		step.ExitCode = 1
+		step.ExecutionError = execErr.Error()
 	}
 	return step
 }
 
-// executionErrorStep creates a PipelineStepResult for errors that occur before
-// function execution (e.g. image pull failure, missing exec).
-func executionErrorStep(fns []kptfilev1.Function, err error) kptfilev1.PipelineStepResult {
-	if len(fns) == 0 {
-		return kptfilev1.PipelineStepResult{ExecutionError: err.Error()}
-	}
-	// Use the first function in the list that failed to resolve
-	fn := fns[0]
+// preExecFailureStep creates a PipelineStepResult for errors that occur before
+// the function is executed (e.g. image pull failure, missing exec permission).
+// ExitCode is set to 1 to indicate failure; the executionError field provides
+// the specific reason the function could not be started.
+func preExecFailureStep(fn kptfilev1.Function, err error) kptfilev1.PipelineStepResult {
 	return kptfilev1.PipelineStepResult{
 		Name:           fn.Name,
 		Image:          fn.Image,
 		ExecPath:       fn.Exec,
+		ExitCode:       1,
 		ExecutionError: err.Error(),
 	}
 }

internal/util/render/executor_test.go

@@ -808,7 +808,7 @@ func TestCaptureStepResult_FromFnResults(t *testing.T) {
 	})
 
 	fn := kptfilev1.Function{Name: "validate", Image: "gatekeeper:latest"}
-	step := captureStepResult(fn, fnResults, 0)
+	step := captureStepResult(fn, fnResults, 0, nil)
 
 	assert.Equal(t, "validate", step.Name)
 	assert.Equal(t, "gatekeeper:latest", step.Image)
@@ -839,30 +839,28 @@ func TestCaptureStepResult_FromFnResults(t *testing.T) {
 func TestCaptureStepResult_NoNewItems(t *testing.T) {
 	fnResults := fnresult.NewResultList()
 	fn := kptfilev1.Function{Image: "set-namespace:v1"}
-	step := captureStepResult(fn, fnResults, 0)
+	step := captureStepResult(fn, fnResults, 0, fmt.Errorf("output resource list must contain only KRM resources"))
 
 	assert.Equal(t, "set-namespace:v1", step.Image)
-	assert.Equal(t, 0, step.ExitCode)
-	assert.Empty(t, step.Stderr)
+	assert.Equal(t, 1, step.ExitCode)
+	assert.Equal(t, "output resource list must contain only KRM resources", step.ExecutionError)
 	assert.Nil(t, step.Results)
 	assert.Nil(t, step.ErrorResults)
 }
 
-func TestExecutionErrorStep(t *testing.T) {
-	fns := []kptfilev1.Function{
-		{Name: "my-fn", Image: "bad-image:v1", Exec: ""},
-	}
-	step := executionErrorStep(fns, fmt.Errorf("pull access denied"))
+func TestPreExecFailureStep(t *testing.T) {
+	fn := kptfilev1.Function{Name: "my-fn", Image: "bad-image:v1", Exec: ""}
+	step := preExecFailureStep(fn, fmt.Errorf("pull access denied"))
 
 	assert.Equal(t, "my-fn", step.Name)
 	assert.Equal(t, "bad-image:v1", step.Image)
 	assert.Equal(t, "pull access denied", step.ExecutionError)
-	assert.Equal(t, 0, step.ExitCode)
+	assert.Equal(t, 1, step.ExitCode)
 	assert.Nil(t, step.Results)
 }
 
-func TestExecutionErrorStep_EmptyFns(t *testing.T) {
-	step := executionErrorStep(nil, fmt.Errorf("no functions"))
+func TestPreExecFailureStep_EmptyFn(t *testing.T) {
+	step := preExecFailureStep(kptfilev1.Function{}, fmt.Errorf("no functions"))
 	assert.Empty(t, step.Name)
 	assert.Empty(t, step.Image)
 	assert.Empty(t, step.ExecPath)

pkg/test/runner/config.go

@@ -58,9 +58,9 @@ type TestCaseConfig struct {
 	// ActualStripLines is a list of lines that should be ignored on the actual function output
 	ActualStripLines []string `json:"actualStripLines,omitempty" yaml:"actualStripLines,omitempty"`
 
-	// DiffStripRegEx is a regular expression. Matching text is removed from
-	// both actual and expected diffs before comparison, making tests
-	// resilient to environment-specific output such as docker daemon errors.
+	// DiffStripRegEx is a regular expression. Lines matching this pattern are
+	// removed from both actual and expected diffs before comparison, making
+	// tests resilient to environment-specific output such as docker daemon errors.
 	DiffStripRegEx string `json:"diffStripRegEx,omitempty" yaml:"diffStripRegEx,omitempty"`
 
 	// StdErr is the expected standard error output and should be checked

pkg/test/runner/runner.go

@@ -480,8 +480,14 @@ func (r *Runner) compareResult(exitErr error, stdout string, inStderr string, tm
 	}
 	expectedDiff := expected.Diff
 	if r.testCase.Config.DiffStripRegEx != "" {
-		actualDiff = normalizeDiff(actualDiff, r.testCase.Config.DiffStripRegEx)
-		expectedDiff = normalizeDiff(expectedDiff, r.testCase.Config.DiffStripRegEx)
+		actualDiff, err = normalizeDiff(actualDiff, r.testCase.Config.DiffStripRegEx)
+		if err != nil {
+			return err
+		}
+		expectedDiff, err = normalizeDiff(expectedDiff, r.testCase.Config.DiffStripRegEx)
+		if err != nil {
+			return err
+		}
 	}
 	if actualDiff != expectedDiff {
 		diffOfDiff, err := diffStrings(actualDiff, expectedDiff)
@@ -644,8 +650,11 @@ func (r *Runner) stripLines(string2Strip string, linesToStrip []string) string {
 // normalizeDiff removes lines matching stripRegEx and normalizes index/hunk
 // headers in the diff string so that environment-specific output does not
 // cause comparison failures.
-func normalizeDiff(diff, stripRegEx string) string {
-	re := regexp.MustCompile(stripRegEx)
+func normalizeDiff(diff, stripRegEx string) (string, error) {
+	re, err := regexp.Compile(stripRegEx)
+	if err != nil {
+		return "", fmt.Errorf("unable to compile DiffStripRegEx %q: %w", stripRegEx, err)
+	}
 	indexRE := regexp.MustCompile(`^index [0-9a-f]+\.\.[0-9a-f]+`)
 	hunkRE := regexp.MustCompile(`^@@ -\d+,\d+ \+\d+,\d+ @@`)
 	var out []string
@@ -657,5 +666,5 @@ func normalizeDiff(diff, stripRegEx string) string {
 		line = hunkRE.ReplaceAllString(line, "@@ NORMALIZED @@")
 		out = append(out, line)
 	}
-	return strings.Join(out, "\n")
+	return strings.Join(out, "\n"), nil
 }