Fix: Avoid object mutation in skill manager and expert resolution

.changeset/fix-object-mutation.md

@@ -0,0 +1,5 @@
+---
+"@perstack/runtime": patch
+---
+
+Avoid object mutation in getSkillManagers and resolveExpertToRun

packages/runtime/src/resolve-expert-to-run.test.ts

@@ -78,7 +78,6 @@ describe("@perstack/runtime: resolveExpertToRun", () => {
     })
     expect(result.key).toBe("remote-expert")
     expect(result.name).toBe("Remote Expert")
-    expect(experts["remote-expert"]).toBeDefined()
   })
 
   it("adds name to skills when converting from API", async () => {
@@ -89,13 +88,12 @@ describe("@perstack/runtime: resolveExpertToRun", () => {
     expect(result.skills["@perstack/base"].name).toBe("@perstack/base")
   })
 
-  it("caches fetched expert in experts record", async () => {
+  it("does not mutate experts record", async () => {
     const experts: Record<string, Expert> = {}
     await resolveExpertToRun("remote-expert", experts, {
       perstackApiBaseUrl: "https://api.test.com",
     })
-    expect(experts["remote-expert"]).toBeDefined()
-    expect(experts["remote-expert"].key).toBe("remote-expert")
+    expect(experts["remote-expert"]).toBeUndefined()
   })
 })
 

packages/runtime/src/resolve-expert-to-run.ts

@@ -17,8 +17,7 @@ export async function resolveExpertToRun(
     apiKey: clientOptions.perstackApiKey,
   })
   const { expert } = await client.registry.experts.get({ expertKey })
-  experts[expertKey] = toRuntimeExpert(expert)
-  return experts[expertKey]
+  return toRuntimeExpert(expert)
 }
 
 function toRuntimeExpert(expert: ApiRegistryExpert): Expert {

packages/runtime/src/setup-experts.test.ts

@@ -33,44 +33,46 @@ describe("@perstack/runtime: setupExperts", () => {
     const mockResolve = vi.fn().mockResolvedValue(baseExpert)
     const result = await setupExperts(baseSetting, mockResolve)
     expect(result.expertToRun).toEqual(baseExpert)
-    expect(result.experts).toEqual({})
+    expect(result.experts).toEqual({ "test-expert": baseExpert })
     expect(mockResolve).toHaveBeenCalledWith(
       "test-expert",
-      {},
+      expect.any(Object),
       {
         perstackApiBaseUrl: "https://api.perstack.dev",
         perstackApiKey: undefined,
       },
     )
   })
 
-  it("copies experts from setting", async () => {
+  it("copies experts from setting and does not mutate original", async () => {
     const existingExpert: Expert = { ...baseExpert, key: "existing", name: "existing" }
     const settingWithExperts = {
       ...baseSetting,
       experts: { existing: existingExpert },
     }
     const mockResolve = vi.fn().mockResolvedValue(baseExpert)
     const result = await setupExperts(settingWithExperts, mockResolve)
-    expect(result.experts).toEqual({ existing: existingExpert })
+    expect(result.experts).toEqual({ existing: existingExpert, "test-expert": baseExpert })
     expect(result.experts).not.toBe(settingWithExperts.experts)
+    expect(settingWithExperts.experts).toEqual({ existing: existingExpert })
   })
 
-  it("resolves all delegates", async () => {
+  it("resolves all delegates and adds them to experts map", async () => {
     const expertWithDelegates: Expert = {
       ...baseExpert,
       delegates: ["delegate-1", "delegate-2"],
     }
-    const delegateExpert: Expert = { ...baseExpert, key: "delegate", name: "delegate" }
+    const delegateExpert1: Expert = { ...baseExpert, key: "delegate-1", name: "delegate-1" }
+    const delegateExpert2: Expert = { ...baseExpert, key: "delegate-2", name: "delegate-2" }
     const mockResolve = vi
       .fn()
       .mockResolvedValueOnce(expertWithDelegates)
-      .mockResolvedValueOnce(delegateExpert)
-      .mockResolvedValueOnce(delegateExpert)
-    await setupExperts(baseSetting, mockResolve)
+      .mockResolvedValueOnce(delegateExpert1)
+      .mockResolvedValueOnce(delegateExpert2)
+    const result = await setupExperts(baseSetting, mockResolve)
     expect(mockResolve).toHaveBeenCalledTimes(3)
-    expect(mockResolve).toHaveBeenNthCalledWith(2, "delegate-1", {}, expect.any(Object))
-    expect(mockResolve).toHaveBeenNthCalledWith(3, "delegate-2", {}, expect.any(Object))
+    expect(result.experts["delegate-1"]).toEqual(delegateExpert1)
+    expect(result.experts["delegate-2"]).toEqual(delegateExpert2)
   })
 
   it("throws when delegate resolution returns falsy", async () => {
@@ -96,7 +98,7 @@ describe("@perstack/runtime: setupExperts", () => {
     await setupExperts(settingWithKey, mockResolve)
     expect(mockResolve).toHaveBeenCalledWith(
       "test-expert",
-      {},
+      expect.any(Object),
       {
         perstackApiBaseUrl: "https://api.perstack.dev",
         perstackApiKey: "my-api-key",

packages/runtime/src/setup-experts.ts

@@ -23,11 +23,13 @@ export async function setupExperts(
     perstackApiKey: setting.perstackApiKey,
   }
   const expertToRun = await resolveExpertToRun(expertKey, experts, clientOptions)
+  experts[expertKey] = expertToRun
   for (const delegateName of expertToRun.delegates) {
     const delegate = await resolveExpertToRun(delegateName, experts, clientOptions)
     if (!delegate) {
       throw new Error(`Delegate ${delegateName} not found`)
     }
+    experts[delegateName] = delegate
   }
   return { expertToRun, experts }
 }

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

@@ -29,28 +29,32 @@ export async function getSkillManagers(
     throw new Error("Base skill is not defined")
   }
   const allManagers: BaseSkillManager[] = []
-  const mcpSkills = Object.values(skills).filter(
-    (skill) => skill.type === "mcpStdioSkill" || skill.type === "mcpSseSkill",
-  )
-  for (const skill of mcpSkills) {
-    if (perstackBaseSkillCommand && skill.type === "mcpStdioSkill") {
-      const matchesBaseByPackage = skill.command === "npx" && skill.packageName === "@perstack/base"
-      const matchesBaseByArgs =
-        skill.command === "npx" &&
-        Array.isArray(skill.args) &&
-        skill.args.includes("@perstack/base")
-      if (matchesBaseByPackage || matchesBaseByArgs) {
-        const [overrideCommand, ...overrideArgs] = perstackBaseSkillCommand
-        if (!overrideCommand) {
-          throw new Error("perstackBaseSkillCommand must have at least one element")
+  const mcpSkills = Object.values(skills)
+    .filter((skill) => skill.type === "mcpStdioSkill" || skill.type === "mcpSseSkill")
+    .map((skill) => {
+      if (perstackBaseSkillCommand && skill.type === "mcpStdioSkill") {
+        const matchesBaseByPackage =
+          skill.command === "npx" && skill.packageName === "@perstack/base"
+        const matchesBaseByArgs =
+          skill.command === "npx" &&
+          Array.isArray(skill.args) &&
+          skill.args.includes("@perstack/base")
+        if (matchesBaseByPackage || matchesBaseByArgs) {
+          const [overrideCommand, ...overrideArgs] = perstackBaseSkillCommand
+          if (!overrideCommand) {
+            throw new Error("perstackBaseSkillCommand must have at least one element")
+          }
+          return {
+            ...skill,
+            command: overrideCommand,
+            packageName: undefined,
+            args: overrideArgs,
+            lazyInit: false,
+          }
         }
-        skill.command = overrideCommand
-        skill.packageName = undefined
-        skill.args = overrideArgs
-        skill.lazyInit = false
       }
-    }
-  }
+      return skill
+    })
   const mcpSkillManagers = mcpSkills.map((skill) => {
     const manager = new McpSkillManager(skill, env, runId, eventListener)
     allManagers.push(manager)