diff --git a/.changeset/fix-object-mutation.md b/.changeset/fix-object-mutation.md new file mode 100644 index 00000000..bcfc4802 --- /dev/null +++ b/.changeset/fix-object-mutation.md @@ -0,0 +1,5 @@ +--- +"@perstack/runtime": patch +--- + +Avoid object mutation in getSkillManagers and resolveExpertToRun diff --git a/packages/runtime/src/resolve-expert-to-run.test.ts b/packages/runtime/src/resolve-expert-to-run.test.ts index 4c9f0cdd..34393251 100644 --- a/packages/runtime/src/resolve-expert-to-run.test.ts +++ b/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 = {} 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() }) }) diff --git a/packages/runtime/src/resolve-expert-to-run.ts b/packages/runtime/src/resolve-expert-to-run.ts index a7218087..4812c5a2 100644 --- a/packages/runtime/src/resolve-expert-to-run.ts +++ b/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 { diff --git a/packages/runtime/src/setup-experts.test.ts b/packages/runtime/src/setup-experts.test.ts index 5aa78f7a..1e9e79d6 100644 --- a/packages/runtime/src/setup-experts.test.ts +++ b/packages/runtime/src/setup-experts.test.ts @@ -33,10 +33,10 @@ 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, @@ -44,7 +44,7 @@ describe("@perstack/runtime: setupExperts", () => { ) }) - 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, @@ -52,25 +52,27 @@ describe("@perstack/runtime: setupExperts", () => { } 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", diff --git a/packages/runtime/src/setup-experts.ts b/packages/runtime/src/setup-experts.ts index 50185d5c..c31956bb 100644 --- a/packages/runtime/src/setup-experts.ts +++ b/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 } } diff --git a/packages/runtime/src/skill-manager/helpers.ts b/packages/runtime/src/skill-manager/helpers.ts index d4e1d3db..d7a3b264 100644 --- a/packages/runtime/src/skill-manager/helpers.ts +++ b/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)