fix(workspace): fetch PR refs from upstream and pin applied commit (audit #3)

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",

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)
 	}

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))
+}