Browse Source

fix: address greptile session review

Alvin Unreal 3 months ago
parent
commit
ec0d4574e7

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

@@ -490,6 +490,17 @@
         }
       }
     },
+    "sessionManager": {
+      "type": "object",
+      "properties": {
+        "maxSessionsPerAgent": {
+          "default": 2,
+          "type": "integer",
+          "minimum": 1,
+          "maximum": 10
+        }
+      }
+    },
     "todoContinuation": {
       "type": "object",
       "properties": {

+ 171 - 0
src/hooks/task-session-manager/index.test.ts

@@ -173,6 +173,132 @@ describe('task-session-manager hook', () => {
     expect(system.system.join('\n')).not.toContain('exp-1');
   });
 
+  test('drops resumed predecessor when success returns a new task id', async () => {
+    const { hook } = createHook();
+
+    await hook['tool.execute.before'](
+      {
+        tool: 'task',
+        sessionID: 'parent-1',
+        callID: 'call-1',
+      },
+      {
+        args: {
+          subagent_type: 'explorer',
+          description: 'config schema',
+        },
+      },
+    );
+    await hook['tool.execute.after'](
+      {
+        tool: 'task',
+        sessionID: 'parent-1',
+        callID: 'call-1',
+      },
+      {
+        output:
+          'task_id: child-1 (for resuming to continue this task if needed)',
+      },
+    );
+
+    await hook['tool.execute.before'](
+      {
+        tool: 'task',
+        sessionID: 'parent-1',
+        callID: 'call-2',
+      },
+      {
+        args: {
+          subagent_type: 'explorer',
+          description: 'continue schema work',
+          task_id: 'exp-1',
+        },
+      },
+    );
+    await hook['tool.execute.after'](
+      {
+        tool: 'task',
+        sessionID: 'parent-1',
+        callID: 'call-2',
+      },
+      {
+        output:
+          'task_id: child-2 (for resuming to continue this task if needed)',
+      },
+    );
+
+    const system = { system: ['base'] };
+    await hook['experimental.chat.system.transform'](
+      { sessionID: 'parent-1' },
+      system,
+    );
+
+    expect(system.system.join('\n')).toContain('continue schema work');
+    expect(system.system.join('\n')).not.toContain('config schema');
+  });
+
+  test('does not drop remembered session on non-runtime session text', async () => {
+    const { hook } = createHook();
+
+    await hook['tool.execute.before'](
+      {
+        tool: 'task',
+        sessionID: 'parent-1',
+        callID: 'call-1',
+      },
+      {
+        args: {
+          subagent_type: 'explorer',
+          description: 'config schema',
+        },
+      },
+    );
+    await hook['tool.execute.after'](
+      {
+        tool: 'task',
+        sessionID: 'parent-1',
+        callID: 'call-1',
+      },
+      {
+        output:
+          'task_id: child-1 (for resuming to continue this task if needed)',
+      },
+    );
+
+    await hook['tool.execute.before'](
+      {
+        tool: 'task',
+        sessionID: 'parent-1',
+        callID: 'call-2',
+      },
+      {
+        args: {
+          subagent_type: 'explorer',
+          description: 'continue schema work',
+          task_id: 'exp-1',
+        },
+      },
+    );
+    await hook['tool.execute.after'](
+      {
+        tool: 'task',
+        sessionID: 'parent-1',
+        callID: 'call-2',
+      },
+      {
+        output: 'Found no session cookies in fixtures, continuing analysis.',
+      },
+    );
+
+    const system = { system: ['base'] };
+    await hook['experimental.chat.system.transform'](
+      { sessionID: 'parent-1' },
+      system,
+    );
+
+    expect(system.system.join('\n')).toContain('exp-1 config schema');
+  });
+
   test('ignores sessions that are not orchestrator-managed', async () => {
     const { hook } = createHook({ shouldManageSession: () => false });
 
@@ -252,4 +378,49 @@ describe('task-session-manager hook', () => {
     );
     expect(afterChildDelete.system).toEqual(['base']);
   });
+
+  test('cleans pending calls when parent session is deleted', async () => {
+    const { hook } = createHook();
+
+    await hook['tool.execute.before'](
+      {
+        tool: 'task',
+        sessionID: 'parent-1',
+        callID: 'call-1',
+      },
+      {
+        args: {
+          subagent_type: 'oracle',
+          description: 'architecture review',
+        },
+      },
+    );
+
+    await hook.event({
+      event: {
+        type: 'session.deleted',
+        properties: { sessionID: 'parent-1' },
+      },
+    });
+
+    await hook['tool.execute.after'](
+      {
+        tool: 'task',
+        sessionID: 'parent-1',
+        callID: 'call-1',
+      },
+      {
+        output:
+          'task_id: child-1 (for resuming to continue this task if needed)',
+      },
+    );
+
+    const system = { system: ['base'] };
+    await hook['experimental.chat.system.transform'](
+      { sessionID: 'parent-1' },
+      system,
+    );
+
+    expect(system.system).toEqual(['base']);
+  });
 });

+ 54 - 9
src/hooks/task-session-manager/index.ts

@@ -14,6 +14,7 @@ interface TaskArgs {
 }
 
 interface PendingTaskCall {
+  callId: string;
   parentSessionId: string;
   agentType: AgentName;
   label: string;
@@ -32,6 +33,8 @@ const AGENT_NAME_SET = new Set<AgentName>([
   'councillor',
 ]);
 
+const MAX_PENDING_TASK_CALLS = 100;
+
 function isAgentName(value: unknown): value is AgentName {
   return typeof value === 'string' && AGENT_NAME_SET.has(value as AgentName);
 }
@@ -49,15 +52,43 @@ export function createTaskSessionManagerHook(
 ) {
   const sessionManager = new SessionManager(options.maxSessionsPerAgent);
   const pendingCalls = new Map<string, PendingTaskCall>();
+  const pendingCallOrder: string[] = [];
 
   function isMissingRememberedSessionError(output: string): boolean {
-    const normalized = output.toLowerCase();
+    const firstLine = output.split(/\r?\n/, 1)[0]?.trim().toLowerCase() ?? '';
     return (
-      normalized.includes('session') &&
-      (normalized.includes('not found') || normalized.includes('no session'))
+      firstLine.startsWith('[error]') &&
+      firstLine.includes('session') &&
+      (firstLine.includes('not found') || firstLine.includes('no session'))
     );
   }
 
+  function rememberPendingCall(call: PendingTaskCall): void {
+    pendingCalls.set(call.callId, call);
+    pendingCallOrder.push(call.callId);
+
+    while (pendingCallOrder.length > MAX_PENDING_TASK_CALLS) {
+      const evictedCallId = pendingCallOrder.shift();
+      if (!evictedCallId) {
+        break;
+      }
+      pendingCalls.delete(evictedCallId);
+    }
+  }
+
+  function takePendingCall(callId?: string): PendingTaskCall | undefined {
+    if (!callId) return undefined;
+    const pending = pendingCalls.get(callId);
+    pendingCalls.delete(callId);
+
+    const orderIndex = pendingCallOrder.indexOf(callId);
+    if (orderIndex >= 0) {
+      pendingCallOrder.splice(orderIndex, 1);
+    }
+
+    return pending;
+  }
+
   return {
     'tool.execute.before': async (
       input: { tool: string; sessionID?: string; callID?: string },
@@ -80,7 +111,8 @@ export function createTaskSessionManagerHook(
       });
 
       if (input.callID) {
-        pendingCalls.set(input.callID, {
+        rememberPendingCall({
+          callId: input.callID,
           parentSessionId: input.sessionID,
           agentType: args.subagent_type,
           label,
@@ -110,7 +142,8 @@ export function createTaskSessionManagerHook(
         remembered.taskId,
       );
       if (input.callID) {
-        pendingCalls.set(input.callID, {
+        rememberPendingCall({
+          callId: input.callID,
           parentSessionId: input.sessionID,
           agentType: args.subagent_type,
           label,
@@ -125,10 +158,7 @@ export function createTaskSessionManagerHook(
     ): Promise<void> => {
       if (input.tool.toLowerCase() !== 'task') return;
 
-      const pending = input.callID ? pendingCalls.get(input.callID) : undefined;
-      if (input.callID) {
-        pendingCalls.delete(input.callID);
-      }
+      const pending = takePendingCall(input.callID);
 
       if (!pending || typeof output.output !== 'string') return;
       const taskId = parseTaskIdFromTaskOutput(output.output);
@@ -146,6 +176,14 @@ export function createTaskSessionManagerHook(
         return;
       }
 
+      if (pending.resumedTaskId && pending.resumedTaskId !== taskId) {
+        sessionManager.drop(
+          pending.parentSessionId,
+          pending.agentType,
+          pending.resumedTaskId,
+        );
+      }
+
       sessionManager.remember({
         parentSessionId: pending.parentSessionId,
         taskId,
@@ -180,6 +218,13 @@ export function createTaskSessionManagerHook(
 
       sessionManager.clearParent(sessionId);
       sessionManager.dropTask(sessionId);
+
+      for (const [callId, pending] of pendingCalls.entries()) {
+        if (pending.parentSessionId !== sessionId) {
+          continue;
+        }
+        takePendingCall(callId);
+      }
     },
   };
 }