diff --git a/cli/src/__tests__/agent-lifecycle.test.ts b/cli/src/__tests__/agent-lifecycle.test.ts index 4e2917ce83e..41817823db9 100644 --- a/cli/src/__tests__/agent-lifecycle.test.ts +++ b/cli/src/__tests__/agent-lifecycle.test.ts @@ -83,7 +83,15 @@ describe("agent lifecycle commands", () => { await run(["agent", "runtime-state:reset-session", AGENT_ID, "--task-key", "task-1"]); await run(["agent", "task-sessions", AGENT_ID]); await run(["agent", "skills", AGENT_ID]); - await run(["agent", "skills:sync", AGENT_ID, "--desired-skills", "paperclip,github"]); + await run([ + "agent", + "skills:sync", + AGENT_ID, + "--desired-skills", + "paperclip,github", + "--mode", + "replace", + ]); await run(["agent", "instructions-path:update", AGENT_ID, "--payload-json", JSON.stringify({ path: "/tmp/AGENTS.md" })]); await run(["agent", "instructions-bundle", AGENT_ID]); await run(["agent", "instructions-bundle:update", AGENT_ID, "--payload-json", JSON.stringify({ mode: "managed" })]); diff --git a/cli/src/__tests__/company.test.ts b/cli/src/__tests__/company.test.ts index 1532047f6f8..a548c8821a4 100644 --- a/cli/src/__tests__/company.test.ts +++ b/cli/src/__tests__/company.test.ts @@ -504,6 +504,17 @@ describe("renderCompanyImportResult", () => { { slug: "cto", id: "agent-2", action: "updated", name: "CTO", reason: "replace strategy" }, { slug: "ops", id: null, action: "skipped", name: "Ops", reason: "skip strategy" }, ], + skills: [ + { + originalKey: "company/source/review", + originalSlug: "review", + key: "company/target/review-2", + slug: "review-2", + id: "skill-1", + action: "renamed", + reason: "rename strategy", + }, + ], projects: [ { slug: "app", id: "project-1", action: "created", name: "App", reason: null }, { slug: "ops", id: "project-2", action: "updated", name: "Operations", reason: "replace strategy" }, @@ -525,8 +536,10 @@ describe("renderCompanyImportResult", () => { expect(rendered).toContain("Company"); expect(rendered).toContain("https://paperclip.example/PAP/dashboard"); expect(rendered).toContain("3 agents total (1 created, 1 updated, 1 skipped)"); + expect(rendered).toContain("1 skill total (1 renamed)"); expect(rendered).toContain("3 projects total (1 created, 1 updated, 1 skipped)"); expect(rendered).toContain("Agent results"); + expect(rendered).toContain("Skill results"); expect(rendered).toContain("Project results"); expect(rendered).toContain("Using claude-local adapter"); expect(rendered).toContain("Review API keys"); diff --git a/cli/src/__tests__/skills.test.ts b/cli/src/__tests__/skills.test.ts index 038b9812f1b..80d0dd3c791 100644 --- a/cli/src/__tests__/skills.test.ts +++ b/cli/src/__tests__/skills.test.ts @@ -479,6 +479,8 @@ describe("skills CLI commands", () => { "review-prs", "--skill", "paperclip/qa", + "--mode", + "add", "--company-id", "company-1", "--api-base", @@ -498,7 +500,7 @@ describe("skills CLI commands", () => { "http://paperclip.test/api/agents/agent-1/skills/sync", expect.objectContaining({ method: "POST", - body: JSON.stringify({ desiredSkills: ["review-prs", "paperclip/qa"] }), + body: JSON.stringify({ desiredSkills: ["review-prs", "paperclip/qa"], mode: "add" }), }), ); expect(JSON.parse(String(logSpy.mock.calls[0]?.[0]))).toEqual(snapshot); diff --git a/cli/src/commands/client/agent.ts b/cli/src/commands/client/agent.ts index 8144352c491..6d6677e4244 100644 --- a/cli/src/commands/client/agent.ts +++ b/cli/src/commands/client/agent.ts @@ -71,6 +71,7 @@ interface AgentResetSessionOptions extends BaseClientOptions { interface AgentSkillsSyncOptions extends BaseClientOptions { desiredSkills: string; + mode: string; } interface AgentInstructionsFileOptions extends BaseClientOptions { @@ -589,10 +590,17 @@ export function registerAgentCommands(program: Command): void { .description("Sync desired skills onto an agent") .argument("", "Agent ID") .requiredOption("--desired-skills ", "Desired skill names") + .requiredOption( + "--mode ", + "Merge mode: add keeps other skills; remove deletes only named skills; replace destructively overwrites the complete set", + ) .action(async (agentId: string, opts: AgentSkillsSyncOptions) => { try { const ctx = resolveCommandContext(opts); - const payload = agentSkillSyncSchema.parse({ desiredSkills: parseCsv(opts.desiredSkills) }); + const payload = agentSkillSyncSchema.parse({ + desiredSkills: parseCsv(opts.desiredSkills), + mode: opts.mode, + }); const result = await ctx.api.post(apiPath`/api/agents/${agentId}/skills/sync`, payload); printOutput(result, { json: ctx.json }); } catch (err) { diff --git a/cli/src/commands/client/company.ts b/cli/src/commands/client/company.ts index bd0d162a502..72864f6482c 100644 --- a/cli/src/commands/client/company.ts +++ b/cli/src/commands/client/company.ts @@ -548,6 +548,16 @@ function summarizeImportAgentResults(agents: CompanyPortabilityImportResult["age return `${agents.length} ${pluralize(agents.length, "agent")} total (${parts.join(", ")})`; } +function summarizeImportSkillResults(skills: CompanyPortabilityImportResult["skills"]): string { + if (skills.length === 0) return "0 skills changed"; + const actions = ["created", "renamed", "replaced", "skipped"] as const; + const parts = actions.flatMap((action) => { + const count = skills.filter((skill) => skill.action === action).length; + return count > 0 ? [`${count} ${action}`] : []; + }); + return `${skills.length} ${pluralize(skills.length, "skill")} total (${parts.join(", ")})`; +} + function summarizeImportProjectResults(projects: CompanyPortabilityImportResult["projects"]): string { if (projects.length === 0) return "0 projects changed"; const created = projects.filter((project) => project.action === "created").length; @@ -681,10 +691,12 @@ export function renderCompanyImportResult( result: CompanyPortabilityImportResult, meta: { targetLabel: string; companyUrl?: string; infoMessages?: string[] }, ): string { + const skills = result.skills ?? []; const lines: string[] = [ `${pc.bold("Target")} ${meta.targetLabel}`, `${pc.bold("Company")} ${result.company.name} (${actionChip(result.company.action)})`, `${pc.bold("Agents")} ${summarizeImportAgentResults(result.agents)}`, + `${pc.bold("Skills")} ${summarizeImportSkillResults(skills)}`, `${pc.bold("Projects")} ${summarizeImportProjectResults(result.projects)}`, ]; @@ -701,6 +713,15 @@ export function renderCompanyImportResult( reason: agent.reason, })), ); + appendPreviewExamples( + lines, + "Skill results", + skills.map((skill) => ({ + action: skill.action, + label: `${skill.originalSlug} -> ${skill.slug}`, + reason: skill.reason, + })), + ); appendPreviewExamples( lines, "Project results", diff --git a/cli/src/commands/client/skills.ts b/cli/src/commands/client/skills.ts index c97ebd18df1..f24316c9c49 100644 --- a/cli/src/commands/client/skills.ts +++ b/cli/src/commands/client/skills.ts @@ -1,17 +1,19 @@ import { Command } from "commander"; -import type { - Agent, - AgentSkillSnapshot, - CatalogSkill, - CompanySkill, - CompanySkillAuditResult, - CompanySkillDetail, - CompanySkillFileDetail, - CompanySkillImportResult, - CompanySkillInstallCatalogResult, - CompanySkillListItem, - CompanySkillProjectScanResult, - CompanySkillUpdateStatus, +import { + agentSkillAssignmentModeSchema, + type AgentSkillAssignmentMode, + type Agent, + type AgentSkillSnapshot, + type CatalogSkill, + type CompanySkill, + type CompanySkillAuditResult, + type CompanySkillDetail, + type CompanySkillFileDetail, + type CompanySkillImportResult, + type CompanySkillInstallCatalogResult, + type CompanySkillListItem, + type CompanySkillProjectScanResult, + type CompanySkillUpdateStatus, } from "@paperclipai/shared"; import { readFile } from "node:fs/promises"; import { stdin as input, stdout as output } from "node:process"; @@ -69,6 +71,7 @@ interface ConfirmedSkillOptions extends SkillsOptions { interface AgentSkillSyncOptions extends SkillsOptions { skill?: string[]; + mode: AgentSkillAssignmentMode; } type CompanySkillReferenceTarget = Pick; @@ -502,9 +505,13 @@ function registerAgentSkillCommands(skills: Command): void { addCommonClientOptions( agent .command("sync") - .description("Replace an agent's desired company skills and sync runtime state") + .description("Merge an agent's desired company skills and sync runtime state") .argument("", "Agent ID or shortname/url-key") .option("--skill ", "Desired company skill ID, key, or slug; may be repeated", collectOptionValue, [] as string[]) + .requiredOption( + "--mode ", + "Merge mode: add keeps other skills; remove deletes only named skills; replace destructively overwrites the complete set", + ) .action(async (agentRef: string, opts: AgentSkillSyncOptions) => { try { const desiredSkills = opts.skill ?? []; @@ -513,16 +520,17 @@ function registerAgentSkillCommands(skills: Command): void { } const ctx = resolveCommandContext(opts, { requireCompany: true }); const agentRow = await resolveAgent(ctx, agentRef); + const mode = agentSkillAssignmentModeSchema.parse(opts.mode); const snapshot = await ctx.api.post( `/api/agents/${encodeURIComponent(agentRow.id)}/skills/sync`, - { desiredSkills }, + { desiredSkills, mode }, ); if (ctx.json) { printOutput(snapshot, { json: true }); return; } console.log( - `Desired company skills replaced for ${agentRow.name} (${agentRow.id}); runtime sync returned ${snapshot?.entries.length ?? 0} entrie(s).`, + `Desired company skills updated with ${mode} mode for ${agentRow.name} (${agentRow.id}); runtime sync returned ${snapshot?.entries.length ?? 0} entrie(s).`, ); printAgentSkillSnapshot(snapshot, agentRow); } catch (err) { @@ -548,7 +556,7 @@ function registerAgentSkillCommands(skills: Command): void { ); const snapshot = await ctx.api.post( `/api/agents/${encodeURIComponent(agentRow.id)}/skills/sync`, - { desiredSkills: [] }, + { desiredSkills: [], mode: "replace" }, ); if (ctx.json) { printOutput(snapshot, { json: true }); diff --git a/doc/CLI.md b/doc/CLI.md index 5b36f799589..5c75aeb4e85 100644 --- a/doc/CLI.md +++ b/doc/CLI.md @@ -365,7 +365,7 @@ pnpm paperclipai agent runtime-state pnpm paperclipai agent runtime-state:reset-session [--task-key ] pnpm paperclipai agent task-sessions pnpm paperclipai agent skills -pnpm paperclipai agent skills:sync --desired-skills paperclip,github +pnpm paperclipai agent skills:sync --desired-skills paperclip,github --mode add pnpm paperclipai agent instructions-path:update --payload-json '{"path":"/path/to/AGENTS.md"}' pnpm paperclipai agent instructions-bundle pnpm paperclipai agent instructions-bundle:update --payload-json '{"mode":"managed"}' @@ -465,9 +465,10 @@ By default the command creates a `todo` issue assigned to the target agent and w 1. **Company install** — adds or updates a row in `company_skills` for the whole company. This is what `skills install`, `skills import`, `skills create`, and `skills scan-projects` do. -2. **Agent attach** — replaces an agent's *desired* company skill set - (`skills agent sync`/`clear`). This is a desired-state operation on the - agent's adapter config; it does not change the company library. +2. **Agent attach** — merges an agent's *desired* company skill set with an + explicit `add`, `remove`, or `replace` mode (`skills agent sync`/`clear`). + This is a desired-state operation on the agent's adapter config; it does not + change the company library. 3. **Adapter runtime sync** — the adapter reconciles the desired skill set with files on disk and reports an `AgentSkillSnapshot` (`skills agent list`). `skills agent sync` triggers this automatically after updating desired state. @@ -564,12 +565,14 @@ maintenance loop for catalog-installed skills: ```sh pnpm paperclipai skills agent list --company-id -pnpm paperclipai skills agent sync --skill [--skill ...] --company-id +pnpm paperclipai skills agent sync --skill [--skill ...] --mode --company-id pnpm paperclipai skills agent clear --yes --company-id ``` -`skills agent sync` replaces the agent's non-required desired skill set (it is -not additive) and returns the resulting adapter `AgentSkillSnapshot`. +`skills agent sync` requires a merge mode and returns the resulting adapter +`AgentSkillSnapshot`. `add` preserves all unnamed assignments, `remove` deletes +only named assignments, and `replace` destructively overwrites the complete +non-required desired skill set. `skills agent clear` sends an empty desired list. Required Paperclip skills are still enforced by the server in both cases. diff --git a/docs/cli/control-plane-commands.md b/docs/cli/control-plane-commands.md index 406b3190bd7..70adf99ad24 100644 --- a/docs/cli/control-plane-commands.md +++ b/docs/cli/control-plane-commands.md @@ -91,7 +91,7 @@ pnpm paperclipai skills import ./skills/my-skill --company-id pnpm paperclipai skills import owner/repo/path/to/skill --company-id # Attach desired company skills to an agent after install/import -pnpm paperclipai skills agent sync --skill github-pr-workflow --company-id +pnpm paperclipai skills agent sync --skill github-pr-workflow --mode add --company-id ``` ## Approval Commands diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index cbb2b9e49bf..b4ce278490c 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -764,6 +764,7 @@ export type { CatalogTeamInstallResult, InstalledCatalogTeam, AgentSkillSyncMode, + AgentSkillAssignmentMode, AgentSkillState, AgentSkillOrigin, AgentDesiredSkillEntry, @@ -1595,6 +1596,7 @@ export { type ProbeEnvironmentConfig, agentSkillStateSchema, agentSkillSyncModeSchema, + agentSkillAssignmentModeSchema, agentDesiredSkillEntrySchema, agentDesiredSkillSelectionSchema, agentSkillEntrySchema, diff --git a/packages/shared/src/types/adapter-skills.ts b/packages/shared/src/types/adapter-skills.ts index 998dda9a54d..ac77f7e013e 100644 --- a/packages/shared/src/types/adapter-skills.ts +++ b/packages/shared/src/types/adapter-skills.ts @@ -1,5 +1,7 @@ export type AgentSkillSyncMode = "unsupported" | "persistent" | "ephemeral"; +export type AgentSkillAssignmentMode = "add" | "remove" | "replace"; + export type AgentSkillState = | "available" | "configured" @@ -46,5 +48,6 @@ export interface AgentSkillSnapshot { } export interface AgentSkillSyncRequest { + mode: AgentSkillAssignmentMode; desiredSkills: Array; } diff --git a/packages/shared/src/types/company-portability.ts b/packages/shared/src/types/company-portability.ts index b43afdda2e3..3cc0eb4a6f0 100644 --- a/packages/shared/src/types/company-portability.ts +++ b/packages/shared/src/types/company-portability.ts @@ -402,6 +402,15 @@ export interface CompanyPortabilityImportResult { name: string; reason: string | null; }[]; + skills: { + originalKey: string; + originalSlug: string; + key: string; + slug: string; + id: string; + action: "created" | "renamed" | "replaced" | "skipped"; + reason: string | null; + }[]; projects: { slug: string; id: string | null; diff --git a/packages/shared/src/types/index.ts b/packages/shared/src/types/index.ts index 6027c15d39e..e23ae3daeb7 100644 --- a/packages/shared/src/types/index.ts +++ b/packages/shared/src/types/index.ts @@ -250,6 +250,7 @@ export type { } from "./teams-catalog.js"; export type { AgentSkillSyncMode, + AgentSkillAssignmentMode, AgentSkillState, AgentSkillOrigin, AgentDesiredSkillEntry, diff --git a/packages/shared/src/validators/adapter-skills.ts b/packages/shared/src/validators/adapter-skills.ts index 96c0c4998d5..d0befc91103 100644 --- a/packages/shared/src/validators/adapter-skills.ts +++ b/packages/shared/src/validators/adapter-skills.ts @@ -21,6 +21,12 @@ export const agentSkillSyncModeSchema = z.enum([ "ephemeral", ]); +export const agentSkillAssignmentModeSchema = z.enum([ + "add", + "remove", + "replace", +]); + export const agentDesiredSkillEntrySchema = z.object({ key: z.string().min(1), versionId: z.string().uuid().nullable(), @@ -59,6 +65,7 @@ export const agentSkillSnapshotSchema = z.object({ }); export const agentSkillSyncSchema = z.object({ + mode: agentSkillAssignmentModeSchema, desiredSkills: z.array(agentDesiredSkillSelectionSchema), }); diff --git a/packages/shared/src/validators/index.ts b/packages/shared/src/validators/index.ts index d93c6649dd6..b0923b60848 100644 --- a/packages/shared/src/validators/index.ts +++ b/packages/shared/src/validators/index.ts @@ -282,6 +282,7 @@ export { export { agentSkillStateSchema, agentSkillSyncModeSchema, + agentSkillAssignmentModeSchema, agentDesiredSkillEntrySchema, agentDesiredSkillSelectionSchema, agentSkillEntrySchema, diff --git a/server/src/__tests__/agent-skills-routes.test.ts b/server/src/__tests__/agent-skills-routes.test.ts index db6df76cc17..6dd77801ed1 100644 --- a/server/src/__tests__/agent-skills-routes.test.ts +++ b/server/src/__tests__/agent-skills-routes.test.ts @@ -488,7 +488,7 @@ describe.sequential("agent skill routes", () => { await createApp(), (baseUrl) => request(baseUrl) .post("/api/agents/11111111-1111-4111-8111-111111111111/skills/sync?companyId=company-1") - .send({ desiredSkills: ["paperclip"] }), + .send({ desiredSkills: ["paperclip"], mode: "replace" }), ); expect(syncRes.status, JSON.stringify(syncRes.body)).toBe(200); const syncCall = mockSecretService.resolveAdapterConfigForRuntime.mock.calls.at(-1); @@ -588,7 +588,7 @@ describe.sequential("agent skill routes", () => { const res = await requestApp(await createApp(), (baseUrl) => request(baseUrl) .post("/api/agents/11111111-1111-4111-8111-111111111111/skills/sync?companyId=company-1") - .send({ desiredSkills: ["paperclip"] })); + .send({ desiredSkills: ["paperclip"], mode: "replace" })); expect(res.status, JSON.stringify(res.body)).toBe(200); expect(mockAgentService.update).toHaveBeenCalledWith( @@ -615,12 +615,102 @@ describe.sequential("agent skill routes", () => { ); }); + it("requires an explicit actionable merge mode for skill sync", async () => { + const res = await requestApp(await createApp(), (baseUrl) => request(baseUrl) + .post("/api/agents/11111111-1111-4111-8111-111111111111/skills/sync?companyId=company-1") + .send({ desiredSkills: ["paperclip"] })); + + expect(res.status, JSON.stringify(res.body)).toBe(422); + expect(res.body.error).toContain('"add", "remove", or "replace"'); + expect(res.body.error).toContain('"replace" only to overwrite'); + expect(mockAgentService.update).not.toHaveBeenCalled(); + }); + + it("adds only named desired skills while preserving existing assignments", async () => { + mockAgentService.getById.mockResolvedValue({ + ...makeAgent("claude_local"), + adapterConfig: { + paperclipSkillSync: { desiredSkills: ["company-1/keep"] }, + }, + }); + + const res = await requestApp(await createApp(), (baseUrl) => request(baseUrl) + .post("/api/agents/11111111-1111-4111-8111-111111111111/skills/sync?companyId=company-1") + .send({ desiredSkills: ["paperclip"], mode: "add" })); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockAgentService.update).toHaveBeenCalledWith( + expect.any(String), + expect.objectContaining({ + adapterConfig: expect.objectContaining({ + paperclipSkillSync: { + desiredSkills: ["company-1/keep", "paperclipai/paperclip/paperclip"], + }, + }), + }), + expect.any(Object), + ); + }); + + it("removes only named desired skills while preserving other assignments", async () => { + mockAgentService.getById.mockResolvedValue({ + ...makeAgent("claude_local"), + adapterConfig: { + paperclipSkillSync: { + desiredSkills: ["company-1/keep", "paperclipai/paperclip/paperclip"], + }, + }, + }); + + const res = await requestApp(await createApp(), (baseUrl) => request(baseUrl) + .post("/api/agents/11111111-1111-4111-8111-111111111111/skills/sync?companyId=company-1") + .send({ desiredSkills: ["paperclip"], mode: "remove" })); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockAgentService.update).toHaveBeenCalledWith( + expect.any(String), + expect.objectContaining({ + adapterConfig: expect.objectContaining({ + paperclipSkillSync: { desiredSkills: ["company-1/keep"] }, + }), + }), + expect.any(Object), + ); + }); + + it("replaces the complete desired skill set only when explicitly requested", async () => { + mockAgentService.getById.mockResolvedValue({ + ...makeAgent("claude_local"), + adapterConfig: { + paperclipSkillSync: { desiredSkills: ["company-1/keep"] }, + }, + }); + + const res = await requestApp(await createApp(), (baseUrl) => request(baseUrl) + .post("/api/agents/11111111-1111-4111-8111-111111111111/skills/sync?companyId=company-1") + .send({ desiredSkills: ["paperclip"], mode: "replace" })); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockAgentService.update).toHaveBeenCalledWith( + expect.any(String), + expect.objectContaining({ + adapterConfig: expect.objectContaining({ + paperclipSkillSync: { + desiredSkills: ["paperclipai/paperclip/paperclip"], + }, + }), + }), + expect.any(Object), + ); + }); + it("rejects version pins while beta skills are disabled", async () => { mockAgentService.getById.mockResolvedValue(makeAgent("claude_local")); const res = await requestApp(await createApp(), (baseUrl) => request(baseUrl) .post("/api/agents/11111111-1111-4111-8111-111111111111/skills/sync?companyId=company-1") .send({ + mode: "replace", desiredSkills: [{ key: "paperclipai/paperclip/paperclip", versionId: "22222222-2222-4222-8222-222222222222", @@ -640,6 +730,7 @@ describe.sequential("agent skill routes", () => { const res = await requestApp(await createApp(), (baseUrl) => request(baseUrl) .post("/api/agents/11111111-1111-4111-8111-111111111111/skills/sync?companyId=company-1") .send({ + mode: "replace", desiredSkills: [{ key: "paperclipai/paperclip/paperclip", versionId }], })); @@ -687,7 +778,7 @@ describe.sequential("agent skill routes", () => { const res = await requestApp(await createApp(), (baseUrl) => request(baseUrl) .post("/api/agents/11111111-1111-4111-8111-111111111111/skills/sync?companyId=company-1") - .send({ desiredSkills: ["paperclip", "stale/removed/skill"] })); + .send({ desiredSkills: ["paperclip", "stale/removed/skill"], mode: "replace" })); expect(res.status, JSON.stringify(res.body)).toBe(200); // Stale key preserved in the persisted config alongside the resolved skill. @@ -741,7 +832,7 @@ describe.sequential("agent skill routes", () => { const res = await requestApp(await createApp(), (baseUrl) => request(baseUrl) .post("/api/agents/11111111-1111-4111-8111-111111111111/skills/sync?companyId=company-1") - .send({ desiredSkills: ["paperclipai/paperclip/paperclip"] })); + .send({ desiredSkills: ["paperclipai/paperclip/paperclip"], mode: "replace" })); expect(res.status, JSON.stringify(res.body)).toBe(200); expect(mockAdapter.syncSkills).toHaveBeenCalled(); @@ -796,7 +887,7 @@ describe.sequential("agent skill routes", () => { const res = await requestApp(await createApp(), (baseUrl) => request(baseUrl) .post("/api/agents/11111111-1111-4111-8111-111111111111/skills/sync?companyId=company-1") - .send({ desiredSkills: ["paperclipai/paperclip/paperclip"] })); + .send({ desiredSkills: ["paperclipai/paperclip/paperclip"], mode: "replace" })); expect(res.status, JSON.stringify(res.body)).toBe(200); expect(mockAdapter.syncSkills).toHaveBeenCalledWith( @@ -816,7 +907,7 @@ describe.sequential("agent skill routes", () => { const res = await requestApp(await createApp(), (baseUrl) => request(baseUrl) .post("/api/agents/11111111-1111-4111-8111-111111111111/skills/sync?companyId=company-1") - .send({ desiredSkills: ["paperclip"] })); + .send({ desiredSkills: ["paperclip"], mode: "replace" })); expect(res.status, JSON.stringify(res.body)).toBe(200); expect(mockAgentService.update).toHaveBeenCalledWith( diff --git a/server/src/__tests__/company-portability.test.ts b/server/src/__tests__/company-portability.test.ts index 63c3a149438..9631ddd3b7b 100644 --- a/server/src/__tests__/company-portability.test.ts +++ b/server/src/__tests__/company-portability.test.ts @@ -3005,6 +3005,18 @@ describe("company portability", () => { id: "agent-created", name: "ClaudeCoder", }); + companySkillSvc.importPackageFiles.mockResolvedValueOnce([{ + skill: { + id: "skill-imported", + key: paperclipKey, + slug: "paperclip", + }, + action: "renamed", + originalKey: "paperclip", + originalSlug: "paperclip", + requestedRefs: ["paperclip"], + reason: "Existing skill matched; renamed to paperclip-2.", + }]); const exported = await portability.exportBundle("company-1", { include: { @@ -3017,7 +3029,7 @@ describe("company portability", () => { agentSvc.list.mockResolvedValue([]); - await portability.importBundle({ + const result = await portability.importBundle({ source: { type: "inline", rootPath: exported.rootPath, @@ -3039,8 +3051,17 @@ describe("company portability", () => { const textOnlyFiles = Object.fromEntries(Object.entries(exported.files).filter(([, v]) => typeof v === "string")); expect(companySkillSvc.importPackageFiles).toHaveBeenCalledWith("company-imported", textOnlyFiles, { - onConflict: "replace", + onConflict: "rename", }); + expect(result.skills).toEqual([{ + originalKey: "paperclip", + originalSlug: "paperclip", + key: paperclipKey, + slug: "paperclip", + id: "skill-imported", + action: "renamed", + reason: "Existing skill matched; renamed to paperclip-2.", + }]); expect(agentSvc.create).toHaveBeenCalledWith("company-imported", expect.objectContaining({ adapterConfig: expect.objectContaining({ paperclipSkillSync: { @@ -3301,7 +3322,7 @@ describe("company portability", () => { "agents/cmo/AGENTS.md": expect.any(String), }), { - onConflict: "replace", + onConflict: "rename", }, ); expect(companySkillSvc.importPackageFiles).toHaveBeenCalledWith( @@ -3310,7 +3331,7 @@ describe("company portability", () => { "agents/claudecoder/AGENTS.md": expect.any(String), }), { - onConflict: "replace", + onConflict: "rename", }, ); expect(agentSvc.create).toHaveBeenCalledTimes(1); diff --git a/server/src/__tests__/company-skills-service.test.ts b/server/src/__tests__/company-skills-service.test.ts index c2c3541bfcc..1de80114abb 100644 --- a/server/src/__tests__/company-skills-service.test.ts +++ b/server/src/__tests__/company-skills-service.test.ts @@ -1605,6 +1605,72 @@ describeEmbeddedPostgres("companySkillService.list", () => { ]); }); + it("defaults package conflicts to skip and reports skip, rename, and explicit replace outcomes", async () => { + const companyId = randomUUID(); + await db.insert(companies).values({ + id: companyId, + name: "Paperclip", + issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`, + requireBoardApprovalForNewAgents: false, + }); + const original = await svc.createLocalSkill(companyId, { + name: "Conflict Skill", + slug: "conflict-skill", + markdown: "---\nname: Conflict Skill\n---\n\n# Original\n", + }); + const packageFiles = { + "skills/conflict-skill/SKILL.md": [ + "---", + "name: Imported Conflict Skill", + "slug: conflict-skill", + "description: Incoming package version", + "---", + "", + "# Imported", + "", + ].join("\n"), + }; + + const skipped = await svc.importPackageFiles(companyId, packageFiles); + expect(skipped).toEqual([ + expect.objectContaining({ + action: "skipped", + originalSlug: "conflict-skill", + skill: expect.objectContaining({ id: original.id, name: "Conflict Skill" }), + }), + ]); + await expect(svc.getById(companyId, original.id)).resolves.toMatchObject({ + name: "Conflict Skill", + markdown: expect.stringContaining("# Original"), + }); + + const renamed = await svc.importPackageFiles(companyId, packageFiles, { onConflict: "rename" }); + expect(renamed).toEqual([ + expect.objectContaining({ + action: "renamed", + originalSlug: "conflict-skill", + skill: expect.objectContaining({ + name: "Imported Conflict Skill", + slug: "conflict-skill-2", + }), + }), + ]); + expect((await svc.list(companyId)).filter((skill) => skill.slug.startsWith("conflict-skill"))).toHaveLength(2); + + const replaced = await svc.importPackageFiles(companyId, packageFiles, { onConflict: "replace" }); + expect(replaced).toEqual([ + expect.objectContaining({ + action: "replaced", + originalSlug: "conflict-skill", + skill: expect.objectContaining({ id: original.id, name: "Imported Conflict Skill" }), + }), + ]); + await expect(svc.getById(companyId, original.id)).resolves.toMatchObject({ + name: "Imported Conflict Skill", + markdown: expect.stringContaining("# Imported"), + }); + }); + it("rejects executable external package skills before persistence", async () => { const companyId = randomUUID(); await db.insert(companies).values({ diff --git a/server/src/routes/agents.ts b/server/src/routes/agents.ts index f8d991895f0..97754e352d8 100644 --- a/server/src/routes/agents.ts +++ b/server/src/routes/agents.ts @@ -1,4 +1,4 @@ -import { Router, type Request, type Response } from "express"; +import { Router, type NextFunction, type Request, type Response } from "express"; import { generateKeyPairSync, randomUUID } from "node:crypto"; import path from "node:path"; import type { Db } from "@paperclipai/db"; @@ -18,6 +18,7 @@ import { resetAgentSessionSchema, testAdapterEnvironmentSchema, type AgentDesiredSkillEntry, + type AgentSkillAssignmentMode, type AgentSkillSnapshot, type InstanceSchedulerHeartbeatAgent, upsertAgentInstructionsFileSchema, @@ -117,6 +118,35 @@ import { touchesAgentProfileChangeConsentFields, } from "../services/change-consent-gate.js"; +const AGENT_SKILL_ASSIGNMENT_MODES = ["add", "remove", "replace"] as const; + +function requireAgentSkillAssignmentMode(req: Request, _res: Response, next: NextFunction) { + if (!AGENT_SKILL_ASSIGNMENT_MODES.includes(req.body?.mode)) { + throw unprocessable( + 'Skill sync requires mode: "add", "remove", or "replace". ' + + 'Use "replace" only to overwrite the complete desired skill set.', + ); + } + next(); +} + +function mergeDesiredSkillEntries( + current: AgentDesiredSkillEntry[], + requested: AgentDesiredSkillEntry[], + mode: AgentSkillAssignmentMode, +) { + if (mode === "replace") return requested; + + const requestedKeys = new Set(requested.map((entry) => entry.key)); + if (mode === "remove") { + return current.filter((entry) => !requestedKeys.has(entry.key)); + } + + const merged = new Map(current.map((entry) => [entry.key, entry])); + for (const entry of requested) merged.set(entry.key, entry); + return Array.from(merged.values()); +} + const RUN_LOG_DEFAULT_LIMIT_BYTES = 256_000; const RUN_LOG_MAX_LIMIT_BYTES = 1024 * 1024; @@ -1625,6 +1655,7 @@ export function agentRoutes( adapterType: string, adapterConfig: Record, requestedDesiredSkills: AgentDesiredSkillEntry[] | undefined, + mode: AgentSkillAssignmentMode, options: { tolerateUnknownDesiredSkills?: boolean } = {}, ) { if (!requestedDesiredSkills) { @@ -1647,22 +1678,44 @@ export function agentRoutes( await companySkills.resolveRequestedSkillEntries(companyId, requestedDesiredSkills, { tolerateUnknownReferences: options.tolerateUnknownDesiredSkills, }); - // Runtime materialization + version selection only ever consider skills that - // actually resolve to the company library; stale keys can't be materialized. - const runtimeSkillEntries = await companySkills.listRuntimeSkillEntries(companyId, { - materializeMissing: shouldMaterializeRuntimeSkillsForAdapter(adapterType), - versionSelections: skillVersionSelectionMap(resolvedRequestedSkillEntries), - }); - const resolvedDesiredSkillEntries = resolvedRequestedSkillEntries.filter( + const requestedSkillEntries = [ + ...resolvedRequestedSkillEntries, + ...unresolvedDesiredSkillKeys.map((key) => ({ key, versionId: null })), + ].filter( (entry, index, entries) => entries.findIndex((candidate) => candidate.key === entry.key) === index, ); - // Preserve stale/unresolvable keys in the persisted desired set so they stay - // visible (and explicitly removable) instead of vanishing on the next save. - const desiredSkillEntries: AgentDesiredSkillEntry[] = [ - ...resolvedDesiredSkillEntries, - ...unresolvedDesiredSkillKeys.map((key) => ({ key, versionId: null })), - ]; + + const currentPreference = readPaperclipSkillSyncPreference(adapterConfig); + const { resolved: resolvedCurrentSkillEntries, unresolved: unresolvedCurrentSkillKeys } = + currentPreference.desiredSkillEntries.length > 0 + ? await companySkills.resolveRequestedSkillEntries( + companyId, + currentPreference.desiredSkillEntries, + { tolerateUnknownReferences: true }, + ) + : { resolved: [], unresolved: [] }; + const currentSkillEntries = [ + ...resolvedCurrentSkillEntries, + ...unresolvedCurrentSkillKeys.map((key) => ({ key, versionId: null })), + ].filter( + (entry, index, entries) => entries.findIndex((candidate) => candidate.key === entry.key) === index, + ); + + const desiredSkillEntries = mergeDesiredSkillEntries(currentSkillEntries, requestedSkillEntries, mode); const desiredSkills = desiredSkillEntries.map((entry) => entry.key); + const resolvedKeys = new Set([ + ...resolvedCurrentSkillEntries.map((entry) => entry.key), + ...resolvedRequestedSkillEntries.map((entry) => entry.key), + ]); + // Runtime materialization + version selection only ever consider final + // assignments that resolve to the company library; stale keys remain + // persisted and explicitly removable without reaching adapter runtimes. + const runtimeSkillEntries = await companySkills.listRuntimeSkillEntries(companyId, { + materializeMissing: shouldMaterializeRuntimeSkillsForAdapter(adapterType), + versionSelections: skillVersionSelectionMap( + desiredSkillEntries.filter((entry) => resolvedKeys.has(entry.key)), + ), + }); return { adapterConfig: writePaperclipSkillSyncPreference(adapterConfig, desiredSkillEntries), @@ -1992,6 +2045,7 @@ export function agentRoutes( router.post( "/agents/:id/skills/sync", + requireAgentSkillAssignmentMode, validate(agentSkillSyncSchema), async (req, res) => { const id = req.params.id as string; @@ -2010,6 +2064,7 @@ export function agentRoutes( agent.adapterType, agent.adapterConfig as Record, requestedSkills, + req.body.mode, // Toggling a resolvable skill must not fail just because the agent // already carries stale desired keys (e.g. a skill removed from the // library). Preserve those keys so they remain visible/removable. @@ -2074,6 +2129,7 @@ export function agentRoutes( adapterType: updated.adapterType, desiredSkills, desiredSkillEntries, + assignmentMode: req.body.mode, mode: snapshot.mode, supported: snapshot.supported, entryCount: snapshot.entries.length, @@ -2494,6 +2550,7 @@ export function agentRoutes( hireInput.adapterType, requestedAdapterConfig, normalizeDesiredSkillSelections(Array.isArray(requestedDesiredSkills) ? requestedDesiredSkills : undefined), + "add", ); const normalizedAdapterConfig = await normalizeMediatedAdapterConfigForPersistence({ companyId, @@ -2689,6 +2746,7 @@ export function agentRoutes( createInput.adapterType, requestedAdapterConfig, normalizeDesiredSkillSelections(Array.isArray(requestedDesiredSkills) ? requestedDesiredSkills : undefined), + "add", ); const normalizedAdapterConfig = await normalizeMediatedAdapterConfigForPersistence({ companyId, diff --git a/server/src/routes/openapi.ts b/server/src/routes/openapi.ts index a94702fb767..e11a2f4edff 100644 --- a/server/src/routes/openapi.ts +++ b/server/src/routes/openapi.ts @@ -1975,7 +1975,7 @@ registry.registerPath({ params: z.object({ id: z.string() }), body: jsonBody(agentSkillSyncSchema), }, - responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized, 404: r.notFound }, + responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized, 404: r.notFound, 422: r.unprocessable }, }); registry.registerPath({ diff --git a/server/src/services/company-portability.ts b/server/src/services/company-portability.ts index 8818598ec4a..cdf4cb56303 100644 --- a/server/src/services/company-portability.ts +++ b/server/src/services/company-portability.ts @@ -203,7 +203,7 @@ function assertInlineSourceComplete(source: CompanyPortabilityImport["source"]) } function resolveSkillConflictStrategy(mode: ImportMode, collisionStrategy: CompanyPortabilityCollisionStrategy) { - if (mode === "board_full") return "replace" as const; + if (mode === "board_full") return collisionStrategy; return collisionStrategy === "skip" ? "skip" as const : "rename" as const; } @@ -5329,7 +5329,7 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { desiredSkillRefMap.set(importedSkill.originalSlug, importedSkill.skill.key); if (importedSkill.action === "skipped") { warnings.push(`Skipped skill ${importedSkill.originalSlug}; existing skill ${importedSkill.skill.slug} was kept.`); - } else if (importedSkill.originalKey !== importedSkill.skill.key) { + } else if (importedSkill.action === "renamed") { warnings.push(`Imported skill ${importedSkill.originalSlug} as ${importedSkill.skill.slug} to avoid overwriting an existing skill.`); } } @@ -6148,6 +6148,15 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { action: companyAction, }, agents: resultAgents, + skills: importedSkills.map((result) => ({ + originalKey: result.originalKey, + originalSlug: result.originalSlug, + key: result.skill.key, + slug: result.skill.slug, + id: result.skill.id, + action: result.action, + reason: result.reason, + })), projects: resultProjects, routines: resultRoutines, envInputs: sourceManifest.envInputs ?? [], diff --git a/server/src/services/company-skills.ts b/server/src/services/company-skills.ts index 4776fadfa24..22348d87647 100644 --- a/server/src/services/company-skills.ts +++ b/server/src/services/company-skills.ts @@ -257,7 +257,7 @@ type PackageSkillConflictStrategy = "replace" | "rename" | "skip"; export type ImportPackageSkillResult = { skill: CompanySkill; - action: "created" | "updated" | "skipped"; + action: "created" | "renamed" | "replaced" | "skipped"; originalKey: string; originalSlug: string; requestedRefs: string[]; @@ -5826,6 +5826,14 @@ export function companySkillService(db: Db) { const importedSkills = readInlineSkillImports(companyId, normalizedFiles); if (importedSkills.length === 0) return []; + // Conflict handling must never bypass source and reserved-key checks. In + // particular, the safe default "skip" path still validates the incoming + // package before returning the existing skill. + for (const skill of importedSkills) { + assertImportedSkillKeyAllowed(skill); + assertImportedSkillSourceAllowed(skill); + } + for (const skill of importedSkills) { if (skill.sourceType !== "catalog") continue; const materializedDir = await materializeCatalogSkillFiles(companyId, skill, normalizedFiles); @@ -5834,7 +5842,7 @@ export function companySkillService(db: Db) { } } - const conflictStrategy = options?.onConflict ?? "replace"; + const conflictStrategy = options?.onConflict ?? "skip"; const existingSkills = await listFull(companyId); const existingByKey = new Map(existingSkills.map((skill) => [skill.key, skill])); const existingBySlug = new Map( @@ -5848,8 +5856,7 @@ export function companySkillService(db: Db) { skill: ImportedSkill; originalKey: string; originalSlug: string; - existingBefore: CompanySkill | null; - actionHint: "created" | "updated"; + actionHint: "created" | "renamed" | "replaced"; reason: string | null; }> = []; const out: ImportPackageSkillResult[] = []; @@ -5863,17 +5870,29 @@ export function companySkillService(db: Db) { const conflict = existingByIncomingKey ?? existingByIncomingSlug; if (!conflict || conflictStrategy === "replace") { - toPersist.push(importedSkill); + const skillToPersist = conflict && conflict.key !== importedSkill.key + ? { + ...importedSkill, + key: conflict.key, + slug: conflict.slug, + metadata: { + ...(importedSkill.metadata ?? {}), + skillKey: conflict.key, + importedFromSkillKey: originalKey, + importedFromSkillSlug: originalSlug, + }, + } + : importedSkill; + toPersist.push(skillToPersist); prepared.push({ - skill: importedSkill, + skill: skillToPersist, originalKey, originalSlug, - existingBefore: existingByIncomingKey, - actionHint: existingByIncomingKey ? "updated" : "created", - reason: existingByIncomingKey ? "Existing skill key matched; replace strategy." : null, + actionHint: conflict ? "replaced" : "created", + reason: conflict ? "Existing skill matched; explicit replace strategy." : null, }); - usedSlugs.add(normalizedSlug); - usedKeys.add(importedSkill.key); + usedSlugs.add(normalizeSkillSlug(skillToPersist.slug) ?? skillToPersist.slug); + usedKeys.add(skillToPersist.key); continue; } @@ -5907,8 +5926,7 @@ export function companySkillService(db: Db) { skill: renamedSkill, originalKey, originalSlug, - existingBefore: null, - actionHint: "created", + actionHint: "renamed", reason: `Existing skill matched; renamed to ${renamedSlug}.`, }); usedSlugs.add(renamedSlug); diff --git a/ui/src/api/agents.ts b/ui/src/api/agents.ts index 45e134c3cd1..6b42b1f581f 100644 --- a/ui/src/api/agents.ts +++ b/ui/src/api/agents.ts @@ -1,6 +1,7 @@ import type { Agent, AgentDesiredSkillEntry, + AgentSkillAssignmentMode, AgentPermissions, AgentDetail, AgentInstructionsBundle, @@ -178,8 +179,12 @@ export const agentsApi = { listKeys: (id: string, companyId?: string) => api.get(agentPath(id, companyId, "/keys")), skills: (id: string, companyId?: string) => api.get(agentPath(id, companyId, "/skills")), - syncSkills: (id: string, desiredSkills: Array, companyId?: string) => - api.post(agentPath(id, companyId, "/skills/sync"), { desiredSkills }), + syncSkills: ( + id: string, + desiredSkills: Array, + mode: AgentSkillAssignmentMode, + companyId?: string, + ) => api.post(agentPath(id, companyId, "/skills/sync"), { desiredSkills, mode }), createKey: (id: string, name: string, companyId?: string, scope?: AgentApiKeyScope) => api.post(agentPath(id, companyId, "/keys"), { name, ...(scope ? { scope } : {}) }), revokeKey: (agentId: string, keyId: string, companyId?: string) => diff --git a/ui/src/components/skill-studio/AgentsUsingSkillDialog.tsx b/ui/src/components/skill-studio/AgentsUsingSkillDialog.tsx index aa88f29119b..439e0ce92c1 100644 --- a/ui/src/components/skill-studio/AgentsUsingSkillDialog.tsx +++ b/ui/src/components/skill-studio/AgentsUsingSkillDialog.tsx @@ -189,7 +189,7 @@ export function AgentsUsingSkillDialog({ if (desiredSetsEqual(currentEntries, nextEntries)) { return { agentId: agent.id, changed: false }; } - await agentsApi.syncSkills(agent.id, nextEntries, companyId); + await agentsApi.syncSkills(agent.id, nextEntries, "replace", companyId); return { agentId: agent.id, changed: true }; }, onSuccess: async ({ agentId }) => { diff --git a/ui/src/pages/CompanyImport.test.tsx b/ui/src/pages/CompanyImport.test.tsx index ac531c8ab51..dd2fefcb97b 100644 --- a/ui/src/pages/CompanyImport.test.tsx +++ b/ui/src/pages/CompanyImport.test.tsx @@ -160,6 +160,7 @@ function buildImportResult(): CompanyPortabilityImportResult { return { company: { id: "company-2", name: "Imported Test", action: "created" }, agents: [{ slug: "coder", id: "agent-1", action: "created", name: "Coder", reason: null }], + skills: [], projects: [], routines: [{ slug: "weekly-report", id: "routine-1", action: "created", title: "Weekly Report", status: "paused" }], envInputs: [], diff --git a/ui/src/pages/CompanyImport.tsx b/ui/src/pages/CompanyImport.tsx index 5aecd4a4374..3339867d6de 100644 --- a/ui/src/pages/CompanyImport.tsx +++ b/ui/src/pages/CompanyImport.tsx @@ -1455,6 +1455,7 @@ export function CompanyImport() { if (importOutcome) { const { result, dashboardPath } = importOutcome; + const skillResults = result.skills ?? []; const activationItems = importOutcome.pausedAutomations ? buildActivationItems(result) : []; const pendingCount = activationItems.filter( (item) => activationChecked.has(item.key) && !activatedKeys.has(item.key), @@ -1465,11 +1466,31 @@ export function CompanyImport() {

Import complete

{result.company.name}: {result.agents.length} agent{result.agents.length === 1 ? "" : "s"},{" "} + {skillResults.length} skill{skillResults.length === 1 ? "" : "s"},{" "} {result.projects.length} project{result.projects.length === 1 ? "" : "s"}, and{" "} {result.routines.length} routine{result.routines.length === 1 ? "" : "s"} processed.

+ {skillResults.length > 0 && ( +
+
+

Skill import results

+
+
+ {skillResults.map((skill) => ( +
+ {skill.originalSlug} + {skill.action} + {skill.slug !== skill.originalSlug && ( + as {skill.slug} + )} +
+ ))} +
+
+ )} + {result.warnings.length > 0 && (
{result.warnings.map((w) => ( diff --git a/ui/src/pages/CompanySkills.tsx b/ui/src/pages/CompanySkills.tsx index be8a9f41ad8..2bb0859f07f 100644 --- a/ui/src/pages/CompanySkills.tsx +++ b/ui/src/pages/CompanySkills.tsx @@ -4850,7 +4850,7 @@ export function CompanySkills() { const attachAgentsMutation = useMutation({ mutationFn: async (input: { agentId: string; desiredSkills: Array }) => { - return agentsApi.syncSkills(input.agentId, input.desiredSkills, selectedCompanyId ?? undefined); + return agentsApi.syncSkills(input.agentId, input.desiredSkills, "replace", selectedCompanyId ?? undefined); }, onSuccess: async () => { await Promise.all([ diff --git a/ui/src/pages/agent-skills/AgentSkillsTab.tsx b/ui/src/pages/agent-skills/AgentSkillsTab.tsx index 44adb974373..4ec4d2b82ee 100644 --- a/ui/src/pages/agent-skills/AgentSkillsTab.tsx +++ b/ui/src/pages/agent-skills/AgentSkillsTab.tsx @@ -112,7 +112,7 @@ export function AgentSkillsTab({ agent, companyId }: { agent: Agent; companyId?: const syncSkills = useMutation({ mutationFn: (desiredSkills: Array) => - agentsApi.syncSkills(agent.id, desiredSkills, companyId), + agentsApi.syncSkills(agent.id, desiredSkills, "replace", companyId), onSuccess: async (snapshot) => { queryClient.setQueryData(queryKeys.agents.skills(agent.id), snapshot); lastSavedSkillsRef.current = snapshot.desiredSkills; diff --git a/ui/storybook/stories/team-catalog.stories.tsx b/ui/storybook/stories/team-catalog.stories.tsx index c30ee006cf4..7fd0d1e4c0d 100644 --- a/ui/storybook/stories/team-catalog.stories.tsx +++ b/ui/storybook/stories/team-catalog.stories.tsx @@ -352,6 +352,7 @@ export const InstallSuccess: Story = { { slug: "cto", id: "a2", action: "created", name: "CTO", reason: null }, { slug: "cmo", id: "a3", action: "created", name: "CMO (from Core Exec Team)", reason: null }, ], + skills: [], projects: [{ slug: "launch", id: "p1", action: "created", name: "Launch", reason: null }], routines: [], envInputs: [],