ros/automaker
1
0
Fork 0
mirror of https://github.com/AutoMaker-Org/automaker.git synced 2026-03-17 10:03:08 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Address code review comments

Browse Source
This commit is contained in:
gsxdsm
2026-02-16 11:53:09 -08:00
parent eed5e20438
commit 462dbf1522
12 changed files with 147 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
apps/server/src/routes/auto-mode/routes/approve-plan.ts
View File

@@ -17,7 +17,7 @@ export function createApprovePlanHandler(autoModeService: AutoModeServiceCompat)
approved: boolean;
editedPlan?: string;
feedback?: string;
projectPath?: string;
projectPath: string;
};
if (!featureId) {
@@ -36,6 +36,14 @@ export function createApprovePlanHandler(autoModeService: AutoModeServiceCompat)
return;
}
if (!projectPath) {
res.status(400).json({
success: false,
error: 'projectPath is required',
});
return;
}
// Note: We no longer check hasPendingApproval here because resolvePlanApproval
// can handle recovery when pending approval is not in Map but feature has planSpec.status='generated'
// This supports cases where the server restarted while waiting for approval
@@ -48,7 +56,7 @@ export function createApprovePlanHandler(autoModeService: AutoModeServiceCompat)
// Resolve the pending approval (with recovery support)
const result = await autoModeService.resolvePlanApproval(
projectPath || '',
projectPath,
featureId,
approved,
editedPlan,

23
apps/server/src/routes/features/routes/list.ts
View File

@@ -33,18 +33,23 @@ export function createListHandler(
// We don't await this to keep the list response fast
// Note: detectOrphanedFeatures handles errors internally and always resolves
if (autoModeService) {
autoModeService.detectOrphanedFeatures(projectPath).then((orphanedFeatures) => {
if (orphanedFeatures.length > 0) {
logger.info(
`[ProjectLoad] Detected ${orphanedFeatures.length} orphaned feature(s) in ${projectPath}`
);
for (const { feature, missingBranch } of orphanedFeatures) {
autoModeService
.detectOrphanedFeatures(projectPath)
.then((orphanedFeatures) => {
if (orphanedFeatures.length > 0) {
logger.info(
`[ProjectLoad] Orphaned: ${feature.title || feature.id} - branch "${missingBranch}" no longer exists`
`[ProjectLoad] Detected ${orphanedFeatures.length} orphaned feature(s) in ${projectPath}`
);
for (const { feature, missingBranch } of orphanedFeatures) {
logger.info(
`[ProjectLoad] Orphaned: ${feature.title || feature.id} - branch "${missingBranch}" no longer exists`
);
}
}
}
});
})
.catch((error) => {
logger.warn(`[ProjectLoad] Orphan detection failed for ${projectPath}:`, error);
});
}
res.json({ success: true, features });

5
apps/server/src/routes/github/routes/validate-issue.ts
View File

@@ -25,7 +25,7 @@ import {
isOpencodeModel,
supportsStructuredOutput,
} from '@automaker/types';
import { resolvePhaseModel } from '@automaker/model-resolver';
import { resolvePhaseModel, resolveModelString } from '@automaker/model-resolver';
import { extractJson } from '../../../lib/json-extractor.js';
import { writeValidation } from '../../../lib/validation-storage.js';
import { streamingQuery } from '../../../providers/simple-query-service.js';
@@ -190,9 +190,10 @@ ${basePrompt}`;
// CRITICAL: For custom providers (GLM, MiniMax), pass the provider's model ID (e.g. "GLM-4.7")
// to the API, NOT the resolved Claude model - otherwise we get "model not found"
// For standard Claude models, resolve aliases (e.g., 'opus' -> 'claude-opus-4-20250514')
const effectiveModel = claudeCompatibleProvider
? (model as string)
: providerResolvedModel || (model as string);
: providerResolvedModel || resolveModelString(model as string);
logger.info(`Using model: ${effectiveModel}`);
// Use streamingQuery with event callbacks

2
apps/server/src/services/agent-executor.ts
View File

@@ -560,6 +560,7 @@ export class AgentExecutor {
revText += b.text || '';
this.eventBus.emitAutoModeEvent('auto_mode_progress', {
featureId,
branchName,
content: b.text,
});
}
@@ -638,6 +639,7 @@ export class AgentExecutor {
cwd: o.workDir,
allowedTools: o.sdkOptions?.allowedTools as string[] | undefined,
abortController: o.abortController,
thinkingLevel: o.thinkingLevel,
mcpServers:
o.mcpServers && Object.keys(o.mcpServers).length > 0
? (o.mcpServers as Record<string, { command: string }>)

20
apps/server/src/services/auto-loop-coordinator.ts
View File

@@ -31,8 +31,16 @@ export interface ProjectAutoLoopState {
branchName: string | null;
}
/**
* Generate a unique key for a worktree auto-loop instance.
*
* When branchName is null, this represents the main worktree (uses '__main__' sentinel).
* Named branches always use their exact name — the caller is responsible for passing
* null for the primary branch (main/master/etc.) so key matching stays consistent
* with ConcurrencyManager's dynamic primary branch resolution.
*/
export function getWorktreeAutoLoopKey(projectPath: string, branchName: string | null): string {
return `${projectPath}::${(branchName === 'main' ? null : branchName) ?? '__main__'}`;
return `${projectPath}::${branchName ?? '__main__'}`;
}
export type ExecuteFeatureFn = (
@@ -404,11 +412,15 @@ export class AutoLoopCoordinator {
reject(new Error('Aborted'));
return;
}
const timeout = setTimeout(resolve, ms);
signal?.addEventListener('abort', () => {
const onAbort = () => {
clearTimeout(timeout);
reject(new Error('Aborted'));
});
};
const timeout = setTimeout(() => {
signal?.removeEventListener('abort', onAbort);
resolve();
}, ms);
signal?.addEventListener('abort', onAbort);
});
}
}

67
apps/server/src/services/auto-mode/facade.ts
View File

@@ -17,7 +17,7 @@ import { promisify } from 'util';
import type { Feature, PlanningMode, ThinkingLevel } from '@automaker/types';
import { DEFAULT_MAX_CONCURRENCY, stripProviderPrefix } from '@automaker/types';
import { createLogger, loadContextFiles, classifyError } from '@automaker/utils';
import { getFeatureDir } from '@automaker/platform';
import { getFeatureDir, spawnProcess } from '@automaker/platform';
import * as secureFs from '../../lib/secure-fs.js';
import { validateWorkingDirectory } from '../../lib/sdk-options.js';
import { getPromptCustomization, getProviderByModelId } from '../../lib/settings-helpers.js';
@@ -48,6 +48,24 @@ import type {
const execAsync = promisify(exec);
const logger = createLogger('AutoModeServiceFacade');
/**
* Execute git command with array arguments to prevent command injection.
*/
async function execGitCommand(args: string[], cwd: string): Promise<string> {
const result = await spawnProcess({
command: 'git',
args,
cwd,
});
if (result.exitCode === 0) {
return result.stdout;
} else {
const errorMessage = result.stderr || `Git command failed with code ${result.exitCode}`;
throw new Error(errorMessage);
}
}
/**
* AutoModeServiceFacade provides a clean interface for auto-mode functionality.
*
@@ -589,19 +607,8 @@ ${prompt}
Address the follow-up instructions above. Review the previous work and make the requested changes or fixes.`;
try {
this.eventBus.emitAutoModeEvent('auto_mode_feature_start', {
featureId,
projectPath: this.projectPath,
branchName: feature?.branchName ?? null,
feature: {
id: featureId,
title: feature?.title || 'Follow-up',
description: feature?.description || 'Following up on feature',
},
});
// NOTE: Facade does not have runAgent - this method requires AutoModeService
// For now, throw to indicate routes should use AutoModeService.followUpFeature
// Do NOT emit start events before throwing to prevent false start events
throw new Error(
'followUpFeature not fully implemented in facade - use AutoModeService.followUpFeature instead'
);
@@ -691,18 +698,22 @@ Address the follow-up instructions above. Review the previous work and make the
// Use project path
}
} else {
const sanitizedFeatureId = featureId.replace(/[^a-zA-Z0-9_-]/g, '-');
const legacyWorktreePath = path.join(this.projectPath, '.worktrees', sanitizedFeatureId);
try {
await secureFs.access(legacyWorktreePath);
workDir = legacyWorktreePath;
} catch {
// Use project path
// Use worktreeResolver instead of manual .worktrees lookup
const feature = await this.featureStateManager.loadFeature(this.projectPath, featureId);
const branchName = feature?.branchName;
if (branchName) {
const resolved = await this.worktreeResolver.findWorktreeForBranch(
this.projectPath,
branchName
);
if (resolved) {
workDir = resolved;
}
}
}
try {
const { stdout: status } = await execAsync('git status --porcelain', { cwd: workDir });
const status = await execGitCommand(['status', '--porcelain'], workDir);
if (!status.trim()) {
return null;
}
@@ -712,9 +723,9 @@ Address the follow-up instructions above. Review the previous work and make the
feature?.description?.split('\n')[0]?.substring(0, 60) || `Feature ${featureId}`;
const commitMessage = `feat: ${title}\n\nImplemented by Automaker auto-mode`;
await execAsync('git add -A', { cwd: workDir });
await execAsync(`git commit -m "${commitMessage.replace(/"/g, '\\"')}"`, { cwd: workDir });
const { stdout: hash } = await execAsync('git rev-parse HEAD', { cwd: workDir });
await execGitCommand(['add', '-A'], workDir);
await execGitCommand(['commit', '-m', commitMessage], workDir);
const hash = await execGitCommand(['rev-parse', 'HEAD'], workDir);
this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', {
featureId,
@@ -975,10 +986,10 @@ Address the follow-up instructions above. Review the previous work and make the
return orphanedFeatures;
}
// Get existing branches
const { stdout } = await execAsync(
'git for-each-ref --format="%(refname:short)" refs/heads/',
{ cwd: this.projectPath }
// Get existing branches (using safe array-based command)
const stdout = await execGitCommand(
['for-each-ref', '--format=%(refname:short)', 'refs/heads/'],
this.projectPath
);
const existingBranches = new Set(
stdout

9
apps/server/src/services/execution-service.ts
View File

@@ -190,9 +190,9 @@ ${feature.spec}
}
}
let worktreePath: string | null = null;
let worktreePath: string | null = providedWorktreePath ?? null;
const branchName = feature.branchName;
if (useWorktrees && branchName) {
if (!worktreePath && useWorktrees && branchName) {
worktreePath = await this.worktreeResolver.findWorktreeForBranch(projectPath, branchName);
if (worktreePath) logger.info(`Using worktree for branch "${branchName}": ${worktreePath}`);
}
@@ -289,6 +289,11 @@ ${feature.spec}
testAttempts: 0,
maxTestAttempts: 5,
});
// Check if pipeline set a terminal status (e.g., merge_conflict) — don't overwrite it
const refreshed = await this.loadFeatureFn(projectPath, featureId);
if (refreshed?.status === 'merge_conflict') {
return;
}
}
const finalStatus = feature.skipTests ? 'waiting_approval' : 'verified';

7
apps/server/src/services/feature-state-manager.ts
View File

@@ -115,6 +115,13 @@ export class FeatureStateManager {
// PERSIST BEFORE EMIT (Pitfall 2)
await atomicWriteJson(featurePath, feature, { backupCount: DEFAULT_BACKUP_COUNT });
// Emit status change event so UI can react without polling
this.emitAutoModeEvent('feature_status_changed', {
featureId,
projectPath,
status,
});
// Create notifications for important status changes
const notificationService = getNotificationService();
if (status === 'waiting_approval') {

44
apps/server/src/services/merge-service.ts
View File

@@ -4,12 +4,8 @@
* Extracted from worktree merge route to allow internal service calls.
*/
import { exec } from 'child_process';
import { promisify } from 'util';
import { createLogger } from '@automaker/utils';
import { spawnProcess } from '@automaker/platform';
const execAsync = promisify(exec);
const logger = createLogger('MergeService');
export interface MergeOptions {
@@ -80,9 +76,23 @@ export async function performMerge(
const mergeTo = targetBranch || 'main';
// Validate source branch exists
// Validate branch names early to reject invalid input before any git operations
if (!isValidBranchName(branchName)) {
return {
success: false,
error: `Invalid source branch name: "${branchName}"`,
};
}
if (!isValidBranchName(mergeTo)) {
return {
success: false,
error: `Invalid target branch name: "${mergeTo}"`,
};
}
// Validate source branch exists (using safe array-based command)
try {
await execAsync(`git rev-parse --verify ${branchName}`, { cwd: projectPath });
await execGitCommand(['rev-parse', '--verify', branchName], projectPath);
} catch {
return {
success: false,
@@ -90,9 +100,9 @@ export async function performMerge(
};
}
// Validate target branch exists
// Validate target branch exists (using safe array-based command)
try {
await execAsync(`git rev-parse --verify ${mergeTo}`, { cwd: projectPath });
await execGitCommand(['rev-parse', '--verify', mergeTo], projectPath);
} catch {
return {
success: false,
@@ -100,13 +110,14 @@ export async function performMerge(
};
}
// Merge the feature branch into the target branch
const mergeCmd = options?.squash
? `git merge --squash ${branchName}`
: `git merge ${branchName} -m "${options?.message || `Merge ${branchName} into ${mergeTo}`}"`;
// Merge the feature branch into the target branch (using safe array-based commands)
const mergeMessage = options?.message || `Merge ${branchName} into ${mergeTo}`;
const mergeArgs = options?.squash
? ['merge', '--squash', branchName]
: ['merge', branchName, '-m', mergeMessage];
try {
await execAsync(mergeCmd, { cwd: projectPath });
await execGitCommand(mergeArgs, projectPath);
} catch (mergeError: unknown) {
// Check if this is a merge conflict
const err = mergeError as { stdout?: string; stderr?: string; message?: string };
@@ -125,11 +136,10 @@ export async function performMerge(
throw mergeError;
}
// If squash merge, need to commit
// If squash merge, need to commit (using safe array-based command)
if (options?.squash) {
await execAsync(`git commit -m "${options?.message || `Merge ${branchName} (squash)`}"`, {
cwd: projectPath,
});
const squashMessage = options?.message || `Merge ${branchName} (squash)`;
await execGitCommand(['commit', '-m', squashMessage], projectPath);
}
// Optionally delete the worktree and branch after merging

3
apps/server/src/services/pipeline-orchestrator.ts
View File

@@ -460,6 +460,7 @@ export class PipelineOrchestrator {
const session = this.testRunnerService.getSession(sessionId);
if (session && session.status !== 'running' && session.status !== 'pending') {
clearInterval(checkInterval);
clearTimeout(timeoutId);
resolve({
status: session.status,
exitCode: session.exitCode,
@@ -469,7 +470,7 @@ export class PipelineOrchestrator {
});
}
}, 1000);
setTimeout(() => {
const timeoutId = setTimeout(() => {
clearInterval(checkInterval);
resolve({ status: 'failed', exitCode: null, duration: 600000 });
}, 600000);

13
apps/server/src/services/plan-approval-service.ts
View File

@@ -83,6 +83,13 @@ export class PlanApprovalService {
);
return new Promise((resolve, reject) => {
// Prevent duplicate registrations for the same key — reject and clean up existing entry
const existing = this.pendingApprovals.get(key);
if (existing) {
existing.reject(new Error('Superseded by a new waitForApproval call'));
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(() => {
@@ -226,11 +233,11 @@ export class PlanApprovalService {
status: approved ? 'approved' : 'rejected',
approvedAt: approved ? new Date().toISOString() : undefined,
reviewedByUser: true,
content: editedPlan, // Update content if user provided an edited version
...(editedPlan !== undefined && { content: editedPlan }), // Only update content if user provided an edited version
});
// If rejected with feedback, emit event so client knows the rejection reason
if (!approved && feedback) {
// If rejected, emit event so client knows the rejection reason (even without feedback)
if (!approved) {
this.eventBus.emitAutoModeEvent('plan_rejected', {
featureId,
projectPath,