fix(core): propagate filesystem cleanup failures in team_delete

packages/core/src/agents/team/teamHelpers.test.ts

@@ -48,28 +48,43 @@ vi.mock('../../config/storage.js', async (importOriginal) => {
   };
 });
 
+// Mock node:fs/promises to allow per-test override of fs.rm.
+// All other functions pass through to the real implementation.
+let rmMockOverride:
+  | ((...args: Parameters<typeof fs.rm>) => Promise<unknown>)
+  | null = null;
 // Optional readFile hook for simulating mid-reclaim I/O failures.
 // While a hook is installed and returns a value, that value is used;
 // otherwise the real readFile runs.
 vi.mock('node:fs/promises', async (importOriginal) => {
-  const actual = await importOriginal<typeof import('node:fs/promises')>();
-  type ReadFileHook = (...args: Parameters<typeof actual.readFile>) => unknown;
+  const original = await importOriginal<typeof import('node:fs/promises')>();
+  type ReadFileHook = (...args: Parameters<typeof original.readFile>) => unknown;
   let readFileHook: ReadFileHook | undefined;
   return {
-    ...actual,
+    ...original,
+    default: original,
     __setReadFileHook: (fn: ReadFileHook | undefined) => {
       readFileHook = fn;
     },
-    readFile: (...args: Parameters<typeof actual.readFile>) => {
+    rm: (...args: Parameters<typeof fs.rm>) => {
+      if (rmMockOverride) return rmMockOverride(...args);
+      return original.rm(...args);
+    },
+    readFile: (...args: Parameters<typeof original.readFile>) => {
       const hooked = readFileHook?.(...args);
       if (hooked !== undefined) {
         return hooked;
       }
-      return actual.readFile(...args);
+      return original.readFile(...args);
     },
   };
 });
 
+function setFsRmMock(
+  fn: ((...args: Parameters<typeof fs.rm>) => Promise<unknown>) | null,
+) {
+  rmMockOverride = fn;
+}
 const { __setReadFileHook } = (await import('node:fs/promises')) as unknown as {
   __setReadFileHook: (fn?: unknown) => void;
 };
@@ -387,6 +402,11 @@ describe('file I/O', () => {
   });
 
   describe('deleteTeamDirs', () => {
+    afterEach(() => {
+      setFsRmMock(null);
+      vi.restoreAllMocks();
+    });
+
     it('deletes team and task directories', async () => {
       await writeTeamFile('doomed', makeTeamFile());
       const tasksDir = getTasksDir('doomed');
@@ -402,6 +422,46 @@ describe('file I/O', () => {
     it('does not throw for missing directories', async () => {
       await expect(deleteTeamDirs('nonexistent')).resolves.not.toThrow();
     });
+
+    it('throws AggregateError when both rm calls fail (e.g. EACCES)', async () => {
+      const eaccesError = Object.assign(new Error('permission denied'), {
+        code: 'EACCES',
+      });
+      setFsRmMock(() => Promise.reject(eaccesError));
+
+      const err: unknown = await deleteTeamDirs('any-team').then(
+        () => undefined,
+        (e: unknown) => e,
+      );
+      expect(err).toBeInstanceOf(AggregateError);
+      expect((err as AggregateError).errors).toHaveLength(2);
+      // Member errno/path detail must survive in the wrapper message —
+      // serializers reading only `.message`/`.stack` never see `.errors`.
+      expect((err as AggregateError).message).toContain('permission denied');
+    });
+
+    it('throws AggregateError when both rm calls fail (EIO)', async () => {
+      const eioError = Object.assign(new Error('I/O error'), { code: 'EIO' });
+      setFsRmMock(() => Promise.reject(eioError));
+
+      await expect(deleteTeamDirs('any-team')).rejects.toThrow(AggregateError);
+    });
+
+    it('throws the single error when only the second rm call fails', async () => {
+      const eaccesError = Object.assign(new Error('permission denied'), {
+        code: 'EACCES',
+      });
+      let callCount = 0;
+      setFsRmMock(() => {
+        callCount++;
+        if (callCount === 1) return Promise.resolve();
+        return Promise.reject(eaccesError);
+      });
+
+      await expect(deleteTeamDirs('any-team')).rejects.toThrow(
+        'permission denied',
+      );
+    });
   });
 
   describe('tryReclaimStaleTeam', () => {

packages/core/src/agents/team/teamHelpers.ts

@@ -374,16 +374,38 @@ export async function tryReclaimStaleTeam(teamName: string): Promise<boolean> {
 
 /**
  * Delete an entire team directory and its associated task
- * directory. Silently ignores missing directories.
+ * directory. Missing directories are silently ignored because
+ * fs.rm is called with { force: true }.
+ * Throws on real filesystem failures (EACCES, EIO, etc.).
+ * When both removals fail, throws an AggregateError covering both.
  */
 export async function deleteTeamDirs(teamName: string): Promise<void> {
   const teamDir = getTeamDir(teamName);
   const tasksDir = getTasksDir(teamName);
 
-  await Promise.allSettled([
+  const results = await Promise.allSettled([
     fs.rm(teamDir, { recursive: true, force: true }),
     fs.rm(tasksDir, { recursive: true, force: true }),
   ]);
+
+  const errors = results
+    .filter((r): r is PromiseRejectedResult => r.status === 'rejected')
+    .map((r) => r.reason);
+
+  if (errors.length === 1) {
+    throw errors[0];
+  }
+  if (errors.length > 1) {
+    // Fold member messages into the wrapper message: serializers that
+    // only read `.stack`/`.message` (e.g. debugLogger) would otherwise
+    // drop the per-directory errno/path detail of `.errors`.
+    throw new AggregateError(
+      errors,
+      `Failed to delete team directories for "${teamName}": ${errors
+        .map((e) => (e instanceof Error ? e.message : String(e)))
+        .join('; ')}`,
+    );
+  }
 }
 
 /**

packages/core/src/tools/team-delete.test.ts

@@ -9,6 +9,20 @@ import * as fs from 'node:fs/promises';
 import * as os from 'node:os';
 import * as path from 'node:path';
 import { TeamDeleteTool } from './team-delete.js';
+import { deleteTeamDirs } from '../agents/team/teamHelpers.js';
+
+// Mock at the tool boundary so cleanup-failure paths can be injected
+// without relying on real-fs permissions (which root bypasses). Keep the
+// rest of the module real: disposeInboxLocks (mailbox.ts) resolves
+// getInboxesDir through it.
+vi.mock('../agents/team/teamHelpers.js', async (importOriginal) => {
+  const original =
+    await importOriginal<typeof import('../agents/team/teamHelpers.js')>();
+  return {
+    ...original,
+    deleteTeamDirs: vi.fn().mockResolvedValue(undefined),
+  };
+});
 
 vi.mock('../config/storage.js', () => {
   let mockDir = '/tmp/test';
@@ -49,6 +63,8 @@ beforeEach(async () => {
 
 afterEach(async () => {
   await fs.rm(tmpDir, { recursive: true, force: true });
+  vi.mocked(deleteTeamDirs).mockReset();
+  vi.mocked(deleteTeamDirs).mockResolvedValue(undefined);
 });
 
 describe('TeamDeleteTool', () => {
@@ -68,6 +84,54 @@ describe('TeamDeleteTool', () => {
     expect(config.setTeamContext).toHaveBeenCalledWith(null);
   });
 
+  it('resets team state and surfaces failure when directory deletion fails', async () => {
+    // Issue #10210's invariant at the tool boundary: a non-benign
+    // cleanup failure must NOT be converted into complete success,
+    // and it must NOT wedge the session ("team active" forever).
+    const eacces = Object.assign(new Error('EACCES: permission denied'), {
+      code: 'EACCES',
+    });
+    vi.mocked(deleteTeamDirs).mockRejectedValue(eacces);
+
+    const config = makeConfig({ hasManager: true });
+    const result = await new TeamDeleteTool(config)
+      .build({})
+      .execute(new AbortController().signal);
+
+    // The state-reset tail still ran...
+    expect(config.setTeamManager).toHaveBeenCalledWith(null);
+    expect(config.setTeamContext).toHaveBeenCalledWith(null);
+    // ...and the result reports failure instead of complete deletion.
+    expect(result.error).toBeDefined();
+    expect(String(result.llmContent)).toContain('cleanup failed');
+    expect(String(result.llmContent)).toContain('EACCES: permission denied');
+    expect(String(result.llmContent)).not.toContain('deleted.');
+    expect(result.error?.message).toContain('cleanup failed');
+    expect(result.error?.message).toContain('EACCES: permission denied');
+  });
+
+  it('runs the delayed second sweep even when the first sweep fails', async () => {
+    // The second sweep exists to catch the straggler-writeMessage
+    // race; a first-sweep throw must not cancel it. A retry that
+    // succeeds means the directories are genuinely gone.
+    vi.mocked(deleteTeamDirs)
+      .mockRejectedValueOnce(
+        Object.assign(new Error('ENOTEMPTY: directory not empty'), {
+          code: 'ENOTEMPTY',
+        }),
+      )
+      .mockResolvedValueOnce(undefined);
+
+    const config = makeConfig({ hasManager: true });
+    const result = await new TeamDeleteTool(config)
+      .build({})
+      .execute(new AbortController().signal);
+
+    expect(deleteTeamDirs).toHaveBeenCalledTimes(2);
+    expect(result.error).toBeUndefined();
+    expect(result.llmContent).toContain('deleted');
+  });
+
   it('returns error when no team is active', async () => {
     const tool = new TeamDeleteTool(makeConfig());
     const invocation = tool.build({});

packages/core/src/tools/team-delete.ts

@@ -83,9 +83,31 @@ class TeamDeleteInvocation extends BaseToolInvocation<
     // we delete it — leaving an orphan dir that wedges the team
     // name on the next `team_create`. Sweep once more after a
     // short delay to catch the race.
-    await deleteTeamDirs(teamName);
+    //
+    // Filesystem errors (EACCES, EIO, etc.) must NOT prevent the
+    // state-reset tail below — otherwise the session is left
+    // permanently in a "team active" state with no recovery. Each
+    // sweep is wrapped separately so a first-sweep failure cannot
+    // skip the delayed second sweep (the race-catcher above). A
+    // failure is surfaced only if the FINAL sweep still fails: if
+    // the retry succeeds the directories are gone and deletion
+    // genuinely completed.
+    let fsCleanupError: unknown;
+    try {
+      await deleteTeamDirs(teamName);
+    } catch (err) {
+      debug.warn('First cleanup sweep failed; retrying after delay:', err);
+    }
     await new Promise((r) => setTimeout(r, 250));
-    await deleteTeamDirs(teamName);
+    try {
+      await deleteTeamDirs(teamName);
+    } catch (err) {
+      fsCleanupError = err;
+      debug.warn(
+        'Filesystem cleanup failed; resetting team state anyway:',
+        err,
+      );
+    }
 
     // Drop this team's in-process inbox locks now that its inboxes are
     // gone, so the lock map doesn't retain a dead Mutex per inbox for
@@ -97,6 +119,20 @@ class TeamDeleteInvocation extends BaseToolInvocation<
     this.config.setTeamContext(null);
     unregisterLeader();
 
+    if (fsCleanupError) {
+      // State was reset so the session is not wedged, but the cleanup
+      // failure must NOT be converted into a complete-success claim —
+      // directories may remain on disk (issue #10210).
+      const detail =
+        fsCleanupError instanceof Error
+          ? fsCleanupError.message
+          : String(fsCleanupError);
+      const msg =
+        `Team "${teamName}" was torn down, but filesystem cleanup ` +
+        `failed: ${detail}. Team directories may remain on disk.`;
+      return { llmContent: msg, returnDisplay: msg, error: { message: msg } };
+    }
+
     const display: TeamResultDisplay = {
       type: 'team_result',
       teamName,