diff --git a/server/src/__tests__/agent-commentary-reader.test.ts b/server/src/__tests__/agent-commentary-reader.test.ts index cd23f5871a2..0ee4dbe0f43 100644 --- a/server/src/__tests__/agent-commentary-reader.test.ts +++ b/server/src/__tests__/agent-commentary-reader.test.ts @@ -128,9 +128,21 @@ describe("agent commentary browsing", () => { for (const filters of ["limit=101", "offset=-1", "kind=other", "agentId=bad-id"]) { expect((await request(app).get(`${url}?${filters}`)).status).toBe(400); } + // Agents read only their own commentary unless granted commentary:view. actor = { type: "agent", companyId, agentId: firstAgentId }; expect((await request(app).get(url)).status).toBe(403); + expect((await request(app).get(`${url}/${firstId}`)).status).toBe(200); + actor = { type: "agent", companyId, agentId: secondAgentId }; expect((await request(app).get(`${url}/${firstId}`)).status).toBe(403); + await db.insert(companyMemberships).values({ companyId, principalType: "agent", principalId: secondAgentId, status: "active", membershipRole: "member" }).onConflictDoNothing(); + await db.insert(principalPermissionGrants).values({ companyId, principalType: "agent", principalId: secondAgentId, permissionKey: "commentary:view" }); + try { + expect((await request(app).get(`${url}/${firstId}`)).status).toBe(200); + expect((await request(app).get(url)).status).toBe(200); + } finally { + await db.delete(principalPermissionGrants).where(eq(principalPermissionGrants.principalId, secondAgentId)); + await db.delete(companyMemberships).where(eq(companyMemberships.principalId, secondAgentId)); + } actor = { type: "board", source: "session", userId: "viewer", companyIds: [companyId] }; const denied = await request(app).get(url).expect(403); expect(denied.body.error).toContain("commentary:view"); diff --git a/server/src/__tests__/agent-commentary.integration.test.ts b/server/src/__tests__/agent-commentary.integration.test.ts index bc0655633d6..0bcdf7ea3ec 100644 --- a/server/src/__tests__/agent-commentary.integration.test.ts +++ b/server/src/__tests__/agent-commentary.integration.test.ts @@ -7,7 +7,7 @@ import { fileURLToPath } from "node:url"; import { and, eq, sql } from "drizzle-orm"; import express from "express"; import request from "supertest"; -import { activityLog, agentApiKeys, agentCommentary, agents, authUsers, companyMemberships, heartbeatRuns, issues, nativeRunResults } from "@paperclipai/db"; +import { activityLog, agentApiKeys, agentCommentary, agents, authUsers, companies, companyMemberships, heartbeatRuns, issues, nativeRunResults, principalPermissionGrants } from "@paperclipai/db"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; import { createLocalAgentJwt } from "../agent-auth-jwt.js"; import { submitAgentCommentary } from "../services/agent-commentary.js"; @@ -19,6 +19,7 @@ import { agentCommentaryRoutes } from "../routes/agent-commentary.js"; import { errorHandler } from "../middleware/error-handler.js"; import { agentService } from "../services/agents.js"; import { companyService } from "../services/companies.js"; +import { NEW_STANDARD_AGENT_DEFAULT_GRANT_KEYS } from "../lib/agent-permissions.js"; describe("internal agent commentary through both transports", () => { let server: Awaited>; @@ -125,6 +126,38 @@ describe("internal agent commentary through both transports", () => { expect(await rows(f)).toHaveLength(0); }); + it("lets an agent read only its own commentary unless granted commentary:view, and names the human page when denied", async () => { + const f = await legacy(); + expect((await post(f, { ...input, kind: "complaint" })).status).toBe(201); + const [own] = await rows(f); + const [company] = await server.db.select({ issuePrefix: companies.issuePrefix }).from(companies).where(eq(companies.id, f.companyId)); + const get = (path: string, companyId = f.companyId) => fetch(`${server.apiUrl}/api/companies/${companyId}/agent-commentary${path}`, { headers: { Authorization: `Bearer ${f.token}` } }); + const denied = async (response: Response) => { + expect(response.status).toBe(403); + const body = await response.json() as { error: string; remediation?: string }; + expect(body.error).toBe("Board access required"); + expect(body.remediation).toContain(`[Commentary](/${company!.issuePrefix}/activity/commentary)`); + }; + // Reading other agents' commentary is never a default agent grant. + expect(NEW_STANDARD_AGENT_DEFAULT_GRANT_KEYS).not.toContain("commentary:view"); + // Default: other agents' feedback and the full list are denied; its own is readable. + await denied(await get("")); + await denied(await get(`?agentId=${randomUUID()}`)); + await denied(await get(`/${randomUUID()}`)); + const mine = await get(`?agentId=${f.agentId}`); + expect(mine.status).toBe(200); + expect(((await mine.json()) as { items: Array<{ id: string }> }).items.map(row => row.id)).toEqual([own!.id]); + expect((await get(`/${own!.id}`)).status).toBe(200); + // Another company's request fails on company access, with no page path. + const foreign = await get("", f.foreignCompanyId); + expect(foreign.status).toBe(403); + expect(((await foreign.json()) as { remediation?: string }).remediation).toBeUndefined(); + // The explicit grant allows reading everyone's commentary. + await server.db.insert(companyMemberships).values({ companyId: f.companyId, principalType: "agent", principalId: f.agentId, status: "active", membershipRole: "member" }).onConflictDoNothing(); + await server.db.insert(principalPermissionGrants).values({ companyId: f.companyId, principalType: "agent", principalId: f.agentId, permissionKey: "commentary:view" }); + expect((await get("")).status).toBe(200); + }); + it.each(["cancelled", "succeeded", "failed"])("rejects submissions and replay after the run is %s", async (status) => { const f = await legacy(); expect((await post(f, { ...input, kind: "complaint" })).status).toBe(201); diff --git a/server/src/__tests__/openapi-routes.test.ts b/server/src/__tests__/openapi-routes.test.ts index e04f4375914..baae442e78c 100644 --- a/server/src/__tests__/openapi-routes.test.ts +++ b/server/src/__tests__/openapi-routes.test.ts @@ -478,13 +478,13 @@ describe("openapi routes", () => { expect(operation.responses["404"]).toBeDefined(); }); - it("documents strict run-attributed feedback and board-only commentary reads", () => { + it("documents strict run-attributed feedback and board-or-agent commentary reads", () => { const { spec } = loadSpecRoutes(); const path = spec.paths["/api/companies/{companyId}/agent-commentary"]; expect(Object.keys(path)).toEqual(["get", "post"]); const detail = spec.paths["/api/companies/{companyId}/agent-commentary/{id}"].get; for (const read of [path.get, detail]) { - expect(read.security).toEqual([{ BoardSessionAuth: [] }, { BoardApiKeyAuth: [] }]); + expect(read.security).toEqual([{ BoardSessionAuth: [] }, { BoardApiKeyAuth: [] }, { AgentBearerAuth: [] }]); expect(read.responses["403"]).toBeDefined(); expect(read.responses["404"]).toBeDefined(); } diff --git a/server/src/routes/agent-commentary.ts b/server/src/routes/agent-commentary.ts index 35aa021c61b..6921ea58bc9 100644 --- a/server/src/routes/agent-commentary.ts +++ b/server/src/routes/agent-commentary.ts @@ -1,5 +1,6 @@ import { Router, type Request } from "express"; -import type { Db } from "@paperclipai/db"; +import { companies, type Db } from "@paperclipai/db"; +import { eq } from "drizzle-orm"; import { agentCommentaryQuerySchema, isUuidLike } from "@paperclipai/shared"; import { badRequest, forbidden, notFound } from "../errors.js"; import { submitAgentCommentary } from "../services/agent-commentary.js"; @@ -13,9 +14,23 @@ export function agentCommentaryRoutes(db: Db) { const reader = agentCommentaryReader(db); const access = accessService(db); - async function authorizeRead(req: Request, companyId: string) { + // Agents can submit commentary. They read only their own entries unless they + // hold commentary:view, which no agent has by default. A denial names the + // human page so the agent can send the person somewhere useful. + async function denyAgentRead(companyId: string): Promise { + const [company] = await db.select({ issuePrefix: companies.issuePrefix }).from(companies).where(eq(companies.id, companyId)).limit(1); + throw forbidden("Board access required", company ? { + remediation: `Agents read only their own commentary unless granted commentary:view. People with commentary access can read it on the Commentary page; give them this link: [Commentary](/${company.issuePrefix}/activity/commentary).`, + } : undefined); + } + + async function agentCanReadAll(req: Request, companyId: string) { + return req.actor.type === "agent" && !!req.actor.agentId + && await access.hasPermission(companyId, "agent", req.actor.agentId, "commentary:view"); + } + + async function authorizeBoardRead(req: Request, companyId: string) { assertBoard(req); - assertCompanyAccess(req, companyId); if (req.actor.source !== "local_implicit" && !req.actor.isInstanceAdmin && (!req.actor.userId || !await access.canUser(companyId, req.actor.userId, "commentary:view"))) { throw forbidden("Missing permission: commentary:view"); @@ -25,16 +40,32 @@ export function agentCommentaryRoutes(db: Db) { router.get("/companies/:companyId/agent-commentary", async (req, res) => { const companyId = req.params.companyId as string; - const readCondition = await authorizeRead(req, companyId); + assertCompanyAccess(req, companyId); const query = agentCommentaryQuerySchema.safeParse(req.query); if (!query.success) throw badRequest("Invalid agent commentary filters", query.error.issues); - res.json(await reader.list(companyId, query.data, readCondition)); + if (req.actor.type === "agent") { + // Without the grant, only an explicit request for its own entries is allowed. + if (!await agentCanReadAll(req, companyId) && query.data.agentId !== req.actor.agentId) await denyAgentRead(companyId); + res.json(await reader.list(companyId, query.data, await issueReadSqlCondition(db, req.actor))); + return; + } + if (req.actor.type !== "board") await denyAgentRead(companyId); + res.json(await reader.list(companyId, query.data, await authorizeBoardRead(req, companyId))); }); router.get("/companies/:companyId/agent-commentary/:id", async (req, res) => { const companyId = req.params.companyId as string; - const readCondition = await authorizeRead(req, companyId); + assertCompanyAccess(req, companyId); const id = req.params.id as string; + if (req.actor.type === "agent") { + const entry = isUuidLike(id) ? await reader.get(companyId, id, await issueReadSqlCondition(db, req.actor)) : null; + if (!await agentCanReadAll(req, companyId) && entry?.agent.id !== req.actor.agentId) await denyAgentRead(companyId); + if (!entry) throw notFound("Commentary not found"); + res.json(entry); + return; + } + if (req.actor.type !== "board") await denyAgentRead(companyId); + const readCondition = await authorizeBoardRead(req, companyId); if (!isUuidLike(id)) throw notFound("Commentary not found"); const entry = await reader.get(companyId, id, readCondition); if (!entry) throw notFound("Commentary not found"); diff --git a/server/src/routes/openapi.ts b/server/src/routes/openapi.ts index 00b9cf811ad..8e7d23d4c00 100644 --- a/server/src/routes/openapi.ts +++ b/server/src/routes/openapi.ts @@ -1686,7 +1686,8 @@ function resolveOperationAuthLevel( if (PUBLIC_OPERATIONS.has(key)) return "public"; if (key === "POST /api/companies/{companyId}/agent-commentary") return "agent_heartbeat"; if (key === "POST /api/agents/me/chat/messages") return "agent_heartbeat"; - if (method.toUpperCase() === "GET" && path.startsWith("/api/companies/{companyId}/agent-commentary")) return "board"; + // Agents may read their own commentary, or all of it with commentary:view; the routes enforce that. + if (method.toUpperCase() === "GET" && path.startsWith("/api/companies/{companyId}/agent-commentary")) return "authenticated"; if (key === "POST /api/mcp/project-tools" || key === "POST /api/companies/{companyId}/slack/tasks/{issueId}/tools") return "agent_run"; if (RUNTIME_TOOLS_OPERATIONS.has(key)) return "runtime_tools"; if (INSTANCE_ADMIN_OPERATIONS.has(key)) return "instance_admin"; @@ -5810,7 +5811,7 @@ registry.registerPath({ path: "/api/companies/{companyId}/agent-commentary", tags: ["agents"], summary: "Browse agent suggestions and complaints", - description: "Board-only. Requires company membership and commentary:view, granted by default to all human company roles. Source-task privacy filters both previews and counts. Returns items, total, limit and offset; previews contain at most 280 characters and omit full bodies.", + description: "Board members need company membership and commentary:view, granted by default to all human company roles. An agent may list only its own commentary (agentId set to itself) unless it holds commentary:view, which is never a default agent grant; otherwise it gets 403 with a remediation naming the Commentary page. Source-task privacy filters both previews and counts. Returns items, total, limit and offset; previews contain at most 280 characters and omit full bodies.", request: { params: z.object({ companyId: z.string() }), query: agentCommentaryQuerySchema }, responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized, 403: r.forbidden, 404: r.notFound }, }); @@ -5820,7 +5821,7 @@ registry.registerPath({ path: "/api/companies/{companyId}/agent-commentary/{id}", tags: ["agents"], summary: "Read a full agent commentary message", - description: "Same board, permission and task-privacy gates as the list. Returns the full body with agent, task and run context. Foreign-company and invisible entries return 404.", + description: "Same permission and task-privacy gates as the list. An agent may read its own entry, or any entry with commentary:view; another agent's entry returns 403 with a remediation. Returns the full body with agent, task and run context. Foreign-company and invisible entries return 404.", request: { params: z.object({ companyId: z.string(), id: z.string().uuid() }) }, responses: { 200: r.ok(), 401: r.unauthorized, 403: r.forbidden, 404: r.notFound }, }); diff --git a/server/src/services/native-runtime/runner-api.test.ts b/server/src/services/native-runtime/runner-api.test.ts index ad6f68a3816..0c3f6640909 100644 --- a/server/src/services/native-runtime/runner-api.test.ts +++ b/server/src/services/native-runtime/runner-api.test.ts @@ -143,6 +143,9 @@ describe("runner API catalog", () => { expect(JSON.stringify(catalog)).not.toContain('"$ref"'); expect(runnerApiOperation("GET /api/companies/{companyId}/decisions").authorization.actor).toBe("board"); expect(runnerApiOperation("DELETE /api/issues/{id}/documents/{key}").authorization.actor).toBe("board"); + // Agents read their own commentary (or all with commentary:view), so the catalog must not say board-only. + expect(runnerApiOperation("GET /api/companies/{companyId}/agent-commentary").authorization.actor).toBe("board_or_agent"); + expect(runnerApiOperation("GET /api/companies/{companyId}/agent-commentary/{id}").authorization.actor).toBe("board_or_agent"); expect(runnerApiOperation("DELETE /api/issues/{id}/documents/{key}").dedicatedTools).toEqual([]); expect(runnerApiOperation(createProject).requestBody?.content["application/json"].schema.required).toContain("name"); expect(runnerApiOperation(createProject).dedicatedTools).toEqual(["create_project"]);