mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-17 01:25:26 +02:00
## 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>
65 lines
4.5 KiB
TypeScript
65 lines
4.5 KiB
TypeScript
import { describe, expect, it, vi } from "vitest";
|
|
import { callCreateSkillTool, createSkillToolInput, callUpdateSkillTool, updateSkillToolInput } from "./skill-tools.js";
|
|
|
|
const input = {
|
|
name: "release-review", description: "Review release notes.", idempotencyKey: "release-review-1",
|
|
markdown: "---\nname: release-review\ndescription: Review release notes.\n---\n\n# Review\nCheck each release note against the change.\n",
|
|
};
|
|
|
|
describe("create skill tool", () => {
|
|
it("requires a usable skill with matching frontmatter", () => {
|
|
expect(createSkillToolInput.parse(input)).toEqual(input);
|
|
for (const markdown of ["I created it", "---\nname: different\ndescription: Review release notes.\n---\nBody", "---\nname: release-review\ndescription: Review release notes.\n---\n"]) {
|
|
expect(createSkillToolInput.safeParse({ ...input, markdown }).success).toBe(false);
|
|
}
|
|
expect(createSkillToolInput.safeParse({ ...input, companyId: "foreign" }).success).toBe(false);
|
|
});
|
|
|
|
it.each([
|
|
{ markdown: "---\nname: [broken\n---\nBody" },
|
|
{ markdown: input.markdown.replace("description: Review release notes.", "description: Different purpose.") },
|
|
{ slug: "another-skill" },
|
|
{ markdown: "No frontmatter." },
|
|
])("rejects invalid skill content before any API call (%j)", async (invalid) => {
|
|
const fetcher = vi.fn();
|
|
await expect(callCreateSkillTool({ arguments: { ...input, ...invalid },
|
|
apiUrl: "http://localhost:3100", token: "test-token", companyId: "company-1" }, fetcher)).rejects.toThrow();
|
|
expect(fetcher).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("uses authenticated company route and returns only a portable skill reference", async () => {
|
|
const fetcher = vi.fn().mockResolvedValue(new Response(JSON.stringify({ id: "skill-1", name: input.name,
|
|
slug: input.name, description: input.description, currentVersionId: "version-1", sourceLocator: "/private/managed/skill", markdown: input.markdown }), { status: 201 }));
|
|
const result = await callCreateSkillTool({ arguments: input, apiUrl: "http://localhost:3100/api", token: "test-token", companyId: "company-1" }, fetcher);
|
|
expect(fetcher).toHaveBeenCalledWith("http://localhost:3100/api/companies/company-1/skills", expect.objectContaining({ method: "POST" }));
|
|
expect(JSON.parse(fetcher.mock.calls[0]![1].body)).toEqual(input);
|
|
expect(result).toEqual({ id: "skill-1", name: input.name, slug: input.name, description: input.description, versionId: "version-1", studioPath: "/skills/studio/skill-1" });
|
|
});
|
|
});
|
|
|
|
describe("update skill tool", () => {
|
|
const args = { skillId: "00000000-0000-4000-8000-000000000001", expectedVersionId: "00000000-0000-4000-8000-000000000002",
|
|
markdown: input.markdown, idempotencyKey: "update-1" };
|
|
|
|
it("rejects incomplete content and untrusted scope without a request", async () => {
|
|
const fetcher = vi.fn();
|
|
expect(updateSkillToolInput.parse(args)).toEqual(args);
|
|
for (const invalid of [{ markdown: "# Missing frontmatter" }, { markdown: input.markdown.replace("# Review\nCheck each release note against the change.", " ") },
|
|
{ companyId: "other" }, { expectedVersionId: "not-a-uuid" }]) {
|
|
await expect(callUpdateSkillTool({ arguments: { ...args, ...invalid }, apiUrl: "http://localhost", token: "token", companyId: "company" }, fetcher)).rejects.toThrow();
|
|
}
|
|
expect(fetcher).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("calls the company file endpoint and returns a compact receipt", async () => {
|
|
const receipt = { skillId: args.skillId, versionId: "next-version", path: "SKILL.md", studioPath: `/skills/studio/${args.skillId}` };
|
|
const fetcher = vi.fn().mockResolvedValue(new Response(JSON.stringify({ ...receipt, content: args.markdown }), { status: 200 }));
|
|
expect(await callUpdateSkillTool({ arguments: args, apiUrl: "http://localhost/api", token: "token", companyId: "company" }, fetcher)).toEqual(receipt);
|
|
expect(fetcher).toHaveBeenCalledWith(`http://localhost/api/companies/company/skills/${args.skillId}/files`, expect.objectContaining({ method: "PATCH" }));
|
|
expect(JSON.parse(fetcher.mock.calls[0]![1].body)).toEqual({ path: "SKILL.md", content: args.markdown,
|
|
expectedVersionId: args.expectedVersionId, idempotencyKey: args.idempotencyKey });
|
|
fetcher.mockResolvedValueOnce(new Response(JSON.stringify({ error: "Skill version changed" }), { status: 409 }));
|
|
await expect(callUpdateSkillTool({ arguments: args, apiUrl: "http://localhost", token: "token", companyId: "company" }, fetcher)).rejects.toThrow("Skill version changed");
|
|
});
|
|
});
|