feat: report invalid CEL expression errors as PR comments

This commit introduces a feature that automatically reports
CEL expression validation errors directly on pull requests
as comments, providing immediate feedback to users about
what went wrong with their pipeline run configurations.

Changes:
- Add CEL validation error collection and reporting mechanism
- Display validation errors in markdown table format on PR comments
- Implement error message sanitization for proper markdown rendering
- Enhance error formatting to escape special characters and remove line breaks
- Streamline error notification flow to reduce duplicate messages

https://issues.redhat.com/browse/SRVKP-7308

Signed-off-by: Zaki Shaikh <[email protected]>

Author: zakisk

pkg/matcher/annotation_matcher.go

@@ -6,17 +6,19 @@ import (
 	"regexp"
 	"strings"
 
-	"github.com/gobwas/glob"
-	"github.com/google/cel-go/common/types"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
 	apipac "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
+	pacerrors "github.com/openshift-pipelines/pipelines-as-code/pkg/errors"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/events"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
+
+	"github.com/gobwas/glob"
+	"github.com/google/cel-go/common/types"
 	tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
 	"go.uber.org/zap"
 )
@@ -205,6 +207,7 @@ func MatchPipelinerunByAnnotation(ctx context.Context, logger *zap.SugaredLogger
 	}
 	logger.Info(infomsg)
 
+	celValidationErrors := []*pacerrors.PacYamlValidations{}
 	for _, prun := range pruns {
 		prMatch := Match{
 			PipelineRun: prun,
@@ -277,6 +280,12 @@ func MatchPipelinerunByAnnotation(ctx context.Context, logger *zap.SugaredLogger
 			out, err := celEvaluate(ctx, celExpr, event, vcx)
 			if err != nil {
 				logger.Errorf("there was an error evaluating the CEL expression, skipping: %v", err)
+				if checkIfCELEvaluateError(err) {
+					celValidationErrors = append(celValidationErrors, &pacerrors.PacYamlValidations{
+						Name: prName,
+						Err:  fmt.Errorf("CEL expression evaluation error: %s", sanitizeErrorAsMarkdown(err)),
+					})
+				}
 				continue
 			}
 			if out != types.True {
@@ -352,6 +361,10 @@ func MatchPipelinerunByAnnotation(ctx context.Context, logger *zap.SugaredLogger
 		matchedPRs = append(matchedPRs, prMatch)
 	}
 
+	if len(celValidationErrors) > 0 {
+		reportCELValidationErrors(ctx, repo, celValidationErrors, eventEmitter, vcx, event)
+	}
+
 	if len(matchedPRs) > 0 {
 		return matchedPRs, nil
 	}

pkg/matcher/annotation_matcher_test.go

@@ -1644,6 +1644,35 @@ func TestMatchPipelinerunByAnnotation(t *testing.T) {
 				`Warning: The PipelineRun 'pipeline-on-cel-test' has 'on-cel-expression' defined along with [on-event, on-target-branch] annotation(s). The 'on-cel-expression' will take precedence and these annotations will be ignored`,
 			},
 		},
+		{
+			name: "invalid-cel-expression-error",
+			args: args{
+				pruns: []*tektonv1.PipelineRun{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "pipeline-on-cel-test",
+							Annotations: map[string]string{
+								keys.OnEvent:         "[pull_request]",
+								keys.OnTargetBranch:  "[main]",
+								keys.OnCelExpression: `event == "pull_request" &`,
+							},
+						},
+					},
+				},
+				runevent: info.Event{
+					URL:               "https://hello/moto",
+					TriggerTarget:     "pull_request",
+					EventType:         "pull_request",
+					BaseBranch:        "main",
+					HeadBranch:        "warn-for-cel",
+					PullRequestNumber: 10,
+					Request: &info.Request{
+						Header: http.Header{},
+					},
+				},
+			},
+			wantErr: true,
+		},
 		{
 			name: "no-match-on-label",
 			args: args{

pkg/matcher/errors.go

@@ -0,0 +1,74 @@
+package matcher
+
+import (
+	"context"
+	"fmt"
+	"strings"
+
+	apipac "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
+	pacerrors "github.com/openshift-pipelines/pipelines-as-code/pkg/errors"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/events"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
+
+	"go.uber.org/zap"
+)
+
+// checkCELEvaluateError checks if error is from CEL evaluation stages.
+func checkIfCELEvaluateError(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	errMsg := err.Error()
+
+	patterns := []string{
+		`failed to parse expression`,
+		`check failed`,
+		`failed to create a Program`,
+		`failed to evaluate`,
+	}
+
+	for _, pattern := range patterns {
+		if strings.Contains(errMsg, pattern) {
+			return true
+		}
+	}
+
+	return false
+}
+
+func reportCELValidationErrors(ctx context.Context, repo *apipac.Repository, validationErrors []*pacerrors.PacYamlValidations, eventEmitter *events.EventEmitter, vcx provider.Interface, event *info.Event) {
+	errorRows := make([]string, 0, len(validationErrors))
+	for _, err := range validationErrors {
+		errorRows = append(errorRows, fmt.Sprintf("| %s | `%s` |", err.Name, err.Err.Error()))
+	}
+	if len(errorRows) == 0 {
+		return
+	}
+	markdownErrMessage := fmt.Sprintf(`%s
+%s`, provider.ValidationErrorTemplate, strings.Join(errorRows, "\n"))
+	if err := vcx.CreateComment(ctx, event, markdownErrMessage, provider.ValidationErrorTemplate); err != nil {
+		eventEmitter.EmitMessage(repo, zap.ErrorLevel, "PipelineRunCommentCreationError",
+			fmt.Sprintf("failed to create comment: %s", err.Error()))
+	}
+}
+
+// sanitizeErrorAsMarkdown prepares a CEL evaluation error string to be rendered
+// inside a GitHub / GitLab markdown table without breaking its layout.
+//
+// Markdown tables use the vertical bar character (`|`) as a column delimiter. If
+// the original error message contains an un-escaped pipe the markdown renderer
+// interprets it as the start of a new column or row which distorts the table
+// produced by Pipelines-as-Code when reporting validation errors.
+//
+// To avoid this we escape every pipe with a backslash (\|). We also replace any
+// newline or carriage-return characters with a single space so that the whole
+// error is kept on one row, preserving readability in the rendered comment.
+func sanitizeErrorAsMarkdown(err error) string {
+	errStr := err.Error()
+	errStr = strings.ReplaceAll(errStr, "|", "\\|")
+	errStr = strings.ReplaceAll(errStr, "\n", " ")
+	errStr = strings.ReplaceAll(errStr, "\r", " ")
+	return errStr
+}

pkg/pipelineascode/errors.go

@@ -14,17 +14,12 @@ import (
 )
 
 const (
-	validationErrorTemplate = `> [!CAUTION]
-> There are some errors in your PipelineRun template.
-
-| PipelineRun | Error |
-|------|-------|`
 	tektonDirMissingError = ".tekton/ directory doesn't exist in repository's root directory"
 )
 
 var regexpIgnoreErrors = regexp.MustCompile(`.*no kind.*is registered for version.*in scheme.*`)
 
-func (p *PacRun) checkAccessOrErrror(ctx context.Context, repo *v1alpha1.Repository, status provider.StatusOpts, viamsg string) (bool, error) {
+func (p *PacRun) checkAccessOrError(ctx context.Context, repo *v1alpha1.Repository, status provider.StatusOpts, viamsg string) (bool, error) {
 	allowed, err := p.vcx.IsAllowed(ctx, p.event)
 	if err != nil {
 		return false, fmt.Errorf("unable to verify event authorization: %w", err)
@@ -64,8 +59,8 @@ func (p *PacRun) reportValidationErrors(ctx context.Context, repo *v1alpha1.Repo
 		return
 	}
 	markdownErrMessage := fmt.Sprintf(`%s
-%s`, validationErrorTemplate, strings.Join(errorRows, "\n"))
-	if err := p.vcx.CreateComment(ctx, p.event, markdownErrMessage, validationErrorTemplate); err != nil {
+%s`, provider.ValidationErrorTemplate, strings.Join(errorRows, "\n"))
+	if err := p.vcx.CreateComment(ctx, p.event, markdownErrMessage, provider.ValidationErrorTemplate); err != nil {
 		p.eventEmitter.EmitMessage(repo, zap.ErrorLevel, "PipelineRunCommentCreationError",
 			fmt.Sprintf("failed to create comment: %s", err.Error()))
 	}

pkg/pipelineascode/errors_test.go

@@ -95,7 +95,7 @@ func TestCheckAccessOrErrror(t *testing.T) {
 			// Call the function
 			repo := &v1alpha1.Repository{}
 			status := provider.StatusOpts{}
-			allowed, err := p.checkAccessOrErrror(context.Background(), repo, status, "via test")
+			allowed, err := p.checkAccessOrError(context.Background(), repo, status, "via test")
 
 			// Verify results
 			if tt.expectedErr {

pkg/pipelineascode/match.go

@@ -144,7 +144,7 @@ is that what you want? make sure you use -n when generating the secret, eg: echo
 			DetailsURL:   p.event.URL,
 			AccessDenied: true,
 		}
-		if allowed, err := p.checkAccessOrErrror(ctx, repo, status, "by GitOps comment on push commit"); !allowed {
+		if allowed, err := p.checkAccessOrError(ctx, repo, status, "by GitOps comment on push commit"); !allowed {
 			return nil, err
 		}
 	}
@@ -160,7 +160,7 @@ is that what you want? make sure you use -n when generating the secret, eg: echo
 			DetailsURL:   p.event.URL,
 			AccessDenied: true,
 		}
-		if allowed, err := p.checkAccessOrErrror(ctx, repo, status, "via "+p.event.TriggerTarget.String()); !allowed {
+		if allowed, err := p.checkAccessOrError(ctx, repo, status, "via "+p.event.TriggerTarget.String()); !allowed {
 			return nil, err
 		}
 	}
@@ -307,7 +307,7 @@ func (p *PacRun) getPipelineRunsFromRepo(ctx context.Context, repo *v1alpha1.Rep
 			DetailsURL:   p.event.URL,
 			AccessDenied: true,
 		}
-		if allowed, err := p.checkAccessOrErrror(ctx, repo, status, "by GitOps comment on push commit"); !allowed {
+		if allowed, err := p.checkAccessOrError(ctx, repo, status, "by GitOps comment on push commit"); !allowed {
 			return nil, err
 		}
 	}

pkg/provider/provider.go

@@ -11,6 +11,12 @@ import (
 	"gopkg.in/yaml.v2"
 )
 
+const ValidationErrorTemplate = `> [!CAUTION]
+> There are some errors in your PipelineRun template.
+
+| PipelineRun | Error |
+|------|-------|`
+
 var (
 	testRetestAllRegex    = regexp.MustCompile(`(?m)^(/retest|/test)\s*$`)
 	testRetestSingleRegex = regexp.MustCompile(`(?m)^(/test|/retest)[ \t]+\S+`)

test/github_pullrequest_test.go

@@ -193,6 +193,39 @@ func TestGithubSecondPullRequestBadYaml(t *testing.T) {
 	t.Fatal("No comments with the pipelinerun error found on the pull request")
 }
 
+func TestGithubInvalidCELExpressionReportingOnPR(t *testing.T) {
+	ctx := context.Background()
+	g := &tgithub.PRTest{
+		Label:         "Github PullRequest Invalid CEL expression",
+		YamlFiles:     []string{"testdata/failures/pipelinerun-invalid-cel.yaml"},
+		NoStatusCheck: true,
+	}
+	g.RunPullRequest(ctx, t)
+	defer g.TearDown(ctx, t)
+
+	maxLoop := 10
+	for i := 0; i < maxLoop; i++ {
+		comments, _, err := g.Provider.Client().Issues.ListComments(
+			ctx, g.Options.Organization, g.Options.Repo, g.PRNumber,
+			&github.IssueListCommentsOptions{})
+		assert.NilError(t, err)
+
+		if len(comments) > 0 {
+			assert.Assert(t, len(comments) == 1, "Should have only one comment created we got way too many: %+v", comments)
+			commentBody := comments[0].GetBody()
+			commentRegexp := regexp.MustCompile(`CEL expression evaluation error: failed to parse expression "event == \"pull request\" |": ERROR: <input>:1:25: Syntax error: token recognition error at: '|'  | event == "pull request" |  | ........................^`)
+			assert.Check(t, commentRegexp.MatchString(commentBody), commentBody, t)
+			g.Logger.Info("CEL expression validation comment has been matched")
+			return
+		}
+
+		g.Cnx.Clients.Log.Infof("Looping %d/%d waiting for a comment to appear", i, maxLoop)
+		time.Sleep(6 * time.Second)
+	}
+
+	t.Fatal("No comments with the pipelinerun error found on the pull request")
+}
+
 // TestGithubPullRequestInvalidSpecValues tests invalid field values of a PipelinRun and
 // ensures that these validation errors are reported on UI.
 func TestGithubPullRequestInvalidSpecValues(t *testing.T) {

test/testdata/failures/pipelinerun-invalid-cel.yaml

@@ -0,0 +1,17 @@
+---
+apiVersion: tekton.dev/v1beta1
+kind: PipelineRun
+metadata:
+  name: "\\ .PipelineName //"
+  annotations:
+    pipelinesascode.tekton.dev/on-cel-expression: event == "pull request" |
+spec:
+  pipelineSpec:
+    tasks:
+      - name: task
+        displayName: "The Task name is Task"
+        taskSpec:
+          steps:
+            - name: task
+              image: registry.access.redhat.com/ubi9/ubi-micro
+              command: ["/bin/echo", "HELLOMOTO"]