fix: resolve Kanban board stuck task state synchronization#1833
fix: resolve Kanban board stuck task state synchronization#1833
Conversation
When an agent process exits with incomplete/stuck subtasks, the task gets
stuck in the Kanban UI — it spins forever, can't be stopped, and can't be
dragged back to planning.
This fix addresses 5 specific state synchronization gaps between the backend
process lifecycle and the frontend XState state machine:
1. Reset execution progress on terminal state transitions (task-store.ts)
- When tasks reach terminal states (human_review, error, done, pr_created),
execution progress is now reset to idle
- Prevents stuck tasks from showing stale progress indicators in UI
2. Propagate final phase updates even after XState settles (agent-events-handlers.ts)
- Final 'complete' or 'failed' phase updates are now sent to renderer
- Previously these were silently dropped, causing UI to never show completion
3. Add fallback exit handler to force state transition (agent-events-handlers.ts)
- If task remains in_progress 500ms after process exit, force to human_review
- Safety net for when XState fails to properly handle PROCESS_EXITED event
4. Disable dragging for in_progress tasks (SortableTaskCard.tsx)
- Prevents users from dragging tasks that are currently running or stuck
- Adds disabled flag to useSortable hook when status is 'in_progress'
5. Allow stop button for stuck tasks (TaskCard.tsx)
- Users can now force-stop stuck tasks using the stop button
- Removed the isStuck check that prevented stopping stuck tasks
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
area/frontend
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a 500ms post-exit fallback that forces USER_STOPPED for tasks in active XState states, exports XSTATE_ACTIVE_STATES, allows stopping stuck/running tasks, disables dragging for in-progress tasks, forwards final-phase execution-progress after XState settles, and clears executionProgress for terminal statuses. Changes
Sequence Diagram(s)sequenceDiagram
participant MainProcess
participant Renderer
participant TaskStore
MainProcess->>Renderer: execution-progress update (phase, progress)
alt XState not settled
Renderer->>TaskStore: forward progress -> update UI/state
else XState settled
alt final phase (complete/failed) and task.status == in_progress
Renderer->>TaskStore: forward final-phase progress -> update UI/state
else
Renderer-->>TaskStore: skip non-final progress
end
end
MainProcess->>MainProcess: process exit detected
MainProcess->>MainProcess: wait 500ms (STUCK_TASK_FALLBACK_TIMEOUT_MS)
MainProcess->>Renderer: lookup task & project, check XSTATE_ACTIVE_STATES
alt task still in active XState
MainProcess->>Renderer: emit UI event -> USER_STOPPED (hasPlan)
Renderer->>TaskStore: apply USER_STOPPED transition
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @AndyMik90, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves critical issues causing tasks on the Kanban board to become stuck due to desynchronization between the backend process lifecycle and the frontend's XState machine. It introduces several robust mechanisms, including a fallback to ensure tasks transition correctly, improved propagation of final task states to the UI, and enhanced user control over stuck tasks, significantly improving the reliability and user experience of the task management system. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several important fixes to resolve state synchronization issues on the Kanban board, preventing tasks from getting stuck. The changes, such as adding a fallback timer, resetting progress on terminal states, and allowing the UI to receive final phase updates, are well-reasoned and directly address the described problem.
I've identified one critical issue: a call to a forceTransition method that does not appear to be defined, which will cause a runtime error. I've also left a comment with a suggestion to improve code maintainability by refactoring a condition. Overall, these are solid fixes, and once the critical issue is addressed, this should significantly improve the stability of the Kanban board.
| setTimeout(() => { | ||
| const { task: checkTask } = findTaskAndProject(taskId, projectId); | ||
| if (checkTask && checkTask.status === 'in_progress') { | ||
| console.warn(`[agent-events-handlers] Task ${taskId} still in_progress 500ms after exit, forcing to human_review`); | ||
| taskStateManager.forceTransition(taskId, 'human_review', 'errors', checkTask, exitProject); | ||
| } | ||
| }, 500); |
There was a problem hiding this comment.
This fallback is a good safety net, but there are a couple of issues here:
-
Critical: The method
taskStateManager.forceTransitiondoes not appear to be defined inTaskStateManagerbased on the provided file context. This will likely cause a runtime error. You may need to implement this method inapps/frontend/src/main/task-state-manager.tsto handle persisting and emitting the new task state. -
Medium: The timeout duration
500is a magic number. It's better to define it as a named constant for readability and maintainability, for example:const STUCK_TASK_FALLBACK_TIMEOUT_MS = 500;.
| // When starting a task and no phase is set yet, default to planning | ||
| // This prevents the "no active phase" UI state during startup race condition | ||
| executionProgress = { phase: 'planning' as ExecutionPhase, phaseProgress: 0, overallProgress: 0 }; | ||
| } else if (status === 'human_review' || status === 'error' || status === 'done' || status === 'pr_created') { |
There was a problem hiding this comment.
For better readability and maintainability, you could use an array with .includes() for this check. It makes it easier to manage the list of terminal statuses in the future.
| } else if (status === 'human_review' || status === 'error' || status === 'done' || status === 'pr_created') { | |
| } else if (['human_review', 'error', 'done', 'pr_created'].includes(status)) { |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const { task: checkTask } = findTaskAndProject(taskId, projectId); | ||
| if (checkTask && checkTask.status === 'in_progress') { | ||
| console.warn(`[agent-events-handlers] Task ${taskId} still in_progress 500ms after exit, forcing to human_review`); | ||
| taskStateManager.forceTransition(taskId, 'human_review', 'errors', checkTask, exitProject); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The 500ms fallback safety net was calling `taskStateManager.forceTransition()` which doesn't exist, causing TypeScript compilation errors. Fixed to: - Use `handleUiEvent()` with `USER_STOPPED` event (proper XState transition) - Look up both task and project fresh (avoid stale closure references) - Add null check for `checkProject` before proceeding This ensures the fallback actually works when XState fails to transition tasks out of in_progress after process exit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts`:
- Around line 99-108: The timeout calls a non-existent method
taskStateManager.forceTransition which will throw at runtime; replace this with
an existing transition handler on TaskStateManager (e.g., handleUiEvent or
handleManualStatusChange) or implement forceTransition on TaskStateManager.
Locate the setTimeout block using findTaskAndProject, taskId and projectId, and
either invoke
taskStateManager.handleUiEvent/taskStateManager.handleManualStatusChange with
arguments to move checkTask to 'human_review' (propagate the same reason
'errors' and exitProject context) or add a forceTransition(taskId,
'human_review', reason, checkTask, exitProject) method to TaskStateManager that
encapsulates the forced transition logic.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🔴 BLOCKED
🔴 Blocked - 3 critical/high/medium issue(s) must be fixed.
Blocked by 1 critical issues
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Critical: forceTransition() does not exist on TaskStateManager — runtime crash (apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts:106)
Findings Summary
- Critical: 1 issue(s)
- Medium: 2 issue(s)
Generated by Auto Claude PR Review
Findings (3 selected of 3 total)
🔴 [29dfc2e8be54] [CRITICAL] forceTransition() does not exist on TaskStateManager — runtime crash
📁 apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts:106
The setTimeout fallback at lines 102-108 calls taskStateManager.forceTransition(...), but this method does not exist on the TaskStateManager class (apps/frontend/src/main/task-state-manager.ts). I read the complete TaskStateManager class (lines 1-431) and confirmed the available public methods are: configure, handleTaskEvent, handleProcessExited, handleUiEvent, handleManualStatusChange, setLastSequence, getLastSequence, getCurrentState, isInPlanReview, clearTask, clearAllTasks. There is no forceTransition method. This means when a task genuinely gets stuck and the setTimeout fires after 500ms, it will throw TypeError: taskStateManager.forceTransition is not a function. Since this is inside a setTimeout, it will be an uncaught async error. This is the core safety net of the PR and it will never work.
Searched via Grep('forceTransition\s*\(') — only one call site found (this line), no definition anywhere in the codebase. | The 500ms fallback safety net calls taskStateManager.forceTransition(...) but this method does not exist on the TaskStateManager class. I verified by reading the complete TaskStateManager class in task-state-manager.ts (430 lines, lines 32-428) and searching the entire codebase via Grep('forceTransition') — only this single call site exists; the method is never defined. When the condition on line 104 triggers (task still in_progress 500ms after process exit), this will throw a TypeError: taskStateManager.forceTransition is not a function. This completely defeats the purpose of the safety net — the feature described in the PR summary ('Add 500ms fallback in exit handler to force transition if task remains in_progress') is non-functional. The closest existing method that could serve this purpose is handleManualStatusChange() or handleUiEvent(), which properly send XState events and persist status. | The new fallback safety net calls taskStateManager.forceTransition(taskId, 'human_review', 'errors', checkTask, exitProject) but the TaskStateManager class in src/main/task-state-manager.ts has no forceTransition method. The class exposes: configure, handleTaskEvent, handleProcessExited, handleUiEvent, handleManualStatusChange, setLastSequence, getLastSequence, getCurrentState, isInPlanReview, clearTask, clearAllTasks. This will throw a TypeError: taskStateManager.forceTransition is not a function at runtime when a task gets stuck and the 500ms timeout fires — exactly the scenario this fix is supposed to handle. Searched all files: Grep('forceTransition', 'apps/frontend/src') found only the call site in agent-events-handlers.ts, confirming the method is never defined. The existing handleManualStatusChange() method appears to be the closest equivalent for forcing a status transition through XState.
Suggested fix:
Add a `forceTransition` method to `TaskStateManager`, or use the existing `handleManualStatusChange` or `handleUiEvent` methods. For example, add to TaskStateManager:
```typescript
forceTransition(taskId: string, status: TaskStatus, reviewReason: ReviewReason, task: Task, project: Project): void {
console.warn(`[TaskStateManager] Force transitioning ${taskId} to ${status}`);
this.setTaskContext(taskId, task, project);
const actor = this.getOrCreateActor(taskId);
// Send PROCESS_EXITED with unexpected=true to trigger error/human_review transition
actor.send({ type: 'PROCESS_EXITED', exitCode: -1, unexpected: true } satisfies TaskEvent);
}
Alternatively, reuse the existing handleManualStatusChange method or directly call emitStatus + persistStatus if the goal is simply to update the UI without going through XState.
#### 🟡 [ccb0a8f00550] [MEDIUM] Stale closure: exitProject captured by setTimeout may be outdated
📁 `apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts:102`
The setTimeout callback at line 102 captures `exitProject` from the outer scope (line 94), but re-fetches `checkTask` via `findTaskAndProject(taskId, projectId)` at line 103. However, the re-fetched project (which would be the second return value) is discarded — only `checkTask` is destructured. If the project data changes during the 500ms window, `exitProject` from the closure would be stale. More importantly, if the forceTransition bug is fixed, the stale `exitProject` reference could cause incorrect persistence (writing to wrong plan file path). The code should use the freshly-fetched project from `findTaskAndProject` instead of the closure variable.
**Suggested fix:**
Destructure the project from the fresh lookup:
setTimeout(() => {
const { task: checkTask, project: checkProject } = findTaskAndProject(taskId, projectId);
if (checkTask && checkTask.status === 'in_progress' && checkProject) {
console.warn(`[agent-events-handlers] Task ${taskId} still in_progress 500ms after exit, forcing to human_review`);
taskStateManager.forceTransition(taskId, 'human_review', 'errors', checkTask, checkProject);
}
}, 500);
#### 🟡 [ff22e4341fda] [MEDIUM] Stale cache may cause fallback to fire when XState already transitioned
📁 `apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts:103`
The setTimeout callback at line 103 calls `findTaskAndProject(taskId, projectId)` which reads from `projectStore.getTasks()`. This store has a 3-second TTL cache (verified in `project-store.ts` line 39: `private readonly CACHE_TTL_MS = 3000`). The XState transition happens synchronously and persists the new status to disk via `persistPlanStatusAndReasonSync`, but if the cache was populated less than 3s ago, `findTaskAndProject` returns the cached (stale) data still showing `status: 'in_progress'`. With the 500ms setTimeout, the cache is very likely still valid (populated during the same event handling cycle). This means `checkTask.status === 'in_progress'` evaluates to `true` even after XState has already successfully transitioned the task — causing the fallback to fire unnecessarily. Currently moot because `forceTransition` doesn't exist (see CRITICAL finding), but once that's fixed, this would trigger redundant forced transitions for tasks that were already properly transitioned by XState. Additionally, `exitProject` is captured at line 94 (before the setTimeout) and may be `undefined` if the project wasn't found — this stale `undefined` reference is passed to the force-transition call.
**Suggested fix:**
Instead of relying on findTaskAndProject (which reads from a cached store), check the XState actor's current state directly using taskStateManager.getCurrentState(taskId). If XState is still in a running state (planning, coding, qa_review, qa_fixing), then force the transition. This avoids the cache staleness issue entirely. Example: const currentState = taskStateManager.getCurrentState(taskId); if (currentState && ['planning', 'coding', 'qa_review', 'qa_fixing'].includes(currentState)) { /* force transition */ }
---
*This review was generated by Auto Claude.*
- Add named constant STUCK_TASK_FALLBACK_TIMEOUT_MS for magic number - Use XState getCurrentState() instead of cached task status to avoid stale cache issues - Check XState active states directly (planning, coding, qa_review, qa_fixing) - Improve logging to show actual XState state name - Add inline comments explaining the stale cache avoidance strategy This resolves all 3 blocking issues from the Auto Claude review: 1. CRITICAL: forceTransition() method - fixed by using handleUiEvent() 2. MEDIUM: Stale closure - fixed by checking XState directly 3. MEDIUM: Magic number - fixed by using named constant Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts`:
- Line 109: Extract the hardcoded array in agent-events-handlers.ts into a
shared constant: add XSTATE_ACTIVE_STATES (ReadonlySet<string>) to
task-state-utils.ts containing 'planning','coding','qa_review','qa_fixing',
export it from the shared state-machines index (alongside
XSTATE_SETTLED_STATES), then replace the local const activeStates in
agent-events-handlers.ts with an import of XSTATE_ACTIVE_STATES so all modules
(PhaseProgressIndicator.tsx, agent-process.ts, CompletablePhase usages)
reference the single source of truth.
- Line 118: Replace the hardcoded hasPlan: true in the call to
taskStateManager.handleUiEvent with a real check for whether the plan file
exists for the task; compute a boolean (e.g., hasPlan) by checking the task's
plan file presence (using existing helpers like checkTask/checkProject or a
small async fs/stat or helper isPlanExists(taskId)) and pass that boolean into
taskStateManager.handleUiEvent(taskId, { type: 'USER_STOPPED', hasPlan },
checkTask, checkProject) so the XState guard noPlanYet can route correctly.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 3 blocking issue(s) require fixes.
Resolution Status
- ✅ Resolved: 3 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 3 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 0 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 1 findings verified as genuine issues
- 👤 Needs Human Review: 1 findings require manual verification
🚨 Blocking Issues
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
- quality: Hardcoded hasPlan: true routes planning-stuck tasks to wrong Kanban column
- quality: [FROM COMMENTS] Inline activeStates array could be extracted to shared constant (CodeRabbit)
Verdict
All 3 previous findings are resolved: the critical forceTransition() crash is fixed (replaced with handleUiEvent), the stale closure issue is fixed (both task and project re-fetched inside setTimeout), and the stale cache issue is fixed (uses getCurrentState() for live XState snapshot). All 20 CI checks are passing.
However, 1 new MEDIUM finding blocks merge: NEW-001 (confirmed valid) — the fallback safety net hardcodes hasPlan: true, causing tasks stuck in the 'planning' state to incorrectly route to human_review instead of backlog. The XState machine's noPlanYet guard requires hasPlan === false to route to backlog, and the established pattern in execution-handlers.ts dynamically determines hasPlan by reading the plan file. Fix: use const hasPlan = currentState !== 'planning'; or adopt the plan-file-check pattern.
Additionally, NEW-002 (LOW, needs human review) notes that the isFinalPhaseUpdate bypass may allow stale phases to reach the renderer, though the impact is limited to transient UI display. CMT-001 (INFO) suggests extracting activeStates to a shared constant for consistency.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (3 selected of 3 total)
🟡 [NEW-001] [MEDIUM] Hardcoded hasPlan: true routes planning-stuck tasks to wrong Kanban column
📁 apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts:118
The fallback safety net at line 118 sends USER_STOPPED with hasPlan: true regardless of which XState active state the task is stuck in. For tasks stuck in 'planning' (process exited before producing a plan), the noPlanYet guard in the XState machine checks event.hasPlan === false to route to backlog. With hasPlan hardcoded to true, these tasks incorrectly land in human_review instead of backlog. The TASK_STOP handler in execution-handlers.ts (lines 299-314) shows the correct pattern: reading the plan file and checking totalCount > 0 to dynamically determine hasPlan.
Suggested fix:
Determine hasPlan dynamically: `const hasPlan = currentState !== 'planning';` or better, adopt the plan-file-check pattern from execution-handlers.ts: read the plan file and check if totalCount > 0.
🔵 [NEW-002] [LOW] Final phase update bypass may allow stale phase to reach renderer after XState settles
📁 apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts:256
The isFinalPhaseUpdate bypass lets complete/failed phases through to the renderer when XState is settled, but emitPhaseFromState already sends the correct final phase via the XState subscriber path. The sequence number protection has a gap: updateTaskStatus resets executionProgress without a sequenceNumber field, and events with seq 0 bypass the guard. This could allow a stale 'failed' phase from the agent process to overwrite a correct 'complete' phase in the renderer. Impact is limited to transient UI phase display (task stays in correct Kanban column).
Suggested fix:
Either remove the isFinalPhaseUpdate bypass entirely (emitPhaseFromState already delivers the correct final phase), or include sequenceNumber: 0 in the updateTaskStatus reset so the guard can protect against stale events.
🟡 [CMT-001] [MEDIUM] [FROM COMMENTS] Inline activeStates array could be extracted to shared constant (CodeRabbit)
📁 apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts:109
CodeRabbit suggests extracting the hardcoded activeStates array into a shared XSTATE_ACTIVE_STATES constant in task-state-utils.ts, alongside the existing XSTATE_SETTLED_STATES. The codebase already follows this pattern for XState constants. This is a valid code hygiene improvement but not a merge blocker — the array is only used in one place and its values are straightforward.
This review was generated by Auto Claude.
…onstants Resolves NEW-001 and CMT-001 from the follow-up review: - Extract XSTATE_ACTIVE_STATES to shared constant in task-state-utils.ts - Export XSTATE_ACTIVE_STATES from state-machines index - Replace hardcoded hasPlan: true with dynamic plan file check - Pattern matches TASK_STOP handler (execution-handlers.ts lines 299-310) - Tasks stuck in 'planning' state now correctly route to backlog, not human_review - Improved logging shows hasPlan value for debugging This ensures the fallback safety net routes tasks to the correct Kanban column based on whether a plan file exists with subtasks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts`:
- Around line 106-136: Extract the inline plan parsing in
agent-events-handlers.ts into a shared utility (e.g.,
hasPlanWithSubtasks(project, task): boolean) placed in plan-file-utils.ts and
used here and in execution-handlers.ts; move the logic that calls getPlanPath
and readFileSync, parses JSON, inspects plan.phases and counts subtasks
(totalCount > 0), and returns true/false, ensuring it swallows/propagates errors
consistently, then replace the inline block in the setTimeout handler (the code
that computes hasPlan before calling taskStateManager.handleUiEvent) with a call
to hasPlanWithSubtasks(findTaskAndProject(...).project,
findTaskAndProject(...).task) or by reusing the already-extracted
checkTask/checkProject variables so the handler only calls the new utility.
- Around line 267-275: The final-phase send logic (isFinalPhaseUpdate /
progress.phase) is allowing 'complete'/'failed' events through after XState
settles and this can race with status updates causing UI flicker; update the
handler that emits "execution-progress" (the block using xstateInTerminalState,
isFinalPhaseUpdate and taskId) to gate sending final phases based on the current
task status (only emit phase updates when the task's status === 'in_progress' or
when the status itself is a matching final status), and also adjust
PhaseProgressIndicator rendering to ignore phase values unless task.status is
in_progress (see PhaseProgressIndicator.tsx) or implement batching in useIpc.ts
so phase updates are applied in the same micro-batch as status updates (plan →
progress → status) to preserve correct ordering.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 2 blocking issue(s) require fixes.
Resolution Status
- ✅ Resolved: 3 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 3 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 5 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 2 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
- quality: Fallback safety net timeout is not cancelled when task is restarted
Verdict
All 20 CI checks are passing ✅. All 3 previous findings are resolved: NEW-001 (hardcoded hasPlan) fixed with dynamic plan detection, CMT-001 (inline activeStates) extracted to shared XSTATE_ACTIVE_STATES constant, and NEW-002 (final phase bypass) dismissed as false positive since sequence number protection in the task store prevents stale updates from overriding correct state.
However, 1 new MEDIUM severity finding was confirmed valid: the fallback safety net setTimeout (NCR-NEW-001) does not store its timer reference and cannot be cancelled. If a user stops a task and restarts it within the 500ms window, the stale callback fires, sees the new execution's active XState state, and forces USER_STOPPED — incorrectly killing the newly restarted task. This is a real logic bug with a narrow but reproducible race window.
1 LOW severity finding also confirmed: duplicate hasPlan detection logic across two files (CMT-NEW-001), which is a code quality concern but not merge-blocking on its own.
5 of 7 total findings were dismissed as false positives after validation, demonstrating strong code quality overall. The author has done excellent work addressing all previous review feedback. Only the timer cancellation race condition needs to be fixed before merge.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (3 selected of 3 total)
🟡 [NCR-NEW-001] [MEDIUM] Fallback safety net timeout is not cancelled when task is restarted
📁 apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts:106
The setTimeout at line 106 captures taskId in a closure but the timer reference is never stored, so it cannot be cancelled. If a user stops a task and restarts it within the 500ms window (STUCK_TASK_FALLBACK_TIMEOUT_MS), the stale callback fires, sees the new execution's active XState state (e.g. 'planning'), and forces USER_STOPPED — incorrectly killing the newly restarted task. There is no generation counter, process ID, or any other mechanism to distinguish 'active because restarted' from 'stuck because old process exited'. Fix: Store the timeout reference and clear it when a new process starts for the same taskId, or capture the XState state at schedule time and verify it hasn't changed in the callback.
Suggested fix:
Store the timer reference in a Map<taskId, NodeJS.Timeout> and clear it in handleProcessStarted or when a new process is spawned for the same taskId. Alternatively, capture the current state at schedule time and compare in the callback: const stateAtSchedule = currentState; then in callback check if state === stateAtSchedule.
🔵 [CMT-NEW-001] [LOW] Duplicate hasPlan detection logic across two handler files
📁 apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts:114
The hasPlan detection pattern (read plan file, parse JSON, count subtasks) is duplicated in agent-events-handlers.ts (lines 114-127) and execution-handlers.ts (lines 299-310) with minor implementation differences (readFileSync vs safeReadFileSync, inline counting vs checkSubtasksCompletion helper). The agent-events-handlers.ts version even references the other in its comment. A shared utility function would improve maintainability.
Suggested fix:
Extract a hasPlanWithSubtasks(project, task) function into plan-file-utils.ts and call it from both locations.
🔵 [CMT-NEW-001] [LOW] [FROM COMMENTS] Duplicate hasPlan detection logic should be extracted to shared utility (CodeRabbit)
📁 apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts:114
CodeRabbit correctly identified that the hasPlan detection pattern is duplicated across agent-events-handlers.ts and execution-handlers.ts. Both perform identical logic with minor implementation differences. A shared utility in plan-file-utils.ts would eliminate this duplication. This is a valid code quality suggestion but not merge-blocking.
Suggested fix:
Create hasPlanWithSubtasks(project, task) in plan-file-utils.ts and use it in both handler files.
This review was generated by Auto Claude.
- Extract hasPlan logic into shared hasPlanWithSubtasks() utility in plan-file-utils.ts to eliminate duplication and centralize plan validation logic - Make setTimeout callback async to avoid blocking readFileSync in event loop - Replace hardcoded terminal status check with cleaner .includes() pattern - Add status gate for final phase updates to prevent UI flicker when failed phase arrives after task has transitioned to human_review - Import and use hasPlanWithSubtasks in agent-events-handlers.ts All review comments addressed: - Gemini: Critical/Medium - forceTransition method, magic number (already fixed in previous commit) - Gemini: Medium - hardcoded status list (now uses .includes) - CodeRabbit: Trivial - extract XSTATE_ACTIVE_STATES (already fixed in previous commit) - CodeRabbit: Minor - derive hasPlan from file check (now uses shared utility) - CodeRabbit: Minor - prevent UI flicker from final phase updates (now gates on status) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| setTimeout(async () => { | ||
| const currentState = taskStateManager.getCurrentState(taskId); | ||
|
|
||
| if (currentState && XSTATE_ACTIVE_STATES.has(currentState)) { | ||
| const { task: checkTask, project: checkProject } = findTaskAndProject(taskId, projectId); | ||
| if (checkTask && checkProject) { | ||
| // Use shared utility to determine if a valid implementation plan exists | ||
| const hasPlan = hasPlanWithSubtasks(checkProject, checkTask); | ||
|
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts`:
- Line 106: The setTimeout callback is marked async which misleadingly suggests
it makes synchronous I/O non-blocking even though hasPlanWithSubtasks uses
readFileSync; remove the unnecessary async from the setTimeout callback to avoid
confusion, and if you need non-blocking behavior replace readFileSync inside
hasPlanWithSubtasks with an async fs.readFile-based implementation (or return a
Promise) so the work is truly non-blocking.
| // force it to human_review after a short delay. This prevents tasks from getting stuck | ||
| // when the process exits without XState properly handling it. | ||
| // We check XState's current state directly to avoid stale cache issues from projectStore. | ||
| setTimeout(async () => { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
async on the setTimeout callback does not make readFileSync non-blocking.
The commit message mentions making the callback async to avoid blocking the event loop, but async only causes the function to return a Promise — it does not offload synchronous I/O. hasPlanWithSubtasks calls readFileSync which will still block the event loop regardless. The async keyword is harmless here (setTimeout discards the return value), but it's misleading about its purpose.
♻️ Suggested fix: remove unnecessary async
- setTimeout(async () => {
+ setTimeout(() => {🤖 Prompt for AI Agents
In `@apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts` at line 106,
The setTimeout callback is marked async which misleadingly suggests it makes
synchronous I/O non-blocking even though hasPlanWithSubtasks uses readFileSync;
remove the unnecessary async from the setTimeout callback to avoid confusion,
and if you need non-blocking behavior replace readFileSync inside
hasPlanWithSubtasks with an async fs.readFile-based implementation (or return a
Promise) so the work is truly non-blocking.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 3 unresolved finding(s) from previous review.
Resolution Status
- ✅ Resolved: 0 previous findings addressed
- ❌ Unresolved: 3 previous findings remain
- 🆕 New Issues: 4 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 2 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 3 findings verified as genuine issues
- 👤 Needs Human Review: 1 findings require manual verification
🚨 Blocking Issues
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
- quality: [UNRESOLVED] Fallback safety net timeout is not cancelled when task is restarted
- quality: Fallback safety net timeout cannot be cancelled when task is restarted
Verdict
All 20 CI checks are passing ✅. The commit addresses 2 of 3 previous findings (hasPlan extraction partially done, CodeRabbit duplication partially resolved), but the MEDIUM severity finding NCR-NEW-001 (fallback safety net timeout cannot be cancelled when task is restarted) remains unresolved and was independently confirmed by both resolution-verifier and new-code-reviewer. The setTimeout at line 106 of agent-events-handlers.ts still does not store its timer reference, creating a race condition where a stale timer could incorrectly stop a freshly restarted task within the 500ms window. The XSTATE_ACTIVE_STATES guard does NOT prevent this because a restarted task would be in an active state (planning/coding) when the stale timer fires. Additionally, the misleading async keyword on the setTimeout callback (flagged by CodeRabbit) was not addressed. Two findings were dismissed as false positives: the readFileSync blocking concern (pre-existing codebase pattern, not a PR regression) and the final-phase-update leak (renderer gracefully ignores non-existent tasks).
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (5 selected of 5 total)
🟡 [NCR-NEW-001] [MEDIUM] [UNRESOLVED] Fallback safety net timeout is not cancelled when task is restarted
📁 apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts:106
The setTimeout at line 106 captures taskId in a closure but the timer reference is never stored, so it cannot be cancelled. If a user stops a task and restarts it within the 500ms window (STUCK_TASK_FALLBACK_TIMEOUT_MS), the stale callback fires, sees the new execution's active XState state (e.g. 'planning'), and forces USER_STOPPED — incorrectly killing the newly restarted task. There is no generation counter, process ID, or any other mechanism to distinguish 'active because restarted' from 'stuck because old process exited'. Fix: Store the timeout reference and clear it when a new process starts for the same taskId, or capture the XState state at schedule time and verify it hasn't changed in the callback.
Resolution note: The setTimeout at line 106 of agent-events-handlers.ts still does not store its timer reference. The return value of setTimeout is not assigned to any variable, so the timer cannot be cancelled if the task is restarted within the 500ms window. The refactoring commit extracted the constant STUCK_TASK_FALLBACK_TIMEOUT_MS and switched to hasPlanWithSubtasks, but the core issue remains.
Suggested fix:
Store the timer reference in a Map<taskId, NodeJS.Timeout> and clear it in handleProcessStarted or when a new process is spawned for the same taskId. Alternatively, capture the current state at schedule time and compare in the callback: const stateAtSchedule = currentState; then in callback check if state === stateAtSchedule.
🟡 [NCR-NEW-001] [MEDIUM] Fallback safety net timeout cannot be cancelled when task is restarted
📁 apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts:106
The setTimeout at line 106 does not store its timer reference. If a task process exits and is restarted within the 500ms window (e.g., via automated queue), the stale timer fires and getCurrentState() returns the NEW active state (planning/coding), which passes the XSTATE_ACTIVE_STATES guard. USER_STOPPED is then incorrectly sent to the restarted task, killing it. Fix: store timer reference in a Map<taskId, NodeJS.Timeout> and clear any existing timer in the TASK_START handler, or add a generation counter to distinguish execution sessions.
Suggested fix:
Store the setTimeout return value in a Map<string, NodeJS.Timeout> keyed by taskId. Before setting a new timer, clear any existing timer for that taskId. Alternatively, add an execution generation counter that increments on each task start, and check it in the timer callback.
🔵 [CMT-NEW-001] [LOW] Duplicate hasPlan detection logic not fully migrated to shared utility
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:299
The hasPlanWithSubtasks utility was created in plan-file-utils.ts and used in agent-events-handlers.ts, but execution-handlers.ts (TASK_STOP handler, lines 299-310) still has its own inline implementation using safeReadFileSync + checkSubtasksCompletion. Both implementations are semantically identical. The second call site should also use the shared utility.
Suggested fix:
Import hasPlanWithSubtasks from plan-file-utils.ts in execution-handlers.ts and replace lines 299-310 with: const hasPlan = hasPlanWithSubtasks(project, task);
🔵 [CMT-001] [LOW] Misleading async keyword on setTimeout callback with no await expressions
📁 apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts:106
The setTimeout callback at line 106 is marked async but contains no await expressions. All operations inside (getCurrentState, findTaskAndProject, hasPlanWithSubtasks with readFileSync) are synchronous. The async keyword implies non-blocking behavior that does not exist, as flagged by CodeRabbit.
Suggested fix:
Remove the async keyword: change `setTimeout(async () => {` to `setTimeout(() => {`
🔵 [CMT-001] [LOW] [FROM COMMENTS] Misleading async keyword on setTimeout callback (from CodeRabbit)
📁 apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts:106
CodeRabbit correctly identified that the async keyword on the setTimeout callback is misleading. The callback contains no await expressions and hasPlanWithSubtasks uses readFileSync. This was not addressed in the latest commit.
Suggested fix:
Remove the async keyword from line 106.
This review was generated by Auto Claude.
Fixed all 3 findings from the latest auto-claude review: 1. [NCR-NEW-001] MEDIUM: Fallback safety net timeout race condition - Store setTimeout timer reference in a Map keyed by taskId - Export cancelFallbackTimer() function to clear pending timers - Call cancelFallbackTimer() at start of TASK_START handler - Prevents stale timer from incorrectly stopping newly restarted tasks - Clean up timer reference after it fires 2. [CMT-NEW-001] LOW: Duplicate hasPlan detection logic - Replace inline hasPlan logic in execution-handlers.ts TASK_STOP - Use shared hasPlanWithSubtasks() utility from plan-file-utils.ts - Eliminates code duplication and ensures consistent behavior 3. [CMT-001] LOW: Misleading async keyword - Remove async from setTimeout callback in agent-events-handlers.ts - No await expressions exist in the callback - All operations (getCurrentState, hasPlanWithSubtasks) are synchronous All changes maintain backward compatibility and fix the race condition where restarting a task within 500ms could be incorrectly stopped by the fallback timer from the previous process exit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
size/M
1 participant
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements