From 00ad9503fe3f5e4589143f79bba489ec91f976bb Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Sat, 7 Nov 2026 12:04:46 -0600 Subject: [PATCH] feat(commentary): agents read only their own commentary unless granted commentary:view (#19842) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Agents can submit complaints and suggestions as commentary, and people with commentary access read them on the Commentary page > - When a person asks an agent in chat what agents have been complaining about, the agent cannot read commentary and gets "Board access required" > - That response does not say where a person can see it, so the agent can name the feed but cannot link it > - Product rule: an agent cannot read other agents' commentary unless it holds `commentary:view`, which is off by default; it may read its own > - This pull request implements that rule and adds a remediation to the denial naming the human page for the agent's own company > - The benefit is a clear permission boundary and a working link for the person instead of a dead end ## Linked Issues or Issue Description **What happened?** In the `activity.agent-feedback` chat eval, the agent correctly said it could not read agent commentary and that only the person could open it, but it gave no link, because the denial named no page. **Expected behavior** An agent denied a commentary read learns which page a person with commentary access can open. **Steps to reproduce** 1. As an agent, call `GET /api/companies/{companyId}/agent-commentary`. 2. Before this change, the 403 body is only `{ "error": "Board access required" }`. ## What Changed - `server/src/routes/agent-commentary.ts`: - An agent with the `commentary:view` grant reads all commentary its task access allows. The grant is not among the default agent grants. - Without the grant, an agent may read its own entries: `?agentId=` on the list, or its own entry by ID. - Without the grant, listing all commentary, filtering by another agent, or reading another agent's entry returns 403 "Board access required". The response's `remediation` names `//activity/commentary` for the agent's own company. - A request for another company still fails on company access first, with no page path. - Board reads are unchanged: `commentary:view` for the user, plus task read conditions. - `agent-commentary.integration.test.ts`: one test covers the full-list and other-agent denials with the page path, own-list and own-entry reads, the cross-company denial, the grant allowing a full list, and that `commentary:view` is not a default agent grant. ## Verification - `npx vitest run src/__tests__/agent-commentary.integration.test.ts` in `server`: 21 tests passed. - The new test fails against the previous route. - Negative control: with the previous route, the new test fails. - Related commentary tests: `agent-commentary-reader`, `agent-commentary-helper`, `commentary-permission-migration` passed. `agent-commentary-skills` fails the same way with and without this change (a missing skill file in a temporary directory locally). - The chat eval rerun with this change is recorded in #19614. ## Risks - Agents can now read their own commentary when they ask for it explicitly; before, every agent read was denied. - The denial's status code and message are unchanged; a `remediation` string is added for same-company agents. - There is no UI yet for granting `commentary:view` to an agent. - An agent learns the company's issue prefix, which it already sees in task identifiers. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, with tool use and code execution in Claude Code. ## 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) - [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 - [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 - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- .../__tests__/agent-commentary-reader.test.ts | 12 ++++++ .../agent-commentary.integration.test.ts | 35 ++++++++++++++- server/src/__tests__/openapi-routes.test.ts | 4 +- server/src/routes/agent-commentary.ts | 43 ++++++++++++++++--- server/src/routes/openapi.ts | 7 +-- .../native-runtime/runner-api.test.ts | 3 ++ 6 files changed, 92 insertions(+), 12 deletions(-) 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"]);