Skip to content

Commit dabd9ff

Browse files
committed
Fix waitForInit to never throw and deduplicate test code
- waitForInit() now never throws - tools proceed regardless of init outcome - If init fails or times out, tools will fail naturally with their own errors - Simplified toolHelpers wrapper (no longer needs error handling) - Updated all 4 tools to use simplified helper - Added test helpers: collectInitEvents() and waitForInitEnd() - Refactored all 6 tests in initWorkspace.test.ts to use helpers - Reduced test duplication by ~100 lines - Updated InitStateManager test to reflect new non-throwing behavior
1 parent 27d3e98 commit dabd9ff

File tree

9 files changed

+136
-162
lines changed

9 files changed

+136
-162
lines changed

src/services/initStateManager.test.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -236,15 +236,15 @@ describe("InitStateManager", () => {
236236
// Get the init promise before clearing
237237
const initPromise = manager.waitForInit(workspaceId);
238238

239-
// Clear in-memory state (should reject pending promises)
239+
// Clear in-memory state (rejects internal promise, but waitForInit catches it)
240240
manager.clearInMemoryState(workspaceId);
241241

242242
// Verify state is cleared
243243
expect(manager.getInitState(workspaceId)).toBeUndefined();
244244

245-
// Verify the promise was rejected
246-
// eslint-disable-next-line @typescript-eslint/await-thenable
247-
await expect(initPromise).rejects.toThrow("Workspace test-workspace was deleted");
245+
// waitForInit never throws - it resolves even when init is canceled
246+
// This allows tools to proceed and fail naturally with their own errors
247+
await expect(initPromise).resolves.toBeUndefined();
248248
});
249249
});
250250

src/services/initStateManager.ts

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -300,16 +300,19 @@ export class InitStateManager extends EventEmitter {
300300
*
301301
* Behavior:
302302
* - No init state: Returns immediately (init not needed or backwards compat)
303-
* - Init succeeded/failed: Returns immediately (let tool proceed/fail naturally)
304-
* - Init running: Waits for completion promise (up to 5 minutes)
303+
* - Init succeeded/failed: Returns immediately (tools proceed regardless of init outcome)
304+
* - Init running: Waits for completion promise (up to 5 minutes, then proceeds anyway)
305+
*
306+
* This method NEVER throws - tools should always proceed. If init fails or times out,
307+
* the tool will either succeed (if init wasn't critical) or fail with its own error
308+
* (e.g., file not found). This provides better error messages than blocking on init.
305309
*
306310
* Promise-based approach eliminates race conditions:
307311
* - Multiple tools share the same promise (no duplicate listeners)
308312
* - No event cleanup needed (promise auto-resolves once)
309313
* - Timeout races handled by Promise.race()
310314
*
311315
* @param workspaceId Workspace ID to wait for
312-
* @throws Error if init times out after 5 minutes
313316
*/
314317
async waitForInit(workspaceId: string): Promise<void> {
315318
const state = this.getInitState(workspaceId);
@@ -320,6 +323,7 @@ export class InitStateManager extends EventEmitter {
320323
}
321324

322325
// Init already completed (success or failure) - proceed immediately
326+
// Tools should work regardless of init outcome
323327
if (state.status !== "running") {
324328
return;
325329
}
@@ -334,14 +338,24 @@ export class InitStateManager extends EventEmitter {
334338
}
335339

336340
const INIT_TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes
337-
const timeoutPromise = new Promise<void>((_, reject) => {
341+
const timeoutPromise = new Promise<void>((resolve) => {
338342
setTimeout(() => {
339-
reject(new Error("Workspace initialization timed out after 5 minutes"));
343+
log.error(
344+
`Init timeout for ${workspaceId} after 5 minutes - tools will proceed anyway. ` +
345+
`Init will continue in background.`
346+
);
347+
resolve(); // Resolve, don't reject - tools should proceed
340348
}, INIT_TIMEOUT_MS);
341349
});
342350

343351
// Race between completion and timeout
344-
// If timeout wins, promise rejection propagates to caller
345-
await Promise.race([promiseEntry.promise, timeoutPromise]);
352+
// Both resolve (no rejection), so tools always proceed
353+
try {
354+
await Promise.race([promiseEntry.promise, timeoutPromise]);
355+
} catch (error) {
356+
// Init promise was rejected (e.g., workspace deleted)
357+
// Log and proceed anyway - let the tool fail with its own error if needed
358+
log.error(`Init wait interrupted for ${workspaceId}: ${error} - proceeding anyway`);
359+
}
346360
}
347361
}

src/services/tools/bash.ts

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -40,15 +40,7 @@ export const createBashTool: ToolFactory = (config: ToolConfiguration) => {
4040
inputSchema: TOOL_DEFINITIONS.bash.schema,
4141
execute: async ({ script, timeout_secs }, { abortSignal }): Promise<BashToolResult> => {
4242
// Wait for workspace initialization to complete (no-op if already complete or not needed)
43-
const initError = await waitForWorkspaceInit(config, "execute bash");
44-
if (initError) {
45-
return {
46-
success: false,
47-
error: initError,
48-
exitCode: -1,
49-
wall_duration_ms: 0,
50-
};
51-
}
43+
await waitForWorkspaceInit(config);
5244

5345
// Validate script is not empty - likely indicates a malformed tool call
5446

src/services/tools/file_edit_insert.ts

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,7 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration)
2626
create,
2727
}): Promise<FileEditInsertToolResult> => {
2828
// Wait for workspace initialization to complete (no-op if already complete or not needed)
29-
const initError = await waitForWorkspaceInit(config, "insert into file");
30-
if (initError) {
31-
return {
32-
success: false,
33-
error: `${WRITE_DENIED_PREFIX} ${initError}`,
34-
};
35-
}
29+
await waitForWorkspaceInit(config);
3630

3731
try {
3832
// Validate no redundant path prefix (must come first to catch absolute paths)

src/services/tools/file_edit_operation.ts

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -42,13 +42,7 @@ export async function executeFileEditOperation<TMetadata>({
4242
FileEditErrorResult | (FileEditDiffSuccessBase & TMetadata)
4343
> {
4444
// Wait for workspace initialization to complete (no-op if already complete or not needed)
45-
const initError = await waitForWorkspaceInit(config, "edit file");
46-
if (initError) {
47-
return {
48-
success: false,
49-
error: `${WRITE_DENIED_PREFIX} ${initError}`,
50-
};
51-
}
45+
await waitForWorkspaceInit(config);
5246

5347
try {
5448
// Validate no redundant path prefix (must come first to catch absolute paths)

src/services/tools/file_read.ts

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,7 @@ export const createFileReadTool: ToolFactory = (config: ToolConfiguration) => {
2323
// Note: abortSignal available but not used - file reads are fast and complete quickly
2424

2525
// Wait for workspace initialization to complete (no-op if already complete or not needed)
26-
const initError = await waitForWorkspaceInit(config, "read file");
27-
if (initError) {
28-
return {
29-
success: false,
30-
error: initError,
31-
};
32-
}
26+
await waitForWorkspaceInit(config);
3327

3428
try {
3529
// Validate no redundant path prefix (must come first to catch absolute paths)

src/services/tools/toolHelpers.ts

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,20 +2,11 @@ import type { ToolConfiguration } from "@/utils/tools/tools";
22

33
/**
44
* Wait for workspace initialization to complete before executing tool.
5-
* Wraps InitStateManager.waitForInit() with consistent error handling.
5+
* Wraps InitStateManager.waitForInit() for tool consistency.
66
*
7-
* Returns null on success, or an error message string on failure.
8-
* The returned error message is ready to use in tool error results.
7+
* This is a no-op wrapper since waitForInit() never throws and always allows
8+
* tools to proceed. Kept for consistency and future extensibility.
99
*/
10-
export async function waitForWorkspaceInit(
11-
config: ToolConfiguration,
12-
operationName: string
13-
): Promise<string | null> {
14-
try {
15-
await config.initStateManager.waitForInit(config.workspaceId);
16-
return null;
17-
} catch (error) {
18-
const errorMsg = error instanceof Error ? error.message : String(error);
19-
return `Cannot ${operationName}: ${errorMsg}`;
20-
}
10+
export async function waitForWorkspaceInit(config: ToolConfiguration): Promise<void> {
11+
await config.initStateManager.waitForInit(config.workspaceId);
2112
}

tests/ipcMain/helpers.ts

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
import type { IpcRenderer } from "electron";
22
import { IPC_CHANNELS, getChatChannel } from "../../src/constants/ipc-constants";
3-
import type { SendMessageOptions, WorkspaceChatMessage } from "../../src/types/ipc";
3+
import type {
4+
SendMessageOptions,
5+
WorkspaceChatMessage,
6+
WorkspaceInitEvent,
7+
} from "../../src/types/ipc";
8+
import { isInitStart, isInitOutput, isInitEnd } from "../../src/types/ipc";
49
import type { Result } from "../../src/types/result";
510
import type { SendMessageError } from "../../src/types/errors";
611
import type { WorkspaceMetadataWithPaths } from "../../src/types/workspace";
@@ -548,6 +553,57 @@ export async function waitForInitComplete(
548553
throw new Error(`Init did not complete within ${timeoutMs}ms - workspace may not be ready`);
549554
}
550555

556+
/**
557+
* Collect all init events for a workspace.
558+
* Filters sentEvents for init-start, init-output, and init-end events.
559+
* Returns the events in chronological order.
560+
*/
561+
export function collectInitEvents(
562+
env: import("./setup").TestEnvironment,
563+
workspaceId: string
564+
): WorkspaceInitEvent[] {
565+
return env.sentEvents
566+
.filter((e) => e.channel === getChatChannel(workspaceId))
567+
.map((e) => e.data as WorkspaceChatMessage)
568+
.filter((msg) => isInitStart(msg) || isInitOutput(msg) || isInitEnd(msg)) as WorkspaceInitEvent[];
569+
}
570+
571+
/**
572+
* Wait for init-end event without checking exit code.
573+
* Use this when you want to test failure cases or inspect the exit code yourself.
574+
* For success-only tests, use waitForInitComplete() which throws on failure.
575+
*/
576+
export async function waitForInitEnd(
577+
env: import("./setup").TestEnvironment,
578+
workspaceId: string,
579+
timeoutMs = 5000
580+
): Promise<void> {
581+
const startTime = Date.now();
582+
let pollInterval = 50;
583+
584+
while (Date.now() - startTime < timeoutMs) {
585+
// Check for init-end event in sentEvents
586+
const initEndEvent = env.sentEvents.find(
587+
(e) =>
588+
e.channel === getChatChannel(workspaceId) &&
589+
typeof e.data === "object" &&
590+
e.data !== null &&
591+
"type" in e.data &&
592+
e.data.type === "init-end"
593+
);
594+
595+
if (initEndEvent) {
596+
return; // Found end event, regardless of exit code
597+
}
598+
599+
await new Promise((resolve) => setTimeout(resolve, pollInterval));
600+
pollInterval = Math.min(pollInterval * 1.5, 500);
601+
}
602+
603+
// Throw error on timeout
604+
throw new Error(`Init did not complete within ${timeoutMs}ms`);
605+
}
606+
551607
/**
552608
* Wait for stream to complete successfully
553609
* Common pattern: create collector, wait for end, assert success

0 commit comments

Comments
 (0)