diff --git a/cli/src/commands/client/company.ts b/cli/src/commands/client/company.ts index b24cf1d8..b7620ec5 100644 --- a/cli/src/commands/client/company.ts +++ b/cli/src/commands/client/company.ts @@ -23,6 +23,11 @@ import { resolveCommandContext, type BaseClientOptions, } from "./common.js"; +import { + buildFeedbackTraceQuery, + normalizeFeedbackTraceExportFormat, + serializeFeedbackTraces, +} from "./feedback.js"; interface CompanyCommandOptions extends BaseClientOptions {} type CompanyDeleteSelectorMode = "auto" | "id" | "prefix"; @@ -180,34 +185,6 @@ function parseCsvValues(input: string | undefined): string[] { return Array.from(new Set(input.split(",").map((part) => part.trim()).filter(Boolean))); } -function buildFeedbackTraceQuery(opts: CompanyFeedbackOptions): string { - const params = new URLSearchParams(); - if (opts.targetType) params.set("targetType", opts.targetType); - if (opts.vote) params.set("vote", opts.vote); - if (opts.status) params.set("status", opts.status); - if (opts.projectId) params.set("projectId", opts.projectId); - if (opts.issueId) params.set("issueId", opts.issueId); - if (opts.from) params.set("from", opts.from); - if (opts.to) params.set("to", opts.to); - if (opts.sharedOnly) params.set("sharedOnly", "true"); - if (opts.includePayload) params.set("includePayload", "true"); - const query = params.toString(); - return query ? `?${query}` : ""; -} - -function normalizeFeedbackExportFormat(value: string | undefined): "json" | "ndjson" { - if (!value || value === "ndjson") return "ndjson"; - if (value === "json") return "json"; - throw new Error(`Unsupported export format: ${value}`); -} - -function serializeFeedbackTraces(traces: FeedbackTrace[], format: string | undefined): string { - if (normalizeFeedbackExportFormat(format) === "json") { - return JSON.stringify(traces, null, 2); - } - return traces.map((trace) => JSON.stringify(trace)).join("\n"); -} - function isInteractiveTerminal(): boolean { return Boolean(process.stdin.isTTY && process.stdout.isTTY); } @@ -1208,17 +1185,14 @@ export function registerCompanyCommands(program: Command): void { try { const ctx = resolveCommandContext(opts, { requireCompany: true }); const traces = (await ctx.api.get( - `/api/companies/${ctx.companyId}/feedback-traces${buildFeedbackTraceQuery({ - ...opts, - includePayload: opts.includePayload ?? true, - })}`, + `/api/companies/${ctx.companyId}/feedback-traces${buildFeedbackTraceQuery(opts, opts.includePayload ?? true)}`, )) ?? []; const serialized = serializeFeedbackTraces(traces, opts.format); if (opts.out?.trim()) { await writeFile(opts.out, serialized, "utf8"); if (ctx.json) { printOutput( - { out: opts.out, count: traces.length, format: normalizeFeedbackExportFormat(opts.format) }, + { out: opts.out, count: traces.length, format: normalizeFeedbackTraceExportFormat(opts.format) }, { json: true }, ); return; diff --git a/cli/src/commands/client/feedback.ts b/cli/src/commands/client/feedback.ts index d2f0f71b..3ac247d9 100644 --- a/cli/src/commands/client/feedback.ts +++ b/cli/src/commands/client/feedback.ts @@ -23,6 +23,17 @@ interface FeedbackFilterOptions extends BaseClientOptions { sharedOnly?: boolean; } +export interface FeedbackTraceQueryOptions { + targetType?: string; + vote?: string; + status?: string; + projectId?: string; + issueId?: string; + from?: string; + to?: string; + sharedOnly?: boolean; +} + interface FeedbackReportOptions extends FeedbackFilterOptions { payloads?: boolean; } @@ -174,7 +185,7 @@ export async function resolveFeedbackCompanyId( return companyId; } -export function buildFeedbackTraceQuery(opts: FeedbackFilterOptions, includePayload = true): string { +export function buildFeedbackTraceQuery(opts: FeedbackTraceQueryOptions, includePayload = true): string { const params = new URLSearchParams(); if (opts.targetType) params.set("targetType", opts.targetType); if (opts.vote) params.set("vote", opts.vote); @@ -189,6 +200,19 @@ export function buildFeedbackTraceQuery(opts: FeedbackFilterOptions, includePayl return query ? `?${query}` : ""; } +export function normalizeFeedbackTraceExportFormat(value: string | undefined): "json" | "ndjson" { + if (!value || value === "ndjson") return "ndjson"; + if (value === "json") return "json"; + throw new Error(`Unsupported export format: ${value}`); +} + +export function serializeFeedbackTraces(traces: FeedbackTrace[], format: string | undefined): string { + if (normalizeFeedbackTraceExportFormat(format) === "json") { + return JSON.stringify(traces, null, 2); + } + return traces.map((trace) => JSON.stringify(trace)).join("\n"); +} + export async function fetchCompanyFeedbackTraces( ctx: ResolvedClientContext, companyId: string, diff --git a/cli/src/commands/client/issue.ts b/cli/src/commands/client/issue.ts index c3517dd1..921c077e 100644 --- a/cli/src/commands/client/issue.ts +++ b/cli/src/commands/client/issue.ts @@ -17,6 +17,11 @@ import { resolveCommandContext, type BaseClientOptions, } from "./common.js"; +import { + buildFeedbackTraceQuery, + normalizeFeedbackTraceExportFormat, + serializeFeedbackTraces, +} from "./feedback.js"; interface IssueBaseOptions extends BaseClientOptions { status?: string; @@ -308,19 +313,19 @@ export function registerIssueCommands(program: Command): void { try { const ctx = resolveCommandContext(opts); const traces = (await ctx.api.get( - `/api/issues/${issueId}/feedback-traces${buildFeedbackTraceQuery({ - ...opts, - includePayload: opts.includePayload ?? true, - })}`, + `/api/issues/${issueId}/feedback-traces${buildFeedbackTraceQuery(opts, opts.includePayload ?? true)}`, )) ?? []; - const serialized = serializeFeedbackTraces(traces, opts.format); - if (opts.out?.trim()) { - await writeFile(opts.out, serialized, "utf8"); - if (ctx.json) { - printOutput({ out: opts.out, count: traces.length, format: normalizeExportFormat(opts.format) }, { json: true }); - return; - } - console.log(`Wrote ${traces.length} feedback trace(s) to ${opts.out}`); + const serialized = serializeFeedbackTraces(traces, opts.format); + if (opts.out?.trim()) { + await writeFile(opts.out, serialized, "utf8"); + if (ctx.json) { + printOutput( + { out: opts.out, count: traces.length, format: normalizeFeedbackTraceExportFormat(opts.format) }, + { json: true }, + ); + return; + } + console.log(`Wrote ${traces.length} feedback trace(s) to ${opts.out}`); return; } process.stdout.write(`${serialized}${serialized.endsWith("\n") ? "" : "\n"}`); @@ -404,29 +409,3 @@ function filterIssueRows(rows: Issue[], match: string | undefined): Issue[] { return text.includes(needle); }); } - -function buildFeedbackTraceQuery(opts: IssueFeedbackOptions): string { - const params = new URLSearchParams(); - if (opts.targetType) params.set("targetType", opts.targetType); - if (opts.vote) params.set("vote", opts.vote); - if (opts.status) params.set("status", opts.status); - if (opts.from) params.set("from", opts.from); - if (opts.to) params.set("to", opts.to); - if (opts.sharedOnly) params.set("sharedOnly", "true"); - if (opts.includePayload) params.set("includePayload", "true"); - const query = params.toString(); - return query ? `?${query}` : ""; -} - -function normalizeExportFormat(value: string | undefined): "json" | "ndjson" { - if (!value || value === "ndjson") return "ndjson"; - if (value === "json") return "json"; - throw new Error(`Unsupported export format: ${value}`); -} - -function serializeFeedbackTraces(traces: FeedbackTrace[], format: string | undefined): string { - if (normalizeExportFormat(format) === "json") { - return JSON.stringify(traces, null, 2); - } - return traces.map((trace) => JSON.stringify(trace)).join("\n"); -} diff --git a/server/src/__tests__/feedback-service.test.ts b/server/src/__tests__/feedback-service.test.ts index af44f866..898ae0aa 100644 --- a/server/src/__tests__/feedback-service.test.ts +++ b/server/src/__tests__/feedback-service.test.ts @@ -762,9 +762,11 @@ describe("feedbackService.saveIssueVote", () => { expect(localTrace?.status).toBe("local_only"); expect(localTrace?.exportId).toBeNull(); + expect(localTrace?.payloadVersion).toBe("paperclip-feedback-v1"); expect(localTrace?.payloadSnapshot?.bundle).toBeNull(); expect(sharedTrace?.status).toBe("pending"); expect(sharedTrace?.exportId).toMatch(/^fbexp_/); + expect(sharedTrace?.payloadVersion).toBe("paperclip-feedback-v1"); }); it("captures Claude project session artifacts as full traces", async () => { diff --git a/server/src/__tests__/issue-feedback-routes.test.ts b/server/src/__tests__/issue-feedback-routes.test.ts new file mode 100644 index 00000000..0bb7dc34 --- /dev/null +++ b/server/src/__tests__/issue-feedback-routes.test.ts @@ -0,0 +1,128 @@ +import express from "express"; +import request from "supertest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { errorHandler } from "../middleware/index.js"; +import { issueRoutes } from "../routes/issues.js"; + +const mockFeedbackService = vi.hoisted(() => ({ + getFeedbackTraceById: vi.fn(), + getFeedbackTraceBundle: vi.fn(), + listIssueVotesForUser: vi.fn(), + listFeedbackTraces: vi.fn(), + saveIssueVote: vi.fn(), +})); + +vi.mock("../services/index.js", () => ({ + accessService: () => ({ + canUser: vi.fn(), + hasPermission: vi.fn(), + }), + agentService: () => ({ + getById: vi.fn(), + }), + documentService: () => ({}), + executionWorkspaceService: () => ({}), + feedbackService: () => mockFeedbackService, + 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: () => ({}), + issueService: () => ({ + getById: vi.fn(), + update: vi.fn(), + addComment: vi.fn(), + findMentionedAgents: vi.fn(), + }), + logActivity: vi.fn(async () => undefined), + projectService: () => ({}), + routineService: () => ({ + syncRunStatusForIssue: vi.fn(async () => undefined), + }), + workProductService: () => ({}), +})); + +function createApp(actor: Record) { + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + (req as any).actor = actor; + next(); + }); + app.use("/api", issueRoutes({} as any, {} as any)); + app.use(errorHandler); + return app; +} + +describe("issue feedback trace routes", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("rejects non-board callers before fetching a feedback trace", async () => { + const app = createApp({ + type: "agent", + agentId: "agent-1", + companyId: "company-1", + source: "agent_key", + runId: "run-1", + }); + + const res = await request(app).get("/api/feedback-traces/trace-1"); + + expect(res.status).toBe(403); + expect(mockFeedbackService.getFeedbackTraceById).not.toHaveBeenCalled(); + }); + + it("returns 404 when a board user lacks access to the trace company", async () => { + mockFeedbackService.getFeedbackTraceById.mockResolvedValue({ + id: "trace-1", + companyId: "company-2", + }); + const app = createApp({ + type: "board", + userId: "user-1", + source: "session", + isInstanceAdmin: false, + companyIds: ["company-1"], + }); + + const res = await request(app).get("/api/feedback-traces/trace-1"); + + expect(res.status).toBe(404); + }); + + it("returns 404 for bundle fetches when a board user lacks access to the trace company", async () => { + mockFeedbackService.getFeedbackTraceBundle.mockResolvedValue({ + id: "trace-1", + companyId: "company-2", + issueId: "issue-1", + files: [], + }); + const app = createApp({ + type: "board", + userId: "user-1", + source: "session", + isInstanceAdmin: false, + companyIds: ["company-1"], + }); + + const res = await request(app).get("/api/feedback-traces/trace-1/bundle"); + + expect(res.status).toBe(404); + }); +}); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 459cb396..7ecbd6de 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -116,6 +116,13 @@ export function issueRoutes(db: Db, storage: StorageService) { return false; } + function actorCanAccessCompany(req: Request, companyId: string) { + if (req.actor.type === "none") return false; + if (req.actor.type === "agent") return req.actor.companyId === companyId; + if (req.actor.source === "local_implicit" || req.actor.isInstanceAdmin) return true; + return (req.actor.companyIds ?? []).includes(companyId); + } + function canCreateAgentsLegacy(agent: { permissions: Record | null | undefined; role: string }) { if (agent.role === "ceo") return true; if (!agent.permissions || typeof agent.permissions !== "object") return false; @@ -1538,31 +1545,30 @@ export function issueRoutes(db: Db, storage: StorageService) { router.get("/feedback-traces/:traceId", async (req, res) => { const traceId = req.params.traceId as string; - const trace = await feedback.getFeedbackTraceById(traceId, parseBooleanQuery(req.query.includePayload) || req.query.includePayload === undefined); - if (!trace) { - res.status(404).json({ error: "Feedback trace not found" }); - return; - } - assertCompanyAccess(req, trace.companyId); if (req.actor.type !== "board") { res.status(403).json({ error: "Only board users can view feedback traces" }); return; } + const includePayload = parseBooleanQuery(req.query.includePayload) || req.query.includePayload === undefined; + const trace = await feedback.getFeedbackTraceById(traceId, includePayload); + if (!trace || !actorCanAccessCompany(req, trace.companyId)) { + res.status(404).json({ error: "Feedback trace not found" }); + return; + } res.json(trace); }); router.get("/feedback-traces/:traceId/bundle", async (req, res) => { const traceId = req.params.traceId as string; - const bundle = await feedback.getFeedbackTraceBundle(traceId); - if (!bundle) { - res.status(404).json({ error: "Feedback trace not found" }); - return; - } - assertCompanyAccess(req, bundle.companyId); if (req.actor.type !== "board") { res.status(403).json({ error: "Only board users can view feedback trace bundles" }); return; } + const bundle = await feedback.getFeedbackTraceBundle(traceId); + if (!bundle || !actorCanAccessCompany(req, bundle.companyId)) { + res.status(404).json({ error: "Feedback trace not found" }); + return; + } res.json(bundle); }); diff --git a/server/src/services/feedback.ts b/server/src/services/feedback.ts index 57175ce9..312cd5bf 100644 --- a/server/src/services/feedback.ts +++ b/server/src/services/feedback.ts @@ -49,6 +49,7 @@ import { getRunLogStore } from "./run-log-store.js"; const FEEDBACK_SCHEMA_VERSION = "paperclip-feedback-envelope-v2"; const FEEDBACK_BUNDLE_VERSION = "paperclip-feedback-bundle-v2"; +const FEEDBACK_PAYLOAD_VERSION = "paperclip-feedback-v1"; const FEEDBACK_DESTINATION = "paperclip_labs_feedback_v1"; const FEEDBACK_CONTEXT_WINDOW = 3; const MAX_EXCERPT_CHARS = 200; @@ -1999,7 +2000,7 @@ export function feedbackService(db: Db, options: FeedbackServiceOptions = {}) { consentVersion: sharedWithLabs ? (consentVersion ?? DEFAULT_FEEDBACK_DATA_SHARING_TERMS_VERSION) : null, schemaVersion: FEEDBACK_SCHEMA_VERSION, bundleVersion: FEEDBACK_BUNDLE_VERSION, - payloadVersion: FEEDBACK_BUNDLE_VERSION, + payloadVersion: FEEDBACK_PAYLOAD_VERSION, payloadDigest: artifacts.payloadDigest, payloadSnapshot: artifacts.payloadSnapshot, targetSummary: artifacts.targetSummary, @@ -2021,7 +2022,7 @@ export function feedbackService(db: Db, options: FeedbackServiceOptions = {}) { consentVersion: sharedWithLabs ? (consentVersion ?? DEFAULT_FEEDBACK_DATA_SHARING_TERMS_VERSION) : null, schemaVersion: FEEDBACK_SCHEMA_VERSION, bundleVersion: FEEDBACK_BUNDLE_VERSION, - payloadVersion: FEEDBACK_BUNDLE_VERSION, + payloadVersion: FEEDBACK_PAYLOAD_VERSION, payloadDigest: artifacts.payloadDigest, payloadSnapshot: artifacts.payloadSnapshot, targetSummary: artifacts.targetSummary,