Fix: Update phase logs when task is stopped #353
Conversation
When a task is stopped, the task status changes to 'backlog' and execution progress resets to 'idle', but the task_logs.json files were not being updated. This caused the logs tab to continue showing "Running" badges for phases that were still marked as 'active' in the log files. Changes: - Add markActivePhasesStopped() method to TaskLogService - Update both main and worktree task_logs.json files - Mark active phases as 'failed' (interrupted) when task is stopped - Call this method in TASK_STOP handler before sending status change event - Emit logs-changed event to update UI immediately Fixes issue where logs tab shows task as running after it's been stopped.
…log update fails The stop button wasn't working because an error in the phase log update code was preventing the task from stopping. Now the log update is wrapped in a try-catch block so the task will always stop, even if updating the logs fails. This ensures the critical stop functionality works while still attempting to update the phase logs when possible.
Replace dynamic require() call with static import at the top of the file. This is cleaner and avoids potential module resolution issues.
…ases
When a task is stopped by the user, phases are now marked as 'stopped' instead
of 'failed'. This is more semantically correct and clearer to users.
Changes:
- Add 'stopped' to TaskLogPhaseStatus type
- Update task-log-service to mark phases as 'stopped' when task is stopped
- Add 'Stopped' badge in TaskLogs UI with muted styling
- Add stopped status styling for phase sections
The 'stopped' status uses muted colors to distinguish it from errors ('failed')
while still indicating the phase didn't complete normally.
📝 WalkthroughWalkthroughAdds a new 'stopped' phase status, updates TaskLogService to mark active phases stopped with atomic file writes and emit refreshed logs, invokes that service during task-stop handling (errors swallowed), and updates UI and i18n to render the new stopped state. Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Task Stop Handler
participant Service as TaskLogService
participant FS as File System
participant Cache as Log Loader
participant Emitter as Event Emitter
participant UI as TaskLogs UI
Handler->>Service: markActivePhasesStopped(specId, projectPath, specsRelPath)
alt update succeeds
Service->>FS: read main log file
Service->>Service: mark active phases as 'stopped' + set completed_at
Service->>FS: write updated main log file (atomic: temp -> rename)
Service->>FS: read worktree log file
Service->>Service: mark active phases as 'stopped' + set completed_at
Service->>FS: write updated worktree log file (atomic)
Service->>Cache: reload logs
Service->>Emitter: emit updated logs
Emitter->>UI: deliver updated logs
UI->>UI: render phases with 'stopped' status (localized, muted styling)
else error during update
Service-->>Handler: swallow/log error (no throw)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)apps/frontend/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
apps/frontend/**/*.{ts,tsx}⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (2)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/frontend/src/main/task-log-service.tsapps/frontend/src/renderer/components/task-detail/TaskLogs.tsxapps/frontend/src/shared/types/task.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{ts,tsx}: Always use translation keys withuseTranslation()for all user-facing text in React/TypeScript frontend components - use formatnamespace:section.key(e.g.,navigation:items.githubPRs)
Never use hardcoded strings in JSX/TSX files for user-facing text - always reference translation keys fromapps/frontend/src/shared/i18n/locales/
Files:
apps/frontend/src/shared/types/task.tsapps/frontend/src/renderer/components/task-detail/TaskLogs.tsxapps/frontend/src/main/task-log-service.tsapps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/shared/types/task.tsapps/frontend/src/renderer/components/task-detail/TaskLogs.tsxapps/frontend/src/main/task-log-service.tsapps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
🧬 Code graph analysis (3)
apps/frontend/src/renderer/components/task-detail/TaskLogs.tsx (1)
.design-system/src/components/Badge.tsx (1)
Badge(9-27)
apps/frontend/src/main/task-log-service.ts (1)
apps/frontend/src/shared/types/task.ts (2)
TaskLogs(87-96)TaskLogPhase(60-60)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (2)
apps/frontend/src/shared/constants/config.ts (1)
getSpecsDir(102-105)apps/frontend/src/main/task-log-service.ts (1)
taskLogService(447-447)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (8)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (2)
12-12: LGTM: Correct singleton import.The import correctly uses the singleton instance exported from task-log-service.ts.
188-200: LGTM: Robust error handling for log updates.The try-catch wrapper ensures task stopping succeeds even if log file updates fail. The error is appropriately logged while allowing the core stop operation to proceed.
apps/frontend/src/shared/types/task.ts (1)
61-61: LGTM: Type extension enables stopped phase status.The addition of 'stopped' to TaskLogPhaseStatus appropriately distinguishes user-initiated stops from failures, supporting the new functionality across the UI and backend.
apps/frontend/src/renderer/components/task-detail/TaskLogs.tsx (1)
242-242: LGTM: Consistent styling for stopped status.The conditional styling for the stopped phase status appropriately uses muted colors to visually distinguish it from active, completed, and failed states.
apps/frontend/src/main/task-log-service.ts (4)
2-2: LGTM: Import addition for file writing.The writeFileSync import is appropriately added to support the new log update functionality.
380-410: LGTM: Well-structured method with appropriate error handling.The
markActivePhasesStoppedmethod correctly:
- Resolves both main and worktree log file paths
- Updates log files via the helper method
- Reloads and emits updated logs to listeners
- Handles missing worktree directories gracefully
The error handling in the caller (execution-handlers.ts) ensures non-critical log update failures don't block task stopping.
412-443: LGTM: Atomic update with proper error handling.The
updateLogFilePhaseStatushelper appropriately:
- Checks file existence before attempting updates
- Only writes when changes are made
- Sets both status and completed_at timestamp
- Catches and logs errors without throwing
Note: The read-modify-write pattern has an inherent race condition risk, but this is acceptable here since log updates are non-critical and failures are logged.
446-447: LGTM: Singleton export for service access.The singleton export enables centralized access to the task log service, which is correctly imported and used in execution-handlers.ts.
- Create taskLogs namespace with status translations (EN/FR) - Update TaskLogs.tsx to use translation keys instead of hardcoded strings - Register taskLogs namespace in i18n configuration - Fix CodeQL race condition warning in task-log-service.ts by removing redundant existsSync check Translation keys added: - status.pending - status.active (Running) - status.completed (Complete) - status.failed - status.stopped - status.interrupted 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
MikeeBuilds
added
bug
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟡 MERGE WITH CHANGES
4 high-priority issues to address
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | Medium | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- High: 4 issue(s)
- Medium: 3 issue(s)
Generated by Auto Claude PR Review
Findings (4 selected of 7 total)
🟠 [HIGH] Non-atomic file writes cause race condition
📁 apps/frontend/src/main/task-log-service.ts:proposed
Python backend uses atomic writes (temp file + rename), but TypeScript writeFileSync() is not atomic and could corrupt files during concurrent access.
Suggested fix:
Use temp file + renameSync() pattern for atomic writes
🟠 [HIGH] Path traversal vulnerability in specId
📁 apps/frontend/src/main/task-log-service.ts:123
specId is used directly in path construction without validation. Malicious specId like '../../etc' could escape project directory.
Suggested fix:
Add specId validation: /^[0-9]{3}-[a-z0-9-]+$/ and reject paths containing '..'
🟠 [HIGH] Missing taskId to specId mapping in TASK_STOP handler
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:183
TASK_STOP receives taskId but markActivePhasesStopped needs specId. Handler must call findTaskAndProject() to get the task's specId.
Suggested fix:
Add: const { task } = findTaskAndProject(taskId); then use task.specId
🟠 [HIGH] TaskLogPhaseStatus missing 'stopped' value
📁 apps/frontend/src/shared/types/task.ts:61
Type definition must include 'stopped' for the feature to work. Currently only has: pending, active, completed, failed.
Suggested fix:
Change to: 'pending' | 'active' | 'completed' | 'failed' | 'stopped'
This review was generated by Auto Claude.
Addresses PR review feedback with the following security improvements: 1. **Atomic file writes**: Use temp file + rename pattern to prevent file corruption during concurrent access. This ensures writes are atomic and matches the pattern used in the Python backend. 2. **Path traversal protection**: Add specId validation to prevent directory traversal attacks. Validates format (XXX-name) and rejects paths containing '..' or directory separators. Fixes the two remaining high-priority security issues from PR review. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/main/task-log-service.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{ts,tsx}: Always use translation keys withuseTranslation()for all user-facing text in React/TypeScript frontend components - use formatnamespace:section.key(e.g.,navigation:items.githubPRs)
Never use hardcoded strings in JSX/TSX files for user-facing text - always reference translation keys fromapps/frontend/src/shared/i18n/locales/
Files:
apps/frontend/src/main/task-log-service.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/task-log-service.ts
🧬 Code graph analysis (1)
apps/frontend/src/main/task-log-service.ts (1)
apps/frontend/src/shared/types/task.ts (2)
TaskLogs(87-96)TaskLogPhase(60-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (3)
apps/frontend/src/main/task-log-service.ts (3)
2-2: LGTM! Imports support atomic file operations.The additional fs imports (writeFileSync, renameSync, unlinkSync) are necessary for the atomic write pattern implemented in the new updateLogFilePhaseStatus method, preventing file corruption during concurrent access.
418-462: Excellent atomic write implementation.The atomic write pattern (temp file + rename) is well-implemented and addresses the file system race condition concern from previous reviews. Key strengths:
- Writes to temp file first, then atomically renames to target (lines 447-448)
- Proper cleanup of temp file on write failure (lines 452-453)
- Only writes when changes are detected (line 441)
- Graceful error handling allows task stopping to succeed even if log updates fail
469-478: No changes needed. The regex pattern/^[0-9]{3}-[a-z0-9-]+$/is correct. All spec IDs in the codebase use lowercase-only format (e.g.,001-test-feature,002-in-progress,013-screenshots-on-tasks). No uppercase spec IDs exist anywhere, so the validation is not overly restrictive.Likely an incorrect or invalid review comment.
| markActivePhasesStopped(specId: string, projectPath: string, specsRelPath: string): void { | ||
| // Validate specId to prevent path traversal attacks | ||
| if (!this.isValidSpecId(specId)) { | ||
| console.error(`[TaskLogService] Invalid specId format: ${specId}`); | ||
| return; | ||
| } | ||
|
|
||
| // Get watched paths for this spec | ||
| const watchedInfo = this.watchedPaths.get(specId); | ||
|
|
||
| // Build paths for main and worktree spec directories | ||
| const mainSpecDir = watchedInfo?.mainSpecDir || path.join(projectPath, specsRelPath, specId); | ||
| const worktreeSpecDir = watchedInfo?.worktreeSpecDir || path.join(projectPath, '.worktrees', specId, specsRelPath, specId); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the full context of the markActivePhasesStopped method and find all callers
echo "=== Looking for markActivePhasesStopped method definition and calls ==="
rg -n 'markActivePhasesStopped' --type=ts
echo -e "\n=== Checking execution-handlers.ts for task stop handlers ==="
rg -n -A 15 -B 3 'TASK_STOP' apps/frontend/src/main/ipc-handlers/task/execution-handlers.tsRepository: AndyMik90/Auto-Claude
Length of output: 1663
🏁 Script executed:
#!/bin/bash
# Verify the sources of the parameters passed to markActivePhasesStopped
echo "=== Finding findTaskAndProject function definition ==="
rg -n -A 15 'function findTaskAndProject|const findTaskAndProject' --type=ts
echo -e "\n=== Finding getSpecsDir function definition ==="
rg -n -A 10 'function getSpecsDir|const getSpecsDir' --type=ts
echo -e "\n=== Checking if project.path and task.specId are user-controllable ==="
rg -n -B 5 -A 3 'interface.*Project|type.*Project' apps/frontend/src/main/ --type=ts | head -50Repository: AndyMik90/Auto-Claude
Length of output: 6557
🏁 Script executed:
#!/bin/bash
# Check where projects are created/added and how paths are validated
echo "=== Looking for project creation/import logic ==="
rg -n 'addProject|createProject|importProject|projects\.push' apps/frontend/src/main/ --type=ts | head -20
echo -e "\n=== Checking ProjectStore addProject method ==="
rg -n -A 20 'addProject.*{' apps/frontend/src/main/project-store.ts
echo -e "\n=== Checking if project paths are validated ==="
rg -n -B 5 -A 10 'path.isAbsolute|path.resolve|normalize' apps/frontend/src/main/project-store.ts | head -40Repository: AndyMik90/Auto-Claude
Length of output: 2562
🏁 Script executed:
#!/bin/bash
# Check the addProject method properly
echo "=== Looking for addProject method implementation ==="
rg -n -A 30 'addProject\(projectPath' apps/frontend/src/main/project-store.ts
echo -e "\n=== Check where project paths come from in IPC handlers ==="
rg -n -B 10 'projectStore.addProject' apps/frontend/src/main/ipc-handlers/project-handlers.ts
echo -e "\n=== Check if paths are validated before being used ==="
rg -n 'existsSync|stat|isDirectory' apps/frontend/src/main/ipc-handlers/project-handlers.ts | head -20Repository: AndyMik90/Auto-Claude
Length of output: 3060
🏁 Script executed:
#!/bin/bash
# Check how the project store is persisted and loaded
echo "=== Check projectStore save/load mechanism ==="
rg -n -A 15 'save\(' apps/frontend/src/main/project-store.ts | head -30
echo -e "\n=== Check how projects are loaded from storage ==="
rg -n -A 15 'load\(' apps/frontend/src/main/project-store.ts | head -40
echo -e "\n=== Check if there's any editing of project paths after creation ==="
rg -n 'path.*=' apps/frontend/src/main/project-store.ts | grep -v 'projectPath\|basePath\|projectName\|autoBuildPath'Repository: AndyMik90/Auto-Claude
Length of output: 2102
🏁 Script executed:
#!/bin/bash
# Final verification: check if projectPath or specsRelPath are ever passed through IPC
echo "=== Check all IPC calls to markActivePhasesStopped ==="
rg -n -B 20 'markActivePhasesStopped' apps/frontend/src/main/ --type=ts | grep -A 20 -B 20 'ipcMain'
echo -e "\n=== Verify projectPath source in execution-handlers ==="
rg -n -B 30 'taskLogService.markActivePhasesStopped' apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts | tail -50
echo -e "\n=== Check if any IPC handler takes path parameters ==="
rg -n 'ipcMain\.(on|handle).*projectPath|ipcMain\.(on|handle).*specsRelPath' apps/frontend/src/main/ --type=tsRepository: AndyMik90/Auto-Claude
Length of output: 4231
🏁 Script executed:
#!/bin/bash
# Check if there are other entry points that could pass arbitrary projectPath/specsRelPath
echo "=== Check all methods that call updateLogFilePhaseStatus ==="
rg -n 'updateLogFilePhaseStatus' apps/frontend/src/main/ --type=ts
echo -e "\n=== Verify getSpecsDir implementation ==="
cat apps/frontend/src/shared/constants/config.ts | grep -A 5 'function getSpecsDir'
echo -e "\n=== Check if there are any other paths to modify project.path or autoBuildPath ==="
rg -n 'project\.path\s*=' apps/frontend/src/main/ --type=tsRepository: AndyMik90/Auto-Claude
Length of output: 774
🏁 Script executed:
#!/bin/bash
# Final verification: trace complete parameter flow
echo "=== Verify complete parameter source chain ==="
echo "1. TASK_STOP handler receives taskId (UUID)"
echo "2. findTaskAndProject(taskId) is called"
echo "3. Let's check what projectStore actually returns"
rg -n -A 20 'getProjects\(\)|getTasks\(' apps/frontend/src/main/project-store.ts
echo -e "\n=== Check if projectStore data can be modified via IPC ==="
rg -n 'ipcMain.*projectStore' apps/frontend/src/main/ --type=ts
echo -e "\n=== Confirm: are project paths validated when retrieved from store ==="
rg -n -B 5 -A 5 'projects\s*=' apps/frontend/src/main/project-store.ts | head -30Repository: AndyMik90/Auto-Claude
Length of output: 2403
🏁 Script executed:
#!/bin/bash
# One final check: see if specId itself is properly validated
echo "=== Check isValidSpecId implementation ==="
rg -n -A 10 'isValidSpecId' apps/frontend/src/main/task-log-service.tsRepository: AndyMik90/Auto-Claude
Length of output: 1074
Consider adding path validation as defense-in-depth, though parameters come from trusted application storage.
While projectPath and specsRelPath are not validated for path traversal, these parameters originate from the application's own persistent storage rather than untrusted user input. They are retrieved from projectStore in the TASK_STOP IPC handler and come from project paths that were already validated with existsSync() during project creation.
However, adding validation for these parameters would still be reasonable as a defense-in-depth measure:
- Check that normalized paths don't contain
..after being loaded from storage - Ensure paths remain within expected project boundaries
The approach in the proposed fix is sound, but the severity is lower than initially indicated since an attacker would need to compromise the application's persistent store file itself, not exploit untrusted IPC input.
🤖 Prompt for AI Agents
In apps/frontend/src/main/task-log-service.ts around lines 388 to 401, add
defense-in-depth validation for projectPath and specsRelPath before using them
to build file system paths: normalize and resolve each input (path.normalize +
path.resolve), reject or bail if the normalized value contains '..' path
segments or if the resolved path is not contained within the expected project
root (compare resolvedSpecPath.startsWith(resolvedProjectRoot + path.sep) or
equivalent), and log an error before returning; keep the existing isValidSpecId
check and use the validated/normalized paths when constructing mainSpecDir and
worktreeSpecDir.
|
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| // Validate format: XXX-name (e.g., 001-feature, 123-bug-fix) | ||
| const specIdPattern = /^[0-9]{3}-[a-z0-9-]+$/; | ||
| return specIdPattern.test(specId); |
There was a problem hiding this comment.
SpecId validation rejects specs numbered 1000 or higher
The isValidSpecId regex pattern ^[0-9]{3}-[a-z0-9-]+$ requires exactly 3 digits at the start. However, spec numbers are generated using padStart(3, '0') which only pads smaller numbers but doesn't truncate larger ones. When a project has 1000+ specs, specIds like 1000-feature-name would fail validation, causing markActivePhasesStopped to silently return without updating the log files. The regex should use {3,} instead of {3} to allow 3 or more digits.
| const logFile = path.join(specDir, 'task_logs.json'); | ||
| return existsSync(logFile); | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing phase_end event emission for stopped status
The emitNewEntries function emits phase_end stream-chunk events when phases transition to completed or failed, but the newly added stopped status is not included in this condition. When a phase transitions from active to stopped, listeners expecting phase_end events won't receive them, creating inconsistent event behavior. The condition at line 319 needs to include || currPhase.status === 'stopped' to maintain consistency across all terminal phase statuses.
5 participants
Summary
Fixes a bug where the logs tab continued to show phases as "Running" even after a task was stopped and moved to the planning board.
Problem
When a user stopped a task in progress:
Root Cause
The phase logs are stored in
task_logs.jsonfiles (separate from task status). When a task was stopped:task_logs.jsonfiles were not updatedstatus: 'active'in the log files continued to display as "Running"Solution
Backend Changes
task-log-service.tsmarkActivePhasesStopped()method to update phase status in log filestask_logs.jsonfilesactivephases asstoppedwhen task is interruptedlogs-changedevent to update UI immediatelyexecution-handlers.tsTASK_STOPhandler to callmarkActivePhasesStopped()Type System Changes
task.tsTaskLogPhaseStatusto include'stopped'as a valid status'failed'for user-initiated stopsUI Changes
TaskLogs.tsxstoppedstatusTesting
Tested the following scenarios:
Files Changed
apps/frontend/src/main/task-log-service.ts- Added phase status update methodsapps/frontend/src/main/ipc-handlers/task/execution-handlers.ts- Call markActivePhasesStopped on task stopapps/frontend/src/shared/types/task.ts- Extended TaskLogPhaseStatus typeapps/frontend/src/renderer/components/task-detail/TaskLogs.tsx- Added Stopped badge and stylingScreenshots
Before: Logs tab showed "Running" even after task was stopped
After: Logs tab correctly shows "Stopped" when task is stopped
Breaking Changes
None - this is purely a bug fix. The new
'stopped'status is backward compatible.Related Issues
Fixes issue where logs tab displays incorrect running status after stopping a task.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.