Fix feedback review findings

Co-Authored-By: Paperclip <noreply@paperclip.ing>

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<FeedbackTrace[]>(
-            `/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;

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,

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<FeedbackTrace[]>(
-            `/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");
-}

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 () => {

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<string, unknown>) {
+  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);
+  });
+});

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<string, unknown> | 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);
   });
 

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,