From 40f19768ea8ed516107db1fa6325b62af08f3f59 Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Mon, 1 Jun 2026 20:56:11 +0530 Subject: [PATCH 1/2] =?UTF-8?q?feat(scm):=20GitHub=20provider=20adapter=20?= =?UTF-8?q?=E2=80=94=20Observe(prURL)=20=E2=86=92=20PRObservation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fresh GitHub SCM provider adapter under backend/internal/adapters/scm/github/ exposing one method: (*Provider).Observe(ctx, prURL) (ports.PRObservation, error) It performs a REST GET on /repos/{o}/{r}/pulls/{n} for the authoritative draft/merged/closed/head-SHA, one GraphQL query for the reviewDecision + mergeStateStatus + statusCheckRollup + unresolved review threads, and (only for failure-class CheckRuns) a REST GET on /actions/jobs/{job_id}/logs to splice the last 20 lines of the failed job into the observation. The package is the observation primitive; the polling loop, cadence selection, daemon wiring, persistence and webhook receiver are all intentionally out of scope (separate PRs / lanes). Closes #27 — this supersedes PR #28's attempt, which targeted types (domain.SCMProvider / SCMSnapshot / ports.SCMObserveRequest) that the PR #62 simplification refactor has since removed. The GraphQL queries and mergeability composition logic are credited to @whoisasx from PR #28's provider.go; the package was re-implemented against the current ports.PRObservation seam (post-#62) rather than rebased. Bot-author detection uses ONLY GitHub's typed signal (__typename "Bot" / User.Type "Bot"). The strings.Contains(login, "bot") fallback from PR #28 was intentionally dropped — aa-18's review flagged it as a false-positive magnet for logins like "robothon" / "lambot123". 46 table-driven tests against httptest.NewServer cover happy path, draft, merged, closed (not merged), CI passing/failing/pending, StatusContext legacy, log-tail extraction (and the best-effort log-fetch failure case), mergeability mergeable/conflicting/blocked (including ci-failing → blocked even when GitHub still says CLEAN — the load-bearing aa-18 contract)/unstable/unknown, review approved/changes-requested/required/none, bot-author filtering (including the robothon false-positive guard), unresolved-only threads, all-bots → empty Comments, ETag-304 cache hit, primary + secondary rate-limit (with errors.As → *RateLimitError), 401 → ErrAuthFailed, malformed JSON → Fetched:false, network error → Fetched:false, Authorization Bearer header injection, StaticTokenSource blank/whitespace rejection, GHTokenSource memoize + invalidate. Verification: - go build ./... clean - go vet ./... clean - gofmt -l backend/internal/adapters/scm/ clean - golangci-lint run ./... (v2.12, repo .golangci.yml) 0 issues - go test -race ./internal/adapters/scm/github/... 46/46 PASS References: - aa-18 review of PR #28: ~/.ao/agent-reports/aa-18.md - aa-26 tracker adapter (sibling Go-adapter pattern): #36 / agent-reports/aa-26.md Co-Authored-By: Claude Opus 4.7 --- backend/internal/adapters/scm/github/auth.go | 139 +++ .../internal/adapters/scm/github/client.go | 431 +++++++ backend/internal/adapters/scm/github/doc.go | 121 ++ .../internal/adapters/scm/github/provider.go | 663 ++++++++++ .../adapters/scm/github/provider_test.go | 1063 +++++++++++++++++ 5 files changed, 2417 insertions(+) create mode 100644 backend/internal/adapters/scm/github/auth.go create mode 100644 backend/internal/adapters/scm/github/client.go create mode 100644 backend/internal/adapters/scm/github/doc.go create mode 100644 backend/internal/adapters/scm/github/provider.go create mode 100644 backend/internal/adapters/scm/github/provider_test.go diff --git a/backend/internal/adapters/scm/github/auth.go b/backend/internal/adapters/scm/github/auth.go new file mode 100644 index 00000000..3349d7c3 --- /dev/null +++ b/backend/internal/adapters/scm/github/auth.go @@ -0,0 +1,139 @@ +package github + +import ( + "context" + "errors" + "os" + "os/exec" + "strings" + "sync" + "time" +) + +// TokenSource yields a GitHub bearer token on demand. Production wires this +// to EnvTokenSource or GHTokenSource; tests inject StaticTokenSource. +type TokenSource interface { + Token(ctx context.Context) (string, error) +} + +// tokenInvalidator is the optional capability of dropping a cached token so +// the next call re-fetches it. The Client invokes this whenever GitHub +// responds with an auth-class failure: the next request will pick up a +// rotated token without restarting the daemon. +type tokenInvalidator interface { + InvalidateToken() +} + +// ErrNoToken is returned when no token source could yield a non-empty token. +var ErrNoToken = errors.New("github scm: no token configured") + +// StaticTokenSource is a literal token, typically used in tests. +type StaticTokenSource string + +// Token returns the literal token, or ErrNoToken if it is blank. +func (s StaticTokenSource) Token(context.Context) (string, error) { + t := strings.TrimSpace(string(s)) + if t == "" { + return "", ErrNoToken + } + return t, nil +} + +// EnvTokenSource reads the first non-empty value from the listed env vars, +// falling back to GITHUB_TOKEN. Order matters: a project-scoped variable +// (AO_GITHUB_TOKEN) should win over the global default. +type EnvTokenSource struct { + EnvVars []string +} + +// Token returns the first non-empty env-var value found, or ErrNoToken. +func (s EnvTokenSource) Token(context.Context) (string, error) { + for _, name := range s.EnvVars { + if v := strings.TrimSpace(os.Getenv(name)); v != "" { + return v, nil + } + } + if v := strings.TrimSpace(os.Getenv("GITHUB_TOKEN")); v != "" { + return v, nil + } + return "", ErrNoToken +} + +const defaultGHTokenCacheTTL = 5 * time.Minute + +// GHTokenSource shells out to `gh auth token` when env vars are not +// configured. It memoizes the result for TokenTTL so we don't fork-exec on +// every request, but the Client invalidates the cache on auth failures so a +// rotated token is picked up on the next call. Tests inject GH so the gh +// binary is never required. +type GHTokenSource struct { + // GH is the shell-out hook. Production leaves this nil and falls back + // to `exec.CommandContext("gh", "auth", "token")`; tests inject a + // fake to avoid touching the real binary. + GH func(ctx context.Context) (string, error) + // TokenTTL is how long a successful read is memoized. Zero means use + // defaultGHTokenCacheTTL. + TokenTTL time.Duration + // Clock allows tests to drive expiration. Zero means time.Now. + Clock func() time.Time + + mu sync.Mutex + token string + expiresAt time.Time +} + +// Token returns the cached token if still fresh, otherwise re-runs gh. +func (s *GHTokenSource) Token(ctx context.Context) (string, error) { + s.mu.Lock() + defer s.mu.Unlock() + now := s.now() + if s.token != "" && now.Before(s.expiresAt) { + return s.token, nil + } + run := s.GH + if run == nil { + run = ghAuthToken + } + out, err := run(ctx) + if err != nil { + return "", err + } + token := strings.TrimSpace(out) + if token == "" { + return "", ErrNoToken + } + s.token = token + s.expiresAt = now.Add(s.ttl()) + return token, nil +} + +// InvalidateToken drops the memoized token so the next Token call shells +// out again. The Client calls this on 401/403-auth responses. +func (s *GHTokenSource) InvalidateToken() { + s.mu.Lock() + defer s.mu.Unlock() + s.token = "" + s.expiresAt = time.Time{} +} + +func (s *GHTokenSource) now() time.Time { + if s.Clock != nil { + return s.Clock() + } + return time.Now() +} + +func (s *GHTokenSource) ttl() time.Duration { + if s.TokenTTL > 0 { + return s.TokenTTL + } + return defaultGHTokenCacheTTL +} + +func ghAuthToken(ctx context.Context) (string, error) { + out, err := exec.CommandContext(ctx, "gh", "auth", "token").Output() + if err != nil { + return "", err + } + return string(out), nil +} diff --git a/backend/internal/adapters/scm/github/client.go b/backend/internal/adapters/scm/github/client.go new file mode 100644 index 00000000..89d69081 --- /dev/null +++ b/backend/internal/adapters/scm/github/client.go @@ -0,0 +1,431 @@ +package github + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/url" + "strconv" + "strings" + "sync" + "time" +) + +const ( + defaultRESTBaseURL = "https://api.github.com" + defaultGraphQLURL = "https://api.github.com/graphql" + defaultUserAgent = "ao-agent-orchestrator/scm-github" +) + +// Sentinel errors. Provider-level callers should match on these via +// errors.Is; the orchestrator's lifecycle code is intentionally insulated +// from raw HTTP status codes. +var ( + ErrNotFound = errors.New("github scm: not found") + ErrAuthFailed = errors.New("github scm: authentication failed") + ErrRateLimited = errors.New("github scm: rate limited") +) + +// RateLimitError carries the structured backoff hints from a rate-limit +// response. Callers that want to back off intelligently can extract +// ResetAt / RetryAfter via errors.As; callers that only need the category +// can use errors.Is(err, ErrRateLimited). +type RateLimitError struct { + ResetAt time.Time + RetryAfter time.Duration + Message string +} + +// Error formats the rate-limit error for logs. +func (e *RateLimitError) Error() string { + if e == nil { + return ErrRateLimited.Error() + } + if e.Message != "" { + return "github scm: rate limited: " + e.Message + } + return ErrRateLimited.Error() +} + +// Is lets errors.Is match a *RateLimitError against ErrRateLimited. +func (e *RateLimitError) Is(target error) bool { return target == ErrRateLimited } + +// ClientOptions configures a Client. Production code sets Token alone; +// tests inject HTTPClient and the URL fields to point at an httptest fake. +type ClientOptions struct { + HTTPClient *http.Client + Token TokenSource + RESTBase string + GraphQLURL string + UserAgent string +} + +// Client is the HTTP wrapper. It owns: +// - bearer-token injection (with cache invalidation on auth failures), +// - ETag cache for REST GETs (so the second observation of the same PR +// burns a free 304 instead of a fresh payload), and +// - sentinel-error classification so callers don't switch on status codes. +type Client struct { + http *http.Client + tokens TokenSource + restBase string + graphqlURL string + userAgent string + + mu sync.Mutex + etagOut map[string]string // key (method+path+query) -> last-seen ETag + bodyOut map[string][]byte // key -> last-seen body for 304 replay + cacheLRU []string // insertion-order keys for FIFO eviction +} + +// cacheMaxEntries caps the number of distinct (method,path,query) tuples +// the in-memory ETag cache will track. A single Provider observes one PR +// at a time today, but the follow-up poller will reuse one Provider for +// the whole daemon — without a cap, long-running daemons would grow this +// map forever. +const cacheMaxEntries = 512 + +// NewClient returns a Client. It is intentionally tolerant of nil +// dependencies: production passes a TokenSource; tests sometimes leave it +// nil and supply Bearer-less fakes. +func NewClient(opts ClientOptions) *Client { + c := &Client{ + http: opts.HTTPClient, + tokens: opts.Token, + restBase: opts.RESTBase, + graphqlURL: opts.GraphQLURL, + userAgent: opts.UserAgent, + etagOut: map[string]string{}, + bodyOut: map[string][]byte{}, + } + if c.http == nil { + c.http = &http.Client{Timeout: 30 * time.Second} + } + if c.restBase == "" { + c.restBase = defaultRESTBaseURL + } + if c.graphqlURL == "" { + c.graphqlURL = defaultGraphQLURL + } + if c.userAgent == "" { + c.userAgent = defaultUserAgent + } + return c +} + +// RESTResponse is what doREST returns to the Provider. NotModified=true +// means the cached body is being served; the byte slice is unchanged from +// the previous fresh fetch. +type RESTResponse struct { + StatusCode int + NotModified bool + ETag string + Body []byte +} + +// doREST performs one REST request with ETag-aware caching. The cache is +// scoped to the (method, path, query) tuple so repeated PR observations +// against the same endpoint replay from the cache while observations of a +// different PR don't share state. Only GET requests participate in the +// cache — mutating methods would mis-replay 304s as the previous payload. +func (c *Client) doREST(ctx context.Context, method, path string, q url.Values, body any) (RESTResponse, error) { + cacheable := method == http.MethodGet + cacheKey := method + " " + path + "?" + q.Encode() + var prevETag string + var prevBody []byte + if cacheable { + c.mu.Lock() + prevETag = c.etagOut[cacheKey] + prevBody = c.bodyOut[cacheKey] + c.mu.Unlock() + } + + var rdr io.Reader + if body != nil { + b, err := json.Marshal(body) + if err != nil { + return RESTResponse{}, fmt.Errorf("github scm: encode %s %s body: %w", method, path, err) + } + rdr = bytes.NewReader(b) + } + + u, err := c.restURL(path, q) + if err != nil { + return RESTResponse{}, fmt.Errorf("github scm: build %s URL: %w", path, err) + } + req, err := http.NewRequestWithContext(ctx, method, u, rdr) + if err != nil { + return RESTResponse{}, fmt.Errorf("github scm: build %s %s request: %w", method, path, err) + } + if body != nil { + req.Header.Set("Content-Type", "application/json") + } + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("X-GitHub-Api-Version", "2022-11-28") + req.Header.Set("User-Agent", c.userAgent) + if prevETag != "" { + req.Header.Set("If-None-Match", prevETag) + } + if err := c.authorize(ctx, req); err != nil { + return RESTResponse{}, err + } + + resp, err := c.http.Do(req) + if err != nil { + return RESTResponse{}, fmt.Errorf("github scm: %s %s: %w", method, path, err) + } + defer func() { _ = resp.Body.Close() }() + + if cacheable && resp.StatusCode == http.StatusNotModified { + // Replay the cached body. Update the ETag if GitHub returned a + // fresher one — some endpoints rotate ETags on weak revalidation. + newETag := resp.Header.Get("ETag") + if newETag != "" && newETag != prevETag { + c.mu.Lock() + c.etagOut[cacheKey] = newETag + c.mu.Unlock() + } + return RESTResponse{StatusCode: resp.StatusCode, NotModified: true, ETag: newETag, Body: prevBody}, nil + } + + b, readErr := io.ReadAll(resp.Body) + if readErr != nil { + return RESTResponse{}, fmt.Errorf("github scm: read %s body: %w", path, readErr) + } + + if resp.StatusCode >= 200 && resp.StatusCode < 300 { + etag := resp.Header.Get("ETag") + if cacheable && etag != "" { + // Defensive copy: GitHub's HTTP body is owned by net/http's + // buffer pool. Holding the raw slice in our cache would let a + // later caller mutate or alias the same backing array. + c.storeCacheEntry(cacheKey, etag, append([]byte(nil), b...)) + } + return RESTResponse{StatusCode: resp.StatusCode, ETag: etag, Body: b}, nil + } + + err = classifyError(resp, b) + if errors.Is(err, ErrAuthFailed) { + c.invalidateToken() + } + return RESTResponse{StatusCode: resp.StatusCode, Body: b}, err +} + +// doGraphQL posts one GraphQL request and returns the decoded data map +// (the "data" field). Top-level GraphQL errors are surfaced as Go errors +// classified by the same sentinels as REST. +func (c *Client) doGraphQL(ctx context.Context, query string, variables map[string]any) (map[string]any, error) { + payload := map[string]any{"query": query} + if variables != nil { + payload["variables"] = variables + } + b, err := json.Marshal(payload) + if err != nil { + return nil, fmt.Errorf("github scm: encode graphql body: %w", err) + } + req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.graphqlURL, bytes.NewReader(b)) + if err != nil { + return nil, fmt.Errorf("github scm: build graphql request: %w", err) + } + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json") + req.Header.Set("User-Agent", c.userAgent) + if err := c.authorize(ctx, req); err != nil { + return nil, err + } + + resp, err := c.http.Do(req) + if err != nil { + return nil, fmt.Errorf("github scm: POST graphql: %w", err) + } + defer func() { _ = resp.Body.Close() }() + respBody, readErr := io.ReadAll(resp.Body) + if readErr != nil { + return nil, fmt.Errorf("github scm: read graphql body: %w", readErr) + } + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + err := classifyError(resp, respBody) + if errors.Is(err, ErrAuthFailed) { + c.invalidateToken() + } + return nil, err + } + var decoded struct { + Data map[string]any `json:"data"` + Errors []struct { + Message string `json:"message"` + Type string `json:"type"` + } `json:"errors"` + } + if err := json.Unmarshal(respBody, &decoded); err != nil { + return nil, fmt.Errorf("github scm: decode graphql response: %w", err) + } + if len(decoded.Errors) > 0 { + msg := decoded.Errors[0].Message + low := strings.ToLower(msg) + switch { + case strings.Contains(low, "rate limit") || strings.Contains(low, "abuse"): + return decoded.Data, &RateLimitError{Message: msg} + case strings.Contains(low, "bad credentials") || strings.Contains(low, "credentials"): + c.invalidateToken() + return decoded.Data, fmt.Errorf("%w: %s", ErrAuthFailed, msg) + case strings.Contains(low, "could not resolve") || strings.Contains(low, "not found"): + return decoded.Data, fmt.Errorf("%w: %s", ErrNotFound, msg) + default: + return decoded.Data, fmt.Errorf("github scm: graphql error: %s", msg) + } + } + return decoded.Data, nil +} + +// fetchPlainText is a small REST helper used for the job-log endpoint, +// which returns text/plain rather than JSON. It does NOT participate in +// the ETag cache (logs are append-only and tiny enough that re-fetch is +// cheap; caching would just inflate memory for no win). +func (c *Client) fetchPlainText(ctx context.Context, path string) ([]byte, error) { + u, err := c.restURL(path, nil) + if err != nil { + return nil, fmt.Errorf("github scm: build %s URL: %w", path, err) + } + req, err := http.NewRequestWithContext(ctx, http.MethodGet, u, http.NoBody) + if err != nil { + return nil, fmt.Errorf("github scm: build %s request: %w", path, err) + } + req.Header.Set("Accept", "text/plain") + req.Header.Set("User-Agent", c.userAgent) + if err := c.authorize(ctx, req); err != nil { + return nil, err + } + resp, err := c.http.Do(req) + if err != nil { + return nil, fmt.Errorf("github scm: GET %s: %w", path, err) + } + defer func() { _ = resp.Body.Close() }() + body, readErr := io.ReadAll(resp.Body) + if readErr != nil { + return nil, fmt.Errorf("github scm: read %s body: %w", path, readErr) + } + if resp.StatusCode >= 200 && resp.StatusCode < 300 { + return body, nil + } + return nil, classifyError(resp, body) +} + +// storeCacheEntry records one (ETag, body) pair under cacheKey and evicts +// the oldest entry once cacheMaxEntries is exceeded. FIFO is intentional: +// the access pattern is "one PR per poll cycle"; an LRU would just add +// bookkeeping without changing eviction order in practice. +func (c *Client) storeCacheEntry(cacheKey, etag string, body []byte) { + c.mu.Lock() + defer c.mu.Unlock() + if _, exists := c.etagOut[cacheKey]; !exists { + c.cacheLRU = append(c.cacheLRU, cacheKey) + } + c.etagOut[cacheKey] = etag + c.bodyOut[cacheKey] = body + for len(c.cacheLRU) > cacheMaxEntries { + evict := c.cacheLRU[0] + c.cacheLRU = c.cacheLRU[1:] + delete(c.etagOut, evict) + delete(c.bodyOut, evict) + } +} + +func (c *Client) authorize(ctx context.Context, req *http.Request) error { + if c.tokens == nil { + return nil + } + token, err := c.tokens.Token(ctx) + if err != nil { + return fmt.Errorf("%w: %w", ErrAuthFailed, err) + } + req.Header.Set("Authorization", "Bearer "+token) + return nil +} + +func (c *Client) invalidateToken() { + if inv, ok := c.tokens.(tokenInvalidator); ok { + inv.InvalidateToken() + } +} + +func (c *Client) restURL(path string, q url.Values) (string, error) { + base, err := url.Parse(c.restBase) + if err != nil { + return "", err + } + if !strings.HasPrefix(path, "/") { + path = "/" + path + } + base.Path = strings.TrimSuffix(base.Path, "/") + path + if q != nil { + base.RawQuery = q.Encode() + } + return base.String(), nil +} + +func classifyError(resp *http.Response, body []byte) error { + msg := githubMessage(body) + switch resp.StatusCode { + case http.StatusNotFound: + return fmt.Errorf("%w: %s", ErrNotFound, msg) + case http.StatusTooManyRequests: + return rateLimited(resp, msg) + case http.StatusUnauthorized: + return fmt.Errorf("%w: %s", ErrAuthFailed, msg) + case http.StatusForbidden: + // GitHub returns 403 for primary rate-limit exhaustion, for + // secondary/abuse limits, and for genuine auth/permission failures. + // Disambiguate by signal: primary limit sets X-RateLimit-Remaining=0; + // secondary/abuse sets Retry-After (often without the Remaining + // header); either case mentions "rate limit" / "abuse" in the body. + // Everything else is an auth/permission failure. + if isRateLimited(resp, msg) { + return rateLimited(resp, msg) + } + return fmt.Errorf("%w: %s", ErrAuthFailed, msg) + } + return fmt.Errorf("github scm: %d %s", resp.StatusCode, msg) +} + +func isRateLimited(resp *http.Response, msg string) bool { + if rem := resp.Header.Get("X-RateLimit-Remaining"); rem != "" { + if n, err := strconv.Atoi(rem); err == nil && n == 0 { + return true + } + } + if resp.Header.Get("Retry-After") != "" { + return true + } + low := strings.ToLower(msg) + return strings.Contains(low, "rate limit") || strings.Contains(low, "abuse detection") || strings.Contains(low, "secondary rate") +} + +func rateLimited(resp *http.Response, msg string) error { + e := &RateLimitError{Message: msg} + if reset := resp.Header.Get("X-RateLimit-Reset"); reset != "" { + if sec, err := strconv.ParseInt(reset, 10, 64); err == nil && sec > 0 { + e.ResetAt = time.Unix(sec, 0) + } + } + if ra := resp.Header.Get("Retry-After"); ra != "" { + if sec, err := strconv.Atoi(ra); err == nil && sec >= 0 { + e.RetryAfter = time.Duration(sec) * time.Second + } + } + return e +} + +func githubMessage(body []byte) string { + var p struct { + Message string `json:"message"` + } + if json.Unmarshal(body, &p) == nil && p.Message != "" { + return p.Message + } + return strings.TrimSpace(string(body)) +} diff --git a/backend/internal/adapters/scm/github/doc.go b/backend/internal/adapters/scm/github/doc.go new file mode 100644 index 00000000..8dee9a34 --- /dev/null +++ b/backend/internal/adapters/scm/github/doc.go @@ -0,0 +1,121 @@ +// Package github observes GitHub pull requests for the PR Manager. +// +// The exported surface is one function: +// +// (*Provider).Observe(ctx, prURL) (ports.PRObservation, error) +// +// It performs a REST GET on /repos/{o}/{r}/pulls/{n} for the authoritative +// state booleans (draft / merged / closed / head SHA), one GraphQL query +// for the reviewDecision + mergeStateStatus + statusCheckRollup + review +// threads, and (only for CheckRuns that concluded failure-class) a REST +// GET on /repos/{o}/{r}/actions/jobs/{job_id}/logs to splice the last 20 +// lines of the failed job into the observation. +// +// The poller / cadence loop is intentionally NOT in this package — it is +// a follow-up PR. This adapter is the observation primitive that loop +// will call. +// +// # State mapping +// +// Each ports.PRObservation field is derived as follows: +// +// - Fetched: false if any required REST/GraphQL call fails; true +// only once all the calls have succeeded. Log-tail +// fetch failures are best-effort: the LogTail is +// stamped with a "" sentinel +// and the observation still surfaces as Fetched=true. +// +// - URL, Number: the URL the caller passed (validated) plus the +// number from REST pulls/{n}. +// +// - Draft: REST pulls/{n}.draft. +// +// - Merged: REST pulls/{n}.merged OR a non-null merged_at. +// +// - Closed: REST pulls/{n}.state == "closed" AND NOT Merged. +// (Closed and Merged are mutually exclusive.) +// +// - CI: derived from the latest commit's statusCheckRollup contexts +// (CheckRun + StatusContext). Failed if ANY context concluded in a +// failure class (failure / cancelled / timed_out / action_required / +// error). Pending if any context is still running / queued. +// Passing if all non-skipped contexts concluded SUCCESS / NEUTRAL. +// Unknown otherwise. Empty rollup falls back to the rollup-level +// "state" field. +// +// - Review: from GraphQL pullRequest.reviewDecision: +// +// | GraphQL | domain.ReviewDecision | +// |------------------------|-------------------------| +// | APPROVED | ReviewApproved | +// | CHANGES_REQUESTED | ReviewChangesRequest | +// | REVIEW_REQUIRED | ReviewRequired | +// | null / unknown | ReviewNone | +// +// - Mergeability: composed in priority order; the first rule that +// matches wins. The primary signal is the GraphQL pullRequest +// payload; the REST pulls/{n} response is consulted only as a +// tiebreaker when GraphQL is empty or has not yet been computed. +// Rules: +// (1) mergeStateStatus == DIRTY -> MergeConflicting +// (2) mergeStateStatus == BLOCKED -> MergeBlocked +// (3) mergeStateStatus == UNSTABLE -> MergeUnstable +// (4) GraphQL mergeable == CONFLICTING -> MergeConflicting +// (5) reviewDecision == changes_requested -> MergeBlocked +// (6) CI == failing -> MergeBlocked +// (7) REST mergeable_state pin — a TIE-BREAKER, not a terminal +// step: "dirty"->MergeConflicting, "blocked"->MergeBlocked, +// "unstable"->MergeUnstable, "clean"->MergeMergeable ONLY if +// GraphQL says MERGEABLE or REST mergeable bool is true +// (otherwise stays unknown — REST lags GraphQL). +// (8) mergeable == MERGEABLE AND mergeStateStatus == CLEAN +// -> MergeMergeable +// (9) otherwise -> MergeUnknown +// +// - Checks[]: one entry per rollup context. For CheckRun rows we use +// name + conclusion + detailsUrl + the head SHA as the CommitHash; +// for StatusContext rows we use context + state + targetUrl. LogTail +// is populated ONLY for failure-class CheckRun entries, by fetching +// /actions/jobs/{job_id}/logs and tailing to the last 20 lines. +// +// - Comments[]: one entry per unresolved review-thread comment. +// Resolved threads are skipped client-side (Resolved on the +// observation is therefore always false). Bot authors are detected +// via GitHub's __typename == "Bot" or User.Type == "Bot" and +// dropped — the legacy strings.Contains(login, "bot") fallback was +// intentionally NOT carried forward (it false-positives on logins +// like "robothon" / "lambot123"; aa-18's review of PR #28 flagged +// this). +// +// # Errors +// +// The Client classifies HTTP failures into three sentinels: +// +// - ErrNotFound — 404 (PR doesn't exist or token can't see it) +// - ErrAuthFailed — 401, or 403 without rate-limit signals +// - ErrRateLimited — 403 with X-RateLimit-Remaining=0, 403 with the +// secondary "abuse detection" body, or 429 +// (also returns *RateLimitError with ResetAt / +// RetryAfter — match via errors.As) +// +// All other transport failures (decode errors, network failures, GraphQL +// "errors" array) bubble up as wrapped errors with Fetched=false on the +// observation, so the PR Manager keeps the prior row rather than +// fabricating a closed/merged transition from a failed observation. +// +// # Caching +// +// The Client maintains an in-memory ETag cache per (method, path, query). +// On the second observation of the same PR the REST GET sends +// If-None-Match and replays the cached body on a 304 — GraphQL is always +// re-fetched because it doesn't expose ETag-based revalidation. +// +// # Out of scope (intentionally — these are different PRs / lanes) +// +// - The poller loop and cadence selection (issue #35). +// - Webhook ingestion (this package is polling-only). +// - Persistence (PR Manager owns the row mapping; see internal/pr). +// - Linear / GitLab providers (separate PRs). +// - Issue tracking (separate lane, see internal/adapters/tracker). +// - Comment-injection-into-session-context (Messenger lane, not SCM). +package github diff --git a/backend/internal/adapters/scm/github/provider.go b/backend/internal/adapters/scm/github/provider.go new file mode 100644 index 00000000..81fd9fbb --- /dev/null +++ b/backend/internal/adapters/scm/github/provider.go @@ -0,0 +1,663 @@ +package github + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/url" + "path" + "strconv" + "strings" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" + "github.com/aoagents/agent-orchestrator/backend/internal/ports" +) + +// ciFailureLogTailLines is the number of trailing lines of a failed job's +// log we splice into the observation. 20 lines is enough to catch the +// usual "X tests failed" tail without bloating the per-PR row. +const ciFailureLogTailLines = 20 + +// ProviderOptions configures a Provider. Production code typically sets +// Token; tests inject a pre-built Client pointed at httptest. +type ProviderOptions struct { + Client *Client + HTTPClient *http.Client + Token TokenSource + RESTBase string + GraphQLURL string + UserAgent string +} + +// Provider observes one GitHub pull request and returns a normalized +// ports.PRObservation for the PR Manager to persist. There is no polling +// loop in v1 — the loop is a follow-up PR (#35); this adapter is the +// observation primitive that loop will call. +type Provider struct { + client *Client +} + +// NewProvider returns a Provider. If opts.Client is supplied it is used +// verbatim; otherwise a Client is built from the other options. When a +// Token source is supplied it is exercised once so missing credentials +// surface at daemon startup rather than at first observation. Tests that +// want an unauthenticated fake pass opts.Client directly. +func NewProvider(opts ProviderOptions) (*Provider, error) { + if opts.Client == nil && opts.Token != nil { + if _, err := opts.Token.Token(context.Background()); err != nil { + return nil, err + } + } + c := opts.Client + if c == nil { + c = NewClient(ClientOptions{ + HTTPClient: opts.HTTPClient, + Token: opts.Token, + RESTBase: opts.RESTBase, + GraphQLURL: opts.GraphQLURL, + UserAgent: opts.UserAgent, + }) + } + return &Provider{client: c}, nil +} + +// Observe fetches the current state of one PR by its github.com URL and +// returns a normalized ports.PRObservation. Any required network call +// failing yields Fetched=false (caller must not infer "PR closed" from a +// failed observation). +func (p *Provider) Observe(ctx context.Context, prURL string) (ports.PRObservation, error) { + out := ports.PRObservation{URL: prURL} + owner, repo, number, err := parsePRURL(prURL) + if err != nil { + return out, err + } + out.Number = number + + rest, err := p.fetchRESTPull(ctx, owner, repo, number) + if err != nil { + // Network/auth/rate-limit failures must surface as Fetched:false. + // Stable terminal states like 404 also surface that way — the PR + // Manager keeps the prior row rather than fabricating closed/merged. + return out, err + } + + out.Draft = rest.Draft + out.Merged = rest.Merged || (rest.MergedAt != "") + out.Closed = strings.EqualFold(rest.State, "closed") && !out.Merged + + gq, err := p.fetchGraphQL(ctx, owner, repo, number) + if err != nil { + return out, err + } + + out.CI = ciSummaryFromGraphQL(gq) + out.Review = reviewDecisionFromGraphQL(gq) + out.Mergeability = mergeabilityFromGraphQL(gq, rest, out.CI, out.Review) + out.Checks = checksFromGraphQL(gq, rest.Head.SHA) + out.Comments = commentsFromGraphQL(gq) + + // Log-tail enrichment is best-effort: a job-log fetch failure must not + // flip the observation to Fetched:false, because we already have the + // authoritative CI summary from GraphQL. Stamp a one-liner instead. + for i := range out.Checks { + if !isFailingCheckStatus(out.Checks[i].Status) { + continue + } + jobID := jobIDForCheck(gq, out.Checks[i].Name) + if jobID == 0 { + continue + } + log, fetchErr := p.fetchJobLogTail(ctx, owner, repo, jobID) + if fetchErr != nil { + out.Checks[i].LogTail = fmt.Sprintf("", scrubError(fetchErr)) + continue + } + out.Checks[i].LogTail = tailLines(log, ciFailureLogTailLines) + } + + out.Fetched = true + return out, nil +} + +// --------------------------------------------------------------------------- +// REST: pull payload +// --------------------------------------------------------------------------- + +type restPull struct { + State string `json:"state"` + Draft bool `json:"draft"` + Merged bool `json:"merged"` + MergedAt string `json:"merged_at"` + Head struct { + SHA string `json:"sha"` + } `json:"head"` + Mergeable *bool `json:"mergeable"` + MergeableState string `json:"mergeable_state"` + MergeStateStatus string `json:"merge_state_status"` +} + +func (p *Provider) fetchRESTPull(ctx context.Context, owner, repo string, number int) (restPull, error) { + resp, err := p.client.doREST(ctx, http.MethodGet, repoPath(owner, repo, "pulls", strconv.Itoa(number)), nil, nil) + if err != nil { + return restPull{}, err + } + if len(resp.Body) == 0 { + return restPull{}, errors.New("github scm: empty pull response") + } + var pull restPull + if err := json.Unmarshal(resp.Body, &pull); err != nil { + return restPull{}, fmt.Errorf("github scm: decode pull: %w", err) + } + return pull, nil +} + +// --------------------------------------------------------------------------- +// GraphQL: the heavy lift +// --------------------------------------------------------------------------- + +const graphQLCheckContextLimit = 50 + +// prObservationQuery is the GraphQL query (derived from PR #28, credited +// to @whoisasx) that pulls everything we need in one round trip: +// - reviewDecision (APPROVED / CHANGES_REQUESTED / REVIEW_REQUIRED / null) +// - mergeable + mergeStateStatus (DIRTY / BLOCKED / UNSTABLE / CLEAN / ...) +// - latest commit's statusCheckRollup (CheckRuns + StatusContexts) so we +// can derive a CIState without an extra REST hop, and so that bot vs +// human is detected via __typename on review comments. +const prObservationQuery = `query($owner:String!,$repo:String!,$number:Int!){ + repository(owner:$owner,name:$repo){ + pullRequest(number:$number){ + number + url + state + isDraft + merged + closed + mergeable + mergeStateStatus + reviewDecision + headRefOid + commits(last:1){ nodes{ commit{ + oid + statusCheckRollup{ + state + contexts(first:CONTEXT_LIMIT){ + nodes{ + __typename + ... on CheckRun { name status conclusion detailsUrl url databaseId } + ... on StatusContext { context state targetUrl } + } + pageInfo{ hasNextPage } + } + } + } } } + reviewThreads(last:100){ nodes{ + id + isResolved + comments(first:100){ nodes{ + id + body + path + line + url + author{ login __typename ... on User { } } + } } + } } + } + } +}` + +func (p *Provider) fetchGraphQL(ctx context.Context, owner, repo string, number int) (map[string]any, error) { + q := strings.Replace(prObservationQuery, "CONTEXT_LIMIT", strconv.Itoa(graphQLCheckContextLimit), 1) + data, err := p.client.doGraphQL(ctx, q, map[string]any{"owner": owner, "repo": repo, "number": number}) + if err != nil { + return nil, err + } + repoData, _ := data["repository"].(map[string]any) + pr, _ := repoData["pullRequest"].(map[string]any) + if pr == nil { + return nil, fmt.Errorf("%w: pull request not found in graphql response", ErrNotFound) + } + return pr, nil +} + +// --------------------------------------------------------------------------- +// REST: per-job log tail +// --------------------------------------------------------------------------- + +func (p *Provider) fetchJobLogTail(ctx context.Context, owner, repo string, jobID int64) (string, error) { + logPath := repoPath(owner, repo, "actions", "jobs", strconv.FormatInt(jobID, 10), "logs") + body, err := p.client.fetchPlainText(ctx, logPath) + if err != nil { + return "", err + } + return string(body), nil +} + +// --------------------------------------------------------------------------- +// Projection helpers +// --------------------------------------------------------------------------- + +// ciSummaryFromGraphQL maps the per-PR status rollup onto domain.CIState. +// If ANY context concluded failure-class we return CIFailing. Otherwise +// any pending context wins over passing. An empty rollup is CIUnknown. +func ciSummaryFromGraphQL(pr map[string]any) domain.CIState { + roll := statusRollup(pr) + if roll == nil { + return domain.CIUnknown + } + contexts, _ := roll["contexts"].(map[string]any) + rawNodes := nodes(contexts["nodes"]) + if len(rawNodes) == 0 { + // GitHub returns a top-level "state" on the rollup even when the + // nodes list is empty (e.g. SUCCESS / FAILURE / PENDING). Honor it + // rather than returning CIUnknown for an otherwise-decided PR. + return mapRollupState(str(roll["state"])) + } + pending, passing := false, false + for _, n := range rawNodes { + st := checkStatusFromGraphQL(n) + switch st { + case domain.PRCheckFailed, domain.PRCheckCancelled: + return domain.CIFailing + case domain.PRCheckQueued, domain.PRCheckInProgress: + pending = true + case domain.PRCheckPassed: + passing = true + } + } + switch { + case pending: + return domain.CIPending + case passing: + return domain.CIPassing + default: + return domain.CIUnknown + } +} + +func mapRollupState(s string) domain.CIState { + switch strings.ToUpper(strings.TrimSpace(s)) { + case "SUCCESS": + return domain.CIPassing + case "FAILURE", "ERROR": + return domain.CIFailing + case "PENDING", "EXPECTED": + return domain.CIPending + default: + return domain.CIUnknown + } +} + +// reviewDecisionFromGraphQL normalizes the GraphQL reviewDecision enum +// onto the domain vocabulary. Re-implemented inline because the helper +// referenced in the task brief lived against types that no longer exist. +func reviewDecisionFromGraphQL(pr map[string]any) domain.ReviewDecision { + switch strings.ToUpper(strings.TrimSpace(str(pr["reviewDecision"]))) { + case "APPROVED": + return domain.ReviewApproved + case "CHANGES_REQUESTED": + return domain.ReviewChangesRequest + case "REVIEW_REQUIRED": + return domain.ReviewRequired + default: + return domain.ReviewNone + } +} + +// mergeabilityFromGraphQL composes the merge verdict from three signals: +// the REST mergeable/rebaseable booleans, the GraphQL mergeStateStatus, +// and the already-derived CIState + ReviewDecision. The rules follow the +// spec table in doc.go. +func mergeabilityFromGraphQL(pr map[string]any, rest restPull, ci domain.CIState, review domain.ReviewDecision) domain.Mergeability { + state := strings.ToUpper(strings.TrimSpace(firstNonEmpty(str(pr["mergeStateStatus"]), rest.MergeStateStatus))) + rawMergeable := strings.ToUpper(strings.TrimSpace(str(pr["mergeable"]))) + + switch state { + case "DIRTY": + return domain.MergeConflicting + case "BLOCKED": + return domain.MergeBlocked + case "UNSTABLE": + return domain.MergeUnstable + } + if rawMergeable == "CONFLICTING" { + return domain.MergeConflicting + } + + if review == domain.ReviewChangesRequest { + return domain.MergeBlocked + } + if ci == domain.CIFailing { + return domain.MergeBlocked + } + + // REST's mergeable_state ("clean" / "blocked" / "behind" / "dirty" / "unstable" + // / "draft" / "unknown") backs up the GraphQL view when GitHub hasn't + // computed the rollup yet. + switch strings.ToLower(strings.TrimSpace(rest.MergeableState)) { + case "clean": + if rawMergeable == "MERGEABLE" || (rest.Mergeable != nil && *rest.Mergeable) { + return domain.MergeMergeable + } + case "dirty": + return domain.MergeConflicting + case "blocked": + return domain.MergeBlocked + case "unstable": + return domain.MergeUnstable + } + + if rawMergeable == "MERGEABLE" && state == "CLEAN" { + return domain.MergeMergeable + } + return domain.MergeUnknown +} + +// checksFromGraphQL projects each context node into a PRCheckObservation. +// StatusContext (commit-status) and CheckRun (Actions) are both flattened +// into the same slice because downstream consumers don't distinguish. +func checksFromGraphQL(pr map[string]any, headSHA string) []ports.PRCheckObservation { + roll := statusRollup(pr) + contexts, _ := roll["contexts"].(map[string]any) + rawNodes := nodes(contexts["nodes"]) + if len(rawNodes) == 0 { + return nil + } + out := make([]ports.PRCheckObservation, 0, len(rawNodes)) + for _, n := range rawNodes { + typ := str(n["__typename"]) + var name, urlOut string + switch typ { + case "CheckRun": + name = str(n["name"]) + urlOut = firstNonEmpty(str(n["detailsUrl"]), str(n["url"])) + case "StatusContext": + name = str(n["context"]) + urlOut = str(n["targetUrl"]) + default: + continue + } + if name == "" { + continue + } + out = append(out, ports.PRCheckObservation{ + Name: name, + CommitHash: headSHA, + Status: checkStatusFromGraphQL(n), + URL: urlOut, + }) + } + return out +} + +// commentsFromGraphQL flattens unresolved review threads into one comment +// per node, dropping bot authors entirely (the spec keeps Resolved=false +// always since we filter resolved threads out client-side). +func commentsFromGraphQL(pr map[string]any) []ports.PRCommentObservation { + threads, _ := pr["reviewThreads"].(map[string]any) + rawNodes := nodes(threads["nodes"]) + if len(rawNodes) == 0 { + return nil + } + var out []ports.PRCommentObservation + for _, th := range rawNodes { + if boolv(th["isResolved"]) { + continue + } + comments, _ := th["comments"].(map[string]any) + for _, cn := range nodes(comments["nodes"]) { + author, _ := cn["author"].(map[string]any) + if isBotAuthor(author) { + continue + } + out = append(out, ports.PRCommentObservation{ + ID: str(cn["id"]), + Author: str(author["login"]), + File: str(cn["path"]), + Line: int(num(cn["line"])), + Body: str(cn["body"]), + Resolved: false, + }) + } + } + return out +} + +// isBotAuthor uses ONLY GitHub's typed signal (__typename or User.Type +// === "Bot"). The strings.Contains(login, "bot") fallback from PR #28 +// was deliberately dropped — aa-18 flagged it as a false-positive +// magnet (logins like "robothon", "lambot123" tripped it). +func isBotAuthor(author map[string]any) bool { + if strings.EqualFold(str(author["__typename"]), "Bot") { + return true + } + if strings.EqualFold(str(author["type"]), "Bot") { + return true + } + return false +} + +// jobIDForCheck looks up the Actions job ID for a check by name, so we +// can call /actions/jobs/{job_id}/logs. StatusContext rows have no job +// ID (they're commit statuses, not Actions runs); those return 0 and +// the log fetch is skipped for them. +func jobIDForCheck(pr map[string]any, name string) int64 { + roll := statusRollup(pr) + contexts, _ := roll["contexts"].(map[string]any) + for _, n := range nodes(contexts["nodes"]) { + if str(n["__typename"]) != "CheckRun" { + continue + } + if str(n["name"]) != name { + continue + } + return int64(num(n["databaseId"])) + } + return 0 +} + +// statusRollup extracts the commits[0].commit.statusCheckRollup blob +// from the GraphQL pullRequest payload. Nil when the PR has no commits +// or GitHub hasn't computed the rollup yet. +func statusRollup(pr map[string]any) map[string]any { + commits, _ := pr["commits"].(map[string]any) + for _, n := range nodes(commits["nodes"]) { + commit, _ := n["commit"].(map[string]any) + roll, _ := commit["statusCheckRollup"].(map[string]any) + if roll != nil { + return roll + } + } + return nil +} + +// checkStatusFromGraphQL maps the (status, conclusion) tuple of one node +// onto the domain enum. Failure-class conclusions always win — pending +// status with a final conclusion of "failure" is still a failed check. +func checkStatusFromGraphQL(n map[string]any) domain.PRCheckStatus { + typ := str(n["__typename"]) + if typ == "StatusContext" { + switch strings.ToUpper(strings.TrimSpace(str(n["state"]))) { + case "SUCCESS": + return domain.PRCheckPassed + case "FAILURE", "ERROR": + return domain.PRCheckFailed + case "PENDING", "EXPECTED": + return domain.PRCheckInProgress + default: + return domain.PRCheckUnknown + } + } + conclusion := strings.ToUpper(strings.TrimSpace(str(n["conclusion"]))) + status := strings.ToUpper(strings.TrimSpace(str(n["status"]))) + switch conclusion { + case "SUCCESS", "NEUTRAL": + return domain.PRCheckPassed + case "FAILURE", "TIMED_OUT", "ACTION_REQUIRED", "STARTUP_FAILURE": + return domain.PRCheckFailed + case "CANCELLED": + return domain.PRCheckCancelled + case "SKIPPED", "STALE": + return domain.PRCheckSkipped + } + switch status { + case "QUEUED", "PENDING", "REQUESTED", "WAITING": + return domain.PRCheckQueued + case "IN_PROGRESS": + return domain.PRCheckInProgress + case "COMPLETED": + // Completed without a conclusion is unusual but reachable — treat + // it as unknown so the caller does not over-trust an absent state. + return domain.PRCheckUnknown + } + return domain.PRCheckUnknown +} + +func isFailingCheckStatus(s domain.PRCheckStatus) bool { + return s == domain.PRCheckFailed || s == domain.PRCheckCancelled +} + +// --------------------------------------------------------------------------- +// URL + path helpers +// --------------------------------------------------------------------------- + +// parsePRURL accepts both the canonical github.com web URL and the API +// pulls URL. Returns owner, repo, number or an error wrapping ErrNotFound +// for shapes we don't recognise (so the caller surfaces them like any +// other "PR isn't on GitHub" outcome). +func parsePRURL(prURL string) (string, string, int, error) { + if prURL == "" { + return "", "", 0, fmt.Errorf("%w: empty PR url", ErrNotFound) + } + u, err := url.Parse(prURL) + if err != nil { + return "", "", 0, fmt.Errorf("%w: parse url: %w", ErrNotFound, err) + } + host := strings.ToLower(u.Host) + // Accept github.com (web) and api.github.com (REST/GraphQL). GitHub + // Enterprise hosts must end in .github.com or .ghe.io (GitHub's own + // dedicated TLDs); anything else looks like a bad URL or a different + // SCM and is rejected. + switch { + case host == "": + // Allow path-only URLs (parsePRURL is also exercised via API + // paths without a host in some tests). + case host == "github.com", host == "www.github.com", host == "api.github.com": + // canonical + case strings.HasSuffix(host, ".github.com") || strings.HasSuffix(host, ".ghe.io"): + // enterprise + default: + return "", "", 0, fmt.Errorf("%w: host %q is not a github host", ErrNotFound, host) + } + parts := strings.Split(strings.Trim(u.Path, "/"), "/") + // Web form: /owner/repo/pull/123 + if len(parts) >= 4 && (parts[2] == "pull" || parts[2] == "pulls") { + owner, repo := parts[0], parts[1] + n, err := strconv.Atoi(parts[3]) + if err != nil || n <= 0 { + return "", "", 0, fmt.Errorf("%w: bad PR number %q", ErrNotFound, parts[3]) + } + return owner, repo, n, nil + } + // API form: /repos/owner/repo/pulls/123 + if len(parts) >= 5 && parts[0] == "repos" && parts[3] == "pulls" { + owner, repo := parts[1], parts[2] + n, err := strconv.Atoi(parts[4]) + if err != nil || n <= 0 { + return "", "", 0, fmt.Errorf("%w: bad PR number %q", ErrNotFound, parts[4]) + } + return owner, repo, n, nil + } + return "", "", 0, fmt.Errorf("%w: not a github PR url: %s", ErrNotFound, prURL) +} + +func repoPath(owner, repo string, elems ...string) string { + all := append([]string{"repos", owner, repo}, elems...) + for i := range all { + all[i] = url.PathEscape(all[i]) + } + return "/" + path.Join(all...) +} + +// --------------------------------------------------------------------------- +// Small JSON-ish accessors +// --------------------------------------------------------------------------- + +func nodes(v any) []map[string]any { + a, ok := v.([]any) + if !ok { + return nil + } + out := make([]map[string]any, 0, len(a)) + for _, item := range a { + if m, ok := item.(map[string]any); ok { + out = append(out, m) + } + } + return out +} + +func str(v any) string { + if s, ok := v.(string); ok { + return s + } + return "" +} + +func boolv(v any) bool { + if b, ok := v.(bool); ok { + return b + } + return false +} + +func num(v any) float64 { + switch t := v.(type) { + case float64: + return t + case int: + return float64(t) + case int64: + return float64(t) + case json.Number: + f, _ := t.Float64() + return f + default: + return 0 + } +} + +func firstNonEmpty(a, b string) string { + if strings.TrimSpace(a) != "" { + return a + } + return b +} + +func tailLines(s string, n int) string { + s = strings.ReplaceAll(strings.TrimSpace(s), "\r\n", "\n") + if s == "" { + return "" + } + lines := strings.Split(s, "\n") + if len(lines) > n { + lines = lines[len(lines)-n:] + } + return strings.Join(lines, "\n") +} + +// scrubError keeps the error message single-line so the LogTail field +// stays a tidy one-liner instead of leaking multi-line API payloads +// into the PR row. +func scrubError(err error) string { + if err == nil { + return "" + } + msg := err.Error() + msg = strings.ReplaceAll(msg, "\n", " ") + msg = strings.ReplaceAll(msg, "\r", " ") + return strings.TrimSpace(msg) +} diff --git a/backend/internal/adapters/scm/github/provider_test.go b/backend/internal/adapters/scm/github/provider_test.go new file mode 100644 index 00000000..82edf58a --- /dev/null +++ b/backend/internal/adapters/scm/github/provider_test.go @@ -0,0 +1,1063 @@ +package github + +import ( + "context" + "encoding/json" + "errors" + "io" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "sync" + "testing" + "time" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" + "github.com/aoagents/agent-orchestrator/backend/internal/ports" +) + +// --------------------------------------------------------------------------- +// Test scaffolding: programmable httptest.Server with route-based dispatch. +// Tests register handlers per "METHOD path" key; unmatched requests fail +// loudly so an accidental extra call surfaces immediately. +// --------------------------------------------------------------------------- + +type recordedReq struct { + Method string + Path string + Header http.Header + Body string +} + +type fakeGH struct { + t *testing.T + server *httptest.Server + mu sync.Mutex + requests []recordedReq + handlers map[string]http.HandlerFunc +} + +func newFakeGH(t *testing.T) *fakeGH { + t.Helper() + f := &fakeGH{t: t, handlers: map[string]http.HandlerFunc{}} + f.server = httptest.NewServer(http.HandlerFunc(f.serve)) + t.Cleanup(f.server.Close) + return f +} + +// on registers a handler for one METHOD + path tuple. Path is taken +// verbatim (no query string). +func (f *fakeGH) on(method, path string, h http.HandlerFunc) { + f.mu.Lock() + defer f.mu.Unlock() + f.handlers[method+" "+path] = h +} + +func (f *fakeGH) serve(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + hdrCopy := r.Header.Clone() + f.mu.Lock() + f.requests = append(f.requests, recordedReq{Method: r.Method, Path: r.URL.Path, Header: hdrCopy, Body: string(body)}) + h, ok := f.handlers[r.Method+" "+r.URL.Path] + f.mu.Unlock() + if !ok { + f.t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.Error(w, "no handler", http.StatusNotImplemented) + return + } + r.Body = io.NopCloser(strings.NewReader(string(body))) + h(w, r) +} + +func (f *fakeGH) calls() []recordedReq { + f.mu.Lock() + defer f.mu.Unlock() + out := make([]recordedReq, len(f.requests)) + copy(out, f.requests) + return out +} + +func (f *fakeGH) callsTo(method, path string) int { + n := 0 + for _, r := range f.calls() { + if r.Method == method && r.Path == path { + n++ + } + } + return n +} + +// newProviderForTest builds a Provider that talks only to the fake. +func newProviderForTest(t *testing.T, f *fakeGH) *Provider { + t.Helper() + p, err := NewProvider(ProviderOptions{ + Token: StaticTokenSource("tkn-test"), + HTTPClient: f.server.Client(), + RESTBase: f.server.URL, + GraphQLURL: f.server.URL + "/graphql", + UserAgent: "ao-scm-test", + }) + if err != nil { + t.Fatalf("NewProvider: %v", err) + } + return p +} + +func ctx() context.Context { return context.Background() } + +// --------------------------------------------------------------------------- +// Fixture builders. Each test composes a REST pull + GraphQL response so +// it can pin the exact shape it cares about without sharing global state +// with other tests. +// --------------------------------------------------------------------------- + +type prFixture struct { + owner, repo string + number int + rest map[string]any + graphql map[string]any + jobLogs map[int64]string // job_id -> log body +} + +func basePRFixture() *prFixture { + return &prFixture{ + owner: "octocat", + repo: "hello", + number: 42, + rest: map[string]any{ + "number": 42, + "title": "Found a bug", + "state": "open", + "draft": false, + "merged": false, + "merged_at": nil, + "html_url": "https://github.com/octocat/hello/pull/42", + "head": map[string]any{"ref": "feat/x", "sha": "deadbeef"}, + "base": map[string]any{"ref": "main"}, + "mergeable": true, + "rebaseable": true, + "mergeable_state": "clean", + "merge_state_status": "CLEAN", + }, + graphql: map[string]any{ + "data": map[string]any{ + "repository": map[string]any{ + "pullRequest": map[string]any{ + "number": 42, + "url": "https://github.com/octocat/hello/pull/42", + "state": "OPEN", + "isDraft": false, + "merged": false, + "closed": false, + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + "reviewDecision": "APPROVED", + "headRefOid": "deadbeef", + "commits": map[string]any{"nodes": []any{ + map[string]any{"commit": map[string]any{ + "oid": "deadbeef", + "statusCheckRollup": map[string]any{ + "state": "SUCCESS", + "contexts": map[string]any{ + "nodes": []any{ + map[string]any{ + "__typename": "CheckRun", + "name": "build", + "status": "COMPLETED", + "conclusion": "SUCCESS", + "detailsUrl": "https://github.com/octocat/hello/runs/9001", + "databaseId": float64(9001), + }, + }, + "pageInfo": map[string]any{"hasNextPage": false}, + }, + }, + }}, + }}, + "reviewThreads": map[string]any{"nodes": []any{}}, + }, + }, + }, + }, + } +} + +// install wires REST + GraphQL handlers onto the fake. +func (f *prFixture) install(t *testing.T, fake *fakeGH) { + restPath := "/repos/" + f.owner + "/" + f.repo + "/pulls/" + strconv.Itoa(f.number) + fake.on(http.MethodGet, restPath, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("ETag", `W/"v1"`) + _ = json.NewEncoder(w).Encode(f.rest) + }) + fake.on(http.MethodPost, "/graphql", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(f.graphql) + }) + for jobID, body := range f.jobLogs { + fake.on(http.MethodGet, "/repos/"+f.owner+"/"+f.repo+"/actions/jobs/"+strconv.FormatInt(jobID, 10)+"/logs", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/plain") + _, _ = w.Write([]byte(body)) + }) + } +} + +// prData mutates the nested GraphQL pullRequest map. +func (f *prFixture) prData(mut func(pr map[string]any)) *prFixture { + repoData := f.graphql["data"].(map[string]any)["repository"].(map[string]any) + pr := repoData["pullRequest"].(map[string]any) + mut(pr) + return f +} + +func (f *prFixture) prURL() string { + return "https://github.com/" + f.owner + "/" + f.repo + "/pull/" + strconv.Itoa(f.number) +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +func TestParsePRURL(t *testing.T) { + cases := []struct { + name string + url string + wantOwner string + wantRepo string + wantNumber int + wantErr bool + }{ + {"web url", "https://github.com/o/r/pull/42", "o", "r", 42, false}, + {"api url", "https://api.github.com/repos/o/r/pulls/42", "o", "r", 42, false}, + {"trailing slash", "https://github.com/o/r/pull/42/", "o", "r", 42, false}, + {"empty", "", "", "", 0, true}, + {"not github", "https://example.com/o/r/pull/1", "", "", 0, true}, + {"bad number", "https://github.com/o/r/pull/abc", "", "", 0, true}, + {"zero", "https://github.com/o/r/pull/0", "", "", 0, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + o, r, n, err := parsePRURL(tc.url) + if tc.wantErr { + if err == nil { + t.Fatalf("expected error, got %s/%s#%d", o, r, n) + } + if !errors.Is(err, ErrNotFound) { + t.Fatalf("err = %v, want wraps ErrNotFound", err) + } + return + } + if err != nil { + t.Fatalf("parse: %v", err) + } + if o != tc.wantOwner || r != tc.wantRepo || n != tc.wantNumber { + t.Fatalf("got %s/%s#%d, want %s/%s#%d", o, r, n, tc.wantOwner, tc.wantRepo, tc.wantNumber) + } + }) + } +} + +func TestObserve_HappyPath(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + fx.install(t, f) + p := newProviderForTest(t, f) + + obs, err := p.Observe(ctx(), fx.prURL()) + if err != nil { + t.Fatalf("Observe: %v", err) + } + if !obs.Fetched { + t.Fatalf("Fetched = false; want true") + } + if obs.URL != fx.prURL() { + t.Errorf("URL = %q, want %q", obs.URL, fx.prURL()) + } + if obs.Number != 42 { + t.Errorf("Number = %d, want 42", obs.Number) + } + if obs.Draft || obs.Merged || obs.Closed { + t.Errorf("Draft/Merged/Closed = %v/%v/%v, want all false", obs.Draft, obs.Merged, obs.Closed) + } + if obs.CI != domain.CIPassing { + t.Errorf("CI = %q, want passing", obs.CI) + } + if obs.Review != domain.ReviewApproved { + t.Errorf("Review = %q, want approved", obs.Review) + } + if obs.Mergeability != domain.MergeMergeable { + t.Errorf("Mergeability = %q, want mergeable", obs.Mergeability) + } + if len(obs.Checks) != 1 { + t.Fatalf("Checks = %#v; want 1 entry", obs.Checks) + } + if obs.Checks[0].Status != domain.PRCheckPassed { + t.Errorf("Checks[0].Status = %q, want passed", obs.Checks[0].Status) + } + if obs.Checks[0].LogTail != "" { + t.Errorf("Checks[0].LogTail = %q; want empty on success", obs.Checks[0].LogTail) + } + if obs.Checks[0].CommitHash != "deadbeef" { + t.Errorf("Checks[0].CommitHash = %q; want deadbeef", obs.Checks[0].CommitHash) + } + if len(obs.Comments) != 0 { + t.Errorf("Comments = %#v; want empty", obs.Comments) + } +} + +func TestObserve_DraftPR(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + fx.rest["draft"] = true + fx.prData(func(pr map[string]any) { pr["isDraft"] = true }) + fx.install(t, f) + p := newProviderForTest(t, f) + + obs, err := p.Observe(ctx(), fx.prURL()) + if err != nil { + t.Fatalf("Observe: %v", err) + } + if !obs.Draft { + t.Errorf("Draft = false; want true") + } +} + +func TestObserve_MergedPR(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + fx.rest["state"] = "closed" + fx.rest["merged"] = true + fx.rest["merged_at"] = "2026-05-30T12:00:00Z" + fx.prData(func(pr map[string]any) { + pr["state"] = "MERGED" + pr["merged"] = true + pr["closed"] = true + }) + fx.install(t, f) + p := newProviderForTest(t, f) + + obs, err := p.Observe(ctx(), fx.prURL()) + if err != nil { + t.Fatalf("Observe: %v", err) + } + if !obs.Merged { + t.Errorf("Merged = false; want true") + } + if obs.Closed { + t.Errorf("Closed = true; want false (merged is mutually exclusive)") + } +} + +func TestObserve_ClosedNotMerged(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + fx.rest["state"] = "closed" + fx.rest["merged"] = false + fx.rest["merged_at"] = nil + fx.prData(func(pr map[string]any) { + pr["state"] = "CLOSED" + pr["closed"] = true + }) + fx.install(t, f) + p := newProviderForTest(t, f) + + obs, err := p.Observe(ctx(), fx.prURL()) + if err != nil { + t.Fatalf("Observe: %v", err) + } + if !obs.Closed { + t.Errorf("Closed = false; want true") + } + if obs.Merged { + t.Errorf("Merged = true; want false") + } +} + +func TestObserve_CIStates(t *testing.T) { + cases := []struct { + name string + nodes []any + wantCI domain.CIState + wantHead domain.PRCheckStatus + }{ + { + name: "passing", + nodes: []any{ + map[string]any{"__typename": "CheckRun", "name": "build", "status": "COMPLETED", "conclusion": "SUCCESS"}, + }, + wantCI: domain.CIPassing, + wantHead: domain.PRCheckPassed, + }, + { + name: "failing wins over passing", + nodes: []any{ + map[string]any{"__typename": "CheckRun", "name": "build", "status": "COMPLETED", "conclusion": "SUCCESS"}, + map[string]any{"__typename": "CheckRun", "name": "lint", "status": "COMPLETED", "conclusion": "FAILURE"}, + }, + wantCI: domain.CIFailing, + }, + { + name: "pending blocks passing-only", + nodes: []any{ + map[string]any{"__typename": "CheckRun", "name": "build", "status": "COMPLETED", "conclusion": "SUCCESS"}, + map[string]any{"__typename": "CheckRun", "name": "test", "status": "IN_PROGRESS"}, + }, + wantCI: domain.CIPending, + }, + { + name: "cancelled is failing", + nodes: []any{ + map[string]any{"__typename": "CheckRun", "name": "deploy", "status": "COMPLETED", "conclusion": "CANCELLED"}, + }, + wantCI: domain.CIFailing, + }, + { + name: "legacy statuscontext failure", + nodes: []any{ + map[string]any{"__typename": "StatusContext", "context": "ci/legacy", "state": "FAILURE", "targetUrl": "https://ci"}, + }, + wantCI: domain.CIFailing, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + fx.prData(func(pr map[string]any) { + commits := pr["commits"].(map[string]any)["nodes"].([]any)[0].(map[string]any) + commit := commits["commit"].(map[string]any) + roll := commit["statusCheckRollup"].(map[string]any) + roll["contexts"].(map[string]any)["nodes"] = tc.nodes + }) + fx.install(t, f) + p := newProviderForTest(t, f) + obs, err := p.Observe(ctx(), fx.prURL()) + if err != nil { + t.Fatalf("Observe: %v", err) + } + if obs.CI != tc.wantCI { + t.Fatalf("CI = %q, want %q", obs.CI, tc.wantCI) + } + }) + } +} + +func TestObserve_LogTailOnFailure(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + fx.jobLogs = map[int64]string{ + 9001: strings.Repeat("line\n", 30) + strings.Join([]string{ + "01", "02", "03", "04", "05", "06", "07", "08", "09", "10", + "11", "12", "13", "14", "15", "16", "17", "18", "19", "FAILED-LAST", + }, "\n"), + } + fx.prData(func(pr map[string]any) { + commits := pr["commits"].(map[string]any)["nodes"].([]any)[0].(map[string]any) + commit := commits["commit"].(map[string]any) + roll := commit["statusCheckRollup"].(map[string]any) + roll["contexts"].(map[string]any)["nodes"] = []any{ + map[string]any{ + "__typename": "CheckRun", + "name": "build", + "status": "COMPLETED", + "conclusion": "FAILURE", + "detailsUrl": "https://github.com/octocat/hello/runs/9001", + "databaseId": float64(9001), + }, + } + }) + fx.install(t, f) + p := newProviderForTest(t, f) + + obs, err := p.Observe(ctx(), fx.prURL()) + if err != nil { + t.Fatalf("Observe: %v", err) + } + if obs.CI != domain.CIFailing { + t.Fatalf("CI = %q, want failing", obs.CI) + } + if len(obs.Checks) != 1 { + t.Fatalf("Checks = %#v", obs.Checks) + } + tail := obs.Checks[0].LogTail + if tail == "" { + t.Fatalf("LogTail empty; expected last %d lines", ciFailureLogTailLines) + } + lines := strings.Split(tail, "\n") + if len(lines) > ciFailureLogTailLines { + t.Fatalf("LogTail has %d lines, want ≤ %d", len(lines), ciFailureLogTailLines) + } + if !strings.Contains(tail, "FAILED-LAST") { + t.Fatalf("LogTail missing the actual tail content: %q", tail) + } +} + +func TestObserve_LogTailFetchFailureIsBestEffort(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + fx.prData(func(pr map[string]any) { + commits := pr["commits"].(map[string]any)["nodes"].([]any)[0].(map[string]any) + commit := commits["commit"].(map[string]any) + roll := commit["statusCheckRollup"].(map[string]any) + roll["contexts"].(map[string]any)["nodes"] = []any{ + map[string]any{ + "__typename": "CheckRun", + "name": "build", + "status": "COMPLETED", + "conclusion": "FAILURE", + "databaseId": float64(9001), + }, + } + }) + fx.install(t, f) + // Job-log endpoint returns 500 — the observation must still come back + // Fetched=true with a synthetic LogTail. + f.on(http.MethodGet, "/repos/octocat/hello/actions/jobs/9001/logs", func(w http.ResponseWriter, r *http.Request) { + http.Error(w, `{"message":"server exploded"}`, http.StatusInternalServerError) + }) + p := newProviderForTest(t, f) + + obs, err := p.Observe(ctx(), fx.prURL()) + if err != nil { + t.Fatalf("Observe: %v", err) + } + if !obs.Fetched { + t.Fatalf("Fetched = false; log-fetch failures must not flip the whole observation") + } + if got := obs.Checks[0].LogTail; !strings.HasPrefix(got, " sentinel", got) + } +} + +func TestObserve_MergeabilityStates(t *testing.T) { + cases := []struct { + name string + mutateREST func(map[string]any) + mutateGQL func(map[string]any) + want domain.Mergeability + }{ + { + name: "mergeable", + // base fixture is the happy path + mutateREST: func(m map[string]any) {}, + mutateGQL: func(m map[string]any) {}, + want: domain.MergeMergeable, + }, + { + name: "conflicting via merge_state_status=DIRTY", + mutateREST: func(m map[string]any) { + m["mergeable_state"] = "dirty" + }, + mutateGQL: func(m map[string]any) { + m["mergeable"] = "CONFLICTING" + m["mergeStateStatus"] = "DIRTY" + }, + want: domain.MergeConflicting, + }, + { + name: "blocked by review", + mutateREST: func(m map[string]any) { + m["mergeable_state"] = "blocked" + }, + mutateGQL: func(m map[string]any) { + m["mergeStateStatus"] = "BLOCKED" + m["reviewDecision"] = "CHANGES_REQUESTED" + }, + want: domain.MergeBlocked, + }, + { + name: "unstable via merge_state_status=UNSTABLE", + mutateREST: func(m map[string]any) { + m["mergeable_state"] = "unstable" + }, + mutateGQL: func(m map[string]any) { + m["mergeStateStatus"] = "UNSTABLE" + }, + want: domain.MergeUnstable, + }, + { + name: "unknown when github hasn't computed yet", + mutateREST: func(m map[string]any) { + m["mergeable"] = nil + m["mergeable_state"] = "unknown" + }, + mutateGQL: func(m map[string]any) { + m["mergeable"] = "UNKNOWN" + m["mergeStateStatus"] = "UNKNOWN" + }, + want: domain.MergeUnknown, + }, + { + // Load-bearing aa-18 contract: CI failing must force + // MergeBlocked even when GitHub still reports the rollup + // as CLEAN (mergeStateStatus has not yet flipped to + // UNSTABLE). Without this guard the LCM would think a + // failing-CI PR is ready to merge. + name: "ci failing forces blocked even when mergeStateStatus is CLEAN", + mutateREST: func(m map[string]any) { + m["mergeable_state"] = "clean" + }, + mutateGQL: func(m map[string]any) { + m["mergeable"] = "MERGEABLE" + m["mergeStateStatus"] = "CLEAN" + commits := m["commits"].(map[string]any)["nodes"].([]any)[0].(map[string]any) + commit := commits["commit"].(map[string]any) + roll := commit["statusCheckRollup"].(map[string]any) + // databaseId=0 so the provider skips the per-job log + // fetch (this test is about mergeability, not log tail). + roll["contexts"].(map[string]any)["nodes"] = []any{ + map[string]any{"__typename": "CheckRun", "name": "lint", "status": "COMPLETED", "conclusion": "FAILURE", "databaseId": float64(0)}, + } + }, + want: domain.MergeBlocked, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + tc.mutateREST(fx.rest) + fx.prData(tc.mutateGQL) + fx.install(t, f) + p := newProviderForTest(t, f) + obs, err := p.Observe(ctx(), fx.prURL()) + if err != nil { + t.Fatalf("Observe: %v", err) + } + if obs.Mergeability != tc.want { + t.Fatalf("Mergeability = %q, want %q", obs.Mergeability, tc.want) + } + }) + } +} + +func TestObserve_ReviewDecisions(t *testing.T) { + cases := []struct { + name string + decision any + want domain.ReviewDecision + }{ + {"approved", "APPROVED", domain.ReviewApproved}, + {"changes requested", "CHANGES_REQUESTED", domain.ReviewChangesRequest}, + {"review required", "REVIEW_REQUIRED", domain.ReviewRequired}, + {"none / null", nil, domain.ReviewNone}, + {"unrecognized falls to none", "WAT", domain.ReviewNone}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + fx.prData(func(pr map[string]any) { pr["reviewDecision"] = tc.decision }) + fx.install(t, f) + p := newProviderForTest(t, f) + obs, err := p.Observe(ctx(), fx.prURL()) + if err != nil { + t.Fatalf("Observe: %v", err) + } + if obs.Review != tc.want { + t.Fatalf("Review = %q, want %q", obs.Review, tc.want) + } + }) + } +} + +func TestObserve_BotAuthorFiltering(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + fx.prData(func(pr map[string]any) { + pr["reviewThreads"] = map[string]any{"nodes": []any{ + map[string]any{ + "id": "T1", + "isResolved": false, + "comments": map[string]any{"nodes": []any{ + map[string]any{ + "id": "C1", + "body": "real human concern", + "path": "foo/bar.go", + "line": float64(12), + "url": "https://github.com/octocat/hello/pull/42#discussion_r1", + "author": map[string]any{"login": "alice", "__typename": "User"}, + }, + }}, + }, + // Bot thread — must be filtered out entirely. + map[string]any{ + "id": "T2", + "isResolved": false, + "comments": map[string]any{"nodes": []any{ + map[string]any{ + "id": "C2", + "body": "dependabot says update", + "path": "go.mod", + "line": float64(1), + "author": map[string]any{"login": "dependabot[bot]", "__typename": "Bot"}, + }, + }}, + }, + // Resolved thread — must also be filtered out. + map[string]any{ + "id": "T3", + "isResolved": true, + "comments": map[string]any{"nodes": []any{ + map[string]any{"id": "C3", "body": "lgtm now", "author": map[string]any{"login": "bob", "__typename": "User"}}, + }}, + }, + // Login like "robothon" — must NOT be treated as a bot (aa-18 + // flagged the strings.Contains(login,"bot") fallback as a + // false-positive magnet; we use the typed signal only). + map[string]any{ + "id": "T4", + "isResolved": false, + "comments": map[string]any{"nodes": []any{ + map[string]any{"id": "C4", "body": "actual comment", "path": "a.go", "line": float64(3), "author": map[string]any{"login": "robothon", "__typename": "User"}}, + }}, + }, + }} + }) + fx.install(t, f) + p := newProviderForTest(t, f) + + obs, err := p.Observe(ctx(), fx.prURL()) + if err != nil { + t.Fatalf("Observe: %v", err) + } + if len(obs.Comments) != 2 { + t.Fatalf("Comments = %#v; want exactly 2 (alice + robothon)", obs.Comments) + } + authors := []string{obs.Comments[0].Author, obs.Comments[1].Author} + if !contains(authors, "alice") { + t.Errorf("missing alice's comment: %v", authors) + } + if !contains(authors, "robothon") { + t.Errorf("robothon misclassified as bot: %v", authors) + } + for _, c := range obs.Comments { + if c.Resolved { + t.Errorf("comment %q marked Resolved=true; observation set is unresolved-only", c.ID) + } + } +} + +// TestObserve_AllBotThreadsYieldsNilComments pins that a PR whose review +// threads are 100% bot-authored produces Comments == nil but a fully +// fetched observation. The PR Manager downstream must handle a nil +// Comments slice without panicking, and Fetched=true means lifecycle +// can still apply the rest of the observation. +func TestObserve_AllBotThreadsYieldsNilComments(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + fx.prData(func(pr map[string]any) { + pr["reviewThreads"] = map[string]any{"nodes": []any{ + map[string]any{ + "id": "T-bot-only", + "isResolved": false, + "comments": map[string]any{"nodes": []any{ + map[string]any{"id": "C1", "body": "auto-merged", "author": map[string]any{"login": "dependabot[bot]", "__typename": "Bot"}}, + map[string]any{"id": "C2", "body": "renovate", "author": map[string]any{"login": "renovate[bot]", "__typename": "Bot"}}, + }}, + }, + }} + }) + fx.install(t, f) + p := newProviderForTest(t, f) + + obs, err := p.Observe(ctx(), fx.prURL()) + if err != nil { + t.Fatalf("Observe: %v", err) + } + if !obs.Fetched { + t.Fatalf("Fetched = false; want true even when all comments are bots") + } + if len(obs.Comments) != 0 { + t.Fatalf("Comments = %#v; want empty (all authors are bots)", obs.Comments) + } +} + +func contains(ss []string, x string) bool { + for _, s := range ss { + if s == x { + return true + } + } + return false +} + +func TestObserve_ETag304Cached(t *testing.T) { + // Second call to the REST pull endpoint must send If-None-Match and + // reuse the cached body, while still completing the rest of the + // observation (GraphQL is always re-fetched — there's no cache for it). + f := newFakeGH(t) + fx := basePRFixture() + var restHits int + restPath := "/repos/" + fx.owner + "/" + fx.repo + "/pulls/" + strconv.Itoa(fx.number) + f.on(http.MethodGet, restPath, func(w http.ResponseWriter, r *http.Request) { + restHits++ + if r.Header.Get("If-None-Match") == `W/"v1"` { + w.Header().Set("ETag", `W/"v1"`) + w.WriteHeader(http.StatusNotModified) + return + } + w.Header().Set("Content-Type", "application/json") + w.Header().Set("ETag", `W/"v1"`) + _ = json.NewEncoder(w).Encode(fx.rest) + }) + f.on(http.MethodPost, "/graphql", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(fx.graphql) + }) + p := newProviderForTest(t, f) + + first, err := p.Observe(ctx(), fx.prURL()) + if err != nil { + t.Fatalf("first Observe: %v", err) + } + second, err := p.Observe(ctx(), fx.prURL()) + if err != nil { + t.Fatalf("second Observe: %v", err) + } + if first.CI != second.CI || first.Mergeability != second.Mergeability { + t.Fatalf("304 replay diverged: %#v vs %#v", first, second) + } + if !second.Fetched { + t.Fatalf("second Fetched = false despite 304 hit") + } + if restHits != 2 { + t.Fatalf("expected 2 hits to the REST pull endpoint (one fresh, one 304), got %d", restHits) + } + // And: the second call must have actually sent If-None-Match. + var sentConditional bool + for _, r := range f.calls() { + if r.Method == http.MethodGet && r.Path == restPath && r.Header.Get("If-None-Match") != "" { + sentConditional = true + break + } + } + if !sentConditional { + t.Fatalf("second call did not send If-None-Match; ETag cache is broken") + } +} + +func TestObserve_PrimaryRateLimit(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + reset := time.Now().Add(2 * time.Minute).Unix() + f.on(http.MethodGet, "/repos/octocat/hello/pulls/42", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("X-RateLimit-Remaining", "0") + w.Header().Set("X-RateLimit-Reset", strconv.FormatInt(reset, 10)) + http.Error(w, `{"message":"API rate limit exceeded"}`, http.StatusForbidden) + }) + // GraphQL would never be reached in this scenario. + p := newProviderForTest(t, f) + + obs, err := p.Observe(ctx(), fx.prURL()) + if !errors.Is(err, ErrRateLimited) { + t.Fatalf("err = %v, want ErrRateLimited", err) + } + if obs.Fetched { + t.Fatalf("Fetched = true on rate-limit error; want false") + } + var rle *RateLimitError + if !errors.As(err, &rle) { + t.Fatalf("err = %v, want *RateLimitError", err) + } + if rle.ResetAt.Unix() != reset { + t.Fatalf("ResetAt = %d, want %d", rle.ResetAt.Unix(), reset) + } +} + +func TestObserve_SecondaryRateLimit(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + f.on(http.MethodGet, "/repos/octocat/hello/pulls/42", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Retry-After", "30") + http.Error(w, `{"message":"You have exceeded a secondary rate limit"}`, http.StatusForbidden) + }) + p := newProviderForTest(t, f) + + obs, err := p.Observe(ctx(), fx.prURL()) + if !errors.Is(err, ErrRateLimited) { + t.Fatalf("err = %v, want ErrRateLimited", err) + } + if obs.Fetched { + t.Fatalf("Fetched = true on rate-limit error") + } + var rle *RateLimitError + if !errors.As(err, &rle) { + t.Fatalf("err = %v, want *RateLimitError", err) + } + if rle.RetryAfter != 30*time.Second { + t.Fatalf("RetryAfter = %v, want 30s", rle.RetryAfter) + } +} + +func TestObserve_AuthFailedSurfacesAsErrAuthFailed(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + f.on(http.MethodGet, "/repos/octocat/hello/pulls/42", func(w http.ResponseWriter, r *http.Request) { + http.Error(w, `{"message":"Bad credentials"}`, http.StatusUnauthorized) + }) + p := newProviderForTest(t, f) + + obs, err := p.Observe(ctx(), fx.prURL()) + if !errors.Is(err, ErrAuthFailed) { + t.Fatalf("err = %v, want ErrAuthFailed", err) + } + if obs.Fetched { + t.Fatalf("Fetched = true on auth-failed; want false") + } +} + +func TestObserve_MalformedJSONIsNotFetched(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + f.on(http.MethodGet, "/repos/octocat/hello/pulls/42", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{not valid json`)) + }) + p := newProviderForTest(t, f) + + obs, err := p.Observe(ctx(), fx.prURL()) + if err == nil { + t.Fatalf("expected decode error, got nil") + } + if obs.Fetched { + t.Fatalf("Fetched = true on decode failure; want false") + } +} + +func TestObserve_NetworkErrorIsNotFetched(t *testing.T) { + // Point the provider at a closed server to force a transport error. + f := newFakeGH(t) + p, err := NewProvider(ProviderOptions{ + Token: StaticTokenSource("tkn"), + HTTPClient: &http.Client{Timeout: 200 * time.Millisecond}, + RESTBase: "http://127.0.0.1:1", // reserved port; refuses connections + GraphQLURL: "http://127.0.0.1:1/graphql", + }) + if err != nil { + t.Fatalf("NewProvider: %v", err) + } + obs, observeErr := p.Observe(ctx(), "https://github.com/o/r/pull/1") + if observeErr == nil { + t.Fatalf("expected network error, got nil") + } + if obs.Fetched { + t.Fatalf("Fetched = true on network error; want false") + } + // Reference f so the test linter doesn't flag it; we don't use the + // fake here but the helper is the canonical way to scope a test. + _ = f +} + +func TestObserve_TokenInjectedAsBearer(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + fx.install(t, f) + p := newProviderForTest(t, f) + if _, err := p.Observe(ctx(), fx.prURL()); err != nil { + t.Fatalf("Observe: %v", err) + } + for _, r := range f.calls() { + if got := r.Header.Get("Authorization"); got != "Bearer tkn-test" { + t.Fatalf("Authorization header on %s %s = %q, want Bearer tkn-test", r.Method, r.Path, got) + } + } +} + +func TestStaticTokenSourceRejectsBlank(t *testing.T) { + if _, err := StaticTokenSource("").Token(context.Background()); !errors.Is(err, ErrNoToken) { + t.Fatalf("err = %v, want ErrNoToken", err) + } + if _, err := StaticTokenSource(" ").Token(context.Background()); !errors.Is(err, ErrNoToken) { + t.Fatalf("blank-with-spaces: err = %v, want ErrNoToken", err) + } +} + +func TestGHTokenSourceUsesInjectedHook(t *testing.T) { + calls := 0 + src := &GHTokenSource{ + GH: func(ctx context.Context) (string, error) { + calls++ + return "from-gh\n", nil + }, + TokenTTL: time.Hour, + } + tok, err := src.Token(context.Background()) + if err != nil { + t.Fatalf("Token: %v", err) + } + if tok != "from-gh" { + t.Fatalf("Token = %q, want %q", tok, "from-gh") + } + // Second call within TTL must be cached. + if _, err := src.Token(context.Background()); err != nil { + t.Fatalf("second Token: %v", err) + } + if calls != 1 { + t.Fatalf("GH called %d times; want 1 (cache miss only)", calls) + } + // Invalidate and the next call must re-run. + src.InvalidateToken() + if _, err := src.Token(context.Background()); err != nil { + t.Fatalf("third Token: %v", err) + } + if calls != 2 { + t.Fatalf("after invalidate, GH called %d times; want 2", calls) + } +} + +// TestObserve_StatusContextLegacyHasNoLogTail pins that we do NOT try to +// fetch a job log for a legacy commit-status row (those have no Actions +// job ID, so /actions/jobs/0/logs would 404 if we let the path leak). +func TestObserve_StatusContextLegacyHasNoLogTail(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + fx.prData(func(pr map[string]any) { + commits := pr["commits"].(map[string]any)["nodes"].([]any)[0].(map[string]any) + commit := commits["commit"].(map[string]any) + roll := commit["statusCheckRollup"].(map[string]any) + roll["contexts"].(map[string]any)["nodes"] = []any{ + map[string]any{"__typename": "StatusContext", "context": "ci/legacy", "state": "FAILURE", "targetUrl": "https://ci"}, + } + }) + fx.install(t, f) + p := newProviderForTest(t, f) + + obs, err := p.Observe(ctx(), fx.prURL()) + if err != nil { + t.Fatalf("Observe: %v", err) + } + if obs.CI != domain.CIFailing { + t.Fatalf("CI = %q, want failing", obs.CI) + } + if len(obs.Checks) != 1 { + t.Fatalf("Checks = %#v", obs.Checks) + } + if obs.Checks[0].LogTail != "" { + t.Fatalf("LogTail = %q; want empty (StatusContext has no job log)", obs.Checks[0].LogTail) + } + if f.callsTo(http.MethodGet, "/repos/octocat/hello/actions/jobs/0/logs") != 0 { + t.Fatalf("unexpected attempt to fetch a /actions/jobs/0/logs URL") + } +} + +// TestObserve_AssertsPRObservationShape is a belt-and-braces compile-time +// guard that PRObservation has the fields we depend on. If the port adds +// or renames a field, this test fails to compile rather than failing at +// runtime. +func TestObserve_AssertsPRObservationShape(t *testing.T) { + var o ports.PRObservation + o.Fetched = true + o.URL = "" + o.Number = 0 + o.Draft = false + o.Merged = false + o.Closed = false + o.CI = domain.CIUnknown + o.Review = domain.ReviewNone + o.Mergeability = domain.MergeUnknown + o.Checks = nil + o.Comments = nil + _ = o +} From fd1aa3e04a15483f30a54a7b0e2b564607b0c548 Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Mon, 1 Jun 2026 21:08:37 +0530 Subject: [PATCH 2/2] fix(scm): address greptile review on #69 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four fixes from the greptile review of PR #69: 1. CI rollup pagination (P1) — when GraphQL reports pageInfo.hasNextPage=true for the statusCheckRollup contexts, a visible "all passing" set could be hiding a failing context on the next page. ciSummaryFromGraphQL now degrades Passing / Pending / Unknown to CIUnknown in that case; a known CIFailing on the visible page is still safe and is NOT degraded. Also bumped the per-page limit from 50 to 100 (GraphQL's documented max for the contexts connection). Two new tests pin both branches. 2. Empty GraphQL inline fragment (P2) — dropped `... on User { }` from the reviewThreads author selection. The empty selection set was technically invalid GraphQL and a future API tightening could reject the query. __typename already tells us whether the actor is a Bot, so the fragment carried no information. 3. rest.MergeStateStatus dead-code (P2) — the field decoded from the non-existent REST `merge_state_status` was always empty, making the firstNonEmpty fallback dead code. Removed the field and switched the tiebreaker to rest.MergeableState (the actual REST field, upper- cased so the same switch covers both GraphQL and REST shapes). 4. Wrong Accept header on /actions/jobs/{id}/logs (P2) — GitHub's REST API validates the Accept header before issuing the 302 to the log blob; sending text/plain risks a 406. Switched to the canonical application/vnd.github+json; the redirected blob serves text/plain regardless. Verification: - go build ./... clean - go vet ./... clean - golangci-lint run ./... 0 issues - go test -race ./internal/adapters/scm/github/... 48 / 48 PASS Co-Authored-By: Claude Opus 4.7 --- .../internal/adapters/scm/github/client.go | 7 ++- .../internal/adapters/scm/github/provider.go | 51 +++++++++++++--- .../adapters/scm/github/provider_test.go | 59 +++++++++++++++++++ 3 files changed, 108 insertions(+), 9 deletions(-) diff --git a/backend/internal/adapters/scm/github/client.go b/backend/internal/adapters/scm/github/client.go index 89d69081..13897672 100644 --- a/backend/internal/adapters/scm/github/client.go +++ b/backend/internal/adapters/scm/github/client.go @@ -295,7 +295,12 @@ func (c *Client) fetchPlainText(ctx context.Context, path string) ([]byte, error if err != nil { return nil, fmt.Errorf("github scm: build %s request: %w", path, err) } - req.Header.Set("Accept", "text/plain") + // The /actions/jobs/{id}/logs endpoint validates the Accept header + // before issuing its 302 to the log blob; sending text/plain here + // gets a 406. The canonical Accept for the GitHub REST API is the + // vnd.github+json media type — the redirected blob serves the + // actual text/plain regardless of what we asked for. + req.Header.Set("Accept", "application/vnd.github+json") req.Header.Set("User-Agent", c.userAgent) if err := c.authorize(ctx, req); err != nil { return nil, err diff --git a/backend/internal/adapters/scm/github/provider.go b/backend/internal/adapters/scm/github/provider.go index 81fd9fbb..b57babdb 100644 --- a/backend/internal/adapters/scm/github/provider.go +++ b/backend/internal/adapters/scm/github/provider.go @@ -133,9 +133,8 @@ type restPull struct { Head struct { SHA string `json:"sha"` } `json:"head"` - Mergeable *bool `json:"mergeable"` - MergeableState string `json:"mergeable_state"` - MergeStateStatus string `json:"merge_state_status"` + Mergeable *bool `json:"mergeable"` + MergeableState string `json:"mergeable_state"` } func (p *Provider) fetchRESTPull(ctx context.Context, owner, repo string, number int) (restPull, error) { @@ -157,7 +156,15 @@ func (p *Provider) fetchRESTPull(ctx context.Context, owner, repo string, number // GraphQL: the heavy lift // --------------------------------------------------------------------------- -const graphQLCheckContextLimit = 50 +// graphQLCheckContextLimit caps how many statusCheckRollup contexts we +// request in one GraphQL hop. 100 is GitHub's documented per-page max +// for the contexts connection. When the rollup has MORE than this many +// contexts the response surfaces pageInfo.hasNextPage=true and +// ciSummaryFromGraphQL is conservative (see the "CIUnknown on +// hasNextPage when not already CIFailing" branch — a partial visible +// set could hide a failure, so we degrade the verdict rather than +// risk reporting a broken PR as passing). +const graphQLCheckContextLimit = 100 // prObservationQuery is the GraphQL query (derived from PR #28, credited // to @whoisasx) that pulls everything we need in one round trip: @@ -202,7 +209,7 @@ const prObservationQuery = `query($owner:String!,$repo:String!,$number:Int!){ path line url - author{ login __typename ... on User { } } + author{ login __typename } } } } } } @@ -241,8 +248,13 @@ func (p *Provider) fetchJobLogTail(ctx context.Context, owner, repo string, jobI // --------------------------------------------------------------------------- // ciSummaryFromGraphQL maps the per-PR status rollup onto domain.CIState. -// If ANY context concluded failure-class we return CIFailing. Otherwise -// any pending context wins over passing. An empty rollup is CIUnknown. +// If ANY visible context concluded failure-class we return CIFailing. +// Otherwise any pending context wins over passing. An empty rollup is +// CIUnknown. When the rollup is paginated (pageInfo.hasNextPage=true) +// the verdict is conservative: a known failure is still safe — failures +// don't get un-failed by more pages — but passing/pending/unknown +// verdicts could hide a failing context on the next page, so we degrade +// them all to CIUnknown rather than risk reporting a broken PR as ready. func ciSummaryFromGraphQL(pr map[string]any) domain.CIState { roll := statusRollup(pr) if roll == nil { @@ -268,6 +280,9 @@ func ciSummaryFromGraphQL(pr map[string]any) domain.CIState { passing = true } } + if pageInfoHasMore(contexts) { + return domain.CIUnknown + } switch { case pending: return domain.CIPending @@ -278,6 +293,18 @@ func ciSummaryFromGraphQL(pr map[string]any) domain.CIState { } } +// pageInfoHasMore reports whether the rollup contexts have a next page +// the current request didn't fetch. We treat a missing pageInfo block +// as "no more" (older API shapes that don't expose pagination simply +// return everything in one page). +func pageInfoHasMore(contexts map[string]any) bool { + pi, ok := contexts["pageInfo"].(map[string]any) + if !ok { + return false + } + return boolv(pi["hasNextPage"]) +} + func mapRollupState(s string) domain.CIState { switch strings.ToUpper(strings.TrimSpace(s)) { case "SUCCESS": @@ -312,7 +339,15 @@ func reviewDecisionFromGraphQL(pr map[string]any) domain.ReviewDecision { // and the already-derived CIState + ReviewDecision. The rules follow the // spec table in doc.go. func mergeabilityFromGraphQL(pr map[string]any, rest restPull, ci domain.CIState, review domain.ReviewDecision) domain.Mergeability { - state := strings.ToUpper(strings.TrimSpace(firstNonEmpty(str(pr["mergeStateStatus"]), rest.MergeStateStatus))) + // REST's mergeable_state is the tiebreaker: GraphQL's + // mergeStateStatus enum (DIRTY / BLOCKED / UNSTABLE / CLEAN / + // UNKNOWN) is the primary; if it is empty we fall back to the + // REST string (lowercase: "dirty" / "blocked" / "unstable" / + // "clean" / "behind" / "unknown") uppercased so the same switch + // covers both shapes. The REST API does NOT expose a + // `merge_state_status` field — earlier revs of this code chased + // that ghost; we use mergeable_state instead. + state := strings.ToUpper(strings.TrimSpace(firstNonEmpty(str(pr["mergeStateStatus"]), rest.MergeableState))) rawMergeable := strings.ToUpper(strings.TrimSpace(str(pr["mergeable"]))) switch state { diff --git a/backend/internal/adapters/scm/github/provider_test.go b/backend/internal/adapters/scm/github/provider_test.go index 82edf58a..eb407bdd 100644 --- a/backend/internal/adapters/scm/github/provider_test.go +++ b/backend/internal/adapters/scm/github/provider_test.go @@ -1007,6 +1007,65 @@ func TestGHTokenSourceUsesInjectedHook(t *testing.T) { } } +// TestObserve_CIPaginationDegradesPassingToUnknown pins the safety +// guard for the GraphQL contexts pagination: when GitHub reports +// hasNextPage=true, a visible "all passing" set could be hiding a +// failure on the next page. The provider must degrade Passing / +// Pending / Unknown to CIUnknown so downstream code doesn't treat a +// possibly-broken PR as ready. A FAILING verdict from the visible +// page is still safe (and must NOT degrade). +func TestObserve_CIPaginationDegradesPassingToUnknown(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + fx.prData(func(pr map[string]any) { + commits := pr["commits"].(map[string]any)["nodes"].([]any)[0].(map[string]any) + commit := commits["commit"].(map[string]any) + roll := commit["statusCheckRollup"].(map[string]any) + ctxs := roll["contexts"].(map[string]any) + // One visible passing context, but hasNextPage=true so a + // failure could be hiding in the unseen tail. + ctxs["nodes"] = []any{ + map[string]any{"__typename": "CheckRun", "name": "build", "status": "COMPLETED", "conclusion": "SUCCESS"}, + } + ctxs["pageInfo"] = map[string]any{"hasNextPage": true} + }) + fx.install(t, f) + p := newProviderForTest(t, f) + + obs, err := p.Observe(ctx(), fx.prURL()) + if err != nil { + t.Fatalf("Observe: %v", err) + } + if obs.CI != domain.CIUnknown { + t.Fatalf("CI = %q, want CIUnknown (hasNextPage must degrade passing)", obs.CI) + } +} + +func TestObserve_CIPaginationDoesNotMaskKnownFailure(t *testing.T) { + f := newFakeGH(t) + fx := basePRFixture() + fx.prData(func(pr map[string]any) { + commits := pr["commits"].(map[string]any)["nodes"].([]any)[0].(map[string]any) + commit := commits["commit"].(map[string]any) + roll := commit["statusCheckRollup"].(map[string]any) + ctxs := roll["contexts"].(map[string]any) + ctxs["nodes"] = []any{ + map[string]any{"__typename": "CheckRun", "name": "lint", "status": "COMPLETED", "conclusion": "FAILURE", "databaseId": float64(0)}, + } + ctxs["pageInfo"] = map[string]any{"hasNextPage": true} + }) + fx.install(t, f) + p := newProviderForTest(t, f) + + obs, err := p.Observe(ctx(), fx.prURL()) + if err != nil { + t.Fatalf("Observe: %v", err) + } + if obs.CI != domain.CIFailing { + t.Fatalf("CI = %q, want CIFailing (a known failure on page 1 must NOT degrade)", obs.CI) + } +} + // TestObserve_StatusContextLegacyHasNoLogTail pins that we do NOT try to // fetch a job log for a legacy commit-status row (those have no Actions // job ID, so /actions/jobs/0/logs would 404 if we let the path leak).