electron-rare/Aperant
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: validate branch pattern before worktree cleanup to prevent deleting wrong branch (#1493)

Browse Source 
* fix: validate branch pattern before worktree cleanup to prevent deleting wrong branch

When a worktree is corrupted or orphaned, `git rev-parse --abbrev-ref HEAD`
can walk up the directory tree and return the main project's current branch
instead of the worktree's branch. This caused worktree cleanup to delete the
wrong branch (e.g., a developer's active feature branch).

Fix: Validate detected branch matches expected pattern (`auto-claude/{specId}`)
before using it for deletion. Falls back to expected branch pattern if
detected branch doesn't match or git command fails.

Also adds `getIsolatedGitEnv()` and `timeout: 30000` to git commands for
consistency and safety.

Fixes ACS-402

* refactor: extract duplicated branch detection logic into shared utility

Extract the duplicated branch-detection logic from execution-handlers.ts
and worktree-handlers.ts into a shared detectWorktreeBranch() utility
function in git-isolation.ts.

This improves maintainability by:
- Eliminating code duplication across two handlers
- Centralizing the branch validation pattern matching logic
- Making future changes easier to maintain in one location

The new function:
- Takes worktreePath, specId, and optional timeout/logPrefix options
- Returns { branch, usingFallback } for proper error handling
- Includes comprehensive JSDoc documentation

Addresses CodeRabbitAI review feedback.

* fix: address Auto Claude PR Review findings (4 issues)

[HIGH] Use strict branch matching only in detectWorktreeBranch()
- Changed from 'detectedBranch.startsWith("auto-claude/")' to strict 'detectedBranch === expectedBranch' matching
- This prevents deleting a different task's auto-claude branch when worktree is corrupted

[MEDIUM] Use ES6 imports instead of inline require()
- Added top-level imports for execFileSync and getToolPath
- Removed inline require() statements for better type safety and consistency

[MEDIUM] Extract usingFallback in worktree-handlers.ts
- Now destructures both branch and usingFallback from detectWorktreeBranch()
- Uses usingFallback in error handling for contextual logging

[MEDIUM] Add error logging to branch deletion catch block
- Logs branch deletion errors with context
- Provides different messages based on whether fallback was used

* fix: include error object in non-fallback branch deletion warning

Per CodeRabbitAI review, line 632 was not logging the error object
when branch deletion failed in the non-fallback case, making failures
harder to diagnose. Now logs branchDeleteError consistently.

* docs: add SECURITY comment for branch validation and fix file corruption

Added detailed SECURITY comment explaining why exact-match validation
is critical for preventing accidental deletion of wrong branches.

Also fixed missing closing brace for detectWorktreeBranch function
that was causing syntax errors.

---------

Co-authored-by: StillKnotKnown <stillknotknown@users.noreply.github.com>
This commit is contained in:
StillKnotKnown
2026-01-26 13:17:25 +02:00
committed by GitHub
parent d940b6adee
commit 8576754a12
3 changed files with 105 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
+12 -17
View File
@@ -17,7 +17,7 @@ import {
} from './plan-file-utils';
import { findTaskWorktree } from '../../worktree-paths';
import { projectStore } from '../../project-store';
import { getIsolatedGitEnv } from '../../utils/git-isolation';
import { getIsolatedGitEnv, detectWorktreeBranch } from '../../utils/git-isolation';
/**
* Atomic file write to prevent TOCTOU race conditions.
@@ -597,21 +597,13 @@ export function registerTaskExecutionHandlers(
console.warn(`[TASK_UPDATE_STATUS] Cleaning up worktree for task ${taskId} (user confirmed)`);
try {
// Get the branch name before removing the worktree
let branch = '';
let usingFallbackBranch = false;
try {
branch = execFileSync(getToolPath('git'), ['rev-parse', '--abbrev-ref', 'HEAD'], {
cwd: worktreePath,
encoding: 'utf-8',
timeout: 30000,
env: getIsolatedGitEnv()
}).trim();
} catch (branchError) {
// If we can't get branch name, use the default pattern
branch = `auto-claude/${task.specId}`;
usingFallbackBranch = true;
console.warn(`[TASK_UPDATE_STATUS] Could not get branch name, using fallback pattern: ${branch}`, branchError);
}
// Use shared utility to validate detected branch matches expected pattern
// This prevents deleting wrong branch when worktree is corrupted/orphaned
const { branch, usingFallback: usingFallbackBranch } = detectWorktreeBranch(
worktreePath,
task.specId,
{ timeout: 30000, logPrefix: '[TASK_UPDATE_STATUS]' }
);
// Remove the worktree
execFileSync(getToolPath('git'), ['worktree', 'remove', '--force', worktreePath], {
@@ -637,7 +629,10 @@ export function registerTaskExecutionHandlers(
// More concerning - fallback pattern didn't match actual branch
console.warn(`[TASK_UPDATE_STATUS] Could not delete branch ${branch} using fallback pattern. Actual branch may still exist and need manual cleanup.`, branchDeleteError);
} else {
console.warn(`[TASK_UPDATE_STATUS] Could not delete branch ${branch} (may not exist or be checked out elsewhere)`);
console.warn(
`[TASK_UPDATE_STATUS] Could not delete branch ${branch} (may not exist or be checked out elsewhere)`,
branchDeleteError
);
}
}
apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts
+20 -8
View File
@@ -19,7 +19,7 @@ import {
findTaskWorktree,
} from '../../worktree-paths';
import { persistPlanStatus, updateTaskMetadataPrUrl } from './plan-file-utils';
import { getIsolatedGitEnv, refreshGitIndex } from '../../utils/git-isolation';
import { getIsolatedGitEnv, detectWorktreeBranch, refreshGitIndex } from '../../utils/git-isolation';
import { killProcessGracefully } from '../../platform';
// Regex pattern for validating git branch names
@@ -2588,25 +2588,37 @@ export function registerWorktreeHandlers(
try {
// Get the branch name before removing
const branch = execFileSync(getToolPath('git'), ['rev-parse', '--abbrev-ref', 'HEAD'], {
cwd: worktreePath,
encoding: 'utf-8'
}).trim();
// Use shared utility to validate detected branch matches expected pattern
// This prevents deleting wrong branch when worktree is corrupted/orphaned
const { branch, usingFallback } = detectWorktreeBranch(
worktreePath,
task.specId,
{ timeout: 30000, logPrefix: '[TASK_WORKTREE_DISCARD]' }
);
// Remove the worktree
execFileSync(getToolPath('git'), ['worktree', 'remove', '--force', worktreePath], {
cwd: project.path,
encoding: 'utf-8'
encoding: 'utf-8',
env: getIsolatedGitEnv(),
timeout: 30000
});
// Delete the branch
try {
execFileSync(getToolPath('git'), ['branch', '-D', branch], {
cwd: project.path,
encoding: 'utf-8'
encoding: 'utf-8',
env: getIsolatedGitEnv(),
timeout: 30000
});
} catch {
} catch (branchDeleteError) {
// Branch might already be deleted or not exist
if (usingFallback) {
console.warn(`[TASK_WORKTREE_DISCARD] Could not delete branch ${branch} using fallback pattern. Actual branch may still exist and need manual cleanup.`, branchDeleteError);
} else {
console.warn(`[TASK_WORKTREE_DISCARD] Could not delete branch ${branch} (may not exist or be checked out elsewhere)`, branchDeleteError);
}
}
// Only send status change to backlog if not skipped
apps/frontend/src/main/utils/git-isolation.ts
+73
View File
@@ -109,6 +109,79 @@ export function getIsolatedGitSpawnOptions(
};
}
/**
* Result type for detectWorktreeBranch function.
*/
export interface WorktreeBranchDetectionResult {
/** The branch name to use for deletion */
branch: string;
/** Whether the fallback branch pattern was used */
usingFallback: boolean;
}
/**
* Detects the branch name in a worktree with safety validation.
*
* This function prevents a critical bug where git rev-parse in a corrupted/orphaned
* worktree can return the main project's current branch instead of the worktree's branch.
* It validates the detected branch matches the expected pattern before using it.
*
* @param worktreePath - Path to the worktree directory
* @param specId - The spec ID used to generate the expected branch name
* @param options - Optional configuration
* @param options.timeout - Timeout in milliseconds for git commands (default: 30000)
* @param options.logPrefix - Prefix for log messages (e.g., "[TASK_UPDATE_STATUS]")
* @returns Object containing the branch name and whether fallback was used
*
* @example
* ```typescript
* import { detectWorktreeBranch } from './utils/git-isolation';
* import { getToolPath } from './cli-tool-manager';
*
* const { branch, usingFallback } = detectWorktreeBranch(
* worktreePath,
* task.specId,
* { timeout: 30000, logPrefix: '[TASK_WORKTREE_DISCARD]' }
* );
* ```
*/
export function detectWorktreeBranch(
worktreePath: string,
specId: string,
options: { timeout?: number; logPrefix?: string } = {}
): WorktreeBranchDetectionResult {
const { timeout = 30000, logPrefix = '[WORKTREE_BRANCH_DETECTION]' } = options;
const expectedBranch = `auto-claude/${specId}`;
let branch = expectedBranch;
let usingFallback = false;
try {
const detectedBranch = execFileSync(getToolPath('git'), ['rev-parse', '--abbrev-ref', 'HEAD'], {
cwd: worktreePath,
encoding: 'utf-8',
timeout,
env: getIsolatedGitEnv()
}).trim();
// SECURITY: Use strict exact-match validation (not prefix matching) to prevent
// accidentally deleting a different task's auto-claude branch. When git rev-parse
// returns an unexpected branch, we MUST fall back to the expected pattern rather
// than risking deletion of the wrong branch. This is critical for data safety.
if (detectedBranch === expectedBranch) {
branch = detectedBranch;
} else {
console.warn(`${logPrefix} Detected branch '${detectedBranch}' doesn't match expected branch '${expectedBranch}', using fallback: ${expectedBranch}`);
usingFallback = true;
}
} catch (branchError) {
// If we can't get branch name, use the default pattern
usingFallback = true;
console.warn(`${logPrefix} Could not get branch name, using fallback pattern: ${branch}`, branchError);
}
return { branch, usingFallback };
}
/**
* Refreshes the git index to ensure accurate status after external commits.
*