Fix: Handle individual close failures in closeSkillManagers

.changeset/fix-close-skill-managers.md

@@ -0,0 +1,5 @@
+---
+"@perstack/runtime": patch
+---
+
+Fix closeSkillManagers to continue closing other managers when one fails

AGENTS.md

@@ -519,6 +519,17 @@ pnpm build              # Build all packages
 
 **All commands must pass.** If any fails, fix before pushing.
 
+### E2E Testing (MANDATORY)
+
+After build passes, run E2E tests by following `E2E.md`:
+
+```bash
+pnpm build  # Must build first
+# Then run E2E tests as documented in E2E.md
+```
+
+**E2E tests must pass before pushing.** This catches runtime issues that unit tests miss.
+
 ## Expert Definition Format
 
 Experts are defined in `perstack.toml`:
@@ -586,4 +597,5 @@ pick = ["attemptCompletion", "think"]
 - [ ] `pnpm check-deps` passes
 - [ ] `pnpm reset && pnpm test` passes
 - [ ] `pnpm build` passes
+- [ ] E2E tests pass (follow `E2E.md`)
 - [ ] Versioning rules in `CONTRIBUTING.md` are followed

CODE_REVIEW_REPORT.md

@@ -18,7 +18,7 @@ Overall, the codebase is well-structured with strong architectural decisions. Th
 | ✅ Verified   | 5     | Confirmed not an issue / working correctly |
 | 📝 Documented | 1     | Behavior documented, no code change needed |
 | ⏸️ Deferred   | 8     | Low priority / E2E scope / future work     |
-| 🔴 Open       | 6     | Runtime package issues (2025-12-03)        |
+| 🔴 Open       | 5     | Runtime package issues (2025-12-03)        |
 | 🟡 Low Prio   | 4     | Runtime minor issues (2025-12-03)          |
 
 ---
@@ -752,29 +752,16 @@ const runSetting = runSettingSchema.parse(JSON.parse(await fileSystem.readFile(.
 
 ### 39. closeSkillManagers Does Not Handle Individual Close Failures
 
-**Status**: 🔴 **Open**
+**Status**: ✅ **Fixed** — Commit `dac71b5`
 
 **Category**: Code Quality  
 **Severity**: Minor
 
 **Location**: `packages/runtime/src/skill-manager/helpers.ts:83-89`
 
-**Issue**: If one manager's `close()` throws, subsequent managers are not closed:
+**Issue**: If one manager's `close()` throws, subsequent managers are not closed.
 
-```typescript
-export async function closeSkillManagers(skillManagers: Record<string, BaseSkillManager>): Promise<void> {
-  for (const skillManager of Object.values(skillManagers)) {
-    await skillManager.close()  // If this throws, loop breaks
-  }
-}
-```
-
-**Impact**: Orphaned MCP processes if one close fails.
-
-**Recommendation**: Use Promise.allSettled or try-catch:
-```typescript
-await Promise.all(Object.values(skillManagers).map(m => m.close().catch(() => {})))
-```
+**Resolution**: Changed to use `Promise.all` with `.catch()` to ensure all managers are closed even if some fail.
 
 ---
 
@@ -932,7 +919,7 @@ const metaInstruction = dedent`
 | #36   | experts object mutation                | 🔴 Open         |
 | #37   | File operation error handling          | 🔴 Open         |
 | #38   | RunSetting schema validation           | 🔴 Open         |
-| #39   | closeSkillManagers failure handling    | 🔴 Open         |
+| #39   | closeSkillManagers failure handling    | ✅ Fixed        |
 | #40   | EventEmitter listener errors           | 🟡 Low priority |
 | #41   | Nested delegates not resolved          | 🟡 By design?   |
 | #42   | model.ts missing default case          | 🟡 Low priority |
@@ -960,7 +947,6 @@ const metaInstruction = dedent`
 - **#35, #36**: Object mutation side effects — defensive copying recommended
 - **#37**: File read errors unhandled — should feed back to LLM
 - **#38**: RunSetting not validated — use Zod schema
-- **#39**: closeSkillManagers partial failure — use Promise.allSettled
 
 **By Design / Needs Clarification**:
 - **#41**: Nested delegates not pre-resolved — document if intentional
@@ -985,3 +971,4 @@ const metaInstruction = dedent`
 | `f7edeb9` | Refactor: Use discriminatedUnion for provider settings             |
 | `5490ebb` | Refactor: Split SkillManager into separate classes                 |
 | `ced1aa6` | Fix: maxSteps off-by-one error in finishing step                   |
+| `dac71b5` | Fix: Handle individual close failures in closeSkillManagers        |

packages/runtime/src/skill-manager.test.ts

@@ -319,8 +319,20 @@ describe("@perstack/runtime: DelegateSkillManager", () => {
 
 describe("@perstack/runtime: closeSkillManagers", () => {
   it("closes all skill managers", async () => {
-    const closeFn1 = vi.fn()
-    const closeFn2 = vi.fn()
+    const closeFn1 = vi.fn().mockResolvedValue(undefined)
+    const closeFn2 = vi.fn().mockResolvedValue(undefined)
+    const skillManagers = {
+      skill1: { close: closeFn1 } as unknown as BaseSkillManager,
+      skill2: { close: closeFn2 } as unknown as BaseSkillManager,
+    }
+    await closeSkillManagers(skillManagers)
+    expect(closeFn1).toHaveBeenCalledTimes(1)
+    expect(closeFn2).toHaveBeenCalledTimes(1)
+  })
+
+  it("continues closing other managers when one fails", async () => {
+    const closeFn1 = vi.fn().mockRejectedValue(new Error("close failed"))
+    const closeFn2 = vi.fn().mockResolvedValue(undefined)
     const skillManagers = {
       skill1: { close: closeFn1 } as unknown as BaseSkillManager,
       skill2: { close: closeFn2 } as unknown as BaseSkillManager,

packages/runtime/src/skill-manager/helpers.ts

@@ -83,9 +83,7 @@ export async function getSkillManagers(
 export async function closeSkillManagers(
   skillManagers: Record<string, BaseSkillManager>,
 ): Promise<void> {
-  for (const skillManager of Object.values(skillManagers)) {
-    await skillManager.close()
-  }
+  await Promise.all(Object.values(skillManagers).map((m) => m.close().catch(() => {})))
 }
 
 export async function getSkillManagerByToolName(