Skip to content

Commit 0196911

Browse files
gsxdsmCopilotDhanushSantoshclaudecoderabbitai[bot]
authored
Bug fixes and stability improvements (#815)
* fix(copilot): correct tool.execution_complete event handling The CopilotProvider was using incorrect event type and data structure for tool execution completion events from the @github/copilot-sdk, causing tool call outputs to be empty. Changes: - Update event type from 'tool.execution_end' to 'tool.execution_complete' - Fix data structure to use nested result.content instead of flat result - Fix error structure to use error.message instead of flat error - Add success field to match SDK event structure - Add tests for empty and missing result handling This aligns with the official @github/copilot-sdk v0.1.16 types defined in session-events.d.ts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(copilot): add edge case test for error with code field Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor(copilot): improve error handling and code quality Code review improvements: - Extract magic string '[ERROR]' to TOOL_ERROR_PREFIX constant - Add null-safe error handling with direct error variable assignment - Include error codes in error messages for better debugging - Add JSDoc documentation for tool.execution_complete handler - Update tests to verify error codes are displayed - Add missing tool_use_id assertion in error test These changes improve: - Code maintainability (no magic strings) - Debugging experience (error codes now visible) - Type safety (explicit null checks) - Test coverage (verify error code formatting) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Changes from fix/bug-fixes-1-0 * test(copilot): add edge case test for error with code field Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Changes from fix/bug-fixes-1-0 * fix: Handle detached HEAD state in worktree discovery and recovery * fix: Remove unused isDevServerStarting prop and md: breakpoint classes * fix: Add missing dependency and sanitize persisted cache data * feat: Ensure NODE_ENV is set to test in vitest configs * feat: Configure Playwright to run only E2E tests * fix: Improve PR tracking and dev server lifecycle management * feat: Add settings-based defaults for planning mode, model config, and custom providers. Fixes #816 * feat: Add worktree and branch selector to graph view * fix: Add timeout and error handling for worktree HEAD ref resolution * fix: use absolute icon path and place icon outside asar on Linux The hicolor icon theme index only lists sizes up to 512x512, so an icon installed only at 1024x1024 is invisible to GNOME/KDE's theme resolver, causing both the app launcher and taskbar to show a generic icon. Additionally, BrowserWindow.icon cannot be read by the window manager when the file is inside app.asar. - extraResources: copy logo_larger.png to resources/ (outside asar) so it lands at /opt/Automaker/resources/logo_larger.png on install - linux.desktop.Icon: set to the absolute resources path, bypassing the hicolor theme lookup and its size constraints entirely - icon-manager.ts: on Linux production use process.resourcesPath so BrowserWindow receives a real filesystem path the WM can read directly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: use linux.desktop.entry for custom desktop Icon field electron-builder v26 rejects arbitrary keys in linux.desktop — the correct schema wraps custom .desktop overrides inside desktop.entry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: set desktop name on Linux so taskbar uses the correct app icon Without app.setDesktopName(), the window manager cannot associate the running Electron process with automaker.desktop. GNOME/KDE fall back to _NET_WM_ICON which defaults to Electron's own bundled icon. Calling app.setDesktopName('automaker.desktop') before any window is created sets the _GTK_APPLICATION_ID hint and XDG app_id so the WM picks up the desktop entry's Icon for the taskbar. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix: memory and context views mobile friendly (#818) * Changes from fix/memory-and-context-mobile-friendly * fix: Improve file extension detection and add path traversal protection * refactor: Extract file extension utilities and add path traversal guards Code review improvements: - Extract isMarkdownFilename and isImageFilename to shared image-utils.ts - Remove duplicated code from context-view.tsx and memory-view.tsx - Add path traversal guard for context fixture utilities (matching memory) - Add 7 new tests for context fixture path traversal protection - Total 61 tests pass Addresses code review feedback from PR #813 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: Add e2e tests for profiles crud and board background persistence * Update apps/ui/playwright.config.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: Add robust test navigation handling and file filtering * fix: Format NODE_OPTIONS configuration on single line * test: Update profiles and board background persistence tests * test: Replace iPhone 13 Pro with Pixel 5 for mobile test consistency * Update apps/ui/src/components/views/context-view.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: Remove test project directory * feat: Filter context files by type and improve mobile menu visibility --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: Improve test reliability and localhost handling * chore: Use explicit TEST_USE_EXTERNAL_BACKEND env var for server cleanup * feat: Add E2E/CI mock mode for provider factory and auth verification * feat: Add remoteBranch parameter to pull and rebase operations * chore: Enhance E2E testing setup with worker isolation and auth state management - Updated .gitignore to include worker-specific test fixtures. - Modified e2e-tests.yml to implement test sharding for improved CI performance. - Refactored global setup to authenticate once and save session state for reuse across tests. - Introduced worker-isolated fixture paths to prevent conflicts during parallel test execution. - Improved test navigation and loading handling for better reliability. - Updated various test files to utilize new auth state management and fixture paths. * fix: Update Playwright configuration and improve test reliability - Increased the number of workers in Playwright configuration for better parallelism in CI environments. - Enhanced the board background persistence test to ensure dropdown stability by waiting for the list to populate before interaction, improving test reliability. * chore: Simplify E2E test configuration and enhance mock implementations - Updated e2e-tests.yml to run tests in a single shard for streamlined CI execution. - Enhanced unit tests for worktree list handling by introducing a mock for execGitCommand, improving test reliability and coverage. - Refactored setup functions to better manage command mocks for git operations in tests. - Improved error handling in mkdirSafe function to account for undefined stats in certain environments. * refactor: Improve test configurations and enhance error handling - Updated Playwright configuration to clear VITE_SERVER_URL, ensuring the frontend uses the Vite proxy and preventing cookie domain mismatches. - Enhanced MergeRebaseDialog logic to normalize selectedBranch for better handling of various ref formats. - Improved global setup with a more robust backend health check, throwing an error if the backend is not healthy after retries. - Refactored project creation tests to handle file existence checks more reliably. - Added error handling for missing E2E source fixtures to guide setup process. - Enhanced memory navigation to handle sandbox dialog visibility more effectively. * refactor: Enhance Git command execution and improve test configurations - Updated Git command execution to merge environment paths correctly, ensuring proper command execution context. - Refactored the Git initialization process to handle errors more gracefully and ensure user configuration is set before creating the initial commit. - Improved test configurations by updating Playwright test identifiers for better clarity and consistency across different project states. - Enhanced cleanup functions in tests to handle directory removal more robustly, preventing errors during test execution. * fix: Resolve React hooks errors from duplicate instances in dependency tree * style: Format alias configuration for improved readability --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: DhanushSantosh <dhanushsantoshs05@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 parent 70d4007 commit 0196911

234 files changed

Lines changed: 15864 additions & 2899 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.github/workflows/e2e-tests.yml

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,13 @@ jobs:
1313
e2e:
1414
runs-on: ubuntu-latest
1515
timeout-minutes: 15
16+
strategy:
17+
fail-fast: false
18+
matrix:
19+
# shardIndex: [1, 2, 3]
20+
# shardTotal: [3]
21+
shardIndex: [1]
22+
shardTotal: [1]
1623

1724
steps:
1825
- name: Checkout code
@@ -91,15 +98,15 @@ jobs:
9198
curl -s http://localhost:3108/api/health | jq . 2>/dev/null || echo "Health check: $(curl -s http://localhost:3108/api/health 2>/dev/null || echo 'No response')"
9299
exit 0
93100
fi
94-
101+
95102
# Check if server process is still running
96103
if ! kill -0 $SERVER_PID 2>/dev/null; then
97104
echo "ERROR: Server process died during wait!"
98105
echo "=== Backend logs ==="
99106
cat backend.log
100107
exit 1
101108
fi
102-
109+
103110
echo "Waiting... ($i/60)"
104111
sleep 1
105112
done
@@ -127,17 +134,23 @@ jobs:
127134
128135
exit 1
129136
130-
- name: Run E2E tests
137+
- name: Run E2E tests (shard ${{ matrix.shardIndex }}/${{ matrix.shardTotal }})
131138
# Playwright automatically starts the Vite frontend via webServer config
132139
# (see apps/ui/playwright.config.ts) - no need to start it manually
133-
run: npm run test --workspace=apps/ui
140+
run: npx playwright test --shard=${{ matrix.shardIndex }}/${{ matrix.shardTotal }}
141+
working-directory: apps/ui
134142
env:
135143
CI: true
136-
VITE_SERVER_URL: http://localhost:3108
137-
SERVER_URL: http://localhost:3108
138144
VITE_SKIP_SETUP: 'true'
139145
# Keep UI-side login/defaults consistent
140146
AUTOMAKER_API_KEY: test-api-key-for-e2e-tests
147+
# Backend is already started above - Playwright config sets
148+
# AUTOMAKER_SERVER_PORT so the Vite proxy forwards /api/* to the backend.
149+
# Do NOT set VITE_SERVER_URL here: it bypasses the Vite proxy and causes
150+
# a cookie domain mismatch (cookies are bound to 127.0.0.1, but
151+
# VITE_SERVER_URL=http://localhost:3108 makes the frontend call localhost).
152+
TEST_USE_EXTERNAL_BACKEND: 'true'
153+
TEST_SERVER_PORT: 3108
141154

142155
- name: Print backend logs on failure
143156
if: failure()
@@ -155,20 +168,29 @@ jobs:
155168
uses: actions/upload-artifact@v4
156169
if: always()
157170
with:
158-
name: playwright-report
171+
name: playwright-report-shard-${{ matrix.shardIndex }}-of-${{ matrix.shardTotal }}
159172
path: apps/ui/playwright-report/
160173
retention-days: 7
161174

162175
- name: Upload test results (screenshots, traces, videos)
163176
uses: actions/upload-artifact@v4
164177
if: always()
165178
with:
166-
name: test-results
179+
name: test-results-shard-${{ matrix.shardIndex }}-of-${{ matrix.shardTotal }}
167180
path: |
168181
apps/ui/test-results/
169182
retention-days: 7
170183
if-no-files-found: ignore
171184

185+
- name: Upload blob report for merging
186+
uses: actions/upload-artifact@v4
187+
if: always()
188+
with:
189+
name: blob-report-shard-${{ matrix.shardIndex }}-of-${{ matrix.shardTotal }}
190+
path: apps/ui/blob-report/
191+
retention-days: 1
192+
if-no-files-found: ignore
193+
172194
- name: Cleanup - Kill backend server
173195
if: always()
174196
run: |

.gitignore

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,11 @@ test/opus-thinking-*/
7070
test/agent-session-test-*/
7171
test/feature-backlog-test-*/
7272
test/running-task-display-test-*/
73+
test/agent-output-modal-responsive-*/
74+
test/fixtures/.worker-*/
75+
test/board-bg-test-*/
76+
test/edit-feature-test-*/
77+
test/open-project-test-*/
7378

7479
# Environment files (keep .example)
7580
.env

apps/server/src/index.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -349,7 +349,9 @@ const ideationService = new IdeationService(events, settingsService, featureLoad
349349

350350
// Initialize DevServerService with event emitter for real-time log streaming
351351
const devServerService = getDevServerService();
352-
devServerService.setEventEmitter(events);
352+
devServerService.initialize(DATA_DIR, events).catch((err) => {
353+
logger.error('Failed to initialize DevServerService:', err);
354+
});
353355

354356
// Initialize Notification Service with event emitter for real-time updates
355357
const notificationService = getNotificationService();

apps/server/src/lib/git.ts

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,27 @@ import { createLogger } from '@automaker/utils';
1313

1414
const logger = createLogger('GitLib');
1515

16+
// Extended PATH so git is found when the process does not inherit a full shell PATH
17+
// (e.g. Electron, some CI, or IDE-launched processes).
18+
const pathSeparator = process.platform === 'win32' ? ';' : ':';
19+
const extraPaths: string[] =
20+
process.platform === 'win32'
21+
? ([
22+
process.env.LOCALAPPDATA && `${process.env.LOCALAPPDATA}\\Programs\\Git\\cmd`,
23+
process.env.PROGRAMFILES && `${process.env.PROGRAMFILES}\\Git\\cmd`,
24+
process.env['ProgramFiles(x86)'] && `${process.env['ProgramFiles(x86)']}\\Git\\cmd`,
25+
].filter(Boolean) as string[])
26+
: [
27+
'/opt/homebrew/bin',
28+
'/usr/local/bin',
29+
'/usr/bin',
30+
'/home/linuxbrew/.linuxbrew/bin',
31+
process.env.HOME ? `${process.env.HOME}/.local/bin` : '',
32+
].filter(Boolean);
33+
34+
const extendedPath = [process.env.PATH, ...extraPaths].filter(Boolean).join(pathSeparator);
35+
const gitEnv = { ...process.env, PATH: extendedPath };
36+
1637
// ============================================================================
1738
// Secure Command Execution
1839
// ============================================================================
@@ -65,7 +86,14 @@ export async function execGitCommand(
6586
command: 'git',
6687
args,
6788
cwd,
68-
...(env !== undefined ? { env } : {}),
89+
env:
90+
env !== undefined
91+
? {
92+
...gitEnv,
93+
...env,
94+
PATH: [gitEnv.PATH, env.PATH].filter(Boolean).join(pathSeparator),
95+
}
96+
: gitEnv,
6997
...(abortController !== undefined ? { abortController } : {}),
7098
});
7199

apps/server/src/lib/settings-helpers.ts

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -689,6 +689,145 @@ export interface ProviderByModelIdResult {
689689
resolvedModel: string | undefined;
690690
}
691691

692+
/** Result from resolveProviderContext */
693+
export interface ProviderContextResult {
694+
/** The provider configuration */
695+
provider: ClaudeCompatibleProvider | undefined;
696+
/** Credentials for API key resolution */
697+
credentials: Credentials | undefined;
698+
/** The resolved Claude model ID for SDK configuration */
699+
resolvedModel: string | undefined;
700+
/** The original model config from the provider if found */
701+
modelConfig: import('@automaker/types').ProviderModel | undefined;
702+
}
703+
704+
/**
705+
* Checks if a provider is enabled.
706+
* Providers with enabled: undefined are treated as enabled (default state).
707+
* Only explicitly set enabled: false means the provider is disabled.
708+
*/
709+
function isProviderEnabled(provider: ClaudeCompatibleProvider): boolean {
710+
return provider.enabled !== false;
711+
}
712+
713+
/**
714+
* Finds a model config in a provider's models array by ID (case-insensitive).
715+
*/
716+
function findModelInProvider(
717+
provider: ClaudeCompatibleProvider,
718+
modelId: string
719+
): import('@automaker/types').ProviderModel | undefined {
720+
return provider.models?.find(
721+
(m) => m.id === modelId || m.id.toLowerCase() === modelId.toLowerCase()
722+
);
723+
}
724+
725+
/**
726+
* Resolves the provider and Claude-compatible model configuration.
727+
*
728+
* This is the central logic for resolving provider context, supporting:
729+
* 1. Explicit lookup by providerId (most reliable for persistence)
730+
* 2. Fallback lookup by modelId across all enabled providers
731+
* 3. Resolution of mapsToClaudeModel for SDK configuration
732+
*
733+
* @param settingsService - Settings service instance
734+
* @param modelId - The model ID to resolve
735+
* @param providerId - Optional explicit provider ID
736+
* @param logPrefix - Prefix for log messages
737+
* @returns Promise resolving to the provider context
738+
*/
739+
export async function resolveProviderContext(
740+
settingsService: SettingsService,
741+
modelId: string,
742+
providerId?: string,
743+
logPrefix = '[SettingsHelper]'
744+
): Promise<ProviderContextResult> {
745+
try {
746+
const globalSettings = await settingsService.getGlobalSettings();
747+
const credentials = await settingsService.getCredentials();
748+
const providers = globalSettings.claudeCompatibleProviders || [];
749+
750+
logger.debug(
751+
`${logPrefix} Resolving provider context: modelId="${modelId}", providerId="${providerId ?? 'none'}", providers count=${providers.length}`
752+
);
753+
754+
let provider: ClaudeCompatibleProvider | undefined;
755+
let modelConfig: import('@automaker/types').ProviderModel | undefined;
756+
757+
// 1. Try resolving by explicit providerId first (most reliable)
758+
if (providerId) {
759+
provider = providers.find((p) => p.id === providerId);
760+
if (provider) {
761+
if (!isProviderEnabled(provider)) {
762+
logger.warn(
763+
`${logPrefix} Explicitly requested provider "${provider.name}" (${providerId}) is disabled (enabled=${provider.enabled})`
764+
);
765+
} else {
766+
logger.debug(
767+
`${logPrefix} Found provider "${provider.name}" (${providerId}), enabled=${provider.enabled ?? 'undefined (treated as enabled)'}`
768+
);
769+
// Find the model config within this provider to check for mappings
770+
modelConfig = findModelInProvider(provider, modelId);
771+
if (!modelConfig && provider.models && provider.models.length > 0) {
772+
logger.debug(
773+
`${logPrefix} Model "${modelId}" not found in provider "${provider.name}". Available models: ${provider.models.map((m) => m.id).join(', ')}`
774+
);
775+
}
776+
}
777+
} else {
778+
logger.warn(
779+
`${logPrefix} Explicitly requested provider "${providerId}" not found. Available providers: ${providers.map((p) => p.id).join(', ')}`
780+
);
781+
}
782+
}
783+
784+
// 2. Fallback to model-based lookup across all providers if modelConfig not found
785+
// Note: We still search even if provider was found, to get the modelConfig for mapping
786+
if (!modelConfig) {
787+
for (const p of providers) {
788+
if (!isProviderEnabled(p) || p.id === providerId) continue; // Skip disabled or already checked
789+
790+
const config = findModelInProvider(p, modelId);
791+
792+
if (config) {
793+
// Only override provider if we didn't find one by explicit ID
794+
if (!provider) {
795+
provider = p;
796+
}
797+
modelConfig = config;
798+
logger.debug(`${logPrefix} Found model "${modelId}" in provider "${p.name}" (fallback)`);
799+
break;
800+
}
801+
}
802+
}
803+
804+
// 3. Resolve the mapped Claude model if specified
805+
let resolvedModel: string | undefined;
806+
if (modelConfig?.mapsToClaudeModel) {
807+
const { resolveModelString } = await import('@automaker/model-resolver');
808+
resolvedModel = resolveModelString(modelConfig.mapsToClaudeModel);
809+
logger.debug(
810+
`${logPrefix} Model "${modelId}" maps to Claude model "${modelConfig.mapsToClaudeModel}" -> "${resolvedModel}"`
811+
);
812+
}
813+
814+
// Log final result for debugging
815+
logger.debug(
816+
`${logPrefix} Provider context resolved: provider=${provider?.name ?? 'none'}, modelConfig=${modelConfig ? 'found' : 'not found'}, resolvedModel=${resolvedModel ?? modelId}`
817+
);
818+
819+
return { provider, credentials, resolvedModel, modelConfig };
820+
} catch (error) {
821+
logger.error(`${logPrefix} Failed to resolve provider context:`, error);
822+
return {
823+
provider: undefined,
824+
credentials: undefined,
825+
resolvedModel: undefined,
826+
modelConfig: undefined,
827+
};
828+
}
829+
}
830+
692831
/**
693832
* Find a ClaudeCompatibleProvider by one of its model IDs.
694833
* Searches through all enabled providers to find one that contains the specified model.

apps/server/src/providers/claude-provider.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,7 @@ export class ClaudeProvider extends BaseProvider {
188188
async *executeQuery(options: ExecuteOptions): AsyncGenerator<ProviderMessage> {
189189
// Validate that model doesn't have a provider prefix
190190
// AgentService should strip prefixes before passing to providers
191+
// Claude doesn't use a provider prefix, so we don't need to specify an expected provider
191192
validateBareModelId(options.model, 'ClaudeProvider');
192193

193194
const {

apps/server/src/providers/codex-provider.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -739,9 +739,9 @@ export class CodexProvider extends BaseProvider {
739739
}
740740

741741
async *executeQuery(options: ExecuteOptions): AsyncGenerator<ProviderMessage> {
742-
// Validate that model doesn't have a provider prefix
742+
// Validate that model doesn't have a provider prefix (except codex- which should already be stripped)
743743
// AgentService should strip prefixes before passing to providers
744-
validateBareModelId(options.model, 'CodexProvider');
744+
validateBareModelId(options.model, 'CodexProvider', 'codex');
745745

746746
try {
747747
const mcpServers = options.mcpServers ?? {};

0 commit comments

Comments
 (0)