fix(llm): default analysis to failed pipelines only

Remove the hard-coded failed-only check from the reconciler and
delegate it to the LLM orchestrator. When no on_cel expression is
defined for a role, the orchestrator defaults to triggering only for
failed PipelineRuns. An explicit on_cel expression overrides this,
allowing roles to run on any pipeline outcome.

Signed-off-by: Akshay Pant <akpant@redhat.com>

Author: theakshaypant

pkg/llm/analyzer.go

@@ -15,6 +15,8 @@ import (
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
 	tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
 	"go.uber.org/zap"
+	corev1 "k8s.io/api/core/v1"
+	apis "knative.dev/pkg/apis"
 )
 
 // AnalysisResult represents the result of an LLM analysis.
@@ -110,7 +112,7 @@ func (a *Analyzer) Analyze(ctx context.Context, request *AnalyzeRequest) ([]Anal
 	for _, role := range config.Roles {
 		roleLogger := analysisLogger.With("role", role.Name)
 
-		shouldTrigger, err := a.shouldTriggerRole(role, celContext)
+		shouldTrigger, err := a.shouldTriggerRole(role, celContext, request.PipelineRun)
 		if err != nil {
 			roleLogger.With("error", err, "cel_expression", role.OnCEL).Warn("Failed to evaluate CEL expression")
 			results = append(results, AnalysisResult{
@@ -296,9 +298,12 @@ func countFailedResults(results []AnalysisResult) int {
 }
 
 // shouldTriggerRole evaluates the CEL expression to determine if a role should be triggered.
-func (a *Analyzer) shouldTriggerRole(role v1alpha1.AnalysisRole, celContext map[string]any) (bool, error) {
+// If no on_cel is provided, defaults to triggering only for failed PipelineRuns.
+func (a *Analyzer) shouldTriggerRole(role v1alpha1.AnalysisRole, celContext map[string]any, pr *tektonv1.PipelineRun) (bool, error) {
 	if role.OnCEL == "" {
-		return true, nil
+		succeededCondition := pr.Status.GetCondition(apis.ConditionSucceeded)
+
+		return succeededCondition != nil && succeededCondition.Status == corev1.ConditionFalse, nil
 	}
 
 	result, err := cel.Value(role.OnCEL, celContext["body"],

pkg/llm/analyzer_test.go

@@ -14,6 +14,7 @@ import (
 	tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
 	"gotest.tools/v3/assert"
 	"k8s.io/client-go/kubernetes/fake"
+	"knative.dev/pkg/apis"
 )
 
 func TestAnalyzerAnalyze(t *testing.T) {
@@ -410,14 +411,17 @@ func TestAnalyzerShouldTriggerRoleEvaluations(t *testing.T) {
 		},
 	}
 
+	failedPR := &tektonv1.PipelineRun{}
+	failedPR.Status.Conditions = append(failedPR.Status.Conditions, apis.Condition{Type: apis.ConditionSucceeded, Status: "False"})
+
 	tests := []struct {
 		name      string
 		role      v1alpha1.AnalysisRole
 		want      bool
 		wantError bool
 	}{
 		{
-			name: "no expression returns true",
+			name: "no expression defaults to failed pipelines only",
 			role: v1alpha1.AnalysisRole{},
 			want: true,
 		},
@@ -440,7 +444,7 @@ func TestAnalyzerShouldTriggerRoleEvaluations(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := analyzer.shouldTriggerRole(tt.role, celContext)
+			got, err := analyzer.shouldTriggerRole(tt.role, celContext, failedPR)
 
 			if tt.wantError {
 				assert.Assert(t, err != nil, "expected error but got none")
@@ -459,57 +463,60 @@ func TestAnalyzerShouldTriggerRole(t *testing.T) {
 	kinteract := &kubeinteraction.Interaction{}
 	analyzer := NewAnalyzer(run, kinteract, logger)
 
+	failedPR := &tektonv1.PipelineRun{}
+	failedPR.Status.Conditions = append(failedPR.Status.Conditions, apis.Condition{Type: apis.ConditionSucceeded, Status: "False"})
+
+	succeededPR := &tektonv1.PipelineRun{}
+	succeededPR.Status.Conditions = append(succeededPR.Status.Conditions, apis.Condition{Type: apis.ConditionSucceeded, Status: "True"})
+
 	tests := []struct {
 		name        string
 		role        v1alpha1.AnalysisRole
 		celContext  map[string]any
+		pr          *tektonv1.PipelineRun
 		wantTrigger bool
 		wantError   bool
 	}{
 		{
-			name: "no cel expression - always trigger",
-			role: v1alpha1.AnalysisRole{
-				Name: "test-role",
-			},
+			name:        "no cel expression - triggers for failed pipeline",
+			role:        v1alpha1.AnalysisRole{Name: "test-role"},
 			celContext:  map[string]any{},
+			pr:          failedPR,
 			wantTrigger: true,
-			wantError:   false,
 		},
 		{
-			name: "simple true expression",
-			role: v1alpha1.AnalysisRole{
-				Name:  "test-role",
-				OnCEL: "true",
-			},
+			name:        "no cel expression - skips succeeded pipeline",
+			role:        v1alpha1.AnalysisRole{Name: "test-role"},
 			celContext:  map[string]any{},
-			wantTrigger: true,
-			wantError:   false,
+			pr:          succeededPR,
+			wantTrigger: false,
 		},
 		{
-			name: "simple false expression",
-			role: v1alpha1.AnalysisRole{
-				Name:  "test-role",
-				OnCEL: "false",
-			},
+			name:        "simple true expression",
+			role:        v1alpha1.AnalysisRole{Name: "test-role", OnCEL: "true"},
 			celContext:  map[string]any{},
-			wantTrigger: false,
-			wantError:   false,
+			pr:          succeededPR,
+			wantTrigger: true,
 		},
 		{
-			name: "invalid cel expression",
-			role: v1alpha1.AnalysisRole{
-				Name:  "test-role",
-				OnCEL: "invalid syntax (",
-			},
+			name:        "simple false expression",
+			role:        v1alpha1.AnalysisRole{Name: "test-role", OnCEL: "false"},
 			celContext:  map[string]any{},
+			pr:          failedPR,
 			wantTrigger: false,
-			wantError:   true,
+		},
+		{
+			name:       "invalid cel expression",
+			role:       v1alpha1.AnalysisRole{Name: "test-role", OnCEL: "invalid syntax ("},
+			celContext: map[string]any{},
+			pr:         failedPR,
+			wantError:  true,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			shouldTrigger, err := analyzer.shouldTriggerRole(tt.role, tt.celContext)
+			shouldTrigger, err := analyzer.shouldTriggerRole(tt.role, tt.celContext, tt.pr)
 
 			if tt.wantError {
 				assert.Assert(t, err != nil, "expected error but got none")

pkg/reconciler/reconciler.go

@@ -10,7 +10,6 @@ import (
 	pipelinerunreconciler "github.com/tektoncd/pipeline/pkg/client/injection/reconciler/pipeline/v1/pipelinerun"
 	tektonv1lister "github.com/tektoncd/pipeline/pkg/client/listers/pipeline/v1"
 	"go.uber.org/zap"
-	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"knative.dev/pkg/logging"
 	pkgreconciler "knative.dev/pkg/reconciler"
@@ -239,14 +238,13 @@ func (r *Reconciler) reportFinalStatus(ctx context.Context, logger *zap.SugaredL
 		finalState = kubeinteraction.StateFailed
 	}
 
-	// Perform LLM analysis only for failed pipeline runs (best-effort, non-blocking)
-	// Users can use CEL expressions in role configurations for more fine-grained control
-	if len(newPr.Status.Conditions) > 0 && newPr.Status.Conditions[0].Status == corev1.ConditionFalse {
-		if err := r.performLLMAnalysis(ctx, logger, repo, newPr, event, provider); err != nil {
-			logger.Warnf("LLM analysis failed (non-blocking): %v", err)
-			r.eventEmitter.EmitMessage(repo, zap.WarnLevel, "LLMAnalysisFailed",
-				fmt.Sprintf("AI/LLM analysis failed for repository %s/%s and pipeline run %s: %v", repo.Namespace, repo.Name, newPr.Name, err))
-		}
+	// LLM Analysis orchestrator checks if it is enabled and if the CEL condition
+	// is matched for the defined roles, defaults to failed PipelineRuns only if
+	// no CEL expression is defined.
+	if err := r.performLLMAnalysis(ctx, logger, repo, newPr, event, provider); err != nil {
+		logger.Warnf("LLM analysis failed (non-blocking): %v", err)
+		r.eventEmitter.EmitMessage(repo, zap.WarnLevel, "LLMAnalysisFailed",
+			fmt.Sprintf("AI/LLM analysis failed for repository %s/%s and pipeline run %s: %v", repo.Namespace, repo.Name, newPr.Name, err))
 	}
 
 	if err := r.updateRepoRunStatus(ctx, logger, newPr, repo, event); err != nil {