From 100477e8876f51add368554643e073a2d1c1becf Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Fri, 23 Oct 2026 17:05:06 -0700 Subject: [PATCH] Avoid live workspace inspection in task context reads (#17396) --- doc/DEVELOPING.md | 6 ++++++ .../execution-workspaces-service.test.ts | 8 ++++++++ .../issue-closed-workspace-routes.test.ts | 5 +++-- .../issue-workspace-command-authz.test.ts | 5 +++-- .../__tests__/issues-goal-context-routes.test.ts | 16 +++++++++------- server/src/routes/issues.ts | 9 +++++---- server/src/services/execution-workspaces.ts | 7 +++++-- 7 files changed, 39 insertions(+), 17 deletions(-) diff --git a/doc/DEVELOPING.md b/doc/DEVELOPING.md index 527372b7f72..4e976e80ba4 100644 --- a/doc/DEVELOPING.md +++ b/doc/DEVELOPING.md @@ -107,6 +107,12 @@ and service tests do not establish that a live model followed the workflow. See [implementation and acceptance](plans/2026-10-19-persistent-case-handoffs.md) and the [agent case reference](../skills/paperclip/references/cases.md). +Task detail, heartbeat context, and closed-workspace admission guards read stored +workspace metadata. They do not inspect Git or refresh live runtime health. +Task context includes persisted runtime service state; delivery state is unknown +until an explicit workspace read or lifecycle inspection computes it. Use the +workspace detail and close-readiness APIs for current Git safety checks. + ## Deployment Modes For mode definitions and intended CLI behavior, see `doc/DEPLOYMENT-MODES.md`. diff --git a/server/src/__tests__/execution-workspaces-service.test.ts b/server/src/__tests__/execution-workspaces-service.test.ts index f59437ed14b..faf0f68bff8 100644 --- a/server/src/__tests__/execution-workspaces-service.test.ts +++ b/server/src/__tests__/execution-workspaces-service.test.ts @@ -949,6 +949,14 @@ describeEmbeddedPostgres("executionWorkspaceService.getCloseReadiness", () => { cwd: seed.worktreePath, baseRef: "main", config, deliveryState: "unknown", runtimeServices: [], }); + const runtimeServiceId = randomUUID(); + const [workspace] = await db.select().from(executionWorkspaces).where(eq(executionWorkspaces.id, seed.executionWorkspaceId)); + await insertRunningWorkspaceService(runtimeServiceId, seed.companyId, workspace!.projectId!, seed.executionWorkspaceId, workspace!.sourceIssueId!, seed.worktreePath); + expect(await service.getStoredById(seed.companyId, seed.executionWorkspaceId, { includeRuntimeServices: true })).toMatchObject({ + deliveryState: "unknown", runtimeServices: [{ id: runtimeServiceId, status: "running", healthStatus: "healthy" }], + }); + await expect(service.getStoredById(randomUUID(), seed.executionWorkspaceId, { includeRuntimeServices: true })) + .rejects.toThrow("Task workspace access is no longer available"); await expect(service.getStoredById(randomUUID(), seed.executionWorkspaceId)) .rejects.toThrow("Task workspace access is no longer available"); expect(await service.getStoredById(seed.companyId, randomUUID())).toBeNull(); diff --git a/server/src/__tests__/issue-closed-workspace-routes.test.ts b/server/src/__tests__/issue-closed-workspace-routes.test.ts index d1875ff5144..e54a939d44d 100644 --- a/server/src/__tests__/issue-closed-workspace-routes.test.ts +++ b/server/src/__tests__/issue-closed-workspace-routes.test.ts @@ -21,7 +21,8 @@ const mockIssueService = vi.hoisted(() => ({ })); const mockExecutionWorkspaceService = vi.hoisted(() => ({ - getById: vi.fn(), + getById: vi.fn(() => { throw new Error("Task routes must not inspect a workspace"); }), + getStoredById: vi.fn(), reopenClosedIsolatedExecutionWorkspaceForIssue: vi.fn(), clearReopenPendingConsumptionForUnconsumedReopen: vi.fn(async () => ({ cleared: true })), refreshReopenPendingConsumption: vi.fn(async () => ({ refreshed: true })), @@ -244,7 +245,7 @@ describe("closed isolated workspace issue routes", () => { beforeEach(() => { vi.clearAllMocks(); mockIssueService.getById.mockResolvedValue(makeIssue()); - mockExecutionWorkspaceService.getById.mockResolvedValue(makeClosedWorkspace()); + mockExecutionWorkspaceService.getStoredById.mockResolvedValue(makeClosedWorkspace()); mockExecutionWorkspaceService.refreshReopenPendingConsumption.mockResolvedValue({ refreshed: true }); // The guard reopens a closed isolated workspace and lets the request // continue. The default is a successful reopen. diff --git a/server/src/__tests__/issue-workspace-command-authz.test.ts b/server/src/__tests__/issue-workspace-command-authz.test.ts index 8b5e09d7a2b..7772649c004 100644 --- a/server/src/__tests__/issue-workspace-command-authz.test.ts +++ b/server/src/__tests__/issue-workspace-command-authz.test.ts @@ -26,7 +26,8 @@ const mockAgentService = vi.hoisted(() => ({ })); const mockExecutionWorkspaceService = vi.hoisted(() => ({ - getById: vi.fn(), + getById: vi.fn(() => { throw new Error("Task routes must not inspect a workspace"); }), + getStoredById: vi.fn(), })); const mockFeedbackService = vi.hoisted(() => ({ @@ -230,7 +231,7 @@ describe("issue workspace command authorization", () => { companyId: "company-1", permissions: null, }); - mockExecutionWorkspaceService.getById.mockResolvedValue(null); + mockExecutionWorkspaceService.getStoredById.mockResolvedValue(null); mockFeedbackService.listIssueVotesForUser.mockResolvedValue([]); mockFeedbackService.saveIssueVote.mockResolvedValue({ vote: null, diff --git a/server/src/__tests__/issues-goal-context-routes.test.ts b/server/src/__tests__/issues-goal-context-routes.test.ts index 863509c312c..80276ce61fe 100644 --- a/server/src/__tests__/issues-goal-context-routes.test.ts +++ b/server/src/__tests__/issues-goal-context-routes.test.ts @@ -34,7 +34,8 @@ const mockDocumentsService = vi.hoisted(() => ({ })); const mockExecutionWorkspaceService = vi.hoisted(() => ({ - getById: vi.fn(), + getById: vi.fn(() => { throw new Error("Task routes must not inspect a workspace"); }), + getStoredById: vi.fn(), })); const mockAccessService = vi.hoisted(() => ({ @@ -221,7 +222,7 @@ describe("issue goal context routes", () => { mockIssueService.listAttachments.mockResolvedValue([]); mockDocumentsService.getIssueDocumentPayload.mockResolvedValue({}); mockDocumentsService.getIssueDocumentByKey.mockResolvedValue(null); - mockExecutionWorkspaceService.getById.mockResolvedValue(null); + mockExecutionWorkspaceService.getStoredById.mockResolvedValue(null); const emptyQuery: any = {}; emptyQuery.from = vi.fn(() => emptyQuery); emptyQuery.innerJoin = vi.fn(() => emptyQuery); @@ -393,7 +394,7 @@ describe("issue goal context routes", () => { updatedAt: new Date("2026-03-20T00:00:00Z"), }, }); - mockExecutionWorkspaceService.getById.mockResolvedValueOnce({ + mockExecutionWorkspaceService.getStoredById.mockResolvedValueOnce({ id: workspaceId, companyId: "company-1", projectId: legacyProjectLinkedIssue.projectId, @@ -526,12 +527,12 @@ describe("issue goal context routes", () => { ]); }); - it("surfaces the current execution workspace from GET /issues/:id/heartbeat-context", async () => { + it.each(["", "/heartbeat-context"])("reads stored workspace context without Git inspection: %s", async (suffix) => { mockIssueService.getById.mockResolvedValue({ ...legacyProjectLinkedIssue, executionWorkspaceId: "55555555-5555-4555-8555-555555555555", }); - mockExecutionWorkspaceService.getById.mockResolvedValue({ + mockExecutionWorkspaceService.getStoredById.mockResolvedValue({ id: "55555555-5555-4555-8555-555555555555", name: "PAP-581 workspace", mode: "isolated_workspace", @@ -549,11 +550,12 @@ describe("issue goal context routes", () => { }); const res = await request(createApp()).get( - "/api/issues/11111111-1111-4111-8111-111111111111/heartbeat-context", + `/api/issues/11111111-1111-4111-8111-111111111111${suffix}`, ); expect(res.status).toBe(200); - expect(mockExecutionWorkspaceService.getById).toHaveBeenCalledWith("55555555-5555-4555-8555-555555555555"); + expect(mockExecutionWorkspaceService.getById).not.toHaveBeenCalled(); + expect(mockExecutionWorkspaceService.getStoredById).toHaveBeenCalledWith("company-1", "55555555-5555-4555-8555-555555555555", { includeRuntimeServices: true }); expect(res.body.currentExecutionWorkspace).toEqual(expect.objectContaining({ id: "55555555-5555-4555-8555-555555555555", mode: "isolated_workspace", diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 01ce215dea1..69398e41a4c 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -7686,11 +7686,12 @@ export function issueRoutes( return commentCreatedAtMs >= activeRunStartedAtMs; } async function getClosedIssueExecutionWorkspace(issue: { + companyId: string; executionWorkspaceId?: string | null; }) { if (!issue.executionWorkspaceId) return null; - const workspace = await executionWorkspacesSvc.getById( - issue.executionWorkspaceId, + const workspace = await executionWorkspacesSvc.getStoredById( + issue.companyId, issue.executionWorkspaceId, ); if (!workspace || !isClosedIsolatedExecutionWorkspace(workspace)) return null; @@ -8925,7 +8926,7 @@ export function issueRoutes( : null; const currentExecutionWorkspacePromise = issue.executionWorkspaceId - ? executionWorkspacesSvc.getById(issue.executionWorkspaceId) + ? executionWorkspacesSvc.getStoredById(issue.companyId, issue.executionWorkspaceId, { includeRuntimeServices: true }) : Promise.resolve(null); const [ { project, goal }, @@ -9292,7 +9293,7 @@ export function issueRoutes( inboxArchiveFieldsPromise, timing.time("channel", () => getExternalChannelBindingSummary(db, issue.companyId, issue.id)), timing.time("workspace", () => issue.executionWorkspaceId - ? executionWorkspacesSvc.getById(issue.executionWorkspaceId) + ? executionWorkspacesSvc.getStoredById(issue.companyId, issue.executionWorkspaceId, { includeRuntimeServices: true }) : Promise.resolve(null)), timing.time("work_products", () => workProductsSvc.listForIssue(issue.id)), timing.time("created_from", () => resolveVisibleIssueCreationSource(req, issue)), diff --git a/server/src/services/execution-workspaces.ts b/server/src/services/execution-workspaces.ts index 8d490f35cbc..6d01ef4c5d0 100644 --- a/server/src/services/execution-workspaces.ts +++ b/server/src/services/execution-workspaces.ts @@ -2910,10 +2910,13 @@ export function executionWorkspaceService(db: Db, opts: ExecutionWorkspaceServic return null; }, - getStoredById: async (companyId: string, id: string) => { + getStoredById: async (companyId: string, id: string, options?: { includeRuntimeServices: boolean }) => { const [row] = await db.select().from(executionWorkspaces).where(eq(executionWorkspaces.id, id)); if (row && row.companyId !== companyId) throw forbidden("Task workspace access is no longer available"); - return row ? toExecutionWorkspace(row) : null; + if (!row) return null; + const runtimeServices = options?.includeRuntimeServices + ? await loadEffectiveRuntimeServicesByExecutionWorkspace(db, companyId, [row]) : null; + return toExecutionWorkspace(row, (runtimeServices?.get(row.id) ?? []).map(toRuntimeService)); }, getById: async (id: string) => {