fix: human_review status persistence bug (worktree plan path fix) (#605)
* fix(kanban): await plan updates before resolving merge (fixes #243) Root cause: updatePlans() was fire-and-forget, causing race condition where resolve() returned before files were written. UI refresh would then read old 'human_review' status instead of 'done'. Fix: Await updatePlans() with try/catch to ensure status persists before UI refresh. Non-fatal error handling preserves existing behavior. Fixes #243 Related: #586, #216 * fix(kanban): clean up worktree after successful full merge (fixes #243) Adds worktree removal after successful full merge (not stage-only). This allows the drag-to-Done workflow since TASK_UPDATE_STATUS blocks setting 'done' status when a worktree exists. Also deletes the task branch (auto-claude/{specId}) after merge. Both operations are non-fatal if they fail. Combined with the previous commit (await updatePlans), this ensures: 1. Status is persisted before UI refresh 2. Worktree is cleaned up so drag-to-Done works 3. Task branches are cleaned up Fixes #243 Related: #586, #216 * fix(kanban): add worktree cleanup to stage-only 'already merged' path (fixes #243) When stageOnly=true (default for human_review tasks) and user clicks 'Stage Changes' but the merge was already committed previously: - Now cleans up the worktree - Deletes the task branch - Sets status to 'done' This complements the earlier fix that only cleaned up on full merge. The combined fix handles both workflows: - 'Merge to Main' button (stageOnly=false): cleanup after merge - 'Stage Changes' button (stageOnly=true): cleanup when detecting already merged Fixes #243 Related: #586, #216 * fix(kanban): default stageOnly to false for proper worktree cleanup (fixes #243) The stageOnly checkbox was defaulting to true for human_review tasks, causing users to click 'Stage Changes' instead of 'Merge to Main'. Stage-only mode: - Stages changes but doesn't commit - User must manually commit - Worktree cleanup only happens on second click (after commit) Full merge mode (now default): - Merges and commits in one step - Worktree is cleaned up immediately - Task moves to Done automatically This is the key fix for #243 - the previous commits added cleanup logic but it wasn't triggered because of this UI default. Fixes #243 Related: #586, #216 * fix(status): prevent human_review from reverting to in_progress The status validation logic was missing a rule to treat 'human_review' as valid when the calculated status is 'in_progress'. This caused tasks in staging to flip back to 'in_progress' on refresh if any subtask was stuck in 'in_progress' state (race condition). Added validation rule: human_review is valid when calculatedStatus is either 'ai_review' OR 'in_progress', since human_review is a more advanced state than both. Fixes the 'done → in_progress' loop after staging. * fix(worktree): correct plan path for worktree status persistence The worktree plan file path was incorrect - it was pointing to: `/.worktrees/taskId/implementation_plan.json` But the actual path should be: `/.worktrees/taskId/.auto-claude/specs/taskId/implementation_plan.json` This caused the worktree plan update to silently fail (ENOENT was swallowed), so the worktree's plan file retained its old status. Since ProjectStore prefers the worktree version when deduplicating, the task would show the stale status from the worktree. This is the root cause of the human_review → in_progress status loop. The first fix (project-store.ts) handles validation, but this fix ensures the worktree plan is actually updated with the new status. --------- Co-authored-by: Alex <63423455+AlexMadera@users.noreply.github.com>
This commit is contained in:
Michael Ludlow
@@ -1657,6 +1657,33 @@ export function registerWorktreeHandlers(
|
||||
message = 'Changes were already merged and committed. Task marked as done.';
|
||||
staged = false;
|
||||
debug('Stage-only requested but merge already committed. Marking as done.');
|
||||
|
||||
// Clean up worktree since merge is complete (fixes #243)
|
||||
// This is the same cleanup as the full merge path, needed because
|
||||
// stageOnly defaults to true for human_review tasks
|
||||
try {
|
||||
if (existsSync(worktreePath)) {
|
||||
execFileSync(getToolPath('git'), ['worktree', 'remove', '--force', worktreePath], {
|
||||
cwd: project.path,
|
||||
encoding: 'utf-8'
|
||||
});
|
||||
debug('Worktree cleaned up (already merged):', worktreePath);
|
||||
|
||||
// Also delete the task branch
|
||||
const taskBranch = `auto-claude/${task.specId}`;
|
||||
try {
|
||||
execFileSync(getToolPath('git'), ['branch', '-D', taskBranch], {
|
||||
cwd: project.path,
|
||||
encoding: 'utf-8'
|
||||
});
|
||||
debug('Task branch deleted:', taskBranch);
|
||||
} catch {
|
||||
// Branch might not exist or already deleted
|
||||
}
|
||||
}
|
||||
} catch (cleanupErr) {
|
||||
debug('Worktree cleanup failed (non-fatal):', cleanupErr);
|
||||
}
|
||||
} else if (isStageOnly && !hasActualStagedChanges) {
|
||||
// Stage-only was requested but no changes to stage (and not committed)
|
||||
// This could mean nothing to merge or an error - keep in human_review for investigation
|
||||
@@ -1677,6 +1704,33 @@ export function registerWorktreeHandlers(
|
||||
planStatus = 'completed';
|
||||
message = 'Changes merged successfully';
|
||||
staged = false;
|
||||
|
||||
// Clean up worktree after successful full merge (fixes #243)
|
||||
// This allows drag-to-Done workflow since TASK_UPDATE_STATUS blocks 'done' when worktree exists
|
||||
try {
|
||||
if (existsSync(worktreePath)) {
|
||||
execFileSync(getToolPath('git'), ['worktree', 'remove', '--force', worktreePath], {
|
||||
cwd: project.path,
|
||||
encoding: 'utf-8'
|
||||
});
|
||||
debug('Worktree cleaned up after full merge:', worktreePath);
|
||||
|
||||
// Also delete the task branch since we merged successfully
|
||||
const taskBranch = `auto-claude/${task.specId}`;
|
||||
try {
|
||||
execFileSync(getToolPath('git'), ['branch', '-D', taskBranch], {
|
||||
cwd: project.path,
|
||||
encoding: 'utf-8'
|
||||
});
|
||||
debug('Task branch deleted:', taskBranch);
|
||||
} catch {
|
||||
// Branch might not exist or already deleted
|
||||
}
|
||||
}
|
||||
} catch (cleanupErr) {
|
||||
debug('Worktree cleanup failed (non-fatal):', cleanupErr);
|
||||
// Non-fatal - merge succeeded, cleanup can be done manually
|
||||
}
|
||||
}
|
||||
|
||||
debug('Merge result. isStageOnly:', isStageOnly, 'newStatus:', newStatus, 'staged:', staged);
|
||||
@@ -1701,9 +1755,11 @@ export function registerWorktreeHandlers(
|
||||
// Issue #243: We must update BOTH the main project's plan AND the worktree's plan (if it exists)
|
||||
// because ProjectStore prefers the worktree version when deduplicating tasks.
|
||||
// OPTIMIZATION: Use async I/O and parallel updates to prevent UI blocking
|
||||
// NOTE: The worktree has the same directory structure as main project
|
||||
const worktreeSpecDir = path.join(worktreePath, project.autoBuildPath || '.auto-claude', 'specs', task.specId);
|
||||
const planPaths = [
|
||||
{ path: path.join(specDir, AUTO_BUILD_PATHS.IMPLEMENTATION_PLAN), isMain: true },
|
||||
{ path: path.join(worktreePath, AUTO_BUILD_PATHS.IMPLEMENTATION_PLAN), isMain: false }
|
||||
{ path: path.join(worktreeSpecDir, AUTO_BUILD_PATHS.IMPLEMENTATION_PLAN), isMain: false }
|
||||
];
|
||||
|
||||
const { promises: fsPromises } = require('fs');
|
||||
@@ -1766,8 +1822,15 @@ export function registerWorktreeHandlers(
|
||||
}
|
||||
};
|
||||
|
||||
// Run async updates without blocking the response
|
||||
updatePlans().catch(err => debug('Background plan update failed:', err));
|
||||
// IMPORTANT: Wait for plan updates to complete before responding (fixes #243)
|
||||
// Previously this was "fire and forget" which caused a race condition:
|
||||
// resolve() would return before files were written, and UI refresh would read old status
|
||||
try {
|
||||
await updatePlans();
|
||||
} catch (err) {
|
||||
debug('Plan update failed:', err);
|
||||
// Non-fatal: UI will still update, but status may not persist across refresh
|
||||
}
|
||||
|
||||
const mainWindow = getMainWindow();
|
||||
if (mainWindow) {
|
||||
|
||||
@@ -565,7 +565,7 @@ export class ProjectStore {
|
||||
|
||||
const isStoredStatusValid =
|
||||
(storedStatus === calculatedStatus) || // Matches calculated
|
||||
(storedStatus === 'human_review' && calculatedStatus === 'ai_review') || // Human review is more advanced than ai_review
|
||||
(storedStatus === 'human_review' && (calculatedStatus === 'ai_review' || calculatedStatus === 'in_progress')) || // Human review is more advanced than ai_review or in_progress (fixes status loop bug)
|
||||
(storedStatus === 'human_review' && isPlanReviewStage) || // Plan review stage (awaiting spec approval)
|
||||
(isActiveProcessStatus && storedStatus === 'in_progress'); // Planning/coding phases should show as in_progress
|
||||
|
||||
|
||||
@@ -27,7 +27,7 @@ export function useTaskDetail({ task }: UseTaskDetailOptions) {
|
||||
const [showDiscardDialog, setShowDiscardDialog] = useState(false);
|
||||
const [workspaceError, setWorkspaceError] = useState<string | null>(null);
|
||||
const [showDiffDialog, setShowDiffDialog] = useState(false);
|
||||
const [stageOnly, setStageOnly] = useState(task.status === 'human_review');
|
||||
const [stageOnly, setStageOnly] = useState(false); // Default to full merge for proper cleanup (fixes #243)
|
||||
const [stagedSuccess, setStagedSuccess] = useState<string | null>(null);
|
||||
const [stagedProjectPath, setStagedProjectPath] = useState<string | undefined>(undefined);
|
||||
const [suggestedCommitMessage, setSuggestedCommitMessage] = useState<string | undefined>(undefined);
|
||||
|
||||
Reference in New Issue
Block a user