mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-21 08:22:20 +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>
117 lines
8.4 KiB
TypeScript
117 lines
8.4 KiB
TypeScript
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();
|
|
});
|
|
});
|