From 39e43c2a5a919227e6bf06843c4445aebe1fd751 Mon Sep 17 00:00:00 2001 From: nkaewam Date: Tue, 28 Jul 2026 19:36:33 +0700 Subject: [PATCH] fix: use MR pipelines endpoint for merged-results CI association Replace the non-existent pipeline-to-MR reverse lookup (404 on GitLab 18.11+) with forward lookup via GET /merge_requests/:iid/pipelines so check completes for stacks with merged-results head pipelines. Co-authored-by: Cursor --- internal/app/check.go | 15 +++--- internal/app/pipeline_test.go | 97 ++++++++++++++++++++++++++++------ internal/gitlab/client.go | 17 ++---- internal/gitlab/client_test.go | 19 +++---- 4 files changed, 104 insertions(+), 44 deletions(-) diff --git a/internal/app/check.go b/internal/app/check.go index 28e2d0e..2ab061a 100644 --- a/internal/app/check.go +++ b/internal/app/check.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "path/filepath" + "slices" "sort" "strconv" "strings" @@ -160,12 +161,12 @@ func (h *Handler) assessCheck(ctx context.Context, inv cli.Invocation, rc reposi domainPipeline.Kind = stack.PipelineUnknown } if kind == stack.PipelineMergedResult { - associated, associationErr := rc.client.PipelineMergeRequests( - ctx, rc.project.ID.String(), head.ID.String()) + mrPipelines, associationErr := rc.client.MergeRequestPipelines( + ctx, rc.project.ID.String(), member.IID) if associationErr != nil { - return cli.Result{}, classifyGlab("read pipeline merge request association", associationErr) + return cli.Result{}, classifyGlab("read merge request pipelines", associationErr) } - domainPipeline.AssociatedWithMR = pipelineAssociatedExactly(associated, member.IID) + domainPipeline.AssociatedWithMR = pipelineAssociatedWithMR(mrPipelines, head.ID.String()) } if kind == stack.PipelineMergedResult { // A merged-results pipeline runs against a synthetic commit, not @@ -388,8 +389,10 @@ func classifyPipelineKind(p gitlab.Pipeline, memberIID int) stack.PipelineKind { } } -func pipelineAssociatedExactly(requests []gitlab.PipelineMergeRequest, memberIID int) bool { - return len(requests) == 1 && requests[0].IID == memberIID +func pipelineAssociatedWithMR(pipelines []gitlab.Pipeline, pipelineID string) bool { + return slices.ContainsFunc(pipelines, func(p gitlab.Pipeline) bool { + return p.ID.String() == pipelineID + }) } func buildAPIStack(observedAt string, rc repositoryContext, selector api.Selector, mode stack.Mode, diff --git a/internal/app/pipeline_test.go b/internal/app/pipeline_test.go index 13af4a9..190c5d6 100644 --- a/internal/app/pipeline_test.go +++ b/internal/app/pipeline_test.go @@ -3,6 +3,7 @@ package app import ( "bytes" "encoding/json" + "fmt" "strconv" "strings" "testing" @@ -10,6 +11,7 @@ import ( "github.com/nkaewam/mrstack/internal/api" "github.com/nkaewam/mrstack/internal/cli" + "github.com/nkaewam/mrstack/internal/gitexec" "github.com/nkaewam/mrstack/internal/gitlab" "github.com/nkaewam/mrstack/internal/stack" ) @@ -158,27 +160,27 @@ func TestClassifyPipelineKind(t *testing.T) { func TestCheckPipelineEvidenceIsSchemaValidForValidAndAmbiguousMergedResults(t *testing.T) { tests := []struct { name string - associated string + mrPipelines string parents func(source, target string) []string disposition string }{ { - name: "valid", associated: `[{"iid":1}]`, + name: "valid", mrPipelines: `[{"id":9}]`, parents: func(source, target string) []string { return []string{target, source} }, disposition: "ready", }, { - name: "association mismatch", associated: `[{"iid":2}]`, + name: "association mismatch", mrPipelines: `[{"id":10}]`, parents: func(source, target string) []string { return []string{source, target} }, disposition: "human_required", }, { - name: "ambiguous association", associated: `[{"iid":1},{"iid":2}]`, + name: "missing pipeline", mrPipelines: `[]`, parents: func(source, target string) []string { return []string{source, target} }, disposition: "human_required", }, { - name: "parent mismatch", associated: `[{"iid":1}]`, + name: "parent mismatch", mrPipelines: `[{"id":9}]`, parents: func(source, target string) []string { return []string{source, strings.Repeat("d", 40)} }, disposition: "human_required", }, @@ -205,7 +207,7 @@ func TestCheckPipelineEvidenceIsSchemaValidForValidAndAmbiguousMergedResults(t * "id":9,"sha":"` + syntheticOID + `","ref":"refs/merge-requests/1/merge", "status":"success","source":"merge_request_event","web_url":"https://gitlab.example/pipelines/9" }`) - responses["/projects/42/pipelines/9/merge_requests"] = json.RawMessage(tt.associated) + responses["/projects/42/merge_requests/1/pipelines?per_page=100"] = json.RawMessage(tt.mrPipelines) responses["/projects/42/repository/commits/"+syntheticOID] = json.RawMessage( `{"id":"` + syntheticOID + `","parent_ids":` + string(parentsJSON) + `}`) handler := &Handler{ @@ -311,6 +313,9 @@ func TestCheckExactSourcePipelineDoesNotRequireMRAssociation(t *testing.T) { if strings.Contains(strings.Join(call, " "), "/pipelines/9/merge_requests") { t.Fatalf("exact-source %s pipeline queried unnecessary MR association: %q", tt.name, call) } + if strings.Contains(strings.Join(call, " "), "/merge_requests/1/pipelines") { + t.Fatalf("exact-source %s pipeline queried MR pipelines: %q", tt.name, call) + } } var envelope struct { Disposition string `json:"disposition"` @@ -335,19 +340,79 @@ func TestCheckExactSourcePipelineDoesNotRequireMRAssociation(t *testing.T) { } } -func TestPipelineAssociationRequiresExactlySelectedMR(t *testing.T) { - if !pipelineAssociatedExactly([]gitlab.PipelineMergeRequest{{IID: 7}}, 7) { - t.Fatal("one exact association was rejected") +func TestCheckMergedResultsDoesNotCallBrokenPipelineToMREndpoint(t *testing.T) { + repo, _, mainOID, sourceOID, _ := createStackRepository(t) + syntheticOID := strings.Repeat("c", 40) + payload := []map[string]any{{ + "iid": 1, "state": "opened", "source_branch": "feature/one", "target_branch": "main", + "sha": sourceOID, "web_url": "https://gitlab.example/group/project/-/merge_requests/1", + "author": map[string]any{"id": 7, "username": "developer"}, + "diff_refs": map[string]any{"base_sha": mainOID, "head_sha": sourceOID, "start_sha": mainOID}, + "head_pipeline": map[string]any{"id": 9}, + }} + var calls [][]string + stateDir, stacksDir := testDirs(t) + responses := glabProjectResponses(true) + addPerMREndpoints(responses, payload) + responses["/projects/42/pipelines/9"] = json.RawMessage(`{ + "id":9,"sha":"` + syntheticOID + `","ref":"refs/merge-requests/1/merge", + "status":"success","source":"merge_request_event","web_url":"https://gitlab.example/pipelines/9" + }`) + responses["/projects/42/merge_requests/1/pipelines?per_page=100"] = json.RawMessage(`[{"id":9}]`) + responses["/projects/42/repository/commits/"+syntheticOID] = json.RawMessage( + `{"id":"` + syntheticOID + `","parent_ids":["` + mainOID + `","` + sourceOID + `"]}`) + handler := &Handler{ + Runner: fakeGlabRunner{ + calls: &calls, + responses: responses, + dynamic: func(endpoint string, _ []string) (gitexec.Result, error) { + if endpoint == "/projects/42/pipelines/9/merge_requests" { + return gitexec.Result{}, fmt.Errorf("glab: HTTP 404") + } + return gitexec.Result{}, nil + }, + }, + Dir: repo, StateDir: stateDir, StacksDir: stacksDir, + Now: func() time.Time { return time.Date(2026, 7, 25, 12, 0, 0, 0, time.UTC) }, + } + registerNamedStack(t, handler, testStackName, 1) + var stdout, stderr bytes.Buffer + exit := cli.RunWithHandler([]string{ + "--json", "--no-input", "--remote", "origin", "check", testStackName, + }, &stdout, &stderr, handler) + if exit != 0 { + t.Fatalf("exit=%d stdout=%s stderr=%s", exit, stdout.String(), stderr.String()) + } + for _, call := range calls { + if strings.Contains(strings.Join(call, " "), "/pipelines/9/merge_requests") { + t.Fatalf("check called broken reverse-lookup endpoint: %q", call) + } + } + var envelope struct { + Disposition string `json:"disposition"` + Stack struct { + SnapshotID string `json:"snapshot_id"` + } `json:"stack"` + } + if err := json.Unmarshal(stdout.Bytes(), &envelope); err != nil { + t.Fatalf("decode output: %v\n%s", err, stdout.String()) + } + if envelope.Disposition != "ready" || envelope.Stack.SnapshotID == "" { + t.Fatalf("check did not complete with snapshot: %+v\n%s", envelope, stdout.String()) + } +} + +func TestPipelineAssociationRequiresHeadPipelineInMRList(t *testing.T) { + if !pipelineAssociatedWithMR([]gitlab.Pipeline{{ID: json.Number("9")}}, "9") { + t.Fatal("matching pipeline was rejected") } - for name, associated := range map[string][]gitlab.PipelineMergeRequest{ - "missing": nil, - "mismatch": {{IID: 8}}, - "ambiguous": {{IID: 7}, {IID: 8}}, - "duplicated": {{IID: 7}, {IID: 7}}, + for name, pipelines := range map[string][]gitlab.Pipeline{ + "missing": nil, + "mismatch": {{ID: json.Number("10")}}, } { t.Run(name, func(t *testing.T) { - if pipelineAssociatedExactly(associated, 7) { - t.Fatalf("invalid association accepted: %#v", associated) + if pipelineAssociatedWithMR(pipelines, "9") { + t.Fatalf("invalid association accepted: %#v", pipelines) } }) } diff --git a/internal/gitlab/client.go b/internal/gitlab/client.go index 5dedfa4..da2bb0a 100644 --- a/internal/gitlab/client.go +++ b/internal/gitlab/client.go @@ -261,13 +261,6 @@ type Pipeline struct { Source string `json:"source"` } -// PipelineMergeRequest is the deliberately small association shape returned -// by GitLab's pipeline-to-merge-request endpoint. The complete MR payload is -// not needed to prove the association. -type PipelineMergeRequest struct { - IID int `json:"iid"` -} - // Commit contains only immutable commit evidence needed to validate a merged // results pipeline. ParentIDs must be checked exactly by the caller. type Commit struct { @@ -319,17 +312,15 @@ func (c Client) Pipeline(ctx context.Context, projectID, pipelineID string) (Pip return pipeline, err } -func (c Client) PipelineMergeRequests(ctx context.Context, projectID, pipelineID string) ([]PipelineMergeRequest, error) { +func (c Client) MergeRequestPipelines(ctx context.Context, projectID string, iid int) ([]Pipeline, error) { prefix, err := projectPrefix(projectID) if err != nil { return nil, err } - if !validDecimalID(pipelineID) { - return nil, errors.New("pipeline ID must be decimal") + if iid <= 0 { + return nil, errors.New("MR IID must be positive") } - var requests []PipelineMergeRequest - err = c.api(ctx, prefix+"/pipelines/"+pipelineID+"/merge_requests", &requests) - return requests, err + return apiArray[Pipeline](c, ctx, fmt.Sprintf("%s/merge_requests/%d/pipelines?per_page=100", prefix, iid)) } func (c Client) Commit(ctx context.Context, projectID, oid string) (Commit, error) { diff --git a/internal/gitlab/client_test.go b/internal/gitlab/client_test.go index 4069ccc..4fcbe1c 100644 --- a/internal/gitlab/client_test.go +++ b/internal/gitlab/client_test.go @@ -101,8 +101,8 @@ func TestIDsAreValidatedBeforeGlab(t *testing.T) { if _, err := client.Pipeline(context.Background(), "1", "bad"); err == nil { t.Fatal("unsafe pipeline ID accepted") } - if _, err := client.PipelineMergeRequests(context.Background(), "1", "-2"); err == nil { - t.Fatal("unsafe pipeline association ID accepted") + if _, err := client.MergeRequestPipelines(context.Background(), "1", -2); err == nil { + t.Fatal("unsafe merge request IID accepted") } if _, err := client.Commit(context.Background(), "1", "not-an-object-id"); err == nil { t.Fatal("unsafe commit ID accepted") @@ -120,13 +120,13 @@ func TestPipelineEvidenceTransportUsesTypedEndpoints(t *testing.T) { if pipeline.ID.String() != "9" || pipeline.Source != "merge_request_event" { t.Fatalf("pipeline evidence lost: %#v", pipeline) } - runner.stdout = []byte(`[{"iid":7}]`) - associated, err := client.PipelineMergeRequests(context.Background(), "42", "9") + runner.stdout = []byte(`[{"id":9,"ref":"refs/merge-requests/7/merge"}]`) + pipelines, err := client.MergeRequestPipelines(context.Background(), "42", 7) if err != nil { t.Fatal(err) } - if len(associated) != 1 || associated[0].IID != 7 { - t.Fatalf("association evidence lost: %#v", associated) + if len(pipelines) != 1 || pipelines[0].ID.String() != "9" { + t.Fatalf("association evidence lost: %#v", pipelines) } runner.stdout = []byte(`{"id":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent_ids":["bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb","cccccccccccccccccccccccccccccccccccccccc"]}`) commit, err := client.Commit(context.Background(), "42", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") @@ -138,12 +138,13 @@ func TestPipelineEvidenceTransportUsesTypedEndpoints(t *testing.T) { } wantEndpoints := []string{ "/projects/42/pipelines/9", - "/projects/42/pipelines/9/merge_requests", + "/projects/42/merge_requests/7/pipelines?per_page=100", "/projects/42/repository/commits/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", } for i, endpoint := range wantEndpoints { - if !reflect.DeepEqual(runner.calls[i], []string{"glab", "api", endpoint}) { - t.Fatalf("call %d=%#v want endpoint %q", i, runner.calls[i], endpoint) + got := runner.calls[i] + if len(got) < 3 || got[0] != "glab" || got[1] != "api" || got[2] != endpoint { + t.Fatalf("call %d=%#v want endpoint %q", i, got, endpoint) } } }