From 7b7d49dc0707103be655e2210716f2ed4cc6450d Mon Sep 17 00:00:00 2001 From: mintaka Date: Sat, 26 Sep 2026 06:30:38 -0400 Subject: [PATCH 1/3] feat(forge): read PR review threads and required checks from GitHub GraphQL (RIG-1728) GetPullRequest and Checks now add one paginated GraphQL leg. It fills PullRequest.Threads (with resolution and bot-suffixed authors) and sets Check.Required from isRequired(pullRequestNumber:). Rulesets make the REST branch-protection endpoint 404. Contexts are read through the PR's last commit, so the App needs only Pull requests: read. The fail-fast rate gate is now keyed per x-ratelimit-resource, so a drained GraphQL bucket no longer blocks REST calls, and RATE_LIMITED on HTTP 200 maps to RateLimitError. Refs RIG-1728 Co-authored-by: Matt Wilkinson --- go/internal/forge/github.go | 180 ++++--- go/internal/forge/github_graphql.go | 427 ++++++++++++++++ go/internal/forge/github_graphql_test.go | 473 ++++++++++++++++++ go/internal/forge/github_test.go | 114 ++++- go/internal/forge/golden_capture_test.go | 111 +++- go/internal/forge/golden_test.go | 2 +- go/internal/forge/linear.go | 6 +- go/internal/forge/notify_reader.go | 8 +- .../testdata/github/get_pull_request.json | 112 ++++- 9 files changed, 1308 insertions(+), 125 deletions(-) create mode 100644 go/internal/forge/github_graphql.go create mode 100644 go/internal/forge/github_graphql_test.go diff --git a/go/internal/forge/github.go b/go/internal/forge/github.go index 8478ff7cf..68682eff5 100644 --- a/go/internal/forge/github.go +++ b/go/internal/forge/github.go @@ -80,22 +80,45 @@ type GitHub struct { // across the HTTP round-trip. mu sync.Mutex - // resetAt is the rate-budget gate: when non-zero and now() is before it, - // the next call fails fast with ErrBudgetExhausted rather than burning the - // tail of the window. It is derived from the last response's - // x-ratelimit-reset (or a 403/429 Retry-After / reset signal). A zero value - // means the gate is OPEN. Absent/malformed headers leave it open (treat - // unknown budget as available — never wedge the gate). Once now() passes - // resetAt the gate re-opens, so a wedged window self-clears after the reset. - // Guarded by mu because the author client is shared between the poll driver - // and write-RPC goroutines (OQ-6). - resetAt time.Time + // resetAt is the rate-budget gate, one per GitHub rate-limit resource + // (REST core vs GraphQL are separate buckets, so draining one must not block + // the other). When non-zero and now() is before it, the next call on that + // resource fails fast with ErrBudgetExhausted rather than burning the tail of + // the window. It is derived from the last response's x-ratelimit-reset (or a + // 403/429 Retry-After / reset signal). A zero value means the gate is OPEN. + // Absent/malformed headers leave it open (treat unknown budget as available — + // never wedge the gate). Once now() passes resetAt the gate re-opens, so a + // wedged window self-clears after the reset. Guarded by mu. + resetAt [numRateResources]time.Time // now is the clock seam (defaults to time.Now in NewGitHub); tests override // it to drive the reset-time gate deterministically without real sleeps. now func() time.Time } +// rateResource is a GitHub rate-limit bucket, named by the x-ratelimit-resource +// response header. Each has its own budget, so each has its own gate. +type rateResource int + +const ( + resourceCore rateResource = iota + resourceGraphQL + numRateResources +) + +// resourceOf reads the bucket a response was charged to. A missing or unknown +// header falls back to the caller's resource, so a GraphQL response never arms core. +func resourceOf(resp *http.Response, fallback rateResource) rateResource { + switch resp.Header.Get("X-Ratelimit-Resource") { + case "core": + return resourceCore + case "graphql": + return resourceGraphQL + default: + return fallback + } +} + // reserve is the rate-budget floor: when x-ratelimit-remaining is at or under // this, the next call fails fast with ErrBudgetExhausted rather than spending // the tail of the window. @@ -164,7 +187,7 @@ func (g *GitHub) ListIssuesPage(ctx context.Context, repo string, f IssueFilter, // Gate check: an armed gate blocks until the injected clock passes resetAt, // then re-opens so the next call issues a real request (whose response // re-records the budget). A zero resetAt means the gate is open. - if hint, blocked := g.gateBlocked(); blocked { + if hint, blocked := g.gateBlocked(resourceCore); blocked { return ListPage{}, fmt.Errorf("forge: github list %q page %d: %w", repo, page, &RateLimitError{RetryAfter: hint}) } @@ -198,18 +221,18 @@ func (g *GitHub) ListIssuesPage(ctx context.Context, repo string, f IssueFilter, // A 304's x-ratelimit-* headers reflect a healthy authorized bucket // (the conditional request was not charged against the primary limit); // record it for the NEXT call, then short-circuit before any body parse. - g.recordBudget(resp) + g.recordBudget(resp, resourceCore) return ListPage{NotModified: true, RateLimitRemaining: remainingHeader(resp)}, nil case resp.StatusCode >= 200 && resp.StatusCode < 300: // A 2xx's headers reflect a healthy bucket; record for the next call. - g.recordBudget(resp) + g.recordBudget(resp, resourceCore) // fallthrough to body parse below default: // Error responses: mapErrorResponse owns the budget decision (it arms the gate // on a true rate-limit signal). A bad-creds 403 carries a low nonzero remaining; // recording it here would arm the gate against the token we are about to // invalidate, suppressing the fresh-token retry the next batch is meant to make. - return ListPage{}, g.mapErrorResponse(resp) + return ListPage{}, g.mapErrorResponse(resp, resourceCore) } body, err := io.ReadAll(resp.Body) @@ -281,7 +304,7 @@ func (r ghComment) toComment() Comment { // ghPull is the wire shape of a GitHub pull request (the create response). Only // the fields forge.PullRequest needs at create time are decoded; the read-side -// roll-ups (Changed/Checks/Reviews/Threads) belong to RIG-1728's GetPullRequest. +// roll-ups (Changed/Checks/Reviews/Threads) are GetPullRequest's (ghPullDetail). type ghPull struct { Number uint64 `json:"number"` Title string `json:"title"` @@ -588,11 +611,15 @@ type ghReviewRow struct { } `json:"user"` } -// GetPullRequest fetches a pull request with its read roll-ups. It is a -// composite of three GETs — the pull detail, the reviews list, and the checks -// roll-up (folded off the head SHA the detail already carries, so the PR is not -// re-fetched). Bodies are RAW. +// GetPullRequest fetches a pull request with its read roll-ups: the pull detail +// and reviews list over REST, then checksForPull for the checks roll-up (folded +// off the detail's head SHA, so the PR is not re-fetched) together with the +// review threads, which only GraphQL exposes. Bodies are RAW. func (g *GitHub) GetPullRequest(ctx context.Context, repo string, number uint64) (PullRequest, error) { + coord, err := newPullCoord(repo, number) + if err != nil { + return PullRequest{}, fmt.Errorf("forge: github get pull request %q#%d: %w", repo, number, err) + } base := g.apiBase() + "/repos/" + repo + "/pulls/" + strconv.FormatUint(number, 10) var detail ghPullDetail @@ -616,14 +643,12 @@ func (g *GitHub) GetPullRequest(ctx context.Context, repo string, number uint64) }) } - checks, err := g.checksForSHA(ctx, repo, detail.Head.SHA) + checks, threads, err := g.checksForPull(ctx, coord, detail.Head.SHA, true) if err != nil { return PullRequest{}, fmt.Errorf("forge: github get pull request %q#%d: %w", repo, number, err) } pr.Checks = checks - - // TODO(RIG-1728): review-thread resolution is GitHub GraphQL-only; the REST read path leaves Threads empty. The write path does not consume Threads; the board/ingestion PR-pane enrichment is RIG-1728's full read/projection scope. - pr.Threads = nil + pr.Threads = threads return pr, nil } @@ -666,14 +691,19 @@ type ghCommitPull struct { } // Checks returns the rolled-up CI/status state for a PR head. It first resolves -// the head SHA (a minimal pull-detail fetch), then delegates to checksForSHA. +// the head SHA (a minimal pull-detail fetch), then delegates to checksForPull, +// the same helper GetPullRequest uses, so Required is set identically on both. func (g *GitHub) Checks(ctx context.Context, repo string, number uint64) (Checks, error) { + coord, err := newPullCoord(repo, number) + if err != nil { + return Checks{}, fmt.Errorf("forge: github checks %q#%d: %w", repo, number, err) + } url := g.apiBase() + "/repos/" + repo + "/pulls/" + strconv.FormatUint(number, 10) var detail ghPullDetail if _, err := g.getJSON(ctx, url, &detail); err != nil { return Checks{}, fmt.Errorf("forge: github checks %q#%d: %w", repo, number, err) } - checks, err := g.checksForSHA(ctx, repo, detail.Head.SHA) + checks, _, err := g.checksForPull(ctx, coord, detail.Head.SHA, false) if err != nil { return Checks{}, fmt.Errorf("forge: github checks %q#%d: %w", repo, number, err) } @@ -699,7 +729,7 @@ func (g *GitHub) BodyLimit() int { return 65536 } func (g *GitHub) getJSON(ctx context.Context, url string, out any) (bool, error) { // Gate check mirrors ListIssuesPage: an armed gate short-circuits without a // request until the injected clock passes resetAt, then re-opens. - if hint, blocked := g.gateBlocked(); blocked { + if hint, blocked := g.gateBlocked(resourceCore); blocked { return false, fmt.Errorf("GET %s: %w", url, &RateLimitError{RetryAfter: hint}) } @@ -726,9 +756,9 @@ func (g *GitHub) getJSON(ctx context.Context, url string, out any) (bool, error) // gate on a true rate-limit signal). A bad-creds 403 carries a low nonzero // remaining; recording it here would arm the gate against the token we are // about to invalidate (same reasoning as ListIssuesPage — no budget on error). - return false, g.mapErrorResponse(resp) + return false, g.mapErrorResponse(resp, resourceCore) } - g.recordBudget(resp) + g.recordBudget(resp, resourceCore) body, err := io.ReadAll(resp.Body) if err != nil { @@ -775,8 +805,8 @@ func getAllPages[E, T any](ctx context.Context, g *GitHub, baseURL string, rows // concatenated, never truncated to page 1). The roll-up State is "failure" if // ANY check is a terminal-bad outcome (failure or cancelled), else "pending" if // ANY is non-terminal or unknown, else "success" (only when every check is a -// terminal pass; an empty set rolls up to success). Required is false everywhere: -// TODO(RIG-1728): required-check status derives from branch protection (a separate API); the write path does not consume Required. +// terminal pass; an empty set rolls up to success). Required is left false: it +// is per pull request, not per SHA, so checksForPull sets it. func (g *GitHub) checksForSHA(ctx context.Context, repo, sha string) (Checks, error) { commitBase := g.apiBase() + "/repos/" + repo + "/commits/" + sha @@ -884,40 +914,46 @@ func rollupChecksState(checks []Check) string { return checkStateSuccess } -// gateBlocked reports whether the fail-fast budget gate is currently armed and, -// when armed, the remaining wait until it re-opens (resetAt-now, clamped >= 0) -// so the caller can surface the hint. It clears a gate whose reset instant has -// passed (re-opening it) as a side effect. Guarded by mu — the gate is shared -// between the poll driver and the write-RPC goroutines (OQ-6). -func (g *GitHub) gateBlocked() (time.Duration, bool) { +// gateBlocked reports whether the fail-fast budget gate for resource is +// currently armed and, when armed, the remaining wait until it re-opens +// (resetAt-now, clamped >= 0) so the caller can surface the hint. It clears a +// gate whose reset instant has passed (re-opening it) as a side effect. Guarded +// by mu — the gate is shared between the poll driver and the write-RPC +// goroutines (OQ-6). +func (g *GitHub) gateBlocked(resource rateResource) (time.Duration, bool) { g.mu.Lock() defer g.mu.Unlock() - if g.resetAt.IsZero() { + at := g.resetAt[resource] + if at.IsZero() { return 0, false } now := g.now() - if now.Before(g.resetAt) { - return g.resetAt.Sub(now), true + if now.Before(at) { + return at.Sub(now), true } - g.resetAt = time.Time{} + g.resetAt[resource] = time.Time{} return 0, false } -// doJSON carries the write-path plumbing once for every write method: the -// resetAt fail-fast gate (a write burst respects the same reserve as the poll -// driver, so it cannot starve it), token auth, budget recording, and error -// mapping. It marshals in to a JSON request body and decodes a 2xx response -// into out. The read path (ListIssuesPage) is intentionally NOT refactored onto -// this in this slice (no RIG-1728 rework). -// -// method is the HTTP verb: the create/comment/review writes POST, and the state -// transitions PATCH. Everything else about the exchange is identical — writes -// are unconditional (no If-None-Match), so the verb is the only axis that -// varies and one helper still carries the whole write path. +// doJSON is the REST write path: doJSONOn against the core rate bucket. func (g *GitHub) doJSON(ctx context.Context, method, url string, in, out any) error { + return g.doJSONOn(ctx, resourceCore, method, url, in, out) +} + +// doJSONOn carries the JSON-body plumbing once for every write method and the +// GraphQL read (graphQL): resource's fail-fast gate (a write burst respects the +// same reserve as the poll driver, so it cannot starve it), token auth, budget +// recording, and error mapping. It marshals in to a JSON request body and +// decodes a 2xx response into out. The REST reads use getJSON instead. +// +// method is the HTTP verb: the create/comment/review writes and the GraphQL +// query POST, and the state transitions PATCH. Everything else about the +// exchange is identical — these calls are unconditional (no If-None-Match), so +// the verb and the rate bucket are the only axes that vary. +func (g *GitHub) doJSONOn(ctx context.Context, resource rateResource, method, url string, in, out any) error { // Gate check mirrors ListIssuesPage: an armed gate short-circuits without a // request until the injected clock passes resetAt, then re-opens. - if hint, blocked := g.gateBlocked(); blocked { + if hint, blocked := g.gateBlocked(resource); blocked { return fmt.Errorf("%s %s: %w", method, url, &RateLimitError{RetryAfter: hint}) } @@ -950,9 +986,9 @@ func (g *GitHub) doJSON(ctx context.Context, method, url string, in, out any) er // the gate on a true rate-limit signal). A bad-creds 403 carries a low // nonzero remaining; recording it here would arm the gate against the // token we are about to invalidate (same reasoning as the read path). - return g.mapErrorResponse(resp) + return g.mapErrorResponse(resp, resource) } - g.recordBudget(resp) + g.recordBudget(resp, resource) respBody, err := io.ReadAll(resp.Body) if err != nil { @@ -1018,29 +1054,31 @@ func (g *GitHub) queryParams(f IssueFilter, page int) url.Values { return q } -// recordBudget updates the fail-fast gate from a response's x-ratelimit-* +// recordBudget updates the fail-fast gate of the bucket the response was +// charged to (x-ratelimit-resource, else fallback) from its x-ratelimit-* // headers. remaining <= reserve arms the gate until x-ratelimit-reset (unix // seconds); absent/malformed headers leave it open (treat unknown budget as // available — never wedge the gate). A missing/unparseable reset with a // low remaining falls back to a bounded skip so the gate still self-clears. -func (g *GitHub) recordBudget(resp *http.Response) { +func (g *GitHub) recordBudget(resp *http.Response, fallback rateResource) { + resource := resourceOf(resp, fallback) g.mu.Lock() defer g.mu.Unlock() raw := resp.Header.Get("X-Ratelimit-Remaining") if raw == "" { - g.resetAt = time.Time{} + g.resetAt[resource] = time.Time{} return } remaining, err := strconv.Atoi(raw) if err != nil { - g.resetAt = time.Time{} + g.resetAt[resource] = time.Time{} return } if remaining > reserve { - g.resetAt = time.Time{} + g.resetAt[resource] = time.Time{} return } - g.armGate(resetFromHeader(resp.Header.Get("X-Ratelimit-Reset"))) + g.armGate(resource, resetFromHeader(resp.Header.Get("X-Ratelimit-Reset"))) } // remainingHeader parses the x-ratelimit-remaining header into the observability @@ -1059,14 +1097,14 @@ func remainingHeader(resp *http.Response) int { return remaining } -// armGate sets the reset-time gate. A zero at (no usable reset time) falls back -// to a bounded skip from now() so the gate self-clears rather than wedging. -// Caller MUST hold g.mu. -func (g *GitHub) armGate(at time.Time) { +// armGate sets resource's reset-time gate. A zero at (no usable reset time) +// falls back to a bounded skip from now() so the gate self-clears rather than +// wedging. Caller MUST hold g.mu. +func (g *GitHub) armGate(resource rateResource, at time.Time) { if at.IsZero() { at = g.now().Add(defaultSkip) } - g.resetAt = at + g.resetAt[resource] = at } // resetFromHeader parses an x-ratelimit-reset value (unix seconds) into an @@ -1084,11 +1122,11 @@ func resetFromHeader(raw string) time.Time { // mapErrorResponse classifies a non-2xx/non-304 response. A 403/429 carrying // retry-after OR a zeroed x-ratelimit-remaining is a rate-limit skip -// (ErrBudgetExhausted, arms the gate, no token re-resolve). A 401, or a 403 -// without rate-limit headers (bad-credentials/permission), is a *StatusError -// AND invalidates the TokenSource so the next batch re-resolves. Any other -// non-2xx is a *StatusError only. -func (g *GitHub) mapErrorResponse(resp *http.Response) error { +// (ErrBudgetExhausted, arms the gate of the bucket it names — else fallback — +// no token re-resolve). A 401, or a 403 without rate-limit headers +// (bad-credentials/permission), is a *StatusError AND invalidates the +// TokenSource so the next batch re-resolves. Any other non-2xx is a *StatusError only. +func (g *GitHub) mapErrorResponse(resp *http.Response, fallback rateResource) error { body, _ := io.ReadAll(resp.Body) // best-effort: message is diagnostic; a read error just yields an empty message var ge ghError _ = json.Unmarshal(body, &ge) // best-effort decode of the diagnostic message; malformed body -> empty message @@ -1102,7 +1140,7 @@ func (g *GitHub) mapErrorResponse(resp *http.Response) error { // the bounded defaultSkip internally. reset := g.rateLimitReset(resp) g.mu.Lock() - g.armGate(reset) + g.armGate(resourceOf(resp, fallback), reset) g.mu.Unlock() var hint time.Duration if !reset.IsZero() { diff --git a/go/internal/forge/github_graphql.go b/go/internal/forge/github_graphql.go new file mode 100644 index 000000000..beae277a4 --- /dev/null +++ b/go/internal/forge/github_graphql.go @@ -0,0 +1,427 @@ +package forge + +// The GitHub GraphQL leg of the pull-request read. Review threads (with their +// resolution) and per-PR required-context status are GraphQL-only, so this file +// adds one POST path beside the REST client. It rides doJSONOn against the +// GraphQL rate bucket, sharing the TokenSource and the HTTP error mapping. + +import ( + "context" + "errors" + "fmt" + "math" + "net/http" + "strings" + "time" +) + +// errMalformedRepo is returned (wrapped) when a GitHub repo coordinate is not +// exactly "owner/name"; GraphQL takes the two halves as separate variables. +var errMalformedRepo = errors.New("forge: malformed github repo, want owner/name") + +// ghGraphQLRateLimited is the errors[].type GitHub returns, on HTTP 200, when +// the GraphQL budget is spent. +const ghGraphQLRateLimited = "RATE_LIMITED" + +// ghTypeBot is the GraphQL __typename of a bot actor. +const ghTypeBot = "Bot" + +// pullCoord is a pull request's GraphQL coordinate. repo is the REST form the +// caller passed; owner and name are its validated halves. +type pullCoord struct { + repo string + owner string + name string + number uint64 +} + +// newPullCoord splits repo into owner and name. The number must fit GraphQL's +// 32-bit Int, or the query would be rejected (or address a different PR). +func newPullCoord(repo string, number uint64) (pullCoord, error) { + owner, name, ok := strings.Cut(repo, "/") + if !ok || owner == "" || name == "" || strings.Contains(name, "/") { + return pullCoord{}, fmt.Errorf("%w: %q", errMalformedRepo, repo) + } + if number > math.MaxInt32 { + return pullCoord{}, fmt.Errorf("forge: github pull request number %d exceeds the GraphQL Int range", number) + } + return pullCoord{repo: repo, owner: owner, name: name, number: number}, nil +} + +// graphQLRequest is the POST body of one GraphQL call. +type graphQLRequest struct { + Query string `json:"query"` + Variables map[string]any `json:"variables"` +} + +// graphQLEnvelope is a typed GraphQL response: data plus any errors. GitHub +// answers a failed query with HTTP 200 and a non-empty errors array. +type graphQLEnvelope[T any] struct { + Data T `json:"data"` + Errors []graphQLError `json:"errors"` +} + +// graphQL posts one query through doJSONOn on the GraphQL rate bucket and +// decodes its data. A non-empty errors array is an error even on HTTP 200, so a +// failed query never reads as an empty result; a RATE_LIMITED entry is the +// budget skip (*RateLimitError), the same as a REST 403/429. +func graphQL[T any](ctx context.Context, g *GitHub, query string, vars map[string]any) (T, error) { + var zero T + var env graphQLEnvelope[T] + if err := g.doJSONOn(ctx, resourceGraphQL, http.MethodPost, g.graphQLURL(), graphQLRequest{Query: query, Variables: vars}, &env); err != nil { + return zero, err + } + if len(env.Errors) == 0 { + return env.Data, nil + } + for _, e := range env.Errors { + if e.Type == ghGraphQLRateLimited { + return zero, fmt.Errorf("forge: github graphql: %s: %w", joinErrors(env.Errors), g.graphQLRateLimited()) + } + } + return zero, fmt.Errorf("forge: github graphql: %s", joinErrors(env.Errors)) +} + +// graphQLRateLimited arms the GraphQL gate after a RATE_LIMITED error and +// returns the skip with its retry hint. doJSONOn has already recorded the +// response's reset header, so an armed gate carries the hint; else arm the +// bounded default skip so the next call still fails fast. +func (g *GitHub) graphQLRateLimited() *RateLimitError { + if hint, blocked := g.gateBlocked(resourceGraphQL); blocked { + return &RateLimitError{RetryAfter: hint} + } + g.mu.Lock() + g.armGate(resourceGraphQL, time.Time{}) + g.mu.Unlock() + return &RateLimitError{} +} + +// ghPageInfo is a GraphQL connection's cursor state. +type ghPageInfo struct { + HasNextPage bool `json:"hasNextPage"` + EndCursor string `json:"endCursor"` +} + +// ghGQLActor is a comment author. It is null for a deleted account ("ghost"). +type ghGQLActor struct { + Login string `json:"login"` + Typename string `json:"__typename"` +} + +// ghGQLComment is one review-thread comment. +type ghGQLComment struct { + Author *ghGQLActor `json:"author"` + Body string `json:"body"` +} + +// ghGQLComments is one page of a thread's comments. +type ghGQLComments struct { + PageInfo ghPageInfo `json:"pageInfo"` + Nodes []ghGQLComment `json:"nodes"` +} + +// ghGQLThread is one review thread. ID lets a thread with more than one page of +// comments be re-queried for the rest. +type ghGQLThread struct { + ID string `json:"id"` + IsResolved bool `json:"isResolved"` + Path string `json:"path"` + Comments ghGQLComments `json:"comments"` +} + +// ghGQLThreads is one page of a pull request's review threads. +type ghGQLThreads struct { + PageInfo ghPageInfo `json:"pageInfo"` + Nodes []ghGQLThread `json:"nodes"` +} + +// ghGQLContext is one status-check-rollup context: a CheckRun carries name, a +// legacy StatusContext carries context. Both carry isRequired for the PR. +type ghGQLContext struct { + Typename string `json:"__typename"` + Name string `json:"name"` + Context string `json:"context"` + IsRequired bool `json:"isRequired"` +} + +// ghGQLContexts is one page of a commit's status-check-rollup contexts. +type ghGQLContexts struct { + PageInfo ghPageInfo `json:"pageInfo"` + Nodes []ghGQLContext `json:"nodes"` +} + +// ghGQLRollup is a commit's status check rollup; null when the commit has none. +type ghGQLRollup struct { + Contexts ghGQLContexts `json:"contexts"` +} + +// ghGQLPullCommits is the PR's last commit (commits(last: 1)). +type ghGQLPullCommits struct { + Nodes []struct { + Commit struct { + StatusCheckRollup *ghGQLRollup `json:"statusCheckRollup"` + } `json:"commit"` + } `json:"nodes"` +} + +// ghGQLPull is the pull request half; each connection is absent when excluded. +type ghGQLPull struct { + ReviewThreads *ghGQLThreads `json:"reviewThreads"` + Commits *ghGQLPullCommits `json:"commits"` +} + +// ghGQLRepo is the repository root of pullReadQuery. +type ghGQLRepo struct { + PullRequest *ghGQLPull `json:"pullRequest"` +} + +// ghGQLPullData is the data of pullReadQuery. +type ghGQLPullData struct { + Repository *ghGQLRepo `json:"repository"` +} + +// ghGQLThreadNode is the data of threadCommentsQuery. +type ghGQLThreadNode struct { + Node *struct { + Comments ghGQLComments `json:"comments"` + } `json:"node"` +} + +// pullReadQuery fetches review threads and required contexts in one call. The +// @include flags let a later page fetch only the connection still paging. The +// contexts are read through the pull request (commits(last: 1)), which needs +// only Pull requests: read, not the Contents access a git-object read would. +// Required-ness depends on the context name and the base-branch rules, not on +// the commit, so the last commit need not equal the REST head SHA. +const pullReadQuery = `query($owner: String!, $name: String!, $number: Int!, $threads: Boolean!, $threadsAfter: String, $contexts: Boolean!, $contextsAfter: String) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + reviewThreads(first: 100, after: $threadsAfter) @include(if: $threads) { + pageInfo { hasNextPage endCursor } + nodes { + id + isResolved + path + comments(first: 100) { + pageInfo { hasNextPage endCursor } + nodes { author { login __typename } body } + } + } + } + commits(last: 1) @include(if: $contexts) { + nodes { + commit { + statusCheckRollup { + contexts(first: 100, after: $contextsAfter) { + pageInfo { hasNextPage endCursor } + nodes { + __typename + ... on CheckRun { name isRequired(pullRequestNumber: $number) } + ... on StatusContext { context isRequired(pullRequestNumber: $number) } + } + } + } + } + } + } + } + } +}` + +// threadCommentsQuery fetches the comments after the first page of one thread. +const threadCommentsQuery = `query($id: ID!, $after: String) { + node(id: $id) { + ... on PullRequestReviewThread { + comments(first: 100, after: $after) { + pageInfo { hasNextPage endCursor } + nodes { author { login __typename } body } + } + } + } +}` + +// graphQLURL derives the GraphQL endpoint from the configured host, next to +// apiBase: api.github.com/graphql, or https:///api/graphql on GHES. +func (g *GitHub) graphQLURL() string { + if g.host == "" || g.host == hostGitHub { + return "https://api.github.com/graphql" + } + return "https://" + g.host + "/api/graphql" +} + +// checksForPull is the one checks path for a pull request (GetPullRequest and +// Checks): the REST roll-up from checksForSHA, with Required set from the PR's +// required contexts. withThreads also returns the review threads from the same +// GraphQL leg, so GetPullRequest pays one leg, not two. +func (g *GitHub) checksForPull(ctx context.Context, c pullCoord, sha string, withThreads bool) (Checks, []ReviewThread, error) { + checks, err := g.checksForSHA(ctx, c.repo, sha) + if err != nil { + return Checks{}, nil, err + } + threads, required, err := g.pullGraphQL(ctx, c, withThreads) + if err != nil { + return Checks{}, nil, fmt.Errorf("forge: github graphql for %q#%d: %w", c.repo, c.number, err) + } + for i := range checks.Checks { + if _, ok := required[checks.Checks[i].Name]; ok { + checks.Checks[i].Required = true + } + } + return checks, threads, nil +} + +// pullGraphQLWalk is the cursor state of one pullGraphQL walk. A connection +// that has finished drops out of later queries through its @include flag. +type pullGraphQLWalk struct { + threads []ReviewThread + required map[string]struct{} + threadsAfter, contextsAfter any // nil sends JSON null: the first page + moreThreads, moreContexts bool +} + +// pullGraphQL walks pullReadQuery to completion: every review-thread page (when +// withThreads) and every context page. It returns the threads in forge order and +// the set of required context names (a CheckRun's name, a StatusContext's +// context), which match the REST check names. +func (g *GitHub) pullGraphQL(ctx context.Context, c pullCoord, withThreads bool) ([]ReviewThread, map[string]struct{}, error) { + w := pullGraphQLWalk{required: map[string]struct{}{}, moreThreads: withThreads, moreContexts: true} + for w.moreThreads || w.moreContexts { + if err := ctx.Err(); err != nil { + return nil, nil, err + } + data, err := graphQL[ghGQLPullData](ctx, g, pullReadQuery, map[string]any{ + "owner": c.owner, "name": c.name, "number": c.number, + "threads": w.moreThreads, "threadsAfter": w.threadsAfter, + "contexts": w.moreContexts, "contextsAfter": w.contextsAfter, + }) + if err != nil { + return nil, nil, err + } + if data.Repository == nil { + return nil, nil, fmt.Errorf("forge: github graphql: repository %q not found", c.repo) + } + pr := data.Repository.PullRequest + if pr == nil { + return nil, nil, fmt.Errorf("forge: github graphql: pull request %q#%d not found", c.repo, c.number) + } + if w.moreThreads { + if err := g.foldThreads(ctx, &w, c, pr.ReviewThreads); err != nil { + return nil, nil, err + } + } + if w.moreContexts { + if err := foldContexts(&w, c, pr.Commits); err != nil { + return nil, nil, err + } + } + } + return w.threads, w.required, nil +} + +// foldThreads appends one page of review threads to w and advances its cursor. +func (g *GitHub) foldThreads(ctx context.Context, w *pullGraphQLWalk, c pullCoord, conn *ghGQLThreads) error { + if conn == nil { + return fmt.Errorf("forge: github graphql: pull request %q#%d has no review threads connection", c.repo, c.number) + } + for _, t := range conn.Nodes { + comments, err := g.threadComments(ctx, t) + if err != nil { + return err + } + w.threads = append(w.threads, ReviewThread{Path: t.Path, Resolved: t.IsResolved, Comments: comments}) + } + var err error + if w.moreThreads, w.threadsAfter, err = nextPage(conn.PageInfo, w.threadsAfter); err != nil { + return fmt.Errorf("forge: github graphql review threads: %w", err) + } + return nil +} + +// foldContexts adds one page of required context names to w and advances its +// cursor. A PR with no commits, or a last commit with no checks or statuses +// (a null rollup), has nothing required. +func foldContexts(w *pullGraphQLWalk, c pullCoord, commits *ghGQLPullCommits) error { + if commits == nil { + return fmt.Errorf("forge: github graphql: pull request %q#%d has no commits connection", c.repo, c.number) + } + if len(commits.Nodes) == 0 || commits.Nodes[0].Commit.StatusCheckRollup == nil { + w.moreContexts = false + return nil + } + contexts := commits.Nodes[0].Commit.StatusCheckRollup.Contexts + for _, cx := range contexts.Nodes { + if !cx.IsRequired { + continue + } + name := cx.Name + if cx.Typename == "StatusContext" { + name = cx.Context + } + w.required[name] = struct{}{} + } + var err error + if w.moreContexts, w.contextsAfter, err = nextPage(contexts.PageInfo, w.contextsAfter); err != nil { + return fmt.Errorf("forge: github graphql status contexts: %w", err) + } + return nil +} + +// threadComments maps a thread's comments in order, fetching every page past +// the first, so a long thread is never truncated. +func (g *GitHub) threadComments(ctx context.Context, t ghGQLThread) ([]ThreadComment, error) { + out := make([]ThreadComment, 0, len(t.Comments.Nodes)) + page := t.Comments + var after any // the first page came inline with the thread + for { + for _, n := range page.Nodes { + out = append(out, toThreadComment(n)) + } + more, next, err := nextPage(page.PageInfo, after) + if err != nil { + return nil, fmt.Errorf("forge: github graphql thread comments: %w", err) + } + if !more { + return out, nil + } + after = next + data, err := graphQL[ghGQLThreadNode](ctx, g, threadCommentsQuery, map[string]any{"id": t.ID, "after": after}) + if err != nil { + return nil, err + } + if data.Node == nil { + return nil, fmt.Errorf("forge: github graphql: review thread %q not found", t.ID) + } + page = data.Node.Comments + } +} + +// toThreadComment maps one wire comment. A null author (a deleted account) maps +// to an empty Author and IsBot false. GraphQL drops the "[bot]" suffix REST +// logins carry, so it is restored to keep one author spelling across both reads. +func toThreadComment(n ghGQLComment) ThreadComment { + tc := ThreadComment{Body: n.Body} + if n.Author != nil { + tc.Author = n.Author.Login + tc.IsBot = n.Author.Typename == ghTypeBot + if tc.IsBot { + tc.Author += "[bot]" + } + } + return tc +} + +// nextPage reads a connection's cursor state against the cursor that fetched +// the page (prev; nil for the first page). A next page with no cursor, or with +// the cursor already used, is an error: asking again would loop forever. +func nextPage(p ghPageInfo, prev any) (bool, any, error) { + if !p.HasNextPage { + return false, nil, nil + } + if p.EndCursor == "" { + return false, nil, errors.New("hasNextPage without an endCursor") + } + if s, ok := prev.(string); ok && s == p.EndCursor { + return false, nil, fmt.Errorf("endCursor %q repeats the previous cursor", p.EndCursor) + } + return true, p.EndCursor, nil +} diff --git a/go/internal/forge/github_graphql_test.go b/go/internal/forge/github_graphql_test.go new file mode 100644 index 000000000..6ccbcf6f3 --- /dev/null +++ b/go/internal/forge/github_graphql_test.go @@ -0,0 +1,473 @@ +package forge + +// Unit tests for the GitHub GraphQL leg (github_graphql.go): paging, error +// branches, the per-resource rate gate, and the author mapping. Response bodies +// are built by marshaling the wire structs; TestGetPullRequestHappy and the +// golden fixture keep literal JSON, so a wrong json tag still fails there. + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "reflect" + "slices" + "strconv" + "strings" + "testing" + "time" +) + +// noRollupGraphQL is a Checks GraphQL leg (threads excluded) whose last commit +// has no status-check rollup, so nothing is required. +const noRollupGraphQL = `{"data":{"repository":{"pullRequest":{"commits":{"nodes":[{"commit":{"statusCheckRollup":null}}]}}}}}` + +// emptyPullGraphQL is a GetPullRequest GraphQL leg with no threads and no +// status-check rollup on the last commit. +const emptyPullGraphQL = `{"data":{"repository":{"pullRequest":{ + "reviewThreads":{"pageInfo":{"hasNextPage":false,"endCursor":null},"nodes":[]}, + "commits":{"nodes":[{"commit":{"statusCheckRollup":null}}]}}}}}` + +// gqlBody marshals a GraphQL data value into a response body. +func gqlBody(t *testing.T, data any) string { + t.Helper() + b, err := json.Marshal(map[string]any{"data": data}) + if err != nil { + t.Fatalf("marshal graphql body: %v", err) + } + return string(b) +} + +// pullPage builds one pullReadQuery page. A nil threads or contexts leaves that +// connection out, as @include(if: false) does; a nil rollup is a commit with no checks. +func pullPage(t *testing.T, threads *ghGQLThreads, commits *ghGQLPullCommits) string { + t.Helper() + return gqlBody(t, ghGQLPullData{Repository: &ghGQLRepo{PullRequest: &ghGQLPull{ReviewThreads: threads, Commits: commits}}}) +} + +// threadsConn is one page of review threads. +func threadsConn(next bool, cursor string, nodes ...ghGQLThread) *ghGQLThreads { + return &ghGQLThreads{PageInfo: ghPageInfo{HasNextPage: next, EndCursor: cursor}, Nodes: nodes} +} + +// thread is one review thread whose comments all fit on the first page. +func thread(id, path string, resolved bool, comments ...ghGQLComment) ghGQLThread { + return ghGQLThread{ID: id, Path: path, IsResolved: resolved, Comments: ghGQLComments{Nodes: comments}} +} + +// comment is one thread comment by login with GraphQL __typename kind. +func comment(login, kind, body string) ghGQLComment { + return ghGQLComment{Author: &ghGQLActor{Login: login, Typename: kind}, Body: body} +} + +// rollup is one page of status contexts on the PR's last commit. +func rollup(next bool, cursor string, nodes ...ghGQLContext) *ghGQLPullCommits { + c := &ghGQLPullCommits{Nodes: make([]struct { + Commit struct { + StatusCheckRollup *ghGQLRollup `json:"statusCheckRollup"` + } `json:"commit"` + }, 1)} + c.Nodes[0].Commit.StatusCheckRollup = &ghGQLRollup{Contexts: ghGQLContexts{PageInfo: ghPageInfo{HasNextPage: next, EndCursor: cursor}, Nodes: nodes}} + return c +} + +// checkRun is a CheckRun context; statusContext is a legacy StatusContext. +func checkRun(name string, required bool) ghGQLContext { + return ghGQLContext{Typename: "CheckRun", Name: name, IsRequired: required} +} + +func statusContext(name string, required bool) ghGQLContext { + return ghGQLContext{Typename: "StatusContext", Context: name, IsRequired: required} +} + +// pullRESTLegs scripts the four REST legs of GetPullRequest with the given +// check runs and no reviews, ahead of the GraphQL responses a test appends. +func pullRESTLegs(checkRuns string, gql ...scriptedResponse) []scriptedResponse { + return append([]scriptedResponse{ + {status: 200, body: `{"number":7,"state":"open","head":{"ref":"f","sha":"s7"},"base":{"ref":"main"},"user":{"login":"a"}}`}, + {status: 200, body: `[]`}, + {status: 200, body: `{"check_runs": [` + checkRuns + `]}`}, + {status: 200, body: `{"statuses": []}`}, + }, gql...) +} + +// ok200 is a scripted 200 response. +func ok200(body string) scriptedResponse { return scriptedResponse{status: 200, body: body} } + +// GetPullRequest walks every review-thread page: page 2 is requested with the +// page-1 endCursor and asks only for threads (the contexts finished on page 1), +// and both pages' threads are returned in order. +func TestGetPullRequestThreadPagination(t *testing.T) { + rt := &scriptedRoundTripper{responses: pullRESTLegs("", + ok200(pullPage(t, threadsConn(true, "CUR1", thread("T1", "a.go", false, comment("x", "User", "one"))), rollup(false, "c"))), + ok200(pullPage(t, threadsConn(false, "CUR2", thread("T2", "b.go", true, comment("y", "User", "two"))), nil)), + )} + g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) + + got, err := g.GetPullRequest(context.Background(), "org/repo", 7) + if err != nil { + t.Fatalf("GetPullRequest: %v", err) + } + if rt.calls != 6 { + t.Fatalf("calls = %d, want 6 (4 REST + 2 GraphQL pages)", rt.calls) + } + if b := readReqBody(t, rt.requests[4]); !strings.Contains(b, `"threadsAfter":null`) { + t.Errorf("page 1 must start with a null cursor: %s", b) + } + b2 := readReqBody(t, rt.requests[5]) + for _, want := range []string{`"threadsAfter":"CUR1"`, `"contexts":false`, `"threads":true`} { + if !strings.Contains(b2, want) { + t.Errorf("page 2 body missing %s: %s", want, b2) + } + } + want := []ReviewThread{ + {Path: "a.go", Comments: []ThreadComment{{Author: "x", Body: "one"}}}, + {Path: "b.go", Resolved: true, Comments: []ThreadComment{{Author: "y", Body: "two"}}}, + } + if !reflect.DeepEqual(got.Threads, want) { + t.Errorf("Threads = %+v, want %+v", got.Threads, want) + } +} + +// Threads and contexts both page: page 2 carries both cursors, page 3 carries +// only the one still paging, and a required context on a later page still marks +// its check. +func TestGetPullRequestThreadsAndContextsPageTogether(t *testing.T) { + runs := `{"name":"build","status":"completed","conclusion":"success","html_url":""}, + {"name":"rollup","status":"completed","conclusion":"success","html_url":""}` + rt := &scriptedRoundTripper{responses: pullRESTLegs(runs, + ok200(pullPage(t, threadsConn(true, "T-1", thread("A", "a.go", false, comment("x", "User", "1"))), rollup(true, "C-1", checkRun("build", false)))), + ok200(pullPage(t, threadsConn(true, "T-2", thread("B", "b.go", false, comment("x", "User", "2"))), rollup(false, "C-2", checkRun("rollup", true)))), + ok200(pullPage(t, threadsConn(false, "T-3", thread("C", "c.go", false, comment("x", "User", "3"))), nil)), + )} + g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) + + got, err := g.GetPullRequest(context.Background(), "org/repo", 7) + if err != nil { + t.Fatalf("GetPullRequest: %v", err) + } + b2 := readReqBody(t, rt.requests[5]) + for _, want := range []string{`"threadsAfter":"T-1"`, `"contextsAfter":"C-1"`, `"threads":true`, `"contexts":true`} { + if !strings.Contains(b2, want) { + t.Errorf("page 2 body missing %s: %s", want, b2) + } + } + b3 := readReqBody(t, rt.requests[6]) + for _, want := range []string{`"threadsAfter":"T-2"`, `"contexts":false`} { + if !strings.Contains(b3, want) { + t.Errorf("page 3 body missing %s: %s", want, b3) + } + } + if len(got.Threads) != 3 || got.Threads[2].Path != "c.go" { + t.Errorf("Threads = %+v, want 3 threads ending in c.go", got.Threads) + } + if got.Checks.Checks[0].Required || !got.Checks.Checks[1].Required { + t.Errorf("Checks = %+v, want only rollup required", got.Checks.Checks) + } +} + +// Checks walks every page of status contexts: a required context on page 2 +// still marks its check, and page 2 is requested with the page-1 cursor. +func TestChecksRequiredContextPagination(t *testing.T) { + rt := &scriptedRoundTripper{responses: []scriptedResponse{ + ok200(`{"number":9,"head":{"sha":"s"},"base":{"ref":"main"},"user":{"login":"a"}}`), + ok200(`{"check_runs": [ + {"name":"build","status":"completed","conclusion":"success","html_url":""}, + {"name":"rollup","status":"completed","conclusion":"success","html_url":""} + ]}`), + ok200(`{"statuses": []}`), + ok200(pullPage(t, nil, rollup(true, "CX1", checkRun("build", false)))), + ok200(pullPage(t, nil, rollup(false, "CX2", checkRun("rollup", true)))), + }} + g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) + + got, err := g.Checks(context.Background(), "org/repo", 9) + if err != nil { + t.Fatalf("Checks: %v", err) + } + if b := readReqBody(t, rt.requests[4]); !strings.Contains(b, `"contextsAfter":"CX1"`) { + t.Errorf("page 2 body = %s, want contextsAfter CX1", b) + } + if got.Checks[0].Required || !got.Checks[1].Required { + t.Errorf("Checks = %+v, want only rollup required", got.Checks) + } +} + +// A required StatusContext marks the legacy status of the same name, and a PR +// whose commits list is empty has nothing required. +func TestChecksRequiredContextShapes(t *testing.T) { + detail := ok200(`{"number":9,"head":{"sha":"s"},"base":{"ref":"main"},"user":{"login":"a"}}`) + status := ok200(`{"statuses": [{"context":"legacy","state":"success","target_url":""}]}`) + + rt := &scriptedRoundTripper{responses: []scriptedResponse{ + detail, ok200(`{"check_runs": []}`), status, + ok200(pullPage(t, nil, rollup(false, "c", statusContext("legacy", true)))), + }} + got, err := newTestGitHub(rt, &fakeTokenSource{token: "t"}).Checks(context.Background(), "org/repo", 9) + if err != nil { + t.Fatalf("Checks: %v", err) + } + if !got.Checks[0].Required { + t.Errorf("legacy status not marked required: %+v", got.Checks) + } + + rt = &scriptedRoundTripper{responses: []scriptedResponse{ + detail, ok200(`{"check_runs": []}`), status, + ok200(pullPage(t, nil, &ghGQLPullCommits{})), + }} + got, err = newTestGitHub(rt, &fakeTokenSource{token: "t"}).Checks(context.Background(), "org/repo", 9) + if err != nil { + t.Fatalf("Checks (no commits): %v", err) + } + if got.Checks[0].Required { + t.Errorf("empty commits list marked a check required: %+v", got.Checks) + } +} + +// A thread with more than one page of comments is completed through the +// thread-node query, so a long thread is never truncated. +func TestGetPullRequestThreadCommentPagination(t *testing.T) { + long := thread("T1", "a.go", false, comment("x", "User", "first")) + long.Comments.PageInfo = ghPageInfo{HasNextPage: true, EndCursor: "C1"} + more := gqlBody(t, map[string]any{"node": map[string]any{"comments": ghGQLComments{ + PageInfo: ghPageInfo{EndCursor: "C2"}, + Nodes: []ghGQLComment{comment("y", ghTypeBot, "second")}, + }}}) + rt := &scriptedRoundTripper{responses: pullRESTLegs("", + ok200(pullPage(t, threadsConn(false, "t", long), rollup(false, "c"))), + ok200(more), + )} + g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) + + got, err := g.GetPullRequest(context.Background(), "org/repo", 7) + if err != nil { + t.Fatalf("GetPullRequest: %v", err) + } + if b := readReqBody(t, rt.requests[5]); !strings.Contains(b, `"id":"T1"`) || !strings.Contains(b, `"after":"C1"`) { + t.Errorf("comment continuation body = %s, want id T1 after C1", b) + } + want := []ThreadComment{{Author: "x", Body: "first"}, {Author: "y[bot]", IsBot: true, Body: "second"}} + if len(got.Threads) != 1 || !slices.Equal(got.Threads[0].Comments, want) { + t.Errorf("Threads = %+v, want one thread with %+v", got.Threads, want) + } +} + +// Author mapping: GraphQL drops the "[bot]" suffix REST logins carry, so a Bot +// author gets it back; a null author (a deleted account) is an empty Author. +func TestToThreadComment(t *testing.T) { + cases := []struct { + name string + in ghGQLComment + want ThreadComment + }{ + {"user", comment("carol", "User", "b"), ThreadComment{Author: "carol", Body: "b"}}, + {"bot gets the REST suffix", comment("dependabot", ghTypeBot, "b"), ThreadComment{Author: "dependabot[bot]", IsBot: true, Body: "b"}}, + {"null author", ghGQLComment{Body: "ghost"}, ThreadComment{Body: "ghost"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := toThreadComment(tc.in); got != tc.want { + t.Errorf("toThreadComment = %+v, want %+v", got, tc.want) + } + }) + } +} + +// Every malformed or dead-end response is an error, never a silent empty or +// truncated result. +func TestPullGraphQLErrorBranches(t *testing.T) { + longThread := thread("T1", "a.go", false, comment("x", "User", "1")) + longThread.Comments.PageInfo = ghPageInfo{HasNextPage: true, EndCursor: "C1"} + cases := []struct { + name string + gql []scriptedResponse + want string + }{ + {"errors array", []scriptedResponse{ok200(`{"data":null,"errors":[{"type":"NOT_FOUND","message":"Could not resolve to a PullRequest with the number of 7."}]}`)}, "Could not resolve to a PullRequest"}, + {"null repository", []scriptedResponse{ok200(`{"data":{"repository":null}}`)}, "repository"}, + {"null pull request", []scriptedResponse{ok200(`{"data":{"repository":{"pullRequest":null}}}`)}, "pull request"}, + {"no threads connection", []scriptedResponse{ok200(pullPage(t, nil, rollup(false, "c")))}, "review threads connection"}, + {"no commits connection", []scriptedResponse{ok200(pullPage(t, threadsConn(false, "t"), nil))}, "commits connection"}, + {"next page without a cursor", []scriptedResponse{ok200(pullPage(t, threadsConn(true, ""), rollup(false, "c")))}, "without an endCursor"}, + {"repeated cursor", []scriptedResponse{ + ok200(pullPage(t, threadsConn(true, "SAME"), rollup(false, "c"))), + ok200(pullPage(t, threadsConn(true, "SAME"), nil)), + }, "repeats the previous cursor"}, + {"null thread node", []scriptedResponse{ + ok200(pullPage(t, threadsConn(false, "t", longThread), rollup(false, "c"))), + ok200(`{"data":{"node":null}}`), + }, "review thread"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rt := &scriptedRoundTripper{responses: pullRESTLegs("", tc.gql...)} + _, err := newTestGitHub(rt, &fakeTokenSource{token: "t"}).GetPullRequest(context.Background(), "org/repo", 7) + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Fatalf("err = %v, want one containing %q", err, tc.want) + } + if rt.calls != 4+len(tc.gql) { + t.Errorf("calls = %d, want %d", rt.calls, 4+len(tc.gql)) + } + }) + } +} + +// nextPage refuses to loop: no cursor, or the cursor that fetched this page. +func TestNextPage(t *testing.T) { + cases := []struct { + name string + p ghPageInfo + prev any + wantMore bool + wantErr bool + }{ + {"last page", ghPageInfo{EndCursor: "A"}, nil, false, false}, + {"first to second", ghPageInfo{HasNextPage: true, EndCursor: "A"}, nil, true, false}, + {"advancing", ghPageInfo{HasNextPage: true, EndCursor: "B"}, "A", true, false}, + {"no cursor", ghPageInfo{HasNextPage: true}, nil, false, true}, + {"repeated cursor", ghPageInfo{HasNextPage: true, EndCursor: "A"}, "A", false, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + more, _, err := nextPage(tc.p, tc.prev) + if more != tc.wantMore || (err != nil) != tc.wantErr { + t.Errorf("nextPage = (%v, %v), want more=%v err=%v", more, err, tc.wantMore, tc.wantErr) + } + }) + } +} + +// A GraphQL RATE_LIMITED error on HTTP 200 is the budget skip: a +// *RateLimitError carrying the reset hint, with the GraphQL gate armed. +func TestGraphQLRateLimitedOn200(t *testing.T) { + base := time.Date(2026, 8, 9, 12, 0, 0, 0, time.UTC) + reset := strconv.FormatInt(base.Add(40*time.Second).Unix(), 10) + rt := &scriptedRoundTripper{responses: pullRESTLegs("", scriptedResponse{ + status: 200, + body: `{"data":null,"errors":[{"type":"RATE_LIMITED","message":"API rate limit exceeded"}]}`, + headers: map[string]string{"X-Ratelimit-Resource": "graphql", "X-Ratelimit-Remaining": "0", "X-Ratelimit-Reset": reset}, + })} + g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) + g.now = func() time.Time { return base } + + _, err := g.GetPullRequest(context.Background(), "org/repo", 7) + rle, ok := errors.AsType[*RateLimitError](err) + if !ok || !errors.Is(err, ErrBudgetExhausted) { + t.Fatalf("err = %v, want a *RateLimitError", err) + } + if rle.RetryAfter != 40*time.Second { + t.Errorf("RetryAfter = %v, want 40s", rle.RetryAfter) + } + if _, blocked := g.gateBlocked(resourceGraphQL); !blocked { + t.Error("GraphQL gate not armed after RATE_LIMITED") + } + if _, blocked := g.gateBlocked(resourceCore); blocked { + t.Error("core gate armed by a GraphQL rate limit") + } +} + +// With no usable reset header, RATE_LIMITED still arms the GraphQL gate (the +// bounded default skip), so the next GraphQL call fails fast. +func TestGraphQLRateLimitedNoResetArmsDefault(t *testing.T) { + rt := &scriptedRoundTripper{responses: pullRESTLegs("", ok200(`{"errors":[{"type":"RATE_LIMITED","message":"slow down"}]}`))} + g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) + + if _, err := g.GetPullRequest(context.Background(), "org/repo", 7); !errors.Is(err, ErrBudgetExhausted) { + t.Fatalf("err = %v, want ErrBudgetExhausted", err) + } + if _, blocked := g.gateBlocked(resourceGraphQL); !blocked { + t.Error("GraphQL gate not armed with the default skip") + } +} + +// The REST core and GraphQL buckets have separate gates: draining one does not +// block the other, and each still blocks itself. +func TestRateGatePerResource(t *testing.T) { + base := time.Date(2026, 8, 9, 12, 0, 0, 0, time.UTC) + reset := strconv.FormatInt(base.Add(30*time.Second).Unix(), 10) + + t.Run("graphql drained does not block REST", func(t *testing.T) { + rt := &scriptedRoundTripper{responses: []scriptedResponse{ + {status: 200, body: `{"data":{}}`, headers: map[string]string{"X-Ratelimit-Resource": "graphql", "X-Ratelimit-Remaining": "0", "X-Ratelimit-Reset": reset}}, + ok200(`{"number":7,"user":{"login":"a"}}`), + }} + g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) + g.now = func() time.Time { return base } + + if _, err := graphQL[struct{}](context.Background(), g, "query{viewer{login}}", nil); err != nil { + t.Fatalf("graphQL: %v", err) + } + if _, err := g.GetIssue(context.Background(), "org/repo", 7); err != nil { + t.Fatalf("REST GET after a drained GraphQL bucket: %v", err) + } + if _, err := graphQL[struct{}](context.Background(), g, "query{viewer{login}}", nil); !errors.Is(err, ErrBudgetExhausted) { + t.Errorf("second graphQL err = %v, want the GraphQL gate to block itself", err) + } + if rt.calls != 2 { + t.Errorf("calls = %d, want 2 (the blocked GraphQL call sends nothing)", rt.calls) + } + }) + + t.Run("REST rate-limit 403 does not block GraphQL", func(t *testing.T) { + rt := &scriptedRoundTripper{responses: []scriptedResponse{ + {status: 403, body: `{"message":"rate limited"}`, headers: map[string]string{"Retry-After": "60"}}, + ok200(`{"data":{}}`), + }} + g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) + g.now = func() time.Time { return base } + + if _, err := g.GetIssue(context.Background(), "org/repo", 7); !errors.Is(err, ErrBudgetExhausted) { + t.Fatalf("GetIssue err = %v, want ErrBudgetExhausted", err) + } + if _, err := graphQL[struct{}](context.Background(), g, "query{viewer{login}}", nil); err != nil { + t.Fatalf("graphQL after a REST rate limit: %v", err) + } + if _, err := g.GetIssue(context.Background(), "org/repo", 7); !errors.Is(err, ErrBudgetExhausted) { + t.Errorf("second GetIssue err = %v, want the core gate to block itself", err) + } + if rt.calls != 2 { + t.Errorf("calls = %d, want 2 (the blocked REST call sends nothing)", rt.calls) + } + }) + + t.Run("resource header routes a GraphQL POST's 403 to its own gate", func(t *testing.T) { + rt := &scriptedRoundTripper{responses: []scriptedResponse{ + {status: 403, body: `{"message":"rate limited"}`, headers: map[string]string{"Retry-After": "60", "X-Ratelimit-Resource": "graphql"}}, + ok200(`{"number":7,"user":{"login":"a"}}`), + }} + g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) + g.now = func() time.Time { return base } + + if _, err := graphQL[struct{}](context.Background(), g, "query{viewer{login}}", nil); !errors.Is(err, ErrBudgetExhausted) { + t.Fatalf("graphQL err = %v, want ErrBudgetExhausted", err) + } + if _, err := g.GetIssue(context.Background(), "org/repo", 7); err != nil { + t.Fatalf("REST GET after a GraphQL 403: %v", err) + } + }) +} + +// A repo that is not exactly owner/name is rejected before any request. +func TestGetPullRequestMalformedRepo(t *testing.T) { + for _, repo := range []string{"repo", "/repo", "org/", "org/repo/extra"} { + rt := &scriptedRoundTripper{} + g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) + if _, err := g.GetPullRequest(context.Background(), repo, 1); !errors.Is(err, errMalformedRepo) { + t.Errorf("repo %q: err = %v, want errMalformedRepo", repo, err) + } + if _, err := g.Checks(context.Background(), repo, 1); !errors.Is(err, errMalformedRepo) { + t.Errorf("Checks repo %q: err = %v, want errMalformedRepo", repo, err) + } + if rt.calls != 0 { + t.Errorf("repo %q: calls = %d, want 0", repo, rt.calls) + } + } +} + +// The GHES GraphQL endpoint is /api/graphql on the configured host. +func TestGraphQLURLGHES(t *testing.T) { + g := NewGitHub(GitHubConfig{Host: "ghe.example.com", Token: &fakeTokenSource{token: "t"}, Client: &http.Client{}}) + if got := g.graphQLURL(); got != "https://ghe.example.com/api/graphql" { + t.Errorf("graphQLURL = %s", got) + } +} diff --git a/go/internal/forge/github_test.go b/go/internal/forge/github_test.go index 33fa0996e..e7d00cfca 100644 --- a/go/internal/forge/github_test.go +++ b/go/internal/forge/github_test.go @@ -10,6 +10,8 @@ import ( "errors" "io" "net/http" + "reflect" + "slices" "strconv" "strings" "sync" @@ -1050,7 +1052,7 @@ func TestGitHubTransitionRespectsBudgetGate(t *testing.T) { g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) now := time.Now() g.now = func() time.Time { return now } - g.resetAt = now.Add(90 * time.Second) + g.resetAt[resourceCore] = now.Add(90 * time.Second) err := tc.call(g) var rle *RateLimitError @@ -1418,9 +1420,34 @@ func TestGetIssueBudgetGateFailFast(t *testing.T) { // --- read path: GetPullRequest ----------------------------------------------- -// GetPullRequest happy path: the 3-leg composite (detail + reviews + checks), -// merged->"merged" state fold, Changed stats, Reviews (bot/human, verdict -// lowercased), embedded Checks populated, Threads nil, and the endpoint chain. +// happyPullGraphQL is TestGetPullRequestHappy's GraphQL leg: a resolved thread +// with a human and a bot comment, an unresolved PR-level thread, and one +// required check run beside a non-required legacy status. +const happyPullGraphQL = `{"data":{"repository":{ + "pullRequest":{"reviewThreads":{ + "pageInfo":{"hasNextPage":false,"endCursor":"t1"}, + "nodes":[ + {"id":"T1","isResolved":true,"path":"main.go","comments":{ + "pageInfo":{"hasNextPage":false,"endCursor":null}, + "nodes":[ + {"author":{"login":"carol","__typename":"User"},"body":"nit"}, + {"author":{"login":"botly","__typename":"Bot"},"body":"fixed"} + ]}}, + {"id":"T2","isResolved":false,"path":"","comments":{ + "pageInfo":{"hasNextPage":false,"endCursor":null}, + "nodes":[{"author":{"login":"dave","__typename":"User"},"body":"why?"}]}} + ]}, + "commits":{"nodes":[{"commit":{"statusCheckRollup":{"contexts":{ + "pageInfo":{"hasNextPage":false,"endCursor":"c1"}, + "nodes":[ + {"__typename":"CheckRun","name":"build","isRequired":true}, + {"__typename":"StatusContext","context":"legacy-ci","isRequired":false} + ]}}}}]}} +}}}` + +// GetPullRequest happy path: the 5-request composite (detail + reviews + check +// runs + statuses + GraphQL), merged->"merged" state fold, Changed stats, Reviews (bot/human, +// verdict lowercased), embedded Checks with Required from GraphQL, and Threads. func TestGetPullRequestHappy(t *testing.T) { const detailBody = `{ "number": 42, @@ -1452,6 +1479,7 @@ func TestGetPullRequestHappy(t *testing.T) { {status: 200, body: reviewsBody}, {status: 200, body: checkRunsBody}, {status: 200, body: statusBody}, + {status: 200, body: happyPullGraphQL}, }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -1466,6 +1494,7 @@ func TestGetPullRequestHappy(t *testing.T) { "https://api.github.com/repos/org/repo/pulls/42/reviews?per_page=100&page=1", "https://api.github.com/repos/org/repo/commits/abc123/check-runs?per_page=100&page=1", "https://api.github.com/repos/org/repo/commits/abc123/status?per_page=100&page=1", + "https://api.github.com/graphql", } if rt.calls != len(wantURLs) { t.Fatalf("calls = %d, want %d", rt.calls, len(wantURLs)) @@ -1474,8 +1503,18 @@ func TestGetPullRequestHappy(t *testing.T) { if got := rt.requests[i].URL.String(); got != want { t.Errorf("request %d URL = %s, want %s", i, got, want) } - if m := rt.requests[i].Method; m != http.MethodGet { - t.Errorf("request %d method = %s, want GET", i, m) + wantMethod := http.MethodGet + if i == len(wantURLs)-1 { + wantMethod = http.MethodPost + } + if m := rt.requests[i].Method; m != wantMethod { + t.Errorf("request %d method = %s, want %s", i, m, wantMethod) + } + } + gqlBody := readReqBody(t, rt.requests[4]) + for _, want := range []string{`"owner":"org"`, `"name":"repo"`, `"number":42`, `"threads":true`, `"contexts":true`} { + if !strings.Contains(gqlBody, want) { + t.Errorf("graphql body missing %s: %s", want, gqlBody) } } @@ -1504,20 +1543,32 @@ func TestGetPullRequestHappy(t *testing.T) { t.Errorf("Reviews[1] = %+v", got.Reviews[1]) } - // Embedded checks populated (both sources), rolled up to success. + // Embedded checks populated (both sources), rolled up to success; only the + // check GraphQL reports as required is marked Required. if got.Checks.HeadSHA != "abc123" { t.Errorf("Checks.HeadSHA = %q, want abc123", got.Checks.HeadSHA) } if got.Checks.State != "success" { t.Errorf("Checks.State = %q, want success", got.Checks.State) } - if len(got.Checks.Checks) != 2 { - t.Errorf("Checks.Checks len = %d, want 2", len(got.Checks.Checks)) + wantChecks := []Check{ + {Name: "build", State: "success", URL: "https://ci/build", Required: true}, + {Name: "legacy-ci", State: "success", URL: "https://ci/legacy", Required: false}, + } + if !slices.Equal(got.Checks.Checks, wantChecks) { + t.Errorf("Checks.Checks = %+v, want %+v", got.Checks.Checks, wantChecks) } - // Threads left empty on the REST read path. - if got.Threads != nil { - t.Errorf("Threads = %v, want nil", got.Threads) + // Threads: forge order, resolution, path, comments in order with bot flag. + wantThreads := []ReviewThread{ + {Path: "main.go", Resolved: true, Comments: []ThreadComment{ + {Author: "carol", Body: "nit"}, + {Author: "botly[bot]", IsBot: true, Body: "fixed"}, + }}, + {Path: "", Resolved: false, Comments: []ThreadComment{{Author: "dave", Body: "why?"}}}, + } + if !reflect.DeepEqual(got.Threads, wantThreads) { + t.Errorf("Threads = %+v, want %+v", got.Threads, wantThreads) } } @@ -1537,6 +1588,7 @@ func TestGetPullRequestOpenState(t *testing.T) { {status: 200, body: `[]`}, {status: 200, body: `{"check_runs": []}`}, {status: 200, body: `{"statuses": []}`}, + {status: 200, body: emptyPullGraphQL}, }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -1576,7 +1628,8 @@ func TestGetPullRequestLegError(t *testing.T) { // --- read path: Checks ------------------------------------------------------- // Checks happy path: resolves the head SHA from pull detail, folds check-runs + -// combined-status, and rolls up a mixed set (one failure) to "failure". +// combined-status, rolls up a mixed set (one failure) to "failure", and marks +// Required from the GraphQL contexts (threads excluded from that query). func TestChecksMixedFailure(t *testing.T) { const detailBody = `{"number":9,"head":{"sha":"deadbeef"},"base":{"ref":"main"},"user":{"login":"a"}}` const checkRunsBody = `{"check_runs": [ @@ -1586,10 +1639,18 @@ func TestChecksMixedFailure(t *testing.T) { const statusBody = `{"statuses": [ {"context": "coverage", "state": "success", "target_url": "https://ci/cov"} ]}` + const graphQLBody = `{"data":{"repository":{"pullRequest":{"commits":{"nodes":[{"commit":{"statusCheckRollup":{"contexts":{ + "pageInfo":{"hasNextPage":false,"endCursor":"c1"}, + "nodes":[ + {"__typename":"CheckRun","name":"lint","isRequired":true}, + {"__typename":"CheckRun","name":"build","isRequired":false}, + {"__typename":"StatusContext","context":"coverage","isRequired":true} + ]}}}}]}}}}}` rt := &scriptedRoundTripper{responses: []scriptedResponse{ {status: 200, body: detailBody}, {status: 200, body: checkRunsBody}, {status: 200, body: statusBody}, + {status: 200, body: graphQLBody}, }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -1602,6 +1663,7 @@ func TestChecksMixedFailure(t *testing.T) { "https://api.github.com/repos/org/repo/pulls/9", "https://api.github.com/repos/org/repo/commits/deadbeef/check-runs?per_page=100&page=1", "https://api.github.com/repos/org/repo/commits/deadbeef/status?per_page=100&page=1", + "https://api.github.com/graphql", } if rt.calls != len(wantURLs) { t.Fatalf("calls = %d, want %d", rt.calls, len(wantURLs)) @@ -1611,6 +1673,12 @@ func TestChecksMixedFailure(t *testing.T) { t.Errorf("request %d URL = %s, want %s", i, got, want) } } + gqlBody := readReqBody(t, rt.requests[3]) + for _, want := range []string{`"threads":false`, `"contexts":true`, `"number":9`} { + if !strings.Contains(gqlBody, want) { + t.Errorf("graphql body missing %s: %s", want, gqlBody) + } + } if got.HeadSHA != "deadbeef" { t.Errorf("HeadSHA = %q, want deadbeef", got.HeadSHA) @@ -1623,11 +1691,12 @@ func TestChecksMixedFailure(t *testing.T) { if len(got.Checks) != 3 { t.Fatalf("Checks len = %d, want 3", len(got.Checks)) } - // Folded entries in order: check-runs first, then statuses. Required false. + // Folded entries in order: check-runs first, then statuses. Required comes + // from the GraphQL contexts, for check runs and legacy statuses alike. want := []Check{ {Name: "build", State: "success", URL: "https://ci/build", Required: false}, - {Name: "lint", State: "failure", URL: "https://ci/lint", Required: false}, - {Name: "coverage", State: "success", URL: "https://ci/cov", Required: false}, + {Name: "lint", State: "failure", URL: "https://ci/lint", Required: true}, + {Name: "coverage", State: "success", URL: "https://ci/cov", Required: true}, } for i, w := range want { if got.Checks[i] != w { @@ -1647,6 +1716,7 @@ func TestChecksPending(t *testing.T) { {status: 200, body: detailBody}, {status: 200, body: checkRunsBody}, {status: 200, body: `{"statuses": []}`}, + {status: 200, body: noRollupGraphQL}, }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -1670,6 +1740,7 @@ func TestChecksAllSuccess(t *testing.T) { {status: 200, body: detailBody}, {status: 200, body: `{"check_runs": [{"name":"build","status":"completed","conclusion":"success","html_url":""}]}`}, {status: 200, body: `{"statuses": [{"context":"cov","state":"success","target_url":""}]}`}, + {status: 200, body: noRollupGraphQL}, }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -1716,6 +1787,7 @@ func TestChecksFollowsPagination(t *testing.T) { }}, {status: 200, body: page2}, {status: 200, body: `{"statuses": []}`}, + {status: 200, body: noRollupGraphQL}, }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -1723,9 +1795,9 @@ func TestChecksFollowsPagination(t *testing.T) { if err != nil { t.Fatalf("Checks: %v", err) } - // detail + 2 check-run pages + 1 status page. - if rt.calls != 4 { - t.Fatalf("calls = %d, want 4 (detail + 2 check-run pages + status)", rt.calls) + // detail + 2 check-run pages + 1 status page + GraphQL. + if rt.calls != 5 { + t.Fatalf("calls = %d, want 5 (detail + 2 check-run pages + status + graphql)", rt.calls) } wantURLs := []string{ "https://api.github.com/repos/org/repo/pulls/9", @@ -1756,6 +1828,7 @@ func TestChecksUnknownNonTerminalStatusPending(t *testing.T) { {status: 200, body: detailBody}, {status: 200, body: `{"check_runs": [{"name":"gate","status":"waiting","conclusion":"","html_url":""}]}`}, {status: 200, body: `{"statuses": []}`}, + {status: 200, body: noRollupGraphQL}, }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -1783,6 +1856,7 @@ func TestChecksCancelledIsFailure(t *testing.T) { {"name":"deploy","status":"completed","conclusion":"cancelled","html_url":""} ]}`}, {status: 200, body: `{"statuses": []}`}, + {status: 200, body: noRollupGraphQL}, }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -1807,6 +1881,7 @@ func TestChecksNeutralRollsUpSuccess(t *testing.T) { {status: 200, body: detailBody}, {status: 200, body: `{"check_runs": [{"name":"advisory","status":"completed","conclusion":"neutral","html_url":""}]}`}, {status: 200, body: `{"statuses": []}`}, + {status: 200, body: noRollupGraphQL}, }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -1830,6 +1905,7 @@ func TestChecksLegacyErrorStatusIsFailure(t *testing.T) { {status: 200, body: detailBody}, {status: 200, body: `{"check_runs": []}`}, {status: 200, body: `{"statuses": [{"context":"legacy","state":"error","target_url":""}]}`}, + {status: 200, body: noRollupGraphQL}, }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) diff --git a/go/internal/forge/golden_capture_test.go b/go/internal/forge/golden_capture_test.go index 155846428..6d5341128 100644 --- a/go/internal/forge/golden_capture_test.go +++ b/go/internal/forge/golden_capture_test.go @@ -29,14 +29,15 @@ import ( // so a request derived from canonicalized coordinates and a Want derived from a // canonicalized response agree (e.g. request head == response head.ref). const ( - canonAccount = "octocat" // login, displayName -> ForgeAccount/Author - canonURL = "https://example.invalid/canonical" // html_url, url, target_url -> URL - canonIDString = "canonical-id" // a string/UUID id (Linear) -> ID / resolve coordinate - canonUpdatedAt = "2026-08-01T12:30:00Z" // updated_at, updatedAt -> UpdatedAt - canonSHA = "canonicalsha" // sha -> HeadSHA - canonRef = "canonical-ref" // ref -> HeadRef/BaseRef - canonTitle = "canonical title" // title -> Title - canonBody = "canonical body" // body, description -> Body + canonAccount = "octocat" // login, displayName -> ForgeAccount/Author + canonURL = "https://example.invalid/canonical" // html_url, url, target_url -> URL + canonIDString = "canonical-id" // a string/UUID id (Linear) -> ID / resolve coordinate + canonUpdatedAt = "2026-08-01T12:30:00Z" // updated_at, updatedAt -> UpdatedAt + canonSHA = "canonicalsha" // sha -> HeadSHA + canonRef = "canonical-ref" // ref -> HeadRef/BaseRef + canonTitle = "canonical title" // title -> Title + canonBody = "canonical body" // body, description -> Body + canonCursorValue = "canonical-cursor" // endCursor (GraphQL paging; no domain field) ) // canonNumber is the numeric sentinel (number, numeric id) as a float64 — the @@ -83,8 +84,8 @@ var volatileFields = map[string]struct{}{ // value during canonicalization. Each entry is a func of the current node so a // key whose JSON type varies (id: numeric on GitHub, a UUID string on Linear) // picks a type-appropriate sentinel; the fixed-value keys ignore the node. The -// keyset is the union of the wire keys every domainToWire entry references -// (asserted coherent in TestUpdateCanonicalizeCoversVolatileFields). +// keyset is the union of the wire keys every domainToWire entry references plus +// wireOnlyVolatile (asserted coherent in TestUpdateCanonicalizeCoversVolatileFields). var wireVolatile = map[string]func(node any) any{ "number": fixedSentinel(canonNumber), "id": canonID, @@ -100,6 +101,23 @@ var wireVolatile = map[string]func(node any) any{ "title": fixedSentinel(canonTitle), "body": fixedSentinel(canonBody), "description": fixedSentinel(canonBody), + "endCursor": canonCursor, +} + +// wireOnlyVolatile names wire keys that change per capture but decode into no +// domain field, so they have no domainToWire entry. A GraphQL endCursor is one: +// it only feeds the next page's request. +var wireOnlyVolatile = map[string]struct{}{ + "endCursor": {}, +} + +// canonCursor sentinels a GraphQL endCursor. A null stays null: it marks a last +// page, and replay must still see the connection end. +func canonCursor(node any) any { + if node == nil { + return nil + } + return canonCursorValue } // domainToWire maps each forge DOMAIN volatile key (the Go field names in @@ -117,9 +135,10 @@ var wireVolatile = map[string]func(node any) any{ // - HeadRef <- ref (ghPull/ghPullDetail.Head.Ref) // - BaseRef <- ref (ghPull/ghPullDetail.Base.Ref) — same wire key as HeadRef // - ForgeAccount <- login,displayName (GitHub user.login; Linear creator/user displayName) -// - Author <- login (ghReviewRow.User.Login on the reviews leg) +// - Author <- login (ghReviewRow.User.Login on the reviews leg; ghGQLActor.Login on +// the GraphQL thread comments, which gets a "[bot]" suffix for a Bot) // - Title <- title -// - Body <- body,description (GitHub body; Linear description) +// - Body <- body,description (GitHub body, incl. GraphQL thread comments; Linear description) var domainToWire = map[string][]string{ "Number": {"number"}, "ID": {"id"}, @@ -352,9 +371,9 @@ func flattenQuery(q url.Values) map[string]string { // TestUpdateCanonicalizeCoversVolatileFields is the "single source of truth" tie: // domainToWire's keyset MUST equal the oracle's volatileFields, and the two tables // MUST agree on the wire keys — every key domainToWire references has a -// wireVolatile sentinel, and every sentinel is referenced. A new domain volatile -// added to volatileFields without a wire mapping — or a wireVolatile key no domain -// field decodes from — fails here. +// wireVolatile sentinel, and every sentinel is referenced by a domain field or +// listed in wireOnlyVolatile. A new domain volatile added to volatileFields +// without a wire mapping — or an unexplained wireVolatile key — fails here. func TestUpdateCanonicalizeCoversVolatileFields(t *testing.T) { got := make(map[string]struct{}, len(domainToWire)) for k := range domainToWire { @@ -366,6 +385,9 @@ func TestUpdateCanonicalizeCoversVolatileFields(t *testing.T) { } referenced := map[string]struct{}{} + for w := range wireOnlyVolatile { + referenced[w] = struct{}{} + } for domain, wires := range domainToWire { for _, w := range wires { referenced[w] = struct{}{} @@ -376,7 +398,7 @@ func TestUpdateCanonicalizeCoversVolatileFields(t *testing.T) { } for w := range wireVolatile { if _, ok := referenced[w]; !ok { - t.Errorf("wireVolatile has key %q not referenced by any domainToWire entry", w) + t.Errorf("wireVolatile has key %q not referenced by any domainToWire entry or wireOnlyVolatile", w) } } } @@ -403,6 +425,7 @@ func TestUpdateCanonicalizeStable(t *testing.T) { "number": 1, "id": 2, "html_url": "h", "url": "u", "target_url": "t", "updated_at": "a", "updatedAt": "b", "login": "l", "displayName": "d", "sha": "s", "ref": "r", "title": "ti", "body": "bo", "description": "de", + "endCursor": "Y3Vyc29y", "last": { "endCursor": null }, "state": "open", "keep": "kept" }`) wantAll := json.RawMessage(`{ @@ -411,7 +434,8 @@ func TestUpdateCanonicalizeStable(t *testing.T) { "updated_at": "2026-08-01T12:30:00Z", "updatedAt": "2026-08-01T12:30:00Z", "login": "octocat", "displayName": "octocat", "sha": "canonicalsha", "ref": "canonical-ref", "title": "canonical title", "body": "canonical body", - "description": "canonical body", "state": "open", "keep": "kept" + "description": "canonical body", "endCursor": "canonical-cursor", "last": { "endCursor": null }, + "state": "open", "keep": "kept" }`) assertJSONEqual(t, "canonicalized every wire-volatile key", canonicalizeWire(all), wantAll) @@ -512,12 +536,13 @@ func TestUpdateCanonicalizeStable(t *testing.T) { // TestUpdateCanonicalizeComposite is the credential-free guard for the single // riskiest capture: get_pull_request, the only fixture with EXTRA legs. It runs // a synthetic live capture — a PR detail GET (asserted) followed by the reviews, -// check_runs, and legacy statuses legs (Extra) — through the REAL assembleFixture -// -> replayFixture pipeline. This exercises what create_issue cannot: the -// responses[prelude+1:] Extra assembly, canonNode's []any recursion into an -// array of objects that themselves carry volatile keys (the two reviews, each an -// Author<-user.login), and volatile substitution across separate legs -// (target_url on the statuses leg, sha on the detail leg vs Checks.HeadSHA). +// check_runs, legacy statuses, and GraphQL threads+contexts legs (Extra) — +// through the REAL assembleFixture -> replayFixture pipeline. This exercises +// what create_issue cannot: the responses[prelude+1:] Extra assembly, +// canonNode's []any recursion into arrays of objects that themselves carry +// volatile keys (the two reviews and the thread comments, each an +// Author<-login), and volatile substitution across separate legs (target_url on +// the statuses leg, sha on the detail leg vs Checks.HeadSHA). func TestUpdateCanonicalizeComposite(t *testing.T) { coords := fixtureRequest{Op: "get_pull_request", Repo: "org/repo", Number: 98765} responses := []capturedResponse{ @@ -544,14 +569,29 @@ func TestUpdateCanonicalizeComposite(t *testing.T) { {status: 200, body: json.RawMessage(`{ "statuses": [ { "context": "legacy-ci", "state": "success", "target_url": "https://ci/live-legacy" } ] }`)}, + // extra leg 4: GraphQL threads + required contexts — a bot comment login + // and body inside nested arrays, a thread node id, and paging cursors. + {status: 200, body: json.RawMessage(`{ "data": { "repository": { "pullRequest": { + "reviewThreads": { "pageInfo": { "hasNextPage": false, "endCursor": "live-cursor" }, "nodes": [ + { "id": "PRRT_live", "isResolved": true, "path": "main.go", "comments": { + "pageInfo": { "hasNextPage": false, "endCursor": "live-c" }, + "nodes": [ { "author": { "login": "dave-live", "__typename": "Bot" }, "body": "live nit" } ] } } + ] }, + "commits": { "nodes": [ { "commit": { "statusCheckRollup": { "contexts": { + "pageInfo": { "hasNextPage": false, "endCursor": "MQ" }, + "nodes": [ { "__typename": "CheckRun", "name": "build", "isRequired": true } ] } } } } ] } + } } } }`)}, } // assembleFixture derives BOTH halves by replay; replayFixture then re-asserts - // the emitted requests (all 4 legs, exact count) and decode(Body)==Want. A + // the emitted requests (all 5 legs, exact count) and decode(Body)==Want. A // regression in Extra-leg assembly or array-of-volatile-objects recursion // fails here credential-free instead of only at live -update time. f := assembleFixture(t, providerGitHub, "get_pull_request_probe", 0, coords, responses) replayFixture(t, providerGitHub, f) + if strings.Contains(string(f.Response.Extra[3].Body), "live-cursor") { + t.Errorf("GraphQL endCursor not canonicalized: %s", f.Response.Extra[3].Body) + } // Want is marshal(decode(Body)); domain types carry no json tags, so they // marshal to their Go field names verbatim. Decode into a local tagged struct @@ -566,10 +606,17 @@ func TestUpdateCanonicalizeComposite(t *testing.T) { Checks struct { HeadSHA string `json:"HeadSHA"` Checks []struct { - Name string `json:"Name"` - URL string `json:"URL"` + Name string `json:"Name"` + URL string `json:"URL"` + Required bool `json:"Required"` } `json:"Checks"` } `json:"Checks"` + Threads []struct { + Comments []struct { + Author string `json:"Author"` + IsBot bool `json:"IsBot"` + } `json:"Comments"` + } `json:"Threads"` } if err := json.Unmarshal(f.Response.Want, &pr); err != nil { t.Fatalf("unmarshal derived get_pull_request Want: %v (%s)", err, f.Response.Want) @@ -611,6 +658,18 @@ func TestUpdateCanonicalizeComposite(t *testing.T) { if pr.Number != uint64(canonNumber) { t.Errorf("derived Number = %d, want %d (canonicalized)", pr.Number, uint64(canonNumber)) } + + // (e) the GraphQL leg: the thread comment's login canonicalizes and keeps the + // bot suffix, and the required context still marks its check. + if len(pr.Threads) != 1 || len(pr.Threads[0].Comments) != 1 { + t.Fatalf("derived Threads = %+v, want one thread with one comment", pr.Threads) + } + if c := pr.Threads[0].Comments[0]; c.Author != canonAccount+"[bot]" || !c.IsBot { + t.Errorf("thread comment = %+v, want %q bot", c, canonAccount+"[bot]") + } + if !pr.Checks.Checks[0].Required || pr.Checks.Checks[1].Required { + t.Errorf("derived Checks = %+v, want only build required", pr.Checks.Checks) + } } // sortedKeys returns a set's keys in sorted order for a stable failure message. diff --git a/go/internal/forge/golden_test.go b/go/internal/forge/golden_test.go index def01fc5d..c70bd2063 100644 --- a/go/internal/forge/golden_test.go +++ b/go/internal/forge/golden_test.go @@ -116,7 +116,7 @@ type fixtureStep struct { // after (Extra) it, and the expected decoded domain value (Want). Prelude // covers a provider's resolve/probe round-trips (Linear team-id + actor probe); // Extra covers a composite read's follow-on fetches (GitHub GetPullRequest's -// reviews + checks legs). The asserted request is the one at index len(Prelude). +// reviews, check-run, status and GraphQL legs). The asserted request is the one at index len(Prelude). type fixtureResponse struct { Status int `json:"status"` Headers map[string]string `json:"headers,omitempty"` diff --git a/go/internal/forge/linear.go b/go/internal/forge/linear.go index fcd796ade..826ccc929 100644 --- a/go/internal/forge/linear.go +++ b/go/internal/forge/linear.go @@ -930,10 +930,12 @@ type graphQLResponse struct { } // graphQLError is one entry of the GraphQL `errors` array. The extensions.code -// discriminates a rate-limit ("RATELIMITED") or auth ("AUTHENTICATION_ERROR") -// failure from an ordinary one. +// discriminates a Linear rate-limit ("RATELIMITED") or auth +// ("AUTHENTICATION_ERROR") failure from an ordinary one; GitHub uses Type +// instead (e.g. "RATE_LIMITED"). type graphQLError struct { Message string `json:"message"` + Type string `json:"type"` Extensions struct { Code string `json:"code"` } `json:"extensions"` diff --git a/go/internal/forge/notify_reader.go b/go/internal/forge/notify_reader.go index 05528a2cd..eb28e7804 100644 --- a/go/internal/forge/notify_reader.go +++ b/go/internal/forge/notify_reader.go @@ -106,7 +106,7 @@ var ( // is present. On error it owns the budget decision via mapErrorResponse (no // budget record on error — the ListIssuesPage/getJSON rule). func (g *GitHub) getJSONCond(ctx context.Context, url, etag string, out any) (notModified bool, newETag string, hasNext bool, err error) { - if hint, blocked := g.gateBlocked(); blocked { + if hint, blocked := g.gateBlocked(resourceCore); blocked { return false, "", false, fmt.Errorf("GET %s: %w", url, &RateLimitError{RetryAfter: hint}) } token, terr := g.token.Token(ctx) @@ -131,12 +131,12 @@ func (g *GitHub) getJSONCond(ctx context.Context, url, etag string, out any) (no switch { case resp.StatusCode == http.StatusNotModified: - g.recordBudget(resp) + g.recordBudget(resp, resourceCore) return true, "", false, nil case resp.StatusCode >= 200 && resp.StatusCode < 300: - g.recordBudget(resp) + g.recordBudget(resp, resourceCore) default: - return false, "", false, g.mapErrorResponse(resp) + return false, "", false, g.mapErrorResponse(resp, resourceCore) } body, berr := io.ReadAll(resp.Body) diff --git a/go/internal/forge/testdata/github/get_pull_request.json b/go/internal/forge/testdata/github/get_pull_request.json index fdc4f9213..e13b16b6e 100644 --- a/go/internal/forge/testdata/github/get_pull_request.json +++ b/go/internal/forge/testdata/github/get_pull_request.json @@ -64,6 +64,94 @@ } ] } + }, + { + "status": 200, + "body": { + "data": { + "repository": { + "pullRequest": { + "reviewThreads": { + "pageInfo": { + "hasNextPage": false, + "endCursor": "Y3Vyc29yOnYyOpHOAAAAAQ==" + }, + "nodes": [ + { + "id": "PRRT_kwDOAAAAAc4AAAAB", + "isResolved": true, + "path": "main.go", + "comments": { + "pageInfo": { + "hasNextPage": false, + "endCursor": "Y3Vyc29yOnYyOpHOAAAAAg==" + }, + "nodes": [ + { + "author": { + "login": "carol", + "__typename": "User" + }, + "body": "nit: rename" + }, + { + "author": { "login": "botly", "__typename": "Bot" }, + "body": "done" + } + ] + } + }, + { + "id": "PRRT_kwDOAAAAAc4AAAAC", + "isResolved": false, + "path": "README.md", + "comments": { + "pageInfo": { + "hasNextPage": false, + "endCursor": "Y3Vyc29yOnYyOpHOAAAAAw==" + }, + "nodes": [ + { + "author": null, + "body": "left by a deleted account" + } + ] + } + } + ] + }, + "commits": { + "nodes": [ + { + "commit": { + "statusCheckRollup": { + "contexts": { + "pageInfo": { + "hasNextPage": false, + "endCursor": "MQ" + }, + "nodes": [ + { + "__typename": "CheckRun", + "name": "build", + "isRequired": true + }, + { + "__typename": "StatusContext", + "context": "legacy-ci", + "isRequired": false + } + ] + } + } + } + } + ] + } + } + } + } + } } ], "want": { @@ -85,7 +173,7 @@ "Name": "build", "State": "success", "URL": "https://ci/build", - "Required": false + "Required": true }, { "Name": "legacy-ci", @@ -109,7 +197,27 @@ "Body": "needs work" } ], - "Threads": null + "Threads": [ + { + "Path": "main.go", + "Resolved": true, + "Comments": [ + { "Author": "carol", "IsBot": false, "Body": "nit: rename" }, + { "Author": "botly[bot]", "IsBot": true, "Body": "done" } + ] + }, + { + "Path": "README.md", + "Resolved": false, + "Comments": [ + { + "Author": "", + "IsBot": false, + "Body": "left by a deleted account" + } + ] + } + ] } } } From ed7f0a79b0d4d484855a812d56b0b8fedca35b78 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sat, 26 Sep 2026 07:16:43 -0400 Subject: [PATCH 2/3] fix(forge): pin GraphQL check contexts to the REST head SHA (RIG-1728) The contexts read through the PR's last commit now select its oid. Each page fails with errHeadMoved when that oid differs from the REST head SHA. Before this, a push between the reads could clear a failing required check's flag. Refs RIG-1728 Co-authored-by: Matt Wilkinson --- go/internal/forge/github_graphql.go | 53 +++++---- go/internal/forge/github_graphql_test.go | 102 +++++++++++++----- go/internal/forge/github_test.go | 20 ++-- go/internal/forge/golden_capture_test.go | 13 ++- .../testdata/github/get_pull_request.json | 1 + 5 files changed, 127 insertions(+), 62 deletions(-) diff --git a/go/internal/forge/github_graphql.go b/go/internal/forge/github_graphql.go index beae277a4..45e74f602 100644 --- a/go/internal/forge/github_graphql.go +++ b/go/internal/forge/github_graphql.go @@ -19,6 +19,10 @@ import ( // exactly "owner/name"; GraphQL takes the two halves as separate variables. var errMalformedRepo = errors.New("forge: malformed github repo, want owner/name") +// errHeadMoved means the PR head changed during a read, so the REST checks and +// the GraphQL contexts describe different commits. The caller re-reads. +var errHeadMoved = errors.New("forge: github pull request head moved during read") + // ghGraphQLRateLimited is the errors[].type GitHub returns, on HTTP 200, when // the GraphQL budget is spent. const ghGraphQLRateLimited = "RATE_LIMITED" @@ -157,11 +161,15 @@ type ghGQLRollup struct { // ghGQLPullCommits is the PR's last commit (commits(last: 1)). type ghGQLPullCommits struct { - Nodes []struct { - Commit struct { - StatusCheckRollup *ghGQLRollup `json:"statusCheckRollup"` - } `json:"commit"` - } `json:"nodes"` + Nodes []ghGQLCommitNode `json:"nodes"` +} + +// ghGQLCommitNode is one PullRequestCommit; oid pins the rollup to a head SHA. +type ghGQLCommitNode struct { + Commit struct { + OID string `json:"oid"` + StatusCheckRollup *ghGQLRollup `json:"statusCheckRollup"` + } `json:"commit"` } // ghGQLPull is the pull request half; each connection is absent when excluded. @@ -189,10 +197,8 @@ type ghGQLThreadNode struct { // pullReadQuery fetches review threads and required contexts in one call. The // @include flags let a later page fetch only the connection still paging. The -// contexts are read through the pull request (commits(last: 1)), which needs -// only Pull requests: read, not the Contents access a git-object read would. -// Required-ness depends on the context name and the base-branch rules, not on -// the commit, so the last commit need not equal the REST head SHA. +// contexts are read through the pull request (commits(last: 1)), so only Pull +// requests: read is needed; the commit oid pins each page to the REST head SHA. const pullReadQuery = `query($owner: String!, $name: String!, $number: Int!, $threads: Boolean!, $threadsAfter: String, $contexts: Boolean!, $contextsAfter: String) { repository(owner: $owner, name: $name) { pullRequest(number: $number) { @@ -211,6 +217,7 @@ const pullReadQuery = `query($owner: String!, $name: String!, $number: Int!, $th commits(last: 1) @include(if: $contexts) { nodes { commit { + oid statusCheckRollup { contexts(first: 100, after: $contextsAfter) { pageInfo { hasNextPage endCursor } @@ -258,7 +265,7 @@ func (g *GitHub) checksForPull(ctx context.Context, c pullCoord, sha string, wit if err != nil { return Checks{}, nil, err } - threads, required, err := g.pullGraphQL(ctx, c, withThreads) + threads, required, err := g.pullGraphQL(ctx, c, sha, withThreads) if err != nil { return Checks{}, nil, fmt.Errorf("forge: github graphql for %q#%d: %w", c.repo, c.number, err) } @@ -273,6 +280,7 @@ func (g *GitHub) checksForPull(ctx context.Context, c pullCoord, sha string, wit // pullGraphQLWalk is the cursor state of one pullGraphQL walk. A connection // that has finished drops out of later queries through its @include flag. type pullGraphQLWalk struct { + sha string // REST head SHA every contexts page must match threads []ReviewThread required map[string]struct{} threadsAfter, contextsAfter any // nil sends JSON null: the first page @@ -280,11 +288,11 @@ type pullGraphQLWalk struct { } // pullGraphQL walks pullReadQuery to completion: every review-thread page (when -// withThreads) and every context page. It returns the threads in forge order and -// the set of required context names (a CheckRun's name, a StatusContext's -// context), which match the REST check names. -func (g *GitHub) pullGraphQL(ctx context.Context, c pullCoord, withThreads bool) ([]ReviewThread, map[string]struct{}, error) { - w := pullGraphQLWalk{required: map[string]struct{}{}, moreThreads: withThreads, moreContexts: true} +// withThreads) and every context page of head commit sha. It returns the threads +// in forge order and the set of required context names (a CheckRun's name, a +// StatusContext's context), which match the REST check names. +func (g *GitHub) pullGraphQL(ctx context.Context, c pullCoord, sha string, withThreads bool) ([]ReviewThread, map[string]struct{}, error) { + w := pullGraphQLWalk{sha: sha, required: map[string]struct{}{}, moreThreads: withThreads, moreContexts: true} for w.moreThreads || w.moreContexts { if err := ctx.Err(); err != nil { return nil, nil, err @@ -339,16 +347,25 @@ func (g *GitHub) foldThreads(ctx context.Context, w *pullGraphQLWalk, c pullCoor // foldContexts adds one page of required context names to w and advances its // cursor. A PR with no commits, or a last commit with no checks or statuses -// (a null rollup), has nothing required. +// (a null rollup), has nothing required. The oid check runs first, so a newer +// head's null rollup cannot clear the old head's required checks. func foldContexts(w *pullGraphQLWalk, c pullCoord, commits *ghGQLPullCommits) error { if commits == nil { return fmt.Errorf("forge: github graphql: pull request %q#%d has no commits connection", c.repo, c.number) } - if len(commits.Nodes) == 0 || commits.Nodes[0].Commit.StatusCheckRollup == nil { + if len(commits.Nodes) == 0 { + w.moreContexts = false + return nil + } + commit := commits.Nodes[0].Commit + if commit.OID != w.sha { + return fmt.Errorf("%w: checks read at %s, contexts at %s", errHeadMoved, w.sha, commit.OID) + } + if commit.StatusCheckRollup == nil { w.moreContexts = false return nil } - contexts := commits.Nodes[0].Commit.StatusCheckRollup.Contexts + contexts := commit.StatusCheckRollup.Contexts for _, cx := range contexts.Nodes { if !cx.IsRequired { continue diff --git a/go/internal/forge/github_graphql_test.go b/go/internal/forge/github_graphql_test.go index 6ccbcf6f3..493a8e754 100644 --- a/go/internal/forge/github_graphql_test.go +++ b/go/internal/forge/github_graphql_test.go @@ -18,15 +18,23 @@ import ( "time" ) -// noRollupGraphQL is a Checks GraphQL leg (threads excluded) whose last commit -// has no status-check rollup, so nothing is required. -const noRollupGraphQL = `{"data":{"repository":{"pullRequest":{"commits":{"nodes":[{"commit":{"statusCheckRollup":null}}]}}}}}` +// headSHA is the REST head SHA of the scripted PRs; a contexts page for any +// other oid is a moved head. +const headSHA = "s7" + +// noRollupGraphQL is a Checks GraphQL leg (threads excluded) whose last commit, +// head sha, has no status-check rollup, so nothing is required. +func noRollupGraphQL(sha string) string { + return `{"data":{"repository":{"pullRequest":{"commits":{"nodes":[{"commit":{"oid":"` + sha + `","statusCheckRollup":null}}]}}}}}` +} // emptyPullGraphQL is a GetPullRequest GraphQL leg with no threads and no -// status-check rollup on the last commit. -const emptyPullGraphQL = `{"data":{"repository":{"pullRequest":{ +// status-check rollup on the last commit, head sha. +func emptyPullGraphQL(sha string) string { + return `{"data":{"repository":{"pullRequest":{ "reviewThreads":{"pageInfo":{"hasNextPage":false,"endCursor":null},"nodes":[]}, - "commits":{"nodes":[{"commit":{"statusCheckRollup":null}}]}}}}}` + "commits":{"nodes":[{"commit":{"oid":"` + sha + `","statusCheckRollup":null}}]}}}}}` +} // gqlBody marshals a GraphQL data value into a response body. func gqlBody(t *testing.T, data any) string { @@ -60,15 +68,12 @@ func comment(login, kind, body string) ghGQLComment { return ghGQLComment{Author: &ghGQLActor{Login: login, Typename: kind}, Body: body} } -// rollup is one page of status contexts on the PR's last commit. -func rollup(next bool, cursor string, nodes ...ghGQLContext) *ghGQLPullCommits { - c := &ghGQLPullCommits{Nodes: make([]struct { - Commit struct { - StatusCheckRollup *ghGQLRollup `json:"statusCheckRollup"` - } `json:"commit"` - }, 1)} - c.Nodes[0].Commit.StatusCheckRollup = &ghGQLRollup{Contexts: ghGQLContexts{PageInfo: ghPageInfo{HasNextPage: next, EndCursor: cursor}, Nodes: nodes}} - return c +// rollup is one page of status contexts on the PR's last commit, oid. +func rollup(oid string, next bool, cursor string, nodes ...ghGQLContext) *ghGQLPullCommits { + var n ghGQLCommitNode + n.Commit.OID = oid + n.Commit.StatusCheckRollup = &ghGQLRollup{Contexts: ghGQLContexts{PageInfo: ghPageInfo{HasNextPage: next, EndCursor: cursor}, Nodes: nodes}} + return &ghGQLPullCommits{Nodes: []ghGQLCommitNode{n}} } // checkRun is a CheckRun context; statusContext is a legacy StatusContext. @@ -84,7 +89,7 @@ func statusContext(name string, required bool) ghGQLContext { // check runs and no reviews, ahead of the GraphQL responses a test appends. func pullRESTLegs(checkRuns string, gql ...scriptedResponse) []scriptedResponse { return append([]scriptedResponse{ - {status: 200, body: `{"number":7,"state":"open","head":{"ref":"f","sha":"s7"},"base":{"ref":"main"},"user":{"login":"a"}}`}, + {status: 200, body: `{"number":7,"state":"open","head":{"ref":"f","sha":"` + headSHA + `"},"base":{"ref":"main"},"user":{"login":"a"}}`}, {status: 200, body: `[]`}, {status: 200, body: `{"check_runs": [` + checkRuns + `]}`}, {status: 200, body: `{"statuses": []}`}, @@ -99,7 +104,7 @@ func ok200(body string) scriptedResponse { return scriptedResponse{status: 200, // and both pages' threads are returned in order. func TestGetPullRequestThreadPagination(t *testing.T) { rt := &scriptedRoundTripper{responses: pullRESTLegs("", - ok200(pullPage(t, threadsConn(true, "CUR1", thread("T1", "a.go", false, comment("x", "User", "one"))), rollup(false, "c"))), + ok200(pullPage(t, threadsConn(true, "CUR1", thread("T1", "a.go", false, comment("x", "User", "one"))), rollup(headSHA, false, "c"))), ok200(pullPage(t, threadsConn(false, "CUR2", thread("T2", "b.go", true, comment("y", "User", "two"))), nil)), )} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -136,8 +141,8 @@ func TestGetPullRequestThreadsAndContextsPageTogether(t *testing.T) { runs := `{"name":"build","status":"completed","conclusion":"success","html_url":""}, {"name":"rollup","status":"completed","conclusion":"success","html_url":""}` rt := &scriptedRoundTripper{responses: pullRESTLegs(runs, - ok200(pullPage(t, threadsConn(true, "T-1", thread("A", "a.go", false, comment("x", "User", "1"))), rollup(true, "C-1", checkRun("build", false)))), - ok200(pullPage(t, threadsConn(true, "T-2", thread("B", "b.go", false, comment("x", "User", "2"))), rollup(false, "C-2", checkRun("rollup", true)))), + ok200(pullPage(t, threadsConn(true, "T-1", thread("A", "a.go", false, comment("x", "User", "1"))), rollup(headSHA, true, "C-1", checkRun("build", false)))), + ok200(pullPage(t, threadsConn(true, "T-2", thread("B", "b.go", false, comment("x", "User", "2"))), rollup(headSHA, false, "C-2", checkRun("rollup", true)))), ok200(pullPage(t, threadsConn(false, "T-3", thread("C", "c.go", false, comment("x", "User", "3"))), nil)), )} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -170,14 +175,14 @@ func TestGetPullRequestThreadsAndContextsPageTogether(t *testing.T) { // still marks its check, and page 2 is requested with the page-1 cursor. func TestChecksRequiredContextPagination(t *testing.T) { rt := &scriptedRoundTripper{responses: []scriptedResponse{ - ok200(`{"number":9,"head":{"sha":"s"},"base":{"ref":"main"},"user":{"login":"a"}}`), + ok200(`{"number":9,"head":{"sha":"` + headSHA + `"},"base":{"ref":"main"},"user":{"login":"a"}}`), ok200(`{"check_runs": [ {"name":"build","status":"completed","conclusion":"success","html_url":""}, {"name":"rollup","status":"completed","conclusion":"success","html_url":""} ]}`), ok200(`{"statuses": []}`), - ok200(pullPage(t, nil, rollup(true, "CX1", checkRun("build", false)))), - ok200(pullPage(t, nil, rollup(false, "CX2", checkRun("rollup", true)))), + ok200(pullPage(t, nil, rollup(headSHA, true, "CX1", checkRun("build", false)))), + ok200(pullPage(t, nil, rollup(headSHA, false, "CX2", checkRun("rollup", true)))), }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -196,12 +201,12 @@ func TestChecksRequiredContextPagination(t *testing.T) { // A required StatusContext marks the legacy status of the same name, and a PR // whose commits list is empty has nothing required. func TestChecksRequiredContextShapes(t *testing.T) { - detail := ok200(`{"number":9,"head":{"sha":"s"},"base":{"ref":"main"},"user":{"login":"a"}}`) + detail := ok200(`{"number":9,"head":{"sha":"` + headSHA + `"},"base":{"ref":"main"},"user":{"login":"a"}}`) status := ok200(`{"statuses": [{"context":"legacy","state":"success","target_url":""}]}`) rt := &scriptedRoundTripper{responses: []scriptedResponse{ detail, ok200(`{"check_runs": []}`), status, - ok200(pullPage(t, nil, rollup(false, "c", statusContext("legacy", true)))), + ok200(pullPage(t, nil, rollup(headSHA, false, "c", statusContext("legacy", true)))), }} got, err := newTestGitHub(rt, &fakeTokenSource{token: "t"}).Checks(context.Background(), "org/repo", 9) if err != nil { @@ -234,7 +239,7 @@ func TestGetPullRequestThreadCommentPagination(t *testing.T) { Nodes: []ghGQLComment{comment("y", ghTypeBot, "second")}, }}}) rt := &scriptedRoundTripper{responses: pullRESTLegs("", - ok200(pullPage(t, threadsConn(false, "t", long), rollup(false, "c"))), + ok200(pullPage(t, threadsConn(false, "t", long), rollup(headSHA, false, "c"))), ok200(more), )} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -286,15 +291,15 @@ func TestPullGraphQLErrorBranches(t *testing.T) { {"errors array", []scriptedResponse{ok200(`{"data":null,"errors":[{"type":"NOT_FOUND","message":"Could not resolve to a PullRequest with the number of 7."}]}`)}, "Could not resolve to a PullRequest"}, {"null repository", []scriptedResponse{ok200(`{"data":{"repository":null}}`)}, "repository"}, {"null pull request", []scriptedResponse{ok200(`{"data":{"repository":{"pullRequest":null}}}`)}, "pull request"}, - {"no threads connection", []scriptedResponse{ok200(pullPage(t, nil, rollup(false, "c")))}, "review threads connection"}, + {"no threads connection", []scriptedResponse{ok200(pullPage(t, nil, rollup(headSHA, false, "c")))}, "review threads connection"}, {"no commits connection", []scriptedResponse{ok200(pullPage(t, threadsConn(false, "t"), nil))}, "commits connection"}, - {"next page without a cursor", []scriptedResponse{ok200(pullPage(t, threadsConn(true, ""), rollup(false, "c")))}, "without an endCursor"}, + {"next page without a cursor", []scriptedResponse{ok200(pullPage(t, threadsConn(true, ""), rollup(headSHA, false, "c")))}, "without an endCursor"}, {"repeated cursor", []scriptedResponse{ - ok200(pullPage(t, threadsConn(true, "SAME"), rollup(false, "c"))), + ok200(pullPage(t, threadsConn(true, "SAME"), rollup(headSHA, false, "c"))), ok200(pullPage(t, threadsConn(true, "SAME"), nil)), }, "repeats the previous cursor"}, {"null thread node", []scriptedResponse{ - ok200(pullPage(t, threadsConn(false, "t", longThread), rollup(false, "c"))), + ok200(pullPage(t, threadsConn(false, "t", longThread), rollup(headSHA, false, "c"))), ok200(`{"data":{"node":null}}`), }, "review thread"}, } @@ -312,6 +317,45 @@ func TestPullGraphQLErrorBranches(t *testing.T) { } } +// The contexts must describe the commit the REST checks were read at. A moved +// head, on page 1 or mid-walk, is errHeadMoved naming both SHAs, never a +// Required set joined from another commit. A null rollup is checked too, so a +// new head with no checks cannot clear the old head's required flags. +func TestPullGraphQLHeadMoved(t *testing.T) { + var moved ghGQLCommitNode + moved.Commit.OID = "newhead" + cases := []struct { + name string + gql []scriptedResponse + }{ + {"page 1 at another commit", []scriptedResponse{ + ok200(pullPage(t, threadsConn(false, "t"), rollup("newhead", false, "c", checkRun("build", true)))), + }}, + {"page 1 null rollup at another commit", []scriptedResponse{ + ok200(pullPage(t, threadsConn(false, "t"), &ghGQLPullCommits{Nodes: []ghGQLCommitNode{moved}})), + }}, + {"head changes on page 2", []scriptedResponse{ + ok200(pullPage(t, threadsConn(false, "t"), rollup(headSHA, true, "C1", checkRun("build", false)))), + ok200(pullPage(t, nil, rollup("newhead", false, "C2", checkRun("rollup", true)))), + }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rt := &scriptedRoundTripper{responses: pullRESTLegs("", tc.gql...)} + _, err := newTestGitHub(rt, &fakeTokenSource{token: "t"}).GetPullRequest(context.Background(), "org/repo", 7) + if !errors.Is(err, errHeadMoved) { + t.Fatalf("err = %v, want errHeadMoved", err) + } + if msg := err.Error(); !strings.Contains(msg, headSHA) || !strings.Contains(msg, "newhead") { + t.Errorf("err = %q, want both SHAs", msg) + } + if rt.calls != 4+len(tc.gql) { + t.Errorf("calls = %d, want %d (no retry, no fallback)", rt.calls, 4+len(tc.gql)) + } + }) + } +} + // nextPage refuses to loop: no cursor, or the cursor that fetched this page. func TestNextPage(t *testing.T) { cases := []struct { diff --git a/go/internal/forge/github_test.go b/go/internal/forge/github_test.go index e7d00cfca..82b2c64aa 100644 --- a/go/internal/forge/github_test.go +++ b/go/internal/forge/github_test.go @@ -1437,7 +1437,7 @@ const happyPullGraphQL = `{"data":{"repository":{ "pageInfo":{"hasNextPage":false,"endCursor":null}, "nodes":[{"author":{"login":"dave","__typename":"User"},"body":"why?"}]}} ]}, - "commits":{"nodes":[{"commit":{"statusCheckRollup":{"contexts":{ + "commits":{"nodes":[{"commit":{"oid":"abc123","statusCheckRollup":{"contexts":{ "pageInfo":{"hasNextPage":false,"endCursor":"c1"}, "nodes":[ {"__typename":"CheckRun","name":"build","isRequired":true}, @@ -1588,7 +1588,7 @@ func TestGetPullRequestOpenState(t *testing.T) { {status: 200, body: `[]`}, {status: 200, body: `{"check_runs": []}`}, {status: 200, body: `{"statuses": []}`}, - {status: 200, body: emptyPullGraphQL}, + {status: 200, body: emptyPullGraphQL("sha5")}, }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -1639,7 +1639,7 @@ func TestChecksMixedFailure(t *testing.T) { const statusBody = `{"statuses": [ {"context": "coverage", "state": "success", "target_url": "https://ci/cov"} ]}` - const graphQLBody = `{"data":{"repository":{"pullRequest":{"commits":{"nodes":[{"commit":{"statusCheckRollup":{"contexts":{ + const graphQLBody = `{"data":{"repository":{"pullRequest":{"commits":{"nodes":[{"commit":{"oid":"deadbeef","statusCheckRollup":{"contexts":{ "pageInfo":{"hasNextPage":false,"endCursor":"c1"}, "nodes":[ {"__typename":"CheckRun","name":"lint","isRequired":true}, @@ -1716,7 +1716,7 @@ func TestChecksPending(t *testing.T) { {status: 200, body: detailBody}, {status: 200, body: checkRunsBody}, {status: 200, body: `{"statuses": []}`}, - {status: 200, body: noRollupGraphQL}, + {status: 200, body: noRollupGraphQL("s")}, }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -1740,7 +1740,7 @@ func TestChecksAllSuccess(t *testing.T) { {status: 200, body: detailBody}, {status: 200, body: `{"check_runs": [{"name":"build","status":"completed","conclusion":"success","html_url":""}]}`}, {status: 200, body: `{"statuses": [{"context":"cov","state":"success","target_url":""}]}`}, - {status: 200, body: noRollupGraphQL}, + {status: 200, body: noRollupGraphQL("s")}, }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -1787,7 +1787,7 @@ func TestChecksFollowsPagination(t *testing.T) { }}, {status: 200, body: page2}, {status: 200, body: `{"statuses": []}`}, - {status: 200, body: noRollupGraphQL}, + {status: 200, body: noRollupGraphQL("pg")}, }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -1828,7 +1828,7 @@ func TestChecksUnknownNonTerminalStatusPending(t *testing.T) { {status: 200, body: detailBody}, {status: 200, body: `{"check_runs": [{"name":"gate","status":"waiting","conclusion":"","html_url":""}]}`}, {status: 200, body: `{"statuses": []}`}, - {status: 200, body: noRollupGraphQL}, + {status: 200, body: noRollupGraphQL("s")}, }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -1856,7 +1856,7 @@ func TestChecksCancelledIsFailure(t *testing.T) { {"name":"deploy","status":"completed","conclusion":"cancelled","html_url":""} ]}`}, {status: 200, body: `{"statuses": []}`}, - {status: 200, body: noRollupGraphQL}, + {status: 200, body: noRollupGraphQL("s")}, }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -1881,7 +1881,7 @@ func TestChecksNeutralRollsUpSuccess(t *testing.T) { {status: 200, body: detailBody}, {status: 200, body: `{"check_runs": [{"name":"advisory","status":"completed","conclusion":"neutral","html_url":""}]}`}, {status: 200, body: `{"statuses": []}`}, - {status: 200, body: noRollupGraphQL}, + {status: 200, body: noRollupGraphQL("s")}, }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) @@ -1905,7 +1905,7 @@ func TestChecksLegacyErrorStatusIsFailure(t *testing.T) { {status: 200, body: detailBody}, {status: 200, body: `{"check_runs": []}`}, {status: 200, body: `{"statuses": [{"context":"legacy","state":"error","target_url":""}]}`}, - {status: 200, body: noRollupGraphQL}, + {status: 200, body: noRollupGraphQL("s")}, }} g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) diff --git a/go/internal/forge/golden_capture_test.go b/go/internal/forge/golden_capture_test.go index 6d5341128..268f513ce 100644 --- a/go/internal/forge/golden_capture_test.go +++ b/go/internal/forge/golden_capture_test.go @@ -97,6 +97,7 @@ var wireVolatile = map[string]func(node any) any{ "login": fixedSentinel(canonAccount), "displayName": fixedSentinel(canonAccount), "sha": fixedSentinel(canonSHA), + "oid": fixedSentinel(canonSHA), "ref": fixedSentinel(canonRef), "title": fixedSentinel(canonTitle), "body": fixedSentinel(canonBody), @@ -105,10 +106,12 @@ var wireVolatile = map[string]func(node any) any{ } // wireOnlyVolatile names wire keys that change per capture but decode into no -// domain field, so they have no domainToWire entry. A GraphQL endCursor is one: -// it only feeds the next page's request. +// domain field, so they have no domainToWire entry. A GraphQL endCursor only +// feeds the next page's request; a commit oid is only compared to head.sha, so it +// shares sha's sentinel. var wireOnlyVolatile = map[string]struct{}{ "endCursor": {}, + "oid": {}, } // canonCursor sentinels a GraphQL endCursor. A null stays null: it marks a last @@ -424,7 +427,7 @@ func TestUpdateCanonicalizeStable(t *testing.T) { all := json.RawMessage(`{ "number": 1, "id": 2, "html_url": "h", "url": "u", "target_url": "t", "updated_at": "a", "updatedAt": "b", "login": "l", "displayName": "d", - "sha": "s", "ref": "r", "title": "ti", "body": "bo", "description": "de", + "sha": "s", "oid": "o", "ref": "r", "title": "ti", "body": "bo", "description": "de", "endCursor": "Y3Vyc29y", "last": { "endCursor": null }, "state": "open", "keep": "kept" }`) @@ -432,7 +435,7 @@ func TestUpdateCanonicalizeStable(t *testing.T) { "number": 42, "id": 42, "html_url": "https://example.invalid/canonical", "url": "https://example.invalid/canonical", "target_url": "https://example.invalid/canonical", "updated_at": "2026-08-01T12:30:00Z", "updatedAt": "2026-08-01T12:30:00Z", - "login": "octocat", "displayName": "octocat", "sha": "canonicalsha", + "login": "octocat", "displayName": "octocat", "sha": "canonicalsha", "oid": "canonicalsha", "ref": "canonical-ref", "title": "canonical title", "body": "canonical body", "description": "canonical body", "endCursor": "canonical-cursor", "last": { "endCursor": null }, "state": "open", "keep": "kept" @@ -577,7 +580,7 @@ func TestUpdateCanonicalizeComposite(t *testing.T) { "pageInfo": { "hasNextPage": false, "endCursor": "live-c" }, "nodes": [ { "author": { "login": "dave-live", "__typename": "Bot" }, "body": "live nit" } ] } } ] }, - "commits": { "nodes": [ { "commit": { "statusCheckRollup": { "contexts": { + "commits": { "nodes": [ { "commit": { "oid": "livesha123abc", "statusCheckRollup": { "contexts": { "pageInfo": { "hasNextPage": false, "endCursor": "MQ" }, "nodes": [ { "__typename": "CheckRun", "name": "build", "isRequired": true } ] } } } } ] } } } } }`)}, diff --git a/go/internal/forge/testdata/github/get_pull_request.json b/go/internal/forge/testdata/github/get_pull_request.json index e13b16b6e..a5c8bd8bb 100644 --- a/go/internal/forge/testdata/github/get_pull_request.json +++ b/go/internal/forge/testdata/github/get_pull_request.json @@ -124,6 +124,7 @@ "nodes": [ { "commit": { + "oid": "abc123", "statusCheckRollup": { "contexts": { "pageInfo": { From 6f17841ef143438f36d166e964fadc1efbefd5f4 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sat, 26 Sep 2026 07:18:25 -0400 Subject: [PATCH 3/3] docs(forge): state errHeadMoved's effect, not a caller retry (RIG-1728) No caller retries errHeadMoved, so the comment now says only that a fresh read is consistent. Refs RIG-1728 Co-authored-by: Matt Wilkinson --- go/internal/forge/github_graphql.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/internal/forge/github_graphql.go b/go/internal/forge/github_graphql.go index 45e74f602..4e6c8b252 100644 --- a/go/internal/forge/github_graphql.go +++ b/go/internal/forge/github_graphql.go @@ -20,7 +20,7 @@ import ( var errMalformedRepo = errors.New("forge: malformed github repo, want owner/name") // errHeadMoved means the PR head changed during a read, so the REST checks and -// the GraphQL contexts describe different commits. The caller re-reads. +// the GraphQL contexts describe different commits; a fresh read is consistent. var errHeadMoved = errors.New("forge: github pull request head moved during read") // ghGraphQLRateLimited is the errors[].type GitHub returns, on HTTP 200, when