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

fix: Address review comments

Browse Source
This commit is contained in:
gsxdsm
2026-02-18 19:52:25 -08:00
parent 4ba0026aa1
commit 4ee160fae4
16 changed files with 184 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
apps/server/src/providers/claude-provider.ts
View File

@@ -350,7 +350,7 @@ export class ClaudeProvider extends BaseProvider {
provider: 'anthropic',
description: 'Balanced performance and cost with enhanced reasoning',
contextWindow: 200000,
maxOutputTokens: 128000,
maxOutputTokens: 64000,
supportsVision: true,
supportsTools: true,
tier: 'standard' as const,

9
apps/server/src/providers/codex-provider.ts
View File

@@ -212,7 +212,6 @@ async function resolveCodexExecutionPlan(options: ExecuteOptions): Promise<Codex
// authIndicators.hasApiKey = API key stored in Codex's own auth file (via `codex login --api-key`)
// Both are "CLI-native" auth — distinct from an API key stored in Automaker's credentials.
const hasCliNativeAuth = authIndicators.hasOAuthToken || authIndicators.hasApiKey;
const cliAuthenticated = hasCliNativeAuth || hasApiKey;
const sdkEligible = isSdkEligible(options);
// If CLI is available and the user authenticated via the CLI (`codex login`),
@@ -767,7 +766,7 @@ export class CodexProvider extends BaseProvider {
codexSettings.enableWebSearch || resolveSearchEnabled(resolvedAllowedTools, restrictTools);
await writeOutputSchemaFile(options.cwd, options.outputFormat);
const imageBlocks = codexSettings.enableImages ? extractImageBlocks(options.prompt) : [];
await writeImageFiles(options.cwd, imageBlocks);
const imagePaths = await writeImageFiles(options.cwd, imageBlocks);
const approvalPolicy =
hasMcpServers && options.mcpAutoApproveTools !== undefined
? options.mcpAutoApproveTools
@@ -810,6 +809,12 @@ export class CodexProvider extends BaseProvider {
}
}
// If images were written to disk, add the image directory so the CLI can access them
if (imagePaths.length > 0) {
const imageDir = path.join(options.cwd, CODEX_INSTRUCTIONS_DIR, IMAGE_TEMP_DIR);
preExecArgs.push(CODEX_ADD_DIR_FLAG, imageDir);
}
// Model is already bare (no prefix) - validated by executeQuery
const args = [
CODEX_EXEC_SUBCOMMAND,

24
apps/server/src/providers/opencode-provider.ts
View File

@@ -464,7 +464,7 @@ export class OpencodeProvider extends CliProvider {
// Without this guard, errors like "ProviderModelNotFoundError" or
// "Resource not found: /path/to/config.json" would false-positive.
if (cleaned.includes('notfounderror') || cleaned.includes('resource not found')) {
return cleaned.includes('/session/') || cleaned.includes('session');
return cleaned.includes('/session/') || /\bsession\b/.test(cleaned);
}
return false;
@@ -720,11 +720,29 @@ export class OpencodeProvider extends CliProvider {
return null;
}
// Final completion (reason: 'stop' or 'end_turn')
// Only treat an explicit allowlist of reasons as true success.
// Reasons like 'length' (context-window truncation) or 'content-filter'
// indicate the model stopped abnormally and must not be surfaced as
// successful completions.
const SUCCESS_REASONS = new Set(['stop', 'end_turn']);
const reason = finishEvent.part?.reason;
if (reason === undefined || SUCCESS_REASONS.has(reason)) {
// Final completion (reason: 'stop', 'end_turn', or unset)
return {
type: 'result',
subtype: 'success',
session_id: finishEvent.sessionID,
result: (finishEvent.part as OpenCodePart & { result?: string })?.result,
};
}
// Non-success, non-tool-calls reason (e.g. 'length', 'content-filter')
return {
type: 'result',
subtype: 'success',
subtype: 'error',
session_id: finishEvent.sessionID,
error: `Step finished with non-success reason: ${reason}`,
result: (finishEvent.part as OpenCodePart & { result?: string })?.result,
};
}

6
apps/server/src/routes/git/index.ts
View File

@@ -13,7 +13,11 @@ export function createGitRoutes(): Router {
router.post('/diffs', validatePathParams('projectPath'), createDiffsHandler());
router.post('/file-diff', validatePathParams('projectPath', 'filePath'), createFileDiffHandler());
router.post('/stage-files', validatePathParams('projectPath'), createStageFilesHandler());
router.post(
'/stage-files',
validatePathParams('projectPath', 'files[]'),
createStageFilesHandler()
);
return router;
}

39
apps/server/src/routes/git/routes/stage-files.ts
View File

@@ -2,6 +2,7 @@
* POST /stage-files endpoint - Stage or unstage files in the main project
*/
import path from 'path';
import type { Request, Response } from 'express';
import { getErrorMessage, logError } from '../common.js';
import { execGitCommand } from '../../../lib/git.js';
@@ -39,17 +40,49 @@ export function createStageFilesHandler() {
return;
}
// Validate and sanitize each file path to prevent path traversal attacks
const base = path.resolve(projectPath) + path.sep;
const sanitizedFiles: string[] = [];
for (const file of files) {
// Reject absolute paths
if (path.isAbsolute(file)) {
res.status(400).json({
success: false,
error: `Invalid file path (absolute paths not allowed): ${file}`,
});
return;
}
// Reject entries containing '..'
if (file.includes('..')) {
res.status(400).json({
success: false,
error: `Invalid file path (path traversal not allowed): ${file}`,
});
return;
}
// Ensure the resolved path stays within the project directory
const resolved = path.resolve(path.join(projectPath, file));
if (resolved !== path.resolve(projectPath) && !resolved.startsWith(base)) {
res.status(400).json({
success: false,
error: `Invalid file path (outside project directory): ${file}`,
});
return;
}
sanitizedFiles.push(file);
}
if (operation === 'stage') {
await execGitCommand(['add', '--', ...files], projectPath);
await execGitCommand(['add', '--', ...sanitizedFiles], projectPath);
} else {
await execGitCommand(['reset', 'HEAD', '--', ...files], projectPath);
await execGitCommand(['reset', 'HEAD', '--', ...sanitizedFiles], projectPath);
}
res.json({
success: true,
result: {
operation,
filesCount: files.length,
filesCount: sanitizedFiles.length,
},
});
} catch (error) {

2
apps/server/src/routes/worktree/index.ts
View File

@@ -298,7 +298,7 @@ export function createWorktreeRoutes(
// Stage/unstage files route
router.post(
'/stage-files',
validatePathParams('worktreePath'),
validatePathParams('worktreePath', 'files[]'),
requireGitRepoOnly,
createStageFilesHandler()
);

17
apps/server/src/routes/worktree/routes/list.ts
View File

@@ -58,9 +58,9 @@ interface WorktreeInfo {
hasChanges?: boolean;
changedFilesCount?: number;
pr?: WorktreePRInfo; // PR info if a PR has been created for this branch
/** Whether a merge or rebase is in progress (has conflicts) */
/** Whether there are actual unresolved conflict files (conflictFiles.length > 0) */
hasConflicts?: boolean;
/** Type of conflict operation in progress */
/** Type of git operation in progress (merge/rebase/cherry-pick), set independently of hasConflicts */
conflictType?: 'merge' | 'rebase' | 'cherry-pick';
/** List of files with conflicts */
conflictFiles?: string[];
@@ -80,6 +80,7 @@ async function detectConflictState(worktreePath: string): Promise<{
// Find the canonical .git directory for this worktree
const { stdout: gitDirRaw } = await execAsync('git rev-parse --git-dir', {
cwd: worktreePath,
timeout: 15000,
});
const gitDir = path.resolve(worktreePath, gitDirRaw.trim());
@@ -122,6 +123,7 @@ async function detectConflictState(worktreePath: string): Promise<{
try {
const { stdout: statusOutput } = await execAsync('git diff --name-only --diff-filter=U', {
cwd: worktreePath,
timeout: 15000,
});
conflictFiles = statusOutput
.trim()
@@ -132,7 +134,7 @@ async function detectConflictState(worktreePath: string): Promise<{
}
return {
hasConflicts: true,
hasConflicts: conflictFiles.length > 0,
conflictType,
conflictFiles,
};
@@ -476,11 +478,14 @@ export function createListHandler() {
// Detect merge/rebase/cherry-pick in progress
try {
const conflictState = await detectConflictState(worktree.path);
if (conflictState.hasConflicts) {
worktree.hasConflicts = true;
// Always propagate conflictType so callers know an operation is in progress,
// even when there are no unresolved conflict files yet.
if (conflictState.conflictType) {
worktree.conflictType = conflictState.conflictType;
worktree.conflictFiles = conflictState.conflictFiles;
}
// hasConflicts is true only when there are actual unresolved files
worktree.hasConflicts = conflictState.hasConflicts;
worktree.conflictFiles = conflictState.conflictFiles;
} catch {
// Ignore conflict detection errors
}

59
apps/server/src/routes/worktree/routes/stage-files.ts
View File

@@ -9,6 +9,8 @@
* the requireGitRepoOnly middleware in index.ts
*/
import path from 'path';
import fs from 'fs/promises';
import type { Request, Response } from 'express';
import { getErrorMessage, logError } from '../common.js';
import { execGitCommand } from '../../../lib/git.js';
@@ -46,19 +48,70 @@ export function createStageFilesHandler() {
return;
}
// Canonicalize the worktree root by resolving symlinks so that
// path-traversal checks are reliable even when symlinks are involved.
let canonicalRoot: string;
try {
canonicalRoot = await fs.realpath(worktreePath);
} catch {
res.status(400).json({
success: false,
error: 'worktreePath does not exist or is not accessible',
});
return;
}
// Validate and sanitize each file path to prevent path traversal attacks.
// Each file entry is resolved against the canonicalized worktree root and
// must remain within that root directory.
const base = canonicalRoot + path.sep;
const sanitizedFiles: string[] = [];
for (const file of files) {
// Reject absolute paths
if (path.isAbsolute(file)) {
res.status(400).json({
success: false,
error: `Invalid file path (absolute paths not allowed): ${file}`,
});
return;
}
// Reject entries containing '..'
if (file.includes('..')) {
res.status(400).json({
success: false,
error: `Invalid file path (path traversal not allowed): ${file}`,
});
return;
}
// Resolve the file path against the canonicalized worktree root and
// ensure the result stays within the worktree directory.
const resolved = path.resolve(canonicalRoot, file);
if (resolved !== canonicalRoot && !resolved.startsWith(base)) {
res.status(400).json({
success: false,
error: `Invalid file path (outside worktree directory): ${file}`,
});
return;
}
// Forward only the original relative path to git — git interprets
// paths relative to its working directory (canonicalRoot / worktreePath),
// so we do not need to pass the resolved absolute path.
sanitizedFiles.push(file);
}
if (operation === 'stage') {
// Stage the specified files
await execGitCommand(['add', '--', ...files], worktreePath);
await execGitCommand(['add', '--', ...sanitizedFiles], worktreePath);
} else {
// Unstage the specified files
await execGitCommand(['reset', 'HEAD', '--', ...files], worktreePath);
await execGitCommand(['reset', 'HEAD', '--', ...sanitizedFiles], worktreePath);
}
res.json({
success: true,
result: {
operation,
filesCount: files.length,
filesCount: sanitizedFiles.length,
},
});
} catch (error) {

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

@@ -16,6 +16,7 @@ import { exec } from 'child_process';
import { promisify } from 'util';
import type { Feature, PlanningMode, ThinkingLevel } from '@automaker/types';
import { DEFAULT_MAX_CONCURRENCY, stripProviderPrefix } from '@automaker/types';
import { resolveModelString } from '@automaker/model-resolver';
import { createLogger, loadContextFiles, classifyError } from '@automaker/utils';
import { getFeatureDir } from '@automaker/platform';
import * as secureFs from '../../lib/secure-fs.js';
@@ -208,7 +209,7 @@ export class AutoModeServiceFacade {
model?: string,
opts?: Record<string, unknown>
) => {
const resolvedModel = model || 'claude-sonnet-4-6';
const resolvedModel = resolveModelString(model, 'claude-sonnet-4-6');
const provider = ProviderFactory.getProviderForModel(resolvedModel);
const effectiveBareModel = stripProviderPrefix(resolvedModel);
@@ -336,7 +337,7 @@ export class AutoModeServiceFacade {
branchName?: string | null;
}
) => {
const resolvedModel = model || 'claude-sonnet-4-6';
const resolvedModel = resolveModelString(model, 'claude-sonnet-4-6');
const provider = ProviderFactory.getProviderForModel(resolvedModel);
const effectiveBareModel = stripProviderPrefix(resolvedModel);

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

@@ -160,7 +160,18 @@ export async function performMerge(
// If squash merge, need to commit (using safe array-based command)
if (options?.squash) {
const squashMessage = options?.message || `Merge ${branchName} (squash)`;
await execGitCommand(['commit', '-m', squashMessage], projectPath);
try {
await execGitCommand(['commit', '-m', squashMessage], projectPath);
} catch (commitError: unknown) {
const err = commitError as { message?: string };
// Emit merge:error so subscribers always receive either merge:success or merge:error
emitter?.emit('merge:error', {
branchName,
targetBranch: mergeTo,
error: err.message || String(commitError),
});
throw commitError;
}
}
// Optionally delete the worktree and branch after merging

3
apps/server/src/services/pull-service.ts
View File

@@ -15,9 +15,8 @@
* and cherry-pick-service.ts.
*/
import { createLogger } from '@automaker/utils';
import { createLogger, getErrorMessage } from '@automaker/utils';
import { execGitCommand, execGitCommandWithLockRetry } from '../lib/git.js';
import { getErrorMessage } from '../routes/worktree/common.js';
const logger = createLogger('PullService');

5
apps/server/src/services/stash-service.ts
View File

@@ -14,10 +14,9 @@
* merge-service.ts.
*/
import { createLogger } from '@automaker/utils';
import { createLogger, getErrorMessage } from '@automaker/utils';
import type { EventEmitter } from '../lib/events.js';
import { execGitCommand, execGitCommandWithLockRetry } from '../lib/git.js';
import { getErrorMessage, logError } from '../routes/worktree/common.js';
const logger = createLogger('StashService');
@@ -242,7 +241,7 @@ export async function applyOrPop(
} catch (error) {
const errorMessage = getErrorMessage(error);
logError(error, `Stash ${operation} failed`);
logger.error(`Stash ${operation} failed`, { error: getErrorMessage(error) });
events?.emit('stash:failure', {
worktreePath,

8
apps/server/src/services/worktree-branch-service.ts
View File

@@ -144,11 +144,11 @@ async function fetchRemotes(cwd: string): Promise<void> {
* Parse a remote branch name like "origin/feature-branch" into its parts
*/
function parseRemoteBranch(branchName: string): { remote: string; branch: string } | null {
const slashIndex = branchName.indexOf('/');
if (slashIndex === -1) return null;
const lastSlash = branchName.lastIndexOf('/');
if (lastSlash === -1) return null;
return {
remote: branchName.substring(0, slashIndex),
branch: branchName.substring(slashIndex + 1),
remote: branchName.substring(0, lastSlash),
branch: branchName.substring(lastSlash + 1),
};
}

16
apps/ui/src/components/ui/git-diff-panel.tsx
View File

@@ -528,6 +528,12 @@ export function GitDiffPanel({
const handleStageFile = useCallback(
async (filePath: string) => {
if (!worktreePath && !projectPath) return;
if (enableStaging && useWorktrees && !worktreePath) {
toast.error('Failed to stage file', {
description: 'worktreePath required when useWorktrees is enabled',
});
return;
}
setStagingInProgress((prev) => new Set(prev).add(filePath));
try {
const api = getElectronAPI();
@@ -574,13 +580,19 @@ export function GitDiffPanel({
});
}
},
[worktreePath, projectPath, useWorktrees, loadDiffs]
[worktreePath, projectPath, useWorktrees, enableStaging, loadDiffs]
);
// Unstage a single file
const handleUnstageFile = useCallback(
async (filePath: string) => {
if (!worktreePath && !projectPath) return;
if (enableStaging && useWorktrees && !worktreePath) {
toast.error('Failed to unstage file', {
description: 'worktreePath required when useWorktrees is enabled',
});
return;
}
setStagingInProgress((prev) => new Set(prev).add(filePath));
try {
const api = getElectronAPI();
@@ -627,7 +639,7 @@ export function GitDiffPanel({
});
}
},
[worktreePath, projectPath, useWorktrees, loadDiffs]
[worktreePath, projectPath, useWorktrees, enableStaging, loadDiffs]
);
const handleStageAll = useCallback(async () => {

9
apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx
View File

@@ -226,6 +226,13 @@ export function CommitWorktreeDialog({
setSelectedFiles(new Set(fileList.map((f) => f.path)));
}
}
} else {
const errorMsg = result.error ?? 'Failed to load diffs';
console.warn('Failed to load diffs for commit dialog:', errorMsg);
if (!cancelled) {
setError(errorMsg);
toast.error(errorMsg);
}
}
}
} catch (err) {
@@ -480,7 +487,7 @@ export function CommitWorktreeDialog({
>
{getStatusLabel(file.status)}
</span>
{isStaged && !isUntracked && (
{isStaged && !isUnstaged && !isUntracked && (
<span className="text-[10px] px-1 py-0.5 rounded border font-medium flex-shrink-0 bg-green-500/15 text-green-400 border-green-500/30">
Staged
</span>

2
apps/ui/src/components/views/board-view/dialogs/discard-worktree-changes-dialog.tsx
View File

@@ -214,6 +214,8 @@ export function DiscardWorktreeChangesDialog({
} else {
if (!cancelled) setError(result.error || 'Failed to fetch diffs');
}
} else {
if (!cancelled) setError('Diff API unavailable');
}
} catch (err) {
if (cancelled) return;