From 671cabb8cfa650e97bfb7be94002ba4e99cc58c9 Mon Sep 17 00:00:00 2001 From: shixi-li Date: Fri, 24 Jul 2026 14:11:21 +0800 Subject: [PATCH 1/2] fix: refresh remote PR worktree bases --- src/isolation-manager.js | 231 ++++++++---- src/orchestrator.js | 10 +- tests/integration/worktree-isolation.test.js | 358 +++++++++++++++++++ tests/orchestrator.test.js | 7 +- 4 files changed, 528 insertions(+), 78 deletions(-) diff --git a/src/isolation-manager.js b/src/isolation-manager.js index 7a88df75..72b5fdb4 100644 --- a/src/isolation-manager.js +++ b/src/isolation-manager.js @@ -28,6 +28,7 @@ const { readRepoSettings } = require('../lib/repo-settings'); const { provisionClaudeCredentials } = require('./claude-credentials'); const DEFAULT_WORKTREE_SETUP_TIMEOUT_MS = 15 * 60 * 1000; +const FRESH_BASE_REF_PREFIX = 'refs/zeroshot/base-fetch'; function runSync(command, args, options = {}) { const timeout = options.timeout ?? 30000; @@ -48,6 +49,60 @@ function runShellSync(command, options = {}) { return runSync('/bin/bash', ['-lc', command], options); } +function resolveCommit(repoRoot, ref) { + return runSync('git', ['rev-parse', '--verify', `${ref}^{commit}`], { + cwd: repoRoot, + encoding: 'utf8', + stdio: 'pipe', + }).trim(); +} + +function deleteTemporaryBaseRef(repoRoot, temporaryRef) { + try { + runSync('git', ['update-ref', '-d', temporaryRef], { + cwd: repoRoot, + encoding: 'utf8', + stdio: 'pipe', + }); + } catch (err) { + console.warn( + `[IsolationManager] Warning: failed to remove temporary base ref ${temporaryRef}: ${err.message}` + ); + } +} + +function fetchFreshRemoteBase(repoRoot, branch) { + const remoteTrackingRef = `refs/remotes/origin/${branch}`; + const temporaryRef = `${FRESH_BASE_REF_PREFIX}/${crypto.randomBytes(16).toString('hex')}`; + + try { + runSync( + 'git', + [ + 'fetch', + '--atomic', + '--no-tags', + 'origin', + `+refs/heads/${branch}:${remoteTrackingRef}`, + `+refs/heads/${branch}:${temporaryRef}`, + ], + { + cwd: repoRoot, + encoding: 'utf8', + stdio: 'pipe', + } + ); + + return { + baseSha: resolveCommit(repoRoot, temporaryRef), + temporaryRef, + }; + } catch (err) { + deleteTemporaryBaseRef(repoRoot, temporaryRef); + throw err; + } +} + function expandHomePath(value) { if (!value) return value; if (value === '~') return os.homedir(); @@ -127,7 +182,7 @@ class IsolationManager { this.containers = new Map(); // clusterId -> containerId this.isolatedDirs = new Map(); // clusterId -> { path, originalDir } this.clusterConfigDirs = new Map(); // clusterId -> configDirPath - this.worktrees = new Map(); // clusterId -> { path, branch, repoRoot } + this.worktrees = new Map(); // clusterId -> { path, branch, repoRoot, baseRef, baseSha } this._exitWatchers = new Map(); // clusterId -> ChildProcess } @@ -1443,7 +1498,7 @@ class IsolationManager { * Creates a git worktree at ~/.zeroshot/worktrees/{clusterId} * @param {string} clusterId - Cluster ID * @param {string} workDir - Original working directory (must be a git repo) - * @returns {{ path: string, branch: string, repoRoot: string }} + * @returns {{ path: string, branch: string, repoRoot: string, baseRef: string, baseSha: string }} */ createWorktreeIsolation(clusterId, workDir, options = {}) { if (!this._isGitRepo(workDir)) { @@ -1485,8 +1540,9 @@ class IsolationManager { * @param {string} workDir - Original working directory * @param {object} [options] - Worktree creation options * @param {string} [options.baseRef] - Git ref to base the worktree branch on + * @param {boolean} [options.requireFreshBase=false] - Require a freshly fetched remote base * @param {number} [options.worktreeSetupTimeoutMs] - Setup command timeout in milliseconds - * @returns {{ path: string, branch: string, repoRoot: string }} + * @returns {{ path: string, branch: string, repoRoot: string, baseRef: string, baseSha: string }} */ createWorktree(clusterId, workDir, options = {}) { const repoRoot = this._getGitRoot(workDir); @@ -1560,20 +1616,41 @@ class IsolationManager { } const worktreeSetupTimeoutMs = resolveWorktreeSetupTimeoutMs(repoSettings, options); - // Best-effort ensure origin/ exists locally if requested. + const baseRef = worktreeBaseRef || 'HEAD'; + let baseSha; + let temporaryBaseRef = null; + if (worktreeBaseRef && worktreeBaseRef.startsWith('origin/')) { const branch = worktreeBaseRef.slice('origin/'.length); - try { - runSync('git', ['fetch', 'origin', branch], { - cwd: repoRoot, - encoding: 'utf8', - stdio: 'pipe', - }); - } catch { - // ignore + const remoteTrackingRef = `refs/remotes/origin/${branch}`; + + if (options.requireFreshBase === true) { + const freshBase = fetchFreshRemoteBase(repoRoot, branch); + baseSha = freshBase.baseSha; + temporaryBaseRef = freshBase.temporaryRef; + } else { + try { + runSync( + 'git', + ['fetch', '--no-tags', 'origin', `+refs/heads/${branch}:${remoteTrackingRef}`], + { + cwd: repoRoot, + encoding: 'utf8', + stdio: 'pipe', + } + ); + } catch { + // Repository worktree settings historically allowed an offline cached remote ref. + } + baseSha = resolveCommit(repoRoot, remoteTrackingRef); } + } else { + baseSha = resolveCommit(repoRoot, baseRef); } + console.log(`[IsolationManager] Worktree base ref: ${baseRef}`); + console.log(`[IsolationManager] Worktree base commit: ${baseSha}`); + // Create branch name from cluster ID (e.g., cluster-cosmic-meteor-87 -> zeroshot/cosmic-meteor-87) const baseBranchName = `zeroshot/${clusterId.replace(/^cluster-/, '')}`; let branchName = baseBranchName; @@ -1581,44 +1658,18 @@ class IsolationManager { // Worktree path in persistent location (survives reboots) const worktreePath = path.join(os.homedir(), '.zeroshot', 'worktrees', clusterId); - // Ensure parent directory exists - const parentDir = path.dirname(worktreePath); - if (!fs.existsSync(parentDir)) { - fs.mkdirSync(parentDir, { recursive: true }); - } - - // Best-effort cleanup of stale worktree metadata and directory. - // IMPORTANT: If a previous run deleted the directory without deregistering the worktree, - // git may keep the branch "checked out" and block deletion/reuse. try { - runSync('git', ['worktree', 'remove', '--force', worktreePath], { - cwd: repoRoot, - encoding: 'utf8', - stdio: 'pipe', - }); - } catch { - // ignore - } - try { - runSync('git', ['worktree', 'prune'], { cwd: repoRoot, encoding: 'utf8', stdio: 'pipe' }); - } catch { - // ignore - } - try { - fs.rmSync(worktreePath, { recursive: true, force: true }); - } catch { - // ignore - } - - const baseRef = worktreeBaseRef || 'HEAD'; - console.log(`[IsolationManager] Worktree base ref: ${baseRef}`); - console.log(`[IsolationManager] Worktree path: ${worktreePath}`); + // Ensure parent directory exists + const parentDir = path.dirname(worktreePath); + if (!fs.existsSync(parentDir)) { + fs.mkdirSync(parentDir, { recursive: true }); + } - // Create worktree with new branch based on baseRef (retry on branch collision/in-use) - for (let attempt = 0; attempt < 10; attempt++) { - // Best-effort delete if branch exists and is not in use by another worktree. + // Best-effort cleanup of stale worktree metadata and directory. + // IMPORTANT: If a previous run deleted the directory without deregistering the worktree, + // git may keep the branch "checked out" and block deletion/reuse. try { - runSync('git', ['branch', '-D', branchName], { + runSync('git', ['worktree', 'remove', '--force', worktreePath], { cwd: repoRoot, encoding: 'utf8', stdio: 'pipe', @@ -1626,38 +1677,72 @@ class IsolationManager { } catch { // ignore } - try { - runSync('git', ['worktree', 'add', '-b', branchName, worktreePath, baseRef], { + runSync('git', ['worktree', 'prune'], { cwd: repoRoot, encoding: 'utf8', stdio: 'pipe', }); - console.log(`[IsolationManager] Worktree setup phase: created branch ${branchName}`); - break; - } catch (err) { - const stderr = ( - err && (err.stderr || err.message) ? String(err.stderr || err.message) : '' - ).toLowerCase(); - const isBranchCollision = - stderr.includes('already exists') || - stderr.includes('cannot delete branch') || - stderr.includes('checked out'); - - if (attempt < 9 && isBranchCollision) { - branchName = `${baseBranchName}-${crypto.randomBytes(3).toString('hex')}`; - try { - runSync('git', ['worktree', 'prune'], { - cwd: repoRoot, - encoding: 'utf8', - stdio: 'pipe', - }); - } catch { - // ignore + } catch { + // ignore + } + try { + fs.rmSync(worktreePath, { recursive: true, force: true }); + } catch { + // ignore + } + + console.log(`[IsolationManager] Worktree path: ${worktreePath}`); + + // Create worktree with new branch based on baseRef (retry on branch collision/in-use) + for (let attempt = 0; attempt < 10; attempt++) { + // Best-effort delete if branch exists and is not in use by another worktree. + try { + runSync('git', ['branch', '-D', branchName], { + cwd: repoRoot, + encoding: 'utf8', + stdio: 'pipe', + }); + } catch { + // ignore + } + + try { + runSync('git', ['worktree', 'add', '-b', branchName, worktreePath, baseSha], { + cwd: repoRoot, + encoding: 'utf8', + stdio: 'pipe', + }); + console.log(`[IsolationManager] Worktree setup phase: created branch ${branchName}`); + break; + } catch (err) { + const stderr = ( + err && (err.stderr || err.message) ? String(err.stderr || err.message) : '' + ).toLowerCase(); + const isBranchCollision = + stderr.includes('already exists') || + stderr.includes('cannot delete branch') || + stderr.includes('checked out'); + + if (attempt < 9 && isBranchCollision) { + branchName = `${baseBranchName}-${crypto.randomBytes(3).toString('hex')}`; + try { + runSync('git', ['worktree', 'prune'], { + cwd: repoRoot, + encoding: 'utf8', + stdio: 'pipe', + }); + } catch { + // ignore + } + continue; } - continue; + throw err; } - throw err; + } + } finally { + if (temporaryBaseRef) { + deleteTemporaryBaseRef(repoRoot, temporaryBaseRef); } } @@ -1684,6 +1769,8 @@ class IsolationManager { path: worktreePath, branch: branchName, repoRoot, + baseRef, + baseSha, }; } diff --git a/src/orchestrator.js b/src/orchestrator.js index 7693f029..bdf2fd19 100644 --- a/src/orchestrator.js +++ b/src/orchestrator.js @@ -1796,18 +1796,20 @@ class Orchestrator { const workDir = options.cwd || process.cwd(); isolationManager = new IsolationManager({}); - // `prBase` is the PR target branch. Use that same local ref as the - // worktree base so repo-local integration commits are not skipped. + // `prBase` is the PR target branch. Base isolated work on its refreshed + // remote-tracking ref so a stale local branch cannot change the plan. const worktreeOptions = {}; if (options.prBase) { - worktreeOptions.baseRef = options.prBase; - this._log(`[Orchestrator] Using worktree base ref: ${options.prBase}`); + worktreeOptions.baseRef = `origin/${options.prBase}`; + worktreeOptions.requireFreshBase = true; + this._log(`[Orchestrator] Using worktree base ref: ${worktreeOptions.baseRef}`); } worktreeInfo = isolationManager.createWorktreeIsolation(clusterId, workDir, worktreeOptions); this._log(`[Orchestrator] Starting cluster in worktree isolation mode`); this._log(`[Orchestrator] Worktree: ${worktreeInfo.path}`); this._log(`[Orchestrator] Branch: ${worktreeInfo.branch}`); + this._log(`[Orchestrator] Worktree base commit: ${worktreeInfo.baseSha}`); } return { isolationManager, containerId, worktreeInfo, image: isolationImage }; diff --git a/tests/integration/worktree-isolation.test.js b/tests/integration/worktree-isolation.test.js index 63662f7d..a9e73bd2 100644 --- a/tests/integration/worktree-isolation.test.js +++ b/tests/integration/worktree-isolation.test.js @@ -18,6 +18,8 @@ const os = require('os'); const { spawnSync } = require('child_process'); const IsolationManager = require('../../src/isolation-manager'); +const Orchestrator = require('../../src/orchestrator'); +const { buildStartOptions } = require('../../lib/start-cluster'); let manager; let testRepoDir; @@ -54,6 +56,120 @@ function createTempGitRepo(prefix) { return repoDir; } +function createStaleRemoteBaseFixture(prefix) { + const fixtureRoot = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + const remoteDir = path.join(fixtureRoot, 'origin.git'); + const sourceDir = path.join(fixtureRoot, 'source'); + const publisherDir = path.join(fixtureRoot, 'publisher'); + + fs.mkdirSync(remoteDir); + runGit(['init', '--bare'], { cwd: remoteDir }); + + fs.mkdirSync(sourceDir); + runGit(['init'], { cwd: sourceDir }); + runGit(['config', 'user.email', 'test@test.com'], { cwd: sourceDir }); + runGit(['config', 'user.name', 'Test User'], { cwd: sourceDir }); + fs.writeFileSync(path.join(sourceDir, 'test.txt'), 'initial content'); + runGit(['add', 'test.txt'], { cwd: sourceDir }); + runGit(['commit', '-m', 'Initial commit'], { cwd: sourceDir }); + runGit(['branch', '-M', 'dev'], { cwd: sourceDir }); + runGit(['remote', 'add', 'origin', remoteDir], { cwd: sourceDir }); + runGit(['push', '-u', 'origin', 'dev'], { cwd: sourceDir }); + + runGit(['clone', '--branch', 'dev', remoteDir, publisherDir], { cwd: fixtureRoot }); + runGit(['config', 'user.email', 'test@test.com'], { cwd: publisherDir }); + runGit(['config', 'user.name', 'Test User'], { cwd: publisherDir }); + fs.writeFileSync(path.join(publisherDir, 'test.txt'), 'remote content'); + runGit(['add', 'test.txt'], { cwd: publisherDir }); + runGit(['commit', '-m', 'Advance remote'], { cwd: publisherDir }); + runGit(['push', 'origin', 'dev'], { cwd: publisherDir }); + + return { + fixtureRoot, + sourceDir, + localSha: runGit(['rev-parse', 'dev'], { cwd: sourceDir }).trim(), + remoteSha: runGit(['rev-parse', 'refs/heads/dev'], { cwd: remoteDir }).trim(), + }; +} + +function findGitBinary() { + for (const directory of (process.env.PATH || '').split(path.delimiter)) { + const candidate = path.join(directory, process.platform === 'win32' ? 'git.exe' : 'git'); + try { + fs.accessSync(candidate, fs.constants.X_OK); + return candidate; + } catch { + // Keep looking. + } + } + throw new Error('Unable to locate the git executable'); +} + +function listTemporaryBaseRefs(repoDir) { + return runGit(['for-each-ref', '--format=%(refname)', 'refs/zeroshot/base-fetch/'], { + cwd: repoDir, + }) + .trim() + .split('\n') + .filter(Boolean); +} + +function createGitInterventionShim( + fixtureRoot, + { repoDir, sharedRefSha = null, failWorktreeAdd = false } +) { + const shimDir = path.join(fixtureRoot, 'git-shim'); + const shimPath = path.join(shimDir, 'git'); + const realGit = findGitBinary(); + fs.mkdirSync(shimDir); + + const script = [ + '#!/usr/bin/env node', + "const { spawnSync } = require('child_process');", + `const realGit = ${JSON.stringify(realGit)};`, + `const repoDir = ${JSON.stringify(repoDir)};`, + `const sharedRefSha = ${JSON.stringify(sharedRefSha)};`, + `const failWorktreeAdd = ${JSON.stringify(failWorktreeAdd)};`, + 'const args = process.argv.slice(2);', + "if (args[0] === 'worktree' && args[1] === 'add') {", + ' const refs = spawnSync(', + ' realGit,', + " ['for-each-ref', '--format=%(refname)', 'refs/zeroshot/base-fetch/'],", + " { cwd: repoDir, encoding: 'utf8', stdio: 'pipe' }", + ' );', + ' if (refs.status !== 0 || refs.error || !refs.stdout.trim()) {', + " process.stderr.write('temporary base ref missing before worktree add\\n');", + ' process.exit(97);', + ' }', + ' if (failWorktreeAdd) {', + " process.stderr.write('injected worktree add failure\\n');", + ' process.exit(98);', + ' }', + ' if (sharedRefSha) {', + ' const update = spawnSync(', + ' realGit,', + " ['update-ref', 'refs/remotes/origin/dev', sharedRefSha],", + " { cwd: repoDir, encoding: 'utf8', stdio: 'pipe' }", + ' );', + ' if (update.status !== 0 || update.error) {', + " process.stderr.write(update.error?.message || update.stderr || 'ref update failed');", + ' process.exit(96);', + ' }', + ' }', + '}', + "const child = spawnSync(realGit, args, { stdio: 'inherit' });", + 'if (child.error) {', + ' process.stderr.write(child.error.message);', + ' process.exit(1);', + '}', + 'process.exit(child.status === null ? 1 : child.status);', + '', + ].join('\n'); + + fs.writeFileSync(shimPath, script, { mode: 0o755 }); + return shimDir; +} + function registerRepositoryHooks() { before(function () { testRepoDir = createTempGitRepo('zs-worktree-test-repo-'); @@ -88,6 +204,7 @@ function registerCreateWorktreeIsolationTests() { registerWorktreeNonGitTest(); registerWorktreeCleanupBeforeCreateTest(); registerWorktreeSetupCommandTest(); + registerRemotePrBaseTests(); }); } @@ -287,6 +404,247 @@ function registerWorktreeSetupCommandTest() { }); } +function registerRemotePrBaseTests() { + it('fetches the remote commit for direct and detached-derived PR options', async function () { + const orchestrator = new Orchestrator({ quiet: true, skipLoad: true }); + const originalRunOptions = process.env.ZEROSHOT_RUN_OPTIONS; + const originalCwd = process.env.ZEROSHOT_CWD; + + try { + const modes = [ + { + name: 'direct', + buildOptions: (fixture) => ({ + cwd: fixture.sourceDir, + worktree: true, + prBase: 'dev', + }), + }, + { + name: 'detached-derived', + buildOptions: (fixture) => { + process.env.ZEROSHOT_CWD = fixture.sourceDir; + process.env.ZEROSHOT_RUN_OPTIONS = JSON.stringify({ + ship: true, + prBase: 'dev', + }); + return buildStartOptions({ + clusterId: 'detached-remote-pr-base', + options: {}, + settings: {}, + }); + }, + }, + ]; + + for (const mode of modes) { + const fixture = createStaleRemoteBaseFixture(`zs-${mode.name}-remote-pr-base-`); + const clusterId = `test-${mode.name}-remote-pr-base-${Date.now()}`; + + assert.notStrictEqual(fixture.localSha, fixture.remoteSha, 'local dev must be stale'); + + let result; + try { + result = await orchestrator._initializeIsolation( + mode.buildOptions(fixture), + {}, + clusterId + ); + const worktreeSha = runGit(['rev-parse', 'HEAD'], { + cwd: result.worktreeInfo.path, + }).trim(); + assert.strictEqual( + worktreeSha, + fixture.remoteSha, + `${mode.name} worktree must start at the fetched origin/dev commit` + ); + assert.strictEqual(result.worktreeInfo.baseRef, 'origin/dev'); + assert.strictEqual(result.worktreeInfo.baseSha, fixture.remoteSha); + assert.deepStrictEqual(listTemporaryBaseRefs(fixture.sourceDir), []); + } finally { + if (result) { + result.isolationManager.cleanupWorktreeIsolation(clusterId); + } + fs.rmSync(fixture.fixtureRoot, { recursive: true, force: true }); + } + } + } finally { + orchestrator.close(); + if (originalRunOptions === undefined) delete process.env.ZEROSHOT_RUN_OPTIONS; + else process.env.ZEROSHOT_RUN_OPTIONS = originalRunOptions; + if (originalCwd === undefined) delete process.env.ZEROSHOT_CWD; + else process.env.ZEROSHOT_CWD = originalCwd; + } + }); + + it('resolves a remote base through its full ref when a local branch shadows it', function () { + const fixture = createStaleRemoteBaseFixture('zs-shadowed-pr-base-'); + const clusterId = `test-shadowed-remote-pr-base-${Date.now()}`; + const remoteManager = new IsolationManager(); + + try { + runGit(['branch', 'origin/dev', fixture.localSha], { cwd: fixture.sourceDir }); + + const info = remoteManager.createWorktreeIsolation(clusterId, fixture.sourceDir, { + baseRef: 'origin/dev', + requireFreshBase: true, + }); + const worktreeSha = runGit(['rev-parse', 'HEAD'], { cwd: info.path }).trim(); + const localShadowSha = runGit(['rev-parse', 'refs/heads/origin/dev^{commit}'], { + cwd: fixture.sourceDir, + }).trim(); + const remoteTrackingSha = runGit(['rev-parse', 'refs/remotes/origin/dev^{commit}'], { + cwd: fixture.sourceDir, + }).trim(); + + assert.strictEqual(localShadowSha, fixture.localSha); + assert.strictEqual(remoteTrackingSha, fixture.remoteSha); + assert.strictEqual(info.baseSha, fixture.remoteSha); + assert.strictEqual(worktreeSha, fixture.remoteSha); + assert.deepStrictEqual(listTemporaryBaseRefs(fixture.sourceDir), []); + } finally { + remoteManager.cleanupWorktreeIsolation(clusterId); + fs.rmSync(fixture.fixtureRoot, { recursive: true, force: true }); + } + }); + + it('fails closed when the requested remote base cannot be refreshed', function () { + const fixture = createStaleRemoteBaseFixture('zs-unavailable-pr-base-'); + const clusterId = `test-unavailable-remote-pr-base-${Date.now()}`; + const unavailableRemote = path.join(fixture.fixtureRoot, 'missing-origin.git'); + const remoteManager = new IsolationManager(); + + try { + runGit(['remote', 'set-url', 'origin', unavailableRemote], { cwd: fixture.sourceDir }); + assert.throws( + () => + remoteManager.createWorktreeIsolation(clusterId, fixture.sourceDir, { + baseRef: 'origin/dev', + requireFreshBase: true, + }), + /Command git failed/, + 'an unavailable remote must not fall back to a stale origin/dev ref' + ); + assert.deepStrictEqual(listTemporaryBaseRefs(fixture.sourceDir), []); + } finally { + remoteManager.cleanupWorktreeIsolation(clusterId); + fs.rmSync(fixture.fixtureRoot, { recursive: true, force: true }); + } + }); + + it('uses a cached remote-tracking ref for offline repository worktree settings', function () { + const fixture = createStaleRemoteBaseFixture('zs-offline-repo-base-'); + const clusterId = `test-offline-repo-base-${Date.now()}`; + const unavailableRemote = path.join(fixture.fixtureRoot, 'missing-origin.git'); + const remoteManager = new IsolationManager(); + const settingsDir = path.join(fixture.sourceDir, '.zeroshot'); + + try { + fs.mkdirSync(settingsDir); + fs.writeFileSync( + path.join(settingsDir, 'settings.json'), + JSON.stringify({ worktree: { baseRef: 'origin/dev' } }) + ); + runGit(['add', '.zeroshot/settings.json'], { cwd: fixture.sourceDir }); + runGit(['commit', '-m', 'Add worktree base setting'], { cwd: fixture.sourceDir }); + runGit(['remote', 'set-url', 'origin', unavailableRemote], { cwd: fixture.sourceDir }); + + const info = remoteManager.createWorktreeIsolation(clusterId, fixture.sourceDir); + const worktreeSha = runGit(['rev-parse', 'HEAD'], { cwd: info.path }).trim(); + + assert.strictEqual(info.baseRef, 'origin/dev'); + assert.strictEqual(info.baseSha, fixture.localSha); + assert.strictEqual(worktreeSha, fixture.localSha); + assert.deepStrictEqual(listTemporaryBaseRefs(fixture.sourceDir), []); + } finally { + remoteManager.cleanupWorktreeIsolation(clusterId); + fs.rmSync(fixture.fixtureRoot, { recursive: true, force: true }); + } + }); + + it('keeps the captured PR base when the shared remote-tracking ref changes', function () { + if (process.platform === 'win32') { + this.skip(); + } + + const fixture = createStaleRemoteBaseFixture('zs-concurrent-pr-base-'); + const clusterId = `test-concurrent-remote-pr-base-${Date.now()}`; + const remoteManager = new IsolationManager(); + const originalPath = process.env.PATH; + const treeSha = runGit(['rev-parse', `${fixture.localSha}^{tree}`], { + cwd: fixture.sourceDir, + }).trim(); + const interferenceSha = runGit( + ['commit-tree', treeSha, '-p', fixture.localSha, '-m', 'Interference commit'], + { cwd: fixture.sourceDir } + ).trim(); + const shimDir = createGitInterventionShim(fixture.fixtureRoot, { + repoDir: fixture.sourceDir, + sharedRefSha: interferenceSha, + }); + + try { + process.env.PATH = `${shimDir}${path.delimiter}${originalPath || ''}`; + const info = remoteManager.createWorktreeIsolation(clusterId, fixture.sourceDir, { + baseRef: 'origin/dev', + requireFreshBase: true, + }); + process.env.PATH = originalPath; + + const worktreeSha = runGit(['rev-parse', 'HEAD'], { cwd: info.path }).trim(); + const remoteTrackingSha = runGit(['rev-parse', 'refs/remotes/origin/dev^{commit}'], { + cwd: fixture.sourceDir, + }).trim(); + + assert.strictEqual(remoteTrackingSha, interferenceSha); + assert.strictEqual(info.baseSha, fixture.remoteSha); + assert.strictEqual(worktreeSha, fixture.remoteSha); + assert.deepStrictEqual(listTemporaryBaseRefs(fixture.sourceDir), []); + } finally { + process.env.PATH = originalPath; + remoteManager.cleanupWorktreeIsolation(clusterId); + fs.rmSync(fixture.fixtureRoot, { recursive: true, force: true }); + } + }); + + it('removes the temporary PR base ref when worktree creation fails', function () { + if (process.platform === 'win32') { + this.skip(); + } + + const fixture = createStaleRemoteBaseFixture('zs-failed-pr-base-'); + const clusterId = `test-failed-remote-pr-base-${Date.now()}`; + const remoteManager = new IsolationManager(); + const originalPath = process.env.PATH; + const shimDir = createGitInterventionShim(fixture.fixtureRoot, { + repoDir: fixture.sourceDir, + failWorktreeAdd: true, + }); + const worktreePath = path.join(os.homedir(), '.zeroshot', 'worktrees', clusterId); + + try { + process.env.PATH = `${shimDir}${path.delimiter}${originalPath || ''}`; + assert.throws( + () => + remoteManager.createWorktreeIsolation(clusterId, fixture.sourceDir, { + baseRef: 'origin/dev', + requireFreshBase: true, + }), + /injected worktree add failure/, + 'worktree creation should surface the original failure' + ); + process.env.PATH = originalPath; + + assert.deepStrictEqual(listTemporaryBaseRefs(fixture.sourceDir), []); + assert.strictEqual(fs.existsSync(worktreePath), false); + } finally { + process.env.PATH = originalPath; + remoteManager.cleanupWorktreeIsolation(clusterId); + fs.rmSync(fixture.fixtureRoot, { recursive: true, force: true }); + } + }); +} + function registerCleanupWorktreeIsolationTests() { describe('cleanupWorktreeIsolation()', function () { registerCleanupWorktreeDirectoryTest(); diff --git a/tests/orchestrator.test.js b/tests/orchestrator.test.js index f00d4683..0c06a2d2 100644 --- a/tests/orchestrator.test.js +++ b/tests/orchestrator.test.js @@ -737,7 +737,7 @@ function defineLifecycleStartTests() { assert.strictEqual(options.containerHome, '/tmp/home'); }); - it('should base PR worktrees on the local PR base branch', async function () { + it('should base PR worktrees on the remote PR base branch', async function () { const config = createSimpleConfig(); const originalCreateWorktreeIsolation = IsolationManager.prototype.createWorktreeIsolation; const calls = []; @@ -767,7 +767,10 @@ function defineLifecycleStartTests() { assert.strictEqual(calls.length, 1); assert.strictEqual(calls[0].workDir, '/tmp/repo'); - assert.deepStrictEqual(calls[0].options, { baseRef: 'predev' }); + assert.deepStrictEqual(calls[0].options, { + baseRef: 'origin/predev', + requireFreshBase: true, + }); }); it('should handle missing input (requires issue, file, or text)', async function () { From bc084e88e9668dceb99b6c69d0cec948fa064388 Mon Sep 17 00:00:00 2001 From: shixi-li Date: Fri, 24 Jul 2026 15:47:52 +0800 Subject: [PATCH 2/2] fix: isolate fresh PR base fetches --- src/isolation-manager.js | 4 +- tests/integration/worktree-isolation.test.js | 47 +++++++++++++++++++- 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/src/isolation-manager.js b/src/isolation-manager.js index 72b5fdb4..b9b0010a 100644 --- a/src/isolation-manager.js +++ b/src/isolation-manager.js @@ -72,7 +72,6 @@ function deleteTemporaryBaseRef(repoRoot, temporaryRef) { } function fetchFreshRemoteBase(repoRoot, branch) { - const remoteTrackingRef = `refs/remotes/origin/${branch}`; const temporaryRef = `${FRESH_BASE_REF_PREFIX}/${crypto.randomBytes(16).toString('hex')}`; try { @@ -82,8 +81,9 @@ function fetchFreshRemoteBase(repoRoot, branch) { 'fetch', '--atomic', '--no-tags', + '--no-write-fetch-head', + '--refmap=', 'origin', - `+refs/heads/${branch}:${remoteTrackingRef}`, `+refs/heads/${branch}:${temporaryRef}`, ], { diff --git a/tests/integration/worktree-isolation.test.js b/tests/integration/worktree-isolation.test.js index a9e73bd2..20698ebd 100644 --- a/tests/integration/worktree-isolation.test.js +++ b/tests/integration/worktree-isolation.test.js @@ -477,7 +477,7 @@ function registerRemotePrBaseTests() { } }); - it('resolves a remote base through its full ref when a local branch shadows it', function () { + it('resolves a remote base through its full ref without updating the shared tracking ref', function () { const fixture = createStaleRemoteBaseFixture('zs-shadowed-pr-base-'); const clusterId = `test-shadowed-remote-pr-base-${Date.now()}`; const remoteManager = new IsolationManager(); @@ -498,7 +498,7 @@ function registerRemotePrBaseTests() { }).trim(); assert.strictEqual(localShadowSha, fixture.localSha); - assert.strictEqual(remoteTrackingSha, fixture.remoteSha); + assert.strictEqual(remoteTrackingSha, fixture.localSha); assert.strictEqual(info.baseSha, fixture.remoteSha); assert.strictEqual(worktreeSha, fixture.remoteSha); assert.deepStrictEqual(listTemporaryBaseRefs(fixture.sourceDir), []); @@ -607,6 +607,49 @@ function registerRemotePrBaseTests() { } }); + it('does not contend on the shared remote-tracking ref when refreshing a PR base', function () { + if (process.platform === 'win32') { + this.skip(); + } + + const fixture = createStaleRemoteBaseFixture('zs-locked-remote-pr-base-'); + const clusterId = `test-locked-remote-pr-base-${Date.now()}`; + const remoteManager = new IsolationManager(); + const gitDirValue = runGit(['rev-parse', '--git-dir'], { cwd: fixture.sourceDir }).trim(); + const gitDir = path.resolve(fixture.sourceDir, gitDirValue); + const sharedRefLock = path.join(gitDir, 'refs', 'remotes', 'origin', 'dev.lock'); + const fetchHeadPath = path.join(gitDir, 'FETCH_HEAD'); + const fetchHeadBefore = fs.existsSync(fetchHeadPath) + ? fs.readFileSync(fetchHeadPath, 'utf8') + : null; + + try { + fs.writeFileSync(sharedRefLock, 'held by concurrent fetch\n'); + + const info = remoteManager.createWorktreeIsolation(clusterId, fixture.sourceDir, { + baseRef: 'origin/dev', + requireFreshBase: true, + }); + const worktreeSha = runGit(['rev-parse', 'HEAD'], { cwd: info.path }).trim(); + const remoteTrackingSha = runGit(['rev-parse', 'refs/remotes/origin/dev^{commit}'], { + cwd: fixture.sourceDir, + }).trim(); + const fetchHeadAfter = fs.existsSync(fetchHeadPath) + ? fs.readFileSync(fetchHeadPath, 'utf8') + : null; + + assert.strictEqual(remoteTrackingSha, fixture.localSha); + assert.strictEqual(info.baseSha, fixture.remoteSha); + assert.strictEqual(worktreeSha, fixture.remoteSha); + assert.strictEqual(fetchHeadAfter, fetchHeadBefore); + assert.deepStrictEqual(listTemporaryBaseRefs(fixture.sourceDir), []); + } finally { + fs.rmSync(sharedRefLock, { force: true }); + remoteManager.cleanupWorktreeIsolation(clusterId); + fs.rmSync(fixture.fixtureRoot, { recursive: true, force: true }); + } + }); + it('removes the temporary PR base ref when worktree creation fails', function () { if (process.platform === 'win32') { this.skip();