From ab5a27438534dbb2ad59a93b8b03b1e29a0ecd4f Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Wed, 14 Oct 2026 12:19:54 +1300 Subject: [PATCH] fix: approve the stored CircleCI workflow when its pipeline listing omits it Fixes #153. ApproveJob found the workflow to approve through the pipeline's workflow listing (to follow reruns). CircleCI's listing sometimes leaves a pipeline's workflows out for a while (returning only setup-workflow), so the lookup failed and the hold job stayed on hold even though the deploy was approved. Now the stored workflow's hold job is approved while it's on hold, without the listing. The listing is only checked when it isn't, to find a rerun: the newest workflow in the pipeline with the same name, created after the original. Without one, the stored workflow's hold job is approved anyway and CircleCI's response says why it can't be. The pipeline is still verified (revision, repository, not a fork) before anything is approved. Co-Authored-By: Claude Opus 5.5 --- .../deploy-approvals/circleci.mdx | 7 +- internal/gateway/circleci/client.go | 169 +++++++--------- internal/gateway/circleci/client_test.go | 188 ++++++++++++++++++ .../circleci/pipeline_verification_test.go | 54 ++--- 4 files changed, 277 insertions(+), 141 deletions(-) create mode 100644 internal/gateway/circleci/client_test.go diff --git a/docs/src/content/docs/integrations/deploy-approvals/circleci.mdx b/docs/src/content/docs/integrations/deploy-approvals/circleci.mdx index 6d907a73..64112471 100644 --- a/docs/src/content/docs/integrations/deploy-approvals/circleci.mdx +++ b/docs/src/content/docs/integrations/deploy-approvals/circleci.mdx @@ -240,8 +240,11 @@ sequenceDiagram CircleCI-->>Gateway: 202 Accepted ``` -The approval runs as a background job a moment after the admin approves. If the workflow was rerun, the gateway -approves the hold job in the newest workflow with the same name in that pipeline. +The approval runs as a background job a moment after the admin approves. It approves the hold job in the workflow +that requested the approval while that job is on hold. If it isn't, and the workflow was rerun, the gateway +approves the hold job in the newest rerun: the newest workflow in that pipeline with the same name, created after +the original. The gateway only looks at the pipeline's workflow list to find a rerun, because CircleCI sometimes +leaves workflows out of that list for a while. The `ci_metadata` comes from the CI job that requested the approval, so the gateway doesn't trust it on its own: the hold job is only approved when the workflow's pipeline is building the approved commit of the app's diff --git a/internal/gateway/circleci/client.go b/internal/gateway/circleci/client.go index fb1f6262..c529b107 100644 --- a/internal/gateway/circleci/client.go +++ b/internal/gateway/circleci/client.go @@ -62,10 +62,10 @@ type ApprovalMetadata struct { ApprovalJobName string `json:"approval_job_name"` } -// ApproveJob approves a pending approval job in a CircleCI workflow. +// ApproveJob approves the hold job named jobName in a CircleCI workflow. // The workflow's pipeline must be building the approved revision of the expected repository; // otherwise ErrPipelineMismatch is returned and nothing is approved. -// It handles workflow reruns by finding the latest workflow with the same name in the pipeline. +// If the workflow was rerun, the hold job in the newest rerun is approved instead (see approvalTarget). func (c *Client) ApproveJob(workflowID, pipelineNumber, jobName string, expect ApprovalExpectation) error { if err := ValidateWorkflowID(workflowID); err != nil { return err @@ -77,13 +77,12 @@ func (c *Client) ApproveJob(workflowID, pipelineNumber, jobName string, expect A return fmt.Errorf("job_name is required") } - // Get the workflow details to find its name and pipeline ID - originalWorkflow, err := c.getWorkflowDetails(workflowID) + original, err := c.getWorkflowDetails(workflowID) if err != nil { return fmt.Errorf("failed to get workflow details: %w", err) } - pipeline, err := c.getPipeline(originalWorkflow.PipelineID) + pipeline, err := c.getPipeline(original.PipelineID) if err != nil { return fmt.Errorf("failed to get pipeline: %w", err) } @@ -91,102 +90,84 @@ func (c *Client) ApproveJob(workflowID, pipelineNumber, jobName string, expect A return err } - // Find the latest workflow with the same name in the pipeline - // Use the pipeline ID (UUID) from the workflow, not the pipeline number - latestWorkflowID, err := c.findLatestWorkflowByName(originalWorkflow.PipelineID, originalWorkflow.Name) + targetWorkflowID, jobID, err := c.approvalTarget(original, jobName) if err != nil { - return fmt.Errorf("failed to find latest workflow: %w", err) + return err } - // Get the workflow jobs to find the approval job ID - workflow, err := c.getWorkflow(latestWorkflowID) - if err != nil { - return fmt.Errorf("failed to get workflow: %w", err) + url := fmt.Sprintf("%s/workflow/%s/approve/%s", c.BaseURL, targetWorkflowID, jobID) + if _, err := c.doRequest(http.MethodPost, url); err != nil { + return err } - // Find the approval job by name - jobID, err := findApprovalJobID(workflow, jobName) + return nil +} + +// approvalTarget returns the workflow and hold job to approve. While the original workflow's hold job is on hold, +// that's the one: CircleCI's pipeline workflow listing sometimes leaves a pipeline's workflows out for a while, so +// the usual case doesn't depend on it. Otherwise the listing is checked for a rerun (a newer workflow with the same +// name), and without one the original's hold job is approved anyway, leaving CircleCI to say why it can't be (for +// example, because it's already approved). +func (c *Client) approvalTarget(original *workflowDetails, jobName string) (workflowID, jobID string, err error) { + job, err := c.findApprovalJob(original.ID, jobName) if err != nil { - return fmt.Errorf("failed to find approval job: %w", err) + return "", "", err } - - // Approve the job - url := fmt.Sprintf("%s/workflow/%s/approve/%s", c.BaseURL, latestWorkflowID, jobID) - if _, err := c.doRequest(http.MethodPost, url); err != nil { - return err + if job.Status == jobStatusOnHold { + return original.ID, job.ID, nil } - return nil + rerunID, err := c.findRerun(original) + if err != nil { + return "", "", fmt.Errorf("failed to look for a rerun of workflow %s: %w", original.ID, err) + } + if rerunID == "" { + return original.ID, job.ID, nil + } + rerunJob, err := c.findApprovalJob(rerunID, jobName) + if err != nil { + return "", "", err + } + return rerunID, rerunJob.ID, nil } -type workflowResponse struct { - ID string `json:"id"` - Jobs []struct { - ID string `json:"id"` - Name string `json:"name"` - Type string `json:"type"` - ApprovalRequest *string `json:"approval_request_id,omitempty"` - Status string `json:"status"` - } `json:"jobs"` +// jobStatusOnHold is the status of an approval job that is waiting to be approved. +const jobStatusOnHold = "on_hold" + +type workflowJob struct { + ID string `json:"id"` + Name string `json:"name"` + Type string `json:"type"` + Status string `json:"status"` } -func (c *Client) getWorkflow(workflowID string) (*workflowResponse, error) { +// findApprovalJob returns the approval job named jobName in a workflow. +func (c *Client) findApprovalJob(workflowID, jobName string) (*workflowJob, error) { url := fmt.Sprintf("%s/workflow/%s/job", c.BaseURL, workflowID) body, err := c.doRequest(http.MethodGet, url) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get workflow jobs: %w", err) } var result struct { - Items []struct { - ID string `json:"id"` - Name string `json:"name"` - Type string `json:"type"` - ApprovalRequest *string `json:"approval_request_id,omitempty"` - Status string `json:"status"` - } `json:"items"` + Items []workflowJob `json:"items"` } - if err := json.Unmarshal(body, &result); err != nil { - return nil, fmt.Errorf("failed to parse workflow response: %w", err) - } - - workflow := &workflowResponse{ - ID: workflowID, - Jobs: make([]struct { - ID string `json:"id"` - Name string `json:"name"` - Type string `json:"type"` - ApprovalRequest *string `json:"approval_request_id,omitempty"` - Status string `json:"status"` - }, len(result.Items)), - } - - for i, item := range result.Items { - workflow.Jobs[i] = struct { - ID string `json:"id"` - Name string `json:"name"` - Type string `json:"type"` - ApprovalRequest *string `json:"approval_request_id,omitempty"` - Status string `json:"status"` - }{ - ID: item.ID, - Name: item.Name, - Type: item.Type, - ApprovalRequest: item.ApprovalRequest, - Status: item.Status, - } + return nil, fmt.Errorf("failed to parse workflow jobs: %w", err) } - return workflow, nil + for i := range result.Items { + if result.Items[i].Name == jobName && result.Items[i].Type == "approval" { + return &result.Items[i], nil + } + } + return nil, fmt.Errorf("approval job %q not found in workflow %s", jobName, workflowID) } type workflowDetails struct { ID string `json:"id"` Name string `json:"name"` - ProjectID string `json:"project_id"` PipelineID string `json:"pipeline_id"` - Status string `json:"status"` CreatedAt string `json:"created_at"` } @@ -206,46 +187,40 @@ func (c *Client) getWorkflowDetails(workflowID string) (*workflowDetails, error) return &details, nil } -type pipelineWorkflow struct { - ID string `json:"id"` - Name string `json:"name"` - Status string `json:"status"` - CreatedAt string `json:"created_at"` -} +// findRerun returns the newest workflow in the original's pipeline that has its name and was created after it, or +// "" when there is none. +func (c *Client) findRerun(original *workflowDetails) (string, error) { + originalCreated, err := time.Parse(time.RFC3339, original.CreatedAt) + if err != nil { + return "", fmt.Errorf("failed to parse the workflow's created_at %q: %w", original.CreatedAt, err) + } -// findLatestWorkflowByName finds the most recent workflow with the given name in a pipeline. -// pipelineID must be the UUID pipeline ID, not the pipeline number. -func (c *Client) findLatestWorkflowByName(pipelineID, workflowName string) (string, error) { - url := fmt.Sprintf("%s/pipeline/%s/workflow", c.BaseURL, pipelineID) + url := fmt.Sprintf("%s/pipeline/%s/workflow", c.BaseURL, original.PipelineID) body, err := c.doRequest(http.MethodGet, url) if err != nil { return "", err } var result struct { - Items []pipelineWorkflow `json:"items"` + Items []struct { + ID string `json:"id"` + Name string `json:"name"` + CreatedAt string `json:"created_at"` + } `json:"items"` } if err := json.Unmarshal(body, &result); err != nil { return "", fmt.Errorf("failed to parse pipeline workflows: %w", err) } - // Find the latest workflow with matching name (workflows are ordered by created_at descending) + rerunID, newest := "", originalCreated for _, wf := range result.Items { - if wf.Name == workflowName { - return wf.ID, nil - } - } - - return "", fmt.Errorf("no workflow found with name %q in pipeline %s", workflowName, pipelineID) -} - -func findApprovalJobID(workflow *workflowResponse, jobName string) (string, error) { - for _, job := range workflow.Jobs { - if job.Name == jobName && job.Type == "approval" { - return job.ID, nil + created, err := time.Parse(time.RFC3339, wf.CreatedAt) + if wf.Name != original.Name || wf.ID == original.ID || err != nil || !created.After(newest) { + continue } + rerunID, newest = wf.ID, created } - return "", fmt.Errorf("approval job %q not found in workflow", jobName) + return rerunID, nil } // ValidateMetadata validates that the metadata contains required CircleCI fields. diff --git a/internal/gateway/circleci/client_test.go b/internal/gateway/circleci/client_test.go new file mode 100644 index 00000000..49a40894 --- /dev/null +++ b/internal/gateway/circleci/client_test.go @@ -0,0 +1,188 @@ +package circleci + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "github.com/stretchr/testify/require" +) + +const ( + rerunWorkflowID = "22222222-3333-4444-5555-666666666666" + setupWorkflowID = "33333333-4444-5555-6666-777777777777" + olderWorkflowID = "44444444-5555-6666-7777-888888888888" +) + +// fakeWorkflow is a workflow in the fake pipeline with one approval job, approve_deploy_us, whose ID is +// "hold-" and whose status is hold. +type fakeWorkflow struct { + id, name, createdAt, hold string +} + +// fakeCircleCI serves the workflow, pipeline and job endpoints ApproveJob uses, and records what it approves. +type fakeCircleCI struct { + pipeline fakePipeline + workflows []fakeWorkflow + listing []string // the workflow IDs GET /pipeline/{id}/workflow returns, newest first + + mu sync.Mutex + approvals []string // "/" + listings int +} + +func (f *fakeCircleCI) workflow(id string) (fakeWorkflow, bool) { + for _, wf := range f.workflows { + if wf.id == id { + return wf, true + } + } + return fakeWorkflow{}, false +} + +func (f *fakeCircleCI) approved() []string { + f.mu.Lock() + defer f.mu.Unlock() + return append([]string(nil), f.approvals...) +} + +func (f *fakeCircleCI) listingCount() int { + f.mu.Lock() + defer f.mu.Unlock() + return f.listings +} + +func (f *fakeCircleCI) serveWorkflow(w http.ResponseWriter, path string) { + id, jobs := strings.CutSuffix(strings.TrimPrefix(path, "/workflow/"), "/job") + wf, ok := f.workflow(id) + switch { + case !ok: + w.WriteHeader(http.StatusNotFound) + case jobs: + writeJSON(w, map[string]interface{}{"items": []map[string]string{ + {"id": "hold-" + wf.id, "name": "approve_deploy_us", "type": "approval", "status": wf.hold}, + }}) + default: + writeJSON(w, map[string]string{ + "id": wf.id, "name": wf.name, "pipeline_id": testPipelineID, "created_at": wf.createdAt, + }) + } +} + +func (f *fakeCircleCI) servePipeline(w http.ResponseWriter, path string) { + switch path { + case "/pipeline/" + testPipelineID: + vcs := map[string]string{ + "revision": f.pipeline.revision, + "target_repository_url": f.pipeline.targetRepo, + "origin_repository_url": f.pipeline.originRepo, + } + writeJSON(w, map[string]interface{}{"id": testPipelineID, "project_slug": f.pipeline.slug, "vcs": vcs}) + case "/pipeline/" + testPipelineID + "/workflow": + f.mu.Lock() + f.listings++ + f.mu.Unlock() + items := make([]map[string]string, 0, len(f.listing)) + for _, id := range f.listing { + wf, _ := f.workflow(id) + items = append(items, map[string]string{"id": wf.id, "name": wf.name, "created_at": wf.createdAt}) + } + writeJSON(w, map[string]interface{}{"items": items}) + default: + w.WriteHeader(http.StatusNotFound) + } +} + +func (f *fakeCircleCI) client(t *testing.T) *Client { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/approve/"): + approval := strings.Replace(strings.TrimPrefix(r.URL.Path, "/workflow/"), "/approve", "", 1) + f.mu.Lock() + f.approvals = append(f.approvals, approval) + f.mu.Unlock() + writeJSON(w, map[string]string{"message": "Accepted."}) + case strings.HasPrefix(r.URL.Path, "/workflow/"): + f.serveWorkflow(w, r.URL.Path) + default: + f.servePipeline(w, r.URL.Path) + } + })) + t.Cleanup(server.Close) + client := NewClient("token") + client.BaseURL = server.URL + return client +} + +func writeJSON(w http.ResponseWriter, v interface{}) { _ = json.NewEncoder(w).Encode(v) } + +func approveForTest(client *Client) error { + return client.ApproveJob(testWorkflowID, "100", "approve_deploy_us", + ApprovalExpectation{Revision: approvedSHA, Repo: "DocSpring/docspring"}) +} + +var matchingPipeline = fakePipeline{revision: approvedSHA, targetRepo: docspringRepo, originRepo: docspringRepo} + +// CircleCI's pipeline workflow listing can leave out a workflow for a while (listing only setup-workflow). The +// stored workflow's hold job is approved without the listing. +func TestApproveJobApprovesStoredWorkflowMissingFromListing(t *testing.T) { + fake := &fakeCircleCI{ + pipeline: matchingPipeline, + workflows: []fakeWorkflow{ + {id: testWorkflowID, name: "test_build_deploy", createdAt: "2026-10-13T03:00:01Z", hold: "on_hold"}, + {id: setupWorkflowID, name: "setup-workflow", createdAt: "2026-10-13T03:00:00Z", hold: "success"}, + }, + listing: []string{setupWorkflowID}, + } + require.NoError(t, approveForTest(fake.client(t))) + require.Equal(t, []string{testWorkflowID + "/hold-" + testWorkflowID}, fake.approved()) + require.Zero(t, fake.listingCount(), "a hold job that's on hold is approved without listing the pipeline") +} + +// A rerun is a newer workflow with the same name in the same pipeline. Its hold job is approved. +func TestApproveJobApprovesTheNewestRerun(t *testing.T) { + fake := &fakeCircleCI{ + pipeline: matchingPipeline, + workflows: []fakeWorkflow{ + {id: testWorkflowID, name: "test_build_deploy", createdAt: "2026-10-13T03:00:01Z", hold: "canceled"}, + {id: rerunWorkflowID, name: "test_build_deploy", createdAt: "2026-10-13T04:00:00.123Z", hold: "on_hold"}, + {id: setupWorkflowID, name: "setup-workflow", createdAt: "2026-10-13T03:00:00Z", hold: "success"}, + }, + listing: []string{rerunWorkflowID, testWorkflowID, setupWorkflowID}, + } + require.NoError(t, approveForTest(fake.client(t))) + require.Equal(t, []string{rerunWorkflowID + "/hold-" + rerunWorkflowID}, fake.approved()) +} + +// Without a rerun, the stored workflow's hold job is approved even when it isn't on hold, so CircleCI's response +// says why it can't be. An older workflow with the same name isn't a rerun. +func TestApproveJobIgnoresOlderWorkflowsWithTheSameName(t *testing.T) { + fake := &fakeCircleCI{ + pipeline: matchingPipeline, + workflows: []fakeWorkflow{ + {id: testWorkflowID, name: "test_build_deploy", createdAt: "2026-10-13T03:00:01Z", hold: "success"}, + {id: olderWorkflowID, name: "test_build_deploy", createdAt: "2026-10-13T02:00:00Z", hold: "on_hold"}, + }, + listing: []string{olderWorkflowID}, + } + require.NoError(t, approveForTest(fake.client(t))) + require.Equal(t, []string{testWorkflowID + "/hold-" + testWorkflowID}, fake.approved()) +} + +// A rerun's pipeline is the verified pipeline, so a mismatch stops the approval before any rerun is considered. +func TestApproveJobChecksThePipelineBeforeFollowingARerun(t *testing.T) { + fake := &fakeCircleCI{ + pipeline: fakePipeline{revision: strings.Repeat("f", 40), targetRepo: docspringRepo, originRepo: docspringRepo}, + workflows: []fakeWorkflow{ + {id: testWorkflowID, name: "test_build_deploy", createdAt: "2026-10-13T03:00:01Z", hold: "canceled"}, + {id: rerunWorkflowID, name: "test_build_deploy", createdAt: "2026-10-13T04:00:00Z", hold: "on_hold"}, + }, + listing: []string{rerunWorkflowID, testWorkflowID}, + } + require.ErrorIs(t, approveForTest(fake.client(t)), ErrPipelineMismatch) + require.Empty(t, fake.approved()) +} diff --git a/internal/gateway/circleci/pipeline_verification_test.go b/internal/gateway/circleci/pipeline_verification_test.go index 06b185c6..632f8b48 100644 --- a/internal/gateway/circleci/pipeline_verification_test.go +++ b/internal/gateway/circleci/pipeline_verification_test.go @@ -1,11 +1,7 @@ package circleci import ( - "encoding/json" - "net/http" - "net/http/httptest" "strings" - "sync/atomic" "testing" "github.com/stretchr/testify/require" @@ -22,49 +18,23 @@ type fakePipeline struct { revision, targetRepo, originRepo, slug string } -// newFakeCircleCI serves the workflow/pipeline/job endpoints ApproveJob uses and counts approvals. -func newFakeCircleCI(t *testing.T, pipeline fakePipeline) (*Client, *int32) { +// newFakeCircleCI serves one workflow, testWorkflowID, whose hold job is on hold and which the pipeline's workflow +// listing includes. +func newFakeCircleCI(t *testing.T, pipeline fakePipeline) (*Client, *fakeCircleCI) { t.Helper() - var approvals int32 - write := func(w http.ResponseWriter, v interface{}) { _ = json.NewEncoder(w).Encode(v) } - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch { - case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/approve/"): - atomic.AddInt32(&approvals, 1) - write(w, map[string]string{"message": "Accepted."}) - case r.URL.Path == "/workflow/"+testWorkflowID: - write(w, map[string]string{"id": testWorkflowID, "name": "deploy", "pipeline_id": testPipelineID}) - case r.URL.Path == "/pipeline/"+testPipelineID: - vcs := map[string]string{ - "revision": pipeline.revision, - "target_repository_url": pipeline.targetRepo, - "origin_repository_url": pipeline.originRepo, - } - write(w, map[string]interface{}{"id": testPipelineID, "project_slug": pipeline.slug, "vcs": vcs}) - case r.URL.Path == "/pipeline/"+testPipelineID+"/workflow": - write(w, map[string]interface{}{"items": []map[string]string{{"id": testWorkflowID, "name": "deploy"}}}) - case r.URL.Path == "/workflow/"+testWorkflowID+"/job": - write(w, map[string]interface{}{"items": []map[string]string{ - {"id": "job-1", "name": "approve_deploy_us", "type": "approval"}, - }}) - default: - w.WriteHeader(http.StatusNotFound) - } - })) - t.Cleanup(server.Close) - client := NewClient("token") - client.BaseURL = server.URL - return client, &approvals + original := fakeWorkflow{id: testWorkflowID, name: "deploy", createdAt: "2026-10-13T03:00:00Z", hold: "on_hold"} + fake := &fakeCircleCI{pipeline: pipeline, workflows: []fakeWorkflow{original}, listing: []string{testWorkflowID}} + return fake.client(t), fake } func TestApproveJobApprovesMatchingPipeline(t *testing.T) { - client, approvals := newFakeCircleCI(t, fakePipeline{ + client, fake := newFakeCircleCI(t, fakePipeline{ revision: approvedSHA, targetRepo: docspringRepo, originRepo: docspringRepo, slug: "gh/DocSpring/docspring", }) err := client.ApproveJob(testWorkflowID, "100", "approve_deploy_us", ApprovalExpectation{Revision: approvedSHA, Repo: "DocSpring/docspring"}) require.NoError(t, err) - require.EqualValues(t, 1, *approvals) + require.Equal(t, []string{testWorkflowID + "/hold-" + testWorkflowID}, fake.approved()) } func TestApproveJobRefusesMismatchedPipelines(t *testing.T) { @@ -86,22 +56,22 @@ func TestApproveJobRefusesMismatchedPipelines(t *testing.T) { } for name, pipeline := range cases { t.Run(name, func(t *testing.T) { - client, approvals := newFakeCircleCI(t, pipeline) + client, fake := newFakeCircleCI(t, pipeline) err := client.ApproveJob(testWorkflowID, "100", "approve_deploy_us", ApprovalExpectation{Revision: approvedSHA, Repo: "DocSpring/docspring"}) require.ErrorIs(t, err, ErrPipelineMismatch) - require.EqualValues(t, 0, *approvals, "nothing may be approved on a mismatch") + require.Empty(t, fake.approved(), "nothing may be approved on a mismatch") }) } } func TestApproveJobRejectsNonUUIDWorkflowID(t *testing.T) { - client, approvals := newFakeCircleCI(t, fakePipeline{revision: approvedSHA}) + client, fake := newFakeCircleCI(t, fakePipeline{revision: approvedSHA}) for _, id := range []string{"test-workflow-1", "../pipeline/x", ""} { err := client.ApproveJob(id, "100", "approve_deploy_us", ApprovalExpectation{Revision: approvedSHA}) require.ErrorIs(t, err, ErrInvalidWorkflowID) } - require.EqualValues(t, 0, *approvals) + require.Empty(t, fake.approved()) } func TestParseMetadataRequiresUUIDWorkflowID(t *testing.T) {