Просмотр исходного кода

fix(subtask): honor parent cancel with timeout=0 and prevent empty subtask block from clobbering merged config

Addresses Codex review feedback on #505:

- P1: promptWithTimeout now races the abort signal even when timeoutMs<=0,
  so cancelling the parent tool still aborts the child session and runs
  the cleanup finally. Previously the no-timeout branch awaited
  session.prompt directly, ignoring the signal.

- P2: SubtaskConfigSchema.timeoutMs is now .optional() instead of
  .default(300000). With a default, an empty 'subtask: {}' block in a
  project config was materialized as { timeoutMs: 300000 } and would
  shallow-overwrite a user-level override during config merging. The
  runtime fallback in createSubtaskTool (?? DEFAULT_SUBTASK_TIMEOUT_MS)
  continues to apply the default. Also added subtask to
  mergePluginConfigs so future sub-fields deep-merge consistently with
  fallback, council, etc.

Tests:
- session.test.ts: covers signal-cancel and resolve paths with
  timeoutMs=0.
- loader.test.ts: covers project 'subtask: {}' preserving user
  timeoutMs.
Omer Faruk Oruc 2 месяцев назад
Родитель
Сommit
0f077a6a31

+ 0 - 1
oh-my-opencode-slim.schema.json

@@ -603,7 +603,6 @@
       "type": "object",
       "properties": {
         "timeoutMs": {
-          "default": 300000,
           "description": "Subtask worker timeout in ms. 0 disables the timeout. Defaults to 300000 (5 minutes).",
           "type": "integer",
           "minimum": 0,

+ 20 - 0
src/config/loader.test.ts

@@ -695,6 +695,26 @@ describe('deepMerge behavior', () => {
     const config = loadPluginConfig(projectDir);
     expect(config.fallback?.chains.writing).toEqual(['openai/gpt-5.5']);
   });
+
+  test('empty project subtask block does not clobber user subtask.timeoutMs', () => {
+    const userOpencodeDir = path.join(userConfigDir, 'opencode');
+    fs.mkdirSync(userOpencodeDir, { recursive: true });
+    fs.writeFileSync(
+      path.join(userOpencodeDir, 'oh-my-opencode-slim.json'),
+      JSON.stringify({ subtask: { timeoutMs: 1800000 } }),
+    );
+
+    const projectDir = path.join(tempDir, 'project');
+    const projectConfigDir = path.join(projectDir, '.opencode');
+    fs.mkdirSync(projectConfigDir, { recursive: true });
+    fs.writeFileSync(
+      path.join(projectConfigDir, 'oh-my-opencode-slim.json'),
+      JSON.stringify({ subtask: {} }),
+    );
+
+    const config = loadPluginConfig(projectDir);
+    expect(config.subtask?.timeoutMs).toBe(1800000);
+  });
 });
 
 describe('preset resolution', () => {

+ 1 - 0
src/config/loader.ts

@@ -204,6 +204,7 @@ export function mergePluginConfigs(
     divoom: deepMerge(base.divoom, override.divoom),
     fallback: deepMerge(base.fallback, override.fallback),
     council: deepMerge(base.council, override.council),
+    subtask: deepMerge(base.subtask, override.subtask),
   };
 }
 

+ 4 - 1
src/config/schema.ts

@@ -253,12 +253,15 @@ export type TodoContinuationConfig = z.infer<
 >;
 
 export const SubtaskConfigSchema = z.object({
+  // Intentionally no .default(): an empty `subtask: {}` block must parse to
+  // `{}` so it cannot shallow-overwrite an inherited value during config
+  // merging. The runtime fallback in createSubtaskTool applies the default.
   timeoutMs: z
     .number()
     .int()
     .min(0)
     .max(24 * 60 * 60 * 1000)
-    .default(5 * 60 * 1000)
+    .optional()
     .describe(
       'Subtask worker timeout in ms. 0 disables the timeout. Defaults to 300000 (5 minutes).',
     ),

+ 38 - 0
src/utils/session.test.ts

@@ -57,6 +57,44 @@ describe('session utilities', () => {
     ).rejects.toThrow('Prompt timed out after 5ms');
   });
 
+  test('promptWithTimeout honors abort signal when timeout is disabled', async () => {
+    const controller = new AbortController();
+    const abort = mock(async () => ({}));
+    const prompt = mock(() => never());
+    const client = {
+      session: {
+        abort,
+        prompt,
+      },
+    } as any;
+
+    queueMicrotask(() => controller.abort());
+
+    await expect(
+      promptWithTimeout(
+        client,
+        { path: { id: 's1' }, body: { parts: [] } },
+        0,
+        controller.signal,
+      ),
+    ).rejects.toThrow('Prompt cancelled');
+  });
+
+  test('promptWithTimeout returns when prompt resolves with no timeout', async () => {
+    const abort = mock(async () => ({}));
+    const prompt = mock(async () => ({}));
+    const client = {
+      session: {
+        abort,
+        prompt,
+      },
+    } as any;
+
+    await expect(
+      promptWithTimeout(client, { path: { id: 's1' }, body: { parts: [] } }, 0),
+    ).resolves.toBeUndefined();
+  });
+
   test('abortSessionWithTimeout rejects if abort hangs', async () => {
     const client = {
       session: {

+ 31 - 24
src/utils/session.ts

@@ -102,12 +102,8 @@ export async function promptWithTimeout(
 ): Promise<void> {
   if (signal?.aborted) throw new Error('Prompt cancelled');
 
-  if (timeoutMs <= 0) {
-    await client.session.prompt(args);
-    return;
-  }
-
   const sessionId = args.path.id;
+  const hasTimeout = timeoutMs > 0;
   let timer: ReturnType<typeof setTimeout> | undefined;
   let onAbort: (() => void) | undefined;
 
@@ -115,25 +111,36 @@ export async function promptWithTimeout(
     const promptPromise = client.session.prompt(args);
     promptPromise.catch(() => {});
 
-    await Promise.race([
-      promptPromise,
-      new Promise<never>((_, reject) => {
-        timer = setTimeout(() => {
-          reject(
-            new OperationTimeoutError(`Prompt timed out after ${timeoutMs}ms`),
-          );
-        }, timeoutMs);
-      }),
-      new Promise<never>((_, reject) => {
-        if (!signal) return;
-        if (signal.aborted) {
-          reject(new Error('Prompt cancelled'));
-          return;
-        }
-        onAbort = () => reject(new Error('Prompt cancelled'));
-        signal.addEventListener('abort', onAbort, { once: true });
-      }),
-    ]);
+    const racers: Array<Promise<unknown>> = [promptPromise];
+
+    if (hasTimeout) {
+      racers.push(
+        new Promise<never>((_, reject) => {
+          timer = setTimeout(() => {
+            reject(
+              new OperationTimeoutError(
+                `Prompt timed out after ${timeoutMs}ms`,
+              ),
+            );
+          }, timeoutMs);
+        }),
+      );
+    }
+
+    if (signal) {
+      racers.push(
+        new Promise<never>((_, reject) => {
+          if (signal.aborted) {
+            reject(new Error('Prompt cancelled'));
+            return;
+          }
+          onAbort = () => reject(new Error('Prompt cancelled'));
+          signal.addEventListener('abort', onAbort, { once: true });
+        }),
+      );
+    }
+
+    await Promise.race(racers);
   } catch (error) {
     if (error instanceof OperationTimeoutError) {
       try {