mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-21 00:05:49 +02:00
## 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
248 lines
5.9 KiB
TypeScript
248 lines
5.9 KiB
TypeScript
// @vitest-environment node
|
|
|
|
import { describe, expect, it } from "vitest";
|
|
import type { Agent } from "@paperclipai/shared";
|
|
import { buildAgentUpdatePatch, type AgentConfigOverlay } from "./agent-config-patch";
|
|
|
|
function makeAgent(): Agent {
|
|
return {
|
|
id: "agent-1",
|
|
companyId: "company-1",
|
|
name: "Agent",
|
|
role: "engineer",
|
|
title: "Engineer",
|
|
icon: null,
|
|
status: "active",
|
|
reportsTo: null,
|
|
capabilities: null,
|
|
adapterType: "claude_local",
|
|
adapterConfig: {
|
|
model: "claude-sonnet-4-6",
|
|
env: {
|
|
OPENAI_API_KEY: {
|
|
type: "plain",
|
|
value: "secret",
|
|
},
|
|
},
|
|
promptTemplate: "Work the issue.",
|
|
},
|
|
runtimeConfig: {
|
|
heartbeat: {
|
|
enabled: true,
|
|
intervalSec: 300,
|
|
},
|
|
},
|
|
budgetMonthlyCents: 0,
|
|
spentMonthlyCents: 0,
|
|
pauseReason: null,
|
|
pausedAt: null,
|
|
lastHeartbeatAt: null,
|
|
createdAt: new Date("2026-01-01T00:00:00.000Z"),
|
|
updatedAt: new Date("2026-01-01T00:00:00.000Z"),
|
|
urlKey: "agent",
|
|
permissions: {
|
|
canCreateAgents: false,
|
|
},
|
|
metadata: null,
|
|
};
|
|
}
|
|
|
|
function makeOverlay(patch?: Partial<AgentConfigOverlay>): AgentConfigOverlay {
|
|
return {
|
|
identity: {},
|
|
adapterConfig: {},
|
|
heartbeat: {},
|
|
runtime: {},
|
|
...patch,
|
|
};
|
|
}
|
|
|
|
describe("buildAgentUpdatePatch", () => {
|
|
it("replaces adapter config and drops env when the last env binding is cleared", () => {
|
|
const patch = buildAgentUpdatePatch(
|
|
makeAgent(),
|
|
makeOverlay({
|
|
adapterConfig: {
|
|
env: undefined,
|
|
},
|
|
}),
|
|
);
|
|
|
|
expect(patch).toEqual({
|
|
adapterConfig: {
|
|
model: "claude-sonnet-4-6",
|
|
promptTemplate: "Work the issue.",
|
|
},
|
|
replaceAdapterConfig: true,
|
|
});
|
|
});
|
|
|
|
it("writes the cheap profile under runtimeConfig.modelProfiles, never on primary adapterConfig", () => {
|
|
const patch = buildAgentUpdatePatch(
|
|
makeAgent(),
|
|
makeOverlay({
|
|
modelProfiles: {
|
|
cheap: {
|
|
enabled: true,
|
|
adapterConfig: { model: "claude-haiku-4-5" },
|
|
},
|
|
},
|
|
}),
|
|
);
|
|
|
|
expect(patch).toEqual({
|
|
runtimeConfig: {
|
|
heartbeat: {
|
|
enabled: true,
|
|
intervalSec: 300,
|
|
},
|
|
modelProfiles: {
|
|
cheap: {
|
|
enabled: true,
|
|
adapterConfig: { model: "claude-haiku-4-5" },
|
|
},
|
|
},
|
|
},
|
|
});
|
|
// The primary adapterConfig is untouched.
|
|
expect(patch.adapterConfig).toBeUndefined();
|
|
});
|
|
|
|
it("writes max-turn continuation policy under runtimeConfig.heartbeat", () => {
|
|
const patch = buildAgentUpdatePatch(
|
|
makeAgent(),
|
|
makeOverlay({
|
|
heartbeat: {
|
|
maxTurnContinuation: {
|
|
enabled: true,
|
|
maxAttempts: 3,
|
|
delayMs: 1000,
|
|
},
|
|
},
|
|
}),
|
|
);
|
|
|
|
expect(patch).toEqual({
|
|
runtimeConfig: {
|
|
heartbeat: {
|
|
enabled: true,
|
|
intervalSec: 300,
|
|
maxTurnContinuation: {
|
|
enabled: true,
|
|
maxAttempts: 3,
|
|
delayMs: 1000,
|
|
},
|
|
},
|
|
},
|
|
});
|
|
});
|
|
|
|
it("merges cheap profile changes onto existing runtimeConfig.modelProfiles state", () => {
|
|
const agent = makeAgent();
|
|
agent.runtimeConfig = {
|
|
heartbeat: { enabled: true, intervalSec: 300 },
|
|
modelProfiles: {
|
|
cheap: {
|
|
enabled: false,
|
|
adapterConfig: { model: "old-cheap" },
|
|
},
|
|
},
|
|
};
|
|
|
|
const patch = buildAgentUpdatePatch(
|
|
agent,
|
|
makeOverlay({
|
|
modelProfiles: {
|
|
cheap: {
|
|
enabled: true,
|
|
},
|
|
},
|
|
}),
|
|
);
|
|
|
|
expect((patch.runtimeConfig as Record<string, unknown>).modelProfiles).toEqual({
|
|
cheap: {
|
|
enabled: true,
|
|
adapterConfig: { model: "old-cheap" },
|
|
},
|
|
});
|
|
});
|
|
|
|
it("clears the cheap profile when the overlay marks it cleared", () => {
|
|
const agent = makeAgent();
|
|
agent.runtimeConfig = {
|
|
heartbeat: { enabled: true, intervalSec: 300 },
|
|
modelProfiles: {
|
|
cheap: {
|
|
enabled: true,
|
|
adapterConfig: { model: "claude-haiku-4-5" },
|
|
},
|
|
},
|
|
};
|
|
|
|
const patch = buildAgentUpdatePatch(
|
|
agent,
|
|
makeOverlay({
|
|
modelProfiles: { cheap: { cleared: true } },
|
|
}),
|
|
);
|
|
|
|
expect(patch.runtimeConfig).toEqual({
|
|
heartbeat: { enabled: true, intervalSec: 300 },
|
|
});
|
|
});
|
|
|
|
it("preserves adapter-agnostic keys when changing adapter types", () => {
|
|
const patch = buildAgentUpdatePatch(
|
|
makeAgent(),
|
|
makeOverlay({
|
|
adapterType: "codex_local",
|
|
adapterConfig: {
|
|
model: "gpt-5.4",
|
|
dangerouslyBypassApprovalsAndSandbox: true,
|
|
},
|
|
}),
|
|
);
|
|
|
|
expect(patch).toEqual({
|
|
adapterType: "codex_local",
|
|
adapterConfig: {
|
|
env: {
|
|
OPENAI_API_KEY: {
|
|
type: "plain",
|
|
value: "secret",
|
|
},
|
|
},
|
|
promptTemplate: "Work the issue.",
|
|
model: "gpt-5.4",
|
|
dangerouslyBypassApprovalsAndSandbox: true,
|
|
},
|
|
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"],
|
|
});
|
|
});
|
|
});
|