From aa6ad7fd9a93d0635500023cb99bf5fbed58594a Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Fri, 20 Mar 2026 12:42:01 +0100 Subject: [PATCH 1/4] feat(attestation): expand PR reviewers with requested status and review state Add Requested and ReviewStatus fields to prinfo.Reviewer to track which reviewers were explicitly requested vs who submitted reviews. For GitHub PRs, merge requested reviewer lists from both event payload and API endpoint, then overlay review states from /reviews API. For GitLab, mark all API-retrieved reviewers as requested. Bump PR info schema to v1.2. Signed-off-by: Javier Rodriguez --- internal/prinfo/prinfo.go | 8 +- internal/prinfo/prinfo_test.go | 78 +++++ internal/prinfo/schemas/generate.go | 6 +- .../prinfo/pr-info-1.2.schema.json | 103 +++++++ internal/schemavalidators/schemavalidators.go | 8 +- .../crafter/materials/chainloop_pr_info.go | 4 +- .../materials/chainloop_pr_info_test.go | 2 +- pkg/attestation/crafter/prmetadata.go | 189 +++++++++++- pkg/attestation/crafter/prmetadata_test.go | 279 +++++++++++++++++- 9 files changed, 656 insertions(+), 21 deletions(-) create mode 100644 internal/schemavalidators/internal_schemas/prinfo/pr-info-1.2.schema.json diff --git a/internal/prinfo/prinfo.go b/internal/prinfo/prinfo.go index a1f79d7e5..2d9e0ed42 100644 --- a/internal/prinfo/prinfo.go +++ b/internal/prinfo/prinfo.go @@ -19,13 +19,15 @@ const ( // EvidenceID is the identifier for the PR/MR info material type EvidenceID = "CHAINLOOP_PR_INFO" // EvidenceSchemaURL is the URL to the JSON schema for PR/MR info - EvidenceSchemaURL = "https://schemas.chainloop.dev/prinfo/1.1/pr-info.schema.json" + EvidenceSchemaURL = "https://schemas.chainloop.dev/prinfo/1.2/pr-info.schema.json" ) // Reviewer represents a reviewer of the PR/MR type Reviewer struct { - Login string `json:"login" jsonschema:"required,description=Username of the reviewer"` - Type string `json:"type" jsonschema:"required,enum=User,enum=Bot,enum=unknown,description=Account type of the reviewer"` + Login string `json:"login" jsonschema:"required,description=Username of the reviewer"` + Type string `json:"type" jsonschema:"required,enum=User,enum=Bot,enum=unknown,description=Account type of the reviewer"` + Requested bool `json:"requested" jsonschema:"description=Whether the reviewer was explicitly requested to review"` + ReviewStatus string `json:"review_status,omitempty" jsonschema:"enum=APPROVED,enum=CHANGES_REQUESTED,enum=COMMENTED,enum=DISMISSED,enum=PENDING,description=The reviewer's current review state if they have submitted a review"` } // Data represents the data payload of the PR/MR info evidence diff --git a/internal/prinfo/prinfo_test.go b/internal/prinfo/prinfo_test.go index cfdf1e78b..e6fa78ddc 100644 --- a/internal/prinfo/prinfo_test.go +++ b/internal/prinfo/prinfo_test.go @@ -185,6 +185,84 @@ func TestValidatePRInfo(t *testing.T) { } } +func TestValidatePRInfoV1_2(t *testing.T) { + testCases := []struct { + name string + data string + wantErr bool + }{ + { + name: "valid reviewer with requested and review_status", + data: `{ + "platform": "github", + "type": "pull_request", + "number": "789", + "url": "https://github.com/owner/repo/pull/789", + "reviewers": [ + {"login": "reviewer1", "type": "User", "requested": true, "review_status": "COMMENTED"}, + {"login": "reviewer2", "type": "User", "requested": false, "review_status": "APPROVED"}, + {"login": "coderabbitai", "type": "Bot", "requested": true} + ] + }`, + wantErr: false, + }, + { + name: "backward compat: reviewer without new fields is still valid", + data: `{ + "platform": "github", + "type": "pull_request", + "number": "789", + "url": "https://github.com/owner/repo/pull/789", + "reviewers": [ + {"login": "reviewer1", "type": "User"} + ] + }`, + wantErr: false, + }, + { + name: "invalid review_status value", + data: `{ + "platform": "github", + "type": "pull_request", + "number": "789", + "url": "https://github.com/owner/repo/pull/789", + "reviewers": [ + {"login": "reviewer1", "type": "User", "review_status": "UNKNOWN_STATE"} + ] + }`, + wantErr: true, + }, + { + name: "additional property in reviewer not allowed", + data: `{ + "platform": "github", + "type": "pull_request", + "number": "789", + "url": "https://github.com/owner/repo/pull/789", + "reviewers": [ + {"login": "reviewer1", "type": "User", "extra": "not allowed"} + ] + }`, + wantErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var data interface{} + err := json.Unmarshal([]byte(tc.data), &data) + require.NoError(t, err) + + err = schemavalidators.ValidatePRInfo(data, schemavalidators.PRInfoVersion1_2) + if tc.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + func TestValidatePRInfoV1_0BackwardCompat(t *testing.T) { testCases := []struct { name string diff --git a/internal/prinfo/schemas/generate.go b/internal/prinfo/schemas/generate.go index dfb21d03c..c4267bd47 100644 --- a/internal/prinfo/schemas/generate.go +++ b/internal/prinfo/schemas/generate.go @@ -1,5 +1,5 @@ // -// Copyright 2025 The Chainloop Authors. +// Copyright 2025-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -23,13 +23,13 @@ import ( "github.com/chainloop-dev/chainloop/internal/prinfo" ) -//go:generate go run ./generate.go --output-dir ../../../internal/schemavalidators/internal_schemas/prinfo --version 1.1 +//go:generate go run ./generate.go --output-dir ../../../internal/schemavalidators/internal_schemas/prinfo --version 1.2 func main() { var outputDir string var version string flag.StringVar(&outputDir, "output-dir", "../../../internal/schemavalidators/internal_schemas/prinfo", "Directory to output the schema files") - flag.StringVar(&version, "version", "1.1", "Schema version") + flag.StringVar(&version, "version", "1.2", "Schema version") flag.Parse() generator := prinfo.NewGenerator() diff --git a/internal/schemavalidators/internal_schemas/prinfo/pr-info-1.2.schema.json b/internal/schemavalidators/internal_schemas/prinfo/pr-info-1.2.schema.json new file mode 100644 index 000000000..095fe4411 --- /dev/null +++ b/internal/schemavalidators/internal_schemas/prinfo/pr-info-1.2.schema.json @@ -0,0 +1,103 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "$id": "https://schemas.chainloop.dev/prinfo/1.2/pr-info.schema.json", + "properties": { + "platform": { + "type": "string", + "enum": [ + "github", + "gitlab" + ], + "description": "The CI/CD platform" + }, + "type": { + "type": "string", + "enum": [ + "pull_request", + "merge_request" + ], + "description": "The type of change request" + }, + "number": { + "type": "string", + "description": "The PR/MR number or identifier" + }, + "title": { + "type": "string", + "description": "The PR/MR title" + }, + "description": { + "type": "string", + "description": "The PR/MR description or body" + }, + "source_branch": { + "type": "string", + "description": "The source branch name" + }, + "target_branch": { + "type": "string", + "description": "The target branch name" + }, + "url": { + "type": "string", + "format": "uri", + "description": "Direct URL to the PR/MR" + }, + "author": { + "type": "string", + "description": "Username of the PR/MR author" + }, + "reviewers": { + "items": { + "properties": { + "login": { + "type": "string", + "description": "Username of the reviewer" + }, + "type": { + "type": "string", + "enum": [ + "User", + "Bot", + "unknown" + ], + "description": "Account type of the reviewer" + }, + "requested": { + "type": "boolean", + "description": "Whether the reviewer was explicitly requested to review" + }, + "review_status": { + "type": "string", + "enum": [ + "APPROVED", + "CHANGES_REQUESTED", + "COMMENTED", + "DISMISSED", + "PENDING" + ], + "description": "The reviewer's current review state if they have submitted a review" + } + }, + "additionalProperties": false, + "type": "object", + "required": [ + "login", + "type" + ] + }, + "type": "array", + "description": "List of reviewers who reviewed or were requested to review" + } + }, + "additionalProperties": false, + "type": "object", + "required": [ + "platform", + "type", + "number", + "url" + ], + "title": "Pull Request / Merge Request Information", + "description": "Schema for Pull Request or Merge Request metadata collected during attestation" +} diff --git a/internal/schemavalidators/schemavalidators.go b/internal/schemavalidators/schemavalidators.go index 1356f6da7..e95473c6d 100644 --- a/internal/schemavalidators/schemavalidators.go +++ b/internal/schemavalidators/schemavalidators.go @@ -50,6 +50,8 @@ const ( PRInfoVersion1_0 PRInfoVersion = "1.0" // PRInfoVersion1_1 represents PR/MR Info version 1.1 schema (adds reviewers). PRInfoVersion1_1 PRInfoVersion = "1.1" + // PRInfoVersion1_2 represents PR/MR Info version 1.2 schema (adds requested and review_status to reviewers). + PRInfoVersion1_2 PRInfoVersion = "1.2" // CycloneDXVersion1_5 represents CycloneDX version 1.5 schema. CycloneDXVersion1_5 CycloneDXVersion = "1.5" // CycloneDXVersion1_6 represents CycloneDX version 1.6 schema. @@ -96,6 +98,8 @@ var ( prInfoSpecVersion1_0 string //go:embed internal_schemas/prinfo/pr-info-1.1.schema.json prInfoSpecVersion1_1 string + //go:embed internal_schemas/prinfo/pr-info-1.2.schema.json + prInfoSpecVersion1_2 string // AI Agent Config schemas //go:embed internal_schemas/aiagentconfig/ai-agent-config-0.1.schema.json @@ -120,6 +124,7 @@ var schemaURLMapping = map[string]string{ "https://chainloop.dev/schemas/runner-context-response-0.1.schema.json": runnerContextSpecVersion0_1, "https://schemas.chainloop.dev/prinfo/1.0/pr-info.schema.json": prInfoSpecVersion1_0, "https://schemas.chainloop.dev/prinfo/1.1/pr-info.schema.json": prInfoSpecVersion1_1, + "https://schemas.chainloop.dev/prinfo/1.2/pr-info.schema.json": prInfoSpecVersion1_2, "https://schemas.chainloop.dev/aiagentconfig/0.1/ai-agent-config.schema.json": aiAgentConfigSpecVersion0_1, } @@ -149,6 +154,7 @@ func init() { compiledPRInfoSchemas = make(map[PRInfoVersion]*jsonschema.Schema) compiledPRInfoSchemas[PRInfoVersion1_0] = compiler.MustCompile("https://schemas.chainloop.dev/prinfo/1.0/pr-info.schema.json") compiledPRInfoSchemas[PRInfoVersion1_1] = compiler.MustCompile("https://schemas.chainloop.dev/prinfo/1.1/pr-info.schema.json") + compiledPRInfoSchemas[PRInfoVersion1_2] = compiler.MustCompile("https://schemas.chainloop.dev/prinfo/1.2/pr-info.schema.json") compiledAIAgentConfigSchemas = make(map[AIAgentConfigVersion]*jsonschema.Schema) compiledAIAgentConfigSchemas[AIAgentConfigVersion0_1] = compiler.MustCompile("https://schemas.chainloop.dev/aiagentconfig/0.1/ai-agent-config.schema.json") @@ -246,7 +252,7 @@ func ValidateChainloopRunnerContext(data interface{}, version RunnerContextVersi // ValidatePRInfo validates the PR/MR info schema. func ValidatePRInfo(data interface{}, version PRInfoVersion) error { if version == "" { - version = PRInfoVersion1_1 + version = PRInfoVersion1_2 } schema, ok := compiledPRInfoSchemas[version] diff --git a/pkg/attestation/crafter/materials/chainloop_pr_info.go b/pkg/attestation/crafter/materials/chainloop_pr_info.go index 552a2b124..f13759fd0 100644 --- a/pkg/attestation/crafter/materials/chainloop_pr_info.go +++ b/pkg/attestation/crafter/materials/chainloop_pr_info.go @@ -1,5 +1,5 @@ // -// Copyright 2025 The Chainloop Authors. +// Copyright 2025-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -75,7 +75,7 @@ func (i *ChainloopPRInfoCrafter) Craft(ctx context.Context, artifactPath string) } // Validate the data against JSON schema - if err := schemavalidators.ValidatePRInfo(rawData, schemavalidators.PRInfoVersion1_1); err != nil { + if err := schemavalidators.ValidatePRInfo(rawData, schemavalidators.PRInfoVersion1_2); err != nil { i.logger.Debug().Err(err).Msg("schema validation failed") return nil, fmt.Errorf("PR info validation failed: %w", err) } diff --git a/pkg/attestation/crafter/materials/chainloop_pr_info_test.go b/pkg/attestation/crafter/materials/chainloop_pr_info_test.go index 339b4498d..6daebb3cd 100644 --- a/pkg/attestation/crafter/materials/chainloop_pr_info_test.go +++ b/pkg/attestation/crafter/materials/chainloop_pr_info_test.go @@ -118,7 +118,7 @@ func TestChainloopPRInfoCrafter_Validation(t *testing.T) { require.NoError(t, err) // Validate the data against JSON schema - err = schemavalidators.ValidatePRInfo(rawData, schemavalidators.PRInfoVersion1_1) + err = schemavalidators.ValidatePRInfo(rawData, schemavalidators.PRInfoVersion1_2) if tc.wantErr { require.Error(t, err) diff --git a/pkg/attestation/crafter/prmetadata.go b/pkg/attestation/crafter/prmetadata.go index dec0e1f8f..df95aa83d 100644 --- a/pkg/attestation/crafter/prmetadata.go +++ b/pkg/attestation/crafter/prmetadata.go @@ -59,14 +59,14 @@ func DetectPRContext(ctx context.Context, runner SupportedRunner) (bool, *PRMeta switch runner.ID() { case schemaapi.CraftingSchema_Runner_GITHUB_ACTION: - return extractGitHubPRMetadata(envVars) + return extractGitHubPRMetadata(ctx, envVars) case schemaapi.CraftingSchema_Runner_GITLAB_PIPELINE: return extractGitLabMRMetadata(ctx, envVars) case schemaapi.CraftingSchema_Runner_DAGGER_PIPELINE: // When running in Dagger, check for parent CI context passed through as env vars // Try Github first if envVars["GITHUB_EVENT_NAME"] != "" { - return extractGitHubPRMetadata(envVars) + return extractGitHubPRMetadata(ctx, envVars) } // Then try Gitlab if envVars["CI_PIPELINE_SOURCE"] != "" { @@ -79,7 +79,7 @@ func DetectPRContext(ctx context.Context, runner SupportedRunner) (bool, *PRMeta } // extractGitHubPRMetadata reads GITHUB_EVENT_PATH JSON and extracts PR metadata -func extractGitHubPRMetadata(envVars map[string]string) (bool, *PRMetadata, error) { +func extractGitHubPRMetadata(ctx context.Context, envVars map[string]string) (bool, *PRMetadata, error) { eventName := envVars["GITHUB_EVENT_NAME"] // Check if this is a pull request event if eventName != "pull_request" && eventName != "pull_request_target" { @@ -118,18 +118,56 @@ func extractGitHubPRMetadata(envVars map[string]string) (bool, *PRMetadata, erro return false, nil, fmt.Errorf("failed to parse event JSON: %w", err) } + // GITHUB_TOKEN is read via os.Getenv to avoid persisting it in the attestation envVars map. + // GITHUB_API_BASE_URL can be overridden (e.g. in tests); defaults to api.github.com. + parts := splitOwnerRepo(envVars["GITHUB_REPOSITORY"]) + owner, repo := parts[0], parts[1] + prNumber := fmt.Sprintf("%d", event.PullRequest.Number) + githubAPIBase := os.Getenv("GITHUB_API_BASE_URL") + if githubAPIBase == "" { + githubAPIBase = "https://api.github.com" + } + token := os.Getenv("GITHUB_TOKEN") + + // Seed the reviewer map from both the event payload and the API requested_reviewers endpoint. + // Both sources mark Requested: true. The event file is always available (no token needed), + // while the API may surface reviewers not yet reflected in the event (e.g. added after dispatch). + reviewerMap := make(map[string]int) // login → index in reviewers slice var reviewers []prinfo.Reviewer + for _, r := range event.PullRequest.RequestedReviewers { + if _, exists := reviewerMap[r.Login]; exists { + continue + } reviewerType := r.Type if reviewerType == "" { reviewerType = "unknown" } + reviewerMap[r.Login] = len(reviewers) reviewers = append(reviewers, prinfo.Reviewer{ - Login: r.Login, - Type: reviewerType, + Login: r.Login, + Type: reviewerType, + Requested: true, }) } + for _, r := range fetchGitHubRequestedReviewers(ctx, githubAPIBase, owner, repo, prNumber, token) { + if _, exists := reviewerMap[r.Login]; !exists { + reviewerMap[r.Login] = len(reviewers) + reviewers = append(reviewers, r) + } + } + + // Merge review activity: update status for existing entries, add new ones with Requested: false. + for _, r := range fetchGitHubReviews(ctx, githubAPIBase, owner, repo, prNumber, token) { + if idx, exists := reviewerMap[r.Login]; exists { + reviewers[idx].ReviewStatus = r.ReviewStatus + } else { + reviewerMap[r.Login] = len(reviewers) + reviewers = append(reviewers, r) + } + } + metadata := &PRMetadata{ Platform: "github", Type: "pull_request", @@ -146,6 +184,142 @@ func extractGitHubPRMetadata(envVars map[string]string) (bool, *PRMetadata, erro return true, metadata, nil } +// splitOwnerRepo splits "owner/repo" into [owner, repo]. +// Returns ["", ""] if the string does not contain a slash. +func splitOwnerRepo(ownerRepo string) [2]string { + for i := 0; i < len(ownerRepo); i++ { + if ownerRepo[i] == '/' { + return [2]string{ownerRepo[:i], ownerRepo[i+1:]} + } + } + return [2]string{"", ""} +} + +// fetchGitHubRequestedReviewers fetches the list of users explicitly requested to review a PR. +// Returns nil on any failure (best-effort). +// baseURL is the GitHub API base (e.g. "https://api.github.com"); can be overridden in tests. +func fetchGitHubRequestedReviewers(ctx context.Context, baseURL, owner, repo, prNumber, token string) []prinfo.Reviewer { + if baseURL == "" || owner == "" || repo == "" || prNumber == "" || token == "" { + return nil + } + + apiURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%s/requested_reviewers", baseURL, owner, repo, prNumber) + + client := &http.Client{Timeout: 10 * time.Second} + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, apiURL, nil) + if err != nil { + return nil + } + req.Header.Set("Authorization", "Bearer "+token) + + resp, err := client.Do(req) + if err != nil { + return nil + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return nil + } + + var result struct { + Users []struct { + Login string `json:"login"` + Type string `json:"type"` + } `json:"users"` + } + if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { + return nil + } + + var reviewers []prinfo.Reviewer + for _, u := range result.Users { + reviewerType := u.Type + if reviewerType == "" { + reviewerType = "unknown" + } + reviewers = append(reviewers, prinfo.Reviewer{ + Login: u.Login, + Type: reviewerType, + Requested: true, + }) + } + + if len(reviewers) == 0 { + return nil + } + return reviewers +} + +// fetchGitHubReviews fetches PR reviews from the GitHub API. +// Returns nil on any failure (best-effort). +// Reviews are deduplicated by login, keeping the most recent state (last entry wins). +// Returned reviewers have Requested: false; callers should set Requested: true for those +// already present in the requested_reviewers list. +// baseURL is the GitHub API base (e.g. "https://api.github.com"); can be overridden in tests. +func fetchGitHubReviews(ctx context.Context, baseURL, owner, repo, prNumber, token string) []prinfo.Reviewer { + if baseURL == "" || owner == "" || repo == "" || prNumber == "" || token == "" { + return nil + } + + apiURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%s/reviews", baseURL, owner, repo, prNumber) + + client := &http.Client{Timeout: 10 * time.Second} + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, apiURL, nil) + if err != nil { + return nil + } + req.Header.Set("Authorization", "Bearer "+token) + + resp, err := client.Do(req) + if err != nil { + return nil + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return nil + } + + var reviews []struct { + User struct { + Login string `json:"login"` + Type string `json:"type"` + } `json:"user"` + State string `json:"state"` + } + if err := json.NewDecoder(resp.Body).Decode(&reviews); err != nil { + return nil + } + + // Deduplicate by login, keeping insertion order but updating to the most recent state. + seen := make(map[string]int) // login → index + var reviewers []prinfo.Reviewer + for _, r := range reviews { + reviewerType := r.User.Type + if reviewerType == "" { + reviewerType = "unknown" + } + if idx, exists := seen[r.User.Login]; exists { + reviewers[idx].ReviewStatus = r.State + continue + } + seen[r.User.Login] = len(reviewers) + reviewers = append(reviewers, prinfo.Reviewer{ + Login: r.User.Login, + Type: reviewerType, + ReviewStatus: r.State, + }) + } + + if len(reviewers) == 0 { + return nil + } + return reviewers +} + // extractGitLabMRMetadata extracts from GitLab environment variables func extractGitLabMRMetadata(ctx context.Context, envVars map[string]string) (bool, *PRMetadata, error) { pipelineSource := envVars["CI_PIPELINE_SOURCE"] @@ -228,8 +402,9 @@ func fetchGitLabReviewers(ctx context.Context, baseURL, projectPath, mrIID, toke var reviewers []prinfo.Reviewer for _, r := range mrResponse.Reviewers { reviewers = append(reviewers, prinfo.Reviewer{ - Login: r.Username, - Type: "unknown", + Login: r.Username, + Type: "unknown", + Requested: true, }) } diff --git a/pkg/attestation/crafter/prmetadata_test.go b/pkg/attestation/crafter/prmetadata_test.go index 9a32d9df7..447687b69 100644 --- a/pkg/attestation/crafter/prmetadata_test.go +++ b/pkg/attestation/crafter/prmetadata_test.go @@ -38,7 +38,7 @@ func TestExtractGitHubPRMetadata(t *testing.T) { validate func(t *testing.T, metadata *PRMetadata) }{ { - name: "valid pull_request event with reviewers", + name: "valid pull_request event with reviewers from event file", envVars: map[string]string{ "GITHUB_EVENT_NAME": "pull_request", "GITHUB_EVENT_PATH": filepath.Join("testdata", "github_pr_event.json"), @@ -57,11 +57,14 @@ func TestExtractGitHubPRMetadata(t *testing.T) { assert.Equal(t, "main", metadata.TargetBranch) assert.Equal(t, "https://github.com/owner/repo/pull/123", metadata.URL) assert.Equal(t, "johndoe", metadata.Author) + // Event file provides requested_reviewers without needing a token. require.Len(t, metadata.Reviewers, 2) assert.Equal(t, "reviewer1", metadata.Reviewers[0].Login) assert.Equal(t, "User", metadata.Reviewers[0].Type) + assert.True(t, metadata.Reviewers[0].Requested) assert.Equal(t, "coderabbitai", metadata.Reviewers[1].Login) assert.Equal(t, "Bot", metadata.Reviewers[1].Type) + assert.True(t, metadata.Reviewers[1].Requested) }, }, { @@ -112,7 +115,7 @@ func TestExtractGitHubPRMetadata(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - isPR, metadata, err := extractGitHubPRMetadata(tc.envVars) + isPR, metadata, err := extractGitHubPRMetadata(context.Background(), tc.envVars) if tc.expectError { require.Error(t, err) @@ -225,8 +228,8 @@ func TestFetchGitLabReviewers(t *testing.T) { mrIID: "10", token: "test-token", expected: []prinfo.Reviewer{ - {Login: "alice", Type: "unknown"}, - {Login: "bot-reviewer", Type: "unknown"}, + {Login: "alice", Type: "unknown", Requested: true}, + {Login: "bot-reviewer", Type: "unknown", Requested: true}, }, }, { @@ -334,5 +337,273 @@ func TestExtractGitLabMRMetadataWithReviewers(t *testing.T) { require.Len(t, metadata.Reviewers, 2) assert.Equal(t, "alice", metadata.Reviewers[0].Login) assert.Equal(t, "unknown", metadata.Reviewers[0].Type) + assert.True(t, metadata.Reviewers[0].Requested) assert.Equal(t, "coderabbitai", metadata.Reviewers[1].Login) + assert.True(t, metadata.Reviewers[1].Requested) +} + +func TestFetchGitHubReviews(t *testing.T) { + testCases := []struct { + name string + handler http.HandlerFunc + owner string + repo string + prNumber string + token string + expected []prinfo.Reviewer + }{ + { + name: "successful response, last state wins for duplicate user", + handler: func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "Bearer test-token", r.Header.Get("Authorization")) + w.Header().Set("Content-Type", "application/json") + // reviewer1 submits two reviews with different states; last state wins + fmt.Fprint(w, `[{"user":{"login":"reviewer1","type":"User"},"state":"COMMENTED"},{"user":{"login":"reviewer1","type":"User"},"state":"APPROVED"},{"user":{"login":"bot-reviewer","type":"Bot"},"state":"COMMENTED"}]`) + }, + owner: "owner", + repo: "repo", + prNumber: "42", + token: "test-token", + expected: []prinfo.Reviewer{ + {Login: "reviewer1", Type: "User", ReviewStatus: "APPROVED"}, + {Login: "bot-reviewer", Type: "Bot", ReviewStatus: "COMMENTED"}, + }, + }, + { + name: "empty reviews array", + handler: func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `[]`) + }, + owner: "owner", + repo: "repo", + prNumber: "42", + token: "test-token", + expected: nil, + }, + { + name: "API returns error", + handler: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + }, + owner: "owner", + repo: "repo", + prNumber: "42", + token: "test-token", + expected: nil, + }, + { + name: "missing token", + handler: nil, + owner: "owner", + repo: "repo", + prNumber: "42", + token: "", + expected: nil, + }, + { + name: "missing owner", + handler: nil, + owner: "", + repo: "repo", + prNumber: "42", + token: "test-token", + expected: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var baseURL string + if tc.handler != nil { + server := httptest.NewServer(tc.handler) + defer server.Close() + baseURL = server.URL + } + + reviewers := fetchGitHubReviews(context.Background(), baseURL, tc.owner, tc.repo, tc.prNumber, tc.token) + + if tc.expected == nil { + assert.Nil(t, reviewers) + } else { + assert.Equal(t, tc.expected, reviewers) + } + }) + } +} + +func TestFetchGitHubReviewsRequestPath(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "/repos/owner/repo/pulls/42/reviews", r.URL.Path) + assert.Equal(t, "Bearer test-token", r.Header.Get("Authorization")) + w.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(w).Encode([]any{})) + })) + defer server.Close() + + fetchGitHubReviews(context.Background(), server.URL, "owner", "repo", "42", "test-token") +} + +func TestFetchGitHubRequestedReviewers(t *testing.T) { + testCases := []struct { + name string + handler http.HandlerFunc + owner string + repo string + prNumber string + token string + expected []prinfo.Reviewer + }{ + { + name: "successful response with users", + handler: func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "Bearer test-token", r.Header.Get("Authorization")) + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"users":[{"login":"reviewer1","type":"User"},{"login":"copilot-pull-request-reviewer[bot]","type":"Bot"}],"teams":[]}`) + }, + owner: "owner", + repo: "repo", + prNumber: "42", + token: "test-token", + expected: []prinfo.Reviewer{ + {Login: "reviewer1", Type: "User", Requested: true}, + {Login: "copilot-pull-request-reviewer[bot]", Type: "Bot", Requested: true}, + }, + }, + { + name: "empty users array", + handler: func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"users":[],"teams":[]}`) + }, + owner: "owner", + repo: "repo", + prNumber: "42", + token: "test-token", + expected: nil, + }, + { + name: "API returns error", + handler: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + }, + owner: "owner", + repo: "repo", + prNumber: "42", + token: "test-token", + expected: nil, + }, + { + name: "missing token", + handler: nil, + owner: "owner", + repo: "repo", + prNumber: "42", + token: "", + expected: nil, + }, + { + name: "missing owner", + handler: nil, + owner: "", + repo: "repo", + prNumber: "42", + token: "test-token", + expected: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var baseURL string + if tc.handler != nil { + server := httptest.NewServer(tc.handler) + defer server.Close() + baseURL = server.URL + } + + reviewers := fetchGitHubRequestedReviewers(context.Background(), baseURL, tc.owner, tc.repo, tc.prNumber, tc.token) + + if tc.expected == nil { + assert.Nil(t, reviewers) + } else { + assert.Equal(t, tc.expected, reviewers) + } + }) + } +} + +func TestFetchGitHubRequestedReviewersRequestPath(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "/repos/owner/repo/pulls/42/requested_reviewers", r.URL.Path) + assert.Equal(t, "Bearer test-token", r.Header.Get("Authorization")) + w.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(w).Encode(map[string]any{"users": []any{}, "teams": []any{}})) + })) + defer server.Close() + + fetchGitHubRequestedReviewers(context.Background(), server.URL, "owner", "repo", "42", "test-token") +} + +func TestExtractGitHubPRMetadataWithAPIReviews(t *testing.T) { + // Mock server handles both /requested_reviewers and /reviews endpoints. + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/repos/owner/repo/pulls/123/requested_reviewers": + assert.Equal(t, "Bearer test-token", r.Header.Get("Authorization")) + // reviewer1 and coderabbitai are already in the event file; newreviewer is only in the API. + // The event file entry for reviewer1/coderabbitai wins on dedup (inserted first). + fmt.Fprint(w, `{"users":[{"login":"reviewer1","type":"User"},{"login":"coderabbitai","type":"Bot"},{"login":"newreviewer","type":"User"}],"teams":[]}`) + case "/repos/owner/repo/pulls/123/reviews": + assert.Equal(t, "Bearer test-token", r.Header.Get("Authorization")) + // reviewer2 has reviewed (not in requested); reviewer1 has also reviewed. + fmt.Fprint(w, `[{"user":{"login":"reviewer2","type":"User"},"state":"APPROVED"},{"user":{"login":"reviewer1","type":"User"},"state":"COMMENTED"}]`) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + t.Setenv("GITHUB_TOKEN", "test-token") + t.Setenv("GITHUB_API_BASE_URL", server.URL) + + envVars := map[string]string{ + "GITHUB_EVENT_NAME": "pull_request", + // github_pr_event.json has requested_reviewers: reviewer1 (User), coderabbitai (Bot) + "GITHUB_EVENT_PATH": filepath.Join("testdata", "github_pr_event.json"), + "GITHUB_HEAD_REF": "feature-branch", + "GITHUB_BASE_REF": "main", + "GITHUB_REPOSITORY": "owner/repo", + } + + isPR, metadata, err := extractGitHubPRMetadata(context.Background(), envVars) + require.NoError(t, err) + require.True(t, isPR) + + // reviewer1: event file + API requested + has reviewed → Requested: true, ReviewStatus: COMMENTED + // coderabbitai: event file + API requested, no review → Requested: true, ReviewStatus: "" + // newreviewer: API requested only, no review → Requested: true, ReviewStatus: "" + // reviewer2: not requested, has reviewed → Requested: false, ReviewStatus: APPROVED + require.Len(t, metadata.Reviewers, 4) + + assert.Equal(t, "reviewer1", metadata.Reviewers[0].Login) + assert.Equal(t, "User", metadata.Reviewers[0].Type) + assert.True(t, metadata.Reviewers[0].Requested) + assert.Equal(t, "COMMENTED", metadata.Reviewers[0].ReviewStatus) + + assert.Equal(t, "coderabbitai", metadata.Reviewers[1].Login) + assert.Equal(t, "Bot", metadata.Reviewers[1].Type) + assert.True(t, metadata.Reviewers[1].Requested) + assert.Equal(t, "", metadata.Reviewers[1].ReviewStatus) + + assert.Equal(t, "newreviewer", metadata.Reviewers[2].Login) + assert.Equal(t, "User", metadata.Reviewers[2].Type) + assert.True(t, metadata.Reviewers[2].Requested) + assert.Equal(t, "", metadata.Reviewers[2].ReviewStatus) + + assert.Equal(t, "reviewer2", metadata.Reviewers[3].Login) + assert.Equal(t, "User", metadata.Reviewers[3].Type) + assert.False(t, metadata.Reviewers[3].Requested) + assert.Equal(t, "APPROVED", metadata.Reviewers[3].ReviewStatus) } From 4ba1356e89e2dc78854bb66c29ba2278e8976c73 Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Fri, 20 Mar 2026 12:56:48 +0100 Subject: [PATCH 2/4] fix(attestation): paginate GitHub reviews API and make requested field required in schema 1.2 Signed-off-by: Javier Rodriguez --- internal/prinfo/prinfo.go | 2 +- internal/prinfo/prinfo_test.go | 4 +- .../prinfo/pr-info-1.2.schema.json | 3 +- pkg/attestation/crafter/prmetadata.go | 109 +++++++++++------- pkg/attestation/crafter/prmetadata_test.go | 71 ++++++++++++ 5 files changed, 143 insertions(+), 46 deletions(-) diff --git a/internal/prinfo/prinfo.go b/internal/prinfo/prinfo.go index 2d9e0ed42..0c69efb30 100644 --- a/internal/prinfo/prinfo.go +++ b/internal/prinfo/prinfo.go @@ -26,7 +26,7 @@ const ( type Reviewer struct { Login string `json:"login" jsonschema:"required,description=Username of the reviewer"` Type string `json:"type" jsonschema:"required,enum=User,enum=Bot,enum=unknown,description=Account type of the reviewer"` - Requested bool `json:"requested" jsonschema:"description=Whether the reviewer was explicitly requested to review"` + Requested bool `json:"requested" jsonschema:"required,description=Whether the reviewer was explicitly requested to review"` ReviewStatus string `json:"review_status,omitempty" jsonschema:"enum=APPROVED,enum=CHANGES_REQUESTED,enum=COMMENTED,enum=DISMISSED,enum=PENDING,description=The reviewer's current review state if they have submitted a review"` } diff --git a/internal/prinfo/prinfo_test.go b/internal/prinfo/prinfo_test.go index e6fa78ddc..d3f206730 100644 --- a/internal/prinfo/prinfo_test.go +++ b/internal/prinfo/prinfo_test.go @@ -207,7 +207,7 @@ func TestValidatePRInfoV1_2(t *testing.T) { wantErr: false, }, { - name: "backward compat: reviewer without new fields is still valid", + name: "reviewer missing required requested field fails validation", data: `{ "platform": "github", "type": "pull_request", @@ -217,7 +217,7 @@ func TestValidatePRInfoV1_2(t *testing.T) { {"login": "reviewer1", "type": "User"} ] }`, - wantErr: false, + wantErr: true, }, { name: "invalid review_status value", diff --git a/internal/schemavalidators/internal_schemas/prinfo/pr-info-1.2.schema.json b/internal/schemavalidators/internal_schemas/prinfo/pr-info-1.2.schema.json index 095fe4411..96e31b1ee 100644 --- a/internal/schemavalidators/internal_schemas/prinfo/pr-info-1.2.schema.json +++ b/internal/schemavalidators/internal_schemas/prinfo/pr-info-1.2.schema.json @@ -83,7 +83,8 @@ "type": "object", "required": [ "login", - "type" + "type", + "requested" ] }, "type": "array", diff --git a/pkg/attestation/crafter/prmetadata.go b/pkg/attestation/crafter/prmetadata.go index df95aa83d..bcaace587 100644 --- a/pkg/attestation/crafter/prmetadata.go +++ b/pkg/attestation/crafter/prmetadata.go @@ -22,6 +22,7 @@ import ( "net/http" "net/url" "os" + "strings" "time" schemaapi "github.com/chainloop-dev/chainloop/app/controlplane/api/workflowcontract/v1" @@ -252,7 +253,7 @@ func fetchGitHubRequestedReviewers(ctx context.Context, baseURL, owner, repo, pr return reviewers } -// fetchGitHubReviews fetches PR reviews from the GitHub API. +// fetchGitHubReviews fetches all PR reviews from the GitHub API, following pagination. // Returns nil on any failure (best-effort). // Reviews are deduplicated by login, keeping the most recent state (last entry wins). // Returned reviewers have Requested: false; callers should set Requested: true for those @@ -263,55 +264,60 @@ func fetchGitHubReviews(ctx context.Context, baseURL, owner, repo, prNumber, tok return nil } - apiURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%s/reviews", baseURL, owner, repo, prNumber) - client := &http.Client{Timeout: 10 * time.Second} - req, err := http.NewRequestWithContext(ctx, http.MethodGet, apiURL, nil) - if err != nil { - return nil - } - req.Header.Set("Authorization", "Bearer "+token) + // Deduplicate by login, keeping insertion order but updating to the most recent state. + seen := make(map[string]int) // login → index + var reviewers []prinfo.Reviewer - resp, err := client.Do(req) - if err != nil { - return nil - } - defer resp.Body.Close() + nextURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%s/reviews?per_page=100", baseURL, owner, repo, prNumber) + for nextURL != "" { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, nextURL, nil) + if err != nil { + return nil + } + req.Header.Set("Authorization", "Bearer "+token) - if resp.StatusCode != http.StatusOK { - return nil - } + resp, err := client.Do(req) + if err != nil { + return nil + } - var reviews []struct { - User struct { - Login string `json:"login"` - Type string `json:"type"` - } `json:"user"` - State string `json:"state"` - } - if err := json.NewDecoder(resp.Body).Decode(&reviews); err != nil { - return nil - } + if resp.StatusCode != http.StatusOK { + resp.Body.Close() + return nil + } - // Deduplicate by login, keeping insertion order but updating to the most recent state. - seen := make(map[string]int) // login → index - var reviewers []prinfo.Reviewer - for _, r := range reviews { - reviewerType := r.User.Type - if reviewerType == "" { - reviewerType = "unknown" + var page []struct { + User struct { + Login string `json:"login"` + Type string `json:"type"` + } `json:"user"` + State string `json:"state"` } - if idx, exists := seen[r.User.Login]; exists { - reviewers[idx].ReviewStatus = r.State - continue + err = json.NewDecoder(resp.Body).Decode(&page) + nextURL = nextPageURL(resp.Header.Get("Link")) + resp.Body.Close() + if err != nil { + return nil + } + + for _, r := range page { + reviewerType := r.User.Type + if reviewerType == "" { + reviewerType = "unknown" + } + if idx, exists := seen[r.User.Login]; exists { + reviewers[idx].ReviewStatus = r.State + continue + } + seen[r.User.Login] = len(reviewers) + reviewers = append(reviewers, prinfo.Reviewer{ + Login: r.User.Login, + Type: reviewerType, + ReviewStatus: r.State, + }) } - seen[r.User.Login] = len(reviewers) - reviewers = append(reviewers, prinfo.Reviewer{ - Login: r.User.Login, - Type: reviewerType, - ReviewStatus: r.State, - }) } if len(reviewers) == 0 { @@ -320,6 +326,25 @@ func fetchGitHubReviews(ctx context.Context, baseURL, owner, repo, prNumber, tok return reviewers } +// nextPageURL parses the GitHub Link header and returns the URL for the next page, +// or an empty string if there is no next page. +// Link header format: ; rel="next", ; rel="last" +func nextPageURL(linkHeader string) string { + for _, part := range strings.Split(linkHeader, ",") { + part = strings.TrimSpace(part) + sections := strings.SplitN(part, ";", 2) + if len(sections) < 2 { + continue + } + urlPart := strings.TrimSpace(sections[0]) + relPart := strings.TrimSpace(sections[1]) + if relPart == `rel="next"` && len(urlPart) >= 2 { + return urlPart[1 : len(urlPart)-1] // strip angle brackets + } + } + return "" +} + // extractGitLabMRMetadata extracts from GitLab environment variables func extractGitLabMRMetadata(ctx context.Context, envVars map[string]string) (bool, *PRMetadata, error) { pipelineSource := envVars["CI_PIPELINE_SOURCE"] diff --git a/pkg/attestation/crafter/prmetadata_test.go b/pkg/attestation/crafter/prmetadata_test.go index 447687b69..744afc3c9 100644 --- a/pkg/attestation/crafter/prmetadata_test.go +++ b/pkg/attestation/crafter/prmetadata_test.go @@ -444,6 +444,77 @@ func TestFetchGitHubReviewsRequestPath(t *testing.T) { fetchGitHubReviews(context.Background(), server.URL, "owner", "repo", "42", "test-token") } +func TestFetchGitHubReviewsPagination(t *testing.T) { + page := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + page++ + switch page { + case 1: + // Set Link header pointing to page 2 + w.Header().Set("Link", fmt.Sprintf(`<%s/repos/owner/repo/pulls/42/reviews?per_page=100&page=2>; rel="next"`, r.Host)) + fmt.Fprint(w, `[{"user":{"login":"reviewer1","type":"User"},"state":"COMMENTED"}]`) + case 2: + // Last page — no Link header + fmt.Fprint(w, `[{"user":{"login":"reviewer2","type":"User"},"state":"APPROVED"},{"user":{"login":"reviewer1","type":"User"},"state":"APPROVED"}]`) + } + })) + defer server.Close() + + // Fix up the Link header host to use the test server URL + page = 0 + server2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + page++ + switch page { + case 1: + w.Header().Set("Link", fmt.Sprintf(`<%s/repos/owner/repo/pulls/42/reviews?per_page=100&page=2>; rel="next"`, server.URL)) + fmt.Fprint(w, `[{"user":{"login":"reviewer1","type":"User"},"state":"COMMENTED"}]`) + case 2: + fmt.Fprint(w, `[{"user":{"login":"reviewer2","type":"User"},"state":"APPROVED"},{"user":{"login":"reviewer1","type":"User"},"state":"APPROVED"}]`) + } + })) + defer server2.Close() + + reviewers := fetchGitHubReviews(context.Background(), server2.URL, "owner", "repo", "42", "test-token") + require.Len(t, reviewers, 2) + // reviewer1 appears on both pages; last state (APPROVED from page 2) wins + assert.Equal(t, "reviewer1", reviewers[0].Login) + assert.Equal(t, "APPROVED", reviewers[0].ReviewStatus) + assert.Equal(t, "reviewer2", reviewers[1].Login) + assert.Equal(t, "APPROVED", reviewers[1].ReviewStatus) +} + +func TestNextPageURL(t *testing.T) { + testCases := []struct { + name string + header string + expected string + }{ + { + name: "next page present", + header: `; rel="next", ; rel="last"`, + expected: "https://api.github.com/repos/o/r/pulls/1/reviews?per_page=100&page=2", + }, + { + name: "no next page", + header: `; rel="first"`, + expected: "", + }, + { + name: "empty header", + header: "", + expected: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expected, nextPageURL(tc.header)) + }) + } +} + func TestFetchGitHubRequestedReviewers(t *testing.T) { testCases := []struct { name string From 9482cad8681e5eb7dcd566db00b03dfb84b8bc19 Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Fri, 20 Mar 2026 13:03:38 +0100 Subject: [PATCH 3/4] fix(attestation): rename unused parameter in prmetadata test Signed-off-by: Javier Rodriguez --- pkg/attestation/crafter/prmetadata_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/attestation/crafter/prmetadata_test.go b/pkg/attestation/crafter/prmetadata_test.go index 744afc3c9..65662c9ce 100644 --- a/pkg/attestation/crafter/prmetadata_test.go +++ b/pkg/attestation/crafter/prmetadata_test.go @@ -463,7 +463,7 @@ func TestFetchGitHubReviewsPagination(t *testing.T) { // Fix up the Link header host to use the test server URL page = 0 - server2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + server2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") page++ switch page { From 19ef67e96f29d86fc92dedb84a6926ec7b2e51fa Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Sat, 21 Mar 2026 11:22:18 +0100 Subject: [PATCH 4/4] fix(attestation): address PR review feedback on GitHub PR metadata - Use GITHUB_REPOSITORY_OWNER env var directly for owner instead of splitting GITHUB_REPOSITORY; remove now-unused splitOwnerRepo helper - Add comment clarifying why both event payload and API are used as sources for requested reviewers (event is a snapshot, API is current) - Decode requested_reviewers API response directly into []prinfo.Reviewer instead of an equivalent anonymous struct Signed-off-by: Javier Rodriguez --- pkg/attestation/crafter/prmetadata.go | 45 +++++++--------------- pkg/attestation/crafter/prmetadata_test.go | 9 +++-- 2 files changed, 19 insertions(+), 35 deletions(-) diff --git a/pkg/attestation/crafter/prmetadata.go b/pkg/attestation/crafter/prmetadata.go index bcaace587..72d6daef7 100644 --- a/pkg/attestation/crafter/prmetadata.go +++ b/pkg/attestation/crafter/prmetadata.go @@ -121,8 +121,8 @@ func extractGitHubPRMetadata(ctx context.Context, envVars map[string]string) (bo // GITHUB_TOKEN is read via os.Getenv to avoid persisting it in the attestation envVars map. // GITHUB_API_BASE_URL can be overridden (e.g. in tests); defaults to api.github.com. - parts := splitOwnerRepo(envVars["GITHUB_REPOSITORY"]) - owner, repo := parts[0], parts[1] + owner := envVars["GITHUB_REPOSITORY_OWNER"] + _, repo, _ := strings.Cut(envVars["GITHUB_REPOSITORY"], "/") prNumber := fmt.Sprintf("%d", event.PullRequest.Number) githubAPIBase := os.Getenv("GITHUB_API_BASE_URL") if githubAPIBase == "" { @@ -152,6 +152,9 @@ func extractGitHubPRMetadata(ctx context.Context, envVars map[string]string) (bo }) } + // Also fetch from the API: the event payload is a snapshot at dispatch time, so reviewers + // added after the event fires won't appear in event.PullRequest.RequestedReviewers. + // The API reflects current state and may include those late additions. for _, r := range fetchGitHubRequestedReviewers(ctx, githubAPIBase, owner, repo, prNumber, token) { if _, exists := reviewerMap[r.Login]; !exists { reviewerMap[r.Login] = len(reviewers) @@ -185,17 +188,6 @@ func extractGitHubPRMetadata(ctx context.Context, envVars map[string]string) (bo return true, metadata, nil } -// splitOwnerRepo splits "owner/repo" into [owner, repo]. -// Returns ["", ""] if the string does not contain a slash. -func splitOwnerRepo(ownerRepo string) [2]string { - for i := 0; i < len(ownerRepo); i++ { - if ownerRepo[i] == '/' { - return [2]string{ownerRepo[:i], ownerRepo[i+1:]} - } - } - return [2]string{"", ""} -} - // fetchGitHubRequestedReviewers fetches the list of users explicitly requested to review a PR. // Returns nil on any failure (best-effort). // baseURL is the GitHub API base (e.g. "https://api.github.com"); can be overridden in tests. @@ -225,32 +217,23 @@ func fetchGitHubRequestedReviewers(ctx context.Context, baseURL, owner, repo, pr } var result struct { - Users []struct { - Login string `json:"login"` - Type string `json:"type"` - } `json:"users"` + Users []prinfo.Reviewer `json:"users"` } if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { return nil } - var reviewers []prinfo.Reviewer - for _, u := range result.Users { - reviewerType := u.Type - if reviewerType == "" { - reviewerType = "unknown" - } - reviewers = append(reviewers, prinfo.Reviewer{ - Login: u.Login, - Type: reviewerType, - Requested: true, - }) + if len(result.Users) == 0 { + return nil } - if len(reviewers) == 0 { - return nil + for i := range result.Users { + result.Users[i].Requested = true + if result.Users[i].Type == "" { + result.Users[i].Type = "unknown" + } } - return reviewers + return result.Users } // fetchGitHubReviews fetches all PR reviews from the GitHub API, following pagination. diff --git a/pkg/attestation/crafter/prmetadata_test.go b/pkg/attestation/crafter/prmetadata_test.go index 65662c9ce..f305267b3 100644 --- a/pkg/attestation/crafter/prmetadata_test.go +++ b/pkg/attestation/crafter/prmetadata_test.go @@ -642,10 +642,11 @@ func TestExtractGitHubPRMetadataWithAPIReviews(t *testing.T) { envVars := map[string]string{ "GITHUB_EVENT_NAME": "pull_request", // github_pr_event.json has requested_reviewers: reviewer1 (User), coderabbitai (Bot) - "GITHUB_EVENT_PATH": filepath.Join("testdata", "github_pr_event.json"), - "GITHUB_HEAD_REF": "feature-branch", - "GITHUB_BASE_REF": "main", - "GITHUB_REPOSITORY": "owner/repo", + "GITHUB_EVENT_PATH": filepath.Join("testdata", "github_pr_event.json"), + "GITHUB_HEAD_REF": "feature-branch", + "GITHUB_BASE_REF": "main", + "GITHUB_REPOSITORY": "owner/repo", + "GITHUB_REPOSITORY_OWNER": "owner", } isPR, metadata, err := extractGitHubPRMetadata(context.Background(), envVars)