fix: don't restart successful pipelineruns on /retest /ok-to-test

/retest and /ok-to-test should only restart failed pipelines; the
changes in the PR ensure that we do not match successful pipelineruns
during invocation of these gitops commands. The tests have also been
updated to use /test instead wherever /retest is being used to restart
successful pipelines

Signed-off-by: Vibhav Bobade <vibhav.bobde@gmail.com>
Assisted-by: Claude-4-Sonnet (via Cursor)

Author: waveywaves

docs/content/_index.md

@@ -35,7 +35,7 @@ tracking using a Git workflow.
 
 <--->
 
-- Pull-request "*GitOps*" actions through comments with  `/retest`, `/test <pipeline-name>` and so on.
+- Pull-request "*GitOps*" actions through comments with `/retest` (reruns failed pipelines), `/test <pipeline-name>` (force rerun specific pipeline) and so on.
 
 - Automatic Task resolution in Pipelines (local Tasks, Artifact Hub, and remote URLs)
 

docs/content/docs/guide/gitops_commands.md

@@ -11,7 +11,7 @@ The advantage of using a `GitOps command` is that it provides a journal of all t
 
 ## GitOps Commands on Pull Requests
 
-For example, when you are on a Pull Request, you may want to restart all your PipelineRuns. To do so, you can add a comment on your Pull Request starting with `/retest`, and all PipelineRuns attached to that Pull Request will be restarted.
+For example, when you are on a Pull Request, you may want to restart failed PipelineRuns. To do so, you can add a comment on your Pull Request starting with `/retest`, and all **failed** PipelineRuns attached to that Pull Request will be restarted. If all previous PipelineRuns for the same commit were successful, no new PipelineRuns will be created to avoid unnecessary duplication.
 
 Example:
 
@@ -22,6 +22,23 @@ failure is not with your PR but seems to be an infrastructure issue.
 /retest
 ```
 
+The `/retest` command will only create new PipelineRuns if:
+
+- Previously **failed** PipelineRuns for the same commit, OR
+- No PipelineRun has been run for the same commit yet
+
+If a successful PipelineRun already exists for the same commit, `/retest` will **skip** triggering a new PipelineRun to avoid unnecessary duplication.
+
+**To force a rerun regardless of previous status**, use:
+
+```text
+/retest <pipelinerun-name>
+```
+
+This will always trigger a new PipelineRun, even if previous runs were successful.
+
+Similar to `/retest`, the `/ok-to-test` command will only trigger new PipelineRuns if no successful PipelineRun already exists for the same commit. This prevents duplicate runs when repository owners repeatedly test the same commit by `/test` and `/retest` command.
+
 If you have multiple `PipelineRun` and you want to target a specific `PipelineRun`, you can use the `/test` command followed by the specific PipelineRun name to restart it. Example:
 
 ```text
@@ -335,12 +352,12 @@ Here are the possible event types:
 
 - `test-all-comment`: The event is a single `/test` that would test every matched PipelineRun.
 - `test-comment`: The event is a `/test <PipelineRun>` comment that would test a specific PipelineRun.
-- `retest-all-comment`: The event is a single `/retest` that would retest every matched PipelineRun.
+- `retest-all-comment`: The event is a single `/retest` that would retest every matched **failed** PipelineRun. If a successful PipelineRun already exists for the same commit, no new PipelineRun will be created.
 - `retest-comment`: The event is a `/retest <PipelineRun>` that would retest a specific PipelineRun.
 - `on-comment`: The event is coming from a custom comment that would trigger a PipelineRun.
 - `cancel-all-comment`: The event is a single `/cancel` that would cancel every matched PipelineRun.
 - `cancel-comment`: The event is a `/cancel <PipelineRun>` that would cancel a specific PipelineRun.
-- `ok-to-test-comment`: The event is a `/ok-to-test` that would allow running the CI for an unauthorized user.
+- `ok-to-test-comment`: The event is a `/ok-to-test` that would allow running the CI for an unauthorized user. If a successful PipelineRun already exists for the same commit, no new PipelineRun will be created.
 
 If a repository owner comments `/ok-to-test` on a pull request from an external contributor but no PipelineRun **matches** the `pull_request` event (or the repository has no `.tekton` directory), Pipelines-as-Code sets a **neutral** commit status. This indicates that no PipelineRun was matched, allowing other workflows—such as auto-merge—to proceed without being blocked.
 

docs/content/docs/guide/policy.md

@@ -23,7 +23,7 @@ or other supported Git providers (currently GitHub and Gitea).
   to trigger the CI for a pull request by commenting `/ok-to-test`. This enables
   CI to run on pull requests submitted by contributors who are not collaborators
   of the repository or organization. It also applies to `/test` and `/retest`
-  commands. This action takes precedence over the `pull_request` action.
+  commands. Note that `/retest` will only trigger failed PipelineRuns. This action takes precedence over the `pull_request` action.
 
 ## Configuring Policies in the Repository CR
 

hack/gh-workflow-ci.sh

@@ -75,7 +75,7 @@ get_tests() {
   ghglabre="Github|Gitlab|Bitbucket"
   if [[ ${target} == "providers" ]]; then
     grep -hioP "^func Test.*(${ghglabre})(\w+)\(" "${testfiles[@]}" | sed -e 's/func[ ]*//' -e 's/($//'
-  elif [[ ${target} == "gitea_others" ]]; then
+    elif [[ ${target} == "gitea_others" ]]; then
     grep -hioP '^func Test(\w+)\(' "${testfiles[@]}" | grep -iPv "(${ghglabre})" | sed -e 's/func[ ]*//' -e 's/($//'
   else
     echo "Invalid target: ${target}"

pkg/matcher/annotation_matcher.go

@@ -11,6 +11,7 @@ import (
 	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/formatting"
 	"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"
@@ -21,6 +22,8 @@ import (
 	"github.com/google/cel-go/common/types"
 	tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
 	"go.uber.org/zap"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"knative.dev/pkg/apis"
 )
 
 const (
@@ -366,12 +369,77 @@ func MatchPipelinerunByAnnotation(ctx context.Context, logger *zap.SugaredLogger
 	}
 
 	if len(matchedPRs) > 0 {
+		// Filter out templates that already have successful PipelineRuns for /retest and /ok-to-test
+		if event.EventType == opscomments.RetestAllCommentEventType.String() ||
+			event.EventType == opscomments.OkToTestCommentEventType.String() {
+			return filterSuccessfulTemplates(ctx, logger, cs, event, repo, matchedPRs), nil
+		}
 		return matchedPRs, nil
 	}
 
 	return nil, fmt.Errorf("%s", buildAvailableMatchingAnnotationErr(event, pruns))
 }
 
+// filterSuccessfulTemplates filters out templates that already have successful PipelineRuns
+// when executing /ok-to-test or /retest gitops commands, implementing per-template checking.
+func filterSuccessfulTemplates(ctx context.Context, logger *zap.SugaredLogger, cs *params.Run, event *info.Event, repo *apipac.Repository, matchedPRs []Match) []Match {
+	if event.SHA == "" {
+		return matchedPRs
+	}
+
+	// Get all existing PipelineRuns for this SHA
+	labelSelector := fmt.Sprintf("%s=%s", keys.SHA, formatting.CleanValueKubernetes(event.SHA))
+	existingPRs, err := cs.Clients.Tekton.TektonV1().PipelineRuns(repo.GetNamespace()).List(ctx, metav1.ListOptions{
+		LabelSelector: labelSelector,
+	})
+	if err != nil {
+		logger.Errorf("failed to list existing PipelineRuns for SHA %s: %v", event.SHA, err)
+		return matchedPRs // Return all templates if we can't check
+	}
+
+	// Create a map of template names to their most recent successful run
+	successfulTemplates := make(map[string]*tektonv1.PipelineRun)
+
+	for i := range existingPRs.Items {
+		pr := &existingPRs.Items[i]
+
+		// Get the original template name this PipelineRun came from
+		originalPRName, ok := pr.GetAnnotations()[keys.OriginalPRName]
+		if !ok {
+			originalPRName, ok = pr.GetLabels()[keys.OriginalPRName]
+		}
+		if !ok {
+			continue // Skip PipelineRuns without template identification
+		}
+
+		// Check if this PipelineRun succeeded
+		if pr.Status.GetCondition(apis.ConditionSucceeded).IsTrue() {
+			// Keep the most recent successful run for each template
+			if existing, exists := successfulTemplates[originalPRName]; !exists ||
+				pr.CreationTimestamp.After(existing.CreationTimestamp.Time) {
+				successfulTemplates[originalPRName] = pr
+			}
+		}
+	}
+
+	// Filter out templates that have successful runs
+	var filteredPRs []Match
+
+	for _, match := range matchedPRs {
+		templateName := getName(match.PipelineRun)
+
+		if successfulPR, hasSuccessfulRun := successfulTemplates[templateName]; hasSuccessfulRun {
+			logger.Infof("skipping template '%s' for sha %s as it already has a successful pipelinerun '%s'",
+				templateName, event.SHA, successfulPR.Name)
+		} else {
+			filteredPRs = append(filteredPRs, match)
+		}
+	}
+
+	// Return the filtered list (which may be empty if all templates were skipped)
+	return filteredPRs
+}
+
 func buildAvailableMatchingAnnotationErr(event *info.Event, pruns []*tektonv1.PipelineRun) string {
 	errmsg := "available annotations of the PipelineRuns annotations in .tekton/ dir:"
 	for _, prun := range pruns {

pkg/matcher/annotation_matcher_test.go

@@ -15,6 +15,7 @@ import (
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/events"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/formatting"
 	"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/clients"
@@ -30,7 +31,10 @@ import (
 	zapobserver "go.uber.org/zap/zaptest/observer"
 	"gotest.tools/v3/assert"
 	"gotest.tools/v3/golden"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"knative.dev/pkg/apis"
+	knativeduckv1 "knative.dev/pkg/apis/duck/v1"
 	rtesting "knative.dev/pkg/reconciler/testing"
 )
 
@@ -2599,3 +2603,172 @@ func TestGetName(t *testing.T) {
 		})
 	}
 }
+
+// checkForExistingSuccessfulPipelineRun is a helper function for testing.
+// It checks if there's an existing successful PipelineRun for the same SHA.
+func checkForExistingSuccessfulPipelineRun(ctx context.Context, logger *zap.SugaredLogger, cs *params.Run, event *info.Event, repo *v1alpha1.Repository) *tektonv1.PipelineRun {
+	// Only check for /retest and /ok-to-test commands
+	if event.EventType != opscomments.RetestAllCommentEventType.String() &&
+		event.EventType != opscomments.OkToTestCommentEventType.String() ||
+		event.SHA == "" {
+		return nil
+	}
+
+	labelSelector := fmt.Sprintf("%s=%s", keys.SHA, formatting.CleanValueKubernetes(event.SHA))
+	existingPRs, err := cs.Clients.Tekton.TektonV1().PipelineRuns(repo.GetNamespace()).List(ctx, metav1.ListOptions{
+		LabelSelector: labelSelector,
+	})
+	if err != nil {
+		logger.Errorf("failed to list existing PipelineRuns for SHA %s: %v", event.SHA, err)
+		return nil
+	}
+
+	// Find the most recent successful PipelineRun
+	var mostRecentSuccessfulPR *tektonv1.PipelineRun
+	for i := range existingPRs.Items {
+		pr := &existingPRs.Items[i]
+		if pr.Status.GetCondition(apis.ConditionSucceeded).IsTrue() {
+			if mostRecentSuccessfulPR == nil ||
+				pr.CreationTimestamp.After(mostRecentSuccessfulPR.CreationTimestamp.Time) {
+				mostRecentSuccessfulPR = pr
+			}
+		}
+	}
+
+	return mostRecentSuccessfulPR
+}
+
+func TestCheckForExistingSuccessfulPipelineRun(t *testing.T) {
+	ctx, _ := rtesting.SetupFakeContext(t)
+	logger := zap.NewExample().Sugar()
+
+	repo := &v1alpha1.Repository{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-repo",
+			Namespace: "test-ns",
+		},
+	}
+
+	// Create a successful PipelineRun
+	pr := &tektonv1.PipelineRun{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-pr",
+			Namespace: "test-ns",
+			Labels: map[string]string{
+				keys.SHA:            "test-sha",
+				keys.OriginalPRName: "test-pr",
+			},
+			Annotations: map[string]string{
+				keys.OriginalPRName: "test-pr",
+			},
+			CreationTimestamp: metav1.Now(),
+		},
+		Status: tektonv1.PipelineRunStatus{
+			Status: knativeduckv1.Status{
+				Conditions: knativeduckv1.Conditions{
+					apis.Condition{
+						Type:   apis.ConditionSucceeded,
+						Status: corev1.ConditionTrue,
+					},
+				},
+			},
+		},
+	}
+
+	// Create a failed PipelineRun with the same SHA but older
+	earlierTime := metav1.NewTime(time.Now().Add(-1 * time.Hour))
+	failedPR := &tektonv1.PipelineRun{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "failed-pr",
+			Namespace: "test-ns",
+			Labels: map[string]string{
+				keys.SHA:            "test-sha",
+				keys.OriginalPRName: "failed-pr",
+			},
+			Annotations: map[string]string{
+				keys.OriginalPRName: "failed-pr",
+			},
+			CreationTimestamp: earlierTime,
+		},
+		Status: tektonv1.PipelineRunStatus{
+			Status: knativeduckv1.Status{
+				Conditions: knativeduckv1.Conditions{
+					apis.Condition{
+						Type:   apis.ConditionSucceeded,
+						Status: corev1.ConditionFalse,
+					},
+				},
+			},
+		},
+	}
+
+	// Setup test clients
+	tdata := testclient.Data{
+		PipelineRuns: []*tektonv1.PipelineRun{pr, failedPR},
+		Repositories: []*v1alpha1.Repository{repo},
+	}
+	stdata, _ := testclient.SeedTestData(t, ctx, tdata)
+
+	cs := &params.Run{
+		Clients: clients.Clients{
+			Log:    logger,
+			Tekton: stdata.Pipeline,
+			Kube:   stdata.Kube,
+		},
+	}
+
+	tests := []struct {
+		name      string
+		eventType string
+		sha       string
+		wantPR    bool
+	}{
+		{
+			name:      "Retest command with matching SHA should find successful PR",
+			eventType: opscomments.RetestAllCommentEventType.String(),
+			sha:       "test-sha",
+			wantPR:    true,
+		},
+		{
+			name:      "Ok-to-test command with matching SHA should find successful PR",
+			eventType: opscomments.OkToTestCommentEventType.String(),
+			sha:       "test-sha",
+			wantPR:    true,
+		},
+		{
+			name:      "Retest command with non-matching SHA should not find PR",
+			eventType: opscomments.RetestAllCommentEventType.String(),
+			sha:       "other-sha",
+			wantPR:    false,
+		},
+		{
+			name:      "Different event type should not find PR",
+			eventType: opscomments.TestAllCommentEventType.String(),
+			sha:       "test-sha",
+			wantPR:    false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			event := &info.Event{
+				EventType: tt.eventType,
+				SHA:       tt.sha,
+			}
+
+			foundPR := checkForExistingSuccessfulPipelineRun(ctx, logger, cs, event, repo)
+
+			if tt.wantPR && foundPR == nil {
+				t.Errorf("Expected to find a successful PipelineRun, but got nil")
+			}
+
+			if !tt.wantPR && foundPR != nil {
+				t.Errorf("Expected not to find a PipelineRun, but found %s", foundPR.Name)
+			}
+
+			if tt.wantPR && foundPR != nil {
+				assert.Equal(t, "test-pr", foundPR.Name)
+			}
+		})
+	}
+}

pkg/pipelineascode/pipelineascode.go

@@ -56,13 +56,21 @@ func NewPacs(event *info.Event, vcx provider.Interface, run *params.Run, pacInfo
 }
 
 func (p *PacRun) Run(ctx context.Context) error {
-	matchedPRs, repo, err := p.matchRepoPR(ctx)
-	if repo != nil && p.event.TriggerTarget == triggertype.PullRequestClosed {
-		if err := p.cancelAllInProgressBelongingToClosedPullRequest(ctx, repo); err != nil {
-			return fmt.Errorf("error cancelling in progress pipelineRuns belonging to pull request %d: %w", p.event.PullRequestNumber, err)
+	// For PullRequestClosed events, skip matching logic and go straight to cancellation
+	if p.event.TriggerTarget == triggertype.PullRequestClosed {
+		repo, err := p.verifyRepoAndUser(ctx)
+		if err != nil {
+			return err
+		}
+		if repo != nil {
+			if err := p.cancelAllInProgressBelongingToClosedPullRequest(ctx, repo); err != nil {
+				return fmt.Errorf("error cancelling in progress pipelineRuns belonging to pull request %d: %w", p.event.PullRequestNumber, err)
+			}
 		}
 		return nil
 	}
+
+	matchedPRs, repo, err := p.matchRepoPR(ctx)
 	if err != nil {
 		createStatusErr := p.vcx.CreateStatus(ctx, p.event, provider.StatusOpts{
 			Status:     CompletedStatus,