mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-11 05:11:49 +01:00
feat(skills): let agents update company skills safely (#15049)
## Thinking Path > - Paperclip is an open source control plane for AI-agent companies. > - Company skills give agents reusable work instructions. > - Skill Studio can edit skill files and save version history. > - Agents can create a skill, but they do not have a first-class update tool. > - An agent update needs a version check and safe retry behavior to prevent lost edits. > - This pull request adds `update_skill` through the existing company skill file API. > - The change keeps company policy, version history, and audit records in one path. ## Linked Issues or Issue Description **Subsystem affected** Cross-cutting: the server API, shared validation, and runner tool catalog. **Problem or motivation** An agent can create a company skill but cannot update its `SKILL.md` through a first-class tool. An unguarded retry can also create duplicate versions or overwrite a newer edit. **Proposed solution** Add `update_skill` with a required current version ID and a retry key. Route it through the existing skill file API. Reject stale versions and changed-input retries. Save the version and audit event together. **Alternatives considered** A separate write endpoint would duplicate the Skill Studio mutation path and policy checks. This PR reuses that path instead. **Roadmap alignment** This work extends Skills Manager and Skill Studio, which are listed in `ROADMAP.md`. **Additional context** The tool accepts a complete `SKILL.md`, not a partial patch. Callers must read the current version before they edit it. ## What Changed - Add optional version and retry fields to the existing skill file update contract. - Add a guarded API update with a stable retry receipt and attributed audit event. - Add `update_skill` to native and semantic runner tool catalogs, with mode and policy gates. - Add unit, integration, protocol, and semantic-tool regression coverage. - Document agent use and extend the OpenAPI request contract. ## Verification - Focused tests and direct server and runner TypeScript checks passed before this PR. - `git diff --check` passed after the rebase onto `master`. - CI passed on the latest PR head, including the full test matrix, typecheck, and build. Local full typecheck and build stopped because `cargo` is not installed. The local full test run ended without a verdict. - No dedicated end-to-end eval scenario was added or run. The protocol coverage and semantic-tool test cover the new action deterministically. - Reviewers can read a skill version, call `update_skill`, repeat the same key, then try a stale version and a changed-input key. Only the first edit must create a new version. ## Risks - File writes and database transactions must stay in sync when a write fails. The integration tests cover failed writes and retry behavior, but CI must verify them on the PR head. - Existing Skill Studio callers do not send the new optional coordination fields. Their request shape remains valid. ## Model Used - OpenAI Codex CLI assisted with this change. The runner did not expose the exact model ID or context window. The agent used code execution and repository tools. ## 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; exact model ID and context window were not exposed) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [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 (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass (focused tests; full suite is pending CI) - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green (53 pass, 4 skip on the latest head) - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
b17019e14d
commit
eb049aebf2
32 files changed
+481
-72
No files matched your search
@@ -102,7 +102,7 @@ describe("PaperclipRunnerToolAuthority", () => {
|
||||
issueId,
|
||||
runId,
|
||||
});
|
||||
expect(authority.definitions()).toHaveLength(35);
|
||||
expect(authority.definitions()).toHaveLength(36);
|
||||
const questions = authority.definitions().find(tool => tool.name === "request_human_input")!;
|
||||
expect(questions.description).toContain("ask only the next unanswered question");
|
||||
expect(questions.description).toContain("Never fabricate answers");
|
||||
@@ -131,6 +131,7 @@ describe("PaperclipRunnerToolAuthority", () => {
|
||||
"list_document_revisions",
|
||||
"write_document",
|
||||
"create_skill",
|
||||
"update_skill",
|
||||
"list_agents",
|
||||
"get_agent",
|
||||
"list_approvals",
|
||||
|
||||
@@ -13,7 +13,7 @@ import { resolveCoreTrustPreset } from "../trust-preset-resolver.js";
|
||||
import { normalizeIssueExecutionPolicy } from "../issue-execution-policy.js";
|
||||
import { buildLowTrustSourceTrust } from "../source-trust.js";
|
||||
import { handoffPlanContext } from "./handoff-plan-context.js";
|
||||
import { callCreateSkillTool } from "../skill-tools.js";
|
||||
import { callCreateSkillTool, callUpdateSkillTool } from "../skill-tools.js";
|
||||
import { callProjectTool } from "../project-tools.js";
|
||||
import { isConnectorTool, executeConnectorTool, type ConnectorAssignment } from "../connector-runtime.js";
|
||||
import { resolveNativeRuntimeMcpSnapshot } from "./runtime-context.js";
|
||||
@@ -97,7 +97,7 @@ const IMPLEMENTED_OPERATIONS = new Set([
|
||||
"search_api", "call_api", "hire_agent",
|
||||
"get_task_context", "get_task_history", "search_tasks", "report_progress", "set_task_title",
|
||||
"request_human_input",
|
||||
"create_skill", "create_task", "reassign_task", "set_dependencies", "create_project", "list_project_repositories", "list_projects", "register_deliverable",
|
||||
"create_skill", "update_skill", "create_task", "reassign_task", "set_dependencies", "create_project", "list_project_repositories", "list_projects", "register_deliverable",
|
||||
"list_documents", "read_document", "list_document_revisions", "write_document",
|
||||
"list_agents", "get_agent", "list_approvals", "get_approval", "get_approval_context",
|
||||
]);
|
||||
@@ -414,6 +414,12 @@ export class PaperclipRunnerToolAuthority {
|
||||
if (!apiUrl || !token) throw new Error("Skill tool authentication is unavailable");
|
||||
return callCreateSkillTool({ arguments: input, apiUrl, token, companyId: this.binding.companyId });
|
||||
}
|
||||
case "update_skill": {
|
||||
const apiUrl = this.binding.apiUrl ?? process.env.PAPERCLIP_API_URL;
|
||||
const token = createLocalAgentJwt(this.binding.agentId, this.binding.companyId, context.actor.adapterType, this.binding.runId, context.run.responsibleUserId);
|
||||
if (!apiUrl || !token) throw new Error("Skill tool authentication is unavailable");
|
||||
return callUpdateSkillTool({ arguments: input, apiUrl, token, companyId: this.binding.companyId });
|
||||
}
|
||||
case "create_project":
|
||||
case "list_project_repositories":
|
||||
case "list_projects": {
|
||||
|
||||
@@ -48,6 +48,7 @@ function dedicatedTools(method: string, path: string): string[] {
|
||||
if (/\/agents\/\{[^}]+\}\/instructions-bundle\/revision\/\{[^}]+\}$/.test(path) && method === "GET") return ["read_agent_instructions"];
|
||||
if (/\/agents\/\{[^}]+\}\/instructions-bundle\/restore$/.test(path) && method === "POST") return ["restore_agent_instructions"];
|
||||
if (/\/companies\/\{[^}]+\}\/skills$/.test(path) && method === "POST") return ["create_skill"];
|
||||
if (/\/companies\/\{[^}]+\}\/skills\/\{[^}]+\}\/files$/.test(path) && method === "PATCH") return ["update_skill"];
|
||||
if (/\/companies\/\{[^}]+\}\/agent-hires$/.test(path) && method === "POST") return ["hire_agent"];
|
||||
if (/\/projects$/.test(path)) return method === "GET" ? ["list_projects"] : method === "POST" ? ["create_project"] : [];
|
||||
if (/\/project-repositories$/.test(path) && method === "GET") return ["list_project_repositories"];
|
||||
|
||||
@@ -0,0 +1,116 @@
|
||||
import { randomUUID } from "node:crypto";
|
||||
import { mkdtemp, rm } from "node:fs/promises";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join } from "node:path";
|
||||
import { eq } from "drizzle-orm";
|
||||
import { activityLog, companyMemberships, heartbeatRuns, issues } from "@paperclipai/db";
|
||||
import { afterAll, beforeAll, describe, expect, it } from "vitest";
|
||||
import { startRunnerApiTestServer } from "../../__tests__/helpers/runner-api-server.js";
|
||||
import { PaperclipRunnerToolAuthority } from "./paperclip-runner-tool-authority.js";
|
||||
import { companySkillPolicyService } from "../company-skill-policy.js";
|
||||
import { companySkillService } from "../company-skills.js";
|
||||
|
||||
describe("runner update_skill through the real skill API", () => {
|
||||
let server: Awaited<ReturnType<typeof startRunnerApiTestServer>>;
|
||||
let home: string;
|
||||
const previousSecret = process.env.PAPERCLIP_AGENT_JWT_SECRET;
|
||||
const previousHome = process.env.PAPERCLIP_HOME;
|
||||
beforeAll(async () => {
|
||||
process.env.PAPERCLIP_AGENT_JWT_SECRET = randomUUID();
|
||||
home = await mkdtemp(join(tmpdir(), "paperclip-update-skill-"));
|
||||
process.env.PAPERCLIP_HOME = home;
|
||||
server = await startRunnerApiTestServer();
|
||||
}, 60_000);
|
||||
afterAll(async () => {
|
||||
await server?.close();
|
||||
if (previousSecret === undefined) delete process.env.PAPERCLIP_AGENT_JWT_SECRET;
|
||||
else process.env.PAPERCLIP_AGENT_JWT_SECRET = previousSecret;
|
||||
if (previousHome === undefined) delete process.env.PAPERCLIP_HOME;
|
||||
else process.env.PAPERCLIP_HOME = previousHome;
|
||||
if (home) await rm(home, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
async function fixture(options: Parameters<typeof server.fixture>[0] = {}) {
|
||||
const result = await server.fixture(options);
|
||||
await server.db.insert(companyMemberships).values({ companyId: result.companyId, principalType: "agent", principalId: result.agentId, status: "active", membershipRole: "member" });
|
||||
const skill = await companySkillService(server.db).createLocalSkill(result.companyId, {
|
||||
name: "release-review", description: "Review releases.",
|
||||
markdown: "---\nname: release-review\ndescription: Review releases.\n---\n\n# Review\nInspect the release.\n",
|
||||
});
|
||||
const args = { skillId: skill.id, expectedVersionId: skill.currentVersionId, idempotencyKey: randomUUID(),
|
||||
markdown: "---\nname: release-review\ndescription: Review releases.\n---\n\n# Review\nInspect the release and its tests.\n" };
|
||||
return { ...result, skill, args };
|
||||
}
|
||||
|
||||
it("persists exactly one version and attributed activity across retries and a replacement run", async () => {
|
||||
const testFixture = await fixture();
|
||||
const { authority, args, companyId, skill, issueId } = testFixture;
|
||||
expect(authority.definitions().some(tool => tool.name === "update_skill")).toBe(true);
|
||||
const first = await authority.execute({ tool: "update_skill", callId: "first", arguments: args });
|
||||
const second = await authority.execute({ tool: "update_skill", callId: "retry", arguments: args });
|
||||
expect(second).toEqual(first);
|
||||
expect(first).toMatchObject({ skillId: skill.id, path: "SKILL.md", versionId: expect.any(String) });
|
||||
const replacementRunId = randomUUID();
|
||||
await server.db.update(heartbeatRuns).set({ status: "failed" }).where(eq(heartbeatRuns.id, testFixture.runId));
|
||||
await server.db.insert(heartbeatRuns).values({ id: replacementRunId, companyId, agentId: testFixture.agentId,
|
||||
status: "running", runtimeMode: "native", nativeIssueId: issueId, invocationSource: "assignment", triggerDetail: "system", contextSnapshot: { issueId } });
|
||||
await server.db.update(issues).set({ executionRunId: replacementRunId }).where(eq(issues.id, issueId));
|
||||
const replacement = new PaperclipRunnerToolAuthority(server.db, { ...testFixture, runId: replacementRunId });
|
||||
expect(await replacement.execute({ tool: "update_skill", callId: "replacement", arguments: args })).toEqual(first);
|
||||
expect(await companySkillService(server.db).listVersions(companyId, skill.id)).toHaveLength(2);
|
||||
expect((await server.db.select().from(activityLog).where(eq(activityLog.action, "company.skill_file_updated")))
|
||||
.filter(event => event.entityId === skill.id))
|
||||
.toEqual([expect.objectContaining({ entityId: skill.id, details: expect.objectContaining({ sourceIssueId: issueId, versionId: (first as { versionId: string }).versionId }) })]);
|
||||
await expect(replacement.execute({ tool: "update_skill", callId: "changed-key", arguments: { ...args, markdown: args.markdown + "More." } })).rejects.toThrow(/different inputs/);
|
||||
await expect(replacement.execute({ tool: "update_skill", callId: "stale", arguments: { ...args, idempotencyKey: randomUUID() } })).rejects.toThrow(/version changed/);
|
||||
});
|
||||
|
||||
it("denies policy changes and cross-company skill IDs", async () => {
|
||||
const first = await fixture();
|
||||
const second = await fixture();
|
||||
await expect(second.authority.execute({ tool: "update_skill", callId: "foreign", arguments: { ...first.args, idempotencyKey: randomUUID() } })).rejects.toThrow();
|
||||
await companySkillPolicyService(server.db).replace({ companyId: first.companyId, expectedRevision: 0,
|
||||
policy: { schemaVersion: 1, defaultEffect: "deny", rules: [] }, activity: { actorType: "user", actorId: "test-board" } });
|
||||
await expect(first.authority.execute({ tool: "update_skill", callId: "denied", arguments: first.args })).rejects.toThrow(/company policy/);
|
||||
expect(await companySkillService(server.db).listVersions(first.companyId, first.skill.id)).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("serializes simultaneous retries and checks policy again after a successful edit", async () => {
|
||||
const testFixture = await fixture();
|
||||
const calls = await Promise.all(["parallel-a", "parallel-b"].map(callId =>
|
||||
testFixture.authority.execute({ tool: "update_skill", callId, arguments: testFixture.args })));
|
||||
expect(calls[0]).toEqual(calls[1]);
|
||||
expect(await companySkillService(server.db).listVersions(testFixture.companyId, testFixture.skill.id)).toHaveLength(2);
|
||||
await companySkillPolicyService(server.db).replace({ companyId: testFixture.companyId, expectedRevision: 0,
|
||||
policy: { schemaVersion: 1, defaultEffect: "deny", rules: [] }, activity: { actorType: "user", actorId: "test-board" } });
|
||||
await expect(testFixture.authority.execute({ tool: "update_skill", callId: "revoked-retry", arguments: testFixture.args })).rejects.toThrow(/company policy/);
|
||||
expect(await companySkillService(server.db).listVersions(testFixture.companyId, testFixture.skill.id)).toHaveLength(2);
|
||||
});
|
||||
|
||||
it("restores the file when the required audit callback fails", async () => {
|
||||
const testFixture = await fixture();
|
||||
const service = companySkillService(server.db);
|
||||
const original = await service.readFile(testFixture.companyId, testFixture.skill.id, "SKILL.md");
|
||||
await expect(service.updateFile(testFixture.companyId, testFixture.skill.id, "SKILL.md", testFixture.args.markdown,
|
||||
null, { expectedVersionId: testFixture.skill.currentVersionId, afterUpdate: async () => { throw new Error("audit unavailable"); } }))
|
||||
.rejects.toThrow("audit unavailable");
|
||||
expect((await service.readFile(testFixture.companyId, testFixture.skill.id, "SKILL.md"))?.content).toBe(original?.content);
|
||||
expect(await service.listVersions(testFixture.companyId, testFixture.skill.id)).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("rejects a competing edit with a different key and the same expected version", async () => {
|
||||
const testFixture = await fixture();
|
||||
const attempts = await Promise.allSettled([testFixture.args, { ...testFixture.args,
|
||||
idempotencyKey: randomUUID(), markdown: testFixture.args.markdown + "Another edit." }].map((argumentsValue, index) =>
|
||||
testFixture.authority.execute({ tool: "update_skill", callId: `race-${index}`, arguments: argumentsValue })));
|
||||
expect(attempts.filter(attempt => attempt.status === "fulfilled")).toHaveLength(1);
|
||||
expect(attempts.filter(attempt => attempt.status === "rejected")).toHaveLength(1);
|
||||
expect(await companySkillService(server.db).listVersions(testFixture.companyId, testFixture.skill.id)).toHaveLength(2);
|
||||
});
|
||||
|
||||
it("hides the tool and rejects invocation in planning mode", async () => {
|
||||
const testFixture = await fixture({ mode: "planning" });
|
||||
expect(new PaperclipRunnerToolAuthority(server.db, { ...testFixture, workMode: "planning" }).definitions().some(tool => tool.name === "update_skill")).toBe(false);
|
||||
await expect(testFixture.authority.execute({ tool: "update_skill", callId: "planning", arguments: testFixture.args })).rejects.toThrow();
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user