Browse Source

fix: prevent empty task completions

Alex Fazzari 2 tuần trước cách đây
mục cha
commit
cf2b1cbc2d

+ 64 - 0
src/hooks/task-session-manager/status-utils.test.ts

@@ -0,0 +1,64 @@
+import { describe, expect, mock, test } from 'bun:test';
+import { BackgroundJobBoard } from '../../utils/background-job-board';
+import { COMPLETED_WITHOUT_TEXT_DIAGNOSTIC } from '../../utils/task';
+import { updateBackgroundJobFromOutput } from './status-utils';
+
+function harness() {
+  const board = new BackgroundJobBoard();
+  board.registerLaunch({
+    taskID: 'child-1',
+    parentSessionID: 'parent-1',
+    agent: 'explorer',
+    description: 'trace bug',
+  });
+  const taskContextTracker = {
+    pendingManagedTaskIds: new Set<string>(),
+    contextFilesForPrompt: () => [],
+    prune: mock(() => {}),
+  };
+  return { board, taskContextTracker };
+}
+
+describe('updateBackgroundJobFromOutput', () => {
+  test('does not enter completed from an empty completed status', () => {
+    const { board, taskContextTracker } = harness();
+    const updated = updateBackgroundJobFromOutput(
+      [
+        'task_id: child-1',
+        'state: completed',
+        '',
+        '<task_result>',
+        '</task_result>',
+      ].join('\n'),
+      board,
+      taskContextTracker,
+    );
+
+    expect(updated).toMatchObject({
+      state: 'error',
+      resultSummary: COMPLETED_WITHOUT_TEXT_DIAGNOSTIC,
+    });
+    expect(board.get('child-1')?.state).not.toBe('completed');
+  });
+
+  test('records completed when the status carries result text', () => {
+    const { board, taskContextTracker } = harness();
+    const updated = updateBackgroundJobFromOutput(
+      [
+        'task_id: child-1',
+        'state: completed',
+        '',
+        '<task_result>',
+        'final findings',
+        '</task_result>',
+      ].join('\n'),
+      board,
+      taskContextTracker,
+    );
+
+    expect(updated).toMatchObject({
+      state: 'completed',
+      resultSummary: 'final findings',
+    });
+  });
+});

+ 10 - 3
src/hooks/task-session-manager/status-utils.ts

@@ -3,7 +3,7 @@ import type {
   BackgroundJobStore,
   ContextFile,
 } from '../../utils';
-import { parseTaskStatusOutput } from '../../utils';
+import { guardCompletedStatusText, parseTaskStatusOutput } from '../../utils';
 import { isRecord as isObjectRecord } from '../../utils/guards';
 import { log } from '../../utils/logger';
 
@@ -76,11 +76,18 @@ export function updateBackgroundJobFromOutput(
     return existing;
   }
 
+  const guarded = guardCompletedStatusText(
+    status.state,
+    status.result,
+    existing?.resultSummary,
+  );
+
   const updated = backgroundJobBoard.updateStatus({
     taskID: status.taskID,
-    state: status.state,
+    state: guarded.state,
     timedOut: status.timedOut,
-    resultSummary: status.result,
+    resultSummary: guarded.resultSummary,
+    lastStatusError: guarded.lastStatusError,
   });
   if (!updated) {
     log('[task-session-manager] ignored status for unknown background job', {

+ 9 - 2
src/hooks/task-session-manager/tool-execute-hooks.ts

@@ -13,6 +13,7 @@ import type {
 import {
   deriveFullObjective,
   deriveTaskSessionLabel,
+  guardCompletedStatusText,
   parseTaskIdFromTaskOutput,
   parseTaskLaunchOutput,
   parseTaskStatusOutput,
@@ -350,12 +351,18 @@ export async function handleToolExecuteAfter(
       deps.clearRehydrateTombstone?.(status.taskID);
       normalizeLateCancelledTaskOutput(output, deps.backgroundJobBoard);
       if (exactCallConfirmed) deps.backgroundJobSupervisor?.onLaunch(record);
+      const guarded = guardCompletedStatusText(
+        status.state,
+        status.result,
+        record.resultSummary,
+      );
       const updated = deps.backgroundJobBoard.updateStatus({
         taskID: status.taskID,
-        state: status.state,
+        state: guarded.state,
         expectedGeneration: record.generation,
         timedOut: status.timedOut,
-        resultSummary: status.result,
+        resultSummary: guarded.resultSummary,
+        lastStatusError: guarded.lastStatusError,
       });
       log('[task-session-manager] foreground task status registered', {
         taskID: status.taskID,

+ 25 - 0
src/utils/background-job-board.test.ts

@@ -677,6 +677,31 @@ describe('BackgroundJobBoard', () => {
     });
   });
 
+  test('rejects an empty completed status as an error', () => {
+    const board = new BackgroundJobBoard();
+    board.registerLaunch({
+      taskID: 'ses_1',
+      parentSessionID: 'parent-1',
+      agent: 'explorer',
+      description: 'map files',
+    });
+
+    board.updateFromStatusOutput(
+      [
+        'task_id: ses_1',
+        'state: completed',
+        '<task_result>',
+        '</task_result>',
+      ].join('\n'),
+    );
+
+    expect(board.get('ses_1')).toMatchObject({
+      state: 'error',
+      resultSummary:
+        'Task ended without a public text result; completion is not confirmed',
+    });
+  });
+
   test('updates error summary from task_error output', () => {
     const board = new BackgroundJobBoard();
     board.registerLaunch({

+ 14 - 3
src/utils/background-job-board.ts

@@ -11,7 +11,11 @@ import {
   recordBackgroundJobSuppression,
 } from './background-job-store';
 import { log } from './logger';
-import { parseTaskStatusOutput, type TaskOutputState } from './task';
+import {
+  guardCompletedStatusText,
+  parseTaskStatusOutput,
+  type TaskOutputState,
+} from './task';
 
 export interface ContextFile {
   path: string;
@@ -413,11 +417,18 @@ export class BackgroundJobBoard implements BackgroundJobStore {
     const status = parseTaskStatusOutput(output);
     if (!status) return undefined;
 
+    const guarded = guardCompletedStatusText(
+      status.state,
+      status.result,
+      this.get(status.taskID)?.resultSummary,
+    );
+
     return this.updateStatus({
       taskID: status.taskID,
-      state: status.state,
+      state: guarded.state,
       timedOut: status.timedOut,
-      resultSummary: status.result,
+      resultSummary: guarded.resultSummary,
+      lastStatusError: guarded.lastStatusError,
     });
   }
 

+ 37 - 0
src/utils/task.test.ts

@@ -1,5 +1,7 @@
 import { describe, expect, test } from 'bun:test';
 import {
+  COMPLETED_WITHOUT_TEXT_DIAGNOSTIC,
+  guardCompletedStatusText,
   parseTaskIdFromTaskOutput,
   parseTaskLaunchOutput,
   parseTaskResultFromOutput,
@@ -8,6 +10,41 @@ import {
   renderRunningTaskPlaceholder,
 } from './task';
 
+describe('guardCompletedStatusText', () => {
+  test('keeps completed only when non-empty result text exists', () => {
+    expect(
+      guardCompletedStatusText('completed', 'final text', undefined),
+    ).toEqual({ state: 'completed', resultSummary: 'final text' });
+  });
+
+  test('downgrades empty completed to error with a diagnostic', () => {
+    const guarded = guardCompletedStatusText('completed', undefined, undefined);
+    expect(guarded.state).toBe('error');
+    expect(guarded.resultSummary).toBe(COMPLETED_WITHOUT_TEXT_DIAGNOSTIC);
+  });
+
+  test('downgrades whitespace-only completed to error', () => {
+    expect(guardCompletedStatusText('completed', '  ', undefined).state).toBe(
+      'error',
+    );
+  });
+
+  test('keeps completed when the board already holds a summary', () => {
+    expect(
+      guardCompletedStatusText('completed', undefined, 'recorded result'),
+    ).toEqual({ state: 'completed', resultSummary: undefined });
+  });
+
+  test('passes non-completed states through unchanged', () => {
+    for (const state of ['running', 'error', 'cancelled'] as const) {
+      expect(guardCompletedStatusText(state, '', undefined)).toEqual({
+        state,
+        resultSummary: '',
+      });
+    }
+  });
+});
+
 describe('renderRunningTaskPlaceholder', () => {
   test('is deterministic and keyed only on the task ID', () => {
     const a = renderRunningTaskPlaceholder('ses_123');

+ 29 - 0
src/utils/task.ts

@@ -134,6 +134,35 @@ export function parseTaskStateFromOutput(
   return undefined;
 }
 
+/** Diagnostic applied when a terminal `completed` report carries no text. */
+export const COMPLETED_WITHOUT_TEXT_DIAGNOSTIC =
+  'Task ended without a public text result; completion is not confirmed';
+
+export interface GuardedTaskStatus {
+  state: TaskOutputState;
+  resultSummary?: string;
+  lastStatusError?: string;
+}
+
+export function guardCompletedStatusText(
+  state: TaskOutputState,
+  result: string | undefined,
+  existingResultSummary: string | undefined,
+): GuardedTaskStatus {
+  if (
+    state === 'completed' &&
+    !result?.trim() &&
+    !existingResultSummary?.trim()
+  ) {
+    return {
+      state: 'error',
+      resultSummary: COMPLETED_WITHOUT_TEXT_DIAGNOSTIC,
+      lastStatusError: COMPLETED_WITHOUT_TEXT_DIAGNOSTIC,
+    };
+  }
+  return { state, resultSummary: result };
+}
+
 export function parseTaskResultFromOutput(output: string): string | undefined {
   // Require matching open/close tags via backreference
   const match = /<task_(result|error)>\s*([\s\S]*?)\s*<\/task_\1>/m.exec(