diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 5ce9d5ba9e..7c244e5b88 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -84,6 +84,19 @@ export { type ResponsibleUserSource, type OriginatingActor, } from "./issue-attribution.js"; +export { + ARTIFACT_REVIEW_DOCUMENT_KEY_PREFIX, + MARKDOWN_ATTACHMENT_CONTENT_TYPES, + MARKDOWN_REVIEW_DOCUMENT_MAX_BYTES, + artifactReviewDocumentKey, + getAttachmentArtifactWorkProductMetadata, + getMarkdownWorkProductAttachmentMetadata, + isArtifactReviewDocumentKey, + isMarkdownArtifactWorkProduct, + isMarkdownAttachmentContent, + workProductIdFromArtifactReviewDocumentKey, + type AttachmentArtifactWorkProductLike, +} from "./markdown-work-products.js"; export { ISSUE_WRITE_DENIAL_CODES, describeIssueWriteDenial, diff --git a/packages/shared/src/markdown-work-products.test.ts b/packages/shared/src/markdown-work-products.test.ts new file mode 100644 index 0000000000..7293d14ad4 --- /dev/null +++ b/packages/shared/src/markdown-work-products.test.ts @@ -0,0 +1,178 @@ +import { describe, expect, it } from "vitest"; +import { + ARTIFACT_REVIEW_DOCUMENT_KEY_PREFIX, + artifactReviewDocumentKey, + getAttachmentArtifactWorkProductMetadata, + getMarkdownWorkProductAttachmentMetadata, + isArtifactReviewDocumentKey, + isMarkdownArtifactWorkProduct, + isMarkdownAttachmentContent, + workProductIdFromArtifactReviewDocumentKey, +} from "./markdown-work-products.js"; + +const ATTACHMENT_ID = "00000000-0000-4000-8000-000000000001"; +const WORK_PRODUCT_ID = "11111111-2222-4333-8444-555555555555"; + +function attachmentMetadata(overrides: Record = {}) { + const contentPath = `/api/attachments/${ATTACHMENT_ID}/content`; + return { + attachmentId: ATTACHMENT_ID, + contentType: "text/markdown", + byteSize: 128, + contentPath, + openPath: contentPath, + downloadPath: `${contentPath}?download=1`, + originalFilename: "notes.md", + ...overrides, + }; +} + +function workProduct(overrides: Record = {}) { + return { + type: "artifact" as const, + provider: "paperclip", + metadata: attachmentMetadata(), + ...overrides, + }; +} + +describe("isMarkdownAttachmentContent", () => { + it.each([ + "text/markdown", + "text/x-markdown", + "application/markdown", + "application/x-markdown", + ])("accepts %s regardless of filename", (contentType) => { + expect(isMarkdownAttachmentContent({ contentType, originalFilename: null })).toBe(true); + }); + + it("ignores MIME parameters and case", () => { + expect( + isMarkdownAttachmentContent({ + contentType: "Text/Markdown; charset=utf-8", + originalFilename: null, + }), + ).toBe(true); + }); + + it.each([ + "text/plain", + "application/octet-stream", + "binary/octet-stream", + "application/x-binary", + ])("accepts a .md filename when the content type is %s", (contentType) => { + expect(isMarkdownAttachmentContent({ contentType, originalFilename: "README.md" })).toBe(true); + }); + + it("accepts a .markdown filename with a generic content type", () => { + expect( + isMarkdownAttachmentContent({ + contentType: "application/octet-stream", + originalFilename: "guide.markdown", + }), + ).toBe(true); + }); + + it("rejects .mdx files", () => { + expect( + isMarkdownAttachmentContent({ + contentType: "application/octet-stream", + originalFilename: "page.mdx", + }), + ).toBe(false); + expect( + isMarkdownAttachmentContent({ contentType: "text/plain", originalFilename: "page.mdx" }), + ).toBe(false); + }); + + it("rejects markdown filenames with non-generic content types", () => { + expect( + isMarkdownAttachmentContent({ contentType: "text/html", originalFilename: "notes.md" }), + ).toBe(false); + }); + + it("rejects non-markdown filenames with generic content types", () => { + expect( + isMarkdownAttachmentContent({ contentType: "text/plain", originalFilename: "notes.txt" }), + ).toBe(false); + }); + + it("rejects a missing content type", () => { + expect(isMarkdownAttachmentContent({ contentType: null, originalFilename: "notes.md" })).toBe( + false, + ); + }); +}); + +describe("getAttachmentArtifactWorkProductMetadata", () => { + it("returns canonical metadata for a valid paperclip artifact work product", () => { + const metadata = getAttachmentArtifactWorkProductMetadata(workProduct()); + expect(metadata?.attachmentId).toBe(ATTACHMENT_ID); + }); + + it("returns null for non-artifact work products", () => { + expect(getAttachmentArtifactWorkProductMetadata(workProduct({ type: "document" }))).toBeNull(); + }); + + it("returns null for non-paperclip providers", () => { + expect(getAttachmentArtifactWorkProductMetadata(workProduct({ provider: "github" }))).toBeNull(); + }); + + it("returns null when metadata is missing or non-canonical", () => { + expect(getAttachmentArtifactWorkProductMetadata(workProduct({ metadata: null }))).toBeNull(); + expect( + getAttachmentArtifactWorkProductMetadata( + workProduct({ + metadata: attachmentMetadata({ openPath: "https://evil.example/content" }), + }), + ), + ).toBeNull(); + }); +}); + +describe("getMarkdownWorkProductAttachmentMetadata", () => { + it("returns metadata only when the attachment is markdown-eligible", () => { + expect(getMarkdownWorkProductAttachmentMetadata(workProduct())).not.toBeNull(); + expect( + getMarkdownWorkProductAttachmentMetadata( + workProduct({ + metadata: attachmentMetadata({ contentType: "application/pdf", originalFilename: "a.pdf" }), + }), + ), + ).toBeNull(); + }); + + it("backs the boolean eligibility helper", () => { + expect(isMarkdownArtifactWorkProduct(workProduct())).toBe(true); + expect( + isMarkdownArtifactWorkProduct( + workProduct({ + metadata: attachmentMetadata({ contentType: "image/png", originalFilename: "a.png" }), + }), + ), + ).toBe(false); + }); +}); + +describe("artifact review document keys", () => { + it("builds a deterministic key that fits the 64-character document key limit", () => { + const key = artifactReviewDocumentKey(WORK_PRODUCT_ID); + expect(key).toBe(`artifact-review-${WORK_PRODUCT_ID}`); + expect(key.length).toBeLessThanOrEqual(64); + expect(key).toMatch(/^[a-z0-9][a-z0-9_-]*$/); + }); + + it("identifies proxy review-document keys", () => { + expect(isArtifactReviewDocumentKey(artifactReviewDocumentKey(WORK_PRODUCT_ID))).toBe(true); + expect(isArtifactReviewDocumentKey("plan")).toBe(false); + expect(isArtifactReviewDocumentKey("notes")).toBe(false); + expect(isArtifactReviewDocumentKey(ARTIFACT_REVIEW_DOCUMENT_KEY_PREFIX)).toBe(false); + }); + + it("round-trips the work product id", () => { + expect( + workProductIdFromArtifactReviewDocumentKey(artifactReviewDocumentKey(WORK_PRODUCT_ID)), + ).toBe(WORK_PRODUCT_ID); + expect(workProductIdFromArtifactReviewDocumentKey("plan")).toBeNull(); + }); +}); diff --git a/packages/shared/src/markdown-work-products.ts b/packages/shared/src/markdown-work-products.ts new file mode 100644 index 0000000000..16a88cfb71 --- /dev/null +++ b/packages/shared/src/markdown-work-products.ts @@ -0,0 +1,99 @@ +import type { + AttachmentArtifactWorkProductMetadata, + IssueWorkProduct, +} from "./types/work-product.js"; +import { attachmentArtifactWorkProductMetadataSchema } from "./validators/work-product.js"; + +/** + * Shared classification for bridging attachment-backed Markdown work products + * into the issue document review experience. Server materialization and UI + * presentation must agree on eligibility, so both sides use these helpers. + */ + +export const MARKDOWN_ATTACHMENT_CONTENT_TYPES = [ + "text/markdown", + "text/x-markdown", + "application/markdown", + "application/x-markdown", +] as const; + +const MARKDOWN_ATTACHMENT_CONTENT_TYPE_SET = new Set(MARKDOWN_ATTACHMENT_CONTENT_TYPES); + +const MARKDOWN_FALLBACK_CONTENT_TYPES = new Set([ + "text/plain", + "application/octet-stream", + "binary/octet-stream", + "application/x-binary", +]); + +const MARKDOWN_FILENAME_EXTENSIONS = [".md", ".markdown"]; + +/** + * Maximum attachment size eligible for review-document materialization. + * Matches the issue-document body limit: a UTF-8 payload of at most this many + * bytes always fits the 524288-character body cap. + */ +export const MARKDOWN_REVIEW_DOCUMENT_MAX_BYTES = 512 * 1024; + +export const ARTIFACT_REVIEW_DOCUMENT_KEY_PREFIX = "artifact-review-"; + +function normalizeAttachmentContentType(contentType: string | null | undefined): string { + return (contentType ?? "").toLowerCase().split(";")[0]?.trim() ?? ""; +} + +export function isMarkdownAttachmentContent(input: { + contentType: string | null | undefined; + originalFilename?: string | null; +}): boolean { + const contentType = normalizeAttachmentContentType(input.contentType); + if (MARKDOWN_ATTACHMENT_CONTENT_TYPE_SET.has(contentType)) return true; + const filename = (input.originalFilename ?? "").toLowerCase(); + if (!MARKDOWN_FILENAME_EXTENSIONS.some((extension) => filename.endsWith(extension))) { + return false; + } + return MARKDOWN_FALLBACK_CONTENT_TYPES.has(contentType); +} + +export type AttachmentArtifactWorkProductLike = Pick< + IssueWorkProduct, + "type" | "provider" | "metadata" +>; + +export function getAttachmentArtifactWorkProductMetadata( + workProduct: AttachmentArtifactWorkProductLike, +): AttachmentArtifactWorkProductMetadata | null { + if (workProduct.type !== "artifact" || workProduct.provider !== "paperclip") return null; + if (!workProduct.metadata) return null; + const parsed = attachmentArtifactWorkProductMetadataSchema.safeParse(workProduct.metadata); + return parsed.success ? parsed.data : null; +} + +export function getMarkdownWorkProductAttachmentMetadata( + workProduct: AttachmentArtifactWorkProductLike, +): AttachmentArtifactWorkProductMetadata | null { + const metadata = getAttachmentArtifactWorkProductMetadata(workProduct); + if (!metadata) return null; + return isMarkdownAttachmentContent(metadata) ? metadata : null; +} + +export function isMarkdownArtifactWorkProduct( + workProduct: AttachmentArtifactWorkProductLike, +): boolean { + return getMarkdownWorkProductAttachmentMetadata(workProduct) !== null; +} + +export function artifactReviewDocumentKey(workProductId: string): string { + return `${ARTIFACT_REVIEW_DOCUMENT_KEY_PREFIX}${workProductId}`; +} + +export function isArtifactReviewDocumentKey(key: string): boolean { + return ( + key.startsWith(ARTIFACT_REVIEW_DOCUMENT_KEY_PREFIX) && + key.length > ARTIFACT_REVIEW_DOCUMENT_KEY_PREFIX.length + ); +} + +export function workProductIdFromArtifactReviewDocumentKey(key: string): string | null { + if (!isArtifactReviewDocumentKey(key)) return null; + return key.slice(ARTIFACT_REVIEW_DOCUMENT_KEY_PREFIX.length); +} diff --git a/server/src/__tests__/artifact-review-document-routes.test.ts b/server/src/__tests__/artifact-review-document-routes.test.ts new file mode 100644 index 0000000000..21dd1ae60f --- /dev/null +++ b/server/src/__tests__/artifact-review-document-routes.test.ts @@ -0,0 +1,334 @@ +import express from "express"; +import request from "supertest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { StorageService } from "../storage/types.js"; + +const ISSUE_ID = "11111111-1111-4111-8111-111111111111"; +const WORK_PRODUCT_ID = "22222222-2222-4222-8222-222222222222"; + +const mockIssueService = vi.hoisted(() => ({ + getById: vi.fn(), + getByIdentifier: vi.fn(), + getAttachmentById: vi.fn(), +})); +const mockCompanyService = vi.hoisted(() => ({ + getById: vi.fn(), +})); +const mockWorkProductService = vi.hoisted(() => ({ + createForIssue: vi.fn(), + getById: vi.fn(), + update: vi.fn(), +})); +const mockAccessService = vi.hoisted(() => ({ + decide: vi.fn(async () => ({ + allowed: true, + explanation: "Allowed by test mock", + })), + canUser: vi.fn(), + hasPermission: vi.fn(), +})); +const mockLogActivity = vi.hoisted(() => vi.fn(async () => undefined)); +const mockEnsureForWorkProduct = vi.hoisted(() => vi.fn()); +const mockSyncDocumentSafely = vi.hoisted(() => vi.fn(async () => undefined)); + +function registerRouteMocks() { + vi.doMock("@paperclipai/shared/telemetry", () => ({ + trackAgentTaskCompleted: vi.fn(), + trackErrorHandlerCrash: vi.fn(), + })); + + vi.doMock("../telemetry.js", () => ({ + getTelemetryClient: vi.fn(() => ({ track: vi.fn() })), + })); + + vi.doMock("../services/issues.js", () => ({ + issueService: () => mockIssueService, + })); + + vi.doMock("../services/activity-log.js", () => ({ + logActivity: mockLogActivity, + })); + + vi.doMock("../services/artifact-review-documents.js", () => ({ + artifactReviewDocumentService: () => ({ + ensureForWorkProduct: mockEnsureForWorkProduct, + }), + })); + + vi.doMock("../services/external-objects.js", () => ({ + externalObjectService: () => ({ + syncDocumentSafely: mockSyncDocumentSafely, + }), + })); + + vi.doMock("../services/index.js", () => ({ + accessService: () => mockAccessService, + agentService: () => ({ + getById: vi.fn(), + }), + companySkillService: () => ({}), + companyService: () => mockCompanyService, + documentAnnotationService: () => ({ remapOpenThreadsForDocument: async () => [] }), + documentService: () => ({}), + executionWorkspaceService: () => ({}), + feedbackService: () => ({ + listIssueVotesForUser: vi.fn(async () => []), + saveIssueVote: vi.fn(async () => ({ vote: null, consentEnabledNow: false, sharingEnabled: false })), + }), + goalService: () => ({}), + heartbeatService: () => ({ + wakeup: vi.fn(async () => undefined), + reportRunActivity: vi.fn(async () => undefined), + getRun: vi.fn(async () => null), + getActiveRunForAgent: vi.fn(async () => null), + cancelRun: vi.fn(async () => null), + }), + instanceSettingsService: () => ({ + get: vi.fn(async () => ({ + id: "instance-settings-1", + general: { + censorUsernameInLogs: false, + feedbackDataSharingPreference: "prompt", + }, + })), + listCompanyIds: vi.fn(async () => ["company-1"]), + }), + issueApprovalService: () => ({}), + issueReferenceService: () => ({ + deleteDocumentSource: async () => undefined, + diffIssueReferenceSummary: () => ({ + addedReferencedIssues: [], + removedReferencedIssues: [], + currentReferencedIssues: [], + }), + emptySummary: () => ({ outbound: [], inbound: [] }), + listIssueReferenceSummary: async () => ({ outbound: [], inbound: [] }), + syncComment: async () => undefined, + syncDocument: async () => undefined, + syncIssue: async () => undefined, + }), + issueThreadInteractionService: () => ({ + listForIssue: vi.fn(async () => []), + expireRequestConfirmationsSupersededByComment: vi.fn(async () => []), + expireStaleRequestConfirmationsForIssueDocument: vi.fn(async () => []), + }), + issueRecoveryActionService: () => ({ + getActiveForIssue: vi.fn(async () => null), + listActiveForIssues: vi.fn(async () => new Map()), + }), + issueService: () => mockIssueService, + logActivity: mockLogActivity, + projectService: () => ({}), + routineService: () => ({ + syncRunStatusForIssue: vi.fn(async () => undefined), + }), + workProductService: () => mockWorkProductService, + })); +} + +function createStorageService(): StorageService { + return { + provider: "local_disk", + putFile: vi.fn(), + getObject: vi.fn(), + headObject: vi.fn(), + deleteObject: vi.fn(), + } as unknown as StorageService; +} + +async function createApp(options?: { companyIds?: string[] }) { + const [{ errorHandler }, { issueRoutes }] = await Promise.all([ + vi.importActual("../middleware/index.js"), + vi.importActual("../routes/issues.js"), + ]); + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + (req as any).actor = { + type: "board", + userId: "local-board", + companyIds: options?.companyIds ?? ["company-1"], + source: "local_implicit", + isInstanceAdmin: false, + }; + next(); + }); + app.use("/api", issueRoutes({} as any, createStorageService())); + app.use(errorHandler); + return app; +} + +function makeIssue() { + return { + id: ISSUE_ID, + companyId: "company-1", + identifier: "PAP-1", + projectId: null, + status: "in_progress", + assigneeAgentId: null, + }; +} + +function makeWorkProduct(overrides: Record = {}) { + const attachmentId = "55555555-5555-4555-8555-555555555555"; + const contentPath = `/api/attachments/${attachmentId}/content`; + return { + id: WORK_PRODUCT_ID, + companyId: "company-1", + issueId: ISSUE_ID, + type: "artifact", + provider: "paperclip", + title: "Verification report", + metadata: { + attachmentId, + contentType: "text/markdown", + byteSize: 128, + contentPath, + openPath: contentPath, + downloadPath: `${contentPath}?download=1`, + originalFilename: "verification-report.md", + }, + createdByRunId: null, + sourceTrust: null, + ...overrides, + }; +} + +function makeDocument(overrides: Record = {}) { + return { + id: "33333333-3333-4333-8333-333333333333", + companyId: "company-1", + issueId: ISSUE_ID, + key: `artifact-review-${WORK_PRODUCT_ID}`, + title: "Verification report", + format: "markdown", + body: "# Report", + latestRevisionId: "44444444-4444-4444-8444-444444444444", + latestRevisionNumber: 1, + ...overrides, + }; +} + +describe("work product review-document route", () => { + beforeEach(() => { + vi.resetModules(); + vi.clearAllMocks(); + registerRouteMocks(); + mockIssueService.getById.mockResolvedValue(makeIssue()); + mockWorkProductService.getById.mockResolvedValue(makeWorkProduct()); + }); + + it("returns 404 when the work product belongs to another issue", async () => { + mockWorkProductService.getById.mockResolvedValue( + makeWorkProduct({ issueId: "55555555-5555-4555-8555-555555555555" }), + ); + + const app = await createApp(); + const res = await request(app).post( + `/api/issues/${ISSUE_ID}/work-products/${WORK_PRODUCT_ID}/review-document`, + ); + + expect(res.status).toBe(404); + expect(mockEnsureForWorkProduct).not.toHaveBeenCalled(); + }); + + it("returns 404 when the work product does not exist", async () => { + mockWorkProductService.getById.mockResolvedValue(null); + + const app = await createApp(); + const res = await request(app).post( + `/api/issues/${ISSUE_ID}/work-products/${WORK_PRODUCT_ID}/review-document`, + ); + + expect(res.status).toBe(404); + expect(mockEnsureForWorkProduct).not.toHaveBeenCalled(); + }); + + it("materializes the review document and logs document creation", async () => { + mockEnsureForWorkProduct.mockResolvedValue({ + document: makeDocument(), + created: true, + revisionChanged: true, + remappedAnnotations: [], + }); + + const app = await createApp(); + const res = await request(app).post( + `/api/issues/${ISSUE_ID}/work-products/${WORK_PRODUCT_ID}/review-document`, + ); + + expect(res.status).toBe(201); + expect(res.body.key).toBe(`artifact-review-${WORK_PRODUCT_ID}`); + expect(mockEnsureForWorkProduct).toHaveBeenCalledWith({ + issue: { id: ISSUE_ID, companyId: "company-1" }, + workProduct: expect.objectContaining({ id: WORK_PRODUCT_ID }), + }); + expect(mockLogActivity).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + action: "issue.document_created", + details: expect.objectContaining({ + workProductId: WORK_PRODUCT_ID, + artifactReviewDocument: true, + }), + }), + ); + expect(mockSyncDocumentSafely).toHaveBeenCalledWith(makeDocument().id); + }); + + it("returns 200 without side effects when the document is already current", async () => { + mockEnsureForWorkProduct.mockResolvedValue({ + document: makeDocument(), + created: false, + revisionChanged: false, + remappedAnnotations: [], + }); + + const app = await createApp(); + const res = await request(app).post( + `/api/issues/${ISSUE_ID}/work-products/${WORK_PRODUCT_ID}/review-document`, + ); + + expect(res.status).toBe(200); + expect(res.body.id).toBe(makeDocument().id); + expect(mockLogActivity).not.toHaveBeenCalled(); + expect(mockSyncDocumentSafely).not.toHaveBeenCalled(); + }); + + it("passes typed materialization errors through to the response", async () => { + const { HttpError } = await vi.importActual("../errors.js"); + mockEnsureForWorkProduct.mockRejectedValue( + new HttpError(415, "Work product attachment is not Markdown"), + ); + + const app = await createApp(); + const res = await request(app).post( + `/api/issues/${ISSUE_ID}/work-products/${WORK_PRODUCT_ID}/review-document`, + ); + + expect(res.status).toBe(415); + expect(res.body.error).toBe("Work product attachment is not Markdown"); + }); + + it("resynchronizes an eligible review document after a title-only work product update", async () => { + const renamed = makeWorkProduct({ title: "Renamed verification report" }); + mockWorkProductService.update.mockResolvedValue(renamed); + mockEnsureForWorkProduct.mockResolvedValue({ + document: makeDocument({ title: "Renamed verification report", latestRevisionNumber: 2 }), + created: false, + revisionChanged: true, + remappedAnnotations: [], + }); + + const app = await createApp(); + const res = await request(app) + .patch(`/api/work-products/${WORK_PRODUCT_ID}`) + .send({ title: "Renamed verification report" }); + + expect(res.status).toBe(200); + expect(mockEnsureForWorkProduct).toHaveBeenCalledWith({ + issue: { id: ISSUE_ID, companyId: "company-1" }, + workProduct: renamed, + }); + }); +}); diff --git a/server/src/__tests__/artifact-review-documents-service.test.ts b/server/src/__tests__/artifact-review-documents-service.test.ts new file mode 100644 index 0000000000..01dc766af4 --- /dev/null +++ b/server/src/__tests__/artifact-review-documents-service.test.ts @@ -0,0 +1,403 @@ +import { randomUUID } from "node:crypto"; +import { Readable } from "node:stream"; +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; +import { + agents, + assets, + companies, + createDb, + documentRevisions, + documents, + heartbeatRuns, + issueAttachments, + issueDocuments, + issues, +} from "@paperclipai/db"; +import { artifactReviewDocumentKey } from "@paperclipai/shared"; +import { + getEmbeddedPostgresTestSupport, + startEmbeddedPostgresTestDatabase, +} from "./helpers/embedded-postgres.js"; +import { artifactReviewDocumentService } from "../services/artifact-review-documents.js"; +import { documentService } from "../services/documents.js"; +import { HttpError } from "../errors.js"; +import type { StorageService } from "../storage/types.js"; + +const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); +const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; + +if (!embeddedPostgresSupport.supported) { + console.warn( + `Skipping embedded Postgres artifact review document tests on this host: ${embeddedPostgresSupport.reason ?? "unsupported environment"}`, + ); +} + +function createStorageService(files: Record = {}): StorageService { + return { + provider: "local_disk", + putFile: vi.fn(), + getObject: vi.fn(async (_companyId, objectKey, options) => { + const body = files[objectKey] ?? Buffer.alloc(0); + const range = options?.range; + const ranged = range ? body.subarray(range.start, range.end + 1) : body; + return { + stream: Readable.from(ranged), + contentType: "text/markdown", + contentLength: ranged.length, + }; + }), + headObject: vi.fn(), + deleteObject: vi.fn(), + }; +} + +describeEmbeddedPostgres("artifactReviewDocumentService", () => { + let tempDb: Awaited> | null = null; + let db!: ReturnType; + + beforeAll(async () => { + tempDb = await startEmbeddedPostgresTestDatabase("paperclip-artifact-review-docs-"); + db = createDb(tempDb.connectionString); + }, 20_000); + + afterEach(async () => { + await db.delete(documentRevisions); + await db.delete(issueDocuments); + await db.delete(documents); + await db.delete(issueAttachments); + await db.delete(assets); + await db.delete(issues); + await db.delete(heartbeatRuns); + await db.delete(agents); + await db.delete(companies); + }); + + afterAll(async () => { + await tempDb?.cleanup(); + }); + + async function seedFixture(options: { + body?: Buffer; + contentType?: string; + originalFilename?: string | null; + byteSize?: number; + } = {}) { + const companyId = randomUUID(); + const issueId = randomUUID(); + const agentId = randomUUID(); + const assetId = randomUUID(); + const attachmentId = randomUUID(); + const workProductId = randomUUID(); + const body = options.body ?? Buffer.from("# Report\n\nHello **markdown**.\n", "utf8"); + const contentType = options.contentType ?? "text/markdown"; + const originalFilename = options.originalFilename === undefined ? "report.md" : options.originalFilename; + const objectKey = `issues/${issueId}/${assetId}`; + + await db.insert(companies).values({ + id: companyId, + name: "Paperclip", + issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`, + requireBoardApprovalForNewAgents: false, + }); + await db.insert(issues).values({ + id: issueId, + companyId, + identifier: "PAP-1534", + title: "Bridge markdown work products", + description: null, + status: "in_progress", + priority: "medium", + }); + await db.insert(agents).values({ + id: agentId, + companyId, + name: "ReportWriter", + role: "engineer", + status: "idle", + adapterType: "codex_local", + adapterConfig: {}, + runtimeConfig: {}, + permissions: {}, + }); + await db.insert(assets).values({ + id: assetId, + companyId, + provider: "local_disk", + objectKey, + contentType, + byteSize: options.byteSize ?? body.length, + sha256: `sha256-${assetId}`, + originalFilename, + createdByAgentId: agentId, + }); + await db.insert(issueAttachments).values({ + id: attachmentId, + companyId, + issueId, + assetId, + }); + + const contentPath = `/api/attachments/${attachmentId}/content`; + const workProduct = { + id: workProductId, + companyId, + issueId, + type: "artifact" as const, + provider: "paperclip", + metadata: { + attachmentId, + contentType, + byteSize: options.byteSize ?? body.length, + contentPath, + openPath: contentPath, + downloadPath: `${contentPath}?download=1`, + originalFilename, + }, + title: "Verification report", + createdByRunId: null, + sourceTrust: null, + }; + + const files: Record = { [objectKey]: body }; + const storage = createStorageService(files); + const svc = artifactReviewDocumentService(db, storage); + return { companyId, issueId, agentId, assetId, attachmentId, workProduct, files, objectKey, svc }; + } + + it("materializes a review document with promoted attribution", async () => { + const fixture = await seedFixture(); + const result = await fixture.svc.ensureForWorkProduct({ + issue: { id: fixture.issueId, companyId: fixture.companyId }, + workProduct: fixture.workProduct, + }); + + expect(result.created).toBe(true); + expect(result.revisionChanged).toBe(true); + expect(result.document.key).toBe(artifactReviewDocumentKey(fixture.workProduct.id)); + expect(result.document.title).toBe("Verification report"); + expect(result.document.latestRevisionNumber).toBe(1); + expect(result.document.latestRevisionId).toBeTruthy(); + expect(result.document.body).toBe("# Report\n\nHello **markdown**.\n"); + expect(result.document.createdByAgentId).toBe(fixture.agentId); + expect(result.document.createdByUserId).toBeNull(); + + const listed = await documentService(db).getIssueDocumentByKey( + fixture.issueId, + artifactReviewDocumentKey(fixture.workProduct.id), + ); + expect(listed?.id).toBe(result.document.id); + }); + + it("is idempotent for unchanged content", async () => { + const fixture = await seedFixture(); + const first = await fixture.svc.ensureForWorkProduct({ + issue: { id: fixture.issueId, companyId: fixture.companyId }, + workProduct: fixture.workProduct, + }); + const second = await fixture.svc.ensureForWorkProduct({ + issue: { id: fixture.issueId, companyId: fixture.companyId }, + workProduct: fixture.workProduct, + }); + + expect(second.created).toBe(false); + expect(second.revisionChanged).toBe(false); + expect(second.document.id).toBe(first.document.id); + expect(second.document.latestRevisionNumber).toBe(1); + + const revisions = await db.select().from(documentRevisions); + expect(revisions).toHaveLength(1); + }); + + it("converges concurrent ensures onto one document", async () => { + const fixture = await seedFixture(); + const input = { + issue: { id: fixture.issueId, companyId: fixture.companyId }, + workProduct: fixture.workProduct, + }; + const [first, second] = await Promise.all([ + fixture.svc.ensureForWorkProduct(input), + fixture.svc.ensureForWorkProduct(input), + ]); + + expect(first.document.id).toBe(second.document.id); + const links = await db.select().from(issueDocuments); + expect(links).toHaveLength(1); + const revisions = await db.select().from(documentRevisions); + expect(revisions).toHaveLength(1); + }); + + it("writes a new revision when the attachment content changes", async () => { + const fixture = await seedFixture(); + const first = await fixture.svc.ensureForWorkProduct({ + issue: { id: fixture.issueId, companyId: fixture.companyId }, + workProduct: fixture.workProduct, + }); + + fixture.files[fixture.objectKey] = Buffer.from("# Report v2\n", "utf8"); + const second = await fixture.svc.ensureForWorkProduct({ + issue: { id: fixture.issueId, companyId: fixture.companyId }, + workProduct: fixture.workProduct, + }); + + expect(second.created).toBe(false); + expect(second.revisionChanged).toBe(true); + expect(second.document.id).toBe(first.document.id); + expect(second.document.latestRevisionNumber).toBe(2); + expect(second.document.body).toBe("# Report v2\n"); + expect(Array.isArray(second.remappedAnnotations)).toBe(true); + }); + + it("synchronizes title and run provenance when the attachment body is unchanged", async () => { + const fixture = await seedFixture(); + const first = await fixture.svc.ensureForWorkProduct({ + issue: { id: fixture.issueId, companyId: fixture.companyId }, + workProduct: fixture.workProduct, + }); + + const renamed = await fixture.svc.ensureForWorkProduct({ + issue: { id: fixture.issueId, companyId: fixture.companyId }, + workProduct: { ...fixture.workProduct, title: "Renamed verification report" }, + }); + expect(renamed.created).toBe(false); + expect(renamed.revisionChanged).toBe(true); + expect(renamed.document.id).toBe(first.document.id); + expect(renamed.document.title).toBe("Renamed verification report"); + expect(renamed.document.latestRevisionNumber).toBe(2); + + const runId = randomUUID(); + await db.insert(heartbeatRuns).values({ + id: runId, + companyId: fixture.companyId, + agentId: fixture.agentId, + status: "succeeded", + }); + const reattributed = await fixture.svc.ensureForWorkProduct({ + issue: { id: fixture.issueId, companyId: fixture.companyId }, + workProduct: { + ...fixture.workProduct, + title: "Renamed verification report", + createdByRunId: runId, + }, + }); + expect(reattributed.revisionChanged).toBe(true); + expect(reattributed.document.latestRevisionNumber).toBe(3); + + const latestRevision = await db + .select() + .from(documentRevisions) + .then((rows) => rows.find((row) => row.id === reattributed.document.latestRevisionId)); + expect(latestRevision?.createdByRunId).toBe(runId); + + const unchanged = await fixture.svc.ensureForWorkProduct({ + issue: { id: fixture.issueId, companyId: fixture.companyId }, + workProduct: { + ...fixture.workProduct, + title: "Renamed verification report", + createdByRunId: runId, + }, + }); + expect(unchanged.revisionChanged).toBe(false); + expect(unchanged.document.latestRevisionNumber).toBe(3); + }); + + it("rejects work products that are not attachment-backed Paperclip artifacts", async () => { + const fixture = await seedFixture(); + await expect( + fixture.svc.ensureForWorkProduct({ + issue: { id: fixture.issueId, companyId: fixture.companyId }, + workProduct: { ...fixture.workProduct, provider: "github" }, + }), + ).rejects.toMatchObject({ status: 422 }); + await expect( + fixture.svc.ensureForWorkProduct({ + issue: { id: fixture.issueId, companyId: fixture.companyId }, + workProduct: { ...fixture.workProduct, metadata: { attachmentId: "not-a-uuid" } }, + }), + ).rejects.toMatchObject({ status: 422 }); + }); + + it("rejects non-Markdown attachments with 415", async () => { + const fixture = await seedFixture({ contentType: "application/pdf", originalFilename: "report.pdf" }); + await expect( + fixture.svc.ensureForWorkProduct({ + issue: { id: fixture.issueId, companyId: fixture.companyId }, + workProduct: fixture.workProduct, + }), + ).rejects.toMatchObject({ status: 415 }); + }); + + it("rejects mismatched issues and cross-issue attachments", async () => { + const fixture = await seedFixture(); + await expect( + fixture.svc.ensureForWorkProduct({ + issue: { id: randomUUID(), companyId: fixture.companyId }, + workProduct: fixture.workProduct, + }), + ).rejects.toMatchObject({ status: 404 }); + + const otherIssueId = randomUUID(); + await db.insert(issues).values({ + id: otherIssueId, + companyId: fixture.companyId, + identifier: "PAP-1535", + title: "Other issue", + description: null, + status: "todo", + priority: "medium", + }); + await expect( + fixture.svc.ensureForWorkProduct({ + issue: { id: otherIssueId, companyId: fixture.companyId }, + workProduct: { ...fixture.workProduct, issueId: otherIssueId }, + }), + ).rejects.toMatchObject({ status: 422 }); + }); + + it("rejects oversized attachments with 413", async () => { + const fixture = await seedFixture({ byteSize: 512 * 1024 + 1 }); + await expect( + fixture.svc.ensureForWorkProduct({ + issue: { id: fixture.issueId, companyId: fixture.companyId }, + workProduct: fixture.workProduct, + }), + ).rejects.toMatchObject({ status: 413 }); + }); + + it("rejects invalid UTF-8 content with 422", async () => { + const fixture = await seedFixture({ body: Buffer.from([0x23, 0x20, 0xff, 0xfe, 0x00]) }); + await expect( + fixture.svc.ensureForWorkProduct({ + issue: { id: fixture.issueId, companyId: fixture.companyId }, + workProduct: fixture.workProduct, + }), + ).rejects.toMatchObject({ status: 422 }); + }); + + it("surfaces a conflict when the review document is locked and content changed", async () => { + const fixture = await seedFixture(); + await fixture.svc.ensureForWorkProduct({ + issue: { id: fixture.issueId, companyId: fixture.companyId }, + workProduct: fixture.workProduct, + }); + await documentService(db).lockIssueDocument({ + issueId: fixture.issueId, + key: artifactReviewDocumentKey(fixture.workProduct.id), + lockedByUserId: "user-board", + }); + + // Unchanged content still resolves without a write. + const unchanged = await fixture.svc.ensureForWorkProduct({ + issue: { id: fixture.issueId, companyId: fixture.companyId }, + workProduct: fixture.workProduct, + }); + expect(unchanged.revisionChanged).toBe(false); + + fixture.files[fixture.objectKey] = Buffer.from("# Locked update\n", "utf8"); + await expect( + fixture.svc.ensureForWorkProduct({ + issue: { id: fixture.issueId, companyId: fixture.companyId }, + workProduct: fixture.workProduct, + }), + ).rejects.toSatisfy((error: unknown) => error instanceof HttpError && error.status === 409); + }); +}); diff --git a/server/src/__tests__/issue-attachment-routes.test.ts b/server/src/__tests__/issue-attachment-routes.test.ts index 2103aa5880..d8f72d1daf 100644 --- a/server/src/__tests__/issue-attachment-routes.test.ts +++ b/server/src/__tests__/issue-attachment-routes.test.ts @@ -515,6 +515,36 @@ describe("issue attachment routes", () => { expect(res.headers["x-content-type-options"]).toBe("nosniff"); }); + it("declares utf-8 for inline markdown attachments", async () => { + const storage = createStorageService(Buffer.from("# Hello\n")); + mockIssueService.getAttachmentById.mockResolvedValue({ + ...makeAttachment("text/markdown", "notes.md"), + byteSize: 8, + }); + + const app = await createApp(storage); + const res = await request(app).get("/api/attachments/attachment-1/content"); + + expect(res.status).toBe(200); + expect(res.headers["content-type"]).toBe("text/markdown; charset=utf-8"); + expect(res.headers["x-content-type-options"]).toBe("nosniff"); + }); + + it("keeps the charset declaration on forced markdown downloads", async () => { + const storage = createStorageService(Buffer.from("# Hello\n")); + mockIssueService.getAttachmentById.mockResolvedValue({ + ...makeAttachment("text/markdown", "notes.md"), + byteSize: 8, + }); + + const app = await createApp(storage); + const res = await request(app).get("/api/attachments/attachment-1/content?download=1"); + + expect(res.status).toBe(200); + expect(res.headers["content-type"]).toBe("text/markdown; charset=utf-8"); + expect(res.headers["content-disposition"]).toBe('attachment; filename="notes.md"'); + }); + it("keeps image attachments inline for previews", async () => { const storage = createStorageService(); mockIssueService.getAttachmentById.mockResolvedValue(makeAttachment("image/png", "preview.png")); diff --git a/server/src/__tests__/openapi-routes.test.ts b/server/src/__tests__/openapi-routes.test.ts index 2d1eeabfdc..d417e0b831 100644 --- a/server/src/__tests__/openapi-routes.test.ts +++ b/server/src/__tests__/openapi-routes.test.ts @@ -210,6 +210,11 @@ describe("openapi routes", () => { }, }); expect(res.body.paths["/api/companies/{companyId}/folders"].post.responses["201"]).toBeDefined(); + expect( + Object.keys( + res.body.paths["/api/issues/{id}/work-products/{workProductId}/review-document"].post.responses, + ).sort(), + ).toEqual(["200", "201", "401", "403", "404", "409", "413", "415", "422"]); expect( res.body.paths["/api/issues/{id}/interactions/{interactionId}/withdraw"].post.summary, ).toBe("Withdraw a pending issue thread interaction"); diff --git a/server/src/errors.ts b/server/src/errors.ts index d1d7d15f8d..07213e99b6 100644 --- a/server/src/errors.ts +++ b/server/src/errors.ts @@ -29,6 +29,14 @@ export function conflict(message: string, details?: unknown) { return new HttpError(409, message, details); } +export function payloadTooLarge(message: string, details?: unknown) { + return new HttpError(413, message, details); +} + +export function unsupportedMediaType(message: string, details?: unknown) { + return new HttpError(415, message, details); +} + export function unprocessable(message: string, details?: unknown) { return new HttpError(422, message, details); } diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index e3c90bb6fd..2413acaa9e 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -67,6 +67,8 @@ import { upsertIssueDocumentSchema, updateIssueSchema, isClosedIsolatedExecutionWorkspace, + isMarkdownArtifactWorkProduct, + isMarkdownAttachmentContent, isUuidLike, normalizeIssueIdentifier as normalizeIssueReferenceIdentifier, type CompactIssue, @@ -138,6 +140,7 @@ import { routineService, workProductService, } from "../services/index.js"; +import { artifactReviewDocumentService } from "../services/artifact-review-documents.js"; import { assertCanResolveProposal } from "../services/secret-proposal-authorization.js"; import { buildDocumentReviewContext, buildPlanReviewContext } from "../services/plan-review-context.js"; import { @@ -2830,6 +2833,7 @@ export function issueRoutes( const executionWorkspacesSvc = executionWorkspaceServiceDirect(db); const workProductsSvc = workProductService(db); const documentsSvc = documentService(db); + const artifactReviewDocumentsSvc = artifactReviewDocumentService(db, storage); const companySkillsSvc = companySkillService(db); const documentAnnotationsSvc = documentAnnotationService(db); const decisionTrainingSvc = decisionTrainingService(db); @@ -7888,9 +7892,110 @@ export function issueRoutes( actor, workProductChanged: true, }); + await materializeArtifactReviewDocumentBestEffort({ issue, workProduct: product, actor }); res.status(201).json(product); }); + async function ensureArtifactReviewDocumentForWorkProduct(input: { + issue: NonNullable>>; + workProduct: NonNullable>>; + actor: ReturnType; + }) { + const { issue, workProduct, actor } = input; + const result = await artifactReviewDocumentsSvc.ensureForWorkProduct({ + issue: { id: issue.id, companyId: issue.companyId }, + workProduct, + }); + if (!result.revisionChanged) return result; + const doc = result.document; + await issueReferencesSvc.syncDocument(doc.id); + await externalObjectsSvc.syncDocumentSafely(doc.id); + await logActivity(db, { + companyId: issue.companyId, + actorType: actor.actorType, + actorId: actor.actorId, + agentId: actor.agentId, + runId: actor.runId, + agentApiKeyId: actor.agentApiKeyId, + action: result.created ? "issue.document_created" : "issue.document_updated", + entityType: "issue", + entityId: issue.id, + details: { + key: doc.key, + documentId: doc.id, + title: doc.title, + format: doc.format, + revisionNumber: doc.latestRevisionNumber, + workProductId: workProduct.id, + artifactReviewDocument: true, + }, + }); + for (const remap of result.remappedAnnotations) { + await logActivity(db, { + companyId: issue.companyId, + actorType: actor.actorType, + actorId: actor.actorId, + agentId: actor.agentId, + runId: actor.runId, + agentApiKeyId: actor.agentApiKeyId, + action: "issue.document_annotation_remapped", + entityType: "issue", + entityId: issue.id, + details: { + key: doc.key, + documentId: doc.id, + threadId: remap.thread.id, + revisionNumber: doc.latestRevisionNumber, + anchorState: remap.thread.anchorState, + anchorConfidence: remap.thread.anchorConfidence, + snapshotId: remap.snapshot.id, + }, + }); + } + await revalidateActiveSourceRecoveryAfterCommittedWrite({ + issue, + trigger: "document", + actor, + documentChanged: true, + }); + return result; + } + + async function materializeArtifactReviewDocumentBestEffort(input: { + issue: NonNullable>>; + workProduct: NonNullable>>; + actor: ReturnType; + }) { + if (!isMarkdownArtifactWorkProduct(input.workProduct)) return; + try { + await ensureArtifactReviewDocumentForWorkProduct(input); + } catch (error) { + // Work-product writes stay fail-open: raw open and download remain + // available, and the explicit review-document endpoint is the retry path. + logger.warn( + { err: error, issueId: input.issue.id, workProductId: input.workProduct.id }, + "markdown work product review-document materialization failed", + ); + } + } + + router.post("/issues/:id/work-products/:workProductId/review-document", async (req, res) => { + const id = req.params.id as string; + const workProductId = req.params.workProductId as string; + const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + if (!issue) return; + if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return; + if (!(await assertDeliverableMutationAllowedByRunContext(req, res, issue))) return; + const workProduct = await workProductsSvc.getById(workProductId); + if (!workProduct || workProduct.issueId !== issue.id || workProduct.companyId !== issue.companyId) { + res.status(404).json({ error: "Work product not found" }); + return; + } + const actor = getActorInfo(req); + const result = await ensureArtifactReviewDocumentForWorkProduct({ issue, workProduct, actor }); + res.status(result.created ? 201 : 200).json(result.document); + }); + router.post("/issues/:id/low-trust/promotions", validate(promoteLowTrustOutputSchema), async (req, res) => { const id = req.params.id as string; const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); @@ -8087,6 +8192,11 @@ export function issueRoutes( actor, workProductChanged: true, }); + const reviewDocumentInputChanged = ["type", "provider", "metadata", "title", "createdByRunId"] + .some((key) => Object.prototype.hasOwnProperty.call(patch, key)); + if (reviewDocumentInputChanged || sourceTrust) { + await materializeArtifactReviewDocumentBestEffort({ issue, workProduct: product, actor }); + } res.json(product); }); @@ -12997,7 +13107,16 @@ export function issueRoutes( objectContentType: object.contentType, originalFilename: attachment.originalFilename, }); - res.setHeader("Content-Type", responseContentType); + // Markdown bodies are stored as UTF-8; declare the charset so inline + // (raw) views do not mojibake. SVG/inline checks below stay on the bare type. + const isMarkdownResponse = isMarkdownAttachmentContent({ + contentType: responseContentType, + originalFilename: attachment.originalFilename, + }); + res.setHeader( + "Content-Type", + isMarkdownResponse ? `${responseContentType}; charset=utf-8` : responseContentType, + ); res.setHeader("Cache-Control", "private, max-age=60"); res.setHeader("X-Content-Type-Options", "nosniff"); if (responseContentType === SVG_CONTENT_TYPE) { diff --git a/server/src/routes/openapi.ts b/server/src/routes/openapi.ts index 01cb4f05b0..d70249047b 100644 --- a/server/src/routes/openapi.ts +++ b/server/src/routes/openapi.ts @@ -536,6 +536,14 @@ const responses = { description: "Conflict", content: { "application/json": { schema: ErrorSchema } }, }, + payloadTooLarge: { + description: "Payload too large", + content: { "application/json": { schema: ErrorSchema } }, + }, + unsupportedMediaType: { + description: "Unsupported media type", + content: { "application/json": { schema: ErrorSchema } }, + }, unprocessable: { description: "Unprocessable entity", content: { "application/json": { schema: ErrorSchema } }, @@ -2318,6 +2326,25 @@ registry.registerPath({ responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized }, }); +registry.registerPath({ + method: "post", + path: "/api/issues/{id}/work-products/{workProductId}/review-document", + tags: ["issues"], + summary: "Ensure the review document for a Markdown work product", + request: { params: z.object({ id: z.string(), workProductId: z.string() }) }, + responses: { + 200: r.ok(), + 201: r.ok(), + 401: r.unauthorized, + 403: r.forbidden, + 404: r.notFound, + 409: r.conflict, + 413: r.payloadTooLarge, + 415: r.unsupportedMediaType, + 422: r.unprocessable, + }, +}); + registry.registerPath({ method: "patch", path: "/api/work-products/{id}", diff --git a/server/src/services/artifact-review-documents.ts b/server/src/services/artifact-review-documents.ts new file mode 100644 index 0000000000..3e86b189ef --- /dev/null +++ b/server/src/services/artifact-review-documents.ts @@ -0,0 +1,245 @@ +import { isDeepStrictEqual } from "node:util"; +import { and, eq } from "drizzle-orm"; +import type { Db } from "@paperclipai/db"; +import { assets, documentRevisions, documents, issueAttachments, issueDocuments } from "@paperclipai/db"; +import { + MARKDOWN_REVIEW_DOCUMENT_MAX_BYTES, + artifactReviewDocumentKey, + getAttachmentArtifactWorkProductMetadata, + isMarkdownAttachmentContent, + type AttachmentArtifactWorkProductMetadata, + type IssueWorkProduct, +} from "@paperclipai/shared"; +import { + HttpError, + conflict, + notFound, + payloadTooLarge, + unprocessable, + unsupportedMediaType, +} from "../errors.js"; +import type { StorageService } from "../storage/types.js"; +import { documentAnnotationService } from "./document-annotations.js"; +import { documentService, issueDocumentSelect, mapIssueDocumentRow } from "./documents.js"; + +export type EnsureArtifactReviewWorkProduct = Pick< + IssueWorkProduct, + "id" | "companyId" | "issueId" | "type" | "provider" | "metadata" | "title" | "createdByRunId" | "sourceTrust" +>; + +export interface EnsureArtifactReviewDocumentInput { + issue: { id: string; companyId: string }; + workProduct: EnsureArtifactReviewWorkProduct; +} + +type RemappedAnnotations = Awaited< + ReturnType["remapOpenThreadsForDocument"]> +>; + +export interface EnsureArtifactReviewDocumentResult { + document: ReturnType & { body?: string }; + created: boolean; + revisionChanged: boolean; + remappedAnnotations: RemappedAnnotations; +} + +/** + * Materializes the review IssueDocument for an eligible Markdown + * attachment-backed work product under the deterministic + * `artifact-review-` key. The operation is idempotent: repeated + * calls return the existing document and only write a new revision when the + * review content or its work-product provenance changed. + */ +export function artifactReviewDocumentService(db: Db, storage: StorageService) { + const documentsSvc = documentService(db); + const annotationsSvc = documentAnnotationService(db); + + const getExistingByKey = async (issueId: string, key: string) => { + const document = await db + .select(issueDocumentSelect) + .from(issueDocuments) + .innerJoin(documents, eq(issueDocuments.documentId, documents.id)) + .where(and(eq(issueDocuments.issueId, issueId), eq(issueDocuments.key, key))) + .then((rows) => (rows[0] ? mapIssueDocumentRow(rows[0], true) : null)); + if (!document) return null; + const latestRevision = document.latestRevisionId + ? await db + .select({ + createdByAgentId: documentRevisions.createdByAgentId, + createdByUserId: documentRevisions.createdByUserId, + createdByRunId: documentRevisions.createdByRunId, + }) + .from(documentRevisions) + .where(and( + eq(documentRevisions.id, document.latestRevisionId), + eq(documentRevisions.documentId, document.id), + eq(documentRevisions.companyId, document.companyId), + )) + .then((rows) => rows[0] ?? null) + : null; + return { document, latestRevision }; + }; + + const readMarkdownAttachmentBody = async ( + issue: { id: string; companyId: string }, + metadata: AttachmentArtifactWorkProductMetadata, + ) => { + const attachment = await db + .select({ + id: issueAttachments.id, + companyId: issueAttachments.companyId, + issueId: issueAttachments.issueId, + objectKey: assets.objectKey, + contentType: assets.contentType, + byteSize: assets.byteSize, + originalFilename: assets.originalFilename, + createdByAgentId: assets.createdByAgentId, + createdByUserId: assets.createdByUserId, + }) + .from(issueAttachments) + .innerJoin(assets, eq(issueAttachments.assetId, assets.id)) + .where(eq(issueAttachments.id, metadata.attachmentId)) + .then((rows) => rows[0] ?? null); + // Unknown and cross-scope attachment ids are deliberately + // indistinguishable: canonical metadata must reference an attachment on + // the same issue and company. + if (!attachment || attachment.companyId !== issue.companyId || attachment.issueId !== issue.id) { + throw unprocessable("Work product attachment must reference an attachment on the same issue", { + code: "invalid_attachment_artifact_metadata", + attachmentId: metadata.attachmentId, + }); + } + if (!isMarkdownAttachmentContent(attachment)) { + throw unsupportedMediaType("Work product attachment is not Markdown", { + code: "unsupported_review_document_content_type", + contentType: attachment.contentType, + }); + } + if (attachment.byteSize > MARKDOWN_REVIEW_DOCUMENT_MAX_BYTES) { + throw payloadTooLarge("Markdown attachment exceeds the review document size limit", { + code: "review_document_too_large", + byteSize: attachment.byteSize, + maxBytes: MARKDOWN_REVIEW_DOCUMENT_MAX_BYTES, + }); + } + + const object = await storage.getObject(attachment.companyId, attachment.objectKey); + const chunks: Buffer[] = []; + let total = 0; + for await (const chunk of object.stream) { + const buffer = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk); + total += buffer.length; + // Defense in depth: enforce the cap during streaming too, so a + // metadata/object size mismatch can never exceed the limit. + if (total > MARKDOWN_REVIEW_DOCUMENT_MAX_BYTES) { + object.stream.destroy(); + throw payloadTooLarge("Markdown attachment exceeds the review document size limit", { + code: "review_document_too_large", + maxBytes: MARKDOWN_REVIEW_DOCUMENT_MAX_BYTES, + }); + } + chunks.push(buffer); + } + + let body: string; + try { + body = new TextDecoder("utf-8", { fatal: true }).decode(Buffer.concat(chunks)); + } catch { + throw unprocessable("Markdown attachment is not valid UTF-8", { + code: "invalid_review_document_encoding", + attachmentId: attachment.id, + }); + } + return { attachment, body }; + }; + + return { + ensureForWorkProduct: async ( + input: EnsureArtifactReviewDocumentInput, + ): Promise => { + const { issue, workProduct } = input; + if (workProduct.issueId !== issue.id || workProduct.companyId !== issue.companyId) { + throw notFound("Work product not found"); + } + const metadata = getAttachmentArtifactWorkProductMetadata(workProduct); + if (!metadata) { + throw unprocessable("Work product is not an attachment-backed Paperclip artifact", { + code: "not_attachment_backed_artifact", + workProductId: workProduct.id, + }); + } + if (!isMarkdownAttachmentContent(metadata)) { + throw unsupportedMediaType("Work product attachment is not Markdown", { + code: "unsupported_review_document_content_type", + contentType: metadata.contentType, + }); + } + + const { attachment, body } = await readMarkdownAttachmentBody(issue, metadata); + const key = artifactReviewDocumentKey(workProduct.id); + // The review document represents agent-authored attachment content, so + // attribution promotes the attachment/work-product provenance instead of + // the actor who triggered materialization. + const attribution = { + createdByAgentId: attachment.createdByAgentId ?? null, + createdByUserId: attachment.createdByUserId ?? null, + createdByRunId: workProduct.createdByRunId ?? null, + sourceTrust: workProduct.sourceTrust ?? null, + }; + + for (let attempt = 0; attempt < 2; attempt += 1) { + const existingState = await getExistingByKey(issue.id, key); + const existing = existingState?.document ?? null; + const latestRevision = existingState?.latestRevision ?? null; + const reviewStateMatches = existing && + existing.body === body && + existing.title === (workProduct.title ?? null) && + existing.format === "markdown" && + latestRevision?.createdByAgentId === attribution.createdByAgentId && + latestRevision.createdByUserId === attribution.createdByUserId && + latestRevision.createdByRunId === attribution.createdByRunId && + isDeepStrictEqual(existing.sourceTrust ?? null, attribution.sourceTrust); + if (reviewStateMatches) { + return { document: existing, created: false, revisionChanged: false, remappedAnnotations: [] }; + } + try { + const result = await documentsSvc.upsertIssueDocument({ + issueId: issue.id, + key, + title: workProduct.title ?? null, + format: "markdown", + body, + changeSummary: existing + ? "Synced from the work product attachment" + : "Materialized from the Markdown work product attachment", + baseRevisionId: existing?.latestRevisionId ?? null, + ...attribution, + lockedDocumentStrategy: "conflict", + }); + const remappedAnnotations = result.created + ? [] + : await annotationsSvc.remapOpenThreadsForDocument({ + issueId: issue.id, + key: result.document.key, + documentId: result.document.id, + nextRevisionId: result.document.latestRevisionId, + nextRevisionNumber: result.document.latestRevisionNumber, + nextBody: result.document.body, + }); + return { + document: result.document, + created: result.created, + revisionChanged: true, + remappedAnnotations, + }; + } catch (error) { + // A concurrent materialization or document write raced this one. + // Re-read once so identical content converges on the winner. + if (error instanceof HttpError && error.status === 409 && attempt === 0) continue; + throw error; + } + } + throw conflict("Concurrent review-document updates did not converge", { key }); + }, + }; +} diff --git a/server/src/services/index.ts b/server/src/services/index.ts index 3b7569b1a6..54e56fc936 100644 --- a/server/src/services/index.ts +++ b/server/src/services/index.ts @@ -23,6 +23,7 @@ export { export { agentInstructionsService, syncInstructionsBundleConfigFromFilePath } from "./agent-instructions.js"; export { assetService } from "./assets.js"; export { documentService, extractLegacyPlanBody } from "./documents.js"; +export { artifactReviewDocumentService } from "./artifact-review-documents.js"; export { statusCardService } from "./status-cards.js"; export { finalizeStatusCardsForStalledGeneration } from "./status-card-finalization.js"; export { documentAnnotationService } from "./document-annotations.js"; diff --git a/ui/src/api/issues.ts b/ui/src/api/issues.ts index 5eae6f0685..4f00784fd7 100644 --- a/ui/src/api/issues.ts +++ b/ui/src/api/issues.ts @@ -342,6 +342,8 @@ export const issuesApi = { unlinkApproval: (id: string, approvalId: string) => api.delete<{ ok: true }>(`/issues/${id}/approvals/${approvalId}`), listWorkProducts: (id: string) => api.get(`/issues/${id}/work-products`), + ensureWorkProductReviewDocument: (id: string, workProductId: string) => + api.post(`/issues/${id}/work-products/${workProductId}/review-document`, {}), createWorkProduct: (id: string, data: Record) => api.post(`/issues/${id}/work-products`, data), updateWorkProduct: (id: string, data: Record) => diff --git a/ui/src/components/issue-properties/IssueProperties.tsx b/ui/src/components/issue-properties/IssueProperties.tsx index 4df10852be..2aadb3994b 100644 --- a/ui/src/components/issue-properties/IssueProperties.tsx +++ b/ui/src/components/issue-properties/IssueProperties.tsx @@ -5,7 +5,12 @@ import { pickTextColorForPillBg } from "@/lib/color-contrast"; import { issueStatusText } from "@/lib/status-colors"; import { copyTextToClipboard } from "@/lib/clipboard"; import { Link } from "@/lib/router"; -import { deriveOriginatingActor, type Issue, type IssueLabel } from "@paperclipai/shared"; +import { + deriveOriginatingActor, + isArtifactReviewDocumentKey, + type Issue, + type IssueLabel, +} from "@paperclipai/shared"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { accessApi } from "../../api/access"; import { agentsApi } from "../../api/agents"; @@ -225,10 +230,15 @@ export function IssueProperties({ enabled: taskChatShellEnabled, }); const { data: paneTabDocuments } = useIssueDocuments(taskChatShellEnabled ? issue.id : null); + // Proxy `artifact-review-*` documents surface only through their Work + // product row, so they must not summon the Plan or Documents surfaces. + const paneTabStandaloneDocuments = (paneTabDocuments ?? []).filter( + (doc) => !isArtifactReviewDocumentKey(doc.key), + ); const hasPlanTab = Boolean(paneTabPlanDocument) || (paneTabAcceptedPlans?.length ?? 0) > 0 - || (paneTabDocuments?.length ?? 0) > 0 + || paneTabStandaloneDocuments.length > 0 || issue.workMode === "planning"; // Artifacts covers the same three sources the tab body composes: work // products, documents (redundant with the Plan tab, intentionally), and @@ -236,7 +246,7 @@ export function IssueProperties({ // no longer summon the tab. const hasArtifactsTab = (paneTabWorkProducts?.length ?? 0) > 0 - || (paneTabDocuments?.length ?? 0) > 0 + || paneTabStandaloneDocuments.length > 0 || selectAgentArtifactAttachments(paneTabAttachments, paneTabWorkProducts).length > 0; const [paneTab, setPaneTab] = useState("properties"); // Once a plan document exists, surface it: switch the pane to the Plan tab so diff --git a/ui/src/components/issue-properties/IssuePropertiesArtifactsTab.tsx b/ui/src/components/issue-properties/IssuePropertiesArtifactsTab.tsx index 4d8ca4749e..dea4ed8a2c 100644 --- a/ui/src/components/issue-properties/IssuePropertiesArtifactsTab.tsx +++ b/ui/src/components/issue-properties/IssuePropertiesArtifactsTab.tsx @@ -1,10 +1,22 @@ import { useEffect, useRef, useState } from "react"; import type { CSSProperties } from "react"; -import { useQuery } from "@tanstack/react-query"; -import type { Issue, IssueDocument, IssueWorkProduct } from "@paperclipai/shared"; +import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; +import type { + AttachmentArtifactWorkProductMetadata, + Issue, + IssueDocument, + IssueWorkProduct, +} from "@paperclipai/shared"; +import { + MARKDOWN_REVIEW_DOCUMENT_MAX_BYTES, + artifactReviewDocumentKey, + getMarkdownWorkProductAttachmentMetadata, + isArtifactReviewDocumentKey, +} from "@paperclipai/shared"; import { ChevronDown, ChevronRight, + Download, ExternalLink, FileText, GitBranch, @@ -15,6 +27,7 @@ import { Server, } from "lucide-react"; import type { LucideIcon } from "lucide-react"; +import { ApiError } from "@/api/client"; import { issuesApi } from "@/api/issues"; import { queryKeys } from "@/lib/queryKeys"; import { useIssueDocuments } from "@/hooks/useIssueDocuments"; @@ -122,6 +135,186 @@ function WorkProductRow({ workProduct }: { workProduct: IssueWorkProduct }) { return
{body}
; } +/** + * Work-product row for an eligible Markdown artifact (LOOA-1533 gap): expands + * in place into the shipped document review surface backed by the + * server-materialized `artifact-review-` issue document instead + * of opening the raw attachment. Raw open and download stay as explicit + * secondary actions. + */ +function MarkdownWorkProductRow({ + issueId, + workProduct, + metadata, + reviewDoc, + openRequestId, +}: { + issueId: string; + workProduct: IssueWorkProduct; + metadata: AttachmentArtifactWorkProductMetadata; + reviewDoc: IssueDocument | undefined; + openRequestId?: number; +}) { + const [expanded, setExpanded] = useState(false); + const [annotationPanelOpen, setAnnotationPanelOpen] = useState(false); + const headerRef = useRef(null); + const location = useLocation(); + const queryClient = useQueryClient(); + const badge = workProductStatusBadge(workProduct.status); + const Chevron = expanded ? ChevronDown : ChevronRight; + const tooLarge = metadata.byteSize > MARKDOWN_REVIEW_DOCUMENT_MAX_BYTES; + + const ensure = useMutation({ + mutationFn: () => issuesApi.ensureWorkProductReviewDocument(issueId, workProduct.id), + onSuccess: async () => { + await queryClient.invalidateQueries({ queryKey: queryKeys.issues.documents(issueId) }); + }, + }); + + const requestPreview = () => { + if (reviewDoc || tooLarge) return; + if (ensure.isPending || ensure.isSuccess || ensure.isError) return; + ensure.mutate(); + }; + + const handleToggle = () => { + setExpanded((open) => { + if (!open) requestPreview(); + return !open; + }); + }; + + useEffect(() => { + if (openRequestId === undefined) return; + setExpanded(true); + }, [openRequestId]); + useEffect(() => { + if (openRequestId === undefined || !expanded) return; + headerRef.current?.scrollIntoView({ behavior: "smooth", block: "center" }); + }, [expanded, openRequestId]); + // Deep links land before the user clicks, so the deep-link expansion has to + // request materialization the same way a manual expand does. + useEffect(() => { + if (openRequestId === undefined) return; + requestPreview(); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [openRequestId, reviewDoc]); + + const unsupportedError = + ensure.error instanceof ApiError && [413, 415, 422].includes(ensure.error.status); + + let expandedBody: React.ReactNode; + if (tooLarge) { + expandedBody = ( +

+ This Markdown file is too large to preview. Use Raw or Download instead. +

+ ); + } else if (reviewDoc) { + expandedBody = reviewDoc.body.trim().length > 0 ? ( + + {reviewDoc.body} + + ) : ( +

Document is empty.

+ ); + } else if (ensure.isError) { + expandedBody = ( +
+

+ {unsupportedError + ? "This file can't be previewed as Markdown. Use Raw or Download instead." + : "Preview failed to load."} +

+ {!unsupportedError ? ( + + ) : null} +
+ ); + } else { + expandedBody =

Preparing preview…

; + } + + return ( +
+
+ + {reviewDoc ? ( + setAnnotationPanelOpen((open) => !open)} + /> + ) : null} + + + + + + +
+ {expanded ? ( +
{expandedBody}
+ ) : null} +
+ ); +} + function DocumentRow({ issueId, doc, @@ -215,7 +408,10 @@ export function IssuePropertiesArtifactsTab({ issue, documentDeepLink }: IssuePr const { data: documents } = useIssueDocuments(issue.id); const workProductRows = workProducts ?? []; - const documentRows = documents ?? []; + // Proxy review documents (`artifact-review-*`) present only through their + // originating Work product row, never as standalone Documents rows. + const documentRows = (documents ?? []).filter((doc) => !isArtifactReviewDocumentKey(doc.key)); + const reviewDocsByKey = new Map((documents ?? []).map((doc) => [doc.key, doc])); const fileRows = selectAgentArtifactAttachments(attachments, workProducts); if (workProductRows.length === 0 && documentRows.length === 0 && fileRows.length === 0) { @@ -232,11 +428,30 @@ export function IssuePropertiesArtifactsTab({ issue, documentDeepLink }: IssuePr <> Work products
    - {workProductRows.map((wp) => ( -
  • - -
  • - ))} + {workProductRows.map((wp) => { + const markdownMetadata = getMarkdownWorkProductAttachmentMetadata(wp); + if (!markdownMetadata) { + return ( +
  • + +
  • + ); + } + const reviewKey = artifactReviewDocumentKey(wp.id); + return ( +
  • + +
  • + ); + })}
) : null} diff --git a/ui/src/components/issue-properties/IssuePropertiesMarkdownWorkProduct.test.tsx b/ui/src/components/issue-properties/IssuePropertiesMarkdownWorkProduct.test.tsx new file mode 100644 index 0000000000..b8c640daf6 --- /dev/null +++ b/ui/src/components/issue-properties/IssuePropertiesMarkdownWorkProduct.test.tsx @@ -0,0 +1,344 @@ +// @vitest-environment jsdom + +import { flushSync } from "react-dom"; +import { createRoot, type Root } from "react-dom/client"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { Issue, IssueAttachment, IssueDocument, IssueWorkProduct } from "@paperclipai/shared"; +import { artifactReviewDocumentKey } from "@paperclipai/shared"; +import { IssuePropertiesArtifactsTab } from "./IssuePropertiesArtifactsTab"; +import { ApiError } from "@/api/client"; + +const mockIssuesApi = vi.hoisted(() => ({ + listAttachments: vi.fn(async (): Promise => []), + listWorkProducts: vi.fn(async (): Promise => []), + ensureWorkProductReviewDocument: vi.fn(async (): Promise => ({})), +})); +vi.mock("@/api/issues", () => ({ issuesApi: mockIssuesApi })); + +const mockUseIssueDocuments = vi.hoisted(() => + vi.fn((): { data: IssueDocument[] } => ({ data: [] })), +); +vi.mock("@/hooks/useIssueDocuments", () => ({ useIssueDocuments: mockUseIssueDocuments })); +vi.mock("@/lib/router", () => ({ useLocation: () => ({ hash: "" }) })); +vi.mock("@/components/MarkdownBody", () => ({ + MarkdownBody: ({ children }: { children: string }) => ( +
{children}
+ ), +})); +vi.mock("@/components/IssueDocumentAnnotations", () => ({ + DocumentAnnotationsCountChip: ({ docKey }: { docKey: string }) => ( + + ), + IssueDocumentAnnotations: ({ doc, children }: { doc: IssueDocument; children: React.ReactNode }) => ( +
{children}
+ ), +})); + +const ATTACHMENT_ID = "00000000-0000-4000-8000-000000000001"; +const WORK_PRODUCT_ID = "11111111-1111-4111-8111-111111111111"; +const REVIEW_KEY = artifactReviewDocumentKey(WORK_PRODUCT_ID); + +const issue = { id: "issue-1", identifier: "PAP-1534", workMode: "standard" } as Issue; + +function makeMarkdownWorkProduct(overrides: Partial = {}): IssueWorkProduct { + const contentPath = `/api/attachments/${ATTACHMENT_ID}/content`; + return { + id: WORK_PRODUCT_ID, + companyId: "company-1", + projectId: null, + issueId: "issue-1", + executionWorkspaceId: null, + runtimeServiceId: null, + type: "artifact", + provider: "paperclip", + externalId: null, + title: "Verification report", + url: null, + status: "ready_for_review", + reviewState: "none", + isPrimary: false, + healthStatus: "unknown", + summary: null, + metadata: { + attachmentId: ATTACHMENT_ID, + contentType: "text/markdown", + byteSize: 64, + contentPath, + openPath: contentPath, + downloadPath: `${contentPath}?download=1`, + originalFilename: "report.md", + }, + createdByRunId: null, + createdAt: new Date("2026-08-01T12:00:00Z"), + updatedAt: new Date("2026-08-01T12:00:00Z"), + ...overrides, + } as IssueWorkProduct; +} + +function makeReviewDocument(overrides: Partial = {}): IssueDocument { + return { + id: "document-1", + companyId: "company-1", + issueId: "issue-1", + key: REVIEW_KEY, + title: "Verification report", + format: "markdown", + body: "# Report\n\nRendered body", + latestRevisionId: "revision-1", + latestRevisionNumber: 1, + createdByAgentId: null, + createdByUserId: null, + updatedByAgentId: null, + updatedByUserId: null, + lockedAt: null, + lockedByAgentId: null, + lockedByUserId: null, + createdAt: new Date("2026-08-01T12:00:00Z"), + updatedAt: new Date("2026-08-01T12:00:00Z"), + ...overrides, + } as IssueDocument; +} + +function makeMarkdownAttachment(): IssueAttachment { + return { + id: ATTACHMENT_ID, + companyId: "company-1", + issueId: "issue-1", + issueCommentId: null, + assetId: "asset-1", + provider: "local", + objectKey: `objects/${ATTACHMENT_ID}`, + contentType: "text/markdown", + byteSize: 64, + sha256: "0".repeat(64), + originalFilename: "report.md", + createdByAgentId: "agent-1", + createdByUserId: null, + createdAt: new Date("2026-08-01T12:00:00Z"), + updatedAt: new Date("2026-08-01T12:00:00Z"), + contentPath: `/api/attachments/${ATTACHMENT_ID}/content`, + } as IssueAttachment; +} + +async function act(callback: () => void | Promise) { + let result: void | Promise = undefined; + flushSync(() => { + result = callback(); + }); + await result; +} + +async function flush() { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); +} + +async function waitForAssertion(assertion: () => void, attempts = 20) { + let lastError: unknown; + for (let attempt = 0; attempt < attempts; attempt += 1) { + try { + assertion(); + return; + } catch (error) { + lastError = error; + await flush(); + } + } + throw lastError; +} + +describe("markdown work product review row", () => { + let container: HTMLDivElement; + let root: Root | null = null; + + beforeEach(() => { + vi.clearAllMocks(); + container = document.createElement("div"); + document.body.appendChild(container); + Element.prototype.scrollIntoView = vi.fn(); + mockIssuesApi.listAttachments.mockResolvedValue([]); + mockIssuesApi.listWorkProducts.mockResolvedValue([makeMarkdownWorkProduct()]); + mockUseIssueDocuments.mockReturnValue({ data: [] }); + }); + + afterEach(async () => { + if (root) { + const currentRoot = root; + await act(async () => currentRoot.unmount()); + root = null; + } + container.remove(); + }); + + async function renderTab(props: { documentDeepLink?: { requestId: number; documentKey: string } | null } = {}) { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false }, mutations: { retry: false } }, + }); + root = createRoot(container); + const currentRoot = root; + await act(async () => + currentRoot.render( + + + , + ), + ); + await waitForAssertion(() => { + expect(container.textContent).toContain("Verification report"); + }); + } + + function expandButton() { + return container.querySelector('button[aria-expanded]') as HTMLButtonElement; + } + + it("renders an expandable row with explicit raw and download actions", async () => { + await renderTab(); + + expect(expandButton().textContent).toContain("Verification report"); + const raw = container.querySelector('a[title="Open raw"]') as HTMLAnchorElement; + const download = container.querySelector('a[title="Download"]') as HTMLAnchorElement; + expect(raw?.getAttribute("href")).toBe(`/api/attachments/${ATTACHMENT_ID}/content`); + expect(raw?.getAttribute("target")).toBe("_blank"); + expect(download?.getAttribute("href")).toBe(`/api/attachments/${ATTACHMENT_ID}/content?download=1`); + }); + + it("expands into the existing document surface without a server call when the document exists", async () => { + mockUseIssueDocuments.mockReturnValue({ data: [makeReviewDocument()] }); + await renderTab(); + + // The proxy document maps onto the work-product row instead of a + // standalone Documents row. + expect(container.textContent).not.toContain("Documents"); + expect(container.querySelector(`[data-testid="annotation-count-${REVIEW_KEY}"]`)).not.toBeNull(); + + await act(async () => expandButton().click()); + expect( + container.querySelector(`[data-testid="annotation-surface-${REVIEW_KEY}"]`)?.getAttribute("data-document-id"), + ).toBe("document-1"); + expect(container.querySelector('[data-testid="markdown-body"]')?.textContent).toContain("Rendered body"); + expect(mockIssuesApi.ensureWorkProductReviewDocument).not.toHaveBeenCalled(); + }); + + it("materializes the document on first expand", async () => { + mockIssuesApi.ensureWorkProductReviewDocument.mockResolvedValue(makeReviewDocument()); + await renderTab(); + + await act(async () => expandButton().click()); + expect(container.textContent).toContain("Preparing preview…"); + await waitForAssertion(() => { + expect(mockIssuesApi.ensureWorkProductReviewDocument).toHaveBeenCalledWith("issue-1", WORK_PRODUCT_ID); + }); + expect(mockIssuesApi.ensureWorkProductReviewDocument).toHaveBeenCalledTimes(1); + }); + + it("shows an unsupported state without retry for typed rejections", async () => { + mockIssuesApi.ensureWorkProductReviewDocument.mockRejectedValue( + new ApiError("Work product attachment is not Markdown", 415, {}), + ); + await renderTab(); + + await act(async () => expandButton().click()); + await waitForAssertion(() => { + expect(container.textContent).toContain("can't be previewed as Markdown"); + }); + expect(container.textContent).not.toContain("Retry"); + }); + + it("offers an explicit retry for recoverable errors", async () => { + mockIssuesApi.ensureWorkProductReviewDocument.mockRejectedValue( + new ApiError("Internal error", 500, {}), + ); + await renderTab(); + + await act(async () => expandButton().click()); + await waitForAssertion(() => { + expect(container.textContent).toContain("Preview failed to load."); + }); + + const retry = Array.from(container.querySelectorAll("button")).find( + (button) => button.textContent === "Retry", + ); + expect(retry).toBeDefined(); + await act(async () => retry?.click()); + await waitForAssertion(() => { + expect(mockIssuesApi.ensureWorkProductReviewDocument).toHaveBeenCalledTimes(2); + }); + }); + + it("marks oversized markdown as too large without calling the server", async () => { + mockIssuesApi.listWorkProducts.mockResolvedValue([ + makeMarkdownWorkProduct({ + metadata: { + ...(makeMarkdownWorkProduct().metadata as Record), + byteSize: 512 * 1024 + 1, + }, + }), + ]); + await renderTab(); + + await act(async () => expandButton().click()); + expect(container.textContent).toContain("too large to preview"); + expect(mockIssuesApi.ensureWorkProductReviewDocument).not.toHaveBeenCalled(); + }); + + it("expands from a document deep link and requests materialization", async () => { + mockIssuesApi.ensureWorkProductReviewDocument.mockResolvedValue(makeReviewDocument()); + await renderTab({ documentDeepLink: { documentKey: REVIEW_KEY, requestId: 1 } }); + + await waitForAssertion(() => { + expect(container.querySelector('button[aria-expanded="true"]')).not.toBeNull(); + expect(mockIssuesApi.ensureWorkProductReviewDocument).toHaveBeenCalledTimes(1); + expect(Element.prototype.scrollIntoView).toHaveBeenCalledWith({ behavior: "smooth", block: "center" }); + }); + }); + + it("hides the promoted markdown attachment from Files but keeps loose attachments", async () => { + const loose = { + ...makeMarkdownAttachment(), + id: "22222222-2222-4222-8222-222222222222", + originalFilename: "loose-notes.md", + contentPath: "/api/attachments/22222222-2222-4222-8222-222222222222/content", + }; + mockIssuesApi.listAttachments.mockResolvedValue([makeMarkdownAttachment(), loose]); + await renderTab(); + + await waitForAssertion(() => { + expect(container.textContent).toContain("loose-notes.md"); + }); + expect(container.textContent).not.toContain("report.md"); + const looseLink = Array.from(container.querySelectorAll("a")).find( + (anchor) => anchor.textContent?.includes("loose-notes.md"), + ); + expect(looseLink?.getAttribute("href")).toBe("/api/attachments/22222222-2222-4222-8222-222222222222/content"); + }); + + it("keeps non-markdown work products on the raw link row", async () => { + const contentPath = `/api/attachments/${ATTACHMENT_ID}/content`; + mockIssuesApi.listWorkProducts.mockResolvedValue([ + makeMarkdownWorkProduct({ + metadata: { + attachmentId: ATTACHMENT_ID, + contentType: "application/pdf", + byteSize: 64, + contentPath, + openPath: contentPath, + downloadPath: `${contentPath}?download=1`, + originalFilename: "report.pdf", + }, + }), + ]); + await renderTab(); + + await waitForAssertion(() => { + const row = Array.from(container.querySelectorAll("a")).find( + (anchor) => anchor.textContent?.includes("Verification report"), + ); + expect(row?.getAttribute("href")).toBe(contentPath); + expect(row?.getAttribute("target")).toBe("_blank"); + }); + expect(container.querySelector("button[aria-expanded]")).toBeNull(); + }); +}); diff --git a/ui/src/components/issue-properties/IssuePropertiesPlansTab.tsx b/ui/src/components/issue-properties/IssuePropertiesPlansTab.tsx index 1a5f90ea4c..b3458fbaa4 100644 --- a/ui/src/components/issue-properties/IssuePropertiesPlansTab.tsx +++ b/ui/src/components/issue-properties/IssuePropertiesPlansTab.tsx @@ -1,6 +1,7 @@ import { useState } from "react"; import { useQuery } from "@tanstack/react-query"; import type { Issue, IssueDocument, IssueThreadInteraction } from "@paperclipai/shared"; +import { isArtifactReviewDocumentKey } from "@paperclipai/shared"; import { issuesApi } from "@/api/issues"; import { queryKeys } from "@/lib/queryKeys"; import { IssuePlanDecompositionsSection } from "@/components/IssuePlanDecompositionsSection"; @@ -92,8 +93,11 @@ export function IssuePropertiesPlansTab({ issue }: IssuePropertiesPlansTabProps) const hasPlans = (data?.length ?? 0) > 0; const pendingPlanConfirmation = hasPendingPlanConfirmation(interactions); // Every other non-system document (e.g. `synthesis`) renders below the plan; - // the `plan` doc itself stays on its dedicated annotated surface above. - const otherDocuments = (documents ?? []).filter((doc) => doc.key !== "plan"); + // the `plan` doc itself stays on its dedicated annotated surface above, and + // proxy `artifact-review-*` documents surface only through their Work product. + const otherDocuments = (documents ?? []).filter( + (doc) => doc.key !== "plan" && !isArtifactReviewDocumentKey(doc.key), + ); if (!planDocument && !hasPlans && otherDocuments.length === 0) { return ( diff --git a/ui/src/lib/issue-artifacts.test.ts b/ui/src/lib/issue-artifacts.test.ts index 026cff4739..42dc6d3ee8 100644 --- a/ui/src/lib/issue-artifacts.test.ts +++ b/ui/src/lib/issue-artifacts.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import type { IssueAttachment, IssueWorkProduct } from "@paperclipai/shared"; import { documentDisplayTitle, + getAttachmentBackedWorkProductAttachmentIds, isAgentAttachment, selectAgentArtifactAttachments, workProductHref, @@ -112,6 +113,54 @@ describe("selectAgentArtifactAttachments", () => { it("tolerates missing inputs", () => { expect(selectAgentArtifactAttachments(null, null)).toEqual([]); }); + + it("dedupes markdown attachments promoted to work products", () => { + // Markdown is excluded from the binary Output surface, so this dedupe must + // not depend on getPromotedOutputAttachmentIds (LOOA-1533 duplicate rows). + const promotedId = "00000000-0000-4000-8000-000000000002"; + const promoted = makeAttachment({ + id: promotedId, + createdByAgentId: "agent-1", + contentType: "text/markdown", + originalFilename: "report.md", + }); + const workProduct = makePromotingWorkProduct(promotedId); + workProduct.metadata = { + ...(workProduct.metadata as Record), + contentType: "text/markdown", + originalFilename: "report.md", + }; + expect(selectAgentArtifactAttachments([promoted], [workProduct])).toEqual([]); + }); +}); + +describe("getAttachmentBackedWorkProductAttachmentIds", () => { + it("collects attachment ids across content types", () => { + const imageId = "00000000-0000-4000-8000-000000000001"; + const markdownId = "00000000-0000-4000-8000-000000000002"; + const markdownWorkProduct = makePromotingWorkProduct(markdownId); + markdownWorkProduct.metadata = { + ...(markdownWorkProduct.metadata as Record), + contentType: "text/markdown", + originalFilename: "report.md", + }; + const ids = getAttachmentBackedWorkProductAttachmentIds([ + makePromotingWorkProduct(imageId), + markdownWorkProduct, + ]); + expect(ids).toEqual(new Set([imageId, markdownId])); + }); + + it("ignores non-canonical or non-paperclip work products", () => { + const attachmentId = "00000000-0000-4000-8000-000000000003"; + const foreign = { ...makePromotingWorkProduct(attachmentId), provider: "github" }; + const invalid = { + ...makePromotingWorkProduct(attachmentId), + metadata: { attachmentId, openPath: "https://evil.example/content" }, + }; + expect(getAttachmentBackedWorkProductAttachmentIds([foreign, invalid])).toEqual(new Set()); + expect(getAttachmentBackedWorkProductAttachmentIds(null)).toEqual(new Set()); + }); }); describe("workProductHref", () => { diff --git a/ui/src/lib/issue-artifacts.ts b/ui/src/lib/issue-artifacts.ts index 7563638c49..f3f3d8a6d1 100644 --- a/ui/src/lib/issue-artifacts.ts +++ b/ui/src/lib/issue-artifacts.ts @@ -1,5 +1,5 @@ import type { IssueAttachment, IssueDocumentSummary, IssueWorkProduct } from "@paperclipai/shared"; -import { getPromotedOutputAttachmentIds } from "./issue-output"; +import { getAttachmentArtifactWorkProductMetadata } from "@paperclipai/shared"; /** * Selectors for the properties pane's Artifacts tab (PAP-491): which @@ -19,6 +19,23 @@ export function isAgentAttachment( return !attachment.createdByUserId && !attachment.issueCommentId; } +/** + * Attachment ids owned by valid attachment-backed Paperclip work products. + * Unlike `getPromotedOutputAttachmentIds`, this includes document-like content + * types (e.g. Markdown) that the binary Output surface intentionally excludes, + * so it is the right dedupe set for the Artifacts tab's Files section. + */ +export function getAttachmentBackedWorkProductAttachmentIds( + workProducts: IssueWorkProduct[] | null | undefined, +): Set { + const ids = new Set(); + for (const workProduct of workProducts ?? []) { + const metadata = getAttachmentArtifactWorkProductMetadata(workProduct); + if (metadata) ids.add(metadata.attachmentId); + } + return ids; +} + /** * Agent-authored attachments minus the ones already promoted to * attachment-backed work products (`metadata.attachmentId`), so the Artifacts @@ -28,7 +45,7 @@ export function selectAgentArtifactAttachments( attachments: IssueAttachment[] | null | undefined, workProducts: IssueWorkProduct[] | null | undefined, ): IssueAttachment[] { - const promoted = getPromotedOutputAttachmentIds(workProducts); + const promoted = getAttachmentBackedWorkProductAttachmentIds(workProducts); return (attachments ?? []).filter( (attachment) => isAgentAttachment(attachment) && !promoted.has(attachment.id), ); diff --git a/ui/src/lib/issue-attachments.ts b/ui/src/lib/issue-attachments.ts index a772527522..3f2b4167a5 100644 --- a/ui/src/lib/issue-attachments.ts +++ b/ui/src/lib/issue-attachments.ts @@ -1,12 +1,7 @@ import type { IssueAttachment } from "@paperclipai/shared"; +import { isMarkdownAttachmentContent } from "@paperclipai/shared"; import { isVideoLikeOutput } from "./issue-output"; -const GENERIC_ATTACHMENT_CONTENT_TYPES = new Set([ - "application/octet-stream", - "binary/octet-stream", - "application/x-binary", -]); - type AttachmentPathLike = { contentPath: string; openPath?: string; @@ -42,17 +37,5 @@ export function isVideoAttachment( export function isMarkdownAttachment( attachment: Pick, ) { - const contentType = normalizedContentType(attachment); - if ( - contentType === "text/markdown" || - contentType === "text/x-markdown" || - contentType === "application/markdown" || - contentType === "application/x-markdown" - ) { - return true; - } - - const filename = (attachment.originalFilename ?? "").toLowerCase(); - if (!filename.endsWith(".md") && !filename.endsWith(".markdown")) return false; - return contentType === "text/plain" || GENERIC_ATTACHMENT_CONTENT_TYPES.has(contentType); + return isMarkdownAttachmentContent(attachment); }