diff --git a/.agents/skills/prepare-paperclip-pr/SKILL.md b/.agents/skills/prepare-paperclip-pr/SKILL.md new file mode 100644 index 0000000000..35bf189d48 --- /dev/null +++ b/.agents/skills/prepare-paperclip-pr/SKILL.md @@ -0,0 +1,85 @@ +--- +name: prepare-paperclip-pr +description: Prepare a Paperclip branch for PR with commits, template body, and checks. +--- +# Prepare Paperclip PR + +The standard Paperclip procedure for turning branch work into a reviewed, +green pull request against `paperclipai/paperclip` master. Apply it once per +PR (if a task splits a branch into several PRs, run the whole procedure for +each one). + +## 0. Preconditions — worktree safety + +* Do all PR work in a **git worktree** on a dedicated branch. The main + `~/paperclip` checkout typically runs the live Paperclip server — never + check out branches there. If you are already on a worktree/branch, verify it + (`git rev-parse --git-dir`, `git branch --show-current`) and proceed. +* If the main checkout is unexpectedly off `master`, fix that first without + losing work (usually: move that branch's work into a worktree). +* Confirm which remote/ref you are targeting (normally `master` on the + `paperclipai/paperclip` repo; the task may name a specific remote such as + `origin` or `public-gh`). + +## 1. Commit everything — lose no work + +* Make **logical commits** of all uncommitted changes before anything else. + Do not stash and forget; do not leave files behind. If commits are missing, + make them. +* Commit messages must end with exactly: + `Co-Authored-By: Paperclip ` + +## 2. Get changes cleanly on top of master + +* Fetch the target remote and rebase (or otherwise replay) your branch on top + of the target master so the PR has no merge conflicts. +* Re-verify after rebase: build/tests relevant to the change still pass at + whatever depth the task warrants. + +## 3. Guardrails checklist (every PR) + +* **Never commit `pnpm-lock.yaml`** — the repo has actions that manage it. + If it is already in a commit, rewrite/drop that change before pushing. +* **Never change `.github/workflows/*`** unless the underlying commit was + explicitly about that and the task calls it out. +* **No design screenshots / wireframe images** committed to the repo unless + they are genuinely part of the work product. +* **Migrations**: numbered incrementally with no conflicts against master. If + master moved and took your number, renumber on top. Make migrations + **idempotent** so users who already applied the old number are safe. +* **Greptile file limit**: keep each PR under **100 changed files**; if a PR + exceeds that, split it into two. + +## 4. Open the PR + +* Follow `CONTRIBUTING.md` (repo root, + https://github.com/paperclipai/paperclip/blob/master/CONTRIBUTING.md) for + the PR title, message format, and issue description. +* Push the branch and open the PR with `gh`. +* Record the PR URL immediately — every report must include URLs to every PR. + +## 5. Review loops + +* Run the **/greploop** company skill: trigger Greptile review, address its + comments, push, and repeat until Greptile gives **5/5 with zero unresolved + comments** (max 20 turns). Do not stop early while turns remain. +* Then run the **/prcheckloop** company skill and address any verification / + CI failures you can. +* RUN GREPTILE UNTIL IT GETS TO 5/5 - DO NOT STOP UNTIL GREPTILE IS 5/5, all + tests pass, all verification checks pass, and there are no merge conflicts. + +## 6. Report back and hand off + +* Comment on the driving task: what you did, the PR URL(s), the worktree path + (use `~` for home), Greptile score, and check status. +* Create a `pull_request` work product for each opened PR (plus `branch` / + `commit` work products where the branch or a commit is itself the handoff). +* If the task requires follow-up per PR (e.g. sub-issues per PR), create them + as the task directs and link them. + +## Hard rules + +* **YOU DO NOT MERGE THE PR YOURSELF. NEVER MERGE THE PR YOURSELF.** +* Never lose work: no orphaned stashes, no dropped files, no force-pushes + that discard commits. +* Always post the URLs to every pull request you created. diff --git a/server/src/__tests__/authorization-service.test.ts b/server/src/__tests__/authorization-service.test.ts index 111843a0f2..62010ed16e 100644 --- a/server/src/__tests__/authorization-service.test.ts +++ b/server/src/__tests__/authorization-service.test.ts @@ -386,6 +386,101 @@ describeEmbeddedPostgres("authorization service", () => { }); }); + it("allows board users with direct skills:create grants to mutate company skills", async () => { + const company = await createCompany(db, "BoardUserSkillGrant"); + const userId = await createUser(db); + await grantUserPermission(db, company.id, userId, "skills:create"); + + const decision = await authorizationService(db).decide({ + actor: { + type: "board", + userId, + companyIds: [company.id], + source: "session", + }, + action: "skill_config:update", + resource: { type: "company", companyId: company.id }, + }); + + expect(decision).toMatchObject({ + allowed: true, + reason: "allow_direct_change", + grant: { + principalType: "user", + principalId: userId, + permissionKey: "skills:create", + }, + }); + }); + + it("allows responsible-user JWT agents with direct skills:create grants to mutate company skills", async () => { + const company = await createCompany(db, "ResponsibleUserSkillGrant"); + const actorAgent = await createAgent(db, company.id); + const responsibleUserId = await createUser(db); + await grantAgentPermission(db, company.id, actorAgent.id, "skills:create"); + await db.insert(companyMemberships).values({ + companyId: company.id, + principalType: "user", + principalId: responsibleUserId, + status: "active", + membershipRole: "operator", + }); + + const decision = await authorizationService(db).decide({ + actor: { + type: "agent", + agentId: actorAgent.id, + companyId: company.id, + onBehalfOfUserId: responsibleUserId, + source: "agent_jwt", + }, + action: "skill_config:update", + resource: { type: "company", companyId: company.id }, + }); + + expect(decision).toMatchObject({ + allowed: true, + reason: "allow_direct_change", + grant: { + principalType: "agent", + principalId: actorAgent.id, + permissionKey: "skills:create", + }, + }); + }); + + it("keeps responsible-user skill mutations denied for viewer memberships", async () => { + const company = await createCompany(db, "ResponsibleUserSkillViewerDenied"); + const actorAgent = await createAgent(db, company.id); + const responsibleUserId = await createUser(db); + await grantAgentPermission(db, company.id, actorAgent.id, "skills:create"); + await db.insert(companyMemberships).values({ + companyId: company.id, + principalType: "user", + principalId: responsibleUserId, + status: "active", + membershipRole: "viewer", + }); + + const decision = await authorizationService(db).decide({ + actor: { + type: "agent", + agentId: actorAgent.id, + companyId: company.id, + onBehalfOfUserId: responsibleUserId, + source: "agent_jwt", + }, + action: "skill_config:update", + resource: { type: "company", companyId: company.id }, + }); + + expect(decision).toMatchObject({ + allowed: false, + code: "RESPONSIBLE_USER_UNAUTHORIZED", + }); + expect(decision.explanation).toContain(`Responsible user ${responsibleUserId} is not authorized`); + }); + it("denies cross-company agent decisions before grant evaluation", async () => { const sourceCompany = await createCompany(db, "Source"); const targetCompany = await createCompany(db, "Target"); diff --git a/server/src/__tests__/company-skills-import-authz-routes.test.ts b/server/src/__tests__/company-skills-import-authz-routes.test.ts new file mode 100644 index 0000000000..2942cb3de2 --- /dev/null +++ b/server/src/__tests__/company-skills-import-authz-routes.test.ts @@ -0,0 +1,202 @@ +import { randomUUID } from "node:crypto"; +import { promises as fs } from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import express from "express"; +import request from "supertest"; +import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { + activityLog, + agents, + authUsers, + companies, + companyMemberships, + companySkills, + createDb, + heartbeatRuns, + principalPermissionGrants, +} from "@paperclipai/db"; +import { + getEmbeddedPostgresTestSupport, + startEmbeddedPostgresTestDatabase, +} from "./helpers/embedded-postgres.js"; +import { createLocalAgentJwt } from "../agent-auth-jwt.js"; +import { actorMiddleware } from "../middleware/auth.js"; +import { errorHandler } from "../middleware/error-handler.js"; +import { companySkillRoutes } from "../routes/company-skills.js"; + +const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); +const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe.sequential : describe.skip; + +if (!embeddedPostgresSupport.supported) { + console.warn( + `Skipping embedded Postgres company skill import auth route tests on this host: ${embeddedPostgresSupport.reason ?? "unsupported environment"}`, + ); +} + +describeEmbeddedPostgres("company skill import authorization routes", () => { + let db!: ReturnType; + let tempDb: Awaited> | null = null; + let paperclipHome: string | null = null; + const cleanupDirs = new Set(); + const previousAgentJwtSecret = process.env.PAPERCLIP_AGENT_JWT_SECRET; + const previousPaperclipHome = process.env.PAPERCLIP_HOME; + + beforeAll(async () => { + process.env.PAPERCLIP_AGENT_JWT_SECRET = "company-skills-import-authz-test-secret"; + paperclipHome = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-company-skills-import-authz-home-")); + process.env.PAPERCLIP_HOME = paperclipHome; + tempDb = await startEmbeddedPostgresTestDatabase("paperclip-company-skills-import-authz-"); + db = createDb(tempDb.connectionString); + }, 20_000); + + afterEach(async () => { + await db.delete(activityLog); + await db.delete(principalPermissionGrants); + await db.delete(companyMemberships); + await db.delete(companySkills); + await db.delete(heartbeatRuns); + await db.delete(agents); + await db.delete(companies); + await db.delete(authUsers); + await Promise.all(Array.from(cleanupDirs, (dir) => fs.rm(dir, { recursive: true, force: true }))); + cleanupDirs.clear(); + }); + + afterAll(async () => { + await tempDb?.cleanup(); + if (paperclipHome) { + await fs.rm(paperclipHome, { recursive: true, force: true }); + } + if (previousAgentJwtSecret === undefined) delete process.env.PAPERCLIP_AGENT_JWT_SECRET; + else process.env.PAPERCLIP_AGENT_JWT_SECRET = previousAgentJwtSecret; + if (previousPaperclipHome === undefined) delete process.env.PAPERCLIP_HOME; + else process.env.PAPERCLIP_HOME = previousPaperclipHome; + }); + + function authenticatedApp() { + const instance = express(); + instance.use(express.json()); + instance.use(actorMiddleware(db, { deploymentMode: "authenticated" })); + instance.use("/api", companySkillRoutes(db)); + instance.use(errorHandler); + return instance; + } + + async function writeSkillFixture() { + const skillDir = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-import-authz-skill-")); + cleanupDirs.add(skillDir); + await fs.writeFile( + path.join(skillDir, "SKILL.md"), + [ + "---", + "name: Import Authz Fixture", + "description: Route-level import authorization fixture.", + "---", + "", + "# Import Authz Fixture", + "", + ].join("\n"), + "utf8", + ); + return skillDir; + } + + async function seedGrantedAgentWithResponsibleUser() { + const [company] = await db.insert(companies).values({ + name: "Company Skill Import Authz", + issuePrefix: `IA${randomUUID().replace(/-/g, "").slice(0, 6)}`, + }).returning(); + const companyId = company!.id; + + const [agent] = await db.insert(agents).values({ + companyId, + name: "Skill Import Agent", + role: "ceo", + adapterType: "codex_local", + adapterConfig: {}, + runtimeConfig: {}, + permissions: { canCreateSkills: false }, + }).returning(); + const agentId = agent!.id; + + const responsibleUserId = `user-${randomUUID()}`; + await db.insert(authUsers).values({ + id: responsibleUserId, + name: "Responsible User", + email: `${responsibleUserId}@example.com`, + emailVerified: true, + image: null, + createdAt: new Date(), + updatedAt: new Date(), + }); + + await db.insert(companyMemberships).values([ + { + companyId, + principalType: "agent", + principalId: agentId, + status: "active", + membershipRole: "member", + }, + { + companyId, + principalType: "user", + principalId: responsibleUserId, + status: "active", + membershipRole: "operator", + }, + ]); + await db.insert(principalPermissionGrants).values({ + companyId, + principalType: "agent", + principalId: agentId, + permissionKey: "skills:create", + scope: null, + grantedByUserId: null, + }); + + const runId = randomUUID(); + await db.insert(heartbeatRuns).values({ + id: runId, + companyId, + agentId, + status: "running", + responsibleUserId, + }); + return { companyId, agent: agent!, responsibleUserId, runId }; + } + + it("lets a standard responsible-user agent JWT with skills:create import a company skill", async () => { + const { companyId, agent, responsibleUserId, runId } = await seedGrantedAgentWithResponsibleUser(); + const skillDir = await writeSkillFixture(); + const token = createLocalAgentJwt(agent.id, companyId, agent.adapterType, runId, responsibleUserId); + expect(token).toBeTruthy(); + + const res = await request(authenticatedApp()) + .post(`/api/companies/${companyId}/skills/import`) + .set("Authorization", `Bearer ${token}`) + .set("X-Paperclip-Run-Id", runId) + .send({ source: skillDir }); + + expect(res.status, JSON.stringify(res.body)).toBe(201); + expect(res.body.imported).toHaveLength(1); + expect(res.body.imported[0]).toMatchObject({ + slug: "import-authz-fixture", + name: "Import Authz Fixture", + sourceType: "local_path", + }); + + const [importActivity] = await db.select().from(activityLog); + expect(importActivity).toMatchObject({ + companyId, + actorType: "agent", + actorId: agent.id, + agentId: agent.id, + runId, + action: "company.skills_imported", + entityType: "company", + entityId: companyId, + }); + }); +}); diff --git a/server/src/services/authorization.ts b/server/src/services/authorization.ts index a2499b2fca..b57976cde8 100644 --- a/server/src/services/authorization.ts +++ b/server/src/services/authorization.ts @@ -474,6 +474,26 @@ function activeResponsibleUserCanAuthorizeIssueAction( ); } +function activeResponsibleUserCanAuthorizeAgentGrantedSkillChange( + action: AuthorizationAction, + membership: ResponsibleUserSnapshot["activeMembership"], + agentDecision: AuthorizationDecision, + actorAgentId: string | null | undefined, +) { + return Boolean( + action === "skill_config:update" && + membership && + membership.status === "active" && + membership.membershipRole !== "viewer" && + agentDecision.allowed && + (agentDecision.reason === "allow_direct_change" || agentDecision.reason === "allow_consented_change") && + agentDecision.grant?.principalType === "agent" && + agentDecision.grant.principalId === actorAgentId && + (agentDecision.grant.permissionKey === "skills:create" || + agentDecision.grant.permissionKey === "skills:suggest-changes"), + ); +} + function scopeBoolean(scope: Record | null | undefined, key: string) { return scope?.[key] === true; } @@ -1521,6 +1541,21 @@ export function authorizationService(db: Db) { }); } } + if (input.action === "agent_config:read") { + return decideWithAgentConfigReadGrant("user", input.actor.userId); + } + if (input.action === "agent_config:update") { + return decideWithProtectedChangeGrants("user", input.actor.userId, { + direct: "agents:configure", + suggest: "agents:suggest-changes", + }); + } + if (input.action === "skill_config:update") { + return decideWithProtectedChangeGrants("user", input.actor.userId, { + direct: "skills:create", + suggest: "skills:suggest-changes", + }); + } if (!permissionKey) { if ( input.action === "agent:read" || @@ -1568,21 +1603,6 @@ export function authorizationService(db: Db) { if (policyEffect.kind === "restricted") return denyRestrictedAssignmentPolicy(policyEffect); return grantDecision; } - if (input.action === "agent_config:read") { - return decideWithAgentConfigReadGrant("user", input.actor.userId); - } - if (input.action === "agent_config:update") { - return decideWithProtectedChangeGrants("user", input.actor.userId, { - direct: "agents:configure", - suggest: "agents:suggest-changes", - }); - } - if (input.action === "skill_config:update") { - return decideWithProtectedChangeGrants("user", input.actor.userId, { - direct: "skills:create", - suggest: "skills:suggest-changes", - }); - } return decidePrincipalGrant({ companyId, principalType: "user", @@ -1865,6 +1885,21 @@ export function authorizationService(db: Db) { ? "RESPONSIBLE_USER_UNAUTHORIZED" : "RESPONSIBLE_USER_UNAVAILABLE"; + if ( + activeResponsibleUserCanAuthorizeAgentGrantedSkillChange( + input.action, + snapshot.activeMembership, + agentDecision, + input.actor.agentId, + ) + ) { + // Skill mutations are governed by the agent's explicit skill-change + // grant. The responsible-user intersection still requires an active + // non-viewer user, but does not require duplicating that grant on the + // responsible user's board account for standard heartbeat JWTs. + return agentDecision; + } + const userDecision = snapshot.userExists && snapshot.activeMembership ? await decideBase({ ...input,