From d0021380e80f9359576bc55f1829b18ae1db5a9f Mon Sep 17 00:00:00 2001 From: Eric Hauser Date: Mon, 3 Aug 2026 10:26:41 -0600 Subject: [PATCH] Mirror changed-file and ownership inputs for reviewer selection Adds three ownership-input surfaces to the versioned v1 schema: changed-file snapshots per PR head (path, previous path for renames, status, fenced by the exact base and head SHAs of one observation -- adversarial tests prove a synchronize racing GraphQL pagination or the final REST hydration cannot persist mixed-head facts), the effective CODEOWNERS source with GitHub precedence (.github/, root, docs/) and ref/SHA provenance plus missing/oversized states, and resolved user/team owners for touched paths. Snapshots are replace-sets; GitHub's 3,000-file cap and omitted lists surface as explicit truncation state with documented resync semantics (boundary-tested at 101/3,000/3,001/omitted), never silent omission. CODEOWNERS resolution is a hand-written, dependency-free resolver verified against GitHub's documented sample and the sharp edges: anchoring, * versus ** slash behavior, trailing-slash directories, last-match-wins including ownerless rules clearing ownership, escaped spaces, comments, CRLF, case handling, and rename-new-path matching. Email owners persist as explicit owner_type=email unresolved facts; unknown/deleted identities are distinguished. Blame is deliberately not mirrored: CONTRACT.md documents the bounded consumer-side computation from changed paths, participation, and commit authorship, keeping ranking policy out of the engine. The documented diff-to-owner SQL join runs verbatim as a contract test. Ownership refreshes on head/base changes, force-pushes, reopen, reconciliation, and default-branch pushes touching a codeowners path (fanning out to cached live open PRs with queue-coalesced keys). One pull_request.changed per changing observation; identical refreshes emit none; C-C2 freshness gates cover equal-parent CODEOWNERS-only changes. Drift detects and heals all three fact families with Go-sorted null-safe comparisons and stable truncated-truth handling; the sampler stays sublinear; recordings remain additive-compatible. Also hardens TestMigrationLockWaitFailureClosesHijackedConnection's leak check: backend exit is asynchronous relative to client close, so the pg_stat_activity count now polls with a bound (flaked twice in gate runs), matching the earlier lock-release hardening. Fixes #11 Co-Authored-By: Claude Fable 5 --- cmd/loadgen/assertions.go | 351 ++++++++ cmd/loadgen/main_test.go | 31 + config/dispatcher-rules.yaml | 3 + db/CONTRACT.md | 177 +++- db/contract_test.go | 111 +++ .../0008_pull_request_change_inputs.sql | 363 +++++++++ db/queries/cache.sql | 461 ++++++++++- db/queries/loadgen.sql | 35 + docs/SYNC_ENGINE.md | 12 +- internal/changeinputs/changeinputs.go | 330 ++++++++ internal/codeowners/resolver.go | 296 +++++++ internal/codeowners/resolver_test.go | 291 +++++++ internal/dispatch/classify.go | 125 ++- internal/dispatch/classify_test.go | 69 +- internal/dispatch/dispatcher_db_test.go | 83 +- .../dispatch/testdata/s142_expected_jobs.json | 10 + internal/drift/drift.go | 31 + internal/drift/drift_db_test.go | 244 +++++- internal/fakegithub/fixture.go | 48 +- internal/fakegithub/graphql.go | 54 ++ internal/fakegithub/rest.go | 28 + internal/fakegithub/server.go | 59 +- internal/fakegithub/truth.go | 67 +- internal/fetch/coordinator.go | 23 + internal/fetch/fetch_db_test.go | 396 ++++++++- internal/fetch/handler.go | 30 +- internal/gh/graphql.go | 111 ++- internal/gh/graphql_files_test.go | 142 ++++ internal/gh/rest.go | 198 +++++ internal/gh/rest_codeowners_test.go | 134 ++++ internal/outbox/outbox.go | 4 +- internal/store/branch.go | 1 - internal/store/cache_db_test.go | 62 ++ internal/store/codeowners.go | 68 ++ internal/store/dbgen/cache.sql.go | 753 +++++++++++++++++- internal/store/dbgen/loadgen.sql.go | 170 ++++ internal/store/dbgen/models.go | 68 ++ internal/store/keys.go | 8 + internal/store/migrate_test.go | 38 +- internal/store/pull_request.go | 195 ++++- internal/store/records.go | 128 +++ internal/sweep/sweep_db_test.go | 12 +- 42 files changed, 5664 insertions(+), 156 deletions(-) create mode 100644 db/migrations/0008_pull_request_change_inputs.sql create mode 100644 internal/changeinputs/changeinputs.go create mode 100644 internal/codeowners/resolver.go create mode 100644 internal/codeowners/resolver_test.go create mode 100644 internal/gh/graphql_files_test.go create mode 100644 internal/gh/rest_codeowners_test.go create mode 100644 internal/store/codeowners.go diff --git a/cmd/loadgen/assertions.go b/cmd/loadgen/assertions.go index 5f8fa5e..5d1b591 100644 --- a/cmd/loadgen/assertions.go +++ b/cmd/loadgen/assertions.go @@ -20,7 +20,9 @@ import ( "github.com/prometheus/common/expfmt" "github.com/prometheus/common/model" + "github.com/ewhauser/ghsync/internal/codeowners" "github.com/ewhauser/ghsync/internal/fakegithub" + "github.com/ewhauser/ghsync/internal/gh" "github.com/ewhauser/ghsync/internal/ingress" ghsyncmetrics "github.com/ewhauser/ghsync/internal/metrics" "github.com/ewhauser/ghsync/internal/store" @@ -916,6 +918,45 @@ type oraclePullRequestComment struct { HeadSHA string } +type oracleChangeSnapshot struct { + Pull int + BaseSHA string + HeadSHA string + FilesTotal int + FilesTruncated bool + CodeownersRef string + CodeownersSHA string + CodeownersPath string + CodeownersState string + CodeownersSource string + CodeownersHash string +} + +type oracleChangedFile struct { + Pull int + Path string + PreviousPath string + ChangeType string + BaseSHA string + HeadSHA string +} + +type oracleFileOwner struct { + Pull int + Path string + Token string + Type string + Name string + ResolutionState string + GitHubID int64 + NodeID string + Login string + SourcePattern string + SourceLine int + BaseSHA string + HeadSHA string +} + type oracleStack struct { ID int64 NodeID string @@ -1105,6 +1146,9 @@ func assertFixtureConverged( cachedComments, ) } + if err := assertChangeInputsConverged(ctx, pool, truth); err != nil { + return err + } expectedStacks := make([]oracleStack, 0, len(truth.Stacks)) for _, stack := range truth.Stacks { @@ -1224,6 +1268,189 @@ func assertFixtureConverged( return nil } +func assertChangeInputsConverged( + ctx context.Context, + pool *pgxpool.Pool, + truth fakegithub.TruthFixtureSnapshot, +) error { + covered := false + for _, pull := range truth.PullRequests { + covered = covered || pull.CodeownersState != "" + } + if !covered { + // Additive compatibility for older committed/custom truth payloads. + return nil + } + type identity struct { + githubID int64 + nodeID string + login string + } + known := make(map[string]identity) + for _, pull := range truth.PullRequests { + for _, review := range pull.Reviews { + if review.Author.Kind == "user" && review.Author.NodeID != "" && + review.Author.Login != "" { + known["user\x00"+strings.ToLower(review.Author.Login)] = identity{ + nodeID: review.Author.NodeID, login: review.Author.Login, + } + } + } + for _, comment := range pull.Comments { + if comment.Author.Kind == "user" && comment.Author.NodeID != "" && + comment.Author.Login != "" { + known["user\x00"+strings.ToLower(comment.Author.Login)] = identity{ + nodeID: comment.Author.NodeID, login: comment.Author.Login, + } + } + } + } + // Review-request identities win in the store because they carry a stable + // database ID and sort ahead of participation-only candidates. + for _, pull := range truth.PullRequests { + for _, request := range pull.ReviewRequests { + if request.Kind == "user" || request.Kind == "team" { + known[request.Kind+"\x00"+strings.ToLower(request.Login)] = identity{ + githubID: request.ID, + nodeID: request.NodeID, + login: request.Login, + } + } + } + } + + expectedSnapshots := make([]oracleChangeSnapshot, 0, len(truth.PullRequests)) + expectedFiles := make([]oracleChangedFile, 0) + expectedOwners := make([]oracleFileOwner, 0) + for _, pull := range truth.PullRequests { + total := pull.ChangedFilesTotal + if total == 0 { + total = len(pull.ChangedFiles) + } + truncated := oracleChangedFilesTruncated( + pull.ChangedFiles, + total, + pull.ChangedFilesOmitted, + ) + expectedSnapshots = append(expectedSnapshots, oracleChangeSnapshot{ + Pull: pull.Number, + BaseSHA: pull.Base.SHA, + HeadSHA: pull.Head.SHA, + FilesTotal: total, + FilesTruncated: truncated, + CodeownersRef: pull.Base.Ref, + CodeownersSHA: pull.Base.SHA, + CodeownersPath: pull.CodeownersPath, + CodeownersState: pull.CodeownersState, + CodeownersSource: pull.CodeownersSource, + CodeownersHash: codeownersSourceHash( + pull.CodeownersState, + pull.CodeownersPath, + pull.CodeownersSource, + ), + }) + if pull.ChangedFilesOmitted { + continue + } + rules := codeowners.Parse(pull.CodeownersSource) + files := boundedOracleChangedFiles(pull.ChangedFiles) + for _, file := range files { + expectedFiles = append(expectedFiles, oracleChangedFile{ + Pull: pull.Number, + Path: file.Path, + PreviousPath: file.PreviousPath, + ChangeType: strings.ToLower(file.ChangeType), + BaseSHA: pull.Base.SHA, + HeadSHA: pull.Head.SHA, + }) + match, ok := codeowners.Resolve(rules, file.Path) + if !ok || pull.CodeownersState != "present" { + continue + } + seen := make(map[string]struct{}, len(match.Owners)) + for _, owner := range match.Owners { + if _, duplicate := seen[owner.Token]; duplicate { + continue + } + seen[owner.Token] = struct{}{} + value := oracleFileOwner{ + Pull: pull.Number, Path: file.Path, Token: owner.Token, + Type: string(owner.Type), Name: owner.Name, + ResolutionState: "unresolved", + SourcePattern: match.Pattern, SourceLine: match.Line, + BaseSHA: pull.Base.SHA, HeadSHA: pull.Head.SHA, + } + lookup := owner.Name + if owner.Type == codeowners.OwnerTeam { + parts := strings.SplitN(owner.Name, "/", 2) + if len(parts) != 2 || + !strings.EqualFold(parts[0], truth.Repository.Owner) { + expectedOwners = append(expectedOwners, value) + continue + } + lookup = parts[1] + } + if owner.Type == codeowners.OwnerUser || + owner.Type == codeowners.OwnerTeam { + if found, ok := known[string(owner.Type)+"\x00"+ + strings.ToLower(lookup)]; ok { + value.ResolutionState = "resolved" + value.GitHubID = found.githubID + value.NodeID = found.nodeID + value.Login = found.login + } + } + expectedOwners = append(expectedOwners, value) + } + } + } + + cachedSnapshots, err := readCachedChangeSnapshots( + ctx, pool, truth.Repository.FullName, + ) + if err != nil { + return err + } + cachedFiles, err := readCachedChangedFiles(ctx, pool, truth.Repository.FullName) + if err != nil { + return err + } + cachedOwners, err := readCachedFileOwners(ctx, pool, truth.Repository.FullName) + if err != nil { + return err + } + sort.Slice(expectedSnapshots, func(i, j int) bool { + return expectedSnapshots[i].Pull < expectedSnapshots[j].Pull + }) + sort.Slice(expectedFiles, func(i, j int) bool { + if expectedFiles[i].Pull == expectedFiles[j].Pull { + return expectedFiles[i].Path < expectedFiles[j].Path + } + return expectedFiles[i].Pull < expectedFiles[j].Pull + }) + sort.Slice(expectedOwners, func(i, j int) bool { + if expectedOwners[i].Pull != expectedOwners[j].Pull { + return expectedOwners[i].Pull < expectedOwners[j].Pull + } + if expectedOwners[i].Path != expectedOwners[j].Path { + return expectedOwners[i].Path < expectedOwners[j].Path + } + return expectedOwners[i].Token < expectedOwners[j].Token + }) + if !reflect.DeepEqual(expectedSnapshots, cachedSnapshots) || + !reflect.DeepEqual(expectedFiles, cachedFiles) || + !reflect.DeepEqual(expectedOwners, cachedOwners) { + return fmt.Errorf( + "pull-request change-input cache mismatch\n"+ + "truth snapshots=%+v files=%+v owners=%+v\n"+ + "cache snapshots=%+v files=%+v owners=%+v", + expectedSnapshots, expectedFiles, expectedOwners, + cachedSnapshots, cachedFiles, cachedOwners, + ) + } + return nil +} + func sortOraclePulls(pulls []oraclePull) { sort.Slice(pulls, func(i, j int) bool { return pulls[i].Number < pulls[j].Number @@ -1392,6 +1619,130 @@ func readCachedPullRequestComments( return result, nil } +func readCachedChangeSnapshots( + ctx context.Context, + pool *pgxpool.Pool, + repo string, +) ([]oracleChangeSnapshot, error) { + rows, err := dbgen.New(pool).ListLoadgenCachedPullRequestChangeSnapshots( + ctx, repo, + ) + if err != nil { + return nil, fmt.Errorf("query cached PR change snapshots: %w", err) + } + result := make([]oracleChangeSnapshot, 0, len(rows)) + for _, row := range rows { + result = append(result, oracleChangeSnapshot{ + Pull: int(row.PrNumber), BaseSHA: row.BaseSha, HeadSHA: row.HeadSha, + FilesTotal: int(row.FilesTotalCount), + FilesTruncated: row.FilesTruncated, + CodeownersRef: row.CodeownersRef, CodeownersSHA: row.CodeownersSha, + CodeownersPath: row.CodeownersPath.String, + CodeownersState: row.CodeownersState, + CodeownersSource: row.CodeownersSource.String, + CodeownersHash: row.CodeownersHash, + }) + } + sort.Slice(result, func(i, j int) bool { + return result[i].Pull < result[j].Pull + }) + return result, nil +} + +func codeownersSourceHash(state, path, source string) string { + digest := sha256.Sum256([]byte(state + "\x00" + path + "\x00" + source)) + return hex.EncodeToString(digest[:]) +} + +func boundedOracleChangedFiles( + files []fakegithub.ChangedFile, +) []fakegithub.ChangedFile { + if len(files) > gh.MaxPullRequestFiles { + return files[:gh.MaxPullRequestFiles] + } + return files +} + +func oracleChangedFilesTruncated( + files []fakegithub.ChangedFile, + total int, + omitted bool, +) bool { + if omitted || total != len(files) || total > gh.MaxPullRequestFiles { + return true + } + for _, file := range boundedOracleChangedFiles(files) { + if strings.EqualFold(file.ChangeType, "renamed") && + file.PreviousPath == "" { + return true + } + } + return false +} + +func readCachedChangedFiles( + ctx context.Context, + pool *pgxpool.Pool, + repo string, +) ([]oracleChangedFile, error) { + rows, err := dbgen.New(pool).ListLoadgenCachedPullRequestChangedFiles( + ctx, repo, + ) + if err != nil { + return nil, fmt.Errorf("query cached PR changed files: %w", err) + } + result := make([]oracleChangedFile, 0, len(rows)) + for _, row := range rows { + result = append(result, oracleChangedFile{ + Pull: int(row.PrNumber), Path: row.Path, + PreviousPath: row.PreviousPath.String, ChangeType: row.ChangeType, + BaseSHA: row.BaseSha, HeadSHA: row.HeadSha, + }) + } + sort.Slice(result, func(i, j int) bool { + if result[i].Pull == result[j].Pull { + return result[i].Path < result[j].Path + } + return result[i].Pull < result[j].Pull + }) + return result, nil +} + +func readCachedFileOwners( + ctx context.Context, + pool *pgxpool.Pool, + repo string, +) ([]oracleFileOwner, error) { + rows, err := dbgen.New(pool).ListLoadgenCachedPullRequestFileOwners( + ctx, repo, + ) + if err != nil { + return nil, fmt.Errorf("query cached PR file owners: %w", err) + } + result := make([]oracleFileOwner, 0, len(rows)) + for _, row := range rows { + result = append(result, oracleFileOwner{ + Pull: int(row.PrNumber), Path: row.Path, Token: row.OwnerToken, + Type: row.OwnerType, Name: row.OwnerName, + ResolutionState: row.ResolutionState, + GitHubID: row.OwnerGhID.Int64, NodeID: row.OwnerNodeID.String, + Login: row.OwnerLogin.String, SourcePattern: row.SourcePattern, + SourceLine: int(row.SourceLine), BaseSHA: row.BaseSha, + HeadSHA: row.HeadSha, + }) + } + sort.Slice(result, func(i, j int) bool { + if result[i].Pull != result[j].Pull { + return result[i].Pull < result[j].Pull + } + if result[i].Path != result[j].Path { + return result[i].Path < result[j].Path + } + return result[i].Token < result[j].Token + }) + return result, nil +} + func readCachedStacks( ctx context.Context, pool *pgxpool.Pool, diff --git a/cmd/loadgen/main_test.go b/cmd/loadgen/main_test.go index 135ffa8..743fe08 100644 --- a/cmd/loadgen/main_test.go +++ b/cmd/loadgen/main_test.go @@ -23,6 +23,7 @@ import ( "github.com/prometheus/common/model" "github.com/ewhauser/ghsync/internal/fakegithub" + "github.com/ewhauser/ghsync/internal/gh" "github.com/ewhauser/ghsync/internal/outbox" "github.com/ewhauser/ghsync/internal/replay" "github.com/ewhauser/ghsync/internal/store" @@ -73,6 +74,36 @@ func TestValidateConfigRejectsUnboundedAndUnownedChaos(t *testing.T) { } } +func TestLoadgenOracleUsesSameChangedFileCapAsMirror(t *testing.T) { + t.Parallel() + files := make([]fakegithub.ChangedFile, gh.MaxPullRequestFiles+1) + for index := range files { + files[index].Path = fmt.Sprintf("file-%04d", index) + } + bounded := boundedOracleChangedFiles(files) + if len(bounded) != gh.MaxPullRequestFiles || + bounded[len(bounded)-1].Path != "file-2999" { + t.Fatalf( + "bounded oracle files = %d ending %q", + len(bounded), bounded[len(bounded)-1].Path, + ) + } +} + +func TestLoadgenOracleMarksRenameWithoutPreviousPathTruncated(t *testing.T) { + t.Parallel() + files := []fakegithub.ChangedFile{{ + Path: "new/name.go", ChangeType: "renamed", + }} + if !oracleChangedFilesTruncated(files, len(files), false) { + t.Fatal("rename without previous path was treated as complete") + } + files[0].PreviousPath = "old/name.go" + if oracleChangedFilesTruncated(files, len(files), false) { + t.Fatal("complete rename was treated as truncated") + } +} + func TestEngineProcessSIGKILLRestart(t *testing.T) { t.Parallel() process := &engineProcess{command: "sleep 30"} diff --git a/config/dispatcher-rules.yaml b/config/dispatcher-rules.yaml index 6e2a23a..20fc1af 100644 --- a/config/dispatcher-rules.yaml +++ b/config/dispatcher-rules.yaml @@ -31,3 +31,6 @@ rules: target: branch # Pending real-payload validation: push payloads have no stack object. stacked_target: stack + - event: push + action: "*" + target: codeowners diff --git a/db/CONTRACT.md b/db/CONTRACT.md index 9401f38..f942095 100644 --- a/db/CONTRACT.md +++ b/db/CONTRACT.md @@ -3,7 +3,7 @@ Contract version: **v1**. The schema begins in the squashed baseline migrations (`0001` tables, `0002` functions and the database-enforced writer fence trigger, `0003` views) and is extended only by checksummed, -append-only migrations such as `0004` through `0007`. +append-only migrations such as `0004` through `0008`. Postgres is the ghsync sync engine’s public delivery interface. Consumers read snapshot-consistent cache rows and follow reference events through @@ -32,6 +32,9 @@ GRANT SELECT ON TABLE pull_request_review_requests, pull_request_reviews, pull_request_comments, + pull_request_change_snapshots, + pull_request_changed_files, + pull_request_file_owners, review_threads, check_runs, check_history, @@ -142,6 +145,55 @@ JSON value may be empty. | `pull_requests` | `tombstoned_at` | `timestamp with time zone` | yes | non-null means not live | | `pull_requests` | `last_checked_at` | `timestamp with time zone` | no | authoritative validation time | | `pull_requests` | `display_until` | `timestamp with time zone` | yes | closed-row display-retention boundary | +| `pull_request_change_snapshots` | `repo_id` | `bigint` | no | primary key part; references pull_requests(repo_id,number) | +| `pull_request_change_snapshots` | `pr_number` | `integer` | no | primary key part; repository-local PR number | +| `pull_request_change_snapshots` | `base_sha` | `text` | no | exact PR base fence; empty is the upstream-unknown sentinel | +| `pull_request_change_snapshots` | `head_sha` | `text` | no | exact PR head fence | +| `pull_request_change_snapshots` | `files_total_count` | `integer` | no | GitHub-reported changed-file total | +| `pull_request_change_snapshots` | `files_truncated` | `boolean` | no | true means the child file set is incomplete | +| `pull_request_change_snapshots` | `codeowners_ref` | `text` | no | PR base ref from which ownership applies | +| `pull_request_change_snapshots` | `codeowners_sha` | `text` | no | exact base commit read; empty when unavailable | +| `pull_request_change_snapshots` | `codeowners_path` | `text` | yes | effective source path selected by precedence | +| `pull_request_change_snapshots` | `codeowners_state` | `text` | no | present, missing, oversized, or unavailable | +| `pull_request_change_snapshots` | `codeowners_source` | `text` | yes | exact effective source; null unless present | +| `pull_request_change_snapshots` | `codeowners_hash` | `text` | no | source-state/path/content identity | +| `pull_request_change_snapshots` | `parent_gh_updated_at` | `timestamp with time zone` | no | parent-observation freshness fence | +| `pull_request_change_snapshots` | `synced_at` | `timestamp with time zone` | no | domain-change time | +| `pull_request_change_snapshots` | `etag` | `text` | no | HTTP validator provenance | +| `pull_request_change_snapshots` | `sync_source` | `text` | no | provenance enum | +| `pull_request_change_snapshots` | `tombstoned_at` | `timestamp with time zone` | yes | non-null means snapshot is not live | +| `pull_request_change_snapshots` | `last_checked_at` | `timestamp with time zone` | no | authoritative validation time | +| `pull_request_changed_files` | `repo_id` | `bigint` | no | primary key part; snapshot join key | +| `pull_request_changed_files` | `pr_number` | `integer` | no | primary key part; snapshot join key | +| `pull_request_changed_files` | `path` | `text` | no | primary key part; current repository-relative path | +| `pull_request_changed_files` | `previous_path` | `text` | yes | prior path for a rename | +| `pull_request_changed_files` | `change_type` | `text` | no | added, deleted, renamed, copied, modified, or changed | +| `pull_request_changed_files` | `base_sha` | `text` | no | copied snapshot base fence | +| `pull_request_changed_files` | `head_sha` | `text` | no | copied snapshot head fence | +| `pull_request_changed_files` | `synced_at` | `timestamp with time zone` | no | domain-change time | +| `pull_request_changed_files` | `etag` | `text` | no | HTTP validator provenance | +| `pull_request_changed_files` | `sync_source` | `text` | no | provenance enum | +| `pull_request_changed_files` | `tombstoned_at` | `timestamp with time zone` | yes | non-null means not in the current file set | +| `pull_request_changed_files` | `last_checked_at` | `timestamp with time zone` | no | authoritative validation time | +| `pull_request_file_owners` | `repo_id` | `bigint` | no | primary key part; changed-file join key | +| `pull_request_file_owners` | `pr_number` | `integer` | no | primary key part; changed-file join key | +| `pull_request_file_owners` | `path` | `text` | no | primary key part; changed-file join key | +| `pull_request_file_owners` | `owner_token` | `text` | no | primary key part; exact source token | +| `pull_request_file_owners` | `owner_type` | `text` | no | user, team, email, or malformed | +| `pull_request_file_owners` | `owner_name` | `text` | no | normalized lookup name; may be empty when malformed | +| `pull_request_file_owners` | `resolution_state` | `text` | no | resolved, unresolved, or deleted | +| `pull_request_file_owners` | `owner_gh_id` | `bigint` | yes | stable database identity when known | +| `pull_request_file_owners` | `owner_node_id` | `text` | yes | stable GraphQL identity when known | +| `pull_request_file_owners` | `owner_login` | `text` | yes | known current user login or team slug | +| `pull_request_file_owners` | `source_pattern` | `text` | no | last matching CODEOWNERS pattern | +| `pull_request_file_owners` | `source_line` | `integer` | no | one-based source line | +| `pull_request_file_owners` | `base_sha` | `text` | no | copied snapshot base fence | +| `pull_request_file_owners` | `head_sha` | `text` | no | copied snapshot head fence | +| `pull_request_file_owners` | `synced_at` | `timestamp with time zone` | no | domain-change time | +| `pull_request_file_owners` | `etag` | `text` | no | HTTP validator provenance | +| `pull_request_file_owners` | `sync_source` | `text` | no | provenance enum | +| `pull_request_file_owners` | `tombstoned_at` | `timestamp with time zone` | yes | non-null means owner is not current for the path | +| `pull_request_file_owners` | `last_checked_at` | `timestamp with time zone` | no | authoritative validation time | | `pull_request_review_requests` | `repo_id` | `bigint` | no | primary key part; references pull_requests(repo_id,number) | | `pull_request_review_requests` | `pr_number` | `integer` | no | primary key part; repository-local PR number | | `pull_request_review_requests` | `reviewer_kind` | `text` | no | primary key part; user or team | @@ -282,6 +334,125 @@ covered by the closed-entity C-R1 validation bound. `check_history` is append-only transition history retained for at least 90 days. Other tombstoned mirror skeletons have no v1 expiry. +### Changed files and ownership inputs + +`pull_request_change_snapshots` is the completeness and provenance fence for +two current replace sets. `pull_request_changed_files` mirrors the GraphQL PR +`files` connection through every cursor, bounded at GitHub's documented 3,000 +file limit. The REST files listing supplies `previous_path` for renames because +GraphQL does not expose it. The parent and every child row carry the exact +`base_sha`/`head_sha` pair observed for the diff. A page-to-page or final REST +fence change rejects the observation and retries it; it never combines pages +from two heads. + +`files_truncated = true` is explicit incomplete truth. It is set when GitHub +omits the connection, reports a total inconsistent with the returned set, +leaves a cursor beyond 3,000 files, or fails to supply a rename's previous +path. The stored rows remain useful positive facts, but consumers MUST NOT +infer that an absent path or owner is absent upstream. The next webhook, +backfill/reconciliation pass, or manual refresh replaces the set and may clear +the flag when GitHub returns a cursor-complete observation. There is no +out-of-band continuation beyond the cap. + +CODEOWNERS is read from the PR base commit, not its head. At the exact +`codeowners_sha`, ghsync selects `.github/CODEOWNERS`, then root `CODEOWNERS`, +then `docs/CODEOWNERS`; the first present file is effective and its path, +base ref, SHA, source, and hash are retained. An effective file at least 3 MiB +is `oversized` and does not fall through. No file at any location is the +successful `missing` empty-ownership state. If GitHub reports the base ref but +not its SHA, `codeowners_sha = ''` and `codeowners_state = 'unavailable'`; +ghsync does not silently read the moving ref. + +The pure resolver is case-sensitive and repository-root-relative. It applies +CODEOWNERS' gitignore-style pattern behavior, including root anchoring, +basename patterns, `*`, `?`, `**`, trailing-slash directory matches, escaped +spaces, and inline comments. The last matching rule wins as a whole. A later +matching rule with no owner tokens explicitly clears ownership for its matched +subtree. Negation (`!`), character ranges, and escaped leading `#` are not +CODEOWNERS features, so such pattern lines are ignored instead of being +interpreted as gitignore. +Duplicate owner tokens on the winning line collapse to one fact. Exact source +tokens are preserved: valid `@user`, `@org/team`, and email tokens receive a +syntactic type, while malformed tokens remain rows with `owner_type = +'malformed'`. Path matching is case-sensitive; source owner-token spelling is +also retained case-for-case, while user and team identity lookup is +case-insensitive to match GitHub login and slug identity. + +User and team tokens resolve only from stable identities already mirrored for +the repository; ghsync makes no live per-owner lookup. `resolved` carries the +known node identity, login/slug, and database ID when available. A token with +no matching identity is `unresolved`; absence alone is never promoted to +`deleted`. `deleted` is a distinct reserved state for an explicit upstream +deletion fact, and carries null identity columns, matching participation's +deleted-actor policy. Email and malformed tokens are always explicit +unresolved facts. These are ownership inputs, not reviewer scores, workload +policy, or recommendations. + +Consumers can join diff to ownership without Go or another GitHub read: + + +```sql +SELECT snapshot.base_sha, snapshot.head_sha, + snapshot.files_total_count, snapshot.files_truncated, + snapshot.codeowners_ref, snapshot.codeowners_sha, + snapshot.codeowners_path, snapshot.codeowners_state, + file.path, file.previous_path, file.change_type, + owner.owner_token, owner.owner_type, owner.resolution_state, + owner.owner_gh_id, owner.owner_node_id, owner.owner_login, + owner.source_pattern, owner.source_line +FROM pull_request_change_snapshots AS snapshot +JOIN pull_request_changed_files AS file + ON file.repo_id = snapshot.repo_id + AND file.pr_number = snapshot.pr_number + AND file.base_sha = snapshot.base_sha + AND file.head_sha = snapshot.head_sha + AND file.tombstoned_at IS NULL +LEFT JOIN pull_request_file_owners AS owner + ON owner.repo_id = file.repo_id + AND owner.pr_number = file.pr_number + AND owner.path = file.path + AND owner.base_sha = snapshot.base_sha + AND owner.head_sha = snapshot.head_sha + AND owner.tombstoned_at IS NULL +WHERE snapshot.repo_id = $1 + AND snapshot.pr_number = $2 + AND snapshot.tombstoned_at IS NULL +ORDER BY file.path, owner.owner_token; +``` + + +ghsync deliberately does not mirror blame: line-by-line history is unbounded +in changed lines, history depth, and API cost. A lower-cost offline overlap +input is the fenced changed-path set above, combined with PR authorship, +identity-keyed review/comment participation below, and commit authorship from +the consumer's mirrored/local Git objects keyed by `head_sha`. A consumer can +restrict commits to its chosen recent window, intersect their touched paths +with `pull_request_changed_files.path`, and then apply its own recency, +workload, or ranking policy. `pull_request_reviews.commit_oid` supplies an +additional source-derived commit association for submitted reviewers. This +keeps the mirror bounded and the engine policy-free while avoiding blame or a +second independently timed GitHub snapshot. + +The replace sets use the participation parent-observation gate: an observation +older than the current PR `gh_updated_at`, or whose base/head no longer equals +the parent row, cannot insert, update, tombstone, or merely freshen ownership +children. Equal parent versions remain eligible so a default-branch +CODEOWNERS change can update source facts even when GitHub does not change the +PR timestamp. Queue uniqueness coalesces repeated branch and PR refresh keys; +the fanout has no arbitrary count cutoff, so every cached open PR on the +affected branch remains covered. One accepted observation emits at most one +`pull_request.changed` reference if the snapshot, file set, or owner set +changes; an identical refresh only advances `last_checked_at` and emits none. + +All consumed `pull_request` actions, including synchronize, force-push/base +change, reopen, and stacked previews, retain a direct PR refresh. Branch pushes +refresh the finite cached set of live open PRs whose head or base uses that +branch, including stacked PRs; closed retained rows are outside this fanout. A +default-branch push that adds, modifies, or removes one of the three effective +CODEOWNERS paths is also classified explicitly and coalesces onto that branch +refresh. Backfill, reconciliation, branch refresh, and webhook work all reach +the same GraphQL/REST hydration path. + `pull_request_review_requests` is the authoritative current request set for a pull request. Live reads filter `tombstoned_at IS NULL` and distinguish users from teams with `reviewer_kind`; `reviewer_gh_id` and `reviewer_node_id` remain @@ -413,8 +584,8 @@ constructors used by the entity writer and deriver. | --- | --- | --- | --- | --- | | `entities` | `repository.changed` | `repo:{installation_id}:{repo_gh_id}` | `repos(installation_id,gh_id)` | `{"version":1}` | | `entities` | `repository.tombstoned` | `repo:{installation_id}:{repo_gh_id}` | `repos(installation_id,gh_id)` | `{"version":1}` | -| `entities` | `pull_request.changed` | `pr:{installation_id}:{repo_gh_id}:{pr_number}` | `pull_requests(repos.installation_id,repos.gh_id,number), pull_request_review_requests(repo_id,pr_number), pull_request_reviews(repo_id,pr_number), pull_request_comments(repo_id,pr_number)` | `{"version":1}` | -| `entities` | `pull_request.tombstoned` | `pr:{installation_id}:{repo_gh_id}:{pr_number}` | `pull_requests(repos.installation_id,repos.gh_id,number), pull_request_review_requests(repo_id,pr_number), pull_request_reviews(repo_id,pr_number), pull_request_comments(repo_id,pr_number)` | `{"version":1}` | +| `entities` | `pull_request.changed` | `pr:{installation_id}:{repo_gh_id}:{pr_number}` | `pull_requests(repos.installation_id,repos.gh_id,number), pull_request_review_requests(repo_id,pr_number), pull_request_reviews(repo_id,pr_number), pull_request_comments(repo_id,pr_number), pull_request_change_snapshots(repo_id,pr_number), pull_request_changed_files(repo_id,pr_number), pull_request_file_owners(repo_id,pr_number)` | `{"version":1}` | +| `entities` | `pull_request.tombstoned` | `pr:{installation_id}:{repo_gh_id}:{pr_number}` | `pull_requests(repos.installation_id,repos.gh_id,number), pull_request_review_requests(repo_id,pr_number), pull_request_reviews(repo_id,pr_number), pull_request_comments(repo_id,pr_number), pull_request_change_snapshots(repo_id,pr_number), pull_request_changed_files(repo_id,pr_number), pull_request_file_owners(repo_id,pr_number)` | `{"version":1}` | | `entities` | `stack.changed` | `stack:{installation_id}:{repo_gh_id}:{stack_number}` | `stacks(repos.installation_id,repos.gh_id,number)` | `{"version":1}` | | `entities` | `stack.tombstoned` | `stack:{installation_id}:{repo_gh_id}:{stack_number}` | `stacks(repos.installation_id,repos.gh_id,number)` | `{"version":1}` | | `entities` | `checks.changed` | `checks:{installation_id}:{repo_gh_id}:{head_sha}` | `check_runs(repos.installation_id,repos.gh_id,head_sha)` | `{"version":1}` | diff --git a/db/contract_test.go b/db/contract_test.go index 01d26f8..f0b276a 100644 --- a/db/contract_test.go +++ b/db/contract_test.go @@ -14,6 +14,7 @@ import ( "github.com/jackc/pgx/v5" "github.com/ewhauser/ghsync/internal/outbox" + "github.com/ewhauser/ghsync/internal/store" "github.com/ewhauser/ghsync/internal/testdb" ) @@ -111,6 +112,116 @@ func TestPublicSchemaManifestMatchesMigratedDatabase(t *testing.T) { } } +func TestDocumentedDiffToOwnerSQLRunsAgainstMigratedDatabase(t *testing.T) { + t.Parallel() + content, err := os.ReadFile("CONTRACT.md") + if err != nil { + t.Fatal(err) + } + const start = "\n```sql\n" + const end = "\n```\n" + _, query, ok := bytes.Cut(content, []byte(start)) + if !ok { + t.Fatal("missing documented diff-to-owner SQL start marker") + } + query, _, ok = bytes.Cut(query, []byte(end)) + if !ok { + t.Fatal("missing documented diff-to-owner SQL end marker") + } + database := testdb.New(t) + now := time.Date(2026, 8, 3, 12, 0, 0, 0, time.UTC) + repository := store.RepositoryRecord{ + InstallationID: 1, OrgID: 1, GitHubID: 11001, + NodeID: "repo-contract-query", Owner: "acme", Name: "contract-query", + FullName: "acme/contract-query", DefaultBranch: "main", + DefaultHeadSHA: "base-contract", GitHubUpdatedAt: now, + } + pull := store.PullRequestRecord{ + Repository: repository, GitHubID: 11002, NodeID: "pr-contract-query", + Number: 7, Title: "contract query", State: "open", + HeadRef: "feature", HeadSHA: "head-contract", + BaseRef: "main", BaseSHA: "base-contract", MembershipKnown: true, + GitHubUpdatedAt: now, SyncedAt: now, Source: store.SyncSourceReconcile, + ChangeInputsKnown: true, + ChangeSnapshot: &store.PullRequestChangeSnapshotRecord{ + BaseSHA: "base-contract", HeadSHA: "head-contract", + FilesTotalCount: 1, CodeownersRef: "main", + CodeownersSHA: "base-contract", CodeownersPath: "CODEOWNERS", + CodeownersState: "present", CodeownersSource: "*.go @owner", + CodeownersHash: "contract-source-hash", + Files: []store.ChangedFileRecord{{ + Path: "src/main.go", ChangeType: "modified", + }}, + Owners: []store.FileOwnerRecord{{ + Path: "src/main.go", OwnerToken: "@owner", OwnerType: "user", + OwnerName: "owner", ResolutionState: "unresolved", + SourcePattern: "*.go", SourceLine: 1, + }}, + }, + } + if _, err := store.NewEntityWriter(database.Pool).ApplyPullRequest( + t.Context(), pull, + ); err != nil { + t.Fatal(err) + } + var repoID int64 + if err := database.Pool.QueryRow( + t.Context(), "SELECT id FROM repos WHERE gh_id = $1", repository.GitHubID, + ).Scan(&repoID); err != nil { + t.Fatal(err) + } + rows, err := database.Pool.Query( + t.Context(), string(query), repoID, int32(pull.Number), + ) + if err != nil { + t.Fatalf("documented diff-to-owner SQL failed: %v", err) + } + defer rows.Close() + if !rows.Next() { + t.Fatalf("documented diff-to-owner SQL returned no row: %v", rows.Err()) + } + values, err := rows.Values() + if err != nil { + t.Fatal(err) + } + if len(values) != 19 || values[8] != "src/main.go" || values[11] != "@owner" { + t.Fatalf("documented diff-to-owner SQL row = %#v", values) + } + if rows.Next() { + t.Fatal("documented diff-to-owner SQL returned duplicate rows") + } + if err := rows.Err(); err != nil { + t.Fatal(err) + } +} + +func TestConsumerGrantIncludesChangeInputTables(t *testing.T) { + t.Parallel() + content, err := os.ReadFile("CONTRACT.md") + if err != nil { + t.Fatal(err) + } + start := bytes.Index(content, []byte("GRANT SELECT ON TABLE\n")) + if start < 0 { + t.Fatal("missing consumer SELECT grant") + } + grant := content[start:] + if end := bytes.Index(grant, []byte("\nTO ghsync_consumer;")); end >= 0 { + grant = grant[:end] + } else { + t.Fatal("unterminated consumer SELECT grant") + } + for _, table := range []string{ + "pull_request_change_snapshots", + "pull_request_changed_files", + "pull_request_file_owners", + } { + if !bytes.Contains(grant, []byte(table)) { + t.Fatalf("consumer SELECT grant omits %s", table) + } + } +} + func TestEventManifestMatchesWriterDefinitions(t *testing.T) { t.Parallel() rows := parseManifest(t, "CONTRACT.md", "v1-events", 5) diff --git a/db/migrations/0008_pull_request_change_inputs.sql b/db/migrations/0008_pull_request_change_inputs.sql new file mode 100644 index 0000000..a6d54a7 --- /dev/null +++ b/db/migrations/0008_pull_request_change_inputs.sql @@ -0,0 +1,363 @@ +-- Bounded current changed-file and CODEOWNERS inputs for pull requests. +-- The parent row is the completeness and provenance fence for both replace +-- sets. A true files_truncated means the listed paths and owners are only the +-- known prefix/subset and consumers must not infer absence. +CREATE TABLE pull_request_change_snapshots ( + repo_id bigint NOT NULL, + pr_number integer NOT NULL, + base_sha text NOT NULL, + head_sha text NOT NULL, + files_total_count integer NOT NULL, + files_truncated boolean NOT NULL, + codeowners_ref text NOT NULL, + codeowners_sha text NOT NULL, + codeowners_path text, + codeowners_state text NOT NULL, + codeowners_source text, + codeowners_hash text NOT NULL, + parent_gh_updated_at timestamp with time zone NOT NULL, + synced_at timestamp with time zone NOT NULL, + etag text DEFAULT ''::text NOT NULL, + sync_source text NOT NULL, + tombstoned_at timestamp with time zone, + last_checked_at timestamp with time zone NOT NULL, + CONSTRAINT pull_request_change_snapshots_pkey PRIMARY KEY ( + repo_id, pr_number + ), + CONSTRAINT pull_request_change_snapshots_pull_request_fkey + FOREIGN KEY (repo_id, pr_number) + REFERENCES pull_requests(repo_id, number), + CONSTRAINT pull_request_change_snapshots_pr_number_check + CHECK (pr_number > 0), + CONSTRAINT pull_request_change_snapshots_file_count_check + CHECK (files_total_count >= 0), + CONSTRAINT pull_request_change_snapshots_codeowners_state_check + CHECK (codeowners_state = ANY (ARRAY[ + 'present'::text, 'missing'::text, 'oversized'::text, + 'unavailable'::text + ])), + CONSTRAINT pull_request_change_snapshots_codeowners_shape_check CHECK ( + (codeowners_state = 'present' AND codeowners_path IS NOT NULL AND + codeowners_source IS NOT NULL) + OR + (codeowners_state = 'missing' AND codeowners_path IS NULL AND + codeowners_source IS NULL) + OR + (codeowners_state = 'oversized' AND codeowners_path IS NOT NULL AND + codeowners_source IS NULL) + OR + (codeowners_state = 'unavailable' AND codeowners_sha = '' AND + codeowners_path IS NULL AND codeowners_source IS NULL) + ), + CONSTRAINT pull_request_change_snapshots_codeowners_hash_check + CHECK (codeowners_hash <> ''::text), + CONSTRAINT pull_request_change_snapshots_sync_source_check + CHECK (sync_source = ANY (ARRAY[ + 'webhook'::text, 'reconcile'::text, 'backfill'::text, + 'manual'::text, 'interactive'::text + ])) +); + +CREATE TABLE pull_request_changed_files ( + repo_id bigint NOT NULL, + pr_number integer NOT NULL, + path text NOT NULL, + previous_path text, + change_type text NOT NULL, + base_sha text NOT NULL, + head_sha text NOT NULL, + synced_at timestamp with time zone NOT NULL, + etag text DEFAULT ''::text NOT NULL, + sync_source text NOT NULL, + tombstoned_at timestamp with time zone, + last_checked_at timestamp with time zone NOT NULL, + CONSTRAINT pull_request_changed_files_pkey PRIMARY KEY ( + repo_id, pr_number, path + ), + CONSTRAINT pull_request_changed_files_snapshot_fkey + FOREIGN KEY (repo_id, pr_number) + REFERENCES pull_request_change_snapshots(repo_id, pr_number), + CONSTRAINT pull_request_changed_files_path_check CHECK (path <> ''::text), + CONSTRAINT pull_request_changed_files_change_type_check CHECK ( + change_type = ANY (ARRAY[ + 'added'::text, 'deleted'::text, 'renamed'::text, + 'copied'::text, 'modified'::text, 'changed'::text + ]) + ), + CONSTRAINT pull_request_changed_files_sync_source_check + CHECK (sync_source = ANY (ARRAY[ + 'webhook'::text, 'reconcile'::text, 'backfill'::text, + 'manual'::text, 'interactive'::text + ])) +); + +CREATE INDEX pull_request_changed_files_live_pr_idx + ON pull_request_changed_files USING btree (repo_id, pr_number, path) + INCLUDE (previous_path, change_type, base_sha, head_sha, last_checked_at) + WHERE tombstoned_at IS NULL; + +CREATE TABLE pull_request_file_owners ( + repo_id bigint NOT NULL, + pr_number integer NOT NULL, + path text NOT NULL, + owner_token text NOT NULL, + owner_type text NOT NULL, + owner_name text NOT NULL, + resolution_state text NOT NULL, + owner_gh_id bigint, + owner_node_id text, + owner_login text, + source_pattern text NOT NULL, + source_line integer NOT NULL, + base_sha text NOT NULL, + head_sha text NOT NULL, + synced_at timestamp with time zone NOT NULL, + etag text DEFAULT ''::text NOT NULL, + sync_source text NOT NULL, + tombstoned_at timestamp with time zone, + last_checked_at timestamp with time zone NOT NULL, + CONSTRAINT pull_request_file_owners_pkey PRIMARY KEY ( + repo_id, pr_number, path, owner_token + ), + CONSTRAINT pull_request_file_owners_changed_file_fkey + FOREIGN KEY (repo_id, pr_number, path) + REFERENCES pull_request_changed_files(repo_id, pr_number, path), + CONSTRAINT pull_request_file_owners_owner_type_check CHECK ( + owner_type = ANY (ARRAY[ + 'user'::text, 'team'::text, 'email'::text, 'malformed'::text + ]) + ), + CONSTRAINT pull_request_file_owners_resolution_state_check CHECK ( + resolution_state = ANY (ARRAY[ + 'resolved'::text, 'unresolved'::text, 'deleted'::text + ]) + ), + CONSTRAINT pull_request_file_owners_identity_shape_check CHECK ( + (resolution_state = 'resolved' AND owner_node_id IS NOT NULL AND + owner_login IS NOT NULL) + OR + (resolution_state <> 'resolved' AND owner_gh_id IS NULL AND + owner_node_id IS NULL AND owner_login IS NULL) + ), + CONSTRAINT pull_request_file_owners_source_line_check + CHECK (source_line > 0), + CONSTRAINT pull_request_file_owners_sync_source_check + CHECK (sync_source = ANY (ARRAY[ + 'webhook'::text, 'reconcile'::text, 'backfill'::text, + 'manual'::text, 'interactive'::text + ])) +); + +CREATE INDEX pull_request_file_owners_live_pr_path_idx + ON pull_request_file_owners USING btree ( + repo_id, pr_number, path, owner_type, owner_token + ) INCLUDE ( + owner_name, resolution_state, owner_gh_id, owner_node_id, owner_login, + source_pattern, source_line, base_sha, head_sha, last_checked_at + ) + WHERE tombstoned_at IS NULL; + +-- Keep drift_entity_keys on its existing cheap key-only dependency. Replace +-- only the payload-bearing pull_request arm so selected PRs build ownership +-- JSON after keyset selection. +ALTER VIEW drift_entities RENAME TO drift_entities_without_change_inputs; + +CREATE VIEW drift_entities AS +SELECT prior.installation_id, + prior.entity_kind, + prior.source_id, + prior.entity_key, + prior.lock_key, + prior.cache_snapshot, + prior.last_checked_at +FROM drift_entities_without_change_inputs AS prior +WHERE prior.entity_kind <> 'pull_request'::text + +UNION ALL + +SELECT repos.installation_id, + 'pull_request'::text, + pull_requests.id, + ('pr:' || repos.full_name || ':' || pull_requests.number)::text, + ('pr:' || repos.installation_id || ':' || repos.gh_id || ':' || + pull_requests.number)::text, + jsonb_build_object( + 'id', pull_requests.gh_id, + 'node_id', pull_requests.node_id, + 'number', pull_requests.number, + 'title', pull_requests.title, + 'state', pull_requests.state, + 'draft', pull_requests.draft, + 'author_login', pull_requests.author_login, + 'head_ref', pull_requests.head_ref, + 'head_sha', pull_requests.head_sha, + 'base_ref', pull_requests.base_ref, + 'base_sha', pull_requests.base_sha, + 'review_decision', pull_requests.review_decision, + 'mergeable_state', pull_requests.mergeable_state, + 'stack_number', pull_requests.stack_number, + 'stack_position', pull_requests.stack_position, + 'review_requests', review_request_snapshot.requests, + 'reviews', review_snapshot.reviews, + 'comments', comment_snapshot.comments, + 'change_inputs', CASE + WHEN change_snapshot.repo_id IS NULL THEN NULL + ELSE jsonb_build_object( + 'base_sha', change_snapshot.base_sha, + 'head_sha', change_snapshot.head_sha, + 'files_total_count', change_snapshot.files_total_count, + 'files_truncated', change_snapshot.files_truncated, + 'codeowners_ref', change_snapshot.codeowners_ref, + 'codeowners_sha', change_snapshot.codeowners_sha, + 'codeowners_path', change_snapshot.codeowners_path, + 'codeowners_state', change_snapshot.codeowners_state, + 'codeowners_hash', change_snapshot.codeowners_hash, + 'files', changed_file_snapshot.files, + 'owners', file_owner_snapshot.owners + ) + END + ), + GREATEST( + pull_requests.last_checked_at, + review_request_snapshot.last_checked_at, + review_snapshot.last_checked_at, + comment_snapshot.last_checked_at, + COALESCE( + change_snapshot.last_checked_at, + pull_requests.last_checked_at + ), + changed_file_snapshot.last_checked_at, + file_owner_snapshot.last_checked_at + ) +FROM pull_requests +JOIN repos ON repos.id = pull_requests.repo_id +LEFT JOIN pull_request_change_snapshots AS change_snapshot + ON change_snapshot.repo_id = pull_requests.repo_id + AND change_snapshot.pr_number = pull_requests.number + AND change_snapshot.tombstoned_at IS NULL +CROSS JOIN LATERAL ( + SELECT COALESCE( + jsonb_agg( + jsonb_build_object( + 'kind', request.reviewer_kind, + 'id', request.reviewer_gh_id, + 'node_id', request.reviewer_node_id, + 'login', request.reviewer_login, + 'head_sha', request.head_sha + ) + ORDER BY request.reviewer_kind, request.reviewer_gh_id + ), + '[]'::jsonb + ) AS requests, + COALESCE(max(request.last_checked_at), pull_requests.last_checked_at) + AS last_checked_at + FROM pull_request_review_requests AS request + WHERE request.repo_id = pull_requests.repo_id + AND request.pr_number = pull_requests.number + AND request.tombstoned_at IS NULL +) AS review_request_snapshot +CROSS JOIN LATERAL ( + SELECT COALESCE( + jsonb_agg( + jsonb_build_object( + 'id', review.gh_id, + 'node_id', review.node_id, + 'author_kind', review.author_kind, + 'author_node_id', review.author_node_id, + 'author_login', review.author_login, + 'state', review.state, + 'submitted_at', CASE + WHEN review.submitted_at IS NULL THEN NULL + ELSE (extract(epoch FROM review.submitted_at) * + 1000000)::bigint + END, + 'commit_oid', review.commit_oid, + 'updated_at', + (extract(epoch FROM review.gh_updated_at) * + 1000000)::bigint, + 'head_sha', review.head_sha + ) + ORDER BY review.node_id + ), + '[]'::jsonb + ) AS reviews, + COALESCE(max(review.last_checked_at), pull_requests.last_checked_at) + AS last_checked_at + FROM pull_request_reviews AS review + WHERE review.repo_id = pull_requests.repo_id + AND review.pr_number = pull_requests.number + AND review.tombstoned_at IS NULL +) AS review_snapshot +CROSS JOIN LATERAL ( + SELECT COALESCE( + jsonb_agg( + jsonb_build_object( + 'id', comment.gh_id, + 'node_id', comment.node_id, + 'author_kind', comment.author_kind, + 'author_node_id', comment.author_node_id, + 'author_login', comment.author_login, + 'created_at', + (extract(epoch FROM comment.created_at) * + 1000000)::bigint, + 'updated_at', + (extract(epoch FROM comment.gh_updated_at) * + 1000000)::bigint, + 'head_sha', comment.head_sha + ) + ORDER BY comment.node_id + ), + '[]'::jsonb + ) AS comments, + COALESCE(max(comment.last_checked_at), pull_requests.last_checked_at) + AS last_checked_at + FROM pull_request_comments AS comment + WHERE comment.repo_id = pull_requests.repo_id + AND comment.pr_number = pull_requests.number + AND comment.tombstoned_at IS NULL +) AS comment_snapshot +CROSS JOIN LATERAL ( + SELECT COALESCE( + jsonb_agg( + jsonb_build_object( + 'path', file.path, + 'previous_path', file.previous_path, + 'change_type', file.change_type + ) ORDER BY file.path + ), + '[]'::jsonb + ) AS files, + COALESCE(max(file.last_checked_at), pull_requests.last_checked_at) + AS last_checked_at + FROM pull_request_changed_files AS file + WHERE file.repo_id = pull_requests.repo_id + AND file.pr_number = pull_requests.number + AND file.tombstoned_at IS NULL +) AS changed_file_snapshot +CROSS JOIN LATERAL ( + SELECT COALESCE( + jsonb_agg( + jsonb_build_object( + 'path', owner.path, + 'owner_token', owner.owner_token, + 'owner_type', owner.owner_type, + 'owner_name', owner.owner_name, + 'resolution_state', owner.resolution_state, + 'owner_gh_id', owner.owner_gh_id, + 'owner_node_id', owner.owner_node_id, + 'owner_login', owner.owner_login, + 'source_pattern', owner.source_pattern, + 'source_line', owner.source_line + ) ORDER BY owner.path, owner.owner_token + ), + '[]'::jsonb + ) AS owners, + COALESCE(max(owner.last_checked_at), pull_requests.last_checked_at) + AS last_checked_at + FROM pull_request_file_owners AS owner + WHERE owner.repo_id = pull_requests.repo_id + AND owner.pr_number = pull_requests.number + AND owner.tombstoned_at IS NULL +) AS file_owner_snapshot +WHERE repos.tombstoned_at IS NULL + AND pull_requests.tombstoned_at IS NULL; diff --git a/db/queries/cache.sql b/db/queries/cache.sql index 012b71e..657576d 100644 --- a/db/queries/cache.sql +++ b/db/queries/cache.sql @@ -43,7 +43,8 @@ SET installation_id = EXCLUDED.installation_id, default_branch = EXCLUDED.default_branch, archived = EXCLUDED.archived, gh_updated_at = EXCLUDED.gh_updated_at, - head_sha = EXCLUDED.head_sha, + head_sha = CASE WHEN EXCLUDED.head_sha = '' THEN repos.head_sha + ELSE EXCLUDED.head_sha END, synced_at = EXCLUDED.synced_at, last_checked_at = EXCLUDED.last_checked_at, etag = EXCLUDED.etag, @@ -56,7 +57,9 @@ WHERE repos.gh_updated_at IS NULL AND ROW( EXCLUDED.installation_id, EXCLUDED.org_id, EXCLUDED.node_id, EXCLUDED.owner, EXCLUDED.name, EXCLUDED.full_name, - EXCLUDED.default_branch, EXCLUDED.archived, EXCLUDED.head_sha + EXCLUDED.default_branch, EXCLUDED.archived, + CASE WHEN EXCLUDED.head_sha = '' THEN repos.head_sha + ELSE EXCLUDED.head_sha END ) IS DISTINCT FROM ROW( repos.installation_id, repos.org_id, repos.node_id, repos.owner, repos.name, repos.full_name, @@ -927,6 +930,459 @@ WHERE repo_id = sqlc.arg(repo_id) AND tombstoned_at IS NULL RETURNING node_id; +-- name: UpsertPullRequestChangeSnapshot :one +-- C-C2 parent freshness plus explicit base/head identity gates the current +-- changed-file and ownership snapshot. Equal parent versions remain eligible +-- so a base-branch CODEOWNERS push or a later complete listing can heal facts +-- without relying on pull_request.updatedAt changing. +WITH eligible AS ( + SELECT pull_requests.repo_id + FROM pull_requests + WHERE pull_requests.repo_id = sqlc.arg(repo_id) + AND pull_requests.number = sqlc.arg(pr_number) + AND pull_requests.tombstoned_at IS NULL + AND pull_requests.head_sha = sqlc.arg(head_sha) + AND pull_requests.base_sha = sqlc.arg(base_sha) + AND ( + pull_requests.gh_updated_at IS NULL + OR pull_requests.gh_updated_at <= sqlc.arg(parent_gh_updated_at) + ) +), +prior AS MATERIALIZED ( + SELECT base_sha, head_sha, files_total_count, files_truncated, + codeowners_ref, codeowners_sha, codeowners_path, + codeowners_state, codeowners_source, codeowners_hash, + tombstoned_at + FROM pull_request_change_snapshots + WHERE repo_id = sqlc.arg(repo_id) + AND pr_number = sqlc.arg(pr_number) +), +upserted AS ( + INSERT INTO pull_request_change_snapshots ( + repo_id, pr_number, base_sha, head_sha, files_total_count, + files_truncated, codeowners_ref, codeowners_sha, codeowners_path, + codeowners_state, codeowners_source, codeowners_hash, + parent_gh_updated_at, synced_at, etag, sync_source, tombstoned_at, + last_checked_at + ) + SELECT sqlc.arg(repo_id), sqlc.arg(pr_number), sqlc.arg(base_sha), + sqlc.arg(head_sha), sqlc.arg(files_total_count), + sqlc.arg(files_truncated), sqlc.arg(codeowners_ref), + sqlc.arg(codeowners_sha), sqlc.narg(codeowners_path), + sqlc.arg(codeowners_state), sqlc.narg(codeowners_source), + sqlc.arg(codeowners_hash), sqlc.arg(parent_gh_updated_at), + sqlc.arg(synced_at), sqlc.arg(etag), sqlc.arg(sync_source), NULL, + sqlc.arg(last_checked_at) + FROM eligible + ON CONFLICT (repo_id, pr_number) DO UPDATE + SET base_sha = EXCLUDED.base_sha, + head_sha = EXCLUDED.head_sha, + files_total_count = EXCLUDED.files_total_count, + files_truncated = EXCLUDED.files_truncated, + codeowners_ref = EXCLUDED.codeowners_ref, + codeowners_sha = EXCLUDED.codeowners_sha, + codeowners_path = EXCLUDED.codeowners_path, + codeowners_state = EXCLUDED.codeowners_state, + codeowners_source = EXCLUDED.codeowners_source, + codeowners_hash = EXCLUDED.codeowners_hash, + parent_gh_updated_at = EXCLUDED.parent_gh_updated_at, + synced_at = CASE + WHEN pull_request_change_snapshots.tombstoned_at IS NOT NULL + OR ROW( + EXCLUDED.base_sha, EXCLUDED.head_sha, + EXCLUDED.files_total_count, EXCLUDED.files_truncated, + EXCLUDED.codeowners_ref, EXCLUDED.codeowners_sha, + EXCLUDED.codeowners_path, EXCLUDED.codeowners_state, + EXCLUDED.codeowners_source, EXCLUDED.codeowners_hash + ) IS DISTINCT FROM ROW( + pull_request_change_snapshots.base_sha, + pull_request_change_snapshots.head_sha, + pull_request_change_snapshots.files_total_count, + pull_request_change_snapshots.files_truncated, + pull_request_change_snapshots.codeowners_ref, + pull_request_change_snapshots.codeowners_sha, + pull_request_change_snapshots.codeowners_path, + pull_request_change_snapshots.codeowners_state, + pull_request_change_snapshots.codeowners_source, + pull_request_change_snapshots.codeowners_hash + ) + THEN EXCLUDED.synced_at + ELSE pull_request_change_snapshots.synced_at + END, + etag = EXCLUDED.etag, + sync_source = CASE + WHEN pull_request_change_snapshots.tombstoned_at IS NOT NULL + OR ROW( + EXCLUDED.base_sha, EXCLUDED.head_sha, + EXCLUDED.files_total_count, EXCLUDED.files_truncated, + EXCLUDED.codeowners_ref, EXCLUDED.codeowners_sha, + EXCLUDED.codeowners_path, EXCLUDED.codeowners_state, + EXCLUDED.codeowners_source, EXCLUDED.codeowners_hash + ) IS DISTINCT FROM ROW( + pull_request_change_snapshots.base_sha, + pull_request_change_snapshots.head_sha, + pull_request_change_snapshots.files_total_count, + pull_request_change_snapshots.files_truncated, + pull_request_change_snapshots.codeowners_ref, + pull_request_change_snapshots.codeowners_sha, + pull_request_change_snapshots.codeowners_path, + pull_request_change_snapshots.codeowners_state, + pull_request_change_snapshots.codeowners_source, + pull_request_change_snapshots.codeowners_hash + ) + THEN EXCLUDED.sync_source + ELSE pull_request_change_snapshots.sync_source + END, + tombstoned_at = NULL, + last_checked_at = EXCLUDED.last_checked_at + WHERE EXCLUDED.parent_gh_updated_at > + pull_request_change_snapshots.parent_gh_updated_at + OR ( + EXCLUDED.parent_gh_updated_at = + pull_request_change_snapshots.parent_gh_updated_at + AND ROW( + EXCLUDED.base_sha, EXCLUDED.head_sha, + EXCLUDED.files_total_count, EXCLUDED.files_truncated, + EXCLUDED.codeowners_ref, EXCLUDED.codeowners_sha, + EXCLUDED.codeowners_path, EXCLUDED.codeowners_state, + EXCLUDED.codeowners_source, EXCLUDED.codeowners_hash + ) IS DISTINCT FROM ROW( + pull_request_change_snapshots.base_sha, + pull_request_change_snapshots.head_sha, + pull_request_change_snapshots.files_total_count, + pull_request_change_snapshots.files_truncated, + pull_request_change_snapshots.codeowners_ref, + pull_request_change_snapshots.codeowners_sha, + pull_request_change_snapshots.codeowners_path, + pull_request_change_snapshots.codeowners_state, + pull_request_change_snapshots.codeowners_source, + pull_request_change_snapshots.codeowners_hash + ) + ) + OR ( + pull_request_change_snapshots.tombstoned_at IS NOT NULL + AND EXCLUDED.last_checked_at > + pull_request_change_snapshots.tombstoned_at + ) + RETURNING repo_id +) +SELECT count(*) +FROM upserted +WHERE NOT EXISTS (SELECT 1 FROM prior) + OR EXISTS ( + SELECT 1 + FROM prior + WHERE prior.tombstoned_at IS NOT NULL + OR ROW( + prior.base_sha, prior.head_sha, prior.files_total_count, + prior.files_truncated, prior.codeowners_ref, + prior.codeowners_sha, prior.codeowners_path, + prior.codeowners_state, prior.codeowners_source, + prior.codeowners_hash + ) IS DISTINCT FROM ROW( + sqlc.arg(base_sha)::text, sqlc.arg(head_sha)::text, + sqlc.arg(files_total_count)::integer, + sqlc.arg(files_truncated)::boolean, + sqlc.arg(codeowners_ref)::text, + sqlc.arg(codeowners_sha)::text, + sqlc.narg(codeowners_path)::text, + sqlc.arg(codeowners_state)::text, + sqlc.narg(codeowners_source)::text, + sqlc.arg(codeowners_hash)::text + ) + ); + +-- name: ReplacePullRequestChangedFiles :many +WITH input AS ( + SELECT element->>'path' AS path, + NULLIF(element->>'previous_path', '') AS previous_path, + element->>'change_type' AS change_type + FROM jsonb_array_elements(sqlc.arg(changed_files)::jsonb) AS element +), +eligible AS ( + SELECT snapshot.repo_id + FROM pull_request_change_snapshots AS snapshot + WHERE snapshot.repo_id = sqlc.arg(repo_id) + AND snapshot.pr_number = sqlc.arg(pr_number) + AND snapshot.tombstoned_at IS NULL + AND snapshot.base_sha = sqlc.arg(base_sha) + AND snapshot.head_sha = sqlc.arg(head_sha) + AND snapshot.parent_gh_updated_at <= sqlc.arg(parent_gh_updated_at) +), +upserted AS ( + INSERT INTO pull_request_changed_files ( + repo_id, pr_number, path, previous_path, change_type, base_sha, + head_sha, synced_at, etag, sync_source, tombstoned_at, + last_checked_at + ) + SELECT sqlc.arg(repo_id), sqlc.arg(pr_number), input.path, + input.previous_path, input.change_type, sqlc.arg(base_sha), + sqlc.arg(head_sha), sqlc.arg(synced_at), sqlc.arg(etag), + sqlc.arg(sync_source), NULL, sqlc.arg(last_checked_at) + FROM input + CROSS JOIN eligible + ON CONFLICT (repo_id, pr_number, path) DO UPDATE + SET previous_path = EXCLUDED.previous_path, + change_type = EXCLUDED.change_type, + base_sha = EXCLUDED.base_sha, + head_sha = EXCLUDED.head_sha, + synced_at = EXCLUDED.synced_at, + etag = EXCLUDED.etag, + sync_source = EXCLUDED.sync_source, + tombstoned_at = NULL, + last_checked_at = EXCLUDED.last_checked_at + WHERE ROW( + EXCLUDED.previous_path, EXCLUDED.change_type, + EXCLUDED.base_sha, EXCLUDED.head_sha + ) IS DISTINCT FROM ROW( + pull_request_changed_files.previous_path, + pull_request_changed_files.change_type, + pull_request_changed_files.base_sha, + pull_request_changed_files.head_sha + ) + OR pull_request_changed_files.tombstoned_at IS NOT NULL + RETURNING path +), +tombstoned AS ( + UPDATE pull_request_changed_files + SET tombstoned_at = sqlc.arg(last_checked_at), + synced_at = sqlc.arg(synced_at), + last_checked_at = sqlc.arg(last_checked_at), + etag = sqlc.arg(etag), + sync_source = sqlc.arg(sync_source) + WHERE repo_id = sqlc.arg(repo_id) + AND pr_number = sqlc.arg(pr_number) + AND tombstoned_at IS NULL + AND EXISTS (SELECT 1 FROM eligible) + AND NOT EXISTS ( + SELECT 1 FROM input + WHERE input.path = pull_request_changed_files.path + ) + RETURNING path +) +SELECT path FROM upserted +UNION ALL +SELECT path FROM tombstoned; + +-- name: ReplacePullRequestFileOwners :many +WITH input AS ( + SELECT element->>'path' AS path, + element->>'owner_token' AS owner_token, + element->>'owner_type' AS owner_type, + element->>'owner_name' AS owner_name, + element->>'resolution_state' AS resolution_state, + NULLIF((element->>'owner_gh_id')::bigint, 0) AS owner_gh_id, + NULLIF(element->>'owner_node_id', '') AS owner_node_id, + NULLIF(element->>'owner_login', '') AS owner_login, + element->>'source_pattern' AS source_pattern, + (element->>'source_line')::integer AS source_line + FROM jsonb_array_elements(sqlc.arg(file_owners)::jsonb) AS element +), +eligible AS ( + SELECT snapshot.repo_id + FROM pull_request_change_snapshots AS snapshot + WHERE snapshot.repo_id = sqlc.arg(repo_id) + AND snapshot.pr_number = sqlc.arg(pr_number) + AND snapshot.tombstoned_at IS NULL + AND snapshot.base_sha = sqlc.arg(base_sha) + AND snapshot.head_sha = sqlc.arg(head_sha) + AND snapshot.parent_gh_updated_at <= sqlc.arg(parent_gh_updated_at) +), +upserted AS ( + INSERT INTO pull_request_file_owners ( + repo_id, pr_number, path, owner_token, owner_type, owner_name, + resolution_state, owner_gh_id, owner_node_id, owner_login, + source_pattern, source_line, base_sha, head_sha, synced_at, etag, + sync_source, tombstoned_at, last_checked_at + ) + SELECT sqlc.arg(repo_id), sqlc.arg(pr_number), input.path, + input.owner_token, input.owner_type, input.owner_name, + input.resolution_state, + input.owner_gh_id, input.owner_node_id, input.owner_login, + input.source_pattern, input.source_line, sqlc.arg(base_sha), + sqlc.arg(head_sha), sqlc.arg(synced_at), sqlc.arg(etag), + sqlc.arg(sync_source), NULL, sqlc.arg(last_checked_at) + FROM input + CROSS JOIN eligible + ON CONFLICT (repo_id, pr_number, path, owner_token) DO UPDATE + SET owner_type = EXCLUDED.owner_type, + owner_name = EXCLUDED.owner_name, + resolution_state = EXCLUDED.resolution_state, + owner_gh_id = EXCLUDED.owner_gh_id, + owner_node_id = EXCLUDED.owner_node_id, + owner_login = EXCLUDED.owner_login, + source_pattern = EXCLUDED.source_pattern, + source_line = EXCLUDED.source_line, + base_sha = EXCLUDED.base_sha, + head_sha = EXCLUDED.head_sha, + synced_at = EXCLUDED.synced_at, + etag = EXCLUDED.etag, + sync_source = EXCLUDED.sync_source, + tombstoned_at = NULL, + last_checked_at = EXCLUDED.last_checked_at + WHERE ROW( + EXCLUDED.owner_type, EXCLUDED.owner_name, + EXCLUDED.resolution_state, + EXCLUDED.owner_gh_id, EXCLUDED.owner_node_id, + EXCLUDED.owner_login, EXCLUDED.source_pattern, + EXCLUDED.source_line, EXCLUDED.base_sha, EXCLUDED.head_sha + ) IS DISTINCT FROM ROW( + pull_request_file_owners.owner_type, + pull_request_file_owners.owner_name, + pull_request_file_owners.resolution_state, + pull_request_file_owners.owner_gh_id, + pull_request_file_owners.owner_node_id, + pull_request_file_owners.owner_login, + pull_request_file_owners.source_pattern, + pull_request_file_owners.source_line, + pull_request_file_owners.base_sha, + pull_request_file_owners.head_sha + ) + OR pull_request_file_owners.tombstoned_at IS NOT NULL + RETURNING (path || ':' || owner_token)::text AS owner_key +), +tombstoned AS ( + UPDATE pull_request_file_owners + SET tombstoned_at = sqlc.arg(last_checked_at), + synced_at = sqlc.arg(synced_at), + last_checked_at = sqlc.arg(last_checked_at), + etag = sqlc.arg(etag), + sync_source = sqlc.arg(sync_source) + WHERE repo_id = sqlc.arg(repo_id) + AND pr_number = sqlc.arg(pr_number) + AND tombstoned_at IS NULL + AND EXISTS (SELECT 1 FROM eligible) + AND NOT EXISTS ( + SELECT 1 FROM input + WHERE input.path = pull_request_file_owners.path + AND input.owner_token = pull_request_file_owners.owner_token + ) + RETURNING (path || ':' || owner_token)::text AS owner_key +) +SELECT owner_key FROM upserted +UNION ALL +SELECT owner_key FROM tombstoned; + +-- name: TouchPullRequestChangeInputsCheckedAt :exec +WITH eligible AS ( + SELECT snapshot.repo_id + FROM pull_request_change_snapshots AS snapshot + WHERE snapshot.repo_id = sqlc.arg(repo_id) + AND snapshot.pr_number = sqlc.arg(pr_number) + AND snapshot.tombstoned_at IS NULL + AND snapshot.parent_gh_updated_at <= sqlc.arg(parent_gh_updated_at) +) +UPDATE pull_request_change_snapshots AS snapshot +SET last_checked_at = GREATEST(snapshot.last_checked_at, sqlc.arg(checked_at)), + etag = CASE WHEN sqlc.arg(etag)::text = '' THEN snapshot.etag + ELSE sqlc.arg(etag)::text END +WHERE snapshot.repo_id = sqlc.arg(repo_id) + AND snapshot.pr_number = sqlc.arg(pr_number) + AND EXISTS (SELECT 1 FROM eligible); + +-- name: TouchPullRequestChangedFilesCheckedAt :exec +UPDATE pull_request_changed_files AS file +SET last_checked_at = GREATEST(file.last_checked_at, sqlc.arg(checked_at)), + etag = CASE WHEN sqlc.arg(etag)::text = '' THEN file.etag + ELSE sqlc.arg(etag)::text END +WHERE file.repo_id = sqlc.arg(repo_id) + AND file.pr_number = sqlc.arg(pr_number) + AND file.tombstoned_at IS NULL + AND EXISTS ( + SELECT 1 FROM pull_request_change_snapshots AS snapshot + WHERE snapshot.repo_id = sqlc.arg(repo_id) + AND snapshot.pr_number = sqlc.arg(pr_number) + AND snapshot.tombstoned_at IS NULL + AND snapshot.parent_gh_updated_at <= sqlc.arg(parent_gh_updated_at) + ); + +-- name: TouchPullRequestFileOwnersCheckedAt :exec +UPDATE pull_request_file_owners AS owner +SET last_checked_at = GREATEST(owner.last_checked_at, sqlc.arg(checked_at)), + etag = CASE WHEN sqlc.arg(etag)::text = '' THEN owner.etag + ELSE sqlc.arg(etag)::text END +WHERE owner.repo_id = sqlc.arg(repo_id) + AND owner.pr_number = sqlc.arg(pr_number) + AND owner.tombstoned_at IS NULL + AND EXISTS ( + SELECT 1 FROM pull_request_change_snapshots AS snapshot + WHERE snapshot.repo_id = sqlc.arg(repo_id) + AND snapshot.pr_number = sqlc.arg(pr_number) + AND snapshot.tombstoned_at IS NULL + AND snapshot.parent_gh_updated_at <= sqlc.arg(parent_gh_updated_at) + ); + +-- name: TombstonePullRequestChangeSnapshot :execrows +UPDATE pull_request_change_snapshots +SET tombstoned_at = sqlc.arg(tombstoned_at), + synced_at = sqlc.arg(tombstoned_at), + last_checked_at = GREATEST(last_checked_at, sqlc.arg(tombstoned_at)), + etag = '', + sync_source = sqlc.arg(sync_source) +WHERE repo_id = sqlc.arg(repo_id) + AND pr_number = sqlc.arg(pr_number) + AND tombstoned_at IS NULL; + +-- name: TombstonePullRequestChangedFiles :execrows +UPDATE pull_request_changed_files +SET tombstoned_at = sqlc.arg(tombstoned_at), + synced_at = sqlc.arg(tombstoned_at), + last_checked_at = GREATEST(last_checked_at, sqlc.arg(tombstoned_at)), + etag = '', + sync_source = sqlc.arg(sync_source) +WHERE repo_id = sqlc.arg(repo_id) + AND pr_number = sqlc.arg(pr_number) + AND tombstoned_at IS NULL; + +-- name: TombstonePullRequestFileOwners :execrows +UPDATE pull_request_file_owners +SET tombstoned_at = sqlc.arg(tombstoned_at), + synced_at = sqlc.arg(tombstoned_at), + last_checked_at = GREATEST(last_checked_at, sqlc.arg(tombstoned_at)), + etag = '', + sync_source = sqlc.arg(sync_source) +WHERE repo_id = sqlc.arg(repo_id) + AND pr_number = sqlc.arg(pr_number) + AND tombstoned_at IS NULL; + +-- name: ListCodeOwnerIdentities :many +WITH candidates AS ( + SELECT request.reviewer_kind AS owner_type, + request.reviewer_gh_id AS owner_gh_id, + request.reviewer_node_id AS owner_node_id, + request.reviewer_login AS owner_login, + request.last_checked_at + FROM pull_request_review_requests AS request + WHERE request.repo_id = sqlc.arg(repo_id) + + UNION ALL + + SELECT 'user'::text, NULL::bigint, review.author_node_id, + review.author_login, review.last_checked_at + FROM pull_request_reviews AS review + WHERE review.repo_id = sqlc.arg(repo_id) + AND review.author_kind = 'user' + AND review.author_node_id IS NOT NULL + AND review.author_login IS NOT NULL + + UNION ALL + + SELECT 'user'::text, NULL::bigint, comment.author_node_id, + comment.author_login, comment.last_checked_at + FROM pull_request_comments AS comment + WHERE comment.repo_id = sqlc.arg(repo_id) + AND comment.author_kind = 'user' + AND comment.author_node_id IS NOT NULL + AND comment.author_login IS NOT NULL +) +SELECT DISTINCT ON (owner_type, lower(owner_login)) + owner_type, COALESCE(owner_gh_id, 0)::bigint AS owner_gh_id, + owner_node_id, owner_login +FROM candidates +ORDER BY owner_type, lower(owner_login), + owner_gh_id IS NOT NULL DESC, last_checked_at DESC, + owner_node_id; + -- name: ReplaceCheckRuns :many WITH input AS ( SELECT (element->>'gh_id')::bigint AS gh_id, @@ -1190,6 +1646,7 @@ JOIN repos ON repos.id = pull_requests.repo_id JOIN repo_aliases ON repo_aliases.repo_id = repos.id WHERE repo_aliases.full_name = sqlc.arg(repo_full_name) AND pull_requests.tombstoned_at IS NULL + AND pull_requests.state = 'open' AND ( pull_requests.head_ref = sqlc.arg(branch) OR pull_requests.base_ref = sqlc.arg(branch) diff --git a/db/queries/loadgen.sql b/db/queries/loadgen.sql index b19903a..93e4a32 100644 --- a/db/queries/loadgen.sql +++ b/db/queries/loadgen.sql @@ -139,6 +139,41 @@ WHERE repo.full_name = sqlc.arg(repo_full_name) AND comment.tombstoned_at IS NULL ORDER BY comment.pr_number, comment.node_id; +-- name: ListLoadgenCachedPullRequestChangeSnapshots :many +SELECT snapshot.pr_number, snapshot.base_sha, snapshot.head_sha, + snapshot.files_total_count, snapshot.files_truncated, + snapshot.codeowners_ref, snapshot.codeowners_sha, + snapshot.codeowners_path, snapshot.codeowners_state, + snapshot.codeowners_source, snapshot.codeowners_hash +FROM pull_request_change_snapshots AS snapshot +JOIN repos AS repo ON repo.id = snapshot.repo_id +WHERE repo.full_name = sqlc.arg(repo_full_name) + AND repo.tombstoned_at IS NULL + AND snapshot.tombstoned_at IS NULL +ORDER BY snapshot.pr_number; + +-- name: ListLoadgenCachedPullRequestChangedFiles :many +SELECT file.pr_number, file.path, file.previous_path, file.change_type, + file.base_sha, file.head_sha +FROM pull_request_changed_files AS file +JOIN repos AS repo ON repo.id = file.repo_id +WHERE repo.full_name = sqlc.arg(repo_full_name) + AND repo.tombstoned_at IS NULL + AND file.tombstoned_at IS NULL +ORDER BY file.pr_number, file.path; + +-- name: ListLoadgenCachedPullRequestFileOwners :many +SELECT owner.pr_number, owner.path, owner.owner_token, owner.owner_type, + owner.owner_name, owner.resolution_state, owner.owner_gh_id, + owner.owner_node_id, owner.owner_login, owner.source_pattern, + owner.source_line, owner.base_sha, owner.head_sha +FROM pull_request_file_owners AS owner +JOIN repos AS repo ON repo.id = owner.repo_id +WHERE repo.full_name = sqlc.arg(repo_full_name) + AND repo.tombstoned_at IS NULL + AND owner.tombstoned_at IS NULL +ORDER BY owner.pr_number, owner.path, owner.owner_token; + -- name: ListLoadgenCachedCheckRuns :many SELECT run.gh_id, diff --git a/docs/SYNC_ENGINE.md b/docs/SYNC_ENGINE.md index 3279633..8b1ac34 100644 --- a/docs/SYNC_ENGINE.md +++ b/docs/SYNC_ENGINE.md @@ -66,6 +66,7 @@ floor for everything. | Server-side "Rebase stack" (force-pushes) | ❌ (undocumented) | Branch rewrites are real ref updates: expect `push` per branch and `pull_request.synchronize` per member — treat either on a stack branch as a whole-stack refresh. **Must be verified empirically in Phase 0**; undocumented whether server-generated rebases emit these | Sweep | | Unstack / dissolve | ❌ | Next `pull_request` event on any ex-member arrives with `stack: null` → stack-object diff fires | Sweep | | Trunk moves (dry-run staleness) | ✅ `push` on base ref | Standard event; enqueue dry-run re-evaluation for stacks based on that ref | — | +| Effective CODEOWNERS changes | ✅ `push` on default branch | Exact `.github/CODEOWNERS`, root `CODEOWNERS`, or `docs/CODEOWNERS` path change → one coalesced branch refresh, fanning out to the finite cached set of live open stacked and loose PRs based on that branch (no arbitrary count cutoff) | Reconciliation | | Stack `open`/closed state | ❌ | Derivable from member PR states | Sweep | Consequences baked into the design: @@ -365,7 +366,8 @@ else, so no pipeline stage does per-event work that could be per-batch work. - **C-P3 — Fetch results are written set-at-a-time.** One fetch produces one transaction, however many rows it touches: a checks refresh upserts all check runs for the SHA via a single `unnest`-based upsert (sqlc), review - threads likewise. Never row-per-statement, never statement-per-event. + threads likewise, and changed files plus owners use two fenced JSONB + replace sets. Never row-per-statement, never statement-per-event. - **C-P4 — Due fetches gang into GraphQL batches.** The fetcher may claim up to K (default 25) due entity-refresh jobs at once and satisfy them with one `nodes(ids:)` GraphQL call, then apply results per entity (each under its @@ -472,6 +474,14 @@ pull_request_reviews(node_id PRIMARY KEY, repo_id, pr_number, author_kind, state, submitted_at, commit_oid, gh_updated_at, ...) pull_request_comments(node_id PRIMARY KEY, repo_id, pr_number, author_kind, created_at, gh_updated_at, ...) -- no bodies +pull_request_change_snapshots(repo_id, pr_number, base_sha, head_sha, + files_total_count, files_truncated, + codeowners_ref, codeowners_sha, + codeowners_path, codeowners_state, ...) +pull_request_changed_files(repo_id, pr_number, path, previous_path, + change_type, base_sha, head_sha, ...) +pull_request_file_owners(repo_id, pr_number, path, owner_token, owner_type, + resolution_state, owner_node_id, ...) review_threads(...), check_runs(...), check_history(...) -- Derivation diff --git a/internal/changeinputs/changeinputs.go b/internal/changeinputs/changeinputs.go new file mode 100644 index 0000000..0193189 --- /dev/null +++ b/internal/changeinputs/changeinputs.go @@ -0,0 +1,330 @@ +// Package changeinputs builds the bounded, source-derived ownership inputs +// attached to one exact pull-request base/head observation. +package changeinputs + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "fmt" + "sort" + "strings" + + "github.com/ewhauser/ghsync/internal/budget" + "github.com/ewhauser/ghsync/internal/codeowners" + "github.com/ewhauser/ghsync/internal/gh" + "github.com/ewhauser/ghsync/internal/store" +) + +// Hydrate reads changed-file rename supplements and the effective CODEOWNERS +// source at the observed base SHA. GraphQL supplies the authoritative file +// paths and connection completeness; REST is used only for facts GraphQL does +// not expose and for a final base/head fence check. +func Hydrate( + ctx context.Context, + rest *gh.RESTClient, + writer *store.EntityWriter, + class budget.Class, + repositoryID int64, + repositoryOwner string, + repositoryName string, + number int, + node *gh.PullRequestNode, +) (*store.PullRequestChangeSnapshotRecord, error) { + if rest == nil || writer == nil || node == nil { + return nil, fmt.Errorf("change-input hydration requires clients and PR") + } + snapshot := &store.PullRequestChangeSnapshotRecord{ + BaseSHA: node.BaseRefOID, + HeadSHA: node.HeadRefOID, + FilesTotalCount: node.ChangedFiles, + CodeownersRef: node.BaseRefName, + CodeownersSHA: node.BaseRefOID, + } + if node.Files == nil { + snapshot.FilesTruncated = true + } else { + snapshot.FilesTruncated = node.Files.Truncated + if node.Files.TotalCount != node.ChangedFiles { + snapshot.FilesTruncated = true + snapshot.FilesTotalCount = max( + node.Files.TotalCount, + node.ChangedFiles, + ) + } + for _, file := range node.Files.Nodes { + changeType := strings.ToLower(file.ChangeType) + snapshot.Files = append(snapshot.Files, store.ChangedFileRecord{ + Path: file.Path, + ChangeType: changeType, + }) + } + if snapshot.FilesTotalCount < len(snapshot.Files) { + snapshot.FilesTotalCount = len(snapshot.Files) + snapshot.FilesTruncated = true + } + } + + needsRenames := false + for index := range snapshot.Files { + if snapshot.Files[index].ChangeType == "renamed" { + needsRenames = true + break + } + } + if needsRenames { + renames, truncated, err := rest.PullRequestFileRenames( + ctx, class, repositoryOwner, repositoryName, number, + ) + if err != nil { + return nil, fmt.Errorf("fetch PR rename paths: %w", err) + } + snapshot.FilesTruncated = snapshot.FilesTruncated || truncated + for index := range snapshot.Files { + file := &snapshot.Files[index] + if file.ChangeType != "renamed" { + continue + } + file.PreviousPath = renames[file.Path] + if file.PreviousPath == "" { + // A rename without its source path is an incomplete snapshot, + // even if the GraphQL connection itself was cursor-complete. + snapshot.FilesTruncated = true + } + } + } + + source := gh.CodeownersSource{State: gh.CodeownersUnavailable} + if node.BaseRefOID != "" { + var err error + source, err = rest.FindCodeowners( + ctx, + class, + repositoryOwner, + repositoryName, + node.BaseRefOID, + ) + if err != nil { + return nil, fmt.Errorf("fetch effective CODEOWNERS: %w", err) + } + } + snapshot.CodeownersPath = source.Path + snapshot.CodeownersState = source.State + snapshot.CodeownersSource = source.Content + snapshot.CodeownersHash = sourceHash(source) + if source.State == gh.CodeownersPresent { + rules := codeowners.Parse(source.Content) + for _, file := range snapshot.Files { + match, ok := codeowners.Resolve(rules, file.Path) + if !ok { + continue + } + seenTokens := make(map[string]struct{}, len(match.Owners)) + for _, owner := range match.Owners { + if _, duplicate := seenTokens[owner.Token]; duplicate { + continue + } + seenTokens[owner.Token] = struct{}{} + snapshot.Owners = append(snapshot.Owners, store.FileOwnerRecord{ + Path: file.Path, + OwnerToken: owner.Token, + OwnerType: string(owner.Type), + OwnerName: owner.Name, + ResolutionState: "unresolved", + SourcePattern: match.Pattern, + SourceLine: match.Line, + }) + } + } + } + resolvedOwners, err := writer.ResolveFileOwnerIdentities( + ctx, repositoryID, repositoryOwner, snapshot.Owners, + ) + if err != nil { + return nil, fmt.Errorf("resolve CODEOWNERS identities: %w", err) + } + snapshot.Owners = resolveObservedIdentities( + resolvedOwners, repositoryOwner, node, + ) + + // Deterministic ordering is part of both replace-set comparison and the + // drift snapshot. Do not depend on GitHub, SQL, or map iteration order. + sort.Slice(snapshot.Files, func(i, j int) bool { + return snapshot.Files[i].Path < snapshot.Files[j].Path + }) + sort.Slice(snapshot.Owners, func(i, j int) bool { + if snapshot.Owners[i].Path == snapshot.Owners[j].Path { + return snapshot.Owners[i].OwnerToken < + snapshot.Owners[j].OwnerToken + } + return snapshot.Owners[i].Path < snapshot.Owners[j].Path + }) + + latest, _, err := rest.GetPull( + ctx, + class, + repositoryOwner, + repositoryName, + number, + "", + ) + if err != nil { + return nil, fmt.Errorf("verify PR change-input fence: %w", err) + } + if latest.GetBase().GetSHA() != snapshot.BaseSHA || + latest.GetHead().GetSHA() != snapshot.HeadSHA { + return nil, fmt.Errorf( + "verify PR change-input fence: base/head changed during observation", + ) + } + return snapshot, nil +} + +func resolveObservedIdentities( + owners []store.FileOwnerRecord, + repositoryOwner string, + node *gh.PullRequestNode, +) []store.FileOwnerRecord { + type identity struct { + githubID int64 + nodeID string + login string + } + known := make(map[string]identity) + for _, request := range node.ReviewRequests.Nodes { + reviewer := request.RequestedReviewer + switch reviewer.Typename { + case "User": + if reviewer.ID != "" && reviewer.Login != "" { + known["user\x00"+strings.ToLower(reviewer.Login)] = identity{ + githubID: reviewer.DatabaseID, + nodeID: reviewer.ID, + login: reviewer.Login, + } + } + case "Team": + if reviewer.ID != "" && reviewer.Slug != "" { + known["team\x00"+strings.ToLower(reviewer.Slug)] = identity{ + githubID: reviewer.DatabaseID, + nodeID: reviewer.ID, + login: reviewer.Slug, + } + } + } + } + for _, review := range node.Reviews.Nodes { + if review.Author != nil && review.Author.Typename == "User" && + review.Author.ID != "" && review.Author.Login != "" { + known["user\x00"+strings.ToLower(review.Author.Login)] = identity{ + nodeID: review.Author.ID, login: review.Author.Login, + } + } + } + for _, comment := range node.Comments.Nodes { + if comment.Author != nil && comment.Author.Typename == "User" && + comment.Author.ID != "" && comment.Author.Login != "" { + known["user\x00"+strings.ToLower(comment.Author.Login)] = identity{ + nodeID: comment.Author.ID, login: comment.Author.Login, + } + } + } + resolved := append([]store.FileOwnerRecord(nil), owners...) + for index := range resolved { + owner := &resolved[index] + if owner.ResolutionState != "unresolved" { + continue + } + lookup := owner.OwnerName + switch owner.OwnerType { + case "user": + case "team": + parts := strings.SplitN(owner.OwnerName, "/", 2) + if len(parts) != 2 || + !strings.EqualFold(parts[0], repositoryOwner) { + continue + } + lookup = parts[1] + default: + continue + } + identity, ok := known[owner.OwnerType+"\x00"+ + strings.ToLower(lookup)] + if !ok { + continue + } + owner.ResolutionState = "resolved" + owner.OwnerGitHubID = identity.githubID + owner.OwnerNodeID = identity.nodeID + owner.OwnerLogin = identity.login + } + return resolved +} + +func sourceHash(source gh.CodeownersSource) string { + digest := sha256.Sum256([]byte( + source.State + "\x00" + source.Path + "\x00" + source.Content, + )) + return hex.EncodeToString(digest[:]) +} + +// Semantic returns the null-safe, source-derived value embedded in drift +// snapshots. Source content itself is deliberately represented by its hash; +// consumers read the mirrored source from the public snapshot table. +func Semantic(snapshot *store.PullRequestChangeSnapshotRecord) map[string]any { + files := make([]map[string]any, 0, len(snapshot.Files)) + for _, file := range snapshot.Files { + var previous any + if file.PreviousPath != "" { + previous = file.PreviousPath + } + files = append(files, map[string]any{ + "path": file.Path, + "previous_path": previous, + "change_type": file.ChangeType, + }) + } + owners := make([]map[string]any, 0, len(snapshot.Owners)) + for index := range snapshot.Owners { + owner := &snapshot.Owners[index] + var githubID any + if owner.OwnerGitHubID > 0 { + githubID = owner.OwnerGitHubID + } + var nodeID, login any + if owner.OwnerNodeID != "" { + nodeID = owner.OwnerNodeID + } + if owner.OwnerLogin != "" { + login = owner.OwnerLogin + } + owners = append(owners, map[string]any{ + "path": owner.Path, + "owner_token": owner.OwnerToken, + "owner_type": owner.OwnerType, + "owner_name": owner.OwnerName, + "resolution_state": owner.ResolutionState, + "owner_gh_id": githubID, + "owner_node_id": nodeID, + "owner_login": login, + "source_pattern": owner.SourcePattern, + "source_line": owner.SourceLine, + }) + } + var path any + if snapshot.CodeownersPath != "" { + path = snapshot.CodeownersPath + } + return map[string]any{ + "base_sha": snapshot.BaseSHA, + "head_sha": snapshot.HeadSHA, + "files_total_count": snapshot.FilesTotalCount, + "files_truncated": snapshot.FilesTruncated, + "codeowners_ref": snapshot.CodeownersRef, + "codeowners_sha": snapshot.CodeownersSHA, + "codeowners_path": path, + "codeowners_state": snapshot.CodeownersState, + "codeowners_hash": snapshot.CodeownersHash, + "files": files, + "owners": owners, + } +} diff --git a/internal/codeowners/resolver.go b/internal/codeowners/resolver.go new file mode 100644 index 0000000..4679b01 --- /dev/null +++ b/internal/codeowners/resolver.go @@ -0,0 +1,296 @@ +// Package codeowners parses and resolves CODEOWNERS path rules without +// applying any reviewer-ranking policy. +package codeowners + +import ( + "fmt" + "regexp" + "strings" + "unicode" +) + +// OwnerType is the syntactic identity kind carried by an owner token. +type OwnerType string + +const ( + OwnerUser OwnerType = "user" + OwnerTeam OwnerType = "team" + OwnerEmail OwnerType = "email" + OwnerMalformed OwnerType = "malformed" +) + +// Owner preserves one source token exactly while exposing its syntactic kind +// and normalized lookup name. +type Owner struct { + Token string + Type OwnerType + Name string +} + +// Rule is one valid CODEOWNERS rule. Pattern is the source spelling, including +// escapes; Line is one-based. +type Rule struct { + Pattern string + Line int + Owners []Owner + matcher *regexp.Regexp +} + +// Match is the last rule that matched one repository-relative file path. +type Match struct { + Pattern string + Line int + Owners []Owner +} + +// Parse applies the CODEOWNERS subset of gitignore syntax. Invalid pattern +// lines are skipped, while malformed owner tokens on a valid rule are kept as +// explicit OwnerMalformed facts. +func Parse(source string) []Rule { + lines := strings.Split(strings.ReplaceAll(source, "\r\n", "\n"), "\n") + rules := make([]Rule, 0, len(lines)) + for index, line := range lines { + pattern, tokens, ok := splitRule(line) + if !ok { + continue + } + matcher, err := compilePattern(pattern) + if err != nil { + continue + } + owners := make([]Owner, 0, len(tokens)) + for _, token := range tokens { + owners = append(owners, classifyOwner(token)) + } + rules = append(rules, Rule{ + Pattern: pattern, + Line: index + 1, + Owners: owners, + matcher: matcher, + }) + } + return rules +} + +// Resolve returns the owners from the last matching rule. Paths are matched +// case-sensitively and are interpreted relative to the repository root. +func Resolve(rules []Rule, path string) (Match, bool) { + path = strings.TrimPrefix(path, "/") + var result Match + matched := false + for index := range rules { + rule := &rules[index] + if rule.matcher.MatchString(path) { + result = Match{ + Pattern: rule.Pattern, + Line: rule.Line, + Owners: append([]Owner(nil), rule.Owners...), + } + matched = true + } + } + return result, matched +} + +func splitRule(line string) (string, []string, bool) { + line = strings.TrimRight(line, "\r") + index := 0 + for index < len(line) && isSpace(line[index]) { + index++ + } + if index == len(line) || line[index] == '#' { + return "", nil, false + } + // GitHub explicitly does not support escaping a leading comment marker. + if strings.HasPrefix(line[index:], `\#`) { + return "", nil, false + } + start := index + escaped := false + for index < len(line) { + character := line[index] + if escaped { + escaped = false + index++ + continue + } + if character == '\\' { + escaped = true + index++ + continue + } + if isSpace(character) { + break + } + index++ + } + if escaped { + return "", nil, false + } + pattern := line[start:index] + var tokens []string + for index < len(line) { + for index < len(line) && isSpace(line[index]) { + index++ + } + if index == len(line) || line[index] == '#' { + break + } + start = index + for index < len(line) && !isSpace(line[index]) { + index++ + } + tokens = append(tokens, line[start:index]) + } + return pattern, tokens, pattern != "" +} + +func compilePattern(pattern string) (*regexp.Regexp, error) { + if strings.HasPrefix(pattern, "!") { + return nil, fmt.Errorf("CODEOWNERS negation is unsupported") + } + if hasUnescaped(pattern, '[') || hasUnescaped(pattern, ']') { + return nil, fmt.Errorf("CODEOWNERS character ranges are unsupported") + } + anchored := strings.HasPrefix(pattern, "/") + if anchored { + pattern = strings.TrimPrefix(pattern, "/") + } + directory := strings.HasSuffix(pattern, "/") + if directory { + pattern = strings.TrimSuffix(pattern, "/") + } + if pattern == "" { + return nil, fmt.Errorf("empty CODEOWNERS pattern") + } + hasSlash := strings.Contains(pattern, "/") + var expression strings.Builder + if anchored || hasSlash { + expression.WriteByte('^') + } else { + expression.WriteString(`(?:^|.*/)`) + } + for index := 0; index < len(pattern); { + character := pattern[index] + switch character { + case '\\': + if index+1 >= len(pattern) { + return nil, fmt.Errorf("dangling CODEOWNERS escape") + } + expression.WriteString(regexp.QuoteMeta(pattern[index+1 : index+2])) + index += 2 + case '*': + runEnd := index + for runEnd < len(pattern) && pattern[runEnd] == '*' { + runEnd++ + } + segmentStart := index == 0 || pattern[index-1] == '/' + if runEnd-index == 2 && segmentStart { + switch { + case runEnd == len(pattern): + expression.WriteString(`.*`) + index = runEnd + continue + case pattern[runEnd] == '/': + expression.WriteString(`(?:[^/]+/)*`) + index = runEnd + 1 + continue + } + } + expression.WriteString(`[^/]*`) + index = runEnd + case '?': + expression.WriteString(`[^/]`) + index++ + default: + expression.WriteString(regexp.QuoteMeta(pattern[index : index+1])) + index++ + } + } + // GitHub's documented literal-directory examples omit a trailing slash + // (for example, /apps/github and **/logs) but still apply to files below + // that directory. Wildcard leaf patterns such as docs/* remain limited to + // the path depth they explicitly match. + if directory || !hasUnescapedWildcard(lastPatternComponent(pattern)) { + expression.WriteString(`(?:/.*)?`) + } + expression.WriteByte('$') + matcher, err := regexp.Compile(expression.String()) + if err != nil { + return nil, fmt.Errorf("compile CODEOWNERS pattern: %w", err) + } + return matcher, nil +} + +func lastPatternComponent(pattern string) string { + if index := strings.LastIndex(pattern, "/"); index >= 0 { + return pattern[index+1:] + } + return pattern +} + +func hasUnescapedWildcard(value string) bool { + return hasUnescaped(value, '*') || hasUnescaped(value, '?') +} + +func hasUnescaped(value string, target byte) bool { + escaped := false + for index := 0; index < len(value); index++ { + if escaped { + escaped = false + continue + } + if value[index] == '\\' { + escaped = true + continue + } + if value[index] == target { + return true + } + } + return false +} + +func classifyOwner(token string) Owner { + owner := Owner{Token: token, Type: OwnerMalformed} + if name, ok := strings.CutPrefix(token, "@"); ok { + parts := strings.Split(name, "/") + switch { + case len(parts) == 1 && validOwnerPart(parts[0]): + owner.Type = OwnerUser + owner.Name = parts[0] + case len(parts) == 2 && validOwnerPart(parts[0]) && + validOwnerPart(parts[1]): + owner.Type = OwnerTeam + owner.Name = name + } + return owner + } + if strings.Count(token, "@") == 1 { + parts := strings.SplitN(token, "@", 2) + if parts[0] != "" && parts[1] != "" && + !strings.ContainsAny(token, " /\t") { + owner.Type = OwnerEmail + owner.Name = token + } + } + return owner +} + +func validOwnerPart(value string) bool { + if value == "" { + return false + } + for _, character := range value { + if unicode.IsLetter(character) || unicode.IsDigit(character) || + character == '-' || character == '_' { + continue + } + return false + } + return true +} + +func isSpace(value byte) bool { + return value == ' ' || value == '\t' || value == '\v' || value == '\f' +} diff --git a/internal/codeowners/resolver_test.go b/internal/codeowners/resolver_test.go new file mode 100644 index 0000000..f054b13 --- /dev/null +++ b/internal/codeowners/resolver_test.go @@ -0,0 +1,291 @@ +package codeowners + +import ( + "reflect" + "testing" +) + +// githubDocumentedExample is the sample CODEOWNERS file from GitHub's +// "About code owners" documentation. Keep the rules and ordering intact: the +// repeated /apps block is intentional and exercises whole-rule precedence. +const githubDocumentedExample = "# This is a comment.\n" + + "# Each line is a file pattern followed by one or more owners.\n" + + "\n" + + "# These owners will be the default owners for everything in\n" + + "# the repo. Unless a later match takes precedence,\n" + + "# @global-owner1 and @global-owner2 will be requested for\n" + + "# review when someone opens a pull request.\n" + + "* @global-owner1 @global-owner2\n" + + "\n" + + "# Order is important; the last matching pattern takes the most\n" + + "# precedence. When someone opens a pull request that only\n" + + "# modifies JS files, only @js-owner and not the global\n" + + "# owner(s) will be requested for a review.\n" + + "*.js @js-owner #This is an inline comment.\n" + + "\n" + + "# You can also use email addresses if you prefer. They'll be\n" + + "# used to look up users just like we do for commit author\n" + + "# emails.\n" + + "*.go docs@example.com\n" + + "\n" + + "# Teams can be specified as code owners as well. Teams should\n" + + "# be identified in the format @org/team-name. Teams must have\n" + + "# explicit write access to the repository. In this example,\n" + + "# the octocats team in the octo-org organization owns all .txt files.\n" + + "*.txt @octo-org/octocats\n" + + "\n" + + "# In this example, @doctocat owns any files in the build/logs\n" + + "# directory at the root of the repository and any of its\n" + + "# subdirectories.\n" + + "/build/logs/ @doctocat\n" + + "\n" + + "# The `docs/*` pattern will match files like\n" + + "# `docs/getting-started.md` but not further nested files like\n" + + "# `docs/build-app/troubleshooting.md`.\n" + + "docs/* docs@example.com\n" + + "\n" + + "# In this example, @octocat owns any file in an apps directory\n" + + "# anywhere in your repository.\n" + + "apps/ @octocat\n" + + "\n" + + "# In this example, @doctocat owns any file in the `/docs`\n" + + "# directory in the root of your repository and any of its\n" + + "# subdirectories.\n" + + "/docs/ @doctocat\n" + + "\n" + + "# In this example, any change inside the `/scripts` directory\n" + + "# will require approval from @doctocat or @octocat.\n" + + "/scripts/ @doctocat @octocat\n" + + "\n" + + "# In this example, @octocat owns any file in a `/logs` directory such as\n" + + "# `/build/logs`, `/scripts/logs`, and `/deeply/nested/logs`. Any changes\n" + + "# in a `/logs` directory will require approval from @octocat.\n" + + "**/logs @octocat\n" + + "\n" + + "# In this example, @octocat owns any file in the `/apps`\n" + + "# directory in the root of your repository except for the `/apps/github`\n" + + "# subdirectory, as its owners are left empty. Without an owner, changes\n" + + "# to `apps/github` can be made with the approval of any user who has\n" + + "# write access to the repository.\n" + + "/apps/ @octocat\n" + + "/apps/github\n" + + "\n" + + "# In this example, @octocat owns any file in the `/apps`\n" + + "# directory in the root of your repository except for the `/apps/github`\n" + + "# subdirectory, as this subdirectory has its own owner @doctocat\n" + + "/apps/ @octocat\n" + + "/apps/github @doctocat\n" + +func TestGitHubDocumentedCODEOWNERSExample(t *testing.T) { + t.Parallel() + rules := Parse(githubDocumentedExample) + tests := []struct { + path string + pattern string + owners []string + }{ + {"README.md", "*", []string{"@global-owner1", "@global-owner2"}}, + {"web/app.js", "*.js", []string{"@js-owner"}}, + {"pkg/tool.go", "*.go", []string{"docs@example.com"}}, + {"notes.txt", "*.txt", []string{"@octo-org/octocats"}}, + {"build/logs/output.log", "**/logs", []string{"@octocat"}}, + {"docs/getting-started.md", "/docs/", []string{"@doctocat"}}, + {"deep/apps/main.rb", "apps/", []string{"@octocat"}}, + {"scripts/release.sh", "/scripts/", []string{"@doctocat", "@octocat"}}, + {"deeply/nested/logs/output.log", "**/logs", []string{"@octocat"}}, + {"apps/github/api.go", "/apps/github", []string{"@doctocat"}}, + } + for _, test := range tests { + t.Run(test.path, func(t *testing.T) { + t.Parallel() + match, ok := Resolve(rules, test.path) + if !ok { + t.Fatal("path did not match") + } + owners := make([]string, 0, len(match.Owners)) + for _, owner := range match.Owners { + owners = append(owners, owner.Token) + } + if match.Pattern != test.pattern || + !reflect.DeepEqual(owners, test.owners) { + t.Fatalf( + "match = %q %v, want %q %v", + match.Pattern, owners, test.pattern, test.owners, + ) + } + }) + } +} + +func TestResolveCODEOWNERSLastMatchAndWildcards(t *testing.T) { + t.Parallel() + rules := Parse(`# global +* @global +*.go @go-owner +/cmd/** @cli +internal/* @direct +internal/**/generated/*.go @generator +docs/ @docs +docs/My\ File/** @spaces +`) + tests := []struct { + path string + pattern string + owners []string + }{ + {"README.md", "*", []string{"@global"}}, + {"pkg/cache/store.go", "*.go", []string{"@go-owner"}}, + {"cmd/ghsyncd/main.go", "/cmd/**", []string{"@cli"}}, + {"internal/top.go", "internal/*", []string{"@direct"}}, + {"internal/deep/top.go", "*.go", []string{"@go-owner"}}, + {"internal/a/generated/table.go", "internal/**/generated/*.go", []string{"@generator"}}, + {"internal/generated/table.go", "internal/**/generated/*.go", []string{"@generator"}}, + {"docs/nested/guide.md", "docs/", []string{"@docs"}}, + {"docs/My File/api/index.md", "docs/My\\ File/**", []string{"@spaces"}}, + } + for _, test := range tests { + t.Run(test.path, func(t *testing.T) { + t.Parallel() + match, ok := Resolve(rules, test.path) + if !ok { + t.Fatal("path did not match") + } + owners := make([]string, 0, len(match.Owners)) + for _, owner := range match.Owners { + owners = append(owners, owner.Token) + } + if match.Pattern != test.pattern || !reflect.DeepEqual(owners, test.owners) { + t.Fatalf("match = %q %v, want %q %v", match.Pattern, owners, test.pattern, test.owners) + } + }) + } +} + +func TestParseCODEOWNERSCommentsUnsupportedNegationAndOwnerKinds(t *testing.T) { + t.Parallel() + rules := Parse(`# comment +\#not-a-pattern @ignored +!secret/** @negated +lib/[ab].go @range +*.js @octocat @acme/frontend bad-token docs@example.com # inline comment +*.txt @later +*.txt @final +`) + if len(rules) != 3 { + t.Fatalf("rules = %#v, want three valid rules", rules) + } + match, ok := Resolve(rules, "web/app.js") + if !ok || match.Line != 5 { + t.Fatalf("JavaScript match = %#v, %v", match, ok) + } + want := []Owner{ + {Token: "@octocat", Type: OwnerUser, Name: "octocat"}, + {Token: "@acme/frontend", Type: OwnerTeam, Name: "acme/frontend"}, + {Token: "bad-token", Type: OwnerMalformed}, + {Token: "docs@example.com", Type: OwnerEmail, Name: "docs@example.com"}, + } + if !reflect.DeepEqual(match.Owners, want) { + t.Fatalf("owners = %#v, want %#v", match.Owners, want) + } + match, ok = Resolve(rules, "notes.txt") + if !ok || match.Line != 7 || match.Owners[0].Token != "@final" { + t.Fatalf("last-match result = %#v, %v", match, ok) + } +} + +func TestCODEOWNERSStarDoesNotCrossDirectory(t *testing.T) { + t.Parallel() + rules := Parse("docs/* @direct\ndocs/** @recursive\n") + match, ok := Resolve(rules[:1], "docs/api/index.md") + if ok { + t.Fatalf("single star unexpectedly matched %#v", match) + } + match, ok = Resolve(rules, "docs/api/index.md") + if !ok || match.Owners[0].Token != "@recursive" { + t.Fatalf("double star match = %#v, %v", match, ok) + } +} + +func TestCODEOWNERSSharpEdges(t *testing.T) { + t.Parallel() + tests := []struct { + name string + source string + path string + matched bool + pattern string + owners []string + }{ + { + name: "slashless directory matches anywhere", + source: "apps/ @anywhere\n", path: "nested/apps/file.go", + matched: true, pattern: "apps/", owners: []string{"@anywhere"}, + }, + { + name: "leading slash anchors to root", + source: "/apps/ @root\n", path: "nested/apps/file.go", + }, + { + name: "single star does not cross slash", + source: "docs/* @direct\n", path: "docs/api/index.md", + }, + { + name: "double star crosses slash", + source: "docs/** @recursive\n", path: "docs/api/index.md", + matched: true, pattern: "docs/**", owners: []string{"@recursive"}, + }, + { + name: "literal directory without slash owns descendants", + source: "/apps/github @github\n", path: "apps/github/api/main.go", + matched: true, pattern: "/apps/github", owners: []string{"@github"}, + }, + { + name: "later ownerless rule clears ownership", + source: "/apps/ @apps\n/apps/github\n", path: "apps/github/api.go", + matched: true, pattern: "/apps/github", owners: []string{}, + }, + { + name: "paths are case sensitive", + source: "Docs/ @docs\n", path: "docs/guide.md", + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + match, ok := Resolve(Parse(test.source), test.path) + if ok != test.matched { + t.Fatalf("match = %#v, %v", match, ok) + } + if !ok { + return + } + owners := make([]string, 0, len(match.Owners)) + for _, owner := range match.Owners { + owners = append(owners, owner.Token) + } + if match.Pattern != test.pattern || + !reflect.DeepEqual(owners, test.owners) { + t.Fatalf( + "match = %q %v, want %q %v", + match.Pattern, owners, test.pattern, test.owners, + ) + } + }) + } +} + +func TestCODEOWNERSCRLFEmailAndOwnerTokenCase(t *testing.T) { + t.Parallel() + rules := Parse("*.go Docs@Example.com @OctoCat\r\n") + match, ok := Resolve(rules, "pkg/main.go") + if !ok { + t.Fatal("CRLF rule did not match") + } + want := []Owner{ + {Token: "Docs@Example.com", Type: OwnerEmail, Name: "Docs@Example.com"}, + {Token: "@OctoCat", Type: OwnerUser, Name: "OctoCat"}, + } + if !reflect.DeepEqual(match.Owners, want) { + t.Fatalf("owners = %#v, want %#v", match.Owners, want) + } +} diff --git a/internal/dispatch/classify.go b/internal/dispatch/classify.go index 8ac4a53..bd26ff5 100644 --- a/internal/dispatch/classify.go +++ b/internal/dispatch/classify.go @@ -10,6 +10,7 @@ import ( "io" "net/http" "os" + "slices" "strconv" "strings" @@ -38,14 +39,19 @@ const ( TargetStack Target = "stack" TargetChecks Target = "checks" TargetBranch Target = "branch" + // TargetCodeowners routes only a default-branch push that touched one of + // GitHub's three effective CODEOWNERS locations. It intentionally reuses + // refresh_branch so ordinary push and source-change hints coalesce. + TargetCodeowners Target = "codeowners" // TargetResolveStackMembership carries only the PR key. Its M3 worker // consults cached membership and refreshes both the old and new stacks. TargetResolveStackMembership Target = "resolve_stack_membership" ) // Rule is the config-driven event/action → refresh mapping. StackedTarget -// escalates a pull request event when its payload carries the stack preview -// object (SYNC_ENGINE §2.1). +// adds stack maintenance when a pull request payload carries the stack preview +// object (SYNC_ENGINE §2.1); the direct PR refresh remains authoritative for +// PR-scoped connections such as changed files. type Rule struct { Event string `json:"event" yaml:"event"` Action string `json:"action" yaml:"action"` @@ -96,6 +102,7 @@ func DefaultRules() []Rule { Target: TargetBranch, StackedTarget: TargetStack, }, + {Event: "push", Action: ActionAny, Target: TargetCodeowners}, } } @@ -189,7 +196,7 @@ func validateRule(rule Rule) error { rule.Target, ) } - case TargetBranch: + case TargetBranch, TargetCodeowners: if rule.Event != "push" { return fmt.Errorf("target %q requires event push", rule.Target) } @@ -230,6 +237,7 @@ func validTarget(target Target) bool { TargetStack, TargetChecks, TargetBranch, + TargetCodeowners, TargetResolveStackMembership: return true default: @@ -242,8 +250,19 @@ type payloadEnvelope struct { Number int `json:"number"` Ref string `json:"ref"` Repository struct { - FullName string `json:"full_name"` + FullName string `json:"full_name"` + DefaultBranch string `json:"default_branch"` } `json:"repository"` + Commits []struct { + Added []string `json:"added"` + Modified []string `json:"modified"` + Removed []string `json:"removed"` + } `json:"commits"` + HeadCommit *struct { + Added []string `json:"added"` + Modified []string `json:"modified"` + Removed []string `json:"removed"` + } `json:"head_commit"` PullRequest struct { Number int `json:"number"` Stack *payloadStackRef `json:"stack"` @@ -348,31 +367,34 @@ func (c Classifier) classifyContent( continue } matchedRules++ - target := rule.Target - if rule.StackedTarget != "" && payloadStack(&payload) != nil { - target = rule.StackedTarget - } - key, emit, err := intentKey(target, event, &payload) - if err != nil { - return classification{}, err - } - if !emit { - continue + targets := []Target{rule.Target} + if rule.StackedTarget != "" && payloadStack(&payload) != nil && + rule.Target != TargetBranch { + targets = append(targets, rule.StackedTarget) } - kind, err := jobKind(target) - if err != nil { - return classification{}, err - } - identity := kind + "\x00" + key - if _, duplicate := seen[identity]; duplicate { - continue + for _, target := range targets { + key, emit, err := intentKey(target, event, &payload) + if err != nil { + return classification{}, err + } + if !emit { + continue + } + kind, err := jobKind(target) + if err != nil { + return classification{}, err + } + identity := kind + "\x00" + key + if _, duplicate := seen[identity]; duplicate { + continue + } + seen[identity] = struct{}{} + intents = append(intents, Intent{ + Kind: kind, + Key: key, + Priority: PriorityEvent, + }) } - seen[identity] = struct{}{} - intents = append(intents, Intent{ - Kind: kind, - Key: key, - Priority: PriorityEvent, - }) } result := classification{ intents: intents, @@ -466,6 +488,21 @@ func intentKey( return "", false, fmt.Errorf("push payload has an empty branch ref") } return "branch:" + repo + ":" + branch, true, nil + case TargetCodeowners: + const branchPrefix = "refs/heads/" + if !strings.HasPrefix(payload.Ref, branchPrefix) { + return "", false, nil + } + branch := strings.TrimPrefix(payload.Ref, branchPrefix) + if branch == "" { + return "", false, fmt.Errorf("push payload has an empty branch ref") + } + if payload.Repository.DefaultBranch == "" || + branch != payload.Repository.DefaultBranch || + !pushTouchesCodeowners(payload) { + return "", false, nil + } + return "branch:" + repo + ":" + branch, true, nil default: return "", false, fmt.Errorf("unsupported dispatch target %q", target) } @@ -479,7 +516,7 @@ func jobKind(target Target) (string, error) { return queue.KindRefreshStack, nil case TargetChecks: return queue.KindRefreshChecks, nil - case TargetBranch: + case TargetBranch, TargetCodeowners: return queue.KindRefreshBranch, nil case TargetResolveStackMembership: return queue.KindResolveStackMembership, nil @@ -488,6 +525,38 @@ func jobKind(target Target) (string, error) { } } +func pushTouchesCodeowners(payload *payloadEnvelope) bool { + isCodeowners := func(path string) bool { + switch path { + case ".github/CODEOWNERS", "CODEOWNERS", "docs/CODEOWNERS": + return true + default: + return false + } + } + for _, commit := range payload.Commits { + for _, paths := range [][]string{ + commit.Added, commit.Modified, commit.Removed, + } { + if slices.ContainsFunc(paths, isCodeowners) { + return true + } + } + } + if payload.HeadCommit != nil { + for _, paths := range [][]string{ + payload.HeadCommit.Added, + payload.HeadCommit.Modified, + payload.HeadCommit.Removed, + } { + if slices.ContainsFunc(paths, isCodeowners) { + return true + } + } + } + return false +} + type stackPointer struct { Number int } diff --git a/internal/dispatch/classify_test.go b/internal/dispatch/classify_test.go index 144dba3..530056a 100644 --- a/internal/dispatch/classify_test.go +++ b/internal/dispatch/classify_test.go @@ -52,6 +52,10 @@ func TestDefaultClassifierHintCoverage(t *testing.T) { "pull_request":{"number":4812,"stack":{"number":142}} }`, want: []Intent{ + { + Kind: queue.KindRefreshPR, Key: "pr:acme/monolith:4812", + Priority: PriorityEvent, + }, { Kind: queue.KindRefreshStack, Key: "stack:acme/monolith:142", Priority: PriorityEvent, @@ -72,6 +76,11 @@ func TestDefaultClassifierHintCoverage(t *testing.T) { "pull_request":{"number":4815,"stack":{"number":142}} }`, want: []Intent{ + { + Kind: queue.KindRefreshPR, + Key: "pr:acme/monolith:4815", + Priority: PriorityEvent, + }, { Kind: queue.KindRefreshStack, Key: "stack:acme/monolith:142", Priority: PriorityEvent, @@ -218,10 +227,13 @@ func TestDefaultClassifierHintCoverage(t *testing.T) { "repository":{"full_name":"acme/monolith"}, "stack":{"number":142} }`, - want: []Intent{{ - Kind: queue.KindRefreshStack, Key: "stack:acme/monolith:142", - Priority: PriorityEvent, - }}, + want: []Intent{ + { + Kind: queue.KindRefreshBranch, + Key: "branch:acme/monolith:refactor/bm25f-ranker", + Priority: PriorityEvent, + }, + }, }, { name: "tag push is not a branch hint", @@ -247,6 +259,51 @@ func TestDefaultClassifierHintCoverage(t *testing.T) { } } +func TestCodeownersPushRuleTargetsOnlyEffectiveDefaultBranchPaths( + t *testing.T, +) { + t.Parallel() + classifier := NewClassifier([]Rule{{ + Event: "push", Action: ActionAny, Target: TargetCodeowners, + }}) + tests := []struct { + name string + body string + want bool + }{ + { + name: "modified effective path", + body: `{"ref":"refs/heads/main","repository":{"full_name":"acme/monolith","default_branch":"main"},"commits":[{"modified":[".github/CODEOWNERS"]}]}`, + want: true, + }, + { + name: "removed fallback path", + body: `{"ref":"refs/heads/main","repository":{"full_name":"acme/monolith","default_branch":"main"},"head_commit":{"removed":["docs/CODEOWNERS"]}}`, + want: true, + }, + { + name: "non default branch", + body: `{"ref":"refs/heads/topic","repository":{"full_name":"acme/monolith","default_branch":"main"},"commits":[{"modified":["CODEOWNERS"]}]}`, + }, + { + name: "unrelated default branch file", + body: `{"ref":"refs/heads/main","repository":{"full_name":"acme/monolith","default_branch":"main"},"commits":[{"modified":["src/CODEOWNERS"]}]}`, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + intents, err := classifier.Classify("push", []byte(test.body)) + if err != nil { + t.Fatal(err) + } + if (len(intents) == 1) != test.want { + t.Fatalf("intents = %#v, want emitted=%v", intents, test.want) + } + }) + } +} + func TestLoadRulesFileFailsClosedOnSchemaAndSemanticErrors(t *testing.T) { t.Parallel() tests := map[string]string{ @@ -396,6 +453,10 @@ func TestUnknownStackBaseSHARetainsEagerStackFetch(t *testing.T) { t.Fatalf("unknown SHA produced suppression hint: %+v", result.stackHint) } want := []Intent{ + { + Kind: queue.KindRefreshPR, Key: "pr:acme/monolith:72787", + Priority: PriorityEvent, + }, { Kind: queue.KindRefreshStack, Key: "stack:acme/monolith:72787", Priority: PriorityEvent, diff --git a/internal/dispatch/dispatcher_db_test.go b/internal/dispatch/dispatcher_db_test.go index 6d3cc16..ae28928 100644 --- a/internal/dispatch/dispatcher_db_test.go +++ b/internal/dispatch/dispatcher_db_test.go @@ -354,7 +354,6 @@ func TestRebaseStormEscalatesStackBranchesWithoutSlidingDebounce(t *testing.T) { fake := fakegithub.New(fakegithub.DefaultFixture(), testWebhookSecret) repo := "acme/rebase-storm" branches := []string{"stack/layer-1", "stack/layer-2", "stack/layer-3"} - stackKey := "stack:" + repo + ":142" for index := range 20 { branch := branches[index%len(branches)] @@ -396,57 +395,57 @@ func TestRebaseStormEscalatesStackBranchesWithoutSlidingDebounce(t *testing.T) { t.Fatalf("simulated storm duration = %s, want 10s", elapsed) } - var key string - var jobCount int - var scheduledAt time.Time - err = pool.QueryRow(context.Background(), ` - SELECT args->>'key', count(*) OVER (), scheduled_at + rows, err := pool.Query(context.Background(), ` + SELECT args->>'key', scheduled_at FROM river_job WHERE args->>'key' LIKE $1 - `, "%:"+repo+":%").Scan(&key, &jobCount, &scheduledAt) + ORDER BY args->>'key' + `, "%:"+repo+":%") if err != nil { t.Fatal(err) } - if key != stackKey || jobCount != 1 { - t.Fatalf("storm jobs = key %q count %d, want %q count 1", key, jobCount, stackKey) - } + defer rows.Close() first := time.Date(2026, 7, 28, 20, 0, 0, 0, time.UTC) - wantScheduled := first.Add(5 * time.Second) - if !scheduledAt.Equal(wantScheduled) { - t.Fatalf("%s scheduled at %s, want %s", key, scheduledAt, wantScheduled) - } - var generation int64 - if err := pool.QueryRow(context.Background(), ` - SELECT generation - FROM refresh_intent_generations - WHERE kind = $1 AND refresh_key = $2 - `, queue.KindRefreshStack, stackKey).Scan(&generation); err != nil { - t.Fatal(err) + want := map[string]struct { + generation int64 + scheduled time.Time + }{ + "branch:" + repo + ":stack/layer-1": {7, first.Add(5 * time.Second)}, + "branch:" + repo + ":stack/layer-2": {7, first.Add(5500 * time.Millisecond)}, + "branch:" + repo + ":stack/layer-3": {6, first.Add(6 * time.Second)}, } - if generation != 20 { - t.Fatalf("storm generation = %d, want 20 exact dispatch signals", generation) + gotJobs := 0 + for rows.Next() { + var key string + var scheduledAt time.Time + if err := rows.Scan(&key, &scheduledAt); err != nil { + t.Fatal(err) + } + expected, ok := want[key] + if !ok { + t.Fatalf("unexpected storm job %q", key) + } + if !scheduledAt.Equal(expected.scheduled) { + t.Fatalf("%s scheduled at %s, want %s", key, scheduledAt, expected.scheduled) + } + var generation int64 + if err := pool.QueryRow(context.Background(), ` + SELECT generation + FROM refresh_intent_generations + WHERE kind = $1 AND refresh_key = $2 + `, queue.KindRefreshBranch, key).Scan(&generation); err != nil { + t.Fatal(err) + } + if generation != expected.generation { + t.Fatalf("%s generation = %d, want %d", key, generation, expected.generation) + } + gotJobs++ } - var eventReceivedAt, firstReceivedAt time.Time - if err := pool.QueryRow(context.Background(), ` - SELECT generations.event_received_at, min(deliveries.received_at) - FROM refresh_intent_generations AS generations - CROSS JOIN webhook_deliveries AS deliveries - WHERE generations.kind = $1 - AND generations.refresh_key = $2 - AND deliveries.delivery_guid LIKE 'storm-%' - GROUP BY generations.event_received_at - `, queue.KindRefreshStack, stackKey).Scan( - &eventReceivedAt, - &firstReceivedAt, - ); err != nil { + if err := rows.Err(); err != nil { t.Fatal(err) } - if !eventReceivedAt.Equal(firstReceivedAt) { - t.Fatalf( - "event SLO origin = %s, want earliest delivery %s", - eventReceivedAt, - firstReceivedAt, - ) + if gotJobs != len(want) { + t.Fatalf("storm jobs = %d, want %d branch-coalesced jobs", gotJobs, len(want)) } } diff --git a/internal/dispatch/testdata/s142_expected_jobs.json b/internal/dispatch/testdata/s142_expected_jobs.json index 4010889..a827e3c 100644 --- a/internal/dispatch/testdata/s142_expected_jobs.json +++ b/internal/dispatch/testdata/s142_expected_jobs.json @@ -1,5 +1,10 @@ [ {"kind":"refresh_pr","key":"pr:acme/monolith:4800","priority":"event"}, + {"kind":"refresh_pr","key":"pr:acme/monolith:4810","priority":"event"}, + {"kind":"refresh_pr","key":"pr:acme/monolith:4812","priority":"event"}, + {"kind":"refresh_pr","key":"pr:acme/monolith:4815","priority":"event"}, + {"kind":"refresh_pr","key":"pr:acme/monolith:4816","priority":"event"}, + {"kind":"refresh_pr","key":"pr:acme/monolith:4820","priority":"event"}, {"kind":"refresh_pr","key":"pr:acme/monolith:4830","priority":"event"}, {"kind":"refresh_pr","key":"pr:acme/monolith:4831","priority":"event"}, {"kind":"refresh_pr","key":"pr:acme/monolith:4832","priority":"event"}, @@ -10,6 +15,11 @@ {"kind":"resolve_stack_membership","key":"pr:acme/monolith:4816","priority":"event"}, {"kind":"resolve_stack_membership","key":"pr:acme/monolith:4820","priority":"event"}, {"kind":"refresh_stack","key":"stack:acme/monolith:142","priority":"event"}, + {"kind":"refresh_branch","key":"branch:acme/monolith:refactor/tokenizer","priority":"event"}, + {"kind":"refresh_branch","key":"branch:acme/monolith:refactor/bm25f-ranker","priority":"event"}, + {"kind":"refresh_branch","key":"branch:acme/monolith:feat/relevance-debug","priority":"event"}, + {"kind":"refresh_branch","key":"branch:acme/monolith:feat/results-rewire","priority":"event"}, + {"kind":"refresh_branch","key":"branch:acme/monolith:feat/relevance-telemetry","priority":"event"}, {"kind":"refresh_checks","key":"checks:acme/monolith:8f31c2d","priority":"event"}, {"kind":"refresh_checks","key":"checks:acme/monolith:bbbb001","priority":"event"}, {"kind":"refresh_checks","key":"checks:acme/monolith:bbbb003","priority":"event"}, diff --git a/internal/drift/drift.go b/internal/drift/drift.go index e81dafa..d70d84f 100644 --- a/internal/drift/drift.go +++ b/internal/drift/drift.go @@ -21,6 +21,7 @@ import ( "github.com/riverqueue/river" "github.com/ewhauser/ghsync/internal/budget" + "github.com/ewhauser/ghsync/internal/changeinputs" "github.com/ewhauser/ghsync/internal/gh" "github.com/ewhauser/ghsync/internal/observer" "github.com/ewhauser/ghsync/internal/opsstate" @@ -870,6 +871,29 @@ func (s *Service) fullFetch( if len(nodes) != 1 || nodes[0] == nil { return tombstoneSnapshot(), spec, nil } + if nodes[0].BaseRefOID != pull.GetBase().GetSHA() || + nodes[0].HeadRefOID != pull.GetHead().GetSHA() { + return nil, spec, fmt.Errorf( + "drift fetch pull request %s: base/head changed during observation", + key, + ) + } + changeSnapshot, err := changeinputs.Hydrate( + ctx, + s.rest, + s.writer, + budget.Sweep, + nodes[0].Repository.DatabaseID, + owner, + name, + number, + nodes[0], + ) + if err != nil { + return nil, spec, fmt.Errorf( + "drift fetch PR change inputs %s: %w", key, err, + ) + } return encodeSnapshot(map[string]any{ "id": pull.GetID(), "node_id": pull.GetNodeID(), @@ -889,6 +913,7 @@ func (s *Service) fullFetch( "review_requests": semanticReviewRequests(pull), "reviews": semanticPullRequestReviews(nodes[0]), "comments": semanticPullRequestComments(nodes[0]), + "change_inputs": changeinputs.Semantic(changeSnapshot), }), spec, nil case "stack": repo, number, err := numberedKey(key, "stack:") @@ -1358,6 +1383,12 @@ func semanticIdentity(item map[string]any) string { if id, ok := item["id"].(string); ok && id != "" { return "string:" + id } + if path, ok := item["path"].(string); ok && path != "" { + if token, ok := item["owner_token"].(string); ok && token != "" { + return "owner:" + path + "\x00" + token + } + return "path:" + path + } return "" } diff --git a/internal/drift/drift_db_test.go b/internal/drift/drift_db_test.go index 6980906..03858e1 100644 --- a/internal/drift/drift_db_test.go +++ b/internal/drift/drift_db_test.go @@ -2,6 +2,7 @@ package drift import ( "context" + "fmt" "net/http/httptest" "reflect" "strings" @@ -51,6 +52,7 @@ type driftHarness struct { fake *fakegithub.Server fixture fakegithub.Fixture service *Service + handler *fetch.Handler riverClient *river.Client[pgx.Tx] } @@ -189,6 +191,20 @@ func newReadyDriftHarness(t *testing.T) *driftHarness { t.Fatal(err) } } + for index := range fixture.PullRequests { + pull := &fixture.PullRequests[index] + if err := handler.RefreshPR( + ctx, + queue.RefreshRequest{ + Args: queue.NewRefreshPRArgs( + fmt.Sprintf("pr:acme/monolith:%d", pull.Number), + ).RefreshArgs, + Queue: queue.QueueSweep, + }, + ); err != nil { + t.Fatal(err) + } + } waitForCacheProducers(t, pool) if _, err := pool.Exec(ctx, ` INSERT INTO installation_backfill_cursors ( @@ -203,6 +219,7 @@ func newReadyDriftHarness(t *testing.T) *driftHarness { fake: fake, fixture: fixture, service: service, + handler: handler, riverClient: riverClient, } } @@ -242,6 +259,14 @@ func TestDriftTreatsUnknownBaseSHAAsConvergedTruth(t *testing.T) { harness.fixture.PullRequests[1].Stack.Base.SHA = "" harness.fixture.Stacks[0].Base.SHA = "" harness.fake.SetFixture(harness.fixture) + if err := harness.handler.RefreshPR(ctx, queue.RefreshRequest{ + Args: queue.NewRefreshPRArgs( + "pr:acme/monolith:4812", + ).RefreshArgs, + Queue: queue.QueueSweep, + }); err != nil { + t.Fatal(err) + } findings, err := harness.service.Detect(ctx, DetectArgs{ InstallationID: 1, @@ -401,6 +426,146 @@ func TestDriftDetectsAndHealsParticipationDivergence(t *testing.T) { } } +func TestDriftDetectsAndHealsChangeInputDivergence(t *testing.T) { + t.Parallel() + harness := newReadyDriftHarness(t) + ctx := t.Context() + if _, err := harness.pool.Exec(ctx, ` + UPDATE pull_request_change_snapshots AS snapshot + SET files_truncated = true, + codeowners_hash = 'corrupt-source-hash' + FROM repos + WHERE repos.id = snapshot.repo_id + AND repos.full_name = 'acme/monolith' + AND snapshot.pr_number = 4812; + + UPDATE pull_request_changed_files AS file + SET change_type = 'added' + FROM repos + WHERE repos.id = file.repo_id + AND repos.full_name = 'acme/monolith' + AND file.pr_number = 4812 + AND file.path = 'internal/ranker.go'; + + UPDATE pull_request_file_owners AS owner + SET resolution_state = 'unresolved', owner_gh_id = NULL, + owner_node_id = NULL, owner_login = NULL + FROM repos + WHERE repos.id = owner.repo_id + AND repos.full_name = 'acme/monolith' + AND owner.pr_number = 4812 + AND owner.owner_token = '@acme/search-platform' + `); err != nil { + t.Fatal(err) + } + findings, err := harness.service.Detect(ctx, DetectArgs{ + InstallationID: 1, + SampleSize: 100, + }) + if err != nil { + t.Fatal(err) + } + if len(findings) != 1 || + findings[0].EntityKey != "pr:acme/monolith:4812" || + !strings.Contains(string(findings[0].Diff), "change_inputs") { + t.Fatalf("change-input drift findings = %+v", findings) + } + waitForCacheProducers(t, harness.pool) + var changeType, resolution, nodeID, codeownersHash string + var truncated bool + if err := harness.pool.QueryRow(ctx, ` + SELECT file.change_type, owner.resolution_state, owner.owner_node_id, + snapshot.files_truncated, snapshot.codeowners_hash + FROM pull_request_changed_files AS file + JOIN pull_request_file_owners AS owner + ON owner.repo_id = file.repo_id + AND owner.pr_number = file.pr_number + AND owner.path = file.path + JOIN pull_request_change_snapshots AS snapshot + ON snapshot.repo_id = file.repo_id + AND snapshot.pr_number = file.pr_number + WHERE file.pr_number = 4812 + AND file.path = 'internal/ranker.go' + AND owner.owner_token = '@acme/search-platform' + AND file.tombstoned_at IS NULL + AND owner.tombstoned_at IS NULL + `).Scan( + &changeType, &resolution, &nodeID, &truncated, &codeownersHash, + ); err != nil { + t.Fatal(err) + } + if changeType != "modified" || resolution != "resolved" || + nodeID != "T_kwDOABCDEF6001" || truncated || + codeownersHash == "corrupt-source-hash" { + t.Fatalf( + "healed change inputs = %q/%q/%q truncated=%v hash=%q", + changeType, resolution, nodeID, truncated, codeownersHash, + ) + } + if findings, err := harness.service.Detect(ctx, DetectArgs{ + InstallationID: 1, + SampleSize: 100, + }); err != nil { + t.Fatal(err) + } else if len(findings) != 0 { + t.Fatalf("post-heal change-input findings = %+v", findings) + } +} + +func TestDriftTreatsTruncatedChangeSnapshotAsConvergedTruth(t *testing.T) { + t.Parallel() + harness := newReadyDriftHarness(t) + ctx := t.Context() + pull := &harness.fixture.PullRequests[1] + pull.ChangedFiles = make([]fakegithub.ChangedFile, 101) + for index := range pull.ChangedFiles { + pull.ChangedFiles[index] = fakegithub.ChangedFile{ + Path: fmt.Sprintf("src/truncated-%03d.go", index), + ChangeType: "modified", + } + } + pull.ChangedFilesTotal = 102 + harness.fake.SetFixture(harness.fixture) + if err := harness.handler.RefreshPR(ctx, queue.RefreshRequest{ + Args: queue.NewRefreshPRArgs( + "pr:acme/monolith:4812", + ).RefreshArgs, + Queue: queue.QueueSweep, + }); err != nil { + t.Fatal(err) + } + var total, files int + var truncated bool + if err := harness.pool.QueryRow(ctx, ` + SELECT snapshot.files_total_count, snapshot.files_truncated, + count(file.path) + FROM pull_request_change_snapshots AS snapshot + LEFT JOIN pull_request_changed_files AS file + ON file.repo_id = snapshot.repo_id + AND file.pr_number = snapshot.pr_number + AND file.tombstoned_at IS NULL + WHERE snapshot.pr_number = 4812 + AND snapshot.tombstoned_at IS NULL + GROUP BY snapshot.files_total_count, snapshot.files_truncated + `).Scan(&total, &truncated, &files); err != nil { + t.Fatal(err) + } + if total != 102 || !truncated || files != 101 { + t.Fatalf( + "truncated snapshot total=%d truncated=%v files=%d", + total, truncated, files, + ) + } + if findings, err := harness.service.Detect(ctx, DetectArgs{ + InstallationID: 1, + SampleSize: 100, + }); err != nil { + t.Fatal(err) + } else if len(findings) != 0 { + t.Fatalf("truncated truth produced drift loop: %+v", findings) + } +} + func TestDetectSamplesWithUnrelatedBusySweepQueue(t *testing.T) { t.Parallel() harness := newReadyDriftHarness(t) @@ -766,6 +931,23 @@ func TestStackDriftIgnoresMemberUpdatedAtChurn(t *testing.T) { ); err != nil { t.Fatal(err) } + // Seed every PR-scoped connection before asserting that stack-only + // updated_at churn is ignored. Missing change-input snapshots are genuine + // drift now, not part of the unrelated-field tolerance under test. + for index := range fixture.PullRequests { + pull := &fixture.PullRequests[index] + if err := handler.RefreshPR( + ctx, + queue.RefreshRequest{ + Args: queue.NewRefreshPRArgs( + fmt.Sprintf("pr:acme/monolith:%d", pull.Number), + ).RefreshArgs, + Queue: queue.QueueSweep, + }, + ); err != nil { + t.Fatal(err) + } + } if err := handler.RefreshChecks( ctx, queue.RefreshRequest{ @@ -923,6 +1105,20 @@ func TestDriftDetectorRecordsDiffAndSelfHealsWithoutWebhook( ); err != nil { t.Fatal(err) } + for index := range fixture.PullRequests { + pull := &fixture.PullRequests[index] + if err := handler.RefreshPR( + ctx, + queue.RefreshRequest{ + Args: queue.NewRefreshPRArgs( + fmt.Sprintf("pr:acme/monolith:%d", pull.Number), + ).RefreshArgs, + Queue: queue.QueueSweep, + }, + ); err != nil { + t.Fatal(err) + } + } if err := handler.RefreshChecks( ctx, queue.RefreshRequest{ @@ -1225,7 +1421,8 @@ func TestDriftDetectorRecordsDiffAndSelfHealsWithoutWebhook( func waitForCacheProducers(t *testing.T, pool *pgxpool.Pool) { t.Helper() - deadline := time.Now().Add(10 * time.Second) + ticker := time.NewTicker(20 * time.Millisecond) + defer ticker.Stop() for { var activeJobs int64 var outstandingGenerations int64 @@ -1249,14 +1446,14 @@ func waitForCacheProducers(t *testing.T, pool *pgxpool.Pool) { if activeJobs == 0 && outstandingGenerations == 0 { return } - if time.Now().After(deadline) { + select { + case <-ticker.C: + case <-t.Context().Done(): t.Fatalf( - "cache producers did not quiesce: jobs=%d generations=%d", - activeJobs, - outstandingGenerations, + "cache producers did not quiesce before test cancellation: jobs=%d generations=%d", + activeJobs, outstandingGenerations, ) } - time.Sleep(20 * time.Millisecond) } } @@ -1311,6 +1508,41 @@ func TestSemanticDiffNormalizesNullableDatabaseIDsByNodeIDInGo( } } +func TestSemanticDiffNormalizesChangedFilesAndOwnersInGo(t *testing.T) { + t.Parallel() + cache := []byte(`{ + "change_inputs": { + "files": [ + {"path":"z.go","previous_path":null}, + {"path":"a.go","previous_path":null} + ], + "owners": [ + {"path":"z.go","owner_token":"@z","owner_gh_id":null}, + {"path":"a.go","owner_token":"@a","owner_gh_id":null} + ] + } + }`) + upstream := []byte(`{ + "change_inputs": { + "files": [ + {"previous_path":null,"path":"a.go"}, + {"previous_path":null,"path":"z.go"} + ], + "owners": [ + {"owner_gh_id":null,"owner_token":"@a","path":"a.go"}, + {"owner_gh_id":null,"owner_token":"@z","path":"z.go"} + ] + } + }`) + equal, diff, err := semanticDiff(cache, upstream) + if err != nil { + t.Fatal(err) + } + if !equal || string(diff) != "{}" { + t.Fatalf("Go-sorted change-input compare equal=%v diff=%s", equal, diff) + } +} + // The sampler resolves its keyset against drift_entity_keys and then reads // the snapshots back out of drift_entities by source_id. That is only // equivalent to ordering drift_entities directly while the two views project diff --git a/internal/fakegithub/fixture.go b/internal/fakegithub/fixture.go index 66a4eaf..2e4b5e8 100644 --- a/internal/fakegithub/fixture.go +++ b/internal/fakegithub/fixture.go @@ -1,6 +1,9 @@ package fakegithub -import "time" +import ( + "maps" + "time" +) func cloneFixture(source *Fixture) Fixture { clone := *source @@ -49,6 +52,10 @@ func cloneFixture(source *Fixture) Fixture { []IssueComment(nil), pull.Comments..., ) + clone.PullRequests[index].ChangedFiles = append( + []ChangedFile(nil), + pull.ChangedFiles..., + ) if pull.Stack != nil { stack := *pull.Stack clone.PullRequests[index].Stack = &stack @@ -69,6 +76,11 @@ func cloneFixture(source *Fixture) Fixture { } } clone.CheckRuns = append([]CheckRun(nil), source.CheckRuns...) + clone.Contents = make(map[string]map[string]string, len(source.Contents)) + for ref, files := range source.Contents { + clone.Contents[ref] = make(map[string]string, len(files)) + maps.Copy(clone.Contents[ref], files) + } for index := range clone.CheckRuns { clone.CheckRuns[index].StartedAt = cloneTime( clone.CheckRuns[index].StartedAt, @@ -242,6 +254,25 @@ func DefaultFixture() Fixture { -time.Duration(len(pulls)-index) * 24 * time.Hour, ) } + pulls[0].ChangedFiles = []ChangedFile{{ + Path: "internal/tokenizer.go", ChangeType: "modified", + }} + pulls[1].ChangedFiles = []ChangedFile{ + {Path: "internal/ranker.go", ChangeType: "modified"}, + { + Path: "docs/ranking.md", PreviousPath: "docs/search.md", + ChangeType: "renamed", + }, + } + pulls[2].ChangedFiles = []ChangedFile{{ + Path: "cmd/relevance-debug/main.go", ChangeType: "added", + }} + pulls[3].ChangedFiles = []ChangedFile{{ + Path: "web/results.ts", ChangeType: "modified", + }} + pulls[4].ChangedFiles = []ChangedFile{{ + Path: "dashboards/relevance.json", ChangeType: "added", + }} stackPulls := make([]StackPullRequest, 0, len(pulls)) for index := range pulls { pull := &pulls[index] @@ -311,5 +342,20 @@ func DefaultFixture() Fixture { AppSlug: "github-actions", StartedAt: &started, CompletedAt: &completed, }, }, + Contents: defaultCodeownersContents(pulls), + } +} + +func defaultCodeownersContents(pulls []PullRequest) map[string]map[string]string { + const source = `* @reviewer +internal/** @acme/search-platform @unknown-user +docs/ docs@example.com malformed-owner +` + contents := make(map[string]map[string]string) + for index := range pulls { + contents[pulls[index].Base.SHA] = map[string]string{ + ".github/CODEOWNERS": source, + } } + return contents } diff --git a/internal/fakegithub/graphql.go b/internal/fakegithub/graphql.go index 0fb1d2d..13e7e31 100644 --- a/internal/fakegithub/graphql.go +++ b/internal/fakegithub/graphql.go @@ -66,6 +66,32 @@ func (s *Server) graphql(w http.ResponseWriter, r *http.Request) { nodes = append(nodes, node) } data["nodes"] = nodes + case strings.Contains( + request.Query, + "GhsyncPullRequestFilesPage", + ): + id, after := graphQLCursorVariables(request.Variables) + for fixtureIndex := range fixtures { + fx := &fixtures[fixtureIndex] + for pullIndex := range fx.PullRequests { + pull := &fx.PullRequests[pullIndex] + if pull.NodeID == id { + var files any + if !pull.ChangedFilesOmitted { + files = graphQLChangedFiles(pull, after) + } + data["node"] = map[string]any{ + "baseRefOid": nullableSHA(pull.Base.SHA), + "headRefOid": pull.Head.SHA, + "files": files, + } + break + } + } + if data["node"] != nil { + break + } + } case strings.Contains( request.Query, "GhsyncPullRequestReviewRequestsPage", @@ -185,6 +211,10 @@ func graphQLPullRequest( repository *Repository, pull *PullRequest, ) map[string]any { + var files any + if !pull.ChangedFilesOmitted { + files = graphQLChangedFiles(pull, 0) + } return map[string]any{ "id": pull.NodeID, "databaseId": pull.ID, @@ -199,6 +229,7 @@ func graphQLPullRequest( "headRefOid": pull.Head.SHA, "baseRefName": pull.Base.Ref, "baseRefOid": nullableSHA(pull.Base.SHA), + "changedFiles": changedFilesTotal(pull), "author": map[string]any{"login": pull.AuthorLogin}, "repository": map[string]any{ "id": repository.NodeID, @@ -216,12 +247,35 @@ func graphQLPullRequest( }, }, "reviewRequests": graphQLReviewRequests(pull.ReviewRequests, 0), + "files": files, "reviews": graphQLReviews(pull.Reviews, 0), "comments": graphQLIssueComments(pull.Comments, 0), "reviewThreads": graphQLReviewThreads(pull.ReviewThreads, 0), } } +func graphQLChangedFiles(pull *PullRequest, after int) map[string]any { + start, end, pageInfo := graphQLPage(len(pull.ChangedFiles), after) + nodes := make([]map[string]any, 0, end-start) + for _, file := range pull.ChangedFiles[start:end] { + nodes = append(nodes, map[string]any{ + "path": file.Path, + "changeType": strings.ToUpper(file.ChangeType), + }) + } + return map[string]any{ + "nodes": nodes, "pageInfo": pageInfo, + "totalCount": changedFilesTotal(pull), + } +} + +func changedFilesTotal(pull *PullRequest) int { + if pull.ChangedFilesTotal > 0 { + return pull.ChangedFilesTotal + } + return len(pull.ChangedFiles) +} + const fakeGraphQLConnectionLimit = 100 func graphQLReviewRequests( diff --git a/internal/fakegithub/rest.go b/internal/fakegithub/rest.go index e7a8f41..50bbb02 100644 --- a/internal/fakegithub/rest.go +++ b/internal/fakegithub/rest.go @@ -213,6 +213,34 @@ func (s *Server) getPull(w http.ResponseWriter, r *http.Request) { http.NotFound(w, r) } +func (s *Server) listPullFiles(w http.ResponseWriter, r *http.Request) { + pull, ok := s.pullForRequest(w, r) + if !ok { + return + } + files := append([]ChangedFile(nil), pull.ChangedFiles...) + files = paginate(files, r, w) + s.writeConditionalJSON(w, r, files) +} + +func (s *Server) getRepositoryContent(w http.ResponseWriter, r *http.Request) { + fx, ok := s.checkRepo(w, r) + if !ok { + return + } + ref := r.URL.Query().Get("ref") + path := r.PathValue("path") + files := fx.Contents[ref] + content, ok := files[path] + if !ok { + http.NotFound(w, r) + return + } + w.Header().Set("Content-Type", "application/octet-stream") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(content)) +} + func (s *Server) listPullReviews(w http.ResponseWriter, r *http.Request) { pull, ok := s.pullForRequest(w, r) if !ok { diff --git a/internal/fakegithub/server.go b/internal/fakegithub/server.go index cf3b7d3..81807a6 100644 --- a/internal/fakegithub/server.go +++ b/internal/fakegithub/server.go @@ -69,25 +69,36 @@ func nullableSHA(value string) any { // PullRequest is fixture truth for REST and GraphQL pull responses. type PullRequest struct { - ID int64 `json:"id"` - NodeID string `json:"node_id"` - Number int `json:"number"` - Title string `json:"title"` - State string `json:"state"` - Draft bool `json:"draft"` - AuthorLogin string `json:"-"` - ReviewDecision string `json:"review_decision"` - MergeableState string `json:"mergeable_state"` - Head PullRequestBranch `json:"head"` - Base Base `json:"base"` - UpdatedAt time.Time `json:"updated_at"` - CreatedAt time.Time `json:"-"` - MergedAt *time.Time `json:"-"` - Stack *StackRef `json:"stack"` - ReviewThreads []ReviewThread `json:"-"` - ReviewRequests []ReviewRequest `json:"-"` - Reviews []PullRequestReview `json:"-"` - Comments []IssueComment `json:"-"` + ID int64 `json:"id"` + NodeID string `json:"node_id"` + Number int `json:"number"` + Title string `json:"title"` + State string `json:"state"` + Draft bool `json:"draft"` + AuthorLogin string `json:"-"` + ReviewDecision string `json:"review_decision"` + MergeableState string `json:"mergeable_state"` + Head PullRequestBranch `json:"head"` + Base Base `json:"base"` + UpdatedAt time.Time `json:"updated_at"` + CreatedAt time.Time `json:"-"` + MergedAt *time.Time `json:"-"` + Stack *StackRef `json:"stack"` + ReviewThreads []ReviewThread `json:"-"` + ReviewRequests []ReviewRequest `json:"-"` + Reviews []PullRequestReview `json:"-"` + Comments []IssueComment `json:"-"` + ChangedFiles []ChangedFile `json:"-"` + ChangedFilesTotal int `json:"-"` + ChangedFilesOmitted bool `json:"-"` +} + +// ChangedFile is fixture truth for one pull-request changed-file node. The +// prior path is served by REST because GitHub's GraphQL type omits it. +type ChangedFile struct { + Path string `json:"filename"` + PreviousPath string `json:"previous_filename,omitempty"` + ChangeType string `json:"status"` } // Stack is fixture truth for the gh-stack preview API. @@ -265,6 +276,8 @@ type Fixture struct { Stacks []Stack PullRequests []PullRequest CheckRuns []CheckRun + // Contents is keyed by exact Git ref/SHA and repository-relative path. + Contents map[string]map[string]string } // RateLimitStep scripts one response's server-authoritative rate state. @@ -485,6 +498,10 @@ func New(fixture Fixture, webhookSecret string, options ...Option) *Server { //n mux.HandleFunc("GET /repos/{owner}/{repo}/stacks/{number}", s.getStack) mux.HandleFunc("GET /repos/{owner}/{repo}/pulls", s.listPulls) mux.HandleFunc("GET /repos/{owner}/{repo}/pulls/{number}", s.getPull) + mux.HandleFunc( + "GET /repos/{owner}/{repo}/pulls/{number}/files", + s.listPullFiles, + ) mux.HandleFunc( "GET /repos/{owner}/{repo}/pulls/{number}/reviews", s.listPullReviews, @@ -497,6 +514,10 @@ func New(fixture Fixture, webhookSecret string, options ...Option) *Server { //n "GET /repos/{owner}/{repo}/commits/{sha}/check-runs", s.listCheckRuns, ) + mux.HandleFunc( + "GET /repos/{owner}/{repo}/contents/{path...}", + s.getRepositoryContent, + ) mux.HandleFunc("POST /graphql", s.graphql) mux.HandleFunc("POST /app/installations/{id}/access_tokens", s.installationToken) mux.HandleFunc("GET /app/hook/deliveries", s.listAppHookDeliveries) diff --git a/internal/fakegithub/truth.go b/internal/fakegithub/truth.go index eaa3ae5..9f64c1f 100644 --- a/internal/fakegithub/truth.go +++ b/internal/fakegithub/truth.go @@ -178,22 +178,28 @@ type TruthFixtureSnapshot struct { } type TruthPullRequestSnapshot struct { - ID int64 `json:"id"` - NodeID string `json:"node_id"` - Number int `json:"number"` - Title string `json:"title"` - State string `json:"state"` - Draft bool `json:"draft"` - AuthorLogin string `json:"author_login"` - ReviewDecision string `json:"review_decision"` - MergeableState string `json:"mergeable_state"` - Head PullRequestBranch `json:"head"` - Base Base `json:"base"` - UpdatedAt time.Time `json:"updated_at"` - Stack *StackRef `json:"stack"` - ReviewRequests []ReviewRequest `json:"review_requests"` - Reviews []PullRequestReview `json:"reviews"` - Comments []IssueComment `json:"comments"` + ID int64 `json:"id"` + NodeID string `json:"node_id"` + Number int `json:"number"` + Title string `json:"title"` + State string `json:"state"` + Draft bool `json:"draft"` + AuthorLogin string `json:"author_login"` + ReviewDecision string `json:"review_decision"` + MergeableState string `json:"mergeable_state"` + Head PullRequestBranch `json:"head"` + Base Base `json:"base"` + UpdatedAt time.Time `json:"updated_at"` + Stack *StackRef `json:"stack"` + ReviewRequests []ReviewRequest `json:"review_requests"` + Reviews []PullRequestReview `json:"reviews"` + Comments []IssueComment `json:"comments"` + ChangedFiles []ChangedFile `json:"changed_files"` + ChangedFilesTotal int `json:"changed_files_total"` + ChangedFilesOmitted bool `json:"changed_files_omitted"` + CodeownersPath string `json:"codeowners_path,omitempty"` + CodeownersSource string `json:"codeowners_source,omitempty"` + CodeownersState string `json:"codeowners_state"` } type TruthReviewThreadSnapshot struct { @@ -827,8 +833,18 @@ func snapshotFixture(fixture Fixture) TruthFixtureSnapshot { []IssueComment(nil), pull.Comments..., ), + ChangedFiles: append( + []ChangedFile(nil), pull.ChangedFiles..., + ), + ChangedFilesTotal: changedFilesTotal(&pull), + ChangedFilesOmitted: pull.ChangedFilesOmitted, }, ) + storedPull := &snapshot.PullRequests[len(snapshot.PullRequests)-1] + storedPull.CodeownersPath, storedPull.CodeownersSource, + storedPull.CodeownersState = fixtureCodeowners( + &fixture, pull.Base.SHA, + ) for _, thread := range pull.ReviewThreads { updatedAt := pull.UpdatedAt for _, comment := range thread.Comments { @@ -895,3 +911,22 @@ func snapshotFixture(fixture Fixture) TruthFixtureSnapshot { }) return snapshot } + +func fixtureCodeowners(fixture *Fixture, ref string) (string, string, string) { + if ref == "" { + return "", "", "unavailable" + } + for _, path := range []string{ + ".github/CODEOWNERS", "CODEOWNERS", "docs/CODEOWNERS", + } { + content, ok := fixture.Contents[ref][path] + if !ok { + continue + } + if len(content) >= 3<<20 { + return path, "", "oversized" + } + return path, content, "present" + } + return "", "", "missing" +} diff --git a/internal/fetch/coordinator.go b/internal/fetch/coordinator.go index e0ed470..7f4d1e1 100644 --- a/internal/fetch/coordinator.go +++ b/internal/fetch/coordinator.go @@ -10,6 +10,7 @@ import ( "time" "github.com/ewhauser/ghsync/internal/budget" + "github.com/ewhauser/ghsync/internal/changeinputs" "github.com/ewhauser/ghsync/internal/gh" "github.com/ewhauser/ghsync/internal/store" ) @@ -69,6 +70,7 @@ type prCoordinator struct { window time.Duration max int graphQL *gh.GraphQLClient + rest *gh.RESTClient writer *store.EntityWriter installationID int64 orgID int64 @@ -76,6 +78,7 @@ type prCoordinator struct { func newPRCoordinator( graphQL *gh.GraphQLClient, + rest *gh.RESTClient, writer *store.EntityWriter, installationID int64, orgID int64, @@ -89,6 +92,7 @@ func newPRCoordinator( window: window, max: defaultBatchSize, graphQL: graphQL, + rest: rest, writer: writer, installationID: installationID, orgID: orgID, @@ -262,6 +266,25 @@ func (c *prCoordinator) execute(batch *pendingPullBatch) { c.installationID, c.orgID, ) + snapshot, err := changeinputs.Hydrate( + callCtx, + c.rest, + c.writer, + item.class, + record.Repository.GitHubID, + record.Repository.Owner, + record.Repository.Name, + record.Number, + node, + ) + if err != nil { + results[item] = pullBatchResult{ + err: fmt.Errorf("hydrate PR change inputs: %w", err), + } + continue + } + record.ChangeSnapshot = snapshot + record.ChangeInputsKnown = true repoID := record.Repository.GitHubID if repoErr := repositoryFailures[repoID]; repoErr != nil { results[item] = pullBatchResult{err: repoErr} diff --git a/internal/fetch/fetch_db_test.go b/internal/fetch/fetch_db_test.go index 32d5226..69dc0fa 100644 --- a/internal/fetch/fetch_db_test.go +++ b/internal/fetch/fetch_db_test.go @@ -379,6 +379,21 @@ func TestIdenticalPR200OnlyAdvancesLastCheckedAt(t *testing.T) { baseTime := time.Date(2026, 7, 28, 12, 0, 0, 0, time.UTC) repository := testRepository("acme/recheck", 2400, baseTime) pull := testPull(&repository, baseTime, "same-head") + pull.ChangeInputsKnown = true + pull.ChangeSnapshot = &store.PullRequestChangeSnapshotRecord{ + BaseSHA: "base", HeadSHA: "same-head", FilesTotalCount: 1, + CodeownersRef: "main", CodeownersSHA: "base", + CodeownersPath: "CODEOWNERS", CodeownersState: "present", + CodeownersSource: "* @unknown", CodeownersHash: "hash-one", + Files: []store.ChangedFileRecord{{ + Path: "src/main.go", ChangeType: "modified", + }}, + Owners: []store.FileOwnerRecord{{ + Path: "src/main.go", OwnerToken: "@unknown", OwnerType: "user", + OwnerName: "unknown", ResolutionState: "unresolved", + SourcePattern: "*", SourceLine: 1, + }}, + } if _, err := writer.ApplyPullRequest(context.Background(), pull); err != nil { t.Fatal(err) } @@ -433,6 +448,137 @@ func TestIdenticalPR200OnlyAdvancesLastCheckedAt(t *testing.T) { if events != 1 { t.Fatalf("identical PR change events = %d, want 1 initial event", events) } + var snapshotSynced, snapshotChecked time.Time + if err := pool.QueryRow(context.Background(), ` + SELECT synced_at, last_checked_at + FROM pull_request_change_snapshots + WHERE repo_id = (SELECT id FROM repos WHERE gh_id = 2400) + AND pr_number = 42 + `).Scan(&snapshotSynced, &snapshotChecked); err != nil { + t.Fatal(err) + } + if !snapshotSynced.Equal(initialSynced) || + !snapshotChecked.After(initialChecked) { + t.Fatalf( + "identical change inputs synced=%s checked=%s", + snapshotSynced, snapshotChecked, + ) + } + + ownershipChanged := pull + ownershipChanged.SyncedAt = pull.SyncedAt.Add(time.Minute) + changedSnapshot := *pull.ChangeSnapshot + changedSnapshot.CodeownersSource = "* @new-owner" + changedSnapshot.CodeownersHash = "hash-two" + changedSnapshot.Owners = []store.FileOwnerRecord{{ + Path: "src/main.go", OwnerToken: "@new-owner", OwnerType: "user", + OwnerName: "new-owner", ResolutionState: "unresolved", + SourcePattern: "*", SourceLine: 1, + }} + ownershipChanged.ChangeSnapshot = &changedSnapshot + result, err = writer.ApplyPullRequest( + context.Background(), ownershipChanged, + ) + if err != nil { + t.Fatal(err) + } + if !result.ChangeInputsChanged || !result.Applied || result.DomainChanged { + t.Fatalf("equal-parent ownership change result = %+v", result) + } + if err := pool.QueryRow(context.Background(), ` + SELECT count(*) FROM change_events + WHERE kind = 'pull_request.changed' + AND entity_key = 'pr:1:2400:42' + `).Scan(&events); err != nil { + t.Fatal(err) + } + if events != 2 { + t.Fatalf("ownership change events = %d, want exactly 2 total", events) + } + + stale := ownershipChanged + stale.GitHubUpdatedAt = baseTime.Add(-time.Minute) + stale.SyncedAt = ownershipChanged.SyncedAt.Add(time.Minute) + staleSnapshot := changedSnapshot + staleSnapshot.Files = []store.ChangedFileRecord{{ + Path: "stale.go", ChangeType: "added", + }} + staleSnapshot.CodeownersSource = "* @stale-owner" + staleSnapshot.CodeownersHash = "stale-hash" + staleSnapshot.Owners = nil + stale.ChangeSnapshot = &staleSnapshot + result, err = writer.ApplyPullRequest(context.Background(), stale) + if err != nil { + t.Fatal(err) + } + if result.Applied || result.ChangeInputsChanged { + t.Fatalf("stale parent changed ownership children: %+v", result) + } + var livePath string + if err := pool.QueryRow(context.Background(), ` + SELECT path FROM pull_request_changed_files + WHERE repo_id = (SELECT id FROM repos WHERE gh_id = 2400) + AND pr_number = 42 AND tombstoned_at IS NULL + `).Scan(&livePath); err != nil { + t.Fatal(err) + } + if livePath != "src/main.go" { + t.Fatalf("stale parent replaced changed files with %q", livePath) + } + var liveHash, liveOwner string + if err := pool.QueryRow(context.Background(), ` + SELECT snapshot.codeowners_hash, owner.owner_token + FROM pull_request_change_snapshots AS snapshot + JOIN pull_request_file_owners AS owner + ON owner.repo_id = snapshot.repo_id + AND owner.pr_number = snapshot.pr_number + AND owner.tombstoned_at IS NULL + WHERE snapshot.repo_id = (SELECT id FROM repos WHERE gh_id = 2400) + AND snapshot.pr_number = 42 + AND snapshot.tombstoned_at IS NULL + `).Scan(&liveHash, &liveOwner); err != nil { + t.Fatal(err) + } + if liveHash != "hash-two" || liveOwner != "@new-owner" { + t.Fatalf( + "stale parent replaced snapshot/owner with %q/%q", + liveHash, liveOwner, + ) + } + var beforeParentAdvance time.Time + if err := pool.QueryRow(context.Background(), ` + SELECT synced_at + FROM pull_request_change_snapshots + WHERE repo_id = (SELECT id FROM repos WHERE gh_id = 2400) + AND pr_number = 42 + `).Scan(&beforeParentAdvance); err != nil { + t.Fatal(err) + } + newerParent := ownershipChanged + newerParent.GitHubUpdatedAt = baseTime.Add(time.Minute) + newerParent.SyncedAt = stale.SyncedAt.Add(time.Minute) + result, err = writer.ApplyPullRequest(context.Background(), newerParent) + if err != nil { + t.Fatal(err) + } + if !result.DomainChanged || result.ChangeInputsChanged { + t.Fatalf("parent-only freshness advance result = %+v", result) + } + var afterParentAdvance time.Time + if err := pool.QueryRow(context.Background(), ` + SELECT synced_at + FROM pull_request_change_snapshots + WHERE repo_id = (SELECT id FROM repos WHERE gh_id = 2400) + AND pr_number = 42 + `).Scan(&afterParentAdvance); err != nil { + t.Fatal(err) + } + if !afterParentAdvance.Equal(beforeParentAdvance) { + t.Fatalf( + "parent-only freshness changed snapshot synced_at %s -> %s", + beforeParentAdvance, afterParentAdvance, + ) + } } func TestPullRequestBatchIsolatesPoisonEntity(t *testing.T) { @@ -1157,6 +1303,216 @@ func TestRepositoryRulesLockedCASDirtyEventAndConditionalRecheck(t *testing.T) { } } +func TestPullRequestRefreshMirrorsFencedFilesAndCodeowners(t *testing.T) { + t.Parallel() + pool := fetchTestDatabase(t) + fixture := fakegithub.DefaultFixture() + _, server, handler, riverClient := newDirectHandler( + t, pool, fixture, 5*time.Millisecond, 100, + ) + defer server.Close() + handler.SetRiverClient(riverClient) + request := queue.RefreshRequest{ + Args: queue.NewRefreshPRArgs( + "pr:acme/monolith:4812", + ).RefreshArgs, + Queue: queue.QueueEvent, + } + if err := handler.RefreshPR(context.Background(), request); err != nil { + t.Fatal(err) + } + var baseSHA, headSHA, sourcePath, sourceState string + var total int + var truncated bool + if err := pool.QueryRow(context.Background(), ` + SELECT base_sha, head_sha, files_total_count, files_truncated, + codeowners_path, codeowners_state + FROM pull_request_change_snapshots + WHERE pr_number = 4812 AND tombstoned_at IS NULL + `).Scan( + &baseSHA, &headSHA, &total, &truncated, &sourcePath, &sourceState, + ); err != nil { + t.Fatal(err) + } + if baseSHA != "bbbb001" || headSHA != "8f31c2d" || total != 2 || + truncated || sourcePath != ".github/CODEOWNERS" || + sourceState != "present" { + t.Fatalf( + "change snapshot = %q/%q total=%d truncated=%v source=%q/%q", + baseSHA, headSHA, total, truncated, sourcePath, sourceState, + ) + } + var previous string + if err := pool.QueryRow(context.Background(), ` + SELECT previous_path + FROM pull_request_changed_files + WHERE pr_number = 4812 AND path = 'docs/ranking.md' + AND tombstoned_at IS NULL + `).Scan(&previous); err != nil { + t.Fatal(err) + } + if previous != "docs/search.md" { + t.Fatalf("rename previous path = %q", previous) + } + type ownerState struct { + token string + kind string + state string + id string + } + rows, err := pool.Query(context.Background(), ` + SELECT owner_token, owner_type, resolution_state, + COALESCE(owner_node_id, '') + FROM pull_request_file_owners + WHERE pr_number = 4812 AND tombstoned_at IS NULL + ORDER BY path, owner_token + `) + if err != nil { + t.Fatal(err) + } + defer rows.Close() + var owners []ownerState + for rows.Next() { + var owner ownerState + if err := rows.Scan( + &owner.token, &owner.kind, &owner.state, &owner.id, + ); err != nil { + t.Fatal(err) + } + owners = append(owners, owner) + } + if err := rows.Err(); err != nil { + t.Fatal(err) + } + want := []ownerState{ + {token: "docs@example.com", kind: "email", state: "unresolved"}, + {token: "malformed-owner", kind: "malformed", state: "unresolved"}, + { + token: "@acme/search-platform", kind: "team", state: "resolved", + id: "T_kwDOABCDEF6001", + }, + {token: "@unknown-user", kind: "user", state: "unresolved"}, + } + if !reflect.DeepEqual(owners, want) { + t.Fatalf("file owners = %#v, want %#v", owners, want) + } +} + +func TestPullRequestRefreshResolvesRenameByNewPathAndOwnerCase(t *testing.T) { + t.Parallel() + pool := fetchTestDatabase(t) + fixture := fakegithub.DefaultFixture() + contents, ok := fixture.Contents["bbbb001"] + if !ok { + t.Fatal("fixture has no CODEOWNERS content at rename base") + } + contents[".github/CODEOWNERS"] = + "* @Reviewer\n" + + "internal/ranker.go\n" + + "docs/search.md @old-owner\n" + + "docs/ranking.md @Reviewer\n" + _, server, handler, riverClient := newDirectHandler( + t, pool, fixture, 5*time.Millisecond, 100, + ) + defer server.Close() + handler.SetRiverClient(riverClient) + request := queue.RefreshRequest{ + Args: queue.NewRefreshPRArgs( + "pr:acme/monolith:4812", + ).RefreshArgs, + Queue: queue.QueueEvent, + } + if err := handler.RefreshPR(context.Background(), request); err != nil { + t.Fatal(err) + } + var token, state, login, pattern string + if err := pool.QueryRow(context.Background(), ` + SELECT owner_token, resolution_state, owner_login, source_pattern + FROM pull_request_file_owners + WHERE pr_number = 4812 + AND path = 'docs/ranking.md' + AND tombstoned_at IS NULL + `).Scan(&token, &state, &login, &pattern); err != nil { + t.Fatal(err) + } + if token != "@Reviewer" || state != "resolved" || login != "reviewer" || + pattern != "docs/ranking.md" { + t.Fatalf( + "renamed-path owner = %q/%q/%q via %q", + token, state, login, pattern, + ) + } + var oldPathOwners int + if err := pool.QueryRow(context.Background(), ` + SELECT count(*) + FROM pull_request_file_owners + WHERE pr_number = 4812 + AND owner_token = '@old-owner' + AND tombstoned_at IS NULL + `).Scan(&oldPathOwners); err != nil { + t.Fatal(err) + } + if oldPathOwners != 0 { + t.Fatalf("rename resolved %d owners from previous_path", oldPathOwners) + } + var clearedOwners int + if err := pool.QueryRow(context.Background(), ` + SELECT count(*) + FROM pull_request_file_owners + WHERE pr_number = 4812 + AND path = 'internal/ranker.go' + AND tombstoned_at IS NULL + `).Scan(&clearedOwners); err != nil { + t.Fatal(err) + } + if clearedOwners != 0 { + t.Fatalf("ownerless later rule retained %d owners", clearedOwners) + } +} + +func TestPullRequestRefreshRejectsSynchronizeDuringHydration(t *testing.T) { + t.Parallel() + pool := fetchTestDatabase(t) + fixture := fakegithub.DefaultFixture() + path := "/repos/acme/monolith/pulls/4812" + _, server, handler, riverClient := newDirectHandler( + t, + pool, + fixture, + 5*time.Millisecond, + 100, + fakegithub.WithRequestHook(func(method, requestPath string, count int, fx *fakegithub.Fixture) { + if method != "GET" || requestPath != path || count != 2 { + return + } + fx.PullRequests[1].Head.SHA = "synchronized-head" + fx.PullRequests[1].ChangedFiles = []fakegithub.ChangedFile{{ + Path: "new/from-synchronize.go", ChangeType: "added", + }} + }), + ) + defer server.Close() + handler.SetRiverClient(riverClient) + err := handler.RefreshPR(context.Background(), queue.RefreshRequest{ + Args: queue.NewRefreshPRArgs( + "pr:acme/monolith:4812", + ).RefreshArgs, + Queue: queue.QueueEvent, + }) + if err == nil || !strings.Contains(err.Error(), "base/head changed") { + t.Fatalf("synchronize race error = %v", err) + } + var snapshots int + if err := pool.QueryRow(context.Background(), ` + SELECT count(*) FROM pull_request_change_snapshots + `).Scan(&snapshots); err != nil { + t.Fatal(err) + } + if snapshots != 0 { + t.Fatalf("synchronize race persisted %d snapshots", snapshots) + } +} + func TestPRETagSurvivesGraphQLAndChecksRecheckUses304(t *testing.T) { t.Parallel() pool := fetchTestDatabase(t) @@ -2398,10 +2754,10 @@ func TestResolveStackMembershipCompletesWithNullHistoricalBaseSHA( ); err != nil { t.Fatal(err) } - if stackJobs < 1 || stackJobs > 3 || completedStackJobs != stackJobs || + if stackJobs < 1 || stackJobs > 4 || completedStackJobs != stackJobs || maxStackAttempts != 1 { t.Fatalf( - "open unknown-SHA stack refreshes/completed/max-attempt = %d/%d/%d, want 1..3/all/1", + "open unknown-SHA stack refreshes/completed/max-attempt = %d/%d/%d, want 1..4/all/1", stackJobs, completedStackJobs, maxStackAttempts, @@ -2470,22 +2826,25 @@ func TestRefreshPRWorkerPersistsNullGraphQLBaseRefOID(t *testing.T) { t.Fatalf("GraphQL null baseRefOid cached as %q, want unknown", cachedBaseSHA) } var jobs, completed, maxAttempts int + var jobErrors string if err := harness.pool.QueryRow(t.Context(), ` SELECT count(*), count(*) FILTER (WHERE state = 'completed'), - COALESCE(max(attempt), 0) + COALESCE(max(attempt), 0), + COALESCE(string_agg(errors::text, E'\n'), '') FROM river_job WHERE kind = 'refresh_pr' AND args->>'key' = $1 - `, key).Scan(&jobs, &completed, &maxAttempts); err != nil { + `, key).Scan(&jobs, &completed, &maxAttempts, &jobErrors); err != nil { t.Fatal(err) } - if jobs != 1 || completed != 1 || maxAttempts != 1 { + if jobs != 2 || completed != 2 || maxAttempts != 1 { t.Fatalf( - "GraphQL refresh_pr jobs/completed/max-attempt = %d/%d/%d, want 1/1/1", + "GraphQL refresh_pr jobs/completed/max-attempt = %d/%d/%d, want 2/2/1; errors: %s", jobs, completed, maxAttempts, + jobErrors, ) } } @@ -2541,6 +2900,16 @@ func TestOrderIndependenceFinalCacheState(t *testing.T) { } harness.dispatchAll() harness.waitIdle() + // Reconciliation must converge the source-rich PR observation after any + // event-order race with parent-only repository or stack observations. + if _, err := harness.river.Insert( + t.Context(), + queue.NewRefreshPRArgs(fmt.Sprintf("pr:%s:4812", repo)), + queue.NewRefreshInsertOptsForQueue(queue.QueueSweep, time.Time{}), + ); err != nil { + t.Fatal(err) + } + harness.waitIdle() got := snapshotCache(t, harness.pool, repo) harness.close() if !reflect.DeepEqual(got, want) { @@ -2559,11 +2928,11 @@ func expectedOrderCacheSnapshot() cacheSnapshot { // and snapshotCache. It prevents an identically empty or consistently // malformed implementation from satisfying C-I4 by self-comparison. return cacheSnapshot{ - Repos: `[{"gh_id": 2001, "node_id": "R_acme_order", "archived": false, "head_sha": "", "full_name": "acme/order", "tombstoned": false, "sync_source": "webhook", "gh_updated_at": "2026-07-28T12:00:00Z", "default_branch": "main"}]`, + Repos: `[{"gh_id": 2001, "node_id": "R_acme_order", "archived": false, "head_sha": "aaaa000", "full_name": "acme/order", "tombstoned": false, "sync_source": "reconcile", "gh_updated_at": "2026-07-28T12:00:00Z", "default_branch": "main"}]`, RepoRules: `[]`, Stacks: `[{"open": true, "gh_id": 9876543, "number": 142, "entries": [{"draft": false, "state": "closed", "number": 4810, "head_ref": "refactor/tokenizer", "head_sha": "bbbb001", "updated_at": "2026-07-28T12:00:00Z"}, {"draft": false, "state": "open", "number": 4812, "head_ref": "refactor/bm25f-ranker", "head_sha": "8f31c2d", "updated_at": "2026-07-28T12:00:00Z"}, {"draft": false, "state": "open", "number": 4815, "head_ref": "feat/relevance-debug", "head_sha": "bbbb003", "updated_at": "2026-07-28T12:00:00Z"}, {"draft": false, "state": "open", "number": 4816, "head_ref": "feat/results-rewire", "head_sha": "bbbb004", "updated_at": "2026-07-28T12:00:00Z"}, {"draft": false, "state": "open", "number": 4820, "head_ref": "feat/relevance-telemetry", "head_sha": "bbbb005", "updated_at": "2026-07-28T12:00:00Z"}], "node_id": "S_kwDOABCDEF4AAAAA", "base_ref": "main", "base_sha": "aaaa000", "head_sha": "bbbb005", "tombstoned": false, "sync_source": "webhook", "gh_updated_at": "2026-07-28T12:00:00Z"}]`, Pulls: `[{"gh_id": 804810, "state": "closed", "title": "Tokenizer rewrite for query parser", "number": 4810, "node_id": "PR_kwDOABCDEF4810", "base_ref": "main", "base_sha": "aaaa000", "head_ref": "refactor/tokenizer", "head_sha": "bbbb001", "tombstoned": false, "sync_source": "webhook", "stack_number": 142, "gh_updated_at": "2026-07-28T12:00:00Z", "stack_position": 1, "review_decision": "APPROVED"}, {"gh_id": 804812, "state": "open", "title": "BM25F ranker integration", "number": 4812, "node_id": "PR_kwDOABCDEF4812", "base_ref": "refactor/tokenizer", "base_sha": "bbbb001", "head_ref": "refactor/bm25f-ranker", "head_sha": "8f31c2d", "tombstoned": false, "sync_source": "webhook", "stack_number": 142, "gh_updated_at": "2026-07-28T12:00:00Z", "stack_position": 2, "review_decision": "CHANGES_REQUESTED"}, {"gh_id": 804815, "state": "open", "title": "Relevance debug API endpoint", "number": 4815, "node_id": "PR_kwDOABCDEF4815", "base_ref": "refactor/bm25f-ranker", "base_sha": "8f31c2d", "head_ref": "feat/relevance-debug", "head_sha": "bbbb003", "tombstoned": false, "sync_source": "webhook", "stack_number": 142, "gh_updated_at": "2026-07-28T12:00:00Z", "stack_position": 3, "review_decision": "REVIEW_REQUIRED"}, {"gh_id": 804816, "state": "open", "title": "Results page rewiring", "number": 4816, "node_id": "PR_kwDOABCDEF4816", "base_ref": "feat/relevance-debug", "base_sha": "bbbb003", "head_ref": "feat/results-rewire", "head_sha": "bbbb004", "tombstoned": false, "sync_source": "webhook", "stack_number": 142, "gh_updated_at": "2026-07-28T12:00:00Z", "stack_position": 4, "review_decision": "REVIEW_REQUIRED"}, {"gh_id": 804820, "state": "open", "title": "Relevance telemetry dashboards", "number": 4820, "node_id": "PR_kwDOABCDEF4820", "base_ref": "feat/results-rewire", "base_sha": "bbbb004", "head_ref": "feat/relevance-telemetry", "head_sha": "bbbb005", "tombstoned": false, "sync_source": "webhook", "stack_number": 142, "gh_updated_at": "2026-07-28T12:00:00Z", "stack_position": 5, "review_decision": "REVIEW_REQUIRED"}]`, - Threads: `[]`, + Threads: `[{"id": "PRRT_kwDOABCDEF4812_1", "line": null, "path": "internal/ranker.go", "comments": [{"id": "PRRC_kwDOABCDEF4812_1", "body": "Please cover the tie case.", "updated_at": "2026-07-28T12:00:00Z", "author_login": "reviewer"}], "head_sha": "8f31c2d", "pr_number": 4812, "tombstoned": false, "is_outdated": false, "is_resolved": false, "sync_source": "webhook", "gh_updated_at": "2026-07-28T12:00:00Z"}]`, Checks: `[{"name": "unit", "gh_id": 99001, "status": "completed", "head_sha": "8f31c2d", "conclusion": "failure", "tombstoned": false, "sync_source": "webhook", "gh_updated_at": "2026-07-28T11:55:00Z"}, {"name": "lint", "gh_id": 99002, "status": "completed", "head_sha": "8f31c2d", "conclusion": "success", "tombstoned": false, "sync_source": "webhook", "gh_updated_at": "2026-07-28T11:55:00Z"}]`, CheckHistory: `[{"name": "unit", "status": "completed", "head_sha": "8f31c2d", "conclusion": "failure", "sync_source": "webhook", "gh_updated_at": "2026-07-28T11:55:00Z", "check_run_gh_id": 99001}, {"name": "lint", "status": "completed", "head_sha": "8f31c2d", "conclusion": "success", "sync_source": "webhook", "gh_updated_at": "2026-07-28T11:55:00Z", "check_run_gh_id": 99002}]`, Dirty: `["pr:1:2001:4810", "pr:1:2001:4812", "pr:1:2001:4815", "pr:1:2001:4816", "pr:1:2001:4820", "stack:1:2001:142"]`, @@ -2581,6 +2950,17 @@ func TestStormAssertsFetchCount(t *testing.T) { "stack": map[string]any{"number": 142}, }, } + if _, err := harness.river.Insert( + t.Context(), + queue.NewRefreshStackArgs("stack:acme/storm:142"), + queue.NewRefreshInsertOptsForQueue(queue.QueueEvent, time.Time{}), + ); err != nil { + t.Fatal(err) + } + // A real branch push fans out from previously mirrored branch membership. + // Seed that membership directly so this test measures storm coalescing, + // not cold-start behavior. + harness.waitIdle() harness.emit("storm-warm", warm) harness.dispatchAll() harness.waitIdle() diff --git a/internal/fetch/handler.go b/internal/fetch/handler.go index bcaf01a..f01d747 100644 --- a/internal/fetch/handler.go +++ b/internal/fetch/handler.go @@ -17,6 +17,7 @@ import ( "github.com/riverqueue/river" "github.com/ewhauser/ghsync/internal/budget" + "github.com/ewhauser/ghsync/internal/changeinputs" "github.com/ewhauser/ghsync/internal/gh" "github.com/ewhauser/ghsync/internal/queue" "github.com/ewhauser/ghsync/internal/repoutil" @@ -72,6 +73,7 @@ func New(options Options) (*Handler, error) { } handler.coordinator = newPRCoordinator( options.GraphQL, + options.REST, writer, options.InstallationID, options.OrgID, @@ -352,8 +354,14 @@ func (h *Handler) refreshPRREST( key.Repo, key.Number, ) + // A hydration request must observe the PR-scoped GraphQL connections even + // if a concurrent stack refresh has just populated the parent and ETag. + // Sending that ETag could yield 304 and incorrectly skip changed files, + // participation, and ownership on this cold-start path. if metadataErr == nil { - etag = metadata.ETag + if !hydrateGraphQL { + etag = metadata.ETag + } } else if !errors.Is(metadataErr, pgx.ErrNoRows) { return fmt.Errorf("read PR ETag: %w", metadataErr) } @@ -444,6 +452,26 @@ func (h *Handler) refreshPRREST( graphQLRecord.MembershipKnown = true graphQLRecord.StackSummary = record.StackSummary record = graphQLRecord + snapshot, hydrateErr := changeinputs.Hydrate( + ctx, + h.rest, + h.writer, + class, + repository.GitHubID, + owner, + repoName, + record.Number, + nodes[0], + ) + if hydrateErr != nil { + return fmt.Errorf( + "hydrate PR change inputs %s: %w", + requestKey(key), + hydrateErr, + ) + } + record.ChangeSnapshot = snapshot + record.ChangeInputsKnown = true } _, err = h.writer.ApplyPullRequestObserved( ctx, diff --git a/internal/gh/graphql.go b/internal/gh/graphql.go index b27f3d0..fdd5fcc 100644 --- a/internal/gh/graphql.go +++ b/internal/gh/graphql.go @@ -18,6 +18,11 @@ const defaultGraphQLResponseBytes = 10 << 20 // MaxPullRequestBatch is GitHub's nodes-per-gang cap used by the coordinator. const MaxPullRequestBatch = 25 +// MaxPullRequestFiles is GitHub's documented changed-file listing cap. The +// mirror stops at this boundary and records explicit truncation when the +// connection reports more files or remains pageable. +const MaxPullRequestFiles = 3000 + // GraphQLClient executes budget-gated installation GraphQL calls. type GraphQLClient struct { client client @@ -93,10 +98,12 @@ type PullRequestNode struct { HeadRefOID string `json:"headRefOid"` BaseRefName string `json:"baseRefName"` BaseRefOID string `json:"baseRefOid"` + ChangedFiles int `json:"changedFiles"` Author struct { Login string `json:"login"` } `json:"author"` - Repository RepositoryNode `json:"repository"` + Repository RepositoryNode `json:"repository"` + Files *PullRequestFilesConnection `json:"files"` ReviewRequests struct { Nodes []ReviewRequestNode `json:"nodes"` PageInfo PageInfo `json:"pageInfo"` @@ -115,6 +122,25 @@ type PullRequestNode struct { } `json:"reviewThreads"` } +// PullRequestChangedFileNode is one GraphQL changed-file fact. GitHub's +// GraphQL type does not expose the prior path for a rename; the fetch layer +// supplements renamed nodes from the bounded REST files endpoint. +type PullRequestChangedFileNode struct { + Path string `json:"path"` + PreviousPath string `json:"-"` + ChangeType string `json:"changeType"` +} + +// PullRequestFilesConnection carries the authoritative page set and its +// completeness state. Truncated is derived locally from GitHub's 3,000-file +// cap, an inconsistent total, or an unfinished cursor. +type PullRequestFilesConnection struct { + Nodes []PullRequestChangedFileNode `json:"nodes"` + PageInfo PageInfo `json:"pageInfo"` + TotalCount int `json:"totalCount"` + Truncated bool `json:"-"` +} + // ActorNode preserves every GraphQL Actor variant instead of projecting only // users. Author is nil when GitHub retains the fact but no longer exposes the // deleted actor. @@ -249,6 +275,7 @@ const pullRequestNodesQuery = `query GhsyncPullRequestNodes($ids: [ID!]!) { headRefOid baseRefName baseRefOid + changedFiles author { login } repository { id @@ -260,6 +287,11 @@ const pullRequestNodesQuery = `query GhsyncPullRequestNodes($ids: [ID!]!) { owner { login } defaultBranchRef { name target { oid } } } + files(first: 100) { + pageInfo { hasNextPage endCursor } + totalCount + nodes { path changeType } + } reviewRequests(first: 100) { pageInfo { hasNextPage endCursor } nodes { @@ -311,6 +343,24 @@ const pullRequestNodesQuery = `query GhsyncPullRequestNodes($ids: [ID!]!) { rateLimit { cost limit remaining resetAt } }` +const pullRequestFilesPageQuery = `query GhsyncPullRequestFilesPage( + $id: ID!, + $after: String +) { + node(id: $id) { + ... on PullRequest { + baseRefOid + headRefOid + files(first: 100, after: $after) { + pageInfo { hasNextPage endCursor } + totalCount + nodes { path changeType } + } + } + } + rateLimit { cost limit remaining resetAt } +}` + const pullRequestReviewsPageQuery = `query GhsyncPullRequestReviewsPage( $id: ID!, $after: String @@ -473,6 +523,65 @@ func (c *GraphQLClient) completePullRequestReviewConnections( class budget.Class, pull *PullRequestNode, ) error { + if pull.Files != nil { + if len(pull.Files.Nodes) > MaxPullRequestFiles { + pull.Files.Nodes = pull.Files.Nodes[:MaxPullRequestFiles] + pull.Files.Truncated = true + } + for pull.Files.PageInfo.HasNextPage && + len(pull.Files.Nodes) < MaxPullRequestFiles { + if pull.Files.PageInfo.EndCursor == nil { + return fmt.Errorf("files hasNextPage without endCursor") + } + var data struct { + Node *struct { + BaseRefOID string `json:"baseRefOid"` + HeadRefOID string `json:"headRefOid"` + Files *PullRequestFilesConnection `json:"files"` + } `json:"node"` + } + _, err := c.Call( + ctx, + class, + pullRequestFilesPageQuery, + map[string]any{ + "id": pull.ID, + "after": *pull.Files.PageInfo.EndCursor, + }, + &data, + ) + if err != nil { + return fmt.Errorf("paginate files for %s: %w", pull.ID, err) + } + if data.Node == nil || data.Node.Files == nil { + pull.Files.Truncated = true + break + } + if data.Node.BaseRefOID != pull.BaseRefOID || + data.Node.HeadRefOID != pull.HeadRefOID { + return fmt.Errorf( + "paginate files for %s: pull request SHA fence changed", + pull.ID, + ) + } + remaining := MaxPullRequestFiles - len(pull.Files.Nodes) + pageNodes := data.Node.Files.Nodes + if len(pageNodes) > remaining { + pageNodes = pageNodes[:remaining] + pull.Files.Truncated = true + } + pull.Files.Nodes = append(pull.Files.Nodes, pageNodes...) + pull.Files.PageInfo = data.Node.Files.PageInfo + if data.Node.Files.TotalCount != pull.Files.TotalCount { + pull.Files.Truncated = true + } + } + if pull.Files.PageInfo.HasNextPage || + pull.Files.TotalCount != len(pull.Files.Nodes) || + pull.Files.TotalCount > MaxPullRequestFiles { + pull.Files.Truncated = true + } + } for pull.ReviewRequests.PageInfo.HasNextPage { if pull.ReviewRequests.PageInfo.EndCursor == nil { return fmt.Errorf("reviewRequests hasNextPage without endCursor") diff --git a/internal/gh/graphql_files_test.go b/internal/gh/graphql_files_test.go new file mode 100644 index 0000000..0813568 --- /dev/null +++ b/internal/gh/graphql_files_test.go @@ -0,0 +1,142 @@ +package gh_test + +import ( + "context" + "fmt" + "net/http/httptest" + "strings" + "testing" + + "github.com/ewhauser/ghsync/internal/budget" + "github.com/ewhauser/ghsync/internal/fakegithub" + "github.com/ewhauser/ghsync/internal/gh" +) + +func TestPullRequestFilesPaginationAndCompletenessBoundaries(t *testing.T) { + t.Parallel() + tests := []struct { + name string + count int + total int + omitted bool + wantNodes int + truncated bool + }{ + { + name: "cursor complete across page boundary", count: 101, + total: 101, wantNodes: 101, + }, + { + name: "exact documented cap is complete", count: 3000, + total: 3000, wantNodes: gh.MaxPullRequestFiles, + }, + { + name: "documented cap is explicit", count: 3001, + total: 3001, wantNodes: gh.MaxPullRequestFiles, truncated: true, + }, + { + name: "reported total mismatch is explicit", count: 100, + total: 101, wantNodes: 100, truncated: true, + }, + { + name: "omitted connection is preserved", count: 1, + total: 1, omitted: true, truncated: true, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + fixture := fakegithub.DefaultFixture() + pull := &fixture.PullRequests[0] + pull.ChangedFiles = make([]fakegithub.ChangedFile, test.count) + for index := range pull.ChangedFiles { + pull.ChangedFiles[index] = fakegithub.ChangedFile{ + Path: fmt.Sprintf("src/file-%04d.go", index), + ChangeType: "modified", + } + } + pull.ChangedFilesTotal = test.total + pull.ChangedFilesOmitted = test.omitted + server := httptest.NewServer(fakegithub.New(fixture, "secret")) + t.Cleanup(server.Close) + client, err := gh.NewGraphQLClient( + server.URL, + budget.New(server.Client(), budget.Options{}), + gh.StaticToken("fake-installation-files"), + ) + if err != nil { + t.Fatal(err) + } + nodes, _, err := client.BatchPullRequests( + context.Background(), + budget.Interactive, + []string{pull.NodeID}, + ) + if err != nil { + t.Fatal(err) + } + if len(nodes) != 1 || nodes[0] == nil { + t.Fatalf("nodes = %#v", nodes) + } + if test.omitted { + if nodes[0].Files != nil { + t.Fatalf("omitted files = %#v, want nil", nodes[0].Files) + } + return + } + files := nodes[0].Files + if len(files.Nodes) != test.wantNodes || + files.Truncated != test.truncated || + files.TotalCount != test.total { + t.Fatalf( + "files nodes=%d total=%d truncated=%v, want %d/%d/%v", + len(files.Nodes), files.TotalCount, files.Truncated, + test.wantNodes, test.total, test.truncated, + ) + } + }) + } +} + +func TestPullRequestFilesPaginationRejectsMidObservationSHARace(t *testing.T) { + t.Parallel() + fixture := fakegithub.DefaultFixture() + pull := &fixture.PullRequests[0] + pull.ChangedFiles = make([]fakegithub.ChangedFile, 101) + for index := range pull.ChangedFiles { + pull.ChangedFiles[index] = fakegithub.ChangedFile{ + Path: fmt.Sprintf("old/file-%03d.go", index), ChangeType: "modified", + } + } + fake := fakegithub.New( + fixture, + "secret", + fakegithub.WithRequestHook(func(method, path string, count int, fx *fakegithub.Fixture) { + if method != "POST" || path != "/graphql" || count != 2 { + return + } + fx.PullRequests[0].Head.SHA = "synchronized-head" + fx.PullRequests[0].ChangedFiles[100] = fakegithub.ChangedFile{ + Path: "new/from-synchronize.go", ChangeType: "added", + } + }), + ) + server := httptest.NewServer(fake) + t.Cleanup(server.Close) + client, err := gh.NewGraphQLClient( + server.URL, + budget.New(server.Client(), budget.Options{}), + gh.StaticToken("fake-installation-files-race"), + ) + if err != nil { + t.Fatal(err) + } + _, _, err = client.BatchPullRequests( + context.Background(), + budget.Interactive, + []string{pull.NodeID}, + ) + if err == nil || !strings.Contains(err.Error(), "SHA fence changed") { + t.Fatalf("pagination race error = %v", err) + } +} diff --git a/internal/gh/rest.go b/internal/gh/rest.go index 6779ab3..034d121 100644 --- a/internal/gh/rest.go +++ b/internal/gh/rest.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "io" "net/http" "net/url" "strconv" @@ -15,6 +16,35 @@ import ( "github.com/ewhauser/ghsync/internal/budget" ) +const ( + // MaxCodeownersBytes is GitHub's effective CODEOWNERS size boundary. + // Files at or above this size are not loaded by GitHub. + MaxCodeownersBytes = 3 << 20 +) + +// PullRequestFile is the REST rename supplement for a GraphQL changed file. +type PullRequestFile struct { + Path string `json:"filename"` + PreviousPath string `json:"previous_filename"` + Status string `json:"status"` +} + +// CodeownersSource is the first source found in GitHub's precedence order. +type CodeownersSource struct { + Path string + Content string + State string +} + +const ( + CodeownersPresent = "present" + CodeownersMissing = "missing" + CodeownersOversized = "oversized" + // CodeownersUnavailable means the PR base commit is explicitly unknown, + // so no source can be read without silently falling back to another ref. + CodeownersUnavailable = "unavailable" +) + // StackBase identifies a stack's base ref and commit. type StackBase struct { Ref string `json:"ref"` @@ -209,6 +239,12 @@ type ListCheckRunsOptions struct { Page int } +// ListPullRequestFilesOptions controls changed-file REST pagination. +type ListPullRequestFilesOptions struct { + PerPage int + Page int +} + // ListStacksOptions controls stack filtering and pagination. type ListStacksOptions struct { PullRequest int @@ -438,6 +474,168 @@ func (c *RESTClient) GetPull( return &pull, response, nil } +// ListPullRequestFiles fetches one REST changed-file page. The GraphQL files +// connection remains authoritative; this endpoint supplies previous_filename, +// which the GraphQL PullRequestChangedFile type does not expose. +func (c *RESTClient) ListPullRequestFiles( + ctx context.Context, + class budget.Class, + owner string, + repo string, + number int, + options ListPullRequestFilesOptions, +) ([]PullRequestFile, *RESTResponse, error) { + query := make(url.Values) + setPagination(query, options.PerPage, options.Page) + path := fmt.Sprintf( + "repos/%s/%s/pulls/%d/files", + url.PathEscape(owner), + url.PathEscape(repo), + number, + ) + var files []PullRequestFile + response, err := c.client.getJSON(ctx, class, path, query, "", &files) + return files, response, err +} + +// PullRequestFileRenames follows the REST listing to GitHub's documented +// 3,000-file cap and returns only rename source paths. Truncated is true if a +// cursor remains at the cap. +func (c *RESTClient) PullRequestFileRenames( + ctx context.Context, + class budget.Class, + owner string, + repo string, + number int, +) (map[string]string, bool, error) { + renames := make(map[string]string) + count := 0 + for page := 1; count < MaxPullRequestFiles; page++ { + files, response, err := c.ListPullRequestFiles( + ctx, + class, + owner, + repo, + number, + ListPullRequestFilesOptions{PerPage: 100, Page: page}, + ) + if err != nil { + return nil, false, fmt.Errorf("list PR files page %d: %w", page, err) + } + for _, file := range files { + if count == MaxPullRequestFiles { + break + } + count++ + if strings.EqualFold(file.Status, "renamed") && + file.Path != "" && file.PreviousPath != "" { + renames[file.Path] = file.PreviousPath + } + } + if response.NextPage == 0 { + return renames, false, nil + } + if count == MaxPullRequestFiles { + return renames, true, nil + } + page = response.NextPage - 1 + } + return renames, true, nil +} + +// FindCodeowners reads the first source present at an exact Git ref using +// GitHub's .github, repository-root, then docs precedence. A missing source is +// a successful explicit state; an oversized first source is effective empty +// ownership and does not fall through to lower-precedence files. +func (c *RESTClient) FindCodeowners( + ctx context.Context, + class budget.Class, + owner string, + repo string, + ref string, +) (CodeownersSource, error) { + for _, path := range []string{ + ".github/CODEOWNERS", + "CODEOWNERS", + "docs/CODEOWNERS", + } { + body, status, err := c.getRepositoryContent( + ctx, class, owner, repo, path, ref, + ) + if status == http.StatusNotFound { + continue + } + if err != nil { + return CodeownersSource{}, fmt.Errorf( + "fetch CODEOWNERS %s at %s: %w", path, ref, err, + ) + } + if len(body) >= MaxCodeownersBytes { + return CodeownersSource{ + Path: path, State: CodeownersOversized, + }, nil + } + return CodeownersSource{ + Path: path, Content: string(body), State: CodeownersPresent, + }, nil + } + return CodeownersSource{State: CodeownersMissing}, nil +} + +func (c *RESTClient) getRepositoryContent( + ctx context.Context, + class budget.Class, + owner string, + repo string, + path string, + ref string, +) ([]byte, int, error) { + segments := strings.Split(path, "/") + for index := range segments { + segments[index] = url.PathEscape(segments[index]) + } + query := make(url.Values) + query.Set("ref", ref) + endpoint := fmt.Sprintf( + "repos/%s/%s/contents/%s", + url.PathEscape(owner), + url.PathEscape(repo), + strings.Join(segments, "/"), + ) + req, err := c.client.request(ctx, http.MethodGet, endpoint, query, nil) + if err != nil { + return nil, 0, err + } + req.Header.Set("Accept", "application/vnd.github.raw+json") + gated, err := c.client.gate.Do( + ctx, + class, + budget.NewRESTRequest(req).BeforeSend(c.client.authorize), + ) + if err != nil { + if gated != nil { + _ = closeResponseBody(gated.HTTP) + } + return nil, 0, err + } + response := gated.HTTP + if response.StatusCode < 200 || response.StatusCode > 299 { + status := response.StatusCode + return nil, status, decodeHTTPError(response) + } + defer func() { _ = response.Body.Close() }() + body, err := io.ReadAll(io.LimitReader( + response.Body, + MaxCodeownersBytes+1, + )) + if err != nil { + return nil, response.StatusCode, fmt.Errorf( + "read repository content: %w", err, + ) + } + return body, response.StatusCode, nil +} + // ListCheckRuns fetches one checks page for a head SHA. func (c *RESTClient) ListCheckRuns( ctx context.Context, diff --git a/internal/gh/rest_codeowners_test.go b/internal/gh/rest_codeowners_test.go new file mode 100644 index 0000000..02c4694 --- /dev/null +++ b/internal/gh/rest_codeowners_test.go @@ -0,0 +1,134 @@ +package gh_test + +import ( + "context" + "fmt" + "net/http/httptest" + "strings" + "testing" + + "github.com/ewhauser/ghsync/internal/budget" + "github.com/ewhauser/ghsync/internal/fakegithub" + "github.com/ewhauser/ghsync/internal/gh" +) + +func TestPullRequestFileRenamesPaginationBoundaries(t *testing.T) { + t.Parallel() + for _, count := range []int{101, gh.MaxPullRequestFiles, gh.MaxPullRequestFiles + 1} { + t.Run(fmt.Sprintf("files-%d", count), func(t *testing.T) { + t.Parallel() + fixture := fakegithub.DefaultFixture() + pull := &fixture.PullRequests[0] + pull.ChangedFiles = make([]fakegithub.ChangedFile, count) + for index := range pull.ChangedFiles { + pull.ChangedFiles[index] = fakegithub.ChangedFile{ + Path: fmt.Sprintf("new/file-%04d.go", index), + PreviousPath: fmt.Sprintf("old/file-%04d.go", index), + ChangeType: "renamed", + } + } + server := httptest.NewServer(fakegithub.New(fixture, "secret")) + t.Cleanup(server.Close) + client, err := gh.NewRESTClient( + server.URL, + budget.New(server.Client(), budget.Options{}), + gh.StaticToken("fake-installation-renames"), + ) + if err != nil { + t.Fatal(err) + } + renames, truncated, err := client.PullRequestFileRenames( + context.Background(), budget.Interactive, + fixture.Owner, fixture.Repo, pull.Number, + ) + if err != nil { + t.Fatal(err) + } + want := min(count, gh.MaxPullRequestFiles) + if len(renames) != want || truncated != (count > gh.MaxPullRequestFiles) { + t.Fatalf( + "renames=%d truncated=%v, want %d/%v", + len(renames), truncated, want, + count > gh.MaxPullRequestFiles, + ) + } + }) + } +} + +func TestFindCodeownersPrecedenceMissingAndOversized(t *testing.T) { + t.Parallel() + tests := []struct { + name string + content map[string]string + path string + state string + source string + }{ + { + name: "github directory wins", + content: map[string]string{ + ".github/CODEOWNERS": "* @github", + "CODEOWNERS": "* @root", + "docs/CODEOWNERS": "* @docs", + }, + path: ".github/CODEOWNERS", state: gh.CodeownersPresent, + source: "* @github", + }, + { + name: "root wins over docs", + content: map[string]string{ + "CODEOWNERS": "* @root", + "docs/CODEOWNERS": "* @docs", + }, + path: "CODEOWNERS", state: gh.CodeownersPresent, source: "* @root", + }, + {name: "missing is empty truth", state: gh.CodeownersMissing}, + { + name: "oversized winner does not fall through", + content: map[string]string{ + ".github/CODEOWNERS": strings.Repeat("x", gh.MaxCodeownersBytes), + "CODEOWNERS": "* @root", + }, + path: ".github/CODEOWNERS", state: gh.CodeownersOversized, + }, + { + name: "byte below size boundary remains present", + content: map[string]string{ + "CODEOWNERS": strings.Repeat("x", gh.MaxCodeownersBytes-1), + }, + path: "CODEOWNERS", state: gh.CodeownersPresent, + source: strings.Repeat("x", gh.MaxCodeownersBytes-1), + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + fixture := fakegithub.DefaultFixture() + fixture.Contents = map[string]map[string]string{ + "exact-sha": test.content, + } + server := httptest.NewServer(fakegithub.New(fixture, "secret")) + t.Cleanup(server.Close) + client, err := gh.NewRESTClient( + server.URL, + budget.New(server.Client(), budget.Options{}), + gh.StaticToken("fake-installation-codeowners"), + ) + if err != nil { + t.Fatal(err) + } + source, err := client.FindCodeowners( + context.Background(), budget.Interactive, + fixture.Owner, fixture.Repo, "exact-sha", + ) + if err != nil { + t.Fatal(err) + } + if source.Path != test.path || source.State != test.state || + source.Content != test.source { + t.Fatalf("source = %#v, want %q/%q/%q", source, test.path, test.state, test.source) + } + }) + } +} diff --git a/internal/outbox/outbox.go b/internal/outbox/outbox.go index 9be0f5f..b5e82b7 100644 --- a/internal/outbox/outbox.go +++ b/internal/outbox/outbox.go @@ -76,8 +76,8 @@ type Definition struct { var V1Definitions = []Definition{ {EntitiesStream, RepositoryChangedKind, "repo:{installation_id}:{repo_gh_id}", "repos(installation_id,gh_id)", `{"version":1}`}, {EntitiesStream, RepositoryTombstonedKind, "repo:{installation_id}:{repo_gh_id}", "repos(installation_id,gh_id)", `{"version":1}`}, - {EntitiesStream, PullRequestChangedKind, "pr:{installation_id}:{repo_gh_id}:{pr_number}", "pull_requests(repos.installation_id,repos.gh_id,number), pull_request_review_requests(repo_id,pr_number), pull_request_reviews(repo_id,pr_number), pull_request_comments(repo_id,pr_number)", `{"version":1}`}, - {EntitiesStream, PullRequestTombstonedKind, "pr:{installation_id}:{repo_gh_id}:{pr_number}", "pull_requests(repos.installation_id,repos.gh_id,number), pull_request_review_requests(repo_id,pr_number), pull_request_reviews(repo_id,pr_number), pull_request_comments(repo_id,pr_number)", `{"version":1}`}, + {EntitiesStream, PullRequestChangedKind, "pr:{installation_id}:{repo_gh_id}:{pr_number}", "pull_requests(repos.installation_id,repos.gh_id,number), pull_request_review_requests(repo_id,pr_number), pull_request_reviews(repo_id,pr_number), pull_request_comments(repo_id,pr_number), pull_request_change_snapshots(repo_id,pr_number), pull_request_changed_files(repo_id,pr_number), pull_request_file_owners(repo_id,pr_number)", `{"version":1}`}, + {EntitiesStream, PullRequestTombstonedKind, "pr:{installation_id}:{repo_gh_id}:{pr_number}", "pull_requests(repos.installation_id,repos.gh_id,number), pull_request_review_requests(repo_id,pr_number), pull_request_reviews(repo_id,pr_number), pull_request_comments(repo_id,pr_number), pull_request_change_snapshots(repo_id,pr_number), pull_request_changed_files(repo_id,pr_number), pull_request_file_owners(repo_id,pr_number)", `{"version":1}`}, {EntitiesStream, StackChangedKind, "stack:{installation_id}:{repo_gh_id}:{stack_number}", "stacks(repos.installation_id,repos.gh_id,number)", `{"version":1}`}, {EntitiesStream, StackTombstonedKind, "stack:{installation_id}:{repo_gh_id}:{stack_number}", "stacks(repos.installation_id,repos.gh_id,number)", `{"version":1}`}, {EntitiesStream, ChecksChangedKind, "checks:{installation_id}:{repo_gh_id}:{head_sha}", "check_runs(repos.installation_id,repos.gh_id,head_sha)", `{"version":1}`}, diff --git a/internal/store/branch.go b/internal/store/branch.go index a74de66..6973f77 100644 --- a/internal/store/branch.go +++ b/internal/store/branch.go @@ -54,7 +54,6 @@ func (w *EntityWriter) BranchTargets( fmt.Sprintf("stack:%s:%d", repoFullName, number), ) } - continue } targets = append( targets, diff --git a/internal/store/cache_db_test.go b/internal/store/cache_db_test.go index 55a70d1..4002442 100644 --- a/internal/store/cache_db_test.go +++ b/internal/store/cache_db_test.go @@ -915,6 +915,68 @@ func TestWriteRaceBothOrdersNewerWinsConcurrently(t *testing.T) { } } +func TestRepositoryUnknownHeadSHACannotEraseKnownValue(t *testing.T) { + t.Parallel() + pool, _ := storeTestDatabase(t) + writer := NewEntityWriter(pool) + updatedAt := time.Date(2026, 7, 28, 12, 0, 0, 0, time.UTC) + repository := storeTestRepository("acme/repo-head-sentinel", 2110, updatedAt) + if _, err := writer.ApplyRepository( + t.Context(), repository, SyncSourceWebhook, "", updatedAt, + ); err != nil { + t.Fatal(err) + } + repository.DefaultHeadSHA = "" + repository.GitHubUpdatedAt = updatedAt.Add(time.Minute) + if _, err := writer.ApplyRepository( + t.Context(), repository, SyncSourceWebhook, "", updatedAt.Add(time.Minute), + ); err != nil { + t.Fatal(err) + } + var headSHA string + if err := pool.QueryRow(t.Context(), ` + SELECT head_sha FROM repos WHERE gh_id = $1 + `, repository.GitHubID).Scan(&headSHA); err != nil { + t.Fatal(err) + } + if headSHA != "base" { + t.Fatalf("repository head SHA = %q, want preserved known value", headSHA) + } +} + +func TestBranchTargetsRefreshOnlyOpenPullsOnAffectedBranch(t *testing.T) { + t.Parallel() + pool, _ := storeTestDatabase(t) + writer := NewEntityWriter(pool) + now := time.Date(2026, 8, 3, 12, 0, 0, 0, time.UTC) + repository := storeTestRepository("acme/codeowners-refresh", 2111, now) + pulls := []PullRequestRecord{ + storeTestPull(&repository, now, "open-main-head"), + storeTestPull(&repository, now, "closed-main-head"), + storeTestPull(&repository, now, "open-other-head"), + } + pulls[0].Number, pulls[0].GitHubID, pulls[0].NodeID = 41, 4100, "pr-41" + pulls[1].Number, pulls[1].GitHubID, pulls[1].NodeID = 42, 4200, "pr-42" + pulls[1].State = "closed" + pulls[2].Number, pulls[2].GitHubID, pulls[2].NodeID = 43, 4300, "pr-43" + pulls[2].BaseRef = "release" + for index := range pulls { + if _, err := writer.ApplyPullRequest(t.Context(), pulls[index]); err != nil { + t.Fatal(err) + } + } + targets, err := writer.BranchTargets( + t.Context(), repository.FullName, repository.DefaultBranch, + ) + if err != nil { + t.Fatal(err) + } + want := []string{"pr:acme/codeowners-refresh:41"} + if !reflect.DeepEqual(targets, want) { + t.Fatalf("default-branch targets = %#v, want %#v", targets, want) + } +} + func TestEqualTimestampDomainChangeAndTombstoneResurrection(t *testing.T) { t.Parallel() pool, _ := storeTestDatabase(t) diff --git a/internal/store/codeowners.go b/internal/store/codeowners.go new file mode 100644 index 0000000..c7a362b --- /dev/null +++ b/internal/store/codeowners.go @@ -0,0 +1,68 @@ +package store + +import ( + "context" + "fmt" + "strings" + + "github.com/ewhauser/ghsync/internal/store/dbgen" +) + +// ResolveFileOwnerIdentities fills stable identities from facts already +// mirrored for this repository. It never performs a live GitHub lookup and +// leaves unknown users, teams, email owners, and malformed tokens explicit. +func (w *EntityWriter) ResolveFileOwnerIdentities( + ctx context.Context, + repoGitHubID int64, + repositoryOwner string, + owners []FileOwnerRecord, +) ([]FileOwnerRecord, error) { + repo, err := dbgen.New(w.pool).GetRepoByGitHubID(ctx, repoGitHubID) + if err != nil { + return nil, fmt.Errorf("find CODEOWNERS repository: %w", err) + } + identities, err := dbgen.New(w.pool).ListCodeOwnerIdentities(ctx, repo.ID) + if err != nil { + return nil, fmt.Errorf("list known CODEOWNERS identities: %w", err) + } + type identity struct { + githubID int64 + nodeID string + login string + } + known := make(map[string]identity, len(identities)) + for _, item := range identities { + known[item.OwnerType+"\x00"+strings.ToLower(item.OwnerLogin)] = identity{ + githubID: item.OwnerGhID, + nodeID: item.OwnerNodeID, + login: item.OwnerLogin, + } + } + resolved := append([]FileOwnerRecord(nil), owners...) + for index := range resolved { + owner := &resolved[index] + owner.ResolutionState = "unresolved" + lookup := owner.OwnerName + switch owner.OwnerType { + case "user": + case "team": + parts := strings.SplitN(owner.OwnerName, "/", 2) + if len(parts) != 2 || + !strings.EqualFold(parts[0], repositoryOwner) { + continue + } + lookup = parts[1] + default: + continue + } + identity, ok := known[owner.OwnerType+"\x00"+strings.ToLower(lookup)] + if !ok { + continue + } + owner.ResolutionState = "resolved" + owner.OwnerGitHubID = identity.githubID + owner.OwnerNodeID = identity.nodeID + owner.OwnerLogin = identity.login + } + return resolved, nil +} diff --git a/internal/store/dbgen/cache.sql.go b/internal/store/dbgen/cache.sql.go index 07906ab..a88df7b 100644 --- a/internal/store/dbgen/cache.sql.go +++ b/internal/store/dbgen/cache.sql.go @@ -643,6 +643,77 @@ func (q *Queries) ListCachedPRMemberships(ctx context.Context, arg ListCachedPRM return items, nil } +const listCodeOwnerIdentities = `-- name: ListCodeOwnerIdentities :many +WITH candidates AS ( + SELECT request.reviewer_kind AS owner_type, + request.reviewer_gh_id AS owner_gh_id, + request.reviewer_node_id AS owner_node_id, + request.reviewer_login AS owner_login, + request.last_checked_at + FROM pull_request_review_requests AS request + WHERE request.repo_id = $1 + + UNION ALL + + SELECT 'user'::text, NULL::bigint, review.author_node_id, + review.author_login, review.last_checked_at + FROM pull_request_reviews AS review + WHERE review.repo_id = $1 + AND review.author_kind = 'user' + AND review.author_node_id IS NOT NULL + AND review.author_login IS NOT NULL + + UNION ALL + + SELECT 'user'::text, NULL::bigint, comment.author_node_id, + comment.author_login, comment.last_checked_at + FROM pull_request_comments AS comment + WHERE comment.repo_id = $1 + AND comment.author_kind = 'user' + AND comment.author_node_id IS NOT NULL + AND comment.author_login IS NOT NULL +) +SELECT DISTINCT ON (owner_type, lower(owner_login)) + owner_type, COALESCE(owner_gh_id, 0)::bigint AS owner_gh_id, + owner_node_id, owner_login +FROM candidates +ORDER BY owner_type, lower(owner_login), + owner_gh_id IS NOT NULL DESC, last_checked_at DESC, + owner_node_id +` + +type ListCodeOwnerIdentitiesRow struct { + OwnerType string + OwnerGhID int64 + OwnerNodeID string + OwnerLogin string +} + +func (q *Queries) ListCodeOwnerIdentities(ctx context.Context, repoID int64) ([]ListCodeOwnerIdentitiesRow, error) { + rows, err := q.db.Query(ctx, listCodeOwnerIdentities, repoID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListCodeOwnerIdentitiesRow + for rows.Next() { + var i ListCodeOwnerIdentitiesRow + if err := rows.Scan( + &i.OwnerType, + &i.OwnerGhID, + &i.OwnerNodeID, + &i.OwnerLogin, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listPRScopesByHeadSHA = `-- name: ListPRScopesByHeadSHA :many SELECT pull_requests.number, pull_requests.stack_number, repos.gh_id AS repo_gh_id, repos.installation_id @@ -698,6 +769,7 @@ JOIN repos ON repos.id = pull_requests.repo_id JOIN repo_aliases ON repo_aliases.repo_id = repos.id WHERE repo_aliases.full_name = $1 AND pull_requests.tombstoned_at IS NULL + AND pull_requests.state = 'open' AND ( pull_requests.head_ref = $2 OR pull_requests.base_ref = $2 @@ -978,6 +1050,123 @@ func (q *Queries) ReplaceCheckRuns(ctx context.Context, arg ReplaceCheckRunsPara return items, nil } +const replacePullRequestChangedFiles = `-- name: ReplacePullRequestChangedFiles :many +WITH input AS ( + SELECT element->>'path' AS path, + NULLIF(element->>'previous_path', '') AS previous_path, + element->>'change_type' AS change_type + FROM jsonb_array_elements($1::jsonb) AS element +), +eligible AS ( + SELECT snapshot.repo_id + FROM pull_request_change_snapshots AS snapshot + WHERE snapshot.repo_id = $2 + AND snapshot.pr_number = $3 + AND snapshot.tombstoned_at IS NULL + AND snapshot.base_sha = $4 + AND snapshot.head_sha = $5 + AND snapshot.parent_gh_updated_at <= $6 +), +upserted AS ( + INSERT INTO pull_request_changed_files ( + repo_id, pr_number, path, previous_path, change_type, base_sha, + head_sha, synced_at, etag, sync_source, tombstoned_at, + last_checked_at + ) + SELECT $2, $3, input.path, + input.previous_path, input.change_type, $4, + $5, $7, $8, + $9, NULL, $10 + FROM input + CROSS JOIN eligible + ON CONFLICT (repo_id, pr_number, path) DO UPDATE + SET previous_path = EXCLUDED.previous_path, + change_type = EXCLUDED.change_type, + base_sha = EXCLUDED.base_sha, + head_sha = EXCLUDED.head_sha, + synced_at = EXCLUDED.synced_at, + etag = EXCLUDED.etag, + sync_source = EXCLUDED.sync_source, + tombstoned_at = NULL, + last_checked_at = EXCLUDED.last_checked_at + WHERE ROW( + EXCLUDED.previous_path, EXCLUDED.change_type, + EXCLUDED.base_sha, EXCLUDED.head_sha + ) IS DISTINCT FROM ROW( + pull_request_changed_files.previous_path, + pull_request_changed_files.change_type, + pull_request_changed_files.base_sha, + pull_request_changed_files.head_sha + ) + OR pull_request_changed_files.tombstoned_at IS NOT NULL + RETURNING path +), +tombstoned AS ( + UPDATE pull_request_changed_files + SET tombstoned_at = $10, + synced_at = $7, + last_checked_at = $10, + etag = $8, + sync_source = $9 + WHERE repo_id = $2 + AND pr_number = $3 + AND tombstoned_at IS NULL + AND EXISTS (SELECT 1 FROM eligible) + AND NOT EXISTS ( + SELECT 1 FROM input + WHERE input.path = pull_request_changed_files.path + ) + RETURNING path +) +SELECT path FROM upserted +UNION ALL +SELECT path FROM tombstoned +` + +type ReplacePullRequestChangedFilesParams struct { + ChangedFiles []byte + RepoID int64 + PrNumber int32 + BaseSha string + HeadSha string + ParentGhUpdatedAt pgtype.Timestamptz + SyncedAt pgtype.Timestamptz + Etag string + SyncSource string + LastCheckedAt pgtype.Timestamptz +} + +func (q *Queries) ReplacePullRequestChangedFiles(ctx context.Context, arg ReplacePullRequestChangedFilesParams) ([]string, error) { + rows, err := q.db.Query(ctx, replacePullRequestChangedFiles, + arg.ChangedFiles, + arg.RepoID, + arg.PrNumber, + arg.BaseSha, + arg.HeadSha, + arg.ParentGhUpdatedAt, + arg.SyncedAt, + arg.Etag, + arg.SyncSource, + arg.LastCheckedAt, + ) + if err != nil { + return nil, err + } + defer rows.Close() + var items []string + for rows.Next() { + var path string + if err := rows.Scan(&path); err != nil { + return nil, err + } + items = append(items, path) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const replacePullRequestComments = `-- name: ReplacePullRequestComments :many WITH input AS ( SELECT NULLIF((element->>'gh_id')::bigint, 0) AS gh_id, @@ -1125,6 +1314,150 @@ func (q *Queries) ReplacePullRequestComments(ctx context.Context, arg ReplacePul return items, nil } +const replacePullRequestFileOwners = `-- name: ReplacePullRequestFileOwners :many +WITH input AS ( + SELECT element->>'path' AS path, + element->>'owner_token' AS owner_token, + element->>'owner_type' AS owner_type, + element->>'owner_name' AS owner_name, + element->>'resolution_state' AS resolution_state, + NULLIF((element->>'owner_gh_id')::bigint, 0) AS owner_gh_id, + NULLIF(element->>'owner_node_id', '') AS owner_node_id, + NULLIF(element->>'owner_login', '') AS owner_login, + element->>'source_pattern' AS source_pattern, + (element->>'source_line')::integer AS source_line + FROM jsonb_array_elements($1::jsonb) AS element +), +eligible AS ( + SELECT snapshot.repo_id + FROM pull_request_change_snapshots AS snapshot + WHERE snapshot.repo_id = $2 + AND snapshot.pr_number = $3 + AND snapshot.tombstoned_at IS NULL + AND snapshot.base_sha = $4 + AND snapshot.head_sha = $5 + AND snapshot.parent_gh_updated_at <= $6 +), +upserted AS ( + INSERT INTO pull_request_file_owners ( + repo_id, pr_number, path, owner_token, owner_type, owner_name, + resolution_state, owner_gh_id, owner_node_id, owner_login, + source_pattern, source_line, base_sha, head_sha, synced_at, etag, + sync_source, tombstoned_at, last_checked_at + ) + SELECT $2, $3, input.path, + input.owner_token, input.owner_type, input.owner_name, + input.resolution_state, + input.owner_gh_id, input.owner_node_id, input.owner_login, + input.source_pattern, input.source_line, $4, + $5, $7, $8, + $9, NULL, $10 + FROM input + CROSS JOIN eligible + ON CONFLICT (repo_id, pr_number, path, owner_token) DO UPDATE + SET owner_type = EXCLUDED.owner_type, + owner_name = EXCLUDED.owner_name, + resolution_state = EXCLUDED.resolution_state, + owner_gh_id = EXCLUDED.owner_gh_id, + owner_node_id = EXCLUDED.owner_node_id, + owner_login = EXCLUDED.owner_login, + source_pattern = EXCLUDED.source_pattern, + source_line = EXCLUDED.source_line, + base_sha = EXCLUDED.base_sha, + head_sha = EXCLUDED.head_sha, + synced_at = EXCLUDED.synced_at, + etag = EXCLUDED.etag, + sync_source = EXCLUDED.sync_source, + tombstoned_at = NULL, + last_checked_at = EXCLUDED.last_checked_at + WHERE ROW( + EXCLUDED.owner_type, EXCLUDED.owner_name, + EXCLUDED.resolution_state, + EXCLUDED.owner_gh_id, EXCLUDED.owner_node_id, + EXCLUDED.owner_login, EXCLUDED.source_pattern, + EXCLUDED.source_line, EXCLUDED.base_sha, EXCLUDED.head_sha + ) IS DISTINCT FROM ROW( + pull_request_file_owners.owner_type, + pull_request_file_owners.owner_name, + pull_request_file_owners.resolution_state, + pull_request_file_owners.owner_gh_id, + pull_request_file_owners.owner_node_id, + pull_request_file_owners.owner_login, + pull_request_file_owners.source_pattern, + pull_request_file_owners.source_line, + pull_request_file_owners.base_sha, + pull_request_file_owners.head_sha + ) + OR pull_request_file_owners.tombstoned_at IS NOT NULL + RETURNING (path || ':' || owner_token)::text AS owner_key +), +tombstoned AS ( + UPDATE pull_request_file_owners + SET tombstoned_at = $10, + synced_at = $7, + last_checked_at = $10, + etag = $8, + sync_source = $9 + WHERE repo_id = $2 + AND pr_number = $3 + AND tombstoned_at IS NULL + AND EXISTS (SELECT 1 FROM eligible) + AND NOT EXISTS ( + SELECT 1 FROM input + WHERE input.path = pull_request_file_owners.path + AND input.owner_token = pull_request_file_owners.owner_token + ) + RETURNING (path || ':' || owner_token)::text AS owner_key +) +SELECT owner_key FROM upserted +UNION ALL +SELECT owner_key FROM tombstoned +` + +type ReplacePullRequestFileOwnersParams struct { + FileOwners []byte + RepoID int64 + PrNumber int32 + BaseSha string + HeadSha string + ParentGhUpdatedAt pgtype.Timestamptz + SyncedAt pgtype.Timestamptz + Etag string + SyncSource string + LastCheckedAt pgtype.Timestamptz +} + +func (q *Queries) ReplacePullRequestFileOwners(ctx context.Context, arg ReplacePullRequestFileOwnersParams) ([]string, error) { + rows, err := q.db.Query(ctx, replacePullRequestFileOwners, + arg.FileOwners, + arg.RepoID, + arg.PrNumber, + arg.BaseSha, + arg.HeadSha, + arg.ParentGhUpdatedAt, + arg.SyncedAt, + arg.Etag, + arg.SyncSource, + arg.LastCheckedAt, + ) + if err != nil { + return nil, err + } + defer rows.Close() + var items []string + for rows.Next() { + var owner_key string + if err := rows.Scan(&owner_key); err != nil { + return nil, err + } + items = append(items, owner_key) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const replacePullRequestReviewRequests = `-- name: ReplacePullRequestReviewRequests :many WITH input AS ( SELECT element->>'kind' AS reviewer_kind, @@ -1757,6 +2090,70 @@ func (q *Queries) TombstonePullRequest(ctx context.Context, arg TombstonePullReq return i, err } +const tombstonePullRequestChangeSnapshot = `-- name: TombstonePullRequestChangeSnapshot :execrows +UPDATE pull_request_change_snapshots +SET tombstoned_at = $1, + synced_at = $1, + last_checked_at = GREATEST(last_checked_at, $1), + etag = '', + sync_source = $2 +WHERE repo_id = $3 + AND pr_number = $4 + AND tombstoned_at IS NULL +` + +type TombstonePullRequestChangeSnapshotParams struct { + TombstonedAt pgtype.Timestamptz + SyncSource string + RepoID int64 + PrNumber int32 +} + +func (q *Queries) TombstonePullRequestChangeSnapshot(ctx context.Context, arg TombstonePullRequestChangeSnapshotParams) (int64, error) { + result, err := q.db.Exec(ctx, tombstonePullRequestChangeSnapshot, + arg.TombstonedAt, + arg.SyncSource, + arg.RepoID, + arg.PrNumber, + ) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + +const tombstonePullRequestChangedFiles = `-- name: TombstonePullRequestChangedFiles :execrows +UPDATE pull_request_changed_files +SET tombstoned_at = $1, + synced_at = $1, + last_checked_at = GREATEST(last_checked_at, $1), + etag = '', + sync_source = $2 +WHERE repo_id = $3 + AND pr_number = $4 + AND tombstoned_at IS NULL +` + +type TombstonePullRequestChangedFilesParams struct { + TombstonedAt pgtype.Timestamptz + SyncSource string + RepoID int64 + PrNumber int32 +} + +func (q *Queries) TombstonePullRequestChangedFiles(ctx context.Context, arg TombstonePullRequestChangedFilesParams) (int64, error) { + result, err := q.db.Exec(ctx, tombstonePullRequestChangedFiles, + arg.TombstonedAt, + arg.SyncSource, + arg.RepoID, + arg.PrNumber, + ) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + const tombstonePullRequestComments = `-- name: TombstonePullRequestComments :many UPDATE pull_request_comments SET tombstoned_at = $1, @@ -1802,6 +2199,38 @@ func (q *Queries) TombstonePullRequestComments(ctx context.Context, arg Tombston return items, nil } +const tombstonePullRequestFileOwners = `-- name: TombstonePullRequestFileOwners :execrows +UPDATE pull_request_file_owners +SET tombstoned_at = $1, + synced_at = $1, + last_checked_at = GREATEST(last_checked_at, $1), + etag = '', + sync_source = $2 +WHERE repo_id = $3 + AND pr_number = $4 + AND tombstoned_at IS NULL +` + +type TombstonePullRequestFileOwnersParams struct { + TombstonedAt pgtype.Timestamptz + SyncSource string + RepoID int64 + PrNumber int32 +} + +func (q *Queries) TombstonePullRequestFileOwners(ctx context.Context, arg TombstonePullRequestFileOwnersParams) (int64, error) { + result, err := q.db.Exec(ctx, tombstonePullRequestFileOwners, + arg.TombstonedAt, + arg.SyncSource, + arg.RepoID, + arg.PrNumber, + ) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + const tombstonePullRequestReviewRequests = `-- name: TombstonePullRequestReviewRequests :many UPDATE pull_request_review_requests SET tombstoned_at = $1, @@ -2028,6 +2457,79 @@ func (q *Queries) TouchCheckRunsCheckedAt(ctx context.Context, arg TouchCheckRun return err } +const touchPullRequestChangeInputsCheckedAt = `-- name: TouchPullRequestChangeInputsCheckedAt :exec +WITH eligible AS ( + SELECT snapshot.repo_id + FROM pull_request_change_snapshots AS snapshot + WHERE snapshot.repo_id = $3 + AND snapshot.pr_number = $4 + AND snapshot.tombstoned_at IS NULL + AND snapshot.parent_gh_updated_at <= $5 +) +UPDATE pull_request_change_snapshots AS snapshot +SET last_checked_at = GREATEST(snapshot.last_checked_at, $1), + etag = CASE WHEN $2::text = '' THEN snapshot.etag + ELSE $2::text END +WHERE snapshot.repo_id = $3 + AND snapshot.pr_number = $4 + AND EXISTS (SELECT 1 FROM eligible) +` + +type TouchPullRequestChangeInputsCheckedAtParams struct { + CheckedAt pgtype.Timestamptz + Etag string + RepoID int64 + PrNumber int32 + ParentGhUpdatedAt pgtype.Timestamptz +} + +func (q *Queries) TouchPullRequestChangeInputsCheckedAt(ctx context.Context, arg TouchPullRequestChangeInputsCheckedAtParams) error { + _, err := q.db.Exec(ctx, touchPullRequestChangeInputsCheckedAt, + arg.CheckedAt, + arg.Etag, + arg.RepoID, + arg.PrNumber, + arg.ParentGhUpdatedAt, + ) + return err +} + +const touchPullRequestChangedFilesCheckedAt = `-- name: TouchPullRequestChangedFilesCheckedAt :exec +UPDATE pull_request_changed_files AS file +SET last_checked_at = GREATEST(file.last_checked_at, $1), + etag = CASE WHEN $2::text = '' THEN file.etag + ELSE $2::text END +WHERE file.repo_id = $3 + AND file.pr_number = $4 + AND file.tombstoned_at IS NULL + AND EXISTS ( + SELECT 1 FROM pull_request_change_snapshots AS snapshot + WHERE snapshot.repo_id = $3 + AND snapshot.pr_number = $4 + AND snapshot.tombstoned_at IS NULL + AND snapshot.parent_gh_updated_at <= $5 + ) +` + +type TouchPullRequestChangedFilesCheckedAtParams struct { + CheckedAt pgtype.Timestamptz + Etag string + RepoID int64 + PrNumber int32 + ParentGhUpdatedAt pgtype.Timestamptz +} + +func (q *Queries) TouchPullRequestChangedFilesCheckedAt(ctx context.Context, arg TouchPullRequestChangedFilesCheckedAtParams) error { + _, err := q.db.Exec(ctx, touchPullRequestChangedFilesCheckedAt, + arg.CheckedAt, + arg.Etag, + arg.RepoID, + arg.PrNumber, + arg.ParentGhUpdatedAt, + ) + return err +} + const touchPullRequestCheckedAt = `-- name: TouchPullRequestCheckedAt :exec UPDATE pull_requests SET last_checked_at = GREATEST(last_checked_at, $1), @@ -2096,6 +2598,42 @@ func (q *Queries) TouchPullRequestCommentsCheckedAt(ctx context.Context, arg Tou return err } +const touchPullRequestFileOwnersCheckedAt = `-- name: TouchPullRequestFileOwnersCheckedAt :exec +UPDATE pull_request_file_owners AS owner +SET last_checked_at = GREATEST(owner.last_checked_at, $1), + etag = CASE WHEN $2::text = '' THEN owner.etag + ELSE $2::text END +WHERE owner.repo_id = $3 + AND owner.pr_number = $4 + AND owner.tombstoned_at IS NULL + AND EXISTS ( + SELECT 1 FROM pull_request_change_snapshots AS snapshot + WHERE snapshot.repo_id = $3 + AND snapshot.pr_number = $4 + AND snapshot.tombstoned_at IS NULL + AND snapshot.parent_gh_updated_at <= $5 + ) +` + +type TouchPullRequestFileOwnersCheckedAtParams struct { + CheckedAt pgtype.Timestamptz + Etag string + RepoID int64 + PrNumber int32 + ParentGhUpdatedAt pgtype.Timestamptz +} + +func (q *Queries) TouchPullRequestFileOwnersCheckedAt(ctx context.Context, arg TouchPullRequestFileOwnersCheckedAtParams) error { + _, err := q.db.Exec(ctx, touchPullRequestFileOwnersCheckedAt, + arg.CheckedAt, + arg.Etag, + arg.RepoID, + arg.PrNumber, + arg.ParentGhUpdatedAt, + ) + return err +} + const touchPullRequestReviewRequestsCheckedAt = `-- name: TouchPullRequestReviewRequestsCheckedAt :exec UPDATE pull_request_review_requests SET last_checked_at = GREATEST( @@ -2261,6 +2799,214 @@ func (q *Queries) TouchStackCheckedAt(ctx context.Context, arg TouchStackChecked return err } +const upsertPullRequestChangeSnapshot = `-- name: UpsertPullRequestChangeSnapshot :one +WITH eligible AS ( + SELECT pull_requests.repo_id + FROM pull_requests + WHERE pull_requests.repo_id = $11 + AND pull_requests.number = $12 + AND pull_requests.tombstoned_at IS NULL + AND pull_requests.head_sha = $2 + AND pull_requests.base_sha = $1 + AND ( + pull_requests.gh_updated_at IS NULL + OR pull_requests.gh_updated_at <= $13 + ) +), +prior AS MATERIALIZED ( + SELECT base_sha, head_sha, files_total_count, files_truncated, + codeowners_ref, codeowners_sha, codeowners_path, + codeowners_state, codeowners_source, codeowners_hash, + tombstoned_at + FROM pull_request_change_snapshots + WHERE repo_id = $11 + AND pr_number = $12 +), +upserted AS ( + INSERT INTO pull_request_change_snapshots ( + repo_id, pr_number, base_sha, head_sha, files_total_count, + files_truncated, codeowners_ref, codeowners_sha, codeowners_path, + codeowners_state, codeowners_source, codeowners_hash, + parent_gh_updated_at, synced_at, etag, sync_source, tombstoned_at, + last_checked_at + ) + SELECT $11, $12, $1, + $2, $3, + $4, $5, + $6, $7, + $8, $9, + $10, $13, + $14, $15, $16, NULL, + $17 + FROM eligible + ON CONFLICT (repo_id, pr_number) DO UPDATE + SET base_sha = EXCLUDED.base_sha, + head_sha = EXCLUDED.head_sha, + files_total_count = EXCLUDED.files_total_count, + files_truncated = EXCLUDED.files_truncated, + codeowners_ref = EXCLUDED.codeowners_ref, + codeowners_sha = EXCLUDED.codeowners_sha, + codeowners_path = EXCLUDED.codeowners_path, + codeowners_state = EXCLUDED.codeowners_state, + codeowners_source = EXCLUDED.codeowners_source, + codeowners_hash = EXCLUDED.codeowners_hash, + parent_gh_updated_at = EXCLUDED.parent_gh_updated_at, + synced_at = CASE + WHEN pull_request_change_snapshots.tombstoned_at IS NOT NULL + OR ROW( + EXCLUDED.base_sha, EXCLUDED.head_sha, + EXCLUDED.files_total_count, EXCLUDED.files_truncated, + EXCLUDED.codeowners_ref, EXCLUDED.codeowners_sha, + EXCLUDED.codeowners_path, EXCLUDED.codeowners_state, + EXCLUDED.codeowners_source, EXCLUDED.codeowners_hash + ) IS DISTINCT FROM ROW( + pull_request_change_snapshots.base_sha, + pull_request_change_snapshots.head_sha, + pull_request_change_snapshots.files_total_count, + pull_request_change_snapshots.files_truncated, + pull_request_change_snapshots.codeowners_ref, + pull_request_change_snapshots.codeowners_sha, + pull_request_change_snapshots.codeowners_path, + pull_request_change_snapshots.codeowners_state, + pull_request_change_snapshots.codeowners_source, + pull_request_change_snapshots.codeowners_hash + ) + THEN EXCLUDED.synced_at + ELSE pull_request_change_snapshots.synced_at + END, + etag = EXCLUDED.etag, + sync_source = CASE + WHEN pull_request_change_snapshots.tombstoned_at IS NOT NULL + OR ROW( + EXCLUDED.base_sha, EXCLUDED.head_sha, + EXCLUDED.files_total_count, EXCLUDED.files_truncated, + EXCLUDED.codeowners_ref, EXCLUDED.codeowners_sha, + EXCLUDED.codeowners_path, EXCLUDED.codeowners_state, + EXCLUDED.codeowners_source, EXCLUDED.codeowners_hash + ) IS DISTINCT FROM ROW( + pull_request_change_snapshots.base_sha, + pull_request_change_snapshots.head_sha, + pull_request_change_snapshots.files_total_count, + pull_request_change_snapshots.files_truncated, + pull_request_change_snapshots.codeowners_ref, + pull_request_change_snapshots.codeowners_sha, + pull_request_change_snapshots.codeowners_path, + pull_request_change_snapshots.codeowners_state, + pull_request_change_snapshots.codeowners_source, + pull_request_change_snapshots.codeowners_hash + ) + THEN EXCLUDED.sync_source + ELSE pull_request_change_snapshots.sync_source + END, + tombstoned_at = NULL, + last_checked_at = EXCLUDED.last_checked_at + WHERE EXCLUDED.parent_gh_updated_at > + pull_request_change_snapshots.parent_gh_updated_at + OR ( + EXCLUDED.parent_gh_updated_at = + pull_request_change_snapshots.parent_gh_updated_at + AND ROW( + EXCLUDED.base_sha, EXCLUDED.head_sha, + EXCLUDED.files_total_count, EXCLUDED.files_truncated, + EXCLUDED.codeowners_ref, EXCLUDED.codeowners_sha, + EXCLUDED.codeowners_path, EXCLUDED.codeowners_state, + EXCLUDED.codeowners_source, EXCLUDED.codeowners_hash + ) IS DISTINCT FROM ROW( + pull_request_change_snapshots.base_sha, + pull_request_change_snapshots.head_sha, + pull_request_change_snapshots.files_total_count, + pull_request_change_snapshots.files_truncated, + pull_request_change_snapshots.codeowners_ref, + pull_request_change_snapshots.codeowners_sha, + pull_request_change_snapshots.codeowners_path, + pull_request_change_snapshots.codeowners_state, + pull_request_change_snapshots.codeowners_source, + pull_request_change_snapshots.codeowners_hash + ) + ) + OR ( + pull_request_change_snapshots.tombstoned_at IS NOT NULL + AND EXCLUDED.last_checked_at > + pull_request_change_snapshots.tombstoned_at + ) + RETURNING repo_id +) +SELECT count(*) +FROM upserted +WHERE NOT EXISTS (SELECT 1 FROM prior) + OR EXISTS ( + SELECT 1 + FROM prior + WHERE prior.tombstoned_at IS NOT NULL + OR ROW( + prior.base_sha, prior.head_sha, prior.files_total_count, + prior.files_truncated, prior.codeowners_ref, + prior.codeowners_sha, prior.codeowners_path, + prior.codeowners_state, prior.codeowners_source, + prior.codeowners_hash + ) IS DISTINCT FROM ROW( + $1::text, $2::text, + $3::integer, + $4::boolean, + $5::text, + $6::text, + $7::text, + $8::text, + $9::text, + $10::text + ) + ) +` + +type UpsertPullRequestChangeSnapshotParams struct { + BaseSha string + HeadSha string + FilesTotalCount int32 + FilesTruncated bool + CodeownersRef string + CodeownersSha string + CodeownersPath pgtype.Text + CodeownersState string + CodeownersSource pgtype.Text + CodeownersHash string + RepoID int64 + PrNumber int32 + ParentGhUpdatedAt pgtype.Timestamptz + SyncedAt pgtype.Timestamptz + Etag string + SyncSource string + LastCheckedAt pgtype.Timestamptz +} + +// C-C2 parent freshness plus explicit base/head identity gates the current +// changed-file and ownership snapshot. Equal parent versions remain eligible +// so a base-branch CODEOWNERS push or a later complete listing can heal facts +// without relying on pull_request.updatedAt changing. +func (q *Queries) UpsertPullRequestChangeSnapshot(ctx context.Context, arg UpsertPullRequestChangeSnapshotParams) (int64, error) { + row := q.db.QueryRow(ctx, upsertPullRequestChangeSnapshot, + arg.BaseSha, + arg.HeadSha, + arg.FilesTotalCount, + arg.FilesTruncated, + arg.CodeownersRef, + arg.CodeownersSha, + arg.CodeownersPath, + arg.CodeownersState, + arg.CodeownersSource, + arg.CodeownersHash, + arg.RepoID, + arg.PrNumber, + arg.ParentGhUpdatedAt, + arg.SyncedAt, + arg.Etag, + arg.SyncSource, + arg.LastCheckedAt, + ) + var count int64 + err := row.Scan(&count) + return count, err +} + const upsertPullRequestWriteIfNewer = `-- name: UpsertPullRequestWriteIfNewer :one INSERT INTO pull_requests ( repo_id, gh_id, node_id, number, title, state, draft, author_login, @@ -2508,7 +3254,8 @@ SET installation_id = EXCLUDED.installation_id, default_branch = EXCLUDED.default_branch, archived = EXCLUDED.archived, gh_updated_at = EXCLUDED.gh_updated_at, - head_sha = EXCLUDED.head_sha, + head_sha = CASE WHEN EXCLUDED.head_sha = '' THEN repos.head_sha + ELSE EXCLUDED.head_sha END, synced_at = EXCLUDED.synced_at, last_checked_at = EXCLUDED.last_checked_at, etag = EXCLUDED.etag, @@ -2521,7 +3268,9 @@ WHERE repos.gh_updated_at IS NULL AND ROW( EXCLUDED.installation_id, EXCLUDED.org_id, EXCLUDED.node_id, EXCLUDED.owner, EXCLUDED.name, EXCLUDED.full_name, - EXCLUDED.default_branch, EXCLUDED.archived, EXCLUDED.head_sha + EXCLUDED.default_branch, EXCLUDED.archived, + CASE WHEN EXCLUDED.head_sha = '' THEN repos.head_sha + ELSE EXCLUDED.head_sha END ) IS DISTINCT FROM ROW( repos.installation_id, repos.org_id, repos.node_id, repos.owner, repos.name, repos.full_name, diff --git a/internal/store/dbgen/loadgen.sql.go b/internal/store/dbgen/loadgen.sql.go index 2a4846c..dc642c7 100644 --- a/internal/store/dbgen/loadgen.sql.go +++ b/internal/store/dbgen/loadgen.sql.go @@ -181,6 +181,113 @@ func (q *Queries) ListLoadgenCachedCheckRuns(ctx context.Context, repoFullName s return items, nil } +const listLoadgenCachedPullRequestChangeSnapshots = `-- name: ListLoadgenCachedPullRequestChangeSnapshots :many +SELECT snapshot.pr_number, snapshot.base_sha, snapshot.head_sha, + snapshot.files_total_count, snapshot.files_truncated, + snapshot.codeowners_ref, snapshot.codeowners_sha, + snapshot.codeowners_path, snapshot.codeowners_state, + snapshot.codeowners_source, snapshot.codeowners_hash +FROM pull_request_change_snapshots AS snapshot +JOIN repos AS repo ON repo.id = snapshot.repo_id +WHERE repo.full_name = $1 + AND repo.tombstoned_at IS NULL + AND snapshot.tombstoned_at IS NULL +ORDER BY snapshot.pr_number +` + +type ListLoadgenCachedPullRequestChangeSnapshotsRow struct { + PrNumber int32 + BaseSha string + HeadSha string + FilesTotalCount int32 + FilesTruncated bool + CodeownersRef string + CodeownersSha string + CodeownersPath pgtype.Text + CodeownersState string + CodeownersSource pgtype.Text + CodeownersHash string +} + +func (q *Queries) ListLoadgenCachedPullRequestChangeSnapshots(ctx context.Context, repoFullName string) ([]ListLoadgenCachedPullRequestChangeSnapshotsRow, error) { + rows, err := q.db.Query(ctx, listLoadgenCachedPullRequestChangeSnapshots, repoFullName) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListLoadgenCachedPullRequestChangeSnapshotsRow + for rows.Next() { + var i ListLoadgenCachedPullRequestChangeSnapshotsRow + if err := rows.Scan( + &i.PrNumber, + &i.BaseSha, + &i.HeadSha, + &i.FilesTotalCount, + &i.FilesTruncated, + &i.CodeownersRef, + &i.CodeownersSha, + &i.CodeownersPath, + &i.CodeownersState, + &i.CodeownersSource, + &i.CodeownersHash, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listLoadgenCachedPullRequestChangedFiles = `-- name: ListLoadgenCachedPullRequestChangedFiles :many +SELECT file.pr_number, file.path, file.previous_path, file.change_type, + file.base_sha, file.head_sha +FROM pull_request_changed_files AS file +JOIN repos AS repo ON repo.id = file.repo_id +WHERE repo.full_name = $1 + AND repo.tombstoned_at IS NULL + AND file.tombstoned_at IS NULL +ORDER BY file.pr_number, file.path +` + +type ListLoadgenCachedPullRequestChangedFilesRow struct { + PrNumber int32 + Path string + PreviousPath pgtype.Text + ChangeType string + BaseSha string + HeadSha string +} + +func (q *Queries) ListLoadgenCachedPullRequestChangedFiles(ctx context.Context, repoFullName string) ([]ListLoadgenCachedPullRequestChangedFilesRow, error) { + rows, err := q.db.Query(ctx, listLoadgenCachedPullRequestChangedFiles, repoFullName) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListLoadgenCachedPullRequestChangedFilesRow + for rows.Next() { + var i ListLoadgenCachedPullRequestChangedFilesRow + if err := rows.Scan( + &i.PrNumber, + &i.Path, + &i.PreviousPath, + &i.ChangeType, + &i.BaseSha, + &i.HeadSha, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listLoadgenCachedPullRequestComments = `-- name: ListLoadgenCachedPullRequestComments :many SELECT comment.pr_number, comment.gh_id, comment.node_id, comment.author_kind, comment.author_node_id, comment.author_login, @@ -235,6 +342,69 @@ func (q *Queries) ListLoadgenCachedPullRequestComments(ctx context.Context, repo return items, nil } +const listLoadgenCachedPullRequestFileOwners = `-- name: ListLoadgenCachedPullRequestFileOwners :many +SELECT owner.pr_number, owner.path, owner.owner_token, owner.owner_type, + owner.owner_name, owner.resolution_state, owner.owner_gh_id, + owner.owner_node_id, owner.owner_login, owner.source_pattern, + owner.source_line, owner.base_sha, owner.head_sha +FROM pull_request_file_owners AS owner +JOIN repos AS repo ON repo.id = owner.repo_id +WHERE repo.full_name = $1 + AND repo.tombstoned_at IS NULL + AND owner.tombstoned_at IS NULL +ORDER BY owner.pr_number, owner.path, owner.owner_token +` + +type ListLoadgenCachedPullRequestFileOwnersRow struct { + PrNumber int32 + Path string + OwnerToken string + OwnerType string + OwnerName string + ResolutionState string + OwnerGhID pgtype.Int8 + OwnerNodeID pgtype.Text + OwnerLogin pgtype.Text + SourcePattern string + SourceLine int32 + BaseSha string + HeadSha string +} + +func (q *Queries) ListLoadgenCachedPullRequestFileOwners(ctx context.Context, repoFullName string) ([]ListLoadgenCachedPullRequestFileOwnersRow, error) { + rows, err := q.db.Query(ctx, listLoadgenCachedPullRequestFileOwners, repoFullName) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListLoadgenCachedPullRequestFileOwnersRow + for rows.Next() { + var i ListLoadgenCachedPullRequestFileOwnersRow + if err := rows.Scan( + &i.PrNumber, + &i.Path, + &i.OwnerToken, + &i.OwnerType, + &i.OwnerName, + &i.ResolutionState, + &i.OwnerGhID, + &i.OwnerNodeID, + &i.OwnerLogin, + &i.SourcePattern, + &i.SourceLine, + &i.BaseSha, + &i.HeadSha, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listLoadgenCachedPullRequestReviewRequests = `-- name: ListLoadgenCachedPullRequestReviewRequests :many SELECT request.pr_number, diff --git a/internal/store/dbgen/models.go b/internal/store/dbgen/models.go index 7b11aab..0347b7a 100644 --- a/internal/store/dbgen/models.go +++ b/internal/store/dbgen/models.go @@ -141,6 +141,16 @@ type DerivationDirty struct { MarkedAt pgtype.Timestamptz } +type DriftEntitiesWithoutChangeInput struct { + InstallationID int64 + EntityKind string + SourceID int64 + EntityKey string + LockKey string + CacheSnapshot []byte + LastCheckedAt pgtype.Timestamptz +} + type DriftEntitiesWithoutParticipation struct { InstallationID int64 EntityKind string @@ -260,6 +270,42 @@ type PullRequest struct { DisplayUntil pgtype.Timestamptz } +type PullRequestChangeSnapshot struct { + RepoID int64 + PrNumber int32 + BaseSha string + HeadSha string + FilesTotalCount int32 + FilesTruncated bool + CodeownersRef string + CodeownersSha string + CodeownersPath pgtype.Text + CodeownersState string + CodeownersSource pgtype.Text + CodeownersHash string + ParentGhUpdatedAt pgtype.Timestamptz + SyncedAt pgtype.Timestamptz + Etag string + SyncSource string + TombstonedAt pgtype.Timestamptz + LastCheckedAt pgtype.Timestamptz +} + +type PullRequestChangedFile struct { + RepoID int64 + PrNumber int32 + Path string + PreviousPath pgtype.Text + ChangeType string + BaseSha string + HeadSha string + SyncedAt pgtype.Timestamptz + Etag string + SyncSource string + TombstonedAt pgtype.Timestamptz + LastCheckedAt pgtype.Timestamptz +} + type PullRequestComment struct { NodeID string GhID pgtype.Int8 @@ -278,6 +324,28 @@ type PullRequestComment struct { LastCheckedAt pgtype.Timestamptz } +type PullRequestFileOwner struct { + RepoID int64 + PrNumber int32 + Path string + OwnerToken string + OwnerType string + OwnerName string + ResolutionState string + OwnerGhID pgtype.Int8 + OwnerNodeID pgtype.Text + OwnerLogin pgtype.Text + SourcePattern string + SourceLine int32 + BaseSha string + HeadSha string + SyncedAt pgtype.Timestamptz + Etag string + SyncSource string + TombstonedAt pgtype.Timestamptz + LastCheckedAt pgtype.Timestamptz +} + type PullRequestReview struct { NodeID string GhID pgtype.Int8 diff --git a/internal/store/keys.go b/internal/store/keys.go index 759b03e..db4e178 100644 --- a/internal/store/keys.go +++ b/internal/store/keys.go @@ -135,6 +135,14 @@ func nullableInt8(value int64) pgtype.Int8 { return pgtype.Int8{Int64: value, Valid: value > 0} } +func nullableText(value string) pgtype.Text { + return pgtype.Text{String: value, Valid: value != ""} +} + +func optionalText(value string, valid bool) pgtype.Text { + return pgtype.Text{String: value, Valid: valid} +} + func timestamp(value time.Time) pgtype.Timestamptz { return pgtype.Timestamptz{Time: value, Valid: !value.IsZero()} } diff --git a/internal/store/migrate_test.go b/internal/store/migrate_test.go index b8f3e57..83e460c 100644 --- a/internal/store/migrate_test.go +++ b/internal/store/migrate_test.go @@ -363,20 +363,30 @@ func TestMigrationLockWaitFailureClosesHijackedConnection(t *testing.T) { t.Fatal("contended migration lock ignored its context deadline") } - var remaining int - if err := firstPool.QueryRow(ctx, ` - SELECT count(*) - FROM pg_stat_activity - WHERE datname = current_database() - AND application_name = $1 - `, applicationName).Scan(&remaining); err != nil { - t.Fatal(err) - } - if remaining != 0 { - t.Fatalf( - "failed migration lock leaked %d hijacked database connections", - remaining, - ) + // Backend exit is asynchronous relative to the client-side close of the + // failed lock connection; poll with a generous bound before declaring a + // leak. + deadline := time.Now().Add(10 * time.Second) + for { + var remaining int + if err := firstPool.QueryRow(ctx, ` + SELECT count(*) + FROM pg_stat_activity + WHERE datname = current_database() + AND application_name = $1 + `, applicationName).Scan(&remaining); err != nil { + t.Fatal(err) + } + if remaining == 0 { + break + } + if time.Now().After(deadline) { + t.Fatalf( + "failed migration lock leaked %d hijacked database connections", + remaining, + ) + } + time.Sleep(50 * time.Millisecond) } } diff --git a/internal/store/pull_request.go b/internal/store/pull_request.go index 955d9f9..a4dd4a9 100644 --- a/internal/store/pull_request.go +++ b/internal/store/pull_request.go @@ -126,6 +126,30 @@ func (w *EntityWriter) TouchPullRequest( ); err != nil { return fmt.Errorf("touch PR comments: %w", err) } + changeTouch := dbgen.TouchPullRequestChangeInputsCheckedAtParams{ + CheckedAt: timestamp(checkedAt), + Etag: etag, + RepoID: repo.ID, + PrNumber: int32(number), + ParentGhUpdatedAt: current.GhUpdatedAt, + } + if err := queries.TouchPullRequestChangeInputsCheckedAt( + ctx, changeTouch, + ); err != nil { + return fmt.Errorf("touch PR change snapshot: %w", err) + } + if err := queries.TouchPullRequestChangedFilesCheckedAt( + ctx, + dbgen.TouchPullRequestChangedFilesCheckedAtParams(changeTouch), + ); err != nil { + return fmt.Errorf("touch PR changed files: %w", err) + } + if err := queries.TouchPullRequestFileOwnersCheckedAt( + ctx, + dbgen.TouchPullRequestFileOwnersCheckedAtParams(changeTouch), + ); err != nil { + return fmt.Errorf("touch PR file owners: %w", err) + } } return nil }) @@ -437,6 +461,108 @@ func (w *EntityWriter) applyPullRequest( } } + if pull.ChangeInputsKnown { + snapshot := pull.ChangeSnapshot + changedCount, err := queries.UpsertPullRequestChangeSnapshot( + ctx, + dbgen.UpsertPullRequestChangeSnapshotParams{ + RepoID: repo.ID, + PrNumber: int32(pull.Number), + HeadSha: snapshot.HeadSHA, + BaseSha: snapshot.BaseSHA, + ParentGhUpdatedAt: timestamp(pull.GitHubUpdatedAt), + FilesTotalCount: int32(snapshot.FilesTotalCount), + FilesTruncated: snapshot.FilesTruncated, + CodeownersRef: snapshot.CodeownersRef, + CodeownersSha: snapshot.CodeownersSHA, + CodeownersPath: nullableText(snapshot.CodeownersPath), + CodeownersState: snapshot.CodeownersState, + CodeownersSource: optionalText( + snapshot.CodeownersSource, + snapshot.CodeownersState == "present", + ), + CodeownersHash: snapshot.CodeownersHash, + SyncedAt: timestamp(pull.SyncedAt), + Etag: pull.ETag, + SyncSource: string(pull.Source), + LastCheckedAt: timestamp(pull.SyncedAt), + }, + ) + if err != nil { + return fmt.Errorf("upsert PR change snapshot: %w", err) + } + files, err := encodeChangedFiles(snapshot.Files) + if err != nil { + return err + } + changedFiles, err := queries.ReplacePullRequestChangedFiles( + ctx, + dbgen.ReplacePullRequestChangedFilesParams{ + ChangedFiles: files, + RepoID: repo.ID, + PrNumber: int32(pull.Number), + BaseSha: snapshot.BaseSHA, + HeadSha: snapshot.HeadSHA, + ParentGhUpdatedAt: timestamp(pull.GitHubUpdatedAt), + SyncedAt: timestamp(pull.SyncedAt), + Etag: pull.ETag, + SyncSource: string(pull.Source), + LastCheckedAt: timestamp(pull.SyncedAt), + }, + ) + if err != nil { + return fmt.Errorf("replace PR changed files: %w", err) + } + owners, err := encodeFileOwners(snapshot.Owners) + if err != nil { + return err + } + changedOwners, err := queries.ReplacePullRequestFileOwners( + ctx, + dbgen.ReplacePullRequestFileOwnersParams{ + FileOwners: owners, + RepoID: repo.ID, + PrNumber: int32(pull.Number), + BaseSha: snapshot.BaseSHA, + HeadSha: snapshot.HeadSHA, + ParentGhUpdatedAt: timestamp(pull.GitHubUpdatedAt), + SyncedAt: timestamp(pull.SyncedAt), + Etag: pull.ETag, + SyncSource: string(pull.Source), + LastCheckedAt: timestamp(pull.SyncedAt), + }, + ) + if err != nil { + return fmt.Errorf("replace PR file owners: %w", err) + } + result.ChangeInputsChanged = changedCount > 0 || + len(changedFiles) > 0 || len(changedOwners) > 0 + changeTouch := dbgen.TouchPullRequestChangeInputsCheckedAtParams{ + CheckedAt: timestamp(pull.SyncedAt), + Etag: pull.ETag, + RepoID: repo.ID, + PrNumber: int32(pull.Number), + ParentGhUpdatedAt: timestamp(pull.GitHubUpdatedAt), + } + if err := queries.TouchPullRequestChangeInputsCheckedAt( + ctx, changeTouch, + ); err != nil { + return fmt.Errorf("touch PR change snapshot: %w", err) + } + if err := queries.TouchPullRequestChangedFilesCheckedAt( + ctx, + dbgen.TouchPullRequestChangedFilesCheckedAtParams(changeTouch), + ); err != nil { + return fmt.Errorf("touch PR changed files: %w", err) + } + if err := queries.TouchPullRequestFileOwnersCheckedAt( + ctx, + dbgen.TouchPullRequestFileOwnersCheckedAtParams(changeTouch), + ); err != nil { + return fmt.Errorf("touch PR file owners: %w", err) + } + } + threadsChanged := false if pull.ThreadsKnown { threads, err := encodeReviewThreads(pull.ReviewThreads) @@ -473,7 +599,7 @@ func (w *EntityWriter) applyPullRequest( } result.Applied = result.DomainChanged || threadsChanged || result.ReviewRequestsChanged || result.ReviewsChanged || - result.CommentsChanged + result.CommentsChanged || result.ChangeInputsChanged if result.Applied { scopes := uniqueStrings( derivationScope( @@ -704,6 +830,29 @@ func (w *EntityWriter) TombstonePullRequestObserved( ); err != nil { return fmt.Errorf("tombstone PR comments: %w", err) } + changeTombstone := dbgen.TombstonePullRequestChangeSnapshotParams{ + TombstonedAt: timestamp(at), + SyncSource: string(source), + RepoID: repo.ID, + PrNumber: int32(number), + } + if _, err := queries.TombstonePullRequestFileOwners( + ctx, + dbgen.TombstonePullRequestFileOwnersParams(changeTombstone), + ); err != nil { + return fmt.Errorf("tombstone PR file owners: %w", err) + } + if _, err := queries.TombstonePullRequestChangedFiles( + ctx, + dbgen.TombstonePullRequestChangedFilesParams(changeTombstone), + ); err != nil { + return fmt.Errorf("tombstone PR changed files: %w", err) + } + if _, err := queries.TombstonePullRequestChangeSnapshot( + ctx, changeTombstone, + ); err != nil { + return fmt.Errorf("tombstone PR change snapshot: %w", err) + } } if hook != nil { if txHook := hook(result); txHook != nil { @@ -823,3 +972,47 @@ func encodePullRequestComments( } return value, nil } + +func encodeChangedFiles(files []ChangedFileRecord) ([]byte, error) { + type encodedFile struct { + Path string `json:"path"` + PreviousPath string `json:"previous_path"` + ChangeType string `json:"change_type"` + } + encoded := make([]encodedFile, 0, len(files)) + for _, file := range files { + encoded = append(encoded, encodedFile{ + Path: file.Path, PreviousPath: file.PreviousPath, + ChangeType: strings.ToLower(file.ChangeType), + }) + } + value, err := json.Marshal(encoded) + if err != nil { + return nil, fmt.Errorf("encode PR changed files: %w", err) + } + return value, nil +} + +func encodeFileOwners(owners []FileOwnerRecord) ([]byte, error) { + type encodedOwner struct { + Path string `json:"path"` + OwnerToken string `json:"owner_token"` + OwnerType string `json:"owner_type"` + OwnerName string `json:"owner_name"` + ResolutionState string `json:"resolution_state"` + OwnerGitHubID int64 `json:"owner_gh_id"` + OwnerNodeID string `json:"owner_node_id"` + OwnerLogin string `json:"owner_login"` + SourcePattern string `json:"source_pattern"` + SourceLine int `json:"source_line"` + } + encoded := make([]encodedOwner, 0, len(owners)) + for index := range owners { + encoded = append(encoded, encodedOwner(owners[index])) + } + value, err := json.Marshal(encoded) + if err != nil { + return nil, fmt.Errorf("encode PR file owners: %w", err) + } + return value, nil +} diff --git a/internal/store/records.go b/internal/store/records.go index ba07469..29bc9fd 100644 --- a/internal/store/records.go +++ b/internal/store/records.go @@ -122,6 +122,46 @@ type PullRequestCommentRecord struct { GitHubUpdatedAt time.Time } +// ChangedFileRecord is one member of the bounded current PR diff snapshot. +type ChangedFileRecord struct { + Path string + PreviousPath string + ChangeType string +} + +// FileOwnerRecord is one CODEOWNERS token selected by the last matching rule +// for a changed path. OwnerType is syntactic; ResolutionState distinguishes a +// stable identity from an unresolved or explicitly deleted identity. +type FileOwnerRecord struct { + Path string + OwnerToken string + OwnerType string + OwnerName string + ResolutionState string + OwnerGitHubID int64 + OwnerNodeID string + OwnerLogin string + SourcePattern string + SourceLine int +} + +// PullRequestChangeSnapshotRecord fences changed files and resolved ownership +// by the exact base/head pair returned in one PR observation. +type PullRequestChangeSnapshotRecord struct { + BaseSHA string + HeadSHA string + FilesTotalCount int + FilesTruncated bool + CodeownersRef string + CodeownersSHA string + CodeownersPath string + CodeownersState string + CodeownersSource string + CodeownersHash string + Files []ChangedFileRecord + Owners []FileOwnerRecord +} + // PullRequestRecord is the authoritative pull-request state accepted by the // cache. type PullRequestRecord struct { @@ -152,6 +192,8 @@ type PullRequestRecord struct { ReviewsKnown bool Comments []PullRequestCommentRecord CommentsKnown bool + ChangeSnapshot *PullRequestChangeSnapshotRecord + ChangeInputsKnown bool ETag string SyncedAt time.Time Source SyncSource @@ -249,6 +291,7 @@ type ApplyPullRequestResult struct { ReviewRequestsChanged bool ReviewsChanged bool CommentsChanged bool + ChangeInputsChanged bool StackStateChanged bool OldStackNumber *int NewStackNumber *int @@ -383,9 +426,94 @@ func validatePullRequest(pull *PullRequestRecord) error { } seenComments[comment.NodeID] = struct{}{} } + if pull.ChangeInputsKnown { + if pull.ChangeSnapshot == nil { + return fmt.Errorf("known PR change inputs require a snapshot") + } + snapshot := pull.ChangeSnapshot + if snapshot.HeadSHA == "" || snapshot.HeadSHA != pull.HeadSHA || + snapshot.BaseSHA != pull.BaseSHA || snapshot.FilesTotalCount < 0 || + snapshot.FilesTotalCount < len(snapshot.Files) || + snapshot.CodeownersRef != pull.BaseRef || + snapshot.CodeownersSHA != pull.BaseSHA || + snapshot.CodeownersHash == "" { + return fmt.Errorf("invalid PR change-input snapshot fence") + } + switch snapshot.CodeownersState { + case "present": + if snapshot.CodeownersPath == "" { + return fmt.Errorf("present CODEOWNERS source has no path") + } + case "missing": + if snapshot.CodeownersPath != "" || snapshot.CodeownersSource != "" { + return fmt.Errorf("missing CODEOWNERS source has content") + } + case "unavailable": + if snapshot.CodeownersSHA != "" || snapshot.CodeownersPath != "" || + snapshot.CodeownersSource != "" { + return fmt.Errorf("unavailable CODEOWNERS source has provenance") + } + case "oversized": + if snapshot.CodeownersPath == "" || snapshot.CodeownersSource != "" { + return fmt.Errorf("invalid oversized CODEOWNERS source") + } + default: + return fmt.Errorf("invalid CODEOWNERS source state") + } + seenFiles := make(map[string]struct{}, len(snapshot.Files)) + for _, file := range snapshot.Files { + if file.Path == "" || !validChangeType(file.ChangeType) { + return fmt.Errorf("invalid PR changed file") + } + if _, duplicate := seenFiles[file.Path]; duplicate { + return fmt.Errorf("duplicate PR changed file %s", file.Path) + } + seenFiles[file.Path] = struct{}{} + } + seenOwners := make(map[string]struct{}, len(snapshot.Owners)) + for index := range snapshot.Owners { + owner := &snapshot.Owners[index] + if _, exists := seenFiles[owner.Path]; !exists || + owner.OwnerToken == "" || owner.SourcePattern == "" || + owner.SourceLine <= 0 || !validFileOwner(owner) { + return fmt.Errorf("invalid PR file owner") + } + key := owner.Path + "\x00" + owner.OwnerToken + if _, duplicate := seenOwners[key]; duplicate { + return fmt.Errorf("duplicate PR file owner %s", owner.OwnerToken) + } + seenOwners[key] = struct{}{} + } + } return nil } +func validChangeType(value string) bool { + switch value { + case "added", "deleted", "renamed", "copied", "modified", "changed": + return true + default: + return false + } +} + +func validFileOwner(owner *FileOwnerRecord) bool { + switch owner.OwnerType { + case "user", "team", "email", "malformed": + default: + return false + } + switch owner.ResolutionState { + case "resolved": + return owner.OwnerNodeID != "" && owner.OwnerLogin != "" + case "unresolved", "deleted": + return owner.OwnerGitHubID == 0 && owner.OwnerNodeID == "" && + owner.OwnerLogin == "" + default: + return false + } +} + func validParticipationAuthor(kind, nodeID, login string) bool { switch kind { case "user", "bot", "mannequin", "organization", diff --git a/internal/sweep/sweep_db_test.go b/internal/sweep/sweep_db_test.go index e5f57bc..a23a709 100644 --- a/internal/sweep/sweep_db_test.go +++ b/internal/sweep/sweep_db_test.go @@ -1698,14 +1698,18 @@ func (h *sweepHarness) seedCheckHistory( func waitFor(t *testing.T, condition func() bool) { t.Helper() - deadline := time.Now().Add(10 * time.Second) - for time.Now().Before(deadline) { + ticker := time.NewTicker(20 * time.Millisecond) + defer ticker.Stop() + for { if condition() { return } - time.Sleep(20 * time.Millisecond) + select { + case <-ticker.C: + case <-t.Context().Done(): + t.Fatal("condition did not become true before test cancellation") + } } - t.Fatal("condition did not become true before timeout") } func sweepTestDatabase(t *testing.T) *pgxpool.Pool {