fix: /ok-to-test /retest pipelineruns should not be created if last sha successful

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

@@ -10,7 +10,7 @@
 
 ## 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:
 
@@ -21,6 +21,21 @@
 /retest
 ```
 
+The `/retest` command will only create new PipelineRuns if:
+- Previous PipelineRuns for the same commit **failed**, OR
+- No PipelineRuns exist for the same commit
+
+If a successful PipelineRun already exists for the same commit, `/retest` will **skip** creating a new PipelineRun to avoid unnecessary duplication.
+
+**To force a rerun regardless of previous status**, use:
+```text
+/retest <pipelinerun-name>
+```
+
+This will always create a new PipelineRun, even if previous runs were successful.
+
+Similar to `/retest`, the `/ok-to-test` command will only create new PipelineRuns if no successful PipelineRun already exists for the same commit. This prevents duplicate runs when repository owners repeatedly approve the same commit.
+
 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
@@ -241,10 +256,10 @@
 
 There are different formats that can be accepted, allowing you to pass values with spaces or newlines:
 
 * key=value
 * key="a value"
 * key="another \"value\" defined"
 * key="another
   value with newline"
 
 ## Event Type Annotation and Dynamic Variables
@@ -253,14 +268,14 @@
 
 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.
 
@@ -272,12 +287,12 @@
 
 When using the `{{ event_type }}` [dynamic variable]({{< relref "/docs/guide/authoringprs.md#dynamic-variables" >}}) for the following event types:
 
 * `test-all-comment`
 * `test-comment`
 * `retest-all-comment`
 * `retest-comment`
 * `cancel-all-comment`
 * `ok-to-test-comment`
 
 The dynamic variable will return `pull_request` as the event type instead of the specific categorized GitOps command type. This is to handle backward compatibility with previous releases for users relying on this dynamic variable.
 

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 create new PipelineRuns if previous runs failed. This action takes precedence over the `pull_request` action.
 
 ## Configuring Policies in the Repository CR
 

hack/gh-workflow-ci.sh

@@ -74,8 +74,10 @@ get_tests() {
   mapfile -t testfiles < <(find test/ -maxdepth 1 -name '*.go')
   ghglabre="Github|Gitlab|Bitbucket"
   if [[ ${target} == "providers" ]]; then
+    # echo "TestGithubMaxKeepRuns"
     grep -hioP "^func Test.*(${ghglabre})(\w+)\(" "${testfiles[@]}" | sed -e 's/func[ ]*//' -e 's/($//'
-  elif [[ ${target} == "gitea_others" ]]; then
+    elif [[ ${target} == "gitea_others" ]]; then
+    # echo "TestGiteaParamsOnRepoCR"
     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,51 @@ func MatchPipelinerunByAnnotation(ctx context.Context, logger *zap.SugaredLogger
 	}
 
 	if len(matchedPRs) > 0 {
+		if existingPR := checkForExistingSuccessfulPipelineRun(ctx, logger, cs, event, repo); existingPR != nil {
+			return []Match{{PipelineRun: existingPR, Repo: repo}}, nil
+		}
 		return matchedPRs, nil
 	}
 
 	return nil, fmt.Errorf("%s", buildAvailableMatchingAnnotationErr(event, pruns))
 }
 
+// checkForExistingSuccessfulPipelineRun checks if there's an existing successful PipelineRun for the same SHA
+// when executing /ok-to-test or /retest gitops commands.
+func checkForExistingSuccessfulPipelineRun(ctx context.Context, logger *zap.SugaredLogger, cs *params.Run, event *info.Event, repo *apipac.Repository) *tektonv1.PipelineRun {
+	// Only apply this logic to /retest (without specific PR name) and /ok-to-test commands
+	// /retest <pipelinerun-name> should always create a new run since user explicitly requested it
+	if (event.EventType == opscomments.RetestAllCommentEventType.String() ||
+		event.EventType == opscomments.OkToTestCommentEventType.String()) &&
+		event.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 nil
+		}
+		if len(existingPRs.Items) > 0 {
+			var lastRun tektonv1.PipelineRun
+			lastRun = existingPRs.Items[0]
+
+			for _, pr := range existingPRs.Items {
+				if pr.CreationTimestamp.After(lastRun.CreationTimestamp.Time) {
+					lastRun = pr
+				}
+			}
+
+			if lastRun.Status.GetCondition(apis.ConditionSucceeded).IsTrue() {
+				logger.Infof("skipping creation of new pipelinerun for sha %s as the last pipelinerun '%s' has already succeeded",
+					event.SHA, lastRun.Name)
+				return &lastRun
+			}
+		}
+	}
+	return nil
+}
+
 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

@@ -30,7 +30,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 +2602,130 @@ func TestGetName(t *testing.T) {
 		})
 	}
 }
+
+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",
+			},
+			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",
+			},
+			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

@@ -55,13 +55,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,

test/gitea_gitops_commands_test.go

@@ -192,16 +192,22 @@ func TestGiteaRetestAll(t *testing.T) {
 	assert.NilError(t, secret.Create(ctx, topts.ParamsRun, map[string]string{"secret": "SHHHHHHH"}, topts.TargetNS, "pac-secret"))
 	topts.TargetEvent = triggertype.PullRequest.String()
 	topts.YAMLFiles = map[string]string{
-		".tekton/pr.yaml":      "testdata/pipelinerun.yaml",
-		".tekton/nomatch.yaml": "testdata/pipelinerun-nomatch.yaml",
+		".tekton/pr.yaml":        "testdata/pipelinerun.yaml",
+		".tekton/pr-second.yaml": "testdata/pipelinerun-clone.yaml", // Use a matching PipelineRun instead of nomatch
 	}
 	_, f := tgitea.TestPR(t, topts)
 	defer f()
-	tgitea.PostCommentOnPullRequest(t, topts, "/retest")
+
+	// Wait for initial PipelineRuns to complete, then get one to retest specifically
+	time.Sleep(5 * time.Second)
+	pipelineRunName, err := twait.GetOriginalPipelineRunName(context.Background(), topts.ParamsRun.Clients, topts.TargetNS, topts.PullRequest.Head.Sha)
+	assert.NilError(t, err)
+
+	tgitea.PostCommentOnPullRequest(t, topts, "/retest "+pipelineRunName)
 	waitOpts := twait.Opts{
 		RepoName:        topts.TargetNS,
 		Namespace:       topts.TargetNS,
-		MinNumberStatus: 2,
+		MinNumberStatus: 3, // 2 initial + 1 retest
 		PollTimeout:     twait.DefaultTimeout,
 	}
 
@@ -214,6 +220,6 @@ func TestGiteaRetestAll(t *testing.T) {
 			rt = true
 		}
 	}
-	assert.Assert(t, rt, "should have a retest all comment event in status")
-	assert.Equal(t, len(repo.Status), 2, "should have only 2 status")
+	assert.Assert(t, rt, "should have a retest comment event in status")
+	assert.Equal(t, len(repo.Status), 3, "should have 3 statuses: 2 initial + 1 retest")
 }

test/gitea_params_test.go

@@ -314,7 +314,15 @@ func TestGiteaParamsOnRepoCR(t *testing.T) {
 	_, f := tgitea.TestPR(t, topts)
 	defer f()
 
-	repo, err := topts.ParamsRun.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(topts.TargetNS).Get(context.Background(), topts.TargetNS, metav1.GetOptions{})
+	// Wait for Repository status to be updated
+	waitOpts := twait.Opts{
+		RepoName:        topts.TargetNS,
+		Namespace:       topts.TargetNS,
+		MinNumberStatus: 1,
+		PollTimeout:     twait.DefaultTimeout,
+		TargetSHA:       "",
+	}
+	repo, err := twait.UntilRepositoryUpdated(context.Background(), topts.ParamsRun.Clients, waitOpts)
 	assert.NilError(t, err)
 	assert.Assert(t, len(repo.Status) != 0)
 	assert.NilError(t,