fix(coach): feedback references coach_messages, drops plaintext column

Reviewer note: W5 H7 wrote Coach assistant prose plaintext into
recommendation_feedback.recommendation_text. Same prose lives
encrypted in coach_messages — backups captured the leak.

Migration 0042_coach_feedback_message_ref adds nullable coachMessageId
(FK + cascade) and drops recommendation_text. Aggregator joins +
decrypts on read; admin view reads through the same path.

Fixes: Sr-H3 (Wave-6 v1.4.23 senior-dev review)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

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;

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")

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 = `<undecryptable:${messageId}>`;
-  }
-  // 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",

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

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<Response>
+      )(
+        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,