mirror of
https://github.com/AutoMaker-Org/automaker.git
synced 2026-03-16 21:53:07 +00:00
fix: Resolve null coalescing, feature verification, and test abort handling issues
This commit is contained in:
@@ -333,7 +333,7 @@ export class AgentExecutor {
|
||||
userFeedback
|
||||
);
|
||||
const taskStream = provider.executeQuery(
|
||||
this.buildExecOpts(options, taskPrompt, Math.min(sdkOptions?.maxTurns || 100, 50))
|
||||
this.buildExecOpts(options, taskPrompt, Math.min(sdkOptions?.maxTurns ?? 50, 50))
|
||||
);
|
||||
let taskOutput = '',
|
||||
taskStartDetected = false,
|
||||
|
||||
@@ -227,7 +227,7 @@ export class AutoModeServiceFacade {
|
||||
.replace(/\{\{taskName\}\}/g, task.description)
|
||||
.replace(/\{\{taskIndex\}\}/g, String(taskIndex + 1))
|
||||
.replace(/\{\{totalTasks\}\}/g, String(allTasks.length))
|
||||
.replace(/\{\{taskDescription\}\}/g, task.description || task.description);
|
||||
.replace(/\{\{taskDescription\}\}/g, task.description || task.name);
|
||||
if (feedback) {
|
||||
taskPrompt = taskPrompt.replace(/\{\{userFeedback\}\}/g, feedback);
|
||||
}
|
||||
@@ -636,15 +636,23 @@ Address the follow-up instructions above. Review the previous work and make the
|
||||
*/
|
||||
async verifyFeature(featureId: string): Promise<boolean> {
|
||||
const feature = await this.featureStateManager.loadFeature(this.projectPath, featureId);
|
||||
const sanitizedFeatureId = featureId.replace(/[^a-zA-Z0-9_-]/g, '-');
|
||||
const worktreePath = path.join(this.projectPath, '.worktrees', sanitizedFeatureId);
|
||||
let workDir = this.projectPath;
|
||||
|
||||
try {
|
||||
await secureFs.access(worktreePath);
|
||||
workDir = worktreePath;
|
||||
} catch {
|
||||
// No worktree
|
||||
// Use worktreeResolver to find worktree path (consistent with commitFeature)
|
||||
const branchName = feature?.branchName;
|
||||
if (branchName) {
|
||||
const resolved = await this.worktreeResolver.findWorktreeForBranch(
|
||||
this.projectPath,
|
||||
branchName
|
||||
);
|
||||
if (resolved) {
|
||||
try {
|
||||
await secureFs.access(resolved);
|
||||
workDir = resolved;
|
||||
} catch {
|
||||
// Fall back to project path
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
const verificationChecks = [
|
||||
|
||||
@@ -339,6 +339,7 @@ ${feature.spec}
|
||||
} catch (error) {
|
||||
const errorInfo = classifyError(error);
|
||||
if (errorInfo.isAbort) {
|
||||
await this.updateFeatureStatusFn(projectPath, featureId, 'interrupted');
|
||||
this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', {
|
||||
featureId,
|
||||
featureName: feature?.title,
|
||||
|
||||
@@ -211,6 +211,8 @@ export class FeatureStateManager {
|
||||
*/
|
||||
async resetStuckFeatures(projectPath: string): Promise<void> {
|
||||
const featuresDir = getFeaturesDir(projectPath);
|
||||
let featuresScanned = 0;
|
||||
let featuresReset = 0;
|
||||
|
||||
try {
|
||||
const entries = await secureFs.readdir(featuresDir, { withFileTypes: true });
|
||||
@@ -218,6 +220,7 @@ export class FeatureStateManager {
|
||||
for (const entry of entries) {
|
||||
if (!entry.isDirectory()) continue;
|
||||
|
||||
featuresScanned++;
|
||||
const featurePath = path.join(featuresDir, entry.name, 'feature.json');
|
||||
const result = await readJsonWithRecovery<Feature | null>(featurePath, null, {
|
||||
maxBackups: DEFAULT_BACKUP_COUNT,
|
||||
@@ -271,8 +274,13 @@ export class FeatureStateManager {
|
||||
if (needsUpdate) {
|
||||
feature.updatedAt = new Date().toISOString();
|
||||
await atomicWriteJson(featurePath, feature, { backupCount: DEFAULT_BACKUP_COUNT });
|
||||
featuresReset++;
|
||||
}
|
||||
}
|
||||
|
||||
logger.info(
|
||||
`[resetStuckFeatures] Scanned ${featuresScanned} features, reset ${featuresReset} features for ${projectPath}`
|
||||
);
|
||||
} catch (error) {
|
||||
// If features directory doesn't exist, that's fine
|
||||
if ((error as NodeJS.ErrnoException).code !== 'ENOENT') {
|
||||
@@ -334,6 +342,13 @@ export class FeatureStateManager {
|
||||
|
||||
// PERSIST BEFORE EMIT
|
||||
await atomicWriteJson(featurePath, feature, { backupCount: DEFAULT_BACKUP_COUNT });
|
||||
|
||||
// Emit event for UI update
|
||||
this.emitAutoModeEvent('plan_spec_updated', {
|
||||
featureId,
|
||||
projectPath,
|
||||
planSpec: feature.planSpec,
|
||||
});
|
||||
} catch (error) {
|
||||
logger.error(`Failed to update planSpec for ${featureId}:`, error);
|
||||
}
|
||||
|
||||
@@ -361,8 +361,14 @@ export class PipelineOrchestrator {
|
||||
|
||||
await this.executePipeline(context);
|
||||
|
||||
// Re-fetch feature to check if executePipeline set a terminal status (e.g., merge_conflict)
|
||||
const reloadedFeature = await this.featureLoader.getById(projectPath, featureId);
|
||||
const finalStatus = feature.skipTests ? 'waiting_approval' : 'verified';
|
||||
await this.updateFeatureStatusFn(projectPath, featureId, finalStatus);
|
||||
|
||||
// Only update status if not already in a terminal state
|
||||
if (reloadedFeature && reloadedFeature.status !== 'merge_conflict') {
|
||||
await this.updateFeatureStatusFn(projectPath, featureId, finalStatus);
|
||||
}
|
||||
logger.info(`Pipeline resume completed for feature ${featureId}`);
|
||||
this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', {
|
||||
featureId,
|
||||
@@ -417,7 +423,10 @@ export class PipelineOrchestrator {
|
||||
message: testResult.error || 'Failed to start tests',
|
||||
};
|
||||
|
||||
const completionResult = await this.waitForTestCompletion(testResult.result.sessionId);
|
||||
const completionResult = await this.waitForTestCompletion(
|
||||
testResult.result.sessionId,
|
||||
abortController.signal
|
||||
);
|
||||
if (completionResult.status === 'passed') return { success: true, testsPassed: true };
|
||||
|
||||
const sessionOutput = this.testRunnerService.getSessionOutput(testResult.result.sessionId);
|
||||
@@ -453,10 +462,19 @@ export class PipelineOrchestrator {
|
||||
|
||||
/** Wait for test completion */
|
||||
private async waitForTestCompletion(
|
||||
sessionId: string
|
||||
sessionId: string,
|
||||
signal: AbortSignal
|
||||
): Promise<{ status: TestRunStatus; exitCode: number | null; duration: number }> {
|
||||
return new Promise((resolve) => {
|
||||
const checkInterval = setInterval(() => {
|
||||
// Check for abort
|
||||
if (signal.aborted) {
|
||||
clearInterval(checkInterval);
|
||||
clearTimeout(timeoutId);
|
||||
resolve({ status: 'failed', exitCode: null, duration: 0 });
|
||||
return;
|
||||
}
|
||||
|
||||
const session = this.testRunnerService.getSession(sessionId);
|
||||
if (session && session.status !== 'running' && session.status !== 'pending') {
|
||||
clearInterval(checkInterval);
|
||||
@@ -471,6 +489,12 @@ export class PipelineOrchestrator {
|
||||
}
|
||||
}, 1000);
|
||||
const timeoutId = setTimeout(() => {
|
||||
// Check for abort before timeout resolution
|
||||
if (signal.aborted) {
|
||||
clearInterval(checkInterval);
|
||||
resolve({ status: 'failed', exitCode: null, duration: 0 });
|
||||
return;
|
||||
}
|
||||
clearInterval(checkInterval);
|
||||
resolve({ status: 'failed', exitCode: null, duration: 600000 });
|
||||
}, 600000);
|
||||
@@ -484,12 +508,15 @@ export class PipelineOrchestrator {
|
||||
|
||||
logger.info(`Attempting auto-merge for feature ${featureId} (branch: ${branchName})`);
|
||||
try {
|
||||
// Get the primary branch dynamically instead of hardcoding 'main'
|
||||
const targetBranch = await this.worktreeResolver.getCurrentBranch(projectPath);
|
||||
|
||||
// Call merge service directly instead of HTTP fetch
|
||||
const result = await performMerge(
|
||||
projectPath,
|
||||
branchName,
|
||||
worktreePath || projectPath,
|
||||
'main',
|
||||
targetBranch,
|
||||
{
|
||||
deleteWorktreeAndBranch: false,
|
||||
}
|
||||
@@ -524,12 +551,16 @@ export class PipelineOrchestrator {
|
||||
}
|
||||
}
|
||||
|
||||
/** Build a concise test failure summary for the agent */
|
||||
buildTestFailureSummary(scrollback: string): string {
|
||||
/** Shared helper to parse test output lines and extract failure information */
|
||||
private parseTestLines(scrollback: string): {
|
||||
failedTests: string[];
|
||||
passCount: number;
|
||||
failCount: number;
|
||||
} {
|
||||
const lines = scrollback.split('\n');
|
||||
const failedTests: string[] = [];
|
||||
let passCount = 0,
|
||||
failCount = 0;
|
||||
let passCount = 0;
|
||||
let failCount = 0;
|
||||
|
||||
for (const line of lines) {
|
||||
const trimmed = line.trim();
|
||||
@@ -537,30 +568,34 @@ export class PipelineOrchestrator {
|
||||
const match = trimmed.match(/(?:FAIL|FAILED)\s+(.+)/);
|
||||
if (match) failedTests.push(match[1].trim());
|
||||
failCount++;
|
||||
} else if (trimmed.includes('PASS') || trimmed.includes('PASSED')) passCount++;
|
||||
if (trimmed.match(/^>\s+.*\.(test|spec)\./)) failedTests.push(trimmed.replace(/^>\s+/, ''));
|
||||
} else if (trimmed.includes('PASS') || trimmed.includes('PASSED')) {
|
||||
passCount++;
|
||||
}
|
||||
if (trimmed.match(/^>\s+.*\.(test|spec)\./)) {
|
||||
failedTests.push(trimmed.replace(/^>\s+/, ''));
|
||||
}
|
||||
if (
|
||||
trimmed.includes('AssertionError') ||
|
||||
trimmed.includes('toBe') ||
|
||||
trimmed.includes('toEqual')
|
||||
)
|
||||
) {
|
||||
failedTests.push(trimmed);
|
||||
}
|
||||
}
|
||||
|
||||
return { failedTests, passCount, failCount };
|
||||
}
|
||||
|
||||
/** Build a concise test failure summary for the agent */
|
||||
buildTestFailureSummary(scrollback: string): string {
|
||||
const { failedTests, passCount, failCount } = this.parseTestLines(scrollback);
|
||||
const unique = [...new Set(failedTests)].slice(0, 10);
|
||||
return `Test Results: ${passCount} passed, ${failCount} failed.\n\nFailed tests:\n${unique.map((t) => `- ${t}`).join('\n')}\n\nOutput (last 2000 chars):\n${scrollback.slice(-2000)}`;
|
||||
}
|
||||
|
||||
/** Extract failed test names from scrollback */
|
||||
private extractFailedTestNames(scrollback: string): string[] {
|
||||
const failedTests: string[] = [];
|
||||
for (const line of scrollback.split('\n')) {
|
||||
const trimmed = line.trim();
|
||||
if (trimmed.includes('FAIL') || trimmed.includes('FAILED')) {
|
||||
const match = trimmed.match(/(?:FAIL|FAILED)\s+(.+)/);
|
||||
if (match) failedTests.push(match[1].trim());
|
||||
}
|
||||
}
|
||||
const { failedTests } = this.parseTestLines(scrollback);
|
||||
return [...new Set(failedTests)].slice(0, 20);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -90,25 +90,10 @@ export class PlanApprovalService {
|
||||
this.pendingApprovals.delete(key);
|
||||
}
|
||||
|
||||
// Set up timeout to prevent indefinite waiting and memory leaks
|
||||
// timeoutId stored in closure, NOT in PendingApproval object
|
||||
const timeoutId = setTimeout(() => {
|
||||
const pending = this.pendingApprovals.get(key);
|
||||
if (pending) {
|
||||
logger.warn(
|
||||
`Plan approval for feature ${featureId} timed out after ${timeoutMinutes} minutes`
|
||||
);
|
||||
this.pendingApprovals.delete(key);
|
||||
reject(
|
||||
new Error(
|
||||
`Plan approval timed out after ${timeoutMinutes} minutes - feature execution cancelled`
|
||||
)
|
||||
);
|
||||
}
|
||||
}, timeoutMs);
|
||||
|
||||
// Wrap resolve/reject to clear timeout when approval is resolved
|
||||
// This ensures timeout is ALWAYS cleared on any resolution path
|
||||
// Define wrappers BEFORE setTimeout so they can be used in timeout callback
|
||||
let timeoutId: NodeJS.Timeout;
|
||||
const wrappedResolve = (result: PlanApprovalResult) => {
|
||||
clearTimeout(timeoutId);
|
||||
resolve(result);
|
||||
@@ -119,6 +104,23 @@ export class PlanApprovalService {
|
||||
reject(error);
|
||||
};
|
||||
|
||||
// Set up timeout to prevent indefinite waiting and memory leaks
|
||||
// Now timeoutId assignment happens after wrappers are defined
|
||||
timeoutId = setTimeout(() => {
|
||||
const pending = this.pendingApprovals.get(key);
|
||||
if (pending) {
|
||||
logger.warn(
|
||||
`Plan approval for feature ${featureId} timed out after ${timeoutMinutes} minutes`
|
||||
);
|
||||
this.pendingApprovals.delete(key);
|
||||
wrappedReject(
|
||||
new Error(
|
||||
`Plan approval timed out after ${timeoutMinutes} minutes - feature execution cancelled`
|
||||
)
|
||||
);
|
||||
}
|
||||
}, timeoutMs);
|
||||
|
||||
this.pendingApprovals.set(key, {
|
||||
resolve: wrappedResolve,
|
||||
reject: wrappedReject,
|
||||
|
||||
@@ -325,6 +325,9 @@ export function GraphViewPage() {
|
||||
}
|
||||
} catch (error) {
|
||||
logger.error('Failed to add and start feature:', error);
|
||||
toast.error(
|
||||
`Failed to add and start feature: ${error instanceof Error ? error.message : String(error)}`
|
||||
);
|
||||
}
|
||||
},
|
||||
[handleAddFeature, handleStartImplementation]
|
||||
|
||||
Reference in New Issue
Block a user