diff --git a/.changeset/fix-close-skill-managers.md b/.changeset/fix-close-skill-managers.md new file mode 100644 index 00000000..3ba5c6ee --- /dev/null +++ b/.changeset/fix-close-skill-managers.md @@ -0,0 +1,5 @@ +--- +"@perstack/runtime": patch +--- + +Fix closeSkillManagers to continue closing other managers when one fails diff --git a/AGENTS.md b/AGENTS.md index 5cabc6ec..da096e3a 100644 --- a/AGENTS.md +++ b/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 diff --git a/CODE_REVIEW_REPORT.md b/CODE_REVIEW_REPORT.md index 1ee2a825..7f5bee17 100644 --- a/CODE_REVIEW_REPORT.md +++ b/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): Promise { - 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 | diff --git a/packages/runtime/src/skill-manager.test.ts b/packages/runtime/src/skill-manager.test.ts index 9d39780b..591742cd 100644 --- a/packages/runtime/src/skill-manager.test.ts +++ b/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, diff --git a/packages/runtime/src/skill-manager/helpers.ts b/packages/runtime/src/skill-manager/helpers.ts index 859a92f5..f170e714 100644 --- a/packages/runtime/src/skill-manager/helpers.ts +++ b/packages/runtime/src/skill-manager/helpers.ts @@ -83,9 +83,7 @@ export async function getSkillManagers( export async function closeSkillManagers( skillManagers: Record, ): Promise { - 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(