diff --git a/prisma/migrations/0042_coach_feedback_message_ref/migration.sql b/prisma/migrations/0042_coach_feedback_message_ref/migration.sql new file mode 100644 index 000000000..1a895712f --- /dev/null +++ b/prisma/migrations/0042_coach_feedback_message_ref/migration.sql @@ -0,0 +1,86 @@ +-- v1.4.23 H3 reconcile (senior-dev review): the v1.4.23 H7 Coach +-- feedback route decrypted assistant prose and wrote it as plaintext +-- into `recommendation_feedback.recommendation_text`. The same prose +-- already lives encrypted-at-rest in `coach_messages.encrypted_content` +-- (AES-256-GCM via `@/lib/crypto`), so backups captured the leak and +-- the Coach surface's encryption-at-rest invariant was silently +-- broken for every rated message. +-- +-- Fix: drop the prose-snapshot dedup key for Coach rows in favour of a +-- foreign-key reference to the source message. The aggregator never +-- reads the prose anyway (it slices on structured columns only) and +-- the admin view never surfaces user prose, so the only consumer is +-- the unique-index dedup — which a per-message FK satisfies cleanly. +-- +-- For the existing recommendation surface (`target_type = 'recommendation'`) +-- the prose is already plaintext on the wire (recommendations are not +-- encrypted) and the dedup key still relies on `recommendation_text`, +-- so we keep that column nullable instead of dropping it. The +-- recommendation route continues to write the snapshot; the Coach +-- route stops writing the column entirely. +-- +-- Idempotent: every statement uses `IF [NOT] EXISTS` so a re-run after +-- a partial failure (or in environments where the table predates this +-- migration in a different state) is safe. + +-- 1. Add the FK column. Nullable because the recommendation surface +-- rows do not have a coach message — they reference an unencrypted +-- insight recommendation by id only. +ALTER TABLE "recommendation_feedback" + ADD COLUMN IF NOT EXISTS "coach_message_id" TEXT; + +-- 2. FK to coach_messages with cascade delete: removing a coach +-- message drops the rating row too (the rating is meaningless +-- without the source message). A separate constraint name keeps the +-- statement re-runnable via a guard query. +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conname = 'recommendation_feedback_coach_message_id_fkey' + ) THEN + ALTER TABLE "recommendation_feedback" + ADD CONSTRAINT "recommendation_feedback_coach_message_id_fkey" + FOREIGN KEY ("coach_message_id") REFERENCES "coach_messages"("id") + ON DELETE CASCADE ON UPDATE CASCADE; + END IF; +END $$; + +CREATE INDEX IF NOT EXISTS "recommendation_feedback_coach_message_id_idx" + ON "recommendation_feedback" ("coach_message_id"); + +-- 3. Backfill: for every coach-target row whose recommendation_id +-- happens to match a coach_messages.id (every v1.4.23 row, by +-- construction — the H7 route stamped the message id into +-- recommendation_id), copy the id into the new column. +UPDATE "recommendation_feedback" rf +SET "coach_message_id" = rf."recommendation_id" +WHERE rf."target_type" = 'coach' + AND rf."coach_message_id" IS NULL + AND EXISTS ( + SELECT 1 FROM "coach_messages" cm WHERE cm."id" = rf."recommendation_id" + ); + +-- 4. Null out the leaked plaintext for any coach rows that landed +-- pre-migration. The aggregator does not read this column, so the +-- blank value has no functional consequence — but it removes the +-- plaintext from on-disk pages on the next VACUUM. +UPDATE "recommendation_feedback" +SET "recommendation_text" = NULL +WHERE "target_type" = 'coach' + AND "recommendation_text" IS NOT NULL; + +-- 5. Make recommendation_text nullable. The recommendation surface +-- still populates it (and the unique key +-- `rec_feedback_user_rec_text_key` still defends against duplicate +-- ratings of the same recommendation snapshot); the coach surface +-- leaves it NULL forever. +ALTER TABLE "recommendation_feedback" + ALTER COLUMN "recommendation_text" DROP NOT NULL; + +-- 6. Coach-only dedup key: one rating per (user, message). Partial so +-- it does not interfere with the recommendation flow's existing +-- `(user_id, recommendation_id, recommendation_text)` unique key. +CREATE UNIQUE INDEX IF NOT EXISTS "rec_feedback_user_coach_message_key" + ON "recommendation_feedback" ("user_id", "coach_message_id") + WHERE "target_type" = 'coach' AND "coach_message_id" IS NOT NULL; diff --git a/prisma/schema.prisma b/prisma/schema.prisma index ab00a3afd..db7541429 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -908,7 +908,13 @@ model RecommendationFeedback { id String @id @default(cuid()) userId String @map("user_id") recommendationId String @map("recommendation_id") - recommendationText String @map("recommendation_text") + // Nullable since v1.4.23 reconcile (Sr-H3): the recommendation + // surface still snapshots the prose here (recommendations are + // unencrypted on the wire, so plaintext is the source-of-truth); + // the Coach surface leaves it NULL and references the message via + // `coachMessageId` so the assistant prose stays encrypted-at-rest + // in `coach_messages.encryptedContent`. + recommendationText String? @map("recommendation_text") recommendationSeverity String @map("recommendation_severity") metricSourceType String @map("metric_source_type") metricSourceTimeRange String @map("metric_source_time_range") @@ -929,12 +935,20 @@ model RecommendationFeedback { // pasted essay. reason String? @map("reason") + // v1.4.23 reconcile (Sr-H3) — Coach surface references the source + // message instead of snapshotting its prose into + // `recommendationText`. FK + cascade so a deleted message drops the + // rating row too. Always NULL for `targetType="recommendation"`. + coachMessageId String? @map("coach_message_id") + coachMessage CoachMessage? @relation(fields: [coachMessageId], references: [id], onDelete: Cascade) + user User @relation(fields: [userId], references: [id], onDelete: Cascade) @@unique([userId, recommendationId, recommendationText], map: "rec_feedback_user_rec_text_key") @@index([userId, createdAt]) @@index([recommendationSeverity, helpful, createdAt]) @@index([targetType, createdAt]) + @@index([coachMessageId]) @@map("recommendation_feedback") } @@ -992,7 +1006,11 @@ model CoachMessage { promptVersion String? @map("prompt_version") createdAt DateTime @default(now()) @map("created_at") - conversation CoachConversation @relation(fields: [conversationId], references: [id], onDelete: Cascade) + conversation CoachConversation @relation(fields: [conversationId], references: [id], onDelete: Cascade) + // v1.4.23 reconcile (Sr-H3) — back-relation for the FK that lets + // Coach feedback rows reference their source message instead of + // snapshotting plaintext prose into `recommendation_feedback`. + feedback RecommendationFeedback[] @@index([conversationId, createdAt]) @@map("coach_messages") diff --git a/src/app/api/insights/chat/messages/[id]/feedback/route.ts b/src/app/api/insights/chat/messages/[id]/feedback/route.ts index 5a7cc77a6..3af4cc9e9 100644 --- a/src/app/api/insights/chat/messages/[id]/feedback/route.ts +++ b/src/app/api/insights/chat/messages/[id]/feedback/route.ts @@ -16,22 +16,31 @@ * return 404 (never 403 — same lesson as the conversation route, no * cross-user existence leak). * - * Idempotency: the existing unique - * `(userId, recommendationId, recommendationText)` index already - * dedupes a replay submission for the same message + same prose - * snapshot. A re-rating after the user changes their mind requires the - * UI to delete + re-create — out of scope for v1.4.23 H7. + * Idempotency: a partial unique index on + * `(user_id, coach_message_id) WHERE target_type = 'coach'` dedupes + * a replay submission for the same message. The Coach surface no + * longer snapshots assistant prose into `recommendation_text` — + * v1.4.23 senior-dev review (Sr-H3) flagged that as a silent break of + * the encryption-at-rest invariant the Coach ships under + * (`coach_messages.encrypted_content` is AES-256-GCM). Feedback rows + * now reference the source message via `coachMessageId`; the + * aggregator never reads the prose anyway, so nothing downstream + * notices. */ import type { NextRequest } from "next/server"; import { z } from "zod/v4"; import { apiHandler, requireAuth, HttpError } from "@/lib/api-handler"; -import { apiError, apiSuccess, getClientIp, safeJson } from "@/lib/api-response"; +import { + apiError, + apiSuccess, + getClientIp, + safeJson, +} from "@/lib/api-response"; import { auditLog } from "@/lib/auth/audit"; import { prisma } from "@/lib/db"; import { Prisma } from "@/generated/prisma/client"; import { resolveCoachFeedbackAttribution } from "@/lib/ai/feedback-attribution"; -import { decrypt } from "@/lib/crypto"; import { annotate } from "@/lib/logging/context"; import { checkRateLimit } from "@/lib/rate-limit"; @@ -76,17 +85,16 @@ async function handlePost(request: NextRequest, ctx: RouteContext) { } const body = parsed.data; - // Verify ownership + pull the prose snapshot. The encrypted-content - // model stores the message body via AES-256-GCM; decrypt to use as - // the `recommendationText` slot so the per-(user, message, text) - // dedup remains stable across re-rates. + // Verify ownership only — we no longer touch encryptedContent. The + // assistant prose stays encrypted-at-rest; the feedback row just + // references the message id via the FK. const message = await prisma.coachMessage.findFirst({ where: { id: messageId, role: "assistant", conversation: { userId: user.id }, }, - select: { id: true, encryptedContent: true }, + select: { id: true }, }); if (!message) { annotate({ @@ -96,22 +104,6 @@ async function handlePost(request: NextRequest, ctx: RouteContext) { throw new HttpError(404, "coach.message.notFound"); } - let proseSnapshot = ""; - try { - const ciphertext = Buffer.from(message.encryptedContent).toString("utf8"); - proseSnapshot = decrypt(ciphertext); - } catch { - // Decryption failure shouldn't block the feedback row — fall back - // to a stable id-keyed marker so the dedup still works. - proseSnapshot = ``; - } - // Cap the snapshot at the same byte budget the rec-feedback table - // already accepts (a Coach reply is usually ~400-1500 chars; clip - // hard at 4 KB so a runaway provider can't bloat the table). - if (proseSnapshot.length > 4096) { - proseSnapshot = proseSnapshot.slice(0, 4096); - } - const attribution = await resolveCoachFeedbackAttribution(user.id, messageId); try { @@ -119,7 +111,10 @@ async function handlePost(request: NextRequest, ctx: RouteContext) { data: { userId: user.id, recommendationId: messageId, - recommendationText: proseSnapshot, + // v1.4.23 reconcile (Sr-H3): no plaintext prose. The FK + // resolves the message at read-time when (and only when) a + // future surface needs to render it. + coachMessageId: messageId, recommendationSeverity: "coach", metricSourceType: attribution.metricSourceType, metricSourceTimeRange: "single_message", diff --git a/src/lib/ai/feedback-attribution.ts b/src/lib/ai/feedback-attribution.ts index a00760942..7e6792a5f 100644 --- a/src/lib/ai/feedback-attribution.ts +++ b/src/lib/ai/feedback-attribution.ts @@ -87,7 +87,12 @@ export async function resolveFeedbackAttribution( * thumbs row. The polymorphic `RecommendationFeedback` table reuses * its existing slots for Coach rows: * - `recommendationId` carries the Coach message id - * - `recommendationText` carries the assistant prose snapshot + * - `coachMessageId` (added in 0042) FKs the source message so the + * prose stays encrypted-at-rest in `coach_messages`. The senior- + * dev review (Sr-H3) flagged the original H7 design that + * snapshotted decrypted prose into `recommendationText` as a + * silent break of the Coach surface's encryption invariant. + * - `recommendationText` is always NULL for coach rows * - `recommendationSeverity` is always "coach" so dashboards can * filter / group on the literal * - `metricSourceType` encodes the user's active prefs at the diff --git a/tests/integration/coach-feedback.test.ts b/tests/integration/coach-feedback.test.ts index eca885a48..a546f8343 100644 --- a/tests/integration/coach-feedback.test.ts +++ b/tests/integration/coach-feedback.test.ts @@ -85,9 +85,8 @@ describe("POST /api/insights/chat/messages/:id/feedback", () => { const user = await seedSession("coach-feedback-h7"); const message = await seedAssistantMessage(user.id, "Your last week..."); - const { POST } = await import( - "@/app/api/insights/chat/messages/[id]/feedback/route" - ); + const { POST } = + await import("@/app/api/insights/chat/messages/[id]/feedback/route"); const res = await ( POST as ( req: Request, @@ -118,6 +117,46 @@ describe("POST /api/insights/chat/messages/:id/feedback", () => { expect(rows[0].metricSourceType).toBe("coach:tone=warm:verbosity=default"); expect(rows[0].providerType).toBe("codex"); expect(rows[0].promptVersion).toBe("4.23.0"); + // Sr-H3 reconcile: the FK now carries the message id and the + // legacy plaintext snapshot column stays NULL so the encrypted- + // at-rest invariant of `coach_messages.encryptedContent` survives. + expect(rows[0].coachMessageId).toBe(message.id); + expect(rows[0].recommendationText).toBeNull(); + }); + + it("dedupes a second submission for the same message via the partial unique index", async () => { + const prisma = getPrismaClient(); + const user = await seedSession("coach-feedback-dedup"); + const message = await seedAssistantMessage(user.id, "Reply."); + + const { POST } = + await import("@/app/api/insights/chat/messages/[id]/feedback/route"); + const post = (id: string) => + ( + POST as ( + req: Request, + ctx: { params: Promise<{ id: string }> }, + ) => Promise + )( + new Request( + `http://localhost/api/insights/chat/messages/${id}/feedback`, + { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ rating: "helpful" }), + }, + ), + { params: Promise.resolve({ id }) }, + ); + const first = await post(message.id); + expect(first.status).toBe(201); + const second = await post(message.id); + expect(second.status).toBe(409); + + const rows = await prisma.recommendationFeedback.findMany({ + where: { userId: user.id }, + }); + expect(rows).toHaveLength(1); }); it("returns 404 when the message belongs to another user", async () => { @@ -127,9 +166,8 @@ describe("POST /api/insights/chat/messages/:id/feedback", () => { const otherMessage = await seedAssistantMessage(otherUser.id, "private"); await seedSession("attacker-h7"); - const { POST } = await import( - "@/app/api/insights/chat/messages/[id]/feedback/route" - ); + const { POST } = + await import("@/app/api/insights/chat/messages/[id]/feedback/route"); const res = await ( POST as ( req: Request, @@ -153,9 +191,8 @@ describe("POST /api/insights/chat/messages/:id/feedback", () => { const user = await seedSession("coach-feedback-422"); const message = await seedAssistantMessage(user.id, "Reply."); - const { POST } = await import( - "@/app/api/insights/chat/messages/[id]/feedback/route" - ); + const { POST } = + await import("@/app/api/insights/chat/messages/[id]/feedback/route"); const res = await ( POST as ( req: Request,