-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix: Update phase logs when task is stopped #353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
3a81e28
fbdfad6
3c0f229
bc9cf39
8b64fb4
9c2fea0
1d31e4a
c2857ce
52d4c36
7d2ae82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import path from 'path'; | ||
| import { existsSync, readFileSync, watchFile } from 'fs'; | ||
| import { existsSync, readFileSync, writeFileSync, watchFile, renameSync, unlinkSync } from 'fs'; | ||
| import { EventEmitter } from 'events'; | ||
| import type { TaskLogs, TaskLogPhase, TaskLogStreamChunk, TaskPhaseLog } from '../shared/types'; | ||
|
|
||
|
|
@@ -376,6 +376,106 @@ export class TaskLogService extends EventEmitter { | |
| const logFile = path.join(specDir, 'task_logs.json'); | ||
| return existsSync(logFile); | ||
| } | ||
|
|
||
| /** | ||
| * Mark all active phases as stopped when a task is interrupted | ||
| * Updates both main and worktree log files if they exist | ||
| * | ||
| * @param specId - The spec ID | ||
| * @param projectPath - Project root path (needed to find worktree) | ||
| * @param specsRelPath - Relative path to specs (e.g., ".auto-claude/specs") | ||
| */ | ||
| 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); | ||
|
|
||
|
Comment on lines
+388
to
+401
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 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 However, adding validation for these parameters would still be reasonable as a defense-in-depth measure:
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 |
||
| // Update main spec dir logs | ||
| this.updateLogFilePhaseStatus(path.join(mainSpecDir, 'task_logs.json')); | ||
|
|
||
| // Update worktree spec dir logs if it exists | ||
| if (existsSync(worktreeSpecDir)) { | ||
| this.updateLogFilePhaseStatus(path.join(worktreeSpecDir, 'task_logs.json')); | ||
| } | ||
|
|
||
| // Reload and emit updated logs | ||
| const updatedLogs = this.loadLogs(mainSpecDir, projectPath, specsRelPath, specId); | ||
| if (updatedLogs) { | ||
| this.logCache.set(mainSpecDir, updatedLogs); | ||
| this.emit('logs-changed', specId, updatedLogs); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Update a single log file to mark active phases as stopped | ||
| * Uses atomic write (temp file + rename) to prevent corruption | ||
| */ | ||
| private updateLogFilePhaseStatus(logFilePath: string): void { | ||
| try { | ||
| // Read file - if it doesn't exist, readFileSync will throw and we'll catch it | ||
| const content = readFileSync(logFilePath, 'utf-8'); | ||
| const logs = JSON.parse(content) as TaskLogs; | ||
|
|
||
| let hasChanges = false; | ||
| const phases: TaskLogPhase[] = ['planning', 'coding', 'validation']; | ||
|
|
||
| for (const phase of phases) { | ||
| if (logs.phases[phase]?.status === 'active') { | ||
| // Mark as stopped since the task was stopped by the user | ||
| logs.phases[phase].status = 'stopped'; | ||
| logs.phases[phase].completed_at = new Date().toISOString(); | ||
| hasChanges = true; | ||
| } | ||
| } | ||
|
|
||
| // Only write if we made changes | ||
| if (hasChanges) { | ||
| logs.updated_at = new Date().toISOString(); | ||
|
|
||
| // Use atomic write: temp file + rename to prevent corruption | ||
| const tempFilePath = `${logFilePath}.tmp`; | ||
| try { | ||
| writeFileSync(tempFilePath, JSON.stringify(logs, null, 2), 'utf-8'); | ||
| renameSync(tempFilePath, logFilePath); | ||
| console.log(`[TaskLogService] Marked active phases as stopped in ${logFilePath}`); | ||
| } catch (writeError) { | ||
| // Clean up temp file if it exists | ||
| if (existsSync(tempFilePath)) { | ||
| unlinkSync(tempFilePath); | ||
| } | ||
| throw writeError; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| // File doesn't exist, is corrupted, or write failed - log and continue | ||
| console.error(`[TaskLogService] Failed to update log file ${logFilePath}:`, error); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Validate specId format to prevent path traversal attacks | ||
| * Valid format: 3 digits followed by dash and alphanumeric characters with dashes | ||
| * Example: "001-feature-name" or "123-bug-fix" | ||
| */ | ||
| private isValidSpecId(specId: string): boolean { | ||
| // Check for path traversal patterns | ||
| if (specId.includes('..') || specId.includes('/') || specId.includes('\\')) { | ||
| return false; | ||
| } | ||
|
|
||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SpecId validation rejects specs numbered 1000 or higherThe |
||
| } | ||
| } | ||
|
|
||
| // Singleton instance | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| { | ||
| "status": { | ||
| "pending": "Pending", | ||
| "active": "Running", | ||
| "completed": "Complete", | ||
| "failed": "Failed", | ||
| "stopped": "Stopped", | ||
| "interrupted": "Interrupted" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| { | ||
| "status": { | ||
| "pending": "En attente", | ||
| "active": "En cours", | ||
| "completed": "Terminé", | ||
| "failed": "Échoué", | ||
| "stopped": "Arrêté", | ||
| "interrupted": "Interrompu" | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing phase_end event emission for stopped status
The
emitNewEntriesfunction emitsphase_endstream-chunk events when phases transition tocompletedorfailed, but the newly addedstoppedstatus is not included in this condition. When a phase transitions fromactivetostopped, listeners expectingphase_endevents 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.