diff --git a/core/workspace/BUILD.bazel b/core/workspace/BUILD.bazel index d762ffb3..b3e50acb 100644 --- a/core/workspace/BUILD.bazel +++ b/core/workspace/BUILD.bazel @@ -18,6 +18,7 @@ go_library( go_test( name = "workspace_test", srcs = [ + "gitrequest_realgit_test.go", "gitrequest_test.go", "request_test.go", "workspace_test.go", diff --git a/core/workspace/gitrequest.go b/core/workspace/gitrequest.go index 7432848d..fd07fd0a 100644 --- a/core/workspace/gitrequest.go +++ b/core/workspace/gitrequest.go @@ -24,43 +24,97 @@ import ( ) type gitRequest struct { - git git.Interface - requestID string - baseRef string - commit string - logger *zap.SugaredLogger + git git.Interface + requestID string + baseRef string + commit string + upstreamRemote string + logger *zap.SugaredLogger } -func NewGitRequest(git git.Interface, requestPath string, baseRef string, commit string, logger *zap.SugaredLogger) Request { +// NewGitRequest creates a Request that applies a GitHub pull request. +// +// upstreamRemote is the real remote URL (e.g. the GitHub HTTPS/SSH URL) +// from which PR refs are fetched. Worker clones created with --local have +// their "origin" pointing at the pool's local origin directory, which does +// not expose pull/* refs, so fetches must target the upstream directly. +// +// commit pins the content that is applied: the diff is computed between +// baseRef and commit (not the floating PR head), and commit must be an +// ancestor of the current PR head as a sanity check. This ensures the +// materialized tree is stable for a given (URL, commit) cache key even as +// the PR advances. +func NewGitRequest(git git.Interface, requestPath string, baseRef string, commit string, upstreamRemote string, logger *zap.SugaredLogger) Request { // get the last part of the request path requestID := filepath.Base(requestPath) return &gitRequest{ - git: git, - requestID: requestID, - baseRef: baseRef, - commit: commit, - logger: logger, + git: git, + requestID: requestID, + baseRef: baseRef, + commit: commit, + upstreamRemote: upstreamRemote, + logger: logger, } } // Apply applies the change request to the workspace. +// +// PR refs are fetched from upstreamRemote (the real GitHub URL) rather than +// "origin", because worker clones created with --local have their origin +// pointing at the pool's local directory which lacks pull/* refs. +// +// The diff is computed between baseRef and the pinned commit (not the +// floating PR head). The commit must be an ancestor of the current PR head +// as a sanity check, but the actual content applied is always the pinned +// commit so the materialized tree is deterministic for a given cache key. func (r *gitRequest) Apply(ctx context.Context) error { - r.logger.Infow("gitRequest: Applying PR", zap.String("request_id", r.requestID), zap.String("base_ref", r.baseRef), zap.String("commit", r.commit)) - ref := fmt.Sprintf("+pull/%s/head:pull/%s/head", r.requestID, r.requestID) - err := r.git.Fetch(ctx, "origin", ref, "--force", "--no-tags") + r.logger.Infow("gitRequest: Applying PR", + zap.String("request_id", r.requestID), + zap.String("base_ref", r.baseRef), + zap.String("commit", r.commit), + zap.String("upstream_remote", r.upstreamRemote), + ) + + // Fetch the PR head ref from the upstream remote (not "origin") so the + // ancestor check can verify the pinned commit belongs to this PR. + prRef := fmt.Sprintf("pull/%s/head", r.requestID) + fetchRef := fmt.Sprintf("+refs/%s:refs/%s", prRef, prRef) + err := r.git.Fetch(ctx, r.upstreamRemote, fetchRef, "--force", "--no-tags") if err != nil { - return fmt.Errorf("fetch PR %s: %w", r.requestID, err) + return fmt.Errorf("fetch PR %s from upstream: %w", r.requestID, err) } - if r.commit != "" { - isAncestor, err := r.git.IsAncestor(ctx, r.commit, fmt.Sprintf("pull/%s/head", r.requestID)) - if err != nil { - return fmt.Errorf("failed to read PR commit history: %w", err) - } - if !isAncestor { - return fmt.Errorf("commit %q is not an ancestor of PR %s", r.commit, r.requestID) + + // Check whether the pinned commit object is already present locally. + // It almost always will be, since commit must be an ancestor of the PR + // head we just fetched. Only when the object is missing (e.g. shallow + // clone, or unusual ref topology) do we attempt a bare-SHA fetch as a + // best-effort fallback. Many git servers refuse bare-SHA fetches unless + // uploadpack.allowReachableSHA1InWant is enabled, so this path must not + // be required for the normal case. + if _, revErr := r.git.RevParse(ctx, r.commit+"^{commit}"); revErr != nil { + r.logger.Infow("gitRequest: pinned commit not found locally, attempting bare-SHA fetch", + zap.String("commit", r.commit), + zap.Error(revErr), + ) + if fetchErr := r.git.Fetch(ctx, r.upstreamRemote, r.commit, "--force", "--no-tags"); fetchErr != nil { + return fmt.Errorf("pinned commit %s is not available locally or from upstream: %w", r.commit, fetchErr) } } - patch, err := r.git.Diff(ctx, r.baseRef, fmt.Sprintf("pull/%s/head", r.requestID), "--binary", "--merge-base") + + // Sanity-check: the pinned commit must be an ancestor of the current PR + // head. This catches stale or bogus commit values without silently + // applying unrelated content. + isAncestor, err := r.git.IsAncestor(ctx, r.commit, prRef) + if err != nil { + return fmt.Errorf("failed to read PR commit history: %w", err) + } + if !isAncestor { + return fmt.Errorf("commit %q is not an ancestor of PR %s", r.commit, r.requestID) + } + + // Diff against the pinned commit, not the floating PR head. This makes + // the materialized tree deterministic for the (URL, commit) cache key. + patch, err := r.git.Diff(ctx, r.baseRef, r.commit, "--binary", "--merge-base") if err != nil { return fmt.Errorf("compute diff for PR %s: %w", r.requestID, err) } diff --git a/core/workspace/gitrequest_realgit_test.go b/core/workspace/gitrequest_realgit_test.go new file mode 100644 index 00000000..61422593 --- /dev/null +++ b/core/workspace/gitrequest_realgit_test.go @@ -0,0 +1,259 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package workspace_test + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/uber/tango/core/git" + "github.com/uber/tango/core/workspace" + "go.uber.org/zap" +) + +// runGit is a test helper that runs a git command in the given directory. +func runGit(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=test", + "GIT_AUTHOR_EMAIL=test@test.com", + "GIT_COMMITTER_NAME=test", + "GIT_COMMITTER_EMAIL=test@test.com", + ) + out, err := cmd.CombinedOutput() + require.NoError(t, err, "git %v in %s failed: %s", args, dir, string(out)) + return string(out) +} + +// setupBareRepoWithPR creates a bare repo with a base commit and a PR ref +// (refs/pull/1/head). Returns (bare repo path, base SHA, PR commit SHA). +// The PR branch adds a file "pr.txt" with the given content. +func setupBareRepoWithPR(t *testing.T, prContent string) (bareDir, baseSHA, prSHA string) { + t.Helper() + + // Create a work repo, commit a base, then push to bare. + workDir := filepath.Join(t.TempDir(), "work") + bareDir = filepath.Join(t.TempDir(), "bare.git") + + require.NoError(t, os.MkdirAll(workDir, 0o755)) + runGit(t, workDir, "init") + runGit(t, workDir, "checkout", "-b", "main") + + // Base commit + require.NoError(t, os.WriteFile(filepath.Join(workDir, "base.txt"), []byte("base"), 0o644)) + runGit(t, workDir, "add", "base.txt") + runGit(t, workDir, "commit", "-m", "base commit") + + // Create bare clone + runGit(t, workDir, "clone", "--bare", workDir, bareDir) + + // Get base SHA from the bare repo + cmd := exec.Command("git", "rev-parse", "HEAD") + cmd.Dir = bareDir + out, err := cmd.Output() + require.NoError(t, err) + baseSHA = string(out[:len(out)-1]) + + // Create PR commit in the work repo + runGit(t, workDir, "checkout", "-b", "pr-branch") + require.NoError(t, os.WriteFile(filepath.Join(workDir, "pr.txt"), []byte(prContent), 0o644)) + runGit(t, workDir, "add", "pr.txt") + runGit(t, workDir, "commit", "-m", "PR commit: "+prContent) + + cmd = exec.Command("git", "rev-parse", "HEAD") + cmd.Dir = workDir + out, err = cmd.Output() + require.NoError(t, err) + prSHA = string(out[:len(out)-1]) + + // Push the PR ref to the bare repo + runGit(t, workDir, "push", bareDir, "HEAD:refs/pull/1/head") + + return bareDir, baseSHA, prSHA +} + +// TestGitRequest_PinnedCommit_StableTree verifies that applying a pinned +// commit yields the same tree even after the PR head advances. This is the +// critical property for cache correctness: the (URL, commit) cache key must +// always resolve to the same content. +func TestGitRequest_PinnedCommit_StableTree(t *testing.T) { + t.Parallel() + + ctx := context.Background() + logger := zap.NewNop().Sugar() + + // Set up the bare repo with a PR. + bareDir, baseSHA, prSHA1 := setupBareRepoWithPR(t, "version-1") + + // --- First apply: pin to prSHA1 --- + + // Clone the bare repo into a worker (simulating what repomanager does) + workerDir1 := filepath.Join(t.TempDir(), "worker1") + runGit(t, t.TempDir(), "clone", bareDir, workerDir1) + runGit(t, workerDir1, "checkout", baseSHA) + + g1 := git.New(workerDir1, logger) + req1 := workspace.NewGitRequest(g1, "1", baseSHA, prSHA1, bareDir, logger) + err := req1.Apply(ctx) + require.NoError(t, err) + + tree1, err := g1.RevParse(ctx, "HEAD^{tree}") + require.NoError(t, err) + + // --- Advance the PR head in the bare repo --- + + // Create a new work repo to push a new commit to the PR ref. + advanceDir := filepath.Join(t.TempDir(), "advance") + runGit(t, t.TempDir(), "clone", bareDir, advanceDir) + // Fetch the PR ref explicitly since regular clones don't include refs/pull/*. + runGit(t, advanceDir, "fetch", "origin", "refs/pull/1/head:refs/pull/1/head") + runGit(t, advanceDir, "checkout", "-b", "pr-branch", "refs/pull/1/head") + require.NoError(t, os.WriteFile(filepath.Join(advanceDir, "pr.txt"), []byte("version-2"), 0o644)) + runGit(t, advanceDir, "add", "pr.txt") + runGit(t, advanceDir, "commit", "-m", "advance PR") + runGit(t, advanceDir, "push", bareDir, "HEAD:refs/pull/1/head") + + // --- Second apply: same pinned commit (prSHA1) --- + + workerDir2 := filepath.Join(t.TempDir(), "worker2") + runGit(t, t.TempDir(), "clone", bareDir, workerDir2) + runGit(t, workerDir2, "checkout", baseSHA) + + g2 := git.New(workerDir2, logger) + req2 := workspace.NewGitRequest(g2, "1", baseSHA, prSHA1, bareDir, logger) + err = req2.Apply(ctx) + require.NoError(t, err) + + tree2, err := g2.RevParse(ctx, "HEAD^{tree}") + require.NoError(t, err) + + // The two trees must be identical because the same pinned commit was + // applied, even though the PR head advanced between the two applies. + assert.Equal(t, tree1, tree2, "pinned commit must yield the same tree regardless of PR head advancement") +} + +// TestGitRequest_UpstreamFetch_WorksFromLocalClone verifies that PR refs +// are correctly fetched from the upstream (bare repo) even when the worker +// clone's "origin" is a local directory that does not expose pull/* refs +// (the scenario described in audit #3a). +func TestGitRequest_UpstreamFetch_WorksFromLocalClone(t *testing.T) { + t.Parallel() + + ctx := context.Background() + logger := zap.NewNop().Sugar() + + bareDir, baseSHA, prSHA := setupBareRepoWithPR(t, "pr-content") + + // Simulate the repomanager: clone bare -> origin, then clone origin -> worker (--local). + // The worker's "origin" points at the intermediate clone, not the bare repo, + // so it does NOT have pull/* refs. + originDir := filepath.Join(t.TempDir(), "origin") + runGit(t, t.TempDir(), "clone", bareDir, originDir) + + workerDir := filepath.Join(t.TempDir(), "worker") + runGit(t, t.TempDir(), "clone", "--local", originDir, workerDir) + runGit(t, workerDir, "checkout", baseSHA) + + // Apply the PR using the bare repo as the upstream remote (not "origin"). + g := git.New(workerDir, logger) + req := workspace.NewGitRequest(g, "1", baseSHA, prSHA, bareDir, logger) + err := req.Apply(ctx) + require.NoError(t, err) + + // Verify the PR content was applied. + content, err := os.ReadFile(filepath.Join(workerDir, "pr.txt")) + require.NoError(t, err) + assert.Equal(t, "pr-content", string(content)) +} + +// TestGitRequest_PinnedCommit_RejectsStaleCommit verifies that a commit +// that is no longer an ancestor of the PR head (e.g. after a force-push) +// is rejected. +func TestGitRequest_PinnedCommit_RejectsStaleCommit(t *testing.T) { + t.Parallel() + + ctx := context.Background() + logger := zap.NewNop().Sugar() + + bareDir, baseSHA, _ := setupBareRepoWithPR(t, "original") + + // Force-push a completely new history to the PR ref. + rewriteDir := filepath.Join(t.TempDir(), "rewrite") + runGit(t, t.TempDir(), "clone", bareDir, rewriteDir) + runGit(t, rewriteDir, "checkout", "-b", "new-pr") + require.NoError(t, os.WriteFile(filepath.Join(rewriteDir, "other.txt"), []byte("different"), 0o644)) + runGit(t, rewriteDir, "add", "other.txt") + runGit(t, rewriteDir, "commit", "-m", "new PR history") + runGit(t, rewriteDir, "push", "--force", bareDir, "HEAD:refs/pull/1/head") + + // Try to apply with the OLD commit SHA (which is no longer an ancestor). + workerDir := filepath.Join(t.TempDir(), "worker") + runGit(t, t.TempDir(), "clone", bareDir, workerDir) + runGit(t, workerDir, "checkout", baseSHA) + + g := git.New(workerDir, logger) + // Use a made-up SHA that won't be an ancestor of the new PR head. + req := workspace.NewGitRequest(g, "1", baseSHA, "0000000000000000000000000000000000000000", bareDir, logger) + err := req.Apply(ctx) + require.Error(t, err) +} + +// TestGitRequest_NoBareShAFetch_WhenCommitReachable verifies that Apply +// succeeds without ever issuing a bare-SHA fetch when the pinned commit is +// an ancestor of the PR head (the normal case). This matters because many +// git servers refuse bare-SHA fetches unless uploadpack.allowAnySHA1InWant +// or allowReachableSHA1InWant is enabled. +// +// The test uses a --local clone chain (bare -> origin -> worker) where the +// worker's "origin" is the intermediate clone, NOT the bare repo. Since +// the pinned commit is reachable from the PR head that we fetch from the +// bare repo, RevParse finds it in the local odb and the fallback fetch +// path is never exercised. +func TestGitRequest_NoBareShAFetch_WhenCommitReachable(t *testing.T) { + t.Parallel() + + ctx := context.Background() + logger := zap.NewNop().Sugar() + + bareDir, baseSHA, prSHA := setupBareRepoWithPR(t, "reachable-content") + + // Clone chain: bare -> origin -> worker (--local) + originDir := filepath.Join(t.TempDir(), "origin") + runGit(t, t.TempDir(), "clone", bareDir, originDir) + + workerDir := filepath.Join(t.TempDir(), "worker") + runGit(t, t.TempDir(), "clone", "--local", originDir, workerDir) + runGit(t, workerDir, "checkout", baseSHA) + + // Apply with the pinned commit that is an ancestor of the PR head. + // The commit object arrives as part of the PR-head ref fetch, so no + // separate bare-SHA fetch is needed. + g := git.New(workerDir, logger) + req := workspace.NewGitRequest(g, "1", baseSHA, prSHA, bareDir, logger) + err := req.Apply(ctx) + require.NoError(t, err) + + // Verify the correct content was applied. + content, err := os.ReadFile(filepath.Join(workerDir, "pr.txt")) + require.NoError(t, err) + assert.Equal(t, "reachable-content", string(content)) +} diff --git a/core/workspace/gitrequest_test.go b/core/workspace/gitrequest_test.go index d0bc84e4..edc1c80d 100644 --- a/core/workspace/gitrequest_test.go +++ b/core/workspace/gitrequest_test.go @@ -26,30 +26,39 @@ import ( "go.uber.org/zap" ) +const _testUpstream = "git@github.com:org/repo" + func TestNewGitRequest_InvalidPath(t *testing.T) { - req := NewGitRequest(nil, "invalid", "baseRef", "", zap.NewNop().Sugar()) + req := NewGitRequest(nil, "invalid", "baseRef", "", _testUpstream, zap.NewNop().Sugar()) require.NotNil(t, req) } func TestNewGitRequest_ExtractsID(t *testing.T) { - r := NewGitRequest(nil, "/org/repo/pull/456", "baseRef", "abc123", zap.NewNop().Sugar()) + r := NewGitRequest(nil, "/org/repo/pull/456", "baseRef", "abc123", _testUpstream, zap.NewNop().Sugar()) gr, ok := r.(*gitRequest) assert.True(t, ok, "expected *gitRequest, got %T", r) assert.Equal(t, "456", gr.requestID) assert.Equal(t, "baseRef", gr.baseRef) assert.Equal(t, "abc123", gr.commit) + assert.Equal(t, _testUpstream, gr.upstreamRemote) } func TestGitRequest_Apply_CommitIsAncestor_Success(t *testing.T) { ctrl := gomock.NewController(t) git := gitmock.NewMockInterface(ctrl) - git.EXPECT().Fetch(gomock.Any(), "origin", gomock.Any(), gomock.Any()).Return(nil) + + // Fetch PR head from upstream remote (not "origin") + git.EXPECT().Fetch(gomock.Any(), _testUpstream, "+refs/pull/123/head:refs/pull/123/head", "--force", "--no-tags").Return(nil) + // Commit found locally via RevParse -- no bare-SHA fetch needed + git.EXPECT().RevParse(gomock.Any(), "deadbeef^{commit}").Return("deadbeef", nil) git.EXPECT().IsAncestor(gomock.Any(), "deadbeef", "pull/123/head").Return(true, nil) - git.EXPECT().Diff(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil) + // Diff against pinned commit (not PR head) + git.EXPECT().Diff(gomock.Any(), "baseRef", "deadbeef", "--binary", "--merge-base").Return(nil, nil) git.EXPECT().ApplyPatch(gomock.Any(), gomock.Any()).Return(nil) git.EXPECT().Commit(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) git.EXPECT().SubmoduleUpdate(gomock.Any()).Return(nil) - req := NewGitRequest(git, "123", "baseRef", "deadbeef", zap.NewNop().Sugar()) + + req := NewGitRequest(git, "123", "baseRef", "deadbeef", _testUpstream, zap.NewNop().Sugar()) err := req.Apply(context.Background()) require.NoError(t, err) } @@ -57,9 +66,12 @@ func TestGitRequest_Apply_CommitIsAncestor_Success(t *testing.T) { func TestGitRequest_Apply_CommitNotAncestor_ReturnsError(t *testing.T) { ctrl := gomock.NewController(t) git := gitmock.NewMockInterface(ctrl) - git.EXPECT().Fetch(gomock.Any(), "origin", gomock.Any(), gomock.Any()).Return(nil) + + git.EXPECT().Fetch(gomock.Any(), _testUpstream, "+refs/pull/456/head:refs/pull/456/head", "--force", "--no-tags").Return(nil) + git.EXPECT().RevParse(gomock.Any(), "deadbeef^{commit}").Return("deadbeef", nil) git.EXPECT().IsAncestor(gomock.Any(), "deadbeef", "pull/456/head").Return(false, nil) - req := NewGitRequest(git, "456", "baseRef", "deadbeef", zap.NewNop().Sugar()) + + req := NewGitRequest(git, "456", "baseRef", "deadbeef", _testUpstream, zap.NewNop().Sugar()) err := req.Apply(context.Background()) require.Error(t, err) assert.Contains(t, err.Error(), "deadbeef") @@ -68,10 +80,101 @@ func TestGitRequest_Apply_CommitNotAncestor_ReturnsError(t *testing.T) { func TestGitRequest_Apply_IsAncestorFails_ReturnsError(t *testing.T) { ctrl := gomock.NewController(t) git := gitmock.NewMockInterface(ctrl) - git.EXPECT().Fetch(gomock.Any(), "origin", gomock.Any(), gomock.Any()).Return(nil) + + git.EXPECT().Fetch(gomock.Any(), _testUpstream, "+refs/pull/789/head:refs/pull/789/head", "--force", "--no-tags").Return(nil) + git.EXPECT().RevParse(gomock.Any(), "deadbeef^{commit}").Return("deadbeef", nil) git.EXPECT().IsAncestor(gomock.Any(), "deadbeef", "pull/789/head").Return(false, errors.New("ancestor check failed")) - req := NewGitRequest(git, "789", "baseRef", "deadbeef", zap.NewNop().Sugar()) + + req := NewGitRequest(git, "789", "baseRef", "deadbeef", _testUpstream, zap.NewNop().Sugar()) err := req.Apply(context.Background()) require.Error(t, err) assert.Contains(t, err.Error(), "failed to read PR commit history") } + +func TestGitRequest_Apply_FetchFromUpstream(t *testing.T) { + // Verifies that PR refs are fetched from the upstream remote, not "origin". + ctrl := gomock.NewController(t) + git := gitmock.NewMockInterface(ctrl) + + upstream := "https://github.com/myorg/myrepo.git" + git.EXPECT().Fetch(gomock.Any(), upstream, "+refs/pull/42/head:refs/pull/42/head", "--force", "--no-tags").Return(nil) + git.EXPECT().RevParse(gomock.Any(), "abc123^{commit}").Return("abc123", nil) + git.EXPECT().IsAncestor(gomock.Any(), "abc123", "pull/42/head").Return(true, nil) + git.EXPECT().Diff(gomock.Any(), "main", "abc123", "--binary", "--merge-base").Return(nil, nil) + git.EXPECT().ApplyPatch(gomock.Any(), gomock.Any()).Return(nil) + git.EXPECT().Commit(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + git.EXPECT().SubmoduleUpdate(gomock.Any()).Return(nil) + + req := NewGitRequest(git, "42", "main", "abc123", upstream, zap.NewNop().Sugar()) + err := req.Apply(context.Background()) + require.NoError(t, err) +} + +func TestGitRequest_Apply_PinsToDiffAgainstCommit(t *testing.T) { + // Verifies that the diff is computed against the pinned commit, not the + // floating PR head, ensuring stable tree materialization. + ctrl := gomock.NewController(t) + git := gitmock.NewMockInterface(ctrl) + + git.EXPECT().Fetch(gomock.Any(), _testUpstream, "+refs/pull/10/head:refs/pull/10/head", "--force", "--no-tags").Return(nil) + git.EXPECT().RevParse(gomock.Any(), "pinnedSHA^{commit}").Return("pinnedSHA", nil) + git.EXPECT().IsAncestor(gomock.Any(), "pinnedSHA", "pull/10/head").Return(true, nil) + // The key assertion: diff target is "pinnedSHA", not "pull/10/head" + git.EXPECT().Diff(gomock.Any(), "baseRef", "pinnedSHA", "--binary", "--merge-base").Return([]byte("patch"), nil) + git.EXPECT().ApplyPatch(gomock.Any(), []byte("patch")).Return(nil) + git.EXPECT().Commit(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + git.EXPECT().SubmoduleUpdate(gomock.Any()).Return(nil) + + req := NewGitRequest(git, "10", "baseRef", "pinnedSHA", _testUpstream, zap.NewNop().Sugar()) + err := req.Apply(context.Background()) + require.NoError(t, err) +} + +func TestGitRequest_Apply_FetchPRHeadFails(t *testing.T) { + ctrl := gomock.NewController(t) + git := gitmock.NewMockInterface(ctrl) + + git.EXPECT().Fetch(gomock.Any(), _testUpstream, gomock.Any(), "--force", "--no-tags").Return(errors.New("network error")) + + req := NewGitRequest(git, "99", "baseRef", "abc", _testUpstream, zap.NewNop().Sugar()) + err := req.Apply(context.Background()) + require.Error(t, err) +} + +func TestGitRequest_Apply_CommitMissingLocally_FallbackFetchSucceeds(t *testing.T) { + // When the commit object is not found locally (RevParse fails), a + // best-effort bare-SHA fetch is attempted. If that succeeds, Apply + // continues normally. + ctrl := gomock.NewController(t) + git := gitmock.NewMockInterface(ctrl) + + git.EXPECT().Fetch(gomock.Any(), _testUpstream, "+refs/pull/99/head:refs/pull/99/head", "--force", "--no-tags").Return(nil) + // RevParse fails -- commit not in local odb + git.EXPECT().RevParse(gomock.Any(), "abc^{commit}").Return("", errors.New("unknown revision")) + // Fallback bare-SHA fetch succeeds + git.EXPECT().Fetch(gomock.Any(), _testUpstream, "abc", "--force", "--no-tags").Return(nil) + git.EXPECT().IsAncestor(gomock.Any(), "abc", "pull/99/head").Return(true, nil) + git.EXPECT().Diff(gomock.Any(), "baseRef", "abc", "--binary", "--merge-base").Return(nil, nil) + git.EXPECT().ApplyPatch(gomock.Any(), gomock.Any()).Return(nil) + git.EXPECT().Commit(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + git.EXPECT().SubmoduleUpdate(gomock.Any()).Return(nil) + + req := NewGitRequest(git, "99", "baseRef", "abc", _testUpstream, zap.NewNop().Sugar()) + err := req.Apply(context.Background()) + require.NoError(t, err) +} + +func TestGitRequest_Apply_CommitMissingLocally_FallbackFetchFails(t *testing.T) { + // When neither local presence nor bare-SHA fetch can provide the commit, + // Apply returns an error. + ctrl := gomock.NewController(t) + git := gitmock.NewMockInterface(ctrl) + + git.EXPECT().Fetch(gomock.Any(), _testUpstream, "+refs/pull/99/head:refs/pull/99/head", "--force", "--no-tags").Return(nil) + git.EXPECT().RevParse(gomock.Any(), "abc^{commit}").Return("", errors.New("unknown revision")) + git.EXPECT().Fetch(gomock.Any(), _testUpstream, "abc", "--force", "--no-tags").Return(errors.New("upload-pack: not our ref")) + + req := NewGitRequest(git, "99", "baseRef", "abc", _testUpstream, zap.NewNop().Sugar()) + err := req.Apply(context.Background()) + require.Error(t, err) +} diff --git a/core/workspace/request.go b/core/workspace/request.go index c5bc2ef2..5b83a4fd 100644 --- a/core/workspace/request.go +++ b/core/workspace/request.go @@ -29,14 +29,19 @@ type Request interface { } // NewRequest creates a new request based on the request URL. -func NewRequest(rawURL string, g git.Interface, baseRef string, commit string, logger *zap.SugaredLogger) (Request, error) { +// +// upstreamRemote is the real remote URL (e.g. GitHub HTTPS/SSH) from which +// PR refs are fetched. It is threaded through to the underlying request +// implementation (e.g. gitRequest) so fetches target the upstream rather +// than the worker clone's local "origin". +func NewRequest(rawURL string, g git.Interface, baseRef string, commit string, upstreamRemote string, logger *zap.SugaredLogger) (Request, error) { u, err := url.Parse(rawURL) if err != nil { return nil, err } switch u.Scheme { case "github": - return NewGitRequest(g, u.Path, baseRef, commit, logger), nil + return NewGitRequest(g, u.Path, baseRef, commit, upstreamRemote, logger), nil } return nil, fmt.Errorf("unsupported scheme: %v", u.Scheme) } diff --git a/core/workspace/request_test.go b/core/workspace/request_test.go index 15db59ff..e3c887ec 100644 --- a/core/workspace/request_test.go +++ b/core/workspace/request_test.go @@ -26,13 +26,14 @@ func TestNewRequest_Github_Success(t *testing.T) { rawURL := "github://org/repo/pull/123" var g git.Interface = nil - req, err := NewRequest(rawURL, g, "baseRef", "abc123", zap.NewNop().Sugar()) + req, err := NewRequest(rawURL, g, "baseRef", "abc123", _testUpstream, zap.NewNop().Sugar()) require.NoError(t, err) require.NotNil(t, req) gr, ok := req.(*gitRequest) require.True(t, ok, "returned Request should be *gitRequest") require.Equal(t, "123", gr.requestID) require.Equal(t, "abc123", gr.commit) + require.Equal(t, _testUpstream, gr.upstreamRemote) require.Nil(t, gr.git) } @@ -40,7 +41,7 @@ func TestNewRequest_InvalidURL(t *testing.T) { rawURL := "://bad" var g git.Interface = nil - req, err := NewRequest(rawURL, g, "baseRef", "", zap.NewNop().Sugar()) + req, err := NewRequest(rawURL, g, "baseRef", "", "", zap.NewNop().Sugar()) require.Error(t, err) require.Nil(t, req) } @@ -49,7 +50,7 @@ func TestNewRequest_InvalidScheme(t *testing.T) { rawURL := "phabricator://bad" var g git.Interface = nil - req, err := NewRequest(rawURL, g, "baseRef", "", zap.NewNop().Sugar()) + req, err := NewRequest(rawURL, g, "baseRef", "", "", zap.NewNop().Sugar()) require.Error(t, err) require.Nil(t, req) } diff --git a/internal/mapper/build_description.go b/internal/mapper/build_description.go index a9f1ddd1..a3353d67 100644 --- a/internal/mapper/build_description.go +++ b/internal/mapper/build_description.go @@ -2,6 +2,7 @@ package mapper import ( "errors" + "fmt" "github.com/uber/tango/entity" "github.com/uber/tango/tangopb" @@ -21,27 +22,36 @@ func ProtoToBuildDescription(desc *tangopb.BuildDescription) (entity.BuildDescri if desc.GetBaseSha() == "" { return entity.BuildDescription{}, errors.New("build description base_sha is required") } + changeRequests, err := toChangeRequests(desc.GetRequests()) + if err != nil { + return entity.BuildDescription{}, err + } return entity.BuildDescription{ Remote: desc.GetRemote(), BaseSha: desc.GetBaseSha(), - ChangeRequests: toChangeRequests(desc.GetRequests()), + ChangeRequests: changeRequests, Strategy: toComputationStrategy(desc.GetStrategy()), }, nil } // toChangeRequests converts a slice of proto Request to domain ChangeRequests. -func toChangeRequests(requests []*tangopb.Request) []entity.ChangeRequest { +// Returns an error if any request is missing its commit field, since commit +// participates in the cache key and must pin the applied content. +func toChangeRequests(requests []*tangopb.Request) ([]entity.ChangeRequest, error) { if len(requests) == 0 { - return nil + return nil, nil } out := make([]entity.ChangeRequest, len(requests)) for i, r := range requests { + if r.GetCommit() == "" { + return nil, fmt.Errorf("request %d (%s): commit is required — it pins the applied content and forms the cache key", i, r.GetUrl()) + } out[i] = entity.ChangeRequest{ URL: r.GetUrl(), Commit: r.GetCommit(), } } - return out + return out, nil } // toComputationStrategy converts a proto ComputationStrategy to the domain ComputationStrategy. diff --git a/internal/mapper/build_description_test.go b/internal/mapper/build_description_test.go index 3bd1bb15..8978685b 100644 --- a/internal/mapper/build_description_test.go +++ b/internal/mapper/build_description_test.go @@ -37,6 +37,17 @@ func TestProtoToBuildDescription(t *testing.T) { desc: &tangopb.BuildDescription{Remote: "git@example.com:org/repo"}, wantErr: true, }, + { + name: "request missing commit", + desc: &tangopb.BuildDescription{ + Remote: "git@example.com:org/repo", + BaseSha: "abc123", + Requests: []*tangopb.Request{ + {Url: "https://example.com/pr/1"}, + }, + }, + wantErr: true, + }, { name: "full", desc: &tangopb.BuildDescription{ diff --git a/orchestrator/native_orchestrator.go b/orchestrator/native_orchestrator.go index c57e6c1a..d07b7a75 100644 --- a/orchestrator/native_orchestrator.go +++ b/orchestrator/native_orchestrator.go @@ -149,7 +149,7 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, req entity.GetT gitModule := gitFactory(ws.Path()) for _, req := range build.ChangeRequests { - request, err := workspace.NewRequest(req.URL, gitModule, build.BaseSha, req.Commit, logger) + request, err := workspace.NewRequest(req.URL, gitModule, build.BaseSha, req.Commit, build.Remote, logger) if err != nil { return nil, fmt.Errorf("create request for %q: %w", req.URL, err) }