From 1a79522a4e634aaa577e02e0c5bf03f35f9dc435 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Fri, 23 Oct 2026 19:59:21 -0700 Subject: [PATCH] perf: narrow run-to-issue authorization reads (#17405) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Run pages show links to issues associated with a run. > - The issue-link route checks the run's company and issue visibility before it reads those links. > - That check needs four binding fields, but the route reads the full run. > - Full runs can include large prompt, output, and result fields. > - This pull request uses the existing narrow authorization read. > - The route transfers less database data and keeps its permission checks and response contract. ## Linked Issues or Issue Description **Problem or motivation** `GET /api/heartbeat-runs/:runId/issues` calls `heartbeat.getRun` for a permission check. Its consumers only need the run ID, company ID, scope kind, and issue ID. The full read also fetches context, output excerpts, result data, and runtime metadata. Repeated link reads pay for that unused data. **Proposed solution** Use `getRunReadAccess`, the existing four-field read from #17348. Keep authentication, company access, company-scope authorization, run issue visibility, and linked-issue filtering in their current owners. Keep the existing empty response for missing and inaccessible runs. **Alternatives considered** A new query contract is unnecessary because the existing read provides all required fields. A permission cache could become stale. Changing the full run reader could affect consumers that need run details. **Roadmap alignment** This is a focused improvement to an existing route. It adds no feature, route, provider, cache, or background work. Searches for run-issue links, narrow run authorization, and `getRunReadAccess` found no open duplicate. Related work: #17348. ## What Changed - Change the issue-link route to use the existing narrow run read. - Reject full run reads in the route tests. Cover readable company and issue scopes, denied issue access, missing bindings, anonymous requests, and the missing/cross-company empty response. - Add a developer note about the read contract. ## Verification - `pnpm --filter @paperclipai/server exec vitest run src/__tests__/activity-routes.test.ts src/services/heartbeat/run-state.test.ts src/__tests__/agent-live-run-routes.test.ts`: 126 tests passed. - `pnpm build`: passed. - `pnpm -r typecheck`: passed. - The route tests use the real run visibility helper with only the four binding fields. The full reader throws if a route calls it. The existing run-state suite checks the exact four-column selection and covers database wiring with disposable Postgres. - `git diff --check`: passed. Reviewed the committed diff for secrets and private identifiers before push. - Apex review: 5/5 on exact head `888a4ce9ea88bb2b284eb5e28fafc01b6f48b386`, completed successfully, with zero review threads. - Full current-head CI passed: 54 successful checks and two expected skips. [Workflow attempt 2](https://github.com/paperclipai/paperclip/actions/runs/40051399577/attempts/2). [Initial serialized shard failure](https://github.com/paperclipai/paperclip/actions/runs/40051399577/job/120744710081): the first company-history case in unchanged `agent-live-run-routes.test.ts` hit its 15-second timeout; the shard had 391 passing tests and one failure. That file, its agent route, the heartbeat state reader, and the service export are unchanged from base. The five company-history cases pass on unchanged base `9fb28d06ffebbb615f413765078aeeb7c4291311` (9.12s suite) and this head (8.29s suite). They also passed in the 126-test focused run. The timeout was not reproduced locally; its cause is not established. Only the failed shard was retried on the same head. It passed in attempt 2. This establishes recovery; the initial timeout remains in this record and its cause is unproven. - The prior local full-suite failures and baseline triage remain documented in #17384. No new local full-suite pass is claimed here. This change uses focused local tests and the full current-head CI gate. - No hosted user journey, staging deployment, production deployment, or billed savings is claimed for this follow-up. ## Risks - Run visibility depends on the canonical `scopeKind` and `issueId`. The existing helper returns those same fields. It does not use prompt or result data for access decisions. - Linked issues still pass the existing SQL visibility filter. Missing and cross-company runs still return `200 []`. Anonymous requests still fail before the run read. - The database query count stays the same. The change reduces data selected for this authorization read. - No schema or API response changes. ## Model Used OpenAI Codex, GPT-6 family. The exact deployment model ID and context window are not exposed in this session. Used reasoning, repository tools, and code execution. ## 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 - [x] All Paperclip CI gates are green - [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 --- doc/DEVELOPING.md | 4 ++ server/src/__tests__/activity-routes.test.ts | 51 ++++++++++++++++++-- server/src/routes/activity.ts | 2 +- 3 files changed, 52 insertions(+), 5 deletions(-) diff --git a/doc/DEVELOPING.md b/doc/DEVELOPING.md index 0322ea4a90f..fd25491f0d4 100644 --- a/doc/DEVELOPING.md +++ b/doc/DEVELOPING.md @@ -2885,6 +2885,10 @@ Sandbox-provider driver lookup filters plugin manifests by driver key and kind in Postgres before returning rows. Install order, status and worker checks still select the provider. Each lookup reads current registry state without a cache. +Run-to-issue links authorize through `getRunReadAccess`, which reads only the run +ID, company ID, scope kind, and issue ID. The existing visibility policy then +filters linked issues without loading the run's prompt, output, or result. + Run the deterministic native event replay with the normal server test runner: ```sh diff --git a/server/src/__tests__/activity-routes.test.ts b/server/src/__tests__/activity-routes.test.ts index 4d654036ecd..578c8a2738a 100644 --- a/server/src/__tests__/activity-routes.test.ts +++ b/server/src/__tests__/activity-routes.test.ts @@ -12,6 +12,7 @@ const mockActivityService = vi.hoisted(() => ({ const mockHeartbeatService = vi.hoisted(() => ({ getRun: vi.fn(), + getRunReadAccess: vi.fn(), })); const mockIssueService = vi.hoisted(() => ({ @@ -67,6 +68,7 @@ async function createApp( source: "session", isInstanceAdmin: false, }, + database: unknown = {}, ) { vi.resetModules(); const [{ errorHandler }, { activityRoutes }] = await Promise.all([ @@ -82,7 +84,7 @@ async function createApp( }; next(); }); - app.use("/api", activityRoutes({} as any)); + app.use("/api", activityRoutes(database as any)); app.use(errorHandler); return app; } @@ -118,6 +120,9 @@ describe("activity routes", () => { beforeEach(() => { for (const mock of Object.values(mockActivityService)) mock.mockReset(); for (const mock of Object.values(mockHeartbeatService)) mock.mockReset(); + mockHeartbeatService.getRun.mockImplementation(() => { + throw new Error("Run issue links must use the narrow authorization read"); + }); for (const mock of Object.values(mockIssueService)) mock.mockReset(); mockAccessService.decide.mockReset(); mockAccessService.canUser.mockReset(); @@ -344,13 +349,15 @@ describe("activity routes", () => { }); it("passes task read filtering when listing links for a readable company run", async () => { - mockHeartbeatService.getRun.mockResolvedValue({ + mockHeartbeatService.getRunReadAccess.mockResolvedValue({ id: "run-1", companyId: "company-1", scopeKind: "company", issueId: null, }); mockActivityService.issuesForRun.mockResolvedValue([{ issueId: "public-issue", title: "Visible task" }]); const app = await createApp({ type: "agent", companyId: "company-1", agentId: "agent-1" }); const res = await requestApp(app, baseUrl => request(baseUrl).get("/api/heartbeat-runs/run-1/issues")); expect(res.status).toBe(200); + expect(mockHeartbeatService.getRunReadAccess).toHaveBeenCalledWith("run-1"); + expect(mockHeartbeatService.getRun).not.toHaveBeenCalled(); const { issueReadSqlCondition } = await import("../services/authorization.js"); expect(issueReadSqlCondition).toHaveBeenCalledWith({}, expect.objectContaining({ type: "agent", companyId: "company-1", agentId: "agent-1", @@ -360,7 +367,7 @@ describe("activity routes", () => { }); it("returns 200 [] (not 404) when listing issues for another company's run, preserving API contract and the cross-tenant oracle", async () => { - mockHeartbeatService.getRun.mockResolvedValue({ + mockHeartbeatService.getRunReadAccess.mockResolvedValue({ id: "run-2", companyId: "company-2", }); @@ -374,7 +381,7 @@ describe("activity routes", () => { }); it("returns 200 [] (not 404) for a non-existent heartbeat run, matching the cross-tenant response", async () => { - mockHeartbeatService.getRun.mockResolvedValue(null); + mockHeartbeatService.getRunReadAccess.mockResolvedValue(null); const app = await createApp(); const res = await requestApp(app, (baseUrl) => request(baseUrl).get("/api/heartbeat-runs/missing-run/issues")); @@ -390,6 +397,42 @@ describe("activity routes", () => { expect(res.status).toBe(401); expect(mockHeartbeatService.getRun).not.toHaveBeenCalled(); + expect(mockHeartbeatService.getRunReadAccess).not.toHaveBeenCalled(); expect(mockActivityService.issuesForRun).not.toHaveBeenCalled(); }); + + it.each([true, false])("preserves issue-scoped run authorization when read permission is %s", async (allowed) => { + mockHeartbeatService.getRunReadAccess.mockResolvedValue({ + id: "run-1", companyId: "company-1", scopeKind: "issue", issueId: "issue-private", + }); + mockAccessService.decide.mockImplementation(async ({ action }) => ({ + allowed: action === "issue:read" ? allowed : true, + })); + mockActivityService.issuesForRun.mockResolvedValue([{ issueId: "visible-issue" }]); + const db = { + select: () => ({ from: () => ({ where: async () => [{ id: "issue-private" }] }) }), + }; + const app = await createApp({ type: "agent", companyId: "company-1", agentId: "agent-1" }, db); + const res = await requestApp(app, baseUrl => request(baseUrl).get("/api/heartbeat-runs/run-1/issues")); + expect(res.status).toBe(200); + expect(res.body).toEqual(allowed ? [{ issueId: "visible-issue" }] : []); + expect(mockAccessService.decide).toHaveBeenCalledWith(expect.objectContaining({ + action: "issue:read", resource: expect.objectContaining({ companyId: "company-1", issueId: "issue-private" }), + })); + expect(mockHeartbeatService.getRunReadAccess).toHaveBeenCalledWith("run-1"); + expect(mockHeartbeatService.getRun).not.toHaveBeenCalled(); + expect(mockActivityService.issuesForRun).toHaveBeenCalledTimes(allowed ? 1 : 0); + }); + + it("does not read issue links for a run without a trusted issue binding", async () => { + mockHeartbeatService.getRunReadAccess.mockResolvedValue({ + id: "run-1", companyId: "company-1", scopeKind: "issue", issueId: null, + }); + const app = await createApp(); + const res = await requestApp(app, baseUrl => request(baseUrl).get("/api/heartbeat-runs/run-1/issues")); + expect(res.status).toBe(200); + expect(res.body).toEqual([]); + expect(mockActivityService.issuesForRun).not.toHaveBeenCalled(); + expect(mockHeartbeatService.getRun).not.toHaveBeenCalled(); + }); }); diff --git a/server/src/routes/activity.ts b/server/src/routes/activity.ts index 61e2e8b8756..5239562f7a6 100644 --- a/server/src/routes/activity.ts +++ b/server/src/routes/activity.ts @@ -372,7 +372,7 @@ export function activityRoutes(db: Db) { router.get("/heartbeat-runs/:runId/issues", async (req, res) => { assertAuthenticated(req); const runId = req.params.runId as string; - const run = await heartbeat.getRun(runId); + const run = await heartbeat.getRunReadAccess(runId); if (!run || !hasCompanyAccess(req, run.companyId)) { // Return `200 []` for both "doesn't exist" and "cross-tenant" — preserves the // legacy API contract while keeping the cross-tenant existence oracle closed