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

Improve pull request flow, add branch selection for worktree creation, fix auto-mode concurrency count (#787)

Browse Source 
* Changes from fix/fetch-before-pull-fetch

* feat: Improve pull request flow, add branch selection for worktree creation, fix for automode concurrency count

* feat: Add validation for remote names and improve error handling

* Address PR comments and mobile layout fixes

* ```
refactor: Extract PR target resolution logic into dedicated service
```

* feat: Add app shell UI and improve service imports. Address PR comments

* fix: Improve security validation and cache handling in git operations

* feat: Add GET /list endpoint and improve parameter handling

* chore: Improve validation, accessibility, and error handling across apps

* chore: Format vite server port configuration

* fix: Add error handling for gh pr list command and improve offline fallbacks

* fix: Preserve existing PR creation time and improve remote handling
This commit is contained in:
gsxdsm
2026-02-19 21:55:12 -08:00
committed by GitHub
parent ee52333636
commit 7df2182818
80 changed files with 4729 additions and 1107 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -168,6 +168,20 @@ ${feature.spec}
feature = await this.loadFeatureFn(projectPath, featureId);
if (!feature) throw new Error(`Feature ${featureId} not found`);
// Update status to in_progress immediately after acquiring the feature.
// This prevents a race condition where the UI reloads features and sees the
// feature still in 'backlog' status while it's actually being executed.
// Only do this for the initial call (not internal/recursive calls which would
// redundantly update the status).
if (
!options?._calledInternally &&
(feature.status === 'backlog' ||
feature.status === 'ready' ||
feature.status === 'interrupted')
) {
await this.updateFeatureStatusFn(projectPath, featureId, 'in_progress');
}
if (!options?.continuationPrompt) {
if (feature.planSpec?.status === 'approved') {
const prompts = await getPromptCustomization(this.settingsService, '[ExecutionService]');
@@ -199,7 +213,18 @@ ${feature.spec}
validateWorkingDirectory(workDir);
tempRunningFeature.worktreePath = worktreePath;
tempRunningFeature.branchName = branchName ?? null;
await this.updateFeatureStatusFn(projectPath, featureId, 'in_progress');
// Ensure status is in_progress (may already be set from the early update above,
// but internal/recursive calls skip the early update and need it here).
// Mirror the external guard: only transition when the feature is still in
// backlog, ready, or interrupted to avoid overwriting a concurrent terminal status.
if (
options?._calledInternally &&
(feature.status === 'backlog' ||
feature.status === 'ready' ||
feature.status === 'interrupted')
) {
await this.updateFeatureStatusFn(projectPath, featureId, 'in_progress');
}
this.eventBus.emitAutoModeEvent('auto_mode_feature_start', {
featureId,
projectPath,

32
apps/server/src/services/feature-loader.ts
View File

@@ -225,6 +225,14 @@ export class FeatureLoader {
return null;
}
// Clear transient runtime flag - titleGenerating is only meaningful during
// the current session's async title generation. If it was persisted (e.g.,
// app closed before generation completed), it would cause the UI to show
// "Generating title..." indefinitely.
if (feature.titleGenerating) {
delete feature.titleGenerating;
}
return feature;
});
@@ -323,7 +331,14 @@ export class FeatureLoader {
logRecoveryWarning(result, `Feature ${featureId}`, logger);
return result.data;
const feature = result.data;
// Clear transient runtime flag (same as in getAll)
if (feature?.titleGenerating) {
delete feature.titleGenerating;
}
return feature;
}
/**
@@ -367,8 +382,15 @@ export class FeatureLoader {
descriptionHistory: initialHistory,
};
// Remove transient runtime fields before persisting to disk.
// titleGenerating is UI-only state that tracks in-flight async title generation.
// Persisting it can cause cards to show "Generating title..." indefinitely
// if the app restarts before generation completes.
const featureToWrite = { ...feature };
delete featureToWrite.titleGenerating;
// Write feature.json atomically with backup support
await atomicWriteJson(featureJsonPath, feature, { backupCount: DEFAULT_BACKUP_COUNT });
await atomicWriteJson(featureJsonPath, featureToWrite, { backupCount: DEFAULT_BACKUP_COUNT });
logger.info(`Created feature ${featureId}`);
return feature;
@@ -452,9 +474,13 @@ export class FeatureLoader {
descriptionHistory: updatedHistory,
};
// Remove transient runtime fields before persisting (same as create)
const featureToWrite = { ...updatedFeature };
delete featureToWrite.titleGenerating;
// Write back to file atomically with backup support
const featureJsonPath = this.getFeatureJsonPath(projectPath, featureId);
await atomicWriteJson(featureJsonPath, updatedFeature, { backupCount: DEFAULT_BACKUP_COUNT });
await atomicWriteJson(featureJsonPath, featureToWrite, { backupCount: DEFAULT_BACKUP_COUNT });
logger.info(`Updated feature ${featureId}`);
return updatedFeature;

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

@@ -4,7 +4,7 @@
* Extracted from worktree merge route to allow internal service calls.
*/
import { createLogger, isValidBranchName } from '@automaker/utils';
import { createLogger, isValidBranchName, isValidRemoteName } from '@automaker/utils';
import { type EventEmitter } from '../lib/events.js';
import { execGitCommand } from '@automaker/git-utils';
const logger = createLogger('MergeService');
@@ -13,6 +13,8 @@ export interface MergeOptions {
squash?: boolean;
message?: string;
deleteWorktreeAndBranch?: boolean;
/** Remote name to fetch from before merging (defaults to 'origin') */
remote?: string;
}
export interface MergeServiceResult {
@@ -35,7 +37,11 @@ export interface MergeServiceResult {
* @param branchName - Source branch to merge
* @param worktreePath - Path to the worktree (used for deletion if requested)
* @param targetBranch - Branch to merge into (defaults to 'main')
* @param options - Merge options (squash, message, deleteWorktreeAndBranch)
* @param options - Merge options
* @param options.squash - If true, perform a squash merge
* @param options.message - Custom merge commit message
* @param options.deleteWorktreeAndBranch - If true, delete worktree and branch after merge
* @param options.remote - Remote name to fetch from before merging (defaults to 'origin')
*/
export async function performMerge(
projectPath: string,
@@ -88,6 +94,33 @@ export async function performMerge(
};
}
// Validate the remote name to prevent git option injection.
// Reject invalid remote names so the caller knows their input was wrong,
// consistent with how invalid branch names are handled above.
const remote = options?.remote || 'origin';
if (!isValidRemoteName(remote)) {
logger.warn('Invalid remote name supplied to merge-service', {
remote,
projectPath,
});
return {
success: false,
error: `Invalid remote name: "${remote}"`,
};
}
// Fetch latest from remote before merging to ensure we have up-to-date refs
try {
await execGitCommand(['fetch', remote], projectPath);
} catch (fetchError) {
logger.warn('Failed to fetch from remote before merge; proceeding with local refs', {
remote,
projectPath,
error: (fetchError as Error).message,
});
// Non-fatal: proceed with local refs if fetch fails (e.g. offline)
}
// Emit merge:start after validating inputs
emitter?.emit('merge:start', { branchName, targetBranch: mergeTo, worktreePath });

225
apps/server/src/services/pr-service.ts Normal file
View File

@@ -0,0 +1,225 @@
/**
* Service for resolving PR target information from git remotes.
*
* Extracts remote-parsing and target-resolution logic that was previously
* inline in the create-pr route handler.
*/
// TODO: Move execAsync/execEnv to a shared lib (lib/exec.ts or @automaker/utils) so that
// services no longer depend on route internals. Tracking issue: route-to-service dependency
// inversion. For now, a local thin wrapper is used within the service boundary.
import { exec } from 'child_process';
import { promisify } from 'util';
import { createLogger, isValidRemoteName } from '@automaker/utils';
// Thin local wrapper — duplicates the route-level execAsync/execEnv until a
// shared lib/exec.ts (or @automaker/utils export) is created.
const execAsync = promisify(exec);
const pathSeparator = process.platform === 'win32' ? ';' : ':';
const _additionalPaths: string[] = [];
if (process.platform === 'win32') {
if (process.env.LOCALAPPDATA)
_additionalPaths.push(`${process.env.LOCALAPPDATA}\\Programs\\Git\\cmd`);
if (process.env.PROGRAMFILES) _additionalPaths.push(`${process.env.PROGRAMFILES}\\Git\\cmd`);
if (process.env['ProgramFiles(x86)'])
_additionalPaths.push(`${process.env['ProgramFiles(x86)']}\\Git\\cmd`);
} else {
_additionalPaths.push(
'/opt/homebrew/bin',
'/usr/local/bin',
'/home/linuxbrew/.linuxbrew/bin',
`${process.env.HOME}/.local/bin`
);
}
const execEnv = {
...process.env,
PATH: [process.env.PATH, ..._additionalPaths.filter(Boolean)].filter(Boolean).join(pathSeparator),
};
const logger = createLogger('PRService');
export interface ParsedRemote {
owner: string;
repo: string;
}
export interface PrTargetResult {
repoUrl: string | null;
targetRepo: string | null;
pushOwner: string | null;
upstreamRepo: string | null;
originOwner: string | null;
parsedRemotes: Map<string, ParsedRemote>;
}
/**
* Parse all git remotes for the given repo path and resolve the PR target.
*
* @param worktreePath - Working directory of the repository / worktree
* @param pushRemote - Remote used for pushing (e.g. "origin")
* @param targetRemote - Explicit remote to target the PR against (optional)
*
* @throws {Error} When targetRemote is specified but not found among repository remotes
* @throws {Error} When pushRemote is not found among parsed remotes (when targetRemote is specified)
*/
export async function resolvePrTarget({
worktreePath,
pushRemote,
targetRemote,
}: {
worktreePath: string;
pushRemote: string;
targetRemote?: string;
}): Promise<PrTargetResult> {
// Validate remote names — pushRemote is a required string so the undefined
// guard is unnecessary, but targetRemote is optional.
if (!isValidRemoteName(pushRemote)) {
throw new Error(`Invalid push remote name: "${pushRemote}"`);
}
if (targetRemote !== undefined && !isValidRemoteName(targetRemote)) {
throw new Error(`Invalid target remote name: "${targetRemote}"`);
}
let repoUrl: string | null = null;
let upstreamRepo: string | null = null;
let originOwner: string | null = null;
const parsedRemotes: Map<string, ParsedRemote> = new Map();
try {
const { stdout: remotes } = await execAsync('git remote -v', {
cwd: worktreePath,
env: execEnv,
});
// Parse remotes to detect fork workflow and get repo URL
const lines = remotes.split(/\r?\n/); // Handle both Unix and Windows line endings
for (const line of lines) {
// Try multiple patterns to match different remote URL formats
// Pattern 1: git@github.com:owner/repo.git (fetch)
// Pattern 2: https://github.com/owner/repo.git (fetch)
// Pattern 3: https://github.com/owner/repo (fetch)
let match = line.match(
/^([a-zA-Z0-9._-]+)\s+.*[:/]([^/]+)\/([^/\s]+?)(?:\.git)?\s+\(fetch\)/
);
if (!match) {
// Try SSH format: git@github.com:owner/repo.git
match = line.match(
/^([a-zA-Z0-9._-]+)\s+git@[^:]+:([^/]+)\/([^\s]+?)(?:\.git)?\s+\(fetch\)/
);
}
if (!match) {
// Try HTTPS format: https://github.com/owner/repo.git
match = line.match(
/^([a-zA-Z0-9._-]+)\s+https?:\/\/[^/]+\/([^/]+)\/([^\s]+?)(?:\.git)?\s+\(fetch\)/
);
}
if (match) {
const [, remoteName, owner, repo] = match;
parsedRemotes.set(remoteName, { owner, repo });
if (remoteName === 'upstream') {
upstreamRepo = `${owner}/${repo}`;
repoUrl = `https://github.com/${owner}/${repo}`;
} else if (remoteName === 'origin') {
originOwner = owner;
if (!repoUrl) {
repoUrl = `https://github.com/${owner}/${repo}`;
}
}
}
}
} catch (err) {
// Log the failure for debugging — control flow falls through to auto-detection
logger.debug('Failed to parse git remotes', { worktreePath, error: err });
}
// When targetRemote is explicitly provided but remote parsing failed entirely
// (parsedRemotes is empty), we cannot validate or resolve the requested remote.
// Silently proceeding to auto-detection would ignore the caller's explicit intent,
// so we fail fast with a clear error instead.
if (targetRemote && parsedRemotes.size === 0) {
throw new Error(
`targetRemote "${targetRemote}" was specified but no remotes could be parsed from the repository. ` +
`Ensure the repository has at least one configured remote (parsedRemotes is empty).`
);
}
// When a targetRemote is explicitly specified, validate that it is known
// before using it. Silently falling back to auto-detection when the caller
// explicitly requested a remote that doesn't exist is misleading, so we
// fail fast here instead.
if (targetRemote && parsedRemotes.size > 0 && !parsedRemotes.has(targetRemote)) {
throw new Error(`targetRemote "${targetRemote}" not found in repository remotes`);
}
// When a targetRemote is explicitly specified, override fork detection
// to use the specified remote as the PR target
let targetRepo: string | null = null;
let pushOwner: string | null = null;
if (targetRemote && parsedRemotes.size > 0) {
const targetInfo = parsedRemotes.get(targetRemote);
const pushInfo = parsedRemotes.get(pushRemote);
// If the push remote is not found in the parsed remotes, we cannot
// determine the push owner and would build incorrect URLs. Fail fast
// instead of silently proceeding with null values.
if (!pushInfo) {
logger.warn('Push remote not found in parsed remotes', {
pushRemote,
targetRemote,
availableRemotes: [...parsedRemotes.keys()],
});
throw new Error(`Push remote "${pushRemote}" not found in repository remotes`);
}
if (targetInfo) {
targetRepo = `${targetInfo.owner}/${targetInfo.repo}`;
repoUrl = `https://github.com/${targetInfo.owner}/${targetInfo.repo}`;
}
pushOwner = pushInfo.owner;
// Override the auto-detected upstream/origin with explicit targetRemote
// Only treat as cross-remote if target differs from push remote
if (targetRemote !== pushRemote && targetInfo) {
upstreamRepo = targetRepo;
originOwner = pushOwner;
} else if (targetInfo) {
// Same remote for push and target - regular (non-fork) workflow
upstreamRepo = null;
originOwner = targetInfo.owner;
repoUrl = `https://github.com/${targetInfo.owner}/${targetInfo.repo}`;
}
}
// Fallback: Try to get repo URL from git config if remote parsing failed
if (!repoUrl) {
try {
const { stdout: originUrl } = await execAsync('git config --get remote.origin.url', {
cwd: worktreePath,
env: execEnv,
});
const url = originUrl.trim();
// Parse URL to extract owner/repo
// Handle both SSH (git@github.com:owner/repo.git) and HTTPS (https://github.com/owner/repo.git)
const match = url.match(/[:/]([^/]+)\/([^/\s]+?)(?:\.git)?$/);
if (match) {
const [, owner, repo] = match;
originOwner = owner;
repoUrl = `https://github.com/${owner}/${repo}`;
}
} catch {
// Failed to get repo URL from config
}
}
return {
repoUrl,
targetRepo,
pushOwner,
upstreamRepo,
originOwner,
parsedRemotes,
};
}

41
apps/server/src/services/rebase-service.ts
View File

@@ -7,7 +7,7 @@
import fs from 'fs/promises';
import path from 'path';
import { createLogger, getErrorMessage } from '@automaker/utils';
import { createLogger, getErrorMessage, isValidRemoteName } from '@automaker/utils';
import { execGitCommand, getCurrentBranch, getConflictFiles } from '@automaker/git-utils';
const logger = createLogger('RebaseService');
@@ -16,6 +16,11 @@ const logger = createLogger('RebaseService');
// Types
// ============================================================================
export interface RebaseOptions {
/** Remote name to fetch from before rebasing (defaults to 'origin') */
remote?: string;
}
export interface RebaseResult {
success: boolean;
error?: string;
@@ -36,9 +41,14 @@ export interface RebaseResult {
*
* @param worktreePath - Path to the git worktree
* @param ontoBranch - The branch to rebase onto (e.g., 'origin/main')
* @param options - Optional rebase options (remote name for fetch)
* @returns RebaseResult with success/failure information
*/
export async function runRebase(worktreePath: string, ontoBranch: string): Promise<RebaseResult> {
export async function runRebase(
worktreePath: string,
ontoBranch: string,
options?: RebaseOptions
): Promise<RebaseResult> {
// Reject empty, whitespace-only, or dash-prefixed branch names.
const normalizedOntoBranch = ontoBranch?.trim() ?? '';
if (normalizedOntoBranch === '' || normalizedOntoBranch.startsWith('-')) {
@@ -59,6 +69,33 @@ export async function runRebase(worktreePath: string, ontoBranch: string): Promi
};
}
// Validate the remote name to prevent git option injection.
// Reject invalid remote names so the caller knows their input was wrong,
// consistent with how invalid branch names are handled above.
const remote = options?.remote || 'origin';
if (!isValidRemoteName(remote)) {
logger.warn('Invalid remote name supplied to rebase-service', {
remote,
worktreePath,
});
return {
success: false,
error: `Invalid remote name: "${remote}"`,
};
}
// Fetch latest from remote before rebasing to ensure we have up-to-date refs
try {
await execGitCommand(['fetch', remote], worktreePath);
} catch (fetchError) {
logger.warn('Failed to fetch from remote before rebase; proceeding with local refs', {
remote,
worktreePath,
error: getErrorMessage(fetchError),
});
// Non-fatal: proceed with local refs if fetch fails (e.g. offline)
}
try {
// Pass ontoBranch after '--' so git treats it as a ref, not an option.
// Set LC_ALL=C so git always emits English output regardless of the system