fix: validate base branch when reusing worktrees (coleam00#963)

Wirasm · Wirasm · commit 4211ea69f984 · 2026-04-02T19:53:24.000+03:00
Reused worktrees were never checked against the configured baseBranch, so PRs could silently target the wrong base after a config change. Changes: - Add isAncestorOf() git helper using merge-base --is-ancestor - Add baseBranch field to IsolationHints for opt-in validation - Validate base branch in resolver checkExisting and findReusable paths - Add base branch warning in CLI worktree reuse path - Add tests for isAncestorOf and resolver base branch mismatch Fixes coleam00#963
diff --git a/packages/cli/src/commands/workflow.ts b/packages/cli/src/commands/workflow.ts
@@ -429,6 +429,25 @@ export async function workflowRunCommand(
             `--from ${options.fromBranch} was not applied (worktree already exists).`
         );
       }
+      // Validate base branch before reuse
+      const repoConfig = await loadRepoConfig(codebase.default_cwd);
+      const configuredBase =
+        repoConfig?.worktree?.baseBranch ??
+        (await git.getDefaultBranch(git.toRepoPath(codebase.default_cwd)));
+      const isValidBase = await git.isAncestorOf(
+        git.toWorktreePath(existingEnv.working_path),
+        `origin/${configuredBase}`
+      );
+      if (!isValidBase) {
+        getLog().warn(
+          { path: existingEnv.working_path, configuredBase, branch: existingEnv.branch_name },
+          'worktree.reuse_base_branch_mismatch'
+        );
+        console.warn(
+          `Warning: Worktree '${existingEnv.branch_name}' is not based on '${configuredBase}'. ` +
+            `Recreate with: bun run cli complete ${existingEnv.branch_name} --force`
+        );
+      }
       getLog().info({ path: existingEnv.working_path }, 'worktree_reused');
       workingCwd = existingEnv.working_path;
       isolationEnvId = existingEnv.id;
diff --git a/packages/git/src/branch.ts b/packages/git/src/branch.ts
@@ -220,6 +220,47 @@ export async function isBranchMerged(
   }
 }
 
+/**
+ * Check if a ref is an ancestor of HEAD in the given working directory.
+ *
+ * Returns true if ancestorRef is an ancestor of HEAD (worktree is based on that branch).
+ * Returns false if it is not (base branch mismatch detected).
+ * Returns false for expected errors (branch not found, not a git repo).
+ * Throws for unexpected errors (permission denied, corruption).
+ */
+export async function isAncestorOf(
+  workingPath: RepoPath | WorktreePath,
+  ancestorRef: string
+): Promise<boolean> {
+  try {
+    await execFileAsync('git', [
+      '-C',
+      workingPath,
+      'merge-base',
+      '--is-ancestor',
+      ancestorRef,
+      'HEAD',
+    ]);
+    return true;
+  } catch (error) {
+    const err = error as Error & { code?: number | string; stderr?: string };
+    // exit code 1 = not an ancestor — expected case
+    if (err.code === 1) return false;
+    const errorText = `${err.message} ${err.stderr ?? ''}`.toLowerCase();
+    const isExpectedError =
+      errorText.includes('not a git repository') ||
+      errorText.includes('unknown revision') ||
+      errorText.includes('not a valid object name') ||
+      errorText.includes('no such file') ||
+      err.code === 'ENOENT';
+    if (isExpectedError) return false;
+    getLog().error({ err: error, workingPath, ancestorRef }, 'branch.ancestor_check_failed');
+    throw new Error(
+      `Failed to check if ${ancestorRef} is ancestor of HEAD at ${workingPath}: ${(err as Error).message}`
+    );
+  }
+}
+
 /**
  * Get the last commit date for a repository or worktree.
  *
diff --git a/packages/git/src/git.test.ts b/packages/git/src/git.test.ts
@@ -1139,6 +1139,79 @@ branch refs/heads/feature/auth
     });
   });
 
+  describe('isAncestorOf', () => {
+    let execSpy: Mock<typeof git.execFileAsync>;
+
+    beforeEach(() => {
+      execSpy = spyOn(git, 'execFileAsync');
+    });
+
+    afterEach(() => {
+      execSpy.mockRestore();
+    });
+
+    test('returns true when ref is ancestor of HEAD (exit 0)', async () => {
+      execSpy.mockResolvedValue({ stdout: '', stderr: '' });
+
+      const result = await git.isAncestorOf('/worktrees/feature' as git.WorktreePath, 'origin/dev');
+      expect(result).toBe(true);
+      expect(execSpy).toHaveBeenCalledWith('git', [
+        '-C',
+        '/worktrees/feature',
+        'merge-base',
+        '--is-ancestor',
+        'origin/dev',
+        'HEAD',
+      ]);
+    });
+
+    test('returns false when ref is not ancestor of HEAD (exit code 1)', async () => {
+      const err = Object.assign(new Error(''), { code: 1, stderr: '' });
+      execSpy.mockRejectedValue(err);
+
+      const result = await git.isAncestorOf(
+        '/worktrees/feature' as git.WorktreePath,
+        'origin/main'
+      );
+      expect(result).toBe(false);
+    });
+
+    test('returns false on expected errors (not a git repository)', async () => {
+      execSpy.mockRejectedValue(new Error('fatal: not a git repository'));
+
+      const result = await git.isAncestorOf(
+        '/worktrees/feature' as git.WorktreePath,
+        'origin/main'
+      );
+      expect(result).toBe(false);
+    });
+
+    test('returns false on expected errors (unknown revision)', async () => {
+      execSpy.mockRejectedValue(
+        Object.assign(new Error('unknown revision'), { stderr: 'unknown revision' })
+      );
+
+      const result = await git.isAncestorOf(
+        '/worktrees/feature' as git.WorktreePath,
+        'origin/missing'
+      );
+      expect(result).toBe(false);
+    });
+
+    test('throws and logs on unexpected errors', async () => {
+      mockLogger.error.mockClear();
+      execSpy.mockRejectedValue(new Error('fatal: permission denied'));
+
+      await expect(
+        git.isAncestorOf('/worktrees/feature' as git.WorktreePath, 'origin/dev')
+      ).rejects.toThrow('Failed to check if origin/dev is ancestor of HEAD');
+      expect(mockLogger.error).toHaveBeenCalledWith(
+        expect.objectContaining({ ancestorRef: 'origin/dev' }),
+        'branch.ancestor_check_failed'
+      );
+    });
+  });
+
   // ==========================================================================
   // repo.ts
   // ==========================================================================
diff --git a/packages/git/src/index.ts b/packages/git/src/index.ts
@@ -33,6 +33,7 @@ export {
   hasUncommittedChanges,
   commitAllChanges,
   isBranchMerged,
+  isAncestorOf,
   getLastCommitDate,
 } from './branch';
 
diff --git a/packages/isolation/src/resolver.test.ts b/packages/isolation/src/resolver.test.ts
@@ -687,3 +687,107 @@ describe('IsolationResolver', () => {
     ).toThrow('staleThresholdDays must be positive, got -1');
   });
 });
+  test('existing env — emits warning when base branch mismatches', async () => {
+    const env = makeEnvRow();
+    isAncestorOfSpy.mockResolvedValue(false);
+    const resolver = createResolver({
+      store: makeMockStore({ getById: async () => env }),
+    });
+
+    const result = await resolver.resolve({
+      existingEnvId: 'env-1',
+      codebase: defaultCodebase,
+      hints: { baseBranch: 'dev' },
+      platformType: 'web',
+    });
+
+    expect(result.status).toBe('resolved');
+    if (result.status === 'resolved') {
+      expect(result.warnings).toBeDefined();
+      expect(result.warnings?.[0]).toContain("not based on 'dev'");
+    }
+  });
+
+  test('existing env — no warning when base branch matches', async () => {
+    const env = makeEnvRow();
+    isAncestorOfSpy.mockResolvedValue(true);
+    const resolver = createResolver({
+      store: makeMockStore({ getById: async () => env }),
+    });
+
+    const result = await resolver.resolve({
+      existingEnvId: 'env-1',
+      codebase: defaultCodebase,
+      hints: { baseBranch: 'dev' },
+      platformType: 'web',
+    });
+
+    expect(result.status).toBe('resolved');
+    if (result.status === 'resolved') {
+      expect(result.warnings).toBeUndefined();
+    }
+  });
+
+  test('workflow reuse — emits warning when base branch mismatches', async () => {
+    const env = makeEnvRow();
+    isAncestorOfSpy.mockResolvedValue(false);
+    const resolver = createResolver({
+      store: makeMockStore({
+        findActiveByWorkflow: async (_cid, wt, wid) =>
+          wt === 'issue' && wid === '42' ? env : null,
+      }),
+    });
+
+    const result = await resolver.resolve({
+      existingEnvId: null,
+      codebase: defaultCodebase,
+      hints: { workflowType: 'issue', workflowId: '42', baseBranch: 'dev' },
+      platformType: 'web',
+    });
+
+    expect(result.status).toBe('resolved');
+    if (result.status === 'resolved') {
+      expect(result.warnings).toBeDefined();
+      expect(result.warnings?.[0]).toContain("not based on 'dev'");
+    }
+  });
+
+  test('existing env — no base branch check when baseBranch not in hints', async () => {
+    const env = makeEnvRow();
+    const resolver = createResolver({
+      store: makeMockStore({ getById: async () => env }),
+    });
+
+    await resolver.resolve({
+      existingEnvId: 'env-1',
+      codebase: defaultCodebase,
+      platformType: 'web',
+    });
+
+    expect(isAncestorOfSpy).not.toHaveBeenCalled();
+  });
+
+  // --- Constructor validation tests ---
+
+  test('throws on zero staleThresholdDays', () => {
+    expect(
+      () =>
+        new IsolationResolver({
+          store: makeMockStore(),
+          provider: makeMockProvider(),
+          staleThresholdDays: 0,
+        })
+    ).toThrow('staleThresholdDays must be positive, got 0');
+  });
+
+  test('throws on negative staleThresholdDays', () => {
+    expect(
+      () =>
+        new IsolationResolver({
+          store: makeMockStore(),
+          provider: makeMockProvider(),
+          staleThresholdDays: -1,
+        })
+    ).toThrow('staleThresholdDays must be positive, got -1');
+  });
+});
diff --git a/packages/isolation/src/resolver.ts b/packages/isolation/src/resolver.ts
@@ -13,6 +13,7 @@ import {
   getCanonicalRepoPath,
   findWorktreeByBranch,
   toBranchName,
+  isAncestorOf,
 } from '@archon/git';
 import type { RepoPath } from '@archon/git';
 
@@ -84,9 +85,11 @@ export class IsolationResolver {
    * Resolve isolation for a conversation request.
    */
   async resolve(request: ResolveRequest): Promise<IsolationResolution> {
+    const baseBranch = request.hints?.baseBranch;
+
     // 1. Check existing isolation reference
     if (request.existingEnvId) {
-      const existing = await this.checkExisting(request.existingEnvId);
+      const existing = await this.checkExisting(request.existingEnvId, baseBranch);
       if (existing) return existing;
       // Stale — tell caller to clear and retry
       return { status: 'stale_cleaned', previousEnvId: request.existingEnvId };
@@ -103,13 +106,14 @@ export class IsolationResolver {
     const workflowId = hints?.workflowId ?? '';
 
     // 3. Check for existing environment with same workflow
-    const reusable = await this.findReusable(codebase.id, workflowType, workflowId);
+    const reusable = await this.findReusable(codebase.id, workflowType, workflowId, baseBranch);
     if (reusable) {
       return {
         status: 'resolved',
-        env: reusable,
-        cwd: reusable.working_path,
+        env: reusable.env,
+        cwd: reusable.env.working_path,
         method: { type: 'workflow_reuse' },
+        ...(reusable.warnings.length > 0 ? { warnings: reusable.warnings } : {}),
       };
     }
 
@@ -146,14 +150,35 @@ export class IsolationResolver {
   /**
    * Check if an existing environment reference is still valid.
    */
-  private async checkExisting(envId: string): Promise<IsolationResolution | null> {
+  private async checkExisting(
+    envId: string,
+    baseBranch?: string
+  ): Promise<IsolationResolution | null> {
     const env = await this.store.getById(envId);
     if (env && (await worktreeExists(toWorktreePath(env.working_path)))) {
+      const warnings: string[] = [];
+      if (baseBranch) {
+        const isValid = await isAncestorOf(
+          toWorktreePath(env.working_path),
+          `origin/${baseBranch}`
+        );
+        if (!isValid) {
+          warnings.push(
+            `Worktree branch '${env.branch_name}' is not based on '${baseBranch}'. ` +
+              `Recreate with: archon complete ${env.branch_name} --force`
+          );
+          getLog().warn(
+            { envId, branchName: env.branch_name, baseBranch },
+            'isolation.reuse_base_branch_mismatch'
+          );
+        }
+      }
       return {
         status: 'resolved',
         env,
         cwd: env.working_path,
         method: { type: 'existing' },
+        ...(warnings.length > 0 ? { warnings } : {}),
       };
     }
 
@@ -170,14 +195,32 @@ export class IsolationResolver {
   private async findReusable(
     codebaseId: string,
     workflowType: IsolationWorkflowType,
-    workflowId: string
-  ): Promise<IsolationEnvironmentRow | null> {
+    workflowId: string,
+    baseBranch?: string
+  ): Promise<{ env: IsolationEnvironmentRow; warnings: string[] } | null> {
     const existing = await this.store.findActiveByWorkflow(codebaseId, workflowType, workflowId);
     if (!existing) return null;
 
     if (await worktreeExists(toWorktreePath(existing.working_path))) {
       getLog().debug({ workflowType, workflowId }, 'isolation_reuse_existing');
-      return existing;
+      const warnings: string[] = [];
+      if (baseBranch) {
+        const isValid = await isAncestorOf(
+          toWorktreePath(existing.working_path),
+          `origin/${baseBranch}`
+        );
+        if (!isValid) {
+          warnings.push(
+            `Worktree branch '${existing.branch_name}' is not based on '${baseBranch}'. ` +
+              `Recreate with: archon complete ${existing.branch_name} --force`
+          );
+          getLog().warn(
+            { workflowType, workflowId, branchName: existing.branch_name, baseBranch },
+            'isolation.reuse_base_branch_mismatch'
+          );
+        }
+      }
+      return { env: existing, warnings };
     }
 
     await this.markDestroyedBestEffort(existing.id);
diff --git a/packages/isolation/src/types.ts b/packages/isolation/src/types.ts
@@ -208,6 +208,9 @@ export interface IsolationHints {
   /** Start-point branch for new task worktree creation. Only consumed when workflowType === 'task'. */
   fromBranch?: BranchName;
 
+  /** Expected base branch for this workflow. When set, reused worktrees are validated with merge-base. */
+  baseBranch?: string;
+
   // Cross-reference hints
   linkedIssues?: number[];
   linkedPRs?: number[];
