diff --git a/packages/skills-catalog/src/catalog-builder.test.ts b/packages/skills-catalog/src/catalog-builder.test.ts index 3ef41e55..63d3cae0 100644 --- a/packages/skills-catalog/src/catalog-builder.test.ts +++ b/packages/skills-catalog/src/catalog-builder.test.ts @@ -222,6 +222,86 @@ describe("skills catalog manifest", () => { expect(result.manifest.skills[0]?.files.map((file) => file.path)).toEqual(["SKILL.md", "scripts/run.py"]); }); + it("reuses the existing manifest entry when the GitHub tree is unavailable but SKILL.md can be fetched", async () => { + const packageDir = await createCatalogPackage(); + await writeReference(packageDir, "optional", "research", "remote-research", { + source: { + type: "github", + hostname: "github.com", + owner: "example", + repo: "remote-skill", + ref: "v1.0.0", + commit: "0123456789abcdef0123456789abcdef01234567", + path: "skills/remote-research", + }, + files: ["SKILL.md", "scripts/**"], + recommendedForRoles: ["researcher"], + tags: ["research"], + }); + await fs.mkdir(path.join(packageDir, "generated"), { recursive: true }); + await fs.writeFile( + path.join(packageDir, "generated", "catalog.json"), + formatCatalogManifest({ + schemaVersion: 1, + packageName: "@paperclipai/skills-catalog", + packageVersion: "0.3.1", + generatedAt: "2026-05-26T00:00:00.000Z", + skills: [{ + id: "paperclipai:optional:research:remote-research", + key: "paperclipai/optional/research/remote-research", + kind: "optional", + category: "research", + slug: "remote-research", + name: "Remote Research", + description: "Research recent discussion from a pinned upstream skill.", + path: "catalog/optional/research/remote-research", + entrypoint: "SKILL.md", + trustLevel: "scripts_executables", + compatibility: "compatible", + defaultInstall: false, + recommendedForRoles: ["researcher"], + requires: [], + tags: ["research"], + files: [ + { + path: "SKILL.md", + kind: "skill", + sizeBytes: 128, + sha256: "a".repeat(64), + }, + ], + contentHash: `sha256:${"c".repeat(64)}`, + source: { + type: "github", + hostname: "github.com", + owner: "example", + repo: "remote-skill", + ref: "v1.0.0", + commit: "0123456789abcdef0123456789abcdef01234567", + path: "skills/remote-research", + url: "https://github.com/example/remote-skill/tree/v1.0.0/skills/remote-research", + }, + }], + }), + "utf8", + ); + vi.stubGlobal("fetch", vi.fn(async (url: string) => { + if (url.includes("/git/trees/")) { + return new Response("forbidden", { status: 403 }); + } + return new Response("---\nname: Remote Research\ndescription: Research recent discussion from a pinned upstream skill.\n---\n"); + })); + + const result = await buildCatalogManifest({ + packageDir, + generatedAt: "2026-05-26T00:00:00.000Z", + }); + + expect(result.errors).toEqual([]); + expect(result.manifest.skills).toHaveLength(1); + expect(result.manifest.skills[0]?.files.map((file) => file.path)).toEqual(["SKILL.md"]); + }); + it("reports frontmatter, directory, uniqueness, and inventory errors together", async () => { const packageDir = await createCatalogPackage(); await writeSkill(packageDir, "bundled", "Bad_Category", "duplicate", { diff --git a/packages/skills-catalog/src/catalog-builder.ts b/packages/skills-catalog/src/catalog-builder.ts index a042e035..1bc9d578 100644 --- a/packages/skills-catalog/src/catalog-builder.ts +++ b/packages/skills-catalog/src/catalog-builder.ts @@ -392,8 +392,14 @@ async function buildReferencedCatalogSkill( const requires = readStringArrayField(descriptor.requires, "requires", prefix, errors); const tags = readStringArrayField(descriptor.tags, "tags", prefix, errors); - if (!files.some((file) => file.path === SKILL_ENTRYPOINT && file.kind === "skill")) { + const hasSkillEntrypoint = files.some((file) => file.path === SKILL_ENTRYPOINT && file.kind === "skill"); + if (!hasSkillEntrypoint) { errors.push(`${prefix} referenced inventory does not contain SKILL.md.`); + const nextErrors = errors.slice(errorStart); + if (fallbackSkill && canFallbackToExistingReferencedSkill(nextErrors)) { + errors.splice(errorStart, nextErrors.length); + return fallbackSkill; + } } if (!name || !description) { const nextErrors = errors.slice(errorStart); diff --git a/server/src/__tests__/authorization-service.test.ts b/server/src/__tests__/authorization-service.test.ts index 426ab9c8..12292707 100644 --- a/server/src/__tests__/authorization-service.test.ts +++ b/server/src/__tests__/authorization-service.test.ts @@ -6,6 +6,7 @@ import { companyMemberships, createDb, instanceUserRoles, + issueComments, issues, principalPermissionGrants, projects, @@ -124,6 +125,7 @@ describeEmbeddedPostgres("authorization service", () => { }, 20_000); afterEach(async () => { + await db.delete(issueComments); await db.delete(principalPermissionGrants); await db.delete(companyMemberships); await db.delete(instanceUserRoles); @@ -619,6 +621,212 @@ describeEmbeddedPostgres("authorization service", () => { }); }); + it("allows mentioned agents to read and comment on assigned issues without granting issue mutation", async () => { + const company = await createCompany(db, "MentionCommentAuth"); + const allowedProject = await createProject(db, company.id, "MentionAllowed"); + const targetProject = await createProject(db, company.id, "MentionTarget"); + const ownerAgent = await createAgent(db, company.id, { role: "engineer" }); + const mentionedAgent = await createAgent(db, company.id, { + role: "engineer", + permissions: { + trustPreset: LOW_TRUST_REVIEW_PRESET, + authorizationPolicy: { + trustBoundary: { + mode: LOW_TRUST_REVIEW_PRESET, + companyId: company.id, + projectIds: [allowedProject.id], + }, + }, + }, + }); + const issue = await createIssue(db, company.id, { + title: "Mention-scoped comment target", + projectId: targetProject.id, + assigneeAgentId: ownerAgent.id, + }); + + const authorization = authorizationService(db); + const actor = { type: "agent", agentId: mentionedAgent.id, companyId: company.id, source: "agent_key" } as const; + const resource = { + type: "issue", + companyId: company.id, + issueId: issue.id, + projectId: issue.projectId, + assigneeAgentId: ownerAgent.id, + status: issue.status, + } as const; + + await expect(authorization.decide({ + actor, + action: "issue:comment", + resource, + })).resolves.toMatchObject({ allowed: false, reason: "deny_low_trust_boundary" }); + + const deletedMention = await db.insert(issueComments).values({ + companyId: company.id, + issueId: issue.id, + body: `[@Mentioned Agent](agent://${mentionedAgent.id}) this deleted comment should not count`, + deletedAt: new Date(), + }).returning().then((rows) => rows[0]!); + expect(deletedMention.id).toBeTruthy(); + + await expect(authorization.decide({ + actor, + action: "issue:comment", + resource, + })).resolves.toMatchObject({ allowed: false, reason: "deny_low_trust_boundary" }); + + await db.insert(issueComments).values({ + companyId: company.id, + issueId: issue.id, + body: `[@Mentioned Agent](agent://${mentionedAgent.id}) please respond here`, + authorAgentId: ownerAgent.id, + }); + + await expect(authorization.decide({ + actor, + action: "issue:read", + resource, + })).resolves.toMatchObject({ + allowed: true, + reason: "allow_issue_mention_grant", + }); + await expect(authorization.decide({ + actor, + action: "issue:comment", + resource, + })).resolves.toMatchObject({ + allowed: true, + reason: "allow_issue_mention_grant", + }); + await expect(authorization.decide({ + actor, + action: "issue:mutate", + resource, + })).resolves.toMatchObject({ allowed: false, reason: "deny_low_trust_boundary" }); + }); + + it("does not grant mention-scoped issue access from self-authored or unauthorized-author comments", async () => { + const company = await createCompany(db, "MentionCommentDenied"); + const allowedProject = await createProject(db, company.id, "MentionDeniedAllowed"); + const targetProject = await createProject(db, company.id, "MentionDeniedTarget"); + const ownerAgent = await createAgent(db, company.id, { role: "engineer" }); + const mentionedAgent = await createAgent(db, company.id, { + role: "engineer", + permissions: { + trustPreset: LOW_TRUST_REVIEW_PRESET, + authorizationPolicy: { + trustBoundary: { + mode: LOW_TRUST_REVIEW_PRESET, + companyId: company.id, + projectIds: [allowedProject.id], + }, + }, + }, + }); + const unrelatedAgent = await createAgent(db, company.id, { role: "engineer" }); + const issue = await createIssue(db, company.id, { + title: "Mention-scoped comment denial target", + projectId: targetProject.id, + assigneeAgentId: ownerAgent.id, + }); + + const authorization = authorizationService(db); + const actor = { type: "agent", agentId: mentionedAgent.id, companyId: company.id, source: "agent_key" } as const; + const resource = { + type: "issue", + companyId: company.id, + issueId: issue.id, + projectId: issue.projectId, + assigneeAgentId: ownerAgent.id, + status: issue.status, + } as const; + + await db.insert(issueComments).values([ + { + companyId: company.id, + issueId: issue.id, + body: `Self mention [@Mentioned Agent](agent://${mentionedAgent.id})`, + authorAgentId: mentionedAgent.id, + }, + { + companyId: company.id, + issueId: issue.id, + body: `Unauthorized mention [@Mentioned Agent](agent://${mentionedAgent.id})`, + authorAgentId: unrelatedAgent.id, + }, + ]); + + await expect(authorization.decide({ + actor, + action: "issue:read", + resource, + })).resolves.toMatchObject({ allowed: false, reason: "deny_low_trust_boundary" }); + await expect(authorization.decide({ + actor, + action: "issue:comment", + resource, + })).resolves.toMatchObject({ allowed: false, reason: "deny_low_trust_boundary" }); + }); + + it("allows active board-user comments to create mention-scoped issue grants", async () => { + const company = await createCompany(db, "MentionCommentBoardGrant"); + const allowedProject = await createProject(db, company.id, "MentionBoardAllowed"); + const targetProject = await createProject(db, company.id, "MentionBoardTarget"); + const ownerAgent = await createAgent(db, company.id, { role: "engineer" }); + const mentionedAgent = await createAgent(db, company.id, { + role: "engineer", + permissions: { + trustPreset: LOW_TRUST_REVIEW_PRESET, + authorizationPolicy: { + trustBoundary: { + mode: LOW_TRUST_REVIEW_PRESET, + companyId: company.id, + projectIds: [allowedProject.id], + }, + }, + }, + }); + const issue = await createIssue(db, company.id, { + title: "Mention-scoped board grant target", + projectId: targetProject.id, + assigneeAgentId: ownerAgent.id, + }); + const boardUserId = `user-${randomUUID()}`; + await db.insert(companyMemberships).values({ + companyId: company.id, + principalType: "user", + principalId: boardUserId, + status: "active", + membershipRole: "member", + }); + await db.insert(issueComments).values({ + companyId: company.id, + issueId: issue.id, + body: `Board mention [@Mentioned Agent](agent://${mentionedAgent.id})`, + authorUserId: boardUserId, + }); + + const actor = { type: "agent", agentId: mentionedAgent.id, companyId: company.id, source: "agent_key" } as const; + const resource = { + type: "issue", + companyId: company.id, + issueId: issue.id, + projectId: issue.projectId, + assigneeAgentId: ownerAgent.id, + status: issue.status, + } as const; + + await expect(authorizationService(db).decide({ + actor, + action: "issue:comment", + resource, + })).resolves.toMatchObject({ + allowed: true, + reason: "allow_issue_mention_grant", + }); + }); + it("limits viewer members to read-only visibility actions", async () => { const company = await createCompany(db, "BoardViewerVisibility"); const userId = `user-${randomUUID()}`; diff --git a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts index 83271022..701f3d50 100644 --- a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts +++ b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts @@ -23,6 +23,7 @@ const mockIssueService = vi.hoisted(() => ({ getWakeableParentAfterChildCompletion: vi.fn(), list: vi.fn(), listAttachments: vi.fn(), + listComments: vi.fn(), listWakeableBlockedDependents: vi.fn(), remove: vi.fn(), removeAttachment: vi.fn(), @@ -67,6 +68,7 @@ const mockStorageService = vi.hoisted(() => ({ const mockIssueThreadInteractionService = vi.hoisted(() => ({ expireRequestConfirmationsSupersededByComment: vi.fn(async () => []), expireStaleRequestConfirmationsForIssueDocument: vi.fn(async () => []), + expireRequestConfirmationsSupersededByHistoricalComments: vi.fn(async () => []), listForIssue: vi.fn(async () => []), })); const mockIssueApprovalService = vi.hoisted(() => ({ @@ -339,12 +341,14 @@ describe("agent issue mutation checkout ownership", () => { mockAccessService.decide.mockImplementation(async (input: { action: string }) => ({ allowed: input.action === "tasks:assign" || + input.action === "issue:comment" || input.action === "issue:read" || input.action === "issue:mutate" || input.action === "company_scope:read", action: input.action, reason: input.action === "tasks:assign" || + input.action === "issue:comment" || input.action === "issue:read" || input.action === "issue:mutate" || input.action === "company_scope:read" @@ -352,6 +356,7 @@ describe("agent issue mutation checkout ownership", () => { : "deny_missing_grant", explanation: input.action === "tasks:assign" || + input.action === "issue:comment" || input.action === "issue:read" || input.action === "issue:mutate" || input.action === "company_scope:read" @@ -375,7 +380,16 @@ describe("agent issue mutation checkout ownership", () => { mockIssueService.getWakeableParentAfterChildCompletion.mockReset(); mockIssueService.list.mockReset(); mockIssueService.listAttachments.mockReset(); + mockIssueService.listComments.mockReset(); mockIssueService.listWakeableBlockedDependents.mockReset(); + mockIssueThreadInteractionService.expireRequestConfirmationsSupersededByComment.mockReset(); + mockIssueThreadInteractionService.expireRequestConfirmationsSupersededByComment.mockResolvedValue([]); + mockIssueThreadInteractionService.expireStaleRequestConfirmationsForIssueDocument.mockReset(); + mockIssueThreadInteractionService.expireStaleRequestConfirmationsForIssueDocument.mockResolvedValue([]); + mockIssueThreadInteractionService.expireRequestConfirmationsSupersededByHistoricalComments.mockReset(); + mockIssueThreadInteractionService.expireRequestConfirmationsSupersededByHistoricalComments.mockResolvedValue([]); + mockIssueThreadInteractionService.listForIssue.mockReset(); + mockIssueThreadInteractionService.listForIssue.mockResolvedValue([]); mockIssueRecoveryActionService.getActiveForIssue.mockReset(); mockIssueRecoveryActionService.getActiveForIssue.mockResolvedValue(null); mockIssueRecoveryActionService.listActiveForIssues.mockReset(); @@ -530,6 +544,14 @@ describe("agent issue mutation checkout ownership", () => { body: "comment", }); mockIssueService.listAttachments.mockResolvedValue([]); + mockIssueService.listComments.mockResolvedValue([ + { + id: "comment-1", + issueId, + companyId, + body: "Mentioned reply context.", + }, + ]); mockIssueService.remove.mockResolvedValue(makeIssue({ status: "cancelled" })); mockIssueService.getAttachmentById.mockResolvedValue({ id: "attachment-1", @@ -636,7 +658,6 @@ describe("agent issue mutation checkout ownership", () => { it.each([ ["patch", (app: express.Express) => request(app).patch(`/api/issues/${issueId}`).send({ title: "Blocked" })], ["delete", (app: express.Express) => request(app).delete(`/api/issues/${issueId}`)], - ["comment", (app: express.Express) => request(app).post(`/api/issues/${issueId}/comments`).send({ body: "blocked" })], [ "document upsert", (app: express.Express) => @@ -676,6 +697,148 @@ describe("agent issue mutation checkout ownership", () => { expect(mockStorageService.deleteObject).not.toHaveBeenCalled(); }); + it("allows mentioned peer agents to post comments without ownership of an active checkout", async () => { + mockAccessService.decide.mockImplementation(async (input: { action: string }) => ({ + allowed: input.action === "issue:comment", + action: input.action, + reason: input.action === "issue:comment" ? "allow_issue_mention_grant" : "deny_missing_grant", + explanation: + input.action === "issue:comment" + ? "Allowed by a mention-scoped issue comment grant." + : "Missing permission.", + })); + + const res = await request(await createApp(peerActor())) + .post(`/api/issues/${issueId}/comments`) + .send({ body: "I can respond here." }); + + expect(res.status, JSON.stringify(res.body)).toBe(201); + expect(mockIssueService.addComment).toHaveBeenCalledWith( + issueId, + "I can respond here.", + expect.any(Object), + expect.any(Object), + ); + expect(mockIssueService.update).not.toHaveBeenCalled(); + }); + + it("rejects non-mentioned peer agents from posting comments", async () => { + mockAccessService.decide.mockImplementation(async (input: { action: string }) => ({ + allowed: input.action === "issue:read", + action: input.action, + reason: input.action === "issue:read" ? "allow_explicit_grant" : "deny_missing_grant", + explanation: input.action === "issue:read" ? "Allowed by test read grant." : "Missing permission.", + })); + + const res = await request(await createApp(peerActor())) + .post(`/api/issues/${issueId}/comments`) + .send({ body: "I was not mentioned." }); + + expect(res.status, JSON.stringify(res.body)).toBe(403); + expect(res.body.error).toBe("Issue is outside this actor's authorization boundary"); + expect(mockIssueService.addComment).not.toHaveBeenCalled(); + }); + + it("rejects peer agents from listing comments when issue read is outside their boundary", async () => { + mockAccessService.decide.mockImplementation(async (input: { action: string }) => ({ + allowed: false, + action: input.action, + reason: "deny_low_trust_boundary", + explanation: "Issue is outside this low-trust boundary.", + })); + + const res = await request(await createApp(peerActor())) + .get(`/api/issues/${issueId}/comments`); + + expect(res.status, JSON.stringify(res.body)).toBe(403); + expect(res.body.error).toBe("Issue is outside this actor's authorization boundary"); + expect(mockAccessService.decide).toHaveBeenCalledWith(expect.objectContaining({ action: "issue:read" })); + }); + + it("rejects peer agents from listing interactions when issue read is outside their boundary", async () => { + mockAccessService.decide.mockImplementation(async (input: { action: string }) => ({ + allowed: false, + action: input.action, + reason: "deny_low_trust_boundary", + explanation: "Issue is outside this low-trust boundary.", + })); + + const res = await request(await createApp(peerActor())) + .get(`/api/issues/${issueId}/interactions`); + + expect(res.status, JSON.stringify(res.body)).toBe(403); + expect(res.body.error).toBe("Issue is outside this actor's authorization boundary"); + expect(mockAccessService.decide).toHaveBeenCalledWith(expect.objectContaining({ action: "issue:read" })); + expect(mockIssueThreadInteractionService.listForIssue).not.toHaveBeenCalled(); + }); + + it("allows mentioned peer agents to list comments through an issue read grant", async () => { + mockAccessService.decide.mockImplementation(async (input: { action: string }) => ({ + allowed: input.action === "issue:read", + action: input.action, + reason: input.action === "issue:read" ? "allow_issue_mention_grant" : "deny_missing_grant", + explanation: + input.action === "issue:read" + ? "Allowed by a mention-scoped issue comment grant." + : "Missing permission.", + })); + + const res = await request(await createApp(peerActor())) + .get(`/api/issues/${issueId}/comments`); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(res.body).toEqual([ + expect.objectContaining({ + id: "comment-1", + body: "Mentioned reply context.", + }), + ]); + expect(mockAccessService.decide).toHaveBeenCalledWith(expect.objectContaining({ action: "issue:read" })); + expect(mockIssueService.listComments).toHaveBeenCalledWith(issueId, { + afterCommentId: null, + order: "desc", + limit: null, + }); + }); + + it("keeps true issue mutations denied for mentioned peer agents", async () => { + mockIssueService.getById.mockResolvedValue(makeIssue({ status: "todo", assigneeAgentId: ownerAgentId })); + mockAccessService.decide.mockImplementation(async (input: { action: string }) => ({ + allowed: input.action === "issue:comment" || input.action === "issue:mutate", + action: input.action, + reason: + input.action === "issue:comment" + ? "allow_issue_mention_grant" + : input.action === "issue:mutate" + ? "allow_explicit_grant" + : "deny_missing_grant", + explanation: + input.action === "issue:comment" + ? "Allowed by a mention-scoped issue comment grant." + : input.action === "issue:mutate" + ? "Allowed by test boundary default." + : "Missing permission.", + })); + + const res = await request(await createApp(peerActor())) + .patch(`/api/issues/${issueId}`) + .send({ status: "done" }); + + expect(res.status, JSON.stringify(res.body)).toBe(403); + expect(res.body.error).toBe("Agent cannot mutate another agent's issue"); + expect(mockIssueService.update).not.toHaveBeenCalled(); + }); + + it("denies cross-company agents before comment authorization is evaluated", async () => { + const res = await request(await createApp(peerActor({ companyId: "99999999-9999-4999-8999-999999999999" }))) + .post(`/api/issues/${issueId}/comments`) + .send({ body: "Wrong company." }); + + expect(res.status, JSON.stringify(res.body)).toBe(403); + expect(mockAccessService.decide).not.toHaveBeenCalledWith(expect.objectContaining({ action: "issue:comment" })); + expect(mockIssueService.addComment).not.toHaveBeenCalled(); + }); + it("rejects the checked-out owner without a run id on attachment upload (401)", async () => { // Regression: an agent-authenticated client (e.g. the CLI's attachment:upload) // that fails to send X-Paperclip-Run-Id must be rejected — mutating your own @@ -1074,7 +1237,6 @@ describe("agent issue mutation checkout ownership", () => { it.each([ ["todo", "patch", (app: express.Express) => request(app).patch(`/api/issues/${issueId}`).send({ title: "Todo update" })], - ["todo", "comment", (app: express.Express) => request(app).post(`/api/issues/${issueId}/comments`).send({ body: "Todo noise" })], ["blocked", "patch", (app: express.Express) => request(app).patch(`/api/issues/${issueId}`).send({ title: "Blocked update" })], ])("rejects peer agent %s issue %s mutations outside active checkout ownership", async (status, _kind, sendRequest) => { mockIssueService.getById.mockResolvedValue(makeIssue({ status: status as "todo" | "blocked", assigneeAgentId: ownerAgentId })); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 6aa2c3c8..2565eb3f 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -1844,7 +1844,7 @@ export function issueRoutes( assigneeUserId: string | null; status: string; }, - action: "issue:read" | "issue:mutate", + action: "issue:comment" | "issue:read" | "issue:mutate", ) { return access.decide({ actor: req.actor, @@ -1876,6 +1876,48 @@ export function issueRoutes( return false; } + async function assertAgentIssueCommentAllowed( + req: Request, + res: Response, + issue: { + id: string; + companyId: string; + projectId: string | null; + parentId: string | null; + status: string; + assigneeAgentId: string | null; + assigneeUserId: string | null; + }, + ) { + if (req.actor.type !== "agent") return true; + const actorAgentId = req.actor.agentId; + if (!actorAgentId) { + res.status(403).json({ error: "Agent authentication required" }); + return false; + } + const watchdogScope = await resolveTaskWatchdogMutationScope(db, req.actor); + if (watchdogScope.kind !== "none") { + const scopeResult = await taskWatchdogScopeAllowsIssueMutation(db, watchdogScope, issue); + if (scopeResult.kind === "invalid") { + res.status(403).json({ + error: scopeResult.detail, + details: { + issueId: issue.id, + securityPrinciples: ["Least Privilege", "Complete Mediation", "Fail Securely"], + }, + }); + return false; + } + return assertFreshTaskWatchdogSourceMutation(res, watchdogScope, issue); + } + const boundaryDecision = await decideIssueAccess(req, issue, "issue:comment"); + if (!boundaryDecision.allowed) { + res.status(403).json({ error: "Issue is outside this actor's authorization boundary" }); + return false; + } + return true; + } + async function filterIssuesForActor[1]>(req: Request, rows: T[]) { const decisions = await Promise.all(rows.map((issue) => decideIssueAccess(req, issue, "issue:read"))); return rows.filter((_, index) => decisions[index]?.allowed); @@ -6703,6 +6745,7 @@ export function issueRoutes( return; } assertCompanyAccess(req, issue.companyId); + if (!(await assertIssueReadAllowed(req, res, issue))) return; const afterCommentId = typeof req.query.after === "string" && req.query.after.trim().length > 0 ? req.query.after.trim() @@ -6737,6 +6780,7 @@ export function issueRoutes( return; } assertCompanyAccess(req, issue.companyId); + if (!(await assertIssueReadAllowed(req, res, issue))) return; const actor = getActorInfo(req); const interactionSvc = issueThreadInteractionService(db); const expiredInteractions = await interactionSvc.expireRequestConfirmationsSupersededByHistoricalComments(issue); @@ -7314,7 +7358,7 @@ export function issueRoutes( return; } assertCompanyAccess(req, issue.companyId); - if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return; + if (!(await assertAgentIssueCommentAllowed(req, res, issue))) return; if (!assertStructuredCommentFieldsAllowed(req, res, { presentation: req.body.presentation, metadata: req.body.metadata, @@ -7329,12 +7373,20 @@ export function issueRoutes( const reopenRequested = req.body.reopen === true; const resumeRequested = req.body.resume === true; const interruptRequested = req.body.interrupt === true; + const isClosed = isClosedIssueStatus(issue.status); + const isBlocked = issue.status === "blocked"; + if ( + isClosed && + req.actor.type === "agent" && + issue.assigneeAgentId !== null && + issue.assigneeAgentId !== req.actor.agentId + ) { + if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return; + } if (resumeRequested === true && !(await assertExplicitResumeIntentAllowed(req, res, issue))) return; if (resumeRequested !== true && reopenRequested === true && req.actor.type === "agent") { if (!(await assertExplicitResumeIntentAllowed(req, res, issue))) return; } - const isClosed = isClosedIssueStatus(issue.status); - const isBlocked = issue.status === "blocked"; const explicitMoveToTodoRequested = reopenRequested || resumeRequested === true; const scheduledRetryForHumanComment = shouldHumanCommentResumeInProgressScheduledRetry({ diff --git a/server/src/services/authorization.ts b/server/src/services/authorization.ts index 50aff187..a4c51300 100644 --- a/server/src/services/authorization.ts +++ b/server/src/services/authorization.ts @@ -1,22 +1,24 @@ -import { and, eq, sql } from "drizzle-orm"; +import { and, eq, inArray, isNull, sql } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; import { agents, companyMemberships, heartbeatRuns, instanceUserRoles, + issueComments, issues, principalPermissionGrants, projects, } from "@paperclipai/db"; import type { PermissionKey, PrincipalType } from "@paperclipai/shared"; -import { LOW_TRUST_REVIEW_PRESET, type LowTrustBoundary } from "@paperclipai/shared"; +import { LOW_TRUST_REVIEW_PRESET, extractAgentMentionIds, type LowTrustBoundary } from "@paperclipai/shared"; import { LOW_TRUST_ISSUE_ANCESTRY_MAX_DEPTH, isIssueWithinLowTrustBoundary, resolveCoreTrustPreset, type TrustPresetResolution, } from "./trust-preset-resolver.js"; +import { logger } from "../middleware/logger.js"; export type AuthorizationActor = { @@ -45,6 +47,7 @@ export type AuthorizationAction = | "agent:read" | "agent:wake" | "company_scope:read" + | "issue:comment" | "issue:mutate" | "issue:read" | "project:read" @@ -76,6 +79,7 @@ export type AuthorizationDecision = { | "allow_instance_admin" | "allow_explicit_grant" | "allow_legacy_agent_creator" + | "allow_issue_mention_grant" | "allow_self" | "allow_company_agent" | "allow_company_member" @@ -118,7 +122,7 @@ function permissionForAction(action: AuthorizationAction): PermissionKey | null ) { return null; } - if (action === "issue:mutate") return null; + if (action === "issue:comment" || action === "issue:mutate") return null; return action; } @@ -753,13 +757,27 @@ export function authorizationService(db: Db) { : lowTrustDeny("Project is outside this low-trust boundary."); } - if (input.action === "issue:read" || input.action === "issue:mutate") { + if (input.action === "issue:comment" || input.action === "issue:read" || input.action === "issue:mutate") { if (input.resource.type !== "issue") { return lowTrustDeny("Low-trust issue access is missing an issue resource."); } - return await issueResourceWithinLowTrustBoundary(boundary, input.resource) - ? lowTrustAllow("Allowed inside the low-trust issue boundary.") - : lowTrustDeny("Issue is outside this low-trust boundary."); + if (await issueResourceWithinLowTrustBoundary(boundary, input.resource)) { + return lowTrustAllow("Allowed inside the low-trust issue boundary."); + } + if ( + input.action !== "issue:mutate" && + input.resource.issueId && + await agentHasMentionGrantOnIssue({ + action: input.action, + companyId: boundary.companyId, + issueId: input.resource.issueId, + issueAssigneeAgentId: input.resource.assigneeAgentId ?? null, + actorAgentId: input.actorAgentId, + }) + ) { + return allowIssueMentionGrant(input.action); + } + return lowTrustDeny("Issue is outside this low-trust boundary."); } if (input.action === "tasks:assign") { @@ -850,6 +868,93 @@ export function authorizationService(db: Db) { return isAgentInSubtree(db, companyId, managerAgentId, assigneeAgentId); } + function commentAuthorCanGrantIssueMention(input: { + mentionedAgentId: string; + issueAssigneeAgentId: string | null; + authorAgentId: string | null; + authorUserId: string | null; + activeAuthorUserIds: Set; + }) { + if (input.authorAgentId) { + if (input.authorAgentId === input.mentionedAgentId) return false; + return input.issueAssigneeAgentId === input.authorAgentId; + } + if (input.authorUserId) { + return input.activeAuthorUserIds.has(input.authorUserId); + } + return false; + } + + async function agentHasMentionGrantOnIssue(input: { + action: AuthorizationAction; + companyId: string; + issueId: string; + issueAssigneeAgentId: string | null; + actorAgentId: string; + }) { + const rows = await db + .select({ + id: issueComments.id, + body: issueComments.body, + authorAgentId: issueComments.authorAgentId, + authorUserId: issueComments.authorUserId, + }) + .from(issueComments) + .where(and( + eq(issueComments.companyId, input.companyId), + eq(issueComments.issueId, input.issueId), + isNull(issueComments.deletedAt), + sql`${issueComments.body} LIKE ${"%agent://" + input.actorAgentId + "%"}`, + )); + + const mentionRows = rows.filter((row) => extractAgentMentionIds(row.body).includes(input.actorAgentId)); + const authorUserIds = [...new Set(mentionRows.flatMap((row) => row.authorUserId ? [row.authorUserId] : []))]; + const activeAuthorUserIds = new Set( + authorUserIds.length === 0 + ? [] + : await db + .select({ principalId: companyMemberships.principalId }) + .from(companyMemberships) + .where(and( + eq(companyMemberships.companyId, input.companyId), + eq(companyMemberships.principalType, "user"), + eq(companyMemberships.status, "active"), + inArray(companyMemberships.principalId, authorUserIds), + )) + .then((memberships) => memberships.map((membership) => membership.principalId)), + ); + + for (const row of mentionRows) { + const authorCanGrant = commentAuthorCanGrantIssueMention({ + mentionedAgentId: input.actorAgentId, + issueAssigneeAgentId: input.issueAssigneeAgentId, + authorAgentId: row.authorAgentId, + authorUserId: row.authorUserId, + activeAuthorUserIds, + }); + if (authorCanGrant) { + logger.info({ + actorAgentId: input.actorAgentId, + issueId: input.issueId, + companyId: input.companyId, + commentId: row.id, + grantedAction: input.action, + grant: "issue_mention_comment", + }, "authorized issue mention-scoped comment grant"); + return true; + } + } + return false; + } + + function allowIssueMentionGrant(action: AuthorizationAction): AuthorizationDecision { + return allow({ + action, + reason: "allow_issue_mention_grant", + explanation: "Allowed by a mention-scoped issue comment grant.", + }); + } + async function decide(input: { actor: AuthorizationActor; action: AuthorizationAction; @@ -956,7 +1061,10 @@ export function authorizationService(db: Db) { explanation: "Allowed by active cloud tenant company membership.", }); } - if (input.action === "issue:mutate" && membership.membershipRole !== "viewer") { + if ( + (input.action === "issue:comment" || input.action === "issue:mutate") && + membership.membershipRole !== "viewer" + ) { return allow({ action: input.action, reason: "allow_company_member", @@ -1092,6 +1200,7 @@ export function authorizationService(db: Db) { input.action === "agent:read" || input.action === "agent:wake" || input.action === "company_scope:read" || + input.action === "issue:comment" || input.action === "issue:read" || input.action === "project:read" || input.action === "runtime:manage" || @@ -1154,7 +1263,7 @@ export function authorizationService(db: Db) { }); } - if (input.action === "issue:mutate") { + if (input.action === "issue:comment" || input.action === "issue:mutate") { const resource = input.resource.type === "issue" ? input.resource : null; if (resource?.assigneeAgentId === actorAgentId) { return allow({ @@ -1170,6 +1279,19 @@ export function authorizationService(db: Db) { explanation: "Allowed because the issue has no agent assignee.", }); } + if ( + input.action === "issue:comment" && + resource?.issueId && + await agentHasMentionGrantOnIssue({ + action: input.action, + companyId, + issueId: resource.issueId, + issueAssigneeAgentId: resource.assigneeAgentId ?? null, + actorAgentId, + }) + ) { + return allowIssueMentionGrant(input.action); + } } if ( input.action === "agent_config:update" && diff --git a/skills/paperclip/references/api-reference.md b/skills/paperclip/references/api-reference.md index e50aad34..e87a52ec 100644 --- a/skills/paperclip/references/api-reference.md +++ b/skills/paperclip/references/api-reference.md @@ -243,6 +243,37 @@ Interpretation: There is **no separate execution-decision endpoint**. Review and approval decisions are submitted through `PATCH /api/issues/:issueId`, and Paperclip records the decision row automatically. +### Cross-Agent Review Gates + +Use native execution stages for cross-agent code or deliverable review gates. The gate belongs on the source issue's `executionPolicy.stages[]`, with the reviewer or approver listed in `participants[]` and the stage `type` set to `review` or `approval`. + +Minimal agent-review gate: + +```json +PATCH /api/issues/:issueId +{ + "executionPolicy": { + "stages": [ + { + "type": "review", + "participants": [ + { "type": "agent", "agentId": "" } + ] + } + ] + } +} +``` + +When the executor finishes work, move the source issue to `in_review`. Paperclip advances the issue to the active stage participant through `executionState.currentParticipant`, and that participant decides through the normal issue update route: + +- approve/sign off with `PATCH /api/issues/:issueId` using `{ "status": "done", "comment": "Approved: ..." }` +- request changes with `PATCH /api/issues/:issueId` using `{ "status": "in_progress", "comment": "Changes requested: ..." }` + +Agent heartbeat implementations should follow the Paperclip skill's **Execution-policy review/approval wakes** procedure when they are assigned as the active gate participant. + +Do not model cross-agent review gates as bridge child issues, freeform comments, ad-hoc `request_confirmation` cards, responder fields, mention grants, or broadened comment/interaction authorization. Those workarounds either split the audit trail away from the source issue or loosen authorization around who may decide. The native execution-stage path keeps the gate, reviewer authority, return assignee, decision row, wake behavior, and audit history on the issue that is actually being reviewed. + --- ## Worked Example: IC Heartbeat