fix(agents): preserve skill selection when switching adapter type (#8975)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Each agent runs on an adapter (`claude_local`, `codex_local`, …) and can be assigned company skills that are synced into its runtime > - An agent's desired-skill selection is persisted inside its single `adapterConfig` JSON blob under `paperclipSkillSync`, even though the selection is a company-level, adapter-agnostic choice > - When a user changes an agent's adapter type, both the server PATCH handler and the UI patch builder rebuild `adapterConfig` and carry over only a hardcoded allow-list of adapter-agnostic keys (`env`, `cwd`, instructions bundle, …) > - `paperclipSkillSync` was missing from both allow-lists, so switching adapters (e.g. claude_local → codex_local) silently wiped every assigned skill > - This pull request adds `paperclipSkillSync` to the adapter-agnostic preservation list on both layers and covers it with regression tests > - The benefit is that switching an agent's adapter no longer destroys its skill configuration — skills are preserved exactly like env/cwd/instructions already are ## Linked Issues or Issue Description Fixes #8974 ## What Changed - **Server (authoritative fix)** — `server/src/routes/agents.ts`: added `"paperclipSkillSync"` to the `ADAPTER_AGNOSTIC_KEYS` list in the `changingAdapterType` branch of `PATCH /agents/:id`. On an adapter-type change the handler now restores the skill-sync selection from the existing persisted config when the incoming config omits it — the same mechanism already used for `env`, `cwd`, and the instructions bundle. This protects every API/CLI client, not just the UI. - **UI (defense in depth)** — `ui/src/lib/agent-config-patch.ts`: added `"paperclipSkillSync"` to the client-side `ADAPTER_AGNOSTIC_KEYS` in `buildAgentUpdatePatch`, so the optimistic patch the client builds on an adapter switch stops stripping the key before it reaches the server. - **Tests** — added regression tests on both layers: - `server/src/__tests__/agent-instructions-routes.test.ts`: `PATCH`ing `adapterType` (claude_local → codex_local) with `replaceAdapterConfig: true` keeps `adapterConfig.paperclipSkillSync`. - `ui/src/lib/agent-config-patch.test.ts`: `buildAgentUpdatePatch` preserves `paperclipSkillSync` when the overlay changes the adapter type. ## Verification ``` # server (run from repo root) cd server && ../node_modules/.bin/vitest run \ src/__tests__/agent-instructions-routes.test.ts \ src/__tests__/agent-skills-routes.test.ts \ src/__tests__/agent-adapter-validation-routes.test.ts \ src/__tests__/agent-permissions-routes.test.ts # 83 passed ../node_modules/.bin/tsc --noEmit -p tsconfig.json # clean # ui cd ui && ./node_modules/.bin/vitest run src/lib/agent-config-patch.test.ts # 7 passed pnpm --filter @paperclipai/ui typecheck # clean ``` Both new tests fail without the corresponding source change (verified red → green). Manual: create an agent on `claude_local`, assign skills, switch it to `codex_local`, and confirm `GET /api/agents/:id/skills` still returns the desired skills. ## Risks Low risk. - The change only *adds* one key to an existing preservation allow-list; it does not alter how any other key is handled. Behavior for agents without a `paperclipSkillSync` block is unchanged (the key is simply absent and nothing is copied). - `paperclipSkillSync` is adapter-agnostic (company skill keys, not adapter-specific), so carrying it across an adapter switch is always safe — a target adapter that does not support skill sync just ignores it, and switching back restores the selection. - Same-adapter config edits already merged and preserved the key; this only closes the adapter-type-change gap, matching the existing env/cwd/instructions behavior. - Follow-up (not in this PR to keep it minimal): the server and client `ADAPTER_AGNOSTIC_KEYS` lists are maintained separately and already diverge (`instructionsFilePath` is client-only); a shared constant could prevent future drift. ## Model Used Claude Opus 4.8 (`claude-opus-4-8`, 1M-token context), extended thinking enabled, with tool use (file edit, shell, GitHub CLI) via Claude Code. A read-only sub-agent was used to trace the root cause across the server and UI layers. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work (bug fix, not a feature) - [x] I have searched GitHub for duplicate or related PRs and linked them above (none found) - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (`fix/preserve-skills-on-adapter-type-switch`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes (N/A — internal config-preservation fix, no user-facing docs or API contract change) - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green (pending CI on this PR) - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups (pending review) - [x] I will address all Greptile and reviewer comments before requesting merge
This commit is contained in:
parent
a328ec953a
commit
eb2cb916be
|
|
@ -328,6 +328,45 @@ describe("agent instructions bundle routes", () => {
|
|||
);
|
||||
});
|
||||
|
||||
it("preserves paperclip skill-sync selections when switching adapters", async () => {
|
||||
// Desired skills live inside the per-adapter config under
|
||||
// `paperclipSkillSync`, yet they are adapter-agnostic company-level
|
||||
// selections. Switching adapter type must not silently wipe them — the
|
||||
// server carries them over from the existing config the same way it
|
||||
// preserves env/cwd and the instructions bundle.
|
||||
mockAgentService.getById.mockResolvedValue({
|
||||
...makeAgent(),
|
||||
adapterType: "claude_local",
|
||||
adapterConfig: {
|
||||
model: "claude-sonnet-4",
|
||||
paperclipSkillSync: { desiredSkills: ["research", "code-review"] },
|
||||
},
|
||||
});
|
||||
|
||||
const res = await requestApp(await createApp(), (baseUrl) => request(baseUrl)
|
||||
.patch("/api/agents/11111111-1111-4111-8111-111111111111?companyId=company-1")
|
||||
.send({
|
||||
adapterType: "codex_local",
|
||||
replaceAdapterConfig: true,
|
||||
adapterConfig: {
|
||||
model: "gpt-5.4",
|
||||
},
|
||||
}));
|
||||
|
||||
expect(res.status, JSON.stringify(res.body)).toBe(200);
|
||||
expect(mockAgentService.update).toHaveBeenCalledWith(
|
||||
"11111111-1111-4111-8111-111111111111",
|
||||
expect.objectContaining({
|
||||
adapterType: "codex_local",
|
||||
adapterConfig: expect.objectContaining({
|
||||
model: "gpt-5.4",
|
||||
paperclipSkillSync: { desiredSkills: ["research", "code-review"] },
|
||||
}),
|
||||
}),
|
||||
expect.any(Object),
|
||||
);
|
||||
});
|
||||
|
||||
it("merges same-adapter config patches so instructions metadata is not dropped", async () => {
|
||||
mockAgentService.getById.mockResolvedValue({
|
||||
...makeAgent(),
|
||||
|
|
|
|||
|
|
@ -2870,9 +2870,13 @@ export function agentRoutes(
|
|||
// Preserve adapter-agnostic keys (env, cwd, etc.) from the existing config
|
||||
// when the adapter type changes. Without this, a PATCH that includes
|
||||
// adapterConfig but omits these keys would silently drop them.
|
||||
// `paperclipSkillSync` holds the agent's desired-skill selection, which is
|
||||
// a company-level (adapter-agnostic) choice even though it is persisted
|
||||
// inside the per-adapter config; switching adapters must not wipe it.
|
||||
const ADAPTER_AGNOSTIC_KEYS = [
|
||||
"env", "cwd", "timeoutSec", "graceSec",
|
||||
"promptTemplate", "bootstrapPromptTemplate",
|
||||
"paperclipSkillSync",
|
||||
] as const;
|
||||
for (const key of ADAPTER_AGNOSTIC_KEYS) {
|
||||
if (rawEffectiveAdapterConfig[key] === undefined && existingAdapterConfig[key] !== undefined) {
|
||||
|
|
|
|||
|
|
@ -220,4 +220,28 @@ describe("buildAgentUpdatePatch", () => {
|
|||
replaceAdapterConfig: true,
|
||||
});
|
||||
});
|
||||
|
||||
it("preserves paperclip skill-sync selections when changing adapter types", () => {
|
||||
// Desired skills are adapter-agnostic (company-level selections) but are
|
||||
// persisted inside the per-adapter config under `paperclipSkillSync`. A
|
||||
// patch that switches adapters must carry them over instead of wiping the
|
||||
// agent's skills.
|
||||
const agent = makeAgent();
|
||||
agent.adapterConfig = {
|
||||
...agent.adapterConfig,
|
||||
paperclipSkillSync: { desiredSkills: ["research", "code-review"] },
|
||||
};
|
||||
|
||||
const patch = buildAgentUpdatePatch(
|
||||
agent,
|
||||
makeOverlay({
|
||||
adapterType: "codex_local",
|
||||
adapterConfig: { model: "gpt-5.4" },
|
||||
}),
|
||||
);
|
||||
|
||||
expect((patch.adapterConfig as Record<string, unknown>).paperclipSkillSync).toEqual({
|
||||
desiredSkills: ["research", "code-review"],
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -27,6 +27,10 @@ const ADAPTER_AGNOSTIC_KEYS = [
|
|||
"timeoutSec",
|
||||
"graceSec",
|
||||
"bootstrapPromptTemplate",
|
||||
// Desired-skill selection is a company-level, adapter-agnostic choice even
|
||||
// though it is persisted inside the per-adapter config; keep it when the
|
||||
// adapter type changes so switching adapters does not wipe the agent's skills.
|
||||
"paperclipSkillSync",
|
||||
] as const;
|
||||
|
||||
function omitUndefinedEntries(value: Record<string, unknown>) {
|
||||
|
|
|
|||
Loading…
Reference in New Issue