From 825416530d1540a7de712ace3093fdd13377d1ca Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Thu, 30 Jul 2026 12:18:52 -0700 Subject: [PATCH] fix(workspace): pin applied content to the URI head SHA Apply diffed against the floating pull/{id}/head ref, so two requests carrying the same change URI could materialize different trees under the same cache key if the PR advanced between them. Diff against the pinned head SHA from the URI instead; the ancestor check still guards against stale or foreign SHAs. Add real-git tests (local bare repo with refs/pull/1/head): pinned content applies end-to-end, the tree is stable across PR advancement, and a non-ancestor SHA is rejected. --- core/workspace/BUILD.bazel | 1 + core/workspace/gitrequest.go | 4 +- core/workspace/gitrequest_realgit_test.go | 161 ++++++++++++++++++++++ core/workspace/gitrequest_test.go | 14 ++ 4 files changed, 179 insertions(+), 1 deletion(-) create mode 100644 core/workspace/gitrequest_realgit_test.go diff --git a/core/workspace/BUILD.bazel b/core/workspace/BUILD.bazel index 8a48972b..af29035b 100644 --- a/core/workspace/BUILD.bazel +++ b/core/workspace/BUILD.bazel @@ -19,6 +19,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 304492a0..f8d18659 100644 --- a/core/workspace/gitrequest.go +++ b/core/workspace/gitrequest.go @@ -58,7 +58,9 @@ func (r *gitRequest) Apply(ctx context.Context) error { if !isAncestor { return fmt.Errorf("head SHA %q is not an ancestor of PR %s", r.headSHA, r.requestID) } - patch, err := r.git.Diff(ctx, r.baseRef, fmt.Sprintf("pull/%s/head", r.requestID), "--binary", "--merge-base") + // Diff against the pinned head SHA so the materialized tree is + // deterministic for a given change URI even as the PR advances. + patch, err := r.git.Diff(ctx, r.baseRef, r.headSHA, "--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..bcfbb976 --- /dev/null +++ b/core/workspace/gitrequest_realgit_test.go @@ -0,0 +1,161 @@ +// Copyright (c) 2026 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" + "strings" + "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" +) + +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 strings.TrimSpace(string(out)) +} + +func setupBareRepoWithPR(t *testing.T, prContent string) (bareDir, baseSHA, prSHA string) { + t.Helper() + + workDir := filepath.Join(t.TempDir(), "work") + bareDir = filepath.Join(t.TempDir(), "bare.git") + + require.NoError(t, os.MkdirAll(workDir, 0o755)) + runGit(t, workDir, "init", "-b", "main") + + 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") + baseSHA = runGit(t, workDir, "rev-parse", "HEAD") + + 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) + prSHA = runGit(t, workDir, "rev-parse", "HEAD") + + runGit(t, t.TempDir(), "clone", "--bare", workDir, bareDir) + runGit(t, workDir, "push", bareDir, "HEAD:refs/pull/1/head") + runGit(t, bareDir, "branch", "-D", "pr-branch") + + return bareDir, baseSHA, prSHA +} + +func cloneWorker(t *testing.T, bareDir, baseSHA string) string { + t.Helper() + workerDir := filepath.Join(t.TempDir(), "worker") + runGit(t, t.TempDir(), "clone", bareDir, workerDir) + runGit(t, workerDir, "checkout", baseSHA) + // git.Interface commits need a repo-local identity: the Bazel test + // sandbox has no global git config. + runGit(t, workerDir, "config", "user.name", "test") + runGit(t, workerDir, "config", "user.email", "test@test.com") + return workerDir +} + +func advancePR(t *testing.T, bareDir, newContent string) { + t.Helper() + advanceDir := filepath.Join(t.TempDir(), "advance") + runGit(t, t.TempDir(), "clone", bareDir, advanceDir) + runGit(t, advanceDir, "fetch", "origin", "refs/pull/1/head:refs/pull/1/head") + runGit(t, advanceDir, "checkout", "refs/pull/1/head") + require.NoError(t, os.WriteFile(filepath.Join(advanceDir, "pr.txt"), []byte(newContent), 0o644)) + runGit(t, advanceDir, "add", "pr.txt") + runGit(t, advanceDir, "commit", "-m", "advance PR") + runGit(t, advanceDir, "push", bareDir, "HEAD:refs/pull/1/head") +} + +func TestGitRequest_RealGit_AppliesPinnedContent(t *testing.T) { + t.Parallel() + ctx := context.Background() + logger := zap.NewNop().Sugar() + + bareDir, baseSHA, prSHA := setupBareRepoWithPR(t, "pr-content") + workerDir := cloneWorker(t, bareDir, baseSHA) + + req := workspace.NewGitRequest(git.New(workerDir, logger), "1", baseSHA, prSHA, logger) + require.NoError(t, req.Apply(ctx)) + + content, err := os.ReadFile(filepath.Join(workerDir, "pr.txt")) + require.NoError(t, err) + assert.Equal(t, "pr-content", string(content)) +} + +func TestGitRequest_RealGit_StableTreeAcrossPRAdvance(t *testing.T) { + t.Parallel() + ctx := context.Background() + logger := zap.NewNop().Sugar() + + bareDir, baseSHA, prSHA1 := setupBareRepoWithPR(t, "version-1") + + worker1 := cloneWorker(t, bareDir, baseSHA) + g1 := git.New(worker1, logger) + require.NoError(t, workspace.NewGitRequest(g1, "1", baseSHA, prSHA1, logger).Apply(ctx)) + tree1, err := g1.RevParse(ctx, "HEAD^{tree}") + require.NoError(t, err) + + advancePR(t, bareDir, "version-2") + + worker2 := cloneWorker(t, bareDir, baseSHA) + g2 := git.New(worker2, logger) + require.NoError(t, workspace.NewGitRequest(g2, "1", baseSHA, prSHA1, logger).Apply(ctx)) + tree2, err := g2.RevParse(ctx, "HEAD^{tree}") + require.NoError(t, err) + + assert.Equal(t, tree1, tree2, "the pinned head SHA must yield the same tree regardless of PR head advancement") + + content, err := os.ReadFile(filepath.Join(worker2, "pr.txt")) + require.NoError(t, err) + assert.Equal(t, "version-1", string(content), "the applied content must be the pinned version, not the advanced head") +} + +func TestGitRequest_RealGit_RejectsNonAncestorSHA(t *testing.T) { + t.Parallel() + ctx := context.Background() + logger := zap.NewNop().Sugar() + + bareDir, baseSHA, _ := setupBareRepoWithPR(t, "original") + + sideDir := filepath.Join(t.TempDir(), "side") + runGit(t, t.TempDir(), "clone", bareDir, sideDir) + runGit(t, sideDir, "checkout", "-b", "side", baseSHA) + require.NoError(t, os.WriteFile(filepath.Join(sideDir, "other.txt"), []byte("different"), 0o644)) + runGit(t, sideDir, "add", "other.txt") + runGit(t, sideDir, "commit", "-m", "side commit") + sideSHA := runGit(t, sideDir, "rev-parse", "HEAD") + runGit(t, sideDir, "push", bareDir, "HEAD:refs/heads/side") + + workerDir := cloneWorker(t, bareDir, baseSHA) + req := workspace.NewGitRequest(git.New(workerDir, logger), "1", baseSHA, sideSHA, logger) + require.Error(t, req.Apply(ctx)) +} diff --git a/core/workspace/gitrequest_test.go b/core/workspace/gitrequest_test.go index efcdbda0..b06e6333 100644 --- a/core/workspace/gitrequest_test.go +++ b/core/workspace/gitrequest_test.go @@ -70,3 +70,17 @@ func TestGitRequest_Apply_IsAncestorFails_ReturnsError(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "failed to read PR commit history") } + +func TestGitRequest_Apply_DiffsAgainstPinnedHeadSHA(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().IsAncestor(gomock.Any(), "pinnedsha", "pull/10/head").Return(true, nil) + 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", zap.NewNop().Sugar()) + err := req.Apply(context.Background()) + require.NoError(t, err) +}